[PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups

[PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups

[Suma Hegde]

Instead of manually calling devm_device_add_groups(), use
dev_groups.

Signed-off-by: Suma Hegde <suma.hegde@amd.com>
Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
---
This is based on the suggestions from Hans de Goede when Greg
Kroah-Hartman had suggested to switch to use device_add_groups().

 drivers/platform/x86/amd/hsmp.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
index 1927be901108..d6b43d8e798b 100644
--- a/drivers/platform/x86/amd/hsmp.c
+++ b/drivers/platform/x86/amd/hsmp.c
@@ -693,15 +693,29 @@ static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
 		hsmp_create_attr_list(attr_grp, dev, i);
 	}
 
-	return devm_device_add_groups(dev, hsmp_attr_grps);
+	dev->driver->dev_groups = hsmp_attr_grps;
+
+	return 0;
 }
 
+/* Number of sysfs groups to be created in case of ACPI probing */
+#define NUM_HSMP_SYSFS_GRPS	1
+
 static int hsmp_create_acpi_sysfs_if(struct device *dev)
 {
+	const struct attribute_group **hsmp_attr_grps;
 	struct attribute_group *attr_grp;
 	u16 sock_ind;
 	int ret;
 
+	/* Null terminated list of attribute groups */
+	hsmp_attr_grps = devm_kcalloc(dev, NUM_HSMP_SYSFS_GRPS + 1,
+				      sizeof(*hsmp_attr_grps),
+				      GFP_KERNEL);
+
+	if (!hsmp_attr_grps)
+		return -ENOMEM;
+
 	attr_grp = devm_kzalloc(dev, sizeof(struct attribute_group), GFP_KERNEL);
 	if (!attr_grp)
 		return -ENOMEM;
@@ -716,7 +730,12 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
 	if (ret)
 		return ret;
 
-	return devm_device_add_group(dev, attr_grp);
+	hsmp_attr_grps[0] = attr_grp;
+	hsmp_attr_grps[1] = NULL;
+
+	dev->driver->dev_groups	= hsmp_attr_grps;
+
+	return 0;
 }
 
 static int hsmp_cache_proto_ver(u16 sock_ind)
-- 
2.25.1

Re: [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups

[Hans de Goede]

Hi Suma,

On 4/10/24 2:17 PM, Suma Hegde wrote:
> Instead of manually calling devm_device_add_groups(), use
> dev_groups.
> 
> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> ---
> This is based on the suggestions from Hans de Goede when Greg
> Kroah-Hartman had suggested to switch to use device_add_groups().
> 
>  drivers/platform/x86/amd/hsmp.c | 23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 1927be901108..d6b43d8e798b 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -693,15 +693,29 @@ static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
>  		hsmp_create_attr_list(attr_grp, dev, i);
>  	}
>  
> -	return devm_device_add_groups(dev, hsmp_attr_grps);
> +	dev->driver->dev_groups = hsmp_attr_grps;
> +
> +	return 0;
>  }

You are now modifying the driver struct while the driver is being
probed(). That is really a bad idea.

The idea is to assign a static set of driver groups directly in
the driver declaration:

static struct platform_driver amd_hsmp_driver = {
        .probe          = hsmp_pltdrv_probe,
        .remove_new     = hsmp_pltdrv_remove,
        .driver         = {
                .name   = DRIVER_NAME,
                .acpi_match_table = amd_hsmp_acpi_ids,
        },
};

And if you then need certain sysfs attributes to only be shown
in certain conditions add an is_visible callback to your
const struct attribute_group, note you can use separate
is_visible callbacks per group to hide / unhide the entire
groupin one go.

Regards,

Hans




>  
> +/* Number of sysfs groups to be created in case of ACPI probing */
> +#define NUM_HSMP_SYSFS_GRPS	1
> +
>  static int hsmp_create_acpi_sysfs_if(struct device *dev)
>  {
> +	const struct attribute_group **hsmp_attr_grps;
>  	struct attribute_group *attr_grp;
>  	u16 sock_ind;
>  	int ret;
>  
> +	/* Null terminated list of attribute groups */
> +	hsmp_attr_grps = devm_kcalloc(dev, NUM_HSMP_SYSFS_GRPS + 1,
> +				      sizeof(*hsmp_attr_grps),
> +				      GFP_KERNEL);
> +
> +	if (!hsmp_attr_grps)
> +		return -ENOMEM;
> +
>  	attr_grp = devm_kzalloc(dev, sizeof(struct attribute_group), GFP_KERNEL);
>  	if (!attr_grp)
>  		return -ENOMEM;
> @@ -716,7 +730,12 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	return devm_device_add_group(dev, attr_grp);
> +	hsmp_attr_grps[0] = attr_grp;
> +	hsmp_attr_grps[1] = NULL;
> +
> +	dev->driver->dev_groups	= hsmp_attr_grps;
> +
> +	return 0;
>  }
>  
>  static int hsmp_cache_proto_ver(u16 sock_ind)

