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

Bjorn Andersson
2019-10-08T18:06:50Z

On Mon 07 Oct 13:37 PDT 2019, 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>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
> Changes in v2:
> - Address comments from Stephen Boyd about "soc_dev" clean up in error paths.
>
> Changes in v1:
> - Remove NULL initialization of "soc_attr_groups"
> - Taken care of freeing "soc_attr_groups" in soc_release()
> - Addressed Stephen Boyd comments on usage of "kalloc"
>
> drivers/base/soc.c | 30 +++++++++++++++++-------------
> include/linux/sys_soc.h | 1 +
> 2 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/soc.c b/drivers/base/soc.c
> index 7c0c5ca..4af11a4 100644
> --- a/drivers/base/soc.c
> +++ b/drivers/base/soc.c
> @@ -104,15 +104,12 @@ 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);
>
> + ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> + kfree(soc_dev->dev.groups);
> kfree(soc_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;
> int ret;
>
> if (!soc_bus_type.p) {
> @@ -136,10 +134,18 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
> goto out1;
> }
>
> + soc_attr_groups = kcalloc(3, sizeof(*soc_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;
> +
> /* 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;
> @@ -150,15 +156,15 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
> dev_set_name(&soc_dev->dev, "soc%d", soc_dev->soc_dev_num);
>
> ret = device_register(&soc_dev->dev);
> - if (ret)
> - goto out3;
> + if (ret) {
> + put_device(&soc_dev->dev);
> + return ERR_PTR(ret);
> + }
>
> return soc_dev;
>
> out3:
> - ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> - put_device(&soc_dev->dev);
> - soc_dev = NULL;
> + kfree(soc_attr_groups);
> out2:
> kfree(soc_dev);
> out1:
> @@ -169,8 +175,6 @@ struct soc_device *soc_device_register(struct soc_device_attribute *soc_dev_attr
> /* Ensure soc_dev->attr is freed prior to calling soc_device_unregister. */
> void soc_device_unregister(struct soc_device *soc_dev)
> {
> - ida_simple_remove(&soc_ida, soc_dev->soc_dev_num);
> -
> device_unregister(&soc_dev->dev);
> early_soc_dev_attr = NULL;
> }
> 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
>