Re: [PATCH] PCI: stm32: Fix LTSSM EP race with start link.

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Nov 17, 2025 at 01:04:47PM +0100, Christian Bruel wrote:
> On 11/14/25 19:59, Bjorn Helgaas wrote:
> > On Fri, Nov 14, 2025 at 08:45:52AM +0100, Christian Bruel wrote:
> > > If the host has deasserted PERST# and started link training before the link
> > > is started on EP side, enabling LTSSM before the endpoint registers are
> > > initialized in the perst_irq handler results in probing incorrect values.
> > > 
> > > Thus, wait for the PERST# level-triggered interrupt to start link training
> > > at the end of initialization and cleanup the stm32_pcie_[start stop]_link
> > > functions.
> > 
> > I've seen this kind of thing in other drivers, and I wondered whether
> > it was safe because the host asserts and deasserts PERST#
> > asynchronously, independent of anything the endpoint is doing.
> > 
> > I assume it's possible that the host deasserts PERST# before this
> > driver has the stm32_pcie_ep_perst_irq_thread() thread set up.  If
> > that happens and the driver doesn't see the PERST# interrupt, does
> > everything still work correctly?
> 
> yes it does. the PERST# interrupt is level-triggered and, if already
> de-asserted, fires only when enabled (it is NOAUTOEN) with start_link.
> 
> At that point, the host can enumerate by performing a manual rescan or
> rebind the PCIe driver, restarting the entire probe sequence.
> 
> Tested the pcie_epf_test driver with various power-up sequences: full
> power-up the host or device first, and stop or standby PM suspend/resume.

Help me think through this.  I guess the interesting case is when the
host boots first and enumerates devices before the stm32 endpoint is
ready, right?

I suppose the endpoint LTSSM is initially disabled, so the link is
down, and the host enumeration didn't find anything?

Where does the link come up?  I see the pci_epc_start_store() that
eventually leads to stm32_pcie_start_link(), which enables perst_irq.
Since you requested perst_irq with IRQF_TRIGGER_HIGH, and PERST# is
deasserted, does that trigger stm32_pcie_ep_perst_irq_thread() and
call stm32_pcie_perst_deassert() to enable the LTSSM?

When the link comes up, if the Downstream Port supports hotplug and
pciehp is enabled, it might notice the link-up event and treat this as
a hot-add?

Otherwise the user would have to manually rescan to notice the
endpoint?

> > > Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> > > ---
> > >   drivers/pci/controller/dwc/pcie-stm32-ep.c | 38 ++++++------------------------
> > >   1 file changed, 7 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/drivers/pci/controller/dwc/pcie-stm32-ep.c b/drivers/pci/controller/dwc/pcie-stm32-ep.c
> > > index 3400c7cd2d88a279c49ef36a99fc7537c381c384..d0654bb43759bb8d0f0d7badbf7bdae839241fcf 100644
> > > --- a/drivers/pci/controller/dwc/pcie-stm32-ep.c
> > > +++ b/drivers/pci/controller/dwc/pcie-stm32-ep.c
> > > @@ -37,36 +37,9 @@ static void stm32_pcie_ep_init(struct dw_pcie_ep *ep)
> > >   		dw_pcie_ep_reset_bar(pci, bar);
> > >   }
> > > -static int stm32_pcie_enable_link(struct dw_pcie *pci)
> > > -{
> > > -	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> > > -
> > > -	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> > > -			   STM32MP25_PCIECR_LTSSM_EN,
> > > -			   STM32MP25_PCIECR_LTSSM_EN);
> > > -
> > > -	return dw_pcie_wait_for_link(pci);
> > > -}
> > > -
> > > -static void stm32_pcie_disable_link(struct dw_pcie *pci)
> > > -{
> > > -	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> > > -
> > > -	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR, STM32MP25_PCIECR_LTSSM_EN, 0);
> > > -}
> > > -
> > >   static int stm32_pcie_start_link(struct dw_pcie *pci)
> > >   {
> > >   	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> > > -	int ret;
> > > -
> > > -	dev_dbg(pci->dev, "Enable link\n");
> > > -
> > > -	ret = stm32_pcie_enable_link(pci);
> > > -	if (ret) {
> > > -		dev_err(pci->dev, "PCIe cannot establish link: %d\n", ret);
> > > -		return ret;
> > > -	}
> > >   	enable_irq(stm32_pcie->perst_irq);
> > > @@ -77,11 +50,7 @@ static void stm32_pcie_stop_link(struct dw_pcie *pci)
> > >   {
> > >   	struct stm32_pcie *stm32_pcie = to_stm32_pcie(pci);
> > > -	dev_dbg(pci->dev, "Disable link\n");
> > > -
> > >   	disable_irq(stm32_pcie->perst_irq);
> > > -
> > > -	stm32_pcie_disable_link(pci);
> > >   }
> > >   static int stm32_pcie_raise_irq(struct dw_pcie_ep *ep, u8 func_no,
> > > @@ -152,6 +121,8 @@ static void stm32_pcie_perst_assert(struct dw_pcie *pci)
> > >   	dev_dbg(dev, "PERST asserted by host\n");
> > > +	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR, STM32MP25_PCIECR_LTSSM_EN, 0);
> > > +
> > >   	pci_epc_deinit_notify(ep->epc);
> > >   	stm32_pcie_disable_resources(stm32_pcie);
> > > @@ -192,6 +163,11 @@ static void stm32_pcie_perst_deassert(struct dw_pcie *pci)
> > >   	pci_epc_init_notify(ep->epc);
> > > +	/* Enable link training */
> > > +	regmap_update_bits(stm32_pcie->regmap, SYSCFG_PCIECR,
> > > +			   STM32MP25_PCIECR_LTSSM_EN,
> > > +			   STM32MP25_PCIECR_LTSSM_EN);
> > > +
> > >   	return;
> > >   err_disable_resources:
> > > 
> > > ---
> > > base-commit: 31115ecec74fe5c679a149d7037009f26b3aa8a9
> > > change-id: 20251113-perst_ep-0b57b9679cf9
> > > 
> > > Best regards,
> > > -- 
> > > Christian Bruel <christian.bruel@foss.st.com>
> > > 
>