lkml.org 
[lkml]   [2015]   [Dec]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2015-12-01 08:01    [W:0.126 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site