On Mon, 5 Sep 2016, Felipe Balbi wrote: > >> [ 34.123281] ==> bulk_out_complete 484 > > > > received CBW. Now, bh->state is set to BUF_STATE_FULL, and ... > > > >> [ 34.123283] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1 > > > > wakeup_thread() is called, however ... > > > >> [ 34.123283] ==> get_next_command 2251 -> state != full > > > > get_next_command() still sees bh->state != BUF_STATE_FULL. Okay, that's very strange. It suggests something may actively be changing bh->state back to BUF_STATE_EMPTY. I can't imagine what. > >> [ 34.123286] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep > >> [ 34.123289] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep > > > > so it goes to sleep and never wakes up again. It looks like the thread wakes up and goes back to sleep right away several times. This wouldn't happen if the problem were that the CPU didn't see the update to bh->state. > one thing that caught my attention is that not all accesses to bh->state > are locked. Is that an oversight or is there a valid reason for that? To the largest extent possible, I tried to write the driver so that the thread wouldn't have to do any locking, only the completion handlers would. Looking back on it now, I realize that even less locking should be necessary, because for each IN buffer the thread is a producer and the completion handler is a consumer, and for each OUT buffer the handler is a producer and the thread is a consumer. Producer/consumer patterns don't need locking, just proper ordering. As for what ordering is needed, I'll discuss that in the other email thread. > Considering that accessing bh->state would always guarantee a full > barrier (at least on x86, per Peter Z), I'm wondering if we should make > every access to bh->state locked. Or, perhaps, they should all be > preceeded with a call to smp_mb() in order to guarantee that previous > writes to bh->state are seen by current CPU? > > I can test that out, but I'll wait for a proper patch from you as I > cannot predict all memory ordering problems that could arise from > current code. Still, f_mass_storage.c looks really odd :-s It definitely could use some clean-up attention. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
