Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
2016-12-26 9:01 GMT+01:00 Baolin Wang :
> On some platfroms(like x86 platform), when one core is running the USB gadget
> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also can
> respond other interrupts from dwc3 controller and modify the event buffer by
> dwc3_interrupt() function, that will cause getting the wrong event count in
> irq thread handler to make the USB function abnormal.
>
> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this race.
>
Interesting, I always think we mask interrupt in dwc3_interrupt() by setting
DWC3_GEVNTSIZ_INTMASK
And unmask interrupt when we end dwc3_thread_interrupt().
So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(),
or I miss something?
Do you have some traces that indicate this masking will not work correctly?
BTW, what value you get when problem occured, 0xFFFC?
BR
Janusz
> Signed-off-by: Baolin Wang
> ---
> drivers/usb/dwc3/gadget.c |6 +-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 6785595..1a1e1f4 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2894,10 +2894,13 @@ static irqreturn_t dwc3_check_event_buf(struct
> dwc3_event_buffer *evt)
> return IRQ_HANDLED;
> }
>
> + spin_lock(&dwc->lock);
> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> count &= DWC3_GEVNTCOUNT_MASK;
> - if (!count)
> + if (!count) {
> + spin_unlock(&dwc->lock);
> return IRQ_NONE;
> + }
>
> evt->count = count;
> evt->flags |= DWC3_EVENT_PENDING;
> @@ -2914,6 +2917,7 @@ static irqreturn_t dwc3_check_event_buf(struct
> dwc3_event_buffer *evt)
> memcpy(evt->cache, evt->buf, count - amount);
>
> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
> + spin_unlock(&dwc->lock);
>
> return IRQ_WAKE_THREAD;
> }
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Janusz Dziedzic
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
Hi,
On 27 December 2016 at 18:52, Janusz Dziedzic wrote:
> 2016-12-26 9:01 GMT+01:00 Baolin Wang :
>> On some platfroms(like x86 platform), when one core is running the USB gadget
>> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also
>> can
>> respond other interrupts from dwc3 controller and modify the event buffer by
>> dwc3_interrupt() function, that will cause getting the wrong event count in
>> irq thread handler to make the USB function abnormal.
>>
>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this
>> race.
>>
> Interesting, I always think we mask interrupt in dwc3_interrupt() by setting
> DWC3_GEVNTSIZ_INTMASK
> And unmask interrupt when we end dwc3_thread_interrupt().
>
> So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(),
> or I miss something?
> Do you have some traces that indicate this masking will not work correctly?
Yes, but we just masked the interrupts described in DEVTEN register,
and we did not mask all the interrupts, like the endpoint command
complete event, transfer complete event and so on, so we can still get
interrupts.
>
> BTW, what value you get when problem occured, 0xFFFC?
Yes, something like this, the event count become huge.
>
> BR
> Janusz
>
>> Signed-off-by: Baolin Wang
>> ---
>> drivers/usb/dwc3/gadget.c |6 +-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 6785595..1a1e1f4 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2894,10 +2894,13 @@ static irqreturn_t dwc3_check_event_buf(struct
>> dwc3_event_buffer *evt)
>> return IRQ_HANDLED;
>> }
>>
>> + spin_lock(&dwc->lock);
>> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>> count &= DWC3_GEVNTCOUNT_MASK;
>> - if (!count)
>> + if (!count) {
>> + spin_unlock(&dwc->lock);
>> return IRQ_NONE;
>> + }
>>
>> evt->count = count;
>> evt->flags |= DWC3_EVENT_PENDING;
>> @@ -2914,6 +2917,7 @@ static irqreturn_t dwc3_check_event_buf(struct
>> dwc3_event_buffer *evt)
>> memcpy(evt->cache, evt->buf, count - amount);
>>
>> dwc3_writel(dwc->regs, DWC3_GEVNTCOUNT(0), count);
>> + spin_unlock(&dwc->lock);
>>
>> return IRQ_WAKE_THREAD;
>> }
>> --
>> 1.7.9.5
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Janusz Dziedzic
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
Hi, Lu Baolu writes: > On 12/26/2016 04:01 PM, Baolin Wang wrote: >> On some platfroms(like x86 platform), when one core is running the USB gadget >> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also >> can >> respond other interrupts from dwc3 controller and modify the event buffer by >> dwc3_interrupt() function, that will cause getting the wrong event count in >> irq thread handler to make the USB function abnormal. >> >> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this >> race. > > Why not spin_lock_irq ones? This lock seems to be used in both > normal and interrupt threads. Or, I missed anything? this is top half handler. Interrupts are already disabled. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
Hi, Janusz Dziedzic writes: > 2016-12-26 9:01 GMT+01:00 Baolin Wang : >> On some platfroms(like x86 platform), when one core is running the USB gadget >> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also >> can >> respond other interrupts from dwc3 controller and modify the event buffer by >> dwc3_interrupt() function, that will cause getting the wrong event count in >> irq thread handler to make the USB function abnormal. >> >> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this >> race. >> > Interesting, I always think we mask interrupt in dwc3_interrupt() by setting > DWC3_GEVNTSIZ_INTMASK > And unmask interrupt when we end dwc3_thread_interrupt(). > > So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(), > or I miss something? > Do you have some traces that indicate this masking will not work correctly? that's the very question I have. We are already masking interrupts from this controller. The only thing this could race with is usb_ep_queue(), but that gets nowhere close to anything we're doing in the top half handler, so there's really no danger of anything bad happening. I'd like to see traces as well. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
Hi,
Baolin Wang writes:
> Hi,
>
> On 27 December 2016 at 18:52, Janusz Dziedzic
> wrote:
>> 2016-12-26 9:01 GMT+01:00 Baolin Wang :
>>> On some platfroms(like x86 platform), when one core is running the USB
>>> gadget
>>> irq thread handler by dwc3_thread_interrupt(), meanwhile another core also
>>> can
>>> respond other interrupts from dwc3 controller and modify the event buffer by
>>> dwc3_interrupt() function, that will cause getting the wrong event count in
>>> irq thread handler to make the USB function abnormal.
>>>
>>> We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this
>>> race.
>>>
>> Interesting, I always think we mask interrupt in dwc3_interrupt() by setting
>> DWC3_GEVNTSIZ_INTMASK
>> And unmask interrupt when we end dwc3_thread_interrupt().
>>
>> So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(),
>> or I miss something?
>> Do you have some traces that indicate this masking will not work correctly?
>
> Yes, but we just masked the interrupts described in DEVTEN register,
> and we did not mask all the interrupts, like the endpoint command
> complete event, transfer complete event and so on, so we can still get
> interrupts.
not true, we masked interrupts for the entire event buffer:
> static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
> {
> struct dwc3 *dwc = evt->dwc;
> u32 count;
> u32 reg;
>
> if (pm_runtime_suspended(dwc->dev)) {
> pm_runtime_get(dwc->dev);
> disable_irq_nosync(dwc->irq_gadget);
> dwc->pending_events = true;
> return IRQ_HANDLED;
> }
>
> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
> count &= DWC3_GEVNTCOUNT_MASK;
> if (!count)
> return IRQ_NONE;
>
> evt->count = count;
> evt->flags |= DWC3_EVENT_PENDING;
>
> /* Mask interrupt */
> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
> reg |= DWC3_GEVNTSIZ_INTMASK;
See here ?!?
> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
>
> return IRQ_WAKE_THREAD;
> }
>> BTW, what value you get when problem occured, 0xFFFC?
>
> Yes, something like this, the event count become huge.
please send us tracepoint data. You probably need to compress
it. Something like 256k of trace data is probably enough, so:
# mkdir -p /t
# mount -t tracefs none /t
# cd /t
# echo 256 > buffer_size_kb
# echo 1 > events/dwc3/enable
# echo 0 > events/dwc3/dwc3_readl/enable
# echo 0 > events/dwc3/dwc3_writel/enable
(reproduce)
# cp /t/trace /path/to/non-volatile/media/trace.txt
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/2] usb: dwc3-pci: Fix dr_mode misspelling
Hi,
Hans de Goede writes:
> usb_get_dr_mode() expects the device-property to be spelled
> "dr_mode" not "dr-mode".
>
> Spelling it properly fixes the following warning showing up in dmesg:
> [ 8704.500545] dwc3 dwc3.2.auto: Configuration mismatch. dr_mode forced to
> gadget
>
> Signed-off-by: Hans de Goede
> ---
> drivers/usb/dwc3/dwc3-pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index 6df0f5d..9c65713 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -81,7 +81,7 @@ static int dwc3_pci_quirks(struct pci_dev *pdev, struct
> platform_device *dwc3)
> int ret;
>
> struct property_entry properties[] = {
> - PROPERTY_ENTRY_STRING("dr-mode", "peripheral"),
> + PROPERTY_ENTRY_STRING("dr_mode", "peripheral"),
> { }
> };
>
I've manually applied this to my testing/fixes. Please check that it's okay.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v2 RESEND] usb: gadget: udc-core: Rescan pending list on driver unbind
Hi,
Krzysztof Opasiak writes:
> Since:
>
> commit 855ed04a3758 ("usb: gadget: udc-core: independent registration
> of gadgets and gadget drivers")
>
> if we load gadget module but there is no free udc available
> then it will be stored on a pending gadgets list.
>
> $ modprobe g_zero.ko
> $ modprobe g_ether.ko
> [] udc-core: couldn't find an available UDC - added [g_ether] to list
> of pending drivers
>
> We scan this list each time when new UDC appears in system.
> But we can get a free UDC each time after gadget unbind.
> This commit add scanning of that list directly after unbinding
> gadget from udc.
>
> Thanks to this, when we unload first gadget:
>
> $ rmmod g_zero.ko
>
> gadget which is pending is automatically
> attached to that UDC (if name matches).
>
> Signed-off-by: Krzysztof Opasiak
Do we need to Cc stable on this?
--
balbi
signature.asc
Description: PGP signature
Re: net/gadget: slab-out-of-bounds write in dev_config
Hi,
Alan Stern writes:
> On Tue, 6 Dec 2016, Andrey Konovalov wrote:
>
>> Hi!
>>
>> I've got the following error report while running the syzkaller fuzzer.
>>
>> ep0_write() doesn't check the length, so a user can cause an
>> out-of-bounds with both size and data controlled.
>> There's a comment which says "IN DATA+STATUS caller makes len <=
>> wLength". While I'm not exactly sure what that means, the length seems
>> to be passed unmodified directly from dev_config().
>
> You're right about the comment being misleading. It looks like
> somebody forgot to actually do the check.
>
>> This doesn't seem to be a critical security issue since gadgetfs can't
>> be mounted from a user namespace.
>>
>> On commit 3c49de52d5647cda8b42c4255cf8a29d1e22eff5 (Dec 2).
>>
>> ==
>> BUG: KASAN: slab-out-of-bounds in dev_config+0x86f/0x1190 at addr
>> 88003c47e160
>> Write of size 65537 by task syz-executor0/6356
>> CPU: 3 PID: 6356 Comm: syz-executor0 Not tainted 4.9.0-rc7+ #19
>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>> 88003c107ad8 81f96aba 3dc11ef0 110007820eee
>> ed0007820ee6 88003dc11f00 41b58ab3 8598b4c8
>> 81f96828 813fb4a0 88003b6eadc0 88003c107738
>> Call Trace:
>> [< inline >] __dump_stack lib/dump_stack.c:15
>> [] dump_stack+0x292/0x398 lib/dump_stack.c:51
>> [] kasan_object_err+0x1c/0x70 mm/kasan/report.c:159
>> [< inline >] print_address_description mm/kasan/report.c:197
>> [] kasan_report_error+0x1f0/0x4e0 mm/kasan/report.c:286
>> [] kasan_report+0x35/0x40 mm/kasan/report.c:306
>> [< inline >] check_memory_region_inline mm/kasan/kasan.c:308
>> [] check_memory_region+0x139/0x190 mm/kasan/kasan.c:315
>> [] kasan_check_write+0x14/0x20 mm/kasan/kasan.c:326
>> [< inline >] copy_from_user arch/x86/include/asm/uaccess.h:689
>> [< inline >] ep0_write drivers/usb/gadget/legacy/inode.c:1135
>> [] dev_config+0x86f/0x1190
>> drivers/usb/gadget/legacy/inode.c:1759
>> [] __vfs_write+0x5d5/0x760 fs/read_write.c:510
>> [] vfs_write+0x170/0x4e0 fs/read_write.c:560
>> [< inline >] SYSC_write fs/read_write.c:607
>> [] SyS_write+0xfb/0x230 fs/read_write.c:599
>> [] entry_SYSCALL_64_fastpath+0x1f/0xc2
>
> How does this patch work out?
>
> Alan Stern
>
>
>
> Index: usb-4.x/drivers/usb/gadget/legacy/inode.c
> ===
> --- usb-4.x.orig/drivers/usb/gadget/legacy/inode.c
> +++ usb-4.x/drivers/usb/gadget/legacy/inode.c
> @@ -1126,7 +1126,7 @@ ep0_write (struct file *fd, const char _
> /* data and/or status stage for control request */
> } else if (dev->state == STATE_DEV_SETUP) {
>
> - /* IN DATA+STATUS caller makes len <= wLength */
> + len = min(len, (size_t) dev->setup_wLength);
> if (dev->setup_in) {
> retval = setup_req (dev->gadget->ep0, dev->req, len);
> if (retval == 0) {
>
I already have a patch from Greg for this. See [1]
[1]
https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=230bc0cb8ff222d9f0fbbd93a80393140b39481f
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: mtu3: fix U3 port link issue
Hi, Chunfeng Yun writes: > the issue is introduced when @is_u3_ip is used in mtu3_device_enabe() > before initialized in mtu3_mem_alloc(), so get global IP information > at first before used by following functins. > > Signed-off-by: Chunfeng Yun patch doesn't apply to my testing/fixes. Please rebase -- balbi signature.asc Description: PGP signature
Re: [PATCH v2] usb: dwc3: pci: Add "linux,sysdev_is_parent" property
Hi,
John Youn writes:
> Calling platform_device_add_properties() replaces existing properties so
> the "linux,sysdev_is_parent" property doesn't get set. Add this property
> to each platform.
>
> Fixes: d64ff406e51e ("usb: dwc3: use bus->sysdev for DMA configuration")
> Signed-off-by: John Youn
> ---
>
> Hi Felipe,
>
> Can you queue for 4.10 fixes? The original commit breaks HAPS.
I had totally missed this patch of yours and wrote my own. I'll replace
mine since yours came first. Thanks :-)
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 0/9] usb: dwc2: Fix checkpatch issues
Hi John, John Youn writes: > The dwc2 driver triggers several hundred checkpatch warnings and > checks. None of them are actual problems but this is still very noisy. > This series fixes most of those issues. > > The first patch in the series is the result of running checkpatch with > '--fix'. All of these changes were inspected for correctness. > > Regards, > John > > > John Youn (9): > usb: dwc2: Cleanup some checkpatch issues > usb: dwc2: Add identifier in prototypes > usb: dwc2: Fix comment alignment and format > usb: dwc2: Fix logical continuations > usb: dwc2: Fix brace usage > usb: dwc2: Fix lines over 80 characters > usb: dwc2: Fix code indentation after conditionals > usb: dwc2: Remove 'return' from void function > usb: dwc2: Fix sizeof in kzalloc if you need any of these fixes during the -rc cycle, you need to extract them to a series of their own and remove dependencies from cleanups. Without that, the entire series will go to -next. -- balbi signature.asc Description: PGP signature
Re: usb/gadget: warning in dummy_free_request
Hi, Andrey Konovalov writes: > Hi! > > I've got the following error report while running the syzkaller fuzzer. > > On commit 3c49de52d5647cda8b42c4255cf8a29d1e22eff5 (Dec 2). > > WARNING: CPU: 0 PID: 5257 at drivers/usb/gadget/udc/dummy_hcd.c:672 > dummy_free_request+0x153/0x170 > Kernel panic - not syncing: panic_on_warn set ... > > usb 2-1: string descriptor 0 read error: -71 > usb 2-1: New USB device found, idVendor=, idProduct= > usb 2-1: New USB device strings: Mfr=0, Product=170, SerialNumber=0 > CPU: 0 PID: 5257 Comm: syz-executor0 Not tainted 4.9.0-rc7+ #16 > Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011 > 88006af0ec48 81f96aba 0200 11000d5e1d1c > ed000d5e1d14 0a06 41b58ab3 8598b4c8 > 81f96828 41b58ab3 85942a10 81432790 > Call Trace: > [< inline >] __dump_stack lib/dump_stack.c:15 > [] dump_stack+0x292/0x398 lib/dump_stack.c:51 > [] panic+0x1cb/0x3a9 kernel/panic.c:179 > [] __warn+0x1c4/0x1e0 kernel/panic.c:542 > [] warn_slowpath_null+0x2c/0x40 kernel/panic.c:585 > [] dummy_free_request+0x153/0x170 > drivers/usb/gadget/udc/dummy_hcd.c:672 > [] usb_ep_free_request+0xc0/0x420 > drivers/usb/gadget/udc/core.c:195 > [] gadgetfs_unbind+0x131/0x190 > drivers/usb/gadget/legacy/inode.c:1612 > [] usb_gadget_remove_driver+0x10f/0x2b0 > drivers/usb/gadget/udc/core.c:1228 > [] usb_gadget_unregister_driver+0x1b6/0x2c0 > drivers/usb/gadget/udc/core.c:1357 > [] dev_release+0x80/0x160 > drivers/usb/gadget/legacy/inode.c:1187 > [] __fput+0x332/0x7f0 fs/file_table.c:208 > [] fput+0x15/0x20 fs/file_table.c:244 > [] task_work_run+0x19b/0x270 kernel/task_work.c:116 > [< inline >] exit_task_work include/linux/task_work.h:21 > [] do_exit+0x16aa/0x2530 kernel/exit.c:828 > [] do_group_exit+0x149/0x420 kernel/exit.c:932 > [] get_signal+0x76d/0x17b0 kernel/signal.c:2307 > [] do_signal+0xd2/0x2120 arch/x86/kernel/signal.c:807 > [] exit_to_usermode_loop+0x170/0x200 > arch/x86/entry/common.c:156 > [< inline >] prepare_exit_to_usermode arch/x86/entry/common.c:190 > [] syscall_return_slowpath+0x3d3/0x420 > arch/x86/entry/common.c:259 > [] entry_SYSCALL_64_fastpath+0xc0/0xc2 > Dumping ftrace buffer: >(ftrace buffer empty) > Kernel Offset: disabled There have been several emails like this one. Can you check if my branch testing/fixes is working for you? -- balbi signature.asc Description: PGP signature
[PATCH 3/3] usb: dwc3: core: avoid Overflow events
Now that we're handling so many transfers at a time and for some dwc3 revisions LPM events *must* be enabled, we can fall into a situation where too many events fire and we start receiving Overflow events. Let's do what XHCI does and allocate a full page for the Event Ring, this will avoid any future issues. Cc: # v4.9 Signed-off-by: Felipe Balbi --- drivers/usb/dwc3/core.h | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 99fa8b8b06fe..3cc09542f32a 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -43,9 +43,7 @@ #define DWC3_XHCI_RESOURCES_NUM2 #define DWC3_SCRATCHBUF_SIZE 4096/* each buffer is assumed to be 4KiB */ -#define DWC3_EVENT_SIZE4 /* bytes */ -#define DWC3_EVENT_MAX_NUM 64 /* 2 events/endpoint */ -#define DWC3_EVENT_BUFFERS_SIZE(DWC3_EVENT_SIZE * DWC3_EVENT_MAX_NUM) +#define DWC3_EVENT_BUFFERS_SIZE4096 #define DWC3_EVENT_TYPE_MASK 0xfe #define DWC3_EVENT_TYPE_DEV0 -- 2.11.0 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] usb: dwc3: gadget: make use of No Response Update Transfer
commit 8897a761c371ceefda98570953185daa3c059ad1 upstream
No Response Update Transfer is a special type of
Update Transfer command which can be used whenever
we're not relying on XferNotReady to prepare
transfers. With this, we don't need to wait for
CMDACT to be cleared and issue further commands to
the endpoint straight away.
Let's start using this version to skip the long-ish
wait.
Cc: # v4.9
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 24 +++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 178aece29b26..ae01c2c1cc5c 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -233,6 +233,7 @@ static int __dwc3_gadget_wakeup(struct dwc3 *dwc);
int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
struct dwc3_gadget_ep_cmd_params *params)
{
+ const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
struct dwc3 *dwc = dep->dwc;
u32 timeout = 500;
u32 reg;
@@ -276,7 +277,28 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned
cmd,
dwc3_writel(dep->regs, DWC3_DEPCMDPAR1, params->param1);
dwc3_writel(dep->regs, DWC3_DEPCMDPAR2, params->param2);
- dwc3_writel(dep->regs, DWC3_DEPCMD, cmd | DWC3_DEPCMD_CMDACT);
+ /*
+* Synopsys Databook 2.60a states in section 6.3.2.5.6 of that if we're
+* not relying on XferNotReady, we can make use of a special "No
+* Response Update Transfer" command where we should clear both CmdAct
+* and CmdIOC bits.
+*
+* With this, we don't need to wait for command completion and can
+* straight away issue further commands to the endpoint.
+*
+* NOTICE: We're making an assumption that control endpoints will never
+* make use of Update Transfer command. This is a safe assumption
+* because we can never have more than one request at a time with
+* Control Endpoints. If anybody changes that assumption, this chunk
+* needs to be updated accordingly.
+*/
+ if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_UPDATETRANSFER &&
+ !usb_endpoint_xfer_isoc(desc))
+ cmd &= ~(DWC3_DEPCMD_CMDIOC | DWC3_DEPCMD_CMDACT);
+ else
+ cmd |= DWC3_DEPCMD_CMDACT;
+
+ dwc3_writel(dep->regs, DWC3_DEPCMD, cmd);
do {
reg = dwc3_readl(dep->regs, DWC3_DEPCMD);
if (!(reg & DWC3_DEPCMD_CMDACT)) {
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] usb: dwc3: gadget: properly check ep cmd
commit 514f227b20addf01297b3df24be6b4161f69 upstream.
The cmd argument we pass to
dwc3_send_gadget_ep_cmd() could contain extra
arguments embedded. When checking for StartTransfer
command, we need to make sure to match only lower 4
bits which contain the actual command and ignore the
rest.
Cc: # v4.9
Reported-by: Janusz Dziedzic
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/core.h | 2 ++
drivers/usb/dwc3/gadget.c | 2 +-
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 6b60e42626a2..99fa8b8b06fe 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -450,6 +450,8 @@
#define DWC3_DEPCMD_SETTRANSFRESOURCE (0x02 << 0)
#define DWC3_DEPCMD_SETEPCONFIG(0x01 << 0)
+#define DWC3_DEPCMD_CMD(x) ((x) & 0xf)
+
/* The EP number goes 0..31 so ep0 is always out and ep1 is always in */
#define DWC3_DALEPENA_EP(n)(1 << n)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1dfa56a5f1c5..178aece29b26 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -258,7 +258,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned
cmd,
}
}
- if (cmd == DWC3_DEPCMD_STARTTRANSFER) {
+ if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
int needs_wakeup;
needs_wakeup = (dwc->link_state == DWC3_LINK_STATE_U1 ||
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] usb: dwc3: gadget: always unmap EP0 requests
commit 0416e494ce7d ("usb: dwc3: ep0: correct cache
sync issue in case of ep0_bounced") introduced a bug
where we would leak DMA resources which would cause
us to starve the system of them resulting in failing
DMA transfers.
Fix the bug by making sure that we always unmap EP0
requests since those are *always* mapped.
Fixes: 0416e494ce7d ("usb: dwc3: ep0: correct cache
sync issue in case of ep0_bounced")
Cc:
Tested-by: Tomasz Medrek
Reported-by: Janusz Dziedzic
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/gadget.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index efddaf5d11d1..0286e3141864 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -180,11 +180,11 @@ void dwc3_gadget_giveback(struct dwc3_ep *dep, struct
dwc3_request *req,
if (req->request.status == -EINPROGRESS)
req->request.status = status;
- if (dwc->ep0_bounced && dep->number == 0)
+ if (dwc->ep0_bounced && dep->number <= 1)
dwc->ep0_bounced = false;
- else
- usb_gadget_unmap_request_by_dev(dwc->sysdev,
- &req->request, req->direction);
+
+ usb_gadget_unmap_request_by_dev(dwc->sysdev,
+ &req->request, req->direction);
trace_dwc3_gadget_giveback(req);
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] usb: dwc3: ep0: add dwc3_ep0_prepare_one_trb()
For now this is just a cleanup patch, no functional
changes. We will be using the new function to fix a
bug introduced long ago by commit 0416e494ce7d
("usb: dwc3: ep0: correct cache sync issue in case
of ep0_bounced") and further worsened by commit
c0bd5456a470 ("usb: dwc3: ep0: handle non maxpacket
aligned transfers > 512")
Cc:
Reported-by: Janusz Dziedzic
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/ep0.c | 26 --
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 4878d187c7d4..06febd07ec4e 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -39,18 +39,13 @@ static void __dwc3_ep0_do_control_status(struct dwc3 *dwc,
struct dwc3_ep *dep);
static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
struct dwc3_ep *dep, struct dwc3_request *req);
-static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
- u32 len, u32 type, bool chain)
+static void dwc3_ep0_prepare_one_trb(struct dwc3 *dwc, u8 epnum,
+ dma_addr_t buf_dma, u32 len, u32 type, bool chain)
{
- struct dwc3_gadget_ep_cmd_params params;
struct dwc3_trb *trb;
struct dwc3_ep *dep;
- int ret;
-
dep = dwc->eps[epnum];
- if (dep->flags & DWC3_EP_BUSY)
- return 0;
trb = &dwc->ep0_trb[dep->trb_enqueue];
@@ -71,15 +66,26 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum,
dma_addr_t buf_dma,
trb->ctrl |= (DWC3_TRB_CTRL_IOC
| DWC3_TRB_CTRL_LST);
- if (chain)
+ trace_dwc3_prepare_trb(dep, trb);
+}
+
+static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
+ u32 len, u32 type, bool chain)
+{
+ struct dwc3_gadget_ep_cmd_params params;
+ struct dwc3_ep *dep;
+ int ret;
+
+ dep = dwc->eps[epnum];
+ if (dep->flags & DWC3_EP_BUSY)
return 0;
+ dwc3_ep0_prepare_one_trb(dwc, epnum, buf_dma, len, type, chain);
+
memset(¶ms, 0, sizeof(params));
params.param0 = upper_32_bits(dwc->ep0_trb_addr);
params.param1 = lower_32_bits(dwc->ep0_trb_addr);
- trace_dwc3_prepare_trb(dep, trb);
-
ret = dwc3_send_gadget_ep_cmd(dep, DWC3_DEPCMD_STARTTRANSFER, ¶ms);
if (ret < 0)
return ret;
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] usb: dwc3: ep0: explicitly call dwc3_ep0_prepare_one_trb()
Let's call dwc3_ep0_prepare_one_trb() explicitly
because there are occasions where we will need more
than one TRB to handle an EP0 transfer.
A follow-up patch will fix one bug related to
multiple-TRB Data Phases when it comes to
mapping/unmapping requests for DMA.
Cc:
Reported-by: Janusz Dziedzic
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/ep0.c | 28 +++-
1 file changed, 15 insertions(+), 13 deletions(-)
diff --git a/drivers/usb/dwc3/ep0.c b/drivers/usb/dwc3/ep0.c
index 06febd07ec4e..9bb1f8526f3e 100644
--- a/drivers/usb/dwc3/ep0.c
+++ b/drivers/usb/dwc3/ep0.c
@@ -69,8 +69,7 @@ static void dwc3_ep0_prepare_one_trb(struct dwc3 *dwc, u8
epnum,
trace_dwc3_prepare_trb(dep, trb);
}
-static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum, dma_addr_t buf_dma,
- u32 len, u32 type, bool chain)
+static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum)
{
struct dwc3_gadget_ep_cmd_params params;
struct dwc3_ep *dep;
@@ -80,8 +79,6 @@ static int dwc3_ep0_start_trans(struct dwc3 *dwc, u8 epnum,
dma_addr_t buf_dma,
if (dep->flags & DWC3_EP_BUSY)
return 0;
- dwc3_ep0_prepare_one_trb(dwc, epnum, buf_dma, len, type, chain);
-
memset(¶ms, 0, sizeof(params));
params.param0 = upper_32_bits(dwc->ep0_trb_addr);
params.param1 = lower_32_bits(dwc->ep0_trb_addr);
@@ -286,8 +283,9 @@ void dwc3_ep0_out_start(struct dwc3 *dwc)
complete(&dwc->ep0_in_setup);
- ret = dwc3_ep0_start_trans(dwc, 0, dwc->ctrl_req_addr, 8,
+ dwc3_ep0_prepare_one_trb(dwc, 0, dwc->ctrl_req_addr, 8,
DWC3_TRBCTL_CONTROL_SETUP, false);
+ ret = dwc3_ep0_start_trans(dwc, 0);
WARN_ON(ret < 0);
}
@@ -918,9 +916,9 @@ static void dwc3_ep0_complete_data(struct dwc3 *dwc,
dwc->ep0_next_event = DWC3_EP0_COMPLETE;
- ret = dwc3_ep0_start_trans(dwc, epnum,
- dwc->ctrl_req_addr, 0,
- DWC3_TRBCTL_CONTROL_DATA, false);
+ dwc3_ep0_prepare_one_trb(dwc, epnum, dwc->ctrl_req_addr,
+ 0, DWC3_TRBCTL_CONTROL_DATA, false);
+ ret = dwc3_ep0_start_trans(dwc, epnum);
WARN_ON(ret < 0);
}
}
@@ -999,9 +997,10 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
req->direction = !!dep->number;
if (req->request.length == 0) {
- ret = dwc3_ep0_start_trans(dwc, dep->number,
+ dwc3_ep0_prepare_one_trb(dwc, dep->number,
dwc->ctrl_req_addr, 0,
DWC3_TRBCTL_CONTROL_DATA, false);
+ ret = dwc3_ep0_start_trans(dwc, dep->number);
} else if (!IS_ALIGNED(req->request.length, dep->endpoint.maxpacket)
&& (dep->number == 0)) {
u32 transfer_size = 0;
@@ -1017,7 +1016,7 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
if (req->request.length > DWC3_EP0_BOUNCE_SIZE) {
transfer_size = ALIGN(req->request.length - maxpacket,
maxpacket);
- ret = dwc3_ep0_start_trans(dwc, dep->number,
+ dwc3_ep0_prepare_one_trb(dwc, dep->number,
req->request.dma,
transfer_size,
DWC3_TRBCTL_CONTROL_DATA,
@@ -1029,18 +1028,20 @@ static void __dwc3_ep0_do_control_data(struct dwc3 *dwc,
dwc->ep0_bounced = true;
- ret = dwc3_ep0_start_trans(dwc, dep->number,
+ dwc3_ep0_prepare_one_trb(dwc, dep->number,
dwc->ep0_bounce_addr, transfer_size,
DWC3_TRBCTL_CONTROL_DATA, false);
+ ret = dwc3_ep0_start_trans(dwc, dep->number);
} else {
ret = usb_gadget_map_request_by_dev(dwc->sysdev,
&req->request, dep->number);
if (ret)
return;
- ret = dwc3_ep0_start_trans(dwc, dep->number, req->request.dma,
+ dwc3_ep0_prepare_one_trb(dwc, dep->number, req->request.dma,
req->request.length, DWC3_TRBCTL_CONTROL_DATA,
false);
+ ret = dwc3_ep0_start_trans(dwc, dep->number);
}
WARN_ON(ret < 0);
@@ -1054,8 +1055,9 @@ static int dwc3_ep0_start_control_status(struct dwc3_ep
*dep)
type = dwc->three_stage_setup ? DWC3_TRBCTL_CONTROL_STATUS3
: DWC3_TRBCTL_CONTROL_STATUS2;
- return dwc3_ep0_start_trans(dwc, dep->number,
+ dwc3_ep0_prepare_one_trb(dwc, de
Re: [PATCH] usb: dwc3: gadget: Avoid race between dwc3 interrupt handler and irq thread handler
Hi,
On 27 December 2016 at 19:11, Felipe Balbi wrote:
>
> Hi,
>
> Baolin Wang writes:
>> Hi,
>>
>> On 27 December 2016 at 18:52, Janusz Dziedzic
>> wrote:
>>> 2016-12-26 9:01 GMT+01:00 Baolin Wang :
On some platfroms(like x86 platform), when one core is running the USB
gadget
irq thread handler by dwc3_thread_interrupt(), meanwhile another core also
can
respond other interrupts from dwc3 controller and modify the event buffer
by
dwc3_interrupt() function, that will cause getting the wrong event count in
irq thread handler to make the USB function abnormal.
We should add spin_lock/unlock() in dwc3_check_event_buf() to avoid this
race.
>>> Interesting, I always think we mask interrupt in dwc3_interrupt() by setting
>>> DWC3_GEVNTSIZ_INTMASK
>>> And unmask interrupt when we end dwc3_thread_interrupt().
>>>
>>> So, we shouldn't get any IRQ from HW during dwc3_thread_interrupt(),
>>> or I miss something?
>>> Do you have some traces that indicate this masking will not work correctly?
>>
>> Yes, but we just masked the interrupts described in DEVTEN register,
>> and we did not mask all the interrupts, like the endpoint command
>> complete event, transfer complete event and so on, so we can still get
>> interrupts.
>
> not true, we masked interrupts for the entire event buffer:
Yes, you are right and I missed that. I should reproduce this problem
and analyse the real reason.
>
>> static irqreturn_t dwc3_check_event_buf(struct dwc3_event_buffer *evt)
>> {
>> struct dwc3 *dwc = evt->dwc;
>> u32 count;
>> u32 reg;
>>
>> if (pm_runtime_suspended(dwc->dev)) {
>> pm_runtime_get(dwc->dev);
>> disable_irq_nosync(dwc->irq_gadget);
>> dwc->pending_events = true;
>> return IRQ_HANDLED;
>> }
>>
>> count = dwc3_readl(dwc->regs, DWC3_GEVNTCOUNT(0));
>> count &= DWC3_GEVNTCOUNT_MASK;
>> if (!count)
>> return IRQ_NONE;
>>
>> evt->count = count;
>> evt->flags |= DWC3_EVENT_PENDING;
>>
>> /* Mask interrupt */
>> reg = dwc3_readl(dwc->regs, DWC3_GEVNTSIZ(0));
>> reg |= DWC3_GEVNTSIZ_INTMASK;
>
> See here ?!?
>
>> dwc3_writel(dwc->regs, DWC3_GEVNTSIZ(0), reg);
>>
>> return IRQ_WAKE_THREAD;
>> }
>
>>> BTW, what value you get when problem occured, 0xFFFC?
>>
>> Yes, something like this, the event count become huge.
>
> please send us tracepoint data. You probably need to compress
> it. Something like 256k of trace data is probably enough, so:
>
> # mkdir -p /t
> # mount -t tracefs none /t
> # cd /t
> # echo 256 > buffer_size_kb
> # echo 1 > events/dwc3/enable
> # echo 0 > events/dwc3/dwc3_readl/enable
> # echo 0 > events/dwc3/dwc3_writel/enable
>
> (reproduce)
>
> # cp /t/trace /path/to/non-volatile/media/trace.txt
Okay, I try to do that. Thanks.
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc3: pci: add Intel Gemini Lake PCI ID
From: Heikki Krogerus
Intel Gemini Lake SoC has the same DWC3 than Broxton. Add
the new ID to the supported Devices.
Signed-off-by: Heikki Krogerus
Signed-off-by: Felipe Balbi
---
drivers/usb/dwc3/dwc3-pci.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index b01156343513..cce0a220b6b0 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -38,6 +38,7 @@
#define PCI_DEVICE_ID_INTEL_BXT_M 0x1aaa
#define PCI_DEVICE_ID_INTEL_APL0x5aaa
#define PCI_DEVICE_ID_INTEL_KBP0xa2b0
+#define PCI_DEVICE_ID_INTEL_GLK0x31aa
#define PCI_INTEL_BXT_DSM_UUID "732b85d5-b7a7-4a1b-9ba0-4bbd00ffd511"
#define PCI_INTEL_BXT_FUNC_PMU_PWR 4
@@ -267,6 +268,7 @@ static const struct pci_device_id dwc3_pci_id_table[] = {
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_BXT_M), },
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_APL), },
{ PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_KBP), },
+ { PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_GLK), },
{ PCI_DEVICE(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_NL_USB), },
{ }/* Terminating Entry */
};
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMS56x not working reliably with uas driver
On Thu, 2016-12-22 at 17:44 -0500, Alan Stern wrote: > I don't see how this patch fixes anything. Unless I'm mistaken, it > just avoids the problem by preventing the system from issuing the > command that provokes the error, rather than really fixing the > underlying error. Please clarify. If a reset leads to a disconnect, isn't that exactly what we want? Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] usb: musb: blackfin: add bfin_fifo_offset in bfin_ops
On Thu, Dec 22, 2016 at 05:11:20PM +0100, Jérémy Lefaure wrote:
> From: Jérémy Lefaure
>
> The function bfin_fifo_offset is defined but not used:
>
> drivers/usb/musb/blackfin.c:36:12: warning: ‘bfin_fifo_offset’ defined
> but not used [-Wunused-function]
> static u32 bfin_fifo_offset(u8 epnum)
> ^~~~
>
> Adding bfin_fifo_offset to bfin_ops fixes this warning and allows musb
> core to call this function instead of default_fifo_offset.
>
> Fixes: cc92f6818f6e ("usb: musb: Populate new IO functions for blackfin")
> Signed-off-by: Jérémy Lefaure
Applied. Thanks.
-Bin.
> ---
> drivers/usb/musb/blackfin.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/musb/blackfin.c b/drivers/usb/musb/blackfin.c
> index f43edc43268b..4418574a36a1 100644
> --- a/drivers/usb/musb/blackfin.c
> +++ b/drivers/usb/musb/blackfin.c
> @@ -469,6 +469,7 @@ static const struct musb_platform_ops bfin_ops = {
> .init = bfin_musb_init,
> .exit = bfin_musb_exit,
>
> + .fifo_offset= bfin_fifo_offset,
> .readb = bfin_readb,
> .writeb = bfin_writeb,
> .readw = bfin_readw,
> --
> 2.11.0
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: net2280 Driver Bug
On Mon, 26 Dec 2016, Linus Torvalds wrote:
> On Dec 26, 2016 4:04 PM, "Alan Stern" wrote:
>
>
> Note that the usage of tmp in the "if (unlikely(req->td->dmadesc == 0)) {"
> branch really is not conflicting, because that branch breaks out of the
> enclosing "while" loop.
>
>
> No.
>
> Look closer.
>
> It does indeed breast out of the loop, but before it dies that, it uses
> "tmp" again. And it wants the *old* tmp, not the new one..
Are you certain that it doesn't want the new value of tmp? How can you
tell? -- it was not immediately obvious to me. This was what I had in
mind when I wrote that it wasn't clear whether your patch was entirely
correct.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: net/gadget: slab-out-of-bounds write in dev_config
On Tue, 27 Dec 2016, Felipe Balbi wrote:
>
> Hi,
>
> Alan Stern writes:
> > On Tue, 6 Dec 2016, Andrey Konovalov wrote:
> >
> >> Hi!
> >>
> >> I've got the following error report while running the syzkaller fuzzer.
> >>
> >> ep0_write() doesn't check the length, so a user can cause an
> >> out-of-bounds with both size and data controlled.
> >> There's a comment which says "IN DATA+STATUS caller makes len <=
> >> wLength". While I'm not exactly sure what that means, the length seems
> >> to be passed unmodified directly from dev_config().
> >
> > You're right about the comment being misleading. It looks like
> > somebody forgot to actually do the check.
> >
> >> This doesn't seem to be a critical security issue since gadgetfs can't
> >> be mounted from a user namespace.
> >>
> >> On commit 3c49de52d5647cda8b42c4255cf8a29d1e22eff5 (Dec 2).
> >>
> >> ==
> >> BUG: KASAN: slab-out-of-bounds in dev_config+0x86f/0x1190 at addr
> >> 88003c47e160
> >> Write of size 65537 by task syz-executor0/6356
> >> CPU: 3 PID: 6356 Comm: syz-executor0 Not tainted 4.9.0-rc7+ #19
> >> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs
> >> 01/01/2011
> >> 88003c107ad8 81f96aba 3dc11ef0 110007820eee
> >> ed0007820ee6 88003dc11f00 41b58ab3 8598b4c8
> >> 81f96828 813fb4a0 88003b6eadc0 88003c107738
> >> Call Trace:
> >> [< inline >] __dump_stack lib/dump_stack.c:15
> >> [] dump_stack+0x292/0x398 lib/dump_stack.c:51
> >> [] kasan_object_err+0x1c/0x70 mm/kasan/report.c:159
> >> [< inline >] print_address_description mm/kasan/report.c:197
> >> [] kasan_report_error+0x1f0/0x4e0 mm/kasan/report.c:286
> >> [] kasan_report+0x35/0x40 mm/kasan/report.c:306
> >> [< inline >] check_memory_region_inline mm/kasan/kasan.c:308
> >> [] check_memory_region+0x139/0x190 mm/kasan/kasan.c:315
> >> [] kasan_check_write+0x14/0x20 mm/kasan/kasan.c:326
> >> [< inline >] copy_from_user arch/x86/include/asm/uaccess.h:689
> >> [< inline >] ep0_write drivers/usb/gadget/legacy/inode.c:1135
> >> [] dev_config+0x86f/0x1190
> >> drivers/usb/gadget/legacy/inode.c:1759
> >> [] __vfs_write+0x5d5/0x760 fs/read_write.c:510
> >> [] vfs_write+0x170/0x4e0 fs/read_write.c:560
> >> [< inline >] SYSC_write fs/read_write.c:607
> >> [] SyS_write+0xfb/0x230 fs/read_write.c:599
> >> [] entry_SYSCALL_64_fastpath+0x1f/0xc2
> >
> > How does this patch work out?
> >
> > Alan Stern
> >
> >
> >
> > Index: usb-4.x/drivers/usb/gadget/legacy/inode.c
> > ===
> > --- usb-4.x.orig/drivers/usb/gadget/legacy/inode.c
> > +++ usb-4.x/drivers/usb/gadget/legacy/inode.c
> > @@ -1126,7 +1126,7 @@ ep0_write (struct file *fd, const char _
> > /* data and/or status stage for control request */
> > } else if (dev->state == STATE_DEV_SETUP) {
> >
> > - /* IN DATA+STATUS caller makes len <= wLength */
> > + len = min(len, (size_t) dev->setup_wLength);
> > if (dev->setup_in) {
> > retval = setup_req (dev->gadget->ep0, dev->req, len);
> > if (retval == 0) {
> >
>
> I already have a patch from Greg for this. See [1]
>
> [1]
> https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/commit/?id=230bc0cb8ff222d9f0fbbd93a80393140b39481f
The two patches fix different problems. My patch goes on the pathway
where dev->state > STATE_DEV_OPENED in dev_config(), and Greg's patch
handles the case where it is <=.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMS56x not working reliably with uas driver
On Tue, 27 Dec 2016, Oliver Neukum wrote: > On Thu, 2016-12-22 at 17:44 -0500, Alan Stern wrote: > > I don't see how this patch fixes anything. Unless I'm mistaken, it > > just avoids the problem by preventing the system from issuing the > > command that provokes the error, rather than really fixing the > > underlying error. > > Please clarify. If a reset leads to a disconnect, isn't that > exactly what we want? I didn't express myself clearly enough. Yes, if a reset leads to a disconnect then avoiding the reset will avoid problems. But the _real_ error here is that xhci-hcd says "ERROR Transfer event for disabled endpoint or incorrect stream ring" when the disconnect occurs during reset. That shouldn't happen, no matter what quirks the device has. It indicates a bug either in uas or in xhci-hcd. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMS56x not working reliably with uas driver
On Tue, 2016-12-27 at 10:20 -0500, Alan Stern wrote: > On Tue, 27 Dec 2016, Oliver Neukum wrote: > > > On Thu, 2016-12-22 at 17:44 -0500, Alan Stern wrote: > > > I don't see how this patch fixes anything. Unless I'm mistaken, it > > > just avoids the problem by preventing the system from issuing the > > > command that provokes the error, rather than really fixing the > > > underlying error. > > > > Please clarify. If a reset leads to a disconnect, isn't that > > exactly what we want? > > I didn't express myself clearly enough. Yes, if a reset leads to a > disconnect then avoiding the reset will avoid problems. Good. Then we need to clarify whether the device was physically disconnected when the logs were taken. > But the _real_ error here is that xhci-hcd says "ERROR Transfer event > for disabled endpoint or incorrect stream ring" when the disconnect > occurs during reset. That shouldn't happen, no matter what quirks the > device has. It indicates a bug either in uas or in xhci-hcd. True. I am afraid that there necessarily is a window for resetting a disconnected device. But the check you proposed is better. however, I'd like to encapsulate that together with a test for logical disconnect. Uas is unlikely to be the only driver that has this issue. Regards Oliver -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMS56x not working reliably with uas driver
Hi Alan, On Tuesday 27 December 2016 08:50 PM, Alan Stern wrote: On Tue, 27 Dec 2016, Oliver Neukum wrote: On Thu, 2016-12-22 at 17:44 -0500, Alan Stern wrote: I don't see how this patch fixes anything. Unless I'm mistaken, it just avoids the problem by preventing the system from issuing the command that provokes the error, rather than really fixing the underlying error. Please clarify. If a reset leads to a disconnect, isn't that exactly what we want? I didn't express myself clearly enough. Yes, if a reset leads to a disconnect then avoiding the reset will avoid problems. But the _real_ error here is that xhci-hcd says "ERROR Transfer event for disabled endpoint or incorrect stream ring" when the disconnect occurs during reset. I think there is some misunderstanding of the issues. "ERROR Transfer event for disabled endpoint or incorrect stream ring" This particular message is during the connect of the device and not during the disconnect. To avoid this message the unusual_uas.h patch was sent earlier. During disconnect of the device I get "scsi host4: uas_post_reset: alloc streams error -19 after reset" and I dont get the same with the modified patch which Alan suggested, instead I get a proper disconnect. That shouldn't happen, no matter what quirks the device has. It indicates a bug either in uas or in xhci-hcd. Alan Stern Regards, -George -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] HID: usbhid: Add quirk for the Futaba TOSD-5711BB VFD
Sorry, first time patcher here, and forgot to include the list when I
sent this patch to Jiri and Benjamin last week.
The Futaba TOSD-5711BB VFD crashes when the initial HID report is requested,
register the display in hid-ids and tell hid-quirks to not do the init.
Signed-off-by: Alex Wood
---
drivers/hid/hid-ids.h | 3 +++
drivers/hid/usbhid/hid-quirks.c | 1 +
2 files changed, 4 insertions(+)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 575aa65..5843f25 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -362,6 +362,9 @@
#define USB_VENDOR_ID_FLATFROG 0x25b5
#define USB_DEVICE_ID_MULTITOUCH_3200 0x0002
+#define USB_VENDOR_ID_FUTABA0x0547
+#define USB_DEVICE_ID_LED_DISPLAY 0x7000
+
#define USB_VENDOR_ID_ESSENTIAL_REALITY0x0d7f
#define USB_DEVICE_ID_ESSENTIAL_REALITY_P5 0x0100
diff --git a/drivers/hid/usbhid/hid-quirks.c b/drivers/hid/usbhid/hid-quirks.c
index e6cfd32..aee5adc 100644
--- a/drivers/hid/usbhid/hid-quirks.c
+++ b/drivers/hid/usbhid/hid-quirks.c
@@ -86,6 +86,7 @@ static const struct hid_blacklist {
{ USB_VENDOR_ID_ELO, USB_DEVICE_ID_ELO_TS2700, HID_QUIRK_NOGET },
{ USB_VENDOR_ID_FORMOSA, USB_DEVICE_ID_FORMOSA_IR_RECEIVER,
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_FREESCALE, USB_DEVICE_ID_FREESCALE_MX28,
HID_QUIRK_NOGET },
+ { USB_VENDOR_ID_FUTABA, USB_DEVICE_ID_LED_DISPLAY,
HID_QUIRK_NO_INIT_REPORTS },
{ USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0A4A, HID_QUIRK_ALWAYS_POLL },
{ USB_VENDOR_ID_HP,
USB_PRODUCT_ID_HP_LOGITECH_OEM_USB_OPTICAL_MOUSE_0B4A, HID_QUIRK_ALWAYS_POLL },
{ USB_VENDOR_ID_HP, USB_PRODUCT_ID_HP_PIXART_OEM_USB_OPTICAL_MOUSE,
HID_QUIRK_ALWAYS_POLL },
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: mtu3: fix U3 port link issue
Hi, On Tue, 2016-12-27 at 13:22 +0200, Felipe Balbi wrote: > Hi, > > Chunfeng Yun writes: > > the issue is introduced when @is_u3_ip is used in mtu3_device_enabe() > > before initialized in mtu3_mem_alloc(), so get global IP information > > at first before used by following functins. > > > > Signed-off-by: Chunfeng Yun > > patch doesn't apply to my testing/fixes. Please rebase It's already accepted into kernel4.10-rc1 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: net2280 Driver Bug
I'm not entirely sure either, that is why I wanted the specs.
Alan sent me the PLX NET2280 specs, but I'm using the USB3380/USB3381 - the
USB3 version of the device.
As it seems from the code, the HW interface is mostly the same, with some minor
changes, so it is the same driver (net2280.c) but with some branches for
USB3380 when needed.
Maybe the problem has something to do with a minor difference between the
NET2280 and USB3380, that was over looked when adding the support for the
USB3380.
So, if any of you have the specs for that one, I would love to get them and see
if there is such a difference in that area.
Anyway, as for the proposed path, this was my logic:
1. The req->td->dmadesc equals to 0 iff:
-- There was a transaction ending with a short packet, and
-- The read() to read it was shorter than the transaction length, and
-- The read() to complete it is longer than the residue.
I believe this is true from the printouts of various cases, but I can't be
positive it is correct.
2. Entering this if, there should be no more data in the endpoint (a short
packet terminated the transaction). If there is, the transaction wasn't really
done and we should exit and wait for it to finish entirely. That is the inner
if.
That inner if should never happen, but it is there to be on the safe side.
That is why it is marked with the comment /* paranoia */.
The size of the data available in the endpoint is ep->dma->dmacount and it
is read to tmp.
This entire clause is based on my own educated guesses.
3. If we passed that inner if without breaking in the original code, than tmp &
DMA_BYTE_MASK_COUNT== 0.
That means we will always pass dma bytes count of 0 to dma_done(), meaning
all the requested bytes were read.
4. dma_done() reports back to the upper layer that the request (read()) was
done and how many bytes were read. In the original code that would always be
the request size, regardless of the actual size of the data.
That did not make sense to me at all.
5. However, the original value of tmp is req->td->dmacount, which is the
dmacount value when the request's dma transaction was finished. And that is a
much more reasonable value to report back to the caller.
As you can see, this is based a lot on educated guesses and debug printouts of
various cases. That is why I would like to get your input on this, to make sure
I'm on the right track.
To recreate the problem., try reading from a bulk out endpoint in a loop, 1024
* n bytes in each iteration. Connect the PLX to a host you can control, and
send to that endpoint 1024 * n +x bytes such that 0 < x < 1024 * n and (x %
1024) != 0
You would expect the first read() to return 1024 * n and the second read to
return x, but you will get the first read to return 1024 *n and the second one
to return 1024 * n.
That is true for every positive integer n.
My patch, solves the problem, and does not break any of the other cases I've
tried.
To conclude:
I would love to get your input on this patch, as it is based on personal
debugging and guesses, without knowing the HW specs.
I would also love to get the USB3380 specs, so I could verify some aspects of
this problem myself, and for future references.
Peace and love and marry Christmas and happy Hanukah,
Raz
-Original Message-
From: Alan Stern [mailto:st...@rowland.harvard.edu]
Sent: Tuesday, December 27, 2016 5:06 PM
To: Linus Torvalds
Cc: Greg Kroah-Hartman ; USB list
; Felipe Balbi ; Raz Manor
; Tim Harvey ; Heikki Krogerus
; Jussi Kivilinna
; Colin Ian King
Subject: RE: net2280 Driver Bug
On Mon, 26 Dec 2016, Linus Torvalds wrote:
> On Dec 26, 2016 4:04 PM, "Alan Stern" wrote:
>
>
> Note that the usage of tmp in the "if (unlikely(req->td->dmadesc == 0)) {"
> branch really is not conflicting, because that branch breaks out of
> the enclosing "while" loop.
>
>
> No.
>
> Look closer.
>
> It does indeed breast out of the loop, but before it dies that, it
> uses "tmp" again. And it wants the *old* tmp, not the new one..
Are you certain that it doesn't want the new value of tmp? How can you tell?
-- it was not immediately obvious to me. This was what I had in mind when I
wrote that it wasn't clear whether your patch was entirely correct.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: JMS56x not working reliably with uas driver
On Tue, 27 Dec 2016, George Cherian wrote: > Hi Alan, > > > On Tuesday 27 December 2016 08:50 PM, Alan Stern wrote: > > On Tue, 27 Dec 2016, Oliver Neukum wrote: > > > >> On Thu, 2016-12-22 at 17:44 -0500, Alan Stern wrote: > >>> I don't see how this patch fixes anything. Unless I'm mistaken, it > >>> just avoids the problem by preventing the system from issuing the > >>> command that provokes the error, rather than really fixing the > >>> underlying error. > >> Please clarify. If a reset leads to a disconnect, isn't that > >> exactly what we want? > > I didn't express myself clearly enough. Yes, if a reset leads to a > > disconnect then avoiding the reset will avoid problems. > > > > But the _real_ error here is that xhci-hcd says "ERROR Transfer event > > for disabled endpoint or incorrect stream ring" when the disconnect > > occurs during reset. > I think there is some misunderstanding of the issues. > > "ERROR Transfer event for disabled endpoint or incorrect stream ring" > This particular message is during the connect of the device and not > during the disconnect. > To avoid this message the unusual_uas.h patch was sent earlier. Right -- the patch _avoids_ the message. But it doesn't fix the actual error/bug that caused to message to be logged in the first place. > During disconnect of the device I get "scsi host4: uas_post_reset: alloc > streams error -19 after reset" and > I dont get the same with the modified patch which Alan suggested, > instead I get a proper disconnect. Good. On Tue, 27 Dec 2016, Oliver Neukum wrote: > On Tue, 2016-12-27 at 10:20 -0500, Alan Stern wrote: > > On Tue, 27 Dec 2016, Oliver Neukum wrote: > > > > > On Thu, 2016-12-22 at 17:44 -0500, Alan Stern wrote: > > > > I don't see how this patch fixes anything. Unless I'm mistaken, it > > > > just avoids the problem by preventing the system from issuing the > > > > command that provokes the error, rather than really fixing the > > > > underlying error. > > > > > > Please clarify. If a reset leads to a disconnect, isn't that > > > exactly what we want? > > > > I didn't express myself clearly enough. Yes, if a reset leads to a > > disconnect then avoiding the reset will avoid problems. > > Good. Then we need to clarify whether the device was physically > disconnected when the logs were taken. According to George, it was connected when the first error message occurred and physically disconnected when the second message occurred. > > But the _real_ error here is that xhci-hcd says "ERROR Transfer event > > for disabled endpoint or incorrect stream ring" when the disconnect > > occurs during reset. That shouldn't happen, no matter what quirks the > > device has. It indicates a bug either in uas or in xhci-hcd. > > True. I am afraid that there necessarily is a window for resetting a > disconnected device. But the check you proposed is better. > however, I'd like to encapsulate that together with a test for > logical disconnect. Uas is unlikely to be the only driver that has > this issue. True enough. We could have a usb_device_is_disconnected() inline helper for this purpose. There's no need to try to distinguish between physical and logical disconnects, as far as I can see. However, as George points out, the "Transfer event" error has nothing to do with disconnection. It was triggered by the device's bogus response to a REPORT OPCODES command, or something of the sort. We should be able to handle bogus responses without logging ERROR messages, because such messages indicate a bug in a kernel driver. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
A lot of messages "gspca_zc3xx 1-9.1.1:1.0: URB error -71, resubmitting" in kernel log without clear reason
Hi! Any idea how find root of problem? Thanks. -- Best Regards, Mike Gavrilov. system.log.bz2 Description: BZip2 compressed data
