Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready

From: Niklas Cassel <cassel@kernel.org>
To: Bjorn Helgaas <helgaas@kernel.org>
Cc: "Lorenzo Pieralisi" <lpieralisi@kernel.org>,
	"Krzysztof Wilczyński" <kwilczynski@kernel.org>,
	"Manivannan Sadhasivam" <mani@kernel.org>,
	"Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Heiko Stuebner" <heiko@sntech.de>,
	"Wilfred Mallawa" <wilfred.mallawa@wdc.com>,
	"Damien Le Moal" <dlemoal@kernel.org>,
	"Laszlo Fiat" <laszlo.fiat@proton.me>,
	linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH 1/4] PCI: dw-rockchip: Do not enumerate bus before endpoint devices are ready
Date: Thu, 12 Jun 2025 13:19:45 +0200	[thread overview]
Message-ID: <aEq30a7vJmKyYWYT@ryzen> (raw)
In-Reply-To: <20250611211456.GA869983@bhelgaas>

On Wed, Jun 11, 2025 at 04:14:56PM -0500, Bjorn Helgaas wrote:
> On Wed, Jun 11, 2025 at 12:51:42PM +0200, Niklas Cassel wrote:
> > Commit ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since we can
> > detect Link Up") changed so that we no longer call dw_pcie_wait_for_link(),
> > and instead enumerate the bus directly after receiving the Link Up IRQ.
> > 
> > This means that there is no longer any delay between link up and the bus
> > getting enumerated.
> 
> Minor quibble about "no longer any delay": dw_pcie_wait_for_link()
> doesn't contain any explicit delay *after* we notice the link is up,
> so we didn't guarantee sufficient delay even before ec9fd499b9c6.
> 
> If the link came up before the first check, dw_pcie_wait_for_link()
> didn't delay at all.  Otherwise, it delayed 90ms * N, and we have no
> idea when in the 90ms period the link came up, so the post link-up
> delay was effectively some random amount between 0 and 90ms.
> 
> I would propose something like:
> 
>   PCI: dw-rockchip: Wait PCIE_T_RRS_READY_MS after link-up IRQ
> 
>   Per PCIe r6.0, sec 6.6.1, software must generally wait a minimum of
>   100ms (PCIE_T_RRS_READY_MS) after Link training completes before
>   sending a Configuration Request.
> 
>   Prior to ec9fd499b9c6 ("PCI: dw-rockchip: Don't wait for link since
>   we can detect Link Up"), dw-rockchip used dw_pcie_wait_for_link(),
>   which waited between 0 and 90ms after the link came up before we
>   enumerate the bus, and this was apparently enough for most devices.
> 
>   After ec9fd499b9c6, rockchip_pcie_rc_sys_irq_thread() started
>   enumeration immediately when handling the link-up IRQ, and devices
>   (e.g., Laszlo Fiat's PLEXTOR PX-256M8PeGN NVMe SSD) may not be ready
>   to handle config requests yet.
> 
>   Delay PCIE_T_RRS_READY_MS after the link-up IRQ before starting
>   enumeration.

Ok, I will shamelessly use this text verbatim.

> I think the comment at the PCIE_T_RRS_READY_MS definition should be
> enough (although it might need to be updated to mention link-up).
> This delay is going to be a standard piece of every driver, so it
> won't require special notice.

Looking at pci.h, we already have a comment mentioning exactly this
(link-up):
https://github.com/torvalds/linux/blob/v6.16-rc1/drivers/pci/pci.h#L51-L63

I will change the patches to use PCIE_RESET_CONFIG_DEVICE_WAIT_MS instead.

Kind regards,
Niklas