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@xxxxxxx>
> Reviewed-by: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@xxxxxxx>
> ---
> 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)
