All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1
@ 2017-07-03  8:00 Enric Balletbo i Serra
  2017-07-03  8:00 ` [PATCH v3 2/2] thermal: core: introduce thermal zone device mode control Enric Balletbo i Serra
  2017-07-03 21:08 ` [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1 Rafael J. Wysocki
  0 siblings, 2 replies; 6+ messages in thread
From: Enric Balletbo i Serra @ 2017-07-03  8:00 UTC (permalink / raw)
  To: Zhang Rui, rjw, Len Brown, linux-acpi, linux-kernel
  Cc: Guenter Roeck, Sameer Nanda

From: Sameer Nanda <snanda@chromium.org>

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.

Moved setting of tz_enabled to 1 into acpi_thermal_add itself.

Signed-off-by: Sameer Nanda <snanda@chromium.org>
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v2:
 - Zhang Rui: 
   - Make sure tz->tz_enabled is set properly before registering the zone.

Changes since v1:
 - This patch is new from v1 [1]

 [1] https://patchwork.kernel.org/patch/9804229/

 drivers/acpi/thermal.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
index 1d0417b..cd0fe92 100644
--- a/drivers/acpi/thermal.c
+++ b/drivers/acpi/thermal.c
@@ -930,8 +930,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;
@@ -1088,6 +1086,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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH v3 2/2] thermal: core: introduce thermal zone device mode control
  2017-07-03  8:00 [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1 Enric Balletbo i Serra
@ 2017-07-03  8:00 ` Enric Balletbo i Serra
  2017-07-03 21:07   ` Rafael J. Wysocki
  2017-07-03 21:08 ` [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1 Rafael J. Wysocki
  1 sibling, 1 reply; 6+ messages in thread
From: Enric Balletbo i Serra @ 2017-07-03  8:00 UTC (permalink / raw)
  To: Zhang Rui, rjw, Len Brown, linux-acpi, linux-kernel
  Cc: Guenter Roeck, Sameer Nanda

From: Zhang Rui <rui.zhang@intel.com>

Thermal "mode" sysfs attribute is introduced to enable/disable a thermal zone,
and .get_mode()/.set_mode() callback is introduced for platform thermal driver
to enable/disable the hardware thermal control logic. And thermal core takes
no action upon thermal zone enable/disable.

Actually, this is not quite right because thermal core still pokes those
disabled thermal zones occasionally, e.g. upon system resume.

To fix this, a new flag 'mode' is introduced in struct thermal_zone_device
to represent the thermal zone mode, and several decisions have been made
based on this flag, including
1. check the thermal zone mode right after it's registered.
2. skip updating thermal zone if the zone is disabled
3. stop the polling timer when the thermal zone is disabled

Note: basically, this patch doesn't affect the existing always-enabled
thermal zones much, with just one exception -
thermal zone .get_mode() must be well prepared to reflect the real thermal
zone status upon the thermal zone registration.

Signed-off-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Changes since v2:
 - Rework by Zhang Rui.
Changes since v1:
 - Implement in thermal subsystem instead of ACPI thermal driver (Zhang Rui)

 drivers/thermal/thermal_core.c  | 37 +++++++++++++++++++++++++++++++++++--
 drivers/thermal/thermal_sysfs.c | 22 ++++++----------------
 include/linux/thermal.h         |  3 +++
 3 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c
index 5a51c74..1959257 100644
--- a/drivers/thermal/thermal_core.c
+++ b/drivers/thermal/thermal_core.c
@@ -306,9 +306,9 @@ static void monitor_thermal_zone(struct thermal_zone_device *tz)
 {
 	mutex_lock(&tz->lock);
 
-	if (tz->passive)
+	if (tz->mode == THERMAL_DEVICE_ENABLED && tz->passive)
 		thermal_zone_device_set_polling(tz, tz->passive_delay);
-	else if (tz->polling_delay)
+	else if (tz->mode == THERMAL_DEVICE_ENABLED && tz->polling_delay)
 		thermal_zone_device_set_polling(tz, tz->polling_delay);
 	else
 		thermal_zone_device_set_polling(tz, 0);
@@ -464,11 +464,35 @@ static void thermal_zone_device_reset(struct thermal_zone_device *tz)
 		pos->initialized = false;
 }
 
+int thermal_zone_set_mode(struct thermal_zone_device *tz,
+				 enum thermal_device_mode mode)
+{
+	int result;
+
+	if (!tz->ops->set_mode)
+		return -EPERM;
+
+	result = tz->ops->set_mode(tz, mode);
+	if (result)
+		return result;
+
+	if (tz->mode != mode) {
+		tz->mode = mode;
+		monitor_thermal_zone(tz);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(thermal_zone_set_mode);
+
 void thermal_zone_device_update(struct thermal_zone_device *tz,
 				enum thermal_notify_event event)
 {
 	int count;
 
+	/* Do nothing if the thermal zone is disabled */
+	if (tz->mode == THERMAL_DEVICE_DISABLED)
+		return;
+
 	if (atomic_read(&in_suspend))
 		return;
 
@@ -1287,6 +1311,15 @@ thermal_zone_device_register(const char *type, int trips, int mask,
 	INIT_DELAYED_WORK(&tz->poll_queue, thermal_zone_device_check);
 
 	thermal_zone_device_reset(tz);
+
+	if (tz->ops->get_mode) {
+		enum thermal_device_mode mode;
+
+		result = tz->ops->get_mode(tz, &mode);
+		tz->mode = result ? THERMAL_DEVICE_ENABLED : mode;
+	} else
+		tz->mode = THERMAL_DEVICE_ENABLED;
+
 	/* Update the new thermal zone and mark it as already updated. */
 	if (atomic_cmpxchg(&tz->need_update, 1, 0))
 		thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c
index a694de9..1b25dc8 100644
--- a/drivers/thermal/thermal_sysfs.c
+++ b/drivers/thermal/thermal_sysfs.c
@@ -51,17 +51,8 @@ static ssize_t
 mode_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
-	enum thermal_device_mode mode;
-	int result;
-
-	if (!tz->ops->get_mode)
-		return -EPERM;
 
-	result = tz->ops->get_mode(tz, &mode);
-	if (result)
-		return result;
-
-	return sprintf(buf, "%s\n", mode == THERMAL_DEVICE_ENABLED ? "enabled"
+	return sprintf(buf, "%s\n", tz->mode == THERMAL_DEVICE_ENABLED ? "enabled"
 		       : "disabled");
 }
 
@@ -70,18 +61,17 @@ mode_store(struct device *dev, struct device_attribute *attr,
 	   const char *buf, size_t count)
 {
 	struct thermal_zone_device *tz = to_thermal_zone(dev);
+	enum thermal_device_mode mode;
 	int result;
 
-	if (!tz->ops->set_mode)
-		return -EPERM;
-
 	if (!strncmp(buf, "enabled", sizeof("enabled") - 1))
-		result = tz->ops->set_mode(tz, THERMAL_DEVICE_ENABLED);
+		mode = THERMAL_DEVICE_ENABLED;
 	else if (!strncmp(buf, "disabled", sizeof("disabled") - 1))
-		result = tz->ops->set_mode(tz, THERMAL_DEVICE_DISABLED);
+		mode = THERMAL_DEVICE_DISABLED;
 	else
-		result = -EINVAL;
+		return -EINVAL;
 
+	result = thermal_zone_set_mode(tz, mode);
 	if (result)
 		return result;
 
diff --git a/include/linux/thermal.h b/include/linux/thermal.h
index dab11f9..2f427de 100644
--- a/include/linux/thermal.h
+++ b/include/linux/thermal.h
@@ -210,6 +210,7 @@ struct thermal_zone_device {
 	struct thermal_attr *trip_type_attrs;
 	struct thermal_attr *trip_hyst_attrs;
 	void *devdata;
+	enum thermal_device_mode mode;
 	int trips;
 	unsigned long trips_disabled;	/* bitmap for disabled trips */
 	int passive_delay;
@@ -465,6 +466,8 @@ struct thermal_zone_device *thermal_zone_get_zone_by_name(const char *name);
 int thermal_zone_get_temp(struct thermal_zone_device *tz, int *temp);
 int thermal_zone_get_slope(struct thermal_zone_device *tz);
 int thermal_zone_get_offset(struct thermal_zone_device *tz);
+int thermal_zone_set_mode(struct thermal_zone_device *tz,
+			  enum thermal_device_mode mode);
 
 int get_tz_trend(struct thermal_zone_device *, int);
 struct thermal_instance *get_thermal_instance(struct thermal_zone_device *,
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] thermal: core: introduce thermal zone device mode control
  2017-07-03  8:00 ` [PATCH v3 2/2] thermal: core: introduce thermal zone device mode control Enric Balletbo i Serra
@ 2017-07-03 21:07   ` Rafael J. Wysocki
  0 siblings, 0 replies; 6+ messages in thread
From: Rafael J. Wysocki @ 2017-07-03 21:07 UTC (permalink / raw)
  To: Enric Balletbo i Serra
  Cc: Zhang Rui, Rafael J. Wysocki, Len Brown, ACPI Devel Maling List,
	Linux Kernel Mailing List, Guenter Roeck, Sameer Nanda

On Mon, Jul 3, 2017 at 10:00 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> From: Zhang Rui <rui.zhang@intel.com>
>
> Thermal "mode" sysfs attribute is introduced to enable/disable a thermal zone,
> and .get_mode()/.set_mode() callback is introduced for platform thermal driver
> to enable/disable the hardware thermal control logic. And thermal core takes
> no action upon thermal zone enable/disable.
>
> Actually, this is not quite right because thermal core still pokes those
> disabled thermal zones occasionally, e.g. upon system resume.
>
> To fix this, a new flag 'mode' is introduced in struct thermal_zone_device
> to represent the thermal zone mode, and several decisions have been made
> based on this flag, including
> 1. check the thermal zone mode right after it's registered.
> 2. skip updating thermal zone if the zone is disabled
> 3. stop the polling timer when the thermal zone is disabled
>
> Note: basically, this patch doesn't affect the existing always-enabled
> thermal zones much, with just one exception -
> thermal zone .get_mode() must be well prepared to reflect the real thermal
> zone status upon the thermal zone registration.
>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> Tested-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>

I'm thinking that Rui is going to push this patch himself, in which
case there's no need to resend (and you should add your sign-off to
the patch when resending it anyway).

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1
  2017-07-03  8:00 [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1 Enric Balletbo i Serra
  2017-07-03  8:00 ` [PATCH v3 2/2] thermal: core: introduce thermal zone device mode control Enric Balletbo i Serra
@ 2017-07-03 21:08 ` Rafael J. Wysocki
  2017-07-04  1:43   ` Zhang Rui
  1 sibling, 1 reply; 6+ messages in thread
From: Rafael J. Wysocki @ 2017-07-03 21:08 UTC (permalink / raw)
  To: Enric Balletbo i Serra, Zhang Rui
  Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List,
	Linux Kernel Mailing List, Guenter Roeck, Sameer Nanda

On Mon, Jul 3, 2017 at 10:00 AM, Enric Balletbo i Serra
<enric.balletbo@collabora.com> wrote:
> From: Sameer Nanda <snanda@chromium.org>
>
> 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.
>
> Moved setting of tz_enabled to 1 into acpi_thermal_add itself.
>
> Signed-off-by: Sameer Nanda <snanda@chromium.org>
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Changes since v2:
>  - Zhang Rui:
>    - Make sure tz->tz_enabled is set properly before registering the zone.
>
> Changes since v1:
>  - This patch is new from v1 [1]
>
>  [1] https://patchwork.kernel.org/patch/9804229/
>
>  drivers/acpi/thermal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> index 1d0417b..cd0fe92 100644
> --- a/drivers/acpi/thermal.c
> +++ b/drivers/acpi/thermal.c
> @@ -930,8 +930,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;
> @@ -1088,6 +1086,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);
> --

