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

[Christian Bruel <christian.bruel@foss.st.com>]

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