Page 1 of 2

Linux kernel patch for CDC problems

Posted: Mon Dec 10, 2007 9:32 am
by floe
Hello everybody,

I've had some problems getting the AVRCDC example program to run under Linux 2.6, as all of my machines have a UHCI controller which is known to cause some problems (it follows the USB standard very strictly and doesn't allow low-speed bulk transfers). After some digging in the LKML, I've put together a small patch which has been floating around in pieces for a while now. For reference, I'm posting it here because it's where most people will look first.

Yours, Florian

Code: Select all

--- old/drivers/usb/host/uhci-q.c       2007-12-10 08:22:18.000000000 +0100
+++ new/drivers/usb/host/uhci-q.c       2007-12-10 08:06:28.000000000 +0100
@@ -1046,14 +1046,14 @@
        int ret;
 
        /* Can't have low-speed bulk transfers */
-       if (urb->dev->speed == USB_SPEED_LOW)
-               return -EINVAL;
+       /*if (urb->dev->speed == USB_SPEED_LOW)
+               return -EINVAL;*/
 
        if (qh->state != QH_STATE_ACTIVE)
-               qh->skel = SKEL_BULK;
+               qh->skel = (urb->dev->speed == USB_SPEED_LOW ? SKEL_LS_CONTROL : SKEL_BULK);
        ret = uhci_submit_common(uhci, urb, qh);
-       if (ret == 0)
+       if (ret == 0 && urb->dev->speed != USB_SPEED_LOW)
                uhci_add_fsbr(uhci, urb);
        return ret;
 }


Posted: Wed Dec 12, 2007 1:06 am
by bbb
Hi!

I think I saw a patch which changed the BULK endpoints into INTERRUPT on low speed devices. And I think it should have gone to the official tree somehow...

Posted: Thu Dec 13, 2007 10:35 am
by floe
Yes, you are right: LKML post

This is included starting with 2.6.23, AFAICT. However, I think that the CDC-ACM driver needs a patch to support this, too, and I couldn't find anything. Has anyone tried the AVRCDC with a stock 2.6.23 kernel?

Yours, Florian

Posted: Thu Jan 31, 2008 4:13 pm
by texmex
floe wrote:Yes, you are right: LKML post

This is included starting with 2.6.23, AFAICT. However, I think that the CDC-ACM driver needs a patch to support this, too, and I couldn't find anything. Has anyone tried the AVRCDC with a stock 2.6.23 kernel?

Yours, Florian


For me it works on a stock 2.6.22.1 Kernel and it doesn't on 2.6.24.

2.6.22.1:
usb 2-2: new low speed USB device using uhci_hcd and address 3
usb 2-2: configuration #1 chosen from 1 choice
cdc_acm 2-2:1.0: ttyACM0: USB ACM device
usbcore: registered new interface driver cdc_acm
drivers/usb/class/cdc-acm.c: v0.25:USB Abstract Control Model driver for USB modems and ISDN adapters

2.6.24:
usb 1-2: new low speed USB device using uhci_hcd and address 3
usb 1-2: config 1 interface 1 altsetting 0 endpoint 0x1 is Bulk; changing to Interrupt
usb 1-2: config 1 interface 1 altsetting 0 endpoint 0x81 is Bulk; changing to Interrupt
usb 1-2: configuration #1 chosen from 1 choice
cdc_acm 1-2:1.0: ttyACM0: USB ACM device
usbcore: registered new interface driver cdc_acm
drivers/usb/class/cdc-acm.c: v0.25:USB Abstract Control Model driver for USB modems and ISDN adapters

Posted: Sat Feb 02, 2008 6:04 pm
by texmex
As mentioned in my previous post the CDC-ACM driver does not work with kernel 2.6.24 (presumably since 2.6.23).

This is caused by a change in the usb driver which changes the bulk endpoint into an interrupt endpoint.

I'm really not in all this usb stuff and i don't know what i'm talking about.
But disabling this endpoint change makes my AVR-CDC-ACM Device working again.

This is what i've done to drivers/usb/core/config.c:

Code: Select all

