[PATCH v3] serial: 8250_dw: Preserve original value of DLF register

[PATCH v3] serial: 8250_dw: Preserve original value of DLF register

[Ruihong Luo]

Preserve the original value of the Divisor Latch Fraction (DLF) register.
When the DLF register is modified without preservation, it can disrupt
the baudrate settings established by firmware or bootloader, leading to
data corruption and the generation of unreadable or distorted characters.

Fixes: 701c5e73b296 ("serial: 8250_dw: add fractional divisor support")
Signed-off-by: Ruihong Luo <colorsu1922@gmail.com>
---
v3:
- modify the commit message
- use personal email to sign
 drivers/tty/serial/8250/8250_dwlib.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index 75f32f054ebb..d30957722da8 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -244,7 +244,7 @@ void dw8250_setup_port(struct uart_port *p)
 	struct dw8250_port_data *pd = p->private_data;
 	struct dw8250_data *data = to_dw8250_data(pd);
 	struct uart_8250_port *up = up_to_u8250p(p);
-	u32 reg;
+	u32 reg, orig;
 
 	pd->hw_rs485_support = dw8250_detect_rs485_hw(p);
 	if (pd->hw_rs485_support) {
@@ -270,9 +270,11 @@ void dw8250_setup_port(struct uart_port *p)
 	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
 		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
 
+	/* Preserve value written by firmware or bootloader  */
+	orig = dw8250_readl_ext(p, DW_UART_DLF);
 	dw8250_writel_ext(p, DW_UART_DLF, ~0U);
 	reg = dw8250_readl_ext(p, DW_UART_DLF);
-	dw8250_writel_ext(p, DW_UART_DLF, 0);
+	dw8250_writel_ext(p, DW_UART_DLF, orig);
 
 	if (reg) {
 		pd->dlf_size = fls(reg);
-- 
2.39.2

Re: [PATCH v3] serial: 8250_dw: Preserve original value of DLF register

[Andy Shevchenko]

On Fri, Jun 30, 2023 at 08:38:08AM +0800, Ruihong Luo wrote:
> Preserve the original value of the Divisor Latch Fraction (DLF) register.
> When the DLF register is modified without preservation, it can disrupt
> the baudrate settings established by firmware or bootloader, leading to
> data corruption and the generation of unreadable or distorted characters.

Eventually it gets to the point of clean and good fix, thank you!
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Note, you are not supposed to do anything. Either Greg will pick this up after
merge window closes, or he will comment on the issues he finds, if any.
The process might take a few weeks (2 weeks of merge window + Greg's backlog
size, which is huge).

> Fixes: 701c5e73b296 ("serial: 8250_dw: add fractional divisor support")
> Signed-off-by: Ruihong Luo <colorsu1922@gmail.com>
> ---
> v3:
> - modify the commit message
> - use personal email to sign
>  drivers/tty/serial/8250/8250_dwlib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
> index 75f32f054ebb..d30957722da8 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -244,7 +244,7 @@ void dw8250_setup_port(struct uart_port *p)
>  	struct dw8250_port_data *pd = p->private_data;
>  	struct dw8250_data *data = to_dw8250_data(pd);
>  	struct uart_8250_port *up = up_to_u8250p(p);
> -	u32 reg;
> +	u32 reg, orig;
>  
>  	pd->hw_rs485_support = dw8250_detect_rs485_hw(p);
>  	if (pd->hw_rs485_support) {
> @@ -270,9 +270,11 @@ void dw8250_setup_port(struct uart_port *p)
>  	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>  
> +	/* Preserve value written by firmware or bootloader  */
> +	orig = dw8250_readl_ext(p, DW_UART_DLF);
>  	dw8250_writel_ext(p, DW_UART_DLF, ~0U);
>  	reg = dw8250_readl_ext(p, DW_UART_DLF);
> -	dw8250_writel_ext(p, DW_UART_DLF, 0);
> +	dw8250_writel_ext(p, DW_UART_DLF, orig);
>  
>  	if (reg) {
>  		pd->dlf_size = fls(reg);
> -- 
> 2.39.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] serial: 8250_dw: Preserve original value of DLF register

[Ilpo Järvinen]

On Fri, 30 Jun 2023, Ruihong Luo wrote:

> Preserve the original value of the Divisor Latch Fraction (DLF) register.
> When the DLF register is modified without preservation, it can disrupt
> the baudrate settings established by firmware or bootloader, leading to
> data corruption and the generation of unreadable or distorted characters.
> 
> Fixes: 701c5e73b296 ("serial: 8250_dw: add fractional divisor support")

Add also:

Cc: stable@vger.kernel.org

> Signed-off-by: Ruihong Luo <colorsu1922@gmail.com>
> ---
> v3:
> - modify the commit message
> - use personal email to sign
>  drivers/tty/serial/8250/8250_dwlib.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
> index 75f32f054ebb..d30957722da8 100644
> --- a/drivers/tty/serial/8250/8250_dwlib.c
> +++ b/drivers/tty/serial/8250/8250_dwlib.c
> @@ -244,7 +244,7 @@ void dw8250_setup_port(struct uart_port *p)
>  	struct dw8250_port_data *pd = p->private_data;
>  	struct dw8250_data *data = to_dw8250_data(pd);
>  	struct uart_8250_port *up = up_to_u8250p(p);
> -	u32 reg;
> +	u32 reg, orig;
>  
>  	pd->hw_rs485_support = dw8250_detect_rs485_hw(p);
>  	if (pd->hw_rs485_support) {
> @@ -270,9 +270,11 @@ void dw8250_setup_port(struct uart_port *p)
>  	dev_dbg(p->dev, "Designware UART version %c.%c%c\n",
>  		(reg >> 24) & 0xff, (reg >> 16) & 0xff, (reg >> 8) & 0xff);
>  
> +	/* Preserve value written by firmware or bootloader  */
> +	orig = dw8250_readl_ext(p, DW_UART_DLF);
>  	dw8250_writel_ext(p, DW_UART_DLF, ~0U);
>  	reg = dw8250_readl_ext(p, DW_UART_DLF);
> -	dw8250_writel_ext(p, DW_UART_DLF, 0);
> +	dw8250_writel_ext(p, DW_UART_DLF, orig);
>  
>  	if (reg) {
>  		pd->dlf_size = fls(reg);

The change looks good but I'd prefer the variable to be called old_dlf 
which is inline how 8250_port names its variables does when it preserves a 
registers across some auto-detection write.

-- 
 i.