Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding

[Viresh Kumar <viresh.kumar@linaro.org>]

On 25-11-15, 12:51, Stephen Boyd wrote:
> > +int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions,
> > +				unsigned int count)
> > +{
> > +	struct device_opp *dev_opp;
> > +	int ret = 0;
> > +
> > +	/* Hold our list modification lock here */
> > +	mutex_lock(&dev_opp_list_lock);
> > +
> > +	dev_opp = _add_device_opp(dev);
> 
> So this function will publish an opp to the list...

Not an opp but opp-dev.

> > +	if (!dev_opp) {
> > +		ret = -ENOMEM;
> > +		goto unlock;
> > +	}
> > +
> > +	/* Do we already have a version hierarchy associated with dev_opp? */
> > +	if (dev_opp->supported_hw) {
> > +		dev_err(dev, "%s: Already have supported hardware list\n",
> > +			__func__);
> > +		ret = -EBUSY;
> > +		goto err;
> > +	}
> > +
> > +	dev_opp->supported_hw = kmemdup(versions, count * sizeof(*versions),
> > +					GFP_KERNEL);
> 
> And then we're going to modify said opp here under the mutex
> lock.

opp-dev ..

> > +	if (!dev_opp->supported_hw) {
> > +		ret = -ENOMEM;
> > +		goto err;
> > +	}
> > +
> > +	dev_opp->supported_hw_count = count;
> 
> So we've properly handled the concurrent writer case (which is
> probably not even real), but we have improperly handled the case
> where a reader is running in parallel to the writer. We should
> only list_add_rcu the pointer once we're done modifying the
> pointer we created. Otherwise a reader can come along and see the
> half initialized structure, which is not good.

This function will be called, from some platform code, before the OPP
table is initialized. It isn't useful to call it after the OPPs are
added for the device. So there wouldn't be any concurrent reader.

> I'm worried that the RCU locking is messed up in other places in
> this file now too. Hopefully not, but it's going to require an
> audit.

I do share your concern.. But this stuff should be okay. Even the
other patch as well.

-- 
viresh