Index: config.c
===================================================================
--- config.c    (Revision 22)
+++ config.c    (Arbeitskopie)
@@ -136,13 +136,17 @@
         */
        if (to_usb_device(ddev)->speed == USB_SPEED_LOW &&
                        usb_endpoint_xfer_bulk(d)) {
-               dev_warn(ddev, "config %d interface %d altsetting %d "
-                   "endpoint 0x%X is Bulk; changing to Interrupt\n",
-                   cfgno, inum, asnum, d->bEndpointAddress);
-               endpoint->desc.bmAttributes = USB_ENDPOINT_XFER_INT;
-               endpoint->desc.bInterval = 1;
-               if (le16_to_cpu(endpoint->desc.wMaxPacketSize) > 8)
-                       endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
+          dev_warn(ddev, "config %d interface %d altsetting %d "
+               "endpoint 0x%X is Bulk; this violates USB spec for "
+               "low speed devices.\n",
+               cfgno, inum, asnum, d->bEndpointAddress);
+//             dev_warn(ddev, "config %d interface %d altsetting %d "
+//                 "endpoint 0x%X is Bulk; changing to Interrupt\n",
+//                 cfgno, inum, asnum, d->bEndpointAddress);
+//             endpoint->desc.bmAttributes = USB_ENDPOINT_XFER_INT;
+//             endpoint->desc.bInterval = 1;
+//             if (le16_to_cpu(endpoint->desc.wMaxPacketSize) > 8)
+//                     endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
        }
 
        /* Skip over any Class Specific or Vendor Specific descriptors;

Posted: Sun Feb 03, 2008 6:25 pm
by christian
I don't know what this recent change does to break CDC ACM devices on low speed USB, but I can tell you that interrupt- and bulk-endpoints are equivalent except that bulk endpoints are polled "as fast as possible" while interrupt endpoints are polled with a given rate.

Posted: Wed Feb 13, 2008 4:39 pm
by Guest
christian wrote:I don't know what this recent change does to break CDC ACM devices on low speed USB, but I can tell you that interrupt- and bulk-endpoints are equivalent except that bulk endpoints are polled "as fast as possible" while interrupt endpoints are polled with a given rate.


"polled with a given rate" doesn't sound like what i know as interrupt controlled :-).
So do you think the necessary changes at the USB Client should be quite simple to support the interrupt endpoints instead of bulk endpoints?
Obviously it doesn't work out of the box.

Kind regards,
Klaus

Posted: Thu Feb 14, 2008 12:19 am
by christian
USB is a host driven serial bus. Real interrupts are therefore not possible, all interrupt-like semantics are implemented with polling.

I don't understand why this change should break anything. There is no difference between interrupt- and bulk-endpoints in AVR-USB. Only the timing of the API calls should differ. This may trigger a bug.

Posted: Tue Mar 25, 2008 8:07 pm
by jschmelzer
Hello everybody!

I am facing the same problem with Kernel 2.6 and an Intel chipset. I want to use my Asus EeePC (Xandros with Kernel 2.6.21.4) to communicate with the "Experimentierboard " from Ulrich Radig. You can find the project under this Link:

http://www.ulrichradig.de/home/index.ph ... ntierboard

There is a Tiny2313 used with the CDC-2313 firmware.

The circuit is running fine with Windows XP and also with a different PC with nVidia MCP55 chipset running gOS. I tried a third PC with gOS and Intel chipset. There, I was able to duplicate the issue again.

As I am just a beginner with Linux AND AVR C-programming, is there an easy way to solve the problem, or do I need to patch and compile my own Kernel? I would like to stay with my current installation on the EeePC. It was quite a tough job for me to compile and install the AVR-GCC toolchain...

dmesg is reporting this:

usb 2-2: new low speed USB device using uhci_hcd and address 12
usb 2-2: configuration #1 chosen from 1 choice
cdc_acm 2-2:1.0: ttyACM0: USB ACM device

Anybody has an idea for me to get ahead?

Thanks in advance.
BR,
Joerg

Posted: Wed Mar 26, 2008 8:18 pm
by jschmelzer
OK, I decided to try to fix my problem by building a kernel with modified USB driver. When I understood the posting from Florian right, the code should be looking like this (removed the lines with a "-" and add the lines with "+"):

Code: Select all

static int uhci_submit_bulk(struct uhci_hcd *uhci, struct urb *urb,
      struct uhci_qh *qh)
{
   int ret;

   /* Can't have low-speed bulk transfers */
   /*if (urb->dev->speed == USB_SPEED_LOW)
      return -EINVAL;*/

   if (qh->state != QH_STATE_ACTIVE)
      qh->skel = (urb->dev->speed == USB_SPEED_LOW ? SKEL_LS_CONTROL : SKEL_BULK);
   ret = uhci_submit_common(uhci, urb, qh);
   if (ret == 0 && urb->dev->speed != USB_SPEED_LOW)
      uhci_add_fsbr(uhci, urb);
   return ret;
}


I downloaded the sources and modified the USB driver. Now I want to compile a new kernel out of that sources. Could somebody just confirm, that I am on the right way?

BR,
Joerg

