Hi Felipe, >-----Original Message----- >From: Felipe Balbi [mailto:balbi@xxxxxxxxxx] >Sent: Wednesday, June 5, 2019 1:34 PM >To: Anurag Kumar Vulisha <anuragku@xxxxxxxxxx>; Greg Kroah-Hartman ><gregkh@xxxxxxxxxxxxxxxxxxx> >Cc: linux-usb@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; >v.anuragkumar@xxxxxxxxx; Anurag Kumar Vulisha <anuragku@xxxxxxxxxx> >Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for finding last SG >entry > > >Hi, > >Anurag Kumar Vulisha <anurag.kumar.vulisha@xxxxxxxxxx> 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
