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.