USB: f_mass_storage: improve memory barriers and synchronization
authorAlan Stern <stern@rowland.harvard.edu>
Thu, 13 Apr 2017 20:37:01 +0000 (16:37 -0400)
committerFelipe Balbi <felipe.balbi@linux.intel.com>
Tue, 16 May 2017 07:38:33 +0000 (10:38 +0300)
This patch reworks the way f_mass_storage.c handles memory barriers
and synchronization:

The driver now uses a wait_queue instead of doing its own
task-state manipulations (even though only one task will ever
use the wait_queue).

The thread_wakeup_needed variable is removed.  It was only a
source of trouble; although it was what the driver tested to
see whether it should wake up, what we really wanted to see
was whether a USB transfer had completed.

All the explicit memory barriers scattered throughout the
driver are replaced by a few calls to smp_load_acquire() and
smp_store_release().

The inreq_busy and outreq_busy fields are removed.  In their
place, the driver keeps track of the current I/O direction by
splitting BUF_STATE_BUSY into two states: BUF_STATE_SENDING
and BUF_STATE_RECEIVING.

The buffer states are no longer protected by a lock.  Mutual
exclusion isn't needed; the state is changed only by the
driver's main thread when it owns the buffer, and only by the
request completion routine when the gadget core owns the buffer.

The do_write() and throw_away_data() routines were reorganized
to make efficient use of the new sleeping mechanism.  This
resulted in the removal of one indentation level in those
routines, making the patch appear to be more more complicated
than it really is.

In a few places, the driver allowed itself to be frozen although
it really shouldn't have (in the middle of executing a SCSI
command).  Those places have been fixed.

The logic in the exception handler for aborting transfers and
waiting for them to stop has been simplified.

Tested-by: Thinh Nguyen <thinhn@synopsys.com>
Signed-off-by: Alan Stern <stern@rowland.harvard.edu>
Signed-off-by: Felipe Balbi <felipe.balbi@linux.intel.com>
drivers/usb/gadget/function/f_mass_storage.c
drivers/usb/gadget/function/storage_common.h

