On Fri, 26 Apr 2013, victor yeo wrote:
> Please see the attached kagen2_ep_queue(). As long as it is called, it
> will queue the request and read packets from hardware, in the same
> if-else branch for bulk out endpoint. The interrupt handler will NOT
> accept packet if request is NOT queued. If request is queued,
> interrupt handler will accept the packet.
This code is wrong. See what happens when a request for ep1-out is
submitted:
> /* ep1 OUT endpoint */
> else if (in == 0)
> {
> // read from EP1 OUT buffer
> if (num == 1)
> {
> unsigned int val;
> unsigned int val_arr[8];
> int i;
>
> // get byte count from hardware
> val = readl(dev->base_addr + 0x008);
> len = val & 0xFF;
Why do you expect there to be any data in the hardware FIFO at this
point? You said that the hardware will not accept any data if a
request is not queued. Well, before this point the request wasn't
queued, so there shouldn't be any data.
>
> // read from hardware fifo1 data
> for (i = 0; i < len/4; i++)
> {
> val_arr[i] = readl(dev->base_addr + 0x084);
> }
val_arr is declared as an array of 8 unsigned ints. That means its
total size is 32 bytes. What happens if len > 32? You will end up
overwriting part of the stack.
> list_add_tail(&ka_req->queue, &ka_ep->queue);
>
> ka_req->req.actual = len;
> memcpy(ka_req->req.buf, &val_arr[0], len);
>
> ka_req->req.complete(&ka_ep->ep, &ka_req->req);
You should not call req.complete until the request really is complete.
For example, what happens here if the host hasn't sent any packets yet?
Or what happens if req is waiting to receive 1024 bytes but the host
has sent only 512 bytes so far?
Also, all the data gets copied _twice_: once from the hardware FIFO
into val_arr, and then again from val_arr into req.buf. That's a waste
of time; the data should be copied directly from the FIFO into req.buf.
> list_del_init(&ka_req->queue);
>
> // clear hardware OUT1CS register
> val = readl(dev->base_addr + 0x008);
> val &= 0x00ffffff;
> writel(val, dev->base_addr + 0x008);
Does this really clear the register? It looks like the low-order 8
bits (which got read into "len" earlier) remain unchanged.
> }
> }
There probably are many other problems like these which need to be
fixed.
> Somehow, there is still timing problem in UDC driver and it is hard to
> pin down the root cause. It could be due to interaction of UDC driver
> queue() and gadget driver fsg_main_thread() main loop.
>
> 1) When writing to gen2 gadget, SCSI_READ_10 or or SCSI_REQUEST_SENSE
> commands are received by UDC driver, but gadget did not process the
> commands. (cannot get past get_next_command() in fsg_main_thread)
Then add printk statements inside get_next_command() so you can see
exactly where it's getting stuck.
> 2) Repeatedly (many many times), the same SCSI_READ_10 command is
> received by UDC driver, processed by gadget driver, and UDC driver
> sends out data and CSW to host. On usbmon trace, only one instance of
> the SCSI_READ_10 is observed.
This is probably because you are copying the same data from the FIFO to
req.buffer many times.
> 3) More severe case, if removing one printk statement in
> bulk_in_complete(), USB gadget device cannot be recognised by host.
I think there must still be many bugs remaining in the UDC driver.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
