| Message ID | 20221017130910.2307118-6-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | thermal/core: Protect thermal device operations against removal | expand |
On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote: > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only > to be reacquired in the subsequent call to thermal_zone_device_update(). > > Introduce __thermal_zone_device_update() as locked version of Did you mean "unlocked"? > thermal_zone_device_update() and call it from > thermal_zone_device_set_mode() without releasing the lock to avoid > the extra release/acuire sequence. > > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > drivers/thermal/thermal_core.c | 57 ++++++++++++++++++---------------- > 1 file changed, 31 insertions(+), 26 deletions(-) > > diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c > index 562ece8d16aa..9facd9c5b70f 100644 > --- a/drivers/thermal/thermal_core.c > +++ b/drivers/thermal/thermal_core.c > @@ -403,6 +403,34 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz) > pos->initialized = false; > } > > +static void __thermal_zone_device_update(struct thermal_zone_device *tz, > + enum thermal_notify_event event) > +{ > + int count; > + > + if (atomic_read(&in_suspend)) > + return; > + > + if (WARN_ONCE(!tz->ops->get_temp, > + "'%s' must not be called without 'get_temp' ops set\n", > + __func__)) > + return; > + > + if (!thermal_zone_device_is_enabled(tz)) > + return; > + > + update_temperature(tz); > + > + __thermal_zone_set_trips(tz); > + > + tz->notify_event = event; > + > + for (count = 0; count < tz->num_trips; count++) > + handle_thermal_trip(tz, count); > + > + monitor_thermal_zone(tz); > +} > + > static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, > enum thermal_device_mode mode) > { > @@ -423,9 +451,9 @@ static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, > if (!ret) > tz->mode = mode; > > - mutex_unlock(&tz->lock); > + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > > - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); > + mutex_unlock(&tz->lock); > > if (mode == THERMAL_DEVICE_ENABLED) > thermal_notify_tz_enable(tz->id); > @@ -457,31 +485,8 @@ int thermal_zone_device_is_enabled(struct thermal_zone_device *tz) > void thermal_zone_device_update(struct thermal_zone_device *tz, > enum thermal_notify_event event) > { > - int count; > - > - if (atomic_read(&in_suspend)) > - return; > - > - if (WARN_ONCE(!tz->ops->get_temp, "'%s' must not be called without " > - "'get_temp' ops set\n", __func__)) > - return; > - > mutex_lock(&tz->lock); > - > - if (!thermal_zone_device_is_enabled(tz)) > - goto out; > - > - update_temperature(tz); > - > - __thermal_zone_set_trips(tz); > - > - tz->notify_event = event; > - > - for (count = 0; count < tz->num_trips; count++) > - handle_thermal_trip(tz, count); > - > - monitor_thermal_zone(tz); > -out: > + __thermal_zone_device_update(tz, event); > mutex_unlock(&tz->lock); > } > EXPORT_SYMBOL_GPL(thermal_zone_device_update); > -- > 2.36.2 >
On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote: > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only > > to be reacquired in the subsequent call to thermal_zone_device_update(). > > > > Introduce __thermal_zone_device_update() as locked version of > > Did you mean "unlocked"? > No, I did mean "locked", as in "must be called with thermal zone device mutex acquired". locked: void __thermal_zone_device_update(struct thermal_zone_device *tz, enum thermal_notify_event event) { ... } unlocked: void thermal_zone_device_update(struct thermal_zone_device *tz, enum thermal_notify_event event) { mutex_lock(&tz->lock); if (device_is_registered(&tz->device)) __thermal_zone_device_update(tz, event); mutex_unlock(&tz->lock); } Should I phrase or explain it differently ? Thanks, Guenter
On Thu, Nov 10, 2022 at 1:25 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote: > > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only > > > to be reacquired in the subsequent call to thermal_zone_device_update(). > > > > > > Introduce __thermal_zone_device_update() as locked version of > > > > Did you mean "unlocked"? > > > No, I did mean "locked", as in "must be called with thermal zone device > mutex acquired". > > locked: > > void __thermal_zone_device_update(struct thermal_zone_device *tz, > enum thermal_notify_event event) > { > ... > } > > unlocked: > > void thermal_zone_device_update(struct thermal_zone_device *tz, > enum thermal_notify_event event) > { > mutex_lock(&tz->lock); > if (device_is_registered(&tz->device)) > __thermal_zone_device_update(tz, event); > mutex_unlock(&tz->lock); > } Thanks for the explanation. > Should I phrase or explain it differently ? I would rather say "bare" or something like that so it is all clear to people like me, but it is your call.
On Thu, Nov 10, 2022 at 02:01:49PM +0100, Rafael J. Wysocki wrote: > On Thu, Nov 10, 2022 at 1:25 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote: > > > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only > > > > to be reacquired in the subsequent call to thermal_zone_device_update(). > > > > > > > > Introduce __thermal_zone_device_update() as locked version of > > > > > > Did you mean "unlocked"? > > > > > No, I did mean "locked", as in "must be called with thermal zone device > > mutex acquired". > > > > locked: > > > > void __thermal_zone_device_update(struct thermal_zone_device *tz, > > enum thermal_notify_event event) > > { > > ... > > } > > > > unlocked: > > > > void thermal_zone_device_update(struct thermal_zone_device *tz, > > enum thermal_notify_event event) > > { > > mutex_lock(&tz->lock); > > if (device_is_registered(&tz->device)) > > __thermal_zone_device_update(tz, event); > > mutex_unlock(&tz->lock); > > } > > Thanks for the explanation. > > > Should I phrase or explain it differently ? > > I would rather say "bare" or something like that so it is all clear to > people like me, but it is your call. I updated the commit description to use "must be called with thermal device mutex held". I kept 'locked' in the subject; I don't think using 'bare' there would add any clarity. Hope that is ok. Thanks, Guenter
On Thu, Nov 10, 2022 at 3:12 PM Guenter Roeck <linux@roeck-us.net> wrote: > > On Thu, Nov 10, 2022 at 02:01:49PM +0100, Rafael J. Wysocki wrote: > > On Thu, Nov 10, 2022 at 1:25 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > On Wed, Nov 09, 2022 at 08:15:17PM +0100, Rafael J. Wysocki wrote: > > > > On Mon, Oct 17, 2022 at 3:09 PM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > > > > > In thermal_zone_device_set_mode(), the thermal zone mutex is released only > > > > > to be reacquired in the subsequent call to thermal_zone_device_update(). > > > > > > > > > > Introduce __thermal_zone_device_update() as locked version of > > > > > > > > Did you mean "unlocked"? > > > > > > > No, I did mean "locked", as in "must be called with thermal zone device > > > mutex acquired". > > > > > > locked: > > > > > > void __thermal_zone_device_update(struct thermal_zone_device *tz, > > > enum thermal_notify_event event) > > > { > > > ... > > > } > > > > > > unlocked: > > > > > > void thermal_zone_device_update(struct thermal_zone_device *tz, > > > enum thermal_notify_event event) > > > { > > > mutex_lock(&tz->lock); > > > if (device_is_registered(&tz->device)) > > > __thermal_zone_device_update(tz, event); > > > mutex_unlock(&tz->lock); > > > } > > > > Thanks for the explanation. > > > > > Should I phrase or explain it differently ? > > > > I would rather say "bare" or something like that so it is all clear to > > people like me, but it is your call. > > I updated the commit description to use "must be called with thermal > device mutex held". I kept 'locked' in the subject; I don't think using > 'bare' there would add any clarity. Hope that is ok. It is.
diff --git a/drivers/thermal/thermal_core.c b/drivers/thermal/thermal_core.c index 562ece8d16aa..9facd9c5b70f 100644 --- a/drivers/thermal/thermal_core.c +++ b/drivers/thermal/thermal_core.c @@ -403,6 +403,34 @@ static void thermal_zone_device_init(struct thermal_zone_device *tz) pos->initialized = false; } +static void __thermal_zone_device_update(struct thermal_zone_device *tz, + enum thermal_notify_event event) +{ + int count; + + if (atomic_read(&in_suspend)) + return; + + if (WARN_ONCE(!tz->ops->get_temp, + "'%s' must not be called without 'get_temp' ops set\n", + __func__)) + return; + + if (!thermal_zone_device_is_enabled(tz)) + return; + + update_temperature(tz); + + __thermal_zone_set_trips(tz); + + tz->notify_event = event; + + for (count = 0; count < tz->num_trips; count++) + handle_thermal_trip(tz, count); + + monitor_thermal_zone(tz); +} + static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, enum thermal_device_mode mode) { @@ -423,9 +451,9 @@ static int thermal_zone_device_set_mode(struct thermal_zone_device *tz, if (!ret) tz->mode = mode; - mutex_unlock(&tz->lock); + __thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); - thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED); + mutex_unlock(&tz->lock); if (mode == THERMAL_DEVICE_ENABLED) thermal_notify_tz_enable(tz->id); @@ -457,31 +485,8 @@ int thermal_zone_device_is_enabled(struct thermal_zone_device *tz) void thermal_zone_device_update(struct thermal_zone_device *tz, enum thermal_notify_event event) { - int count; - - if (atomic_read(&in_suspend)) - return; - - if (WARN_ONCE(!tz->ops->get_temp, "'%s' must not be called without " - "'get_temp' ops set\n", __func__)) - return; - mutex_lock(&tz->lock); - - if (!thermal_zone_device_is_enabled(tz)) - goto out; - - update_temperature(tz); - - __thermal_zone_set_trips(tz); - - tz->notify_event = event; - - for (count = 0; count < tz->num_trips; count++) - handle_thermal_trip(tz, count); - - monitor_thermal_zone(tz); -out: + __thermal_zone_device_update(tz, event); mutex_unlock(&tz->lock); } EXPORT_SYMBOL_GPL(thermal_zone_device_update);
In thermal_zone_device_set_mode(), the thermal zone mutex is released only to be reacquired in the subsequent call to thermal_zone_device_update(). Introduce __thermal_zone_device_update() as locked version of thermal_zone_device_update() and call it from thermal_zone_device_set_mode() without releasing the lock to avoid the extra release/acuire sequence. Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- drivers/thermal/thermal_core.c | 57 ++++++++++++++++++---------------- 1 file changed, 31 insertions(+), 26 deletions(-)