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@xxxxxxxxxx> 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. -- i.
