linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/1] Allow hardware flow control to be used
@ 2017-01-11 19:48 Jason Uy
  2017-01-11 19:48 ` [PATCH v2 1/1] serial: 8250_dw: " Jason Uy
  0 siblings, 1 reply; 21+ messages in thread
From: Jason Uy @ 2017-01-11 19:48 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Andy Shevchenko, Kefeng Wang, Noam Camus, Heikki Krogerus,
	Wang Hongcheng, Jason Uy, linux-serial, linux-kernel,
	bcm-kernel-feedback-list

<clip: as before>

Changes from v1:
 - Rebased to v4.10-rc4
 - repo: https://github.com/torvalds/linux.git

Regards,
Jason
-- 
1.9.1

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-01-11 19:48 [PATCH v2 0/1] Allow hardware flow control to be used Jason Uy
@ 2017-01-11 19:48 ` Jason Uy
  2017-01-11 19:53   ` Andy Shevchenko
                     ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Jason Uy @ 2017-01-11 19:48 UTC (permalink / raw)
  To: Greg Kroah-Hartman, Jiri Slaby
  Cc: Andy Shevchenko, Kefeng Wang, Noam Camus, Heikki Krogerus,
	Wang Hongcheng, Jason Uy, linux-serial, linux-kernel,
	bcm-kernel-feedback-list

In the most common use case, the Synopsys DW UART driver does not
set the set_termios callback function.  This prevents UPSTAT_AUTOCTS
from being set when the UART flag CRTSCTS is set.  As a result, the
driver will use software flow control as opposed to hardware flow
control.

To fix the problem, the set_termios callback function is set to the
DW specific function.  The logic to set UPSTAT_AUTOCTS is moved so
that any clock error will not affect setting the hardware flow
control.

Reviewed-by: Scott Branden <scott.branden@broadcom.com>
Reviewed-by: Ray Jui <ray.jui@broadcom.com>
Signed-off-by: Jason Uy <jason.uy@broadcom.com>
---
 drivers/tty/serial/8250/8250_dw.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index c89fafc..c89ae45 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -248,11 +248,11 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 	if (!ret)
 		p->uartclk = rate;
 
+out:
 	p->status &= ~UPSTAT_AUTOCTS;
 	if (termios->c_cflag & CRTSCTS)
 		p->status |= UPSTAT_AUTOCTS;
 
-out:
 	serial8250_do_set_termios(p, termios, old);
 }
 
@@ -326,13 +326,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
 			p->serial_in = dw8250_serial_in32;
 			data->uart_16550_compatible = true;
 		}
-		p->set_termios = dw8250_set_termios;
 	}
 
 	/* Platforms with iDMA */
 	if (platform_get_resource_byname(to_platform_device(p->dev),
 					 IORESOURCE_MEM, "lpss_priv")) {
-		p->set_termios = dw8250_set_termios;
 		data->dma.rx_param = p->dev->parent;
 		data->dma.tx_param = p->dev->parent;
 		data->dma.fn = dw8250_idma_filter;
@@ -414,6 +412,7 @@ static int dw8250_probe(struct platform_device *pdev)
 	p->serial_in	= dw8250_serial_in;
 	p->serial_out	= dw8250_serial_out;
 	p->set_ldisc	= dw8250_set_ldisc;
+	p->set_termios	= dw8250_set_termios;
 
 	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
 	if (!p->membase)
-- 
1.9.1

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-01-11 19:48 ` [PATCH v2 1/1] serial: 8250_dw: " Jason Uy
@ 2017-01-11 19:53   ` Andy Shevchenko
  2017-01-13  1:33   ` Kefeng Wang
       [not found]   ` <CAAG0J9-n0toSJL8Ze8Esq81dYnpfrTd42bMiR94zw_btBLjsww@mail.gmail.com>
  2 siblings, 0 replies; 21+ messages in thread
From: Andy Shevchenko @ 2017-01-11 19:53 UTC (permalink / raw)
  To: Jason Uy, Greg Kroah-Hartman, Jiri Slaby
  Cc: Kefeng Wang, Noam Camus, Heikki Krogerus, Wang Hongcheng,
	linux-serial, linux-kernel, bcm-kernel-feedback-list

On Wed, 2017-01-11 at 11:48 -0800, Jason Uy wrote:
> In the most common use case, the Synopsys DW UART driver does not
> set the set_termios callback function.  This prevents UPSTAT_AUTOCTS
> from being set when the UART flag CRTSCTS is set.  As a result, the
> driver will use software flow control as opposed to hardware flow
> control.
> 
> To fix the problem, the set_termios callback function is set to the
> DW specific function.  The logic to set UPSTAT_AUTOCTS is moved so
> that any clock error will not affect setting the hardware flow
> control.
> 
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>

FWIW:
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Jason Uy <jason.uy@broadcom.com>
> ---
>  drivers/tty/serial/8250/8250_dw.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c
> index c89fafc..c89ae45 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -248,11 +248,11 @@ static void dw8250_set_termios(struct uart_port
> *p, struct ktermios *termios,
>  	if (!ret)
>  		p->uartclk = rate;
>  
> +out:
>  	p->status &= ~UPSTAT_AUTOCTS;
>  	if (termios->c_cflag & CRTSCTS)
>  		p->status |= UPSTAT_AUTOCTS;
>  
> -out:
>  	serial8250_do_set_termios(p, termios, old);
>  }
>  
> @@ -326,13 +326,11 @@ static void dw8250_quirks(struct uart_port *p,
> struct dw8250_data *data)
>  			p->serial_in = dw8250_serial_in32;
>  			data->uart_16550_compatible = true;
>  		}
> -		p->set_termios = dw8250_set_termios;
>  	}
>  
>  	/* Platforms with iDMA */
>  	if (platform_get_resource_byname(to_platform_device(p->dev),
>  					 IORESOURCE_MEM,
> "lpss_priv")) {
> -		p->set_termios = dw8250_set_termios;
>  		data->dma.rx_param = p->dev->parent;
>  		data->dma.tx_param = p->dev->parent;
>  		data->dma.fn = dw8250_idma_filter;
> @@ -414,6 +412,7 @@ static int dw8250_probe(struct platform_device
> *pdev)
>  	p->serial_in	= dw8250_serial_in;
>  	p->serial_out	= dw8250_serial_out;
>  	p->set_ldisc	= dw8250_set_ldisc;
> +	p->set_termios	= dw8250_set_termios;
>  
>  	p->membase = devm_ioremap(dev, regs->start,
> resource_size(regs));
>  	if (!p->membase)

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-01-11 19:48 ` [PATCH v2 1/1] serial: 8250_dw: " Jason Uy
  2017-01-11 19:53   ` Andy Shevchenko
@ 2017-01-13  1:33   ` Kefeng Wang
       [not found]   ` <CAAG0J9-n0toSJL8Ze8Esq81dYnpfrTd42bMiR94zw_btBLjsww@mail.gmail.com>
  2 siblings, 0 replies; 21+ messages in thread
From: Kefeng Wang @ 2017-01-13  1:33 UTC (permalink / raw)
  To: Jason Uy, Greg Kroah-Hartman, Jiri Slaby
  Cc: Andy Shevchenko, Noam Camus, Heikki Krogerus, Wang Hongcheng,
	linux-serial, linux-kernel, bcm-kernel-feedback-list



On 2017/1/12 3:48, Jason Uy wrote:
> In the most common use case, the Synopsys DW UART driver does not
> set the set_termios callback function.  This prevents UPSTAT_AUTOCTS
> from being set when the UART flag CRTSCTS is set.  As a result, the
> driver will use software flow control as opposed to hardware flow
> control.
> 
> To fix the problem, the set_termios callback function is set to the
> DW specific function.  The logic to set UPSTAT_AUTOCTS is moved so
> that any clock error will not affect setting the hardware flow
> control.
> 
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> Reviewed-by: Ray Jui <ray.jui@broadcom.com>
> Signed-off-by: Jason Uy <jason.uy@broadcom.com>

Reviewed-by: Kefeng Wang <wangkefeng.wang@huawei.com>

> ---
>  drivers/tty/serial/8250/8250_dw.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index c89fafc..c89ae45 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -248,11 +248,11 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>  	if (!ret)
>  		p->uartclk = rate;
>  
> +out:
>  	p->status &= ~UPSTAT_AUTOCTS;
>  	if (termios->c_cflag & CRTSCTS)
>  		p->status |= UPSTAT_AUTOCTS;
>  
> -out:
>  	serial8250_do_set_termios(p, termios, old);
>  }
>  
> @@ -326,13 +326,11 @@ static void dw8250_quirks(struct uart_port *p, struct dw8250_data *data)
>  			p->serial_in = dw8250_serial_in32;
>  			data->uart_16550_compatible = true;
>  		}
> -		p->set_termios = dw8250_set_termios;
>  	}
>  
>  	/* Platforms with iDMA */
>  	if (platform_get_resource_byname(to_platform_device(p->dev),
>  					 IORESOURCE_MEM, "lpss_priv")) {
> -		p->set_termios = dw8250_set_termios;
>  		data->dma.rx_param = p->dev->parent;
>  		data->dma.tx_param = p->dev->parent;
>  		data->dma.fn = dw8250_idma_filter;
> @@ -414,6 +412,7 @@ static int dw8250_probe(struct platform_device *pdev)
>  	p->serial_in	= dw8250_serial_in;
>  	p->serial_out	= dw8250_serial_out;
>  	p->set_ldisc	= dw8250_set_ldisc;
> +	p->set_termios	= dw8250_set_termios;
>  
>  	p->membase = devm_ioremap(dev, regs->start, resource_size(regs));
>  	if (!p->membase)
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
       [not found]   ` <CAAG0J9-n0toSJL8Ze8Esq81dYnpfrTd42bMiR94zw_btBLjsww@mail.gmail.com>
