Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Anurag Kumar Vulisha <anuragku@xilinx.com>
To:     Felipe Balbi <balbi@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>
CC:     "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "v.anuragkumar@gmail.com" <v.anuragkumar@gmail.com>
Subject: RE: [PATCH] usb: dwc3: gadget: Correct the logic for finding last SG
 entry
Thread-Topic: [PATCH] usb: dwc3: gadget: Correct the logic for finding last SG
 entry
Thread-Index: AQHVFi8wapUkz2GgO0KPkbPlYU1PoKaMvgMAgAH+Z2A=
Date:   Thu, 6 Jun 2019 15:32:26 +0000
Message-ID: <BYAPR02MB559181C009B74446A797838DA7170@BYAPR02MB5591.namprd02.prod.outlook.com>
References: <1559141985-17104-1-git-send-email-anurag.kumar.vulisha@xilinx.com>
 <87y32gcvc1.fsf@linux.intel.com>
In-Reply-To: <87y32gcvc1.fsf@linux.intel.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: xilinx.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: d1100a4a-327e-474c-00c8-08d6ea942e4a
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Jun 2019 15:32:27.0406
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: anuragku@xilinx.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR02MB4582
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Felipe,

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Wednesday, June 5, 2019 1:34 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>
>Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org;
>v.anuragkumar@gmail.com; Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for finding last=
 SG
>entry
>
>
>Hi,
>
>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> 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 =3D 8
>> num_mapped_sgs =3D 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 o=
r 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 alon=
g 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 e=
ntry
with valid dma address. So that we can make use of that function instead of=
 sg_is_last().

Thanks,
Anurag Kumar Vulisha