I sent a correction for the patch. Removed const where it shouldn't be, don't know why it was compiled in the first place. As for the usage of const in general, I believe it is a very good practice that should be used anywhere possible, for both human and compiler better understanding of the code. -----Original Message----- From: Alan Stern [mailto:stern@xxxxxxxxxxxxxxxxxxx] Sent: Wednesday, February 8, 2017 6:41 PM To: Raz Manor <Raz.Manor@xxxxxxxxxx> Cc: Felipe Balbi <balbi@xxxxxxxxxx>; Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>; linux-usb@xxxxxxxxxxxxxxx Subject: Re: [PATCH 2/2] net2280: Fix tmp reusage in net2280 driver On Wed, 8 Feb 2017, Raz Manor wrote: > In the function scan_dma_completions() there is a reusage of tmp > variable. That coused a wrong value being used in some case when > reading a short packet terminated transaction from an endpoint, in 2 > concecutive reads. > > This was my logic for the patch: > > The req->td->dmadesc equals to 0 iff: > -- There was a transaction ending with a short packet, and > -- The read() to read it was shorter than the transaction length, and > -- The read() to complete it is longer than the residue. > I believe this is true from the printouts of various cases, but I > can't be positive it is correct. > > Entering this if, there should be no more data in the endpoint (a > short packet terminated the transaction). > If there is, the transaction wasn't really done and we should exit and > wait for it to finish entirely. That is the inner if. > That inner if should never happen, but it is there to be on the safe > side. That is why it is marked with the comment /* paranoia */. > The size of the data available in the endpoint is ep->dma->dmacount > and it is read to tmp. > This entire clause is based on my own educated guesses. > > If we passed that inner if without breaking in the original code, than > tmp & DMA_BYTE_MASK_COUNT== 0. > That means we will always pass dma bytes count of 0 to dma_done(), > meaning all the requested bytes were read. > > dma_done() reports back to the upper layer that the request (read()) > was done and how many bytes were read. > In the original code that would always be the request size, regardless > of the actual size of the data. > That did not make sense to me at all. > > However, the original value of tmp is req->td->dmacount, which is the > dmacount value when the request's dma transaction was finished. And > that is a much more reasonable value to report back to the caller. > > To recreate the problem: > Read from a bulk out endpoint in a loop, 1024 * n bytes in each > iteration. > Connect the PLX to a host you can control. > Send to that endpoint 1024 * n + x bytes, such that 0 < x < 1024 * n > and (x % 1024) != 0 You would expect the first read() to return 1024 * > n and the second read() to return x. > But you will get the first read to return 1024 * n and the second one > to return 1024 * n. > That is true for every positive integer n. > > Cc: Felipe Balbi <balbi@xxxxxxxxxx> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> > Cc: linux-usb@xxxxxxxxxxxxxxx > Signed-off-by: Raz Manor <Raz.Manor@xxxxxxxxxx> > --- > drivers/usb/gadget/udc/net2280.c | 25 +++++++++++++------------ > 1 file changed, 13 insertions(+), 12 deletions(-) > > diff --git a/drivers/usb/gadget/udc/net2280.c > b/drivers/usb/gadget/udc/net2280.c > index 8550441..a853347 100644 > --- a/drivers/usb/gadget/udc/net2280.c > +++ b/drivers/usb/gadget/udc/net2280.c > @@ -1146,15 +1146,15 @@ static int scan_dma_completions(struct net2280_ep *ep) > */ > while (!list_empty(&ep->queue)) { > struct net2280_request *req; > - u32 tmp; > + u32 const req_dma_count; What is the "const" doing here? It looks like a mistake. You aren't allowed to have a const definition without an initializer. > > req = list_entry(ep->queue.next, > struct net2280_request, queue); > if (!req->valid) > break; > rmb(); > - tmp = le32_to_cpup(&req->td->dmacount); > - if ((tmp & BIT(VALID_BIT)) != 0) > + req_dma_count = le32_to_cpup(&req->td->dmacount); This assignment is not allowed if req_dma_count truly is const. A const variable cannot be assigned to. > + if ((req_dma_count & BIT(VALID_BIT)) != 0) > break; > > /* SHORT_PACKET_TRANSFERRED_INTERRUPT handles "usb-short" > @@ -1163,40 +1163,41 @@ static int scan_dma_completions(struct net2280_ep *ep) > */ > if (unlikely(req->td->dmadesc == 0)) { > /* paranoia */ > - tmp = readl(&ep->dma->dmacount); > - if (tmp & DMA_BYTE_COUNT_MASK) > + u32 const ep_dmacount = readl(&ep->dma->dmacount); This also looks odd, although at least it doesn't violate the language spec. I suggest you don't use const definitions at all in this patch. Alan Stern -- 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
