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

  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.

Cc: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
Cc: Carlos Bilbao <carlos.bilbao@amd.com>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
Cc: platform-driver-x86@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
v2: rebased against platform/for-next

 drivers/platform/x86/amd/hsmp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

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.

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()

So AFAICT this should keep using the devm_ variant to properly
cleanup the sysfs attributes.

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.

Regards,

Hans





> 
> 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.
> 
> Cc: Naveen Krishna Chatradhi <naveenkrishna.chatradhi@amd.com>
> Cc: Carlos Bilbao <carlos.bilbao@amd.com>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: "Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>
> Cc: platform-driver-x86@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
> v2: rebased against platform/for-next
> 
>  drivers/platform/x86/amd/hsmp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/amd/hsmp.c b/drivers/platform/x86/amd/hsmp.c
> index 1927be901108..d84ea66eecc6 100644
> --- a/drivers/platform/x86/amd/hsmp.c
> +++ b/drivers/platform/x86/amd/hsmp.c
> @@ -693,7 +693,7 @@ static int hsmp_create_non_acpi_sysfs_if(struct device *dev)
>  		hsmp_create_attr_list(attr_grp, dev, i);
>  	}
>  
> -	return devm_device_add_groups(dev, hsmp_attr_grps);
> +	return device_add_groups(dev, hsmp_attr_grps);
>  }
>  
>  static int hsmp_create_acpi_sysfs_if(struct device *dev)
  

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.

thanks,

greg k-h
  

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.
  

Hi,

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@redhat.com>

Regards,

Hans
  

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@redhat.com>

Greg,

Does this same stuff apply to devm_device_add_group() which was added 
along the ACPI changes?

And the changelog is quite misleading as is, it should be changed to 
match the real motivation.
  

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@redhat.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.)

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

thanks,

greg k-h
  

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