[PATCH] usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[PATCH] usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Jack Pham]

OUT endpoint requests may somtimes have this flag set when
preparing to be submitted to HW indicating that there is an
additional TRB chained to the request for alignment purposes.
If that request is removed before the controller can execute the
transfer (e.g. ep_dequeue/ep_disable), the request will not go
through the dwc3_gadget_ep_cleanup_completed_request() handler
and will not have its needs_extra_trb flag cleared when
dwc3_gadget_giveback() is called.  This same request could be
later requeued for a new transfer that does not require an
extra TRB and if it is successfully completed, the cleanup
and TRB reclamation will incorrectly process the additional TRB
which belongs to the next request, and incorrectly advances the
TRB dequeue pointer, thereby messing up calculation of the next
requeust's actual/remaining count when it completes.

The right thing to do here is to ensure that the flag is cleared
before it is given back to the function driver.  A good place
to do that is in dwc3_gadget_del_and_unmap_request().

Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
Hi Felipe,

There's probably zero chance this is making it to 4.20, so if you take
this after 4.21-rc1 so be it. But should this be Cc: stable? If so it
needs to be sent separately for <= 4.19 as needs_extra_trb was previously
req->unaligned and req->zero.

Thanks,
Jack

 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2ecde30ad0b7..e97b14f444c8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -177,6 +177,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
 	req->started = false;
 	list_del(&req->list);
 	req->remaining = 0;
+	req->needs_extra_trb = false;
 
 	if (req->request.status == -EINPROGRESS)
 		req->request.status = status;
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Jack Pham]

OUT endpoint requests may somtimes have this flag set when
preparing to be submitted to HW indicating that there is an
additional TRB chained to the request for alignment purposes.
If that request is removed before the controller can execute the
transfer (e.g. ep_dequeue/ep_disable), the request will not go
through the dwc3_gadget_ep_cleanup_completed_request() handler
and will not have its needs_extra_trb flag cleared when
dwc3_gadget_giveback() is called.  This same request could be
later requeued for a new transfer that does not require an
extra TRB and if it is successfully completed, the cleanup
and TRB reclamation will incorrectly process the additional TRB
which belongs to the next request, and incorrectly advances the
TRB dequeue pointer, thereby messing up calculation of the next
requeust's actual/remaining count when it completes.

The right thing to do here is to ensure that the flag is cleared
before it is given back to the function driver.  A good place
to do that is in dwc3_gadget_del_and_unmap_request().

Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
Hi Felipe,

There's probably zero chance this is making it to 4.20, so if you take
this after 4.21-rc1 so be it. But should this be Cc: stable? If so it
needs to be sent separately for <= 4.19 as needs_extra_trb was previously
req->unaligned and req->zero.

Thanks,
Jack

 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2ecde30ad0b7..e97b14f444c8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -177,6 +177,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
 	req->started = false;
 	list_del(&req->list);
 	req->remaining = 0;
+	req->needs_extra_trb = false;
 
 	if (req->request.status == -EINPROGRESS)
 		req->request.status = status;

Re: [PATCH] usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Felipe Balbi]

Hi,

Jack Pham <jackp@codeaurora.org> writes:

> OUT endpoint requests may somtimes have this flag set when
> preparing to be submitted to HW indicating that there is an
> additional TRB chained to the request for alignment purposes.
> If that request is removed before the controller can execute the
> transfer (e.g. ep_dequeue/ep_disable), the request will not go
> through the dwc3_gadget_ep_cleanup_completed_request() handler
> and will not have its needs_extra_trb flag cleared when
> dwc3_gadget_giveback() is called.  This same request could be
> later requeued for a new transfer that does not require an
> extra TRB and if it is successfully completed, the cleanup
> and TRB reclamation will incorrectly process the additional TRB
> which belongs to the next request, and incorrectly advances the
> TRB dequeue pointer, thereby messing up calculation of the next
> requeust's actual/remaining count when it completes.
>
> The right thing to do here is to ensure that the flag is cleared
> before it is given back to the function driver.  A good place
> to do that is in dwc3_gadget_del_and_unmap_request().
>
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
> Hi Felipe,
>
> There's probably zero chance this is making it to 4.20, so if you take
> this after 4.21-rc1 so be it. But should this be Cc: stable? If so it
> needs to be sent separately for <= 4.19 as needs_extra_trb was previously
> req->unaligned and req->zero.

