From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751560Ab1HKSFO (ORCPT ); Thu, 11 Aug 2011 14:05:14 -0400 Received: from mga09.intel.com ([134.134.136.24]:57422 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751013Ab1HKSFL (ORCPT ); Thu, 11 Aug 2011 14:05:11 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.67,352,1309762800"; d="scan'208";a="37603659" Date: Thu, 11 Aug 2011 11:05:07 -0700 From: Sarah Sharp To: Daniel Mack Cc: Alan Stern , Florian Mickler , Oliver Neukum , linux-usb@vger.kernel.org, alsa-devel@alsa-project.org, Takashi Iwai , Clemens Ladisch , pedrib@gmail.com, William Light , Greg KH , linux-kernel@vger.kernel.org, Robert Hancock Subject: Re: Allocating buffers for USB transfers (again) Message-ID: <20110811180507.GC5884@xanatos> References: <4E42A4AE.4020008@gmail.com> <20110810231552.GA7194@xanatos> <20110811164539.GB5884@xanatos> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 11, 2011 at 07:27:15PM +0200, Daniel Mack wrote: > On Thu, Aug 11, 2011 at 6:45 PM, Sarah Sharp > wrote: > > On Thu, Aug 11, 2011 at 02:57:41AM +0200, Daniel Mack wrote: > >> On Thu, Aug 11, 2011 at 1:15 AM, Sarah Sharp > >> wrote: > >> > On Wed, Aug 10, 2011 at 05:33:02PM +0200, Daniel Mack wrote: > >> >> On 08/10/2011 04:32 PM, Alan Stern wrote: > >> >> >Looking at the driver's current code, it appears that your patch > >> >> >does not fix the bug properly.  Using discontiguous regions in the > >> >> >transfer buffer is perfectly okay.  The real problem is later on, > >> >> >where you do: > >> >> > > >> >> >if (send_it) { out->number_of_packets = FRAMES_PER_URB; > >> >> > > >> >> >This should be > >> >> > > >> >> >out->number_of_packets = outframe; > >> >> > > >> >> >The way it is now, the USB stack will try to use data from all the > >> >> >frame descriptors, and the last few will be stale because the loop > >> >> >doesn't set them. > >> >> > >> >> That's actually true, even though it doesn't seem to cause any trouble. > >> >> I tested everything here of course, and the output URBs return back from > >> >> the USB stack with their length fields zeroed out, which then > >> >> causes the stack to send packets with zero-length fields at the end. > >> > > >> > Actually, it causes system hangs when the driver is loaded on a device > >> > attached to a USB 3.0 port, as Alan Stern pointed out: > >> > > >> > https://bugzilla.kernel.org/show_bug.cgi?id=40702 > >> > >> Yes, I've noticed this. > >> > >> > Please don't submit zero-length transfers.  The xHCI driver just isn't > >> > able to handle it.  Arguably, it probably should have just rejected your > >> > URB when it found a zero length buffer, so I'll probably be submitting a > >> > patch to fix that. > >> > >> According to the spec, sending zero-length frames should be fine, no? > >> Is there any particular reason why XCHI can't handle this while EHCI > >> can? And does my patch fix the driver for XHCI? > > > > Ok, yes, you're correct that the xHCI spec allows the transfer length to > > be set to zero.  In the case where the frame buffer is zero-length, is > > the buffer pointer still valid?  It's not clear from the spec whether it > > needs to be. > > Well, the buffer pointer is set in the URB, not in its individual iso > subframes which just denotes them via the offset field. So yes, it is > valid in my case. But it doesn't matter anymore, as the code which > does that is now gone :) Do you mean it was removed with this patch: http://git.kernel.org/?p=linux/kernel/git/tiwai/sound-2.6.git;a=commitdiff;h=15439b Because according to Matej, he applied that patch, plus my patch to reject zero-length buffers[1], and he saw debugging that indicated he *did* see zero-length buffers. Is there any chance your driver might submit a zero-length buffer in the middle of the isochronous URB transfer array? Sarah Sharp [1] http://git.kernel.org/?p=linux/kernel/git/sarah/xhci.git;a=commitdiff;h=e70d79bda63050729fcef654167d22eb59380aa6