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:   Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To:     Anurag Kumar Vulisha <anuragku@xilinx.com>,
        Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        "balbi@kernel.org" <balbi@kernel.org>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
CC:     "v.anuragkumar@gmail.com" <v.anuragkumar@gmail.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB
 full in __dwc3_prepare_one_trb()
Thread-Topic: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB
 full in __dwc3_prepare_one_trb()
Thread-Index: AQHUNiVe6PVhPxbQeEm3tnXrG8X9Mg==
Date:   Fri, 7 Sep 2018 02:00:58 +0000
Message-ID: <30102591E157244384E984126FC3CB4F544AD412@us01wembx1.internal.synopsys.com>
References: <1534508695-12642-1-git-send-email-anurag.kumar.vulisha@xilinx.com>
 <1534508695-12642-2-git-send-email-anurag.kumar.vulisha@xilinx.com>
 <30102591E157244384E984126FC3CB4F544AB897@us01wembx1.internal.synopsys.com>
 <BN3PR0201MB0993325C7125DB1A084BF9BBA7020@BN3PR0201MB0993.namprd02.prod.outlook.com>
 <30102591E157244384E984126FC3CB4F544ACFED@us01wembx1.internal.synopsys.com>
 <BN3PR0201MB09932A2E75DAA9A1B06DDB3AA7010@BN3PR0201MB0993.namprd02.prod.outlook.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Anurag,=0A=
=0A=
On 9/6/2018 8:12 AM, Anurag Kumar Vulisha wrote:=0A=
> Hi Thinh,=0A=
>=0A=
>> -----Original Message-----=0A=
>> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]=0A=
>> Sent: Thursday, September 06, 2018 7:28 AM=0A=
>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; Thinh Nguyen=0A=
>> <Thinh.Nguyen@synopsys.com>; balbi@kernel.org; gregkh@linuxfoundation.or=
g=0A=
>> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux-=0A=
>> kernel@vger.kernel.org=0A=
>> Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TR=
B full in=0A=
>> __dwc3_prepare_one_trb()=0A=
>>=0A=
>> Hi,=0A=
>>=0A=
>> On 9/5/2018 2:19 AM, Anurag Kumar Vulisha wrote:=0A=
>>> Hi Thinh,=0A=
>>>=0A=
>>> Thanks for spending your time in reviewing this code, please find my=0A=
>>> comments inline=0A=
>>>=0A=
>>>> -----Original Message-----=0A=
>>>> From: Thinh Nguyen [mailto:Thinh.Nguyen@synopsys.com]=0A=
>>>> Sent: Wednesday, September 05, 2018 11:04 AM=0A=
>>>> To: Anurag Kumar Vulisha <anuragku@xilinx.com>; balbi@kernel.org;=0A=
>>>> gregkh@linuxfoundation.org=0A=
>>>> Cc: v.anuragkumar@gmail.com; linux-usb@vger.kernel.org; linux-=0A=
>>>> kernel@vger.kernel.org=0A=
>>>> Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking=
=0A=
>>>> TRB full in=0A=
>>>> __dwc3_prepare_one_trb()=0A=
>>>>=0A=
>>>> Hi Anurag,=0A=
>>>>=0A=
>>>>> +	trb->ctrl |=3D DWC3_TRB_CTRL_HWO;=0A=
>>>>> +=0A=
>>>>>  	if ((!no_interrupt && !chain) ||=0A=
>>>>>  			(dwc3_calc_trbs_left(dep) =3D=3D 0))=0A=
>>>>>  		trb->ctrl |=3D DWC3_TRB_CTRL_IOC;=0A=
>>>>> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct=0A=
>>>>> dwc3_ep=0A=
>>>> *dep, struct dwc3_trb *trb,=0A=
>>>>>  	if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capab=
le)=0A=
>>>>>  		trb->ctrl |=3D DWC3_TRB_CTRL_SID_SOFN(stream_id);=0A=
>>>>>=0A=
>>>>> -	trb->ctrl |=3D DWC3_TRB_CTRL_HWO;=0A=
>>>>> -=0A=
>>>>>  	trace_dwc3_prepare_trb(dep, trb);=0A=
>>>>>  }=0A=
>>>>>=0A=
>>>> How do you reproduce this issue? We should not set HWO before setting=
=0A=
>>>> other trb->ctrl bits. Can you provide a driver tracepoint? If there's=
=0A=
>>>> an issue with the check if ((!no_interrupt && !chain) ||=0A=
>>>> dwc3_calc_trbs_left =3D=3D 0), then we may need to fix the check there=
.=0A=
>>>>=0A=
>>> This issue gets triggered very rarely on long runs when dep->trb_enqueu=
e =3D=3D dep-=0A=
>>> trb_dequeue.=0A=
>>> In __dwc3_prepare_one_trb() , IOC bit is set when no TRBs are=0A=
>>> available, so that a complete event can be generated and TRBs can be=0A=
>>> cleaned  after complete . Dwc3_calc_trbs_left() is called to determine=
=0A=
>>> the available TRBs, which depends on the previous TRB's HWO bit set=0A=
>>> when=0A=
>>> dep->trb_enqueue =3D=3D dep->trb_dequeue. There are chances where the=
=0A=
>>> dep->dwc3_calc_trbs_left() wrongly=0A=
>>> returns DWC3_TRB_NUM -1 instead of 0 , even though there are no=0A=
>>> available TRBs. Please consider the below example=0A=
>>>=0A=
>>> 1.  Consider a TRB passed to __dwc3_prepare_one_trb() is the last avail=
able TRB in=0A=
>> the pool.=0A=
>>> 2.  __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments t=
he dep-=0A=
>>> trb_enqueue=0A=
>>>    before preparing the TRB and since the current TRB is the last avail=
able,=0A=
>> incrementing=0A=
>>>   dep->enqueue will make dep->enqueue =3D=3D dep->dequeue 3. IOC bit is=
=0A=
>>> set by __dwc3_prepare_one_trb()  only when dwc3_calc_trbs_left()=0A=
>>> returns 0 (empty TRBs) 4. Since dep->enqueue =3D=3D dep->dequeue and th=
e previous=0A=
>> TRB(the one which we are working)=0A=
>>>     doesn't yet has the HWO bit set, dwc3_calc_trbs_left() returns DWC3=
_TRB_NUM=0A=
>> -1 instead of=0A=
>>>     zero (Though there are no available TRBs) 5. Since=0A=
>>> Dwc3_calc_trbs_left() returns non-zero value, IOC bit is not set in=0A=
>> __dwc3_prepare_one_trb()=0A=
>>>    for the last TRB and no complete event is generated. Because of this=
 no further=0A=
