linux-kernel - Re: [PATCH] usb: dwc3: gadget: giveback request if start transfer fail

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] [day] [month] [year] [list]

Message-ID: <20140502161042.GO4631@saruman.home>
Date:	Fri, 2 May 2014 11:10:42 -0500
From:	Felipe Balbi <balbi@...com>
To:	Zhuang Jin Can <jin.can.zhuang@...el.com>
CC:	Felipe Balbi <balbi@...com>, <linux-usb@...r.kernel.org>,
	<linux-omap@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	David Cohen <david.a.cohen@...ux.intel.com>
Subject: Re: [PATCH] usb: dwc3: gadget: giveback request if start transfer
 fail

Hi,

On Sat, May 03, 2014 at 12:05:41AM -0400, Zhuang Jin Can wrote:
> On Thu, May 01, 2014 at 10:13:28AM -0500, Felipe Balbi wrote:
> > On Thu, May 01, 2014 at 04:44:52PM -0400, Zhuang Jin Can wrote:
> > > On Wed, Apr 30, 2014 at 02:58:29PM -0500, Felipe Balbi wrote:
> > > > On Thu, May 01, 2014 at 02:36:08AM -0400, Zhuang Jin Can wrote:
> > > > > At least we should giveback the current request to the
> > > > > gadget. Otherwise, the gadget will be stuck without knowing
> > > > > anything.
> > > > > 
> > > > > It was oberved that the failure can happen if the request is
> > > > > queued when the run/stop bit of controller is not set.
> > > > 
> > > > why is your gadget queueing any requests before calling ->udc_start() ?
> > > > 
> > > > A better question, what modification have you done to udc-core.c which
> > > > broke this ? udc-core *always* calls ->udc_start() by the time you load
> > > > a gadget driver so this case will *never* happen. Whatever modification
> > > > you did, broke this assumption and I will *not* accept this patch
> > > > because the bug is elsewhere and *not* in mainline kernel.
> > > > 
> > > It's found in Android using kernel 3.10.20. Android has its own
> > > usb_composite_driver usb/gadget/android.c (not in mainline), and it
> > 
> > so you found something on an old kernel using an out-of-tree gadget
> > driver.
> > 
> > > allows userspace to disconnect the pullup (i.e clear run/stop bit in dwc3)
> > > and remove the gadget functions like adb, mtp and then add new functions
> > > like rndis, acm. The problem is when you disconnect the pullup, a gadget
> > > maybe in the middle of queuing a request, and result in the "start
> > > transfer cmd failure". I think this is also a common issue for other
> > 
> > Android gadget needs to learn how to cope with that.
> > 
> Agree.
> 
> > > usb_composite_drivers too. Normally, if one of the gadget deactivate its
> > > own function, the pullup will be disconnected, other gadgets won't get
> > > notified until their requests are failed. So it makes dwc3 more robust
> > > to deal with these situations.
> > 
> > Right, but Android gadget can run on top of several other UDCs and you
> > want to have a single one of them cope with android's bug ?
> > 
> > You'd be better off getting google to accept a bugfix to the android
> > gadget, since that's where the problem lies.
> > 
> I agree. I'll try to push the fix to google.

alright, thanks

> It's really hard to fix the race condition (for me), as any gadget or
> /sys/class/udc/soft_connect can just disconnect the pullup anytime they
> want. The only thing I can do is giving back the request to the
> gadget if the condition happens.

even in that case, gadget driver's ->disconnect() will be called and
that should be enough to tell the gadget driver 'hey, don't queue
anything right now because you're not talking to any host'.

cheers

-- 
balbi

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