diff mbox

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

Message ID 20151201065252.GD4459@ubuntu
State New
Headers show

Commit Message

Viresh Kumar Dec. 1, 2015, 6:52 a.m. UTC 
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:



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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Viresh Kumar Dec. 4, 2015, 8:23 a.m. UTC | #1 
On 01-12-15, 12:22, Viresh Kumar wrote:
> 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:


@Rafael: Stephen has gone for vacations until end of year :(, can you
please help with some reviews ?

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 8, 2015, 3:01 p.m. UTC | #2 
On 04-12-15, 13:53, Viresh Kumar wrote:
> On 01-12-15, 12:22, Viresh Kumar wrote:

> > 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:

> 

> @Rafael: Stephen has gone for vacations until end of year :(, can you

> please help with some reviews ?


Ping !!

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 8, 2015, 4:31 p.m. UTC | #3 
On 08-12-15, 17:50, Rafael J. Wysocki wrote:
> I don't remember that code clearly and it would take some time for me to

> recall it and then figure out the details about the changes.  Not impossible,

> but quite honestly I have other things to spend that time on.

> 

> Also, are the patches so urgent that they really can't wait for Stephen to get

> back?  I'd really prefer the discussion between you guys to settle without my

> intervention.


I am talking only about two patches, which are required to base other
work that Lee would be doing. I will resend the patches with all the
details in them. If Stephen finds something wrong about them, we can
get that fixed later as well.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Viresh Kumar Dec. 9, 2015, 2:34 a.m. UTC | #4 
On 09-12-15, 02:15, Rafael J. Wysocki wrote:
> If there are only the two patches, why don't you ask Lee to include them

> into the series he's working on?  The dependency would be clear then too.


His series was already out, and it was really about STi's cpufreq
driver and so I have resent my two patches again.

-- 
viresh
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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__);