linux-kernel - Re: [PATCH v3 1/2] usb: dwc2: host: Don't retry NAKed transactions right away

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: <2B3535C5ECE8B5419E3ECBE300772909026A0246C8@US01WEMBX2.internal.synopsys.com>
Date:   Wed, 6 Dec 2017 06:06:35 +0000
From:   John Youn <John.Youn@...opsys.com>
To:     Stefan Wahren <stefan.wahren@...e.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "John.Youn@...opsys.com" <John.Youn@...opsys.com>
CC:     Douglas Anderson <dianders@...omium.org>,
        "amstan@...omium.org" <amstan@...omium.org>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "johan@...nel.org" <johan@...nel.org>,
        "eric@...olt.net" <eric@...olt.net>,
        "mka@...omium.org" <mka@...omium.org>,
        "john.stultz@...aro.org" <john.stultz@...aro.org>,
        "linux-rpi-kernel@...ts.infradead.org" 
        <linux-rpi-kernel@...ts.infradead.org>,
        "jwerner@...omium.org" <jwerner@...omium.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/2] usb: dwc2: host: Don't retry NAKed transactions
 right away

On 12/05/2017 08:18 AM, Stefan Wahren wrote:
> Hi Felipe,
> Hi John,
>
>
> Am 30.10.2017 um 18:08 schrieb Douglas Anderson:
>> On rk3288-veyron devices on Chrome OS it was found that plugging in an
>> Arduino-based USB device could cause the system to lockup, especially
>> if the CPU Frequency was at one of the slower operating points (like
>> 100 MHz / 200 MHz).
>>
>> Upon tracing, I found that the following was happening:
>> * The USB device (full speed) was connected to a high speed hub and
>>    then to the rk3288.  Thus, we were dealing with split transactions,
>>    which is all handled in software on dwc2.
>> * Userspace was initiating a BULK IN transfer
>> * When we sent the SSPLIT (to start the split transaction), we got an
>>    ACK.  Good.  Then we issued the CSPLIT.
>> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>>    the interrupt handler) started to retry and sent another SSPLIT.
>> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>>    sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>>    handler.
>> * The handling of the interrupts was (because of the low CPU speed and
>>    the inefficiency of the dwc2 interrupt handler) was actually taking
>>    _longer_ than it took the other side to send the ACK/NAK.  Thus we
>>    were _always_ in the USB interrupt routine.
>> * The fact that USB interrupts were always going off was preventing
>>    other things from happening in the system.  This included preventing
>>    the system from being able to transition to a higher CPU frequency.
>>
>> As I understand it, there is no requirement to retry super quickly
>> after a NAK, we just have to retry sometime in the future.  Thus one
>> solution to the above is to just add a delay between getting a NAK and
>> retrying the transmission.  If this delay is sufficiently long to get
>> out of the interrupt routine then the rest of the system will be able
>> to make forward progress.  Even a 25 us delay would probably be
>> enough, but we'll be extra conservative and try to delay 1 ms (the
>> exact amount depends on HZ and the accuracy of the jiffy and how close
>> the current jiffy is to ticking, but could be as much as 20 ms or as
>> little as 1 ms).
>>
>> Presumably adding a delay like this could impact the USB throughput,
>> so we only add the delay with repeated NAKs.
>>
>> NOTE: Upon further testing of a pl2303 serial adapter, I found that
>> this fix may help with problems there.  Specifically I found that the
>> pl2303 serial adapters tend to respond with a NAK when they have
>> nothing to say and thus we end with this same sequence.
>>
>> Signed-off-by: Douglas Anderson <dianders@...omium.org>
>> Cc: stable@...r.kernel.org
>> Reviewed-by: Julius Werner <jwerner@...omium.org>
>> Tested-by: Stefan Wahren <stefan.wahren@...e.com>
>> ---
>>
>> Changes in v3:
>> - Add tested-by for Stefan Wahren
>> - Sent to Felipe Balbi as candiate to land this.
>> - Add Cc for stable (it's always been broken so go as far is as easy)
>>
>> Changes in v2:
>> - Address https://urldefense.proofpoint.com/v2/url?u=http-3A__crosreview.com_737520&d=DwICaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=U3o8uKoKhWme5_V9D-eeCkB11BFwt4KvWztBgdE9ZpA&m=Y_xpJ6Ks0XAK5_bQgmeQEvgKThZtPBQJ3cejNCGfEvM&s=olyPwyYvn_072esVwYxrCduKOKKJPUgc1YHX-CNhM1s&e= feedback
>>
>
> does it need a resend?
>

You can add my acked-by:

Acked-by: John Youn <johnyoun@...opsys.com>

Regards,
John