[1/1,SRU,B,D,E,F] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: add retry for get scalar status
diff mbox series

Message ID 20200108075945.20347-2-acelan.kao@canonical.com
State New
Headers show
Series
  • Dell AIO can't adjust brightness
Related show

Commit Message

AceLan Kao Jan. 8, 2020, 7:59 a.m. UTC
BugLink: https://bugs.launchpad.net/bugs/1858761

Found on new platforms that UART require more than 1 second to respond
commands in the first 10 seconds after booted.
dell_uart_get_scalar_status() is the first command we send to scalar and
this command should be more reliable than other commands, and make sure
we got correct response from scalar. So, add retry and increase the read
timeout to 2 seconds.

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

Comments

Seth Forshee Jan. 8, 2020, 5:21 p.m. UTC | #1
On Wed, Jan 08, 2020 at 03:59:45PM +0800, AceLan Kao wrote:
> BugLink: https://bugs.launchpad.net/bugs/1858761
> 
> Found on new platforms that UART require more than 1 second to respond
> commands in the first 10 seconds after booted.
> dell_uart_get_scalar_status() is the first command we send to scalar and
> this command should be more reliable than other commands, and make sure
> we got correct response from scalar. So, add retry and increase the read
> timeout to 2 seconds.
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>

It looks like this happens during probe, does it affect boot speed?
Obviously it should only impact a limited set of hw platforms, just
curious.

Thanks,
Seth

> ---
>  drivers/platform/x86/dell-uart-backlight.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
> index 90b28865896a..76e9a60a9388 100644
> --- a/drivers/platform/x86/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell-uart-backlight.c
> @@ -318,7 +318,7 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>  	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
>  	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>  	int rx_len;
> -	int status = 0;
> +	int status = 0, retry = 20;
>  
>  	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>  
> @@ -328,7 +328,11 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>  	}
>  
>  	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> -	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +	do {
> +		rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +		if (rx_len == 0)
> +			msleep(100);
> +	} while (rx_len == 0 && --retry);
>  
>  	mutex_unlock(&dell_pdata->brightness_mutex);
>  
> -- 
> 2.17.1
> 
> 
> -- 
> kernel-team mailing list
> kernel-team@lists.ubuntu.com
> https://lists.ubuntu.com/mailman/listinfo/kernel-team
AceLan Kao Jan. 9, 2020, 3:29 a.m. UTC | #2
No, it won't impact boot speed, msleep() could be re-schedule.
And yes, it only impacts those machines can't reply
dell_uart_get_scalar_status() in time.

Seth Forshee <seth.forshee@canonical.com> 於 2020年1月9日 週四 上午1:21寫道:
>
> On Wed, Jan 08, 2020 at 03:59:45PM +0800, AceLan Kao wrote:
> > BugLink: https://bugs.launchpad.net/bugs/1858761
> >
> > Found on new platforms that UART require more than 1 second to respond
> > commands in the first 10 seconds after booted.
> > dell_uart_get_scalar_status() is the first command we send to scalar and
> > this command should be more reliable than other commands, and make sure
> > we got correct response from scalar. So, add retry and increase the read
> > timeout to 2 seconds.
> >
> > Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
>
> It looks like this happens during probe, does it affect boot speed?
> Obviously it should only impact a limited set of hw platforms, just
> curious.
>
> Thanks,
> Seth
>
> > ---
> >  drivers/platform/x86/dell-uart-backlight.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
> > index 90b28865896a..76e9a60a9388 100644
> > --- a/drivers/platform/x86/dell-uart-backlight.c
> > +++ b/drivers/platform/x86/dell-uart-backlight.c
> > @@ -318,7 +318,7 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
> >       struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
> >       struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
> >       int rx_len;
> > -     int status = 0;
> > +     int status = 0, retry = 20;
> >
> >       dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
> >
> > @@ -328,7 +328,11 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
> >       }
> >
> >       dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> > -     rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > +     do {
> > +             rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> > +             if (rx_len == 0)
> > +                     msleep(100);
> > +     } while (rx_len == 0 && --retry);
> >
> >       mutex_unlock(&dell_pdata->brightness_mutex);
> >
> > --
> > 2.17.1
> >
> >
> > --
> > kernel-team mailing list
> > kernel-team@lists.ubuntu.com
> > https://lists.ubuntu.com/mailman/listinfo/kernel-team
Stefan Bader Jan. 16, 2020, 2:03 p.m. UTC | #3
On 08.01.20 08:59, AceLan Kao wrote:
> BugLink: https://bugs.launchpad.net/bugs/1858761
> 
> Found on new platforms that UART require more than 1 second to respond
> commands in the first 10 seconds after booted.
> dell_uart_get_scalar_status() is the first command we send to scalar and
> this command should be more reliable than other commands, and make sure
> we got correct response from scalar. So, add retry and increase the read
> timeout to 2 seconds.
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---

