Re: [PATCH] base: soc: Handle custom soc information sysfs entries

mnalajal
2019-10-03T21:11:06Z

On 2019-10-03 11:33, Greg KH wrote:
> On Thu, Oct 03, 2019 at 11:23:45AM -0700, mnalajal@codeaurora.org
> wrote:
>> On 2019-10-03 00:06, Greg KH wrote:
>> > On Wed, Oct 02, 2019 at 05:06:14PM -0700, Murali Nalajala wrote:
>> > > Soc framework exposed sysfs entries are not sufficient for some
>> > > of the h/w platforms. Currently there is no interface where soc
>> > > drivers can expose further information about their SoCs via soc
>> > > framework. This change address this limitation where clients can
>> > > pass their custom entries as attribute group and soc framework
>> > > would expose them as sysfs properties.
>> > >
>> > > Signed-off-by: Murali Nalajala <mnalajal@codeaurora.org>
>> > > ---
>> > > drivers/base/soc.c | 26 ++++++++++++++++++--------
>> > > include/linux/sys_soc.h | 1 +
>> > > 2 files changed, 19 insertions(+), 8 deletions(-)
>> > >
>> > > diff --git a/drivers/base/soc.c b/drivers/base/soc.c
>> > > index 7c0c5ca..ec70a58 100644
>> > > --- a/drivers/base/soc.c
>> > > +++ b/drivers/base/soc.c
>> > > @@ -15,6 +15,8 @@
>> > > #include <linux/err.h>
>> > > #include <linux/glob.h>
>> > >
>> > > +#define NUM_ATTR_GROUPS 3
>> > > +
>> > > static DEFINE_IDA(soc_ida);
>> > >
>> > > static ssize_t soc_info_get(struct device *dev,
>> > > @@ -104,11 +106,6 @@ static ssize_t soc_info_get(struct device *dev,
>> > > .is_visible = soc_attribute_mode,
>> > > };
>> > >
>> > > -static const struct attribute_group *soc_attr_groups[] = {
>> > > - &soc_attr_group,
>> > > - NULL,
>> > > -};
>> > > -
>> > > static void soc_release(struct device *dev)
>> > > {
>> > > struct soc_device *soc_dev = container_of(dev, struct soc_device,
>> > > dev);
>> > > @@ -121,6 +118,7 @@ static void soc_release(struct device *dev)
>> > > struct soc_device *soc_device_register(struct soc_device_attribute
>> > > *soc_dev_attr)
>> > > {
>> > > struct soc_device *soc_dev;
>> > > + const struct attribute_group **soc_attr_groups = NULL;
>> > > int ret;
>> > >
>> > > if (!soc_bus_type.p) {
>> > > @@ -136,10 +134,20 @@ struct soc_device *soc_device_register(struct
>> > > soc_device_attribute *soc_dev_attr
>> > > goto out1;
>> > > }
>> > >
>> > > + soc_attr_groups = kzalloc(sizeof(*soc_attr_groups) *
>> > > + NUM_ATTR_GROUPS, GFP_KERNEL);
>> > > + if (!soc_attr_groups) {
>> > > + ret = -ENOMEM;
>> > > + goto out2;
>> > > + }
>> > > + soc_attr_groups[0] = &soc_attr_group;
>> > > + soc_attr_groups[1] = soc_dev_attr->custom_attr_group;
>> > > + soc_attr_groups[2] = NULL;
>> > > +
>> > > /* Fetch a unique (reclaimable) SOC ID. */
>> > > ret = ida_simple_get(&soc_ida, 0, 0, GFP_KERNEL);
>> > > if (ret < 0)
>> > > - goto out2;
>> > > + goto out3;
>> > > soc_dev->soc_dev_num = ret;
>> > >
>> > > soc_dev->attr = soc_dev_attr;
>> > > @@ -151,14 +159,16 @@ struct soc_device *soc_device_register(struct
>> > > soc_device_attribute *soc_dev_attr
>> > >
>> > > ret = device_register(&soc_dev->dev);
>> > > if (ret)
>> > > - goto out3;
>> > > + goto out4;
>> > >
>> > > return soc_dev;
>> > >
>> > > -out3:
>> > > +out4:
>> > > ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
>> > > put_device(&soc_dev->dev);
>> > > soc_dev = NULL;
>> > > +out3:
>> > > + kfree(soc_attr_groups);
>> > > out2:
>> > > kfree(soc_dev);
>> > > out1:
>> > > diff --git a/include/linux/sys_soc.h b/include/linux/sys_soc.h
>> > > index 48ceea8..d9b3cf0 100644
>> > > --- a/include/linux/sys_soc.h
>> > > +++ b/include/linux/sys_soc.h
>> > > @@ -15,6 +15,7 @@ struct soc_device_attribute {
>> > > const char *serial_number;
>> > > const char *soc_id;
>> > > const void *data;
>> > > + const struct attribute_group *custom_attr_group;
>> >
>> > Shouldn't you make this:
>> > const struct attribute_group **soc_groups;
>> >
>> > to match up with the rest of the way the driver core works?
>> Assumption is, soc drivers send their custom attribute group and soc
>> framework has already soc_attr_group" (basic info exposed).
>> With my changes i am combining these two groups and passing to
>> "device_register()".
>> I do not think soc drivers have a requirement where they can pass
>> various
>> groups rather one single group attribute.
>
> Ok, I guess this is "good enough" such that no individual SOC driver
> will want to create subdirs and lots of fun like that. If they do,
> then
> we can change the api at that point in time :)
>
> thanks,
>
> greg k-h

I trying to fix an issue in the existing "soc_device_register()" code.
This looks to me a memory leak.

ret = device_register(&soc_dev->dev);
if (ret)
goto out3;
return soc_dev;
out3:
ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
put_device(&soc_dev->dev);
soc_dev = NULL;
out2:
kfree(soc_dev);

Here we are assigning "soc_dev=NULL" before freeing. I see this
assignment is unnecessary here.