Re: [PATCH v2 10/15] PCI: rcar-gen4: Take over the iMSI-RX interrupt

[Koichiro Den]

On Sun, Oct 04, 2026 at 02:53:27AM +0200, Marek Vasut wrote:
> On 9/28/26 6:52 PM, Koichiro Den wrote:
> 
> [...]
> 
> > @@ -724,6 +773,9 @@ static void rcar_gen4_pcie_quiesce_irqs(struct rcar_gen4_pcie *rcar)
> >   		rcar->reinit_pending = true;
> >   		rcar_gen4_pcie_app_irq_sync_locked(rcar);
> >   	}
> > +
> > +	/* The MSI status lives in DBI; keep the handler away during the reset. */
> 
> Would it make sense to add lockdep_assert_held(&rcar->reset_lock) here, to
> make it clear that this IRQ disable is protected by the reset lock ?

The reset_lock is going away in v3, per my reply at:
https://lore.kernel.org/r/eivzz4zumuu32zrycw6wpt3orzk2dvlzuvj34krmbhjf7a3a6u@ccvu2nkupzid/
These helpers are reached from reset_root_port(), which the core serializes as I
wrote in the above link, and from .init()/.deinit() during probe/remove and
suspend/resume, where no reset can be in flight. Sorry for the noise.

> 
> > +	disable_irq(rcar->msi_irq);
> >   }
> >   static void rcar_gen4_pcie_resume_irqs(struct rcar_gen4_pcie *rcar,
> > @@ -733,6 +785,8 @@ static void rcar_gen4_pcie_resume_irqs(struct rcar_gen4_pcie *rcar,
> >   		rcar->reinit_pending = !recovered;
> >   		rcar_gen4_pcie_app_irq_sync_locked(rcar);
> >   	}
> > +
> 
> Would it make sense to add lockdep_assert_held(&rcar->reset_lock) here, to
> make it clear that this IRQ enable is protected by the reset lock ?

ditto.

> 
> > +	enable_irq(rcar->msi_irq);
> >   }
> >   /*
> > @@ -804,11 +858,17 @@ static int rcar_gen4_pcie_host_init(struct dw_pcie_rp *pp)
> >   	ret = rcar_gen4_pcie_host_setup(pp);
> >   	if (ret)
> > -		goto err;
> > +		goto err_deinit;
> > +
> > +	ret = rcar_gen4_pcie_msi_irq_init(rcar);
> 
> Would it make sense to acquire the IRQ a bit earlier, so you could avoid
> rcar_gen4_pcie_host_perst_assert(pp, true); in the fail path ?
> 
> I think if the MSI acquisition fails, perst signal would pulse (rapid
> sequence of deassert and assert), and that could be avoided.

Good point, thanks. I agree that such a pulse is possible.

However, if we simply move the request earlier within .init(), before
deasserting PERST#, request_irq() would enable the line right away before the
controller is configured, and since .init()/.deinit() are also reused for
suspend/resume, keeping the request in .init() means freeing and re-requesting
the IRQ on every sleep cycle.

So for v3, I'm planning to move the request out of .init() entirely and into
probe, using IRQF_NO_AUTOEN. That way:
- The request happens before touching clocks, resets or PERST#, so a failure
  bails out before the hardware is touched.
- .init() only needs to enable the IRQ once the controller is set up, which
  cannot fail.
- The registration stays intact across suspend/resume, so .deinit() simply
  disables the IRQ and .init() re-enables it.

.. That said, this may well be what you had in mind in the first place. If so,
apologies for the detour.

> 
> > +	if (ret)
> > +		goto err_assert_perst;
> >   	return 0;
> > -err:
> > +err_assert_perst:
> > +	rcar_gen4_pcie_host_perst_assert(pp, true);
> > +err_deinit:
> >   	rcar->drvdata->deinit(rcar);
> >   	return ret;
> >   }
> > @@ -818,6 +878,9 @@ static void rcar_gen4_pcie_host_deinit(struct dw_pcie_rp *pp)
> >   	struct dw_pcie *dw = to_dw_pcie_from_pp(pp);
> >   	struct rcar_gen4_pcie *rcar = to_rcar_gen4_pcie(dw);
> > +	/* Stop the handler before asserting reset and disabling the clocks. */
> > +	free_irq(rcar->msi_irq, rcar);
> > +
> >   	rcar_gen4_pcie_host_perst_assert(pp, true);
> >   	rcar->drvdata->deinit(rcar);
> Shouldn't the IRQ be released only after reset is asserted and clock are
> stopped , otherwise it might accidentally fire and cause unhandled IRQ event
> ?

That would be a concern with a shared line, where the remaining handlers would
see spurious interrupts. This one is requested exclusively, so free_irq()
removes the last action and shuts the line down, ie. it is masked at the GIC
from then on, and nothing the controller does afterwards reaches a handler.

The order matters the other way round. The handler reads the APP status and, for
MSIs, DBI, so it must not run once the reset is asserted and the clocks are off,
or it would hang. That is why the handler is stopped first.

With the v3 plan above this becomes: .deinit() calls disable_irq(), which also
waits for a running handler to finish, before asserting the reset (again fine
because the line is ours alone), and the registration itself is released by
devres at driver detach, ie. only after the controller is down. I believe both
concerns will be eliminated by that design in v3.

Best regards,
Koichiro Den