Message-ID: <cf86344b-d9f1-4d3c-9fe9-deeb4ade9304@gmx.de>
Date: Thu, 27 Nov 2025 21:29:15 +0100
From: Armin Wolf <W_Armin@....de>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: 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

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.

> 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).

>> 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.

> 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.

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.

>>>>         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".

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

Thanks,
Armin Wolf

Powered by blists - more mailing lists