Re: g_mass_storage not queueing requests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux