Handling of bounce buffers by rh_call_control

Handling of bounce buffers by rh_call_control

[Richard Curnow]

The following patch

===== drivers/usb/hcd.c 1.10 vs edited =====
--- 1.10/drivers/usb/hcd.c      Mon Mar 31 14:22:42 2003
+++ edited/drivers/usb/hcd.c    Wed Dec 17 11:26:53 2003
@@ -323,7 +323,7 @@
        struct usb_ctrlrequest *cmd = (struct usb_ctrlrequest *) urb->setup_packet;
        u16             typeReq, wValue, wIndex, wLength;
        const u8        *bufp = 0;
-       u8              *ubuf = urb->transfer_buffer;
+       u8              *ubuf = (u8 *) urb->transfer_dma;
        int             len = 0;
 
        typeReq  = (cmd->bRequestType << 8) | cmd->bRequest;

seems to be needed to make the following code later in the function work

	if (bufp) {
		if (urb->transfer_buffer_length < len)
			len = urb->transfer_buffer_length;
		urb->actual_length = len;
		// always USB_DIR_IN, toward host
		memcpy (ubuf, bufp, len);
	}

in the case where bounce buffers are being used to implement PCI DMA
operations.  Without the patch, the subsequent pci_unmap_single copies
the contents of the bounce buffer over the top of urb->transfer_buffer,
destroying what the memcpy() put there.

My USB knowledge is pretty limited, so I've no idea whether this patch
adversely affects anything else in rh_call_control.

The patch is against 2.4.23-pre-something, but I see the code's the same
in 2.6.

-- 
Richard \\\ SH-4/SH-5 Core & Debug Architect
Curnow  \\\         SuperH (UK) Ltd, Bristol

Re: Handling of bounce buffers by rh_call_control

[David Brownell]

Hi,

Richard Curnow wrote:
> The following patch
> 
> ===== drivers/usb/hcd.c 1.10 vs edited =====
> --- 1.10/drivers/usb/hcd.c      Mon Mar 31 14:22:42 2003
> +++ edited/drivers/usb/hcd.c    Wed Dec 17 11:26:53 2003
> @@ -323,7 +323,7 @@
>         struct usb_ctrlrequest *cmd = (struct usb_ctrlrequest *) urb->setup_packet;
>         u16             typeReq, wValue, wIndex, wLength;
>         const u8        *bufp = 0;
> -       u8              *ubuf = urb->transfer_buffer;
> +       u8              *ubuf = (u8 *) urb->transfer_dma;
>         int             len = 0;
>  
>         typeReq  = (cmd->bRequestType << 8) | cmd->bRequest;
> 
> seems to be needed to make the following code later in the function work
> 
> 	if (bufp) {
> 		if (urb->transfer_buffer_length < len)
> 			len = urb->transfer_buffer_length;
> 		urb->actual_length = len;
> 		// always USB_DIR_IN, toward host
> 		memcpy (ubuf, bufp, len);

Which is why that particular patch is wrong:  "ubuf" is a dma address,
not expected to work for memcpy().


> 	}
> 
> in the case where bounce buffers are being used to implement PCI DMA
> operations.  Without the patch, the subsequent pci_unmap_single copies
> the contents of the bounce buffer over the top of urb->transfer_buffer,
> destroying what the memcpy() put there.

A similar bug was fixed in 2.6 by a patch from Russell King.  I think
the 2.4 version has lingered since it only affect EHCI, and most folk
using EHCI won't use bounce buffers.

Could you look at that patch (bk revtool or bkweb will show it, early
September 2002) and provide a 2.4 version?


> My USB knowledge is pretty limited, so I've no idea whether this patch
> adversely affects anything else in rh_call_control.
> 
> The patch is against 2.4.23-pre-something, but I see the code's the same
> in 2.6.

What changed in 2.6 was that hcd_submit_urb() has a special case for
the root hub.  That's always PIO, so the code now bypasses all the
DMA mapping code when an URB is destined for a root hub.  The original
code accidentally assumed no bounce buffering would go on.

