linux-kernel - Re: [PATCH 2/3] usb: dwc3: ulpi: Replace CPU-based busyloop with Protocol-based one

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87h7qgc9hg.fsf@kernel.org>
Date:   Tue, 27 Oct 2020 11:18:51 +0200
From:   Felipe Balbi <balbi@...nel.org>
To:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Felipe Balbi <felipe.balbi@...ux.intel.com>,
        David Cohen <david.a.cohen@...ux.intel.com>,
        Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Serge Semin <Sergey.Semin@...kalelectronics.ru>,
        Serge Semin <fancer.lancer@...il.com>,
        Alexey Malahov <Alexey.Malahov@...kalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@...kalelectronics.ru>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        Felipe Balbi <balbi@...com>
Subject: Re: [PATCH 2/3] usb: dwc3: ulpi: Replace CPU-based busyloop with
 Protocol-based one


Hi,

Serge Semin <Sergey.Semin@...kalelectronics.ru> writes:

> Originally the procedure of the ULPI transaction finish detection has been
> developed as a simple busy-loop with just decrementing counter and no
> delays. It's wrong since on different systems the loop will take a
> different time to complete. So if the system bus and CPU are fast enough
> to overtake the ULPI bus and the companion PHY reaction, then we'll get to
> take a false timeout error. Fix this by converting the busy-loop procedure
> to take the standard bus speed, address value and the registers access
> mode into account for the busy-loop delay calculation.
>
> Here is the way the fix works. It's known that the ULPI bus is clocked
> with 60MHz signal. In accordance with [1] the ULPI bus protocol is created
> so to spend 5 and 6 clock periods for immediate register write and read
> operations respectively, and 6 and 7 clock periods - for the extended
> register writes and reads. Based on that we can easily pre-calculate the
> time which will be needed for the controller to perform a requested IO
> operation. Note we'll still preserve the attempts counter in case if the
> DWC USB3 controller has got some internals delays.
>
> [1] UTMI+ Low Pin Interface (ULPI) Specification, Revision 1.1,
>     October 20, 2004, pp. 30 - 36.
>
> Fixes: 88bc9d194ff6 ("usb: dwc3: add ULPI interface support")
> Signed-off-by: Serge Semin <Sergey.Semin@...kalelectronics.ru>
> ---
>  drivers/usb/dwc3/ulpi.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/ulpi.c b/drivers/usb/dwc3/ulpi.c
> index 20f5d9aba317..0dbc826355a5 100644
> --- a/drivers/usb/dwc3/ulpi.c
> +++ b/drivers/usb/dwc3/ulpi.c
> @@ -7,6 +7,8 @@
>   * Author: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
>   */
>  
> +#include <linux/delay.h>
> +#include <linux/time64.h>
>  #include <linux/ulpi/regs.h>
>  
>  #include "core.h"
> @@ -17,12 +19,22 @@
>  		DWC3_GUSB2PHYACC_ADDR(ULPI_ACCESS_EXTENDED) | \
>  		DWC3_GUSB2PHYACC_EXTEND_ADDR(a) : DWC3_GUSB2PHYACC_ADDR(a))
>  
> -static int dwc3_ulpi_busyloop(struct dwc3 *dwc)
> +#define DWC3_ULPI_BASE_DELAY	DIV_ROUND_UP(NSEC_PER_SEC, 60000000L)
> +
> +static int dwc3_ulpi_busyloop(struct dwc3 *dwc, u8 addr, bool read)
>  {
> +	unsigned long ns = 5L * DWC3_ULPI_BASE_DELAY;
>  	unsigned count = 1000;
>  	u32 reg;
>  
> +	if (addr >= ULPI_EXT_VENDOR_SPECIFIC)
> +		ns += DWC3_ULPI_BASE_DELAY;
> +
> +	if (read)
> +		ns += DWC3_ULPI_BASE_DELAY;
> +
>  	while (count--) {
> +		ndelay(ns);

could we allow for a sleep here instead of a delay? Also, I wonder if
you need to make this so complex or should we just take the larger
access time of 7 clock cycles.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (858 bytes)