VIMAGE hang, USB panic

[Hans Petter Selasky]

On Sunday 12 September 2010 20:22:14 Marius Strobl wrote:
> On Mon, Jul 26, 2010 at 08:59:20AM +0200, Hans Petter Selasky wrote:
> > On Sunday 25 July 2010 16:32:52 Nathaniel W Filardo wrote:
> > > Speaking of USB hotplug, attaching a micro 4-port hub, I get this (both
> > > with and without VIMAGE):
> > > 
> > > ugen0.2: <vendor 0x05e3> at usbus0
> > > uhub1: <vendor 0x05e3 USB2.0 Hub, class 9/0, rev 2.00/9.01, addr 2> on
> > > usbus0
> > > panic: iommu_dvmamap_create: bogus preallocation size , nsegments = 2,
> > > maxpre = 2, maxsize = 1
> > > cpuid = 0
> > 
> > > KDB: stack backtrace:
> > Hi,
> > 
> > Looks like the following check fails. The allocation size USB requests is
> > 1- byte. For the check to not fail, we need to request more bytes then
> > the max- segments parameter specified.
> 
> Or decrease the nsegments parameter. I think that the sanity check is
> right and the parameters supplied are bogus as 1 byte cannot be split
> across 2 segments.
> 
> >         maxpre = imin(dt->dt_nsegments, IOMMU_MAX_PRE_SEG);
> >         presz = dt->dt_maxsize / maxpre;
> >         KASSERT(presz != 0, ("%s: bogus preallocation size , nsegments =
> >         %d, "
> >         
> >             "maxpre = %d, maxsize = %lu", __func__, dt->dt_nsegments,
> >             maxpre, dt->dt_maxsize));
> > 
> > Does not look like a problem in the USB stack.
> 
> I think it is as the maxsize and nsegments parameters supplied in this
> case make no sense. Moreover, maxsegsz should be <= maxsize, thus I'd
> like to commit the following change:

Hi,

Yes, the values supplied are not too tight. The patch you've made to 
usb_busdma.c seems OK to me.

> 
> Index: usb_busdma.c
> ===================================================================
> --- usb_busdma.c	(revision 212478)
> +++ usb_busdma.c	(working copy)
> @@ -366,9 +366,9 @@ usb_dma_tag_create(struct usb_dma_tag *udt,
>  	     /* filter    */ NULL,
>  	     /* filterarg */ NULL,
>  	     /* maxsize   */ size,
> -	     /* nsegments */ (align == 1) ?
> +	     /* nsegments */ (align == 1 && size > 1) ?
>  	    (2 + (size / USB_PAGE_SIZE)) : 1,
> -	     /* maxsegsz  */ (align == 1) ?
> +	     /* maxsegsz  */ (align == 1 && size > USB_PAGE_SIZE) ?
>  	    USB_PAGE_SIZE : size,
>  	     /* flags     */ BUS_DMA_KEEP_PG_OFFSET,
>  	     /* lockfn    */ &usb_dma_lock_cb,
> 
> Why is this using one more segment than splitting size in USB_PAGE_SIZE
> sized segments btw? > If this isn't actually needed then replacing the 2
> with a 1 would be the more preferable fix IMO.

We are using 2 because a virtual memory load operation of less than 
USB_PAGE_SIZE, can be split accross two pages at maximum.

> 
> However, specifying such small allocation sizes together with
> BUS_SPACE_UNRESTRICTED as nsegments, which unlike what usb(4)
> currently does the I'd would consider as valid combinations,
> will also trigger this assertion, so I'll remove it.

It is usually not allocation that are of this size, but rather DMA load 
mappings.

--HPS