Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Mon, 13 Feb 2023 09:46:11 +0100
Message-Id: <CQHAY4Z0W4HZ.18W214TYCYNUK@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:     "Luca Weiss" <luca.weiss@fairphone.com>,
        "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> <CO9EJJ4E661K.21S3LVYWH9HMM@otso>
In-Reply-To: <CO9EJJ4E661K.21S3LVYWH9HMM@otso>
Precedence: bulk

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:
> > >=20
> > > Currently, if the trigger type defined by the platform like DT does n=
ot
> > > match the driver requested trigger type, the below warning is shown
> > > during platform_get_irq() but only during the second time of the driv=
e
> > > probe (due to probe deferral or module unload/load).
> > >=20
> > > irq: type mismatch, failed to map hwirq-9 for interrupt-controller@b2=
20000!
> > >=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 =
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 s=
ets
> > > the driver requested trigger type. Later if the driver gets probed ag=
ain,
> > > platform_get_irq() throws the "type mismatch" warning and fails.
> > >=20
> > > Ideally, request_irq() should throw the error during the first time i=
tself,
> > > 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.
> >
> > --=20
> > Without deviation from the norm, progress is not possible.