All of lore.kernel.org
 help / color / mirror / Atom feed
* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-08-02  0:45 He, Bo
  0 siblings, 0 replies; 17+ messages in thread
From: He, Bo @ 2018-08-02  0:45 UTC (permalink / raw)
  To: Vincent Pelletier, Felipe Balbi
  Cc: Alan Stern, Roger Quadros, Lars-Peter Clausen, Sam Protsenko,
	linux-usb, Praneeth Bajjuri, Bai, Jie A

Your patch fix the issue BUG: scheduling while atomic:
    BUG: scheduling while atomic: SettingsProvide/3433/0x00000104
    Preemption disabled at:
    [<ffffffff81e00053>] __do_softirq+0x53/0x31b
    CPU: 1 PID: 3433 Comm: SettingsProvide Tainted: P     U
    ilt-2e5dc0ac-g3f662bf-dirty #8
    Call Trace:
     <IRQ>
     dump_stack+0x70/0x9e
     __schedule_bug+0x7f/0xd0
     __schedule+0x61d/0x860
     schedule+0x36/0x90
     dwc3_gadget_ep_dequeue+0x27a/0x2e0 [dwc3]
     usb_ep_dequeue+0x23/0x90
     ffs_aio_cancel+0x4c/0x70
     kiocb_cancel+0x40/0x50
     free_ioctx_users+0x6e/0x100
     percpu_ref_switch_to_atomic_rcu+0x159/0x160
     rcu_process_callbacks+0x1a7/0x520
     __do_softirq+0x11a/0x31b
     irq_exit+0xb5/0xc0
     smp_apic_timer_interrupt+0x7c/0x160

    the BUG is introduced form the patch:
    commit: cf3113d89
    usb: dwc3: gadget: properly increment dequeue pointer on ep_dequeue

   the commit: cf3113d89 call the below function maybe in softirq context:
wait_event_lock_irq(dep->wait_end_transfer,
                                       !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
                                       dwc->lock);

-----Original Message-----
From: Vincent Pelletier <plr.vincent@gmail.com> 
Sent: Wednesday, August 1, 2018 11:04 PM
To: Felipe Balbi <felipe.balbi@linux.intel.com>
Cc: Alan Stern <stern@rowland.harvard.edu>; Roger Quadros <rogerq@ti.com>; Lars-Peter Clausen <lars@metafoo.de>; Sam Protsenko <semen.protsenko@linaro.org>; linux-usb@vger.kernel.org; Praneeth Bajjuri <praneeth@ti.com>; He, Bo <bo.he@intel.com>; Bai, Jie A <jie.a.bai@intel.com>
Subject: Re: usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers

Hello,

Bo He, CC'ed, found a regression introduced in my patch, discussed in this thread, and submitted a patch:
  Subject: [PATCH] fix panic at pwq_activate_delayed_work.
  Date: Wed, 1 Aug 2018 10:14:38 +0000
  Message-ID: <CD6925E8781EFD4D8E11882D20FC406D529834D2@SHSMSX104.ccr.corp.intel.com>

On Fri, 29 Jun 2018 09:32:43 +0300, Felipe Balbi <felipe.balbi@linux.intel.com> wrote:
> Hmm, that's what I remember, but we don't have that documented and 
> dwc3 has a sleep in its dequeue, which I need to remove for other 
> reasons anyway.

Given the above comment from Felipe, I expected my patch would be dropped in favour of making dwc3 not sleep in dequeue, as it seems to be the actual root cause.

Should my patch be reverted ? It adds complexity which, I believe, becomes superfluous if dequeue does not sleep anywhere.

Or maybe non-sleeping dequeue is not there yet, and a solution right now (later revertable) is better, in which case my change would be worth fixing ?
---
Vincent Pelletier
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2019-01-17 16:29 Evan Green
  0 siblings, 0 replies; 17+ messages in thread
From: Evan Green @ 2019-01-17 16:29 UTC (permalink / raw)
  To: He, Bo; +Cc: linux-usb, Sam Protsenko, Felipe Balbi, Alan Stern

On Wed, Jan 16, 2019 at 7:37 PM He, Bo <bo.he@intel.com> wrote:
>
> Hi, Green:
>         if you check the latest kernel, you can see the below 3 patches introduce the cancelled_list feature can fix the issue:
>
> commit fec9095bdef4e7c988adb603d0d4f92ee735d4a1
> Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> Date:   Wed Aug 1 13:56:50 2018 +0300
>
>     usb: dwc3: gadget: remove wait_end_transfer
>
>     Now that we have a list of cancelled requests, we can skip over TRBs
>     when END_TRANSFER command completes.
>
>     Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> commit d4f1afe5e896c18ae01099a85dab5e1a198bd2a8
> Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> Date:   Wed Aug 1 13:54:25 2018 +0300
>
>     usb: dwc3: gadget: move requests to cancelled_list
>
>     Whenever we have a request in flight, we can move it to the cancelled
>     list and later simply iterate over that list and skip over any TRBs we
>     find.
>
>     Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>
> commit d5443bbf5fc8f8389cce146b1fc2987cdd229d12
> Author: Felipe Balbi <felipe.balbi@linux.intel.com>
> Date:   Wed Aug 1 13:53:29 2018 +0300
>
>     usb: dwc3: gadget: introduce cancelled_list
>
>     This list will host cancelled requests who still have TRBs being
>     processed.
>
>     Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
>

Oh, excellent. I'll check those out. Thanks!
-Evan

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2019-01-16 23:56 Evan Green
  0 siblings, 0 replies; 17+ messages in thread
From: Evan Green @ 2019-01-16 23:56 UTC (permalink / raw)
  To: linux-usb; +Cc: Sam Protsenko, Felipe Balbi, Alan Stern, bo.he

On Wed, Jan 16, 2019 at 3:20 PM Vincent Pelletier <evgreen@chromium.org> wrote:
>
> This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
> as is the case for (at least) dwc3.
>
> [  382.200896] BUG: scheduling while atomic: screen/1808/0x00000100
> [  382.207124] 4 locks held by screen/1808:
> [  382.211266]  #0:  (rcu_callback){....}, at: [<c10b4ff0>] rcu_process_callbacks+0x260/0x440
> [  382.219949]  #1:  (rcu_read_lock_sched){....}, at: [<c1358ba0>] percpu_ref_switch_to_atomic_rcu+0xb0/0x130
> [  382.230034]  #2:  (&(&ctx->ctx_lock)->rlock){....}, at: [<c11f0c73>] free_ioctx_users+0x23/0xd0
> [  382.230096]  #3:  (&(&ffs->eps_lock)->rlock){....}, at: [<f81e7710>] ffs_aio_cancel+0x20/0x60 [usb_f_fs]
> [  382.230160] Modules linked in: usb_f_fs libcomposite configfs bnep btsdio bluetooth ecdh_generic brcmfmac brcmutil intel_powerclamp coretemp dwc3 kvm_intel ulpi udc_core kvm irqbypass crc32_pclmul crc32c_intel pcbc dwc3_pci aesni_intel aes_i586 crypto_simd cryptd ehci_pci ehci_hcd gpio_keys usbcore basincove_gpadc industrialio usb_common
> [  382.230407] CPU: 1 PID: 1808 Comm: screen Not tainted 4.14.0-edison+ #117
> [  382.230416] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
> [  382.230425] Call Trace:
> [  382.230438]  <SOFTIRQ>
> [  382.230466]  dump_stack+0x47/0x62
> [  382.230498]  __schedule_bug+0x61/0x80
> [  382.230522]  __schedule+0x43/0x7a0
> [  382.230587]  schedule+0x5f/0x70
> [  382.230625]  dwc3_gadget_ep_dequeue+0x14c/0x270 [dwc3]
> [  382.230669]  ? do_wait_intr_irq+0x70/0x70
> [  382.230724]  usb_ep_dequeue+0x19/0x90 [udc_core]
> [  382.230770]  ffs_aio_cancel+0x37/0x60 [usb_f_fs]
> [  382.230798]  kiocb_cancel+0x31/0x40
> [  382.230822]  free_ioctx_users+0x4d/0xd0
> [  382.230858]  percpu_ref_switch_to_atomic_rcu+0x10a/0x130
> [  382.230881]  ? percpu_ref_exit+0x40/0x40
> [  382.230904]  rcu_process_callbacks+0x2b3/0x440
> [  382.230965]  __do_softirq+0xf8/0x26b
> [  382.231011]  ? __softirqentry_text_start+0x8/0x8
> [  382.231033]  do_softirq_own_stack+0x22/0x30
> [  382.231042]  </SOFTIRQ>
> [  382.231071]  irq_exit+0x45/0xc0
> [  382.231089]  smp_apic_timer_interrupt+0x13c/0x150
> [  382.231118]  apic_timer_interrupt+0x35/0x3c
> [  382.231132] EIP: __copy_user_ll+0xe2/0xf0
> [  382.231142] EFLAGS: 00210293 CPU: 1
> [  382.231154] EAX: bfd4508c EBX: 00000004 ECX: 00000003 EDX: f3d8fe50
> [  382.231165] ESI: f3d8fe51 EDI: bfd4508d EBP: f3d8fe14 ESP: f3d8fe08
> [  382.231176]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> [  382.231265]  core_sys_select+0x25f/0x320
> [  382.231346]  ? __wake_up_common_lock+0x62/0x80
> [  382.231399]  ? tty_ldisc_deref+0x13/0x20
> [  382.231438]  ? ldsem_up_read+0x1b/0x40
> [  382.231459]  ? tty_ldisc_deref+0x13/0x20
> [  382.231479]  ? tty_write+0x29f/0x2e0
> [  382.231514]  ? n_tty_ioctl+0xe0/0xe0
> [  382.231541]  ? tty_write_unlock+0x30/0x30
> [  382.231566]  ? __vfs_write+0x22/0x110
> [  382.231604]  ? security_file_permission+0x2f/0xd0
> [  382.231635]  ? rw_verify_area+0xac/0x120
> [  382.231677]  ? vfs_write+0x103/0x180
> [  382.231711]  SyS_select+0x87/0xc0
> [  382.231739]  ? SyS_write+0x42/0x90
> [  382.231781]  do_fast_syscall_32+0xd6/0x1a0
> [  382.231836]  entry_SYSENTER_32+0x47/0x71
> [  382.231848] EIP: 0xb7f75b05
> [  382.231857] EFLAGS: 00000246 CPU: 1
> [  382.231868] EAX: ffffffda EBX: 00000400 ECX: bfd4508c EDX: bfd4510c
> [  382.231878] ESI: 00000000 EDI: 00000000 EBP: 00000000 ESP: bfd45020
> [  382.231889]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b
> [  382.232281] softirq: huh, entered softirq 9 RCU c10b4d90 with preempt_count 00000100, exited with 00000000?
>
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> Tested-by: Sam Protsenko <semen.protsenko@linaro.org>
> Signed-off-by: he, bo <bo.he@intel.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>

