[PATCH 0/3] Check for IRQ trigger type mismatch in __setup_irq()

[PATCH 0/3] Check for IRQ trigger type mismatch in __setup_irq()

[Manivannan Sadhasivam]

Hi,

This series adds a check for detecting the IRQ trigger type mismatch between the
platform (DT) and a device driver. Currently, if there is a mismatch, there
is no error thrown but the driver requested trigger gets set silently. Then
during the second time probe of a driver (due to probe defer or rmmod/insmod),
platform_get_irq() throws a warning similar to below and fails.

irq: type mismatch, failed to map hwirq-9 for interrupt-controller@b220000!

But ideally, during the first time itself, request_irq() should've failed as
the flag mismatch is a hard error. So let's add a check in __setup_irq(), such
that the request_irq() would fail if a mismatch has been detected.

NOTE: This might break platforms those has the flag set incorrectly in DT. One
of such case is SDX55, where the UART node has the trigger set incorrectly.
I fixed it in a couple of places I happen to know. But there could be many...

Thanks,
Mani

Manivannan Sadhasivam (3):
  ARM: dts: qcom: sdx55: Fix the IRQ trigger type for UART
  arm64: dts: qcom: sm8450: Fix the IRQ trigger type for remoteproc
    nodes
  genirq: Check for trigger type mismatch in __setup_irq()

 arch/arm/boot/dts/qcom-sdx55.dtsi    |  2 +-
 arch/arm64/boot/dts/qcom/sm8450.dtsi |  8 ++++----
 kernel/irq/manage.c                  | 14 ++++++++++++--
 3 files changed, 17 insertions(+), 7 deletions(-)

-- 
2.25.1

[PATCH 1/3] ARM: dts: qcom: sdx55: Fix the IRQ trigger type for UART

[Manivannan Sadhasivam]

