Message-ID: <3522e43e-a54c-0ac1-e4b8-beea9b7e0bd0@linux.intel.com>
Date: Tue, 6 Feb 2024 11:01:13 +0200 (EET)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
cc: Hans de Goede <hdegoede@...hat.com>, naveenkrishna.chatradhi@....com, 
    LKML <linux-kernel@...r.kernel.org>, Carlos Bilbao <carlos.bilbao@....com>, 
    platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH v2] platform/x86/amd/hsmp: switch to use
 device_add_groups()

On Mon, 5 Feb 2024, Greg Kroah-Hartman wrote:

> On Mon, Feb 05, 2024 at 12:27:24PM +0200, Ilpo Järvinen wrote:
> > On Fri, 2 Feb 2024, Hans de Goede wrote:
> > > On 2/2/24 16:32, Greg Kroah-Hartman wrote:
> > > > On Fri, Feb 02, 2024 at 08:49:39AM +0100, Hans de Goede wrote:
> > > >> Hi Greg,
> > > >>
> > > >> On 2/2/24 03:44, 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.
> > > >>
> > > >> As I mentioned in my reply to v1, this is not correct.
> > > >>
> > > >> There is a global data struct, but that holds a struct device
> > > >> pointer, not the device struct.
> > > > 
> > > > Ooops, I misread that:
> > > > 	static struct hsmp_plat_device plat_dev;
> > > > was not the actual device struct anymore.
> > > > 
> > > >> The device itself is created with platform_device_alloc() +
> > > >> platform_device_add() from module-init and it is removed
> > > >> on module-exit by calling platform_device_unregister()
> > > > 
> > > > Ok, much better.
> > > > 
> > > >> So AFAICT this should keep using the devm_ variant to properly
> > > >> cleanup the sysfs attributes.
> > > > 
> > > > This devm_ variant is odd, and should never have been created as the
> > > > sysfs core always cleans up the sysfs attributes when a device is
> > > > removed, there is no need for it (i.e. they do the same thing.)
> > > > 
> > > > That's why I want to get rid of it, it's pointless :)
> > > > 
> > > >> But what this really needs is to be converted to using
> > > >> amd_hsmp_driver.driver.dev_groups rather then manually
> > > >> calling devm_device_add_groups() I have already asked
> > > >> Suma Hegde (AMD) to take a look at this.
> > > > 
> > > > The initial issue I saw with this is that these attributes are being
> > > > created dynamically, so using dev_groups can be a bit harder.  The code
> > > > paths here are twisty and not obvious as it seems to want to support
> > > > devices of multiple types in the same codebase at the same time.
> > > > 
> > > > But yes, using dev_groups is ideal, and if that happens, I'm happy.
> > > > It's just that there are now only 2 in-kernel users of
> > > > devm_device_add_groups() and I have a patch series to get rid of the
> > > > other one, and so this would be the last, hence my attention to this.
> > > > 
> > > > Again, moving from devm_device_add_groups() to device_add_groups() is a
> > > > no-op from a functional standpoint, so this should be fine.
> > > 
> > > Ok, I was not aware that the core automatically cleans up
> > > all the attributes anyways.
> > > 
> > > In that case this fine with me and I agree with merging this
> > > so that you can entirely remove the  devm_ variant:
> > > 
> > > Reviewed-by: Hans de Goede <hdegoede@...hat.com>
> > 
> > Greg,
> > 
> > Does this same stuff apply to devm_device_add_group() which was added 
> > along the ACPI changes?
> 
> Probably, I haven't looked at that yet.
>
> > And the changelog is quite misleading as is, it should be changed to 
> > match the real motivation.
> 
> "Motivation" isn't always needed in a changelog text, I was trying to
> describe why this specific instance was not needed, not the overall
> pointlessness of the function :)
> 
> I got the text wrong about this being a static variable (but one is
> still in there, so it's confusing.)

Yes, I mainly meant the not-dynamic part is no longer true so I don't want 
to apply it as is and you likely want to extend the patch to include the 
newly introduced devm_device_add_group() call conversion.

> I'll be glad to reword this if needed to just say "This function is
> pointless, does nothing, and is about to be removed from the kernel so
> stop using it", or something along those lines...

Given that it wasn't obvious to either me nor Hans, it would have been 
useful to mention it. The current wording had a different undertone so
after the driver got changed it looked as if the patch become obsolete.
...But that turned out was not the case because of motivation outside of
the one given in the commit message, so yes, if we look this from a
reviewer perspective, it would be useful information to tell the function 
is pointless and does nothing useful on top of the other function.


-- 
 i.

Powered by blists - more mailing lists