Re: [PATCH v2] platform/x86/amd/hsmp: switch to use device_add_groups()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 2 Feb 2024, 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.

It wants to provide metrics for each socket. The ACPI part was a recent 
addition (as you've now probably discovered) and works slighty differently 
because the discovered structure is different but it's not really that 
different otherwise.

-- 
 i.

> 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.