From: Rafael J. Wysocki <[email protected]>
Commit b51306c (PCI: Set device power state to PCI_D0 for device
without native PM support) modified pci_platform_power_transition()
by adding code causing dev->current_state for devices that don't
support native PCI PM but are power-manageable by the platform to be
changed to PCI_D0 regardless of the value returned by the preceding
platform_pci_set_power_state(). In particular, that also is done
if the platform_pci_set_power_state() has been successful, which
causes the correct power state of the device set by
pci_update_current_state() in that case to be overwritten by PCI_D0.
Fix that mistake by making the fallback to PCI_D0 only happen if
the platform_pci_set_power_state() has returned an error.
Reported-by: Chris J. Benenati <[email protected]>
Signed-off-by: Rafael J. Wysocki <[email protected]>
Cc: <[email protected]>
---
drivers/pci/pci.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
Index: linux-pm/drivers/pci/pci.c
===================================================================
--- linux-pm.orig/drivers/pci/pci.c
+++ linux-pm/drivers/pci/pci.c
@@ -646,8 +646,7 @@ static int pci_platform_power_transition
error = platform_pci_set_power_state(dev, state);
if (!error)
pci_update_current_state(dev, state);
- /* Fall back to PCI_D0 if native PM is not supported */
- if (!dev->pm_cap)
+ else if (!dev->pm_cap) /* Fall back to PCI_D0 */
dev->current_state = PCI_D0;
} else {
error = -ENODEV;
On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <[email protected]> wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Commit b51306c (PCI: Set device power state to PCI_D0 for device
> without native PM support) modified pci_platform_power_transition()
> by adding code causing dev->current_state for devices that don't
> support native PCI PM but are power-manageable by the platform to be
> changed to PCI_D0 regardless of the value returned by the preceding
> platform_pci_set_power_state(). In particular, that also is done
> if the platform_pci_set_power_state() has been successful, which
> causes the correct power state of the device set by
> pci_update_current_state() in that case to be overwritten by PCI_D0.
>
> Fix that mistake by making the fallback to PCI_D0 only happen if
> the platform_pci_set_power_state() has returned an error.
>
> Reported-by: Chris J. Benenati <[email protected]>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> Cc: <[email protected]>
> ---
> drivers/pci/pci.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> Index: linux-pm/drivers/pci/pci.c
> ===================================================================
> --- linux-pm.orig/drivers/pci/pci.c
> +++ linux-pm/drivers/pci/pci.c
> @@ -646,8 +646,7 @@ static int pci_platform_power_transition
> error = platform_pci_set_power_state(dev, state);
> if (!error)
> pci_update_current_state(dev, state);
> - /* Fall back to PCI_D0 if native PM is not supported */
> - if (!dev->pm_cap)
> + else if (!dev->pm_cap) /* Fall back to PCI_D0 */
> dev->current_state = PCI_D0;
> } else {
> error = -ENODEV;
>
Acked-by: Yinghai Lu <[email protected]>
also could simplify it further.
---
drivers/pci/pci.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
Index: linux-2.6/drivers/pci/pci.c
===================================================================
--- linux-2.6.orig/drivers/pci/pci.c
+++ linux-2.6/drivers/pci/pci.c
@@ -646,15 +646,11 @@ static int pci_platform_power_transition
error = platform_pci_set_power_state(dev, state);
if (!error)
pci_update_current_state(dev, state);
- /* Fall back to PCI_D0 if native PM is not supported */
- if (!dev->pm_cap)
- dev->current_state = PCI_D0;
- } else {
+ } else
error = -ENODEV;
- /* Fall back to PCI_D0 if native PM is not supported */
- if (!dev->pm_cap)
- dev->current_state = PCI_D0;
- }
+
+ if (error && !dev->pm_cap) /* Fall back to PCI_D0 */
+ dev->current_state = PCI_D0;
return error;
}
On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <[email protected]> wrote:
> On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <[email protected]> wrote:
>> From: Rafael J. Wysocki <[email protected]>
>>
>> Commit b51306c (PCI: Set device power state to PCI_D0 for device
>> without native PM support) modified pci_platform_power_transition()
>> by adding code causing dev->current_state for devices that don't
>> support native PCI PM but are power-manageable by the platform to be
>> changed to PCI_D0 regardless of the value returned by the preceding
>> platform_pci_set_power_state(). In particular, that also is done
>> if the platform_pci_set_power_state() has been successful, which
>> causes the correct power state of the device set by
>> pci_update_current_state() in that case to be overwritten by PCI_D0.
>>
>> Fix that mistake by making the fallback to PCI_D0 only happen if
>> the platform_pci_set_power_state() has returned an error.
>>
>> Reported-by: Chris J. Benenati <[email protected]>
>> Signed-off-by: Rafael J. Wysocki <[email protected]>
>> Cc: <[email protected]>
I folded in Yinghai's simplification and added a URL to Chris's report
and applied to my pci/rafael-pm branch:
http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/rafael-pm
If it still looks good to you, Rafael, I'll merge it to next for v3.10.
I don't know what the user-visible effect of this is, and it's been
around for a long time, but if you think this should go in v3.9
instead, let me know.
Bjorn
>> ---
>> drivers/pci/pci.c | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> Index: linux-pm/drivers/pci/pci.c
>> ===================================================================
>> --- linux-pm.orig/drivers/pci/pci.c
>> +++ linux-pm/drivers/pci/pci.c
>> @@ -646,8 +646,7 @@ static int pci_platform_power_transition
>> error = platform_pci_set_power_state(dev, state);
>> if (!error)
>> pci_update_current_state(dev, state);
>> - /* Fall back to PCI_D0 if native PM is not supported */
>> - if (!dev->pm_cap)
>> + else if (!dev->pm_cap) /* Fall back to PCI_D0 */
>> dev->current_state = PCI_D0;
>> } else {
>> error = -ENODEV;
>>
>
> Acked-by: Yinghai Lu <[email protected]>
>
> also could simplify it further.
>
> ---
> drivers/pci/pci.c | 12 ++++--------
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> Index: linux-2.6/drivers/pci/pci.c
> ===================================================================
> --- linux-2.6.orig/drivers/pci/pci.c
> +++ linux-2.6/drivers/pci/pci.c
> @@ -646,15 +646,11 @@ static int pci_platform_power_transition
> error = platform_pci_set_power_state(dev, state);
> if (!error)
> pci_update_current_state(dev, state);
> - /* Fall back to PCI_D0 if native PM is not supported */
> - if (!dev->pm_cap)
> - dev->current_state = PCI_D0;
> - } else {
> + } else
> error = -ENODEV;
> - /* Fall back to PCI_D0 if native PM is not supported */
> - if (!dev->pm_cap)
> - dev->current_state = PCI_D0;
> - }
> +
> + if (error && !dev->pm_cap) /* Fall back to PCI_D0 */
> + dev->current_state = PCI_D0;
>
> return error;
> }
On Monday, April 15, 2013 02:18:18 PM Bjorn Helgaas wrote:
> On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <[email protected]> wrote:
> > On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <[email protected]> wrote:
> >> From: Rafael J. Wysocki <[email protected]>
> >>
> >> Commit b51306c (PCI: Set device power state to PCI_D0 for device
> >> without native PM support) modified pci_platform_power_transition()
> >> by adding code causing dev->current_state for devices that don't
> >> support native PCI PM but are power-manageable by the platform to be
> >> changed to PCI_D0 regardless of the value returned by the preceding
> >> platform_pci_set_power_state(). In particular, that also is done
> >> if the platform_pci_set_power_state() has been successful, which
> >> causes the correct power state of the device set by
> >> pci_update_current_state() in that case to be overwritten by PCI_D0.
> >>
> >> Fix that mistake by making the fallback to PCI_D0 only happen if
> >> the platform_pci_set_power_state() has returned an error.
> >>
> >> Reported-by: Chris J. Benenati <[email protected]>
> >> Signed-off-by: Rafael J. Wysocki <[email protected]>
> >> Cc: <[email protected]>
>
> I folded in Yinghai's simplification and added a URL to Chris's report
> and applied to my pci/rafael-pm branch:
> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/rafael-pm
>
> If it still looks good to you, Rafael, I'll merge it to next for v3.10.
Well, the simplification adds an extra check for error into both code paths,
so I'm not sure if it's really worth it (that's why I didn't do it originally
in the first place), but I suppose that's not a big deal. It looks good as far
as the correctness is concerned anyway.
> I don't know what the user-visible effect of this is, and it's been
> around for a long time, but if you think this should go in v3.9
> instead, let me know.
I think v3.9+stable should be sufficient.
Thanks,
Rafael
> >> ---
> >> drivers/pci/pci.c | 3 +--
> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >>
> >> Index: linux-pm/drivers/pci/pci.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/pci/pci.c
> >> +++ linux-pm/drivers/pci/pci.c
> >> @@ -646,8 +646,7 @@ static int pci_platform_power_transition
> >> error = platform_pci_set_power_state(dev, state);
> >> if (!error)
> >> pci_update_current_state(dev, state);
> >> - /* Fall back to PCI_D0 if native PM is not supported */
> >> - if (!dev->pm_cap)
> >> + else if (!dev->pm_cap) /* Fall back to PCI_D0 */
> >> dev->current_state = PCI_D0;
> >> } else {
> >> error = -ENODEV;
> >>
> >
> > Acked-by: Yinghai Lu <[email protected]>
> >
> > also could simplify it further.
> >
> > ---
> > drivers/pci/pci.c | 12 ++++--------
> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >
> > Index: linux-2.6/drivers/pci/pci.c
> > ===================================================================
> > --- linux-2.6.orig/drivers/pci/pci.c
> > +++ linux-2.6/drivers/pci/pci.c
> > @@ -646,15 +646,11 @@ static int pci_platform_power_transition
> > error = platform_pci_set_power_state(dev, state);
> > if (!error)
> > pci_update_current_state(dev, state);
> > - /* Fall back to PCI_D0 if native PM is not supported */
> > - if (!dev->pm_cap)
> > - dev->current_state = PCI_D0;
> > - } else {
> > + } else
> > error = -ENODEV;
> > - /* Fall back to PCI_D0 if native PM is not supported */
> > - if (!dev->pm_cap)
> > - dev->current_state = PCI_D0;
> > - }
> > +
> > + if (error && !dev->pm_cap) /* Fall back to PCI_D0 */
> > + dev->current_state = PCI_D0;
> >
> > return error;
> > }
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
On Tue, Apr 16, 2013 at 12:01 PM, Rafael J. Wysocki <[email protected]> wrote:
> On Monday, April 15, 2013 02:18:18 PM Bjorn Helgaas wrote:
>> On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <[email protected]> wrote:
>> > On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <[email protected]> wrote:
>> >> From: Rafael J. Wysocki <[email protected]>
>> >>
>> >> Commit b51306c (PCI: Set device power state to PCI_D0 for device
>> >> without native PM support) modified pci_platform_power_transition()
>> >> by adding code causing dev->current_state for devices that don't
>> >> support native PCI PM but are power-manageable by the platform to be
>> >> changed to PCI_D0 regardless of the value returned by the preceding
>> >> platform_pci_set_power_state(). In particular, that also is done
>> >> if the platform_pci_set_power_state() has been successful, which
>> >> causes the correct power state of the device set by
>> >> pci_update_current_state() in that case to be overwritten by PCI_D0.
>> >>
>> >> Fix that mistake by making the fallback to PCI_D0 only happen if
>> >> the platform_pci_set_power_state() has returned an error.
>> >>
>> >> Reported-by: Chris J. Benenati <[email protected]>
>> >> Signed-off-by: Rafael J. Wysocki <[email protected]>
>> >> Cc: <[email protected]>
>>
>> I folded in Yinghai's simplification and added a URL to Chris's report
>> and applied to my pci/rafael-pm branch:
>> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/rafael-pm
>>
>> If it still looks good to you, Rafael, I'll merge it to next for v3.10.
>
> Well, the simplification adds an extra check for error into both code paths,
> so I'm not sure if it's really worth it (that's why I didn't do it originally
> in the first place), but I suppose that's not a big deal. It looks good as far
> as the correctness is concerned anyway.
>
>> I don't know what the user-visible effect of this is, and it's been
>> around for a long time, but if you think this should go in v3.9
>> instead, let me know.
>
> I think v3.9+stable should be sufficient.
It sounds like you meant to say "v3.10+stable should be sufficient".
Is that right, or do you want me to push this to Linus before v3.9 is
released?
>> >> ---
>> >> drivers/pci/pci.c | 3 +--
>> >> 1 file changed, 1 insertion(+), 2 deletions(-)
>> >>
>> >> Index: linux-pm/drivers/pci/pci.c
>> >> ===================================================================
>> >> --- linux-pm.orig/drivers/pci/pci.c
>> >> +++ linux-pm/drivers/pci/pci.c
>> >> @@ -646,8 +646,7 @@ static int pci_platform_power_transition
>> >> error = platform_pci_set_power_state(dev, state);
>> >> if (!error)
>> >> pci_update_current_state(dev, state);
>> >> - /* Fall back to PCI_D0 if native PM is not supported */
>> >> - if (!dev->pm_cap)
>> >> + else if (!dev->pm_cap) /* Fall back to PCI_D0 */
>> >> dev->current_state = PCI_D0;
>> >> } else {
>> >> error = -ENODEV;
>> >>
>> >
>> > Acked-by: Yinghai Lu <[email protected]>
>> >
>> > also could simplify it further.
>> >
>> > ---
>> > drivers/pci/pci.c | 12 ++++--------
>> > 1 file changed, 4 insertions(+), 8 deletions(-)
>> >
>> > Index: linux-2.6/drivers/pci/pci.c
>> > ===================================================================
>> > --- linux-2.6.orig/drivers/pci/pci.c
>> > +++ linux-2.6/drivers/pci/pci.c
>> > @@ -646,15 +646,11 @@ static int pci_platform_power_transition
>> > error = platform_pci_set_power_state(dev, state);
>> > if (!error)
>> > pci_update_current_state(dev, state);
>> > - /* Fall back to PCI_D0 if native PM is not supported */
>> > - if (!dev->pm_cap)
>> > - dev->current_state = PCI_D0;
>> > - } else {
>> > + } else
>> > error = -ENODEV;
>> > - /* Fall back to PCI_D0 if native PM is not supported */
>> > - if (!dev->pm_cap)
>> > - dev->current_state = PCI_D0;
>> > - }
>> > +
>> > + if (error && !dev->pm_cap) /* Fall back to PCI_D0 */
>> > + dev->current_state = PCI_D0;
>> >
>> > return error;
>> > }
> --
> I speak only for myself.
> Rafael J. Wysocki, Intel Open Source Technology Center.
On Tuesday, April 16, 2013 12:31:55 PM Bjorn Helgaas wrote:
> On Tue, Apr 16, 2013 at 12:01 PM, Rafael J. Wysocki <[email protected]> wrote:
> > On Monday, April 15, 2013 02:18:18 PM Bjorn Helgaas wrote:
> >> On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <[email protected]> wrote:
> >> > On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <[email protected]> wrote:
> >> >> From: Rafael J. Wysocki <[email protected]>
> >> >>
> >> >> Commit b51306c (PCI: Set device power state to PCI_D0 for device
> >> >> without native PM support) modified pci_platform_power_transition()
> >> >> by adding code causing dev->current_state for devices that don't
> >> >> support native PCI PM but are power-manageable by the platform to be
> >> >> changed to PCI_D0 regardless of the value returned by the preceding
> >> >> platform_pci_set_power_state(). In particular, that also is done
> >> >> if the platform_pci_set_power_state() has been successful, which
> >> >> causes the correct power state of the device set by
> >> >> pci_update_current_state() in that case to be overwritten by PCI_D0.
> >> >>
> >> >> Fix that mistake by making the fallback to PCI_D0 only happen if
> >> >> the platform_pci_set_power_state() has returned an error.
> >> >>
> >> >> Reported-by: Chris J. Benenati <[email protected]>
> >> >> Signed-off-by: Rafael J. Wysocki <[email protected]>
> >> >> Cc: <[email protected]>
> >>
> >> I folded in Yinghai's simplification and added a URL to Chris's report
> >> and applied to my pci/rafael-pm branch:
> >> http://git.kernel.org/cgit/linux/kernel/git/helgaas/pci.git/log/?h=pci/rafael-pm
> >>
> >> If it still looks good to you, Rafael, I'll merge it to next for v3.10.
> >
> > Well, the simplification adds an extra check for error into both code paths,
> > so I'm not sure if it's really worth it (that's why I didn't do it originally
> > in the first place), but I suppose that's not a big deal. It looks good as far
> > as the correctness is concerned anyway.
> >
> >> I don't know what the user-visible effect of this is, and it's been
> >> around for a long time, but if you think this should go in v3.9
> >> instead, let me know.
> >
> > I think v3.9+stable should be sufficient.
>
> It sounds like you meant to say "v3.10+stable should be sufficient".
> Is that right, or do you want me to push this to Linus before v3.9 is
> released?
Yes, I meant v3.10+stable. Sorry.
Thanks,
Rafael
> >> >> ---
> >> >> drivers/pci/pci.c | 3 +--
> >> >> 1 file changed, 1 insertion(+), 2 deletions(-)
> >> >>
> >> >> Index: linux-pm/drivers/pci/pci.c
> >> >> ===================================================================
> >> >> --- linux-pm.orig/drivers/pci/pci.c
> >> >> +++ linux-pm/drivers/pci/pci.c
> >> >> @@ -646,8 +646,7 @@ static int pci_platform_power_transition
> >> >> error = platform_pci_set_power_state(dev, state);
> >> >> if (!error)
> >> >> pci_update_current_state(dev, state);
> >> >> - /* Fall back to PCI_D0 if native PM is not supported */
> >> >> - if (!dev->pm_cap)
> >> >> + else if (!dev->pm_cap) /* Fall back to PCI_D0 */
> >> >> dev->current_state = PCI_D0;
> >> >> } else {
> >> >> error = -ENODEV;
> >> >>
> >> >
> >> > Acked-by: Yinghai Lu <[email protected]>
> >> >
> >> > also could simplify it further.
> >> >
> >> > ---
> >> > drivers/pci/pci.c | 12 ++++--------
> >> > 1 file changed, 4 insertions(+), 8 deletions(-)
> >> >
> >> > Index: linux-2.6/drivers/pci/pci.c
> >> > ===================================================================
> >> > --- linux-2.6.orig/drivers/pci/pci.c
> >> > +++ linux-2.6/drivers/pci/pci.c
> >> > @@ -646,15 +646,11 @@ static int pci_platform_power_transition
> >> > error = platform_pci_set_power_state(dev, state);
> >> > if (!error)
> >> > pci_update_current_state(dev, state);
> >> > - /* Fall back to PCI_D0 if native PM is not supported */
> >> > - if (!dev->pm_cap)
> >> > - dev->current_state = PCI_D0;
> >> > - } else {
> >> > + } else
> >> > error = -ENODEV;
> >> > - /* Fall back to PCI_D0 if native PM is not supported */
> >> > - if (!dev->pm_cap)
> >> > - dev->current_state = PCI_D0;
> >> > - }
> >> > +
> >> > + if (error && !dev->pm_cap) /* Fall back to PCI_D0 */
> >> > + dev->current_state = PCI_D0;
> >> >
> >> > return error;
> >> > }
> > --
> > I speak only for myself.
> > Rafael J. Wysocki, Intel Open Source Technology Center.
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.