Messages in this thread | ![/](/images/icornerl.gif) | | From | "Rafael J. Wysocki" <> | Date | Wed, 20 Dec 2023 15:53:04 +0100 | Subject | Re: [PATCH v2 6/8] thermal/sysfs: Update instance->weight under tz lock |
| |
On Tue, Dec 12, 2023 at 2:48 PM Lukasz Luba <lukasz.luba@arm.com> wrote: > > The user-space can change thermal instance weight value while the > throtte() callback is running for a governor.
This needs to be more precise IMV. Something like
"User space can change the weight of a thermal instance via sysfs while the .throttle() callback is running for a governor, because weight_store() does not use the zone lock."
> The IPA governor uses the > 'weight' value for power calculation and also keeps it in 'total_weight'. > Therefore, the 'weight' value must not change during the throttle() > callback. Use 'tz->lock' mutex which also guards the throttle() to make > the update value safe.
"The IPA governor uses instance weight values for power calculations and caches the sum of them as total_weight, so it gets confused when one of them changes while its .throttle() callback is running.
To prevent that from happening, use thermal zone locking in weight_store()."
> > Signed-off-by: Lukasz Luba <lukasz.luba@arm.com> > --- > drivers/thermal/thermal_sysfs.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/thermal/thermal_sysfs.c b/drivers/thermal/thermal_sysfs.c > index eef40d4f3063..df85df7d4a88 100644 > --- a/drivers/thermal/thermal_sysfs.c > +++ b/drivers/thermal/thermal_sysfs.c > @@ -961,6 +961,7 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr, > const char *buf, size_t count) > { > struct thermal_instance *instance; > + struct thermal_zone_device *tz;
IMO the local var doesn't help much. You may as well use instance->tz->lock directly below.
> int ret, weight; > > ret = kstrtoint(buf, 0, &weight); > @@ -968,7 +969,12 @@ ssize_t weight_store(struct device *dev, struct device_attribute *attr, > return ret; > > instance = container_of(attr, struct thermal_instance, weight_attr); > + tz = instance->tz; > + > + /* Don't race with governors using the 'weight' value */ > + mutex_lock(&tz->lock); > instance->weight = weight; > + mutex_unlock(&tz->lock); > > return count; > } > --
| ![\](/images/icornerr.gif) |