Message ID | 20230914101341.233733-2-acelan.kao@canonical.com |
---|---|
State | New |
Headers | show |
Series | dell-uart-backlight fails to communicate | expand |
On Thu, Sep 14, 2023 at 06:13:41PM +0800, AceLan Kao wrote: > From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com> > > BugLink: https://bugs.launchpad.net/bug/2035299 > > After applying fbf84fb368923 ("UBUNTU: SAUCE: platform/x86: > dell-uart-backlight: replace chars_in_buffer() with flush_chars()"), it > seems that the read() command may fail to receive a response, even when > increasing the retry times. It never gets a response when it fails. > > To fix this, try adding a small delay after the write() function as a workaround. I'm wondering if there's a better way to determine when the "write" actually completed, so that we can safely issue a read without doing the msleep() that is often a bit unpredictable. If msleep() is really the only way to do it, why don't we put that at the end of dell_uart_write()? I suppose any write would be affected by this issue potentially... Thanks, -Andrea > > Fixes: fbf84fb368923 ("UBUNTU: SAUCE: platform/x86: dell-uart-backlight: replace chars_in_buffer() with flush_chars()") > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > --- > drivers/platform/x86/dell/dell-uart-backlight.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > index 120701e5b8b13..c489f7997767f 100644 > --- a/drivers/platform/x86/dell/dell-uart-backlight.c > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > @@ -248,6 +248,7 @@ static int dell_uart_set_bl_power(struct backlight_device *bd, int power) > } > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > + msleep(1); > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > mutex_unlock(&dell_pdata->brightness_mutex); > @@ -275,6 +276,7 @@ static int dell_uart_get_brightness(struct backlight_device *bd) > } > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > + msleep(1); > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > mutex_unlock(&dell_pdata->brightness_mutex); > @@ -304,6 +306,7 @@ static int dell_uart_update_status(struct backlight_device *bd) > } > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > + msleep(1); > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > mutex_unlock(&dell_pdata->brightness_mutex); > @@ -330,6 +333,7 @@ static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata) > } > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > + msleep(1); > while (retry-- > 0) { > /* first byte is data length */ > dell_uart_read(uart, bl_cmd->ret, 1); > @@ -371,6 +375,7 @@ static int dell_uart_get_display_mode(struct dell_uart_backlight *dell_pdata) > } > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > + msleep(1); > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > mutex_unlock(&dell_pdata->brightness_mutex); > -- > 2.34.1 > > > -- > kernel-team mailing list > kernel-team@lists.ubuntu.com > https://lists.ubuntu.com/mailman/listinfo/kernel-team
On Thu, Sep 14, 2023 at 02:00:13PM +0200, Andrea Righi wrote: > On Thu, Sep 14, 2023 at 06:13:41PM +0800, AceLan Kao wrote: > > From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com> > > > > BugLink: https://bugs.launchpad.net/bug/2035299 > > > > After applying fbf84fb368923 ("UBUNTU: SAUCE: platform/x86: > > dell-uart-backlight: replace chars_in_buffer() with flush_chars()"), it > > seems that the read() command may fail to receive a response, even when > > increasing the retry times. It never gets a response when it fails. > > > > To fix this, try adding a small delay after the write() function as a workaround. > > I'm wondering if there's a better way to determine when the "write" > actually completed, so that we can safely issue a read without doing the > msleep() that is often a bit unpredictable. > > If msleep() is really the only way to do it, why don't we put that at > the end of dell_uart_write()? I suppose any write would be affected by > this issue potentially... > > Thanks, > -Andrea Any opinion/update on this? This patch is still unapplied. Thanks, -Andrea > > > > > Fixes: fbf84fb368923 ("UBUNTU: SAUCE: platform/x86: dell-uart-backlight: replace chars_in_buffer() with flush_chars()") > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > > --- > > drivers/platform/x86/dell/dell-uart-backlight.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > > index 120701e5b8b13..c489f7997767f 100644 > > --- a/drivers/platform/x86/dell/dell-uart-backlight.c > > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > > @@ -248,6 +248,7 @@ static int dell_uart_set_bl_power(struct backlight_device *bd, int power) > > } > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > + msleep(1); > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > @@ -275,6 +276,7 @@ static int dell_uart_get_brightness(struct backlight_device *bd) > > } > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > + msleep(1); > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > @@ -304,6 +306,7 @@ static int dell_uart_update_status(struct backlight_device *bd) > > } > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > + msleep(1); > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > @@ -330,6 +333,7 @@ static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata) > > } > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > + msleep(1); > > while (retry-- > 0) { > > /* first byte is data length */ > > dell_uart_read(uart, bl_cmd->ret, 1); > > @@ -371,6 +375,7 @@ static int dell_uart_get_display_mode(struct dell_uart_backlight *dell_pdata) > > } > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > + msleep(1); > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > -- > > 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年9月28日 週四 上午3:13寫道: > > On Thu, Sep 14, 2023 at 02:00:13PM +0200, Andrea Righi wrote: > > On Thu, Sep 14, 2023 at 06:13:41PM +0800, AceLan Kao wrote: > > > From: "Chia-Lin Kao (AceLan)" <acelan.kao@canonical.com> > > > > > > BugLink: https://bugs.launchpad.net/bug/2035299 > > > > > > After applying fbf84fb368923 ("UBUNTU: SAUCE: platform/x86: > > > dell-uart-backlight: replace chars_in_buffer() with flush_chars()"), it > > > seems that the read() command may fail to receive a response, even when > > > increasing the retry times. It never gets a response when it fails. > > > > > > To fix this, try adding a small delay after the write() function as a workaround. > > > > I'm wondering if there's a better way to determine when the "write" > > actually completed, so that we can safely issue a read without doing the > > msleep() that is often a bit unpredictable. > > > > If msleep() is really the only way to do it, why don't we put that at > > the end of dell_uart_write()? I suppose any write would be affected by > > this issue potentially... > > > > Thanks, > > -Andrea > > Any opinion/update on this? This patch is still unapplied. Ah, sorry, I missed this thread. The issue is kind of weird that doing more retry in the dell_uart_read() doesn't work, i've tried adjusting the retry mdelay() and the retry times in the read function, but it doesn't help. The only workaround is to make a short sleep after write(). I don't think this is the right way to fix the issue and on old platforms we don't have to do this, so I don't want to add the short sleep in the write function. Move the sleep() into write function makes the code cleaner, but I want it more verbose to remind me there is a sleep between write and read, and not to hide the workaround in the write function. So that, maybe someday I can see the verbose code and check if there is a better solution with the new kernel. > > Thanks, > -Andrea > > > > > > > > > Fixes: fbf84fb368923 ("UBUNTU: SAUCE: platform/x86: dell-uart-backlight: replace chars_in_buffer() with flush_chars()") > > > Signed-off-by: Chia-Lin Kao (AceLan) <acelan.kao@canonical.com> > > > --- > > > drivers/platform/x86/dell/dell-uart-backlight.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c > > > index 120701e5b8b13..c489f7997767f 100644 > > > --- a/drivers/platform/x86/dell/dell-uart-backlight.c > > > +++ b/drivers/platform/x86/dell/dell-uart-backlight.c > > > @@ -248,6 +248,7 @@ static int dell_uart_set_bl_power(struct backlight_device *bd, int power) > > > } > > > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > > + msleep(1); > > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > > @@ -275,6 +276,7 @@ static int dell_uart_get_brightness(struct backlight_device *bd) > > > } > > > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > > + msleep(1); > > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > > @@ -304,6 +306,7 @@ static int dell_uart_update_status(struct backlight_device *bd) > > > } > > > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > > + msleep(1); > > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > > @@ -330,6 +333,7 @@ static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata) > > > } > > > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > > + msleep(1); > > > while (retry-- > 0) { > > > /* first byte is data length */ > > > dell_uart_read(uart, bl_cmd->ret, 1); > > > @@ -371,6 +375,7 @@ static int dell_uart_get_display_mode(struct dell_uart_backlight *dell_pdata) > > > } > > > > > > dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); > > > + msleep(1); > > > rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); > > > > > > mutex_unlock(&dell_pdata->brightness_mutex); > > > -- > > > 2.34.1 > > > > > > > > > -- > > > kernel-team mailing list > > > kernel-team@lists.ubuntu.com > > > https://lists.ubuntu.com/mailman/listinfo/kernel-team
diff --git a/drivers/platform/x86/dell/dell-uart-backlight.c b/drivers/platform/x86/dell/dell-uart-backlight.c index 120701e5b8b13..c489f7997767f 100644 --- a/drivers/platform/x86/dell/dell-uart-backlight.c +++ b/drivers/platform/x86/dell/dell-uart-backlight.c @@ -248,6 +248,7 @@ static int dell_uart_set_bl_power(struct backlight_device *bd, int power) } dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); + msleep(1); rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); mutex_unlock(&dell_pdata->brightness_mutex); @@ -275,6 +276,7 @@ static int dell_uart_get_brightness(struct backlight_device *bd) } dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); + msleep(1); rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); mutex_unlock(&dell_pdata->brightness_mutex); @@ -304,6 +306,7 @@ static int dell_uart_update_status(struct backlight_device *bd) } dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); + msleep(1); rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); mutex_unlock(&dell_pdata->brightness_mutex); @@ -330,6 +333,7 @@ static int dell_uart_show_firmware_ver(struct dell_uart_backlight *dell_pdata) } dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); + msleep(1); while (retry-- > 0) { /* first byte is data length */ dell_uart_read(uart, bl_cmd->ret, 1); @@ -371,6 +375,7 @@ static int dell_uart_get_display_mode(struct dell_uart_backlight *dell_pdata) } dell_uart_write(uart, bl_cmd->cmd, bl_cmd->tx_len); + msleep(1); rx_len = dell_uart_read(uart, bl_cmd->ret, bl_cmd->rx_len); mutex_unlock(&dell_pdata->brightness_mutex);