| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <67dbd8f6-2efe-4a73-a611-9081f364d2d4@foss.st.com>
Date: Tue, 18 Nov 2025 19:34:01 +0100
From: Christian Bruel <christian.bruel@...s.st.com>
To: Bjorn Helgaas <helgaas@...nel.org>
CC: Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kwilczynski@...nel.org>,
"Manivannan
Sadhasivam" <mani@...nel.org>,
Rob Herring <robh@...nel.org>, Bjorn Helgaas
<bhelgaas@...gle.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
"Alexandre Torgue" <alexandre.torgue@...s.st.com>,
<linux-pci@...r.kernel.org>,
<linux-stm32@...md-mailman.stormreply.com>,
<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] PCI: stm32: Fix LTSSM EP race with start link.
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@...s.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@...s.st.com>
>>>>
>>
Powered by blists - more mailing lists