Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Daniel Lezcano]

On 28/03/2021 19:26, Greg KH wrote:

[ ... ]

>>> So why are you trying to add a kref here as the structure already has
>>> support for proper lifetimes?
>>
>> Right, I'll revisit that part. Thanks for the review.
>>
>> I've a branch which is pulled by Rafael [1]. These parts are already
>> merged in the dtpm/next branch but not yet in Rafael's tree.
> 
> I would recommend fixing that up if you can rebase it.  If not, you need
> to revert it and start over.  I'll be glad to review it if you cc: me on
> the patches.
> 
>> I think a rebase is possible but I would like to avoid that. Would be a
>> patch on top of the dtpm/next acceptable given your flow with Android ?
> 
> This has nothing to do with the Android kernel workflow, sorry.  I am
> concerned about proper kernel development and keeping bugs out of it.


Fair enough, I will fix it up and send a v5.

Thanks

  -- Daniel


-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Greg KH]

On Sun, Mar 28, 2021 at 06:07:10PM +0200, Daniel Lezcano wrote:
> On 28/03/2021 13:24, Greg KH wrote:
> > On Sun, Mar 28, 2021 at 01:11:30PM +0200, Daniel Lezcano wrote:
> >>
> >> Hi Greg,
> >>
> >> On 28/03/2021 08:50, Greg KH wrote:
> >>
> >> [ ... ]
> >>
> > And any reason why you are not using "real" struct devices in this
> > subsystem?  You seem to be rolling your own infrastructure for no good
> > reason.  I imagine you want sysfs support next, right?
> 
>  Actually, the framework is on top of powercap, so it has de facto the
>  sysfs support. On the other side, the dtpm backends are tied with the
>  device they manage.
> >>>
> >>> So why are they not a "real" device in the driver model?  It looks like
> >>> you almost are wanting all of that functionality and are having to
> >>> implement it "by hand" instead.
> >>
> >> I'm sorry I misunderstanding your point. dtpm is the backend for the
> >> powercap subsystem which is the generic subsystem to do power limitation.
> >>
> >> We have:
> >>
> >> struct dtpm_cpu {
> >>struct dtpm dtmp;
> >>...
> >> }
> >>
> >> struct dtpm {
> >>struct powercap powecap;
> >> };
> >>
> >> struct powercap {
> >>struct device dev;
> >> };
> > 
> > Oh nice.  So you can not use a kref here at all as you already have a
> > reference counted device controling your structure.  You can not have 2
> > references trying to control the same structure, that way lies madness
> > and bugs :(
> > 
> > So why are you trying to add a kref here as the structure already has
> > support for proper lifetimes?
> 
> Right, I'll revisit that part. Thanks for the review.
> 
> I've a branch which is pulled by Rafael [1]. These parts are already
> merged in the dtpm/next branch but not yet in Rafael's tree.

I would recommend fixing that up if you can rebase it.  If not, you need
to revert it and start over.  I'll be glad to review it if you cc: me on
the patches.

> I think a rebase is possible but I would like to avoid that. Would be a
> patch on top of the dtpm/next acceptable given your flow with Android ?

This has nothing to do with the Android kernel workflow, sorry.  I am
concerned about proper kernel development and keeping bugs out of it.

thanks,

greg k-h

Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Daniel Lezcano]

On 28/03/2021 13:24, Greg KH wrote:
> On Sun, Mar 28, 2021 at 01:11:30PM +0200, Daniel Lezcano wrote:
>>
>> Hi Greg,
>>
>> On 28/03/2021 08:50, Greg KH wrote:
>>
>> [ ... ]
>>
> And any reason why you are not using "real" struct devices in this
> subsystem?  You seem to be rolling your own infrastructure for no good
> reason.  I imagine you want sysfs support next, right?

 Actually, the framework is on top of powercap, so it has de facto the
 sysfs support. On the other side, the dtpm backends are tied with the
 device they manage.
>>>
>>> So why are they not a "real" device in the driver model?  It looks like
>>> you almost are wanting all of that functionality and are having to
>>> implement it "by hand" instead.
>>
>> I'm sorry I misunderstanding your point. dtpm is the backend for the
>> powercap subsystem which is the generic subsystem to do power limitation.
>>
>> We have:
>>
>> struct dtpm_cpu {
>>  struct dtpm dtmp;
>>  ...
>> }
>>
>> struct dtpm {
>>  struct powercap powecap;
>> };
>>
>> struct powercap {
>>  struct device dev;
>> };
> 
> Oh nice.  So you can not use a kref here at all as you already have a
> reference counted device controling your structure.  You can not have 2
> references trying to control the same structure, that way lies madness
> and bugs :(
> 
> So why are you trying to add a kref here as the structure already has
> support for proper lifetimes?

Right, I'll revisit that part. Thanks for the review.

I've a branch which is pulled by Rafael [1]. These parts are already
merged in the dtpm/next branch but not yet in Rafael's tree.

I think a rebase is possible but I would like to avoid that. Would be a
patch on top of the dtpm/next acceptable given your flow with Android ?

  -- Daniel

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/daniel.lezcano/linux.git/log/?h=dtpm/next

-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Greg KH]

