Re: [PATCH 3/3] genirq: Check for trigger type mismatch in __setup_irq()

[Luca Weiss]

Hi Marc,

On Mon Jun 6, 2022 at 10:49 AM CEST, Marc Zyngier wrote:
> On Mon, 30 May 2022 09:08:42 +0100,
> Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> wrote:
> > 
> > Currently, if the trigger type defined by the platform like DT does not
> > match the driver requested trigger type, the below warning is shown
> > during platform_get_irq() but only during the second time of the drive
> > probe (due to probe deferral or module unload/load).
> > 
> > irq: type mismatch, failed to map hwirq-9 for interrupt-controller@b220000!
> > 
> > Consider a typical usecase of requesting an IRQ in a driver:
> > 
> > ```
> > 	/* Assume DT has set the trigger type to IRQF_TYPE_LEVEL_HIGH */
> > 
> > 	q6v5->wdog_irq = platform_get_irq_byname(pdev, "wdog");
> > 	if (q6v5->wdog_irq <= 0)
> > 		return q6v5->wdog_irq;
> > 
> > 	ret = devm_request_threaded_irq(&pdev->dev, q6v5->wdog_irq,
> > 					NULL, q6v5_wdog_interrupt,
> > 					IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > 					"q6v5 wdog", q6v5);
> > 	if (ret) {
> > 		dev_err(&pdev->dev, "failed to acquire wdog IRQ\n");
> > 		return ret;
> > 	}
> > ```
> > 
> > For the first time probe of a driver, platform_get_irq_byname() does not
> > return an error and it sets the platform requested trigger type. Then,
> > request_irq() also does not check for the trigger type mismatch and sets
> > the driver requested trigger type. Later if the driver gets probed again,
> > platform_get_irq() throws the "type mismatch" warning and fails.
> > 
> > Ideally, request_irq() should throw the error during the first time itself,
> > when it detects the trigger type mismatch. So let's add a check in
> > __setup_irq() for checking the trigger type mismatch.
>
> No, that's wrong. The whole point is to be able to *override* the
> default that is exposed by the device tree or ACPI. We have countless
> examples of that, and they cannot be broken.
>
> If the issue exists after an unload, then it is a unload time that the
> previous behaviour should be restored.

So you're saying something like that the drivers where this issue
appears don't free the irq properly? I've seen a very similar issue now
on qcom-sm6350 platform with dwc3-qcom and qcom_q6v5_pas drivers.

Temporarily I've adjusted dts to match the IRQ flags requested in the
driver to avoid these "failed to map hwirq-" errors. I'd be happy to fix
those drivers (if they should be) but I need some pointers here what
needs to be done as from my understanding if an irq is requested with
devm_request_threaded_irq (e.g. dwc3-qcom.c) then it should be freed
automatically on EPROBE_DEFER because of devm?

All commits in mainline that mention "failed to map hwirq" seem to just
remove the hardcoded IRQ type from the driver instead.

Regards
Luca

>
> Thanks,
>
> 	M.
>
> -- 
> Without deviation from the norm, progress is not possible.