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

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

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.

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().

2. Ideally amd_hsmp_platdev should be the only global static variable
and plat_dev should simply be drvdata of the platform_device.

Suma, can you take a look at maybe fixing these, especially 1. ?

Regards,

Hans