- Dave

Re: Handling of bounce buffers by rh_call_control

[bill davidsen]

In article <3FE08470.5040801@pacbell.net>,
David Brownell  <david-b@pacbell.net> wrote:
| Hi,
| 
| Richard Curnow wrote:
| > The following patch
| > 
| > ===== drivers/usb/hcd.c 1.10 vs edited =====
| > --- 1.10/drivers/usb/hcd.c      Mon Mar 31 14:22:42 2003
| > +++ edited/drivers/usb/hcd.c    Wed Dec 17 11:26:53 2003
| > @@ -323,7 +323,7 @@
| >         struct usb_ctrlrequest *cmd = (struct usb_ctrlrequest *) urb->setup_packet;
| >         u16             typeReq, wValue, wIndex, wLength;
| >         const u8        *bufp = 0;
| > -       u8              *ubuf = urb->transfer_buffer;
| > +       u8              *ubuf = (u8 *) urb->transfer_dma;
| >         int             len = 0;
| >  
| >         typeReq  = (cmd->bRequestType << 8) | cmd->bRequest;
| > 
| > seems to be needed to make the following code later in the function work
| > 
| > 	if (bufp) {
| > 		if (urb->transfer_buffer_length < len)
| > 			len = urb->transfer_buffer_length;
| > 		urb->actual_length = len;
| > 		// always USB_DIR_IN, toward host
| > 		memcpy (ubuf, bufp, len);
| 
| Which is why that particular patch is wrong:  "ubuf" is a dma address,
| not expected to work for memcpy().

But the existing code most certainly does use it with memcpy. I'm
looking at test11-mm1 source, but the last memcpy line he noted is most
definitely in the existing source.

Or did I misunderstand what you meant by "not expected to work for
memcpy()?" It may be that the code around the memcpy is wrong and should
be using ubuf, and that no diddling with fix it, but someone clearly DID
expect it to work ;-)
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: Handling of bounce buffers by rh_call_control

[David Brownell]

> | > -       u8              *ubuf = urb->transfer_buffer;
> | > +       u8              *ubuf = (u8 *) urb->transfer_dma;
> | >         int             len = 0;
> | >  
> | > ...
> | > 		memcpy (ubuf, bufp, len);
> | 
> | Which is why that particular patch is wrong:  "ubuf" is a dma address,
> | not expected to work for memcpy().
> 
> But the existing code most certainly does use it with memcpy. I'm
> looking at test11-mm1 source, but the last memcpy line he noted is most
> definitely in the existing source.

As in, "ubuf is NOW a dma address ... it wasn't one before that patch,
it was a regular kernel mappped address.  Otherwise memcpy() would
never have worked.  Changing it would break more typical kernels, with
no need for bounce buffering.

The right fix is just to bypass the DMA mapping for root hubs,
as in that 2.6 patch from Russell that I mentioned.  It's the
unmap that was causing trouble, since it clobbered the data
which memcpy() had just stored into that buffer.

- Dave

Re: Handling of bounce buffers by rh_call_control

[Richard Curnow]

Hi David,

