Re: [PATCH 1/8] PM / devfreq: Lock devfreq in trans_stat_show

[Matthias Kaehlcke <mka@chromium.org>]

On Thu, Sep 19, 2019 at 06:42:22PM +0000, Leonard Crestez wrote:
> On 19.09.2019 00:28, Matthias Kaehlcke wrote:
> > Hi Leonard,
> > 
> > this series doesn't indicate the version, from the change history in
> > the cover letter I suppose it is v5.
> 
> Sorry about that, I forgot --subject-prefix. It is indeed v5
> 
> > On Wed, Sep 18, 2019 at 03:18:20AM +0300, Leonard Crestez wrote:
> >> There is no locking in this sysfs show function so stats printing can
> >> race with a devfreq_update_status called as part of freq switching or
> >> with initialization.
> >>
> >> Also add an assert in devfreq_update_status to make it clear that lock
> >> must be held by caller.
> > 
> > This and some other patches look like generic improvements and not
> > directly related to the series "PM / devfreq: Add dev_pm_qos
> > support". If there are no dependencies I think it is usually better to
> > send the improvements separately, it keeps the series more focussed
> > and might reduce version churn. Just my POV though ;-)
> 
> The locking cleanups are required in order to initialize pm_qos request 
> and notifiers without introducing lockdep warnings.
> 
> pm_qos calls notifiers under dev_pm_qos_mtx and those notifiers needs to 
> take &devfreq->lock. This means initializing pm_qos notifiers and 
> requests must be done outside &devfreq->lock which needs some cleanups 
> in devfreq_add_device.

Thanks for the clarification!

> This particular patch is a more loosely related bugfix. Devfreq 
> maintainers: would it help to post it separately?

If it's just this single patch it probably isn't a problem, I'd be
more concerned about multiple unrelated patches or if the changes are
complex.

> >> @@ -1415,15 +1416,20 @@ static ssize_t trans_stat_show(struct device *dev,
> >>   	struct devfreq *devfreq = to_devfreq(dev);
> >>   	ssize_t len;
> >>   	int i, j;
> >>   	unsigned int max_state = devfreq->profile->max_state;
> >>   
> >> +	mutex_lock(&devfreq->lock);
> >>   	if (!devfreq->stop_polling &&
> >> -			devfreq_update_status(devfreq, devfreq->previous_freq))
> >> -		return 0;
> >> -	if (max_state == 0)
> >> -		return sprintf(buf, "Not Supported.\n");
> >> +			devfreq_update_status(devfreq, devfreq->previous_freq)) {
> >> +		len = 0;
> > 
> > you could assign 'len' in the declaration instead, but it's just
> > another option, it'ss fine as is
> >> +		goto out;
> >> +	}
> >> +	if (max_state == 0) {
> >> +		len = sprintf(buf, "Not Supported.\n");
> >> +		goto out;
> >> +	}
> > 
> > This leaves the general structure of the code as is, which is great,
> > but since you are already touching this part you can consider to
> > improve it: 'max_state' is constant after device creation, hence the
> > check could be done at the beginning, which IMO would be clearer, it
> > could also save an unnecessary devfreq_update_status() call and it
> > wouldn't be necessary to hold the lock (one goto less).
> 
> Now that I look at this more closely &devfreq->lock only really needs to 
> be held during the stats update, it can be released during sprintf.

right, another simplification :)