Re: [PATCH] serial: qcom-geni: add force suspend/resume to system sleep callbacks

[Praveen Talari]

Hi Mukesh

On 02-07-2026 14:47, Mukesh Savaliya wrote:
> Hi Praveen,
>
> On 7/2/2026 8:18 AM, Praveen Talari wrote:
> [...]
> > > > +     * When no_console_suspend is set the console must remain active
> > > > +     * across system sleep, so skip the force suspend path.
> > > > +     */
> > > > +    if (uart_console(uport) && !uport->suspended)
> > > > +        return 0;
> > > Rather use console_suspend_enabled and take action to go force suspend.
> > In uart_suspend_port(), uport->suspended is updated only after the 
> > console_suspend_enabled check. Therefore, its value directly reflects 
> > whether the console suspend path was taken:
> >
> > uport->suspended == 0 → the console was not suspended.
> > uport->suspended == 1 → the console was suspended.
> >
> > Looking at the code below, when console_suspend_enabled is disabled 
> > for a console port, the function returns before setting 
> > uport->suspended = 1. As a result, uport->suspended remains 0, which 
> > accurately indicates that the console was not suspended.
> > Therefore, I believe using uport->suspended is the more appropriate 
> > check here. Please let me know your thoughts.
>
> I think it would be good to use console_suspend_enabled and take 
> action. Not to depend on after effect of the primary decision maker 
> variable.

Yes but i don't see strong reason not to use uport->suspended.

> > Code snippet from core layer
> >
> > int uart_suspend_port(struct uart_driver *drv, struct uart_port *uport)
> > {
> > [...]
> >
> >      /*
> >       * Nothing to do if the console is not suspending
> >       * except stop_rx to prevent any asynchronous data
> >       * over RX line. However ensure that we will be
> >       * able to Re-start_rx later.
> >       */
> >      if (!console_suspend_enabled && uart_console(uport)) {
> >          if (uport->ops->start_rx) {
> >              guard(uart_port_lock_irq)(uport);
> >              uport->ops->stop_rx(uport);
> >          }
> >          device_set_awake_path(uport->dev);
> >          return 0;
> >      }
> >
> >      uport->suspended = 1;
> >
> >      if (tty_port_initialized(port)) {
> > [...]
> > }
> >
> > > Here, it sounds opposite, if port is resumed, you don't go to 
> > > suspend within suspend function.
> > It is straightforward: uport->suspended remains 0 even after 
> > uart_suspend_port() is called, which indicates that the console has 
> > not been suspended.
> > > > +
> > > > +    return pm_runtime_force_suspend(dev);
> > > Is this really required ? if  uart_suspend_port() successful, what 
> > > will happen with this ?
> >
> > Yes, this is covered in the commit message. The key point is that 
> > uart_suspend_port() may not trigger the runtime suspend callback if 
> > the runtime-PM usage count remains non-zero. In such cases, 
> > pm_runtime_force_suspend() is needed to ensure that the hardware 
> > resources are properly suspended during system sleep like our i2c/spi 
> > supported.
> I hardly see any other uart serial driver using 
> pm_runtime_force_suspend(), any do not see serial driver using runtime 
> PM ops directly. That being said, let's covered all use cases/testing 
> with this change.

It depends on driver design and using of PM runtime framework usage.

This change was tested two devices like system suspend and wakeup irq 
functionality.


Thanks,

Praveen Talari

> > > [...]