Rui,

Can I just apply this, or do I need to work for a thermal core update?
 In the latter case, can you take care of this one too, please?

Thanks,
Rafael

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1
  2017-07-03 21:08 ` [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1 Rafael J. Wysocki
@ 2017-07-04  1:43   ` Zhang Rui
  2017-12-20 11:27     ` Enric Balletbo Serra
  0 siblings, 1 reply; 6+ messages in thread
From: Zhang Rui @ 2017-07-04  1:43 UTC (permalink / raw)
  To: Rafael J. Wysocki, Enric Balletbo i Serra
  Cc: Rafael J. Wysocki, Len Brown, ACPI Devel Maling List,
	Linux Kernel Mailing List, Guenter Roeck, Sameer Nanda

On Mon, 2017-07-03 at 23:08 +0200, Rafael J. Wysocki wrote:
> On Mon, Jul 3, 2017 at 10:00 AM, Enric Balletbo i Serra
> <enric.balletbo@collabora.com> wrote:
> > 
> > From: Sameer Nanda <snanda@chromium.org>
> > 
> > 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.
> > 
> > Moved setting of tz_enabled to 1 into acpi_thermal_add itself.
> > 
> > Signed-off-by: Sameer Nanda <snanda@chromium.org>
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
> > >
> > ---
> > Changes since v2:
> >  - Zhang Rui:
> >    - Make sure tz->tz_enabled is set properly before registering
> > the zone.
> > 
> > Changes since v1:
> >  - This patch is new from v1 [1]
> > 
> >  [1] https://patchwork.kernel.org/patch/9804229/
> > 
> >  drivers/acpi/thermal.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> > 
> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
> > index 1d0417b..cd0fe92 100644
> > --- a/drivers/acpi/thermal.c
> > +++ b/drivers/acpi/thermal.c
> > @@ -930,8 +930,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;
> > @@ -1088,6 +1086,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);
> > --
> Rui,
> 
> Can I just apply this, or do I need to work for a thermal core
> update?
>  In the latter case, can you take care of this one too, please?
> 
Yes, I will take both of the patches.

thanks,
rui

> Thanks,
> Rafael

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1
  2017-07-04  1:43   ` Zhang Rui
