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

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

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.