The thermal zone device structure is defined in the exported thermal
header include/linux/thermal.h
Given the definition being public, the structure is exposed to the
external components other than the thermal framework core code. It
results the drivers are tampering the structure internals like taking
the lock or changing the field values.
Obviously that is bad for several reasons as the drivers can hook the
thermal framework behavior and makes very difficult the changes in the
core code as external components depend on it directly.
Moreover, the thermal trip points being reworked, we don't want the
drivers to access the trips array directly in the thermal zone
structure and doing assumptions on how they are organized.
This series provides a second set of changes moving to the thermal
zone device structure self-encapsulation.
The ACPI and the Menlon drivers are using the thermal zone's device
fields to create symlinks and new attributes in the sysfs thermal zone
directory. These changes provide a hopefully temporary wrapper to
access it in order to allow moving forward in the thermal zone device
self-encapsulation and a Kconfig option to disable by default such a
extra sysfs information.
Changelog:
v4:
- Encapsulate extra sysfs information inside a function for
ACPI but remove the Kconfig option
- Encapsulate extra sysfs information inside a function,
create the stubs and put that conditionnal to a Kconfig
option for Menlow
v3:
- Split the Kconfig option to be driver related when disabling
the specific attributes
- Use the thermal zone's device wrapper to write a trace in
the pch intel driver
v2:
- Add the Kconfig option to remove specific attributes
- Add a thermal_zone_device() wrapper to access tz->device
Daniel Lezcano (6):
thermal/core: Encapsulate tz->device field
thermal/drivers/intel_pch_thermal: Use thermal driver device to write
a trace
thermal/drivers/acpi: Use thermal_zone_device()
thermal/drivers/menlow: Use thermal_zone_device()
thermal/drivers/acpi: Move to dedicated function sysfs extra attr
creation
thermal/drivers/intel_menlow: Make additionnal sysfs information
optional
drivers/acpi/thermal.c | 47 +++++++++++++++--------
drivers/thermal/intel/Kconfig | 11 ++++++
drivers/thermal/intel/intel_menlow.c | 18 +++++++--
drivers/thermal/intel/intel_pch_thermal.c | 3 +-
drivers/thermal/thermal_core.c | 6 +++
include/linux/thermal.h | 1 +
6 files changed, 67 insertions(+), 19 deletions(-)
--
2.34.1
In order to get the device associated with the thermal zone, let's use
the wrapper thermal_zone_device() instead of accessing directly the
content of the thermal zone device structure.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/intel/intel_menlow.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
index 5a6ad0552311..d720add918ff 100644
--- a/drivers/thermal/intel/intel_menlow.c
+++ b/drivers/thermal/intel/intel_menlow.c
@@ -422,7 +422,8 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
result = intel_menlow_add_one_attribute("aux0", 0644,
aux0_show, aux0_store,
- &thermal->device, handle);
+ thermal_zone_device(thermal),
+ handle);
if (result)
return AE_ERROR;
@@ -436,7 +437,8 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
result = intel_menlow_add_one_attribute("aux1", 0644,
aux1_show, aux1_store,
- &thermal->device, handle);
+ thermal_zone_device(thermal),
+ handle);
if (result) {
intel_menlow_unregister_sensor();
return AE_ERROR;
@@ -449,7 +451,8 @@ static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
result = intel_menlow_add_one_attribute("bios_enabled", 0444,
bios_enabled_show, NULL,
- &thermal->device, handle);
+ thermal_zone_device(thermal),
+ handle);
if (result) {
intel_menlow_unregister_sensor();
return AE_ERROR;
--
2.34.1
The pch_critical() callback accesses the thermal zone device structure
internals, it dereferences the thermal zone struct device and the 'type'.
Use the available accessors instead of accessing the structure directly.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/intel/intel_pch_thermal.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
index dce50d239357..b3905e34c507 100644
--- a/drivers/thermal/intel/intel_pch_thermal.c
+++ b/drivers/thermal/intel/intel_pch_thermal.c
@@ -127,7 +127,8 @@ static int pch_thermal_get_temp(struct thermal_zone_device *tzd, int *temp)
static void pch_critical(struct thermal_zone_device *tzd)
{
- dev_dbg(&tzd->device, "%s: critical temperature reached\n", tzd->type);
+ dev_dbg(thermal_zone_device(tzd), "%s: critical temperature reached\n",
+ thermal_zone_device_type(tzd));
}
static struct thermal_zone_device_ops tzd_ops = {
--
2.34.1
The ACPI thermal driver creates extra sysfs attributes in its own
directory pointing to the thermal zone it is related to and add a
pointer to the sysfs ACPI thermal device from the thermal zone sysfs
entry.
This is very specific to this ACPI thermal driver, let's encapsulate
the related creation/deletion code to group it inside a function we
can identify later for removal if needed.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/acpi/thermal.c | 52 ++++++++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 5763db4528b8..9a90b1a117cd 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -787,9 +787,34 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = {
.critical = acpi_thermal_zone_device_critical,
};
+static int acpi_thermal_zone_sysfs_add(struct acpi_thermal *tz)
+{
+ struct device *tzdev = thermal_zone_device(tz->thermal_zone);
+ int ret;
+
+ ret = sysfs_create_link(&tz->device->dev.kobj,
+ &tzdev->kobj, "thermal_zone");
+ if (ret)
+ return ret;
+
+ ret = sysfs_create_link(&tzdev->kobj,
+ &tz->device->dev.kobj, "device");
+ if (ret)
+ sysfs_remove_link(&tz->device->dev.kobj, "thermal_zone");
+
+ return ret;
+}
+
+static void acpi_thermal_zone_sysfs_remove(struct acpi_thermal *tz)
+{
+ struct device *tzdev = thermal_zone_device(tz->thermal_zone);
+
+ sysfs_remove_link(&tz->device->dev.kobj, "thermal_zone");
+ sysfs_remove_link(&tzdev->kobj, "device");
+}
+
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
- struct device *tzdev;
int trips = 0;
int result;
acpi_status status;
@@ -821,23 +846,15 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
if (IS_ERR(tz->thermal_zone))
return -ENODEV;
- tzdev = thermal_zone_device(tz->thermal_zone);
-
- result = sysfs_create_link(&tz->device->dev.kobj,
- &tzdev->kobj, "thermal_zone");
+ result = acpi_thermal_zone_sysfs_add(tz);
if (result)
goto unregister_tzd;
-
- result = sysfs_create_link(&tzdev->kobj,
- &tz->device->dev.kobj, "device");
- if (result)
- goto remove_tz_link;
-
+
status = acpi_bus_attach_private_data(tz->device->handle,
tz->thermal_zone);
if (ACPI_FAILURE(status)) {
result = -ENODEV;
- goto remove_dev_link;
+ goto remove_links;
}
result = thermal_zone_device_enable(tz->thermal_zone);
@@ -851,10 +868,8 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
acpi_bus_detach:
acpi_bus_detach_private_data(tz->device->handle);
-remove_dev_link:
- sysfs_remove_link(&tzdev->kobj, "device");
-remove_tz_link:
- sysfs_remove_link(&tz->device->dev.kobj, "thermal_zone");
+remove_links:
+ acpi_thermal_zone_sysfs_remove(tz);
unregister_tzd:
thermal_zone_device_unregister(tz->thermal_zone);
@@ -863,10 +878,7 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
static void acpi_thermal_unregister_thermal_zone(struct acpi_thermal *tz)
{
- struct device *tzdev = thermal_zone_device(tz->thermal_zone);
-
- sysfs_remove_link(&tz->device->dev.kobj, "thermal_zone");
- sysfs_remove_link(&tzdev->kobj, "device");
+ acpi_thermal_zone_sysfs_remove(tz);
thermal_zone_device_unregister(tz->thermal_zone);
tz->thermal_zone = NULL;
acpi_bus_detach_private_data(tz->device->handle);
--
2.34.1
The Menlon thermal driver creates auxiliary trip points in the thermal
zone sysfs directory. It is specific to Menlon. Actually these trip
points could be generalized with the generic trip points in the future.
Let's make the code optional and disable it by default so we have a
consistency with the attributes in the thermal zone sysfs
directories. If that hurts we will enable by default this option
instead of disabling it.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/intel/Kconfig | 11 +++++++++++
drivers/thermal/intel/intel_menlow.c | 9 +++++++++
2 files changed, 20 insertions(+)
diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
index cb7e7697cf1e..ef7ffe6b56a0 100644
--- a/drivers/thermal/intel/Kconfig
+++ b/drivers/thermal/intel/Kconfig
@@ -112,6 +112,17 @@ config INTEL_MENLOW
If unsure, say N.
+config INTEL_MENLOW_SYSFS_ADDON
+ bool "Enable extra sysfs attributes in the thermal zone"
+ depends on INTEL_MENLOW
+ def_bool n
+ help
+ Create auxiliary trip points in the thermal zone sysfs
+ directory. This is specific to this driver. By default those
+ are disabled and are candidate for removal, if you need these
+ information anyway, enable the option or upgrade the
+ userspace program using them.
+
config INTEL_HFI_THERMAL
bool "Intel Hardware Feedback Interface"
depends on NET
diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
index d720add918ff..605983be516c 100644
--- a/drivers/thermal/intel/intel_menlow.c
+++ b/drivers/thermal/intel/intel_menlow.c
@@ -367,6 +367,7 @@ static ssize_t bios_enabled_show(struct device *dev,
return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
}
+#ifdef CONFIG_INTEL_MENLOW_SYSFS_ADDON
static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
void *store, struct device *dev,
acpi_handle handle)
@@ -398,6 +399,14 @@ static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
return 0;
}
+#else
+static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
+ void *store, struct device *dev,
+ acpi_handle handle)
+{
+ return 0;
+}
+#endif
static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
void *context, void **rv)
--
2.34.1
On Wed, Apr 19, 2023 at 10:33 AM Daniel Lezcano
<[email protected]> wrote:
>
> The Menlon thermal driver creates auxiliary trip points in the thermal
> zone sysfs directory. It is specific to Menlon. Actually these trip
> points could be generalized with the generic trip points in the future.
>
> Let's make the code optional and disable it by default so we have a
> consistency with the attributes in the thermal zone sysfs
> directories. If that hurts we will enable by default this option
> instead of disabling it.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/intel/Kconfig | 11 +++++++++++
> drivers/thermal/intel/intel_menlow.c | 9 +++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> index cb7e7697cf1e..ef7ffe6b56a0 100644
> --- a/drivers/thermal/intel/Kconfig
> +++ b/drivers/thermal/intel/Kconfig
> @@ -112,6 +112,17 @@ config INTEL_MENLOW
>
> If unsure, say N.
>
> +config INTEL_MENLOW_SYSFS_ADDON
> + bool "Enable extra sysfs attributes in the thermal zone"
> + depends on INTEL_MENLOW
> + def_bool n
> + help
> + Create auxiliary trip points in the thermal zone sysfs
> + directory. This is specific to this driver. By default those
> + are disabled and are candidate for removal, if you need these
> + information anyway, enable the option or upgrade the
> + userspace program using them.
> +
> config INTEL_HFI_THERMAL
> bool "Intel Hardware Feedback Interface"
> depends on NET
> diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
> index d720add918ff..605983be516c 100644
> --- a/drivers/thermal/intel/intel_menlow.c
> +++ b/drivers/thermal/intel/intel_menlow.c
> @@ -367,6 +367,7 @@ static ssize_t bios_enabled_show(struct device *dev,
> return sprintf(buf, "%s\n", bios_enabled ? "enabled" : "disabled");
> }
>
> +#ifdef CONFIG_INTEL_MENLOW_SYSFS_ADDON
> static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
> void *store, struct device *dev,
> acpi_handle handle)
> @@ -398,6 +399,14 @@ static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
>
> return 0;
> }
> +#else
> +static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
> + void *store, struct device *dev,
> + acpi_handle handle)
> +{
> + return 0;
> +}
After looking at it once more, I think that this driver isn't really
functional without the additional sysfs attributes, so if
CONFIG_INTEL_MENLOW_SYSFS_ADDON is set, the driver effectively becomes
dead code.
That's rather unfortunate and I'm not sure how to deal with it ATM.
I can queue up the rest of the series for 6.4-rc1 (in which case it
will be pushed in the second half of the merge window), but this
particular patch requires more thought IMV.
> +#endif
>
> static acpi_status intel_menlow_register_sensor(acpi_handle handle, u32 lvl,
> void *context, void **rv)
> --
On 20/04/2023 19:24, Rafael J. Wysocki wrote:
[ ... ]
> After looking at it once more, I think that this driver isn't really
> functional without the additional sysfs attributes, so if
> CONFIG_INTEL_MENLOW_SYSFS_ADDON is set, the driver effectively becomes
> dead code.
>
> That's rather unfortunate and I'm not sure how to deal with it ATM.
>
> I can queue up the rest of the series for 6.4-rc1 (in which case it
> will be pushed in the second half of the merge window), but this
> particular patch requires more thought IMV.
I'm fine if you drop this one from the series.
Thanks for looking at it more deeply
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
On Thu, Apr 20, 2023 at 11:40 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 20/04/2023 19:24, Rafael J. Wysocki wrote:
>
> [ ... ]
>
> > After looking at it once more, I think that this driver isn't really
> > functional without the additional sysfs attributes, so if
> > CONFIG_INTEL_MENLOW_SYSFS_ADDON is set, the driver effectively becomes
> > dead code.
> >
> > That's rather unfortunate and I'm not sure how to deal with it ATM.
> >
> > I can queue up the rest of the series for 6.4-rc1 (in which case it
> > will be pushed in the second half of the merge window), but this
> > particular patch requires more thought IMV.
>
> I'm fine if you drop this one from the series.
>
> Thanks for looking at it more deeply
No problem.
After some private conversation with Srinivas regarding this driver,
I've just posted a patch to get rid of it entirely and so I'm going to
skip all of the patches touching the Menlow driver in this series.
On Wed, Apr 19, 2023 at 10:33 AM Daniel Lezcano
<[email protected]> wrote:
>
> The thermal zone device structure is defined in the exported thermal
> header include/linux/thermal.h
>
> Given the definition being public, the structure is exposed to the
> external components other than the thermal framework core code. It
> results the drivers are tampering the structure internals like taking
> the lock or changing the field values.
>
> Obviously that is bad for several reasons as the drivers can hook the
> thermal framework behavior and makes very difficult the changes in the
> core code as external components depend on it directly.
>
> Moreover, the thermal trip points being reworked, we don't want the
> drivers to access the trips array directly in the thermal zone
> structure and doing assumptions on how they are organized.
>
> This series provides a second set of changes moving to the thermal
> zone device structure self-encapsulation.
>
> The ACPI and the Menlon drivers are using the thermal zone's device
> fields to create symlinks and new attributes in the sysfs thermal zone
> directory. These changes provide a hopefully temporary wrapper to
> access it in order to allow moving forward in the thermal zone device
> self-encapsulation and a Kconfig option to disable by default such a
> extra sysfs information.
>
> Changelog:
> v4:
> - Encapsulate extra sysfs information inside a function for
> ACPI but remove the Kconfig option
> - Encapsulate extra sysfs information inside a function,
> create the stubs and put that conditionnal to a Kconfig
> option for Menlow
> v3:
> - Split the Kconfig option to be driver related when disabling
> the specific attributes
> - Use the thermal zone's device wrapper to write a trace in
> the pch intel driver
> v2:
> - Add the Kconfig option to remove specific attributes
> - Add a thermal_zone_device() wrapper to access tz->device
>
> Daniel Lezcano (6):
> thermal/core: Encapsulate tz->device field
> thermal/drivers/intel_pch_thermal: Use thermal driver device to write
> a trace
> thermal/drivers/acpi: Use thermal_zone_device()
> thermal/drivers/menlow: Use thermal_zone_device()
> thermal/drivers/acpi: Move to dedicated function sysfs extra attr
> creation
> thermal/drivers/intel_menlow: Make additionnal sysfs information
> optional
>
> drivers/acpi/thermal.c | 47 +++++++++++++++--------
> drivers/thermal/intel/Kconfig | 11 ++++++
> drivers/thermal/intel/intel_menlow.c | 18 +++++++--
> drivers/thermal/intel/intel_pch_thermal.c | 3 +-
> drivers/thermal/thermal_core.c | 6 +++
> include/linux/thermal.h | 1 +
> 6 files changed, 67 insertions(+), 19 deletions(-)
>
> --
Patches [4/6] and [6/6] were superseded by the Menlow driver removal.
I've applied the rest as 6.4-rc material, with some subject
adjustments and after removing some trailing white space in a few
places.
Thanks!
On 27/04/2023 19:23, Rafael J. Wysocki wrote:
> On Wed, Apr 19, 2023 at 10:33 AM Daniel Lezcano
> <[email protected]> wrote:
>>
>> The thermal zone device structure is defined in the exported thermal
>> header include/linux/thermal.h
>>
>> Given the definition being public, the structure is exposed to the
>> external components other than the thermal framework core code. It
>> results the drivers are tampering the structure internals like taking
>> the lock or changing the field values.
>>
>> Obviously that is bad for several reasons as the drivers can hook the
>> thermal framework behavior and makes very difficult the changes in the
>> core code as external components depend on it directly.
>>
>> Moreover, the thermal trip points being reworked, we don't want the
>> drivers to access the trips array directly in the thermal zone
>> structure and doing assumptions on how they are organized.
>>
>> This series provides a second set of changes moving to the thermal
>> zone device structure self-encapsulation.
>>
>> The ACPI and the Menlon drivers are using the thermal zone's device
>> fields to create symlinks and new attributes in the sysfs thermal zone
>> directory. These changes provide a hopefully temporary wrapper to
>> access it in order to allow moving forward in the thermal zone device
>> self-encapsulation and a Kconfig option to disable by default such a
>> extra sysfs information.
>>
[ ... ]
> Patches [4/6] and [6/6] were superseded by the Menlow driver removal.
>
> I've applied the rest as 6.4-rc material, with some subject
> adjustments and after removing some trailing white space in a few
> places.
Thanks!
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog