Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1280600 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-01 08:00 +0100
    Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-04 09:30 +0100
      Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 16:10 +0100
        Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-08 17:30 +0100
          Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-08 17:40 +0100
            Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-09 01:50 +0100
              Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding Viresh Kumar <viresh.kumar@linaro.org> - 2015-12-09 03:40 +0100
                Re: [PATCH V2 2/3] PM / OPP: Parse 'opp-supported-hw' binding "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2015-12-09 22:40 +0100

#1280600 — 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1283628

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286567

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286651

On Tuesday, December 08, 2015 08:31:05 PM Viresh Kumar wrote:
> 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 !!

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.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286672

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1286939

On Tuesday, December 08, 2015 10:01:26 PM Viresh Kumar wrote:
> 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.

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.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287022

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1287884

On Wednesday, December 09, 2015 08:04:20 AM Viresh Kumar wrote:
> 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.

OK, I'll look at them shortly.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web