Received-SPF: pass (google.com: domain of linux-kernel+bounces-50023-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 2 Feb 2024 07:32:32 -0800
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Hans de Goede <hdegoede@redhat.com>
Cc: naveenkrishna.chatradhi@amd.com, linux-kernel@vger.kernel.org,
	Carlos Bilbao <carlos.bilbao@amd.com>,
	Ilpo =?iso-8859-1?Q?J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>,
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v2] platform/x86/amd/hsmp: switch to use
 device_add_groups()
Message-ID: <2024020243-blinks-pantomime-c51e@gregkh>
References: <2024020145-junior-outnumber-3e76@gregkh>
 <07010c54-2e44-463b-9a9b-95697fd30ffd@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <07010c54-2e44-463b-9a9b-95697fd30ffd@redhat.com>

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