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

[Marek Vasut <marek.vasut@mailbox.org>]

On 10/5/26 7:05 PM, Koichiro Den wrote:

Hello Den-san,

I apologize for my delayed reply.

>>> +	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.

This is nice, thank you !

>>> +	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.
Thank you !