linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	Felipe Balbi <balbi@kernel.org>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>
Subject: Re: [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust
Date: Wed, 13 Nov 2019 19:40:25 +0000	[thread overview]
Message-ID: <4762d3c8-a4e5-c133-fbbf-e7374f05b2ed@synopsys.com> (raw)
In-Reply-To: <20191113080205.tsqnzjgwtdmmudef@pengutronix.de>

Hi,

Michael Olbrich wrote:
> Hi,
>
> On Wed, Nov 13, 2019 at 03:55:30AM +0000, Thinh Nguyen wrote:
>> Michael Olbrich wrote:
>>> Currently __dwc3_gadget_start_isoc must be called very shortly after
>>> XferNotReady. Otherwise the frame number is outdated and start transfer
>>> will fail, even with several retries.
>>>
>>> DSTS provides the lower 14 bit of the frame number. Use it in combination
>>> with the frame number provided by XferNotReady to guess the current frame
>>> number. This will succeed unless more than one 14 rollover has happened
>>> since XferNotReady.
>>>
>>> Start transfer might still fail if the frame number is increased
>>> immediately after DSTS is read. So retries are still needed.
>>> Don't drop the current request if this happens. This way it is not lost and
>>> can be used immediately to try again with the next frame number.
>>>
>>> With this change, __dwc3_gadget_start_isoc is still not successfully in all
>>> cases bit it increases the acceptable delay after XferNotReady
>>> significantly.
>>>
>>> Signed-off-by: Michael Olbrich <m.olbrich@pengutronix.de>
>>> ---
>>>    drivers/usb/dwc3/core.h   |  1 +
>>>    drivers/usb/dwc3/gadget.c | 31 +++++++++++++++++++++++--------
>>>    2 files changed, 24 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>>> index 3dd783b889cb..c5b223656e08 100644
>>> --- a/drivers/usb/dwc3/core.h
>>> +++ b/drivers/usb/dwc3/core.h
>>> @@ -709,6 +709,7 @@ struct dwc3_ep {
>>>    	u8			type;
>>>    	u8			resource_index;
>>>    	u32			frame_number;
>>> +	u32			last_frame_number;
>> There's no need to add a new field for last_frame_number. Just store the
>> value in a local variable in __dwc3_gadget_start_isoc().
> I'm using it to check for rollover, so __dwc3_gadget_start_isoc does not
> help. I introduced it because I caused a second (incorrect) rollover when
> the first try failed. But now that I think about it, it should be possible
> without the extra variable.

We don't need this extra field for dwc3_ep. If you need to add a new 
one, then you need to also describe that field in the struct dwc3_ep.

>
>>>    	u32			interval;
>>>    
>>>    	char			name[20];
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 173f5329d3d9..ac4673d91939 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -1204,7 +1204,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
>>>    	}
>>>    }
>>>    
>>> -static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>> +static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep, bool keep_req)
>>>    {
>>>    	struct dwc3_gadget_ep_cmd_params params;
>>>    	struct dwc3_request		*req;
>>> @@ -1242,7 +1242,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>>    	}
>>>    
>>>    	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
>>> -	if (ret < 0) {
>>> +	if (ret < 0 && (!keep_req || ret != -EAGAIN)) {
>>>    		/*
>>>    		 * FIXME we need to iterate over the list of requests
>>>    		 * here and stop, unmap, free and del each of the linked
>>> @@ -1254,7 +1254,7 @@ static int __dwc3_gadget_kick_transfer(struct dwc3_ep *dep)
>>>    		return ret;
>>>    	}
>>>    
>>> -	return 0;
>>> +	return ret;
>>>    }
>>>    
>>>    static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>>> @@ -1377,7 +1377,7 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
>>>    	dep->start_cmd_status = 0;
>>>    	dep->combo_num = 0;
>>>    
>>> -	return __dwc3_gadget_kick_transfer(dep);
>>> +	return __dwc3_gadget_kick_transfer(dep, false);
>>>    }
>>>    
>>>    static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>> @@ -1402,9 +1402,23 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>>>    	}
>>>    
>>>    	for (i = 0; i < DWC3_ISOC_MAX_RETRIES; i++) {
>>> -		dep->frame_number = DWC3_ALIGN_FRAME(dep, i + 1);
>>> +		/*
>>> +		 * last_frame_number is set from XferNotReady and may be
>>> +		 * already out of date. DSTS only provides the lower 14 bit
>>> +		 * of the current frame number. So add the upper two bits of
>>> +		 * last_frame_number and handle a possible rollover.
>>> +		 * This will provide the correct frame_number unless more than
>>> +		 * rollover has happened since XferNotReady.
>>> +		 */
>>> +		u32 frame = __dwc3_gadget_get_frame(dwc);
>>> +
>>> +		dep->frame_number = (dep->last_frame_number & ~0x3fff) | frame;
>>> +		if (frame < (dep->last_frame_number & 0x3fff))
>>> +			dep->frame_number += 0x4000;
>> Use BIT(14) rather than 0x4000? It's clearer in in my opinion. We
> Ok.
>
>> started using 0x3fff in multiple places now, can we create a macro for that?
> Makes sense. Any preferences for the name? <something>_MASK I guess, but I
> don't know what the correct name for the 14 bit frame number should be.

Maybe you can use DWC3_DSTS_SOFFN(~0) as a mask.

BR,
Thinh

  reply	other threads:[~2019-11-13 19:40 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-11 15:26 [PATCH 0/2] usb: dwc3: gadget: improve isoc handling Michael Olbrich
2019-11-11 15:26 ` [PATCH 1/2] usb: dwc3: gadget: make starting isoc transfers more robust Michael Olbrich
2019-11-12 20:41   ` kbuild test robot
2019-11-13  3:55   ` Thinh Nguyen
2019-11-13  8:02     ` Michael Olbrich
2019-11-13 19:40       ` Thinh Nguyen [this message]
2019-11-11 15:26 ` [PATCH 2/2] usb: dwc3: gadget: restart the transfer if a isoc request is queued too late Michael Olbrich
2019-11-13  3:55   ` Thinh Nguyen
2019-11-13  7:53     ` Michael Olbrich
2019-11-13 15:39       ` Alan Stern
2019-11-13 19:14         ` Thinh Nguyen
2019-11-14 12:14           ` Michael Olbrich
2019-11-14 20:11             ` Thinh Nguyen
2019-11-15 21:06               ` Alan Stern
2019-11-28 11:36                 ` Michael Olbrich
2019-11-28 17:59                   ` Alan Stern
2019-12-02 15:41                     ` Michael Olbrich
2019-12-02 17:02                       ` Alan Stern
2020-04-09  7:59               ` Michael Grzeschik
2020-04-10  1:09                 ` Thinh Nguyen
2020-04-10 22:03                   ` Michael Grzeschik
2020-04-11  0:59                     ` Thinh Nguyen
2020-04-21  6:51                       ` Michael Grzeschik
2020-04-21 23:41                         ` Thinh Nguyen

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=4762d3c8-a4e5-c133-fbbf-e7374f05b2ed@synopsys.com \
    --to=thinh.nguyen@synopsys.com \
    --cc=balbi@kernel.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel@pengutronix.de \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    /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).