| Message ID | 1438731339-58317-3-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Lina Iyer <lina.iyer@linaro.org> writes: > Remove check for driver of a device, for runtime PM. Device may be > suspended without an explicit driver. This check seems to be vestigial > and incorrect in the current context. This one should probably have been RFC. For a little more context here, this was uncovered when experimenting with using runtime PM for CPU devices which don't have a dev->driver. This check might have made sense before PM domains, but with PM domains, it's entirely possible to have a simple device without a driver and the PM domain handles all the necesary PM, so I think this check could/should be removed. Thoughts? Kevin > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Signed-off-by: Kevin Hilman <khilman@linaro.org> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > drivers/base/power/domain.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 5fd1306..ef8d19f 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -394,8 +394,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd) > if (stat > PM_QOS_FLAGS_NONE) > return -EBUSY; > > - if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev) > - || pdd->dev->power.irq_safe)) > + if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe) > not_suspended++; > }
Hi Kevin, On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote: > Lina Iyer <lina.iyer@linaro.org> writes: > >> Remove check for driver of a device, for runtime PM. Device may be >> suspended without an explicit driver. This check seems to be vestigial >> and incorrect in the current context. > > This one should probably have been RFC. > > For a little more context here, this was uncovered when experimenting > with using runtime PM for CPU devices which don't have a dev->driver. > > This check might have made sense before PM domains, but with PM domains, > it's entirely possible to have a simple device without a driver and the > PM domain handles all the necesary PM, so I think this check > could/should be removed. > > Thoughts? Simple devices without a driver aren't handled automatically. At minimum, the driver should call pm_runtime_enable(), cfr. drivers/bus/simple-pm-bus.c. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Geert Uytterhoeven <geert@linux-m68k.org> writes: > Hi Kevin, > > On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote: >> Lina Iyer <lina.iyer@linaro.org> writes: >> >>> Remove check for driver of a device, for runtime PM. Device may be >>> suspended without an explicit driver. This check seems to be vestigial >>> and incorrect in the current context. >> >> This one should probably have been RFC. >> >> For a little more context here, this was uncovered when experimenting >> with using runtime PM for CPU devices which don't have a dev->driver. >> >> This check might have made sense before PM domains, but with PM domains, >> it's entirely possible to have a simple device without a driver and the >> PM domain handles all the necesary PM, so I think this check >> could/should be removed. >> >> Thoughts? > > Simple devices without a driver aren't handled automatically. > At minimum, the driver should call pm_runtime_enable(), cfr. > drivers/bus/simple-pm-bus.c. That's correct, and in the proof-of-concept stuff I hacked up and in Lina's series, the CPU "devices" do indeed to this. Without that, they wouldn't end up ever taking this codepath through genpd's runtime_suspend and power_off hooks. Also, I'm not sure if your comment was meant to be an objection to the patch? or if you're OK with it. Thanks for the feedback, Kevin
Hi Kevin, On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote: > Geert Uytterhoeven <geert@linux-m68k.org> writes: >> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote: >>> This check might have made sense before PM domains, but with PM domains, >>> it's entirely possible to have a simple device without a driver and the >>> PM domain handles all the necesary PM, so I think this check >>> could/should be removed. >>> >>> Thoughts? >> >> Simple devices without a driver aren't handled automatically. >> At minimum, the driver should call pm_runtime_enable(), cfr. >> drivers/bus/simple-pm-bus.c. > > That's correct, and in the proof-of-concept stuff I hacked up and in > Lina's series, the CPU "devices" do indeed to this. Without that, they > wouldn't end up ever taking this codepath through genpd's > runtime_suspend and power_off hooks. > > Also, I'm not sure if your comment was meant to be an objection to the > patch? or if you're OK with it. My comment was purely meant as a response to "it's entirely possible to have a simple device without a driver and the PM domain handles all the necesary PM". I have no objections to the patch (read: I'm not sufficiently familiar with the code to make educated guesses about the side effects of this change ;-). Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Kevin, > > On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote: >> Geert Uytterhoeven <geert@linux-m68k.org> writes: >>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote: >>>> This check might have made sense before PM domains, but with PM domains, >>>> it's entirely possible to have a simple device without a driver and the >>>> PM domain handles all the necesary PM, so I think this check >>>> could/should be removed. >>>> >>>> Thoughts? >>> >>> Simple devices without a driver aren't handled automatically. >>> At minimum, the driver should call pm_runtime_enable(), cfr. >>> drivers/bus/simple-pm-bus.c. >> >> That's correct, and in the proof-of-concept stuff I hacked up and in >> Lina's series, the CPU "devices" do indeed to this. Without that, they >> wouldn't end up ever taking this codepath through genpd's >> runtime_suspend and power_off hooks. >> >> Also, I'm not sure if your comment was meant to be an objection to the >> patch? or if you're OK with it. > > My comment was purely meant as a response to "it's entirely possible to have a > simple device without a driver and the PM domain handles all the necesary PM". Right, so if the PM domain does the pm_runtime_enable() for these "simple" devices without drivers, they can still exist without a driver, and the PM domain doing all the magic. > I have no objections to the patch (read: I'm not sufficiently familiar with > the code to make educated guesses about the side effects of this change ;-). OK, thanks for clarifiying. Kevin
Hi Kevin, On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote: > On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote: >>> Geert Uytterhoeven <geert@linux-m68k.org> writes: >>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote: >>>>> This check might have made sense before PM domains, but with PM domains, >>>>> it's entirely possible to have a simple device without a driver and the >>>>> PM domain handles all the necesary PM, so I think this check >>>>> could/should be removed. >>>>> >>>>> Thoughts? >>>> >>>> Simple devices without a driver aren't handled automatically. >>>> At minimum, the driver should call pm_runtime_enable(), cfr. >>>> drivers/bus/simple-pm-bus.c. >>> >>> That's correct, and in the proof-of-concept stuff I hacked up and in >>> Lina's series, the CPU "devices" do indeed to this. Without that, they >>> wouldn't end up ever taking this codepath through genpd's >>> runtime_suspend and power_off hooks. >>> >>> Also, I'm not sure if your comment was meant to be an objection to the >>> patch? or if you're OK with it. >> >> My comment was purely meant as a response to "it's entirely possible to have a >> simple device without a driver and the PM domain handles all the necesary PM". > > Right, so if the PM domain does the pm_runtime_enable() for these > "simple" devices without drivers, they can still exist without a > driver, and the PM domain doing all the magic. Is it possible to let the PM Domain do the pm_runtime_enable() itself in the absence of a driver? If yes, I wouldn't have needed simple-pm-bus.c. What if a driver is bound later? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Geert Uytterhoeven <geert@linux-m68k.org> writes: > Hi Kevin, > > On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote: >> On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote: >>>> Geert Uytterhoeven <geert@linux-m68k.org> writes: >>>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote: >>>>>> This check might have made sense before PM domains, but with PM domains, >>>>>> it's entirely possible to have a simple device without a driver and the >>>>>> PM domain handles all the necesary PM, so I think this check >>>>>> could/should be removed. >>>>>> >>>>>> Thoughts? >>>>> >>>>> Simple devices without a driver aren't handled automatically. >>>>> At minimum, the driver should call pm_runtime_enable(), cfr. >>>>> drivers/bus/simple-pm-bus.c. >>>> >>>> That's correct, and in the proof-of-concept stuff I hacked up and in >>>> Lina's series, the CPU "devices" do indeed to this. Without that, they >>>> wouldn't end up ever taking this codepath through genpd's >>>> runtime_suspend and power_off hooks. >>>> >>>> Also, I'm not sure if your comment was meant to be an objection to the >>>> patch? or if you're OK with it. >>> >>> My comment was purely meant as a response to "it's entirely possible to have a >>> simple device without a driver and the PM domain handles all the necesary PM". >> >> Right, so if the PM domain does the pm_runtime_enable() for these >> "simple" devices without drivers, they can still exist without a >> driver, and the PM domain doing all the magic. > > Is it possible to let the PM Domain do the pm_runtime_enable() itself in > the absence of a driver? Well, I suppose it's possible, not sure it's recommended. :) > If yes, I wouldn't have needed simple-pm-bus.c. > What if a driver is bound later? Yeah, you're approach is better. Kevin
On Fri, Aug 21 2015 at 15:04 -0600, Kevin Hilman wrote: >Geert Uytterhoeven <geert@linux-m68k.org> writes: > >> Hi Kevin, >> >> On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote: >>> On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>>> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> wrote: >>>>> Geert Uytterhoeven <geert@linux-m68k.org> writes: >>>>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> wrote: >>>>>>> This check might have made sense before PM domains, but with PM domains, >>>>>>> it's entirely possible to have a simple device without a driver and the >>>>>>> PM domain handles all the necesary PM, so I think this check >>>>>>> could/should be removed. >>>>>>> >>>>>>> Thoughts? >>>>>> >>>>>> Simple devices without a driver aren't handled automatically. >>>>>> At minimum, the driver should call pm_runtime_enable(), cfr. >>>>>> drivers/bus/simple-pm-bus.c. >>>>> >>>>> That's correct, and in the proof-of-concept stuff I hacked up and in >>>>> Lina's series, the CPU "devices" do indeed to this. Without that, they >>>>> wouldn't end up ever taking this codepath through genpd's >>>>> runtime_suspend and power_off hooks. >>>>> >>>>> Also, I'm not sure if your comment was meant to be an objection to the >>>>> patch? or if you're OK with it. >>>> >>>> My comment was purely meant as a response to "it's entirely possible to have a >>>> simple device without a driver and the PM domain handles all the necesary PM". >>> >>> Right, so if the PM domain does the pm_runtime_enable() for these >>> "simple" devices without drivers, they can still exist without a >>> driver, and the PM domain doing all the magic. >> >> Is it possible to let the PM Domain do the pm_runtime_enable() itself in >> the absence of a driver? > >Well, I suppose it's possible, not sure it's recommended. :) > >> If yes, I wouldn't have needed simple-pm-bus.c. >> What if a driver is bound later? > >Yeah, you're approach is better. > I am not sure I understand the approach? Initialize the CPU devices as "simple-pm-bus" compatible? Thanks, Lina
Hi Lina, On Mon, Aug 24, 2015 at 9:50 PM, Lina Iyer <lina.iyer@linaro.org> wrote: > On Fri, Aug 21 2015 at 15:04 -0600, Kevin Hilman wrote: >> Geert Uytterhoeven <geert@linux-m68k.org> writes: >>> On Fri, Aug 14, 2015 at 7:19 PM, Kevin Hilman <khilman@kernel.org> wrote: >>>> On Fri, Aug 14, 2015 at 12:24 AM, Geert Uytterhoeven >>>> <geert@linux-m68k.org> wrote: >>>>> On Fri, Aug 14, 2015 at 5:40 AM, Kevin Hilman <khilman@kernel.org> >>>>> wrote: >>>>>> Geert Uytterhoeven <geert@linux-m68k.org> writes: >>>>>>> On Wed, Aug 12, 2015 at 9:50 PM, Kevin Hilman <khilman@kernel.org> >>>>>>> wrote: >>>>>>>> This check might have made sense before PM domains, but with PM >>>>>>>> domains, >>>>>>>> it's entirely possible to have a simple device without a driver and >>>>>>>> the >>>>>>>> PM domain handles all the necesary PM, so I think this check >>>>>>>> could/should be removed. >>>>>>>> >>>>>>>> Thoughts? >>>>>>> >>>>>>> Simple devices without a driver aren't handled automatically. >>>>>>> At minimum, the driver should call pm_runtime_enable(), cfr. >>>>>>> drivers/bus/simple-pm-bus.c. >>>>>> >>>>>> That's correct, and in the proof-of-concept stuff I hacked up and in >>>>>> Lina's series, the CPU "devices" do indeed to this. Without that, >>>>>> they >>>>>> wouldn't end up ever taking this codepath through genpd's >>>>>> runtime_suspend and power_off hooks. >>>>>> >>>>>> Also, I'm not sure if your comment was meant to be an objection to the >>>>>> patch? or if you're OK with it. >>>>> >>>>> My comment was purely meant as a response to "it's entirely possible to >>>>> have a >>>>> simple device without a driver and the PM domain handles all the >>>>> necesary PM". >>>> >>>> Right, so if the PM domain does the pm_runtime_enable() for these >>>> "simple" devices without drivers, they can still exist without a >>>> driver, and the PM domain doing all the magic. >>> >>> Is it possible to let the PM Domain do the pm_runtime_enable() itself in >>> the absence of a driver? >> >> Well, I suppose it's possible, not sure it's recommended. :) >> >>> If yes, I wouldn't have needed simple-pm-bus.c. >>> What if a driver is bound later? >> >> Yeah, you're approach is better. >> > I am not sure I understand the approach? Initialize the CPU devices as > "simple-pm-bus" compatible? No, Kevin and I are discussing about transparent buses that need power management, not about CPU devices. Cfr. commit 89d463ea106dba53 ("drivers: bus: Add Simple Power-Managed Bus Driver"). Sorry for the confusion... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 5 August 2015 at 01:35, Lina Iyer <lina.iyer@linaro.org> wrote: > Remove check for driver of a device, for runtime PM. Device may be > suspended without an explicit driver. This check seems to be vestigial > and incorrect in the current context. > > Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com> > Signed-off-by: Kevin Hilman <khilman@linaro.org> > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> I think this makes perfect sense! Moreover in *theory*, I have considered both cases for how a device currently gets attached/detached to its genpd and I can't find any issues with $subject patch. With *both* cases I mean attaching the device using *pm_genpd_[name]add_device() API or via the ->probe() sequence using the dev_pm_domain_attach() API. Now, of course that statement also relies on that drivers behaves properly from a runtime PM point of view, for example make sure to do pm_runtime_disable() at ->remove(). Acked-by: Ulf Hansson <ulf.hansson@linaro.org> Kind regards Uffe > --- > drivers/base/power/domain.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 5fd1306..ef8d19f 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -394,8 +394,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd) > if (stat > PM_QOS_FLAGS_NONE) > return -EBUSY; > > - if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev) > - || pdd->dev->power.irq_safe)) > + if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe) > not_suspended++; > } > > -- > 2.1.4 >
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 5fd1306..ef8d19f 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -394,8 +394,7 @@ static int pm_genpd_poweroff(struct generic_pm_domain *genpd) if (stat > PM_QOS_FLAGS_NONE) return -EBUSY; - if (pdd->dev->driver && (!pm_runtime_suspended(pdd->dev) - || pdd->dev->power.irq_safe)) + if (!pm_runtime_suspended(pdd->dev) || pdd->dev->power.irq_safe) not_suspended++; }