Re: [bug report] Revert "PCI/ASPM: Remove pcie_aspm_pm_state_change()"

[Johan Hovold <johan@kernel.org>]

On Mon, Jan 22, 2024 at 12:28:49PM -0600, Bjorn Helgaas wrote:
> [+cc Johan, Kai-Heng]
> 
> On Mon, Jan 22, 2024 at 05:43:09PM +0300, Dan Carpenter wrote:
> > Hello Bjorn Helgaas,
> > 
> > The patch f93e71aea6c6: "Revert "PCI/ASPM: Remove
> > pcie_aspm_pm_state_change()"" from Jan 1, 2024 (linux-next), leads to
> > the following Smatch static checker warning:
> > 
> > 	drivers/pci/pcie/aspm.c:1017 pcie_aspm_pm_state_change()
> > 	warn: sleeping in atomic context
> 
> Thanks Dan, this is probably related to the lockdep issue Johan
> reported here:
> https://lore.kernel.org/r/ZZu0qx2cmn7IwTyQ@hovoldconsulting.com

This looks like a separate issue actually.

> > drivers/pci/pcie/aspm.c
> >     1007 void pcie_aspm_pm_state_change(struct pci_dev *pdev)
> >     1008 {
> >     1009         struct pcie_link_state *link = pdev->link_state;
> >     1010 
> >     1011         if (aspm_disabled || !link)
> >     1012                 return;
> >     1013         /*
> >     1014          * Devices changed PM state, we should recheck if latency
> >     1015          * meets all functions' requirement
> >     1016          */
> > --> 1017         down_read(&pci_bus_sem);
> > 
> > This is a revert from a patch from 2022 which was before I had written
> > this "sleeping in atomic" static checker thing.
> > 
> >     1018         mutex_lock(&aspm_lock);
> >     1019         pcie_update_aspm_capable(link->root);
> >     1020         pcie_config_aspm_path(link);
> >     1021         mutex_unlock(&aspm_lock);
> >     1022         up_read(&pci_bus_sem);
> >     1023 }
> > 
> > The call trees that Smatch is complaining about are:
> > 
> > vortex_boomerang_interrupt() <- disables preempt
> > -> _vortex_interrupt()
> > -> _boomerang_interrupt()
> >    -> vortex_error()
> >       -> vortex_up()
> > velocity_suspend() <- disables preempt
> > -> velocity_set_power_state()
> >          -> pci_set_power_state()
> >             -> pci_set_low_power_state()
> >                -> pcie_aspm_pm_state_change()

Based on a very quick look, I don't think it has ever been valid to call
pci_set_power_state() from atomic context as it for, for example, can
call pci_bus_set_current_state() which also takes the bus semaphore.

Johan