we need a Cc stable, indeed. And a Fixes tag.

-- 
balbi

usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Felipe Balbi]

Hi,

Jack Pham <jackp@codeaurora.org> writes:

> OUT endpoint requests may somtimes have this flag set when
> preparing to be submitted to HW indicating that there is an
> additional TRB chained to the request for alignment purposes.
> If that request is removed before the controller can execute the
> transfer (e.g. ep_dequeue/ep_disable), the request will not go
> through the dwc3_gadget_ep_cleanup_completed_request() handler
> and will not have its needs_extra_trb flag cleared when
> dwc3_gadget_giveback() is called.  This same request could be
> later requeued for a new transfer that does not require an
> extra TRB and if it is successfully completed, the cleanup
> and TRB reclamation will incorrectly process the additional TRB
> which belongs to the next request, and incorrectly advances the
> TRB dequeue pointer, thereby messing up calculation of the next
> requeust's actual/remaining count when it completes.
>
> The right thing to do here is to ensure that the flag is cleared
> before it is given back to the function driver.  A good place
> to do that is in dwc3_gadget_del_and_unmap_request().
>
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
> Hi Felipe,
>
> There's probably zero chance this is making it to 4.20, so if you take
> this after 4.21-rc1 so be it. But should this be Cc: stable? If so it
> needs to be sent separately for <= 4.19 as needs_extra_trb was previously
> req->unaligned and req->zero.

we need a Cc stable, indeed. And a Fixes tag.

[PATCH v2] usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Jack Pham]

OUT endpoint requests may somtimes have this flag set when
preparing to be submitted to HW indicating that there is an
additional TRB chained to the request for alignment purposes.
If that request is removed before the controller can execute the
transfer (e.g. ep_dequeue/ep_disable), the request will not go
through the dwc3_gadget_ep_cleanup_completed_request() handler
and will not have its needs_extra_trb flag cleared when
dwc3_gadget_giveback() is called.  This same request could be
later requeued for a new transfer that does not require an
extra TRB and if it is successfully completed, the cleanup
and TRB reclamation will incorrectly process the additional TRB
which belongs to the next request, and incorrectly advances the
TRB dequeue pointer, thereby messing up calculation of the next
requeust's actual/remaining count when it completes.

The right thing to do here is to ensure that the flag is cleared
before it is given back to the function driver.  A good place
to do that is in dwc3_gadget_del_and_unmap_request().

Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Cc: stable@vger.kernel.org
Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
v2: Added Fixes tag and Cc: stable

Felipe, as I mentioned in the cover for v1, for stable (from 4.11 where
c6267a51639b first landed through 4.20), the fix needs to be modified to
assign to the separate req->unaligned and req->zero flags in lieu of
needs_extra_trb which appeared in 5.0-rc1 in:

	commit 1a22ec643580626f439c8583edafdcc73798f2fb
	Author: Felipe Balbi <felipe.balbi@linux.intel.com>
	Date:   Wed Aug 1 13:15:05 2018 +0300

	    usb: dwc3: gadget: combine unaligned and zero flags

Do I need to send a separate patch for <= 4.20 or will you handle it?
It's straightforward really, the code change should instead be

+	req->unaligned = false;
+	req->zero = false;

Thanks,
Jack

 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2ecde30ad0b7..e97b14f444c8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -177,6 +177,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
 	req->started = false;
 	list_del(&req->list);
 	req->remaining = 0;
+	req->needs_extra_trb = false;
 
 	if (req->request.status == -EINPROGRESS)
 		req->request.status = status;
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[v2] usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Jack Pham]

OUT endpoint requests may somtimes have this flag set when
preparing to be submitted to HW indicating that there is an
additional TRB chained to the request for alignment purposes.
If that request is removed before the controller can execute the
transfer (e.g. ep_dequeue/ep_disable), the request will not go
through the dwc3_gadget_ep_cleanup_completed_request() handler
and will not have its needs_extra_trb flag cleared when
dwc3_gadget_giveback() is called.  This same request could be
later requeued for a new transfer that does not require an
extra TRB and if it is successfully completed, the cleanup
and TRB reclamation will incorrectly process the additional TRB
which belongs to the next request, and incorrectly advances the
TRB dequeue pointer, thereby messing up calculation of the next
requeust's actual/remaining count when it completes.

