[PATCH] acpi: thermal: honor "mode" sysfs file setting

[PATCH] acpi: thermal: honor "mode" sysfs file setting

[Enric Balletbo i Serra]

From: Sameer Nanda <snanda@chromium.org>

Under each thermal zone there is a file called "mode". Writing enabled
or disabled to this file allows a given thermal zone to be enabled or
disabled. Honor writes to this file by enabling or disabling the
polling timers.

With this change, in the acpi_thermal_add path, acpi_thermal_get_info
gets called before acpi_thermal_register_thermal_zone. Since tz_enabled
was getting set to 1 only in acpi_thermal_register_thermal_zone,
acpi_thermal_get_info ended up disabling thermal polling so moved the
setting of tz_enabled to 1 into acpi_thermal_add itself.

After this patch echoing enabled|disabled to "mode" sysfs will start/stop
the polling of the temperature.

Signed-off-by: Sameer Nanda <snanda@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
 drivers/acpi/thermal.c | 28 +++++++++++++++++++++-------
 1 file changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 1d0417b..68ad9fe 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -223,6 +223,17 @@ static int acpi_thermal_get_polling_frequency(struct acpi_thermal *tz)
 	if (!tz)
 		return -EINVAL;
 
+	if (tz->tz_enabled == THERMAL_DEVICE_DISABLED) {
+		tz->polling_frequency = 0;
+		return 0;
+	}
+
+	/* Get default polling frequency [_TZP] (optional) */
+	if (tzp) {
+		tz->polling_frequency = tzp;
+		return 0;
+	}
+
 	status = acpi_evaluate_integer(tz->device->handle, "_TZP", NULL, &tmp);
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
@@ -582,6 +593,14 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
 		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
 			"%s kernel ACPI thermal control\n",
 			tz->tz_enabled ? "Enable" : "Disable"));
+
+		acpi_thermal_get_polling_frequency(tz);
+
+		mutex_lock(&tz->thermal_zone->lock);
+		tz->thermal_zone->polling_delay = tz->polling_frequency * 100;
+		tz->thermal_zone->passive_delay = tz->polling_frequency * 100;
+		mutex_unlock(&tz->thermal_zone->lock);
+
 		acpi_thermal_check(tz);
 	}
 	return 0;
@@ -930,8 +949,6 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
 	if (ACPI_FAILURE(status))
 		return -ENODEV;
 
-	tz->tz_enabled = 1;
-
 	dev_info(&tz->device->dev, "registered as thermal_zone%d\n",
 		 tz->thermal_zone->id);
 	return 0;
@@ -1039,11 +1056,7 @@ static int acpi_thermal_get_info(struct acpi_thermal *tz)
 	if (!result)
 		tz->flags.cooling_mode = 1;
 
-	/* Get default polling frequency [_TZP] (optional) */
-	if (tzp)
-		tz->polling_frequency = tzp;
-	else
-		acpi_thermal_get_polling_frequency(tz);
+	acpi_thermal_get_polling_frequency(tz);
 
 	return 0;
 }
@@ -1088,6 +1101,7 @@ static int acpi_thermal_add(struct acpi_device *device)
 		return -ENOMEM;
 
 	tz->device = device;
+	tz->tz_enabled = 1;
 	strcpy(tz->name, device->pnp.bus_id);
 	strcpy(acpi_device_name(device), ACPI_THERMAL_DEVICE_NAME);
 	strcpy(acpi_device_class(device), ACPI_THERMAL_CLASS);
-- 
2.9.3

Re: [PATCH] acpi: thermal: honor "mode" sysfs file setting

[Rafael J. Wysocki]

