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 <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: [email protected]
> Signed-off-by: Raz Manor <[email protected]>
> ---
> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
