Re: [PATCH] PCI: keystone: Don't enable BAR0 if link is not detected

[Bjorn Helgaas]

Hi Siddharth,

On Wed, Oct 11, 2023 at 06:04:51PM +0530, Siddharth Vadapalli wrote:
> Since the function dw_pcie_host_init() ignores the absence of link under
> the assumption that link can come up later, it is possible that the
> pci_host_probe(bridge) function is invoked even when no endpoint device
> is connected. In such a situation, the ks_pcie_v3_65_add_bus() function
> configures BAR0 when the link is not up, resulting in Completion Timeouts
> during the MSI configuration performed later by the PCI Express Port driver
> to setup AER, PME and other services. Thus, leave BAR0 disabled if link is
> not yet detected when the ks_pcie_v3_65_add_bus() function is invoked.

I'm trying to make sense of this.  In this path:

  pci_host_probe
    pci_scan_root_bus_bridge
      pci_register_host_bridge
	bus = pci_alloc_bus(NULL)    # root bus
	bus->ops->add_bus(bus)
	  ks_pcie_v3_65_add_bus

The BAR0 in question must belong to a Root Port.  And it sounds like
the issue must be related to MSI-X, since the original MSI doesn't
involve any BARs.

I don't understand why the Root Port's BAR0 is related to the link
being up.  MSI-X configuration of the Root Port (presumably using
BAR0) shouldn't involve any transactions to devices *below* the Root
Port, and I would expect that BAR to be available (readable and
writable) regardless of whether the link is up.

If we skip the BAR0 configuration because the link is down at the time
of pci_host_probe(), when *do* we do that configuration?  I don't see
another path to ks_pcie_v3_65_add_bus() for the root bus later.

Do you know what exactly causes the Completion Timeout?  Is this a
read to BAR0, or some attempted access to a downstream device, or
something else?

Keystone is the only driver that uses .add_bus() for this, so it seems
a little weird, but maybe this is related to some Keystone-specific
hardware design.

> Signed-off-by: Siddharth Vadapalli <s-vadapalli@xxxxxx>
> ---
> 
> Hello,
> 
> This patch is based on linux-next tagged next-20231011.
> 
> Regards,
> Siddharth.
> 
>  drivers/pci/controller/dwc/pci-keystone.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pci-keystone.c b/drivers/pci/controller/dwc/pci-keystone.c
> index 49aea6ce3e87..ac2ad112d616 100644
> --- a/drivers/pci/controller/dwc/pci-keystone.c
> +++ b/drivers/pci/controller/dwc/pci-keystone.c
> @@ -459,7 +459,8 @@ static int ks_pcie_v3_65_add_bus(struct pci_bus *bus)
>  	struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
>  	struct keystone_pcie *ks_pcie = to_keystone_pcie(pci);
>  
> -	if (!pci_is_root_bus(bus))
> +	/* Don't enable BAR0 if link is not yet up at this point */
> +	if (!pci_is_root_bus(bus) || !dw_pcie_link_up(pci))
>  		return 0;
>  
>  	/* Configure and set up BAR0 */
> -- 
> 2.34.1
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel