Hi,
Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
> On Sat, 3 Sep 2016, Felipe Balbi wrote:
>
>> Hi,
>>
>> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
>> > On Fri, 2 Sep 2016, Felipe Balbi wrote:
>> >
>> >>
>> >> Hi,
>> >>
>> >> Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> writes:
>> >>
>> >> [...]
>> >>
>> >> > No, that would be too late. The barrier needs to go between the write
>> >> > to common->thead_wakeup_needed and the call to wake_up_process().
>> >>
>> >> alright, I tested this but it doesn't work. It still hangs :-(
>> >
>> > Okay, I was wrong. But see my recent reply to Paul; maybe the
>> > smp_wmb() in wakeup_thread() needs to be smp_mb().
>>
>> okay, this seems to be working so far:
>>
>> modified drivers/usb/gadget/function/f_mass_storage.c
>> @@ -395,7 +395,7 @@ static int fsg_set_halt(struct fsg_dev *fsg, struct usb_ep *ep)
>> /* Caller must hold fsg->lock */
>> static void wakeup_thread(struct fsg_common *common)
>> {
>> - smp_wmb(); /* ensure the write of bh->state is complete */
>> + smp_mb(); /* 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)
>> @@ -626,7 +626,7 @@ static int sleep_thread(struct fsg_common *common, bool can_freeze)
>> }
>> __set_current_state(TASK_RUNNING);
>> common->thread_wakeup_needed = 0;
>> - smp_rmb(); /* ensure the latest bh->state is visible */
>> + smp_mb(); /* ensure the latest bh->state is visible */
>> return rc;
>> }
>>
>> I'll keep it running over the weekend.
>
> Can you also check the question I mentioned earlier? That is, if you
> revert to the original code then when the thread hangs, what does the
> call to received_cbw() in get_next_command() return? Or does that
> routine not get called at all because the preceding sleep_thread() is
> the one that doesn't return?
here are the last few calls
> [ 34.122210] ==> bulk_out_complete 484
CBW received
> [ 34.122213] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
set thread_wakeup_needed to 1 and wakeup fsg_main_thread
> [ 34.122255] ==> fsg_main_thread 2587 -> get_next_command() -> 0
get_next_command()
> [ 34.122328] ==> fsg_main_thread 2608 -> send_status -> 0
CSW
> [ 34.122331] ==> get_next_command 2251 -> state != full
now waiting for CBW:
> [ 34.122333] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
thread goes to sleep
> [ 34.122379] ==> bulk_in_complete 461
We send data to host
> [ 34.122381] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122383] ==> get_next_command 2251 -> state != full
still waiting for CBW
> [ 34.122385] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122386] ==> bulk_in_complete 461
send data to host
> [ 34.122388] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122389] ==> get_next_command 2251 -> state != full
still waiting for CBW
> [ 34.122391] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122468] ==> bulk_in_complete 461
> [ 34.122469] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122470] ==> get_next_command 2251 -> state != full
> [ 34.122473] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122473] ==> bulk_in_complete 461
> [ 34.122474] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122476] ==> get_next_command 2251 -> state != full
> [ 34.122479] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122556] ==> bulk_in_complete 461
> [ 34.122557] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122559] ==> get_next_command 2251 -> state != full
> [ 34.122561] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122561] ==> bulk_in_complete 461
> [ 34.122563] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122563] ==> get_next_command 2251 -> state != full
> [ 34.122565] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122647] ==> bulk_in_complete 461
> [ 34.122648] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122650] ==> get_next_command 2251 -> state != full
> [ 34.122653] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122654] ==> bulk_in_complete 461
> [ 34.122655] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122656] ==> get_next_command 2251 -> state != full
> [ 34.122658] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122716] ==> bulk_in_complete 461
> [ 34.122717] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
yada, yada, yada. All the other transfers. If you count all
bulk_in_complete prints, you'll see there are 9 of them. 8x 16kiB +
13-byte CSW.
> [ 34.122719] ==> bulk_out_complete 484
> [ 34.122720] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
another CBW
> [ 34.122721] ==> fsg_main_thread 2587 -> get_next_command() -> 0
> [ 34.122802] ==> fsg_main_thread 2608 -> send_status -> 0
> [ 34.122805] ==> get_next_command 2251 -> state != full
> [ 34.122807] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122850] ==> bulk_in_complete 461
> [ 34.122851] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122855] ==> get_next_command 2251 -> state != full
> [ 34.122856] ==> bulk_in_complete 461
> [ 34.122857] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122858] ==> get_next_command 2251 -> state != full
> [ 34.122860] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.122939] ==> bulk_in_complete 461
> [ 34.122940] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122945] ==> bulk_in_complete 461
> [ 34.122947] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.122988] ==> get_next_command 2251 -> state != full
> [ 34.123008] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.123055] ==> bulk_in_complete 461
> [ 34.123070] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.123073] ==> bulk_in_complete 461
> [ 34.123074] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.123105] ==> get_next_command 2251 -> state != full
> [ 34.123125] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.123167] ==> bulk_in_complete 461
> [ 34.123169] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.123179] ==> bulk_in_complete 461
> [ 34.123181] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
> [ 34.123185] ==> get_next_command 2251 -> state != full
> [ 34.123202] ===> sleep_thread 634: caller fsg_main_thread+0xaa1/0xb00 [usb_f_mass_storage]: going to sleep
> [ 34.123279] ==> bulk_in_complete 461
> [ 34.123280] ==> wakeup_thread 403: caller usb_gadget_giveback_request+0x7/0x10: thread_wakeup_needed = 1
Another block of 128kiB
> [ 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.
> [ 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.
--
balbi
Attachment:
signature.asc
Description: PGP signature