Fixes a problem and should not have impact on other platforms than the targeted
one. Not acking for Disco since that goes EOL Jan-23. The derivative kernels
other then oem-osp1 do not need this and oem-osp1 get it separately.

-Stefan

>  drivers/platform/x86/dell-uart-backlight.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
> index 90b28865896a..76e9a60a9388 100644
> --- a/drivers/platform/x86/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell-uart-backlight.c
> @@ -318,7 +318,7 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>  	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
>  	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>  	int rx_len;
> -	int status = 0;
> +	int status = 0, retry = 20;
>  
>  	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>  
> @@ -328,7 +328,11 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>  	}
>  
>  	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> -	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +	do {
> +		rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +		if (rx_len == 0)
> +			msleep(100);
> +	} while (rx_len == 0 && --retry);
>  
>  	mutex_unlock(&dell_pdata->brightness_mutex);
>  
>
Connor Kuehl Jan. 17, 2020, 11:18 p.m. UTC | #4
On 1/7/20 11:59 PM, AceLan Kao wrote:
> BugLink: https://bugs.launchpad.net/bugs/1858761
> 
> Found on new platforms that UART require more than 1 second to respond
> commands in the first 10 seconds after booted.
> dell_uart_get_scalar_status() is the first command we send to scalar and
> this command should be more reliable than other commands, and make sure
> we got correct response from scalar. So, add retry and increase the read
> timeout to 2 seconds.
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>

Acked-by: Connor Kuehl <connor.kuehl@canonical.com>

> ---
>   drivers/platform/x86/dell-uart-backlight.c | 8 ++++++--
>   1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
> index 90b28865896a..76e9a60a9388 100644
> --- a/drivers/platform/x86/dell-uart-backlight.c
> +++ b/drivers/platform/x86/dell-uart-backlight.c
> @@ -318,7 +318,7 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>   	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
>   	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
>   	int rx_len;
> -	int status = 0;
> +	int status = 0, retry = 20;
>   
>   	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
>   
> @@ -328,7 +328,11 @@ static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
>   	}
>   
>   	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
> -	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +	do {
> +		rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
> +		if (rx_len == 0)
> +			msleep(100);
> +	} while (rx_len == 0 && --retry);
>   
>   	mutex_unlock(&dell_pdata->brightness_mutex);
>   
>
Seth Forshee Jan. 21, 2020, 11:10 p.m. UTC | #5
On Wed, Jan 08, 2020 at 03:59:45PM +0800, AceLan Kao wrote:
> BugLink: https://bugs.launchpad.net/bugs/1858761
> 
> Found on new platforms that UART require more than 1 second to respond
> commands in the first 10 seconds after booted.
> dell_uart_get_scalar_status() is the first command we send to scalar and
> this command should be more reliable than other commands, and make sure
> we got correct response from scalar. So, add retry and increase the read
> timeout to 2 seconds.
> 
> Signed-off-by: AceLan Kao <acelan.kao@canonical.com>

Applied to focal/master-next and unstable/master, thanks!

Patch
diff mbox series

diff --git a/drivers/platform/x86/dell-uart-backlight.c b/drivers/platform/x86/dell-uart-backlight.c
index 90b28865896a..76e9a60a9388 100644
--- a/drivers/platform/x86/dell-uart-backlight.c
+++ b/drivers/platform/x86/dell-uart-backlight.c
@@ -318,7 +318,7 @@  static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
 	struct dell_uart_bl_cmd *bl_cmd = &uart_cmd[DELL_UART_GET_SCALAR];
 	struct uart_8250_port *uart = serial8250_get_port(dell_pdata->line);
 	int rx_len;
-	int status = 0;
+	int status = 0, retry = 20;
 
 	dell_uart_dump_cmd(__func__, "tx: ", bl_cmd->cmd, bl_cmd->tx_len);
 
@@ -328,7 +328,11 @@  static int dell_uart_get_scalar_status(struct dell_uart_backlight *dell_pdata)
 	}
 
 	dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len);
-	rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
+	do {
+		rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len);
+		if (rx_len == 0)
+			msleep(100);
+	} while (rx_len == 0 && --retry);
 
 	mutex_unlock(&dell_pdata->brightness_mutex);