lkml.org 
[lkml]   [2019]   [Jun]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] rt2x00: fix rx queue hang
Hello

On Wed, Jun 26, 2019 at 03:28:00PM +0200, Soeren Moch wrote:
> Hi Stanislaw,
>
> the good news is: your patch below also solves the issue for me. But
> removing the ENTRY_DATA_STATUS_PENDING check in
> rt2x00usb_kick_rx_entry() alone does not help, while removing this check
> in rt2x00usb_work_rxdone() alone does the trick.
>
> So the real race seems to be that the flags set in the completion
> handler are not yet visible on the cpu core running the workqueue. And
> because the worker is not rescheduled when aborted, the entry can just
> wait forever.
> Do you think this could make sense?

Yes.

> > I'm somewhat reluctant to change the order, because TX processing
> > might relay on it (we first mark we wait for TX status and
> > then mark entry is no longer owned by hardware).
> OK, maybe it's just good luck that changing the order solves the rx
> problem. Or can memory barriers associated with the spinlock in
> rt2x00lib_dmadone() be responsible for that?
> (I'm testing on a armv7 system, Cortex-A9 quadcore.)

I'm not sure, rt2x00queue_index_inc() also disable/enable interrupts,
so maybe that make race not reproducible.

> While looking at it, why we double-clear ENTRY_OWNER_DEVICE_DATA in
> rt2x00usb_interrupt_rxdone() directly and in rt2x00lib_dmadone() again,

rt2x00lib_dmadone() is called also on other palaces (error paths)
when we have to clear flags.

> while not doing the same for tx?

If I remember correctly we have some races on rx (not happened on tx)
that was solved by using test_and_clear_bit(ENTRY_OWNER_DEVICE_DATA).

> Would it make more sense to possibly
> set ENTRY_DATA_IO_FAILED before clearing ENTRY_OWNER_DEVICE_DATA in
> rt2x00usb_interrupt_rxdone() as for tx?

I don't think so, ENTRY_DATA_IO_FAILED should be only set on error
case.

> > However on RX
> > side ENTRY_DATA_STATUS_PENDING bit make no sense as we do not
> > wait for status. We should remove ENTRY_DATA_STATUS_PENDING on
> > RX side and perhaps this also will solve issue you observe.
> I agree that removing the unnecessary checks is a good idea in any case.
> > Could you please check below patch, if it fixes the problem as well?
> At least I could not trigger the problem within transferring 10GB of
> data. But maybe the probability for triggering this bug is just lower
> because ENTRY_OWNER_DEVICE_DATA is cleared some time before
> ENTRY_DATA_STATUS_PENDING is set?

Not sure. Anyway, could you post patch removing not needed checks
with proper description/changelog ?

Stanislaw

\
 
 \ /
  Last update: 2019-06-29 10:51    [W:0.069 / U:0.500 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site