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@xxxxxxxxxxxxxx>
> ---
>  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