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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140417150110.GB20889@saruman.home>
Date:	Thu, 17 Apr 2014 10:01:10 -0500
From:	Felipe Balbi <balbi@...com>
To:	sundeep subbaraya <sundeep.lkml@...il.com>
CC:	<balbi@...com>,
	Subbaraya Sundeep Bhatta <subbaraya.sundeep.bhatta@...inx.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Michal Simek <michals@...inx.com>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	Subbaraya Sundeep Bhatta <sbhatta@...inx.com>
Subject: Re: [PATCH v2 2/2] usb: gadget: Add xilinx axi usb2 device support

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@...com> 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@...com> 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

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)