[PATCH v2 1/3] usb: f_mass_storage: Check bh->state in sleep_thread
Sean Anderson
sean.anderson at seco.com
Tue Nov 23 21:11:43 CET 2021
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 at 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
More information about the U-Boot
mailing list