Posted: Sun Apr 06, 2008 10:00 am
by jschmelzer
Finally, I managed to compile the kernel with the modified USB-driver (like in my previous post). Data is now received by my Linux PC. But there is a high failure rate. 70% of the data is corrupted. I double checked already with a XP PC, that the controller board is OK. What can I do now?

Posted: Mon Apr 14, 2008 5:04 pm
by texmex
Hello Christian, floe, Guest et al.

After several weeks i investigated the AVR-CDC problem in conjunction
with the linux kernel again. I think there is no good news but i have to
admit again that i don't have any clue of all this usb stuff.
So everything i write is a suggestion. But since there are no better
explanations i think it's justified and i try to abstract my cognitions.

Several times was noticed that CDC does not work with low-speed USB
transfer according to the USB spec. I'm not sure whether this is true
directly.
As far as i have seen there are no bulk-endpoints in the spec for
low-speed USB devices but CDC expects bulk-endpoints according to
the spec. This leads indeed into the situation that CDC should not
work on low-speed USB.

The linux kernel reflects this exactly. However in a few steps:

Up to Kernel Version 2.6.23 there was a small code snippet in
drivers/usb/host/uhci-q.c:

Code: Select all

     if (urb->dev->speed == USB_SPEED_LOW)
        return -EINVAL;

it is executed for bulk endpoints and therefore they are
ignored at low speed.

Since this is at quite low-level in the uhci code it leads to the
interesting situation that the AVR-CDC Device works nevertheless if
connected to an High-Speed USB2.0 Hub. Because on Host side it is
handled by ehci driver then and the code above will never be executed.

It's easy to use the already mentioned patch for
"drivers/usb/host/uhci-q.c":

Code: Select all

1045,1046c1045,1046
<       if (urb->dev->speed == USB_SPEED_LOW)
<               return -EINVAL;
---
> //    if (urb->dev->speed == USB_SPEED_LOW)
> //            return -EINVAL;

Then AVR-CDC works even on direct connected USB Ports using uhci.

Unfortunately with Kernel 2.6.24 there appeared a more generic
solution for the low-speed bulk endpoints in drivers/usb/core/config.c.
It's commented like this:

Code: Select all

  /* Some buggy low-speed devices have Bulk endpoints, which is
   * explicitly forbidden by the USB spec.  In an attempt to make
   * them usable, we will try treating them as Interrupt endpoints.
   */

It leads to the following Kernel messages if i plug in my AVR-CDC:

Code: Select all

 usb 3-4.7: USB disconnect, address 41
 usb 3-4.7: new low speed USB device using ehci_hcd and address 42
 usb 3-4.7: config 1 interface 1 altsetting 0 endpoint 0x1 is Bulk; changing to Interrupt
 usb 3-4.7: config 1 interface 1 altsetting 0 endpoint 0x81 is Bulk; changing to Interrupt
 usb 3-4.7: configuration #1 chosen from 1 choice
 cdc_acm 3-4.7:1.0: ttyACM0: USB ACM device

This is bad for AVR-CDC because it does not support interrupt
endpoints. At first i wondered if this would be necessarry at all
because the kernel changes the endpoint from bulk to interrupt already.
Christian mentioned that there is not much difference between bulk and
interrupt endpoints too. Somebody else suggested an adjustment in
AVR-CDC.

So i simply changed the endpoint in the AVR-CDC Code from bulk to
interrupt. The kernel warning disappeared but it still doesn't work.

Then i discovered that there is a generic serial usb driver in the
kernel tree which is derived from the cdc_acm module and i thought it
should work with AVR-CDC too. With the interrupt endpoint enabled
AVR-CDC device the kernel says this:

Code: Select all

usbcore: registered new interface driver usbserial
drivers/usb/serial/usb-serial.c: USB Serial support registered for generic
usbserial_generic 3-4.7:1.0: Generic device with no bulk out, not allowed.
usbserial_generic: probe of 3-4.7:1.0 failed with error -5
usbserial_generic 3-4.7:1.1: Generic device with no bulk out, not allowed.
usbserial_generic: probe of 3-4.7:1.1 failed with error -5
usbcore: registered new interface driver usbserial_generic
drivers/usb/serial/usb-serial.c: USB Serial Driver core

After having a look at usb-serial.c and cdc-acm.c i found that the
driver insists on bulk endpoints but don't get it because the kernel
changed them to interrupt endpoints before or interrupt endpoints are
already introduced by my changed AVR-CDC firmware.

All this conforms very well with the USB specification and therefore
doesn't work.

