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

["Luca Weiss" <luca.weiss@fairphone.com>]

On Fri Nov 11, 2022 at 11:41 AM CET, Luca Weiss wrote:
> 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@linaro.org> 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.

I'm still interested in a way to fix this since I'm carrying a patch in
my own tree to adjust the dts. Please let me know how to proceed here.

Regards
Luca

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