[1/1,SRU,M,Unstable] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: replace chars_in_buffer() with flush_chars()

From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>

BugLink: https://bugs.launchpad.net/bugs/2032174

After v6.5, calls tty->driver->ops->chars_in_buffer() may lead to a
deadlock and got soft lockup error. Try using tty->driver->ops->flush_chars()
to archive to same scenario.

There is no functional change, even if uart_flush_chars() doesn't do what
we expected, the driver should be able to keep working, as the backlight
commands is only 3 or 4 bytes short and has little chance to stay in the
queue partially.

Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
---
 drivers/platform/x86/dell/dell-uart-backlight.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

On Tue, Aug 22, 2023 at 11:57:40AM +0800, AceLan Kao wrote:
> From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/2032174
> 
> After v6.5, calls tty->driver->ops->chars_in_buffer() may lead to a
> deadlock and got soft lockup error. Try using tty->driver->ops->flush_chars()
> to archive to same scenario.
> 
> There is no functional change, even if uart_flush_chars() doesn't do what
> we expected, the driver should be able to keep working, as the backlight
> commands is only 3 or 4 bytes short and has little chance to stay in the
> queue partially.
> 
> Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>

Is there an upstream thread for this? If so it would be nice to add a
link, reference, or a "cherry picked from" line with the URL of the
upstream mail thread.

Thanks,
-Andrea

> ---
>  drivers/platform/x86/dell/dell-uart-backlight.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> index 3f8ea639d557..120701e5b8b1 100644
> --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> @@ -39,7 +39,8 @@ static struct file *ftty;
>  
>  unsigned int (*io_serial_in)(struct uart_port *p, int offset);
>  int (*uart_write)(struct tty_struct *tty, const unsigned char *buf, int count);
> -unsigned int (*uart_chars_in_buffer)(struct tty_struct *tty);
> +void (*uart_flush_chars)(struct tty_struct *tty);
> +
>  
>  static bool force;
>  module_param(force, bool, 0444);
> @@ -160,8 +161,7 @@ static int dell_uart_write(struct uart_8250_port *up, __u8 *buf, int len)
>  	tty_port_tty_wakeup(&port->state->port);
>  	tty = tty_port_tty_get(&port->state->port);
>  	actual = uart_write(tty, buf, len);
> -	while (uart_chars_in_buffer(tty))
> -		udelay(10);
> +	uart_flush_chars(tty);
>  
>  	return actual;
>  }
> @@ -401,7 +401,7 @@ static int dell_uart_startup(struct dell_uart_backlight *dell_pdata)
>  	tty = port->state->port.tty;
>  	io_serial_in = port->serial_in;
>  	uart_write = tty->driver->ops->write;
> -	uart_chars_in_buffer = tty->driver->ops->chars_in_buffer;
> +	uart_flush_chars = tty->driver->ops->flush_chars;
>  
>  	return 0;
>  }
> -- 
> 2.34.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

Andrea Righi <andrea.righi@canonical.com> 於 2023年8月23日 週三 下午2:05寫道：
>
> On Tue, Aug 22, 2023 at 11:57:40AM +0800, AceLan Kao wrote:
> > From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
> >
> > BugLink: https://bugs.launchpad.net/bugs/2032174
> >
> > After v6.5, calls tty->driver->ops->chars_in_buffer() may lead to a
> > deadlock and got soft lockup error. Try using tty->driver->ops->flush_chars()
> > to archive to same scenario.
> >
> > There is no functional change, even if uart_flush_chars() doesn't do what
> > we expected, the driver should be able to keep working, as the backlight
> > commands is only 3 or 4 bytes short and has little chance to stay in the
> > queue partially.
> >
> > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
>
> Is there an upstream thread for this? If so it would be nice to add a
> link, reference, or a "cherry picked from" line with the URL of the
> upstream mail thread.

Hi Andrea,

No, currently dell-uart-backlight driver is Ubuntu only, so no upstream link.

This driver need to be re-written to use serdev to be able to submit
to upstream,
and serdev seems to have issues with ACPI ID table, I can't make it be loaded.
Current drivers that uses serdev are for ARM system, although some of them also
provide ACPI ID table in the driver, but I think it probably doesn't work.

>
> Thanks,
> -Andrea
>
> > ---
> >  drivers/platform/x86/dell/dell-uart-backlight.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c
> > index 3f8ea639d557..120701e5b8b1 100644
> > --- a/drivers/platform/x86/dell/dell-uart-backlight.c
> > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c
> > @@ -39,7 +39,8 @@ static struct file *ftty;
> >
> >  unsigned int (*io_serial_in)(struct uart_port *p, int offset);
> >  int (*uart_write)(struct tty_struct *tty, const unsigned char *buf, int count);
> > -unsigned int (*uart_chars_in_buffer)(struct tty_struct *tty);
> > +void (*uart_flush_chars)(struct tty_struct *tty);
> > +
> >
> >  static bool force;
> >  module_param(force, bool, 0444);
> > @@ -160,8 +161,7 @@ static int dell_uart_write(struct uart_8250_port *up, __u8 *buf, int len)
> >       tty_port_tty_wakeup(&port->state->port);
> >       tty = tty_port_tty_get(&port->state->port);
> >       actual = uart_write(tty, buf, len);
> > -     while (uart_chars_in_buffer(tty))
> > -             udelay(10);
> > +     uart_flush_chars(tty);
> >
> >       return actual;
> >  }
> > @@ -401,7 +401,7 @@ static int dell_uart_startup(struct dell_uart_backlight *dell_pdata)
> >       tty = port->state->port.tty;
> >       io_serial_in = port->serial_in;
> >       uart_write = tty->driver->ops->write;
> > -     uart_chars_in_buffer = tty->driver->ops->chars_in_buffer;
> > +     uart_flush_chars = tty->driver->ops->flush_chars;
> >
> >       return 0;
> >  }
> > --
> > 2.34.1
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team

On Wed, Aug 23, 2023 at 02:45:00PM +0800, AceLan Kao wrote:
> Andrea Righi <andrea.righi@canonical.com> 於 2023年8月23日 週三 下午2:05寫道：
> >
> > On Tue, Aug 22, 2023 at 11:57:40AM +0800, AceLan Kao wrote:
> > > From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com>
> > >
> > > BugLink: https://bugs.launchpad.net/bugs/2032174
> > >
> > > After v6.5, calls tty->driver->ops->chars_in_buffer() may lead to a
> > > deadlock and got soft lockup error. Try using tty->driver->ops->flush_chars()
> > > to archive to same scenario.
> > >
> > > There is no functional change, even if uart_flush_chars() doesn't do what
> > > we expected, the driver should be able to keep working, as the backlight
> > > commands is only 3 or 4 bytes short and has little chance to stay in the
> > > queue partially.
> > >
> > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com>
> >
> > Is there an upstream thread for this? If so it would be nice to add a
> > link, reference, or a "cherry picked from" line with the URL of the
> > upstream mail thread.
> 
> Hi Andrea,
> 
> No, currently dell-uart-backlight driver is Ubuntu only, so no upstream link.
> 
> This driver need to be re-written to use serdev to be able to submit
> to upstream,
> and serdev seems to have issues with ACPI ID table, I can't make it be loaded.
> Current drivers that uses serdev are for ARM system, although some of them also
> provide ACPI ID table in the driver, but I think it probably doesn't work.

Ah! I was missing that it's *all* SAUCE. Ok in that case, yes, there's
not an upstream thread of course.

Therefore, applied to mantic/linux-unstable.

Thanks for the clarification,
-Andrea