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@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 > >
