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

[sundeep subbaraya]

Hi Felipe,

On Thu, Apr 17, 2014 at 8:31 PM, Felipe Balbi <balbi@xxxxxx> wrote:
> 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 does gadget driver decides to queue the same request again?
if -EBUSY is returned from ep_queue or req.status != 0 in completion
routine?

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.

do i need to have another local queue for requests waiting since
buffers are busy?
Or dequeue the request ?

>
>> 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

Thanks,
Sundeep.B.S.

>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/