diff mbox series

[1/1,SRU,M] UBUNTU: SAUCE: platform/x86: dell-uart-backlight: add small delay after write command

Message ID 20230914101341.233733-2-acelan.kao@canonical.com
State New
Headers show
Series dell-uart-backlight fails to communicate | expand

Commit Message

AceLan Kao Sept. 14, 2023, 10:13 a.m. UTC 
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.

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(+)

Comments

Andrea Righi Sept. 14, 2023, noon UTC | #1 
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
Andrea Righi Sept. 27, 2023, 7:13 p.m. UTC | #2 
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
AceLan Kao Oct. 2, 2023, 1:19 a.m. UTC | #3 
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 mbox series

Patch

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);