[PATCH] PCI / PM: Fix fallback to PCI_D0 in pci_platform_power_transition()

[PATCH] PCI / PM: Fix fallback to PCI_D0 in pci_platform_power_transition()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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 <chris.j.benenati@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: <stable@vger.kernel.org>
---
 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;

Re: [PATCH] PCI / PM: Fix fallback to PCI_D0 in pci_platform_power_transition()

[Yinghai Lu]

On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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 <chris.j.benenati@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: <stable@vger.kernel.org>
> ---
>  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 <yinghai@kernel.org>

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;
 }

Re: [PATCH] PCI / PM: Fix fallback to PCI_D0 in pci_platform_power_transition()

[Bjorn Helgaas]

On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> 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 <chris.j.benenati@intel.com>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> Cc: <stable@vger.kernel.org>

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 <yinghai@kernel.org>
>
> 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;
>  }

Re: [PATCH] PCI / PM: Fix fallback to PCI_D0 in pci_platform_power_transition()

[Rafael J. Wysocki]

On Monday, April 15, 2013 02:18:18 PM Bjorn Helgaas wrote:
> On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> > On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> 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 <chris.j.benenati@intel.com>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> Cc: <stable@vger.kernel.org>
> 
> 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 <yinghai@kernel.org>
> >
> > 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.

Re: [PATCH] PCI / PM: Fix fallback to PCI_D0 in pci_platform_power_transition()

[Bjorn Helgaas]

On Tue, Apr 16, 2013 at 12:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> On Monday, April 15, 2013 02:18:18 PM Bjorn Helgaas wrote:
>> On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <yinghai@kernel.org> wrote:
>> > On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >>
>> >> 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 <chris.j.benenati@intel.com>
>> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >> Cc: <stable@vger.kernel.org>
>>
>> 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 <yinghai@kernel.org>
>> >
>> > 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.

Re: [PATCH] PCI / PM: Fix fallback to PCI_D0 in pci_platform_power_transition()

[Rafael J. Wysocki]

On Tuesday, April 16, 2013 12:31:55 PM Bjorn Helgaas wrote:
> On Tue, Apr 16, 2013 at 12:01 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> > On Monday, April 15, 2013 02:18:18 PM Bjorn Helgaas wrote:
> >> On Fri, Apr 12, 2013 at 11:50 PM, Yinghai Lu <yinghai@kernel.org> wrote:
> >> > On Fri, Apr 12, 2013 at 4:58 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
> >> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >>
> >> >> 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 <chris.j.benenati@intel.com>
> >> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> >> Cc: <stable@vger.kernel.org>
> >>
> >> 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 <yinghai@kernel.org>
> >> >
> >> > 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.