Hi, Felipe Balbi <felipe.ba...@linux.intel.com> writes:
[...]
>> 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
okay, I found the one place where changing smp_wmb() to smp_mb() solves
the problem
@@ -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)
Based on what Peter Z said on our other thread, this shouldn't be
necessary for this particular occasion. bulk_out_complete() writes to
bh->state inside a spin_lock() which is implemented with a full barrier
in x86.
So, how come we need another full barrier in wakeup_thread() ?
Is is because the call to wakeup_process() is *also* inside the
spinlocked region and the full barrier is added only at spin_unlock()
time? (just wondering here)
cheers
--
balbi
signature.asc
Description: PGP signature
