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

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

[Murali Nalajala]

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;
 };
 
 /**
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Greg KH]

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(-)

Can you change a soc driver to use this?  I don't think that this patch
works because:

> 
> 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;

You set this, but never do anything with it that I can see.  What am I
missing?

> +
>  	/* 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:

You don't free it when the soc is removed?

thanks,

greg k-h

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

[Greg KH]

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?

thanks,

greg k-h

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

[mnalajal]

On 2019-10-03 00:05, 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(-)
> 
> Can you change a soc driver to use this?  I don't think that this patch
> works because:
> 
>> 
>> 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;
> 
> You set this, but never do anything with it that I can see.  What am I
> missing?
no, since i am using the "soc_attr_groups" name as it here you do not 
see the assignment below.
It is something like this soc_dev->dev.groups = soc_attr_groups;
> 
>> +
>>  	/* 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:
> 
> You don't free it when the soc is removed?
agree, will fix it in my next patch.
> 
> thanks,
> 
> greg k-h
These changes are verified at my side on SM8250 with mode static 
compilation and module.

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

[mnalajal]

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.
> 
> thanks,
> 
> greg k-h

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

[Greg KH]

On Thu, Oct 03, 2019 at 11:17:39AM -0700, mnalajal@codeaurora.org wrote:
> On 2019-10-03 00:05, 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(-)
> > 
> > Can you change a soc driver to use this?  I don't think that this patch
> > works because:
> > 
> > > 
> > > 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;
> > 
> > You set this, but never do anything with it that I can see.  What am I
> > missing?
> no, since i am using the "soc_attr_groups" name as it here you do not see
> the assignment below.
> It is something like this soc_dev->dev.groups = soc_attr_groups;

Ah, I see that now.  Tricky :)

nice work,

greg k-h

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

[Greg KH]

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

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

[mnalajal]

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.

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

[Bjorn Andersson]

On Thu 03 Oct 14:11 PDT 2019, mnalajal@codeaurora.org wrote:

> 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);

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

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

[Stephen Boyd]

Quoting Murali Nalajala (2019-10-02 17:06:14)
> @@ -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;

Don't initialize this to NULL because it is only tested after it's been
unconditionally assigned to the result of the allocation.

>         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) *

Please use kcalloc() instead and drop the define for NUM_ATTR_GROUPS
because it's used once.

> +                                               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;

Drop this assignment to NULL because kzalloc() and kcalloc() zero out
the memory anyway.

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

[mnalajal]

On 2019-10-03 16:22, Stephen Boyd wrote:
> Quoting Murali Nalajala (2019-10-02 17:06:14)
>> @@ -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;
> 
> Don't initialize this to NULL because it is only tested after it's been
> unconditionally assigned to the result of the allocation.
Done
> 
>>         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) *
> 
> Please use kcalloc() instead and drop the define for NUM_ATTR_GROUPS
> because it's used once.
> 
done
>> +                                               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;
> 
> Drop this assignment to NULL because kzalloc() and kcalloc() zero out
> the memory anyway.
done