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:
v2:
- Add the Kconfig option to remove specific attributes
- Add a thermal_zone_device() wrapper to access tz->device
Daniel Lezcano (7):
thermal/drivers/intel_pch_thermal: Use thermal driver device to write
a trace
thermal/core: Encapsulate tz->device field
thermal/drivers/acpi: Use thermal_zone_device()
thermal/drivers/menlow: Use thermal_zone_device()
thermal/core: Prepare sanitizing thermal class sysfs content
thermal/drivers/acpi: Make cross dev link optional by configuration
thermal/drivers/intel_menlow: Make additionnal sysfs information
optional
drivers/acpi/thermal.c | 57 +++++++++++++++++------
drivers/thermal/Kconfig | 12 +++++
drivers/thermal/intel/intel_menlow.c | 12 +++--
drivers/thermal/intel/intel_pch_thermal.c | 5 +-
drivers/thermal/thermal_core.c | 6 +++
include/linux/thermal.h | 1 +
6 files changed, 74 insertions(+), 19 deletions(-)
--
2.34.1
The pch_critical() callback accesses the thermal zone device structure
internals, it dereferences the thermal zone struct device and the 'type'.
For the former, the driver related device should be use instead and
for the latter an accessor already exists. Use them instead of
accessing the internals.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/intel/intel_pch_thermal.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
index dce50d239357..0de46057db2a 100644
--- a/drivers/thermal/intel/intel_pch_thermal.c
+++ b/drivers/thermal/intel/intel_pch_thermal.c
@@ -127,7 +127,10 @@ 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);
+ struct pch_thermal_device *ptd = thermal_zone_device_priv(tzd);
+
+ dev_dbg(&ptd->pdev->dev, "%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 a link in the thermal zone device
sysfs directory pointing to the device sysfs directory. At the same
time, it creates a back pointer link from the device to the thermal
zone device sysfs directory.
From a generic perspective, having a device pointer in the sysfs
thermal zone directory may make sense. But the opposite is not true as
the same driver can be related to multiple thermal zones.
The usage of these information is very specific to ACPI and it is
questionable if they are really needed.
Let's make the code optional and disable it by default.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/acpi/thermal.c | 62 ++++++++++++++++++++++++++++--------------
1 file changed, 42 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 5763db4528b8..70f1d28810f2 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -787,9 +787,44 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = {
.critical = acpi_thermal_zone_device_critical,
};
+#ifdef CONFIG_THERMAL_SYSFS_OBSOLETE_SINGULARITY
+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");
+}
+#else
+static inline int acpi_thermal_zone_sysfs_add(struct acpi_thermal *tz)
+{
+ return 0;
+}
+static inline void acpi_thermal_zone_sysfs_remove(struct acpi_thermal *tz)
+{
+}
+#endif
+
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
- struct device *tzdev;
int trips = 0;
int result;
acpi_status status;
@@ -821,23 +856,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 +878,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 +888,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
There are still some drivers needing to play with the thermal zone
device internals. That is not the best but until we can figure out if
the information is really needed, let's encapsulate the field used in
the thermal zone device structure, so we can move forward relocating
the thermal zone device structure definition in the thermal framework
private headers.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/thermal_core.c | 6 ++++++
include/linux/thermal.h | 1 +
2 files changed, 7 insertions(+)
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index c5025aca22ee..842f678c1c3e 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -1398,6 +1398,12 @@ int thermal_zone_device_id(struct thermal_zone_device *tzd)
}
EXPORT_SYMBOL_GPL(thermal_zone_device_id);
+struct device *thermal_zone_device(struct thermal_zone_device *tzd)
+{
+ return &tzd->device;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_device);
+
/**
* thermal_zone_device_unregister - removes the registered thermal zone device
* @tz: the thermal zone device to remove
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index 82ddb32f9876..87837094d549 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -313,6 +313,7 @@ thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int
void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
int thermal_zone_device_id(struct thermal_zone_device *tzd);
+struct device *thermal_zone_device(struct thermal_zone_device *tzd);
int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
struct thermal_cooling_device *,
--
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/acpi/thermal.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 255efa73ed70..5763db4528b8 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -789,6 +789,7 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = {
static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
{
+ struct device *tzdev;
int trips = 0;
int result;
acpi_status status;
@@ -820,12 +821,14 @@ 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,
- &tz->thermal_zone->device.kobj, "thermal_zone");
+ &tzdev->kobj, "thermal_zone");
if (result)
goto unregister_tzd;
- result = sysfs_create_link(&tz->thermal_zone->device.kobj,
+ result = sysfs_create_link(&tzdev->kobj,
&tz->device->dev.kobj, "device");
if (result)
goto remove_tz_link;
@@ -849,7 +852,7 @@ 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(&tz->thermal_zone->device.kobj, "device");
+ sysfs_remove_link(&tzdev->kobj, "device");
remove_tz_link:
sysfs_remove_link(&tz->device->dev.kobj, "thermal_zone");
unregister_tzd:
@@ -860,8 +863,10 @@ 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(&tz->thermal_zone->device.kobj, "device");
+ sysfs_remove_link(&tzdev->kobj, "device");
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 specific files in the thermal zone
sysfs class. It is specific to Menlon and these entries look debug
code. It is probable these are not needed.
Let's make the code optional and disable it by default.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/intel/intel_menlow.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/thermal/intel/intel_menlow.c b/drivers/thermal/intel/intel_menlow.c
index d720add918ff..d46dacea1b4d 100644
--- a/drivers/thermal/intel/intel_menlow.c
+++ b/drivers/thermal/intel/intel_menlow.c
@@ -374,6 +374,9 @@ static int intel_menlow_add_one_attribute(char *name, umode_t mode, void *show,
struct intel_menlow_attribute *attr;
int result;
+ if (!IS_ENABLED(CONFIG_THERMAL_SYSFS_OBSOLETE_SINGULARITY))
+ return 0;
+
attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
if (!attr)
return -ENOMEM;
--
2.34.1
Some drivers are accessing the thermal zone device structure to create
specific entries in /sys/class/thermal regardless the documentation.
It is questionable as the specific information should be in the
driver's sysfs directory, not the framework it is dealing with.
It has been long time these specific attributes were added in the
thermal sysfs directory and are limited to the ACPI thermal driver and
the Menlon driver.
It is probable those are not really needed, so in order to figure out
if that is the case, let's create a default option disabling the
attribute in order to prepare a definitive removal.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/thermal/Kconfig | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
index 4cd7ab707315..cca4e5cf6f30 100644
--- a/drivers/thermal/Kconfig
+++ b/drivers/thermal/Kconfig
@@ -33,6 +33,18 @@ config THERMAL_STATISTICS
If in doubt, say N.
+config THERMAL_SYSFS_OBSOLETE_SINGULARITY
+ bool "Enable obsolete and undocumented sysfs extra information"
+ def_bool n
+ help
+ Enable sysfs extra information added in the thermal zone and
+ the driver specific sysfs directories. That could be a link
+ to the associated thermal zone as well as a link pointing to
+ the device from the thermal zone. 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 THERMAL_EMERGENCY_POWEROFF_DELAY_MS
int "Emergency poweroff delay in milli-seconds"
default 0
--
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
On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
<[email protected]> wrote:
>
> The pch_critical() callback accesses the thermal zone device structure
> internals, it dereferences the thermal zone struct device and the 'type'.
>
> For the former, the driver related device should be use instead and
> for the latter an accessor already exists. Use them instead of
> accessing the internals.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/intel/intel_pch_thermal.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
> index dce50d239357..0de46057db2a 100644
> --- a/drivers/thermal/intel/intel_pch_thermal.c
> +++ b/drivers/thermal/intel/intel_pch_thermal.c
> @@ -127,7 +127,10 @@ 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);
> + struct pch_thermal_device *ptd = thermal_zone_device_priv(tzd);
> +
> + dev_dbg(&ptd->pdev->dev, "%s: critical temperature reached\n",
> + thermal_zone_device_type(tzd));
No, this just makes the code more complex than it is and the only
reason seems to be "cleanliness".
Something like
thermal_zone_dbg(tzd, "critical temperature reached\n");
would work, the above doesn't. Sorry.
> }
>
> static struct thermal_zone_device_ops tzd_ops = {
> --
On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
<[email protected]> wrote:
>
> There are still some drivers needing to play with the thermal zone
> device internals. That is not the best but until we can figure out if
> the information is really needed, let's encapsulate the field used in
> the thermal zone device structure, so we can move forward relocating
> the thermal zone device structure definition in the thermal framework
> private headers.
I'm not really sure why this is needed, so please explain.
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/thermal_core.c | 6 ++++++
> include/linux/thermal.h | 1 +
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> index c5025aca22ee..842f678c1c3e 100644
> --- a/drivers/thermal/thermal_core.c
> +++ b/drivers/thermal/thermal_core.c
> @@ -1398,6 +1398,12 @@ int thermal_zone_device_id(struct thermal_zone_device *tzd)
> }
> EXPORT_SYMBOL_GPL(thermal_zone_device_id);
>
> +struct device *thermal_zone_device(struct thermal_zone_device *tzd)
> +{
> + return &tzd->device;
> +}
> +EXPORT_SYMBOL_GPL(thermal_zone_device);
> +
> /**
> * thermal_zone_device_unregister - removes the registered thermal zone device
> * @tz: the thermal zone device to remove
> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> index 82ddb32f9876..87837094d549 100644
> --- a/include/linux/thermal.h
> +++ b/include/linux/thermal.h
> @@ -313,6 +313,7 @@ thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int
> void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
> const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
> int thermal_zone_device_id(struct thermal_zone_device *tzd);
> +struct device *thermal_zone_device(struct thermal_zone_device *tzd);
>
> int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
> struct thermal_cooling_device *,
> --
> 2.34.1
>
On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
<[email protected]> wrote:
>
> Some drivers are accessing the thermal zone device structure to create
> specific entries in /sys/class/thermal regardless the documentation.
>
> It is questionable as the specific information should be in the
> driver's sysfs directory, not the framework it is dealing with.
>
> It has been long time these specific attributes were added in the
> thermal sysfs directory and are limited to the ACPI thermal driver and
> the Menlon driver.
>
> It is probable those are not really needed, so in order to figure out
> if that is the case, let's create a default option disabling the
> attribute in order to prepare a definitive removal.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/thermal/Kconfig | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig
> index 4cd7ab707315..cca4e5cf6f30 100644
> --- a/drivers/thermal/Kconfig
> +++ b/drivers/thermal/Kconfig
> @@ -33,6 +33,18 @@ config THERMAL_STATISTICS
>
> If in doubt, say N.
>
> +config THERMAL_SYSFS_OBSOLETE_SINGULARITY
> + bool "Enable obsolete and undocumented sysfs extra information"
I was talking about making an extra Kconfig option in the Menlow
driver to make the extra sysfs stuff depend on.
Throwing ACPI in the same bucket is a non-starter.
> + def_bool n
> + help
> + Enable sysfs extra information added in the thermal zone and
> + the driver specific sysfs directories. That could be a link
> + to the associated thermal zone as well as a link pointing to
> + the device from the thermal zone. 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 THERMAL_EMERGENCY_POWEROFF_DELAY_MS
> int "Emergency poweroff delay in milli-seconds"
> default 0
> --
> 2.34.1
>
On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
<[email protected]> wrote:
>
> The ACPI thermal driver creates a link in the thermal zone device
> sysfs directory pointing to the device sysfs directory. At the same
> time, it creates a back pointer link from the device to the thermal
> zone device sysfs directory.
>
> From a generic perspective, having a device pointer in the sysfs
> thermal zone directory may make sense. But the opposite is not true as
> the same driver can be related to multiple thermal zones.
>
> The usage of these information is very specific to ACPI and it is
> questionable if they are really needed.
>
> Let's make the code optional and disable it by default.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/acpi/thermal.c | 62 ++++++++++++++++++++++++++++--------------
> 1 file changed, 42 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 5763db4528b8..70f1d28810f2 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -787,9 +787,44 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = {
> .critical = acpi_thermal_zone_device_critical,
> };
>
> +#ifdef CONFIG_THERMAL_SYSFS_OBSOLETE_SINGULARITY
It is OK to move the code to the separate functions below, but it is
not OK to make it depend on the Kconfig option above.
The extra sysfs things were added in different drivers for different
reasons. Making them all depend on one Kconfig option is just wrong.
> +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");
> +}
> +#else
> +static inline int acpi_thermal_zone_sysfs_add(struct acpi_thermal *tz)
> +{
> + return 0;
> +}
> +static inline void acpi_thermal_zone_sysfs_remove(struct acpi_thermal *tz)
> +{
> +}
> +#endif
> +
> static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
> {
> - struct device *tzdev;
> int trips = 0;
> int result;
> acpi_status status;
> @@ -821,23 +856,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 +878,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 +888,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
>
On 11/04/2023 20:16, Rafael J. Wysocki wrote:
> On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> The pch_critical() callback accesses the thermal zone device structure
>> internals, it dereferences the thermal zone struct device and the 'type'.
>>
>> For the former, the driver related device should be use instead and
>> for the latter an accessor already exists. Use them instead of
>> accessing the internals.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/intel/intel_pch_thermal.c | 5 ++++-
>> 1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
>> index dce50d239357..0de46057db2a 100644
>> --- a/drivers/thermal/intel/intel_pch_thermal.c
>> +++ b/drivers/thermal/intel/intel_pch_thermal.c
>> @@ -127,7 +127,10 @@ 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);
>> + struct pch_thermal_device *ptd = thermal_zone_device_priv(tzd);
>> +
>> + dev_dbg(&ptd->pdev->dev, "%s: critical temperature reached\n",
>> + thermal_zone_device_type(tzd));
>
> No, this just makes the code more complex than it is and the only
> reason seems to be "cleanliness".
>
> Something like
>
> thermal_zone_dbg(tzd, "critical temperature reached\n");
>
> would work, the above doesn't. Sorry.
Why not add a trace directly in handle_critical_trips() in
thermal_core.c and remove this one ?
--
<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 11/04/2023 20:19, Rafael J. Wysocki wrote:
> On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> There are still some drivers needing to play with the thermal zone
>> device internals. That is not the best but until we can figure out if
>> the information is really needed, let's encapsulate the field used in
>> the thermal zone device structure, so we can move forward relocating
>> the thermal zone device structure definition in the thermal framework
>> private headers.
>
> I'm not really sure why this is needed, so please explain.
Some drivers are accessing tz->device, that implies they have the
knowledge of the thermal_zone_device structure but we want to
self-encapsulate this structure and reduce the scope of the structure to
the thermal core only.
The ACPI and the Menlon drivers are the drivers accessing tz->device.
By adding this wrapper, these drivers do no longer need the thermal zone
device structure definition.
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/thermal/thermal_core.c | 6 ++++++
>> include/linux/thermal.h | 1 +
>> 2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>> index c5025aca22ee..842f678c1c3e 100644
>> --- a/drivers/thermal/thermal_core.c
>> +++ b/drivers/thermal/thermal_core.c
>> @@ -1398,6 +1398,12 @@ int thermal_zone_device_id(struct thermal_zone_device *tzd)
>> }
>> EXPORT_SYMBOL_GPL(thermal_zone_device_id);
>>
>> +struct device *thermal_zone_device(struct thermal_zone_device *tzd)
>> +{
>> + return &tzd->device;
>> +}
>> +EXPORT_SYMBOL_GPL(thermal_zone_device);
>> +
>> /**
>> * thermal_zone_device_unregister - removes the registered thermal zone device
>> * @tz: the thermal zone device to remove
>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>> index 82ddb32f9876..87837094d549 100644
>> --- a/include/linux/thermal.h
>> +++ b/include/linux/thermal.h
>> @@ -313,6 +313,7 @@ thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int
>> void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
>> const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
>> int thermal_zone_device_id(struct thermal_zone_device *tzd);
>> +struct device *thermal_zone_device(struct thermal_zone_device *tzd);
>>
>> int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
>> struct thermal_cooling_device *,
>> --
>> 2.34.1
>>
--
<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 11/04/2023 20:26, Rafael J. Wysocki wrote:
> On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> The ACPI thermal driver creates a link in the thermal zone device
>> sysfs directory pointing to the device sysfs directory. At the same
>> time, it creates a back pointer link from the device to the thermal
>> zone device sysfs directory.
>>
>> From a generic perspective, having a device pointer in the sysfs
>> thermal zone directory may make sense. But the opposite is not true as
>> the same driver can be related to multiple thermal zones.
>>
>> The usage of these information is very specific to ACPI and it is
>> questionable if they are really needed.
>>
>> Let's make the code optional and disable it by default.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/acpi/thermal.c | 62 ++++++++++++++++++++++++++++--------------
>> 1 file changed, 42 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>> index 5763db4528b8..70f1d28810f2 100644
>> --- a/drivers/acpi/thermal.c
>> +++ b/drivers/acpi/thermal.c
>> @@ -787,9 +787,44 @@ static struct thermal_zone_device_ops acpi_thermal_zone_ops = {
>> .critical = acpi_thermal_zone_device_critical,
>> };
>>
>> +#ifdef CONFIG_THERMAL_SYSFS_OBSOLETE_SINGULARITY
>
> It is OK to move the code to the separate functions below, but it is
> not OK to make it depend on the Kconfig option above.
>
> The extra sysfs things were added in different drivers for different
> reasons. Making them all depend on one Kconfig option is just wrong.
Ok, I'll do the changes accordingly.
Thanks for reviewing the series
[ ... ]
--
<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 Tue, Apr 11, 2023 at 10:10 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 11/04/2023 20:16, Rafael J. Wysocki wrote:
> > On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> The pch_critical() callback accesses the thermal zone device structure
> >> internals, it dereferences the thermal zone struct device and the 'type'.
> >>
> >> For the former, the driver related device should be use instead and
> >> for the latter an accessor already exists. Use them instead of
> >> accessing the internals.
> >>
> >> Signed-off-by: Daniel Lezcano <[email protected]>
> >> ---
> >> drivers/thermal/intel/intel_pch_thermal.c | 5 ++++-
> >> 1 file changed, 4 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/thermal/intel/intel_pch_thermal.c b/drivers/thermal/intel/intel_pch_thermal.c
> >> index dce50d239357..0de46057db2a 100644
> >> --- a/drivers/thermal/intel/intel_pch_thermal.c
> >> +++ b/drivers/thermal/intel/intel_pch_thermal.c
> >> @@ -127,7 +127,10 @@ 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);
> >> + struct pch_thermal_device *ptd = thermal_zone_device_priv(tzd);
> >> +
> >> + dev_dbg(&ptd->pdev->dev, "%s: critical temperature reached\n",
> >> + thermal_zone_device_type(tzd));
> >
> > No, this just makes the code more complex than it is and the only
> > reason seems to be "cleanliness".
> >
> > Something like
> >
> > thermal_zone_dbg(tzd, "critical temperature reached\n");
> >
> > would work, the above doesn't. Sorry.
>
> Why not add a trace directly in handle_critical_trips() in
> thermal_core.c and remove this one ?
That would work too.
On Tue, Apr 11, 2023 at 10:20 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 11/04/2023 20:19, Rafael J. Wysocki wrote:
> > On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
> > <[email protected]> wrote:
> >>
> >> There are still some drivers needing to play with the thermal zone
> >> device internals. That is not the best but until we can figure out if
> >> the information is really needed, let's encapsulate the field used in
> >> the thermal zone device structure, so we can move forward relocating
> >> the thermal zone device structure definition in the thermal framework
> >> private headers.
> >
> > I'm not really sure why this is needed, so please explain.
>
> Some drivers are accessing tz->device, that implies they have the
> knowledge of the thermal_zone_device structure but we want to
> self-encapsulate this structure and reduce the scope of the structure to
> the thermal core only.
>
> The ACPI and the Menlon drivers are the drivers accessing tz->device.
>
> By adding this wrapper, these drivers do no longer need the thermal zone
> device structure definition.
So you want to move the definition of struct thermal_zone_device from
include/linux/thermal.h into a local header in drivers/thermal/ and
make it entirely local to the thermal core IIUC.
Which would be forcing the callers of
thermal_zone_device_register_with_trips() (and friends) to use
pointers to a data type that's not completely defined (from their
perspective), but they would still have access to the trips array
passed to that function.
That doesn't sound particularly consistent and what's the purpose of doing it?
> >> Signed-off-by: Daniel Lezcano <[email protected]>
> >> ---
> >> drivers/thermal/thermal_core.c | 6 ++++++
> >> include/linux/thermal.h | 1 +
> >> 2 files changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
> >> index c5025aca22ee..842f678c1c3e 100644
> >> --- a/drivers/thermal/thermal_core.c
> >> +++ b/drivers/thermal/thermal_core.c
> >> @@ -1398,6 +1398,12 @@ int thermal_zone_device_id(struct thermal_zone_device *tzd)
> >> }
> >> EXPORT_SYMBOL_GPL(thermal_zone_device_id);
> >>
> >> +struct device *thermal_zone_device(struct thermal_zone_device *tzd)
> >> +{
> >> + return &tzd->device;
> >> +}
> >> +EXPORT_SYMBOL_GPL(thermal_zone_device);
> >> +
> >> /**
> >> * thermal_zone_device_unregister - removes the registered thermal zone device
> >> * @tz: the thermal zone device to remove
> >> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
> >> index 82ddb32f9876..87837094d549 100644
> >> --- a/include/linux/thermal.h
> >> +++ b/include/linux/thermal.h
> >> @@ -313,6 +313,7 @@ thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int
> >> void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
> >> const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
> >> int thermal_zone_device_id(struct thermal_zone_device *tzd);
> >> +struct device *thermal_zone_device(struct thermal_zone_device *tzd);
> >>
> >> int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
> >> struct thermal_cooling_device *,
> >> --
> >> 2.34.1
> >>
>
> --
> <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 12/04/2023 20:56, Rafael J. Wysocki wrote:
> On Tue, Apr 11, 2023 at 10:20 PM Daniel Lezcano
> <[email protected]> wrote:
>>
>> On 11/04/2023 20:19, Rafael J. Wysocki wrote:
>>> On Mon, Apr 10, 2023 at 10:53 PM Daniel Lezcano
>>> <[email protected]> wrote:
>>>>
>>>> There are still some drivers needing to play with the thermal zone
>>>> device internals. That is not the best but until we can figure out if
>>>> the information is really needed, let's encapsulate the field used in
>>>> the thermal zone device structure, so we can move forward relocating
>>>> the thermal zone device structure definition in the thermal framework
>>>> private headers.
>>>
>>> I'm not really sure why this is needed, so please explain.
>>
>> Some drivers are accessing tz->device, that implies they have the
>> knowledge of the thermal_zone_device structure but we want to
>> self-encapsulate this structure and reduce the scope of the structure to
>> the thermal core only.
>>
>> The ACPI and the Menlon drivers are the drivers accessing tz->device.
>>
>> By adding this wrapper, these drivers do no longer need the thermal zone
>> device structure definition.
>
> So you want to move the definition of struct thermal_zone_device from
> include/linux/thermal.h into a local header in drivers/thermal/ and
> make it entirely local to the thermal core IIUC.
>
> Which would be forcing the callers of
> thermal_zone_device_register_with_trips() (and friends) to use
> pointers to a data type that's not completely defined (from their
> perspective), but they would still have access to the trips array
> passed to that function.
>
> That doesn't sound particularly consistent and what's the purpose of doing it?
The first thing is prevent drivers tampering with the thermal core
structure internals, so moving the structures from
include/linux/thermal.h to drivers/thermal/thermal_core.h is one step to
this direction.
As you point it, drivers can still do things with the trip arrays passed
as parameter. For that the idea is to do the same as:
https://lore.kernel.org/all/[email protected]/
So the array is an initialization data and it will stay private to the
thermal core code.
In order to update the trip points, we add a
thermal_zone_device_trips_update() function which does a kmemdup,
pointer switch, kfree of the previous trips and a notification.
I addressed that in response to your ACPI series questions:
https://lore.kernel.org/all/[email protected]/
Does it make sense with this additional information ?
>>>> Signed-off-by: Daniel Lezcano <[email protected]>
>>>> ---
>>>> drivers/thermal/thermal_core.c | 6 ++++++
>>>> include/linux/thermal.h | 1 +
>>>> 2 files changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
>>>> index c5025aca22ee..842f678c1c3e 100644
>>>> --- a/drivers/thermal/thermal_core.c
>>>> +++ b/drivers/thermal/thermal_core.c
>>>> @@ -1398,6 +1398,12 @@ int thermal_zone_device_id(struct thermal_zone_device *tzd)
>>>> }
>>>> EXPORT_SYMBOL_GPL(thermal_zone_device_id);
>>>>
>>>> +struct device *thermal_zone_device(struct thermal_zone_device *tzd)
>>>> +{
>>>> + return &tzd->device;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(thermal_zone_device);
>>>> +
>>>> /**
>>>> * thermal_zone_device_unregister - removes the registered thermal zone device
>>>> * @tz: the thermal zone device to remove
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 82ddb32f9876..87837094d549 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -313,6 +313,7 @@ thermal_zone_device_register_with_trips(const char *, struct thermal_trip *, int
>>>> void *thermal_zone_device_priv(struct thermal_zone_device *tzd);
>>>> const char *thermal_zone_device_type(struct thermal_zone_device *tzd);
>>>> int thermal_zone_device_id(struct thermal_zone_device *tzd);
>>>> +struct device *thermal_zone_device(struct thermal_zone_device *tzd);
>>>>
>>>> int thermal_zone_bind_cooling_device(struct thermal_zone_device *, int,
>>>> struct thermal_cooling_device *,
>>>> --
>>>> 2.34.1
>>>>
>>
>> --
>> <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
>>
--
<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