| Message ID | 20200108075945.20347-2-acelan.kao@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | Dell AIO can't adjust brightness | expand |
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
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
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); > >
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); > >
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!
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);
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(-)