* [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
@ 2016-05-26 4:58 Baolin Wang
2016-05-26 6:22 ` Felipe Balbi
0 siblings, 1 reply; 9+ messages in thread
From: Baolin Wang @ 2016-05-26 4:58 UTC (permalink / raw)
To: balbi, gregkh; +Cc: broonie, linux-usb, linux-kernel, baolin.wang
When handling the endpoint interrupt handler, it maybe disable the endpoint
from another core user to set the USB endpoint descriptor pointor to be NULL
while issuing usb_gadget_giveback_request() function to release lock. So it
will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs with
one NULL USB endpoint descriptor.
Thus this patch introduces safety dwc3_endpoint_xfer_xxx() APIs to check
endpoint type with checking if the endpoint flag 'DWC3_EP_ENABLED' is set or
not.
[ 4007.812527] c1 init(1) call enable_store(0)
[ 4007.812633] c0 dwc3 20500000.dwc3: ep1out disabled while handling ep event
[ 4007.812657] c0 Unable to handle kernel NULL pointer dereference at virtual address 00000003
[ 4007.812669] c0 pgd = ffffffc0260a8000
[ 4007.812681] c0 [00000003] *pgd=00000000b4ab4003, *pud=00000000b4ab4003, *pmd=0000000000000000
[ 4007.812709] c0 Internal error: Oops: 96000006 [#1] PREEMPT SMP
[ 4007.818441] c0 Modules linked in: bcmdhd
[ 4007.821854] c3 warning saudio_recv cache is empty!
[ 4007.821860] c3 aud_recv_cmd ENODATA
[ 4007.821869] c3 aud_send_cmd in,cmd =11 id:10 ret-value:0
[ 4007.821876] c3 saudio_send: dst=1, channel=0, timeout=-1
[ 4007.840767] c0 mali_kbase(O)
[ 4007.843707] c0
[ 4007.843889] c0 CPU: 0 PID: 8126 Comm: adbd Tainted: G W O 3.18.12+ #1
[ 4007.851153] c0 Hardware name: Spreadtrum SP9860g Board (DT)
[ 4007.856693] c0 task: ffffffc02638a400 ti: ffffffc026148000 task.ti: ffffffc026148000
[ 4007.864404] c0 PC is at dwc3_interrupt_bh+0x710/0x112c
[ 4007.869503] c0 LR is at dwc3_interrupt_bh+0x70c/0x112c
[ 4007.874604] c0 pc : [<ffffffc0005bc48c>] lr : [<ffffffc0005bc488>] pstate: 200001c5
[ 4007.882218] c0 sp : ffffffc02614b970
[ 4007.885766] c0 x29: ffffffc02614b970 x28: ffffffc000c75f90
[ 4007.891044] c0 x27: 0000000000000008 x26: 0000000000000000
[ 4007.896323] c0 x25: 0000000000000000 x24: 000000000000030c
[ 4007.901602] c0 x23: ffffffc000ff1000 x22: 0000000000000004
[ 4007.906881] c0 x21: ffffffc03d9fb5d8 x20: ffffffc13e573600
[ 4007.912160] c0 x19: ffffffc13e133020 x18: 0000000000000007
[ 4007.917437] c0 x17: 000000000000000e x16: 0000000000000001
[ 4007.922717] c0 x15: 0000000000000007 x14: 0fffffffffffffff
[ 4007.927996] c0 x13: 0000000000000001 x12: 0101010101010101
[ 4007.933274] c0 x11: 00000000000c4f5d x10: 0000000000000002
[ 4007.938553] c0 x9 : 0000000000000000 x8 : 00000000000c4f5e
[ 4007.943833] c0 x7 : 696c646e61682065 x6 : ffffffc000fa6cdc
[ 4007.949111] c0 x5 : 0000000000000000 x4 : 0000000000000105
[ 4007.954390] c0 x3 : 0000000000000000 x2 : 0000000000000000
[ 4007.959669] c0 x1 : 000000005533250a x0 : 0000000000000000
......
[ 4009.150699] c0 Call trace:
[ 4009.153394] c0 [<ffffffc0005bc48c>] dwc3_interrupt_bh+0x710/0x112c
[ 4009.159532] c0 [<ffffffc0000c0370>] tasklet_action+0xb0/0x188
[ 4009.165243] c0 [<ffffffc0000bf598>] __do_softirq+0x114/0x3b4
[ 4009.170867] c0 [<ffffffc0000bfb9c>] irq_exit+0xa8/0xdc
[ 4009.175975] c0 [<ffffffc00010cc44>] __handle_domain_irq+0x90/0xf8
[ 4009.182030] c0 [<ffffffc00008249c>] gic_handle_irq+0x58/0x1b4
[ 4009.187739] c0 Exception stack(0xffffffc02614bb70 to 0xffffffc02614bc90)
[ 4009.194404] c0 bb60: 00000140 00000000 3e133108 ffffffc1
[ 4009.202802] c0 bb80: 2614bcd0 ffffffc0 009cff0c ffffffc0 80000145 00000000 00000018 00000000
[ 4009.211194] c0 bba0: 00000400 00000000 00459900 ffffffc0 00000000 00000000 00d1aa50 ffffffc0
[ 4009.219589] c0 bbc0: 2614bc00 ffffffc0 26148000 ffffffc0 00000001 00000000 3d2ce4e0 ffffffc1
[ 4009.227984] c0 bbe0: bdc00000 00000000 00000001 00000000 2638a980 ffffffc0 2614ba40 ffffffc0
[ 4009.236379] c0 bc00: 00000400 00000000 00000001 00000000 1d37bbb0 ffffffc0 00000013 00000000
[ 4009.244772] c0 bc20: 0000000e 00000000 00000007 00000000 00000001 00000000 0000000e 00000000
[ 4009.253167] c0 bc40: 00000007 00000000 00000140 00000000 3e133108 ffffffc1 3e133108 ffffffc1
[ 4009.261560] c0 bc60: 00000140 00000000 3eb833c0 ffffffc1 00000018 00000000 00000400 00000000
[ 4009.269951] c0 bc80: 1eb29b80 ffffffc1 3acd7c00 ffffffc0
[ 4009.275233] c0 [<ffffffc0000860e8>] el1_irq+0x68/0xdc
[ 4009.280255] c0 [<ffffffc0005b940c>] dwc3_gadget_ep_dequeue+0x180/0x1b8
[ 4009.286741] c0 [<ffffffc0005feb48>] ffs_epfile_io+0x330/0x374
[ 4009.292453] c0 [<ffffffc0005fec3c>] ffs_epfile_read+0x30/0x40
[ 4009.298169] c0 [<ffffffc0001ecb3c>] vfs_read+0xa0/0x1dc
[ 4009.303357] c0 [<ffffffc0001ed644>] SyS_read+0x50/0xb0
Signed-off-by: Baolin Wang <baolin.wang@linaro.org>
---
drivers/usb/dwc3/gadget.c | 95 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 72 insertions(+), 23 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8e4a1b1..5d095f2 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -36,6 +36,56 @@
#include "io.h"
/**
+* dwc3_endpoint_xfer_bulk - check if the endpoint has bulk transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type bulk, otherwise it returns false.
+*/
+static inline int dwc3_endpoint_xfer_bulk(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_BULK);
+}
+
+/**
+* dwc3_endpoint_xfer_control - check if the endpoint has control transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type control, otherwise it returns false.
+*/
+static inline int dwc3_endpoint_xfer_control(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_CONTROL);
+}
+
+/**
+* dwc3_endpoint_xfer_int - check if the endpoint has interrupt transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type interrupt, otherwise it returns
+* false.
+*/
+static inline int dwc3_endpoint_xfer_int(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_INT);
+}
+
+/**
+* dwc3_endpoint_xfer_isoc - check if the endpoint has isochronous transfer type
+* @ep: endpoint to be checked
+*
+* Returns true if the endpoint is of type isochronous, otherwise it returns
+* false.
+*/
+static inline int dwc3_endpoint_xfer_isoc(struct dwc3_ep *ep)
+{
+ return ((ep->flags & DWC3_EP_ENABLED) &&
+ ep->type == USB_ENDPOINT_XFER_ISOC);
+}
+
+/**
* dwc3_gadget_set_test_mode - Enables USB2 Test Modes
* @dwc: pointer to our context structure
* @mode: the mode to set (J, K SE0 NAK, Force Enable)
@@ -198,8 +248,8 @@ int dwc3_gadget_resize_tx_fifos(struct dwc3 *dwc)
if (!(dep->flags & DWC3_EP_ENABLED))
continue;
- if (usb_endpoint_xfer_bulk(dep->endpoint.desc)
- || usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ if (dwc3_endpoint_xfer_bulk(dep) ||
+ dwc3_endpoint_xfer_isoc(dep))
mult = 3;
/*
@@ -248,7 +298,7 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct dwc3_request *req,
*/
if (((dep->busy_slot & DWC3_TRB_MASK) ==
DWC3_TRB_NUM- 1) &&
- usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ dwc3_endpoint_xfer_isoc(dep))
dep->busy_slot++;
} while(++i < req->request.num_mapped_sgs);
req->queued = false;
@@ -567,7 +617,7 @@ static int __dwc3_gadget_ep_enable(struct dwc3_ep *dep,
reg |= DWC3_DALEPENA_EP(dep->number);
dwc3_writel(dwc->regs, DWC3_DALEPENA, reg);
- if (!usb_endpoint_xfer_isoc(desc))
+ if (!dwc3_endpoint_xfer_isoc(dep))
goto out;
/* Link TRB for ISOC. The HWO bit is never reset */
@@ -795,7 +845,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
dep->free_slot++;
/* Skip the LINK-TRB on ISOC */
if (((dep->free_slot & DWC3_TRB_MASK) == DWC3_TRB_NUM - 1) &&
- usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ dwc3_endpoint_xfer_isoc(dep))
dep->free_slot++;
trb->size = DWC3_TRB_SIZE_LENGTH(length);
@@ -829,7 +879,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
if (!req->request.no_interrupt && !chain)
trb->ctrl |= DWC3_TRB_CTRL_IOC;
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
trb->ctrl |= DWC3_TRB_CTRL_CSP;
} else if (last) {
@@ -839,7 +889,7 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
if (chain)
trb->ctrl |= DWC3_TRB_CTRL_CHN;
- if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
+ if (dwc3_endpoint_xfer_bulk(dep) && dep->stream_capable)
trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(req->request.stream_id);
trb->ctrl |= DWC3_TRB_CTRL_HWO;
@@ -869,7 +919,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
trbs_left = (dep->busy_slot - dep->free_slot) & DWC3_TRB_MASK;
/* Can't wrap around on a non-isoc EP since there's no link TRB */
- if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (!dwc3_endpoint_xfer_isoc(dep)) {
max = DWC3_TRB_NUM - (dep->free_slot & DWC3_TRB_MASK);
if (trbs_left > max)
trbs_left = max;
@@ -895,7 +945,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
* processed from the first TRB until the last one. Since we
* don't wrap around we have to start at the beginning.
*/
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
dep->busy_slot = 1;
dep->free_slot = 1;
} else {
@@ -905,7 +955,7 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep, bool starting)
}
/* The last TRB is a link TRB, not used for xfer */
- if ((trbs_left <= 1) && usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ if ((trbs_left <= 1) && dwc3_endpoint_xfer_isoc(dep))
return;
list_for_each_entry_safe(req, n, &dep->request_list, list) {
@@ -1123,8 +1173,8 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
* This will save one IRQ (XFER_NOT_READY) and possibly make it a
* little bit faster.
*/
- if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
- !usb_endpoint_xfer_int(dep->endpoint.desc) &&
+ if (!dwc3_endpoint_xfer_isoc(dep) &&
+ !dwc3_endpoint_xfer_int(dep) &&
!(dep->flags & DWC3_EP_BUSY)) {
ret = __dwc3_gadget_kick_transfer(dep, 0, true);
goto out;
@@ -1148,7 +1198,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
* you can receive xfernotready again and can have
* notion of current microframe.
*/
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
if (list_empty(&dep->req_queued)) {
dwc3_stop_active_transfer(dwc, dep->number, true);
dep->flags = DWC3_EP_ENABLED;
@@ -1168,7 +1218,7 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request *req)
* kick the transfer here after queuing a request, otherwise the
* core may not see the modified TRB(s).
*/
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+ if (dwc3_endpoint_xfer_isoc(dep) &&
(dep->flags & DWC3_EP_BUSY) &&
!(dep->flags & DWC3_EP_MISSED_ISOC)) {
WARN_ON_ONCE(!dep->resource_index);
@@ -1304,7 +1354,7 @@ int __dwc3_gadget_ep_set_halt(struct dwc3_ep *dep, int value, int protocol)
struct dwc3 *dwc = dep->dwc;
int ret;
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
dev_err(dwc->dev, "%s is of Isochronous type\n", dep->name);
return -EINVAL;
}
@@ -1971,7 +2021,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
do {
slot = req->start_slot + i;
if ((slot == DWC3_TRB_NUM - 1) &&
- usb_endpoint_xfer_isoc(dep->endpoint.desc))
+ dwc3_endpoint_xfer_isoc(dep))
slot++;
slot %= DWC3_TRB_NUM;
trb = &dep->trb_pool[slot];
@@ -1988,7 +2038,7 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
break;
} while (1);
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
+ if (dwc3_endpoint_xfer_isoc(dep) &&
list_empty(&dep->req_queued)) {
if (list_empty(&dep->request_list)) {
/*
@@ -2021,8 +2071,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
status = -ECONNRESET;
clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
- if (clean_busy && (is_xfer_complete ||
- usb_endpoint_xfer_isoc(dep->endpoint.desc)))
+ if (clean_busy && (is_xfer_complete || dwc3_endpoint_xfer_isoc(dep)))
dep->flags &= ~DWC3_EP_BUSY;
/*
@@ -2050,7 +2099,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
dwc->u1u2 = 0;
}
- if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (!dwc3_endpoint_xfer_isoc(dep)) {
int ret;
ret = __dwc3_gadget_kick_transfer(dep, 0, is_xfer_complete);
@@ -2079,7 +2128,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
case DWC3_DEPEVT_XFERCOMPLETE:
dep->resource_index = 0;
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
dwc3_trace(trace_dwc3_gadget,
"%s is an Isochronous endpoint\n",
dep->name);
@@ -2092,7 +2141,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
dwc3_endpoint_transfer_complete(dwc, dep, event);
break;
case DWC3_DEPEVT_XFERNOTREADY:
- if (usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
+ if (dwc3_endpoint_xfer_isoc(dep)) {
dwc3_gadget_start_isoc(dwc, dep, event);
} else {
int active;
@@ -2115,7 +2164,7 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
break;
case DWC3_DEPEVT_STREAMEVT:
- if (!usb_endpoint_xfer_bulk(dep->endpoint.desc)) {
+ if (!dwc3_endpoint_xfer_bulk(dep)) {
dev_err(dwc->dev, "Stream event for non-Bulk %s\n",
dep->name);
return;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 4:58 [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type Baolin Wang
@ 2016-05-26 6:22 ` Felipe Balbi
2016-05-26 7:11 ` Baolin Wang
0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2016-05-26 6:22 UTC (permalink / raw)
To: Baolin Wang, gregkh; +Cc: broonie, linux-usb, linux-kernel, baolin.wang
[-- Attachment #1: Type: text/plain, Size: 692 bytes --]
Hi,
Baolin Wang <baolin.wang@linaro.org> writes:
> When handling the endpoint interrupt handler, it maybe disable the endpoint
> from another core user to set the USB endpoint descriptor pointor to be NULL
> while issuing usb_gadget_giveback_request() function to release lock. So it
> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs with
> one NULL USB endpoint descriptor.
too complex, Baolin :-) Can you see if this helps:
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=88bf752cfb55e57a78e27c931c9fef63240c739a
The only situation when that can happen is while we drop our lock on
dwc3_gadget_giveback().
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 6:22 ` Felipe Balbi
@ 2016-05-26 7:11 ` Baolin Wang
2016-05-26 7:48 ` Felipe Balbi
0 siblings, 1 reply; 9+ messages in thread
From: Baolin Wang @ 2016-05-26 7:11 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Greg KH, Mark Brown, USB, LKML
Hi Felipe,
On 26 May 2016 at 14:22, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> When handling the endpoint interrupt handler, it maybe disable the endpoint
>> from another core user to set the USB endpoint descriptor pointor to be NULL
>> while issuing usb_gadget_giveback_request() function to release lock. So it
>> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs with
>> one NULL USB endpoint descriptor.
>
> too complex, Baolin :-) Can you see if this helps:
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=88bf752cfb55e57a78e27c931c9fef63240c739a
>
> The only situation when that can happen is while we drop our lock on
> dwc3_gadget_giveback().
OK, But line 1974 and line 2025 as below may be at risk? So I think
can we have a common place to solve the problem in case
usb_endpoint_xfer_xxx() APIs are issued at this risk? What do you
think? Thanks.
1956 static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
1957 const struct dwc3_event_depevt *event, int status)
1958 {
1959 struct dwc3_request *req;
1960 struct dwc3_trb *trb;
1961 unsigned int slot;
1962 unsigned int i;
1963 int ret;
1964
1965 do {
1966 req = next_request(&dep->req_queued);
1967 if (WARN_ON_ONCE(!req))
1968 return 1;
1969
1970 i = 0;
1971 do {
1972 slot = req->start_slot + i;
1973 if ((slot == DWC3_TRB_NUM - 1) &&
1974 usb_endpoint_xfer_isoc(dep->endpoint.desc))
1975 slot++;
1976 slot %= DWC3_TRB_NUM;
1977 trb = &dep->trb_pool[slot];
1978
1979 ret = __dwc3_cleanup_done_trbs(dwc, dep, req, trb,
1980 event, status);
1981 if (ret)
1982 break;
1983 } while (++i < req->request.num_mapped_sgs);
1984
1985 dwc3_gadget_giveback(dep, req, status);
1986
1987 if (ret)
1988 break;
1989 } while (1);
.......
2011 static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
2012 struct dwc3_ep *dep, const struct dwc3_event_depevt *event)
2013 {
2014 unsigned status = 0;
2015 int clean_busy;
2016 u32 is_xfer_complete;
2017
2018 is_xfer_complete = (event->endpoint_event ==
DWC3_DEPEVT_XFERCOMPLETE);
2019
2020 if (event->status & DEPEVT_STATUS_BUSERR)
2021 status = -ECONNRESET;
2022
2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
2024 if (clean_busy && (is_xfer_complete ||
2025
usb_endpoint_xfer_isoc(dep->endpoint.desc)))
2026 dep->flags &= ~DWC3_EP_BUSY;
>
> --
> balbi
--
Baolin.wang
Best Regards
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 7:11 ` Baolin Wang
@ 2016-05-26 7:48 ` Felipe Balbi
2016-05-26 8:24 ` Baolin Wang
0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2016-05-26 7:48 UTC (permalink / raw)
To: Baolin Wang; +Cc: Greg KH, Mark Brown, USB, LKML
[-- Attachment #1: Type: text/plain, Size: 5478 bytes --]
Hi,
Baolin Wang <baolin.wang@linaro.org> writes:
> Hi Felipe,
>
> On 26 May 2016 at 14:22, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>> When handling the endpoint interrupt handler, it maybe disable the endpoint
>>> from another core user to set the USB endpoint descriptor pointor to be NULL
>>> while issuing usb_gadget_giveback_request() function to release lock. So it
>>> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs with
>>> one NULL USB endpoint descriptor.
>>
>> too complex, Baolin :-) Can you see if this helps:
>>
>> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=88bf752cfb55e57a78e27c931c9fef63240c739a
>>
>> The only situation when that can happen is while we drop our lock on
>> dwc3_gadget_giveback().
>
> OK, But line 1974 and line 2025 as below may be at risk? So I think
> can we have a common place to solve the problem in case
> usb_endpoint_xfer_xxx() APIs are issued at this risk? What do you
> think? Thanks.
>
> 1956 static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
> 1957 const struct dwc3_event_depevt *event, int status)
> 1958 {
> 1959 struct dwc3_request *req;
> 1960 struct dwc3_trb *trb;
> 1961 unsigned int slot;
> 1962 unsigned int i;
> 1963 int ret;
> 1964
> 1965 do {
> 1966 req = next_request(&dep->req_queued);
> 1967 if (WARN_ON_ONCE(!req))
> 1968 return 1;
> 1969
> 1970 i = 0;
> 1971 do {
> 1972 slot = req->start_slot + i;
> 1973 if ((slot == DWC3_TRB_NUM - 1) &&
> 1974 usb_endpoint_xfer_isoc(dep->endpoint.desc))
this is executed still with locks held.
> 1975 slot++;
> 1976 slot %= DWC3_TRB_NUM;
> 1977 trb = &dep->trb_pool[slot];
> 1978
> 1979 ret = __dwc3_cleanup_done_trbs(dwc, dep, req, trb,
> 1980 event, status);
> 1981 if (ret)
> 1982 break;
> 1983 } while (++i < req->request.num_mapped_sgs);
> 1984
> 1985 dwc3_gadget_giveback(dep, req, status);
the problem can only show up after this call
> 1986
> 1987 if (ret)
> 1988 break;
> 1989 } while (1);
> .......
>
> 2011 static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
> 2012 struct dwc3_ep *dep, const struct dwc3_event_depevt *event)
> 2013 {
> 2014 unsigned status = 0;
> 2015 int clean_busy;
> 2016 u32 is_xfer_complete;
> 2017
> 2018 is_xfer_complete = (event->endpoint_event ==
> DWC3_DEPEVT_XFERCOMPLETE);
> 2019
> 2020 if (event->status & DEPEVT_STATUS_BUSERR)
> 2021 status = -ECONNRESET;
> 2022
> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
> 2024 if (clean_busy && (is_xfer_complete ||
> 2025
note the patch I linked you. This is where I added a bail out if the
descriptor is NULL. Here's the patch for reference:
commit 4d100e870616ccd30cf29abf21d437ec746e1f54
Author: Felipe Balbi <felipe.balbi@linux.intel.com>
Date: Wed May 18 12:37:21 2016 +0300
usb: dwc3: gadget: fix for possible endpoint disable race
when we call dwc3_gadget_giveback(), we end up
releasing our controller's lock. Another thread
could get scheduled and disable the endpoint,
subsequently setting dep->endpoint.desc to NULL.
In that case, we would end up dereferencing a NULL
pointer which would result in a Kernel Oops. Let's
avoid the problem by simply returning early if we
have a NULL descriptor.
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index f31a59cd5162..3d0573c74b13 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2019,6 +2019,14 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
break;
} while (1);
+ /*
+ * Our endpoint might get disabled by another thread during
+ * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
+ * early on so DWC3_EP_BUSY flag gets cleared
+ */
+ if (!dep->endpoint.desc)
+ return 1;
+
if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
list_empty(&dep->started_list)) {
if (list_empty(&dep->pending_list)) {
@@ -2085,6 +2093,14 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
dwc->u1u2 = 0;
}
+ /*
+ * Our endpoint might get disabled by another thread during
+ * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
+ * early on so DWC3_EP_BUSY flag gets cleared
+ */
+ if (!dep->endpoint.desc)
+ return;
+
if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
int ret;
Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
gadget.c (as in my testing/next from today) won't even get executed, so
we're safe there.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 7:48 ` Felipe Balbi
@ 2016-05-26 8:24 ` Baolin Wang
2016-05-26 9:45 ` Felipe Balbi
0 siblings, 1 reply; 9+ messages in thread
From: Baolin Wang @ 2016-05-26 8:24 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Greg KH, Mark Brown, USB, LKML
Hi,
On 26 May 2016 at 15:48, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> Hi Felipe,
>>
>> On 26 May 2016 at 14:22, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>> When handling the endpoint interrupt handler, it maybe disable the endpoint
>>>> from another core user to set the USB endpoint descriptor pointor to be NULL
>>>> while issuing usb_gadget_giveback_request() function to release lock. So it
>>>> will be one bug to check the endpoint type by usb_endpoint_xfer_xxx() APIs with
>>>> one NULL USB endpoint descriptor.
>>>
>>> too complex, Baolin :-) Can you see if this helps:
>>>
>>> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=88bf752cfb55e57a78e27c931c9fef63240c739a
>>>
>>> The only situation when that can happen is while we drop our lock on
>>> dwc3_gadget_giveback().
>>
>> OK, But line 1974 and line 2025 as below may be at risk? So I think
>> can we have a common place to solve the problem in case
>> usb_endpoint_xfer_xxx() APIs are issued at this risk? What do you
>> think? Thanks.
>>
>> 1956 static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
>> 1957 const struct dwc3_event_depevt *event, int status)
>> 1958 {
>> 1959 struct dwc3_request *req;
>> 1960 struct dwc3_trb *trb;
>> 1961 unsigned int slot;
>> 1962 unsigned int i;
>> 1963 int ret;
>> 1964
>> 1965 do {
>> 1966 req = next_request(&dep->req_queued);
>> 1967 if (WARN_ON_ONCE(!req))
>> 1968 return 1;
>> 1969
>> 1970 i = 0;
>> 1971 do {
>> 1972 slot = req->start_slot + i;
>> 1973 if ((slot == DWC3_TRB_NUM - 1) &&
>> 1974 usb_endpoint_xfer_isoc(dep->endpoint.desc))
>
> this is executed still with locks held.
Yeah, that's right.
>
>> 1975 slot++;
>> 1976 slot %= DWC3_TRB_NUM;
>> 1977 trb = &dep->trb_pool[slot];
>> 1978
>> 1979 ret = __dwc3_cleanup_done_trbs(dwc, dep, req, trb,
>> 1980 event, status);
>> 1981 if (ret)
>> 1982 break;
>> 1983 } while (++i < req->request.num_mapped_sgs);
>> 1984
>> 1985 dwc3_gadget_giveback(dep, req, status);
>
> the problem can only show up after this call
>
>> 1986
>> 1987 if (ret)
>> 1988 break;
>> 1989 } while (1);
>> .......
>>
>> 2011 static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
>> 2012 struct dwc3_ep *dep, const struct dwc3_event_depevt *event)
>> 2013 {
>> 2014 unsigned status = 0;
>> 2015 int clean_busy;
>> 2016 u32 is_xfer_complete;
>> 2017
>> 2018 is_xfer_complete = (event->endpoint_event ==
>> DWC3_DEPEVT_XFERCOMPLETE);
>> 2019
>> 2020 if (event->status & DEPEVT_STATUS_BUSERR)
>> 2021 status = -ECONNRESET;
>> 2022
>> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
>> 2024 if (clean_busy && (is_xfer_complete ||
>> 2025
>
> note the patch I linked you. This is where I added a bail out if the
> descriptor is NULL. Here's the patch for reference:
>
> commit 4d100e870616ccd30cf29abf21d437ec746e1f54
> Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> Date: Wed May 18 12:37:21 2016 +0300
>
> usb: dwc3: gadget: fix for possible endpoint disable race
>
> when we call dwc3_gadget_giveback(), we end up
> releasing our controller's lock. Another thread
> could get scheduled and disable the endpoint,
> subsequently setting dep->endpoint.desc to NULL.
>
> In that case, we would end up dereferencing a NULL
> pointer which would result in a Kernel Oops. Let's
> avoid the problem by simply returning early if we
> have a NULL descriptor.
>
> Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f31a59cd5162..3d0573c74b13 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2019,6 +2019,14 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, struct dwc3_ep *dep,
> break;
> } while (1);
>
> + /*
> + * Our endpoint might get disabled by another thread during
> + * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
> + * early on so DWC3_EP_BUSY flag gets cleared
> + */
> + if (!dep->endpoint.desc)
> + return 1;
> +
> if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> list_empty(&dep->started_list)) {
> if (list_empty(&dep->pending_list)) {
> @@ -2085,6 +2093,14 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 *dwc,
> dwc->u1u2 = 0;
> }
>
> + /*
> + * Our endpoint might get disabled by another thread during
> + * dwc3_gadget_giveback(). If that happens, we're just gonna return 1
> + * early on so DWC3_EP_BUSY flag gets cleared
> + */
> + if (!dep->endpoint.desc)
> + return;
> +
> if (!usb_endpoint_xfer_isoc(dep->endpoint.desc)) {
> int ret;
>
> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
> gadget.c (as in my testing/next from today) won't even get executed, so
> we're safe there.
Never will be executed? then we can remove the
usb_endpoint_xfer_isoc() (line 2025) at risk?
2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
2024 if (clean_busy && (is_xfer_complete ||
2025
usb_endpoint_xfer_isoc(dep->endpoint.desc)))
2026 dep->flags &= ~DWC3_EP_BUSY;
>
> --
> balbi
--
Baolin.wang
Best Regards
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 8:24 ` Baolin Wang
@ 2016-05-26 9:45 ` Felipe Balbi
2016-05-26 10:27 ` Baolin Wang
0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2016-05-26 9:45 UTC (permalink / raw)
To: Baolin Wang; +Cc: Greg KH, Mark Brown, USB, LKML
[-- Attachment #1: Type: text/plain, Size: 744 bytes --]
Hi,
Baolin Wang <baolin.wang@linaro.org> writes:
<trim>
>> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
>> gadget.c (as in my testing/next from today) won't even get executed, so
>> we're safe there.
>
> Never will be executed? then we can remove the
> usb_endpoint_xfer_isoc() (line 2025) at risk?
>
> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
> 2024 if (clean_busy && (is_xfer_complete ||
> 2025
> usb_endpoint_xfer_isoc(dep->endpoint.desc)))
> 2026 dep->flags &= ~DWC3_EP_BUSY;
hmm, now that I look at this again, in case of XferInProgress, we could
still have a problem.
I'll fix it up in that commit I pointed you to.
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 9:45 ` Felipe Balbi
@ 2016-05-26 10:27 ` Baolin Wang
2016-05-26 10:27 ` Felipe Balbi
0 siblings, 1 reply; 9+ messages in thread
From: Baolin Wang @ 2016-05-26 10:27 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Greg KH, Mark Brown, USB, LKML
On 26 May 2016 at 17:45, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>
> <trim>
>
>>> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
>>> gadget.c (as in my testing/next from today) won't even get executed, so
>>> we're safe there.
>>
>> Never will be executed? then we can remove the
>> usb_endpoint_xfer_isoc() (line 2025) at risk?
>>
>> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
>> 2024 if (clean_busy && (is_xfer_complete ||
>> 2025
>> usb_endpoint_xfer_isoc(dep->endpoint.desc)))
>> 2026 dep->flags &= ~DWC3_EP_BUSY;
>
> hmm, now that I look at this again, in case of XferInProgress, we could
> still have a problem.
>
> I'll fix it up in that commit I pointed you to.
Great. Thanks.
>
> --
> balbi
--
Baolin.wang
Best Regards
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 10:27 ` Baolin Wang
@ 2016-05-26 10:27 ` Felipe Balbi
2016-05-26 10:34 ` Baolin Wang
0 siblings, 1 reply; 9+ messages in thread
From: Felipe Balbi @ 2016-05-26 10:27 UTC (permalink / raw)
To: Baolin Wang; +Cc: Greg KH, Mark Brown, USB, LKML
[-- Attachment #1: Type: text/plain, Size: 1084 bytes --]
Hi,
Baolin Wang <baolin.wang@linaro.org> writes:
> On 26 May 2016 at 17:45, Felipe Balbi <balbi@kernel.org> wrote:
>>
>> Hi,
>>
>> Baolin Wang <baolin.wang@linaro.org> writes:
>>
>> <trim>
>>
>>>> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
>>>> gadget.c (as in my testing/next from today) won't even get executed, so
>>>> we're safe there.
>>>
>>> Never will be executed? then we can remove the
>>> usb_endpoint_xfer_isoc() (line 2025) at risk?
>>>
>>> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
>>> 2024 if (clean_busy && (is_xfer_complete ||
>>> 2025
>>> usb_endpoint_xfer_isoc(dep->endpoint.desc)))
>>> 2026 dep->flags &= ~DWC3_EP_BUSY;
>>
>> hmm, now that I look at this again, in case of XferInProgress, we could
>> still have a problem.
>>
>> I'll fix it up in that commit I pointed you to.
>
> Great. Thanks.
fixed now:
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=983b84268656ff2686253b05097d28003bbec52f
--
balbi
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type
2016-05-26 10:27 ` Felipe Balbi
@ 2016-05-26 10:34 ` Baolin Wang
0 siblings, 0 replies; 9+ messages in thread
From: Baolin Wang @ 2016-05-26 10:34 UTC (permalink / raw)
To: Felipe Balbi; +Cc: Greg KH, Mark Brown, USB, LKML
On 26 May 2016 at 18:27, Felipe Balbi <balbi@kernel.org> wrote:
>
> Hi,
>
> Baolin Wang <baolin.wang@linaro.org> writes:
>> On 26 May 2016 at 17:45, Felipe Balbi <balbi@kernel.org> wrote:
>>>
>>> Hi,
>>>
>>> Baolin Wang <baolin.wang@linaro.org> writes:
>>>
>>> <trim>
>>>
>>>>> Also note that the usb_endpoint_xfer_isoc() call on line 2067 of
>>>>> gadget.c (as in my testing/next from today) won't even get executed, so
>>>>> we're safe there.
>>>>
>>>> Never will be executed? then we can remove the
>>>> usb_endpoint_xfer_isoc() (line 2025) at risk?
>>>>
>>>> 2023 clean_busy = dwc3_cleanup_done_reqs(dwc, dep, event, status);
>>>> 2024 if (clean_busy && (is_xfer_complete ||
>>>> 2025
>>>> usb_endpoint_xfer_isoc(dep->endpoint.desc)))
>>>> 2026 dep->flags &= ~DWC3_EP_BUSY;
>>>
>>> hmm, now that I look at this again, in case of XferInProgress, we could
>>> still have a problem.
>>>
>>> I'll fix it up in that commit I pointed you to.
>>
>> Great. Thanks.
>
> fixed now:
>
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?h=testing/next&id=983b84268656ff2686253b05097d28003bbec52f
OK. I'll test it again with applying your patch. Thanks.
>
> --
> balbi
--
Baolin.wang
Best Regards
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-05-26 10:35 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-26 4:58 [PATCH] dwc3: gadget: Introduce dwc3_endpoint_xfer_xxx() to check endpoint type Baolin Wang
2016-05-26 6:22 ` Felipe Balbi
2016-05-26 7:11 ` Baolin Wang
2016-05-26 7:48 ` Felipe Balbi
2016-05-26 8:24 ` Baolin Wang
2016-05-26 9:45 ` Felipe Balbi
2016-05-26 10:27 ` Baolin Wang
2016-05-26 10:27 ` Felipe Balbi
2016-05-26 10:34 ` Baolin Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox