Re: [PATCH] platform/x86/amd/hsmp: switch to use device_add_groups()

["Hegde, Suma" <Suma.Hegde@amd.com>]

On 2/1/2024 9:04 PM, Greg Kroah-Hartman wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>
>
> On Thu, Feb 01, 2024 at 03:50:08PM +0100, Hans de Goede wrote:
>> Hi Greg,
>>
>> On 2/1/24 15:47, Greg Kroah-Hartman wrote:
>>> On Thu, Feb 01, 2024 at 04:34:30PM +0200, Ilpo Järvinen wrote:
>>>> On Thu, 1 Feb 2024, Greg Kroah-Hartman wrote:
>>>>
>>>>> On Thu, Feb 01, 2024 at 06:50:33PM +0530, Hegde, Suma wrote:
>>>>>> On 1/29/2024 6:16 PM, Ilpo Järvinen wrote:
>>>>>>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
>>>>>>>
>>>>>>>
>>>>>>> + Cc Suma Hegde.
>>>>>>>
>>>>>>> On Sun, 28 Jan 2024, Greg Kroah-Hartman wrote:
>>>>>>>
>>>>>>>> The use of devm_*() functions works properly for when the device
>>>>>>>> structure itself is dynamic, but the hsmp driver is attempting to have a
>>>>>>>> local, static, struct device and then calls devm_() functions attaching
>>>>>>>> memory to the device that will never be freed.
>>>>>>>>
>>>>>>>> The logic of having a static struct device is almost never a wise
>>>>>>>> choice, but for now, just remove the use of devm_device_add_groups() in
>>>>>>>> this driver as it obviously is not needed.
>>>>>> Hi Greg,
>>>>>>
>>>>>> Could you please hold on merging this patch for a week? I will push a patch
>>>>>> for converting platform specific structure's memory allocation from static
>>>>>> to a dynamic
>>>>>>
>>>>>> allocation.
>>>>> Push it where?  Ususally we do "first patch wins" type stuff, why not
>>>>> just do your work on top of mine?
>>>>>
>>>>> Also, when you do make the needed changes, please remove the explicit
>>>>> call to create sysfs groups and use the default groups pointer instead,
>>>>> that will make things much simpler and avoid races in the code.
>>>> Hi Greg,
>>>>
>>>> Well, if you really want to "win" :-), please provide an updated version
>>>> which considers the changes already made in the for-next branch (the
>>>> current one won't apply).
>>> Fair enough, I don't want to "win", I just want to squash any "hold off
>>> and don't make any changes to this file because I was going to plan on
>>> doing something else here in the future" type of stuff, as that is what
>>> has been documented to take down many projects in the past.
>>>
>>> That's why we almost always take patches that people have submitted
>>> today, instead of ignoring them for potential future changes, unless of
>>> course, they are not acceptable.
>>>
>>> I'll rebase on linux-next, rejecting it for that reason is totally valid :)
>> I checked the code in linux-next and the dev passed to devm_device_add_groups()
>> now is &amd_hsmp_platdev->dev and amd_hsmp_platdev gets properly removed
>> from hsmp_plt_exit(), so I believe that keeping the devm_... call is
>> the right thing to do.
> I'm trying to delete the devm_device_add_groups() function entirely from
> the kernel, so I might disagree with you there :)
>
>> With that said this driver really could use some modernization
>> (even though it is not a very old driver):
>>
>> 1. The sysfs attribute registration should really switch to using
>> amd_hsmp_driver.driver.dev_groups rather then manually
>> calling devm_device_add_groups().
> Yes, I'm all for that, I'll look at that this afternoon.  That's my main
> goal here, to get rid of ALL manual group additions in the tree if at
> all possible.  And for those rare cases that it isn't, because they are
> dynamically created, work on a solution for those.
>
> thanks,
>
> greg k-h\

Thanks Hans for the suggestion. I will push patches addressing both 1 & 
2  on review-ilpo branch.

Regards,

Suma