Received-SPF: pass (google.com: domain of linux-kernel+bounces-52410-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Mon, 5 Feb 2024 12:27:24 +0200 (EET)
To: Hans de Goede <hdegoede@redhat.com>, 
    Greg Kroah-Hartman <gregkh@linuxfoundation.org>
cc: naveenkrishna.chatradhi@amd.com, LKML <linux-kernel@vger.kernel.org>, 
    Carlos Bilbao <carlos.bilbao@amd.com>, platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH v2] platform/x86/amd/hsmp: switch to use
 device_add_groups()
In-Reply-To: <06e92b87-4d48-4519-b1db-6d7605bf3962@redhat.com>
Message-ID: <e8605fd7-1ffc-00ff-ec3b-e125085d4e92@linux.intel.com>
References: <2024020145-junior-outnumber-3e76@gregkh> <07010c54-2e44-463b-9a9b-95697fd30ffd@redhat.com> <2024020243-blinks-pantomime-c51e@gregkh> <06e92b87-4d48-4519-b1db-6d7605bf3962@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

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.

-- 
 i.