On Thursday, June 22, 2017 02:45:42 PM Enric Balletbo i Serra wrote:
> From: Sameer Nanda <snanda@chromium.org>
> 
> Under each thermal zone there is a file called "mode". Writing enabled
> or disabled to this file allows a given thermal zone to be enabled or
> disabled. Honor writes to this file by enabling or disabling the
> polling timers.
> 
> With this change, in the acpi_thermal_add path, acpi_thermal_get_info
> gets called before acpi_thermal_register_thermal_zone. Since tz_enabled
> was getting set to 1 only in acpi_thermal_register_thermal_zone,
> acpi_thermal_get_info ended up disabling thermal polling so moved the
> setting of tz_enabled to 1 into acpi_thermal_add itself.
> 
> After this patch echoing enabled|disabled to "mode" sysfs will start/stop
> the polling of the temperature.
> 
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

Rui, Srinivas, can you please have a look at this one and let me know what you
think?

> ---
>  drivers/acpi/thermal.c | 28 +++++++++++++++++++++-------
>  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 1d0417b..68ad9fe 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -223,6 +223,17 @@ static int acpi_thermal_get_polling_frequency(struct acpi_thermal *tz)
>  	if (!tz)
>  		return -EINVAL;
>  
> +	if (tz->tz_enabled == THERMAL_DEVICE_DISABLED) {
> +		tz->polling_frequency = 0;
> +		return 0;
> +	}
> +
> +	/* Get default polling frequency [_TZP] (optional) */
> +	if (tzp) {
> +		tz->polling_frequency = tzp;
> +		return 0;
> +	}
> +
>  	status = acpi_evaluate_integer(tz->device->handle, "_TZP", NULL, &tmp);
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
> @@ -582,6 +593,14 @@ static int thermal_set_mode(struct thermal_zone_device *thermal,
>  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>  			"%s kernel ACPI thermal control\n",
>  			tz->tz_enabled ? "Enable" : "Disable"));
> +
> +		acpi_thermal_get_polling_frequency(tz);
> +
> +		mutex_lock(&tz->thermal_zone->lock);
> +		tz->thermal_zone->polling_delay = tz->polling_frequency * 100;
> +		tz->thermal_zone->passive_delay = tz->polling_frequency * 100;
> +		mutex_unlock(&tz->thermal_zone->lock);
> +
>  		acpi_thermal_check(tz);
>  	}
>  	return 0;
> @@ -930,8 +949,6 @@ static int acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
>  	if (ACPI_FAILURE(status))
>  		return -ENODEV;
>  
> -	tz->tz_enabled = 1;
> -
>  	dev_info(&tz->device->dev, "registered as thermal_zone%d\n",
>  		 tz->thermal_zone->id);
>  	return 0;
> @@ -1039,11 +1056,7 @@ static int acpi_thermal_get_info(struct acpi_thermal *tz)
>  	if (!result)
>  		tz->flags.cooling_mode = 1;
>  
> -	/* Get default polling frequency [_TZP] (optional) */
> -	if (tzp)
> -		tz->polling_frequency = tzp;
> -	else
> -		acpi_thermal_get_polling_frequency(tz);
> +	acpi_thermal_get_polling_frequency(tz);
>  
>  	return 0;
>  }
> @@ -1088,6 +1101,7 @@ static int acpi_thermal_add(struct acpi_device *device)
>  		return -ENOMEM;
>  
>  	tz->device = device;
> +	tz->tz_enabled = 1;
>  	strcpy(tz->name, device->pnp.bus_id);
>  	strcpy(acpi_device_name(device), ACPI_THERMAL_DEVICE_NAME);
>  	strcpy(acpi_device_class(device), ACPI_THERMAL_CLASS);
> 

Re: [PATCH] acpi: thermal: honor "mode" sysfs file setting

[Zhang Rui]

