Message-ID: <CAJZ5v0iH8jkqJaSNtqaTHxt_305DeiEq0AqQCo4Eho5hMKkU4Q@mail.gmail.com>
Date: Thu, 27 Nov 2025 23:14:27 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Armin Wolf <W_Armin@....de>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Daniel Lezcano <daniel.lezcano@...aro.org>, 
	Zhang Rui <rui.zhang@...el.com>, Lukasz Luba <lukasz.luba@....com>, Len Brown <lenb@...nel.org>, 
	Jonathan Corbet <corbet@....net>, Ido Schimmel <idosch@...dia.com>, Petr Machata <petrm@...dia.com>, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	etnaviv@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org, 
	linux-tegra@...r.kernel.org, linux-acpi@...r.kernel.org, 
	linux-doc@...r.kernel.org, netdev@...r.kernel.org, 
	linux-wireless@...r.kernel.org, ath10k@...ts.infradead.org, 
	ath11k@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, 
	linux-mediatek@...ts.infradead.org, platform-driver-x86@...r.kernel.org, 
	linux-pci@...r.kernel.org, imx@...ts.linux.dev, 
	linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH RFC RESEND 0/8] thermal: core: Allow setting the parent
 device of thermal zone/cooling devices

On Thu, Nov 27, 2025 at 9:29 PM Armin Wolf <W_Armin@....de> wrote:
>
> Am 27.11.25 um 19:22 schrieb Rafael J. Wysocki:
>
> > On Sat, Nov 22, 2025 at 3:18 PM Armin Wolf <W_Armin@....de> wrote:
> >> Am 21.11.25 um 21:35 schrieb Rafael J. Wysocki:
> >>
> >>> On Thu, Nov 20, 2025 at 4:41 AM Armin Wolf <W_Armin@....de> wrote:
> > [...]
> >
> >>>> ---
> >>>> Armin Wolf (8):
> >>>>         thermal: core: Allow setting the parent device of cooling devices
> >>>>         thermal: core: Set parent device in thermal_of_cooling_device_register()
> >>>>         ACPI: processor: Stop creating "device" sysfs link
> >>> That link is not to the cooling devices' parent, but to the ACPI
> >>> device object (a struct acpi_device) that corresponds to the parent.
> >>> The parent of the cooling device should be the processor device, not
> >>> its ACPI companion, so I'm not sure why there would be a conflict.
> >>   From the perspective of the Linux device core, a parent device does not have to be
> >> a "physical" device. In the case of the ACPI processor driver, the ACPI device is used,
> >> so the cooling device registered by said driver belongs to the ACPI device.
> > Well, that's a problem.  A struct acpi_device should not be a parent
> > of anything other than a struct acpi_device.
>
> Understandable, in this case we should indeed use the the CPU device, especially since the fwnode
> associated with it already points to the correct ACPI processor object (at least on my machine).
>
> >> I agree that using the Linux processor device would make more sense, but this will require
> >> changes inside the ACPI processor driver.
> > So be it.
>
> OK.
>
> >> As for the "device" symlink: The conflict would be a naming conflict, as both "device" symlinks
> >> (the one created by the ACPI processor driver and the one created by the device core) will
> >> be created in the same directory (which is the directory of the cooling device).
> > I see.
> >
> > But why is the new symlink needed in the first place?  If the device
> > has a parent, it will appear under that parent in /sys/devices/, won't
> > it?
> >
> > Currently, all of the thermal class devices appear under
> > /sys/devices/virtual/thermal/ because they have no parents and they
> > all get a class parent kobject under /sys/devices/virtual/, as that's
> > what get_device_parent() does.
> >
> > If they have real parents, they will appear under those parents, so
> > why will the parents need to be pointed to additionally?
>
> The "device" smylink is a comfort feature provided by the device core itself to allow user space
> application to traverse the device tree from bottom to top, like a double-linked list. We cannot
> disable the creation of this symlink, nor should we.

I think you mean device_add_class_symlinks(), but that's just for
class devices.  Of course, thermal devices are class devices, so
they'll get those links if they get parents.  Fair enough.

