From: James Hogan <james.hogan@imgtec.com> 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 Date: Sat, 4 Mar 2017 00:11:30 +0000 [thread overview] Message-ID: <20170304001130.GV996@jhogan-linux.le.imgtec.org> (raw) In-Reply-To: <69272eeb863f39633c51b0312c9ccd2f@mail.gmail.com> [-- 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 --]
WARNING: multiple messages have this Message-ID (diff)
From: James Hogan <james.hogan@imgtec.com> 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 Date: Sat, 4 Mar 2017 00:11:30 +0000 [thread overview] Message-ID: <20170304001130.GV996@jhogan-linux.le.imgtec.org> (raw) Message-ID: <20170304001130.BgRLuBTgLaXgUbViNNMtx4qKzpFckkG8tGrhI2iZfuU@z> (raw) In-Reply-To: <69272eeb863f39633c51b0312c9ccd2f@mail.gmail.com> [-- 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 --]
next prev parent reply other threads:[~2017-03-04 0:11 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 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 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 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-03 23:07 ` James Hogan 2017-03-04 0:02 ` Jason Uy 2017-03-04 0:11 ` James Hogan [this message] 2017-03-04 0:11 ` James Hogan 2017-03-03 23:23 ` 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 13:09 ` James Hogan 2017-03-04 14:37 ` Andy Shevchenko 2017-03-06 10:16 ` James Hogan 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-13 11:14 ` James Hogan 2017-03-14 2:21 ` Greg Kroah-Hartman
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20170304001130.GV996@jhogan-linux.le.imgtec.org \ --to=james.hogan@imgtec.com \ --cc=andriy.shevchenko@linux.intel.com \ --cc=annie.wang@amd.com \ --cc=bcm-kernel-feedback-list@broadcom.com \ --cc=david.daney@cavium.com \ --cc=gregkh@linuxfoundation.org \ --cc=heikki.krogerus@linux.intel.com \ --cc=heiko@sntech.de \ --cc=jason.uy@broadcom.com \ --cc=jslaby@suse.com \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mips@linux-mips.org \ --cc=linux-serial@vger.kernel.org \ --cc=linux@armlinux.org.uk \ --cc=noamc@ezchip.com \ --cc=ray.jui@broadcom.com \ --cc=viresh.kumar@st.com \ --cc=wangkefeng.wang@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.