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

[Bjorn Helgaas <helgaas@kernel.org>]

[+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 is definitely an open issue that should be resolved.

Bjorn

> 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()
> 
> So what Smatch is saying is the vortex_boomerang_interrupt() and
> velocity_suspend() hold spin locks and then set the power state.  The
> call trees are quite long so I'm not really able to be sure if this is
> a false positive or not...  I wish this warning were more useful.
> 
> These emails are a one time thing.  Just reply if it's a false positive
> and I'll note it down.  Otherwise feel free to ignore it.
> 
> regards,
> dan carpenter