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

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

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