Hi Alan,
Felipe Balbi <felipe.balbi@xxxxxxxxxxxxxxx> writes:
> 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.
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?
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
cheers
--
balbi
Attachment:
signature.asc
Description: PGP signature
