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

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(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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;
>  }
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.