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
