On Mon, May 05, 2025 at 05:05:10PM +0200, Niklas Cassel wrote:

Hello Mani,

On Thu, Apr 17, 2025 at 10:46:31PM +0530, Manivannan Sadhasivam via B4 Relay wrote:

quoted

@@ -1571,6 +1652,9 @@ static irqreturn_t qcom_pcie_global_irq_thread(int irq, void *data)
 		pci_unlock_rescan_remove();
 
 		qcom_pcie_icc_opp_update(pcie);
+	} else if (FIELD_GET(PARF_INT_ALL_LINK_DOWN, status)) {
+		dev_dbg(dev, "Received Link down event\n");
+		pci_host_handle_link_down(pp->bridge);
 	} else {
 		dev_WARN_ONCE(dev, 1, "Received unknown event. INT_STATUS: 0x%08x\n",
 			      status);

From debugging an unrelated problem, I noticed that dw-rockchip can
sometimes have both "link up" bit and "hot reset or link down" bit set
at the same time, when reading the status register.

That's a good catch. It is quite possible that both fields could be set at once,
so 'if..else' is indeed a bad idea.

Perhaps the link went down very quickly and then was established again
by the time the threaded IRQ handler gets to run.

Your code seems to do an if + else if.

Without knowing how the events work for your platforms, I would guess
that it should also be possible to have multiple events set.

I agree.

In you code, if both LINK UP and hot reset/link down are set,
I would assume that you driver will not do the right thing.

Perhaps you want to swap the order? So that link down is handled first,
and then link up is handled. (If you convert to "if + if "instead of
"if + else if" that is.)

Makes sense. I'll incorporate this change in v5, thanks!

- Mani

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