[v3] usb: dwc2: gadget: Add scatter-gather mode

Message ID 20190121134447.13944-1-m.szyprowski@samsung.com
State New
Headers show
Series
  • [v3] usb: dwc2: gadget: Add scatter-gather mode
Related show

Commit Message

Marek Szyprowski Jan. 21, 2019, 1:44 p.m.
From: Andrzej Pietrasiewicz <andrzej.p@samsung.com>


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 no longer works at Samsung, so I decided to continue this work.

Here are some implementation details, which helps to understand the
proposed changes.

Non-isochronous transfers:
The code for filling a single DDMA entry is extracted from
dwc2_gadget_config_nonisoc_xfer_ddma() as
dwc2_gadget_fill_nonisoc_xfer_ddma_one() function. The original function
now either calls the extracted code (in case of physically contiguous
request buffer) or iterates over scatterlist elements, for each of them
calling the mentioned code.

Isochronous transfers:
If the usb request contains a scatterlist, the address from the first
element is used as dma address at dwc2_gadget_fill_isoc_desc() invocations.
Current code for isoc transfers does not allow more than 4096 bytes per
transfer, so it is assumed there is only one element in the scatterlist.
If there are more, a warning is issued. Please see the fragment under
the comment:

/* In DDMA mode for ISOC's don't queue request if length greater
 * than descriptor limits.
 */

in dwc2_hsotg_ep_queue() to see the limits for isoc transfers.
---
Changelog:
v2..v3:
	- fixed null pointer issue when processing ep0 zlp
	- renamed _dwc2_gadget_config_nonisoc_xfer_ddma() to
	  dwc2_gadget_fill_nonisoc_xfer_ddma_one()
	- rewrote commit message to explain the reason for
	  the proposed changes

v1..v2:
	- using_desc_dma() called to check if DDMA available
---
 drivers/usb/dwc2/gadget.c | 113 ++++++++++++++++++++++++++------------
 1 file changed, 77 insertions(+), 36 deletions(-)

-- 
2.17.1

Comments

Andrzej Pietrasiewicz March 12, 2019, 11:39 a.m. | #1
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
Minas Harutyunyan March 12, 2019, 1:23 p.m. | #2
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
Andrzej Pietrasiewicz March 15, 2019, 11:27 a.m. | #3
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.
Andrzej Pietrasiewicz March 15, 2019, 11:31 a.m. | #4
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.
Minas Harutyunyan March 18, 2019, 10:32 a.m. | #5
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
Marek Szyprowski March 29, 2019, 3:17 p.m. | #6
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
Minas Harutyunyan April 1, 2019, 10:01 a.m. | #7
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;
>
Andrzej Pietrasiewicz April 1, 2019, 10:49 a.m. | #8
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
Minas Harutyunyan April 1, 2019, 11:33 a.m. | #9
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),
>
Minas Harutyunyan April 30, 2019, 6:39 a.m. | #10
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

Patch

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;