Re: [Regression] Commit "ACPI: PM: Allow transitions to D0 to occur in special cases"

["Rafael J. Wysocki" <rjw@rjwysocki.net>]

On Wednesday, July 31, 2019 11:30:01 PM CEST Bjorn Helgaas wrote:
> [+cc Thunderbolt folks, see
> https://lore.kernel.org/r/578BD3F1-B185-471B-A3EB-FF71BA34B822@canonical.com
> for beginning of thread]
> 
> On Thu, Aug 01, 2019 at 12:04:29AM +0800, Kai-Heng Feng wrote:
> > Hi,
> > 
> > After commit "ACPI: PM: Allow transitions to D0 to occur in special cases”,
> 
> This is f850a48a0799 ("ACPI: PM: Allow transitions to D0 to occur in
> special cases").
> 
> > Thunderbolt on XPS 9380 spews the following when it runtime resumes:
> > [   36.136554] pci_raw_set_power_state: 25 callbacks suppressed
> > [   36.136558] pcieport 0000:03:00.0: Refused to change power state,
> > currently in D3
> 
> We really should be smarter about what we print here, maybe something
> like the patch below?
> 
> pci_raw_set_power_state() prints "Refused to change power state" if
> (in this case) the value of (PCI_PM_CTRL & PCI_PM_CTRL_STATE_MASK) is
> 0x3.  Most likely we got 0xffff from PCI_PM_CTRL because the device is
> in D3cold.  If the device is in D3cold, pci_raw_set_power_state() has
> no hope of doing anything because it only uses PCI PM config
> registers, and they're inaccessible in D3cold.
> 
> Presumably there's some platform PM method that is supposed to take
> the device out of D3cold, and maybe we're missing that somehow?

Yes, there is.

> Based on an lspci I found at [1], I suspect 03:00.0 is a Thunderbolt
> switch leading to [bus 04-6d].  From your log, it looks like these
> devices don't work:
> 
>   03:00.0 Thunderbolt Upstream Port
>   04:00.0 Thunderbolt Downstream Port
>   04:01.0 Thunderbolt Downstream Port (Slot 1)
>   04:02.0 Thunderbolt Downstream Port
>   04:04.0 Thunderbolt Downstream Port (Slot 4)
>   05:00.0 Thunderbolt NHI
>   39:00.0 XHCI USB
> 
> If 03:00.0 is stuck in D3cold, that would explain why none of these
> things work.
> 
> [1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1826125
> 
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 29ed5ec1ac27..63ca963ebff9 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -851,6 +852,11 @@ static int pci_raw_set_power_state(struct pci_dev *dev, pci_power_t state)
>  		return -EIO;
>  
>  	pci_read_config_word(dev, dev->pm_cap + PCI_PM_CTRL, &pmcsr);
> +	if (pmcsr == (u16) ~0) {

Is the "device not accessible" the only case in which we can get all ones from this?

If so, the change will be fine by me.

> +		pci_err(dev, "device not responding; can't change to power state D%d\n",
> +			state);

But I wouldn't break this line.

> +		return -EIO;
> +	}
>  
>  	/*
>  	 * If we're (effectively) in D3, force entire word to 0.
> 

Thanks!