Received-SPF: pass (google.com: domain of linux-kernel+bounces-49680-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <a04b3af5-ae3a-45b9-a871-0c04d6002166@amd.com>
Date: Fri, 2 Feb 2024 16:03:53 +0530
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] platform/x86/amd/hsmp: switch to use device_add_groups()
Content-Language: en-US
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 Hans de Goede <hdegoede@redhat.com>
Cc: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.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>
 <fb66b037-eeba-45d6-be41-215478cf3ba9@redhat.com>
 <2024020122-undertone-hammock-b08d@gregkh>
From: "Hegde, Suma" <Suma.Hegde@amd.com>
In-Reply-To: <2024020122-undertone-hammock-b08d@gregkh>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Z2djNHh0VURoTzZESVB0U1RCUkN0ZU9qWWdLUEhSOGZEVFhvVnhzNlF4VXpq?=
 =?utf-8?B?T3hseWdzc29WMjQwVHBMMXlNZDJPMXAxcFpqeHVBMitHdE93d0c5TTVmOUVQ?=
 =?utf-8?B?ZStCemRNb2QrWnMwMWdHem1EcVhYRWw1NkZkZVJ4K2pqWUh3OVdMOVNuaDZN?=
 =?utf-8?B?VHBOZHduM1d5RzkxS0c1aEE3dm5yRWcwaldmVjBiajdQL1ZmWkFIZzN6NXl1?=
 =?utf-8?B?NEQzTklZZnllOUlYbzljVTRWNitYaTN5N0VWSHBrNmVHcHNGV0dZM0N2SFA3?=
 =?utf-8?B?SmhteGpZSkFtTWppSzNsWnRGenFyclhhd0ZMYlBCYzYwcGlKWnNpRWI0Nzlx?=
 =?utf-8?B?K0RrRHNEc2ZGNlNla0hKQWtRcmozN2FkQmhTdW96WTlraGpOcVZRRXh3RFI5?=
 =?utf-8?B?SEJqekVXanZMazk1bit1a2YraG0xd3VLY2xaTXRhMU9idnA0bXBQbktLMFI3?=
 =?utf-8?B?bW10SjZNaXRST2k3OUFibkpCQVlHci9yc2ZQVVZqUEdGVld5dmdFODluRjZ1?=
 =?utf-8?B?TUlUR1k1ODlWR1NvY0RrTFRPcFlKQitKWFJnY2VPVEpLT0ZzcUZ6WU01T013?=
 =?utf-8?B?bU9JY2RkN2pMOWg5dk1Nbm9raUI2Z3kzKzhYc3JiOW9zYXBPaGR3SW5pdTNn?=
 =?utf-8?B?SUk4b3JGN2lNUnFaQzYxdkR1eXVQUUVHd0FoOHpKWGUxaEFoSzhYdjdobStv?=
 =?utf-8?B?SDZBa2JyTnc5OVRBRjRnZnhYSnNhcEVjTkhBd3R1T0lnOE9oT2VkMmlNNVcw?=
 =?utf-8?B?dTIxZlVacUJ3VmR4ZEZWZzFlMDdnT3JaTlFDcUlkeGhpUFFoQW5CU2MxaGZM?=
 =?utf-8?B?VmMyK3BGYXh4Rk5xakF6MHl4a0ZuRE5HUUQ2WTFPY0tYVkZkRXZJM2lQazZp?=
 =?utf-8?B?MGxIVkdLK0p3TmlpUi9lUHQrRW1uWVU0L2NqbWJaSzJ5RExveDE3eUVuZVYw?=
 =?utf-8?B?bUpOYlM0anNTSmFRZjhVMFFZbGdoQXQ0OVM1QUd4NEMwV2Fta1l4RTBqaFh2?=
 =?utf-8?B?M1FHTDIvVXhxdkNOZUJoTTRUS3ZackxJMTlvT3VPcVkvTzNKOUFuUVNQQ0V0?=
 =?utf-8?B?WHBxbUViRHJrc0E0OGhPbGFVeTh0L0c3ZktYNVp5L1lXU2J4ZWk0MDgyemJ0?=
 =?utf-8?B?UGV2Q2dnZXR0TkQ2UVlCbGVjdS9MbE04MUhEMlNzMC9aazRjQ2xDckZDR2V1?=
 =?utf-8?B?enJWT1hXdVZ3V3V6dmFKZ1pBVmZXTVlBL1M5ZEVyRGNLb2Rady8wbEdMUkpO?=
 =?utf-8?B?STMxektMRTNILzY2YjBsNzlKakczd0xhYjNjUlY1Z2Z1RVhwK21EQUdrUytK?=
 =?utf-8?B?aU5Ta2FPalFITi9kMWt3MTN2QStxTVljMVY1Y1BjY0g3dnltZ0w4ZzZOanJV?=
 =?utf-8?B?eUJhYnZTVFY3WnlXL2VPMU4rVmNCQ2YvMVlWZEw0VHAzSHlBcmpDMmFyWmZF?=
 =?utf-8?B?YU1GZ212bWxpWk9XTjBHbmNZcWR6Zm1iL2RYUitHSkF6cGYxN3hubURYd0Ri?=
 =?utf-8?B?c3RuRWkrdHBYVUlOTVhsajlNSnNjYitJQTU3QVRUcnRhWnRFWjVtaW9qQUxl?=
 =?utf-8?B?Z2ZBams3eGhFOUZKeUI1SGE1K0hkWTFwV2N5b3VLNW8zUDJFK1ZHdzdzSkNP?=
 =?utf-8?B?SWxmc1JMc1pMeThsTU9KaVdyeGNQUFREN0o2RlV4RUlaa2VSKzZKYVdrMC9m?=
 =?utf-8?B?ZGNJZ3U3SWZ3VDlOZCtPSnUreURtaFVSc0tzaEFldTh3bXM3U2N0SVg0RTVi?=
 =?utf-8?B?cWlkWUxDLzg2Q1F0WFBXUVRBOGxXL3hKUkozVWg0MnZCY0lXWk5CZXV1em9E?=
 =?utf-8?B?MWc3ejZENnpOOVlvTGRkYTljZVM1by9sZEE5STAzTmlUc1ZMQUw1ZG05WXNR?=
 =?utf-8?B?RzNLemwrOGdUalhlT1BGVU1aVThncU1CaHpocFJVWTBKUHQ3QmZaSXRrUWdT?=
 =?utf-8?B?UHdOZkY2eFZmUlh3RlJvbGFKSGQweWJ6VXZnenEzck1lekE4QjBHcDVmQkwz?=
 =?utf-8?B?SW1Rd2ttcEQwajVUVUZyaXRMVWEwekt1VlcwZ3lnblUra0E1ODNlQmZ0UG5p?=
 =?utf-8?B?dkZ6UFh1Mzdab3J6anNkb1hZc3pva1U2bmpIbzR6bVZ0TGloYUlvYnZQZXl5?=
 =?utf-8?Q?WuKs/xbMF6aSjakdjtPK1C3Is?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6d4998dc-fea8-4710-4f1b-08dc23da7826
X-MS-Exchange-CrossTenant-AuthSource: LV2PR12MB5966.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2024 10:34:00.8359
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: jX640zqNALJ7BC/skuZwuaeIzRULjksIIqeVu/qrkn1kKosUQaV4snLvx4KwoZt32sDqTKddUTPQlwUxwbVy/w==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR12MB6701


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