Re: [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups

[Hegde, Suma]

Hi Hans,


On 4/10/2024 5:54 PM, Hans de Goede wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> Hi Suma,
>
> On 4/10/24 2:17 PM, Suma Hegde wrote:
>> Instead of manually calling devm_device_add_groups(), use
>> dev_groups.
>>
>> Signed-off-by: Suma Hegde <suma.hegde@amd.com>
>> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
>> ---
>> This is based on the suggestions from Hans de Goede when Greg
>> Kroah-Hartman had suggested to switch to use device_add_groups().
>>
>>   drivers/platform/x86/amd/hsmp.c | 23 +++++++++++++++++++++--
>>   1 file changed, 21 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
>> index 1927be901108..d6b43d8e798b 100644
>> --- a/drivers/platform/x86/amd/hsmp.c
>> +++ b/drivers/platform/x86/amd/hsmp.c
>> @@ -693,15 +693,29 @@ static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
>>                hsmp_create_attr_list(attr_grp, dev, i);
>>        }
>>
>> -     return devm_device_add_groups(dev, hsmp_attr_grps);
>> +     dev->driver->dev_groups = hsmp_attr_grps;
>> +
>> +     return 0;
>>   }
> You are now modifying the driver struct while the driver is being
> probed(). That is really a bad idea.
>
> The idea is to assign a static set of driver groups directly in
> the driver declaration:
>
> static struct platform_driver amd_hsmp_driver = {
>          .probe          = hsmp_pltdrv_probe,
>          .remove_new     = hsmp_pltdrv_remove,
>          .driver         = {
>                  .name   = DRIVER_NAME,
>                  .acpi_match_table = amd_hsmp_acpi_ids,
>          },
> };
>
> And if you then need certain sysfs attributes to only be shown
> in certain conditions add an is_visible callback to your
> const struct attribute_group, note you can use separate
> is_visible callbacks per group to hide / unhide the entire
> groupin one go.
>
> Regards,
>
> Hans

Thank you for your response.  We are dynamically creating groups based 
on number of sockets available in the system.

The number of sockets in the system is known only after hsmp_plt_init() 
call. Hence I couldn't add it in

amd_hsmp_driver structure.

Now that I came to know that its a bad idea, I will check for other 
possible solution.

Thank you,

Suma

>
>> +/* Number of sysfs groups to be created in case of ACPI probing */
>> +#define NUM_HSMP_SYSFS_GRPS  1
>> +
>>   static int hsmp_create_acpi_sysfs_if(struct device *dev)
>>   {
>> +     const struct attribute_group **hsmp_attr_grps;
>>        struct attribute_group *attr_grp;
>>        u16 sock_ind;
>>        int ret;
>>
>> +     /* Null terminated list of attribute groups */
>> +     hsmp_attr_grps = devm_kcalloc(dev, NUM_HSMP_SYSFS_GRPS + 1,
>> +                                   sizeof(*hsmp_attr_grps),
>> +                                   GFP_KERNEL);
>> +
>> +     if (!hsmp_attr_grps)
>> +             return -ENOMEM;
>> +
>>        attr_grp = devm_kzalloc(dev, sizeof(struct attribute_group), GFP_KERNEL);
>>        if (!attr_grp)
>>                return -ENOMEM;
>> @@ -716,7 +730,12 @@ static int hsmp_create_acpi_sysfs_if(struct device *dev)
>>        if (ret)
>>                return ret;
>>
>> -     return devm_device_add_group(dev, attr_grp);
>> +     hsmp_attr_grps[0] = attr_grp;
>> +     hsmp_attr_grps[1] = NULL;
>> +
>> +     dev->driver->dev_groups = hsmp_attr_grps;
>> +
>> +     return 0;
>>   }
>>
>>   static int hsmp_cache_proto_ver(u16 sock_ind)