index a0890a058f09c5f956cb2c2525a952e92f5d2647..e80b9c123a9dd27eb9e547210e1d1ee34b3fcc63 100644 (file)
@@ -260,12 +260,13 @@ struct fsg_common {
        struct usb_gadget       *gadget;
        struct usb_composite_dev *cdev;
        struct fsg_dev          *fsg, *new_fsg;
+       wait_queue_head_t       io_wait;
        wait_queue_head_t       fsg_wait;
 
        /* filesem protects: backing files in use */
        struct rw_semaphore     filesem;
 
-       /* lock protects: state, all the req_busy's */
+       /* lock protects: state and thread_task */
        spinlock_t              lock;
 
        struct usb_ep           *ep0;           /* Copy of gadget->ep0 */
@@ -303,7 +304,6 @@ struct fsg_common {
        unsigned int            running:1;
        unsigned int            sysfs:1;
 
-       int                     thread_wakeup_needed;
        struct completion       thread_notifier;
        struct task_struct      *thread_task;
 
@@ -393,16 +393,6 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
 
 /* These routines may be called in process context or in_irq */
 
-/* Caller must hold fsg->lock */
-static void wakeup_thread(struct fsg_common *common)
-{
-       smp_wmb();      /* ensure the write of bh->state is complete */
-       /* Tell the main thread that something has happened */
-       common->thread_wakeup_needed = 1;
-       if (common->thread_task)
-               wake_up_process(common->thread_task);
-}
-
 static void raise_exception(struct fsg_common *common, enum fsg_state new_state)
 {
        unsigned long           flags;
@@ -456,13 +446,9 @@ static void bulk_in_complete(struct usb_ep *ep, struct usb_request *req)
        if (req->status == -ECONNRESET)         /* Request was cancelled */
                usb_ep_fifo_flush(ep);
 
-       /* Hold the lock while we update the request and buffer states */
-       smp_wmb();
-       spin_lock(&common->lock);
-       bh->inreq_busy = 0;
-       bh->state = BUF_STATE_EMPTY;
-       wakeup_thread(common);
-       spin_unlock(&common->lock);
+       /* Synchronize with the smp_load_acquire() in sleep_thread() */
+       smp_store_release(&bh->state, BUF_STATE_EMPTY);
+       wake_up(&common->io_wait);
 }
 
 static void bulk_out_complete(struct usb_ep *ep, struct usb_request *req)
@@ -477,13 +463,9 @@ static void bulk_out_complete(struct usb_ep *ep, struct usb_request *req)
        if (req->status == -ECONNRESET)         /* Request was cancelled */
                usb_ep_fifo_flush(ep);
 
-       /* Hold the lock while we update the request and buffer states */
-       smp_wmb();
-       spin_lock(&common->lock);
-       bh->outreq_busy = 0;
-       bh->state = BUF_STATE_FULL;
-       wakeup_thread(common);
-       spin_unlock(&common->lock);
+       /* Synchronize with the smp_load_acquire() in sleep_thread() */
+       smp_store_release(&bh->state, BUF_STATE_FULL);
+       wake_up(&common->io_wait);
 }
 
 static int _fsg_common_get_max_lun(struct fsg_common *common)
@@ -559,43 +541,39 @@ static int fsg_setup(struct usb_function *f,
 /* All the following routines run in process context */
 
 /* Use this for bulk or interrupt transfers, not ep0 */
-static void start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
-                          struct usb_request *req, int *pbusy,
-                          enum fsg_buffer_state *state)
+static int start_transfer(struct fsg_dev *fsg, struct usb_ep *ep,
+                          struct usb_request *req)
 {
        int     rc;
 
        if (ep == fsg->bulk_in)
                dump_msg(fsg, "bulk-in", req->buf, req->length);
 
-       spin_lock_irq(&fsg->common->lock);
-       *pbusy = 1;
-       *state = BUF_STATE_BUSY;
-       spin_unlock_irq(&fsg->common->lock);
-
        rc = usb_ep_queue(ep, req, GFP_KERNEL);
-       if (rc == 0)
-               return;  /* All good, we're done */
-
-       *pbusy = 0;
-       *state = BUF_STATE_EMPTY;
+       if (rc) {
 
-       /* We can't do much more than wait for a reset */
+               /* We can't do much more than wait for a reset */
+               req->status = rc;
 
-       /*
-        * Note: currently the net2280 driver fails zero-length
-        * submissions if DMA is enabled.
-        */
-       if (rc != -ESHUTDOWN && !(rc == -EOPNOTSUPP && req->length == 0))
-               WARNING(fsg, "error in submission: %s --> %d\n", ep->name, rc);
+               /*
+                * Note: currently the net2280 driver fails zero-length
+                * submissions if DMA is enabled.
+                */
+               if (rc != -ESHUTDOWN &&
+                               !(rc == -EOPNOTSUPP && req->length == 0))
+                       WARNING(fsg, "error in submission: %s --> %d\n",
+                                       ep->name, rc);
+       }
+       return rc;
 }
 
 static bool start_in_transfer(struct fsg_common *common, struct fsg_buffhd *bh)
 {
        if (!fsg_is_set(common))
                return false;
-       start_transfer(common->fsg, common->fsg->bulk_in,
-                      bh->inreq, &bh->inreq_busy, &bh->state);
+       bh->state = BUF_STATE_SENDING;
+       if (start_transfer(common->fsg, common->fsg->bulk_in, bh->inreq))
+               bh->state = BUF_STATE_EMPTY;
        return true;
 }
 
@@ -603,32 +581,31 @@ static bool start_out_transfer(struct fsg_common *common, struct fsg_buffhd *bh)
 {
        if (!fsg_is_set(common))
                return false;
-       start_transfer(common->fsg, common->fsg->bulk_out,
-                      bh->outreq, &bh->outreq_busy, &bh->state);
+       bh->state = BUF_STATE_RECEIVING;
+       if (start_transfer(common->fsg, common->fsg->bulk_out, bh->outreq))
+               bh->state = BUF_STATE_FULL;
        return true;
 }
 
-static int sleep_thread(struct fsg_common *common, bool can_freeze)
+static int sleep_thread(struct fsg_common *common, bool can_freeze,
+               struct fsg_buffhd *bh)
 {
-       int     rc = 0;
+       int     rc;
 
-       /* Wait until a signal arrives or we are woken up */
-       for (;;) {
-               if (can_freeze)
-                       try_to_freeze();
-               set_current_state(TASK_INTERRUPTIBLE);
-               if (signal_pending(current)) {
-                       rc = -EINTR;
-                       break;
-               }
-               if (common->thread_wakeup_needed)
-                       break;
-               schedule();
-       }
-       __set_current_state(TASK_RUNNING);
-       common->thread_wakeup_needed = 0;
-       smp_rmb();      /* ensure the latest bh->state is visible */
-       return rc;
+       /* Wait until a signal arrives or bh is no longer busy */
+       if (can_freeze)
+               /*
+                * synchronize with the smp_store_release(&bh->state) in
+                * bulk_in_complete() or bulk_out_complete()
+                */
+               rc = wait_event_freezable(common->io_wait,
+                               bh && smp_load_acquire(&bh->state) >=
+                                       BUF_STATE_EMPTY);
+       else
+               rc = wait_event_interruptible(common->io_wait,
+                               bh && smp_load_acquire(&bh->state) >=
+                                       BUF_STATE_EMPTY);
+       return rc ? -EINTR : 0;
 }
 
 
@@ -688,11 +665,9 @@ static int do_read(struct fsg_common *common)
 
                /* Wait for the next buffer to become available */
                bh = common->next_buffhd_to_fill;
-               while (bh->state != BUF_STATE_EMPTY) {
-                       rc = sleep_thread(common, false);
-                       if (rc)
-                               return rc;
-               }
+               rc = sleep_thread(common, false, bh);
+               if (rc)
+                       return rc;
 
                /*
                 * If we were asked to read past the end of file,
@@ -869,84 +844,80 @@ static int do_write(struct fsg_common *common)
                bh = common->next_buffhd_to_drain;
                if (bh->state == BUF_STATE_EMPTY && !get_some_more)
                        break;                  /* We stopped early */
-               if (bh->state == BUF_STATE_FULL) {
-                       smp_rmb();
-                       common->next_buffhd_to_drain = bh->next;
-                       bh->state = BUF_STATE_EMPTY;
-
-                       /* Did something go wrong with the transfer? */
-                       if (bh->outreq->status != 0) {
-                               curlun->sense_data = SS_COMMUNICATION_FAILURE;
-                               curlun->sense_data_info =
+
+               /* Wait for the data to be received */
+               rc = sleep_thread(common, false, bh);
+               if (rc)
+                       return rc;
+
+               common->next_buffhd_to_drain = bh->next;
+               bh->state = BUF_STATE_EMPTY;
+
+               /* Did something go wrong with the transfer? */
+               if (bh->outreq->status != 0) {
+                       curlun->sense_data = SS_COMMUNICATION_FAILURE;
+                       curlun->sense_data_info =
                                        file_offset >> curlun->blkbits;
-                               curlun->info_valid = 1;
-                               break;
-                       }
+                       curlun->info_valid = 1;
+                       break;
+               }
 
-                       amount = bh->outreq->actual;
-                       if (curlun->file_length - file_offset < amount) {
-                               LERROR(curlun,
-                                      "write %u @ %llu beyond end %llu\n",
+               amount = bh->outreq->actual;
+               if (curlun->file_length - file_offset < amount) {
+                       LERROR(curlun, "write %u @ %llu beyond end %llu\n",
                                       amount, (unsigned long long)file_offset,
                                       (unsigned long long)curlun->file_length);
-                               amount = curlun->file_length - file_offset;
-                       }
+                       amount = curlun->file_length - file_offset;
+               }
 
-                       /* Don't accept excess data.  The spec doesn't say
-                        * what to do in this case.  We'll ignore the error.
-                        */
-                       amount = min(amount, bh->bulk_out_intended_length);
-
-                       /* Don't write a partial block */
-                       amount = round_down(amount, curlun->blksize);
-                       if (amount == 0)
-                               goto empty_write;
-
-                       /* Perform the write */
-                       file_offset_tmp = file_offset;
-                       nwritten = vfs_write(curlun->filp,
-                                            (char __user *)bh->buf,
-                                            amount, &file_offset_tmp);
-                       VLDBG(curlun, "file write %u @ %llu -> %d\n", amount,
-                             (unsigned long long)file_offset, (int)nwritten);
-                       if (signal_pending(current))
-                               return -EINTR;          /* Interrupted! */
-
-                       if (nwritten < 0) {
-                               LDBG(curlun, "error in file write: %d\n",
-                                    (int)nwritten);
-                               nwritten = 0;
-                       } else if (nwritten < amount) {
-                               LDBG(curlun, "partial file write: %d/%u\n",
-                                    (int)nwritten, amount);
-                               nwritten = round_down(nwritten, curlun->blksize);
-                       }
-                       file_offset += nwritten;
-                       amount_left_to_write -= nwritten;
-                       common->residue -= nwritten;
+               /*
+                * Don't accept excess data.  The spec doesn't say
+                * what to do in this case.  We'll ignore the error.
+                */
+               amount = min(amount, bh->bulk_out_intended_length);
 
-                       /* If an error occurred, report it and its position */
-                       if (nwritten < amount) {
-                               curlun->sense_data = SS_WRITE_ERROR;
-                               curlun->sense_data_info =
+               /* Don't write a partial block */
+               amount = round_down(amount, curlun->blksize);
+               if (amount == 0)
+                       goto empty_write;
+
+               /* Perform the write */
+               file_offset_tmp = file_offset;
+               nwritten = vfs_write(curlun->filp, (char __user *)bh->buf,
+                               amount, &file_offset_tmp);
+               VLDBG(curlun, "file write %u @ %llu -> %d\n", amount,
+                               (unsigned long long)file_offset, (int)nwritten);
+               if (signal_pending(current))
+                       return -EINTR;          /* Interrupted! */
+
+               if (nwritten < 0) {
+                       LDBG(curlun, "error in file write: %d\n",
+                                       (int) nwritten);
+                       nwritten = 0;
+               } else if (nwritten < amount) {
+                       LDBG(curlun, "partial file write: %d/%u\n",
+                                       (int) nwritten, amount);
+                       nwritten = round_down(nwritten, curlun->blksize);
+               }
+               file_offset += nwritten;
+               amount_left_to_write -= nwritten;
+               common->residue -= nwritten;
+
+               /* If an error occurred, report it and its position */
+               if (nwritten < amount) {
+                       curlun->sense_data = SS_WRITE_ERROR;
+                       curlun->sense_data_info =
                                        file_offset >> curlun->blkbits;
-                               curlun->info_valid = 1;
-                               break;
-                       }
+                       curlun->info_valid = 1;
+                       break;
+               }
 
  empty_write:
-                       /* Did the host decide to stop early? */
-                       if (bh->outreq->actual < bh->bulk_out_intended_length) {
-                               common->short_packet_received = 1;
-                               break;
-                       }
-                       continue;
+               /* Did the host decide to stop early? */
+               if (bh->outreq->actual < bh->bulk_out_intended_length) {
+                       common->short_packet_received = 1;
+                       break;
                }
-
-               /* Wait for something to happen */
-               rc = sleep_thread(common, false);
-               if (rc)
-                       return rc;
        }
 
        return -EIO;            /* No default reply */
@@ -1471,7 +1442,7 @@ static int wedge_bulk_in_endpoint(struct fsg_dev *fsg)
 
 static int throw_away_data(struct fsg_common *common)
 {
-       struct fsg_buffhd       *bh;
+       struct fsg_buffhd       *bh, *bh2;
        u32                     amount;
        int                     rc;
 
@@ -1479,26 +1450,10 @@ static int throw_away_data(struct fsg_common *common)
             bh->state != BUF_STATE_EMPTY || common->usb_amount_left > 0;
             bh = common->next_buffhd_to_drain) {
 
-               /* Throw away the data in a filled buffer */
-               if (bh->state == BUF_STATE_FULL) {
-                       smp_rmb();
-                       bh->state = BUF_STATE_EMPTY;
-                       common->next_buffhd_to_drain = bh->next;
-
-                       /* A short packet or an error ends everything */
-                       if (bh->outreq->actual < bh->bulk_out_intended_length ||
-                           bh->outreq->status != 0) {
-                               raise_exception(common,
-                                               FSG_STATE_ABORT_BULK_OUT);
-                               return -EINTR;
-                       }
-                       continue;
-               }
-
                /* Try to submit another request if we need one */
-               bh = common->next_buffhd_to_fill;
-               if (bh->state == BUF_STATE_EMPTY
-                && common->usb_amount_left > 0) {
+               bh2 = common->next_buffhd_to_fill;
+               if (bh2->state == BUF_STATE_EMPTY &&
+                               common->usb_amount_left > 0) {
                        amount = min(common->usb_amount_left, FSG_BUFLEN);
 
                        /*
@@ -1506,19 +1461,30 @@ static int throw_away_data(struct fsg_common *common)
                         * equal to the buffer size, which is divisible by
                         * the bulk-out maxpacket size.
                         */
-                       set_bulk_out_req_length(common, bh, amount);
-                       if (!start_out_transfer(common, bh))
+                       set_bulk_out_req_length(common, bh2, amount);
+                       if (!start_out_transfer(common, bh2))
                                /* Dunno what to do if common->fsg is NULL */
                                return -EIO;
-                       common->next_buffhd_to_fill = bh->next;
+                       common->next_buffhd_to_fill = bh2->next;
                        common->usb_amount_left -= amount;
                        continue;
                }
 
-               /* Otherwise wait for something to happen */
-               rc = sleep_thread(common, true);
+               /* Wait for the data to be received */
+               rc = sleep_thread(common, false, bh);
                if (rc)
                        return rc;
+
+               /* Throw away the data in a filled buffer */
+               bh->state = BUF_STATE_EMPTY;
+               common->next_buffhd_to_drain = bh->next;
+
+               /* A short packet or an error ends everything */
+               if (bh->outreq->actual < bh->bulk_out_intended_length ||
+                               bh->outreq->status != 0) {
+                       raise_exception(common, FSG_STATE_ABORT_BULK_OUT);
+                       return -EINTR;
+               }
        }
        return 0;
 }
@@ -1636,11 +1602,9 @@ static void send_status(struct fsg_common *common)
 
        /* Wait for the next buffer to become available */
        bh = common->next_buffhd_to_fill;
-       while (bh->state != BUF_STATE_EMPTY) {
-               rc = sleep_thread(common, true);
-               if (rc)
-                       return;
-       }
+       rc = sleep_thread(common, false, bh);
+       if (rc)
+               return;
 
        if (curlun) {
                sd = curlun->sense_data;
@@ -1839,11 +1803,10 @@ static int do_scsi_command(struct fsg_common *common)
        /* Wait for the next buffer to become available for data or status */
        bh = common->next_buffhd_to_fill;
        common->next_buffhd_to_drain = bh;
-       while (bh->state != BUF_STATE_EMPTY) {
-               rc = sleep_thread(common, true);
-               if (rc)
-                       return rc;
-       }
+       rc = sleep_thread(common, false, bh);
+       if (rc)
+               return rc;
+
        common->phase_error = 0;
        common->short_packet_received = 0;
 
@@ -2186,11 +2149,9 @@ static int get_next_command(struct fsg_common *common)
 
        /* Wait for the next buffer to become available */
        bh = common->next_buffhd_to_fill;
-       while (bh->state != BUF_STATE_EMPTY) {
-               rc = sleep_thread(common, true);
-               if (rc)
-                       return rc;
-       }
+       rc = sleep_thread(common, true, bh);
+       if (rc)
+               return rc;
 
        /* Queue a request to read a Bulk-only CBW */
        set_bulk_out_req_length(common, bh, US_BULK_CB_WRAP_LEN);
@@ -2205,12 +2166,10 @@ static int get_next_command(struct fsg_common *common)
         */
 
        /* Wait for the CBW to arrive */
-       while (bh->state != BUF_STATE_FULL) {
-               rc = sleep_thread(common, true);
-               if (rc)
-                       return rc;
-       }
-       smp_rmb();
+       rc = sleep_thread(common, true, bh);
+       if (rc)
+               return rc;
+
        rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;
        bh->state = BUF_STATE_EMPTY;
 
@@ -2374,23 +2333,14 @@ static void handle_exception(struct fsg_common *common)
        if (likely(common->fsg)) {
                for (i = 0; i < common->fsg_num_buffers; ++i) {
                        bh = &common->buffhds[i];
-                       if (bh->inreq_busy)
+                       if (bh->state == BUF_STATE_SENDING)
                                usb_ep_dequeue(common->fsg->bulk_in, bh->inreq);
-                       if (bh->outreq_busy)
+                       if (bh->state == BUF_STATE_RECEIVING)
                                usb_ep_dequeue(common->fsg->bulk_out,
                                               bh->outreq);
-               }
 
-               /* Wait until everything is idle */
-               for (;;) {
-                       int num_active = 0;
-                       for (i = 0; i < common->fsg_num_buffers; ++i) {
-                               bh = &common->buffhds[i];
-                               num_active += bh->inreq_busy + bh->outreq_busy;
-                       }
-                       if (num_active == 0)
-                               break;
-                       if (sleep_thread(common, true))
+                       /* Wait for a transfer to become idle */
+                       if (sleep_thread(common, false, bh))
                                return;
                }
 
@@ -2518,7 +2468,7 @@ static int fsg_main_thread(void *common_)
                }
 
                if (!common->running) {
-                       sleep_thread(common, true);
+                       sleep_thread(common, true, NULL);
                        continue;
                }
 
@@ -2648,6 +2598,7 @@ static struct fsg_common *fsg_common_setup(struct fsg_common *common)
        spin_lock_init(&common->lock);
        kref_init(&common->ref);
        init_completion(&common->thread_notifier);
+       init_waitqueue_head(&common->io_wait);
        init_waitqueue_head(&common->fsg_wait);
        common->state = FSG_STATE_TERMINATED;
        memset(common->luns, 0, sizeof(common->luns));
index e6095dfbf1d5588e18f03277ae47486aa935c3e5..e0814a96013203752b2ab7400e2b70463c2cd82a 100644 (file)
@@ -133,9 +133,10 @@ static inline bool fsg_lun_is_open(struct fsg_lun *curlun)
 #define FSG_MAX_LUNS   16
 
 enum fsg_buffer_state {
+       BUF_STATE_SENDING = -2,
+       BUF_STATE_RECEIVING,
        BUF_STATE_EMPTY = 0,
-       BUF_STATE_FULL,
-       BUF_STATE_BUSY
+       BUF_STATE_FULL
 };
 
 struct fsg_buffhd {
@@ -151,9 +152,7 @@ struct fsg_buffhd {
        unsigned int                    bulk_out_intended_length;
 
        struct usb_request              *inreq;
-       int                             inreq_busy;
        struct usb_request              *outreq;
-       int                             outreq_busy;
 };
 
 enum fsg_state {