| Message ID | 20220530080842.37024-4-manivannan.sadhasivam@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Check for IRQ trigger type mismatch in __setup_irq() | expand |
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.
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.
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
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(-)