linux-kernel - Re: [PATCH 3/3] genirq: Check for trigger type mismatch in __setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 11 Nov 2022 11:41:54 +0100
From:   "Luca Weiss" <luca.weiss@...rphone.com>
To:     "Marc Zyngier" <maz@...nel.org>,
        "Manivannan Sadhasivam" <manivannan.sadhasivam@...aro.org>
Cc:     <tglx@...utronix.de>, <bjorn.andersson@...aro.org>,
        <linux-kernel@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH 3/3] genirq: Check for trigger type mismatch in
 __setup_irq()

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@...aro.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@...0000!
> > 
> > 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.