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

mnalajal
2019-10-03T18:17:45Z

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.