Re: [PATCH v2 2/2] thermal: core: Allow to disable polling when disabling thermal zone.

From: Zhang Rui
Date: Fri Jun 30 2017 - 23:06:18 EST

Next message: Eduardo Valentin: "Re: [Patch v2 3/3] arm64: dts: register Hi3660's thermal sensor"
Previous message: Eduardo Valentin: "Re: [Patch v2 1/3] dt-bindings: Document the hi3660 thermal sensor bindings"
In reply to: Enric Balletbo Serra: "Re: [PATCH v2 2/2] thermal: core: Allow to disable polling when disabling thermal zone."
Next in thread: Zhang Rui: "Re: [PATCH v2 1/2] acpi: thermal: update thermal_zone after enable the driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi, Enric,

On Fri, 2017-06-30 at 10:15 +0200, Enric Balletbo Serra wrote:
> Hi Rui,
>
> 2017-06-30 7:05 GMT+02:00 Zhang Rui <rui.zhang@xxxxxxxxx>:
> >
> > On Thu, 2017-06-29 at 18:50 +0200, Enric Balletbo i Serra wrote:
> > >
> > > Under each thermal zone there is a optional file called "mode".
> > > Writing
> > > enabled or disabled to this file allows a given thermal zone to
> > > be
> > > enabled
> > > or disabled, but in current code, the monitoring queue doesn't
> > > stops.
> > > Add
> > > the code to disable polling when disabling thermal zone and
> > > enable
> > > polling
> > > when enabling the thermal zone.
> > >
> > > This patch is based on the original Sameer Nanda <snanda@chromium
> > > .org
> > > >
> > > >
> > > patch that implemented this idea for the ACPI thermal driver.
> > >
> > Before these two patches, only platform thermal driver cares about
> > "mode", thermal core does nothing but invokes platform .set_mode()
> > callback upon sysfs I/F write.
> > But after this patch set, "mode" becomes something that we should
> > take into account in thermal core as well.
> > Thus, IMO, we have a couple of things more to do, like the
> > prototype
> > patch attached, which I have not tested yet.
> >
> > From 8bf51fe65bd386b7e8c3c2ca8b9f7d321c92b22f Mon Sep 17 00:00:00
> > 2001
> > From: Zhang Rui <rui.zhang@xxxxxxxxx>
> > Date: Fri, 30 Jun 2017 11:11:45 +0800
> > Subject: [RFC PATCH] Thermal: introduce thermal zone device mode
> > control
> >
> > 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@xxxxxxxxx>
> > ---
> > Â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..89b2254 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->enabled == THERMAL_DEVICE_ENABLED && tz->passive)
> tz->mode
>
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂthermal_zone_device_set_polling(tz, tz-
> > >passive_delay);
> > -ÂÂÂÂÂÂÂelse if (tz->polling_delay)
> > +ÂÂÂÂÂÂÂelse if (tz->enabled == THERMAL_DEVICE_ENABLED && tz-
> > >polling_delay)
> tz->mode
>
> >
> > ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ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()) {
> remove the ()
>
> >
> > +ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ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..95d2587 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.7.4
> >
> There are some build issues but once fixed the patch works as
> expected, many thanks.
>
> Tested-by: Enric Balletbo i Serra <enric.balletbo@xxxxxxxxxxxxx>
>
> Rui, do you want I send a fixed version of this patch?

Please send out a fixed version.
But for this patch, I'd like to leave it in linux-next for sometime
before pushing it, as it makes some functional changes. And it will be
queued for 4.14 if there is no other problems.

thanks,
rui
> Or do you want
> to fix yourself?
>
> Best regards,
> Â Enric

Next message: Eduardo Valentin: "Re: [Patch v2 3/3] arm64: dts: register Hi3660's thermal sensor"
Previous message: Eduardo Valentin: "Re: [Patch v2 1/3] dt-bindings: Document the hi3660 thermal sensor bindings"
In reply to: Enric Balletbo Serra: "Re: [PATCH v2 2/2] thermal: core: Allow to disable polling when disabling thermal zone."
Next in thread: Zhang Rui: "Re: [PATCH v2 1/2] acpi: thermal: update thermal_zone after enable the driver."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]