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: AQHUNiVThZEOCvrQMkaSQCCLSEec7aThZIpg
Date:   Wed, 5 Sep 2018 09:19:40 +0000
Message-ID: <BN3PR0201MB0993325C7125DB1A084BF9BBA7020@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>
In-Reply-To: <30102591E157244384E984126FC3CB4F544AB897@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: 6ce9e2e9-1601-4560-fe7e-08d61310b579
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Sep 2018 09:19:40.2517
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN3PR0201MB0932
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,

Thanks for spending your time in reviewing this code, please find my commen=
ts 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,
>
>On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote:
>> Availability of TRB's are calculated using dwc3_calc_trbs_left(), which
>> determines available TRB's based on the HWO bit set in a TRB.
>>
>> __dwc3_prepare_one_trb() is called with a TRB which needs to be prepared
>> for transfer. This __dwc3_prepare_one_trb() calls dwc3_calc_trbs_left()
>> to determine total available TRBs and set IOC bit if the total available
>> TRBs are zero. Since the present working TRB(which is passed as an
>> argument to __dwc3_prepare_one_trb() )  doesn't have HWO bit already set=
,
>> there are chances where dwc3_calc_trbs_left() wrongly calculates this
>> present working TRB as free(since the HWO bit is not yet set). This coul=
d
>> be a problem. This patch correct this issue by setting HWO bit before
>> calling dwc3_calc_trbs_left()
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@xilinx.com>
>> ---
>>  Changes in v2:
>> 	1. Changed the commit message
>> ---
>>  drivers/usb/dwc3/gadget.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 69bf137..f73d219 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *d=
ep,
>struct dwc3_trb *trb,
>>  			trb->ctrl |=3D DWC3_TRB_CTRL_ISP_IMI;
>>  	}
>>
>> +	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_capable)
>>  		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=20
to determine the available TRBs, which depends on the previous TRB's HWO bi=
t set when=20
dep->trb_enqueue =3D=3D dep->trb_dequeue. There are chances where the dwc3_=
calc_trbs_left() wrongly
returns DWC3_TRB_NUM -1 instead of 0 , even though there are no available T=
RBs. Please
consider the below example

1.  Consider a TRB passed to __dwc3_prepare_one_trb() is the last available=
 TRB in the pool.=20
2.  __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the d=
ep->trb_enqueue
   before preparing the TRB and since the current TRB is the last available=
, incrementing
  dep->enqueue will make dep->enqueue =3D=3D dep->dequeue=20
3. IOC bit is set by __dwc3_prepare_one_trb()  only when dwc3_calc_trbs_lef=
t()  returns 0 (empty TRBs)
4. Since dep->enqueue =3D=3D dep->dequeue and the previous TRB(the one whic=
h 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 HWO =
bit before setting IOC bit.
I agree that HWO bit should always be set at the last, but I didn't find an=
y better logic for fixing this.
Please suggest if any better way to handle this situation.  =20

Thanks,
Anurag Kumar Vulisha