Hi Vincent,
We finally caught up to the apply and revert of this change, and are
now experiencing the issue that this patch originally tried to fix. Is
anybody still looking at this issue?

Before I saw all this on the lists I was doing some thinking about how
to make dwc3_gadget_ep_dequeue not sleep. Basically that would mean
spinning without the sleep somehow. This seemed to get pretty tricky
with what appears to be a queue-like nature for dwc3 interrupts (I am
not at all familiar with dwc3). You'd have to go chase down where the
interrupt could be, either in the hardware or in the software queue.

But then I wondered about the original nature of needing to wait for
the transfer completion in order to remove all the TRBs. Is this
because we're worried that the hardware will be sitting on top of a
TRB we're removing, so then we free and corrupt the next pointer, and
then hardware follows it somewhere crazy? Does DWC3 have a register
for seeing which TRB is currently being processed? If so, could we
have a while loop near clearing the _HWO bit to make sure hardware is
not looking at each TRB we are clearing out.

Or maybe more simply, is there a way to stop the whole machine and
then restart it in a graceful way?

-Evan
ps- Apologies for replying to the original message and not the end of
the thread. I had to bounce the message into my inbox, and couldn't
figure out how to have Patchwork give me the full thread.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-10-23 14:22 Lars-Peter Clausen
  0 siblings, 0 replies; 17+ messages in thread
From: Lars-Peter Clausen @ 2018-10-23 14:22 UTC (permalink / raw)
  To: Felipe Balbi, Alan Stern, Roger Quadros
  Cc: Sam Protsenko, Vincent Pelletier, linux-usb, Praneeth Bajjuri

On 06/29/2018 08:32 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Alan Stern <stern@rowland.harvard.edu> writes:
>> On Thu, 21 Jun 2018, Roger Quadros wrote:
>>
>>>>> Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue() directly from here?
>>>>>> What is the purpose of the spin_lock()?
>>>>
>>>> I agree that the lock doesn't seem to be necessary. But I believe the whole
>>>> thing is already running in non-sleeping context, even before the spinlock
>>>> is taken. So this wouldn't help much.
>>>>
>>>> Even the io_cancel() syscall takes a spinlock before invoking the cancel
>>>> function. So this issue is not exclusive to program termination.
>>>>
>>>> Are there any documented guidelines on which context usb_ep_dequeue() should
>>>> be able to be called in? The sleep in the dwc3 driver seems to be a recent
>>>> addition.
>>>
>>> drivers/usb/udc/gadget/core.c has the only documentation, but context is not mentioned there.
>>> Felipe, what do you suggest?
>>
>> As far as I remember, usb_ep_dequeue() is supposed to be more or less 
>> analogous to usb_ep_queue(); drivers should be allowed to call either 
>> routine in an atomic context.
> 
> Hmm, that's what I remember, but we don't have that documented and dwc3
> has a sleep in its dequeue, which I need to remove for other reasons
> anyway.
> 
> Can we get a patch updating documentation to make it clear that both
> queue and dequeue should be callable from any context?
> 

Felipe, for the DWC3 do you see an issue with moving the giveback and the
cleanup of the TRBs simply into END_TRANSFER interrupt? That seems to work
for me for fixing the issue, but I don't have a full understanding of all
the dependencies of the DWC3 gadget driver and don't know if it could
introduce other issues.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-10-23 12:20 Lars-Peter Clausen
  0 siblings, 0 replies; 17+ messages in thread
From: Lars-Peter Clausen @ 2018-10-23 12:20 UTC (permalink / raw)
  To: Vincent Pelletier, He, Bo
  Cc: Felipe Balbi, Alan Stern, Roger Quadros, Sam Protsenko,
	linux-usb, Praneeth Bajjuri, Bai, Jie A

