Potential issue with pci_prepare_to_sleep if there's no platform support for D3cold transition

Potential issue with pci_prepare_to_sleep if there's no platform support for D3cold transition

[Heiner Kallweit]

If there's no platform support for transition to D3cold, then
pci_set_power_state(dev, D3cold) still returns 0, even though
power state is transitioned to D3hot only. We called
pci_enable_wake(dev, D3cold, wakeup) before, therefore PME for
D3hot may not be enabled. Is this a bug?

Background:
In __pci_set_power_state we have the following:

error = pci_set_low_power_state(dev, PCI_D3hot, locked);
if (pci_platform_power_transition(dev, PCI_D3cold))
	return error;

The acpi_pci_set_power_state() stub returns -ENODEV.
Therefore, if error=0,  __pci_set_power_state() will
return 0 if pci_platform_power_transition() fails.

Re: Potential issue with pci_prepare_to_sleep if there's no platform support for D3cold transition

[Lukas Wunner]

[cc += Rafael, linux-acpi]

On Mon, Apr 21, 2025 at 10:05:59PM +0200, Heiner Kallweit wrote:
> If there's no platform support for transition to D3cold, then
> pci_set_power_state(dev, D3cold) still returns 0, even though
> power state is transitioned to D3hot only. We called
> pci_enable_wake(dev, D3cold, wakeup) before, therefore PME for
> D3hot may not be enabled. Is this a bug?
> 
> Background:
> In __pci_set_power_state we have the following:
> 
> error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> if (pci_platform_power_transition(dev, PCI_D3cold))
> 	return error;
> 
> The acpi_pci_set_power_state() stub returns -ENODEV.
> Therefore, if error=0,  __pci_set_power_state() will
> return 0 if pci_platform_power_transition() fails.

pci_prepare_to_sleep() calls pci_target_state() right at the top.

If wakeup is supported and enabled, pci_target_state() is supposed
to find the deepest power state supporting wakeup.  If D3cold doesn't
support wakeup, D3hot or a shallower state is returned.

Hence I don't quite understand how the scenario you're describing
could occur in practice.  Are you seeing actual issues and have tracked
them down to incorrect handling in pci_prepare_to_sleep()?

Thanks,

Lukas

Re: Potential issue with pci_prepare_to_sleep if there's no platform support for D3cold transition

[Rafael J. Wysocki]

On Tue, Apr 22, 2025 at 8:28 AM Lukas Wunner <lukas@wunner.de> wrote:
>
> [cc += Rafael, linux-acpi]
>
> On Mon, Apr 21, 2025 at 10:05:59PM +0200, Heiner Kallweit wrote:
> > If there's no platform support for transition to D3cold, then
> > pci_set_power_state(dev, D3cold) still returns 0, even though
> > power state is transitioned to D3hot only. We called
> > pci_enable_wake(dev, D3cold, wakeup) before, therefore PME for
> > D3hot may not be enabled. Is this a bug?

On platforms using ACPI, no it isn't.

Internally, pci_enable_wake() evaluates _DSW and it doesn't
distinguish between D3hot and D3cold as per the spec.

> > Background:
> > In __pci_set_power_state we have the following:
> >
> > error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > if (pci_platform_power_transition(dev, PCI_D3cold))
> >       return error;
> >
> > The acpi_pci_set_power_state() stub returns -ENODEV.
> > Therefore, if error=0,  __pci_set_power_state() will
> > return 0 if pci_platform_power_transition() fails.
>
> pci_prepare_to_sleep() calls pci_target_state() right at the top.
>
> If wakeup is supported and enabled, pci_target_state() is supposed
> to find the deepest power state supporting wakeup.  If D3cold doesn't
> support wakeup, D3hot or a shallower state is returned.
>
> Hence I don't quite understand how the scenario you're describing
> could occur in practice.  Are you seeing actual issues and have tracked
> them down to incorrect handling in pci_prepare_to_sleep()?

Re: Potential issue with pci_prepare_to_sleep if there's no platform support for D3cold transition

[Heiner Kallweit]

