| Message ID | 20210722183811.76441-1-sean.anderson@seco.com |
|---|---|
| State | Deferred |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2,1/3] usb: f_mass_storage: Check bh->state in sleep_thread | expand |
On 7/22/21 2:38 PM, Sean Anderson wrote: > Every caller of sleep_thread except one wraps it in a second loop which > waits for a buffer head to be filled. Since sleep_thread is already > (infinitely) looping, we can move this check down. Some parts of the code > have been reorganized to better take advantage of this property and and be > structured closer to their Linux counterparts. > > In addition, sleep_thread now returns -EINTR if we need to wake up, > mirroring Linux. This takes advantage of the existing logic regarding > sleep_thread, which typically returns immediately on error. With this > change, any exception causes the current operation to be backed out of > immediately. > > Lastly, we only clear thread_wakeup_needed in handle_exception, mirroring > Linux's signal-handling logic. Any subsequent calls to sleep_thread will > still wake up. > > While these changes are conceptually different, their implementation is > interdependent, so they are all performed at once. > > These changes are primarily inspired by Linux commit 225785aec726 ("USB: > f_mass_storage: improve memory barriers and synchronization"). > > Signed-off-by: Sean Anderson <sean.anderson@seco.com> > --- > > Changes in v2: > - Fix some checkpatch lint > - Make comment for sleep thread match loop condition > > drivers/usb/gadget/f_mass_storage.c | 228 ++++++++++++---------------- > drivers/usb/gadget/storage_common.c | 2 +- > 2 files changed, 101 insertions(+), 129 deletions(-) > > diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c > index 45f0504b6e..6f39c24503 100644 > --- a/drivers/usb/gadget/f_mass_storage.c > +++ b/drivers/usb/gadget/f_mass_storage.c > @@ -652,15 +652,17 @@ static void busy_indicator(void) > state = 0; > } > > -static int sleep_thread(struct fsg_common *common) > +static int sleep_thread(struct fsg_common *common, struct fsg_buffhd *bh) > { > - int rc = 0; > int i = 0, k = 0; > > - /* Wait until a signal arrives or we are woken up */ > - for (;;) { > - if (common->thread_wakeup_needed) > - break; > + /* > + * Wait until a signal arrives, we are woken up, or the buffer is no > + * longer busy. > + */ > + while (!bh || bh->state == BUF_STATE_BUSY) { > + if (exception_in_progress(common)) > + return -EINTR; > > if (++i == 20000) { > busy_indicator(); > @@ -682,8 +684,7 @@ static int sleep_thread(struct fsg_common *common) > > usb_gadget_handle_interrupts(controller_index); > } > - common->thread_wakeup_needed = 0; > - return rc; > + return 0; > } > > /*-------------------------------------------------------------------------*/ > @@ -744,11 +745,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); > - if (rc) > - return rc; > - } > + rc = sleep_thread(common, bh); > + if (rc) > + return rc; > > /* If we were asked to read past the end of file, > * end with an empty buffer. */ > @@ -922,67 +921,63 @@ 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) { > - 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->info_valid = 1; > - break; > - } > + rc = sleep_thread(common, bh); > + if (rc) > + return rc; > > - amount = bh->outreq->actual; > + common->next_buffhd_to_drain = bh->next; > + bh->state = BUF_STATE_EMPTY; > > - /* Perform the write */ > - rc = ums[common->lun].write_sector(&ums[common->lun], > - file_offset / SECTOR_SIZE, > - amount / SECTOR_SIZE, > - (char __user *)bh->buf); > - if (!rc) > - return -EIO; > - nwritten = rc * SECTOR_SIZE; > - > - VLDBG(curlun, "file write %u @ %llu -> %d\n", amount, > - (unsigned long long) file_offset, > - (int) nwritten); > - > - 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 -= (nwritten & 511); > - /* Round down to a block */ > - } > - file_offset += nwritten; > - amount_left_to_write -= nwritten; > - common->residue -= nwritten; > - > - /* If an error occurred, report it and its position */ > - if (nwritten < amount) { > - printf("nwritten:%zd amount:%u\n", nwritten, > - amount); > - curlun->sense_data = SS_WRITE_ERROR; > - curlun->info_valid = 1; > - break; > - } > + /* Did something go wrong with the transfer? */ > + if (bh->outreq->status != 0) { > + curlun->sense_data = SS_COMMUNICATION_FAILURE; > + curlun->info_valid = 1; > + break; > + } > > - /* Did the host decide to stop early? */ > - if (bh->outreq->actual != bh->outreq->length) { > - common->short_packet_received = 1; > - break; > - } > - continue; > + amount = bh->outreq->actual; > + > + /* Perform the write */ > + rc = ums[common->lun].write_sector(&ums[common->lun], > + file_offset / SECTOR_SIZE, > + amount / SECTOR_SIZE, > + (char __user *)bh->buf); > + if (!rc) > + return -EIO; > + nwritten = rc * SECTOR_SIZE; > + > + VLDBG(curlun, "file write %u @ %llu -> %d\n", amount, > + (unsigned long long)file_offset, (int)nwritten); > + > + 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 -= (nwritten & 511); > + /* Round down to a block */ > } > + file_offset += nwritten; > + amount_left_to_write -= nwritten; > + common->residue -= nwritten; > > - /* Wait for something to happen */ > - rc = sleep_thread(common); > - if (rc) > - return rc; > + /* If an error occurred, report it and its position */ > + if (nwritten < amount) { > + printf("nwritten:%zd amount:%u\n", nwritten, > + amount); > + curlun->sense_data = SS_WRITE_ERROR; > + curlun->info_valid = 1; > + break; > + } > + > + /* Did the host decide to stop early? */ > + if (bh->outreq->actual != bh->outreq->length) { > + common->short_packet_received = 1; > + break; > + } > } > > return -EIO; /* No default reply */ > @@ -1430,13 +1425,10 @@ static int pad_with_zeros(struct fsg_dev *fsg) > bh->state = BUF_STATE_EMPTY; /* For the first iteration */ > fsg->common->usb_amount_left = nkeep + fsg->common->residue; > while (fsg->common->usb_amount_left > 0) { > - > /* Wait for the next buffer to be free */ > - while (bh->state != BUF_STATE_EMPTY) { > - rc = sleep_thread(fsg->common); > - if (rc) > - return rc; > - } > + rc = sleep_thread(fsg->common, bh); > + if (rc) > + return rc; > > nsend = min(fsg->common->usb_amount_left, FSG_BUFLEN); > memset(bh->buf + nkeep, 0, nsend - nkeep); > @@ -1461,23 +1453,7 @@ 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) { > - 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->outreq->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) { > amount = min(common->usb_amount_left, FSG_BUFLEN); > @@ -1494,13 +1470,24 @@ static int throw_away_data(struct fsg_common *common) > return -EIO; > common->next_buffhd_to_fill = bh->next; > common->usb_amount_left -= amount; > - continue; > } > > - /* Otherwise wait for something to happen */ > - rc = sleep_thread(common); > + /* Wait for the data to be received */ > + rc = sleep_thread(common, 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->outreq->length || > + bh->outreq->status != 0) { > + raise_exception(common, > + FSG_STATE_ABORT_BULK_OUT); > + return -EINTR; > + } > } > return 0; > } > @@ -1613,11 +1600,9 @@ static int 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); > - if (rc) > - return rc; > - } > + rc = sleep_thread(common, bh); > + if (rc) > + return rc; > > if (curlun) > sd = curlun->sense_data; > @@ -1790,11 +1775,9 @@ 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); > - if (rc) > - return rc; > - } > + rc = sleep_thread(common, bh); > + if (rc) > + return rc; > common->phase_error = 0; > common->short_packet_received = 0; > > @@ -2118,11 +2101,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); > - if (rc) > - return rc; > - } > + rc = sleep_thread(common, bh); > + if (rc) > + return rc; > > /* Queue a request to read a Bulk-only CBW */ > set_bulk_out_req_length(common, bh, USB_BULK_CB_WRAP_LEN); > @@ -2137,11 +2118,9 @@ static int get_next_command(struct fsg_common *common) > * next_buffhd_to_fill. */ > > /* Wait for the CBW to arrive */ > - while (bh->state != BUF_STATE_FULL) { > - rc = sleep_thread(common); > - if (rc) > - return rc; > - } > + rc = sleep_thread(common, bh); > + if (rc) > + return rc; > > rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO; > bh->state = BUF_STATE_EMPTY; > @@ -2291,6 +2270,7 @@ static void handle_exception(struct fsg_common *common) > struct fsg_lun *curlun; > unsigned int exception_req_tag; > > + common->thread_wakeup_needed = 0; > /* Cancel all the pending transfers */ > if (common->fsg) { > for (i = 0; i < FSG_NUM_BUFFERS; ++i) { > @@ -2300,18 +2280,9 @@ static void handle_exception(struct fsg_common *common) > if (bh->outreq_busy) > usb_ep_dequeue(common->fsg->bulk_out, > bh->outreq); > - } > > - /* Wait until everything is idle */ > - for (;;) { > - int num_active = 0; > - for (i = 0; i < 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)) > + /* Wait until the transfer is idle */ > + if (sleep_thread(common, bh)) > return; > } > > @@ -2403,15 +2374,16 @@ int fsg_main_thread(void *common_) > } > > if (!common->running) { > - ret = sleep_thread(common); > - if (ret) > + ret = sleep_thread(common, NULL); > + if (ret != -EINTR) > return ret; > - > continue; > } > > ret = get_next_command(common); > - if (ret) > + if (ret == -EINTR) > + continue; > + else if (ret) > return ret; > > if (!exception_in_progress(common)) > diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c > index 5674e8fe49..723549ef09 100644 > --- a/drivers/usb/gadget/storage_common.c > +++ b/drivers/usb/gadget/storage_common.c > @@ -318,7 +318,7 @@ static struct fsg_lun *fsg_lun_from_dev(struct device *dev) > enum fsg_buffer_state { > BUF_STATE_EMPTY = 0, > BUF_STATE_FULL, > - BUF_STATE_BUSY > + BUF_STATE_BUSY, > }; > > struct fsg_buffhd { > ping? Can you have a look at this, Lukasz? --Sean
On 10/28/21 3:40 PM, Sean Anderson wrote: > On 7/22/21 2:38 PM, Sean Anderson wrote: >> Every caller of sleep_thread except one wraps it in a second loop which >> waits for a buffer head to be filled. Since sleep_thread is already >> (infinitely) looping, we can move this check down. Some parts of the code >> have been reorganized to better take advantage of this property and and be >> structured closer to their Linux counterparts. >> >> In addition, sleep_thread now returns -EINTR if we need to wake up, >> mirroring Linux. This takes advantage of the existing logic regarding >> sleep_thread, which typically returns immediately on error. With this >> change, any exception causes the current operation to be backed out of >> immediately. >> >> Lastly, we only clear thread_wakeup_needed in handle_exception, mirroring >> Linux's signal-handling logic. Any subsequent calls to sleep_thread will >> still wake up. >> >> While these changes are conceptually different, their implementation is >> interdependent, so they are all performed at once. >> >> These changes are primarily inspired by Linux commit 225785aec726 ("USB: >> f_mass_storage: improve memory barriers and synchronization"). >> >> Signed-off-by: Sean Anderson <sean.anderson@seco.com> >> --- >> >> Changes in v2: >> - Fix some checkpatch lint >> - Make comment for sleep thread match loop condition >> >> drivers/usb/gadget/f_mass_storage.c | 228 ++++++++++++---------------- >> drivers/usb/gadget/storage_common.c | 2 +- >> 2 files changed, 101 insertions(+), 129 deletions(-) >> >> diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c >> index 45f0504b6e..6f39c24503 100644 >> --- a/drivers/usb/gadget/f_mass_storage.c >> +++ b/drivers/usb/gadget/f_mass_storage.c >> @@ -652,15 +652,17 @@ static void busy_indicator(void) >> state = 0; >> } >> -static int sleep_thread(struct fsg_common *common) >> +static int sleep_thread(struct fsg_common *common, struct fsg_buffhd *bh) >> { >> - int rc = 0; >> int i = 0, k = 0; >> - /* Wait until a signal arrives or we are woken up */ >> - for (;;) { >> - if (common->thread_wakeup_needed) >> - break; >> + /* >> + * Wait until a signal arrives, we are woken up, or the buffer is no >> + * longer busy. >> + */ >> + while (!bh || bh->state == BUF_STATE_BUSY) { >> + if (exception_in_progress(common)) >> + return -EINTR; >> if (++i == 20000) { >> busy_indicator(); >> @@ -682,8 +684,7 @@ static int sleep_thread(struct fsg_common *common) >> usb_gadget_handle_interrupts(controller_index); >> } >> - common->thread_wakeup_needed = 0; >> - return rc; >> + return 0; >> } >> /*-------------------------------------------------------------------------*/ >> @@ -744,11 +745,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); >> - if (rc) >> - return rc; >> - } >> + rc = sleep_thread(common, bh); >> + if (rc) >> + return rc; >> /* If we were asked to read past the end of file, >> * end with an empty buffer. */ >> @@ -922,67 +921,63 @@ 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) { >> - 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->info_valid = 1; >> - break; >> - } >> + rc = sleep_thread(common, bh); >> + if (rc) >> + return rc; >> - amount = bh->outreq->actual; >> + common->next_buffhd_to_drain = bh->next; >> + bh->state = BUF_STATE_EMPTY; >> - /* Perform the write */ >> - rc = ums[common->lun].write_sector(&ums[common->lun], >> - file_offset / SECTOR_SIZE, >> - amount / SECTOR_SIZE, >> - (char __user *)bh->buf); >> - if (!rc) >> - return -EIO; >> - nwritten = rc * SECTOR_SIZE; >> - >> - VLDBG(curlun, "file write %u @ %llu -> %d\n", amount, >> - (unsigned long long) file_offset, >> - (int) nwritten); >> - >> - 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 -= (nwritten & 511); >> - /* Round down to a block */ >> - } >> - file_offset += nwritten; >> - amount_left_to_write -= nwritten; >> - common->residue -= nwritten; >> - >> - /* If an error occurred, report it and its position */ >> - if (nwritten < amount) { >> - printf("nwritten:%zd amount:%u\n", nwritten, >> - amount); >> - curlun->sense_data = SS_WRITE_ERROR; >> - curlun->info_valid = 1; >> - break; >> - } >> + /* Did something go wrong with the transfer? */ >> + if (bh->outreq->status != 0) { >> + curlun->sense_data = SS_COMMUNICATION_FAILURE; >> + curlun->info_valid = 1; >> + break; >> + } >> - /* Did the host decide to stop early? */ >> - if (bh->outreq->actual != bh->outreq->length) { >> - common->short_packet_received = 1; >> - break; >> - } >> - continue; >> + amount = bh->outreq->actual; >> + >> + /* Perform the write */ >> + rc = ums[common->lun].write_sector(&ums[common->lun], >> + file_offset / SECTOR_SIZE, >> + amount / SECTOR_SIZE, >> + (char __user *)bh->buf); >> + if (!rc) >> + return -EIO; >> + nwritten = rc * SECTOR_SIZE; >> + >> + VLDBG(curlun, "file write %u @ %llu -> %d\n", amount, >> + (unsigned long long)file_offset, (int)nwritten); >> + >> + 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 -= (nwritten & 511); >> + /* Round down to a block */ >> } >> + file_offset += nwritten; >> + amount_left_to_write -= nwritten; >> + common->residue -= nwritten; >> - /* Wait for something to happen */ >> - rc = sleep_thread(common); >> - if (rc) >> - return rc; >> + /* If an error occurred, report it and its position */ >> + if (nwritten < amount) { >> + printf("nwritten:%zd amount:%u\n", nwritten, >> + amount); >> + curlun->sense_data = SS_WRITE_ERROR; >> + curlun->info_valid = 1; >> + break; >> + } >> + >> + /* Did the host decide to stop early? */ >> + if (bh->outreq->actual != bh->outreq->length) { >> + common->short_packet_received = 1; >> + break; >> + } >> } >> return -EIO; /* No default reply */ >> @@ -1430,13 +1425,10 @@ static int pad_with_zeros(struct fsg_dev *fsg) >> bh->state = BUF_STATE_EMPTY; /* For the first iteration */ >> fsg->common->usb_amount_left = nkeep + fsg->common->residue; >> while (fsg->common->usb_amount_left > 0) { >> - >> /* Wait for the next buffer to be free */ >> - while (bh->state != BUF_STATE_EMPTY) { >> - rc = sleep_thread(fsg->common); >> - if (rc) >> - return rc; >> - } >> + rc = sleep_thread(fsg->common, bh); >> + if (rc) >> + return rc; >> nsend = min(fsg->common->usb_amount_left, FSG_BUFLEN); >> memset(bh->buf + nkeep, 0, nsend - nkeep); >> @@ -1461,23 +1453,7 @@ 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) { >> - 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->outreq->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) { >> amount = min(common->usb_amount_left, FSG_BUFLEN); >> @@ -1494,13 +1470,24 @@ static int throw_away_data(struct fsg_common *common) >> return -EIO; >> common->next_buffhd_to_fill = bh->next; >> common->usb_amount_left -= amount; >> - continue; >> } >> - /* Otherwise wait for something to happen */ >> - rc = sleep_thread(common); >> + /* Wait for the data to be received */ >> + rc = sleep_thread(common, 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->outreq->length || >> + bh->outreq->status != 0) { >> + raise_exception(common, >> + FSG_STATE_ABORT_BULK_OUT); >> + return -EINTR; >> + } >> } >> return 0; >> } >> @@ -1613,11 +1600,9 @@ static int 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); >> - if (rc) >> - return rc; >> - } >> + rc = sleep_thread(common, bh); >> + if (rc) >> + return rc; >> if (curlun) >> sd = curlun->sense_data; >> @@ -1790,11 +1775,9 @@ 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); >> - if (rc) >> - return rc; >> - } >> + rc = sleep_thread(common, bh); >> + if (rc) >> + return rc; >> common->phase_error = 0; >> common->short_packet_received = 0; >> @@ -2118,11 +2101,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); >> - if (rc) >> - return rc; >> - } >> + rc = sleep_thread(common, bh); >> + if (rc) >> + return rc; >> /* Queue a request to read a Bulk-only CBW */ >> set_bulk_out_req_length(common, bh, USB_BULK_CB_WRAP_LEN); >> @@ -2137,11 +2118,9 @@ static int get_next_command(struct fsg_common *common) >> * next_buffhd_to_fill. */ >> /* Wait for the CBW to arrive */ >> - while (bh->state != BUF_STATE_FULL) { >> - rc = sleep_thread(common); >> - if (rc) >> - return rc; >> - } >> + rc = sleep_thread(common, bh); >> + if (rc) >> + return rc; >> rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO; >> bh->state = BUF_STATE_EMPTY; >> @@ -2291,6 +2270,7 @@ static void handle_exception(struct fsg_common *common) >> struct fsg_lun *curlun; >> unsigned int exception_req_tag; >> + common->thread_wakeup_needed = 0; >> /* Cancel all the pending transfers */ >> if (common->fsg) { >> for (i = 0; i < FSG_NUM_BUFFERS; ++i) { >> @@ -2300,18 +2280,9 @@ static void handle_exception(struct fsg_common *common) >> if (bh->outreq_busy) >> usb_ep_dequeue(common->fsg->bulk_out, >> bh->outreq); >> - } >> - /* Wait until everything is idle */ >> - for (;;) { >> - int num_active = 0; >> - for (i = 0; i < 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)) >> + /* Wait until the transfer is idle */ >> + if (sleep_thread(common, bh)) >> return; >> } >> @@ -2403,15 +2374,16 @@ int fsg_main_thread(void *common_) >> } >> if (!common->running) { >> - ret = sleep_thread(common); >> - if (ret) >> + ret = sleep_thread(common, NULL); >> + if (ret != -EINTR) >> return ret; >> - >> continue; >> } >> ret = get_next_command(common); >> - if (ret) >> + if (ret == -EINTR) >> + continue; >> + else if (ret) >> return ret; >> if (!exception_in_progress(common)) >> diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c >> index 5674e8fe49..723549ef09 100644 >> --- a/drivers/usb/gadget/storage_common.c >> +++ b/drivers/usb/gadget/storage_common.c >> @@ -318,7 +318,7 @@ static struct fsg_lun *fsg_lun_from_dev(struct device *dev) >> enum fsg_buffer_state { >> BUF_STATE_EMPTY = 0, >> BUF_STATE_FULL, >> - BUF_STATE_BUSY >> + BUF_STATE_BUSY, >> }; >> struct fsg_buffhd { >> > > ping? > > Can you have a look at this, Lukasz? Hm, looks like this is not going to get reviewed... More generally, it seems like Lukasz doesn't have the time to maintain the clock/dfu subsystems. I would be willing to be a reviewer for clock, but I don't know enough about USB to review things properly. --Sean
diff --git a/drivers/usb/gadget/f_mass_storage.c b/drivers/usb/gadget/f_mass_storage.c index 45f0504b6e..6f39c24503 100644 --- a/drivers/usb/gadget/f_mass_storage.c +++ b/drivers/usb/gadget/f_mass_storage.c @@ -652,15 +652,17 @@ static void busy_indicator(void) state = 0; } -static int sleep_thread(struct fsg_common *common) +static int sleep_thread(struct fsg_common *common, struct fsg_buffhd *bh) { - int rc = 0; int i = 0, k = 0; - /* Wait until a signal arrives or we are woken up */ - for (;;) { - if (common->thread_wakeup_needed) - break; + /* + * Wait until a signal arrives, we are woken up, or the buffer is no + * longer busy. + */ + while (!bh || bh->state == BUF_STATE_BUSY) { + if (exception_in_progress(common)) + return -EINTR; if (++i == 20000) { busy_indicator(); @@ -682,8 +684,7 @@ static int sleep_thread(struct fsg_common *common) usb_gadget_handle_interrupts(controller_index); } - common->thread_wakeup_needed = 0; - return rc; + return 0; } /*-------------------------------------------------------------------------*/ @@ -744,11 +745,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); - if (rc) - return rc; - } + rc = sleep_thread(common, bh); + if (rc) + return rc; /* If we were asked to read past the end of file, * end with an empty buffer. */ @@ -922,67 +921,63 @@ 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) { - 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->info_valid = 1; - break; - } + rc = sleep_thread(common, bh); + if (rc) + return rc; - amount = bh->outreq->actual; + common->next_buffhd_to_drain = bh->next; + bh->state = BUF_STATE_EMPTY; - /* Perform the write */ - rc = ums[common->lun].write_sector(&ums[common->lun], - file_offset / SECTOR_SIZE, - amount / SECTOR_SIZE, - (char __user *)bh->buf); - if (!rc) - return -EIO; - nwritten = rc * SECTOR_SIZE; - - VLDBG(curlun, "file write %u @ %llu -> %d\n", amount, - (unsigned long long) file_offset, - (int) nwritten); - - 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 -= (nwritten & 511); - /* Round down to a block */ - } - file_offset += nwritten; - amount_left_to_write -= nwritten; - common->residue -= nwritten; - - /* If an error occurred, report it and its position */ - if (nwritten < amount) { - printf("nwritten:%zd amount:%u\n", nwritten, - amount); - curlun->sense_data = SS_WRITE_ERROR; - curlun->info_valid = 1; - break; - } + /* Did something go wrong with the transfer? */ + if (bh->outreq->status != 0) { + curlun->sense_data = SS_COMMUNICATION_FAILURE; + curlun->info_valid = 1; + break; + } - /* Did the host decide to stop early? */ - if (bh->outreq->actual != bh->outreq->length) { - common->short_packet_received = 1; - break; - } - continue; + amount = bh->outreq->actual; + + /* Perform the write */ + rc = ums[common->lun].write_sector(&ums[common->lun], + file_offset / SECTOR_SIZE, + amount / SECTOR_SIZE, + (char __user *)bh->buf); + if (!rc) + return -EIO; + nwritten = rc * SECTOR_SIZE; + + VLDBG(curlun, "file write %u @ %llu -> %d\n", amount, + (unsigned long long)file_offset, (int)nwritten); + + 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 -= (nwritten & 511); + /* Round down to a block */ } + file_offset += nwritten; + amount_left_to_write -= nwritten; + common->residue -= nwritten; - /* Wait for something to happen */ - rc = sleep_thread(common); - if (rc) - return rc; + /* If an error occurred, report it and its position */ + if (nwritten < amount) { + printf("nwritten:%zd amount:%u\n", nwritten, + amount); + curlun->sense_data = SS_WRITE_ERROR; + curlun->info_valid = 1; + break; + } + + /* Did the host decide to stop early? */ + if (bh->outreq->actual != bh->outreq->length) { + common->short_packet_received = 1; + break; + } } return -EIO; /* No default reply */ @@ -1430,13 +1425,10 @@ static int pad_with_zeros(struct fsg_dev *fsg) bh->state = BUF_STATE_EMPTY; /* For the first iteration */ fsg->common->usb_amount_left = nkeep + fsg->common->residue; while (fsg->common->usb_amount_left > 0) { - /* Wait for the next buffer to be free */ - while (bh->state != BUF_STATE_EMPTY) { - rc = sleep_thread(fsg->common); - if (rc) - return rc; - } + rc = sleep_thread(fsg->common, bh); + if (rc) + return rc; nsend = min(fsg->common->usb_amount_left, FSG_BUFLEN); memset(bh->buf + nkeep, 0, nsend - nkeep); @@ -1461,23 +1453,7 @@ 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) { - 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->outreq->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) { amount = min(common->usb_amount_left, FSG_BUFLEN); @@ -1494,13 +1470,24 @@ static int throw_away_data(struct fsg_common *common) return -EIO; common->next_buffhd_to_fill = bh->next; common->usb_amount_left -= amount; - continue; } - /* Otherwise wait for something to happen */ - rc = sleep_thread(common); + /* Wait for the data to be received */ + rc = sleep_thread(common, 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->outreq->length || + bh->outreq->status != 0) { + raise_exception(common, + FSG_STATE_ABORT_BULK_OUT); + return -EINTR; + } } return 0; } @@ -1613,11 +1600,9 @@ static int 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); - if (rc) - return rc; - } + rc = sleep_thread(common, bh); + if (rc) + return rc; if (curlun) sd = curlun->sense_data; @@ -1790,11 +1775,9 @@ 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); - if (rc) - return rc; - } + rc = sleep_thread(common, bh); + if (rc) + return rc; common->phase_error = 0; common->short_packet_received = 0; @@ -2118,11 +2101,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); - if (rc) - return rc; - } + rc = sleep_thread(common, bh); + if (rc) + return rc; /* Queue a request to read a Bulk-only CBW */ set_bulk_out_req_length(common, bh, USB_BULK_CB_WRAP_LEN); @@ -2137,11 +2118,9 @@ static int get_next_command(struct fsg_common *common) * next_buffhd_to_fill. */ /* Wait for the CBW to arrive */ - while (bh->state != BUF_STATE_FULL) { - rc = sleep_thread(common); - if (rc) - return rc; - } + rc = sleep_thread(common, bh); + if (rc) + return rc; rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO; bh->state = BUF_STATE_EMPTY; @@ -2291,6 +2270,7 @@ static void handle_exception(struct fsg_common *common) struct fsg_lun *curlun; unsigned int exception_req_tag; + common->thread_wakeup_needed = 0; /* Cancel all the pending transfers */ if (common->fsg) { for (i = 0; i < FSG_NUM_BUFFERS; ++i) { @@ -2300,18 +2280,9 @@ static void handle_exception(struct fsg_common *common) if (bh->outreq_busy) usb_ep_dequeue(common->fsg->bulk_out, bh->outreq); - } - /* Wait until everything is idle */ - for (;;) { - int num_active = 0; - for (i = 0; i < 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)) + /* Wait until the transfer is idle */ + if (sleep_thread(common, bh)) return; } @@ -2403,15 +2374,16 @@ int fsg_main_thread(void *common_) } if (!common->running) { - ret = sleep_thread(common); - if (ret) + ret = sleep_thread(common, NULL); + if (ret != -EINTR) return ret; - continue; } ret = get_next_command(common); - if (ret) + if (ret == -EINTR) + continue; + else if (ret) return ret; if (!exception_in_progress(common)) diff --git a/drivers/usb/gadget/storage_common.c b/drivers/usb/gadget/storage_common.c index 5674e8fe49..723549ef09 100644 --- a/drivers/usb/gadget/storage_common.c +++ b/drivers/usb/gadget/storage_common.c @@ -318,7 +318,7 @@ static struct fsg_lun *fsg_lun_from_dev(struct device *dev) enum fsg_buffer_state { BUF_STATE_EMPTY = 0, BUF_STATE_FULL, - BUF_STATE_BUSY + BUF_STATE_BUSY, }; struct fsg_buffhd {
Every caller of sleep_thread except one wraps it in a second loop which waits for a buffer head to be filled. Since sleep_thread is already (infinitely) looping, we can move this check down. Some parts of the code have been reorganized to better take advantage of this property and and be structured closer to their Linux counterparts. In addition, sleep_thread now returns -EINTR if we need to wake up, mirroring Linux. This takes advantage of the existing logic regarding sleep_thread, which typically returns immediately on error. With this change, any exception causes the current operation to be backed out of immediately. Lastly, we only clear thread_wakeup_needed in handle_exception, mirroring Linux's signal-handling logic. Any subsequent calls to sleep_thread will still wake up. While these changes are conceptually different, their implementation is interdependent, so they are all performed at once. These changes are primarily inspired by Linux commit 225785aec726 ("USB: f_mass_storage: improve memory barriers and synchronization"). Signed-off-by: Sean Anderson <sean.anderson@seco.com> --- Changes in v2: - Fix some checkpatch lint - Make comment for sleep thread match loop condition drivers/usb/gadget/f_mass_storage.c | 228 ++++++++++++---------------- drivers/usb/gadget/storage_common.c | 2 +- 2 files changed, 101 insertions(+), 129 deletions(-)