The trigger type should be LEVEL_HIGH. So fix it!

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm/boot/dts/qcom-sdx55.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/qcom-sdx55.dtsi b/arch/arm/boot/dts/qcom-sdx55.dtsi
index d455795da44c..b75e672c239d 100644
--- a/arch/arm/boot/dts/qcom-sdx55.dtsi
+++ b/arch/arm/boot/dts/qcom-sdx55.dtsi
@@ -206,7 +206,7 @@ gcc: clock-controller@100000 {
 		blsp1_uart3: serial@831000 {
 			compatible = "qcom,msm-uartdm-v1.4", "qcom,msm-uartdm";
 			reg = <0x00831000 0x200>;
-			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_LOW>;
+			interrupts = <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&gcc 30>,
 				 <&gcc 9>;
 			clock-names = "core", "iface";
-- 
2.25.1

Re: (subset) [PATCH 1/3] ARM: dts: qcom: sdx55: Fix the IRQ trigger type for UART

[Bjorn Andersson]

On Mon, 30 May 2022 13:38:40 +0530, Manivannan Sadhasivam wrote:
> The trigger type should be LEVEL_HIGH. So fix it!
> 
> 

Applied, thanks!

[1/3] ARM: dts: qcom: sdx55: Fix the IRQ trigger type for UART
      commit: ae500b351ab0006d933d804a2b7507fe1e98cecc

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

[PATCH 2/3] arm64: dts: qcom: sm8450: Fix the IRQ trigger type for remoteproc nodes

[Manivannan Sadhasivam]

The watchdog IRQ trigger type should be EDGE_RISING. So fix all remoteproc
nodes.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8450.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 934e29b9e153..7c511901e52f 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -854,7 +854,7 @@ remoteproc_slpi: remoteproc@2400000 {
 			compatible = "qcom,sm8450-slpi-pas";
 			reg = <0 0x02400000 0 0x4000>;
 
-			interrupts-extended = <&pdc 9 IRQ_TYPE_LEVEL_HIGH>,
+			interrupts-extended = <&pdc 9 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_slpi_in 0 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_slpi_in 1 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_slpi_in 2 IRQ_TYPE_EDGE_RISING>,
@@ -894,7 +894,7 @@ remoteproc_adsp: remoteproc@30000000 {
 			compatible = "qcom,sm8450-adsp-pas";
 			reg = <0 0x030000000 0 0x100>;
 
-			interrupts-extended = <&pdc 6 IRQ_TYPE_LEVEL_HIGH>,
+			interrupts-extended = <&pdc 6 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_adsp_in 0 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_adsp_in 1 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_adsp_in 2 IRQ_TYPE_EDGE_RISING>,
@@ -934,7 +934,7 @@ remoteproc_cdsp: remoteproc@32300000 {
 			compatible = "qcom,sm8450-cdsp-pas";
 			reg = <0 0x032300000 0 0x1400000>;
 
-			interrupts-extended = <&intc GIC_SPI 578 IRQ_TYPE_LEVEL_HIGH>,
+			interrupts-extended = <&intc GIC_SPI 578 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_cdsp_in 0 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_cdsp_in 1 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_cdsp_in 2 IRQ_TYPE_EDGE_RISING>,
@@ -974,7 +974,7 @@ remoteproc_mpss: remoteproc@4080000 {
 			compatible = "qcom,sm8450-mpss-pas";
 			reg = <0x0 0x04080000 0x0 0x4040>;
 
-			interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_LEVEL_HIGH>,
+			interrupts-extended = <&intc GIC_SPI 264 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_modem_in 0 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_modem_in 1 IRQ_TYPE_EDGE_RISING>,
 					      <&smp2p_modem_in 2 IRQ_TYPE_EDGE_RISING>,
-- 
2.25.1

Re: [PATCH 2/3] arm64: dts: qcom: sm8450: Fix the IRQ trigger type for remoteproc nodes

[Dmitry Baryshkov]

On 30/05/2022 11:08, Manivannan Sadhasivam wrote:
> The watchdog IRQ trigger type should be EDGE_RISING. So fix all remoteproc
> nodes.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

BTW: Could you please send the same patches for sm8250/8350 too?

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] arm64: dts: qcom: sm8450: Fix the IRQ trigger type for remoteproc nodes

[Manivannan Sadhasivam]

On Tue, May 31, 2022 at 01:39:32AM +0300, Dmitry Baryshkov wrote:
> On 30/05/2022 11:08, Manivannan Sadhasivam wrote:
> > The watchdog IRQ trigger type should be EDGE_RISING. So fix all remoteproc
> > nodes.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> 
> BTW: Could you please send the same patches for sm8250/8350 too?
> 

Hmm... I thought I covered them but apparently missed. Fill fix them too.

Thanks,
Mani

> -- 
> With best wishes
> Dmitry

-- 
மணிவண்ணன் சதாசிவம்

Re: (subset) [PATCH 2/3] arm64: dts: qcom: sm8450: Fix the IRQ trigger type for remoteproc nodes

[Bjorn Andersson]

On Mon, 30 May 2022 13:38:41 +0530, Manivannan Sadhasivam wrote:
> The watchdog IRQ trigger type should be EDGE_RISING. So fix all remoteproc
> nodes.
> 
> 

Applied, thanks!

[2/3] arm64: dts: qcom: sm8450: Fix the IRQ trigger type for remoteproc nodes
      commit: 20402c94721a05fe3e581c1bfb88f0b45452766c

Best regards,
-- 
Bjorn Andersson <bjorn.andersson@linaro.org>

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

[Manivannan Sadhasivam]

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.

It should be noted that the platform trigger type could be IRQ_TYPE_NONE
in some cases like IRQ controller inside the GPIOCHIP. For those cases,
the check should be skipped.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 kernel/irq/manage.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index c03f71d5ec10..dd28c4944172 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1480,8 +1480,18 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	 * If the trigger type is not specified by the caller,
 	 * then use the default for this interrupt.
 	 */
-	if (!(new->flags & IRQF_TRIGGER_MASK))
-		new->flags |= irqd_get_trigger_type(&desc->irq_data);
+	flags = irqd_get_trigger_type(&desc->irq_data);
+	if (!(new->flags & IRQF_TRIGGER_MASK)) {
+		new->flags |= flags;
+	} else if (flags && ((new->flags & IRQF_TRIGGER_MASK) != flags)) {
+		/*
+		 * Bail out if the default trigger is not IRQ_TYPE_NONE and the
+		 * caller specified trigger does not match the default trigger type.
+		 */
+		pr_err("Trigger type %u does not match default type %lu for %s (irq %d)\n",
+		       new->flags & IRQF_TRIGGER_MASK, flags, new->name, irq);
+		return -EINVAL;
+	}
 
 	/*
 	 * Check whether the interrupt nests into another interrupt
-- 
2.25.1

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

[Marc Zyngier]

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.

Thanks,

	M.

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

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@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.

Regards
Luca

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

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

[Luca Weiss]

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.