Re: [PATCH] PCI/PM: Put devices to low power state on shutdown

[Mika Westerberg]

On Wed, Oct 09, 2024 at 05:24:03PM -0500, Bjorn Helgaas wrote:
> On Fri, Sep 13, 2024 at 11:01:23AM +0300, Mika Westerberg wrote:
> > On Fri, Sep 13, 2024 at 02:00:58PM +0800, Kai-Heng Feng wrote:
> > > On Fri, Sep 13, 2024 at 12:57 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > On Thu, Sep 12, 2024 at 11:00:43AM +0800, Kai-Heng Feng wrote:
> > > > > On Thu, Sep 12, 2024 at 3:05 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> > > > > > On Fri, Jul 12, 2024 at 02:24:11PM +0800, Kai-Heng Feng wrote:
> > > > > > > Some laptops wake up after poweroff when HP Thunderbolt
> > > > > > > Dock G4 is connected.
> > > > > > >
> > > > > > > The following error message can be found during shutdown:
> > > > > > > pcieport 0000:00:1d.0: AER: Correctable error message received from 0000:09:04.0
> > > > > > > pcieport 0000:09:04.0: PCIe Bus Error: severity=Correctable, type=Data Link Layer, (Receiver ID)
> > > > > > > pcieport 0000:09:04.0:   device [8086:0b26] error status/mask=00000080/00002000
> > > > > > > pcieport 0000:09:04.0:    [ 7] BadDLLP
> > > > > > >
> > > > > > > Calling aer_remove() during shutdown can quiesce the error
> > > > > > > message, however the spurious wakeup still happens.
> > > > > > >
> > > > > > > The issue won't happen if the device is in D3 before
> > > > > > > system shutdown, so putting device to low power state
> > > > > > > before shutdown to solve the issue.
> > > > > > >
> > > > > > > I don't have a sniffer so this is purely guesswork,
> > > > > > > however I believe putting device to low power state it's
> > > > > > > the right thing to do.
> > > > > >
> > > > > > My objection here is that we don't have an explanation of
> > > > > > why this should matter or a pointer to any spec language
> > > > > > about this situation, so it feels a little bit random.
> > ...
> 
> > I don't mean to confuse you guys but with this one too, I wonder if you
> > tried to "disable" the device instead of putting it into D3? On another
> > thread (Mario at least is aware of this) I mentioned that our PCIe SV
> > folks identified a couple issues in Linux implementation around power
> > management and one thing that we are missing is to disable I/O and MMIO
> > upon entering D3.
> > ...
> 
> This is really interesting -- did they discover a functional problem,
> or did they just notice that we don't follow the PCI PM spec?

The latter.

> > +++ b/drivers/pci/pci.c
> > @@ -2218,6 +2218,13 @@ static void do_pci_disable_device(struct pci_dev *dev)
> >  		pci_command &= ~PCI_COMMAND_MASTER;
> >  		pci_write_config_word(dev, PCI_COMMAND, pci_command);
> >  	}
> > +	/*
> > +	 * PCI PM 1.2 sec 8.2.2 says that when a function is put into D3
> > +	 * the OS needs to disable I/O and MMIO space in addition to bus
> > +	 * mastering so do that here.
> > +	 */
> > +	pci_command &= ~(PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
> > +	pci_write_config_word(dev, PCI_COMMAND, pci_command);
> >  
> >  	pcibios_disable_device(dev);
> >  }
> 
> This do_pci_disable_device() proposal is interesting.
> 
> pci_enable_device() turns on PCI_COMMAND_MEMORY and PCI_COMMAND_IO,
> which enables the device to respond to MMIO and I/O port accesses to
> its BARs from the driver.  It also makes sure the device is in D0,
> because BAR access only works in D0.
> 
> pci_set_master() turns on PCI_COMMAND_MASTER, which enables the device
> to perform DMA (including generating MSIs).
> 
> pci_disable_device() *sounds* like it should be the opposite of
> pci_enable_device(), but it's currently basically the same as
> pci_clear_master(), which clears PCI_COMMAND_MASTER to prevent DMA.
> I didn't know about this text in 8.2.2, and I wish I knew why we
> don't currently clear PCI_COMMAND_MEMORY and PCI_COMMAND_IO.
> 
> If we want to pursue this, I think it should be split to its own patch
> and moved out of pci_disable_device() because I don't think this path
> necessary implies putting the device in D3, so I think it would fit
> better with the spec if we cleared PCI_COMMAND_MEMORY and
> PCI_COMMAND_IO in a path that explicitly does put it in D3.
> 
> I think there's a significant chance of breaking something because
> drivers are currently able to access BARs after pci_disable_device(),
> and there are a LOT of callers.  But if there's a problem it would
> fix, we should definitely explore it.

At the moment it does not seem to fix anything as far as I can tell so
not sure how important it is. Of course from spec perspective we should
probably deal with it.