On 09/25/2018 02:46 PM, Vincent Pelletier wrote:
> Hello,
> 
> On Thu, 2 Aug 2018 14:23:51 +0000, Vincent Pelletier
> <plr.vincent@gmail.com> wrote:
>> On Thu, 2 Aug 2018 00:45:14 +0000, "He, Bo" <bo.he@intel.com> wrote:
>>> Your patch fix the issue BUG: scheduling while atomic:  
>>
>> Yes, although from my understanding of Felipe's answer, the actual bug
>> is the "scheduling" part (sleeping in dwc3 UDC) rather than the
>> "atomic" part.
>>
>> So my patch addresses, still if my understanding is correct, the wrong
>> half of the problem, and even introduced the regression you identified.
>> Hence my uncertainty...
> 
> I notice that neither He's patch, nor a dwc3 change to prevent it from
> scheduling inside usb_ep_dequeue are in Linus' master. Please correct
> me if I missed something.
> 
> Just in case my previous emails were not clear:
> - I have no objection to He's patch on its own (and I do not know the
>   code nearly enough to provide a meaningful reviewed-by).
> - I do not intend to work on making changes to dwc3 gadget to stop it
>   from scheduling in usb_ep_dequeue in the foreseeable future.
> 
> So if there is no ongoing work on dwc3 cancel behaviour (Felipe ?),
> please do resume/carry on with reviewing and integrating He's patch.
> 
> It is only *if* dwc3 cancel stops scheduling that I believe my patch
> should be reverted (here is the hash as of Linus' master):
>   commit d52e4d0c0c428bf2ba35074a7495cdb28e2efbae
>   Author: Vincent Pelletier <plr.vincent@gmail.com>
>   Date:   Wed Jun 13 11:05:06 2018 +0000
> 
>       usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
> 
>       This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
>       as is the case for (at least) dwc3.
> and, in my understanding, a consequence is that He's fix would not
> be needed anymore - the bug my patch introduced disappearing in the
> revert.

Hi,

I just started testing with your patch applied and the patch seems to
contain a race condition on its own.

When a aio_cancel() is called ffs_aio_cancel_worker() is queued. Now what
might happen is that the URB completes on its own before the work item has a
chance to run. And it seems that ffs_copy_worker() can even overtake the
cancel work. In that case the data structure associated with the URB is
deleted while the cancel_worker is still queued.

I get the following output on the console

[   91.974151] ODEBUG: free active (active state 0) object type: work_struct
hint: ffs_aio_cancel_worker+0x0/0x20
[   92.211124] [<ffffff8008466ca4>] debug_print_object+0xac/0xc0
[   92.216854] [<ffffff8008467e48>] __debug_check_no_obj_freed+0x1b0/0x208
[   92.223451] [<ffffff8008468104>] debug_check_no_obj_freed+0x1c/0x28
[   92.229701] [<ffffff80081b1a40>] kfree+0x78/0xc8
[   92.234302] [<ffffff80086e7060>] ffs_user_copy_worker+0xd0/0x128
[   92.240293] [<ffffff80080b8620>] process_one_work+0x1e0/0x3c0
[   92.246022] [<ffffff80080b8854>] worker_thread+0x54/0x410
[   92.251403] [<ffffff80080bf8fc>] kthread+0x104/0x130
[   92.256351] [<ffffff8008084db0>] ret_from_fork+0x10/0x18

- Lars

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-09-27  2:31 He, Bo
  0 siblings, 0 replies; 17+ messages in thread
From: He, Bo @ 2018-09-27  2:31 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: Felipe Balbi, Alan Stern, Roger Quadros, Lars-Peter Clausen,
	Sam Protsenko, linux-usb, Praneeth Bajjuri, Bai, Jie A

Hi, Vincent:
	We tried my patch doesn't fix the issue totally, we still see the kernel panic.
	currently solution in our test is: Rvert "usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers"
	and only add the below patch to use the in_interrupt() to avoid the wait_event_lock_irq() is called in interrupt context.

Subject: [PATCH] fix the wait_event_lock_irq run in interrupt context

the patch is to fix the below bug:
BUG: scheduling while atomic: SettingsProvide/3433/0x00000104
Preemption disabled at:
[<ffffffff81e00053>] __do_softirq+0x53/0x31b
CPU: 1 PID: 3433 Comm: SettingsProvide Tainted: P     U
ilt-2e5dc0ac-g3f662bf-dirty #8
Call Trace:
 <IRQ>
 dump_stack+0x70/0x9e
 ? __do_softirq+0x53/0x31b
 __schedule_bug+0x7f/0xd0
 __schedule+0x61d/0x860
 ? _raw_spin_unlock_irqrestore+0x28/0x50
 ? prepare_to_wait_event+0x87/0x170
 schedule+0x36/0x90
 dwc3_gadget_ep_dequeue+0x27a/0x2e0 [dwc3]
 ? finish_wait+0x90/0x90
 usb_ep_dequeue+0x23/0x90
 ffs_aio_cancel+0x4c/0x70
 kiocb_cancel+0x40/0x50
 free_ioctx_users+0x6e/0x100
 percpu_ref_switch_to_atomic_rcu+0x159/0x160
 rcu_process_callbacks+0x1a7/0x520
 __do_softirq+0x11a/0x31b
 irq_exit+0xb5/0xc0
 smp_apic_timer_interrupt+0x7c/0x160

the BUG is introduced form the patch:
commit: cf3113d89
usb: dwc3: gadget: properly increment dequeue pointer on ep_dequeue

Signed-off-by: he, bo <bo.he@intel.com>
---
 drivers/usb/dwc3/gadget.c | 112 +++++++++++++++++++++++-----------------------
 1 file changed, 57 insertions(+), 55 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 7120678..386fd82 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1413,65 +1413,67 @@ static int dwc3_gadget_ep_dequeue(struct usb_ep *ep,
 			/* wait until it is processed */
 			dwc3_stop_active_transfer(dwc, dep->number, true);
 
-			/*
-			 * If request was already started, this means we had to
-			 * stop the transfer. With that we also need to ignore
-			 * all TRBs used by the request, however TRBs can only
-			 * be modified after completion of END_TRANSFER
-			 * command. So what we do here is that we wait for
-			 * END_TRANSFER completion and only after that, we jump
-			 * over TRBs by clearing HWO and incrementing dequeue
-			 * pointer.
-			 *
-			 * Note that we have 2 possible types of transfers here:
-			 *
-			 * i) Linear buffer request
-			 * ii) SG-list based request
-			 *
-			 * SG-list based requests will have r->num_pending_sgs
-			 * set to a valid number (> 0). Linear requests,
-			 * normally use a single TRB.
-			 *
-			 * For each of these two cases, if r->unaligned flag is
-			 * set, one extra TRB has been used to align transfer
-			 * size to wMaxPacketSize.
-			 *
-			 * All of these cases need to be taken into
-			 * consideration so we don't mess up our TRB ring
-			 * pointers.
-			 */
-			wait_event_lock_irq(dep->wait_end_transfer,
-					!(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
-					dwc->lock);
-
-			if (!r->trb)
-				goto out0;
-
-			if (r->num_pending_sgs) {
-				struct dwc3_trb *trb;
-				int i = 0;
-
-				for (i = 0; i < r->num_pending_sgs; i++) {
-					trb = r->trb + i;
-					trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
-					dwc3_ep_inc_deq(dep);
-				}
+			if(likely(!in_interrupt())) {
+				/*
+				 * If request was already started, this means we had to
+				 * stop the transfer. With that we also need to ignore
+				 * all TRBs used by the request, however TRBs can only
+				 * be modified after completion of END_TRANSFER
+				 * command. So what we do here is that we wait for
+				 * END_TRANSFER completion and only after that, we jump
+				 * over TRBs by clearing HWO and incrementing dequeue
+				 * pointer.
+				 *
+				 * Note that we have 2 possible types of transfers here:
+				 *
+				 * i) Linear buffer request
+				 * ii) SG-list based request
+				 *
+				 * SG-list based requests will have r->num_pending_sgs
+				 * set to a valid number (> 0). Linear requests,
+				 * normally use a single TRB.
+				 *
+				 * For each of these two cases, if r->unaligned flag is
+				 * set, one extra TRB has been used to align transfer
+				 * size to wMaxPacketSize.
+				 *
+				 * All of these cases need to be taken into
+				 * consideration so we don't mess up our TRB ring
+				 * pointers.
+				 */
+				wait_event_lock_irq(dep->wait_end_transfer,
+						!(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
+						dwc->lock);
+
+				if (!r->trb)
+					goto out0;
+
+				if (r->num_pending_sgs) {
+					struct dwc3_trb *trb;
+					int i = 0;
+
+					for (i = 0; i < r->num_pending_sgs; i++) {
+						trb = r->trb + i;
+						trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+						dwc3_ep_inc_deq(dep);
+					}
+
+					if (r->unaligned || r->zero) {
+						trb = r->trb + r->num_pending_sgs + 1;
+						trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+						dwc3_ep_inc_deq(dep);
+					}
+				} else {
+					struct dwc3_trb *trb = r->trb;
 
-				if (r->unaligned || r->zero) {
-					trb = r->trb + r->num_pending_sgs + 1;
 					trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
 					dwc3_ep_inc_deq(dep);
-				}
-			} else {
-				struct dwc3_trb *trb = r->trb;
 
-				trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
-				dwc3_ep_inc_deq(dep);
-
-				if (r->unaligned || r->zero) {
-					trb = r->trb + 1;
-					trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
-					dwc3_ep_inc_deq(dep);
+					if (r->unaligned || r->zero) {
+						trb = r->trb + 1;
+						trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
+						dwc3_ep_inc_deq(dep);
+					}
 				}
 			}
 			goto out1;

^ permalink raw reply related	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-09-25 12:46 Vincent Pelletier
  0 siblings, 0 replies; 17+ messages in thread
From: Vincent Pelletier @ 2018-09-25 12:46 UTC (permalink / raw)
  To: He, Bo
  Cc: Felipe Balbi, Alan Stern, Roger Quadros, Lars-Peter Clausen,
	Sam Protsenko, linux-usb, Praneeth Bajjuri, Bai, Jie A

Hello,

On Thu, 2 Aug 2018 14:23:51 +0000, Vincent Pelletier
<plr.vincent@gmail.com> wrote:
> On Thu, 2 Aug 2018 00:45:14 +0000, "He, Bo" <bo.he@intel.com> wrote:
> > Your patch fix the issue BUG: scheduling while atomic:  
> 
> Yes, although from my understanding of Felipe's answer, the actual bug
> is the "scheduling" part (sleeping in dwc3 UDC) rather than the
> "atomic" part.
> 
> So my patch addresses, still if my understanding is correct, the wrong
> half of the problem, and even introduced the regression you identified.
> Hence my uncertainty...

I notice that neither He's patch, nor a dwc3 change to prevent it from
scheduling inside usb_ep_dequeue are in Linus' master. Please correct
me if I missed something.

Just in case my previous emails were not clear:
- I have no objection to He's patch on its own (and I do not know the
  code nearly enough to provide a meaningful reviewed-by).
- I do not intend to work on making changes to dwc3 gadget to stop it
  from scheduling in usb_ep_dequeue in the foreseeable future.

So if there is no ongoing work on dwc3 cancel behaviour (Felipe ?),
please do resume/carry on with reviewing and integrating He's patch.

It is only *if* dwc3 cancel stops scheduling that I believe my patch
should be reverted (here is the hash as of Linus' master):
  commit d52e4d0c0c428bf2ba35074a7495cdb28e2efbae
  Author: Vincent Pelletier <plr.vincent@gmail.com>
  Date:   Wed Jun 13 11:05:06 2018 +0000

      usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers

      This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
      as is the case for (at least) dwc3.
and, in my understanding, a consequence is that He's fix would not
be needed anymore - the bug my patch introduced disappearing in the
revert.

Regards,

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-08-02 14:23 Vincent Pelletier
  0 siblings, 0 replies; 17+ messages in thread
From: Vincent Pelletier @ 2018-08-02 14:23 UTC (permalink / raw)
  To: He, Bo
  Cc: Felipe Balbi, Alan Stern, Roger Quadros, Lars-Peter Clausen,
	Sam Protsenko, linux-usb, Praneeth Bajjuri, Bai, Jie A

On Thu, 2 Aug 2018 00:45:14 +0000, "He, Bo" <bo.he@intel.com> wrote:
> Your patch fix the issue BUG: scheduling while atomic:

Yes, although from my understanding of Felipe's answer, the actual bug
is the "scheduling" part (sleeping in dwc3 UDC) rather than the
"atomic" part.

So my patch addresses, still if my understanding is correct, the wrong
half of the problem, and even introduced the regression you identified.
Hence my uncertainty...

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-08-01 15:03 Vincent Pelletier
  0 siblings, 0 replies; 17+ messages in thread
From: Vincent Pelletier @ 2018-08-01 15:03 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: Alan Stern, Roger Quadros, Lars-Peter Clausen, Sam Protsenko,
	linux-usb, Praneeth Bajjuri, He, Bo, Bai, Jie A

Hello,

Bo He, CC'ed, found a regression introduced in my patch, discussed in
this thread, and submitted a patch:
  Subject: [PATCH] fix panic at pwq_activate_delayed_work.
  Date: Wed, 1 Aug 2018 10:14:38 +0000
  Message-ID: <CD6925E8781EFD4D8E11882D20FC406D529834D2@SHSMSX104.ccr.corp.intel.com>

On Fri, 29 Jun 2018 09:32:43 +0300, Felipe Balbi
<felipe.balbi@linux.intel.com> wrote:
> Hmm, that's what I remember, but we don't have that documented and dwc3
> has a sleep in its dequeue, which I need to remove for other reasons
> anyway.

Given the above comment from Felipe, I expected my patch would be
dropped in favour of making dwc3 not sleep in dequeue, as it seems to
be the actual root cause.

Should my patch be reverted ? It adds complexity which, I believe,
becomes superfluous if dequeue does not sleep anywhere.

Or maybe non-sleeping dequeue is not there yet, and a solution right now
(later revertable) is better, in which case my change would be worth
fixing ?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-29  6:32 Felipe Balbi
  0 siblings, 0 replies; 17+ messages in thread
From: Felipe Balbi @ 2018-06-29  6:32 UTC (permalink / raw)
  To: Alan Stern, Roger Quadros
  Cc: Lars-Peter Clausen, Sam Protsenko, Vincent Pelletier, linux-usb,
	Praneeth Bajjuri

Hi,

Alan Stern <stern@rowland.harvard.edu> writes:
> On Thu, 21 Jun 2018, Roger Quadros wrote:
>
>> >> Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue() directly from here?
>> >>> What is the purpose of the spin_lock()?
>> > 
>> > I agree that the lock doesn't seem to be necessary. But I believe the whole
>> > thing is already running in non-sleeping context, even before the spinlock
>> > is taken. So this wouldn't help much.
>> > 
>> > Even the io_cancel() syscall takes a spinlock before invoking the cancel
>> > function. So this issue is not exclusive to program termination.
>> > 
>> > Are there any documented guidelines on which context usb_ep_dequeue() should
>> > be able to be called in? The sleep in the dwc3 driver seems to be a recent
>> > addition.
>> 
>> drivers/usb/udc/gadget/core.c has the only documentation, but context is not mentioned there.
>> Felipe, what do you suggest?
>
> As far as I remember, usb_ep_dequeue() is supposed to be more or less 
> analogous to usb_ep_queue(); drivers should be allowed to call either 
> routine in an atomic context.

Hmm, that's what I remember, but we don't have that documented and dwc3
has a sleep in its dequeue, which I need to remove for other reasons
anyway.

Can we get a patch updating documentation to make it clear that both
queue and dequeue should be callable from any context?

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-21 15:30 Alan Stern
  0 siblings, 0 replies; 17+ messages in thread
From: Alan Stern @ 2018-06-21 15:30 UTC (permalink / raw)
  To: Roger Quadros
  Cc: Lars-Peter Clausen, Sam Protsenko, Vincent Pelletier,
	Felipe Balbi, linux-usb, Praneeth Bajjuri, Felipe Balbi

On Thu, 21 Jun 2018, Roger Quadros wrote:

> >> Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue() directly from here?
> >>> What is the purpose of the spin_lock()?
> > 
> > I agree that the lock doesn't seem to be necessary. But I believe the whole
> > thing is already running in non-sleeping context, even before the spinlock
> > is taken. So this wouldn't help much.
> > 
> > Even the io_cancel() syscall takes a spinlock before invoking the cancel
> > function. So this issue is not exclusive to program termination.
> > 
> > Are there any documented guidelines on which context usb_ep_dequeue() should
> > be able to be called in? The sleep in the dwc3 driver seems to be a recent
> > addition.
> 
> drivers/usb/udc/gadget/core.c has the only documentation, but context is not mentioned there.
> Felipe, what do you suggest?

As far as I remember, usb_ep_dequeue() is supposed to be more or less 
analogous to usb_ep_queue(); drivers should be allowed to call either 
routine in an atomic context.

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-21 11:10 Roger Quadros
  0 siblings, 0 replies; 17+ messages in thread
From: Roger Quadros @ 2018-06-21 11:10 UTC (permalink / raw)
  To: Lars-Peter Clausen, Sam Protsenko, Vincent Pelletier, Felipe Balbi
  Cc: linux-usb, Alan Stern, Praneeth Bajjuri, Felipe Balbi

On 21/06/18 13:52, Lars-Peter Clausen wrote:
> On 06/21/2018 10:29 AM, Roger Quadros wrote:
> [...]
>>>>  static int ffs_aio_cancel(struct kiocb *kiocb)
>>>>  {
>>>>         struct ffs_io_data *io_data = kiocb->private;
>>>> -       struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
>>>> +       struct ffs_data *ffs = io_data->ffs;
>>>>         int value;
>>>>
>>>>         ENTER();
>>>>
>>>> -       spin_lock_irq(&epfile->ffs->eps_lock);
>>>> -
>>>> -       if (likely(io_data && io_data->ep && io_data->req))
>>>> -               value = usb_ep_dequeue(io_data->ep, io_data->req);
>>>> -       else
>>>> +       if (likely(io_data && io_data->ep && io_data->req)) {
>>>> +               INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
>>>> +               queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
>>>> +               value = -EINPROGRESS;
>>>> +       } else {
>>>>                 value = -EINVAL;
>>>> -
>>>> -       spin_unlock_irq(&epfile->ffs->eps_lock);
>>>> +       }
>>
>> Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue() directly from here?
>>> What is the purpose of the spin_lock()?
> 
> I agree that the lock doesn't seem to be necessary. But I believe the whole
> thing is already running in non-sleeping context, even before the spinlock
> is taken. So this wouldn't help much.
> 
> Even the io_cancel() syscall takes a spinlock before invoking the cancel
> function. So this issue is not exclusive to program termination.
> 
> Are there any documented guidelines on which context usb_ep_dequeue() should
> be able to be called in? The sleep in the dwc3 driver seems to be a recent
> addition.

drivers/usb/udc/gadget/core.c has the only documentation, but context is not mentioned there.
Felipe, what do you suggest?

> 
>>
>>>>
>>>>         return value;
>>>>  }
>>>> --
>>>> 2.17.1
>>>>
>>
>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-21 10:52 Lars-Peter Clausen
  0 siblings, 0 replies; 17+ messages in thread
From: Lars-Peter Clausen @ 2018-06-21 10:52 UTC (permalink / raw)
  To: Roger Quadros, Sam Protsenko, Vincent Pelletier, Felipe Balbi
  Cc: linux-usb, Alan Stern, Praneeth Bajjuri, Felipe Balbi

On 06/21/2018 10:29 AM, Roger Quadros wrote:
[...]
>>>  static int ffs_aio_cancel(struct kiocb *kiocb)
>>>  {
>>>         struct ffs_io_data *io_data = kiocb->private;
>>> -       struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
>>> +       struct ffs_data *ffs = io_data->ffs;
>>>         int value;
>>>
>>>         ENTER();
>>>
>>> -       spin_lock_irq(&epfile->ffs->eps_lock);
>>> -
>>> -       if (likely(io_data && io_data->ep && io_data->req))
>>> -               value = usb_ep_dequeue(io_data->ep, io_data->req);
>>> -       else
>>> +       if (likely(io_data && io_data->ep && io_data->req)) {
>>> +               INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
>>> +               queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
>>> +               value = -EINPROGRESS;
>>> +       } else {
>>>                 value = -EINVAL;
>>> -
>>> -       spin_unlock_irq(&epfile->ffs->eps_lock);
>>> +       }
> 
> Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue() directly from here?
>> What is the purpose of the spin_lock()?

I agree that the lock doesn't seem to be necessary. But I believe the whole
thing is already running in non-sleeping context, even before the spinlock
is taken. So this wouldn't help much.

Even the io_cancel() syscall takes a spinlock before invoking the cancel
function. So this issue is not exclusive to program termination.

Are there any documented guidelines on which context usb_ep_dequeue() should
be able to be called in? The sleep in the dwc3 driver seems to be a recent
addition.

> 
>>>
>>>         return value;
>>>  }
>>> --
>>> 2.17.1
>>>
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-21  8:29 Roger Quadros
  0 siblings, 0 replies; 17+ messages in thread
From: Roger Quadros @ 2018-06-21  8:29 UTC (permalink / raw)
  To: Sam Protsenko, Vincent Pelletier, Felipe Balbi, Lars-Peter Clausen
  Cc: linux-usb, Alan Stern, Praneeth Bajjuri

+Lars-Peter

Vincent,

On 14/06/18 16:23, Sam Protsenko wrote:
> + Roger Quadros
> + Praneeth Bajjuri
> 
> Tested-by: Sam Protsenko <semen.protsenko@linaro.org>
> 
> I've tested it on X15 board (DWC3 controller) on Android master, by
> doing "adb root". Without this patch I see backtrace and kernel panic
> (the same error as described in commit message). When this patch is
> applied, everything works fine.
> 
> Can we please merge this patch, it fixes major bug for us?
> 
> Thanks!
> 
> 
> On 13 June 2018 at 14:05, Vincent Pelletier <plr.vincent@gmail.com> wrote:
>> This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
>> as is the case for (at least) dwc3.
>>
>> [  382.200896] BUG: scheduling while atomic: screen/1808/0x00000100
>> [  382.207124] 4 locks held by screen/1808:
>> [  382.211266]  #0:  (rcu_callback){....}, at: [<c10b4ff0>] rcu_process_callbacks+0x260/0x440
>> [  382.219949]  #1:  (rcu_read_lock_sched){....}, at: [<c1358ba0>] percpu_ref_switch_to_atomic_rcu+0xb0/0x130
>> [  382.230034]  #2:  (&(&ctx->ctx_lock)->rlock){....}, at: [<c11f0c73>] free_ioctx_users+0x23/0xd0
>> [  382.230096]  #3:  (&(&ffs->eps_lock)->rlock){....}, at: [<f81e7710>] ffs_aio_cancel+0x20/0x60 [usb_f_fs]
>> [  382.230160] Modules linked in: usb_f_fs libcomposite configfs bnep btsdio bluetooth ecdh_generic brcmfmac brcmutil intel_powerclamp coretemp dwc3 kvm_intel ulpi udc_core kvm irqbypass crc32_pclmul crc32c_intel pcbc dwc3_pci aesni_intel aes_i586 crypto_simd cryptd ehci_pci ehci_hcd gpio_keys usbcore basincove_gpadc industrialio usb_common
>> [  382.230407] CPU: 1 PID: 1808 Comm: screen Not tainted 4.14.0-edison+ #117
>> [  382.230416] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
>> [  382.230425] Call Trace:
>> [  382.230438]  <SOFTIRQ>
>> [  382.230466]  dump_stack+0x47/0x62
>> [  382.230498]  __schedule_bug+0x61/0x80
>> [  382.230522]  __schedule+0x43/0x7a0
>> [  382.230587]  schedule+0x5f/0x70
>> [  382.230625]  dwc3_gadget_ep_dequeue+0x14c/0x270 [dwc3]
>> [  382.230669]  ? do_wait_intr_irq+0x70/0x70
>> [  382.230724]  usb_ep_dequeue+0x19/0x90 [udc_core]
>> [  382.230770]  ffs_aio_cancel+0x37/0x60 [usb_f_fs]
>> [  382.230798]  kiocb_cancel+0x31/0x40
>> [  382.230822]  free_ioctx_users+0x4d/0xd0
>> [  382.230858]  percpu_ref_switch_to_atomic_rcu+0x10a/0x130
>> [  382.230881]  ? percpu_ref_exit+0x40/0x40
>> [  382.230904]  rcu_process_callbacks+0x2b3/0x440
>> [  382.230965]  __do_softirq+0xf8/0x26b
>> [  382.231011]  ? __softirqentry_text_start+0x8/0x8
>> [  382.231033]  do_softirq_own_stack+0x22/0x30
>> [  382.231042]  </SOFTIRQ>
>> [  382.231071]  irq_exit+0x45/0xc0
>> [  382.231089]  smp_apic_timer_interrupt+0x13c/0x150
>> [  382.231118]  apic_timer_interrupt+0x35/0x3c
>> [  382.231132] EIP: __copy_user_ll+0xe2/0xf0
>> [  382.231142] EFLAGS: 00210293 CPU: 1
>> [  382.231154] EAX: bfd4508c EBX: 00000004 ECX: 00000003 EDX: f3d8fe50
>> [  382.231165] ESI: f3d8fe51 EDI: bfd4508d EBP: f3d8fe14 ESP: f3d8fe08
>> [  382.231176]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
>> [  382.231265]  core_sys_select+0x25f/0x320
>> [  382.231346]  ? __wake_up_common_lock+0x62/0x80
>> [  382.231399]  ? tty_ldisc_deref+0x13/0x20
>> [  382.231438]  ? ldsem_up_read+0x1b/0x40
>> [  382.231459]  ? tty_ldisc_deref+0x13/0x20
>> [  382.231479]  ? tty_write+0x29f/0x2e0
>> [  382.231514]  ? n_tty_ioctl+0xe0/0xe0
>> [  382.231541]  ? tty_write_unlock+0x30/0x30
>> [  382.231566]  ? __vfs_write+0x22/0x110
>> [  382.231604]  ? security_file_permission+0x2f/0xd0
>> [  382.231635]  ? rw_verify_area+0xac/0x120
>> [  382.231677]  ? vfs_write+0x103/0x180
>> [  382.231711]  SyS_select+0x87/0xc0
>> [  382.231739]  ? SyS_write+0x42/0x90
>> [  382.231781]  do_fast_syscall_32+0xd6/0x1a0
>> [  382.231836]  entry_SYSENTER_32+0x47/0x71
>> [  382.231848] EIP: 0xb7f75b05
>> [  382.231857] EFLAGS: 00000246 CPU: 1
>> [  382.231868] EAX: ffffffda EBX: 00000400 ECX: bfd4508c EDX: bfd4510c
>> [  382.231878] ESI: 00000000 EDI: 00000000 EBP: 00000000 ESP: bfd45020
>> [  382.231889]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b
>> [  382.232281] softirq: huh, entered softirq 9 RCU c10b4d90 with preempt_count 00000100, exited with 00000000?
>>
>> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
>> ---
>>  drivers/usb/gadget/function/f_fs.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 199d25700050..01841fdb3144 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -215,6 +215,7 @@ struct ffs_io_data {
>>
>>         struct mm_struct *mm;
>>         struct work_struct work;
>> +       struct work_struct cancellation_work;
>>
>>         struct usb_ep *ep;
>>         struct usb_request *req;
>> @@ -1072,22 +1073,31 @@ ffs_epfile_open(struct inode *inode, struct file *file)
>>         return 0;
>>  }
>>
>> +static void ffs_aio_cancel_worker(struct work_struct *work)
>> +{
>> +       struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
>> +                                                  cancellation_work);
>> +
>> +       ENTER();
>> +
>> +       usb_ep_dequeue(io_data->ep, io_data->req);
>> +}
>> +
>>  static int ffs_aio_cancel(struct kiocb *kiocb)
>>  {
>>         struct ffs_io_data *io_data = kiocb->private;
>> -       struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
>> +       struct ffs_data *ffs = io_data->ffs;
>>         int value;
>>
>>         ENTER();
>>
>> -       spin_lock_irq(&epfile->ffs->eps_lock);
>> -
>> -       if (likely(io_data && io_data->ep && io_data->req))
>> -               value = usb_ep_dequeue(io_data->ep, io_data->req);
>> -       else
>> +       if (likely(io_data && io_data->ep && io_data->req)) {
>> +               INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
>> +               queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
>> +               value = -EINPROGRESS;
>> +       } else {
>>                 value = -EINVAL;
>> -
>> -       spin_unlock_irq(&epfile->ffs->eps_lock);
>> +       }

Can we avoid the spin_lock() and the work-queue and call usb_ep_dequeue() directly from here?

What is the purpose of the spin_lock()?

>>
>>         return value;
>>  }
>> --
>> 2.17.1
>>

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-19 13:20 Sam Protsenko
  0 siblings, 0 replies; 17+ messages in thread
From: Sam Protsenko @ 2018-06-19 13:20 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: linux-usb, Alan Stern, Roger Quadros, Praneeth Bajjuri,
	Vincent Pelletier

Hi Felipe,

If there is no objections, can we please pull this patch? It fixes
kernel panic for us (scheduling in atomic context).

Thanks!


On 14 June 2018 at 16:23, Sam Protsenko <semen.protsenko@linaro.org> wrote:
> + Roger Quadros
> + Praneeth Bajjuri
>
> Tested-by: Sam Protsenko <semen.protsenko@linaro.org>
>
> I've tested it on X15 board (DWC3 controller) on Android master, by
> doing "adb root". Without this patch I see backtrace and kernel panic
> (the same error as described in commit message). When this patch is
> applied, everything works fine.
>
> Can we please merge this patch, it fixes major bug for us?
>
> Thanks!
>
>
> On 13 June 2018 at 14:05, Vincent Pelletier <plr.vincent@gmail.com> wrote:
>> This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
>> as is the case for (at least) dwc3.
>>
>> [  382.200896] BUG: scheduling while atomic: screen/1808/0x00000100
>> [  382.207124] 4 locks held by screen/1808:
>> [  382.211266]  #0:  (rcu_callback){....}, at: [<c10b4ff0>] rcu_process_callbacks+0x260/0x440
>> [  382.219949]  #1:  (rcu_read_lock_sched){....}, at: [<c1358ba0>] percpu_ref_switch_to_atomic_rcu+0xb0/0x130
>> [  382.230034]  #2:  (&(&ctx->ctx_lock)->rlock){....}, at: [<c11f0c73>] free_ioctx_users+0x23/0xd0
>> [  382.230096]  #3:  (&(&ffs->eps_lock)->rlock){....}, at: [<f81e7710>] ffs_aio_cancel+0x20/0x60 [usb_f_fs]
>> [  382.230160] Modules linked in: usb_f_fs libcomposite configfs bnep btsdio bluetooth ecdh_generic brcmfmac brcmutil intel_powerclamp coretemp dwc3 kvm_intel ulpi udc_core kvm irqbypass crc32_pclmul crc32c_intel pcbc dwc3_pci aesni_intel aes_i586 crypto_simd cryptd ehci_pci ehci_hcd gpio_keys usbcore basincove_gpadc industrialio usb_common
>> [  382.230407] CPU: 1 PID: 1808 Comm: screen Not tainted 4.14.0-edison+ #117
>> [  382.230416] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
>> [  382.230425] Call Trace:
>> [  382.230438]  <SOFTIRQ>
>> [  382.230466]  dump_stack+0x47/0x62
>> [  382.230498]  __schedule_bug+0x61/0x80
>> [  382.230522]  __schedule+0x43/0x7a0
>> [  382.230587]  schedule+0x5f/0x70
>> [  382.230625]  dwc3_gadget_ep_dequeue+0x14c/0x270 [dwc3]
>> [  382.230669]  ? do_wait_intr_irq+0x70/0x70
>> [  382.230724]  usb_ep_dequeue+0x19/0x90 [udc_core]
>> [  382.230770]  ffs_aio_cancel+0x37/0x60 [usb_f_fs]
>> [  382.230798]  kiocb_cancel+0x31/0x40
>> [  382.230822]  free_ioctx_users+0x4d/0xd0
>> [  382.230858]  percpu_ref_switch_to_atomic_rcu+0x10a/0x130
>> [  382.230881]  ? percpu_ref_exit+0x40/0x40
>> [  382.230904]  rcu_process_callbacks+0x2b3/0x440
>> [  382.230965]  __do_softirq+0xf8/0x26b
>> [  382.231011]  ? __softirqentry_text_start+0x8/0x8
>> [  382.231033]  do_softirq_own_stack+0x22/0x30
>> [  382.231042]  </SOFTIRQ>
>> [  382.231071]  irq_exit+0x45/0xc0
>> [  382.231089]  smp_apic_timer_interrupt+0x13c/0x150
>> [  382.231118]  apic_timer_interrupt+0x35/0x3c
>> [  382.231132] EIP: __copy_user_ll+0xe2/0xf0
>> [  382.231142] EFLAGS: 00210293 CPU: 1
>> [  382.231154] EAX: bfd4508c EBX: 00000004 ECX: 00000003 EDX: f3d8fe50
>> [  382.231165] ESI: f3d8fe51 EDI: bfd4508d EBP: f3d8fe14 ESP: f3d8fe08
>> [  382.231176]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
>> [  382.231265]  core_sys_select+0x25f/0x320
>> [  382.231346]  ? __wake_up_common_lock+0x62/0x80
>> [  382.231399]  ? tty_ldisc_deref+0x13/0x20
>> [  382.231438]  ? ldsem_up_read+0x1b/0x40
>> [  382.231459]  ? tty_ldisc_deref+0x13/0x20
>> [  382.231479]  ? tty_write+0x29f/0x2e0
>> [  382.231514]  ? n_tty_ioctl+0xe0/0xe0
>> [  382.231541]  ? tty_write_unlock+0x30/0x30
>> [  382.231566]  ? __vfs_write+0x22/0x110
>> [  382.231604]  ? security_file_permission+0x2f/0xd0
>> [  382.231635]  ? rw_verify_area+0xac/0x120
>> [  382.231677]  ? vfs_write+0x103/0x180
>> [  382.231711]  SyS_select+0x87/0xc0
>> [  382.231739]  ? SyS_write+0x42/0x90
>> [  382.231781]  do_fast_syscall_32+0xd6/0x1a0
>> [  382.231836]  entry_SYSENTER_32+0x47/0x71
>> [  382.231848] EIP: 0xb7f75b05
>> [  382.231857] EFLAGS: 00000246 CPU: 1
>> [  382.231868] EAX: ffffffda EBX: 00000400 ECX: bfd4508c EDX: bfd4510c
>> [  382.231878] ESI: 00000000 EDI: 00000000 EBP: 00000000 ESP: bfd45020
>> [  382.231889]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b
>> [  382.232281] softirq: huh, entered softirq 9 RCU c10b4d90 with preempt_count 00000100, exited with 00000000?
>>
>> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
>> ---
>>  drivers/usb/gadget/function/f_fs.c | 26 ++++++++++++++++++--------
>>  1 file changed, 18 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
>> index 199d25700050..01841fdb3144 100644
>> --- a/drivers/usb/gadget/function/f_fs.c
>> +++ b/drivers/usb/gadget/function/f_fs.c
>> @@ -215,6 +215,7 @@ struct ffs_io_data {
>>
>>         struct mm_struct *mm;
>>         struct work_struct work;
>> +       struct work_struct cancellation_work;
>>
>>         struct usb_ep *ep;
>>         struct usb_request *req;
>> @@ -1072,22 +1073,31 @@ ffs_epfile_open(struct inode *inode, struct file *file)
>>         return 0;
>>  }
>>
>> +static void ffs_aio_cancel_worker(struct work_struct *work)
>> +{
>> +       struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
>> +                                                  cancellation_work);
>> +
>> +       ENTER();
>> +
>> +       usb_ep_dequeue(io_data->ep, io_data->req);
>> +}
>> +
>>  static int ffs_aio_cancel(struct kiocb *kiocb)
>>  {
>>         struct ffs_io_data *io_data = kiocb->private;
>> -       struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
>> +       struct ffs_data *ffs = io_data->ffs;
>>         int value;
>>
>>         ENTER();
>>
>> -       spin_lock_irq(&epfile->ffs->eps_lock);
>> -
>> -       if (likely(io_data && io_data->ep && io_data->req))
>> -               value = usb_ep_dequeue(io_data->ep, io_data->req);
>> -       else
>> +       if (likely(io_data && io_data->ep && io_data->req)) {
>> +               INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
>> +               queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
>> +               value = -EINPROGRESS;
>> +       } else {
>>                 value = -EINVAL;
>> -
>> -       spin_unlock_irq(&epfile->ffs->eps_lock);
>> +       }
>>
>>         return value;
>>  }
>> --
>> 2.17.1
>>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-14 13:23 Sam Protsenko
  0 siblings, 0 replies; 17+ messages in thread
From: Sam Protsenko @ 2018-06-14 13:23 UTC (permalink / raw)
  To: Vincent Pelletier
  Cc: linux-usb, Felipe Balbi, Alan Stern, Roger Quadros, Praneeth Bajjuri

+ Roger Quadros
+ Praneeth Bajjuri

Tested-by: Sam Protsenko <semen.protsenko@linaro.org>

I've tested it on X15 board (DWC3 controller) on Android master, by
doing "adb root". Without this patch I see backtrace and kernel panic
(the same error as described in commit message). When this patch is
applied, everything works fine.

Can we please merge this patch, it fixes major bug for us?

Thanks!


On 13 June 2018 at 14:05, Vincent Pelletier <plr.vincent@gmail.com> wrote:
> This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
> as is the case for (at least) dwc3.
>
> [  382.200896] BUG: scheduling while atomic: screen/1808/0x00000100
> [  382.207124] 4 locks held by screen/1808:
> [  382.211266]  #0:  (rcu_callback){....}, at: [<c10b4ff0>] rcu_process_callbacks+0x260/0x440
> [  382.219949]  #1:  (rcu_read_lock_sched){....}, at: [<c1358ba0>] percpu_ref_switch_to_atomic_rcu+0xb0/0x130
> [  382.230034]  #2:  (&(&ctx->ctx_lock)->rlock){....}, at: [<c11f0c73>] free_ioctx_users+0x23/0xd0
> [  382.230096]  #3:  (&(&ffs->eps_lock)->rlock){....}, at: [<f81e7710>] ffs_aio_cancel+0x20/0x60 [usb_f_fs]
> [  382.230160] Modules linked in: usb_f_fs libcomposite configfs bnep btsdio bluetooth ecdh_generic brcmfmac brcmutil intel_powerclamp coretemp dwc3 kvm_intel ulpi udc_core kvm irqbypass crc32_pclmul crc32c_intel pcbc dwc3_pci aesni_intel aes_i586 crypto_simd cryptd ehci_pci ehci_hcd gpio_keys usbcore basincove_gpadc industrialio usb_common
> [  382.230407] CPU: 1 PID: 1808 Comm: screen Not tainted 4.14.0-edison+ #117
> [  382.230416] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
> [  382.230425] Call Trace:
> [  382.230438]  <SOFTIRQ>
> [  382.230466]  dump_stack+0x47/0x62
> [  382.230498]  __schedule_bug+0x61/0x80
> [  382.230522]  __schedule+0x43/0x7a0
> [  382.230587]  schedule+0x5f/0x70
> [  382.230625]  dwc3_gadget_ep_dequeue+0x14c/0x270 [dwc3]
> [  382.230669]  ? do_wait_intr_irq+0x70/0x70
> [  382.230724]  usb_ep_dequeue+0x19/0x90 [udc_core]
> [  382.230770]  ffs_aio_cancel+0x37/0x60 [usb_f_fs]
> [  382.230798]  kiocb_cancel+0x31/0x40
> [  382.230822]  free_ioctx_users+0x4d/0xd0
> [  382.230858]  percpu_ref_switch_to_atomic_rcu+0x10a/0x130
> [  382.230881]  ? percpu_ref_exit+0x40/0x40
> [  382.230904]  rcu_process_callbacks+0x2b3/0x440
> [  382.230965]  __do_softirq+0xf8/0x26b
> [  382.231011]  ? __softirqentry_text_start+0x8/0x8
> [  382.231033]  do_softirq_own_stack+0x22/0x30
> [  382.231042]  </SOFTIRQ>
> [  382.231071]  irq_exit+0x45/0xc0
> [  382.231089]  smp_apic_timer_interrupt+0x13c/0x150
> [  382.231118]  apic_timer_interrupt+0x35/0x3c
> [  382.231132] EIP: __copy_user_ll+0xe2/0xf0
> [  382.231142] EFLAGS: 00210293 CPU: 1
> [  382.231154] EAX: bfd4508c EBX: 00000004 ECX: 00000003 EDX: f3d8fe50
> [  382.231165] ESI: f3d8fe51 EDI: bfd4508d EBP: f3d8fe14 ESP: f3d8fe08
> [  382.231176]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
> [  382.231265]  core_sys_select+0x25f/0x320
> [  382.231346]  ? __wake_up_common_lock+0x62/0x80
> [  382.231399]  ? tty_ldisc_deref+0x13/0x20
> [  382.231438]  ? ldsem_up_read+0x1b/0x40
> [  382.231459]  ? tty_ldisc_deref+0x13/0x20
> [  382.231479]  ? tty_write+0x29f/0x2e0
> [  382.231514]  ? n_tty_ioctl+0xe0/0xe0
> [  382.231541]  ? tty_write_unlock+0x30/0x30
> [  382.231566]  ? __vfs_write+0x22/0x110
> [  382.231604]  ? security_file_permission+0x2f/0xd0
> [  382.231635]  ? rw_verify_area+0xac/0x120
> [  382.231677]  ? vfs_write+0x103/0x180
> [  382.231711]  SyS_select+0x87/0xc0
> [  382.231739]  ? SyS_write+0x42/0x90
> [  382.231781]  do_fast_syscall_32+0xd6/0x1a0
> [  382.231836]  entry_SYSENTER_32+0x47/0x71
> [  382.231848] EIP: 0xb7f75b05
> [  382.231857] EFLAGS: 00000246 CPU: 1
> [  382.231868] EAX: ffffffda EBX: 00000400 ECX: bfd4508c EDX: bfd4510c
> [  382.231878] ESI: 00000000 EDI: 00000000 EBP: 00000000 ESP: bfd45020
> [  382.231889]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b
> [  382.232281] softirq: huh, entered softirq 9 RCU c10b4d90 with preempt_count 00000100, exited with 00000000?
>
> Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 26 ++++++++++++++++++--------
>  1 file changed, 18 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> index 199d25700050..01841fdb3144 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -215,6 +215,7 @@ struct ffs_io_data {
>
>         struct mm_struct *mm;
>         struct work_struct work;
> +       struct work_struct cancellation_work;
>
>         struct usb_ep *ep;
>         struct usb_request *req;
> @@ -1072,22 +1073,31 @@ ffs_epfile_open(struct inode *inode, struct file *file)
>         return 0;
>  }
>
> +static void ffs_aio_cancel_worker(struct work_struct *work)
> +{
> +       struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
> +                                                  cancellation_work);
> +
> +       ENTER();
> +
> +       usb_ep_dequeue(io_data->ep, io_data->req);
> +}
> +
>  static int ffs_aio_cancel(struct kiocb *kiocb)
>  {
>         struct ffs_io_data *io_data = kiocb->private;
> -       struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
> +       struct ffs_data *ffs = io_data->ffs;
>         int value;
>
>         ENTER();
>
> -       spin_lock_irq(&epfile->ffs->eps_lock);
> -
> -       if (likely(io_data && io_data->ep && io_data->req))
> -               value = usb_ep_dequeue(io_data->ep, io_data->req);
> -       else
> +       if (likely(io_data && io_data->ep && io_data->req)) {
> +               INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
> +               queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
> +               value = -EINPROGRESS;
> +       } else {
>                 value = -EINVAL;
> -
> -       spin_unlock_irq(&epfile->ffs->eps_lock);
> +       }
>
>         return value;
>  }
> --
> 2.17.1
>
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 17+ messages in thread

* usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers
@ 2018-06-13 11:05 Vincent Pelletier
  0 siblings, 0 replies; 17+ messages in thread
From: Vincent Pelletier @ 2018-06-13 11:05 UTC (permalink / raw)
  To: linux-usb; +Cc: Sam Protsenko, Felipe Balbi, Alan Stern

This bug happens only when the UDC needs to sleep during usb_ep_dequeue,
as is the case for (at least) dwc3.

[  382.200896] BUG: scheduling while atomic: screen/1808/0x00000100
[  382.207124] 4 locks held by screen/1808:
[  382.211266]  #0:  (rcu_callback){....}, at: [<c10b4ff0>] rcu_process_callbacks+0x260/0x440
[  382.219949]  #1:  (rcu_read_lock_sched){....}, at: [<c1358ba0>] percpu_ref_switch_to_atomic_rcu+0xb0/0x130
[  382.230034]  #2:  (&(&ctx->ctx_lock)->rlock){....}, at: [<c11f0c73>] free_ioctx_users+0x23/0xd0
[  382.230096]  #3:  (&(&ffs->eps_lock)->rlock){....}, at: [<f81e7710>] ffs_aio_cancel+0x20/0x60 [usb_f_fs]
[  382.230160] Modules linked in: usb_f_fs libcomposite configfs bnep btsdio bluetooth ecdh_generic brcmfmac brcmutil intel_powerclamp coretemp dwc3 kvm_intel ulpi udc_core kvm irqbypass crc32_pclmul crc32c_intel pcbc dwc3_pci aesni_intel aes_i586 crypto_simd cryptd ehci_pci ehci_hcd gpio_keys usbcore basincove_gpadc industrialio usb_common
[  382.230407] CPU: 1 PID: 1808 Comm: screen Not tainted 4.14.0-edison+ #117
[  382.230416] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
[  382.230425] Call Trace:
[  382.230438]  <SOFTIRQ>
[  382.230466]  dump_stack+0x47/0x62
[  382.230498]  __schedule_bug+0x61/0x80
[  382.230522]  __schedule+0x43/0x7a0
[  382.230587]  schedule+0x5f/0x70
[  382.230625]  dwc3_gadget_ep_dequeue+0x14c/0x270 [dwc3]
[  382.230669]  ? do_wait_intr_irq+0x70/0x70
[  382.230724]  usb_ep_dequeue+0x19/0x90 [udc_core]
[  382.230770]  ffs_aio_cancel+0x37/0x60 [usb_f_fs]
[  382.230798]  kiocb_cancel+0x31/0x40
[  382.230822]  free_ioctx_users+0x4d/0xd0
[  382.230858]  percpu_ref_switch_to_atomic_rcu+0x10a/0x130
[  382.230881]  ? percpu_ref_exit+0x40/0x40
[  382.230904]  rcu_process_callbacks+0x2b3/0x440
[  382.230965]  __do_softirq+0xf8/0x26b
[  382.231011]  ? __softirqentry_text_start+0x8/0x8
[  382.231033]  do_softirq_own_stack+0x22/0x30
[  382.231042]  </SOFTIRQ>
[  382.231071]  irq_exit+0x45/0xc0
[  382.231089]  smp_apic_timer_interrupt+0x13c/0x150
[  382.231118]  apic_timer_interrupt+0x35/0x3c
[  382.231132] EIP: __copy_user_ll+0xe2/0xf0
[  382.231142] EFLAGS: 00210293 CPU: 1
[  382.231154] EAX: bfd4508c EBX: 00000004 ECX: 00000003 EDX: f3d8fe50
[  382.231165] ESI: f3d8fe51 EDI: bfd4508d EBP: f3d8fe14 ESP: f3d8fe08
[  382.231176]  DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
[  382.231265]  core_sys_select+0x25f/0x320
[  382.231346]  ? __wake_up_common_lock+0x62/0x80
[  382.231399]  ? tty_ldisc_deref+0x13/0x20
[  382.231438]  ? ldsem_up_read+0x1b/0x40
[  382.231459]  ? tty_ldisc_deref+0x13/0x20
[  382.231479]  ? tty_write+0x29f/0x2e0
[  382.231514]  ? n_tty_ioctl+0xe0/0xe0
[  382.231541]  ? tty_write_unlock+0x30/0x30
[  382.231566]  ? __vfs_write+0x22/0x110
[  382.231604]  ? security_file_permission+0x2f/0xd0
[  382.231635]  ? rw_verify_area+0xac/0x120
[  382.231677]  ? vfs_write+0x103/0x180
[  382.231711]  SyS_select+0x87/0xc0
[  382.231739]  ? SyS_write+0x42/0x90
[  382.231781]  do_fast_syscall_32+0xd6/0x1a0
[  382.231836]  entry_SYSENTER_32+0x47/0x71
[  382.231848] EIP: 0xb7f75b05
[  382.231857] EFLAGS: 00000246 CPU: 1
[  382.231868] EAX: ffffffda EBX: 00000400 ECX: bfd4508c EDX: bfd4510c
[  382.231878] ESI: 00000000 EDI: 00000000 EBP: 00000000 ESP: bfd45020
[  382.231889]  DS: 007b ES: 007b FS: 0000 GS: 0033 SS: 007b
[  382.232281] softirq: huh, entered softirq 9 RCU c10b4d90 with preempt_count 00000100, exited with 00000000?

Signed-off-by: Vincent Pelletier <plr.vincent@gmail.com>
---
 drivers/usb/gadget/function/f_fs.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
index 199d25700050..01841fdb3144 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -215,6 +215,7 @@ struct ffs_io_data {
 
 	struct mm_struct *mm;
 	struct work_struct work;
+	struct work_struct cancellation_work;
 
 	struct usb_ep *ep;
 	struct usb_request *req;
@@ -1072,22 +1073,31 @@ ffs_epfile_open(struct inode *inode, struct file *file)
 	return 0;
 }
 
+static void ffs_aio_cancel_worker(struct work_struct *work)
+{
+	struct ffs_io_data *io_data = container_of(work, struct ffs_io_data,
+						   cancellation_work);
+
+	ENTER();
+
+	usb_ep_dequeue(io_data->ep, io_data->req);
+}
+
 static int ffs_aio_cancel(struct kiocb *kiocb)
 {
 	struct ffs_io_data *io_data = kiocb->private;
-	struct ffs_epfile *epfile = kiocb->ki_filp->private_data;
+	struct ffs_data *ffs = io_data->ffs;
 	int value;
 
 	ENTER();
 
-	spin_lock_irq(&epfile->ffs->eps_lock);
-
-	if (likely(io_data && io_data->ep && io_data->req))
-		value = usb_ep_dequeue(io_data->ep, io_data->req);
-	else
+	if (likely(io_data && io_data->ep && io_data->req)) {
+		INIT_WORK(&io_data->cancellation_work, ffs_aio_cancel_worker);
+		queue_work(ffs->io_completion_wq, &io_data->cancellation_work);
+		value = -EINPROGRESS;
+	} else {
 		value = -EINVAL;
-
-	spin_unlock_irq(&epfile->ffs->eps_lock);
+	}
 
 	return value;
 }

^ permalink raw reply related	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2019-01-17 16:29 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-02  0:45 usb: gadget: ffs: Fix BUG when userland exits with submitted AIO transfers He, Bo
  -- strict thread matches above, loose matches on Subject: below --
2019-01-17 16:29 Evan Green
2019-01-16 23:56 Evan Green
2018-10-23 14:22 Lars-Peter Clausen
2018-10-23 12:20 Lars-Peter Clausen
2018-09-27  2:31 He, Bo
2018-09-25 12:46 Vincent Pelletier
2018-08-02 14:23 Vincent Pelletier
2018-08-01 15:03 Vincent Pelletier
2018-06-29  6:32 Felipe Balbi
2018-06-21 15:30 Alan Stern
2018-06-21 11:10 Roger Quadros
2018-06-21 10:52 Lars-Peter Clausen
2018-06-21  8:29 Roger Quadros
2018-06-19 13:20 Sam Protsenko
2018-06-14 13:23 Sam Protsenko
2018-06-13 11:05 Vincent Pelletier

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.