| Message ID | 20190121134447.13944-1-m.szyprowski@samsung.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] usb: dwc2: gadget: Add scatter-gather mode | expand |
Hi Minas, W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze: > Hi, > > SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on > ep1in. It's first BULK IN transaction after GET MAXLUN. > Can you help me reproduce the problem? I have an Odroid U3. > > Meantime it's passing smoke test with mass storage function connected to > Linux host. So the "real" use case with mass storage works, right? Andrzej
Hi Andrzej, On 3/12/2019 3:39 PM, Andrzej Pietrasiewicz wrote: > Hi Minas, > > > W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze: >> Hi, >> >> SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on >> ep1in. It's first BULK IN transaction after GET MAXLUN. >> > Can you help me reproduce the problem? I have an Odroid U3. 1. Download from usb.org USB CV tool. Install on MS Windows. 2. Setup your Odroid U3 as mass storage and connect to MS Windows. 3. Run USB CV. Choose MSC tests to run. I used "USB 3 Gen X Command Verifier". > >> >> Meantime it's passing smoke test with mass storage function connected to >> Linux host. > > So the "real" use case with mass storage works, right? > Yes, it's works. Actually, USB CV use another OS and host driver which can be reason of fail, but mass storage should pass all CV MSC tests besides smoke tests with Linux. > Andrzej > Thanks, Minas
Hi Minas, W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze: > Hi, > > SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on > ep1in. It's first BULK IN transaction after GET MAXLUN. > Thank you for reporting. I bisected and identified my own commit 10209abe87f5ebfd482a00323f5236d6094d0865 Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com> Date: Mon Jan 21 14:44:47 2019 +0100 usb: dwc2: gadget: Add scatter-gather mode as the one introducing the regression. At this moment I don't know how to fix it. I will lokk into it next week. @colleagues from Samsung: can you have a look at the 4412 datasheet and verify that in this patch all bits are properly set for scatter-gather mode? Andrzej PS. @Minas: Is there any reliable way to switch back and forth between running CV software and regular usage of USB? What an aggressive piece of software the USB 3 Gen X Command Verifier is! Its installer warns "not to run the application" in case you use devices connected to the chosen Host Controller because it will take over that HC. I thought this referred to the application being installed and that as long I didn't run the tests (i.e. the installed application) everything would be fine. How wrong I was! The warning should have clearly indicated that it refers to the installer itself! I was then unable to use keyboard/mouse connected to the docking station even if I uninstalled the CV software. I use Linux 99% of the time, but for the remaining 1% I want my Windows installation to be working, I don't know how to repair Windows. I somehow managed to recover but I am not going to install this on my laptop again. I managed to get some other laptop which is not used with a docking station, but still with this software it is best to have a dedicated machine only for that purpose. Maybe it is the Windows itself and the application is implemented the only possible way, I don't know.
Hi again, sorry for long lines. Reformatted now. Hi Minas, W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze: > Hi, > > SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on > ep1in. It's first BULK IN transaction after GET MAXLUN. > Thank you for reporting. I bisected and identified my own commit 10209abe87f5ebfd482a00323f5236d6094d0865 Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com> Date: Mon Jan 21 14:44:47 2019 +0100 usb: dwc2: gadget: Add scatter-gather mode as the one introducing the regression. At this moment I don't know how to fix it. I will lokk into it next week. @colleagues from Samsung: can you have a look at the 4412 datasheet and verify that in this patch all bits are properly set for scatter-gather mode? Andrzej PS. @Minas: Is there any reliable way to switch back and forth between running CV software and regular usage of USB? What an aggressive piece of software the USB 3 Gen X Command Verifier is! Its installer warns "not to run the application" in case you use devices connected to the chosen Host Controller because it will take over that HC. I thought this referred to the application being installed and that as long I didn't run the tests (i.e. the installed application) everything would be fine. How wrong I was! The warning should have clearly indicated that it refers to the installer itself! I was then unable to use keyboard/mouse connected to the docking station even if I uninstalled the CV software. I use Linux 99% of the time, but for the remaining 1% I want my Windows installation to be working, I don't know how to repair Windows. I somehow managed to recover but I am not going to install this on my laptop again. I managed to get some other laptop which is not used with a docking station, but still with this software it is best to have a dedicated machine only for that purpose. Maybe it is the Windows itself and the application is implemented the only possible way, I don't know.
Hi Andrzej, On 3/15/2019 3:32 PM, Andrzej Pietrasiewicz wrote: > Hi again, > > sorry for long lines. Reformatted now. > > Hi Minas, > > > W dniu 12.03.2019 o 08:54, Minas Harutyunyan pisze: >> Hi, >> >> SB CV MSC tests failed starting from Test Case 6 with BNA interrupt on >> ep1in. It's first BULK IN transaction after GET MAXLUN. >> > > Thank you for reporting. > > I bisected and identified my own > > commit 10209abe87f5ebfd482a00323f5236d6094d0865 > Author: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > Date: Mon Jan 21 14:44:47 2019 +0100 > > usb: dwc2: gadget: Add scatter-gather mode > > as the one introducing the regression. > > At this moment I don't know how to fix it. I will lokk into it next week. > > @colleagues from Samsung: can you have a look at the 4412 datasheet > and verify that in this patch all bits are properly set for > scatter-gather mode? > > > Andrzej > > PS. @Minas: Is there any reliable way to switch back and forth between > running CV software and regular usage of USB? What an aggressive piece > of software the USB 3 Gen X Command Verifier is! Its installer warns > "not to run the application" in case you use devices connected to the chosen > Host Controller because it will take over that HC. I thought this > referred to > the application being installed and that as long I didn't run the tests > (i.e. the installed application) everything would be fine. How wrong I was! > The warning should have clearly indicated that it refers to the > installer itself! > I was then unable to use keyboard/mouse connected to the docking station > even if I uninstalled the CV software. I use Linux 99% of the time, but > for the > remaining 1% I want my Windows installation to be working, I don't know how > to repair Windows. I somehow managed to recover but I am not going to > install this on my laptop again. I managed to get some other laptop which > is not used with a docking station, but still with this software it is > best to have > a dedicated machine only for that purpose. Maybe it is the Windows itself > and the application is implemented the only possible way, I don't know. > Yes, using personal laptop for USB CV testing is not good idea. We use lab machine for this kind of testing. Thanks, Minas
Hi Andrzej, On 2019-03-29 15:05, Andrzej Pietrasiewicz wrote: > From: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > > NOT FOR MERGING > > @Minas: can you please test with this patch instead of the original one? > > @Marek: can you please verify if null pointer bug exists in this version? Yes, I've checked it on my test environment and the NULL ptr issue observed in v2 is fixed. I think it is a good idea to send incremental patch to mainline with a 'Fixes:' tag. > This patch adds support for transferring requests, which are > non-contiguous in physical memory, i.e. the data buffer is described by > a scatter-list. This allows transferring large requests without relying > on error-prone contiguous buffer allocations. This way of allocating > requests is already implemented in functionfs and TCM USB functions and > automatically used if UDC driver advertises scatter-gather suppport. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > [mszyprow: fixed null pointer issue, rewrote commit message] > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > [andrzej.p: improved handling zlps] > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/usb/dwc2/gadget.c | 107 +++++++++++++++++++++++++++----------- > 1 file changed, 76 insertions(+), 31 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index e15d8a462085..e76b2e040b4c 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -768,22 +768,13 @@ static u32 dwc2_gadget_get_desc_params(struct dwc2_hsotg_ep *hs_ep, u32 *mask) > return desc_size; > } > > -/* > - * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > - * @hs_ep: The endpoint > - * @dma_buff: DMA address to use > - * @len: Length of the transfer > - * > - * This function will iterate over descriptor chain and fill its entries > - * with corresponding information based on transfer data. > - */ > -static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > +static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, > + struct dwc2_dma_desc **desc, > dma_addr_t dma_buff, > - unsigned int len) > + unsigned int len, > + bool true_last) > { > - struct dwc2_hsotg *hsotg = hs_ep->parent; > int dir_in = hs_ep->dir_in; > - struct dwc2_dma_desc *desc = hs_ep->desc_list; > u32 mps = hs_ep->ep.maxpacket; > u32 maxsize = 0; > u32 offset = 0; > @@ -798,41 +789,82 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > hs_ep->desc_count = 1; > > for (i = 0; i < hs_ep->desc_count; ++i) { > - desc->status = 0; > - desc->status |= (DEV_DMA_BUFF_STS_HBUSY > + (*desc)->status = 0; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HBUSY > << DEV_DMA_BUFF_STS_SHIFT); > > if (len > maxsize) { > if (!hs_ep->index && !dir_in) > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > - desc->status |= (maxsize << > - DEV_DMA_NBYTES_SHIFT & mask); > - desc->buf = dma_buff + offset; > + (*desc)->status |= > + maxsize << DEV_DMA_NBYTES_SHIFT & mask; > + (*desc)->buf = dma_buff + offset; > > len -= maxsize; > offset += maxsize; > } else { > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + if (true_last) > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > if (dir_in) > - desc->status |= (len % mps) ? DEV_DMA_SHORT : > - ((hs_ep->send_zlp) ? DEV_DMA_SHORT : 0); > - if (len > maxsize) > - dev_err(hsotg->dev, "wrong len %d\n", len); > + (*desc)->status |= (len % mps) ? DEV_DMA_SHORT : > + ((hs_ep->send_zlp && true_last) ? > + DEV_DMA_SHORT : 0); > > - desc->status |= > + (*desc)->status |= > len << DEV_DMA_NBYTES_SHIFT & mask; > - desc->buf = dma_buff + offset; > + (*desc)->buf = dma_buff + offset; > } > > - desc->status &= ~DEV_DMA_BUFF_STS_MASK; > - desc->status |= (DEV_DMA_BUFF_STS_HREADY > + (*desc)->status &= ~DEV_DMA_BUFF_STS_MASK; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HREADY > << DEV_DMA_BUFF_STS_SHIFT); > - desc++; > + (*desc)++; > } > } > > +/* > + * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > + * @hs_ep: The endpoint > + * @ureq: Request to transfer > + * @offset: offset in bytes > + * @len: Length of the transfer > + * > + * This function will iterate over descriptor chain and fill its entries > + * with corresponding information based on transfer data. > + */ > +static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > + dma_addr_t dma_buff, > + unsigned int len) > +{ > + struct usb_request *ureq = NULL; > + struct dwc2_dma_desc *desc = hs_ep->desc_list; > + struct scatterlist *sg; > + int i; > + u8 desc_count = 0; > + > + if (hs_ep->req) > + ureq = &hs_ep->req->req; > + > + /* non-DMA sg buffer */ > + if (!ureq || !ureq->num_sgs) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + dma_buff, len, true); > + return; > + } > + > + /* DMA sg buffer */ > + for_each_sg(ureq->sg, sg, ureq->num_sgs, i) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + sg_dma_address(sg) + sg->offset, sg_dma_len(sg), > + sg_is_last(sg)); > + desc_count += hs_ep->desc_count; > + } > + > + hs_ep->desc_count = desc_count; > +} > + > /* > * dwc2_gadget_fill_isoc_desc - fills next isochronous descriptor in chain. > * @hs_ep: The isochronous endpoint. > @@ -944,7 +976,13 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep) > > hs_ep->next_desc = 0; > list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) { > - ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + ret = dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > if (ret) > break; > @@ -1399,7 +1437,13 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, > */ > if (using_desc_dma(hs) && hs_ep->isochronous) { > if (hs_ep->target_frame != TARGET_FRAME_INITIAL) { > - dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > } > return 0; > @@ -4386,6 +4430,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, > hsotg->enabled = 0; > spin_unlock_irqrestore(&hsotg->lock, flags); > > + gadget->sg_supported = using_desc_dma(hsotg); > dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); > > return 0; Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
Hi Andrzej, On 3/29/2019 6:05 PM, Andrzej Pietrasiewicz wrote: > From: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > > NOT FOR MERGING > > @Minas: can you please test with this patch instead of the original one? > Tested new version. No issue seen by running USB CV (Ch9, MSC) tests. Could you please elaborate which else tests you performed? > @Marek: can you please verify if null pointer bug exists in this version? > > This patch adds support for transferring requests, which are > non-contiguous in physical memory, i.e. the data buffer is described by > a scatter-list. This allows transferring large requests without relying > on error-prone contiguous buffer allocations. This way of allocating > requests is already implemented in functionfs and TCM USB functions and > automatically used if UDC driver advertises scatter-gather suppport. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@samsung.com> > [mszyprow: fixed null pointer issue, rewrote commit message] > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > [andrzej.p: improved handling zlps] > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/usb/dwc2/gadget.c | 107 +++++++++++++++++++++++++++----------- > 1 file changed, 76 insertions(+), 31 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index e15d8a462085..e76b2e040b4c 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -768,22 +768,13 @@ static u32 dwc2_gadget_get_desc_params(struct dwc2_hsotg_ep *hs_ep, u32 *mask) > return desc_size; > } > > -/* > - * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > - * @hs_ep: The endpoint > - * @dma_buff: DMA address to use > - * @len: Length of the transfer > - * > - * This function will iterate over descriptor chain and fill its entries > - * with corresponding information based on transfer data. > - */ > -static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > +static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, > + struct dwc2_dma_desc **desc, > dma_addr_t dma_buff, > - unsigned int len) > + unsigned int len, > + bool true_last) > { > - struct dwc2_hsotg *hsotg = hs_ep->parent; > int dir_in = hs_ep->dir_in; > - struct dwc2_dma_desc *desc = hs_ep->desc_list; > u32 mps = hs_ep->ep.maxpacket; > u32 maxsize = 0; > u32 offset = 0; > @@ -798,41 +789,82 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > hs_ep->desc_count = 1; > > for (i = 0; i < hs_ep->desc_count; ++i) { > - desc->status = 0; > - desc->status |= (DEV_DMA_BUFF_STS_HBUSY > + (*desc)->status = 0; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HBUSY > << DEV_DMA_BUFF_STS_SHIFT); > > if (len > maxsize) { > if (!hs_ep->index && !dir_in) > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > - desc->status |= (maxsize << > - DEV_DMA_NBYTES_SHIFT & mask); > - desc->buf = dma_buff + offset; > + (*desc)->status |= > + maxsize << DEV_DMA_NBYTES_SHIFT & mask; > + (*desc)->buf = dma_buff + offset; > > len -= maxsize; > offset += maxsize; > } else { > - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); > + if (true_last) > + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); > > if (dir_in) > - desc->status |= (len % mps) ? DEV_DMA_SHORT : > - ((hs_ep->send_zlp) ? DEV_DMA_SHORT : 0); > - if (len > maxsize) > - dev_err(hsotg->dev, "wrong len %d\n", len); > + (*desc)->status |= (len % mps) ? DEV_DMA_SHORT : > + ((hs_ep->send_zlp && true_last) ? > + DEV_DMA_SHORT : 0); > > - desc->status |= > + (*desc)->status |= > len << DEV_DMA_NBYTES_SHIFT & mask; > - desc->buf = dma_buff + offset; > + (*desc)->buf = dma_buff + offset; > } > > - desc->status &= ~DEV_DMA_BUFF_STS_MASK; > - desc->status |= (DEV_DMA_BUFF_STS_HREADY > + (*desc)->status &= ~DEV_DMA_BUFF_STS_MASK; > + (*desc)->status |= (DEV_DMA_BUFF_STS_HREADY > << DEV_DMA_BUFF_STS_SHIFT); > - desc++; > + (*desc)++; > } > } > > +/* > + * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. > + * @hs_ep: The endpoint > + * @ureq: Request to transfer > + * @offset: offset in bytes > + * @len: Length of the transfer > + * > + * This function will iterate over descriptor chain and fill its entries > + * with corresponding information based on transfer data. > + */ > +static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > + dma_addr_t dma_buff, > + unsigned int len) > +{ > + struct usb_request *ureq = NULL; > + struct dwc2_dma_desc *desc = hs_ep->desc_list; > + struct scatterlist *sg; > + int i; > + u8 desc_count = 0; > + > + if (hs_ep->req) > + ureq = &hs_ep->req->req; > + > + /* non-DMA sg buffer */ > + if (!ureq || !ureq->num_sgs) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + dma_buff, len, true); > + return; > + } > + > + /* DMA sg buffer */ > + for_each_sg(ureq->sg, sg, ureq->num_sgs, i) { > + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > + sg_dma_address(sg) + sg->offset, sg_dma_len(sg), > + sg_is_last(sg)); > + desc_count += hs_ep->desc_count; > + } > + > + hs_ep->desc_count = desc_count; > +} > + > /* > * dwc2_gadget_fill_isoc_desc - fills next isochronous descriptor in chain. > * @hs_ep: The isochronous endpoint. > @@ -944,7 +976,13 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep) > > hs_ep->next_desc = 0; > list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) { > - ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + ret = dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > if (ret) > break; > @@ -1399,7 +1437,13 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, > */ > if (using_desc_dma(hs) && hs_ep->isochronous) { > if (hs_ep->target_frame != TARGET_FRAME_INITIAL) { > - dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, > + dma_addr_t dma_addr = hs_req->req.dma; > + > + if (hs_req->req.num_sgs) { > + WARN_ON(hs_req->req.num_sgs > 1); > + dma_addr = sg_dma_address(hs_req->req.sg); > + } > + dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, > hs_req->req.length); > } > return 0; > @@ -4386,6 +4430,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, > hsotg->enabled = 0; > spin_unlock_irqrestore(&hsotg->lock, flags); > > + gadget->sg_supported = using_desc_dma(hsotg); > dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); > > return 0; >
Hi Minas, W dniu 01.04.2019 o 12:01, Minas Harutyunyan pisze: > >> @Minas: can you please test with this patch instead of the original one? >> > Tested new version. No issue seen by running USB CV (Ch9, MSC) tests. > Could you please elaborate which else tests you performed? Only those reported by you to be failing, that is MSC. Andrzej
On 4/1/2019 2:51 PM, Andrzej Pietrasiewicz wrote: > The patch 10209abe87f5ebfd482a00323f5236d6094d0865 > usb: dwc2: gadget: Add scatter-gather mode > > avoided a NULL pointer dereference (hs_ep->req == NULL) by > calling dwc2_gadget_fill_nonisoc_xfer_dma_one() directly instead of through > the dwc2_gadget_config_nonisoc_xfer_ddma() wrapper, which unconditionally > dereferenced the said pointer. > > However, this was based on an incorrect assumption that in the context of > dwc2_hsotg_program_zlp() the pointer is always NULL, which is not the case. > The result were SB CV MSC tests failing starting from Test Case 6. > > Instead, this patch reverts to calling the wrapper and adds a check for > the pointer being NULL inside the wrapper. > > Fixes: 10209abe87f5 (usb: dwc2: gadget: Add scatter-gather mode) > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> Acked-by: Minas Harutyunyan <hminas@synopsys.com> > --- > drivers/usb/dwc2/gadget.c | 20 ++++++++++++-------- > 1 file changed, 12 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 6812a8a3a98b..e76b2e040b4c 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -835,19 +835,22 @@ static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, > * with corresponding information based on transfer data. > */ > static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, > - struct usb_request *ureq, > - unsigned int offset, > + dma_addr_t dma_buff, > unsigned int len) > { > + struct usb_request *ureq = NULL; > struct dwc2_dma_desc *desc = hs_ep->desc_list; > struct scatterlist *sg; > int i; > u8 desc_count = 0; > > + if (hs_ep->req) > + ureq = &hs_ep->req->req; > + > /* non-DMA sg buffer */ > - if (!ureq->num_sgs) { > + if (!ureq || !ureq->num_sgs) { > dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, > - ureq->dma + offset, len, true); > + dma_buff, len, true); > return; > } > > @@ -1135,7 +1138,7 @@ static void dwc2_hsotg_start_req(struct dwc2_hsotg *hsotg, > offset = ureq->actual; > > /* Fill DDMA chain entries */ > - dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq, offset, > + dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq->dma + offset, > length); > > /* write descriptor chain address to control register */ > @@ -2028,12 +2031,13 @@ static void dwc2_hsotg_program_zlp(struct dwc2_hsotg *hsotg, > dev_dbg(hsotg->dev, "Receiving zero-length packet on ep%d\n", > index); > if (using_desc_dma(hsotg)) { > + /* Not specific buffer needed for ep0 ZLP */ > + dma_addr_t dma = hs_ep->desc_list_dma; > + > if (!index) > dwc2_gadget_set_ep0_desc_chain(hsotg, hs_ep); > > - /* Not specific buffer needed for ep0 ZLP */ > - dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &hs_ep->desc_list, > - hs_ep->desc_list_dma, 0, true); > + dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, dma, 0); > } else { > dwc2_writel(hsotg, DXEPTSIZ_MC(1) | DXEPTSIZ_PKTCNT(1) | > DXEPTSIZ_XFERSIZE(0), >
Hi Felipe, On 4/1/2019 3:33 PM, Minas Harutyunyan wrote: > On 4/1/2019 2:51 PM, Andrzej Pietrasiewicz wrote: >> The patch 10209abe87f5ebfd482a00323f5236d6094d0865 >> usb: dwc2: gadget: Add scatter-gather mode >> >> avoided a NULL pointer dereference (hs_ep->req == NULL) by >> calling dwc2_gadget_fill_nonisoc_xfer_dma_one() directly instead of through >> the dwc2_gadget_config_nonisoc_xfer_ddma() wrapper, which unconditionally >> dereferenced the said pointer. >> >> However, this was based on an incorrect assumption that in the context of >> dwc2_hsotg_program_zlp() the pointer is always NULL, which is not the case. >> The result were SB CV MSC tests failing starting from Test Case 6. >> >> Instead, this patch reverts to calling the wrapper and adds a check for >> the pointer being NULL inside the wrapper. >> >> Fixes: 10209abe87f5 (usb: dwc2: gadget: Add scatter-gather mode) >> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > > Acked-by: Minas Harutyunyan <hminas@synopsys.com> > >> --- >> drivers/usb/dwc2/gadget.c | 20 ++++++++++++-------- >> 1 file changed, 12 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c >> index 6812a8a3a98b..e76b2e040b4c 100644 >> --- a/drivers/usb/dwc2/gadget.c >> +++ b/drivers/usb/dwc2/gadget.c >> @@ -835,19 +835,22 @@ static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, >> * with corresponding information based on transfer data. >> */ >> static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, >> - struct usb_request *ureq, >> - unsigned int offset, >> + dma_addr_t dma_buff, >> unsigned int len) >> { >> + struct usb_request *ureq = NULL; >> struct dwc2_dma_desc *desc = hs_ep->desc_list; >> struct scatterlist *sg; >> int i; >> u8 desc_count = 0; >> >> + if (hs_ep->req) >> + ureq = &hs_ep->req->req; >> + >> /* non-DMA sg buffer */ >> - if (!ureq->num_sgs) { >> + if (!ureq || !ureq->num_sgs) { >> dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, >> - ureq->dma + offset, len, true); >> + dma_buff, len, true); >> return; >> } >> >> @@ -1135,7 +1138,7 @@ static void dwc2_hsotg_start_req(struct dwc2_hsotg *hsotg, >> offset = ureq->actual; >> >> /* Fill DDMA chain entries */ >> - dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq, offset, >> + dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq->dma + offset, >> length); >> >> /* write descriptor chain address to control register */ >> @@ -2028,12 +2031,13 @@ static void dwc2_hsotg_program_zlp(struct dwc2_hsotg *hsotg, >> dev_dbg(hsotg->dev, "Receiving zero-length packet on ep%d\n", >> index); >> if (using_desc_dma(hsotg)) { >> + /* Not specific buffer needed for ep0 ZLP */ >> + dma_addr_t dma = hs_ep->desc_list_dma; >> + >> if (!index) >> dwc2_gadget_set_ep0_desc_chain(hsotg, hs_ep); >> >> - /* Not specific buffer needed for ep0 ZLP */ >> - dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &hs_ep->desc_list, >> - hs_ep->desc_list_dma, 0, true); >> + dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, dma, 0); >> } else { >> dwc2_writel(hsotg, DXEPTSIZ_MC(1) | DXEPTSIZ_PKTCNT(1) | >> DXEPTSIZ_XFERSIZE(0), >> > > This patch is fix for "usb: dwc2: gadget: Add scatter-gather mode" patch to allow pass USB CV. Could you please merge to your testing/next this patch also. Thanks, Minas
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 357beb40b7a3..af419034e438 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -835,22 +835,13 @@ static u32 dwc2_gadget_get_desc_params(struct dwc2_hsotg_ep *hs_ep, u32 *mask) return desc_size; } -/* - * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. - * @hs_ep: The endpoint - * @dma_buff: DMA address to use - * @len: Length of the transfer - * - * This function will iterate over descriptor chain and fill its entries - * with corresponding information based on transfer data. - */ -static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, +static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep, + struct dwc2_dma_desc **desc, dma_addr_t dma_buff, - unsigned int len) + unsigned int len, + bool true_last) { - struct dwc2_hsotg *hsotg = hs_ep->parent; int dir_in = hs_ep->dir_in; - struct dwc2_dma_desc *desc = hs_ep->desc_list; u32 mps = hs_ep->ep.maxpacket; u32 maxsize = 0; u32 offset = 0; @@ -865,39 +856,77 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, hs_ep->desc_count = 1; for (i = 0; i < hs_ep->desc_count; ++i) { - desc->status = 0; - desc->status |= (DEV_DMA_BUFF_STS_HBUSY + (*desc)->status = 0; + (*desc)->status |= (DEV_DMA_BUFF_STS_HBUSY << DEV_DMA_BUFF_STS_SHIFT); if (len > maxsize) { if (!hs_ep->index && !dir_in) - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); - desc->status |= (maxsize << - DEV_DMA_NBYTES_SHIFT & mask); - desc->buf = dma_buff + offset; + (*desc)->status |= + maxsize << DEV_DMA_NBYTES_SHIFT & mask; + (*desc)->buf = dma_buff + offset; len -= maxsize; offset += maxsize; } else { - desc->status |= (DEV_DMA_L | DEV_DMA_IOC); + if (true_last) + (*desc)->status |= (DEV_DMA_L | DEV_DMA_IOC); if (dir_in) - desc->status |= (len % mps) ? DEV_DMA_SHORT : - ((hs_ep->send_zlp) ? DEV_DMA_SHORT : 0); - if (len > maxsize) - dev_err(hsotg->dev, "wrong len %d\n", len); + (*desc)->status |= (len % mps) ? DEV_DMA_SHORT : + ((hs_ep->send_zlp && true_last) ? + DEV_DMA_SHORT : 0); - desc->status |= + (*desc)->status |= len << DEV_DMA_NBYTES_SHIFT & mask; - desc->buf = dma_buff + offset; + (*desc)->buf = dma_buff + offset; } - desc->status &= ~DEV_DMA_BUFF_STS_MASK; - desc->status |= (DEV_DMA_BUFF_STS_HREADY + (*desc)->status &= ~DEV_DMA_BUFF_STS_MASK; + (*desc)->status |= (DEV_DMA_BUFF_STS_HREADY << DEV_DMA_BUFF_STS_SHIFT); - desc++; + (*desc)++; + } +} + +/* + * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain. + * @hs_ep: The endpoint + * @ureq: Request to transfer + * @offset: offset in bytes + * @len: Length of the transfer + * + * This function will iterate over descriptor chain and fill its entries + * with corresponding information based on transfer data. + */ +static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep, + struct usb_request *ureq, + unsigned int offset, + unsigned int len) +{ + struct dwc2_dma_desc *desc = hs_ep->desc_list; + struct scatterlist *sg; + int i; + u8 desc_count = 0; + + /* non-DMA sg buffer */ + if (!ureq->num_sgs) { + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, + ureq->dma + offset, len, true); + return; } + + /* DMA sg buffer */ + for_each_sg(ureq->sg, sg, ureq->num_sgs, i) { + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc, + sg_dma_address(sg) + sg->offset, sg_dma_len(sg), + sg_is_last(sg)); + desc_count += hs_ep->desc_count; + } + + hs_ep->desc_count = desc_count; } /* @@ -1011,7 +1040,13 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep) hs_ep->next_desc = 0; list_for_each_entry_safe(hs_req, treq, &hs_ep->queue, queue) { - ret = dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, + dma_addr_t dma_addr = hs_req->req.dma; + + if (hs_req->req.num_sgs) { + WARN_ON(hs_req->req.num_sgs > 1); + dma_addr = sg_dma_address(hs_req->req.sg); + } + ret = dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, hs_req->req.length); if (ret) break; @@ -1167,7 +1202,7 @@ static void dwc2_hsotg_start_req(struct dwc2_hsotg *hsotg, offset = ureq->actual; /* Fill DDMA chain entries */ - dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq->dma + offset, + dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq, offset, length); /* write descriptor chain address to control register */ @@ -1466,7 +1501,13 @@ static int dwc2_hsotg_ep_queue(struct usb_ep *ep, struct usb_request *req, */ if (using_desc_dma(hs) && hs_ep->isochronous) { if (hs_ep->target_frame != TARGET_FRAME_INITIAL) { - dwc2_gadget_fill_isoc_desc(hs_ep, hs_req->req.dma, + dma_addr_t dma_addr = hs_req->req.dma; + + if (hs_req->req.num_sgs) { + WARN_ON(hs_req->req.num_sgs > 1); + dma_addr = sg_dma_address(hs_req->req.sg); + } + dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr, hs_req->req.length); } return 0; @@ -2054,13 +2095,12 @@ static void dwc2_hsotg_program_zlp(struct dwc2_hsotg *hsotg, dev_dbg(hsotg->dev, "Receiving zero-length packet on ep%d\n", index); if (using_desc_dma(hsotg)) { - /* Not specific buffer needed for ep0 ZLP */ - dma_addr_t dma = hs_ep->desc_list_dma; - if (!index) dwc2_gadget_set_ep0_desc_chain(hsotg, hs_ep); - dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, dma, 0); + /* Not specific buffer needed for ep0 ZLP */ + dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &hs_ep->desc_list, + hs_ep->desc_list_dma, 0, true); } else { dwc2_writel(hsotg, DXEPTSIZ_MC(1) | DXEPTSIZ_PKTCNT(1) | DXEPTSIZ_XFERSIZE(0), @@ -4471,6 +4511,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, hsotg->enabled = 0; spin_unlock_irqrestore(&hsotg->lock, flags); + gadget->sg_supported = using_desc_dma(hsotg); dev_info(hsotg->dev, "bound driver %s\n", driver->driver.name); return 0;