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

["Rafael J. Wysocki" <rjw@sisk.pl>]

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.