Received-SPF: pass (google.com: domain of linux-kernel+bounces-48339-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <fb66b037-eeba-45d6-be41-215478cf3ba9@redhat.com>
Date: Thu, 1 Feb 2024 15:50:08 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] platform/x86/amd/hsmp: switch to use device_add_groups()
Content-Language: en-US, nl
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Cc: "Hegde, Suma" <Suma.Hegde@amd.com>, naveenkrishna.chatradhi@amd.com,
 LKML <linux-kernel@vger.kernel.org>, Carlos Bilbao <carlos.bilbao@amd.com>,
 platform-driver-x86@vger.kernel.org
References: <2024012822-exalted-fidgeting-f180@gregkh>
 <0d110d2a-da0c-017a-0e5a-fc6bef7b066a@linux.intel.com>
 <e73426f6-6d50-4ed7-8613-1ec42fa3f991@amd.com>
 <2024020144-duplicity-nuptials-1cd7@gregkh>
 <f48ebcb1-8f87-efee-08ed-844775c995aa@linux.intel.com>
 <2024020135-sly-theft-0594@gregkh>
From: Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <2024020135-sly-theft-0594@gregkh>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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