On 12/12/2010 11:42 PM, Pavan Kondeti wrote:
> On 12/11/2010 1:35 PM, Artem Leonenko wrote:
>>
>> There is one nasty scenario when CI13xxx driver fails:
>> a) two or more rx requests are queued (g_ether does that)
>> b) rx request completed, interrupt fires and ci13xxx dequeues rq
>> c) request complete() callback gets called and in turn it calls ep_queue()
>> c1) in ep_queue() request gets added to the TAIL of the rx queue list
>> d) ep gets primed with rq from (b)
>> e) interrupt fires
>> f) request gets popped from queue head for hw dequeue
>> G) requets from queue head wasn't enqueued
>> g1) isr_tr_complete_low() doesn't
>> enqueue more requests and it doesn't prime EP,
>> rx traffic stalls
>>
>> Solution:
>> a) enque queued requests ASAP, i.e. before calling complete() callback.
>> b) don't HW enqueue and prime endpoint with recently added request and
>> use the oldest request in the queue.
>>
>> Fixed issues:
>> a) ep_queue() may return an error code despite request was successfully
>> added to the queue (if _hardware_enqueue() fails)
>> b) Added requests are always processed in LIFO order, even if they are
>> added in complete() callback
>> c) Finally more than two and more queued requests are processed
>> consistently,
>> even if they were added in complete() callback
>>
>> And as a part of the fix to make g_ether (and especially booting with
>> NFS root) I had
>> to increase NET_IP_ALIGN to mips32 natural value (4).
>>
>> The fix was successfully tested on MIPS based SoC with 4KEc core and
>> CI13612 core. Board successfully boots with NFS root using g_ether
>> on ci13xxx udc
>>
>> Signed-off-by: Artem Leonenko <tikkeri@xxxxxxxxx>
>> ---
>> drivers/usb/gadget/ci13xxx_udc.c | 15 ++++++++-------
>> drivers/usb/gadget/u_ether.h | 5 +++++
>> 2 files changed, 13 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/ci13xxx_udc.c
>> b/drivers/usb/gadget/ci13xxx_udc.c
>> index d788c7d..2c06cad 100644
>> --- a/drivers/usb/gadget/ci13xxx_udc.c
>> +++ b/drivers/usb/gadget/ci13xxx_udc.c
>> @@ -1789,18 +1789,17 @@ __acquires(mEp->lock)
>>
>> dbg_done(_usb_addr(mEp), mReq->ptr->token, retval);
>>
>> - if (!mReq->req.no_interrupt && mReq->req.complete != NULL) {
>> - spin_unlock(mEp->lock);
>> - mReq->req.complete(&mEp->ep, &mReq->req);
>> - spin_lock(mEp->lock);
>> - }
>> -
>> if (!list_empty(&mEp->qh[mEp->dir].queue)) {
>> mReq = list_entry(mEp->qh[mEp->dir].queue.next,
>> struct ci13xxx_req, queue);
>
> You should use another variable for storing the request pointer. Because
> mReq is used below again to call the request complete function.
My bad, I made a mistake during merging my changes from 2.6.29 to
timesys 2.6.32 and than to linux-usb. Already fixed that in 'v2' patches.
> This patch worked fine (ping works with g_ether) for me after making
> this change. I did not modify NET_IP_ALIGN.
I'm dropping that change then.
>> _hardware_enqueue(mEp, mReq);
>> }
>>
>> + if (!mReq->req.no_interrupt && mReq->req.complete != NULL) {
>> + spin_unlock(mEp->lock);
>> + mReq->req.complete(&mEp->ep, &mReq->req);
>> + spin_lock(mEp->lock);
>> + }
>> done:
>> return retval;
>> }
>> @@ -2170,7 +2169,9 @@ static int ep_queue(struct usb_ep *ep, struct
>> usb_request *req,
>> mReq->req.actual = 0;
>> list_add_tail(&mReq->queue, &mEp->qh[mEp->dir].queue);
>>
>> - retval = _hardware_enqueue(mEp, mReq);
>> + if (list_is_singular(&mEp->qh[mEp->dir].queue))
>> + retval = _hardware_enqueue(mEp, mReq);
>> +
>> if (retval == -EALREADY || retval == -EBUSY) {
>> dbg_event(_usb_addr(mEp), "QUEUE", retval);
>> retval = 0;
>
> Given that _hardware_enqueue is called only when endpoint is in
> un-primed state, may be we should pass the value returned by
> _hardware_enqueue back to usb_ep_queue() caller.
>
How can we do that if caller puts more than one request to the queue? If
there are already requests in the queue then ep_queue returns and
_hardware_enqueue will be called later in isr_tr_complete_low().
>> diff --git a/drivers/usb/gadget/u_ether.h b/drivers/usb/gadget/u_ether.h
>> index 3c8c0c9..c4bb92e 100644
>> --- a/drivers/usb/gadget/u_ether.h
>> +++ b/drivers/usb/gadget/u_ether.h
>> @@ -23,6 +23,11 @@
>> #ifndef __U_ETHER_H
>> #define __U_ETHER_H
>>
>> +#if defined(CONFIG_USB_GADGET_CI13XXX)
>> +#define NET_IP_ALIGN 4
>> +#endif
>> +
>> +
>> #include <linux/err.h>
>> #include <linux/if_ether.h>
>> #include <linux/usb/composite.h>
>> --
>> 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
>
>
--
Cheers,
Artem
--
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