On Sun, Mar 28, 2021 at 01:11:30PM +0200, Daniel Lezcano wrote:
> 
> Hi Greg,
> 
> On 28/03/2021 08:50, Greg KH wrote:
> 
> [ ... ]
> 
> >>> And any reason why you are not using "real" struct devices in this
> >>> subsystem?  You seem to be rolling your own infrastructure for no good
> >>> reason.  I imagine you want sysfs support next, right?
> >>
> >> Actually, the framework is on top of powercap, so it has de facto the
> >> sysfs support. On the other side, the dtpm backends are tied with the
> >> device they manage.
> > 
> > So why are they not a "real" device in the driver model?  It looks like
> > you almost are wanting all of that functionality and are having to
> > implement it "by hand" instead.
> 
> I'm sorry I misunderstanding your point. dtpm is the backend for the
> powercap subsystem which is the generic subsystem to do power limitation.
> 
> We have:
> 
> struct dtpm_cpu {
>   struct dtpm dtmp;
>   ...
> }
> 
> struct dtpm {
>   struct powercap powecap;
> };
> 
> struct powercap {
>   struct device dev;
> };

Oh nice.  So you can not use a kref here at all as you already have a
reference counted device controling your structure.  You can not have 2
references trying to control the same structure, that way lies madness
and bugs :(

So why are you trying to add a kref here as the structure already has
support for proper lifetimes?

thanks,

greg k-h

Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Daniel Lezcano]

Hi Greg,

On 28/03/2021 08:50, Greg KH wrote:

[ ... ]

>>> And any reason why you are not using "real" struct devices in this
>>> subsystem?  You seem to be rolling your own infrastructure for no good
>>> reason.  I imagine you want sysfs support next, right?
>>
>> Actually, the framework is on top of powercap, so it has de facto the
>> sysfs support. On the other side, the dtpm backends are tied with the
>> device they manage.
> 
> So why are they not a "real" device in the driver model?  It looks like
> you almost are wanting all of that functionality and are having to
> implement it "by hand" instead.

I'm sorry I misunderstanding your point. dtpm is the backend for the
powercap subsystem which is the generic subsystem to do power limitation.

We have:

struct dtpm_cpu {
struct dtpm dtmp;
...
}

struct dtpm {
struct powercap powecap;
};

struct powercap {
struct device dev;
};



-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Greg KH]

