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 <redacted>
Cc: Ray Jui <redacted>; Andy Shevchenko
[off-list ref]; Heiko Stuebner [off-list ref]; Greg
Kroah-Hartman [off-list ref]; Jiri Slaby [off-list ref];
Kefeng Wang [off-list ref]; Noam Camus [off-list ref];
Heikki Krogerus [off-list ref]; Wang Hongcheng
[off-list ref]; linux-serial@vger.kernel.org; LKML
[off-list ref]; bcm-kernel-feedback-list@broadcom.com; Linux
MIPS Mailing List [off-list ref]; David Daney
[off-list ref]; Russell King [off-list ref];
linux-clk@vger.kernel.org; Viresh Kumar [off-list ref]
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:

quoted

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

quoted

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
[off-list ref]; Heiko Stuebner [off-list ref]
Cc: Jason Uy <redacted>; Greg Kroah-Hartman
[off-list ref]; Jiri Slaby [off-list ref]; Kefeng
Wang [off-list ref]; Noam Camus [off-list ref];
Heikki Krogerus [off-list ref]; Wang Hongcheng
[off-list ref]; linux-serial@vger.kernel.org; LKML
[off-list ref]; bcm-kernel-feedback-list@broadcom.com;
Linux MIPS Mailing List [off-list ref]; David Daney
[off-list ref]; Russell King [off-list ref];
linux-clk@vger.kernel.org; Viresh Kumar [off-list ref]
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:

quoted

Heiko, you might be interested in this as well.

On Fri, 2017-03-03 at 00:21 +0000, James Hogan wrote:

quoted

On Wed, Mar 01, 2017 at 08:50:20PM +0200, Andy Shevchenko wrote:

quoted

On Wed, 2017-03-01 at 18:02 +0000, James Hogan wrote:

quoted

On 11 January 2017 at 19:48, Jason Uy [off-list ref]
wrote:

quoted

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?

quoted

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

quoted

quoted

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:

quoted

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