So i think there is no chance to get the AVR-CDC device with
an unpatched linux kernel newer than 2.4.23 running. And i don't think
that this could ever be fixed by the AVR-CDC firmware since it is a
problem as a matter of principle.

Maybe the most reasonable solution would be to write a driver or a
fixed version of the cdc_acm or usbserial_generic module which can deal
with interrupt endpoints.

But the easiest way is to patch "drivers/usb/core/config.c"
like this:

Code: Select all

139,145c139,149
<               dev_warn(ddev, "config %d interface %d altsetting %d "
<                   "endpoint 0x%X is Bulk; changing to Interrupt\n",
<                   cfgno, inum, asnum, d->bEndpointAddress);
<               endpoint->desc.bmAttributes = USB_ENDPOINT_XFER_INT;
<               endpoint->desc.bInterval = 1;
<               if (le16_to_cpu(endpoint->desc.wMaxPacketSize) > 8)
<                       endpoint->desc.wMaxPacketSize = cpu_to_le16(8);
---
>          dev_warn(ddev, "config %d interface %d altsetting %d "
>               "endpoint 0x%X is Bulk; this violates USB spec for "
>               "low speed devices.\n",
>               cfgno, inum, asnum, d->bEndpointAddress);
> //            dev_warn(ddev, "config %d interface %d altsetting %d "
> //                "endpoint 0x%X is Bulk; changing to Interrupt\n",
> //                cfgno, inum, asnum, d->bEndpointAddress);
> //            endpoint->desc.bmAttributes = USB_ENDPOINT_XFER_INT;
> //            endpoint->desc.bInterval = 1;
> //            if (le16_to_cpu(endpoint->desc.wMaxPacketSize) > 8)
> //                    endpoint->desc.wMaxPacketSize = cpu_to_le16(8);

I have posted this already in the forum a few weeks ago. But i assume
in most situations both patches are necessary.


It would be very nice if one of the usb experts would write a short
comment whether my ideas could be true.

And sorry for my weak english!

Kind regards,
Klaus

Posted: Mon Apr 14, 2008 6:01 pm
by christian
Yes, Bulk endpoints are not allowed for low speed devices. There is no particular reason for this (at least not these days with USB 2.0 hubs), but the standard defines it this way. [For USB 1.0 hubs, this was a performance problem since polling blocks the bus for a long time.]

Since it's additional work to do this check, most operating systems don't care whether there is a bulk endpoint in a low speed device. I don't think it's a good idea to police things which don't harm just to make sure others conform to the standard.

Converting the bulk endpoint to an interrupt endpoint implicitly would be a great idea, if a reasonable poll interval is chosen. Unfortunately, the implementation of this conversion seems to be broken: The host side driver still searches for a bulk endpoint, of course, since the interface descriptor of the device lists one. The automatic conversion should redirect accesses to the bulk endpoint to the interrupt endpoint which is actually used so that the host software does not need to know about the conversion.

AVR-USB does not need to know about any conversions, at the device end there's really no difference between bulk and interrupt.

For the moment, it's probably best to remove all this "intelligence" and simply allow the bulk endpoint in the kernel driver. That's what all other operating systems to, after all.

Posted: Tue Apr 15, 2008 8:15 am
by texmex
Thanks for your fast reply!

christian wrote:Converting the bulk endpoint to an interrupt endpoint implicitly would be a great idea, if a reasonable poll interval is chosen. Unfortunately, the implementation of this conversion seems to be broken: The host side driver still searches for a bulk endpoint, of course, since the interface descriptor of the device lists one.


Hm, i think the interface descriptor is changed also by the code:

Code: Select all

              dev_warn(ddev, "config %d interface %d altsetting %d "
                  "endpoint 0x%X is Bulk; changing to Interrupt\n",
                  cfgno, inum, asnum, d->bEndpointAddress);
              endpoint->desc.bmAttributes = USB_ENDPOINT_XFER_INT;
              endpoint->desc.bInterval = 1;
              if (le16_to_cpu(endpoint->desc.wMaxPacketSize) > 8)
                      endpoint->desc.wMaxPacketSize = cpu_to_le16(8);


I suppose the problem is that config.c out of the usb core is maintained by different people than the cdc_acm module. And the cdc_acm module still insists on bulk endpoints. Therefore it fails.

For the moment, it's probably best to remove all this "intelligence" and simply allow the bulk endpoint in the kernel driver. That's what all other operating systems to, after all.


Yes, i think so too.


Kind regards,
Klaus

Posted: Tue Apr 15, 2008 10:01 am
by christian
The problem is not only with cdc_acm, but with all host side software. Libusb has different API functions for bulk and interrupt endpoints. Software which is written for one type of endpoint won't accept the other.