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+38v6PXQBOAgAAsIoA=
Date:   Mon, 19 Mar 2018 14:12:59 +0000
Message-ID: <MWHPR02MB2816D32AE891F329232DA730A7D40@MWHPR02MB2816.namprd02.prod.outlook.com>
References: <1521442253-15906-1-git-send-email-anuragku@xilinx.com>
 <87fu4wqwis.fsf@linux.intel.com>
In-Reply-To: <87fu4wqwis.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: 68bd94a4-b8f6-48bb-970e-08d58da384e9
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Mar 2018 14:12:59.1254
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR02MB2511
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 reviewing the patch , please find my comments inline

>-----Original Message-----
>From: Felipe Balbi [mailto:balbi@kernel.org]
>Sent: Monday, March 19, 2018 2:21 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; Anurag Kumar Vulisha <anuragku@xilinx.com>
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>Hi,
>
>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 sglist.
>
>these two should be in separate patches, then.
>
Will create separate patches in next version

>> 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 decreme=
nted in dwc3_cleanup_done_reqs(). Consider a case where the driver failed t=
o 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_tr=
ansfer() 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 fl=
ag num_queued_sgs which counts the number of sgs that got queued successful=
ly and make  for_each_sg() loop for num_mapped_sgs - num_queued_sgs times o=
nly . 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.

>
>>   * @remaining: amount of data remaining
>>   * @epnum: endpoint number to which this request refers
>>   * @trb: pointer to struct dwc3_trb
>> @@ -734,8 +736,10 @@ struct dwc3_request {
>>         struct list_head        list;
>>         struct dwc3_ep          *dep;
>>         struct scatterlist      *sg;
>> +       struct scatterlist      *sg_to_start;
>
>indeed, we seem to need something like this.
>
>>         unsigned                num_pending_sgs;
>> +       unsigned int            num_queued_sgs;
>
>this should be unnecessary.

The explanation given above is valid for this comment also. Please refer th=
e explanation given above

>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 2bda4eb..1cffed5 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -978,11 +978,20 @@ static void dwc3_prepare_one_trb(struct dwc3_ep
>*dep,
>>                 struct dwc3_request *req, unsigned chain, unsigned
>> node)  {
>>         struct dwc3_trb         *trb;
>> -       unsigned                length =3D req->request.length;
>> +       unsigned int            length;
>> +       dma_addr_t              dma;
>>         unsigned                stream_id =3D req->request.stream_id;
>>         unsigned                short_not_ok =3D req->request.short_not_=
ok;
>>         unsigned                no_interrupt =3D req->request.no_interru=
pt;
>> -       dma_addr_t              dma =3D req->request.dma;
>> +
>> +       if (req->request.num_sgs > 0) {
>> +               /* Use scattergather list address and length */
>
>unnecessary comment

Will remove the comment

>
>> +               length =3D sg_dma_len(req->sg_to_start);
>> +               dma =3D sg_dma_address(req->sg_to_start);
>> +       } else {
>> +               length =3D req->request.length;
>> +               dma =3D req->request.dma;
>> +       }
>>
>>         trb =3D &dep->trb_pool[dep->trb_enqueue];
>>
>> @@ -1048,11 +1057,14 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep
>> *dep)  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
>>                 struct dwc3_request *req)  {
>> -       struct scatterlist *sg =3D req->sg;
>> +       struct scatterlist *sg =3D req->sg_to_start;
>>         struct scatterlist *s;
>>         int             i;
>>
>> -       for_each_sg(sg, s, req->num_pending_sgs, i) {
>> +       unsigned int remaining =3D req->request.num_mapped_sgs
>> +               - req->num_queued_sgs;
>
>already tracked as num_pending_sgs

The explanation given above is valid for this comment also. Please refer th=
e above mentioned explanation.=20
>
>> +       for_each_sg(sg, s, remaining, i) {
>>                 unsigned int length =3D req->request.length;
>>                 unsigned int maxp =3D usb_endpoint_maxp(dep->endpoint.de=
sc);
>>                 unsigned int rem =3D length % maxp; @@ -1081,6 +1093,16
>> @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
>>                         dwc3_prepare_one_trb(dep, req, chain, i);
>>                 }
>>
>> +               /* In the case where not able to queue trbs for all
>> + sgs in
>
>wrong comment style
>

Will fix this in next version of patch

>> +                * request because of trb not available, update sg_to_st=
art
>> +                * to next sg from which we can start queing trbs once t=
rbs
>> +                * availbale
>                     ^^^^^^^^^
>                     available
>
>sg_to_start is too long and awkward to type. Sure, I'm nitpicking, but sta=
rt_sg
>would be better.

I agree with you, start_sg looks better. Will fix this in next series of pa=
tch
=20
>
>> +                */
>> +               if (chain)
>> +                       req->sg_to_start =3D sg_next(s);
>> +
>> +               req->num_queued_sgs++;
>> +
>>                 if (!dwc3_calc_trbs_left(dep))
>>                         break;
>>         }
>> @@ -1171,6 +1193,8 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>>                         return;
>>
>>                 req->sg                 =3D req->request.sg;
>> +               req->sg_to_start        =3D req->sg;
>> +               req->num_queued_sgs     =3D 0;
>
>num_queued_sgs is unnecessary.

As said in previous explanation, we use num_queued_sgs to  correctly mainta=
in the incomplete sgs. So, clearing it to 0.

>
>>                 req->num_pending_sgs    =3D req->request.num_mapped_sgs;
>>
>>                 if (req->num_pending_sgs > 0) @@ -2327,8 +2351,14 @@
>> 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_s=
gs)
>> -                       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 lengt=
h we're
>done, there's nothing more left to do. If there is either host sent more d=
ata 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 want 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->r=
emaining field represents the size of TRB which was not transferred. In thi=
s example , as 3 sgs got completed successfully the req-> remaining =3D 0. =
So , request.actual =3D length - 0 (req->remaining)  which means request.ac=
tual =3D=3D length.  Because of this , the condition check  if ((req->reque=
st.actual < length) && req->num_pending_sgs) ) fails even though we have re=
q->num_pending_sgs > 0.  So, corrected the logic to always kick transfer fo=
r two conditions 1) if req->num_pending_sgs > 0  2) if ((req->request.actua=
l < length) && req->num_pending_sgs)) condition satisfies. Please correct m=
e If my understanding is wrong.

