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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 29 Jun 2017 13:19:42 +0800
From:   Zhang Rui <rui.zhang@...el.com>
To:     "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Enric Balletbo i Serra <enric.balletbo@...labora.com>,
        Srinivas Pandruvada <srinivas.pandruvada@...ux.intel.com>
Cc:     Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
        linux-kernel@...r.kernel.org, Guenter Roeck <groeck@...omium.org>,
        Sameer Nanda <snanda@...omium.org>,
        "Zhang, Rui" <rui.zhang@...el.com>
Subject: Re: [PATCH] acpi: thermal: honor "mode" sysfs file setting

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@...omium.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@...omium.org>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@...labora.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@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html