Hi,
Alan Stern <st...@rowland.harvard.edu> writes:
> On Sat, 3 Sep 2016, Felipe Balbi wrote:
>
>> Hi,
>>
>> Alan Stern <st...@rowland.harvard.edu> writes:
>> > On Fri, 2 Sep 2016, Felipe Balbi wrote:
>> >
>> >>
>> >> Hi,
>> >>
>> >> Alan Stern <st...@rowland.harvard.edu> 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
signature.asc
Description: PGP signature