>> TRBs are queued.=0A=
>>> To avoid the above mentioned issue, I moved the code logic for setting =
HWO bit=0A=
>> before setting IOC bit.=0A=
>>> I agree that HWO bit should always be set at the last, but I didn't fin=
d any better=0A=
>> logic for fixing this.=0A=
>>> Please suggest if any better way to handle this situation.=0A=
>>>=0A=
>> I haven't tested it, but you can try this:=0A=
>>=0A=
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c index=
=0A=
>> d7a327eaee12..37171d46390b 100644=0A=
>> --- a/drivers/usb/dwc3/gadget.c=0A=
>> +++ b/drivers/usb/dwc3/gadget.c=0A=
>> @@ -924,8 +924,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *d=
ep,=0A=
>> struct dwc3_trb *trb,=0A=
>>        struct usb_gadget       *gadget =3D &dwc->gadget;=0A=
>>        enum usb_device_speed   speed =3D gadget->speed;=0A=
>>=0A=
>> -       dwc3_ep_inc_enq(dep);=0A=
>> -=0A=
>>        trb->size =3D DWC3_TRB_SIZE_LENGTH(length);=0A=
>>        trb->bpl =3D lower_32_bits(dma);=0A=
>>        trb->bph =3D upper_32_bits(dma);=0A=
>> @@ -1004,7 +1002,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep =
*dep,=0A=
>> struct dwc3_trb *trb,=0A=
>>        }=0A=
>>=0A=
>>        if ((!no_interrupt && !chain) ||=0A=
>> -                       (dwc3_calc_trbs_left(dep) =3D=3D 0))=0A=
>> +                       (dwc3_calc_trbs_left(dep) =3D=3D 1))=0A=
>>                trb->ctrl |=3D DWC3_TRB_CTRL_IOC;=0A=
>>=0A=
>>        if (chain)=0A=
>> @@ -1013,6 +1011,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep =
*dep,=0A=
>> struct dwc3_trb *trb,=0A=
>>        if (usb_endpoint_xfer_bulk(dep->endpoint.desc) &&=0A=
>> dep->stream_capable)=0A=
>>                trb->ctrl |=3D DWC3_TRB_CTRL_SID_SOFN(stream_id);=0A=
>>=0A=
>> +       dwc3_ep_inc_enq(dep);=0A=
>> +=0A=
>>        trb->ctrl |=3D DWC3_TRB_CTRL_HWO;=0A=
>>=0A=
>>        trace_dwc3_prepare_trb(dep, trb);=0A=
>>=0A=
> Thanks for pointing out a solution , the fix looks good. I will test with=
 this fix and resend the patches=0A=
>=0A=
> Best Regards,=0A=
> Anurag Kumar Vulisha=0A=
>=0A=
>=0A=
=0A=
The rest of the patch series looks fine to me.=0A=
Reviewed-by: Thinh Nguyen <thinhn@synopsys.com>=0A=
=0A=
BR,=0A=
Thinh=0A=