2023-05-05 15:45:05

by Krzysztof Kozlowski

Subject: [PATCH] serial: qcom-geni: fix enabling deactivated interrupt

The driver have a race, experienced only with PREEMPT_RT patchset:

CPU0 | CPU1
==================================================================
qcom_geni_serial_probe |
uart_add_one_port |
| serdev_drv_probe
| qca_serdev_probe
| serdev_device_open
| uart_open
| uart_startup
| qcom_geni_serial_startup
| enable_irq
| __irq_startup
| WARN_ON()
| IRQ not activated
request_threaded_irq |
irq_domain_activate_irq |

The warning:

894000.serial: ttyHS1 at MMIO 0x894000 (irq = 144, base_baud = 0) is a MSM
serial serial0: tty port ttyHS1 registered
WARNING: CPU: 7 PID: 107 at kernel/irq/chip.c:241 __irq_startup+0x78/0xd8
...
qcom_geni_serial 894000.serial: serial engine reports 0 RX bytes in!

Adding UART port triggers probe of child serial devices - serdev and
eventually Qualcomm Bluetooth hci_qca driver. This opens UART port
which enables the interrupt before it got activated in
request_threaded_irq(). The issue originates in commit f3974413cf02
("tty: serial: qcom_geni_serial: Wakeup IRQ cleanup") and discussion on
mailing list [1]. However the above commit does not explain why the
uart_add_one_port() is moved above requesting interrupt.

[1] https://lore.kernel.org/all/[email protected]/

Fixes: f3974413cf02 ("tty: serial: qcom_geni_serial: Wakeup IRQ cleanup")
Cc: <[email protected]>
Cc: Stephen Boyd <[email protected]>
Signed-off-by: Krzysztof Kozlowski <[email protected]>
---
drivers/tty/serial/qcom_geni_serial.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 08dc3e2a729c..8582479f0211 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1664,19 +1664,18 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
uport->private_data = &port->private_data;
platform_set_drvdata(pdev, port);

- ret = uart_add_one_port(drv, uport);
- if (ret)
- return ret;
-
irq_set_status_flags(uport->irq, IRQ_NOAUTOEN);
ret = devm_request_irq(uport->dev, uport->irq, qcom_geni_serial_isr,
IRQF_TRIGGER_HIGH, port->name, uport);
if (ret) {
dev_err(uport->dev, "Failed to get IRQ ret %d\n", ret);
- uart_remove_one_port(drv, uport);
return ret;
}

+ ret = uart_add_one_port(drv, uport);
+ if (ret)
+ return ret;
+
/*
* Set pm_runtime status as ACTIVE so that wakeup_irq gets
* enabled/disabled from dev_pm_arm_wake_irq during system
--
2.34.1

2023-05-06 00:12:48

by Stephen Boyd

Subject: Re: [PATCH] serial: qcom-geni: fix enabling deactivated interrupt

Quoting Krzysztof Kozlowski (2023-05-05 08:23:01)
> The driver have a race, experienced only with PREEMPT_RT patchset:
>
> CPU0 | CPU1
> ==================================================================
> qcom_geni_serial_probe |
> uart_add_one_port |
> | serdev_drv_probe
> | qca_serdev_probe
> | serdev_device_open
> | uart_open
> | uart_startup
> | qcom_geni_serial_startup
> | enable_irq
> | __irq_startup
> | WARN_ON()
> | IRQ not activated
> request_threaded_irq |
> irq_domain_activate_irq |
>
> The warning:
>
> 894000.serial: ttyHS1 at MMIO 0x894000 (irq = 144, base_baud = 0) is a MSM
> serial serial0: tty port ttyHS1 registered
> WARNING: CPU: 7 PID: 107 at kernel/irq/chip.c:241 __irq_startup+0x78/0xd8
> ...
> qcom_geni_serial 894000.serial: serial engine reports 0 RX bytes in!
>
> Adding UART port triggers probe of child serial devices - serdev and
> eventually Qualcomm Bluetooth hci_qca driver. This opens UART port
> which enables the interrupt before it got activated in
> request_threaded_irq(). The issue originates in commit f3974413cf02
> ("tty: serial: qcom_geni_serial: Wakeup IRQ cleanup") and discussion on
> mailing list [1]. However the above commit does not explain why the
> uart_add_one_port() is moved above requesting interrupt.
>
> [1] https://lore.kernel.org/all/[email protected]/
>
> Fixes: f3974413cf02 ("tty: serial: qcom_geni_serial: Wakeup IRQ cleanup")
> Cc: <[email protected]>
> Cc: Stephen Boyd <[email protected]>
> Signed-off-by: Krzysztof Kozlowski <[email protected]>
> ---

Reviewed-by: Stephen Boyd <[email protected]>