Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Fri, 11 Nov 2022 11:41:54 +0100
Message-Id: <CO9EJJ4E661K.21S3LVYWH9HMM@otso>
Cc:     <tglx@linutronix.de>, <bjorn.andersson@linaro.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>
Subject: Re: [PATCH 3/3] genirq: Check for trigger type mismatch in
 __setup_irq()
From:   "Luca Weiss" <luca.weiss@fairphone.com>
To:     "Marc Zyngier" <maz@kernel.org>,
        "Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>
References: <20220530080842.37024-1-manivannan.sadhasivam@linaro.org>
 <20220530080842.37024-4-manivannan.sadhasivam@linaro.org>
 <8735gi5g7s.wl-maz@kernel.org>
In-Reply-To: <8735gi5g7s.wl-maz@kernel.org>
Precedence: bulk

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:
> >=20
> > 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).
> >=20
> > irq: type mismatch, failed to map hwirq-9 for interrupt-controller@b220=
000!
> >=20
> > Consider a typical usecase of requesting an IRQ in a driver:
> >=20
> > ```
> > 	/* Assume DT has set the trigger type to IRQF_TYPE_LEVEL_HIGH */
> >=20
> > 	q6v5->wdog_irq =3D platform_get_irq_byname(pdev, "wdog");
> > 	if (q6v5->wdog_irq <=3D 0)
> > 		return q6v5->wdog_irq;
> >=20
> > 	ret =3D 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;
> > 	}
> > ```
> >=20
> > For the first time probe of a driver, platform_get_irq_byname() does no=
t
> > return an error and it sets the platform requested trigger type. Then,
> > request_irq() also does not check for the trigger type mismatch and set=
s
> > the driver requested trigger type. Later if the driver gets probed agai=
n,
> > platform_get_irq() throws the "type mismatch" warning and fails.
> >=20
> > Ideally, request_irq() should throw the error during the first time its=
elf,
> > 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.
>
> --=20
> Without deviation from the norm, progress is not possible.