> > BTW, this means that the layout of /sys/devices/ will change when
> > thermal devices get real parents.  I'm not sure if this is a problem,
> > but certainly something to note.
>
> I know, most applications likely use /sys/class/thermal/, so they are not impacted by this. I will
> note this in the cover letter of the next revision.
>
> >>>>         ACPI: fan: Stop creating "device" sysfs link
> >>>>         ACPI: video: Stop creating "device" sysfs link
> >>> Analogously in the above two cases AFAICS.
> >>>
> >>> The parent of a cooling device should be a "physical" device object,
> >>> like a platform device or a PCI device or similar, not a struct
> >>> acpi_device (which in fact is not a device even).
> >>   From the perspective of the Linux device core, a ACPI device is a perfectly valid device.
> > The driver core is irrelevant here.
> >
> > As I said before, a struct acpi_device object should not be a parent
> > of anything other than a struct acpi_device object.  Those things are
> > not devices and they cannot be used for representing PM dependencies,
> > for example.
> >
> >> I agree that using a platform device or PCI device is better, but this already happens
> >> inside the ACPI fan driver (platform device).
> > So it should not happen there.
>
> I meant that the ACPI fan driver already uses the platform device as the parent device of the
> cooling device, so the ACPI device is only used for interacting with the ACPI control methods
> (and registering sysfs attributes i think).

OK

> >> Only the ACPI video driver created a "device" sysfs link that points to the ACPI device
> >> instead of the PCI device. I just noticed that i accidentally changed this by using the
> >> PCI device as the parent device for the cooling device.
> >>
> >> If you want then we can keep this change.
> > The PCI device should be its parent.
>
> Alright, i will note this in the patch description.
>
> >>>>         thermal: core: Set parent device in thermal_cooling_device_register()
> >>>>         ACPI: thermal: Stop creating "device" sysfs link
> >>> And this link is to the struct acpi_device representing the thermal zone itself.
> >> Correct, the ACPI thermal zone driver is a ACPI driver, meaning that he binds to
> >> ACPI devices. Because of this all (thermal zone) devices created by an instance of
> >> said driver are descendants of the ACPI device said instance is bound to.
> >>
> >> We can of course convert the ACPI thermal zone driver into a platform driver, but
> >> this would be a separate patch series.
> > If you want parents, this needs to be done first, but I'm still not
> > sure what the parent of a thermal zone would represent.
> >
> > In the ACPI case it is kind of easy - it would be the (platform)
> > device corresponding to a given ThermalZone object in the ACPI
> > namespace - but it only has a practical meaning if that device has a
> > specific parent.  For example, if the corresponding ThermalZone object
> > is present in the \_SB scope, the presence of the thermal zone parent
> > won't provide any additional information.
>
> To the device core it will, as the platform device will need to be suspended
> after the thermal zone device has been suspended, among other things.

Let's set suspend aside for now, I think I've explained my viewpoint
on this enough elsewhere.

> > Unfortunately, the language in the specification isn't particularly
> > helpful here: "Thermal zone objects should appear in the namespace
> > under the portion of the system that comprises the thermal zone. For
> > example, a thermal zone that is isolated to a docking station should
> > be defined within the scope of the docking station device."  To me
> > "the portion of the system" is not too meaningful unless it is just
> > one device without children.  That's why _TZD has been added AFAICS.
>
> I think you are confusing the parent device of the ThermalZone ACPI device
> with the parent device of the struct thermal_zone_device.

No, I'm not.

> I begin to wonder if mentioning the ACPI ThermalZone device together with the
> struct thermal_zone_device was a bad idea on my side xd.

Maybe.

> >>>>         thermal: core: Allow setting the parent device of thermal zone devices
> >>> I'm not sure if this is a good idea, at least until it is clear what
> >>> the role of a thermal zone parent device should be.
> >> Take a look at my explanation with the Intel Wifi driver.
> > I did and I think that you want the parent to be a device somehow
> > associated with the thermal zone, but how exactly?  What should that
> > be in the Wifi driver case, the PCI device or something else?
> >
> > And what if the thermal zone affects multiple devices?  Which of them
> > (if any) would be its parent?  And would it be consistent with the
> > ACPI case described above?
> >
> > All of that needs consideration IMV.
>
> I agree, but there is a difference between "this struct thermal_zone_device depends on
> device X to be operational" and "this thermal zone affects device X, device Y and device Z".

Yes, there is.

> This patch series exclusively deals with telling the driver core that "this struct thermal_zone_device
> depends on device X to be operational".

Maybe let's take care of cooling devices first and get back to this later?

Powered by blists - more mailing lists