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:
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: Make cross dev link optional by configuration
thermal/drivers/intel_menlow: Make additionnal sysfs information
optional
drivers/acpi/Kconfig | 13 ++++++
drivers/acpi/thermal.c | 57 +++++++++++++++++------
drivers/thermal/intel/Kconfig | 11 +++++
drivers/thermal/intel/intel_menlow.c | 12 +++--
drivers/thermal/intel/intel_pch_thermal.c | 3 +-
drivers/thermal/thermal_core.c | 6 +++
include/linux/thermal.h | 1 +
7 files changed, 84 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
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.
Some drivers are accessing tz->device, that implies they need to 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.
By adding this wrapper, these drivers won't need the thermal zone
device structure definition and are no longer an obstacle to its
relocation to the private thermal core 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
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 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. If it hurts,
we will revert this change.
Signed-off-by: Daniel Lezcano <[email protected]>
---
drivers/acpi/Kconfig | 13 +++++++++
drivers/acpi/thermal.c | 62 ++++++++++++++++++++++++++++--------------
2 files changed, 55 insertions(+), 20 deletions(-)
diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
index ccbeab9500ec..7df4e18f06ef 100644
--- a/drivers/acpi/Kconfig
+++ b/drivers/acpi/Kconfig
@@ -336,6 +336,19 @@ config ACPI_THERMAL
To compile this driver as a module, choose M here:
the module will be called thermal.
+config ACPI_THERMAL_SYSFS_ADDON
+ bool "Enable thermal sysfs addon"
+ depends on ACPI_THERMAL
+ 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 ACPI_PLATFORM_PROFILE
tristate
diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 5763db4528b8..30fe189d04f8 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_ACPI_THERMAL_SYSFS_ADDON
+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
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/Kconfig | 11 +++++++++++
drivers/thermal/intel/intel_menlow.c | 3 +++
2 files changed, 14 insertions(+)
diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
index cb7e7697cf1e..1405d9cd2bab 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 thermal sysfs addon"
+ depends on INTEL_MENLOW
+ def_bool n
+ help
+ Enable sysfs extra information added in the thermal
+ zone. 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..a169d7e4f537 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_INTEL_MENLOW_SYSFS_ADDON))
+ return 0;
+
attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
if (!attr)
return -ENOMEM;
--
2.34.1
Hi Rafael,
I think I addressed your comments from V2.
Is it fine if I merge this series in the thermal/bleeding-branch ?
On 13/04/2023 13:46, Daniel Lezcano 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:
> 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: Make cross dev link optional by configuration
> thermal/drivers/intel_menlow: Make additionnal sysfs information
> optional
>
> drivers/acpi/Kconfig | 13 ++++++
> drivers/acpi/thermal.c | 57 +++++++++++++++++------
> drivers/thermal/intel/Kconfig | 11 +++++
> drivers/thermal/intel/intel_menlow.c | 12 +++--
> drivers/thermal/intel/intel_pch_thermal.c | 3 +-
> drivers/thermal/thermal_core.c | 6 +++
> include/linux/thermal.h | 1 +
> 7 files changed, 84 insertions(+), 19 deletions(-)
>
--
<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 13, 2023 at 1:47 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. If it hurts,
> we will revert this change.
>
> Signed-off-by: Daniel Lezcano <[email protected]>
> ---
> drivers/acpi/Kconfig | 13 +++++++++
> drivers/acpi/thermal.c | 62 ++++++++++++++++++++++++++++--------------
> 2 files changed, 55 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index ccbeab9500ec..7df4e18f06ef 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -336,6 +336,19 @@ config ACPI_THERMAL
> To compile this driver as a module, choose M here:
> the module will be called thermal.
>
> +config ACPI_THERMAL_SYSFS_ADDON
> + bool "Enable thermal sysfs addon"
> + depends on ACPI_THERMAL
> + 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.
> +
I don't think that the Kconfig option is appropriate and the help text
above isn't really helpful.
> config ACPI_PLATFORM_PROFILE
> tristate
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 5763db4528b8..30fe189d04f8 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_ACPI_THERMAL_SYSFS_ADDON
I agree with moving the code in question to separate functions, but I
don't agree with putting it under the Kconfig option.
> +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 18/04/2023 15:38, Rafael J. Wysocki wrote:
> On Thu, Apr 13, 2023 at 1:47 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. If it hurts,
>> we will revert this change.
>>
>> Signed-off-by: Daniel Lezcano <[email protected]>
>> ---
>> drivers/acpi/Kconfig | 13 +++++++++
>> drivers/acpi/thermal.c | 62 ++++++++++++++++++++++++++++--------------
>> 2 files changed, 55 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
>> index ccbeab9500ec..7df4e18f06ef 100644
>> --- a/drivers/acpi/Kconfig
>> +++ b/drivers/acpi/Kconfig
>> @@ -336,6 +336,19 @@ config ACPI_THERMAL
>> To compile this driver as a module, choose M here:
>> the module will be called thermal.
>>
>> +config ACPI_THERMAL_SYSFS_ADDON
>> + bool "Enable thermal sysfs addon"
>> + depends on ACPI_THERMAL
>> + 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.
>> +
>
> I don't think that the Kconfig option is appropriate and the help text
> above isn't really helpful.
I'm sorry, I'm missing something. Don't we want to make these sysfs
extra information optional and disable them by default ?
>> config ACPI_PLATFORM_PROFILE
>> tristate
>>
>> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>> index 5763db4528b8..30fe189d04f8 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_ACPI_THERMAL_SYSFS_ADDON
>
> I agree with moving the code in question to separate functions, but I
> don't agree with putting it under the Kconfig option.
Do you mean use IS_ENABLED check like what is done with Menlow patch 6 ?
[ ... ]
--
<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 13, 2023 at 1:47 PM Daniel Lezcano
<[email protected]> wrote:
>
> 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/Kconfig | 11 +++++++++++
> drivers/thermal/intel/intel_menlow.c | 3 +++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/thermal/intel/Kconfig b/drivers/thermal/intel/Kconfig
> index cb7e7697cf1e..1405d9cd2bab 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 thermal sysfs addon"
> + depends on INTEL_MENLOW
> + def_bool n
> + help
> + Enable sysfs extra information added in the thermal
This should be something like "Enable extra sysfs attributes in the
thermal zone."
> + zone. 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.
The above part of the help text doesn't add much value IMV. It would
be far better to say what the extra attributes are for instead.
> +
> 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..a169d7e4f537 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_INTEL_MENLOW_SYSFS_ADDON))
> + return 0;
IMV it would be cleaner to put all of the relevant code under the
Kconfig option, so it is clear that it goes away when that option is
not set.
I'm not sure what the compiler will do with it in the current form.
> +
> attr = kzalloc(sizeof(struct intel_menlow_attribute), GFP_KERNEL);
> if (!attr)
> return -ENOMEM;
> --
On 18/04/2023 15:47, Rafael J. Wysocki wrote:
[ ... ]
>> I'm sorry, I'm missing something. Don't we want to make these sysfs
>> extra information optional and disable them by default ?
>
> No, I mean no Kconfig option at all for this one at least for now.
Ok, got it
--
<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 18, 2023 at 3:44 PM Daniel Lezcano
<[email protected]> wrote:
>
> On 18/04/2023 15:38, Rafael J. Wysocki wrote:
> > On Thu, Apr 13, 2023 at 1:47 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. If it hurts,
> >> we will revert this change.
> >>
> >> Signed-off-by: Daniel Lezcano <[email protected]>
> >> ---
> >> drivers/acpi/Kconfig | 13 +++++++++
> >> drivers/acpi/thermal.c | 62 ++++++++++++++++++++++++++++--------------
> >> 2 files changed, 55 insertions(+), 20 deletions(-)
> >>
> >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> >> index ccbeab9500ec..7df4e18f06ef 100644
> >> --- a/drivers/acpi/Kconfig
> >> +++ b/drivers/acpi/Kconfig
> >> @@ -336,6 +336,19 @@ config ACPI_THERMAL
> >> To compile this driver as a module, choose M here:
> >> the module will be called thermal.
> >>
> >> +config ACPI_THERMAL_SYSFS_ADDON
> >> + bool "Enable thermal sysfs addon"
> >> + depends on ACPI_THERMAL
> >> + 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.
> >> +
> >
> > I don't think that the Kconfig option is appropriate and the help text
> > above isn't really helpful.
>
> I'm sorry, I'm missing something. Don't we want to make these sysfs
> extra information optional and disable them by default ?
No, I mean no Kconfig option at all for this one at least for now.
On Mon, Apr 17, 2023 at 3:17 PM Daniel Lezcano
<[email protected]> wrote:
>
>
> Hi Rafael,
>
> I think I addressed your comments from V2.
>
> Is it fine if I merge this series in the thermal/bleeding-branch ?
This series is mostly Intel thermal material, so I'd prefer to take it
via thermal-intel when ready.
Thanks!