On Thu, Apr 13, 2023 at 1:47 PM Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> 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 <daniel.lezcano@xxxxxxxxxx>
> ---
> 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
>