@ 2017-03-01 18:50     ` Andy Shevchenko
  2017-03-03  0:21       ` James Hogan
  0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2017-03-01 18:50 UTC (permalink / raw)
  To: James Hogan, Jason Uy
  Cc: Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang, Noam Camus,
	Heikki Krogerus, Wang Hongcheng, linux-serial, LKML,
	bcm-kernel-feedback-list, Linux MIPS Mailing List, David Daney

On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
> On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com> wrote:
> > In the most common use case, the Synopsys DW UART driver does not
> > set the set_termios callback function.  This prevents UPSTAT_AUTOCTS
> > from being set when the UART flag CRTSCTS is set.  As a result, the
> > driver will use software flow control as opposed to hardware flow
> > control.
> > 
> > To fix the problem, the set_termios callback function is set to the
> > DW specific function.  The logic to set UPSTAT_AUTOCTS is moved so
> > that any clock error will not affect setting the hardware flow
> > control.

> Bisection shows that this patch, commit
> 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
> Cavium Octeon III based UTM-8 board (MIPS architecture).
> 
> I now get the following warning:

> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> [<ffffffff811901a0>] register_console+0x1c8/0x418
> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8

> Then it hangs and the watchdog restarts the machine.
> 
> Any ideas?

1. Does it use clock on that platform?
2. Check if termios is not NULL there.

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-01 18:50     ` Andy Shevchenko
@ 2017-03-03  0:21       ` James Hogan
  2017-03-03 13:31         ` Andy Shevchenko
  0 siblings, 1 reply; 21+ messages in thread
From: James Hogan @ 2017-03-03  0:21 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Jason Uy, Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang,
	Noam Camus, Heikki Krogerus, Wang Hongcheng, linux-serial, LKML,
	bcm-kernel-feedback-list, Linux MIPS Mailing List, David Daney,
	Russell King, linux-clk, Viresh Kumar

[-- Attachment #1: Type: text/plain, Size: 2697 bytes --]

On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
> On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
> > On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com> wrote:
> > > In the most common use case, the Synopsys DW UART driver does not
> > > set the set_termios callback function.  This prevents UPSTAT_AUTOCTS
> > > from being set when the UART flag CRTSCTS is set.  As a result, the
> > > driver will use software flow control as opposed to hardware flow
> > > control.
> > > 
> > > To fix the problem, the set_termios callback function is set to the
> > > DW specific function.  The logic to set UPSTAT_AUTOCTS is moved so
> > > that any clock error will not affect setting the hardware flow
> > > control.
> 
> > Bisection shows that this patch, commit
> > 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
> > Cavium Octeon III based UTM-8 board (MIPS architecture).
> > 
> > I now get the following warning:
> 
> > [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> > [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> > [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> > [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> > [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> > [<ffffffff811901a0>] register_console+0x1c8/0x418
> > [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> > [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> > [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> 
> > Then it hangs and the watchdog restarts the machine.
> > 
> > Any ideas?
> 
> 1. Does it use clock on that platform?

btw, sorry for HTML email blocked from lists, gmail tricked me into it
:-(

I've now dug a little deeper. Essentially what is going on is:

1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns NULL
3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
   doesn't match, since !IS_ERR(NULL)
4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns 0
5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
   returns 0
6) dw8250_set_termios() thinks the frequency for that baud rate has been
   set successfully and writes 0 into uartclk
7) it all goes wrong from there...

The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:

> These calls will return error for platforms that don't select HAVE_CLK

And NULL isn't an error in this API.

Cc'ing some clk folk.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-03  0:21       ` James Hogan
@ 2017-03-03 13:31         ` Andy Shevchenko
  2017-03-03 17:33           ` Ray Jui
  2017-03-03 23:23           ` James Hogan
  0 siblings, 2 replies; 21+ messages in thread
From: Andy Shevchenko @ 2017-03-03 13:31 UTC (permalink / raw)
  To: James Hogan, Heiko Stuebner
  Cc: Jason Uy, Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang,
	Noam Camus, Heikki Krogerus, Wang Hongcheng, linux-serial, LKML,
	bcm-kernel-feedback-list, Linux MIPS Mailing List, David Daney,
	Russell King, linux-clk, Viresh Kumar

Heiko, you might be interested in this as well.

On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
> On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
> > On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
> > > On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com>
> > > wrote:
> > > > In the most common use case, the Synopsys DW UART driver does
> > > > not
> > > > set the set_termios callback function.  This prevents
> > > > UPSTAT_AUTOCTS
> > > > from being set when the UART flag CRTSCTS is set.  As a result,
> > > > the
> > > > driver will use software flow control as opposed to hardware
> > > > flow
> > > > control.
> > > > 
> > > > To fix the problem, the set_termios callback function is set to
> > > > the
> > > > DW specific function.  The logic to set UPSTAT_AUTOCTS is moved
> > > > so
> > > > that any clock error will not affect setting the hardware flow
> > > > control.
> > > Bisection shows that this patch, commit
> > > 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
> > > Cavium Octeon III based UTM-8 board (MIPS architecture).
> > > 
> > > I now get the following warning:
> > > [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> > > [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> > > [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> > > [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> > > [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> > > [<ffffffff811901a0>] register_console+0x1c8/0x418
> > > [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> > > [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> > > [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> > > Then it hangs and the watchdog restarts the machine.
> > > 
> > > Any ideas?
> > 
> > 1. Does it use clock on that platform?

> I've now dug a little deeper. Essentially what is going on is:
> 
> 1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
> 2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns NULL
> 3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
>    doesn't match, since !IS_ERR(NULL)
> 4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns 0
> 5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
>    returns 0
> 6) dw8250_set_termios() thinks the frequency for that baud rate has
> been
>    set successfully and writes 0 into uartclk
> 7) it all goes wrong from there...

So, it means we have need special care of NULL case here, and honestly,
I don't like it. But it seems the only feasible (quick) fix right now.

> The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
> seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
> add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:
> 
> > These calls will return error for platforms that don't select
> > HAVE_CLK
> 
> And NULL isn't an error in this API.

Which is okay. I dunno what should be returned from clk_round_rate() if
clk is NULL. I would fix CLK framework, though I would like to gather
more details.

Btw, I hope you also noticed this one:

http://www.spinics.net/lists/linux-serial/msg25314.html

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-03 13:31         ` Andy Shevchenko
@ 2017-03-03 17:33           ` Ray Jui
  2017-03-03 17:43             ` Jason Uy
  2017-03-03 23:23           ` James Hogan
  1 sibling, 1 reply; 21+ messages in thread
From: Ray Jui @ 2017-03-03 17:33 UTC (permalink / raw)
  To: Andy Shevchenko, James Hogan, Heiko Stuebner
  Cc: Jason Uy, Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang,
	Noam Camus, Heikki Krogerus, Wang Hongcheng, linux-serial, LKML,
	bcm-kernel-feedback-list, Linux MIPS Mailing List, David Daney,
	Russell King, linux-clk, Viresh Kumar

Hi Andy/Jason,

On 3/3/2017 5:31 AM, Andy Shevchenko wrote:
> Heiko, you might be interested in this as well.
> 
> On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
>> On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
>>> On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
>>>> On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com>
>>>> wrote:
>>>>> In the most common use case, the Synopsys DW UART driver does
>>>>> not
>>>>> set the set_termios callback function.  This prevents
>>>>> UPSTAT_AUTOCTS
>>>>> from being set when the UART flag CRTSCTS is set.  As a result,
>>>>> the
>>>>> driver will use software flow control as opposed to hardware
>>>>> flow
>>>>> control.
>>>>>
>>>>> To fix the problem, the set_termios callback function is set to
>>>>> the
>>>>> DW specific function.  The logic to set UPSTAT_AUTOCTS is moved
>>>>> so
>>>>> that any clock error will not affect setting the hardware flow
>>>>> control.
>>>> Bisection shows that this patch, commit
>>>> 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
>>>> Cavium Octeon III based UTM-8 board (MIPS architecture).
>>>>
>>>> I now get the following warning:
>>>> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
>>>> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
>>>> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
>>>> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
>>>> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
>>>> [<ffffffff811901a0>] register_console+0x1c8/0x418
>>>> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
>>>> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
>>>> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
>>>> Then it hangs and the watchdog restarts the machine.
>>>>
>>>> Any ideas?
>>>
>>> 1. Does it use clock on that platform?
> 
>> I've now dug a little deeper. Essentially what is going on is:
>>
>> 1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
>> 2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns NULL
>> 3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
>>    doesn't match, since !IS_ERR(NULL)
>> 4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns 0
>> 5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
>>    returns 0
>> 6) dw8250_set_termios() thinks the frequency for that baud rate has
>> been
>>    set successfully and writes 0 into uartclk
>> 7) it all goes wrong from there...
> 
> So, it means we have need special care of NULL case here, and honestly,
> I don't like it. But it seems the only feasible (quick) fix right now.

I agree. I think it should have been:

if (IS_ERR_OR_NULL(d->clk) || !old)
    goto out;

I think it makes sense to validate to make sure the 'clk' pointer is
valid before proceeding any further down below (regardless of how well
or how not well the clock framework handles it).

Thanks,

Ray

> 
>> The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
>> seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
>> add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:
>>
>>> These calls will return error for platforms that don't select
>>> HAVE_CLK
>>
>> And NULL isn't an error in this API.
> 
> Which is okay. I dunno what should be returned from clk_round_rate() if
> clk is NULL. I would fix CLK framework, though I would like to gather
> more details.
> 
> Btw, I hope you also noticed this one:
> 
> http://www.spinics.net/lists/linux-serial/msg25314.html
> 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-03 17:33           ` Ray Jui
@ 2017-03-03 17:43             ` Jason Uy
  2017-03-03 23:07               ` James Hogan
  0 siblings, 1 reply; 21+ messages in thread
From: Jason Uy @ 2017-03-03 17:43 UTC (permalink / raw)
  To: Ray Jui, Andy Shevchenko, James Hogan, Heiko Stuebner
  Cc: Greg Kroah-Hartman, Jiri Slaby, Kefeng Wang, Noam Camus,
	Heikki Krogerus, Wang Hongcheng, linux-serial, LKML,
	bcm-kernel-feedback-list, Linux MIPS Mailing List, David Daney,
	Russell King, linux-clk, Viresh Kumar

James,

Can you verify that changing the code to the following fixes your problem?

if (IS_ERR_OR_NULL(d->clk) || !old)
    goto out;

Regards,
Jason

-----Original Message-----
From: Ray Jui [mailto:ray.jui@broadcom.com]
Sent: March-03-17 9:34 AM
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; James Hogan
<james.hogan@imgtec.com>; Heiko Stuebner <heiko@sntech.de>
Cc: Jason Uy <jason.uy@broadcom.com>; Greg Kroah-Hartman
<gregkh@linuxfoundation.org>; Jiri Slaby <jslaby@suse.com>; Kefeng Wang
<wangkefeng.wang@huawei.com>; Noam Camus <noamc@ezchip.com>; Heikki Krogerus
<heikki.krogerus@linux.intel.com>; Wang Hongcheng <annie.wang@amd.com>;
linux-serial@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>;
bcm-kernel-feedback-list@broadcom.com; Linux MIPS Mailing List
<linux-mips@linux-mips.org>; David Daney <david.daney@cavium.com>; Russell
King <linux@armlinux.org.uk>; linux-clk@vger.kernel.org; Viresh Kumar
<viresh.kumar@st.com>
Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to
be used

Hi Andy/Jason,

On 3/3/2017 5:31 AM, Andy Shevchenko wrote:
> Heiko, you might be interested in this as well.
>
> On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
>> On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
>>> On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
>>>> On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com>
>>>> wrote:
>>>>> In the most common use case, the Synopsys DW UART driver does not
>>>>> set the set_termios callback function.  This prevents
>>>>> UPSTAT_AUTOCTS from being set when the UART flag CRTSCTS is set.
>>>>> As a result, the driver will use software flow control as opposed
>>>>> to hardware flow control.
>>>>>
>>>>> To fix the problem, the set_termios callback function is set to
>>>>> the DW specific function.  The logic to set UPSTAT_AUTOCTS is
>>>>> moved so that any clock error will not affect setting the hardware
>>>>> flow control.
>>>> Bisection shows that this patch, commit
>>>> 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
>>>> Cavium Octeon III based UTM-8 board (MIPS architecture).
>>>>
>>>> I now get the following warning:
>>>> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
>>>> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
>>>> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
>>>> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
>>>> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
>>>> [<ffffffff811901a0>] register_console+0x1c8/0x418
>>>> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
>>>> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
>>>> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8 Then it hangs and the
>>>> watchdog restarts the machine.
>>>>
>>>> Any ideas?
>>>
>>> 1. Does it use clock on that platform?
>
>> I've now dug a little deeper. Essentially what is going on is:
>>
>> 1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
>> 2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns
>> NULL
>> 3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
>>    doesn't match, since !IS_ERR(NULL)
>> 4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns 0
>> 5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
>>    returns 0
>> 6) dw8250_set_termios() thinks the frequency for that baud rate has
>> been
>>    set successfully and writes 0 into uartclk
>> 7) it all goes wrong from there...
>
> So, it means we have need special care of NULL case here, and
> honestly, I don't like it. But it seems the only feasible (quick) fix
> right now.

I agree. I think it should have been:

if (IS_ERR_OR_NULL(d->clk) || !old)
    goto out;

I think it makes sense to validate to make sure the 'clk' pointer is valid
before proceeding any further down below (regardless of how well or how not
well the clock framework handles it).

Thanks,

Ray

>
>> The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
>> seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
>> add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:
>>
>>> These calls will return error for platforms that don't select
>>> HAVE_CLK
>>
>> And NULL isn't an error in this API.
>
> Which is okay. I dunno what should be returned from clk_round_rate()
> if clk is NULL. I would fix CLK framework, though I would like to
> gather more details.
>
> Btw, I hope you also noticed this one:
>
> http://www.spinics.net/lists/linux-serial/msg25314.html
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-03 17:43             ` Jason Uy
@ 2017-03-03 23:07               ` James Hogan
  2017-03-04  0:02                 ` Jason Uy
  0 siblings, 1 reply; 21+ messages in thread
From: James Hogan @ 2017-03-03 23:07 UTC (permalink / raw)
  To: Jason Uy
  Cc: Ray Jui, Andy Shevchenko, Heiko Stuebner, Greg Kroah-Hartman,
	Jiri Slaby, Kefeng Wang, Noam Camus, Heikki Krogerus,
	Wang Hongcheng, linux-serial, LKML, bcm-kernel-feedback-list,
	Linux MIPS Mailing List, David Daney, Russell King, linux-clk,
	Viresh Kumar

[-- Attachment #1: Type: text/plain, Size: 5598 bytes --]

Hi Jason,

On Fri, Mar 03, 2017 at 09:43:55AM -0800, Jason Uy wrote:
> James,
> 
> Can you verify that changing the code to the following fixes your problem?
> 
> if (IS_ERR_OR_NULL(d->clk) || !old)
>     goto out;

It does, however I'm not at all convinced it is correct. clk_get either
returns a valid opaque clock cookie that can be passed to other clock
functions (which includes NULL), or ERR_PTR(-errno), which IS_ERR()
should catch for errors.

According to this thread:

https://lists.gt.net/linux/kernel/2102623

we should stick to the clk API and use IS_ERR() rather than
IS_ERR_OR_NULL(), but shouldn't be blindly accepting the result of
clk_get_rate() (or I suppose clk_round_rate()), but rather checking for
the value 0 and handling that case as "we don't have a usable clock from
the clk api, fall back to something else".

Cheers
James

> 
> Regards,
> Jason
> 
> -----Original Message-----
> From: Ray Jui [mailto:ray.jui@broadcom.com]
> Sent: March-03-17 9:34 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; James Hogan
> <james.hogan@imgtec.com>; Heiko Stuebner <heiko@sntech.de>
> Cc: Jason Uy <jason.uy@broadcom.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Jiri Slaby <jslaby@suse.com>; Kefeng Wang
> <wangkefeng.wang@huawei.com>; Noam Camus <noamc@ezchip.com>; Heikki Krogerus
> <heikki.krogerus@linux.intel.com>; Wang Hongcheng <annie.wang@amd.com>;
> linux-serial@vger.kernel.org; LKML <linux-kernel@vger.kernel.org>;
> bcm-kernel-feedback-list@broadcom.com; Linux MIPS Mailing List
> <linux-mips@linux-mips.org>; David Daney <david.daney@cavium.com>; Russell
> King <linux@armlinux.org.uk>; linux-clk@vger.kernel.org; Viresh Kumar
> <viresh.kumar@st.com>
> Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to
> be used
> 
> Hi Andy/Jason,
> 
> On 3/3/2017 5:31 AM, Andy Shevchenko wrote:
> > Heiko, you might be interested in this as well.
> >
> > On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
> >> On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
> >>> On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
> >>>> On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com>
> >>>> wrote:
> >>>>> In the most common use case, the Synopsys DW UART driver does not
> >>>>> set the set_termios callback function.  This prevents
> >>>>> UPSTAT_AUTOCTS from being set when the UART flag CRTSCTS is set.
> >>>>> As a result, the driver will use software flow control as opposed
> >>>>> to hardware flow control.
> >>>>>
> >>>>> To fix the problem, the set_termios callback function is set to
> >>>>> the DW specific function.  The logic to set UPSTAT_AUTOCTS is
> >>>>> moved so that any clock error will not affect setting the hardware
> >>>>> flow control.
> >>>> Bisection shows that this patch, commit
> >>>> 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
> >>>> Cavium Octeon III based UTM-8 board (MIPS architecture).
> >>>>
> >>>> I now get the following warning:
> >>>> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> >>>> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> >>>> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> >>>> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> >>>> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> >>>> [<ffffffff811901a0>] register_console+0x1c8/0x418
> >>>> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> >>>> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> >>>> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8 Then it hangs and the
> >>>> watchdog restarts the machine.
> >>>>
> >>>> Any ideas?
> >>>
> >>> 1. Does it use clock on that platform?
> >
> >> I've now dug a little deeper. Essentially what is going on is:
> >>
> >> 1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
> >> 2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns
> >> NULL
> >> 3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
> >>    doesn't match, since !IS_ERR(NULL)
> >> 4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns 0
> >> 5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
> >>    returns 0
> >> 6) dw8250_set_termios() thinks the frequency for that baud rate has
> >> been
> >>    set successfully and writes 0 into uartclk
> >> 7) it all goes wrong from there...
> >
> > So, it means we have need special care of NULL case here, and
> > honestly, I don't like it. But it seems the only feasible (quick) fix
> > right now.
> 
> I agree. I think it should have been:
> 
> if (IS_ERR_OR_NULL(d->clk) || !old)
>     goto out;
> 
> I think it makes sense to validate to make sure the 'clk' pointer is valid
> before proceeding any further down below (regardless of how well or how not
> well the clock framework handles it).
> 
> Thanks,
> 
> Ray
> 
> >
> >> The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
> >> seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
> >> add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:
> >>
> >>> These calls will return error for platforms that don't select
> >>> HAVE_CLK
> >>
> >> And NULL isn't an error in this API.
> >
> > Which is okay. I dunno what should be returned from clk_round_rate()
> > if clk is NULL. I would fix CLK framework, though I would like to
> > gather more details.
> >
> > Btw, I hope you also noticed this one:
> >
> > http://www.spinics.net/lists/linux-serial/msg25314.html
> >

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-03 13:31         ` Andy Shevchenko
  2017-03-03 17:33           ` Ray Jui
@ 2017-03-03 23:23           ` James Hogan
  2017-03-04  2:56             ` Andy Shevchenko
  1 sibling, 1 reply; 21+ messages in thread
From: James Hogan @ 2017-03-03 23:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Heiko Stuebner, Jason Uy, Greg Kroah-Hartman, Jiri Slaby,
	Kefeng Wang, Noam Camus, Heikki Krogerus, Wang Hongcheng,
	linux-serial, LKML, bcm-kernel-feedback-list,
	Linux MIPS Mailing List, David Daney, Russell King, linux-clk,
	Viresh Kumar

[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]

On Fri, Mar 03, 2017 at 03:31:06PM +0200, Andy Shevchenko wrote:
> On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
> > The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
> > seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
> > add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:
> > 
> > > These calls will return error for platforms that don't select
> > > HAVE_CLK
> > 
> > And NULL isn't an error in this API.
> 
> Which is okay. I dunno what should be returned from clk_round_rate() if
> clk is NULL. I would fix CLK framework, though I would like to gather
> more details.

Hmm, the common clock framwork is just one implementation of the clock
API that won't use NULL as a valid clock handle. HAVE_CLK=n is just
another implementation that does return NULL as a valid value, and
accepts that value in the other clk functions.

> Btw, I hope you also noticed this one:
> 
> http://www.spinics.net/lists/linux-serial/msg25314.html

Interesting.

Following Russel's past advise[1], the following patch on top of Heiko's
patch also fixes things for me on Octeon:

[1] https://lists.gt.net/linux/kernel/2102623

If thats an acceptable fix I'll post it properly. Thoughts?

Cheers
James

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 223ac234ddb2..e65808c482f1 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -267,6 +267,8 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 	rate = clk_round_rate(d->clk, baud * 16);
 	if (rate < 0)
 		ret = rate;
+	else if (rate == 0)
+		ret = -ENOENT;
 	else
 		ret = clk_set_rate(d->clk, rate);
 	clk_prepare_enable(d->clk);

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* RE: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-03 23:07               ` James Hogan
@ 2017-03-04  0:02                 ` Jason Uy
  2017-03-04  0:11                   ` James Hogan
  0 siblings, 1 reply; 21+ messages in thread
From: Jason Uy @ 2017-03-04  0:02 UTC (permalink / raw)
  To: James Hogan
  Cc: Ray Jui, Andy Shevchenko, Heiko Stuebner, Greg Kroah-Hartman,
	Jiri Slaby, Kefeng Wang, Noam Camus, Heikki Krogerus,
	Wang Hongcheng, linux-serial, LKML, bcm-kernel-feedback-list,
	Linux MIPS Mailing List, David Daney, Russell King, linux-clk,
	Viresh Kumar

HI James,

Maybe instead of that, we should do this instead

If ((IS_ERR(d->clk) && PTR_ERR(d->clk)) || !old)

Note that this is what is done in the probe function of the dw driver.

Regards,
Jason

-----Original Message-----
From: James Hogan [mailto:james.hogan@imgtec.com]
Sent: March-03-17 3:07 PM
To: Jason Uy <jason.uy@broadcom.com>
Cc: Ray Jui <ray.jui@broadcom.com>; Andy Shevchenko
<andriy.shevchenko@linux.intel.com>; Heiko Stuebner <heiko@sntech.de>; Greg
Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby <jslaby@suse.com>;
Kefeng Wang <wangkefeng.wang@huawei.com>; Noam Camus <noamc@ezchip.com>;
Heikki Krogerus <heikki.krogerus@linux.intel.com>; Wang Hongcheng
<annie.wang@amd.com>; linux-serial@vger.kernel.org; LKML
<linux-kernel@vger.kernel.org>; bcm-kernel-feedback-list@broadcom.com; Linux
MIPS Mailing List <linux-mips@linux-mips.org>; David Daney
<david.daney@cavium.com>; Russell King <linux@armlinux.org.uk>;
linux-clk@vger.kernel.org; Viresh Kumar <viresh.kumar@st.com>
Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to
be used

Hi Jason,

On Fri, Mar 03, 2017 at 09:43:55AM -0800, Jason Uy wrote:
> James,
>
> Can you verify that changing the code to the following fixes your problem?
>
> if (IS_ERR_OR_NULL(d->clk) || !old)
>     goto out;

It does, however I'm not at all convinced it is correct. clk_get either
returns a valid opaque clock cookie that can be passed to other clock
functions (which includes NULL), or ERR_PTR(-errno), which IS_ERR() should
catch for errors.

According to this thread:

https://lists.gt.net/linux/kernel/2102623

we should stick to the clk API and use IS_ERR() rather than
IS_ERR_OR_NULL(), but shouldn't be blindly accepting the result of
clk_get_rate() (or I suppose clk_round_rate()), but rather checking for the
value 0 and handling that case as "we don't have a usable clock from the clk
api, fall back to something else".

Cheers
James

>
> Regards,
> Jason
>
> -----Original Message-----
> From: Ray Jui [mailto:ray.jui@broadcom.com]
> Sent: March-03-17 9:34 AM
> To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; James Hogan
> <james.hogan@imgtec.com>; Heiko Stuebner <heiko@sntech.de>
> Cc: Jason Uy <jason.uy@broadcom.com>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; Jiri Slaby <jslaby@suse.com>; Kefeng
> Wang <wangkefeng.wang@huawei.com>; Noam Camus <noamc@ezchip.com>;
> Heikki Krogerus <heikki.krogerus@linux.intel.com>; Wang Hongcheng
> <annie.wang@amd.com>; linux-serial@vger.kernel.org; LKML
> <linux-kernel@vger.kernel.org>; bcm-kernel-feedback-list@broadcom.com;
> Linux MIPS Mailing List <linux-mips@linux-mips.org>; David Daney
> <david.daney@cavium.com>; Russell King <linux@armlinux.org.uk>;
> linux-clk@vger.kernel.org; Viresh Kumar <viresh.kumar@st.com>
> Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow
> control to be used
>
> Hi Andy/Jason,
>
> On 3/3/2017 5:31 AM, Andy Shevchenko wrote:
> > Heiko, you might be interested in this as well.
> >
> > On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
> >> On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
> >>> On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
> >>>> On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com>
> >>>> wrote:
> >>>>> In the most common use case, the Synopsys DW UART driver does
> >>>>> not set the set_termios callback function.  This prevents
> >>>>> UPSTAT_AUTOCTS from being set when the UART flag CRTSCTS is set.
> >>>>> As a result, the driver will use software flow control as
> >>>>> opposed to hardware flow control.
> >>>>>
> >>>>> To fix the problem, the set_termios callback function is set to
> >>>>> the DW specific function.  The logic to set UPSTAT_AUTOCTS is
> >>>>> moved so that any clock error will not affect setting the
> >>>>> hardware flow control.
> >>>> Bisection shows that this patch, commit
> >>>> 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
> >>>> Cavium Octeon III based UTM-8 board (MIPS architecture).
> >>>>
> >>>> I now get the following warning:
> >>>> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> >>>> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> >>>> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> >>>> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> >>>> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> >>>> [<ffffffff811901a0>] register_console+0x1c8/0x418
> >>>> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> >>>> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> >>>> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8 Then it hangs and
> >>>> the watchdog restarts the machine.
> >>>>
> >>>> Any ideas?
> >>>
> >>> 1. Does it use clock on that platform?
> >
> >> I've now dug a little deeper. Essentially what is going on is:
> >>
> >> 1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
> >> 2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns
> >> NULL
> >> 3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
> >>    doesn't match, since !IS_ERR(NULL)
> >> 4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns
> >> 0
> >> 5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
> >>    returns 0
> >> 6) dw8250_set_termios() thinks the frequency for that baud rate has
> >> been
> >>    set successfully and writes 0 into uartclk
> >> 7) it all goes wrong from there...
> >
> > So, it means we have need special care of NULL case here, and
> > honestly, I don't like it. But it seems the only feasible (quick)
> > fix right now.
>
> I agree. I think it should have been:
>
> if (IS_ERR_OR_NULL(d->clk) || !old)
>     goto out;
>
> I think it makes sense to validate to make sure the 'clk' pointer is
> valid before proceeding any further down below (regardless of how well
> or how not well the clock framework handles it).
>
> Thanks,
>
> Ray
>
> >
> >> The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in
> >> particular seems highly questionable to me, given that commit
> >> 93abe8e4b13a ("clk:
> >> add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:
> >>
> >>> These calls will return error for platforms that don't select
> >>> HAVE_CLK
> >>
> >> And NULL isn't an error in this API.
> >
> > Which is okay. I dunno what should be returned from clk_round_rate()
> > if clk is NULL. I would fix CLK framework, though I would like to
> > gather more details.
> >
> > Btw, I hope you also noticed this one:
> >
> > http://www.spinics.net/lists/linux-serial/msg25314.html
> >

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-04  0:02                 ` Jason Uy
@ 2017-03-04  0:11                   ` James Hogan
  0 siblings, 0 replies; 21+ messages in thread
From: James Hogan @ 2017-03-04  0:11 UTC (permalink / raw)
  To: Jason Uy
  Cc: Ray Jui, Andy Shevchenko, Heiko Stuebner, Greg Kroah-Hartman,
	Jiri Slaby, Kefeng Wang, Noam Camus, Heikki Krogerus,
	Wang Hongcheng, linux-serial, LKML, bcm-kernel-feedback-list,
	Linux MIPS Mailing List, David Daney, Russell King, linux-clk,
	Viresh Kumar

[-- Attachment #1: Type: text/plain, Size: 7263 bytes --]

Hi Jason,

On Fri, Mar 03, 2017 at 04:02:56PM -0800, Jason Uy wrote:
> HI James,
> 
> Maybe instead of that, we should do this instead
> 
> If ((IS_ERR(d->clk) && PTR_ERR(d->clk)) || !old)

IS_ERR(x) matches a range of non-zero negative values, so it implies
PTR_ERR(x) already, so it doesn't change anything.

Cheers
James

> 
> Note that this is what is done in the probe function of the dw driver.
> 
> Regards,
> Jason
> 
> -----Original Message-----
> From: James Hogan [mailto:james.hogan@imgtec.com]
> Sent: March-03-17 3:07 PM
> To: Jason Uy <jason.uy@broadcom.com>
> Cc: Ray Jui <ray.jui@broadcom.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Heiko Stuebner <heiko@sntech.de>; Greg
> Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby <jslaby@suse.com>;
> Kefeng Wang <wangkefeng.wang@huawei.com>; Noam Camus <noamc@ezchip.com>;
> Heikki Krogerus <heikki.krogerus@linux.intel.com>; Wang Hongcheng
> <annie.wang@amd.com>; linux-serial@vger.kernel.org; LKML
> <linux-kernel@vger.kernel.org>; bcm-kernel-feedback-list@broadcom.com; Linux
> MIPS Mailing List <linux-mips@linux-mips.org>; David Daney
> <david.daney@cavium.com>; Russell King <linux@armlinux.org.uk>;
> linux-clk@vger.kernel.org; Viresh Kumar <viresh.kumar@st.com>
> Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to
> be used
> 
> Hi Jason,
> 
> On Fri, Mar 03, 2017 at 09:43:55AM -0800, Jason Uy wrote:
> > James,
> >
> > Can you verify that changing the code to the following fixes your problem?
> >
> > if (IS_ERR_OR_NULL(d->clk) || !old)
> >     goto out;
> 
> It does, however I'm not at all convinced it is correct. clk_get either
> returns a valid opaque clock cookie that can be passed to other clock
> functions (which includes NULL), or ERR_PTR(-errno), which IS_ERR() should
> catch for errors.
> 
> According to this thread:
> 
> https://lists.gt.net/linux/kernel/2102623
> 
> we should stick to the clk API and use IS_ERR() rather than
> IS_ERR_OR_NULL(), but shouldn't be blindly accepting the result of
> clk_get_rate() (or I suppose clk_round_rate()), but rather checking for the
> value 0 and handling that case as "we don't have a usable clock from the clk
> api, fall back to something else".
> 
> Cheers
> James
> 
> >
> > Regards,
> > Jason
> >
> > -----Original Message-----
> > From: Ray Jui [mailto:ray.jui@broadcom.com]
> > Sent: March-03-17 9:34 AM
> > To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>; James Hogan
> > <james.hogan@imgtec.com>; Heiko Stuebner <heiko@sntech.de>
> > Cc: Jason Uy <jason.uy@broadcom.com>; Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org>; Jiri Slaby <jslaby@suse.com>; Kefeng
> > Wang <wangkefeng.wang@huawei.com>; Noam Camus <noamc@ezchip.com>;
> > Heikki Krogerus <heikki.krogerus@linux.intel.com>; Wang Hongcheng
> > <annie.wang@amd.com>; linux-serial@vger.kernel.org; LKML
> > <linux-kernel@vger.kernel.org>; bcm-kernel-feedback-list@broadcom.com;
> > Linux MIPS Mailing List <linux-mips@linux-mips.org>; David Daney
> > <david.daney@cavium.com>; Russell King <linux@armlinux.org.uk>;
> > linux-clk@vger.kernel.org; Viresh Kumar <viresh.kumar@st.com>
> > Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow
> > control to be used
> >
> > Hi Andy/Jason,
> >
> > On 3/3/2017 5:31 AM, Andy Shevchenko wrote:
> > > Heiko, you might be interested in this as well.
> > >
> > > On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
> > >> On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:
> > >>> On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:
> > >>>> On 11 January 2017 at 19:48, Jason Uy <jason.uy@broadcom.com>
> > >>>> wrote:
> > >>>>> In the most common use case, the Synopsys DW UART driver does
> > >>>>> not set the set_termios callback function.  This prevents
> > >>>>> UPSTAT_AUTOCTS from being set when the UART flag CRTSCTS is set.
> > >>>>> As a result, the driver will use software flow control as
> > >>>>> opposed to hardware flow control.
> > >>>>>
> > >>>>> To fix the problem, the set_termios callback function is set to
> > >>>>> the DW specific function.  The logic to set UPSTAT_AUTOCTS is
> > >>>>> moved so that any clock error will not affect setting the
> > >>>>> hardware flow control.
> > >>>> Bisection shows that this patch, commit
> > >>>> 6a171b29937984a5e0bf29d6577b055998f03edb, has broken boot of the
> > >>>> Cavium Octeon III based UTM-8 board (MIPS architecture).
> > >>>>
> > >>>> I now get the following warning:
> > >>>> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> > >>>> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> > >>>> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> > >>>> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> > >>>> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> > >>>> [<ffffffff811901a0>] register_console+0x1c8/0x418
> > >>>> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> > >>>> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> > >>>> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8 Then it hangs and
> > >>>> the watchdog restarts the machine.
> > >>>>
> > >>>> Any ideas?
> > >>>
> > >>> 1. Does it use clock on that platform?
> > >
> > >> I've now dug a little deeper. Essentially what is going on is:
> > >>
> > >> 1) CONFIG_HAVE_CLK=n (Octeon doesn't select it)
> > >> 2) The CONFIG_HAVE_CLK=n implementation of devm_clk_get() returns
> > >> NULL
> > >> 3) The "if (IS_ERR(d->clk) || !old) {" check in dw8250_set_termios()
> > >>    doesn't match, since !IS_ERR(NULL)
> > >> 4) The CONFIG_HAVE_CLK=n implementation of clk_round_rate() returns
> > >> 0
> > >> 5) The CONFIG_HAVE_CLK=n implementation of clk_set_rate(d->clk, 0)
> > >>    returns 0
> > >> 6) dw8250_set_termios() thinks the frequency for that baud rate has
> > >> been
> > >>    set successfully and writes 0 into uartclk
> > >> 7) it all goes wrong from there...
> > >
> > > So, it means we have need special care of NULL case here, and
> > > honestly, I don't like it. But it seems the only feasible (quick)
> > > fix right now.
> >
> > I agree. I think it should have been:
> >
> > if (IS_ERR_OR_NULL(d->clk) || !old)
> >     goto out;
> >
> > I think it makes sense to validate to make sure the 'clk' pointer is
> > valid before proceeding any further down below (regardless of how well
> > or how not well the clock framework handles it).
> >
> > Thanks,
> >
> > Ray
> >
> > >
> > >> The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in
> > >> particular seems highly questionable to me, given that commit
> > >> 93abe8e4b13a ("clk:
> > >> add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:
> > >>
> > >>> These calls will return error for platforms that don't select
> > >>> HAVE_CLK
> > >>
> > >> And NULL isn't an error in this API.
> > >
> > > Which is okay. I dunno what should be returned from clk_round_rate()
> > > if clk is NULL. I would fix CLK framework, though I would like to
> > > gather more details.
> > >
> > > Btw, I hope you also noticed this one:
> > >
> > > http://www.spinics.net/lists/linux-serial/msg25314.html
> > >

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used
  2017-03-03 23:23           ` James Hogan
@ 2017-03-04  2:56             ` Andy Shevchenko
  2017-03-04 13:09               ` [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n James Hogan
  0 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2017-03-04  2:56 UTC (permalink / raw)
  To: James Hogan
  Cc: Andy Shevchenko, Heiko Stuebner, Jason Uy, Greg Kroah-Hartman,
	Jiri Slaby, Kefeng Wang, Noam Camus, Heikki Krogerus,
	Wang Hongcheng, linux-serial, LKML, bcm-kernel-feedback-list,
	Linux MIPS Mailing List, David Daney, Russell King, linux-clk,
	Viresh Kumar

On Sat, Mar 4, 2017 at 1:23 AM, James Hogan <james.hogan@imgtec.com> wrote:
> On Fri, Mar 03, 2017 at 03:31:06PM +0200, Andy Shevchenko wrote:
>> On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:
>> > The CONFIG_HAVE_CLK=n implementation of devm_clk_get() in particular
>> > seems highly questionable to me, given that commit 93abe8e4b13a ("clk:
>> > add non CONFIG_HAVE_CLK routines") which added it 5 years ago says:

>> Btw, I hope you also noticed this one:
>>
>> http://www.spinics.net/lists/linux-serial/msg25314.html
>
> Interesting.
>
> Following Russel's past advise[1], the following patch on top of Heiko's
> patch also fixes things for me on Octeon:
>
> [1] https://lists.gt.net/linux/kernel/2102623
>
> If thats an acceptable fix I'll post it properly. Thoughts?

Fine by me. Looks definitely better than IS_ERR_OR_NULL().

Please, submit as usual with your SoB tag.

> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 223ac234ddb2..e65808c482f1 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -267,6 +267,8 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>         rate = clk_round_rate(d->clk, baud * 16);
>         if (rate < 0)
>                 ret = rate;
> +       else if (rate == 0)
> +               ret = -ENOENT;
>         else
>                 ret = clk_set_rate(d->clk, rate);
>         clk_prepare_enable(d->clk);

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n
  2017-03-04  2:56             ` Andy Shevchenko
@ 2017-03-04 13:09               ` James Hogan
  2017-03-04 14:37                 ` Andy Shevchenko
                                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: James Hogan @ 2017-03-04 13:09 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, James Hogan, Greg Kroah-Hartman, Andy Shevchenko,
	Jason Uy, Kefeng Wang, Heiko Stuebner, David Daney, Russell King,
	linux-serial, linux-clk, linux-mips, bcm-kernel-feedback-list

Commit 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be
used") recently broke the 8250_dw driver on platforms which don't select
HAVE_CLK, as dw8250_set_termios() gets confused by the behaviour of the
fallback HAVE_CLK=n clock API in linux/clk.h which pretends everything
is fine but returns (valid) NULL clocks and 0 HZ clock rates.

That 0 rate is written into the uartclk resulting in a crash at boot,
e.g. on Cavium Octeon III based UTM-8 we get something like this:

1180000000800.serial: ttyS0 at MMIO 0x1180000000800 (irq = 41, base_baud = 25000000) is a OCTEON
------------[ cut here ]------------
WARNING: CPU: 2 PID: 1 at drivers/tty/serial/serial_core.c:441 uart_get_baud_rate+0xfc/0x1f0
...
Call Trace:
...
[<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
[<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
[<ffffffff8149c710>] uart_set_options+0xe8/0x190
[<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
[<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
[<ffffffff811901a0>] register_console+0x1c8/0x418
[<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
[<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
[<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
...

The clock API is defined such that NULL is a valid clock handle so it
wouldn't be right to check explicitly for NULL. Instead treat a
clk_round_rate() return value of 0 as an error which prevents uartclk
being overwritten.

Fixes: 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be used")
Signed-off-by: James Hogan <james.hogan@imgtec.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Jason Uy <jason.uy@broadcom.com>
Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
Cc: Heiko Stuebner <heiko@sntech.de>
Cc: David Daney <david.daney@cavium.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: linux-serial@vger.kernel.org
Cc: linux-clk@vger.kernel.org
Cc: linux-mips@linux-mips.org
Cc: bcm-kernel-feedback-list@broadcom.com
---
 drivers/tty/serial/8250/8250_dw.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 223ac234ddb2..e65808c482f1 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -267,6 +267,8 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
 	rate = clk_round_rate(d->clk, baud * 16);
 	if (rate < 0)
 		ret = rate;
+	else if (rate == 0)
+		ret = -ENOENT;
 	else
 		ret = clk_set_rate(d->clk, rate);
 	clk_prepare_enable(d->clk);
-- 
2.11.1

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n
  2017-03-04 13:09               ` [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n James Hogan
@ 2017-03-04 14:37                 ` Andy Shevchenko
  2017-03-06 10:16                   ` James Hogan
  2017-03-05  0:44                 ` Heiko Stuebner
  2017-03-13 11:14                 ` James Hogan
  2 siblings, 1 reply; 21+ messages in thread
From: Andy Shevchenko @ 2017-03-04 14:37 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-kernel, Greg Kroah-Hartman, Andy Shevchenko, Jason Uy,
	Kefeng Wang, Heiko Stuebner, David Daney, Russell King,
	linux-serial, linux-clk, linux-mips, bcm-kernel-feedback-list

On Sat, Mar 4, 2017 at 3:09 PM, James Hogan <james.hogan@imgtec.com> wrote:
> Commit 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be
> used") recently broke the 8250_dw driver on platforms which don't select
> HAVE_CLK, as dw8250_set_termios() gets confused by the behaviour of the
> fallback HAVE_CLK=n clock API in linux/clk.h which pretends everything
> is fine but returns (valid) NULL clocks and 0 HZ clock rates.
>
> That 0 rate is written into the uartclk resulting in a crash at boot,
> e.g. on Cavium Octeon III based UTM-8 we get something like this:
>
> 1180000000800.serial: ttyS0 at MMIO 0x1180000000800 (irq = 41, base_baud = 25000000) is a OCTEON
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 1 at drivers/tty/serial/serial_core.c:441 uart_get_baud_rate+0xfc/0x1f0
> ...
> Call Trace:
> ...
> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> [<ffffffff811901a0>] register_console+0x1c8/0x418
> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> ...
>
> The clock API is defined such that NULL is a valid clock handle so it
> wouldn't be right to check explicitly for NULL. Instead treat a
> clk_round_rate() return value of 0 as an error which prevents uartclk
> being overwritten.
>

You forgot to add that it is dependent to Heiko's patch
http://www.spinics.net/lists/linux-serial/msg25314.html

Patch looks good to me and shouldn't bring any regression to Intel
hardware (x86 is using clock framework).

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Fixes: 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be used")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jason Uy <jason.uy@broadcom.com>
> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-serial@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: bcm-kernel-feedback-list@broadcom.com
> ---
>  drivers/tty/serial/8250/8250_dw.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
> index 223ac234ddb2..e65808c482f1 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -267,6 +267,8 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>         rate = clk_round_rate(d->clk, baud * 16);
>         if (rate < 0)
>                 ret = rate;
> +       else if (rate == 0)
> +               ret = -ENOENT;
>         else
>                 ret = clk_set_rate(d->clk, rate);
>         clk_prepare_enable(d->clk);
> --
> 2.11.1
>



-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n
  2017-03-04 13:09               ` [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n James Hogan
  2017-03-04 14:37                 ` Andy Shevchenko
@ 2017-03-05  0:44                 ` Heiko Stuebner
  2017-03-13 11:14                 ` James Hogan
  2 siblings, 0 replies; 21+ messages in thread
From: Heiko Stuebner @ 2017-03-05  0:44 UTC (permalink / raw)
  To: James Hogan
  Cc: Andy Shevchenko, linux-kernel, Greg Kroah-Hartman,
	Andy Shevchenko, Jason Uy, Kefeng Wang, David Daney,
	Russell King, linux-serial, linux-clk, linux-mips,
	bcm-kernel-feedback-list

Am Samstag, 4. März 2017, 13:09:58 CET schrieb James Hogan:
> Commit 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be
> used") recently broke the 8250_dw driver on platforms which don't select
> HAVE_CLK, as dw8250_set_termios() gets confused by the behaviour of the
> fallback HAVE_CLK=n clock API in linux/clk.h which pretends everything
> is fine but returns (valid) NULL clocks and 0 HZ clock rates.
> 
> That 0 rate is written into the uartclk resulting in a crash at boot,
> e.g. on Cavium Octeon III based UTM-8 we get something like this:
> 
> 1180000000800.serial: ttyS0 at MMIO 0x1180000000800 (irq = 41, base_baud =
> 25000000) is a OCTEON ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 1 at drivers/tty/serial/serial_core.c:441
> uart_get_baud_rate+0xfc/0x1f0 ...
> Call Trace:
> ...
> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> [<ffffffff811901a0>] register_console+0x1c8/0x418
> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> ...
> 
> The clock API is defined such that NULL is a valid clock handle so it
> wouldn't be right to check explicitly for NULL. Instead treat a
> clk_round_rate() return value of 0 as an error which prevents uartclk
> being overwritten.
> 
> Fixes: 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be
> used") Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jason Uy <jason.uy@broadcom.com>
> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-serial@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: bcm-kernel-feedback-list@broadcom.com
> ---
>  drivers/tty/serial/8250/8250_dw.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_dw.c
> b/drivers/tty/serial/8250/8250_dw.c index 223ac234ddb2..e65808c482f1 100644
> --- a/drivers/tty/serial/8250/8250_dw.c
> +++ b/drivers/tty/serial/8250/8250_dw.c
> @@ -267,6 +267,8 @@ static void dw8250_set_termios(struct uart_port *p,
> struct ktermios *termios, rate = clk_round_rate(d->clk, baud * 16);
>  	if (rate < 0)
>  		ret = rate;
> +	else if (rate == 0)
> +		ret = -ENOENT;
>  	else
>  		ret = clk_set_rate(d->clk, rate);
>  	clk_prepare_enable(d->clk);

Looks good
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n
  2017-03-04 14:37                 ` Andy Shevchenko
@ 2017-03-06 10:16                   ` James Hogan
  2017-03-06 23:38                     ` Jason Uy
  0 siblings, 1 reply; 21+ messages in thread
From: James Hogan @ 2017-03-06 10:16 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, Greg Kroah-Hartman, Andy Shevchenko, Jason Uy,
	Kefeng Wang, Heiko Stuebner, David Daney, Russell King,
	linux-serial, linux-clk, linux-mips, bcm-kernel-feedback-list

[-- Attachment #1: Type: text/plain, Size: 2098 bytes --]

On Sat, Mar 04, 2017 at 04:37:17PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 4, 2017 at 3:09 PM, James Hogan <james.hogan@imgtec.com> wrote:
> > Commit 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be
> > used") recently broke the 8250_dw driver on platforms which don't select
> > HAVE_CLK, as dw8250_set_termios() gets confused by the behaviour of the
> > fallback HAVE_CLK=n clock API in linux/clk.h which pretends everything
> > is fine but returns (valid) NULL clocks and 0 HZ clock rates.
> >
> > That 0 rate is written into the uartclk resulting in a crash at boot,
> > e.g. on Cavium Octeon III based UTM-8 we get something like this:
> >
> > 1180000000800.serial: ttyS0 at MMIO 0x1180000000800 (irq = 41, base_baud = 25000000) is a OCTEON
> > ------------[ cut here ]------------
> > WARNING: CPU: 2 PID: 1 at drivers/tty/serial/serial_core.c:441 uart_get_baud_rate+0xfc/0x1f0
> > ...
> > Call Trace:
> > ...
> > [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> > [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> > [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> > [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> > [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> > [<ffffffff811901a0>] register_console+0x1c8/0x418
> > [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> > [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> > [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> > ...
> >
> > The clock API is defined such that NULL is a valid clock handle so it
> > wouldn't be right to check explicitly for NULL. Instead treat a
> > clk_round_rate() return value of 0 as an error which prevents uartclk
> > being overwritten.
> >
> 
> You forgot to add that it is dependent to Heiko's patch
> http://www.spinics.net/lists/linux-serial/msg25314.html

Indeed I did. Sorry about that.

> 
> Patch looks good to me and shouldn't bring any regression to Intel
> hardware (x86 is using clock framework).
> 
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* RE: [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n
  2017-03-06 10:16                   ` James Hogan
@ 2017-03-06 23:38                     ` Jason Uy
  0 siblings, 0 replies; 21+ messages in thread
From: Jason Uy @ 2017-03-06 23:38 UTC (permalink / raw)
  To: James Hogan, Andy Shevchenko
  Cc: linux-kernel, Greg Kroah-Hartman, Andy Shevchenko, Kefeng Wang,
	Heiko Stuebner, David Daney, Russell King, linux-serial,
	linux-clk, linux-mips, bcm-kernel-feedback-list

Looks good
Reviewed-by: Jason Uy <jason.uy@broadcom.com>

-----Original Message-----
From: James Hogan [mailto:james.hogan@imgtec.com]
Sent: March-06-17 2:16 AM
To: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: linux-kernel@vger.kernel.org; Greg Kroah-Hartman
<gregkh@linuxfoundation.org>; Andy Shevchenko
<andriy.shevchenko@linux.intel.com>; Jason Uy <jason.uy@broadcom.com>;
Kefeng Wang <wangkefeng.wang@huawei.com>; Heiko Stuebner <heiko@sntech.de>;
David Daney <david.daney@cavium.com>; Russell King <linux@armlinux.org.uk>;
linux-serial@vger.kernel.org; linux-clk@vger.kernel.org;
linux-mips@linux-mips.org; bcm-kernel-feedback-list
<bcm-kernel-feedback-list@broadcom.com>
Subject: Re: [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n

On Sat, Mar 04, 2017 at 04:37:17PM +0200, Andy Shevchenko wrote:
> On Sat, Mar 4, 2017 at 3:09 PM, James Hogan <james.hogan@imgtec.com>
> wrote:
> > Commit 6a171b299379 ("serial: 8250_dw: Allow hardware flow control
> > to be
> > used") recently broke the 8250_dw driver on platforms which don't
> > select HAVE_CLK, as dw8250_set_termios() gets confused by the
> > behaviour of the fallback HAVE_CLK=n clock API in linux/clk.h which
> > pretends everything is fine but returns (valid) NULL clocks and 0 HZ
> > clock rates.
> >
> > That 0 rate is written into the uartclk resulting in a crash at
> > boot, e.g. on Cavium Octeon III based UTM-8 we get something like this:
> >
> > 1180000000800.serial: ttyS0 at MMIO 0x1180000000800 (irq = 41,
> > base_baud = 25000000) is a OCTEON ------------[ cut here
> > ]------------
> > WARNING: CPU: 2 PID: 1 at drivers/tty/serial/serial_core.c:441
> > uart_get_baud_rate+0xfc/0x1f0 ...
> > Call Trace:
> > ...
> > [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> > [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> > [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> > [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> > [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> > [<ffffffff811901a0>] register_console+0x1c8/0x418
> > [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> > [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> > [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8 ...
> >
> > The clock API is defined such that NULL is a valid clock handle so
> > it wouldn't be right to check explicitly for NULL. Instead treat a
> > clk_round_rate() return value of 0 as an error which prevents
> > uartclk being overwritten.
> >
>
> You forgot to add that it is dependent to Heiko's patch
> http://www.spinics.net/lists/linux-serial/msg25314.html

Indeed I did. Sorry about that.

>
> Patch looks good to me and shouldn't bring any regression to Intel
> hardware (x86 is using clock framework).
>
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thanks
James

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n
  2017-03-04 13:09               ` [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n James Hogan
  2017-03-04 14:37                 ` Andy Shevchenko
  2017-03-05  0:44                 ` Heiko Stuebner
@ 2017-03-13 11:14                 ` James Hogan
  2017-03-14  2:21                   ` Greg Kroah-Hartman
  2 siblings, 1 reply; 21+ messages in thread
From: James Hogan @ 2017-03-13 11:14 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Andy Shevchenko, Andy Shevchenko, Jason Uy,
	Kefeng Wang, Heiko Stuebner, David Daney, Russell King,
	linux-serial, linux-clk, linux-mips, bcm-kernel-feedback-list

[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]

Hi Greg,

On Sat, Mar 04, 2017 at 01:09:58PM +0000, James Hogan wrote:
> Commit 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be
> used") recently broke the 8250_dw driver on platforms which don't select
> HAVE_CLK, as dw8250_set_termios() gets confused by the behaviour of the
> fallback HAVE_CLK=n clock API in linux/clk.h which pretends everything
> is fine but returns (valid) NULL clocks and 0 HZ clock rates.
> 
> That 0 rate is written into the uartclk resulting in a crash at boot,
> e.g. on Cavium Octeon III based UTM-8 we get something like this:
> 
> 1180000000800.serial: ttyS0 at MMIO 0x1180000000800 (irq = 41, base_baud = 25000000) is a OCTEON
> ------------[ cut here ]------------
> WARNING: CPU: 2 PID: 1 at drivers/tty/serial/serial_core.c:441 uart_get_baud_rate+0xfc/0x1f0
> ...
> Call Trace:
> ...
> [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> [<ffffffff811901a0>] register_console+0x1c8/0x418
> [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> ...
> 
> The clock API is defined such that NULL is a valid clock handle so it
> wouldn't be right to check explicitly for NULL. Instead treat a
> clk_round_rate() return value of 0 as an error which prevents uartclk
> being overwritten.
> 
> Fixes: 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be used")
> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Jason Uy <jason.uy@broadcom.com>
> Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> Cc: Heiko Stuebner <heiko@sntech.de>
> Cc: David Daney <david.daney@cavium.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: linux-serial@vger.kernel.org
> Cc: linux-clk@vger.kernel.org
> Cc: linux-mips@linux-mips.org
> Cc: bcm-kernel-feedback-list@broadcom.com

Any chance we could have this patch in v4.11-rc3?

As Andy pointed out, it depends on Heiko's patch:
https://www.spinics.net/lists/linux-serial/msg25483.html

Thanks
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n
  2017-03-13 11:14                 ` James Hogan
@ 2017-03-14  2:21                   ` Greg Kroah-Hartman
  0 siblings, 0 replies; 21+ messages in thread
From: Greg Kroah-Hartman @ 2017-03-14  2:21 UTC (permalink / raw)
  To: James Hogan
  Cc: linux-kernel, Andy Shevchenko, Andy Shevchenko, Jason Uy,
	Kefeng Wang, Heiko Stuebner, David Daney, Russell King,
	linux-serial, linux-clk, linux-mips, bcm-kernel-feedback-list

On Mon, Mar 13, 2017 at 11:14:07AM +0000, James Hogan wrote:
> Hi Greg,
> 
> On Sat, Mar 04, 2017 at 01:09:58PM +0000, James Hogan wrote:
> > Commit 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be
> > used") recently broke the 8250_dw driver on platforms which don't select
> > HAVE_CLK, as dw8250_set_termios() gets confused by the behaviour of the
> > fallback HAVE_CLK=n clock API in linux/clk.h which pretends everything
> > is fine but returns (valid) NULL clocks and 0 HZ clock rates.
> > 
> > That 0 rate is written into the uartclk resulting in a crash at boot,
> > e.g. on Cavium Octeon III based UTM-8 we get something like this:
> > 
> > 1180000000800.serial: ttyS0 at MMIO 0x1180000000800 (irq = 41, base_baud = 25000000) is a OCTEON
> > ------------[ cut here ]------------
> > WARNING: CPU: 2 PID: 1 at drivers/tty/serial/serial_core.c:441 uart_get_baud_rate+0xfc/0x1f0
> > ...
> > Call Trace:
> > ...
> > [<ffffffff8149c2e4>] uart_get_baud_rate+0xfc/0x1f0
> > [<ffffffff814a5098>] serial8250_do_set_termios+0xb0/0x440
> > [<ffffffff8149c710>] uart_set_options+0xe8/0x190
> > [<ffffffff814a6cdc>] serial8250_console_setup+0x84/0x158
> > [<ffffffff814a11ec>] univ8250_console_setup+0x54/0x70
> > [<ffffffff811901a0>] register_console+0x1c8/0x418
> > [<ffffffff8149f004>] uart_add_one_port+0x434/0x4b0
> > [<ffffffff814a1af8>] serial8250_register_8250_port+0x2d8/0x440
> > [<ffffffff814aa620>] dw8250_probe+0x388/0x5e8
> > ...
> > 
> > The clock API is defined such that NULL is a valid clock handle so it
> > wouldn't be right to check explicitly for NULL. Instead treat a
> > clk_round_rate() return value of 0 as an error which prevents uartclk
> > being overwritten.
> > 
> > Fixes: 6a171b299379 ("serial: 8250_dw: Allow hardware flow control to be used")
> > Signed-off-by: James Hogan <james.hogan@imgtec.com>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Jason Uy <jason.uy@broadcom.com>
> > Cc: Kefeng Wang <wangkefeng.wang@huawei.com>
> > Cc: Heiko Stuebner <heiko@sntech.de>
> > Cc: David Daney <david.daney@cavium.com>
> > Cc: Russell King <linux@armlinux.org.uk>
> > Cc: linux-serial@vger.kernel.org
> > Cc: linux-clk@vger.kernel.org
> > Cc: linux-mips@linux-mips.org
> > Cc: bcm-kernel-feedback-list@broadcom.com
> 
> Any chance we could have this patch in v4.11-rc3?
> 
> As Andy pointed out, it depends on Heiko's patch:
> https://www.spinics.net/lists/linux-serial/msg25483.html

Yes, will be queueing both up soon.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2017-03-14  2:21 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-11 19:48 [PATCH v2 0/1] Allow hardware flow control to be used Jason Uy
2017-01-11 19:48 ` [PATCH v2 1/1] serial: 8250_dw: " Jason Uy
2017-01-11 19:53   ` Andy Shevchenko
2017-01-13  1:33   ` Kefeng Wang
     [not found]   ` <CAAG0J9-n0toSJL8Ze8Esq81dYnpfrTd42bMiR94zw_btBLjsww@mail.gmail.com>
2017-03-01 18:50     ` Andy Shevchenko
2017-03-03  0:21       ` James Hogan
2017-03-03 13:31         ` Andy Shevchenko
2017-03-03 17:33           ` Ray Jui
2017-03-03 17:43             ` Jason Uy
2017-03-03 23:07               ` James Hogan
2017-03-04  0:02                 ` Jason Uy
2017-03-04  0:11                   ` James Hogan
2017-03-03 23:23           ` James Hogan
2017-03-04  2:56             ` Andy Shevchenko
2017-03-04 13:09               ` [PATCH] serial: 8250_dw: Fix breakage when HAVE_CLK=n James Hogan
2017-03-04 14:37                 ` Andy Shevchenko
2017-03-06 10:16                   ` James Hogan
2017-03-06 23:38                     ` Jason Uy
2017-03-05  0:44                 ` Heiko Stuebner
2017-03-13 11:14                 ` James Hogan
2017-03-14  2:21                   ` Greg Kroah-Hartman

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).