@ 2017-12-20 11:27     ` Enric Balletbo Serra
  0 siblings, 0 replies; 6+ messages in thread
From: Enric Balletbo Serra @ 2017-12-20 11:27 UTC (permalink / raw)
  To: Zhang Rui
  Cc: Rafael J. Wysocki, Enric Balletbo i Serra, Rafael J. Wysocki,
	Len Brown, ACPI Devel Maling List, Linux Kernel Mailing List,
	Guenter Roeck, Sameer Nanda

Hi Rui,

2017-07-04 3:43 GMT+02:00 Zhang Rui <rui.zhang@intel.com>:
> On Mon, 2017-07-03 at 23:08 +0200, Rafael J. Wysocki wrote:
>> On Mon, Jul 3, 2017 at 10:00 AM, Enric Balletbo i Serra
>> <enric.balletbo@collabora.com> wrote:
>> >
>> > From: Sameer Nanda <snanda@chromium.org>
>> >
>> > 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.
>> >
>> > Moved setting of tz_enabled to 1 into acpi_thermal_add itself.
>> >
>> > Signed-off-by: Sameer Nanda <snanda@chromium.org>
>> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
>> > >
>> > ---
>> > Changes since v2:
>> >  - Zhang Rui:
>> >    - Make sure tz->tz_enabled is set properly before registering
>> > the zone.
>> >
>> > Changes since v1:
>> >  - This patch is new from v1 [1]
>> >
>> >  [1] https://patchwork.kernel.org/patch/9804229/
>> >
>> >  drivers/acpi/thermal.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/acpi/thermal.c b/drivers/acpi/thermal.c
>> > index 1d0417b..cd0fe92 100644
>> > --- a/drivers/acpi/thermal.c
>> > +++ b/drivers/acpi/thermal.c
>> > @@ -930,8 +930,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;
>> > @@ -1088,6 +1086,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);
>> > --
>> Rui,
>>
>> Can I just apply this, or do I need to work for a thermal core
>> update?
>>  In the latter case, can you take care of this one too, please?
>>
> Yes, I will take both of the patches.
>

Long time since I posted these patches but didn't land, I'm not sure
if you're waiting an action from me. Are fine ? Do you want me to
rebase it on top of mainline and resend?

Thanks,
 Enric

> thanks,
> rui
>
>> Thanks,
>> Rafael

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-20 11:27 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-03  8:00 [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1 Enric Balletbo i Serra
2017-07-03  8:00 ` [PATCH v3 2/2] thermal: core: introduce thermal zone device mode control Enric Balletbo i Serra
2017-07-03 21:07   ` Rafael J. Wysocki
2017-07-03 21:08 ` [PATCH v3 1/2] acpi: thermal: initialize tz_enabled to 1 Rafael J. Wysocki
2017-07-04  1:43   ` Zhang Rui
2017-12-20 11:27     ` Enric Balletbo Serra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.