[PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Praveen Talari]

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

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Krzysztof Kozlowski]

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

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Praveen Talari]

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

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Jorge Ramirez]

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.

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Jorge Ramirez]

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):

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Krzysztof Kozlowski]

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

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Krzysztof Kozlowski]

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

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Bjorn Andersson]

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
> 

Re: [PATCH v2] serial: qcom_geni: Fix pinctrl deadlock on runtime resume

[Praveen Talari]

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