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

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

[Christian Bruel]

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.

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>

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

[Bjorn Helgaas]

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?

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

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

[Christian Bruel]

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.


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

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

[Bjorn Helgaas]

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

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

[Christian Bruel]

On 11/17/25 21:00, Bjorn Helgaas wrote:
> 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?

yes. When the device is not started (start_link) dw_pcie_wait_for_link() 
fails and pci_host_probe() only register the root port:

         /*
          * Note: Skip the link up delay only when a Link Up IRQ is present.
          * If there is no Link Up IRQ, we should not bypass the delay
          * because that would require users to manually rescan for devices.
          */
         if (!pp->use_linkup_irq)
                 /* Ignore errors, the link may come up later */
                 dw_pcie_wait_for_link(pci);

        ret = pci_host_probe(bridge);

> 
> 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.

The link appears when explicitly requested by writing '1' into the bound 
endpoint device driver sysfs 'start' once the device is configured.

see https://www.kernel.org/doc/html/latest/PCI/endpoint/pci-test-howto.html

But how pci_epc_start_store() is iterated from the configfs_write_iter() 
mechanism is beyond my knowledge...

> 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?

perst_gpio is active low. So requesting the perst_irq with 
IRQF_TRIGGER_HIGH triggers when deasserted.

This parts is quite confusing (for me) because gpiod_get_value() 
correctly returns 0 when the gpio is de-asserted, when the irq API does 
not know about active low so we must use TRIGGER_HIGH

> 
> 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?

yes, I just tried with a host pc with hot-plug enabled. lspci found the 
stm32 EP, as you anticipated

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

indeed, this is how I proceed when the host does not support hot-plug,

thank you,

Christian

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

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

[Bjorn Helgaas]

[+cc Lukas in case I got the pciehp part wrong]

On Tue, Nov 18, 2025 at 07:34:01PM +0100, Christian Bruel wrote:
> On 11/17/25 21:00, Bjorn Helgaas wrote:
> > 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.

To back up here, I'm trying to understand the race.

IIUC the relevant events are link training and register init.  In the
current stm32 EP driver, link training can start when the EP userspace
writes to the 'start' configfs file.  And the register init happens
when stm32_pcie_ep_perst_irq_thread() calls
dw_pcie_ep_init_registers().

So I guess the problem is when the EP userspace enables the LTSSM
before the host deasserts PERST#?  And the link train may complete
before stm32_pcie_ep_perst_irq_thread() runs?

And the fix here is to delay enabling the EP LTSSM until after
stm32_pcie_perst_deassert() calls dw_pcie_ep_init_registers()?

> > > > 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?
> 
> yes. When the device is not started (start_link)
> dw_pcie_wait_for_link() fails and pci_host_probe() only register the
> root port:
> 
>         /*
>          * Note: Skip the link up delay only when a Link Up IRQ is present.
>          * If there is no Link Up IRQ, we should not bypass the delay
>          * because that would require users to manually rescan for devices.
>          */
>         if (!pp->use_linkup_irq)
>                 /* Ignore errors, the link may come up later */
>                 dw_pcie_wait_for_link(pci);
> 
>        ret = pci_host_probe(bridge);
> 
> > 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.
> 
> The link appears when explicitly requested by writing '1' into the bound
> endpoint device driver sysfs 'start' once the device is configured.
> 
> see https://www.kernel.org/doc/html/latest/PCI/endpoint/pci-test-howto.html
> 
> But how pci_epc_start_store() is iterated from the configfs_write_iter()
> mechanism is beyond my knowledge...

OK.  I just wanted to learn whether this was all automatic or required
user intervention.

IIUC it's never completely automatic because even if the endpoint
boots first, the endpoint LTSSM isn't enabled and the link won't train
until userspace on the endpoint writes '1' into the 'start' configfs
file.

> > 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?
> 
> perst_gpio is active low. So requesting the perst_irq with
> IRQF_TRIGGER_HIGH triggers when deasserted.
> 
> This part is quite confusing (for me) because gpiod_get_value()
> correctly returns 0 when the gpio is de-asserted, when the irq API
> does not know about active low so we must use TRIGGER_HIGH
> 
> > 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?
> 
> yes, I just tried with a host pc with hot-plug enabled. lspci found
> the stm32 EP, as you anticipated
> 
> > Otherwise the user would have to manually rescan to notice the
> > endpoint?
> 
> indeed, this is how I proceed when the host does not support
> hot-plug,

I think we would prefer if the host would enumerate the endpoint
whenever the endpoint becomes ready, even if that is after the host's
initial enumeration, but I guess that's only possible if the host is
notified when the link comes up.

The main mechanism for that is hotplug, i.e., pciehp handles presence
detect and link layer state changed events, both of which are managed
by the PCIe Slot register set.  Those registers are optional and may
not be implemented, e.g., if a Root Port is connected to a
system-integrated device.

But if they *are* implemented, I hope that pciehp makes it so no user
intervention on the host side is required.

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

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

[Christian Bruel]