On Tue, Apr 22, 2025 at 1:43 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Apr 22, 2025 at 8:28 AM Lukas Wunner <lukas@wunner.de> wrote:
> >
> > [cc += Rafael, linux-acpi]
> >
> > On Mon, Apr 21, 2025 at 10:05:59PM +0200, Heiner Kallweit wrote:
> > > If there's no platform support for transition to D3cold, then
> > > pci_set_power_state(dev, D3cold) still returns 0, even though
> > > power state is transitioned to D3hot only. We called
> > > pci_enable_wake(dev, D3cold, wakeup) before, therefore PME for
> > > D3hot may not be enabled. Is this a bug?
>
> On platforms using ACPI, no it isn't.
>
> Internally, pci_enable_wake() evaluates _DSW and it doesn't
> distinguish between D3hot and D3cold as per the spec.
>
> > > Background:
> > > In __pci_set_power_state we have the following:
> > >
> > > error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > > if (pci_platform_power_transition(dev, PCI_D3cold))
> > >       return error;
> > >
> > > The acpi_pci_set_power_state() stub returns -ENODEV.
> > > Therefore, if error=0,  __pci_set_power_state() will
> > > return 0 if pci_platform_power_transition() fails.
> >
> > pci_prepare_to_sleep() calls pci_target_state() right at the top.
> >
> > If wakeup is supported and enabled, pci_target_state() is supposed
> > to find the deepest power state supporting wakeup.  If D3cold doesn't
> > support wakeup, D3hot or a shallower state is returned.
> >
> > Hence I don't quite understand how the scenario you're describing
> > could occur in practice.  Are you seeing actual issues and have tracked
> > them down to incorrect handling in pci_prepare_to_sleep()?

On non-ACPI systems pci_target_state() just looks at the device
capabilities. The described scenario can happen if device supports
wake from D3cold, but platform has no means to switch a device to
D3cold. I'm thinking of e.g. RTL8125 on ARM. Typically I don't expect
an issue because boot-up default on these devices is to have PME wake
from all states enabled. So it's more of a theoretical exercise at the
moment.

Re: Potential issue with pci_prepare_to_sleep if there's no platform support for D3cold transition

[Heiner Kallweit]

On Wed, Apr 23, 2025 at 11:32 AM Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On Tue, Apr 22, 2025 at 1:43 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Apr 22, 2025 at 8:28 AM Lukas Wunner <lukas@wunner.de> wrote:
> > >
> > > [cc += Rafael, linux-acpi]
> > >
> > > On Mon, Apr 21, 2025 at 10:05:59PM +0200, Heiner Kallweit wrote:
> > > > If there's no platform support for transition to D3cold, then
> > > > pci_set_power_state(dev, D3cold) still returns 0, even though
> > > > power state is transitioned to D3hot only. We called
> > > > pci_enable_wake(dev, D3cold, wakeup) before, therefore PME for
> > > > D3hot may not be enabled. Is this a bug?
> >
> > On platforms using ACPI, no it isn't.
> >
> > Internally, pci_enable_wake() evaluates _DSW and it doesn't
> > distinguish between D3hot and D3cold as per the spec.
> >
> > > > Background:
> > > > In __pci_set_power_state we have the following:
> > > >
> > > > error = pci_set_low_power_state(dev, PCI_D3hot, locked);
> > > > if (pci_platform_power_transition(dev, PCI_D3cold))
> > > >       return error;
> > > >
> > > > The acpi_pci_set_power_state() stub returns -ENODEV.
> > > > Therefore, if error=0,  __pci_set_power_state() will
> > > > return 0 if pci_platform_power_transition() fails.
> > >
> > > pci_prepare_to_sleep() calls pci_target_state() right at the top.
> > >
> > > If wakeup is supported and enabled, pci_target_state() is supposed
> > > to find the deepest power state supporting wakeup.  If D3cold doesn't
> > > support wakeup, D3hot or a shallower state is returned.
> > >
> > > Hence I don't quite understand how the scenario you're describing
> > > could occur in practice.  Are you seeing actual issues and have tracked
> > > them down to incorrect handling in pci_prepare_to_sleep()?
>
> On non-ACPI systems pci_target_state() just looks at the device
> capabilities. The described scenario can happen if device supports
> wake from D3cold, but platform has no means to switch a device to
> D3cold. I'm thinking of e.g. RTL8125 on ARM. Typically I don't expect
> an issue because boot-up default on these devices is to have PME wake
> from all states enabled. So it's more of a theoretical exercise at the
> moment.

I just see that pci_target_state() won't return D3cold for non-ACPI systems.
Therefore sorry for the noise.