From: "Hegde, Suma" <Suma.Hegde@amd.com>
To: Hans de Goede <hdegoede@redhat.com>, platform-driver-x86@vger.kernel.org
Cc: ilpo.jarvinen@linux.intel.com,
Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
Subject: Re: [PATCH] platform/x86/amd/hsmp: Remove devm_* call for sysfs and use dev_groups
Date: Wed, 10 Apr 2024 18:24:21 +0530 [thread overview]
Message-ID: <263b7201-7796-422e-b067-f83b2b645ae6@amd.com> (raw)
In-Reply-To: <9e1605ea-147a-4b17-b4e2-692b34926bf2@redhat.com>
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)
prev parent reply other threads:[~2024-04-10 12:54 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
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 message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=263b7201-7796-422e-b067-f83b2b645ae6@amd.com \
--to=suma.hegde@amd.com \
--cc=hdegoede@redhat.com \
--cc=ilpo.jarvinen@linux.intel.com \
--cc=naveenkrishna.chatradhi@amd.com \
--cc=platform-driver-x86@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).