drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)
A stall was observed in disable_irq() during
pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active
while the UART port was not yet active. This led to a hang in
__synchronize_irq(), as shown in the following trace:
Call trace:
__switch_to+0xe0/0x120
__schedule+0x39c/0x978
schedule+0x5c/0xf8
__synchronize_irq+0x88/0xb4
disable_irq+0x3c/0x4c
msm_pinmux_set_mux+0x508/0x644
pinmux_enable_setting+0x190/0x2dc
pinctrl_commit_state+0x13c/0x208
pinctrl_pm_select_default_state+0x4c/0xa4
geni_se_resources_on+0xe8/0x154
qcom_geni_serial_runtime_resume+0x4c/0x88
pm_generic_runtime_resume+0x2c/0x44
__genpd_runtime_resume+0x30/0x80
genpd_runtime_resume+0x114/0x29c
__rpm_callback+0x48/0x1d8
rpm_callback+0x6c/0x78
rpm_resume+0x530/0x750
__pm_runtime_resume+0x50/0x94
handle_threaded_wake_irq+0x30/0x94
irq_thread_fn+0x2c/0xa8
irq_thread+0x160/0x248
kthread+0x110/0x114
ret_from_fork+0x10/0x20
To fix this, wakeup IRQ setup is moved from probe to UART startup,
ensuring it is only configured when the port is active. Correspondingly,
the wakeup IRQ is cleared during shutdown. This avoids premature IRQ
disable during pinctrl setup and prevents the observed stall. The probe
and remove pathsare simplified by removing redundant wakeup IRQ handling.
Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver")
Reported-by: Alexey Klimov <alexey.klimov@linaro.org>
Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/
Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com>
Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
---
v1 -> v2:
- remove changes from runtime resume/suspend.
- updated commit text based on changes.
- added new a change w.r.t wakeup IRQ setup.
- verified on RB1 (qrb2210-rb1-core-kit).
---
---
drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index 0fdda3a1e70b..9c1bd4e5852c 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1160,6 +1160,7 @@ static int setup_fifos(struct qcom_geni_serial_port *port)
static void qcom_geni_serial_shutdown(struct uart_port *uport)
{
+ struct qcom_geni_serial_port *port = to_dev_port(uport);
disable_irq(uport->irq);
uart_port_lock_irq(uport);
@@ -1168,6 +1169,8 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport)
qcom_geni_serial_cancel_tx_cmd(uport);
uart_port_unlock_irq(uport);
+ if (port->wakeup_irq > 0)
+ dev_pm_clear_wake_irq(uport->dev);
}
static void qcom_geni_serial_flush_buffer(struct uart_port *uport)
@@ -1236,6 +1239,13 @@ static int qcom_geni_serial_startup(struct uart_port *uport)
return ret;
}
+ if (port->wakeup_irq > 0) {
+ ret = dev_pm_set_dedicated_wake_irq(uport->dev,
+ port->wakeup_irq);
+ if (ret)
+ return ret;
+ }
+
uart_port_lock_irq(uport);
qcom_geni_serial_start_rx(uport);
uart_port_unlock_irq(uport);
@@ -1888,17 +1898,8 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
if (ret)
goto error;
- if (port->wakeup_irq > 0) {
+ if (port->wakeup_irq > 0)
device_init_wakeup(&pdev->dev, true);
- ret = dev_pm_set_dedicated_wake_irq(&pdev->dev,
- port->wakeup_irq);
- if (ret) {
- device_init_wakeup(&pdev->dev, false);
- ida_free(&port_ida, uport->line);
- uart_remove_one_port(drv, uport);
- goto error;
- }
- }
return 0;
@@ -1913,7 +1914,6 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
struct uart_port *uport = &port->uport;
struct uart_driver *drv = port->private_data.drv;
- dev_pm_clear_wake_irq(&pdev->dev);
device_init_wakeup(&pdev->dev, false);
ida_free(&port_ida, uport->line);
uart_remove_one_port(drv, &port->uport);
base-commit: 3e8e5822146bc396d2a7e5fbb7be13271665522a
--
2.34.1
On Thu, Sep 18, 2025 at 12:21:02AM +0530, Praveen Talari wrote: > A stall was observed in disable_irq() during > pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active > while the UART port was not yet active. This led to a hang in > __synchronize_irq(), as shown in the following trace: Are you saying that the handle_threaded_wake_irq() in your callstack is the handler for @irq and then in pinctrl_pm_select_default_state() the code tries to disable that same @irq? > > Call trace: > __switch_to+0xe0/0x120 > __schedule+0x39c/0x978 > schedule+0x5c/0xf8 > __synchronize_irq+0x88/0xb4 > disable_irq+0x3c/0x4c > msm_pinmux_set_mux+0x508/0x644 > pinmux_enable_setting+0x190/0x2dc > pinctrl_commit_state+0x13c/0x208 > pinctrl_pm_select_default_state+0x4c/0xa4 > geni_se_resources_on+0xe8/0x154 > qcom_geni_serial_runtime_resume+0x4c/0x88 > pm_generic_runtime_resume+0x2c/0x44 > __genpd_runtime_resume+0x30/0x80 > genpd_runtime_resume+0x114/0x29c > __rpm_callback+0x48/0x1d8 > rpm_callback+0x6c/0x78 > rpm_resume+0x530/0x750 > __pm_runtime_resume+0x50/0x94 > handle_threaded_wake_irq+0x30/0x94 > irq_thread_fn+0x2c/0xa8 > irq_thread+0x160/0x248 > kthread+0x110/0x114 > ret_from_fork+0x10/0x20 > > To fix this, wakeup IRQ setup is moved from probe to UART startup, > ensuring it is only configured when the port is active. Correspondingly, > the wakeup IRQ is cleared during shutdown. The wakeup_irq is the GPIO pin, and the reason why we do this dance of muxing in the GPIO during suspend is so that we can get woken up when the system is powered down. Doesn't what you describe here disable that mechanism? In fact, while the UART is up and running, we don't need wakeup interrupts enabled, because the pin is muxed to the QUP. > This avoids premature IRQ > disable during pinctrl setup and prevents the observed stall. The scheme of pinmuxing a pad to GPIO function, in order to allow powering down the IP block and still be woken up is an important feature, so are we certain that this should be fixed on this side? The purpose of the disable_irq() in msm_pinmux_set_mux() is to reconfigure TLMM (and PDC presumably) to not give us interrupts while we've muxed the pin to a non-GPIO function. But why does that need to be synchronized? It seems worst case there's a parallel thread (or this thread) handling the interrupt right now and then there won't be any more. I.e. isn't the solution to this problem to change disable_irq() in msm_pinmux_set_mux() to disable_irq_no_sync()? Regards, Bjorn > The probe > and remove pathsare simplified by removing redundant wakeup IRQ handling. > > Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver") > Reported-by: Alexey Klimov <alexey.klimov@linaro.org> > Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/ > Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com> > Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com> > --- > v1 -> v2: > - remove changes from runtime resume/suspend. > - updated commit text based on changes. > - added new a change w.r.t wakeup IRQ setup. > - verified on RB1 (qrb2210-rb1-core-kit). > --- > --- > drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++----------- > 1 file changed, 11 insertions(+), 11 deletions(-) > > diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c > index 0fdda3a1e70b..9c1bd4e5852c 100644 > --- a/drivers/tty/serial/qcom_geni_serial.c > +++ b/drivers/tty/serial/qcom_geni_serial.c > @@ -1160,6 +1160,7 @@ static int setup_fifos(struct qcom_geni_serial_port *port) > > static void qcom_geni_serial_shutdown(struct uart_port *uport) > { > + struct qcom_geni_serial_port *port = to_dev_port(uport); > disable_irq(uport->irq); > > uart_port_lock_irq(uport); > @@ -1168,6 +1169,8 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport) > > qcom_geni_serial_cancel_tx_cmd(uport); > uart_port_unlock_irq(uport); > + if (port->wakeup_irq > 0) > + dev_pm_clear_wake_irq(uport->dev); > } > > static void qcom_geni_serial_flush_buffer(struct uart_port *uport) > @@ -1236,6 +1239,13 @@ static int qcom_geni_serial_startup(struct uart_port *uport) > return ret; > } > > + if (port->wakeup_irq > 0) { > + ret = dev_pm_set_dedicated_wake_irq(uport->dev, > + port->wakeup_irq); > + if (ret) > + return ret; > + } > + > uart_port_lock_irq(uport); > qcom_geni_serial_start_rx(uport); > uart_port_unlock_irq(uport); > @@ -1888,17 +1898,8 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) > if (ret) > goto error; > > - if (port->wakeup_irq > 0) { > + if (port->wakeup_irq > 0) > device_init_wakeup(&pdev->dev, true); > - ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, > - port->wakeup_irq); > - if (ret) { > - device_init_wakeup(&pdev->dev, false); > - ida_free(&port_ida, uport->line); > - uart_remove_one_port(drv, uport); > - goto error; > - } > - } > > return 0; > > @@ -1913,7 +1914,6 @@ static void qcom_geni_serial_remove(struct platform_device *pdev) > struct uart_port *uport = &port->uport; > struct uart_driver *drv = port->private_data.drv; > > - dev_pm_clear_wake_irq(&pdev->dev); > device_init_wakeup(&pdev->dev, false); > ida_free(&port_ida, uport->line); > uart_remove_one_port(drv, &port->uport); > > base-commit: 3e8e5822146bc396d2a7e5fbb7be13271665522a > -- > 2.34.1 >
Hi Bjorn On 9/18/2025 7:44 AM, Bjorn Andersson wrote: > On Thu, Sep 18, 2025 at 12:21:02AM +0530, Praveen Talari wrote: >> A stall was observed in disable_irq() during >> pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active >> while the UART port was not yet active. This led to a hang in >> __synchronize_irq(), as shown in the following trace: > > Are you saying that the handle_threaded_wake_irq() in your callstack is > the handler for @irq and then in pinctrl_pm_select_default_state() the > code tries to disable that same @irq? Yes, you are right. > >> >> Call trace: >> __switch_to+0xe0/0x120 >> __schedule+0x39c/0x978 >> schedule+0x5c/0xf8 >> __synchronize_irq+0x88/0xb4 >> disable_irq+0x3c/0x4c >> msm_pinmux_set_mux+0x508/0x644 >> pinmux_enable_setting+0x190/0x2dc >> pinctrl_commit_state+0x13c/0x208 >> pinctrl_pm_select_default_state+0x4c/0xa4 >> geni_se_resources_on+0xe8/0x154 >> qcom_geni_serial_runtime_resume+0x4c/0x88 >> pm_generic_runtime_resume+0x2c/0x44 >> __genpd_runtime_resume+0x30/0x80 >> genpd_runtime_resume+0x114/0x29c >> __rpm_callback+0x48/0x1d8 >> rpm_callback+0x6c/0x78 >> rpm_resume+0x530/0x750 >> __pm_runtime_resume+0x50/0x94 >> handle_threaded_wake_irq+0x30/0x94 >> irq_thread_fn+0x2c/0xa8 >> irq_thread+0x160/0x248 >> kthread+0x110/0x114 >> ret_from_fork+0x10/0x20 >> >> To fix this, wakeup IRQ setup is moved from probe to UART startup, >> ensuring it is only configured when the port is active. Correspondingly, >> the wakeup IRQ is cleared during shutdown. > > The wakeup_irq is the GPIO pin, and the reason why we do this dance of > muxing in the GPIO during suspend is so that we can get woken up when > the system is powered down. > > Doesn't what you describe here disable that mechanism? Yes. > > In fact, while the UART is up and running, we don't need wakeup > interrupts enabled, because the pin is muxed to the QUP. Yes. > >> This avoids premature IRQ >> disable during pinctrl setup and prevents the observed stall. > > The scheme of pinmuxing a pad to GPIO function, in order to allow > powering down the IP block and still be woken up is an important > feature, so are we certain that this should be fixed on this side? > > The purpose of the disable_irq() in msm_pinmux_set_mux() is to > reconfigure TLMM (and PDC presumably) to not give us interrupts while > we've muxed the pin to a non-GPIO function. But why does that need to be > synchronized? It seems worst case there's a parallel thread (or this > thread) handling the interrupt right now and then there won't be any > more. > > I.e. isn't the solution to this problem to change disable_irq() in > msm_pinmux_set_mux() to disable_irq_no_sync()? Based on the initial call stack and code flow analysis, we initially assumed that invoking disable_irq_nosync would be sufficient to unblock the issue. However, we observed a storm of interrupts on that IRQ line. Further investigation revealed that the UART port should not enter runtime suspend while it is open, as per the intended use case. Addressing this behavior resolved both the IRQ storm and the system hang issues. > > Regards, > Bjorn > >> The probe >> and remove pathsare simplified by removing redundant wakeup IRQ handling. >> >> Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver") >> Reported-by: Alexey Klimov <alexey.klimov@linaro.org> >> Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/ >> Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com> >> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com> >> --- >> v1 -> v2: >> - remove changes from runtime resume/suspend. >> - updated commit text based on changes. >> - added new a change w.r.t wakeup IRQ setup. >> - verified on RB1 (qrb2210-rb1-core-kit). >> --- >> --- >> drivers/tty/serial/qcom_geni_serial.c | 22 +++++++++++----------- >> 1 file changed, 11 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c >> index 0fdda3a1e70b..9c1bd4e5852c 100644 >> --- a/drivers/tty/serial/qcom_geni_serial.c >> +++ b/drivers/tty/serial/qcom_geni_serial.c >> @@ -1160,6 +1160,7 @@ static int setup_fifos(struct qcom_geni_serial_port *port) >> >> static void qcom_geni_serial_shutdown(struct uart_port *uport) >> { >> + struct qcom_geni_serial_port *port = to_dev_port(uport); >> disable_irq(uport->irq); >> >> uart_port_lock_irq(uport); >> @@ -1168,6 +1169,8 @@ static void qcom_geni_serial_shutdown(struct uart_port *uport) >> >> qcom_geni_serial_cancel_tx_cmd(uport); >> uart_port_unlock_irq(uport); >> + if (port->wakeup_irq > 0) >> + dev_pm_clear_wake_irq(uport->dev); >> } >> >> static void qcom_geni_serial_flush_buffer(struct uart_port *uport) >> @@ -1236,6 +1239,13 @@ static int qcom_geni_serial_startup(struct uart_port *uport) >> return ret; >> } >> >> + if (port->wakeup_irq > 0) { >> + ret = dev_pm_set_dedicated_wake_irq(uport->dev, >> + port->wakeup_irq); >> + if (ret) >> + return ret; >> + } >> + >> uart_port_lock_irq(uport); >> qcom_geni_serial_start_rx(uport); >> uart_port_unlock_irq(uport); >> @@ -1888,17 +1898,8 @@ static int qcom_geni_serial_probe(struct platform_device *pdev) >> if (ret) >> goto error; >> >> - if (port->wakeup_irq > 0) { >> + if (port->wakeup_irq > 0) >> device_init_wakeup(&pdev->dev, true); >> - ret = dev_pm_set_dedicated_wake_irq(&pdev->dev, >> - port->wakeup_irq); >> - if (ret) { >> - device_init_wakeup(&pdev->dev, false); >> - ida_free(&port_ida, uport->line); >> - uart_remove_one_port(drv, uport); >> - goto error; >> - } >> - } >> >> return 0; >> >> @@ -1913,7 +1914,6 @@ static void qcom_geni_serial_remove(struct platform_device *pdev) >> struct uart_port *uport = &port->uport; >> struct uart_driver *drv = port->private_data.drv; >> >> - dev_pm_clear_wake_irq(&pdev->dev); >> device_init_wakeup(&pdev->dev, false); >> ida_free(&port_ida, uport->line); >> uart_remove_one_port(drv, &port->uport); >> >> base-commit: 3e8e5822146bc396d2a7e5fbb7be13271665522a >> -- >> 2.34.1 >>
On 18/09/2025 03:51, Praveen Talari wrote: > A stall was observed in disable_irq() during > pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active > while the UART port was not yet active. This led to a hang in > __synchronize_irq(), as shown in the following trace: > > Call trace: > __switch_to+0xe0/0x120 > __schedule+0x39c/0x978 > schedule+0x5c/0xf8 > __synchronize_irq+0x88/0xb4 > disable_irq+0x3c/0x4c > msm_pinmux_set_mux+0x508/0x644 > pinmux_enable_setting+0x190/0x2dc > pinctrl_commit_state+0x13c/0x208 > pinctrl_pm_select_default_state+0x4c/0xa4 > geni_se_resources_on+0xe8/0x154 > qcom_geni_serial_runtime_resume+0x4c/0x88 > pm_generic_runtime_resume+0x2c/0x44 > __genpd_runtime_resume+0x30/0x80 > genpd_runtime_resume+0x114/0x29c > __rpm_callback+0x48/0x1d8 > rpm_callback+0x6c/0x78 > rpm_resume+0x530/0x750 > __pm_runtime_resume+0x50/0x94 > handle_threaded_wake_irq+0x30/0x94 > irq_thread_fn+0x2c/0xa8 > irq_thread+0x160/0x248 > kthread+0x110/0x114 > ret_from_fork+0x10/0x20 > > To fix this, wakeup IRQ setup is moved from probe to UART startup, > ensuring it is only configured when the port is active. Correspondingly, > the wakeup IRQ is cleared during shutdown. This avoids premature IRQ > disable during pinctrl setup and prevents the observed stall. The probe > and remove pathsare simplified by removing redundant wakeup IRQ handling. > > Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver") > Reported-by: Alexey Klimov <alexey.klimov@linaro.org> > Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/ > Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com> Where did you receive this tag for this patch exactly? Best regards, Krzysztof
Hi Krzysztof, On 9/18/2025 5:28 AM, Krzysztof Kozlowski wrote: > On 18/09/2025 03:51, Praveen Talari wrote: >> A stall was observed in disable_irq() during >> pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active >> while the UART port was not yet active. This led to a hang in >> __synchronize_irq(), as shown in the following trace: >> >> Call trace: >> __switch_to+0xe0/0x120 >> __schedule+0x39c/0x978 >> schedule+0x5c/0xf8 >> __synchronize_irq+0x88/0xb4 >> disable_irq+0x3c/0x4c >> msm_pinmux_set_mux+0x508/0x644 >> pinmux_enable_setting+0x190/0x2dc >> pinctrl_commit_state+0x13c/0x208 >> pinctrl_pm_select_default_state+0x4c/0xa4 >> geni_se_resources_on+0xe8/0x154 >> qcom_geni_serial_runtime_resume+0x4c/0x88 >> pm_generic_runtime_resume+0x2c/0x44 >> __genpd_runtime_resume+0x30/0x80 >> genpd_runtime_resume+0x114/0x29c >> __rpm_callback+0x48/0x1d8 >> rpm_callback+0x6c/0x78 >> rpm_resume+0x530/0x750 >> __pm_runtime_resume+0x50/0x94 >> handle_threaded_wake_irq+0x30/0x94 >> irq_thread_fn+0x2c/0xa8 >> irq_thread+0x160/0x248 >> kthread+0x110/0x114 >> ret_from_fork+0x10/0x20 >> >> To fix this, wakeup IRQ setup is moved from probe to UART startup, >> ensuring it is only configured when the port is active. Correspondingly, >> the wakeup IRQ is cleared during shutdown. This avoids premature IRQ >> disable during pinctrl setup and prevents the observed stall. The probe >> and remove pathsare simplified by removing redundant wakeup IRQ handling. >> >> Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver") >> Reported-by: Alexey Klimov <alexey.klimov@linaro.org> >> Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/ >> Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com> > > Where did you receive this tag for this patch exactly? Since Jorge was involved in validating the change, I’ve added him under the Tested-by tag. Please correct me if I’m not supposed to add this tag myself. Thanks, Praveen Talari > > Best regards, > Krzysztof
On 18/09/2025 12:55, Praveen Talari wrote: > Hi Krzysztof, > > On 9/18/2025 5:28 AM, Krzysztof Kozlowski wrote: >> On 18/09/2025 03:51, Praveen Talari wrote: >>> A stall was observed in disable_irq() during >>> pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active >>> while the UART port was not yet active. This led to a hang in >>> __synchronize_irq(), as shown in the following trace: >>> >>> Call trace: >>> __switch_to+0xe0/0x120 >>> __schedule+0x39c/0x978 >>> schedule+0x5c/0xf8 >>> __synchronize_irq+0x88/0xb4 >>> disable_irq+0x3c/0x4c >>> msm_pinmux_set_mux+0x508/0x644 >>> pinmux_enable_setting+0x190/0x2dc >>> pinctrl_commit_state+0x13c/0x208 >>> pinctrl_pm_select_default_state+0x4c/0xa4 >>> geni_se_resources_on+0xe8/0x154 >>> qcom_geni_serial_runtime_resume+0x4c/0x88 >>> pm_generic_runtime_resume+0x2c/0x44 >>> __genpd_runtime_resume+0x30/0x80 >>> genpd_runtime_resume+0x114/0x29c >>> __rpm_callback+0x48/0x1d8 >>> rpm_callback+0x6c/0x78 >>> rpm_resume+0x530/0x750 >>> __pm_runtime_resume+0x50/0x94 >>> handle_threaded_wake_irq+0x30/0x94 >>> irq_thread_fn+0x2c/0xa8 >>> irq_thread+0x160/0x248 >>> kthread+0x110/0x114 >>> ret_from_fork+0x10/0x20 >>> >>> To fix this, wakeup IRQ setup is moved from probe to UART startup, >>> ensuring it is only configured when the port is active. Correspondingly, >>> the wakeup IRQ is cleared during shutdown. This avoids premature IRQ >>> disable during pinctrl setup and prevents the observed stall. The probe >>> and remove pathsare simplified by removing redundant wakeup IRQ handling. >>> >>> Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver") >>> Reported-by: Alexey Klimov <alexey.klimov@linaro.org> >>> Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/ >>> Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com> >> >> Where did you receive this tag for this patch exactly? > > Since Jorge was involved in validating the change, I’ve added him under > the Tested-by tag. > > Please correct me if I’m not supposed to add this tag myself. Yes, it is wrong. You did not receive the tag! Which process or document, suggested that you can create such tag? I commented on the patches, so you will add now "Reviewed-by" with my name? No. Best regards, Krzysztof
On 18/09/25 09:25:53, Praveen Talari wrote: > Hi Krzysztof, > > On 9/18/2025 5:28 AM, Krzysztof Kozlowski wrote: > > On 18/09/2025 03:51, Praveen Talari wrote: > > > A stall was observed in disable_irq() during > > > pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active > > > while the UART port was not yet active. This led to a hang in > > > __synchronize_irq(), as shown in the following trace: > > > > > > Call trace: > > > __switch_to+0xe0/0x120 > > > __schedule+0x39c/0x978 > > > schedule+0x5c/0xf8 > > > __synchronize_irq+0x88/0xb4 > > > disable_irq+0x3c/0x4c > > > msm_pinmux_set_mux+0x508/0x644 > > > pinmux_enable_setting+0x190/0x2dc > > > pinctrl_commit_state+0x13c/0x208 > > > pinctrl_pm_select_default_state+0x4c/0xa4 > > > geni_se_resources_on+0xe8/0x154 > > > qcom_geni_serial_runtime_resume+0x4c/0x88 > > > pm_generic_runtime_resume+0x2c/0x44 > > > __genpd_runtime_resume+0x30/0x80 > > > genpd_runtime_resume+0x114/0x29c > > > __rpm_callback+0x48/0x1d8 > > > rpm_callback+0x6c/0x78 > > > rpm_resume+0x530/0x750 > > > __pm_runtime_resume+0x50/0x94 > > > handle_threaded_wake_irq+0x30/0x94 > > > irq_thread_fn+0x2c/0xa8 > > > irq_thread+0x160/0x248 > > > kthread+0x110/0x114 > > > ret_from_fork+0x10/0x20 > > > > > > To fix this, wakeup IRQ setup is moved from probe to UART startup, > > > ensuring it is only configured when the port is active. Correspondingly, > > > the wakeup IRQ is cleared during shutdown. This avoids premature IRQ > > > disable during pinctrl setup and prevents the observed stall. The probe > > > and remove pathsare simplified by removing redundant wakeup IRQ handling. > > > > > > Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver") > > > Reported-by: Alexey Klimov <alexey.klimov@linaro.org> > > > Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/ > > > Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com> > > > > Where did you receive this tag for this patch exactly? > > Since Jorge was involved in validating the change, I’ve added him under the > Tested-by tag. > > Please correct me if I’m not supposed to add this tag myself. let's test a bit further Praveen - we need to validate/trace the wake path on a real scenairo to make sure it is not cpu intensive (although I suspect the 2% was due to the storm you described more than to the code path itself) I can then provide the tested-by on the list.
On 18/09/25 09:25:48, Jorge Ramirez wrote: > On 18/09/25 09:25:53, Praveen Talari wrote: > > Hi Krzysztof, > > > > On 9/18/2025 5:28 AM, Krzysztof Kozlowski wrote: > > > On 18/09/2025 03:51, Praveen Talari wrote: > > > > A stall was observed in disable_irq() during > > > > pinctrl_pm_select_default_state(), triggered by wakeup IRQ being active > > > > while the UART port was not yet active. This led to a hang in > > > > __synchronize_irq(), as shown in the following trace: > > > > > > > > Call trace: > > > > __switch_to+0xe0/0x120 > > > > __schedule+0x39c/0x978 > > > > schedule+0x5c/0xf8 > > > > __synchronize_irq+0x88/0xb4 > > > > disable_irq+0x3c/0x4c > > > > msm_pinmux_set_mux+0x508/0x644 > > > > pinmux_enable_setting+0x190/0x2dc > > > > pinctrl_commit_state+0x13c/0x208 > > > > pinctrl_pm_select_default_state+0x4c/0xa4 > > > > geni_se_resources_on+0xe8/0x154 > > > > qcom_geni_serial_runtime_resume+0x4c/0x88 > > > > pm_generic_runtime_resume+0x2c/0x44 > > > > __genpd_runtime_resume+0x30/0x80 > > > > genpd_runtime_resume+0x114/0x29c > > > > __rpm_callback+0x48/0x1d8 > > > > rpm_callback+0x6c/0x78 > > > > rpm_resume+0x530/0x750 > > > > __pm_runtime_resume+0x50/0x94 > > > > handle_threaded_wake_irq+0x30/0x94 > > > > irq_thread_fn+0x2c/0xa8 > > > > irq_thread+0x160/0x248 > > > > kthread+0x110/0x114 > > > > ret_from_fork+0x10/0x20 > > > > > > > > To fix this, wakeup IRQ setup is moved from probe to UART startup, > > > > ensuring it is only configured when the port is active. Correspondingly, > > > > the wakeup IRQ is cleared during shutdown. This avoids premature IRQ > > > > disable during pinctrl setup and prevents the observed stall. The probe > > > > and remove pathsare simplified by removing redundant wakeup IRQ handling. > > > > > > > > Fixes: 1afa70632c39 ("serial: qcom-geni: Enable PM runtime for serial driver") > > > > Reported-by: Alexey Klimov <alexey.klimov@linaro.org> > > > > Closes: https://lore.kernel.org/all/DC0D53ZTNOBU.E8LSD5E5Z8TX@linaro.org/ > > > > Tested-by: Jorge Ramirez <jorge.ramirez@oss.qualcomm.com> > > > > > > Where did you receive this tag for this patch exactly? > > > > Since Jorge was involved in validating the change, I’ve added him under the > > Tested-by tag. > > > > Please correct me if I’m not supposed to add this tag myself. > > let's test a bit further Praveen - we need to validate/trace the wake > path on a real scenairo to make sure it is not cpu intensive (although I > suspect the 2% was due to the storm you described more than to the code > path itself) > > I can then provide the tested-by on the list. > um bluetooh comms are broken - reverting the runtime_pm patch fixes it. and the proposed fix (V2) does not address this scenario. I agree with the common sentiment, I think the patch should be reverted in linux-next and better test definition shared. [ 1.451715] Bluetooth: Core ver 2.22 [ 1.460668] Bluetooth: HCI device and connection manager initialized [ 1.467034] Bluetooth: HCI socket layer initialized [ 1.471922] Bluetooth: L2CAP socket layer initialized [ 1.476988] Bluetooth: SCO socket layer initialized [ 2.504958] Bluetooth: HCI UART driver ver 2.3 [ 2.509427] Bluetooth: HCI UART protocol H4 registered [ 2.514600] Bluetooth: HCI UART protocol LL registered [ 2.519978] Bluetooth: HCI UART protocol Broadcom registered [ 2.525662] Bluetooth: HCI UART protocol QCA registered [ 2.530915] Bluetooth: HCI UART protocol Marvell registered [ 2.764571] Bluetooth: HIDP (Human Interface Emulation) ver 1.2 [ 2.770503] Bluetooth: HIDP socket layer initialized [ 3.901958] Bluetooth: hci0: setting up wcn399x [ 6.202761] Bluetooth: hci0: command 0xfc00 tx timeout [ 6.212294] Bluetooth: hci0: Reading QCA version information failed (-110) [ 6.219261] Bluetooth: hci0: Retry BT power ON:0 [ 8.538729] Bluetooth: hci0: command 0xfc00 tx timeout [ 8.543988] Bluetooth: hci0: Reading QCA version information failed (-110) [ 8.550989] Bluetooth: hci0: Retry BT power ON:1 [ 10.810736] Bluetooth: hci0: command 0xfc00 tx timeout [ 10.816095] Bluetooth: hci0: Reading QCA version information failed (-110) [ 10.816110] Bluetooth: hci0: Retry BT power ON:2 [ 13.082946] Bluetooth: hci0: command 0xfc00 tx timeout [ 13.088490] Bluetooth: hci0: Reading QCA version information failed (-110):
On 18/09/2025 19:07, Jorge Ramirez wrote: > On 18/09/25 09:25:48, Jorge Ramirez wrote: >> >> let's test a bit further Praveen - we need to validate/trace the wake >> path on a real scenairo to make sure it is not cpu intensive (although I >> suspect the 2% was due to the storm you described more than to the code >> path itself) >> >> I can then provide the tested-by on the list. >> > > um bluetooh comms are broken - reverting the runtime_pm patch fixes it. > and the proposed fix (V2) does not address this scenario. > > I agree with the common sentiment, I think the patch should be reverted > in linux-next and better test definition shared. For the record, the revert was already applied. Any new patch here should carry some more tested-by, before it can get applied. Best regards, Krzysztof
© 2016 - 2025 Red Hat, Inc.