| 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: <2n3wamm3txxc6xbmvf3nnrvaqpgsck3w4a6omxnhex3mqeujib@2tb4svn5d3z6>
Date: Mon, 10 Nov 2025 21:23:02 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Niklas Cassel <cassel@...nel.org>
Cc: Shawn Lin <shawn.lin@...k-chips.com>, FUKAUMI Naoki <naoki@...xa.com>,
Damien Le Moal <dlemoal@...nel.org>, Anand Moon <linux.amoon@...il.com>, linux-pci@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org,
Dragan Simic <dsimic@...jaro.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>,
Krzysztof Wilczyński <kw@...ux.com>, Rob Herring <robh@...nel.org>,
Bjorn Helgaas <bhelgaas@...gle.com>, Heiko Stuebner <heiko@...ech.de>
Subject: Re: [RESEND] Re: [PATCH] PCI: dw-rockchip: Skip waiting for link up
On Mon, Nov 10, 2025 at 01:34:41PM +0100, Niklas Cassel wrote:
> On Mon, Nov 10, 2025 at 06:15:33PM +0800, Shawn Lin wrote:
> > >
> > > Could you try PCIe 2.0 slot on your board?
> >
> > I did, it doesn't work on PCIe 2.0 slot. From the PA, I could see
> > the link is still in training during pci_host_probe() is called.
> > Add some delay before pci_rescan_bus() in pcie-dw-rockchip doesn't
> > help. But the below change should work as we delayed pci_host_probe().
> >
> > --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> > @@ -236,6 +236,8 @@ static int rockchip_pcie_start_link(struct dw_pcie *pci)
> > msleep(PCIE_T_PVPERL_MS);
> > gpiod_set_value_cansleep(rockchip->rst_gpio, 1);
> >
> > + msleep(50);
> > +
> > return 0;
> >
> > Otherwise we got:
> >
> > [ 0.841518] pci_bus 0003:33: busn_res: can not insert [bus 33-31] under
> > [bus 32-31] (conflicts with (null) [bus 32-31])
> > [ 0.842596] pci_bus 0003:33: busn_res: [bus 33-31] end is updated to 33
> > [ 0.843184] pci_bus 0003:33: busn_res: can not insert [bus 33] under [bus
> > 32-31] (conflicts with (null) [bus 32-31])
> > [ 0.844120] pci 0003:32:00.0: devices behind bridge are unusable because
> > [bus 33] cannot be assigned for them
> > [ 0.845229] pci_bus 0003:34: busn_res: can not insert [bus 34-31] under
> > [bus 32-31] (conflicts with (null) [bus 32-31])
> > [ 0.846309] pci_bus 0003:34: busn_res: [bus 34-31] end is updated to 34
> > [ 0.846898] pci_bus 0003:34: busn_res: can not insert [bus 34] under [bus
> > 32-31] (conflicts with (null) [bus 32-31])
> > [ 0.847833] pci 0003:32:06.0: devices behind bridge are unusable because
> > [bus 34] cannot be assigned for them
> > [ 0.848923] pci_bus 0003:35: busn_res: can not insert [bus 35-31] under
> > [bus 32-31] (conflicts with (null) [bus 32-31])
> > [ 0.850014] pci_bus 0003:35: busn_res: [bus 35-31] end is updated to 35
> > [ 0.850605] pci_bus 0003:35: busn_res: can not insert [bus 35] under [bus
> > 32-31] (conflicts with (null) [bus 32-31])
> > [ 0.851540] pci 0003:32:0e.0: devices behind bridge are unusable because
> > [bus 35] cannot be assigned for them
> > [ 0.852424] pci_bus 0003:32: busn_res: [bus 32-31] end is updated to 35
> > [ 0.853028] pci_bus 0003:32: busn_res: can not insert [bus 32-35] under
> > [bus 31] (conflicts with (null) [bus 31])
> > [ 0.853184] hub 3-0:1.0: USB hub found
> > [ 0.853931] pci 0003:31:00.0: devices behind bridge are unusable because
> > [bus 32-35] cannot be assigned for them
> > [ 0.854262] hub 3-0:1.0: 1 port detected
> > [ 0.855144] pcieport 0003:30:00.0: bridge has subordinate 31 but max busn
> > 35
> > [ 0.855722] hub 4-0:1.0: USB hub found
> > [ 0.856109] pci 0003:32:00.0: PCI bridge to [bus 33]
> > [ 0.856939] pci 0003:32:06.0: PCI bridge to [bus 34]
> > [ 0.857133] hub 4-0:1.0: 1 port detected
> > [ 0.857430] pci 0003:32:0e.0: PCI bridge to [bus 35]
> > [ 0.858236] pci 0003:31:00.0: PCI bridge to [bus 32-35]
>
> Mani,
>
> while I see the idea behind your suggested hack:
>
> + if (pdev->vendor == 0x1d87 && pdev->device == 0x3588) {
> + pdev->is_hotplug_bridge = pdev->is_pciehp = 1;
> + return;
> + }
>
>
> Considering what Shawn says, that the switch gets enumerated properly
> if we simply add a msleep() in ->start_link(), which will be called
> by dw_pcie_host_init() before pci_host_probe() is called...
>
Yes, that delay probably gives enough time for the link up IRQ to kick in before
the initial bus scan happens.
> ...we already have a delay in the link up IRQ handler, before calling
> pci_rescan_bus().
>
That delay won't help in this case.
> So, I think that the problem is that we are unconditionally calling
> pci_host_probe() in dw_pcie_host_init(), even for platforms that have
> a link-up IRQ.
>
>
> I think a better solution would be something like:
>
This solution will work as long as the PCIe device is powered ON before
start_link(). For CEM and M.2 Key M connectors, the host controller can power
manage the components. But for other specifications/keys requiring custom power
management, a separate driver would be needed.
That's why I suggested using pwrctrl framework as it can satisfy both usecases.
However, as I said, it needs a bit of rework and I'm close to submitting it.
But until that gets merged, either we need to revert your link up IRQ change or
have the below patch. IMO, the revert seems simple.
- Mani
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index e92513c5bda5..42d987ddab7d 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -565,6 +565,39 @@ static int dw_pcie_host_get_resources(struct dw_pcie_rp *pp)
> return 0;
> }
>
> +static int dw_pcie_host_initial_scan(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct pci_host_bridge *bridge = pp->bridge;
> + int ret;
> +
> + ret = pci_host_probe(bridge);
> + if (ret)
> + return ret;
> +
> + if (pp->ops->post_init)
> + pp->ops->post_init(pp);
> +
> + dwc_pcie_debugfs_init(pci, DW_PCIE_RC_TYPE);
> +
> + return 0;
> +}
> +
> +void dw_pcie_handle_link_up_irq(struct dw_pcie_rp *pp)
> +{
> + if (!pp->initial_linkup_irq_done) {
> + if (dw_pcie_host_initial_scan(pp)) {
> + //TODO: cleanup
> + }
> + pp->initial_linkup_irq_done = true;
> + } else {
> + /* Rescan the bus to enumerate endpoint devices */
> + pci_lock_rescan_remove();
> + pci_rescan_bus(pp->bridge->bus);
> + pci_unlock_rescan_remove();
> + }
> +}
> +
> int dw_pcie_host_init(struct dw_pcie_rp *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -672,15 +705,13 @@ int dw_pcie_host_init(struct dw_pcie_rp *pp)
> if (!pp->use_linkup_irq)
> /* Ignore errors, the link may come up later */
> dw_pcie_wait_for_link(pci);
> -
> - ret = pci_host_probe(bridge);
> - if (ret)
> - goto err_stop_link;
> -
> - if (pp->ops->post_init)
> - pp->ops->post_init(pp);
> -
> - dwc_pcie_debugfs_init(pci, DW_PCIE_RC_TYPE);
> + else
> + /*
> + * For platforms with Link Up IRQ, initial scan will be done
> + * on first Link Up IRQ.
> + */
> + if (dw_pcie_host_initial_scan(pp))
> + goto err_stop_link;
>
> return 0;
>
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index e995f692a1ec..a31bd93490dc 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -427,6 +427,7 @@ struct dw_pcie_rp {
> int msg_atu_index;
> struct resource *msg_res;
> bool use_linkup_irq;
> + bool initial_linkup_irq_done;
> struct pci_eq_presets presets;
> struct pci_config_window *cfg;
> bool ecam_enabled;
> @@ -807,6 +808,7 @@ void dw_pcie_msi_init(struct dw_pcie_rp *pp);
> int dw_pcie_msi_host_init(struct dw_pcie_rp *pp);
> void dw_pcie_free_msi(struct dw_pcie_rp *pp);
> int dw_pcie_setup_rc(struct dw_pcie_rp *pp);
> +void dw_pcie_handle_link_up_irq(struct dw_pcie_rp *pp);
> int dw_pcie_host_init(struct dw_pcie_rp *pp);
> void dw_pcie_host_deinit(struct dw_pcie_rp *pp);
> int dw_pcie_allocate_domains(struct dw_pcie_rp *pp);
> @@ -844,6 +846,9 @@ static inline int dw_pcie_setup_rc(struct dw_pcie_rp *pp)
> return 0;
> }
>
> +static inline void dw_pcie_handle_link_up_irq(struct dw_pcie_rp *pp)
> +{ }
> +
> static inline int dw_pcie_host_init(struct dw_pcie_rp *pp)
> {
> return 0;
> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> index 8a882dcd1e4e..042e5845bdd6 100644
> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
> @@ -468,10 +468,7 @@ static irqreturn_t rockchip_pcie_rc_sys_irq_thread(int irq, void *arg)
> if (rockchip_pcie_link_up(pci)) {
> msleep(PCIE_RESET_CONFIG_WAIT_MS);
> dev_dbg(dev, "Received Link up event. Starting enumeration!\n");
> - /* Rescan the bus to enumerate endpoint devices */
> - pci_lock_rescan_remove();
> - pci_rescan_bus(pp->bridge->bus);
> - pci_unlock_rescan_remove();
> + dw_pcie_handle_link_up_irq(pp);
> }
> }
>
>
>
>
>
> What do you think?
>
>
>
> Kind regards,
> Niklas
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists