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

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

[Dan Carpenter]

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

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

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

[Bjorn Helgaas]

[+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

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

[Johan Hovold]

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