Bug in udd_ep_alloc() in udp_device.c

Go To Last Post
3 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

In this function, they compare the endpoint number (in variable ep) to the number of EPs (in USB_DEVICE_MAX_EP).  That's not correct!

 

For instance, if you have only enabled the Interrupt endpoints, numbers 3 and 6, by setting the sizes for the others to 0 in conf_usb.h, it's going to compare 3 (the endpoint number) to 2 (the number of endpoints defined) and fail.

 

The offending lines are:

 

	if (ep > USB_DEVICE_MAX_EP) {
		return false;
	}

If you only enable the Bulk endpoints, it works by accident because those are numbers 1 and 2.  If you leave all 6 endpoints enabled like it is by default, it works by accident because all the numbers are in the range 1-6.  I ran across this bug because the Linux kernel was doing a set-interface command and that runs into this code, which results in Linux thinking the interface has no endpoints.  If you never issue that command you might not notice the bug.  A lsusb -v command on Linux, for instance, reports all the details about the interface just as exactly as expected (which made it that much harder to track down what was going on.)

 

I did a search but didn't see anyone else complaining of this.  I gather there's no official place to complain about ASF bugs any more (and that even when there was it didn't do much good.)  I could fix this any number of ways; has anyone else done so already?  I'm assuming their original intent was to see if you're trying to set up more interfaces than were configured in conf_usb.h, which would require counting them as you went or something.

 

Duncan

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Huh - when I fix that bug and only use 2 endpoints, the Linux kernel seems happy when setting up the device now...but I get no USB packets to the device at all. Nor when I use 4 endpoints.  Only when I use all 6.  Which makes me pretty certain there's another place in ASF where they're confusing endpoint number with endpoint as an index, or something. similar  I'll keep looking but of course it will mean digging into the Linux side again just to make sure the problem is not there.  Fun times, fun times.

 

Duncan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK, yes, I found the root of the problem. 

 

in udi_vendor_conf.h you have the lines:

 

#define UDI_VENDOR_EP_NB_INT  ((UDI_VENDOR_EPS_SIZE_INT_FS)?2:0)
#define UDI_VENDOR_EP_NB_BULK ((UDI_VENDOR_EPS_SIZE_BULK_FS)?2:0)
#define UDI_VENDOR_EP_NB_ISO  ((UDI_VENDOR_EPS_SIZE_ISO_FS)?2:0)
#define USB_DEVICE_MAX_EP     (UDI_VENDOR_EP_NB_INT+UDI_VENDOR_EP_NB_BULK+UDI_VENDOR_EP_NB_ISO)

In other words, the value is 2, 4, or 6, depending on how many endpoints you have enabled by setting sizes for them in usb_conf.h

 

Problem is, in the header of udc.h there is this definition of that value:

 

 * USB_DEVICE_MAX_EP (Byte)<br>
 * Define the maximum endpoint number used by the USB Device.<br>

...and THAT definition is how it is treated in about eleventy-billion places in the code.  Checking "ep" against that #define value, where "ep" is the endpoint number, which is defined higher up in udi_vendor_conf.h and is not necessarily in numerical order.

 

Epic fail.

 

SO I just changed that line to

 

#define USB_DEVICE_MAX_EP     6

and poof!  Everything works.  I suppose depending on your chip (I'm on a SAM4S) that value might change, but that is the bug.  They #define that value one way, and use it a completely different way, but it all accidentally works if you have all 6 endpoints going,

 

Duncan