On Thu 03 Oct 14:11 PDT 2019, [email protected] wrote: > On 2019-10-03 11:33, Greg KH wrote: > > On Thu, Oct 03, 2019 at 11:23:45AM -0700, [email protected] 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 <[email protected]> > > > > > --- > > > > > 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);
This put_device() will invoke soc_release() which will free soc_dev. > soc_dev = NULL; So setting soc_dev to NULL here turns below kfree() into a nop. > out2: > kfree(soc_dev); > > Here we are assigning "soc_dev=NULL" before freeing. I see this assignment > is unnecessary here. The code works as intended and the assignment prevents a double free. But it's perhaps slightly too clever. Swapping the allocation order of the ida and soc_dev would make this clearer. Regards, Bjorn