>> +                       /* There could be cases where the whole req
>> + can't be
>
>wrong comment style.
>
Will correct this in next series of patches

>> +                        * mapped into TRB's available. In that case, we=
 need
>> +                        * to kick transfer again if (req->num_pending_s=
gs > 0)
>> +                        */
>
>also, the code has already been trying to do that. It just wasn't correct.=
 We don't
>need to add this comment.

Added the comment to describe what is happening here. Please let me know if=
 any more information needs to be added or deleted.

>
>> +                       if (req->num_pending_sgs)
>> +                               return
>> + __dwc3_gadget_kick_transfer(dep);
>
>another num_pending_sgs check? Why?

As explained in the previous comment, changed the logic to make it work for=
 two conditions 1) if req->num_pending_sgs > 0  2) if ((req->request.actual=
 < length) && req->num_pending_sgs)) .=20

>
>> This email and any attachments are intended for the sole use of the
>> named recipient(s) and contain(s) confidential information that may be
>> proprietary, privileged or copyrighted under applicable law. If you
>> are not the intended recipient, do not read, copy, or forward this
>> email message or any attachments. Delete this email message and any
>> attachments immediately.
>
>I can't accept ANY patches from you until you remove this footer. In fact,=
 I'm not
>in the To field, so I'm not a "named recipient" and therefore, I'm deletin=
g your
>email. Talk to your IT department about contributing to public mailing lis=
ts.
>
Sorry for the inconvenience caused. Unknowingly , this message got added. I=
 will ensure that this message won't appear in future emails.

Thanks,
Anurag Kumar Vulisha

>Email, now, deleted.
>
>--
>balbi