Re: [PATCH v7 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Praveen Talari
2025-08-28T05:13:16Z

HI Alexey,

On 8/27/2025 1:24 AM, Alexey Klimov wrote:
> On Tue Aug 26, 2025 at 11:29 AM BST, Praveen Talari wrote:
>> Hi Alexey/Krzysztof,
>>
>>
>> On 8/26/2025 3:36 PM, Krzysztof Kozlowski wrote:
>>> On 26/08/2025 11:37, Alexey Klimov wrote:
>>>> On Tue Aug 26, 2025 at 10:21 AM BST, Krzysztof Kozlowski wrote:
>>>>> On 26/08/2025 11:18, Alexey Klimov wrote:
>>>>>>>> May i know what is testcase which you are running on target?
>>>>>>>
>>>>>>> Boot the board?
>>>>>>>
>>>>>>>> what is target?
>>>>>>>
>>>>>>> It is written in original report. Did you even read it?
>>>>>>>
>>>>>>>> Which usecase is this issue occurring in?
>>>>>>>
>>>>>>> Boot?
>>>>>>
>>>>>> FWIW, what said above by Krzysztof is correct, there is no usecase, just booting the board.
>>>>>>
>>>>> 12 days and nothing improved, right? if this was not dropped now,
>>>>> Alexey, can you send a revert? Author clearly approches stability with a
>>>>> very relaxed way and is just happy that patch was thrown over the wall
>>>>> and job is done.
>>>>>
>>>>>
>>>>> If you do not want to send revert, let me know, I will do it.
>>>>
>>>> I am okay with sending revert, just trying to see if there is any interest
>>>> in fixing this.
>>>
>>> Any interest should have happened after 1 day of reporting linux-next
>>> breakage. It has been like what? 12 days?
>>>
>>> That's typical throw the patch over the wall. Revert.
>>
>> Really sorry for the delay.
>>
>> I forgot to mention earlier that I’ve been actively investigating this
>> issue across different platform SoCs. I was able to reproduce the
>> problem on the SC7280.
>>
>> Here’s a summary of the observed behavior:
>>
>> The issue appears to originate from the qcom_geni_serial driver during
>> device runtime resume. It results in a blocked IRQ thread, which in turn
>> causes system instability.
>>
>> The call trace suggests a deadlock scenario where the IRQ
>> thread—responsible for handling wake-up events—becomes unresponsive
>> while interacting with the pinctrl subsystem.
>>
>> Specifically, the msm_pinmux_set_mux function attempts to invoke
>> disable_irq, which is problematic when called from an IRQ thread context.
>> Since the IRQ itself is a wake-up source, this leads to contention or a
>> self-deadlock situation.
>>
>> I have verified below diff and about to post it
>
> Was the original patch, that introduced the regression, also created by AI tools?
> Just trying to understand how we ended up with untested commit in -master.
>
> Did you test the change below on real hardware?

Yes, i have tested on SA7280 SOC.

Thanks,
Praveen Talari
>
>
>> diff --git a/drivers/tty/serial/qcom_geni_serial.c
>> b/drivers/tty/serial/qcom_geni_serial.c
>> index c9c52c52a98d..cb3b4febd8c2 100644
>> --- a/drivers/tty/serial/qcom_geni_serial.c
>> +++ b/drivers/tty/serial/qcom_geni_serial.c
>> @@ -1848,16 +1848,36 @@ static int __maybe_unused
>> qcom_geni_serial_runtime_suspend(struct device *dev)
>> {
>> struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>> struct uart_port *uport = &port->uport;
>> + int ret;
>> +
>> + ret = geni_serial_resources_off(uport);
>> + if(ret) {
>> + if (device_may_wakeup(dev))
>> + disable_irq_wake(port->wakeup_irq);
>> + }
>>
>> - return geni_serial_resources_off(uport);
>> + if (device_may_wakeup(dev))
>> + enable_irq_wake(port->wakeup_irq);
>> +
>> + return ret;
>> }
>>
>> static int __maybe_unused qcom_geni_serial_runtime_resume(struct
>> device *dev)
>> {
>> struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
>> struct uart_port *uport = &port->uport;
>> + int ret;
>> +
>> + if (device_may_wakeup(dev))
>> + disable_irq_wake(port->wakeup_irq);
>>
>> - return geni_serial_resources_on(uport);
>> + ret = geni_serial_resources_on(uport);
>> + if(ret) {
>> + if (device_may_wakeup(dev))
>> + enable_irq_wake(port->wakeup_irq);
>> + }
>> +
>> + return ret;
>> }
>
> Best regards,
> Alexey
>