As a process of preparing TRBs usb_gadget_map_request_by_dev() is
called from dwc3_prepare_trbs() for mapping the request. This will
call dma_map_sg() if req->num_sgs are greater than 0. dma_map_sg()
will map the sg entries in sglist and return the number of mapped SGs.
As a part of mapping, some sg entries having contigous memory may be
merged together into a single sg (when IOMMU used). So, the number of
mapped sg entries may not be equal to the number of orginal sg entries
in the request (req->num_sgs).
As a part of preparing the TRBs, dwc3_prepare_one_trb_sg() iterates over
the sg entries present in the sglist and calls sg_is_last() to identify
whether the sg entry is last and set IOC bit for the last sg entry. The
sg_is_last() determines last sg if SG_END is set in sg->page_link. When
IOMMU used, dma_map_sg() merges 2 or more sgs into a single sg and it
doesn't retain the page_link properties. Because of this reason the
sg_is_last() may not find SG_END and thus resulting in IOC bit never
getting set.
For example:
Consider a request having 8 sg entries with each entry having a length of
4096 bytes. Assume that sg1 & sg2, sg3 & sg4, sg5 & sg6, sg7 & sg8 are
having contigous memory regions.
Before calling dma_map_sg():
sg1-->sg2-->sg3-->sg4-->sg6-->sg7-->sg8
dma_length: 4K 4K 4K 4K 4K 4K 4K
SG_END: False False False False False False True
num_sgs = 8
num_mapped_sgs = 0
The dma_map_sg() merges sg1 & sg2 memory regions into sg1->dma_address.
Similarly sg3 & sg4 into sg2->dma_address, sg5 & sg6 into the
sg3->dma_address and sg6 & sg8 into sg4->dma_address. Here the memory
regions are merged but the page_link properties like SG_END are not
retained into the merged sgs.
After calling dma_map_sg();
sg1-->sg2-->sg3-->sg4-->sg6-->sg7-->sg8
dma_length: 8K 8K 8K 8K 0K 0K 0K
SG_END: False False False False False False True
num_sgs = 8
num_mapped_sgs = 4
After calling dma_map_sg(), sg1,sg2,sg3,sg4 are having dma_length of
8096 bytes each and remaining sg4,sg5,sg6,sg7 are having 0 bytes of
dma_length.
After dma_map_sg() is performed dma_perpare_trb_sg() iterates on all sg
entries and sets IOC bit only for the sg8 (since sg_is_last() returns true
only for sg8). But after calling dma_map_sg() the valid data are present
only till sg4 and the IOC bit should be set for sg4 TRB only (which is not
happening in the present code)
The above mentioned issue can be fixed by determining last sg based on the
req->num_queued_sgs instead of sg_is_last(). If (req->num_queued_sgs + 1)
is equal to req->num_mapped_sgs, then this sg is the last sg. In the above
example, the dwc3 driver has already queued 3 sgs (upto sg3), so the
num_queued_sgs = 3. On preparing the next sg (i.e sg4), check for last sg
(num_queued_sgs + 1) == num_mapped_sgs becomes true. So, the driver sets
IOC bit for sg4. This patch does the same.
Signed-off-by: Anurag Kumar Vulisha <[email protected]>
---
drivers/usb/dwc3/gadget.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index d676553..0ab59a6 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1062,7 +1062,7 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
unsigned int rem = length % maxp;
unsigned chain = true;
- if (sg_is_last(s))
+ if ((req->num_queued_sgs + 1) == req->request.num_mapped_sgs)
chain = false;
if (rem && usb_endpoint_dir_out(dep->endpoint.desc) && !chain) {
--
2.1.1
Hi,
Anurag Kumar Vulisha <[email protected]> writes:
> As a process of preparing TRBs usb_gadget_map_request_by_dev() is
> called from dwc3_prepare_trbs() for mapping the request. This will
> call dma_map_sg() if req->num_sgs are greater than 0. dma_map_sg()
> will map the sg entries in sglist and return the number of mapped SGs.
> As a part of mapping, some sg entries having contigous memory may be
> merged together into a single sg (when IOMMU used). So, the number of
> mapped sg entries may not be equal to the number of orginal sg entries
> in the request (req->num_sgs).
>
> As a part of preparing the TRBs, dwc3_prepare_one_trb_sg() iterates over
> the sg entries present in the sglist and calls sg_is_last() to identify
> whether the sg entry is last and set IOC bit for the last sg entry. The
> sg_is_last() determines last sg if SG_END is set in sg->page_link. When
> IOMMU used, dma_map_sg() merges 2 or more sgs into a single sg and it
> doesn't retain the page_link properties. Because of this reason the
> sg_is_last() may not find SG_END and thus resulting in IOC bit never
> getting set.
>
> For example:
>
> Consider a request having 8 sg entries with each entry having a length of
> 4096 bytes. Assume that sg1 & sg2, sg3 & sg4, sg5 & sg6, sg7 & sg8 are
> having contigous memory regions.
>
> Before calling dma_map_sg():
> sg1-->sg2-->sg3-->sg4-->sg6-->sg7-->sg8
> dma_length: 4K 4K 4K 4K 4K 4K 4K
> SG_END: False False False False False False True
> num_sgs = 8
> num_mapped_sgs = 0
>
> The dma_map_sg() merges sg1 & sg2 memory regions into sg1->dma_address.
> Similarly sg3 & sg4 into sg2->dma_address, sg5 & sg6 into the
> sg3->dma_address and sg6 & sg8 into sg4->dma_address. Here the memory
> regions are merged but the page_link properties like SG_END are not
> retained into the merged sgs.
isn't this a bug in the scatterlist mapping code? Why doesn't it keep
SG_END?
--
balbi
Hi Felipe,
>-----Original Message-----
>From: Felipe Balbi [mailto:[email protected]]
>Sent: Wednesday, June 5, 2019 1:34 PM
>To: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman
><[email protected]>
>Cc: [email protected]; [email protected];
>[email protected]; Anurag Kumar Vulisha <[email protected]>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for finding last SG
>entry
>
>
>Hi,
>
>Anurag Kumar Vulisha <[email protected]> writes:
>> As a process of preparing TRBs usb_gadget_map_request_by_dev() is
>> called from dwc3_prepare_trbs() for mapping the request. This will
>> call dma_map_sg() if req->num_sgs are greater than 0. dma_map_sg()
>> will map the sg entries in sglist and return the number of mapped SGs.
>> As a part of mapping, some sg entries having contigous memory may be
>> merged together into a single sg (when IOMMU used). So, the number of
>> mapped sg entries may not be equal to the number of orginal sg entries
>> in the request (req->num_sgs).
>>
>> As a part of preparing the TRBs, dwc3_prepare_one_trb_sg() iterates
>> over the sg entries present in the sglist and calls sg_is_last() to
>> identify whether the sg entry is last and set IOC bit for the last sg
>> entry. The
>> sg_is_last() determines last sg if SG_END is set in sg->page_link.
>> When IOMMU used, dma_map_sg() merges 2 or more sgs into a single sg
>> and it doesn't retain the page_link properties. Because of this reason
>> the
>> sg_is_last() may not find SG_END and thus resulting in IOC bit never
>> getting set.
>>
>> For example:
>>
>> Consider a request having 8 sg entries with each entry having a length
>> of
>> 4096 bytes. Assume that sg1 & sg2, sg3 & sg4, sg5 & sg6, sg7 & sg8 are
>> having contigous memory regions.
>>
>> Before calling dma_map_sg():
>> sg1-->sg2-->sg3-->sg4-->sg6-->sg7-->sg8
>> dma_length: 4K 4K 4K 4K 4K 4K 4K
>> SG_END: False False False False False False True
>> num_sgs = 8
>> num_mapped_sgs = 0
>>
>> The dma_map_sg() merges sg1 & sg2 memory regions into sg1-
>>dma_address.
>> Similarly sg3 & sg4 into sg2->dma_address, sg5 & sg6 into the
>> sg3->dma_address and sg6 & sg8 into sg4->dma_address. Here the
>memory
>> regions are merged but the page_link properties like SG_END are not
>> retained into the merged sgs.
>
>isn't this a bug in the scatterlist mapping code? Why doesn't it keep
>SG_END?
>
Thanks for providing your comment.
I don't think it is a bug, instead I feel some enhancement needs to be done in
dma-mapping code. SG_END represents the last sg entry in the sglist and it is
correctly getting set to the last sg entry. The issue happens only when 2 or more
sg entry pages are merged into contiguous dma-able address and sg_is_last()
is used to find the last sg entry with valid dma address. I think that along with
sg_is_last() a new flag (SG_DMA_END) and function (something like sg_dma_is_last() )
needs to be added into dma-mapping code for identifying the last valid sg entry
with valid dma address. So that we can make use of that function instead of sg_is_last().
Thanks,
Anurag Kumar Vulisha
Hi,
Anurag Kumar Vulisha <[email protected]> writes:
>>> The dma_map_sg() merges sg1 & sg2 memory regions into sg1-
>>>dma_address.
>>> Similarly sg3 & sg4 into sg2->dma_address, sg5 & sg6 into the
>>> sg3->dma_address and sg6 & sg8 into sg4->dma_address. Here the
>>memory
>>> regions are merged but the page_link properties like SG_END are not
>>> retained into the merged sgs.
>>
>>isn't this a bug in the scatterlist mapping code? Why doesn't it keep
>>SG_END?
>>
>
> Thanks for providing your comment.
>
> I don't think it is a bug, instead I feel some enhancement needs to be done in
> dma-mapping code.
>
> SG_END represents the last sg entry in the sglist and it is correctly getting
> set to the last sg entry.
>
> The issue happens only when 2 or more sg entry pages are merged into
> contiguous dma-able address and sg_is_last() is used to find the last sg entry
> with valid dma address.
Right, and that's something that's bound to happen. I'm arguing that, perhaps,
dma API should move SG_END in case entries are merged.
> I think that along with sg_is_last() a new flag (SG_DMA_END) and function
> (something like sg_dma_is_last() ) needs to be added into dma-mapping code for
> identifying the last valid sg entry with valid dma address. So that we can
> make use of that function instead of sg_is_last().
Sure, propose a patch to DMA API.
--
balbi
Hi Anurag,
On Fri, Jun 07, 2019 at 09:49:59AM +0300, Felipe Balbi wrote:
> Anurag Kumar Vulisha <[email protected]> writes:
> >>> The dma_map_sg() merges sg1 & sg2 memory regions into sg1-
> >>>dma_address.
> >>> Similarly sg3 & sg4 into sg2->dma_address, sg5 & sg6 into the
> >>> sg3->dma_address and sg6 & sg8 into sg4->dma_address. Here the
> >>memory
> >>> regions are merged but the page_link properties like SG_END are not
> >>> retained into the merged sgs.
> >>
> >>isn't this a bug in the scatterlist mapping code? Why doesn't it keep
> >>SG_END?
> >>
> >
> > Thanks for providing your comment.
> >
> > I don't think it is a bug, instead I feel some enhancement needs to be done in
> > dma-mapping code.
> >
> > SG_END represents the last sg entry in the sglist and it is correctly getting
> > set to the last sg entry.
> >
> > The issue happens only when 2 or more sg entry pages are merged into
> > contiguous dma-able address and sg_is_last() is used to find the last sg entry
> > with valid dma address.
>
> Right, and that's something that's bound to happen. I'm arguing that, perhaps,
> dma API should move SG_END in case entries are merged.
>
> > I think that along with sg_is_last() a new flag (SG_DMA_END) and function
> > (something like sg_dma_is_last() ) needs to be added into dma-mapping code for
> > identifying the last valid sg entry with valid dma address. So that we can
> > make use of that function instead of sg_is_last().
>
> Sure, propose a patch to DMA API.
I'm curious if this was ever resolved. I just ran into this exact issue
with Android ADB which uses 16KB buffers, along with f_fs supporting
S/G since 5.0, combined with our IOMMU which performs this merging
behavior, so it resulted in a single TRB getting queued with CHN=1 and
LST=0 and thus the transfer never completes. Your initial patch resolves
the issue for me, but upon revisiting this discussion I couldn't tell if
you had attempted to patch DMA API instead as per Felipe's suggestion.
Thanks,
Jack
--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Hi Jack,
>-----Original Message-----
>From: Jack Pham [mailto:[email protected]]
>Sent: Wednesday, October 23, 2019 10:28 PM
>To: Felipe Balbi <[email protected]>
>Cc: Anurag Kumar Vulisha <[email protected]>; Greg Kroah-Hartman
><[email protected]>; [email protected]; linux-
>[email protected]; [email protected]
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for finding last SG
>entry
>
>Hi Anurag,
>
>On Fri, Jun 07, 2019 at 09:49:59AM +0300, Felipe Balbi wrote:
>> Anurag Kumar Vulisha <[email protected]> writes:
>> >>> The dma_map_sg() merges sg1 & sg2 memory regions into sg1-
>> >>>dma_address.
>> >>> Similarly sg3 & sg4 into sg2->dma_address, sg5 & sg6 into the
>> >>> sg3->dma_address and sg6 & sg8 into sg4->dma_address. Here the
>> >>memory
>> >>> regions are merged but the page_link properties like SG_END are not
>> >>> retained into the merged sgs.
>> >>
>> >>isn't this a bug in the scatterlist mapping code? Why doesn't it keep
>> >>SG_END?
>> >>
>> >
>> > Thanks for providing your comment.
>> >
>> > I don't think it is a bug, instead I feel some enhancement needs to be done
>in
>> > dma-mapping code.
>> >
>> > SG_END represents the last sg entry in the sglist and it is correctly getting
>> > set to the last sg entry.
>> >
>> > The issue happens only when 2 or more sg entry pages are merged into
>> > contiguous dma-able address and sg_is_last() is used to find the last sg
>entry
>> > with valid dma address.
>>
>> Right, and that's something that's bound to happen. I'm arguing that,
>perhaps,
>> dma API should move SG_END in case entries are merged.
>>
>> > I think that along with sg_is_last() a new flag (SG_DMA_END) and function
>> > (something like sg_dma_is_last() ) needs to be added into dma-mapping
>code for
>> > identifying the last valid sg entry with valid dma address. So that we can
>> > make use of that function instead of sg_is_last().
>>
>> Sure, propose a patch to DMA API.
>
>I'm curious if this was ever resolved. I just ran into this exact issue
>with Android ADB which uses 16KB buffers, along with f_fs supporting
>S/G since 5.0, combined with our IOMMU which performs this merging
>behavior, so it resulted in a single TRB getting queued with CHN=1 and
>LST=0 and thus the transfer never completes. Your initial patch resolves
>the issue for me, but upon revisiting this discussion I couldn't tell if
>you had attempted to patch DMA API instead as per Felipe's suggestion.
>
We were stuck with some internal priority tasks, so couldn't initiate the
discussion with IOMMU maintainers. Will work on preparing the patch
and sending the same very soon.
Thanks,
Anurag Kumar Vulisha
>Thanks,
>Jack
>--
>The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>a Linux Foundation Collaborative Project