linux-kernel - Re: [PATCH] usb: dwc2: gadget: fix ISOC frame overflow handling

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <410670D7E743164D87FA6160E7907A56013A7A0F2B@am04wembxa.internal.synopsys.com>
Date:   Fri, 9 Nov 2018 08:43:24 +0000
From:   Minas Harutyunyan <minas.harutyunyan@...opsys.com>
To:     John Keeping <john@...anate.com>,
        Minas Harutyunyan <minas.harutyunyan@...opsys.com>
CC:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "arthur.petrosyan@...opsys.com" <arthur.petrosyan@...opsys.com>
Subject: Re: [PATCH] usb: dwc2: gadget: fix ISOC frame overflow handling

Hi John,

On 11/8/2018 9:37 PM, John Keeping wrote:
> Hi Minas,
> 
> On Mon, 5 Nov 2018 08:28:07 +0000
> Minas Harutyunyan <minas.harutyunyan@...opsys.com> wrote:
> 
>> On 10/23/2018 5:43 PM, John Keeping wrote:
>>> By clearing the overrun flag as soon as the target frame is next
>>> incremented, we can end up incrementing the target frame more than
>>> expected in dwc2_gadget_handle_ep_disabled() when the endpoint's
>>> interval is greater than 1.  This happens if the target frame has
>>> just wrapped at the point when the endpoint is disabled and the
>>> frame number has not yet done so.
>>>
>>> Instead, wait until the frame number also wraps and then clear the
>>> overrun flag.
>>>
>>> Signed-off-by: John Keeping <john@...anate.com>
>>> ---
>>>    drivers/usb/dwc2/gadget.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>> index 2d6d2c8244de..8da2c052dfa1 100644
>>> --- a/drivers/usb/dwc2/gadget.c
>>> +++ b/drivers/usb/dwc2/gadget.c
>>> @@ -117,7 +117,7 @@ static inline void
>>> dwc2_gadget_incr_frame_num(struct dwc2_hsotg_ep *hs_ep) if
>>> (hs_ep->target_frame > DSTS_SOFFN_LIMIT) { hs_ep->frame_overrun =
>>> true; hs_ep->target_frame &= DSTS_SOFFN_LIMIT;
>>> -	} else {
>>> +	} else if (hs_ep->parent->frame_number <
>>> hs_ep->target_frame) { hs_ep->frame_overrun = false;
>>>    	}
>>>    }
>>>    
>> Did you tested mentioned by you scenario? If you see issue can you
>> provide debug log and point the issue line in the log.
> 
> It only reproduces very occasionally so it's difficult to capture a full
> debug log containing the error.
> 
> I applied this patch to capture logging specifically around this
> scenario:
> 
> -- >8 --
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 220c0f9b89b0..3770b9d3b523 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2722,13 +2722,20 @@ static void dwc2_gadget_handle_ep_disabled(struct dwc2_hsotg_ep *hs_ep)
>          }
>   
>          do {
> +               unsigned int target_frame = hs_ep->target_frame;
> +               bool frame_overrun = hs_ep->frame_overrun;
> +
>                  hs_req = get_ep_head(hs_ep);
>                  if (hs_req)
>                          dwc2_hsotg_complete_request(hsotg, hs_ep, hs_req,
>                                                      -ENODATA);
> +
>                  dwc2_gadget_incr_frame_num(hs_ep);
>                  /* Update current frame number value. */
>                  hsotg->frame_number = dwc2_hsotg_read_frameno(hsotg);
> +
> +               dev_warn(hsotg->dev, "%s: expiring request frame_number=0x%04x target_frame=0x%04x overrun=%u\n",
> +                        __func__, hsotg->frame_number, target_frame, frame_overrun);
>          } while (dwc2_gadget_target_frame_elapsed(hs_ep));
>   
>          dwc2_gadget_start_next_request(hs_ep);
> -- 8< --
> 
> and I captured this log (the first entry is a separate error and then
> the remaining ones show this bug being triggered):
> 
> [  562.571227] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3eb9 target_frame=0x3ec0
> [  562.611213] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff8 target_frame=0x0008
> [  562.611219] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff8 target_frame=0x0010
> [  562.611223] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0018
> [  562.611228] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0020
> [  562.611232] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0028
> [  562.611236] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0030
> [  562.611240] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0038
> [  562.611244] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0040
> [  562.611249] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0048
> [  562.611253] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0050
> [  562.611257] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0058
> [  562.611261] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0060
> [  562.611265] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0068
> [  562.611269] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0070
> [  562.611274] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0078
> [  562.611278] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0080
> [  562.611282] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0088
> [  562.611286] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0090
> [  562.611290] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x0098
> [  562.611294] dwc2 ff580000.usb: dwc2_gadget_handle_ep_disabled: expiring request frame_number=0x3ff9 target_frame=0x00a0
> 
According above patch in debug log should be printed overrun flag also. 
Could you please resend log with this flag.

> 
> This was on v4.19 with an additional patch to disable descriptor DMA
> because that seems to be causing problems on RK3288 although I haven't
> figured out exactly why it's a problem.

In which mode you run tests Slave or Buffer DMA?
> 
> 
> Regards,
> John
> 

Thanks,
Minas