* [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups @ 2024-04-10 12:17 Suma Hegde 2024-04-10 12:24 ` Hans de Goede 0 siblings, 1 reply; 3+ messages in thread From: Suma Hegde @ 2024-04-10 12:17 UTC (permalink / raw) To: platform-driver-x86 Cc: ilpo.jarvinen, hdegoede, Suma Hegde, Naveen Krishna Chatradhi 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups 2024-04-10 12:17 [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups Suma Hegde @ 2024-04-10 12:24 ` Hans de Goede 2024-04-10 12:54 ` Hegde, Suma 0 siblings, 1 reply; 3+ messages in thread From: Hans de Goede @ 2024-04-10 12:24 UTC (permalink / raw) To: Suma Hegde, platform-driver-x86; +Cc: ilpo.jarvinen, Naveen Krishna Chatradhi 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) ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups 2024-04-10 12:24 ` Hans de Goede @ 2024-04-10 12:54 ` Hegde, Suma 0 siblings, 0 replies; 3+ messages in thread From: Hegde, Suma @ 2024-04-10 12:54 UTC (permalink / raw) To: Hans de Goede, platform-driver-x86 Cc: ilpo.jarvinen, Naveen Krishna Chatradhi 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) ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-04-10 12:54 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-04-10 12:17 [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups Suma Hegde 2024-04-10 12:24 ` Hans de Goede 2024-04-10 12:54 ` Hegde, Suma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox