Messages in this thread Patch in this message | | | Date | Tue, 1 Dec 2015 12:22:52 +0530 | From | Viresh Kumar <> | Subject | Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding |
| |
On 27-11-15, 10:15, Viresh Kumar wrote: > > > + 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.
Since these functions are *only* going to be called before any OPPs are added for the device, and hence ruling out any concurrent readers, maybe we can guarantee that with this:
diff --git a/drivers/base/power/opp/core.c b/drivers/base/power/opp/core.c index 5449bae74a44..ec74d98afe75 100644 --- a/drivers/base/power/opp/core.c +++ b/drivers/base/power/opp/core.c @@ -876,6 +876,9 @@ int dev_pm_opp_set_supported_hw(struct device *dev, const u32 *versions, goto err; } + /* Make sure there are no concurrent readers while updating dev_opp */ + WARN_ON(!list_empty(&dev_opp->opp_list)); + dev_opp->supported_hw = kmemdup(versions, count * sizeof(*versions), GFP_KERNEL); if (!dev_opp->supported_hw) { @@ -924,6 +927,9 @@ void dev_pm_opp_put_supported_hw(struct device *dev) goto unlock; } + /* Make sure there are no concurrent readers while updating dev_opp */ + WARN_ON(!list_empty(&dev_opp->opp_list)); + if (!dev_opp->supported_hw) { dev_err(dev, "%s: Doesn't have supported hardware list\n", __func__);
I don't really want to create a duplicate dev_opp here and then replace that in the list, because we know that we have just created it.
Over that, if a reference to dev_opp is used somewhere else, lets say within the pm_opp structure, then updating all OPPs at such times would be really hard.
Lets close this before you go for vacations. I will get whatever solution you feel is right.
-- viresh
| |