| 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: <aRHb4S40a7ZUDop1@ryzen>
Date: Mon, 10 Nov 2025 13:34:41 +0100
From: Niklas Cassel <cassel@...nel.org>
To: Shawn Lin <shawn.lin@...k-chips.com>
Cc: 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>, mani@...nel.org
Subject: Re: [RESEND] Re: [PATCH] PCI: dw-rockchip: Skip waiting for link up
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...
...we already have a delay in the link up IRQ handler, before calling
pci_rescan_bus().
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:
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