iwn: DMA fixes, and race condition fix
ClosedPublic
Actions

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

Details

Reviewers

ivadasz
avos
adrian

Commits

rS310375: [iwn] Several fixes for DMA part of iwn(4)

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

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 6255
Build 6498: arc lint + arc unit

Event Timeline

gonzo updated this revision to Diff 22941.Dec 15 2016, 6:03 AM

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.

Herald added a subscriber: imp. · View Herald TranscriptDec 15 2016, 6:03 AM

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?

avos added inline comments.Dec 15 2016, 7:12 AM

sys/dev/iwn/if_iwn.c
5120	AFAIR, this is wrong; it should be just locked everywhere.

gonzo added inline comments.Dec 15 2016, 5:11 PM

sys/dev/iwn/if_iwn.c
5120	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

In D8804#182208, @adrian wrote:

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.

avos added inline comments.Dec 15 2016, 9:29 PM

sys/dev/iwn/if_iwn.c
5120	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?