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

[Hans de Goede <hdegoede@redhat.com>]

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)