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

[Felipe Balbi]

On Thu, Apr 17, 2014 at 03:01:37PM +0530, sundeep subbaraya wrote:
> Hi Felipe,
> 
> On Wed, Apr 16, 2014 at 11:32 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> > Hi,
> >
> > On Wed, Apr 16, 2014 at 04:09:28PM +0530, sundeep subbaraya wrote:
> >> Hi Felipe,
> >>
> >> On Thu, Apr 3, 2014 at 8:28 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> >>
> >> >> +static int start_dma(struct xusb_ep *ep, u32 src, u32 dst, u32 length)
> >> >
> >> > please prepend this with xudc_, it makes tracing a lot easier.
> >> >
> >> >> +{
> >> >> +     struct xusb_udc *udc;
> >> >> +     int rc = 0;
> >> >> +     unsigned long timeout;
> >> >> +
> >> >> +     udc = ep->udc;
> >> >> +     /*
> >> >> +      * Set the addresses in the DMA source and
> >> >> +      * destination registers and then set the length
> >> >> +      * into the DMA length register.
> >> >> +      */
> >> >> +     udc->write_fn(udc->base_address, XUSB_DMA_DSAR_ADDR_OFFSET, src);
> >> >> +     udc->write_fn(udc->base_address, XUSB_DMA_DDAR_ADDR_OFFSET, dst);
> >> >> +     udc->write_fn(udc->base_address, XUSB_DMA_LENGTH_OFFSET, length);
> >> >> +
> >> >> +     /*
> >> >> +      * Wait till DMA transaction is complete and
> >> >> +      * check whether the DMA transaction was
> >> >> +      * successful.
> >> >> +     */
> >> >> +     while ((udc->read_fn(ep->udc->base_address + XUSB_DMA_STATUS_OFFSET) &
> >> >> +                     XUSB_DMA_DMASR_BUSY) == XUSB_DMA_DMASR_BUSY) {
> >> >> +             timeout = jiffies + 10000;
> >> >> +
> >> >> +             if (time_after(jiffies, timeout)) {
> >> >> +                     rc = -ETIMEDOUT;
> >> >> +                     goto clean;
> >> >> +             }
> >> >> +     }
> >> >
> >> > don't you get an IRQ for DMA completion ? If you do, you could be using
> >> > wait_for_completion()
> >>
> >> This function is called in interrupt context when buffer is ready or
> >> free. It initiates DMA to transfer data from IP buffer to memory.
> >> Hence it waits in busy loop till DMA completes
> >
> > wait, so you start_dma() before your gadget driver asks you to ?
> 
> 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 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.

-- 
balbi

Attachment: signature.asc
Description: Digital signature