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()
Date: Thu, 6 Sep 2018 15:12:39 +0000 [thread overview]
Message-ID: <BN3PR0201MB09932A2E75DAA9A1B06DDB3AA7010@BN3PR0201MB0993.namprd02.prod.outlook.com> (raw)
In-Reply-To: <30102591E157244384E984126FC3CB4F544ACFED@us01wembx1.internal.synopsys.com>
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 |= DWC3_TRB_CTRL_HWO;
>>>> +
>>>> if ((!no_interrupt && !chain) ||
>>>> (dwc3_calc_trbs_left(dep) == 0))
>>>> trb->ctrl |= 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 |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>>>
>>>> - trb->ctrl |= 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 == 0), then we may need to fix the check there.
>>>
>> This issue gets triggered very rarely on long runs when dep->trb_enqueue == 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 == 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 available TRB in
>the pool.
>> 2. __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the dep-
>>trb_enqueue
>> before preparing the TRB and since the current TRB is the last available,
>incrementing
>> dep->enqueue will make dep->enqueue == 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 == 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 HWO 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 = &dwc->gadget;
> enum usb_device_speed speed = gadget->speed;
>
>- dwc3_ep_inc_enq(dep);
>-
> trb->size = DWC3_TRB_SIZE_LENGTH(length);
> trb->bpl = lower_32_bits(dma);
> trb->bph = upper_32_bits(dma);
>@@ -1004,7 +1002,7 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep,
>struct dwc3_trb *trb,
> }
>
> if ((!no_interrupt && !chain) ||
>- (dwc3_calc_trbs_left(dep) == 0))
>+ (dwc3_calc_trbs_left(dep) == 1))
> trb->ctrl |= DWC3_TRB_CTRL_IOC;
>
> if (chain)
>@@ -1013,6 +1011,8 @@ 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 |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>
>+ dwc3_ep_inc_enq(dep);
>+
> trb->ctrl |= DWC3_TRB_CTRL_HWO;
>
> trace_dwc3_prepare_trb(dep, trb);
>
Thanks for pointing out a solution , the fix looks good. I will test with this fix and resend the patches
Best Regards,
Anurag Kumar Vulisha
next prev parent reply other threads:[~2018-09-06 15:13 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-08-17 12:24 [PATCH v2 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
2018-08-17 12:24 ` [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one_trb() Anurag Kumar Vulisha
2018-09-05 5:33 ` Thinh Nguyen
2018-09-05 9:19 ` Anurag Kumar Vulisha
2018-09-06 1:58 ` Thinh Nguyen
2018-09-06 15:12 ` Anurag Kumar Vulisha [this message]
2018-09-07 2:00 ` Thinh Nguyen
2018-08-17 12:24 ` [PATCH v2 2/8] usb: dwc3: update stream id in depcmd Anurag Kumar Vulisha
2018-08-17 12:24 ` [PATCH v2 3/8] usb: dwc3: make controller clear transfer resources after complete Anurag Kumar Vulisha
2018-09-05 5:38 ` Thinh Nguyen
2018-09-05 9:21 ` Anurag Kumar Vulisha
2018-08-17 12:24 ` [PATCH v2 4/8] usb: dwc3: implement stream transfer timeout Anurag Kumar Vulisha
2018-09-05 6:01 ` Thinh Nguyen
2018-09-05 9:24 ` Anurag Kumar Vulisha
2018-08-17 12:24 ` [PATCH v2 5/8] usb: dwc3: don't issue no-op trb for stream capable endpoints Anurag Kumar Vulisha
2018-08-17 12:24 ` [PATCH v2 6/8] usb: dwc3: check for requests in started list " Anurag Kumar Vulisha
2018-08-17 12:24 ` [PATCH v2 7/8] usb: dwc3: Check for IOC/LST bit in both event->status and TRB->ctrl fields Anurag Kumar Vulisha
2018-08-17 12:24 ` [PATCH v2 8/8] usb: dwc3: Check MISSED ISOC bit only for ISOC endpoints Anurag Kumar Vulisha
2018-09-04 9:44 ` [PATCH v2 0/8] usb: dwc3: Fix broken BULK stream support to dwc3 gadget driver Anurag Kumar Vulisha
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BN3PR0201MB09932A2E75DAA9A1B06DDB3AA7010@BN3PR0201MB0993.namprd02.prod.outlook.com \
--to=anuragku@xilinx.com \
--cc=Thinh.Nguyen@synopsys.com \
--cc=balbi@kernel.org \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=v.anuragkumar@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).