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:     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: AQHUNiVThZEOCvrQMkaSQCCLSEec7aTjevBw
Date:   Thu, 6 Sep 2018 15:12:39 +0000
Message-ID: <BN3PR0201MB09932A2E75DAA9A1B06DDB3AA7010@BN3PR0201MB0993.namprd02.prod.outlook.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>
In-Reply-To: <30102591E157244384E984126FC3CB4F544ACFED@us01wembx1.internal.synopsys.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: 697bfe7c-67b0-458f-fdeb-08d6140b2f8e
X-MS-Exchange-CrossTenant-originalarrivaltime: 06 Sep 2018 15:12:39.3108
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR0201MB1011
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 Thinh,

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

Thanks for pointing out a solution , the fix looks good. I will test with t=
his fix and resend the patches

Best Regards,
Anurag Kumar Vulisha