On Thu, 2017-06-29 at 00:14 +0200, Rafael J. Wysocki wrote:
> On Thursday, June 22, 2017 02:45:42 PM Enric Balletbo i Serra wrote:
> > 
> > From: Sameer Nanda <snanda@chromium.org>
> > 
> > Under each thermal zone there is a file called "mode". Writing
> > enabled
> > or disabled to this file allows a given thermal zone to be enabled
> > or
> > disabled. Honor writes to this file by enabling or disabling the
> > polling timers.
> > 
> > With this change, in the acpi_thermal_add path,
> > acpi_thermal_get_info
> > gets called before acpi_thermal_register_thermal_zone. Since
> > tz_enabled
> > was getting set to 1 only in acpi_thermal_register_thermal_zone,
> > acpi_thermal_get_info ended up disabling thermal polling so moved
> > the
> > setting of tz_enabled to 1 into acpi_thermal_add itself.
> > 
> > After this patch echoing enabled|disabled to "mode" sysfs will
> > start/stop
> > the polling of the temperature.
> > 
I see, so there are three logics to decide the polling frequency

1. returned by _TZP, according to ACPI spec
2. overridden by thermal.tzp module parameter
3. cleared when the thermal zone is disabled and restored when the
thermal zone is re-enabled (missing in current code)

what we are doing in this patch is to introduce the third logic, and
move all the code of 1, 2 and 3 into
acpi_thermal_get_polling_frequency().
To align with this change, tz->tz_enable=1 is moved earlier, before
acpi_thermal_get_polling_frequency() being invoked in
acpi_thermal_add().
right?

> > Signed-off-by: Sameer Nanda <snanda@chromium.org>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
> > >
> Rui, Srinivas, can you please have a look at this one and let me know
> what you
> think?
> 
> > 
> > ---
> >  drivers/acpi/thermal.c | 28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index 1d0417b..68ad9fe 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -223,6 +223,17 @@ static int
> > acpi_thermal_get_polling_frequency(struct acpi_thermal *tz)
> >  	if (!tz)
> >  		return -EINVAL;
> >  
> > +	if (tz->tz_enabled == THERMAL_DEVICE_DISABLED) {
> > +		tz->polling_frequency = 0;
> > +		return 0;
> > +	}
> > +
> > +	/* Get default polling frequency [_TZP] (optional) */
> > +	if (tzp) {
> > +		tz->polling_frequency = tzp;
> > +		return 0;
> > +	}
> > +
> >  	status = acpi_evaluate_integer(tz->device->handle, "_TZP",
> > NULL, &tmp);
> >  	if (ACPI_FAILURE(status))
> >  		return -ENODEV;
> > @@ -582,6 +593,14 @@ static int thermal_set_mode(struct
> > thermal_zone_device *thermal,
> >  		ACPI_DEBUG_PRINT((ACPI_DB_INFO,
> >  			"%s kernel ACPI thermal control\n",
> >  			tz->tz_enabled ? "Enable" : "Disable"));
> > +
> > +		acpi_thermal_get_polling_frequency(tz);
> > +
> > +		mutex_lock(&tz->thermal_zone->lock);
> > +		tz->thermal_zone->polling_delay = tz-
> > >polling_frequency * 100;
> > +		tz->thermal_zone->passive_delay = tz-
> > >polling_frequency * 100;

To me, this policy, "disabling polling when disabling thermal zone,
enabling polling after enabling thermal zone" applies to all the
thermal zones, thus it's better to be implemented in thermal subsystem,
rather than ACPI thermal driver.

BTW, two other comments about the code itself,
1. I don't like the way how tz->polling_delay/tz->passive_delay are
changed here. If we have to do this, better using a thermal API, and we
should invoke monitor_thermal_zone() right after updating these two
fields.
2. passive_delay is set to wrong value here.

thanks,
rui
> > +		mutex_unlock(&tz->thermal_zone->lock);
> > +
> >  		acpi_thermal_check(tz);
> >  	}
> >  	return 0;
> > @@ -930,8 +949,6 @@ static int
> > acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
> >  	if (ACPI_FAILURE(status))
> >  		return -ENODEV;
> >  
> > -	tz->tz_enabled = 1;
> > -
> >  	dev_info(&tz->device->dev, "registered as
> > thermal_zone%d\n",
> >  		 tz->thermal_zone->id);
> >  	return 0;
> > @@ -1039,11 +1056,7 @@ static int acpi_thermal_get_info(struct
> > acpi_thermal *tz)
> >  	if (!result)
> >  		tz->flags.cooling_mode = 1;
> >  
> > -	/* Get default polling frequency [_TZP] (optional) */
> > -	if (tzp)
> > -		tz->polling_frequency = tzp;
> > -	else
> > -		acpi_thermal_get_polling_frequency(tz);
> > +	acpi_thermal_get_polling_frequency(tz);
> >  
> >  	return 0;
> >  }
> > @@ -1088,6 +1101,7 @@ static int acpi_thermal_add(struct
> > acpi_device *device)
> >  		return -ENOMEM;
> >  
> >  	tz->device = device;
> > +	tz->tz_enabled = 1;
> >  	strcpy(tz->name, device->pnp.bus_id);
> >  	strcpy(acpi_device_name(device),
> > ACPI_THERMAL_DEVICE_NAME);
> >  	strcpy(acpi_device_class(device), ACPI_THERMAL_CLASS);
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" 
> in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] acpi: thermal: honor "mode" sysfs file setting

