Re: [PATCH 3/5] PCI / PM: Check for error when reading PME status

[Rafael J. Wysocki]

On Mon, Aug 5, 2019 at 10:52 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> From: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
>
> pci_check_pme_status() reads the Power Management capability to determine
> whether a device has generated a PME.  The capability is in config space,
> which is accessible in D0, D1, D2, and D3hot, but not in D3cold.
>
> If we call pci_check_pme_status() on a device that's in D3cold, config
> reads fail and return ~0 data, which we erroneously interpreted as "the
> device has generated a PME".
>
> 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> avoided many of these problems by avoiding pci_check_pme_status() if we
> think the device is in D3cold.  However, it is not a complete fix because
> the device may go to D3cold after we check its power state but before
> pci_check_pme_status() reads the Power Management Status Register.
>
> Return false ("device has not generated a PME") if we get an error response
> reading the Power Management Status Register.
>
> Fixes: 000dd5316e1c ("PCI: Do not poll for PME if the device is in D3cold")
> Fixes: 71a83bd727cc ("PCI/PM: add runtime PM support to PCIe port")
> Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
> ---
>  drivers/pci/pci.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 984171d40858..af6a97d7012b 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2008,6 +2008,9 @@ bool pci_check_pme_status(struct pci_dev *dev)
>
>         pmcsr_pos = dev->pm_cap + PCI_PM_CTRL;
>         pci_read_config_word(dev, pmcsr_pos, &pmcsr);
> +       if (pmcsr == (u16) PCI_ERROR_RESPONSE)
> +               return false;
> +

No, sorry.

We tried that and it didn't work.

pcie_pme_handle_request() depends on this returning "true" for all
bits set, as from its perspective "device is not accessible" may very
well mean "device may have signaled PME".

If you want to make this change, you need to rework
pcie_pme_handle_request() along with it.

>         if (!(pmcsr & PCI_PM_CTRL_PME_STATUS))
>                 return false;