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:     "v.anuragkumar@gmail.com" <v.anuragkumar@gmail.com>,
        "Ajay Yugalkishore Pandey" <APANDEY@xilinx.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
Thread-Topic: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
Thread-Index: AQHTv07Hj2BDwUGZJ0CZRNZYK+38v6PXQBOAgAAsIoCABkl9AIAB+xVg
Date:   Sat, 24 Mar 2018 17:53:50 +0000
Message-ID: <MWHPR02MB281608B4FB50414005A5CEEAA7AF0@MWHPR02MB2816.namprd02.prod.outlook.com>
References: <1521442253-15906-1-git-send-email-anuragku@xilinx.com>
 <87fu4wqwis.fsf@linux.intel.com>
 <MWHPR02MB2816D32AE891F329232DA730A7D40@MWHPR02MB2816.namprd02.prod.outlook.com>
 <87lgejni7v.fsf@linux.intel.com>
In-Reply-To: <87lgejni7v.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)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: f2f554dd-801f-4a1f-f2a5-08d591b033cc
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Mar 2018 17:53:51.0162
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR02MB3230
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,

Thanks for providing your inputs on this patch. Will send v2 with all your
suggestions added.

Thanks,
Anurag Kumar Vulisha

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Friday, March 23, 2018 4:59 PM
>To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Greg Kroah-Hartman
><gregkh@linuxfoundation.org>
>Cc: v.anuragkumar@gmail.com; Ajay Yugalkishore Pandey
><APANDEY@xilinx.com>; linux-usb@vger.kernel.org; linux-
>kernel@vger.kernel.org
>Subject: RE: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>(please configure your email client to break lines at 80 columns ;-)
>
>Hi,
>
>Anurag Kumar Vulisha <anuragku@xilinx.com> writes:
>> Hi Felipe,
>>
>> Thanks for reviewing the patch , please find my comments inline
>
>no issues :-)
>
>>>Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com> writes:
>>>> This patch fixes two issues
>>>>
>>>> 1. The code logic in dwc3_prepare_one_trb() incorrectly uses the
>>>> address and length given in req packet even for scattergather lists.
>>>> This patch correct's the code to use sg->address and sg->length when
>>>> scattergather lists are present.
>>>>
>>>> 2. The present code correctly fetches the req's which were not
>>>> queued from the started_list but fails to start from the sg where it
>>>> previously stopped queuing because of the unavailable TRB's. This
>>>> patch correct's the code to start queuing from the correct sg in sglis=
t.
>>>
>>>these two should be in separate patches, then.
>>>
>> Will create separate patches in next version
>
>thanks, that helps :-)
>
>>>> Signed-off-by: Anurag Kumar Vulisha <anuragku@xilinx.com>
>>>> ---
>>>>  drivers/usb/dwc3/core.h   |  4 ++++
>>>>  drivers/usb/dwc3/gadget.c | 42
>>>> ++++++++++++++++++++++++++++++++++++------
>>>>  2 files changed, 40 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
>>>> 860d2bc..2779e58 100644
>>>> --- a/drivers/usb/dwc3/core.h
>>>> +++ b/drivers/usb/dwc3/core.h
>>>> @@ -718,7 +718,9 @@ struct dwc3_hwparams {
>>>>   * @list: a list_head used for request queueing
>>>>   * @dep: struct dwc3_ep owning this request
>>>>   * @sg: pointer to first incomplete sg
>>>> + * @sg_to_start: pointer to the sg which should be queued next
>>>>   * @num_pending_sgs: counter to pending sgs
>>>> + * @num_queued_sgs: counter to the number of sgs which already got
>>>> + queued
>>>
>>>this is the same as num_pending_sgs.
>>
>> num_pending_sgs is initially pointing to num_mapped_sgs, which gets
>> decremented in dwc3_cleanup_done_reqs().
>>
>> Consider a case where the driver failed to queue all sgs into TRBs
>> because of insufficient TRB number. For example, lets assume req has 5
>> sgs and only 3 got queued. In this scenario ,when the
>> dwc3_cleanup_done_reqs() gets called the value of num_pending_sgs =3D 5.
>> Since the value of num_pending_sgs is greater than 0,
>> __dwc3_gadget_kick_transfer() gets called with num_pending_sgs =3D 5-1 =
=3D
>> 4.
>>
>> Eventually __dwc3_gadget_kick_transfer() calls
>> dwc3_prepare_one_trb_sg() which has for_each_sg() call which loops for
>> num_pending_sgs times (4 times in our example). This is incorrect,
>> ideally it should be called only for 2 times because we have only 2
>> sgs which previously were not queued.
>>
>> So, we added an extra flag num_queued_sgs which counts the number of
>> sgs that got queued successfully and make for_each_sg() loop for
>> num_mapped_sgs - num_queued_sgs times only . In our example case with
>> the updated logic, it will loop for 5 - 3 =3D 2 times only. Because of
>> this reason added num_queued_sgs flag. Please correct me if I am
>> wrong.
>
>That's true. Seems like we do need a new counter.
>
>>>> static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep
>>>> *dep,
>>>>
>>>>                 req->request.actual =3D length - req->remaining;
>>>>
>>>> -               if ((req->request.actual < length) && req->num_pending=
_sgs)
>>>> -                       return __dwc3_gadget_kick_transfer(dep);
>>>> +               if (req->request.actual < length ||
>>>> + req->num_pending_sgs) {
>>>
>>>why do you think this needs to be || instead of &&? If actual =3D=3D
>>>length we're done, there's nothing more left to do. If there is either
>>>host sent more data than it should, or we miscalculated
>>>num_pending_sgs, or we had the wrong length somewhere in some TRBs.
>>>Either of those cases is an error condition we don't want to hide. We wa=
nt
>things to fail in that case.
>>>
>>
>> Consider the same example that we had previously discussed, among the
>> 5 sgs only 3 sgs got queued and all 3 sgs got completed successfully.
>> The req->remaining field represents the size of TRB which was not
>> transferred. In this example , as 3 sgs got completed successfully the
>> req-> remaining =3D 0. So , request.actual =3D length - 0
>> (req->remaining) which means request.actual =3D=3D length.  Because of
>> this , the condition check if ((req->request.actual < length) &&
>> req->num_pending_sgs) ) fails even though we have req->num_pending_sgs
>> > 0.  So, corrected the logic to always kick transfer for two
>> conditions 1) if req->num_pending_sgs > 0 2) if ((req->request.actual
>> < length) && req->num_pending_sgs)) condition satisfies. Please
>> correct me If my understanding is wrong.
>
>fair enough, but then we miss an important (IMO, that is) error case. If
>req->num_pending_sgs > 0 && actual =3D=3D length, then something is super
>wrong. We may just add it as an extra check:
>
>	dev_WARN_ONCE(.... actual =3D=3D len && num_pending_sgs);
>
>--
>balbi