[Enric Balletbo Serra]

Hi Rui,

2017-06-29 7:19 GMT+02:00 Zhang Rui <rui.zhang@intel.com>:
> On Thu, 2017-06-29 at 00:14 +0200, Rafael J. Wysocki wrote:
>> On Thursday, June 22, 2017 02:45:42 PM Enric Balletbo i Serra wrote:
>> >
>> > From: Sameer Nanda <snanda@chromium.org>
>> >
>> > Under each thermal zone there is a file called "mode". Writing
>> > enabled
>> > or disabled to this file allows a given thermal zone to be enabled
>> > or
>> > disabled. Honor writes to this file by enabling or disabling the
>> > polling timers.
>> >
>> > With this change, in the acpi_thermal_add path,
>> > acpi_thermal_get_info
>> > gets called before acpi_thermal_register_thermal_zone. Since
>> > tz_enabled
>> > was getting set to 1 only in acpi_thermal_register_thermal_zone,
>> > acpi_thermal_get_info ended up disabling thermal polling so moved
>> > the
>> > setting of tz_enabled to 1 into acpi_thermal_add itself.
>> >
>> > After this patch echoing enabled|disabled to "mode" sysfs will
>> > start/stop
>> > the polling of the temperature.
>> >
> I see, so there are three logics to decide the polling frequency
>
> 1. returned by _TZP, according to ACPI spec
> 2. overridden by thermal.tzp module parameter
> 3. cleared when the thermal zone is disabled and restored when the
> thermal zone is re-enabled (missing in current code)
>
> what we are doing in this patch is to introduce the third logic, and
> move all the code of 1, 2 and 3 into
> acpi_thermal_get_polling_frequency().
> To align with this change, tz->tz_enable=1 is moved earlier, before
> acpi_thermal_get_polling_frequency() being invoked in
> acpi_thermal_add().
> right?
>

Right.

