| Message ID | 9d50bf4d74ec52e141dd112768570e15a481ad2e.1525096206.git.lorenzo.bianconi@redhat.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Kalle Valo |
| Headers | show |
On Mon, Apr 30, 2018 at 04:12:21PM +0200, Lorenzo Bianconi wrote: > Add mt76x2_mac_load_tx_status routine since tx stats register map is > shared between usb and pci based devices but usb devices do not have a > tx stat irq line as pcie ones and it is necessary to load tx statistics > using a workqueue > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > drivers/net/wireless/mediatek/mt76/mt76x2_mac.c | 43 +++++++++++++++---------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c b/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c > index d18315652583..0ddc84525ffa 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c > +++ b/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c > @@ -515,6 +515,28 @@ mt76x2_send_tx_status(struct mt76x2_dev *dev, struct mt76x2_tx_status *stat, > rcu_read_unlock(); > } > > +static struct mt76x2_tx_status > +mt76x2_mac_load_tx_status(struct mt76x2_dev *dev) > +{ > + struct mt76x2_tx_status stat = {}; > + u32 stat1, stat2; > + > + stat2 = mt76_rr(dev, MT_TX_STAT_FIFO_EXT); > + stat1 = mt76_rr(dev, MT_TX_STAT_FIFO); > + > + stat.valid = !!(stat1 & MT_TX_STAT_FIFO_VALID); > + stat.success = !!(stat1 & MT_TX_STAT_FIFO_SUCCESS); > + stat.aggr = !!(stat1 & MT_TX_STAT_FIFO_AGGR); > + stat.ack_req = !!(stat1 & MT_TX_STAT_FIFO_ACKREQ); > + stat.wcid = FIELD_GET(MT_TX_STAT_FIFO_WCID, stat1); > + stat.rate = FIELD_GET(MT_TX_STAT_FIFO_RATE, stat1); > + > + stat.retry = FIELD_GET(MT_TX_STAT_FIFO_EXT_RETRY, stat2); > + stat.pktid = FIELD_GET(MT_TX_STAT_FIFO_EXT_PKTID, stat2); > + > + return stat; > +} > + Just two minor points. We do not need fill structure if stat is not valid. Also returning mt76x2_tx_status structure might require full copy of it. Returning boolean indicator of validity and pass pointer to struct mt76x2_tx_status to the subroutine would be more efficient. Regards Stanislaw
> On Mon, Apr 30, 2018 at 04:12:21PM +0200, Lorenzo Bianconi wrote: >> Add mt76x2_mac_load_tx_status routine since tx stats register map is >> shared between usb and pci based devices but usb devices do not have a >> tx stat irq line as pcie ones and it is necessary to load tx statistics >> using a workqueue >> >> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> >> --- >> drivers/net/wireless/mediatek/mt76/mt76x2_mac.c | 43 +++++++++++++++---------- >> 1 file changed, 26 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c b/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c >> index d18315652583..0ddc84525ffa 100644 >> --- a/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c >> +++ b/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c >> @@ -515,6 +515,28 @@ mt76x2_send_tx_status(struct mt76x2_dev *dev, struct mt76x2_tx_status *stat, >> rcu_read_unlock(); >> } >> >> +static struct mt76x2_tx_status >> +mt76x2_mac_load_tx_status(struct mt76x2_dev *dev) >> +{ >> + struct mt76x2_tx_status stat = {}; >> + u32 stat1, stat2; >> + >> + stat2 = mt76_rr(dev, MT_TX_STAT_FIFO_EXT); >> + stat1 = mt76_rr(dev, MT_TX_STAT_FIFO); >> + >> + stat.valid = !!(stat1 & MT_TX_STAT_FIFO_VALID); >> + stat.success = !!(stat1 & MT_TX_STAT_FIFO_SUCCESS); >> + stat.aggr = !!(stat1 & MT_TX_STAT_FIFO_AGGR); >> + stat.ack_req = !!(stat1 & MT_TX_STAT_FIFO_ACKREQ); >> + stat.wcid = FIELD_GET(MT_TX_STAT_FIFO_WCID, stat1); >> + stat.rate = FIELD_GET(MT_TX_STAT_FIFO_RATE, stat1); >> + >> + stat.retry = FIELD_GET(MT_TX_STAT_FIFO_EXT_RETRY, stat2); >> + stat.pktid = FIELD_GET(MT_TX_STAT_FIFO_EXT_PKTID, stat2); >> + >> + return stat; >> +} >> + > > Just two minor points. We do not need fill structure if stat is > not valid. Also returning mt76x2_tx_status structure might require > full copy of it. Returning boolean indicator of validity and pass > pointer to struct mt76x2_tx_status to the subroutine would be more > efficient. > > Regards > Stanislaw I agree, thx for the review. I will address your suggestions in v1. Regards, Lorenzo
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c b/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c index d18315652583..0ddc84525ffa 100644 --- a/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c +++ b/drivers/net/wireless/mediatek/mt76/mt76x2_mac.c @@ -515,6 +515,28 @@ mt76x2_send_tx_status(struct mt76x2_dev *dev, struct mt76x2_tx_status *stat, rcu_read_unlock(); } +static struct mt76x2_tx_status +mt76x2_mac_load_tx_status(struct mt76x2_dev *dev) +{ + struct mt76x2_tx_status stat = {}; + u32 stat1, stat2; + + stat2 = mt76_rr(dev, MT_TX_STAT_FIFO_EXT); + stat1 = mt76_rr(dev, MT_TX_STAT_FIFO); + + stat.valid = !!(stat1 & MT_TX_STAT_FIFO_VALID); + stat.success = !!(stat1 & MT_TX_STAT_FIFO_SUCCESS); + stat.aggr = !!(stat1 & MT_TX_STAT_FIFO_AGGR); + stat.ack_req = !!(stat1 & MT_TX_STAT_FIFO_ACKREQ); + stat.wcid = FIELD_GET(MT_TX_STAT_FIFO_WCID, stat1); + stat.rate = FIELD_GET(MT_TX_STAT_FIFO_RATE, stat1); + + stat.retry = FIELD_GET(MT_TX_STAT_FIFO_EXT_RETRY, stat2); + stat.pktid = FIELD_GET(MT_TX_STAT_FIFO_EXT_PKTID, stat2); + + return stat; +} + void mt76x2_mac_poll_tx_status(struct mt76x2_dev *dev, bool irq) { struct mt76x2_tx_status stat = {}; @@ -527,26 +549,13 @@ void mt76x2_mac_poll_tx_status(struct mt76x2_dev *dev, bool irq) trace_mac_txstat_poll(dev); while (!irq || !kfifo_is_full(&dev->txstatus_fifo)) { - u32 stat1, stat2; - spin_lock_irqsave(&dev->irq_lock, flags); - stat2 = mt76_rr(dev, MT_TX_STAT_FIFO_EXT); - stat1 = mt76_rr(dev, MT_TX_STAT_FIFO); - if (!(stat1 & MT_TX_STAT_FIFO_VALID)) { - spin_unlock_irqrestore(&dev->irq_lock, flags); - break; - } - + stat = mt76x2_mac_load_tx_status(dev); spin_unlock_irqrestore(&dev->irq_lock, flags); - stat.valid = 1; - stat.success = !!(stat1 & MT_TX_STAT_FIFO_SUCCESS); - stat.aggr = !!(stat1 & MT_TX_STAT_FIFO_AGGR); - stat.ack_req = !!(stat1 & MT_TX_STAT_FIFO_ACKREQ); - stat.wcid = FIELD_GET(MT_TX_STAT_FIFO_WCID, stat1); - stat.rate = FIELD_GET(MT_TX_STAT_FIFO_RATE, stat1); - stat.retry = FIELD_GET(MT_TX_STAT_FIFO_EXT_RETRY, stat2); - stat.pktid = FIELD_GET(MT_TX_STAT_FIFO_EXT_PKTID, stat2); + if (!stat.valid) + break; + trace_mac_txstat_fetch(dev, &stat); if (!irq) {
Add mt76x2_mac_load_tx_status routine since tx stats register map is shared between usb and pci based devices but usb devices do not have a tx stat irq line as pcie ones and it is necessary to load tx statistics using a workqueue Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- drivers/net/wireless/mediatek/mt76/mt76x2_mac.c | 43 +++++++++++++++---------- 1 file changed, 26 insertions(+), 17 deletions(-)