Re: [PATCH RFC RESEND 0/8] thermal: core: Allow setting the parent device of thermal zone/cooling devices

Am 27.11.25 um 23:14 schrieb Rafael J. Wysocki:

> On Thu, Nov 27, 2025 at 9:29 PM Armin Wolf <[email protected]> wrote:
>> Am 27.11.25 um 19:22 schrieb Rafael J. Wysocki:
>>
>>> On Sat, Nov 22, 2025 at 3:18 PM Armin Wolf <[email protected]> wrote:
>>>> Am 21.11.25 um 21:35 schrieb Rafael J. Wysocki:
>>>>
>>>>> On Thu, Nov 20, 2025 at 4:41 AM Armin Wolf <[email protected]> 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.
>
Agreed.

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