>> > Signed-off-by: Sameer Nanda <snanda@chromium.org>
>> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
>> > >
>> Rui, Srinivas, can you please have a look at this one and let me know
>> what you
>> think?
>>
>> >
>> > ---
>> >  drivers/acpi/thermal.c | 28 +++++++++++++++++++++-------
>> >  1 file changed, 21 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>> > index 1d0417b..68ad9fe 100644
>> > --- a/drivers/acpi/thermal.c
>> > +++ b/drivers/acpi/thermal.c
>> > @@ -223,6 +223,17 @@ static int
>> > acpi_thermal_get_polling_frequency(struct acpi_thermal *tz)
>> >     if (!tz)
>> >             return -EINVAL;
>> >
>> > +   if (tz->tz_enabled == THERMAL_DEVICE_DISABLED) {
>> > +           tz->polling_frequency = 0;
>> > +           return 0;
>> > +   }
>> > +
>> > +   /* Get default polling frequency [_TZP] (optional) */
>> > +   if (tzp) {
>> > +           tz->polling_frequency = tzp;
>> > +           return 0;
>> > +   }
>> > +
>> >     status = acpi_evaluate_integer(tz->device->handle, "_TZP",
>> > NULL, &tmp);
>> >     if (ACPI_FAILURE(status))
>> >             return -ENODEV;
>> > @@ -582,6 +593,14 @@ static int thermal_set_mode(struct
>> > thermal_zone_device *thermal,
>> >             ACPI_DEBUG_PRINT((ACPI_DB_INFO,
>> >                     "%s kernel ACPI thermal control\n",
>> >                     tz->tz_enabled ? "Enable" : "Disable"));
>> > +
>> > +           acpi_thermal_get_polling_frequency(tz);
>> > +
>> > +           mutex_lock(&tz->thermal_zone->lock);
>> > +           tz->thermal_zone->polling_delay = tz-
>> > >polling_frequency * 100;
>> > +           tz->thermal_zone->passive_delay = tz-
>> > >polling_frequency * 100;
>
> To me, this policy, "disabling polling when disabling thermal zone,
> enabling polling after enabling thermal zone" applies to all the
> thermal zones, thus it's better to be implemented in thermal subsystem,
> rather than ACPI thermal driver.
>

Ok, sounds reasonable, maybe we can call get_mode in
thermal_zone_device_set_polling and cancel the workqueue if device is
DISABLED and reenable if device is ENABLED. Guess that can be more
simple. Let me think about it and I'll prepare a second version of the
patch.

> BTW, two other comments about the code itself,
> 1. I don't like the way how tz->polling_delay/tz->passive_delay are
> changed here. If we have to do this, better using a thermal API, and we
> should invoke monitor_thermal_zone() right after updating these two
> fields.
> 2. passive_delay is set to wrong value here.
>

Thanks, I'll look at this and check once I figure out how to move all
to the thermal subsystem.

> thanks,
> rui
>> > +           mutex_unlock(&tz->thermal_zone->lock);
>> > +
>> >             acpi_thermal_check(tz);
>> >     }
>> >     return 0;
>> > @@ -930,8 +949,6 @@ static int
>> > acpi_thermal_register_thermal_zone(struct acpi_thermal *tz)
>> >     if (ACPI_FAILURE(status))
>> >             return -ENODEV;
>> >
>> > -   tz->tz_enabled = 1;
>> > -
>> >     dev_info(&tz->device->dev, "registered as
>> > thermal_zone%d\n",
>> >              tz->thermal_zone->id);
>> >     return 0;
>> > @@ -1039,11 +1056,7 @@ static int acpi_thermal_get_info(struct
>> > acpi_thermal *tz)
>> >     if (!result)
>> >             tz->flags.cooling_mode = 1;
>> >
>> > -   /* Get default polling frequency [_TZP] (optional) */
>> > -   if (tzp)
>> > -           tz->polling_frequency = tzp;
>> > -   else
>> > -           acpi_thermal_get_polling_frequency(tz);
>> > +   acpi_thermal_get_polling_frequency(tz);
>> >
>> >     return 0;
>> >  }
>> > @@ -1088,6 +1101,7 @@ static int acpi_thermal_add(struct
>> > acpi_device *device)
>> >             return -ENOMEM;
>> >
>> >     tz->device = device;
>> > +   tz->tz_enabled = 1;
>> >     strcpy(tz->name, device->pnp.bus_id);
>> >     strcpy(acpi_device_name(device),
>> > ACPI_THERMAL_DEVICE_NAME);
>> >     strcpy(acpi_device_class(device), ACPI_THERMAL_CLASS);
>> >
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-acpi"
>> in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html