Received-SPF: pass (google.com: domain of linux-kernel+bounces-54558-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
Date: Tue, 6 Feb 2024 11:01:13 +0200 (EET)
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
cc: Hans de Goede <hdegoede@redhat.com>, 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: <2024020540-siesta-palm-03cd@gregkh>
Message-ID: <3522e43e-a54c-0ac1-e4b8-beea9b7e0bd0@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> <e8605fd7-1ffc-00ff-ec3b-e125085d4e92@linux.intel.com>
 <2024020540-siesta-palm-03cd@gregkh>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/mixed; BOUNDARY="8323328-1903269187-1707209279=:1141"
Content-ID: <21a85ceb-6a8f-fc9f-b2e0-de572ce81811@linux.intel.com>

  This message is in MIME format.  The first part should be readable text,
  while the remaining parts are likely unreadable without MIME-aware tools.

--8323328-1903269187-1707209279=:1141
Content-Type: text/plain; CHARSET=ISO-8859-15
Content-Transfer-Encoding: QUOTED-PRINTABLE
Content-ID: <a1d6fb3c-5406-0c11-1250-74927851f899@linux.intel.com>

On Mon, 5 Feb 2024, Greg Kroah-Hartman wrote:

> On Mon, Feb 05, 2024 at 12:27:24PM +0200, Ilpo J=E4rvinen 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 att=
aching
> > > >>> 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.
> > > >=20
> > > > Ooops, I misread that:
> > > > =09static struct hsmp_plat_device plat_dev;
> > > > was not the actual device struct anymore.
> > > >=20
> > > >> 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()
> > > >=20
> > > > Ok, much better.
> > > >=20
> > > >> So AFAICT this should keep using the devm_ variant to properly
> > > >> cleanup the sysfs attributes.
> > > >=20
> > > > This devm_ variant is odd, and should never have been created as th=
e
> > > > 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.)
> > > >=20
> > > > That's why I want to get rid of it, it's pointless :)
> > > >=20
> > > >> 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.
> > > >=20
> > > > The initial issue I saw with this is that these attributes are bein=
g
> > > > 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 suppor=
t
> > > > devices of multiple types in the same codebase at the same time.
> > > >=20
> > > > 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 th=
e
> > > > other one, and so this would be the last, hence my attention to thi=
s.
> > > >=20
> > > > Again, moving from devm_device_add_groups() to device_add_groups() =
is a
> > > > no-op from a functional standpoint, so this should be fine.
> > >=20
> > > Ok, I was not aware that the core automatically cleans up
> > > all the attributes anyways.
> > >=20
> > > In that case this fine with me and I agree with merging this
> > > so that you can entirely remove the  devm_ variant:
> > >=20
> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> >=20
> > Greg,
> >=20
> > Does this same stuff apply to devm_device_add_group() which was added=
=20
> > along the ACPI changes?
>=20
> Probably, I haven't looked at that yet.
>
> > And the changelog is quite misleading as is, it should be changed to=20
> > match the real motivation.
>=20
> "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 :)
>=20
> 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=
=20
to apply it as is and you likely want to extend the patch to include the=20
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=20
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.
=2E..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=
=20
is pointless and does nothing useful on top of the other function.


--=20
 i.
--8323328-1903269187-1707209279=:1141--