Page MenuHomeFreeBSD

iwn: DMA fixes, and race condition fix
ClosedPublic

Authored by gonzo on Dec 15 2016, 6:03 AM.

Details

Summary

This review contains several fixes:

  • Fix some dma sync operations to use matching tags
  • Perform BUS_DMASYNC_PREWRITE op before accessing ICT table
Test Plan

I ran following sequence for couple of hours:

  • load if_iwn
  • associate, obtain address and run iperf for 30 seconds
  • stop wlan0
  • unload if_iwn

I didn't see any "iwn0: device timeout" or BAD_COMMAND firmware errors

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gonzo retitled this revision from to iwn: DMA fixes, and race condition fix.
gonzo updated this object.
gonzo edited the test plan for this revision. (Show Details)
gonzo added reviewers: adrian, avos, ivadasz.

weird, how'd we not notice that command race before? How are they trampling on each other? is it some shared buffer, or is it just allocating the same command entry in the TX ring?

sys/dev/iwn/if_iwn.c
5120 ↗(On Diff #22941)

AFAIR, this is wrong; it should be just locked everywhere.

sys/dev/iwn/if_iwn.c
5120 ↗(On Diff #22941)

Yes, I thought so and tried to fix accordingly but it may affect performance in non-subtle way and I have only one iwn(4) card to test all code paths. On the other hand - having dedicated mutex for command queue is very non-intrusive fix that makes sure iwn_cmd accesses command slots sequentially

weird, how'd we not notice that command race before? How are they trampling on each other? is it some shared buffer, or is it just allocating the same command entry in the TX ring?

There are several reports in bugzilla that may be relevant to this issue. If two threads call iwn_cmd in parallel they may end up copying cmd data to the same cmd slot, because slot index is increased at the end of the function.

sys/dev/iwn/if_iwn.c
5120 ↗(On Diff #22941)

And, in theory, can also allow to cleanup locking logic (like in rS280071 )

P.S. Probably, writes to IWN_HBUS_TARG_WRPTR register from other threads should be protected too?

gonzo edited edge metadata.

Leave only DMA fixes

I think locking stuff requires more thought/investigation, so I removed race fix from this review

avos edited edge metadata.
This revision is now accepted and ready to land.Dec 21 2016, 9:22 PM
This revision was automatically updated to reflect the committed changes.