2016-01-31 22:24:43

by Johannes Stezenbach

[permalink] [raw]
Subject: mt76 tx status

Hi Felix,

I noticed in mt76x2_mac_poll_tx_status() you are reading
MT_TX_STAT_FIFO before reading MT_TX_STAT_FIFO_EXT,
however I have a hunch that reading MT_TX_STAT_FIFO clobbers
the corresponding MT_TX_STAT_FIFO_EXT since MT_TX_STAT_FIFO
has the valid bit, and both registers together seem to implement
reading a 64bit FIFO entry. I.e. you need to read
MT_TX_STAT_FIFO_EXT first otherwise you get a mix from
two FIFO entries.

I played a bit with the vendor driver for the mt7610u
and confirmed that multiple reads from MT_TX_STAT_FIFO_EXT
return the same value until MT_TX_STAT_FIFO is read.
(Note the mt7610u vendor driver does not define FIFO_EXT_SUPPORT
but the FIFO seems to exist in hardware. The mt7612u vendor driver
code reads the MT_TX_STAT_FIFO_EXT first.)


Johannes


2016-02-01 12:28:15

by Felix Fietkau

[permalink] [raw]
Subject: Re: mt76 tx status

On 2016-01-31 23:24, Johannes Stezenbach wrote:
> Hi Felix,
>
> I noticed in mt76x2_mac_poll_tx_status() you are reading
> MT_TX_STAT_FIFO before reading MT_TX_STAT_FIFO_EXT,
> however I have a hunch that reading MT_TX_STAT_FIFO clobbers
> the corresponding MT_TX_STAT_FIFO_EXT since MT_TX_STAT_FIFO
> has the valid bit, and both registers together seem to implement
> reading a 64bit FIFO entry. I.e. you need to read
> MT_TX_STAT_FIFO_EXT first otherwise you get a mix from
> two FIFO entries.
>
> I played a bit with the vendor driver for the mt7610u
> and confirmed that multiple reads from MT_TX_STAT_FIFO_EXT
> return the same value until MT_TX_STAT_FIFO is read.
> (Note the mt7610u vendor driver does not define FIFO_EXT_SUPPORT
> but the FIFO seems to exist in hardware. The mt7612u vendor driver
> code reads the MT_TX_STAT_FIFO_EXT first.)
Hi Johannes,

When I wrote that particular part of the code, I still had a lot of
other bugs regarding aggregation and rate probing. Back then, somehow I
got better results reading FIFO before FIFO_EXT.
I just did a few experiments to not only verify that reading FIFO
clobbers FIFO_EXT, but also to ensure that the data from reading
FIFO_EXT first really belongs to the value read from FIFO.

I tested this by checking if the MT_TXWI_PKTID_PROBE flag in the pktid
field (in FIFO_EXT) corresponds to a different rate being selected.

My test results pretty much confirm your findings, so I've committed and
pushed a fix.

Thanks,

- Felix