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

Greg KH
2019-10-03T07:05:07Z

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