From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752089AbdCCXJZ (ORCPT ); Fri, 3 Mar 2017 18:09:25 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:33183 "EHLO imgpgp01.kl.imgtec.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752064AbdCCXJW (ORCPT ); Fri, 3 Mar 2017 18:09:22 -0500 X-PGP-Universal: processed; by imgpgp01.kl.imgtec.org on Sat, 04 Mar 2017 00:11:58 +0000 Date: Fri, 3 Mar 2017 23:07:13 +0000 From: James Hogan To: Jason Uy CC: Ray Jui , Andy Shevchenko , Heiko Stuebner , Greg Kroah-Hartman , Jiri Slaby , Kefeng Wang , Noam Camus , Heikki Krogerus , Wang Hongcheng , , LKML , , Linux MIPS Mailing List , David Daney , Russell King , , Viresh Kumar Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used Message-ID: <20170303230712.GT996@jhogan-linux.le.imgtec.org> References: <1484164100-9805-1-git-send-email-jason.uy@broadcom.com> <1484164100-9805-2-git-send-email-jason.uy@broadcom.com> <1488394220.20145.68.camel@linux.intel.com> <20170303002129.GS996@jhogan-linux.le.imgtec.org> <1488547866.20145.74.camel@linux.intel.com> <3e45a0582dd91dab1a6e9bd6f4339e12@mail.gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mvzZjokS1nTZS3h1" Content-Disposition: inline In-Reply-To: <3e45a0582dd91dab1a6e9bd6f4339e12@mail.gmail.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [192.168.154.110] X-ESG-ENCRYPT-TAG: 1b7d744b Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --mvzZjokS1nTZS3h1 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Jason, On Fri, Mar 03, 2017 at 09:43:55AM -0800, Jason Uy wrote: > James, >=20 > Can you verify that changing the code to the following fixes your problem? >=20 > 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 >=20 > Regards, > Jason >=20 > -----Original Message----- > From: Ray Jui [mailto:ray.jui@broadcom.com] > Sent: March-03-17 9:34 AM > To: Andy Shevchenko ; James Hogan > ; Heiko Stuebner > Cc: Jason Uy ; Greg Kroah-Hartman > ; Jiri Slaby ; Kefeng Wang > ; Noam Camus ; Heikki Kroge= rus > ; Wang Hongcheng ; > linux-serial@vger.kernel.org; LKML ; > bcm-kernel-feedback-list@broadcom.com; Linux MIPS Mailing List > ; David Daney ; Russell > King ; linux-clk@vger.kernel.org; Viresh Kumar > > Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control = to > be used >=20 > Hi Andy/Jason, >=20 > 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 > >>>> 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: > >>>> [] uart_get_baud_rate+0xfc/0x1f0 > >>>> [] serial8250_do_set_termios+0xb0/0x440 > >>>> [] uart_set_options+0xe8/0x190 > >>>> [] serial8250_console_setup+0x84/0x158 > >>>> [] univ8250_console_setup+0x54/0x70 > >>>> [] register_console+0x1c8/0x418 > >>>> [] uart_add_one_port+0x434/0x4b0 > >>>> [] serial8250_register_8250_port+0x2d8/0x440 > >>>> [] 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=3Dn (Octeon doesn't select it) > >> 2) The CONFIG_HAVE_CLK=3Dn 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=3Dn implementation of clk_round_rate() returns 0 > >> 5) The CONFIG_HAVE_CLK=3Dn 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. >=20 > I agree. I think it should have been: >=20 > if (IS_ERR_OR_NULL(d->clk) || !old) > goto out; >=20 > 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 n= ot > well the clock framework handles it). >=20 > Thanks, >=20 > Ray >=20 > > > >> The CONFIG_HAVE_CLK=3Dn 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 > > --mvzZjokS1nTZS3h1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYufcZAAoJEGwLaZPeOHZ6ZxIQAIK+69uA+O+ZucfIIVEtqb5M TVTWaGCoGbTorVa7GI4RAJjfO/0J5QPZyVL8VZiAHI3kovr71VqpVx2dZRtC5ZA/ yABfv14l9GWX/Tvb91A01RssVlTG3pQ7QMURsW8RbqAbnjxuqCZ8IG/bVArw20Py ZYpsYmS4sfxjdQBbtfQVzh1kpEDfCAKL0dvrtzY2sAQ3a4AQptU+Z5sQLMaw+Jmy SUaMXKCM0Efhz6W8TdX9qhX5dm3N63JheO7uW3exDJjRe3fcej2ufSos0wK/xrwr QU+LDVtf8jp7xfFt28ml6ghFEFow2oaYH0GYakg5UvQBV5P2NzP680mznCpBv/fJ Wf2RyZ6YIjFMrYAmUCyyLHLLTFi6oO5qowcfbKYluB7Akr0UcKzrIuI4kGc03C49 LyDs4hyj9EBE063mcbuJSBmRh8+nLdiNE9FMh0C4EtTVRPyObR+R+O50wYzo/37r G0PPFBOmso0cUdqwmBFrhW0bpkogaaWRcWXfEjMcLSZYcncNAVhGbH3xnwcRthJn vsDMiseZ1EIxNQe/qcIxUPv1fytdz8APYo+vY+rDepxEXafh2/M0cye6nTDMXqDF TzseJAhra3G8C4xDH1+ikXyN0ExmUk/j5VC8+cBzpieNxDKsODozablOTzdwQDr2 uurjADJQAFXEiVGIATTO =EetP -----END PGP SIGNATURE----- --mvzZjokS1nTZS3h1-- From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailapp01.imgtec.com ([195.59.15.196]:28869 "EHLO imgpgp01.kl.imgtec.org" rhost-flags-OK-OK-OK-FAIL) by eddie.linux-mips.org with ESMTP id S23993420AbdCCXHTWw40W (ORCPT ); Sat, 4 Mar 2017 00:07:19 +0100 Date: Fri, 3 Mar 2017 23:07:13 +0000 From: James Hogan Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control to be used Message-ID: <20170303230712.GT996@jhogan-linux.le.imgtec.org> References: <1484164100-9805-1-git-send-email-jason.uy@broadcom.com> <1484164100-9805-2-git-send-email-jason.uy@broadcom.com> <1488394220.20145.68.camel@linux.intel.com> <20170303002129.GS996@jhogan-linux.le.imgtec.org> <1488547866.20145.74.camel@linux.intel.com> <3e45a0582dd91dab1a6e9bd6f4339e12@mail.gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mvzZjokS1nTZS3h1" Content-Disposition: inline In-Reply-To: <3e45a0582dd91dab1a6e9bd6f4339e12@mail.gmail.com> Return-Path: Sender: linux-mips-bounce@linux-mips.org Errors-to: linux-mips-bounce@linux-mips.org List-help: List-unsubscribe: List-software: Ecartis version 1.0.0 List-subscribe: List-owner: List-post: List-archive: 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@vger.kernel.org, LKML , bcm-kernel-feedback-list@broadcom.com, Linux MIPS Mailing List , David Daney , Russell King , linux-clk@vger.kernel.org, Viresh Kumar Message-ID: <20170303230713.LlC61Fd_vGi8YNtnzrlQW9OW0ZA71KpVZdBJKeOkEBA@z> --mvzZjokS1nTZS3h1 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi Jason, On Fri, Mar 03, 2017 at 09:43:55AM -0800, Jason Uy wrote: > James, >=20 > Can you verify that changing the code to the following fixes your problem? >=20 > 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 >=20 > Regards, > Jason >=20 > -----Original Message----- > From: Ray Jui [mailto:ray.jui@broadcom.com] > Sent: March-03-17 9:34 AM > To: Andy Shevchenko ; James Hogan > ; Heiko Stuebner > Cc: Jason Uy ; Greg Kroah-Hartman > ; Jiri Slaby ; Kefeng Wang > ; Noam Camus ; Heikki Kroge= rus > ; Wang Hongcheng ; > linux-serial@vger.kernel.org; LKML ; > bcm-kernel-feedback-list@broadcom.com; Linux MIPS Mailing List > ; David Daney ; Russell > King ; linux-clk@vger.kernel.org; Viresh Kumar > > Subject: Re: [PATCH v2 1/1] serial: 8250_dw: Allow hardware flow control = to > be used >=20 > Hi Andy/Jason, >=20 > 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 > >>>> 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: > >>>> [] uart_get_baud_rate+0xfc/0x1f0 > >>>> [] serial8250_do_set_termios+0xb0/0x440 > >>>> [] uart_set_options+0xe8/0x190 > >>>> [] serial8250_console_setup+0x84/0x158 > >>>> [] univ8250_console_setup+0x54/0x70 > >>>> [] register_console+0x1c8/0x418 > >>>> [] uart_add_one_port+0x434/0x4b0 > >>>> [] serial8250_register_8250_port+0x2d8/0x440 > >>>> [] 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=3Dn (Octeon doesn't select it) > >> 2) The CONFIG_HAVE_CLK=3Dn 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=3Dn implementation of clk_round_rate() returns 0 > >> 5) The CONFIG_HAVE_CLK=3Dn 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. >=20 > I agree. I think it should have been: >=20 > if (IS_ERR_OR_NULL(d->clk) || !old) > goto out; >=20 > 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 n= ot > well the clock framework handles it). >=20 > Thanks, >=20 > Ray >=20 > > > >> The CONFIG_HAVE_CLK=3Dn 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 > > --mvzZjokS1nTZS3h1 Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYufcZAAoJEGwLaZPeOHZ6ZxIQAIK+69uA+O+ZucfIIVEtqb5M TVTWaGCoGbTorVa7GI4RAJjfO/0J5QPZyVL8VZiAHI3kovr71VqpVx2dZRtC5ZA/ yABfv14l9GWX/Tvb91A01RssVlTG3pQ7QMURsW8RbqAbnjxuqCZ8IG/bVArw20Py ZYpsYmS4sfxjdQBbtfQVzh1kpEDfCAKL0dvrtzY2sAQ3a4AQptU+Z5sQLMaw+Jmy SUaMXKCM0Efhz6W8TdX9qhX5dm3N63JheO7uW3exDJjRe3fcej2ufSos0wK/xrwr QU+LDVtf8jp7xfFt28ml6ghFEFow2oaYH0GYakg5UvQBV5P2NzP680mznCpBv/fJ Wf2RyZ6YIjFMrYAmUCyyLHLLTFi6oO5qowcfbKYluB7Akr0UcKzrIuI4kGc03C49 LyDs4hyj9EBE063mcbuJSBmRh8+nLdiNE9FMh0C4EtTVRPyObR+R+O50wYzo/37r G0PPFBOmso0cUdqwmBFrhW0bpkogaaWRcWXfEjMcLSZYcncNAVhGbH3xnwcRthJn vsDMiseZ1EIxNQe/qcIxUPv1fytdz8APYo+vY+rDepxEXafh2/M0cye6nTDMXqDF TzseJAhra3G8C4xDH1+ikXyN0ExmUk/j5VC8+cBzpieNxDKsODozablOTzdwQDr2 uurjADJQAFXEiVGIATTO =EetP -----END PGP SIGNATURE----- --mvzZjokS1nTZS3h1--