Bugs in STM32 USB-FS-Device development kit

[shogoonn]

Posted on March 08, 2014 at 06:52

Hi

I've just found a nasty bug while developing a CDC-ACM firmware that reads word values from class-specific requests. The byte order in USBwIndex was reversed, so after a little debugging I found these gems: Starting at line 108 of usb_core.h:

typedef
union
{
uint16_t w;
struct
BW
{
uint8_t bb1;
uint8_t bb0;
}
bw;
} uint16_t_uint8_t;

Since the Cortex-M core is little-endian, it means that the address of least significant byte should be lower than the address of the MSB. Therefore the byte members of the union (bb0 and bb1) must be swapped. Starting at line 874 of usb_core.c:

if
(pInformation->ControlState != PAUSE)
{
pInformation->USBbmRequestType = *pBuf.b++; 
/* bmRequestType */
pInformation->USBbRequest = *pBuf.b++; 
/* bRequest */
pBuf.w += offset; 
/* word not accessed because of 32 bits addressing */
pInformation->USBwValue = ByteSwap(*pBuf.w++); 
/* wValue */
pBuf.w += offset; 
/* word not accessed because of 32 bits addressing */
pInformation->USBwIndex = ByteSwap(*pBuf.w++); 
/* wIndex */
pBuf.w += offset; 
/* word not accessed because of 32 bits addressing */
pInformation->USBwLength = *pBuf.w; 
/* wLength */
}

The two ByteSwap() calls must be omitted, since both the USB packets and the Cortex-M core are little-endian. Curiously enough, the word value of a data length (USBwLength) is handled correctly. This also makes the ByteSwap() function unnecessary, since it's not used anywhere else in the code (library and examples). For portability of the code the ByteSwap() function can be replaced with a proper endianness-aware conversion and applied to USBwValue, USBwIndex and USBwLength. The uint16_t_uint8_t union definition should then be modified accordingly. These easy fixes will not break any existing library code and examples. The USBwValue and USBwIndex members throughout the code are either byte-accessed (via USBwValue0, USBwValue1, USBwIndex0, and USBwIndex1), or compared against zero as word values. The only (almost an) exception is line 271 of usb_core.c:

if
((pInformation->USBwValue != ENDPOINT_STALL)

which works by accident, since the value of the ENDPOINT_STALL enumerator is zero. This also creates some opportunities for optimisation, for example at line 578 of usb_core.c:

if
((pInformation->USBwValue0 > 127) || (pInformation->USBwValue1 != 0)
|| (pInformation->USBwIndex != 0)
|| (pInformation->Current_Configuration != 0))

The two comparisons can be reduced to a single one (pInformation->USBwValue > 127). Also some minor bugs: In the usb_mem.c file, the PMAToUserBufferCopy() function will always write an even number of bytes to user buffer (wNBytes rounded up), which may cause a buffer overflow with odd-length buffer. In the same file the UserToPMABufferCopy() function is just ugly. The data from pbUsrBuf could be read as half-words. It can cause an unaligned memory access, just as the PMAToUserBufferCopy() does. The penalty is a single cycle if there is no memory stall, this however is still faster than the original one which requires two byte-accesses and an ORL. The function will generate a usage fault exception on an address space regions which don't support unaligned access, but I can't imagine such usage scenario. #stm32-usb-library