| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <30102591E157244384E984126FC3CB4F544AD412@us01wembx1.internal.synopsys.com>
Date: Fri, 7 Sep 2018 02:00:58 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Anurag Kumar Vulisha <anuragku@...inx.com>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"balbi@...nel.org" <balbi@...nel.org>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC: "v.anuragkumar@...il.com" <v.anuragkumar@...il.com>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.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 9/6/2018 8:12 AM, Anurag Kumar Vulisha wrote:
> Hi Thinh,
>
>> -----Original Message-----
>> From: Thinh Nguyen [mailto:Thinh.Nguyen@...opsys.com]
>> Sent: Thursday, September 06, 2018 7:28 AM
>> To: Anurag Kumar Vulisha <anuragku@...inx.com>; Thinh Nguyen
>> <Thinh.Nguyen@...opsys.com>; balbi@...nel.org; gregkh@...uxfoundation.org
>> Cc: v.anuragkumar@...il.com; linux-usb@...r.kernel.org; linux-
>> kernel@...r.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@...opsys.com]
>>>> Sent: Wednesday, September 05, 2018 11:04 AM
>>>> To: Anurag Kumar Vulisha <anuragku@...inx.com>; balbi@...nel.org;
>>>> gregkh@...uxfoundation.org
>>>> Cc: v.anuragkumar@...il.com; linux-usb@...r.kernel.org; linux-
>>>> kernel@...r.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
>
>
The rest of the patch series looks fine to me.
Reviewed-by: Thinh Nguyen <thinhn@...opsys.com>
BR,
Thinh
Powered by blists - more mailing lists