The right thing to do here is to ensure that the flag is cleared
before it is given back to the function driver.  A good place
to do that is in dwc3_gadget_del_and_unmap_request().

Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Cc: stable@vger.kernel.org
Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
v2: Added Fixes tag and Cc: stable

Felipe, as I mentioned in the cover for v1, for stable (from 4.11 where
c6267a51639b first landed through 4.20), the fix needs to be modified to
assign to the separate req->unaligned and req->zero flags in lieu of
needs_extra_trb which appeared in 5.0-rc1 in:

	commit 1a22ec643580626f439c8583edafdcc73798f2fb
	Author: Felipe Balbi <felipe.balbi@linux.intel.com>
	Date:   Wed Aug 1 13:15:05 2018 +0300

	    usb: dwc3: gadget: combine unaligned and zero flags

Do I need to send a separate patch for <= 4.20 or will you handle it?
It's straightforward really, the code change should instead be

+	req->unaligned = false;
+	req->zero = false;

Thanks,
Jack

 drivers/usb/dwc3/gadget.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2ecde30ad0b7..e97b14f444c8 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -177,6 +177,7 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
 	req->started = false;
 	list_del(&req->list);
 	req->remaining = 0;
+	req->needs_extra_trb = false;
 
 	if (req->request.status == -EINPROGRESS)
 		req->request.status = status;

Re: [PATCH v2] usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Jack Pham]

Hi Sasha,

On Wed, Jan 16, 2019 at 01:35:52PM +0000, Sasha Levin wrote:
> Hi,
> 
> [This is an automated email]
> 
> This commit has been processed because it contains a "Fixes:" tag,
> fixing commit: c6267a51639b usb: dwc3: gadget: align transfers to wMaxPacketSize.
> 
> The bot has tested the following trees: v4.20.2, v4.19.15, v4.14.93.
> 
> v4.20.2: Build failed! Errors:
>     drivers/usb/dwc3/gadget.c:180:5: error: ‘struct dwc3_request’ has no member named ‘needs_extra_trb’
> 
> v4.19.15: Build failed! Errors:
>     drivers/usb/dwc3/gadget.c:180:5: error: ‘struct dwc3_request’ has no member named ‘needs_extra_trb’
> 
> v4.14.93: Build failed! Errors:
>     drivers/usb/dwc3/gadget.c:185:5: error: ‘struct dwc3_request’ has no member named ‘needs_extra_trb’
> 
> 
> How should we proceed with this patch?

I mentioned in my "cover" message of the patch that it needs a slight
rewrite for -stable since the 'needs_extra_trb' was only introduced in
v5.0-rc1. Now that my patch is in Linus's tree, I can send it separately
for -stable; I'll fix up the commit text slightly to refer to the
variables used before v5.0.

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 4.14,4.19,4.20] usb: dwc3: gadget: Clear req->unaligned and zero flags on cleanup

[Jack Pham]

commit bd6742249b9ca918565e4e3abaa06665e587f4b5 upstream.

OUT endpoint requests may somtimes have one of these flags set
when preparing to be submitted to HW indicating that there is an
additional TRB chained to the request for alignment purposes.
If that request is removed before the controller can execute the
transfer (e.g. ep_dequeue/ep_disable), the request will not go
through the dwc3_gadget_ep_cleanup_completed_request() handler
and will not have its unaligned or zero flag cleared when
dwc3_gadget_giveback() is called.  This same request could be
later requeued for a new transfer that does not require an
extra TRB and if it is successfully completed, the cleanup
and TRB reclamation will incorrectly process the additional TRB
which belongs to the next request, and incorrectly advances the
TRB dequeue pointer, thereby messing up calculation of the next
requeust's actual/remaining count when it completes.

The right thing to do here is to ensure that the flags are cleared
before it is given back to the function driver.  A good place
to do that is in dwc3_gadget_del_and_unmap_request().

Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
Signed-off-by: Jack Pham <jackp@codeaurora.org>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
[jackp: backport to <= 4.20: replaced 'needs_extra_trb' with 'unaligned'
        and 'zero' members in patch and reworded commit text]
Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
 drivers/usb/dwc3/gadget.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 9f92ee03dde7..2a4ea9a1b1e3 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -177,6 +177,8 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
 	req->started = false;
 	list_del(&req->list);
 	req->remaining = 0;
+	req->unaligned = false;
+	req->zero = false;
 
 	if (req->request.status == -EINPROGRESS)
 		req->request.status = status;