* David Brownell <david-b@pacbell.net> [2003-12-17]:
> >
> >	if (bufp) {
> >		if (urb->transfer_buffer_length < len)
> >			len = urb->transfer_buffer_length;
> >		urb->actual_length = len;
> >		// always USB_DIR_IN, toward host
> >		memcpy (ubuf, bufp, len);
> 
> Which is why that particular patch is wrong:  "ubuf" is a dma address,
> not expected to work for memcpy().

Sure, I didn't expect the patch was general but it was enough to make
things work on the platform I'm currently trying to port to, and it
illustrated the problem I was hitting.

> >in the case where bounce buffers are being used to implement PCI DMA
> >operations.  Without the patch, the subsequent pci_unmap_single copies
> >the contents of the bounce buffer over the top of urb->transfer_buffer,
> >destroying what the memcpy() put there.
> 
> A similar bug was fixed in 2.6 by a patch from Russell King.  I think
> the 2.4 version has lingered since it only affect EHCI, and most folk
> using EHCI won't use bounce buffers.

I just happened to have EHCI compiled in and put a USB 2 card into the
board.  The problem meant that the EHCI initialisation wouldn't even
complete, so I waded in to find out why.

IIRC, if I plug a USB2 device into a USB2 card but don't have the EHCI
driver active, the device is just ignored, rather than falling back to
using USB1.1 through OHCI?  We certainly have some USB2 devices we'd
like to use, even if the USB2 bandwidth might be throttled back by the
bounce buffering overhead.

> Could you look at that patch (bk revtool or bkweb will show it, early
> September 2002) and provide a 2.4 version?

I'll dig that out, if it's simple enough to backport I'll see if I can
take a stab at it.

> What changed in 2.6 was that hcd_submit_urb() has a special case for
> the root hub.  That's always PIO, so the code now bypasses all the
> DMA mapping code when an URB is destined for a root hub.

That seems like the right approach.

Cheers
Richard

-- 
Richard \\\ SH-4/SH-5 Core & Debug Architect
Curnow  \\\         SuperH (UK) Ltd, Bristol

iproute2 and 2.6.0 kernel

[Remus]

Hi folks,

I have a linux box with three NICs (two for external ISP, and one local).
Today I tried to use 2.6.0 kernel and something is wrong, because iproute2
does not work corretly.
No routed packets go via second ISP NIC which I use with iproute rules. With
2.4.22 kernel I have no problems at all with packet routing.

I compiled 2.6.0 kernel myself, maybe I missed something in .config file?

Thanks

Remus

Re: iproute2 and 2.6.0 kernel

[bill davidsen]

In article <0d6401c3c576$c6889b00$6e69690a@RIMAS>,
Remus <rmocius@auste.elnet.lt> wrote:
| Hi folks,
| 
| I have a linux box with three NICs (two for external ISP, and one local).
| Today I tried to use 2.6.0 kernel and something is wrong, because iproute2
| does not work corretly.
| No routed packets go via second ISP NIC which I use with iproute rules. With
| 2.4.22 kernel I have no problems at all with packet routing.
| 
| I compiled 2.6.0 kernel myself, maybe I missed something in .config file?

Using iproute2 and identifying the packets with the MARK target? I am
seeing that as well, but I wasn't going to post until I had nice configs
in files, packet traces, and the like. So for now take this as a "me too."
-- 
bill davidsen <davidsen@tmr.com>
  CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.

Re: iproute2 and 2.6.0 kernel

[Remus]

Hi,

Specialy a problem is with "ip" command.

Any ideas guys?

Thanks

Remus


> Hi folks,
>
> I have a linux box with three NICs (two for external ISP, and one local).
> Today I tried to use 2.6.0 kernel and something is wrong, because iproute2
> does not work corretly.
> No routed packets go via second ISP NIC which I use with iproute rules.
With
> 2.4.22 kernel I have no problems at all with packet routing.
>
> I compiled 2.6.0 kernel myself, maybe I missed something in .config file?
>
> Thanks
>
> Remus
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: Handling of bounce buffers by rh_call_control

[David Brownell]

Richard Curnow wrote:
> 
> IIRC, if I plug a USB2 device into a USB2 card but don't have the EHCI
> driver active, the device is just ignored, rather than falling back to
> using USB1.1 through OHCI?  We certainly have some USB2 devices we'd
> like to use, even if the USB2 bandwidth might be throttled back by the
> bounce buffering overhead.

Yes, high speed USB transfers need the EHCI driver.
If that driver isn't running, then OHCI (or UHCI)
should kick in.  (Assuming that driver is running!)

Most high speed storage seems to work with the current
EHCI code, although some hardware acts unhappy when
Linux talks to it faster than Windows does.  That's
more of an issue on 2.6 than on 2.4 though.

- Dave