linux-kernel - Re: [PATCH] platform/x86/amd/hsmp: switch to use device_add

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date: Fri, 2 Feb 2024 16:03:53 +0530
From: "Hegde, Suma" <Suma.Hegde@....com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Hans de Goede <hdegoede@...hat.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
 naveenkrishna.chatradhi@....com, LKML <linux-kernel@...r.kernel.org>,
 Carlos Bilbao <carlos.bilbao@....com>, platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] platform/x86/amd/hsmp: switch to use device_add_groups()


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