-- 
2.17.0

Re: [PATCH v2] usb: dwc3: gadget: Clear req->needs_extra_trb flag on cleanup

[Sasha Levin]

On Fri, Jan 25, 2019 at 05:18:16PM -0800, Jack Pham wrote:
>Hi Sasha,
>
>On Wed, Jan 16, 2019 at 01:35:52PM +0000, Sasha Levin wrote:
>> Hi,
>>
>> [This is an automated email]
>>
>> This commit has been processed because it contains a "Fixes:" tag,
>> fixing commit: c6267a51639b usb: dwc3: gadget: align transfers to wMaxPacketSize.
>>
>> The bot has tested the following trees: v4.20.2, v4.19.15, v4.14.93.
>>
>> v4.20.2: Build failed! Errors:
>>     drivers/usb/dwc3/gadget.c:180:5: error: ‘struct dwc3_request’ has no member named ‘needs_extra_trb’
>>
>> v4.19.15: Build failed! Errors:
>>     drivers/usb/dwc3/gadget.c:180:5: error: ‘struct dwc3_request’ has no member named ‘needs_extra_trb’
>>
>> v4.14.93: Build failed! Errors:
>>     drivers/usb/dwc3/gadget.c:185:5: error: ‘struct dwc3_request’ has no member named ‘needs_extra_trb’
>>
>>
>> How should we proceed with this patch?
>
>I mentioned in my "cover" message of the patch that it needs a slight
>rewrite for -stable since the 'needs_extra_trb' was only introduced in
>v5.0-rc1. Now that my patch is in Linus's tree, I can send it separately
>for -stable; I'll fix up the commit text slightly to refer to the
>variables used before v5.0.

Sorry, the bot is still very much dumb at this point and doesn't read
cover letters :)

Thank you!

--
Thanks,
Sasha

Re: [PATCH 4.14,4.19,4.20] usb: dwc3: gadget: Clear req->unaligned and zero flags on cleanup

[Sasha Levin]

On Fri, Jan 25, 2019 at 05:51:53PM -0800, Jack Pham wrote:
>commit bd6742249b9ca918565e4e3abaa06665e587f4b5 upstream.
>
>OUT endpoint requests may somtimes have one of these flags set
>when preparing to be submitted to HW indicating that there is an
>additional TRB chained to the request for alignment purposes.
>If that request is removed before the controller can execute the
>transfer (e.g. ep_dequeue/ep_disable), the request will not go
>through the dwc3_gadget_ep_cleanup_completed_request() handler
>and will not have its unaligned or zero flag cleared when
>dwc3_gadget_giveback() is called.  This same request could be
>later requeued for a new transfer that does not require an
>extra TRB and if it is successfully completed, the cleanup
>and TRB reclamation will incorrectly process the additional TRB
>which belongs to the next request, and incorrectly advances the
>TRB dequeue pointer, thereby messing up calculation of the next
>requeust's actual/remaining count when it completes.
>
>The right thing to do here is to ensure that the flags are cleared
>before it is given back to the function driver.  A good place
>to do that is in dwc3_gadget_del_and_unmap_request().
>
>Fixes: c6267a51639b ("usb: dwc3: gadget: align transfers to wMaxPacketSize")
>Signed-off-by: Jack Pham <jackp@codeaurora.org>
>Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>[jackp: backport to <= 4.20: replaced 'needs_extra_trb' with 'unaligned'
>        and 'zero' members in patch and reworded commit text]
>Signed-off-by: Jack Pham <jackp@codeaurora.org>
>---
> drivers/usb/dwc3/gadget.c | 2 ++
> 1 file changed, 2 insertions(+)
>
>diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>index 9f92ee03dde7..2a4ea9a1b1e3 100644
>--- a/drivers/usb/dwc3/gadget.c
>+++ b/drivers/usb/dwc3/gadget.c
>@@ -177,6 +177,8 @@ static void dwc3_gadget_del_and_unmap_request(struct dwc3_ep *dep,
> 	req->started = false;
> 	list_del(&req->list);
> 	req->remaining = 0;
>+	req->unaligned = false;
>+	req->zero = false;
>
> 	if (req->request.status == -EINPROGRESS)
> 		req->request.status = status;

Thanks! I'll let Greg pick it up (as the upstream patch was explicitly
tagged for stable).

--
Thanks,
Sasha