Re: [PATCH 1/1] PCI/bwctrl: Disable PCIe BW controller during reset

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Fri, 28 Feb 2025, Lukas Wunner wrote:

> On Thu, Feb 27, 2025 at 06:31:08AM +0100, Lukas Wunner wrote:
> > pcie_bwnotif_irq() accesses the Link Status register without
> > acquiring a runtime PM reference on the PCIe port.  This feels
> > wrong and may also contribute to the issue reported here.
> > Acquiring a runtime PM ref may sleep, so I think you need to
> > change the driver to use a threaded IRQ handler.
> 
> I've realized we've had a discussion before why a threaded IRQ handler
> doesn't make sense...

Yes.
 
> https://lore.kernel.org/all/Z35qJ3H_8u5LQDJ6@wunner.de/
>
> ...but I'm still worried that a Downstream Port in a nested-switch
> configuration may be runtime suspended while the hardirq handler
> is running.  Is there anything preventing that from happening?

I don't think there is.

> To access config space of a port, it's sufficient if its upstream
> bridge is runtime active (i.e. in PCI D0).
> 
> So basically the below is what I have in mind.  This assumes that
> the upstream bridge is still in D0 when the interrupt handler runs
> because in atomic context we can't wait for it to be runtime resumed.
> Seems like a fair assumption to me but what do I know...

bwctrl doesn't even want to resume the port in the irqhandler. If the port
is suspended, why would it have LBMS/LABS, and we disabled notifications 
anyway in suspend handler anyway so we're not even expecting them to come 
during a period of suspend (which does not mean there couldn't be 
interrupts due to other sources).

So there should be no problem in not calling resume for it.

> -- >8 --
> 
> diff --git a/drivers/pci/pcie/bwctrl.c b/drivers/pci/pcie/bwctrl.c
> index 0a5e7efbce2c..fea8f7412266 100644
> --- a/drivers/pci/pcie/bwctrl.c
> +++ b/drivers/pci/pcie/bwctrl.c
> @@ -28,6 +28,7 @@
>  #include <linux/mutex.h>
>  #include <linux/pci.h>
>  #include <linux/pci-bwctrl.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/rwsem.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> @@ -235,9 +236,13 @@ static irqreturn_t pcie_bwnotif_irq(int irq, void *context)
>  	struct pcie_device *srv = context;
>  	struct pcie_bwctrl_data *data = srv->port->link_bwctrl;
>  	struct pci_dev *port = srv->port;
> +	struct device *parent __free(pm_runtime_put) = port->dev.parent;
>  	u16 link_status, events;
>  	int ret;
>  
> +	if (parent)
> +		pm_runtime_get_noresume(parent);
> +

Should this then check if its suspended and return early if it is 
suspended?

pm_runtime_suspended() has some caveats in the kerneldoc though so I'm a 
bit unsure if it can be called safely here, probably not.

>  	ret = pcie_capability_read_word(port, PCI_EXP_LNKSTA, &link_status);
>  	if (ret != PCIBIOS_SUCCESSFUL)
>  		return IRQ_NONE;
> diff --git a/include/linux/pm_runtime.h b/include/linux/pm_runtime.h
> index d39dc863f612..038228de773d 100644
> --- a/include/linux/pm_runtime.h
> +++ b/include/linux/pm_runtime.h
> @@ -448,6 +448,8 @@ static inline int pm_runtime_put(struct device *dev)
>  	return __pm_runtime_idle(dev, RPM_GET_PUT | RPM_ASYNC);
>  }
>  
> +DEFINE_FREE(pm_runtime_put, struct device *, if (_T) pm_runtime_put(_T))
> +
>  /**
>   * __pm_runtime_put_autosuspend - Drop device usage counter and queue autosuspend if 0.
>   * @dev: Target device.
> 

-- 
 i.