Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used

[James Hogan]

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@xxxxxxxxxx]
> Sent: March-03-17 3:07 PM
> To: Jason Uy <jason.uy@xxxxxxxxxxxx>
> Cc: Ray Jui <ray.jui@xxxxxxxxxxxx>; Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxxxxxxxx>; Heiko Stuebner <heiko@xxxxxxxxx>; Greg
> Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby <jslaby@xxxxxxxx>;
> Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>; Noam Camus <noamc@xxxxxxxxxx>;
> Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>; Wang Hongcheng
> <annie.wang@xxxxxxx>; linux-serial@xxxxxxxxxxxxxxx; LKML
> <linux-kernel@xxxxxxxxxxxxxxx>; bcm-kernel-feedback-list@xxxxxxxxxxxx; Linux
> MIPS Mailing List <linux-mips@xxxxxxxxxxxxxx>; David Daney
> <david.daney@xxxxxxxxxx>; Russell King <linux@xxxxxxxxxxxxxxx>;
> linux-clk@xxxxxxxxxxxxxxx; Viresh Kumar <viresh.kumar@xxxxxx>
> 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@xxxxxxxxxxxx]
> > Sent: March-03-17 9:34 AM
> > To: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>; James Hogan
> > <james.hogan@xxxxxxxxxx>; Heiko Stuebner <heiko@xxxxxxxxx>
> > Cc: Jason Uy <jason.uy@xxxxxxxxxxxx>; Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx>; Jiri Slaby <jslaby@xxxxxxxx>; Kefeng
> > Wang <wangkefeng.wang@xxxxxxxxxx>; Noam Camus <noamc@xxxxxxxxxx>;
> > Heikki Krogerus <heikki.krogerus@xxxxxxxxxxxxxxx>; Wang Hongcheng
> > <annie.wang@xxxxxxx>; linux-serial@xxxxxxxxxxxxxxx; LKML
> > <linux-kernel@xxxxxxxxxxxxxxx>; bcm-kernel-feedback-list@xxxxxxxxxxxx;
> > Linux MIPS Mailing List <linux-mips@xxxxxxxxxxxxxx>; David Daney
> > <david.daney@xxxxxxxxxx>; Russell King <linux@xxxxxxxxxxxxxxx>;
> > linux-clk@xxxxxxxxxxxxxxx; Viresh Kumar <viresh.kumar@xxxxxx>
> > 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@xxxxxxxxxxxx>
> > >>>> 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: signature.asc
Description: Digital signature