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: <26c82c13-d047-853e-12af-e916596c5c52@synopsys.com>
Date:   Wed, 23 Mar 2022 01:31:00 +0000
From:   Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To:     Wesley Cheng <wcheng@...eaurora.org>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "quic_jackp@...cinc.com" <quic_jackp@...cinc.com>,
        Thinh Nguyen <Thinh.Nguyen@...opsys.com>
Subject: Re: [RFC PATCH v2 1/3] usb: dwc3: Flush pending SETUP data during
 stop active xfers

Hi Wesley,

Wesley Cheng wrote:
> Hi Thinh,
> 
> On 2/15/2022 4:08 PM, Wesley Cheng wrote:
>> While running the pullup disable sequence, if there are pending SETUP
>> transfers stored in the controller, then the ENDTRANSFER commands on non
>> control eps will fail w/ ETIMEDOUT.  As a suggestion from SNPS, in order
>> to drain potentially cached SETUP packets, SW needs to issue a
>> STARTTRANSFER command.  After issuing the STARTTRANSFER, and retrying the
>> ENDTRANSFER, the command should succeed.  Else, if the endpoints are not
>> properly stopped, the controller halt sequence will fail as well.
>>
>> One limitation is that the current logic will drop the SETUP data
>> being received (ie dropping the SETUP packet), however, it should be
>> acceptable in the pullup disable case, as the device is eventually
>> going to disconnect from the host.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>> ---
> Just wondering if you had any inputs for this particular change?  I
> think on the dequeue path discussion you had some concerns with parts of
> this change?  I think the difficult part for the pullup disable path is
> that we have this API running w/ interrupts disabled, so the EP0 state
> won't be able to advance compared to the dequeue case.

This doesn't sound right. The pullup disable (or device initiated
disconnect) should wait for the EP0 state to be EP0_SETUP_PHASE before
proceeding, which it does.

> 
> Ideally, if there is a way to ensure that we avoid reading further setup
> packets, that would be nice, but from our discussions with Synopsys,
> this was not possible. (controller is always armed and ready to ACK
> setup tokens)
> 
> I did evaluate keeping IRQs enabled and periodically releasing/attaining
> the lock between the stop active xfer calls, but that opened another can
> of worms.  If you think this is the approach we should take, I can take
> a look at this implementation further.
> 

This patch doesn't look right to me. The change I suggested before
should address this (I believe Greg already picked it up). What other
problem do you see, I'm not clear what's the problem here. One potential
problem that I can see is that currently dwc3 driver doesn't wait for
active endpoints to complete/end before clearing the run_stop bit on
device initiated disconnect, but I'm not sure if that's what you're seeing.

Please help clarify further. If there's trace points log, that'd help.

Thanks,
Thinh

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