From 225785aec726f3edd5077be8f084b0b70ca197a8 Mon Sep 17 00:00:00 2001 From: Alan Stern Date: Thu, 13 Apr 2017 16:37:01 -0400 Subject: [PATCH] USB: f_mass_storage: improve memory barriers and synchronization 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 Signed-off-by: Alan Stern Signed-off-by: Felipe Balbi --- drivers/usb/gadget/function/f_mass_storage.c | 355 ++++++++----------- drivers/usb/gadget/function/storage_common.h | 7 +- 2 files changed, 156 insertions(+), 206 deletions(-) diff --git a/drivers/usb/gadget/function/f_mass_storage.c b/drivers/usb/gadget/function/f_mass_storage.c index a0890a058f09..e80b9c123a9d 100644 --- a/drivers/usb/gadget/function/f_mass_storage.c +++ b/drivers/usb/gadget/function/f_mass_storage.c @@ -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)); diff --git a/drivers/usb/gadget/function/storage_common.h b/drivers/usb/gadget/function/storage_common.h index e6095dfbf1d5..e0814a960132 100644 --- a/drivers/usb/gadget/function/storage_common.h +++ b/drivers/usb/gadget/function/storage_common.h @@ -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 { -- 2.20.1