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.