On Sat, Mar 27, 2021 at 08:41:24PM +0100, Daniel Lezcano wrote:
> On 27/03/2021 13:50, Greg KH wrote:
> > On Fri, Mar 12, 2021 at 02:04:08PM +0100, Daniel Lezcano wrote:
> >> A SoC can be differently structured depending on the platform and the
> >> kernel can not be aware of all the combinations, as well as the
> >> specific tweaks for a particular board.
> >>
> >> The creation of the hierarchy must be delegated to userspace.
> >>
> >> These changes provide a registering mechanism where the different
> >> subsystems will initialize their dtpm backends and register with a
> >> name the dtpm node in a list.
> >>
> >> The next changes will provide an userspace interface to create
> >> hierarchically the different nodes. Those will be created by name and
> >> found via the list filled by the different subsystem.
> >>
> >> If a specified name is not found in the list, it is assumed to be a
> >> virtual node which will have children and the default is to allocate
> >> such node.
> >>
> >> When the node register in the list, the function will be dtpm_register
> >> where the previous semantic was to create the node. Thus, the
> >> functions are renamed to reflect their purpose.
> >>
> >> Signed-off-by: Daniel Lezcano 
> >> Reviewed-by: Lukasz Luba 
> >> ---
> 
> [ ... ]
> 
> >> +static void dtpm_release(struct kref *kref)
> >> +{
> >> +  struct dtpm *dtpm = container_of(kref, struct dtpm, kref);
> >> +
> >> +  kfree(dtpm);
> >> +}
> >> +
> >> +/**
> >> + * dtpm_put - Release a reference on a dtpm device
> >> + * @dtpm: a pointer to a dtpm structure
> >> + *
> >> + * Release the reference on the specified dtpm device. The last
> >> + * reference leads to a memory release.
> >> + */
> >> +void dtpm_put(struct dtpm *dtpm)
> >> +{
> >> +  kref_put(>kref, dtpm_release);
> > 
> > You forgot to also grab the dtpm_lock before calling this, right?  What
> > is preventing a get and put from being called at the same time?
> > 
> > You protect things at get time, but not put from what I can see :(
> 
> Thanks for spotting this, I will send a fix for that.
> 
> [ ... ]
> 
> >> +  list_add(>node, _list);
> >> +
> >> +  pr_info("Registered %s\n", name);
> > 
> > When kernel code works properly, it is quiet.  This is debugging code a
> > the most, never something that everyone should be seeing all the time,
> > please remove.
> 
> Initially, a comment asked for debug traces in the code. There are more
> traces in the code at the pr_debug level.
> 
> Is your suggestion to remove the pr_info as well as other debug traces
> or convert those pr_info to pr_debug ?

pr_info() should not be used for "debug traces".

Use real tracepoints for debug traces if you still need them, hopefully
the code is all now debugged properly, right?

Again, when code is working, it is quiet.  Do not leave debugging stuff
like this in a working system.

> 
> [ ... ]
> 
> > And any reason why you are not using "real" struct devices in this
> > subsystem?  You seem to be rolling your own infrastructure for no good
> > reason.  I imagine you want sysfs support next, right?
> 
> Actually, the framework is on top of powercap, so it has de facto the
> sysfs support. On the other side, the dtpm backends are tied with the
> device they manage.

So why are they not a "real" device in the driver model?  It looks like
you almost are wanting all of that functionality and are having to
implement it "by hand" instead.

thanks,

greg k-h

Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Daniel Lezcano]

On 27/03/2021 13:50, Greg KH wrote:
> On Fri, Mar 12, 2021 at 02:04:08PM +0100, Daniel Lezcano wrote:
>> A SoC can be differently structured depending on the platform and the
>> kernel can not be aware of all the combinations, as well as the
>> specific tweaks for a particular board.
>>
>> The creation of the hierarchy must be delegated to userspace.
>>
>> These changes provide a registering mechanism where the different
>> subsystems will initialize their dtpm backends and register with a
>> name the dtpm node in a list.
>>
>> The next changes will provide an userspace interface to create
>> hierarchically the different nodes. Those will be created by name and
>> found via the list filled by the different subsystem.
>>
>> If a specified name is not found in the list, it is assumed to be a
>> virtual node which will have children and the default is to allocate
>> such node.
>>
>> When the node register in the list, the function will be dtpm_register
>> where the previous semantic was to create the node. Thus, the
>> functions are renamed to reflect their purpose.
>>
>> Signed-off-by: Daniel Lezcano 
>> Reviewed-by: Lukasz Luba 
>> ---

[ ... ]

>> +static void dtpm_release(struct kref *kref)
>> +{
>> +struct dtpm *dtpm = container_of(kref, struct dtpm, kref);
>> +
>> +kfree(dtpm);
>> +}
>> +
>> +/**
>> + * dtpm_put - Release a reference on a dtpm device
>> + * @dtpm: a pointer to a dtpm structure
>> + *
>> + * Release the reference on the specified dtpm device. The last
>> + * reference leads to a memory release.
>> + */
>> +void dtpm_put(struct dtpm *dtpm)
>> +{
>> +kref_put(>kref, dtpm_release);
> 
> You forgot to also grab the dtpm_lock before calling this, right?  What
> is preventing a get and put from being called at the same time?
> 
> You protect things at get time, but not put from what I can see :(

Thanks for spotting this, I will send a fix for that.

[ ... ]

>> +list_add(>node, _list);
>> +
>> +pr_info("Registered %s\n", name);
> 
> When kernel code works properly, it is quiet.  This is debugging code a
> the most, never something that everyone should be seeing all the time,
> please remove.

Initially, a comment asked for debug traces in the code. There are more
traces in the code at the pr_debug level.

Is your suggestion to remove the pr_info as well as other debug traces
or convert those pr_info to pr_debug ?

[ ... ]

> And any reason why you are not using "real" struct devices in this
> subsystem?  You seem to be rolling your own infrastructure for no good
> reason.  I imagine you want sysfs support next, right?

Actually, the framework is on top of powercap, so it has de facto the
sysfs support. On the other side, the dtpm backends are tied with the
device they manage.



-- 
 Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

Re: [PATCH v4 2/5] powercap/drivers/dtpm: Create a registering system

[Greg KH]

On Fri, Mar 12, 2021 at 02:04:08PM +0100, Daniel Lezcano wrote:
> A SoC can be differently structured depending on the platform and the
> kernel can not be aware of all the combinations, as well as the
> specific tweaks for a particular board.
> 
> The creation of the hierarchy must be delegated to userspace.
> 
> These changes provide a registering mechanism where the different
> subsystems will initialize their dtpm backends and register with a
> name the dtpm node in a list.
> 
> The next changes will provide an userspace interface to create
> hierarchically the different nodes. Those will be created by name and
> found via the list filled by the different subsystem.
> 
> If a specified name is not found in the list, it is assumed to be a
> virtual node which will have children and the default is to allocate
> such node.
> 
> When the node register in the list, the function will be dtpm_register
> where the previous semantic was to create the node. Thus, the
> functions are renamed to reflect their purpose.
> 
> Signed-off-by: Daniel Lezcano 
> Reviewed-by: Lukasz Luba 
> ---
> 
> V4:
>   - Fixed typo in the commit log
> 
> V2:
>   - Fixed error code path by dropping lock
> ---
>  drivers/powercap/dtpm.c | 161 ++--
>  drivers/powercap/dtpm_cpu.c |   4 +-
>  include/linux/dtpm.h|  12 ++-
>  3 files changed, 164 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/powercap/dtpm.c b/drivers/powercap/dtpm.c
> index 58433b8ef9a1..d00f55f0ee30 100644
> --- a/drivers/powercap/dtpm.c
> +++ b/drivers/powercap/dtpm.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -34,6 +35,14 @@ static DEFINE_MUTEX(dtpm_lock);
>  static struct powercap_control_type *pct;
>  static struct dtpm *root;
>  
> +struct dtpm_node {
> + const char *name;
> + struct dtpm *dtpm;
> + struct list_head node;
> +};
> +
> +static LIST_HEAD(dtpm_list);
> +
>  static int get_time_window_us(struct powercap_zone *pcz, int cid, u64 
> *window)
>  {
>   return -ENOSYS;
> @@ -152,6 +161,138 @@ static int __dtpm_update_power(struct dtpm *dtpm)
>   return ret;
>  }
>  
> +static struct dtpm *__dtpm_lookup(const char *name)
> +{
> + struct dtpm_node *node;
> +
> + list_for_each_entry(node, _list, node) {
> + if (!strcmp(name, node->name))
> + return node->dtpm;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * dtpm_get - Get a reference to a dtpm structure
> + * @name: the name of the dtpm device
> + *
> + * The function looks up in the list of the registered dtpm
> + * devices. If the dtpm device is not found, a virtual one is
> + * allocated. This function must be called to create a dtpm node in
> + * the powercap hierarchy.
> + *
> + * Return: a pointer to a dtpm structure, NULL if there is not enough
> + * memory
> + */
> +struct dtpm *dtpm_get(const char *name)
> +{
> + struct dtpm *dtpm;
> +
> + mutex_lock(_lock);
> + dtpm = __dtpm_lookup(name);
> + if (!dtpm)
> + dtpm = dtpm_alloc(NULL);
> + else
> + kref_get(>kref);
> + mutex_unlock(_lock);
> +
> + return dtpm;
> +}
> +
> +static void dtpm_release(struct kref *kref)
> +{
> + struct dtpm *dtpm = container_of(kref, struct dtpm, kref);
> +
> + kfree(dtpm);
> +}
> +
> +/**
> + * dtpm_put - Release a reference on a dtpm device
> + * @dtpm: a pointer to a dtpm structure
> + *
> + * Release the reference on the specified dtpm device. The last
> + * reference leads to a memory release.
> + */
> +void dtpm_put(struct dtpm *dtpm)
> +{
> + kref_put(>kref, dtpm_release);

You forgot to also grab the dtpm_lock before calling this, right?  What
is preventing a get and put from being called at the same time?

You protect things at get time, but not put from what I can see :(


> +}
> +
> +/**
> + * dtpm_register - Register the dtpm in the dtpm list
> + * @name: a name used as an identifier
> + * @dtpm: the dtpm node to be registered
> + *
> + * Stores the dtpm device in a list.
> + *
> + * Return: 0 on success, -EEXIST if the device name is already present
> + * in the list, -ENOMEM in case of memory allocation failure.
> + */
> +int dtpm_register(const char *name, struct dtpm *dtpm)
> +{
> + struct dtpm_node *node;
> + int ret;
> +
> + mutex_lock(_lock);
> +
> + ret = -EEXIST;
> + if (__dtpm_lookup(name))
> + goto out_unlock;
> +
> + ret = -ENOMEM;
> + node = kzalloc(sizeof(*node), GFP_KERNEL);
> + if (!node)
> + goto out_unlock;
> +
> + node->name = kstrdup(name, GFP_KERNEL);
> + if (!node->name) {
> + kfree(node);
> + goto out_unlock;
> + }
> +
> + node->dtpm = dtpm;
> +
> + list_add(>node, _list);
> +
> + pr_info("Registered %s\n", name);

When kernel code works properly, it is quiet.  This is debugging code a
the most, never something that everyone should be