Re: Memory barrier needed with wake_up_process()?

[Alan Stern]

On Fri, 9 Sep 2016, Felipe Balbi wrote:

> Finally :-) Here's the diff I used:
> 
> diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c
> index 8f3659b65f53..0716024f6b65 100644
> --- a/drivers/usb/gadget/function/f_mass_storage.c
> +++ b/drivers/usb/gadget/function/f_mass_storage.c
> @@ -481,6 +481,8 @@ static void bulk_out_complete(struct usb_ep *ep, struct usb_request *req)
>         spin_lock(&common->lock);
>         bh->outreq_busy = 0;
>         bh->state = BUF_STATE_FULL;
> +       if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
> +               trace_printk("compl: bh %p state %d\n", bh, bh->state);
>         wakeup_thread(common);
>         spin_unlock(&common->lock);
>  }
> @@ -2208,6 +2210,7 @@ static int get_next_command(struct fsg_common *common)
>                 rc = sleep_thread(common, true);
>                 if (rc)
>                         return rc;
> +               trace_printk("next: bh %p state %d\n", bh, bh->state);
>         }
>         smp_rmb();
>         rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;
> 
> 
> And here's trace output:
> 
> # tracer: nop
> #
> # entries-in-buffer/entries-written: 1002/1002   #P:4
> #
> #                              _-----=> irqs-off
> #                             / _----=> need-resched
> #                            | / _---=> hardirq/softirq
> #                            || / _--=> preempt-depth
> #                            ||| /     delay
> #           TASK-PID   CPU#  ||||    TIMESTAMP  FUNCTION
> #              | |       |   ||||       |         |
>     file-storage-3578  [000] .... 21166.789127: fsg_main_thread: next: bh ffff880111e69a00 state 2
>     file-storage-3578  [000] .... 21166.789312: fsg_main_thread: next: bh ffff880111e69a00 state 2
>      irq/17-dwc3-3579  [003] d..1 21166.789395: bulk_out_complete: compl: bh ffff880111e69a00 state 1
>     file-storage-3578  [000] .... 21166.789445: fsg_main_thread: next: bh ffff880111e69a00 state 1

Okay, that's normal.  2 = BUF_STATE_BUSY, 1 = BUF_STATE_FULL.  So we get woken
up a couple of times while the transfer is in progress (probably because some
earlier buffers have finished transferring), then the CBW transfer completes
and the buffer is read.

...

>     file-storage-3578  [002] .... 21167.726827: fsg_main_thread: next: bh ffff880111e69a80 state 2
>      irq/17-dwc3-3579  [003] d..1 21167.727066: bulk_out_complete: compl: bh ffff880111e69a80 state 1
>     file-storage-3578  [002] .... 21167.727072: fsg_main_thread: next: bh ffff880111e69a80 state 1
>     file-storage-3578  [002] .... 21167.729458: fsg_main_thread: next: bh ffff880111e6aac0 state 2
>      irq/17-dwc3-3579  [003] d..1 21167.729666: bulk_out_complete: compl: bh ffff880111e6aac0 state 1
>     file-storage-3578  [002] .... 21167.729670: fsg_main_thread: next: bh ffff880111e6aac0 state 1

And this is where everything stopped?

This also looks normal.  So the question is what happened when 
get_next_command() returned after this?

Felipe, maybe the patch below (in place of your current patch) will
help.  Since the events that it logs are all supposed to be unusual,
you can use printk if you want, but I wrote it with trace_printk.

Alan Stern



Index: usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
===================================================================
--- usb-4.x.orig/drivers/usb/gadget/function/f_mass_storage.c
+++ usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
@@ -415,6 +415,7 @@ static void raise_exception(struct fsg_c
 	 * If a lower-or-equal priority exception is in progress, preempt it
 	 * and notify the main thread by sending it a signal.
 	 */
+	trace_printk("raise_exception %d\n", new_state);
 	spin_lock_irqsave(&common->lock, flags);
 	if (common->state <= new_state) {
 		common->exception_req_tag = common->ep0_req_tag;
@@ -2495,6 +2496,7 @@ static void handle_exception(struct fsg_
 static int fsg_main_thread(void *common_)
 {
 	struct fsg_common	*common = common_;
+	int rc;
 
 	/*
 	 * Allow the thread to be killed by a signal, but set the signal mask
@@ -2518,6 +2520,7 @@ static int fsg_main_thread(void *common_
 	/* The main loop */
 	while (common->state != FSG_STATE_TERMINATED) {
 		if (exception_in_progress(common) || signal_pending(current)) {
+			trace_printk("handling exception\n");
 			handle_exception(common);
 			continue;
 		}
@@ -2527,24 +2530,38 @@ static int fsg_main_thread(void *common_
 			continue;
 		}
 
-		if (get_next_command(common))
+		rc = get_next_command(common);
+		if (rc) {
+			trace_printk("get_next_command -> %d\n", rc);
 			continue;
+		}
 
 		spin_lock_irq(&common->lock);
 		if (!exception_in_progress(common))
 			common->state = FSG_STATE_DATA_PHASE;
 		spin_unlock_irq(&common->lock);
 
-		if (do_scsi_command(common) || finish_reply(common))
+		rc = do_scsi_command(common);
+		if (rc) {
+			trace_printk("do_scsi_command -> %d\n", rc);
+			continue;
+		}
+		rc = finish_reply(common);
+		if (rc) {
+			trace_printk("finish_reply -> %d\n", rc);
 			continue;
+		}
 
 		spin_lock_irq(&common->lock);
 		if (!exception_in_progress(common))
 			common->state = FSG_STATE_STATUS_PHASE;
 		spin_unlock_irq(&common->lock);
 
-		if (send_status(common))
+		rc = send_status(common);
+		if (rc) {
+			trace_printk("send_status -> %d\n", rc);
 			continue;
+		}
 
 		spin_lock_irq(&common->lock);
 		if (!exception_in_progress(common))