On 11/18/25 22:28, Bjorn Helgaas wrote:
> [+cc Lukas in case I got the pciehp part wrong]
> 

> 
> To back up here, I'm trying to understand the race.
> 
> IIUC the relevant events are link training and register init.  In the
> current stm32 EP driver, link training can start when the EP userspace
> writes to the 'start' configfs file.  And the register init happens
> when stm32_pcie_ep_perst_irq_thread() calls
> dw_pcie_ep_init_registers().
> 
> So I guess the problem is when the EP userspace enables the LTSSM
> before the host deasserts PERST#?  And the link train may complete
> before stm32_pcie_ep_perst_irq_thread() runs?

The sequence also violated the spec (4.0, Section 6.6.1 "Conventional 
Reset"), because it allowed the endpoint to enter the Detect state 
before PERST# is deasserted

> 
> And the fix here is to delay enabling the EP LTSSM until after
> stm32_pcie_perst_deassert() calls dw_pcie_ep_init_registers()?
> 

> 
> I think we would prefer if the host would enumerate the endpoint
> whenever the endpoint becomes ready, even if that is after the host's
> initial enumeration, but I guess that's only possible if the host is
> notified when the link comes up.
> 
> The main mechanism for that is hotplug, i.e., pciehp handles presence
> detect and link layer state changed events, both of which are managed
> by the PCIe Slot register set.  Those registers are optional and may
> not be implemented, e.g., if a Root Port is connected to a
> system-integrated device.
> 
> But if they *are* implemented, I hope that pciehp makes it so no user
> intervention on the host side is required.
>


I suppose that hotplug cannot be implemented without some kind of 
presence detection signal from the EP (PRSNT#, ...) ? For now we have no 
  implementation to support this (from gpio or other).

However, using a PC host, I observe that when I resume the host from 
PCIe autosuspend, for example, with 'lspci', PERST# is deasserted, and 
the stm32 PCIe EP device is correctly enumerated without a manual 
rescan. So thanks to power management, a device can be enumerated 
asynchronously but when requested, not when ready.

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

[Manivannan Sadhasivam]

On Wed, Nov 19, 2025 at 04:13:47PM +0100, Christian Bruel wrote:
> 
> 
> On 11/18/25 22:28, Bjorn Helgaas wrote:
> > [+cc Lukas in case I got the pciehp part wrong]
> > 
> 
> > 
> > To back up here, I'm trying to understand the race.
> > 
> > IIUC the relevant events are link training and register init.  In the
> > current stm32 EP driver, link training can start when the EP userspace
> > writes to the 'start' configfs file.  And the register init happens
> > when stm32_pcie_ep_perst_irq_thread() calls
> > dw_pcie_ep_init_registers().
> > 
> > So I guess the problem is when the EP userspace enables the LTSSM
> > before the host deasserts PERST#?  And the link train may complete
> > before stm32_pcie_ep_perst_irq_thread() runs?
> 
> The sequence also violated the spec (4.0, Section 6.6.1 "Conventional
> Reset"), because it allowed the endpoint to enter the Detect state before
> PERST# is deasserted
> 
> > 
> > And the fix here is to delay enabling the EP LTSSM until after
> > stm32_pcie_perst_deassert() calls dw_pcie_ep_init_registers()?
> > 
> 
> > 
> > I think we would prefer if the host would enumerate the endpoint
> > whenever the endpoint becomes ready, even if that is after the host's
> > initial enumeration, but I guess that's only possible if the host is
> > notified when the link comes up.
> > 
> > The main mechanism for that is hotplug, i.e., pciehp handles presence
> > detect and link layer state changed events, both of which are managed
> > by the PCIe Slot register set.  Those registers are optional and may
> > not be implemented, e.g., if a Root Port is connected to a
> > system-integrated device.
> > 
> > But if they *are* implemented, I hope that pciehp makes it so no user
> > intervention on the host side is required.
> > 
> 
> 
> I suppose that hotplug cannot be implemented without some kind of presence
> detection signal from the EP (PRSNT#, ...) ? For now we have no
> implementation to support this (from gpio or other).
> 

Most of the non-PC/Server platforms do not have hot pluggable connectors. So the
lack of PRSNT# signal is very common.

> However, using a PC host, I observe that when I resume the host from PCIe
> autosuspend, for example, with 'lspci', PERST# is deasserted, and the stm32
> PCIe EP device is correctly enumerated without a manual rescan. So thanks to
> power management, a device can be enumerated asynchronously but when
> requested, not when ready.
> 

I would assume that any host will deassert PERST# during boot itself, and keep
the LTSSM in Link.Detect. But if the link is not detected, a host *may* assert
PERST#. And during resume, it will try the same sequence and if your device is
connected, it will get enumerated.

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Manivannan Sadhasivam]

On Fri, 14 Nov 2025 08:45:52 +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.
> 
> [...]

Applied, thanks!

[1/1] PCI: stm32: Fix LTSSM EP race with start link.
      commit: acca17da9c9f53c0aa05bbdef255213d36dc09db

Best regards,
-- 
Manivannan Sadhasivam <mani@kernel.org>