Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support

[Felipe Balbi]

Hi,

On Fri, Apr 18, 2014 at 07:34:08PM +0530, sundeep subbaraya wrote:

<snip>

> >> in ep_queue driver starts dma transfer from/to IP buffer to/from req->buf.
> >> If transfer is completed then request is not added to ep request queue
> >> and returns from ep_queue.
> >> If transfer is not completed (actual < length) then request is added
> >> to queue and returns from ep_queue.
> >
> > This is wrong. Why wouldn't you give gadget driver the chance to decide
> > if it needs to queue the request again or not ?
> 
> When does gadget driver decides to queue the same request again?
> if -EBUSY is returned from ep_queue or req.status != 0 in completion
> routine?

whenever it so decides. Different gadget drivers might have different
requirements. The code is open and sits under drivers/usb/gadget/ why
don't you have a read ?

> there are cases where ping-pong buffers both are busy. currently this driver
> adds request to queue only when buffers are busy. In normal cases request is
> processed without adding to queue.

it shouldn't do that. You can't add requests by yourself. If you can't
initiate transfers right now, let the HW NAK or something.

> do i need to have another local queue for requests waiting since
> buffers are busy?

you should probably move the request to another queue, yes. Here's what
dwc3 does:

. gadget queues request
. dwc3 puts request in a 'queued' list
. HW sends XferNotReady event to notify it needs transfer requests
. dwc3 moves requests to 'pending' list
. dwc3 tells hw to consume 'pending' list

if gadget driver queues another request, we accept it into 'queued' list
and wait for another XferNotReady before moving it to 'pending' and
consuming it.

> Or dequeue the request ?

you could return -EAGAIN, if you wish. But don't add requests by
yourself, this could end up in hard-to-debug scenarios. Remember you
don't *own* the reqeusts, only gadget drivers do. You can keep the
request around until you have space in your FIFOs to start it, but then,
in that case, don't try to list_del() -> start() -> list_add().

> >> when buffer processed interrupt occurs, handler starts dma if there is
> >> request in queue and calls complete call back (when actual == length)
> >> Hence answer is Yes for some transfers (start dma called from
> >> interrupt handler not from ep_queue).
> >
> > this also seems wrong(-ish).
> >
> > 1) as Paul pointed out, you can't rely on jiffies if you're calling this
> > with IRQs disabled.
> >
> > 2) you don't really need to wait until DMA finishes its transaction
> > before returning from start_dma(), just use the DMA completion IRQ,
> >
> > 3) I don't see anywhere any sanitizing of arguments, can your DMA really
> > handle any alignment/unaligned addresses or should you make sure you're
> > getting good arguments?
> >
> > You need to work on this a little bit, I guess.
> 
> Yes am working on this and will be using seperate ep_ops for ep0  like
> drivers/usb/dwc3/gadget.c since ep0 has no dma and ping-pong buffers

ok

-- 
balbi

Attachment: signature.asc
Description: Digital signature