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:
> > @@ -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
--
மணிவண்ணன் சதாசிவம்
