* Re: [PATCH v2] mt76: usb: reduce locking in mt76u_tx_tasklet
From: Felix Fietkau @ 2019-04-25 9:59 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-wireless, lorenzo.bianconi, sgruszka
In-Reply-To: <7e6146236dac0d616519e2a6e4b32fac9a1cae2f.1553541103.git.lorenzo@kernel.org>
On 2019-03-25 20:18, Lorenzo Bianconi wrote:
> Similar to pci counterpart, reduce locking in mt76u_tx_tasklet since
> q->head is managed just in mt76u_tx_tasklet and q->queued is updated
> holding q->lock
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Applied, thanks.
- Felix
^ permalink raw reply
* Re: [PATCH v2] mt76: move mac_work in mt76_dev
From: Felix Fietkau @ 2019-04-25 9:59 UTC (permalink / raw)
To: Lorenzo Bianconi; +Cc: linux-wireless, lorenzo.bianconi, sgruszka
In-Reply-To: <d4eb4aa6f8f8b003442b4f04b0f659e612486263.1552903068.git.lorenzo@kernel.org>
On 2019-03-18 11:21, Lorenzo Bianconi wrote:
> Move mac_work delayed work in mt76_dev data structure since
> it is used by all drivers and it will be reused adding mac work to
> mt7615
>
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
Applied, thanks.
- Felix
^ permalink raw reply
* Re: [PATCH v2 wireless-drivers-next] rtlwifi: rtl8723ae: Fix missing break in switch statement
From: Kalle Valo @ 2019-04-25 9:37 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Ping-Ke Shih, David S. Miller, Larry Finger, linux-wireless,
netdev, linux-kernel, Kees Cook
In-Reply-To: <6056eaca-5120-ec0f-6e72-2e1bfbde52a9@embeddedor.com>
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> Hi all,
>
> Friendly ping:
>
> Who can take this?
It's in my queue.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Herbert Xu @ 2019-04-25 8:35 UTC (permalink / raw)
To: Johannes Berg
Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau,
linux-wireless, Eric Dumazet, netdev
In-Reply-To: <99da695257304d32b65e2db8b7ada06759087c90.camel@sipsolutions.net>
On Tue, Apr 23, 2019 at 02:41:33PM +0200, Johannes Berg wrote:
>
> I guess there might be value in returning NET_XMIT_CN anyway, but I
> think you're right in that we can never return anything but
> NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
> just older ones.
>
> Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?
Pretty sure codel does return NET_XMIT_CN.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Johannes Berg @ 2019-04-25 8:49 UTC (permalink / raw)
To: Herbert Xu
Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau,
linux-wireless, Eric Dumazet, netdev
In-Reply-To: <20190425084428.poni4y7mqox5up5t@gondor.apana.org.au>
On Thu, 2019-04-25 at 16:44 +0800, Herbert Xu wrote:
> On Thu, Apr 25, 2019 at 10:39:52AM +0200, Johannes Berg wrote:
> >
> > Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
> > NET_XMIT_CN. This will not trigger the code you mentioned before though.
>
> You are right that it does not. However, the fact that this
> congestion indication is lost is a bug rather than a feature.
You can argue that way, I guess.
However, *any* queue management algorithm that doesn't *solely* employ
tail drops will necessarily behave this way.
I would argue that you get better queue management if you don't solely
rely on tail drops, so I'd rather pick better queue management than this
(relatively obscure) congestion notification. The more commonly relevant
socket buffer size will work for both.
johannes
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Herbert Xu @ 2019-04-25 8:44 UTC (permalink / raw)
To: Johannes Berg
Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau,
linux-wireless, Eric Dumazet, netdev
In-Reply-To: <0190c32f3d861d9d7a783090653645c5ae3a7fa3.camel@sipsolutions.net>
On Thu, Apr 25, 2019 at 10:39:52AM +0200, Johannes Berg wrote:
>
> Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
> NET_XMIT_CN. This will not trigger the code you mentioned before though.
You are right that it does not. However, the fact that this
congestion indication is lost is a bug rather than a feature.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 5/5] mac80211: set NETIF_F_LLTX when using intermediate tx queues
From: Johannes Berg @ 2019-04-25 8:39 UTC (permalink / raw)
To: Herbert Xu
Cc: Toke Høiland-Jørgensen, Arend Van Spriel, Felix Fietkau,
linux-wireless, Eric Dumazet, netdev
In-Reply-To: <20190425083558.p3kqodaawr2jcfhr@gondor.apana.org.au>
On Thu, 2019-04-25 at 16:35 +0800, Herbert Xu wrote:
> On Tue, Apr 23, 2019 at 02:41:33PM +0200, Johannes Berg wrote:
> >
> > I guess there might be value in returning NET_XMIT_CN anyway, but I
> > think you're right in that we can never return anything but
> > NET_XMIT_SUCCESS or NET_XMIT_CN since we never drop this new packet,
> > just older ones.
> >
> > Which, btw, is exactly the same with net/sched/sch_fq_codel.c, AFAICT?
>
> Pretty sure codel does return NET_XMIT_CN.
Yes, that's what I meant, it can only ever return NET_XMIT_SUCCESS or
NET_XMIT_CN. This will not trigger the code you mentioned before though.
johannes
^ permalink raw reply
* [PATCH v2 2/5] mt76: reduce locking in mt76_dma_tx_cleanup
From: Felix Fietkau @ 2019-04-25 8:38 UTC (permalink / raw)
To: linux-wireless
In-Reply-To: <20190425083844.14186-1-nbd@nbd.name>
q->tail can be safely updated without locking, because there is no
concurrent access. If called from outside of the tasklet (for flushing),
the tasklet is always disabled.
q->queued can be safely read without locking, as long as the decrement
happens within the locked section.
This patch allows cleaning up tx packets outside of the section that holds
the queue lock for improved performance
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
drivers/net/wireless/mediatek/mt76/dma.c | 26 ++++++++++++++++--------
1 file changed, 17 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
index 7145b75b6438..e4a5b34915bf 100644
--- a/drivers/net/wireless/mediatek/mt76/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/dma.c
@@ -149,31 +149,29 @@ mt76_dma_tx_cleanup(struct mt76_dev *dev, enum mt76_txq_id qid, bool flush)
struct mt76_sw_queue *sq = &dev->q_tx[qid];
struct mt76_queue *q = sq->q;
struct mt76_queue_entry entry;
+ unsigned int n_swq_queued[4] = {};
+ unsigned int n_queued = 0;
bool wake = false;
- int last;
+ int i, last;
if (!q)
return;
- spin_lock_bh(&q->lock);
if (flush)
last = -1;
else
last = readl(&q->regs->dma_idx);
- while (q->queued && q->tail != last) {
+ while ((q->queued > n_queued) && q->tail != last) {
mt76_dma_tx_cleanup_idx(dev, q, q->tail, &entry);
if (entry.schedule)
- dev->q_tx[entry.qid].swq_queued--;
+ n_swq_queued[entry.qid]++;
q->tail = (q->tail + 1) % q->ndesc;
- q->queued--;
+ n_queued++;
- if (entry.skb) {
- spin_unlock_bh(&q->lock);
+ if (entry.skb)
dev->drv->tx_complete_skb(dev, qid, &entry);
- spin_lock_bh(&q->lock);
- }
if (entry.txwi) {
mt76_put_txwi(dev, entry.txwi);
@@ -184,6 +182,16 @@ mt76_dma_tx_cleanup(struct mt76_dev *dev, enum mt76_txq_id qid, bool flush)
last = readl(&q->regs->dma_idx);
}
+ spin_lock_bh(&q->lock);
+
+ q->queued -= n_queued;
+ for (i = 0; i < ARRAY_SIZE(n_swq_queued); i++) {
+ if (!n_swq_queued[i])
+ continue;
+
+ dev->q_tx[i].swq_queued -= n_swq_queued[i];
+ }
+
if (flush)
mt76_dma_sync_idx(dev, q);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 5/5] mt76: only schedule txqs from the tx tasklet
From: Felix Fietkau @ 2019-04-25 8:38 UTC (permalink / raw)
To: linux-wireless
In-Reply-To: <20190425083844.14186-1-nbd@nbd.name>
Reduces lock contention from the tx path and improves performance
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
drivers/net/wireless/mediatek/mt76/dma.c | 2 --
drivers/net/wireless/mediatek/mt76/mt7603/dma.c | 2 ++
drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c | 3 +++
drivers/net/wireless/mediatek/mt76/tx.c | 2 +-
4 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
index e4a5b34915bf..5c592566acbb 100644
--- a/drivers/net/wireless/mediatek/mt76/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/dma.c
@@ -205,8 +205,6 @@ mt76_dma_tx_cleanup(struct mt76_dev *dev, enum mt76_txq_id qid, bool flush)
spin_unlock_bh(&q->lock);
- if (!flush)
- mt76_txq_schedule(dev, qid);
if (wake)
ieee80211_wake_queue(dev->hw, qid);
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/dma.c b/drivers/net/wireless/mediatek/mt76/mt7603/dma.c
index f7e3566c96fd..27e2d9f90553 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/dma.c
@@ -145,6 +145,8 @@ mt7603_tx_tasklet(unsigned long data)
for (i = MT_TXQ_MCU; i >= 0; i--)
mt76_queue_tx_cleanup(dev, i, false);
+ mt76_txq_schedule_all(&dev->mt76);
+
mt7603_irq_enable(dev, MT_INT_TX_DONE_ALL);
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
index 958b2a3e4878..af308c0ef395 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
@@ -162,6 +162,9 @@ static void mt76x02_tx_tasklet(unsigned long data)
mt76_queue_tx_cleanup(dev, i, false);
mt76x02_mac_poll_tx_status(dev, false);
+
+ mt76_txq_schedule_all(&dev->mt76);
+
mt76x02_irq_enable(dev, MT_INT_TX_DONE_ALL);
}
diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index dd0c583ab5b1..638b83903c56 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -595,7 +595,7 @@ void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *txq)
if (!test_bit(MT76_STATE_RUNNING, &dev->state))
return;
- mt76_txq_schedule(dev, txq->ac);
+ tasklet_schedule(&dev->tx_tasklet);
}
EXPORT_SYMBOL_GPL(mt76_wake_tx_queue);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 4/5] mt76: move tx tasklet to struct mt76_dev
From: Felix Fietkau @ 2019-04-25 8:38 UTC (permalink / raw)
To: linux-wireless
In-Reply-To: <20190425083844.14186-1-nbd@nbd.name>
Allows it to be scheduled from core code
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 3 ++-
drivers/net/wireless/mediatek/mt76/mt7603/core.c | 2 +-
drivers/net/wireless/mediatek/mt76/mt7603/dma.c | 4 ++--
drivers/net/wireless/mediatek/mt76/mt7603/mac.c | 6 +++---
.../net/wireless/mediatek/mt76/mt7603/mt7603.h | 1 -
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c | 2 +-
drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c | 15 ++++++++-------
drivers/net/wireless/mediatek/mt76/mt76x2/usb.c | 2 +-
drivers/net/wireless/mediatek/mt76/usb.c | 6 +++---
9 files changed, 21 insertions(+), 20 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 176faaac8748..c35305ad2d12 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -386,7 +386,6 @@ struct mt76_usb {
u8 data[32];
struct tasklet_struct rx_tasklet;
- struct tasklet_struct tx_tasklet;
struct delayed_work stat_work;
u8 out_ep[__MT_EP_OUT_MAX];
@@ -448,6 +447,8 @@ struct mt76_dev {
const struct mt76_queue_ops *queue_ops;
int tx_dma_idx[4];
+ struct tasklet_struct tx_tasklet;
+
wait_queue_head_t tx_wait;
struct sk_buff_head status_list;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/core.c b/drivers/net/wireless/mediatek/mt76/mt7603/core.c
index 4668c573f74a..0d06ff67ce44 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/core.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/core.c
@@ -35,7 +35,7 @@ irqreturn_t mt7603_irq_handler(int irq, void *dev_instance)
if (intr & MT_INT_TX_DONE_ALL) {
mt7603_irq_disable(dev, MT_INT_TX_DONE_ALL);
- tasklet_schedule(&dev->tx_tasklet);
+ tasklet_schedule(&dev->mt76.tx_tasklet);
}
if (intr & MT_INT_RX_DONE(0)) {
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/dma.c b/drivers/net/wireless/mediatek/mt76/mt7603/dma.c
index 37cedfcedce4..f7e3566c96fd 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/dma.c
@@ -164,7 +164,7 @@ int mt7603_dma_init(struct mt7603_dev *dev)
init_waitqueue_head(&dev->mt76.mmio.mcu.wait);
skb_queue_head_init(&dev->mt76.mmio.mcu.res_q);
- tasklet_init(&dev->tx_tasklet, mt7603_tx_tasklet, (unsigned long)dev);
+ tasklet_init(&dev->mt76.tx_tasklet, mt7603_tx_tasklet, (unsigned long)dev);
mt76_clear(dev, MT_WPDMA_GLO_CFG,
MT_WPDMA_GLO_CFG_TX_DMA_EN |
@@ -224,6 +224,6 @@ void mt7603_dma_cleanup(struct mt7603_dev *dev)
MT_WPDMA_GLO_CFG_RX_DMA_EN |
MT_WPDMA_GLO_CFG_TX_WRITEBACK_DONE);
- tasklet_kill(&dev->tx_tasklet);
+ tasklet_kill(&dev->mt76.tx_tasklet);
mt76_dma_cleanup(&dev->mt76);
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
index 47f5005ea48a..52956bf8a979 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
@@ -1277,7 +1277,7 @@ static void mt7603_mac_watchdog_reset(struct mt7603_dev *dev)
/* lock/unlock all queues to ensure that no tx is pending */
mt76_txq_schedule_all(&dev->mt76);
- tasklet_disable(&dev->tx_tasklet);
+ tasklet_disable(&dev->mt76.tx_tasklet);
tasklet_disable(&dev->pre_tbtt_tasklet);
napi_disable(&dev->mt76.napi[0]);
napi_disable(&dev->mt76.napi[1]);
@@ -1324,8 +1324,8 @@ static void mt7603_mac_watchdog_reset(struct mt7603_dev *dev)
clear_bit(MT76_RESET, &dev->mt76.state);
mutex_unlock(&dev->mt76.mutex);
- tasklet_enable(&dev->tx_tasklet);
- tasklet_schedule(&dev->tx_tasklet);
+ tasklet_enable(&dev->mt76.tx_tasklet);
+ tasklet_schedule(&dev->mt76.tx_tasklet);
tasklet_enable(&dev->pre_tbtt_tasklet);
mt7603_beacon_set_timer(dev, -1, beacon_int);
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h b/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h
index c355e36966dd..36875ff4c3bc 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mt7603.h
@@ -145,7 +145,6 @@ struct mt7603_dev {
unsigned int reset_cause[__RESET_CAUSE_MAX];
struct delayed_work mac_work;
- struct tasklet_struct tx_tasklet;
struct tasklet_struct pre_tbtt_tasklet;
};
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index 1ef00e971cfa..db2c3c6c2c66 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -332,7 +332,7 @@ static int __maybe_unused mt76x0_resume(struct usb_interface *usb_intf)
goto err;
tasklet_enable(&usb->rx_tasklet);
- tasklet_enable(&usb->tx_tasklet);
+ tasklet_enable(&dev->mt76.tx_tasklet);
ret = mt76x0u_init_hardware(dev);
if (ret)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
index 705c0939d10b..958b2a3e4878 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
@@ -180,7 +180,8 @@ int mt76x02_dma_init(struct mt76x02_dev *dev)
if (!status_fifo)
return -ENOMEM;
- tasklet_init(&dev->tx_tasklet, mt76x02_tx_tasklet, (unsigned long) dev);
+ tasklet_init(&dev->mt76.tx_tasklet, mt76x02_tx_tasklet,
+ (unsigned long) dev);
tasklet_init(&dev->pre_tbtt_tasklet, mt76x02_pre_tbtt_tasklet,
(unsigned long)dev);
@@ -250,7 +251,7 @@ irqreturn_t mt76x02_irq_handler(int irq, void *dev_instance)
if (intr & MT_INT_TX_DONE_ALL) {
mt76x02_irq_disable(dev, MT_INT_TX_DONE_ALL);
- tasklet_schedule(&dev->tx_tasklet);
+ tasklet_schedule(&dev->mt76.tx_tasklet);
}
if (intr & MT_INT_RX_DONE(0)) {
@@ -276,7 +277,7 @@ irqreturn_t mt76x02_irq_handler(int irq, void *dev_instance)
if (intr & MT_INT_TX_STAT) {
mt76x02_mac_poll_tx_status(dev, true);
- tasklet_schedule(&dev->tx_tasklet);
+ tasklet_schedule(&dev->mt76.tx_tasklet);
}
if (intr & MT_INT_GPTIMER) {
@@ -306,7 +307,7 @@ static void mt76x02_dma_enable(struct mt76x02_dev *dev)
void mt76x02_dma_cleanup(struct mt76x02_dev *dev)
{
- tasklet_kill(&dev->tx_tasklet);
+ tasklet_kill(&dev->mt76.tx_tasklet);
mt76_dma_cleanup(&dev->mt76);
}
EXPORT_SYMBOL_GPL(mt76x02_dma_cleanup);
@@ -425,7 +426,7 @@ static void mt76x02_watchdog_reset(struct mt76x02_dev *dev)
set_bit(MT76_RESET, &dev->mt76.state);
tasklet_disable(&dev->pre_tbtt_tasklet);
- tasklet_disable(&dev->tx_tasklet);
+ tasklet_disable(&dev->mt76.tx_tasklet);
for (i = 0; i < ARRAY_SIZE(dev->mt76.napi); i++)
napi_disable(&dev->mt76.napi[i]);
@@ -478,8 +479,8 @@ static void mt76x02_watchdog_reset(struct mt76x02_dev *dev)
clear_bit(MT76_RESET, &dev->mt76.state);
- tasklet_enable(&dev->tx_tasklet);
- tasklet_schedule(&dev->tx_tasklet);
+ tasklet_enable(&dev->mt76.tx_tasklet);
+ tasklet_schedule(&dev->mt76.tx_tasklet);
tasklet_enable(&dev->pre_tbtt_tasklet);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
index d08bb964966b..d1bddd5931bd 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb.c
@@ -124,7 +124,7 @@ static int __maybe_unused mt76x2u_resume(struct usb_interface *intf)
goto err;
tasklet_enable(&usb->rx_tasklet);
- tasklet_enable(&usb->tx_tasklet);
+ tasklet_enable(&dev->mt76.tx_tasklet);
err = mt76x2u_init_hardware(dev);
if (err < 0)
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index d93dadce95ab..15aeda0582e7 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -702,7 +702,7 @@ static void mt76u_complete_tx(struct urb *urb)
dev_err(dev->dev, "tx urb failed: %d\n", urb->status);
e->done = true;
- tasklet_schedule(&dev->usb.tx_tasklet);
+ tasklet_schedule(&dev->tx_tasklet);
}
static int
@@ -843,7 +843,7 @@ static void mt76u_stop_tx(struct mt76_dev *dev)
void mt76u_stop_queues(struct mt76_dev *dev)
{
tasklet_disable(&dev->usb.rx_tasklet);
- tasklet_disable(&dev->usb.tx_tasklet);
+ tasklet_disable(&dev->tx_tasklet);
mt76u_stop_rx(dev);
mt76u_stop_tx(dev);
@@ -898,7 +898,7 @@ int mt76u_init(struct mt76_dev *dev,
struct mt76_usb *usb = &dev->usb;
tasklet_init(&usb->rx_tasklet, mt76u_rx_tasklet, (unsigned long)dev);
- tasklet_init(&usb->tx_tasklet, mt76u_tx_tasklet, (unsigned long)dev);
+ tasklet_init(&dev->tx_tasklet, mt76u_tx_tasklet, (unsigned long)dev);
INIT_DELAYED_WORK(&usb->stat_work, mt76u_tx_status_data);
skb_queue_head_init(&dev->rx_skb[MT_RXQ_MAIN]);
--
2.17.0
^ permalink raw reply related
* [PATCH v2 1/5] mt76: use mac80211 txq scheduling
From: Felix Fietkau @ 2019-04-25 8:38 UTC (permalink / raw)
To: linux-wireless
Performance improvement and preparation for adding airtime fairness support
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
drivers/net/wireless/mediatek/mt76/dma.c | 6 +-
drivers/net/wireless/mediatek/mt76/mac80211.c | 15 +++
drivers/net/wireless/mediatek/mt76/mt76.h | 3 +-
drivers/net/wireless/mediatek/mt76/tx.c | 101 ++++++++----------
drivers/net/wireless/mediatek/mt76/usb.c | 3 +-
5 files changed, 67 insertions(+), 61 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/dma.c b/drivers/net/wireless/mediatek/mt76/dma.c
index cdeca22bf3a7..7145b75b6438 100644
--- a/drivers/net/wireless/mediatek/mt76/dma.c
+++ b/drivers/net/wireless/mediatek/mt76/dma.c
@@ -184,9 +184,7 @@ mt76_dma_tx_cleanup(struct mt76_dev *dev, enum mt76_txq_id qid, bool flush)
last = readl(&q->regs->dma_idx);
}
- if (!flush)
- mt76_txq_schedule(dev, sq);
- else
+ if (flush)
mt76_dma_sync_idx(dev, q);
wake = wake && q->stopped &&
@@ -199,6 +197,8 @@ mt76_dma_tx_cleanup(struct mt76_dev *dev, enum mt76_txq_id qid, bool flush)
spin_unlock_bh(&q->lock);
+ if (!flush)
+ mt76_txq_schedule(dev, qid);
if (wake)
ieee80211_wake_queue(dev->hw, qid);
}
diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c b/drivers/net/wireless/mediatek/mt76/mac80211.c
index 4b63d061c2a0..60b86ca00b3d 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -568,6 +568,7 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
struct ieee80211_sta *sta;
struct mt76_wcid *wcid = status->wcid;
bool ps;
+ int i;
if (ieee80211_is_pspoll(hdr->frame_control) && !wcid) {
sta = ieee80211_find_sta_by_ifaddr(dev->hw, hdr->addr2, NULL);
@@ -614,6 +615,20 @@ mt76_check_sta(struct mt76_dev *dev, struct sk_buff *skb)
dev->drv->sta_ps(dev, sta, ps);
ieee80211_sta_ps_transition(sta, ps);
+
+ if (ps)
+ return;
+
+ for (i = 0; i < ARRAY_SIZE(sta->txq); i++) {
+ struct mt76_txq *mtxq;
+
+ if (!sta->txq[i])
+ continue;
+
+ mtxq = (struct mt76_txq *) sta->txq[i]->drv_priv;
+ if (!skb_queue_empty(&mtxq->retry_q))
+ ieee80211_schedule_txq(dev->hw, sta->txq[i]);
+ }
}
void mt76_rx_complete(struct mt76_dev *dev, struct sk_buff_head *frames,
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index f0d34901c825..e68834ee8393 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -216,7 +216,6 @@ struct mt76_wcid {
};
struct mt76_txq {
- struct list_head list;
struct mt76_sw_queue *swq;
struct mt76_wcid *wcid;
@@ -676,7 +675,7 @@ void mt76_txq_remove(struct mt76_dev *dev, struct ieee80211_txq *txq);
void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *txq);
void mt76_stop_tx_queues(struct mt76_dev *dev, struct ieee80211_sta *sta,
bool send_bar);
-void mt76_txq_schedule(struct mt76_dev *dev, struct mt76_sw_queue *sq);
+void mt76_txq_schedule(struct mt76_dev *dev, enum mt76_txq_id qid);
void mt76_txq_schedule_all(struct mt76_dev *dev);
void mt76_release_buffered_frames(struct ieee80211_hw *hw,
struct ieee80211_sta *sta,
diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index 2c82db0b5834..48f588726b3f 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -479,23 +479,37 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
}
static int
-mt76_txq_schedule_list(struct mt76_dev *dev, struct mt76_sw_queue *sq)
+mt76_txq_schedule_list(struct mt76_dev *dev, enum mt76_txq_id qid)
{
+ struct mt76_sw_queue *sq = &dev->q_tx[qid];
struct mt76_queue *hwq = sq->q;
- struct mt76_txq *mtxq, *mtxq_last;
- int len = 0;
+ struct ieee80211_txq *txq;
+ struct mt76_txq *mtxq;
+ struct mt76_wcid *wcid;
+ int ret = 0;
-restart:
- mtxq_last = list_last_entry(&sq->swq, struct mt76_txq, list);
- while (!list_empty(&sq->swq)) {
+ spin_lock_bh(&hwq->lock);
+ while (1) {
bool empty = false;
- int cur;
+
+ if (sq->swq_queued >= 4)
+ break;
if (test_bit(MT76_OFFCHANNEL, &dev->state) ||
- test_bit(MT76_RESET, &dev->state))
- return -EBUSY;
+ test_bit(MT76_RESET, &dev->state)) {
+ ret = -EBUSY;
+ break;
+ }
+
+ txq = ieee80211_next_txq(dev->hw, qid);
+ if (!txq)
+ break;
+
+ mtxq = (struct mt76_txq *)txq->drv_priv;
+ wcid = mtxq->wcid;
+ if (wcid && test_bit(MT_WCID_FLAG_PS, &wcid->flags))
+ continue;
- mtxq = list_first_entry(&sq->swq, struct mt76_txq, list);
if (mtxq->send_bar && mtxq->aggr) {
struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
struct ieee80211_sta *sta = txq->sta;
@@ -507,38 +521,37 @@ mt76_txq_schedule_list(struct mt76_dev *dev, struct mt76_sw_queue *sq)
spin_unlock_bh(&hwq->lock);
ieee80211_send_bar(vif, sta->addr, tid, agg_ssn);
spin_lock_bh(&hwq->lock);
- goto restart;
}
- list_del_init(&mtxq->list);
-
- cur = mt76_txq_send_burst(dev, sq, mtxq, &empty);
- if (!empty)
- list_add_tail(&mtxq->list, &sq->swq);
-
- if (cur < 0)
- return cur;
-
- len += cur;
-
- if (mtxq == mtxq_last)
- break;
+ ret += mt76_txq_send_burst(dev, sq, mtxq, &empty);
+ if (skb_queue_empty(&mtxq->retry_q))
+ empty = true;
+ ieee80211_return_txq(dev->hw, txq, !empty);
}
+ spin_unlock_bh(&hwq->lock);
- return len;
+ return ret;
}
-void mt76_txq_schedule(struct mt76_dev *dev, struct mt76_sw_queue *sq)
+void mt76_txq_schedule(struct mt76_dev *dev, enum mt76_txq_id qid)
{
+ struct mt76_sw_queue *sq = &dev->q_tx[qid];
int len;
+ if (qid >= 4)
+ return;
+
+ if (sq->swq_queued >= 4)
+ return;
+
rcu_read_lock();
- do {
- if (sq->swq_queued >= 4 || list_empty(&sq->swq))
- break;
- len = mt76_txq_schedule_list(dev, sq);
+ do {
+ ieee80211_txq_schedule_start(dev->hw, qid);
+ len = mt76_txq_schedule_list(dev, qid);
+ ieee80211_txq_schedule_end(dev->hw, qid);
} while (len > 0);
+
rcu_read_unlock();
}
EXPORT_SYMBOL_GPL(mt76_txq_schedule);
@@ -547,13 +560,8 @@ void mt76_txq_schedule_all(struct mt76_dev *dev)
{
int i;
- for (i = 0; i <= MT_TXQ_BK; i++) {
- struct mt76_queue *q = dev->q_tx[i].q;
-
- spin_lock_bh(&q->lock);
- mt76_txq_schedule(dev, &dev->q_tx[i]);
- spin_unlock_bh(&q->lock);
- }
+ for (i = 0; i <= MT_TXQ_BK; i++)
+ mt76_txq_schedule(dev, i);
}
EXPORT_SYMBOL_GPL(mt76_txq_schedule_all);
@@ -575,8 +583,6 @@ void mt76_stop_tx_queues(struct mt76_dev *dev, struct ieee80211_sta *sta,
spin_lock_bh(&hwq->lock);
mtxq->send_bar = mtxq->aggr && send_bar;
- if (!list_empty(&mtxq->list))
- list_del_init(&mtxq->list);
spin_unlock_bh(&hwq->lock);
}
}
@@ -585,24 +591,16 @@ EXPORT_SYMBOL_GPL(mt76_stop_tx_queues);
void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *txq)
{
struct mt76_dev *dev = hw->priv;
- struct mt76_txq *mtxq = (struct mt76_txq *)txq->drv_priv;
- struct mt76_sw_queue *sq = mtxq->swq;
- struct mt76_queue *hwq = sq->q;
if (!test_bit(MT76_STATE_RUNNING, &dev->state))
return;
- spin_lock_bh(&hwq->lock);
- if (list_empty(&mtxq->list))
- list_add_tail(&mtxq->list, &sq->swq);
- mt76_txq_schedule(dev, sq);
- spin_unlock_bh(&hwq->lock);
+ mt76_txq_schedule(dev, txq->ac);
}
EXPORT_SYMBOL_GPL(mt76_wake_tx_queue);
void mt76_txq_remove(struct mt76_dev *dev, struct ieee80211_txq *txq)
{
- struct mt76_queue *hwq;
struct mt76_txq *mtxq;
struct sk_buff *skb;
@@ -610,12 +608,6 @@ void mt76_txq_remove(struct mt76_dev *dev, struct ieee80211_txq *txq)
return;
mtxq = (struct mt76_txq *) txq->drv_priv;
- hwq = mtxq->swq->q;
-
- spin_lock_bh(&hwq->lock);
- if (!list_empty(&mtxq->list))
- list_del_init(&mtxq->list);
- spin_unlock_bh(&hwq->lock);
while ((skb = skb_dequeue(&mtxq->retry_q)) != NULL)
ieee80211_free_txskb(dev->hw, skb);
@@ -626,7 +618,6 @@ void mt76_txq_init(struct mt76_dev *dev, struct ieee80211_txq *txq)
{
struct mt76_txq *mtxq = (struct mt76_txq *) txq->drv_priv;
- INIT_LIST_HEAD(&mtxq->list);
skb_queue_head_init(&mtxq->retry_q);
mtxq->swq = &dev->q_tx[mt76_txq_get_qid(txq)];
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index a3acc070063a..d93dadce95ab 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -645,7 +645,6 @@ static void mt76u_tx_tasklet(unsigned long data)
dev->drv->tx_complete_skb(dev, i, &entry);
spin_lock_bh(&q->lock);
}
- mt76_txq_schedule(dev, sq);
wake = q->stopped && q->queued < q->ndesc - 8;
if (wake)
@@ -656,6 +655,8 @@ static void mt76u_tx_tasklet(unsigned long data)
spin_unlock_bh(&q->lock);
+ mt76_txq_schedule(dev, i);
+
if (!test_and_set_bit(MT76_READING_STATS, &dev->state))
ieee80211_queue_delayed_work(dev->hw,
&dev->usb.stat_work,
--
2.17.0
^ permalink raw reply related
* [PATCH v2 3/5] mt76: store wcid tx rate info in one u32 reduce locking
From: Felix Fietkau @ 2019-04-25 8:38 UTC (permalink / raw)
To: linux-wireless
In-Reply-To: <20190425083844.14186-1-nbd@nbd.name>
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 10 ++++---
.../net/wireless/mediatek/mt76/mt7603/mac.c | 4 +--
.../net/wireless/mediatek/mt76/mt76x02_mac.c | 26 ++++++++++++-------
.../net/wireless/mediatek/mt76/mt76x02_util.c | 1 -
drivers/net/wireless/mediatek/mt76/tx.c | 4 +--
5 files changed, 27 insertions(+), 18 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index e68834ee8393..176faaac8748 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -188,6 +188,11 @@ enum mt76_wcid_flags {
DECLARE_EWMA(signal, 10, 8);
+#define MT_WCID_TX_INFO_RATE GENMASK(15, 0)
+#define MT_WCID_TX_INFO_NSS GENMASK(17, 16)
+#define MT_WCID_TX_INFO_TXPWR_ADJ GENMASK(25, 18)
+#define MT_WCID_TX_INFO_SET BIT(31)
+
struct mt76_wcid {
struct mt76_rx_tid __rcu *aggr[IEEE80211_NUM_TIDS];
@@ -206,10 +211,7 @@ struct mt76_wcid {
u8 rx_check_pn;
u8 rx_key_pn[IEEE80211_NUM_TIDS][6];
- __le16 tx_rate;
- bool tx_rate_set;
- u8 tx_rate_nss;
- s8 max_txpwr_adj;
+ u32 tx_info;
bool sw_iv;
u8 packet_id;
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
index 2d090dacb788..47f5005ea48a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
@@ -717,11 +717,11 @@ void mt7603_wtbl_set_rates(struct mt7603_dev *dev, struct mt7603_sta *sta,
MT_WTBL_UPDATE_RATE_UPDATE |
MT_WTBL_UPDATE_TX_COUNT_CLEAR);
- if (!sta->wcid.tx_rate_set)
+ if (!(sta->wcid.tx_info & MT_WCID_TX_INFO_SET))
mt76_poll(dev, MT_WTBL_UPDATE, MT_WTBL_UPDATE_BUSY, 0, 5000);
sta->rate_count = 2 * MT7603_RATE_RETRY * n_rates;
- sta->wcid.tx_rate_set = true;
+ sta->wcid.tx_info |= MT_WCID_TX_INFO_SET;
}
static enum mt7603_cipher_type
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
index 4a77d509a86b..b81b71ba0930 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
@@ -218,10 +218,17 @@ mt76x02_mac_tx_rate_val(struct mt76x02_dev *dev,
void mt76x02_mac_wcid_set_rate(struct mt76x02_dev *dev, struct mt76_wcid *wcid,
const struct ieee80211_tx_rate *rate)
{
- spin_lock_bh(&dev->mt76.lock);
- wcid->tx_rate = mt76x02_mac_tx_rate_val(dev, rate, &wcid->tx_rate_nss);
- wcid->tx_rate_set = true;
- spin_unlock_bh(&dev->mt76.lock);
+ s8 max_txpwr_adj = mt76x02_tx_get_max_txpwr_adj(dev, rate);
+ __le16 rateval;
+ u32 tx_info;
+ s8 nss;
+
+ rateval = mt76x02_mac_tx_rate_val(dev, rate, &nss);
+ tx_info = FIELD_PREP(MT_WCID_TX_INFO_RATE, rateval) |
+ FIELD_PREP(MT_WCID_TX_INFO_NSS, nss) |
+ FIELD_PREP(MT_WCID_TX_INFO_TXPWR_ADJ, max_txpwr_adj) |
+ MT_WCID_TX_INFO_SET;
+ wcid->tx_info = tx_info;
}
void mt76x02_mac_set_short_preamble(struct mt76x02_dev *dev, bool enable)
@@ -323,6 +330,7 @@ void mt76x02_mac_write_txwi(struct mt76x02_dev *dev, struct mt76x02_txwi *txwi,
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
struct ieee80211_tx_rate *rate = &info->control.rates[0];
struct ieee80211_key_conf *key = info->control.hw_key;
+ u32 wcid_tx_info;
u16 rate_ht_mask = FIELD_PREP(MT_RXWI_RATE_PHY, BIT(1) | BIT(2));
u16 txwi_flags = 0;
u8 nss;
@@ -357,16 +365,16 @@ void mt76x02_mac_write_txwi(struct mt76x02_dev *dev, struct mt76x02_txwi *txwi,
txwi->eiv = *((__le32 *)&ccmp_pn[4]);
}
- spin_lock_bh(&dev->mt76.lock);
if (wcid && (rate->idx < 0 || !rate->count)) {
- txwi->rate = wcid->tx_rate;
- max_txpwr_adj = wcid->max_txpwr_adj;
- nss = wcid->tx_rate_nss;
+ wcid_tx_info = wcid->tx_info;
+ txwi->rate = FIELD_GET(MT_WCID_TX_INFO_RATE, wcid_tx_info);
+ max_txpwr_adj = FIELD_GET(MT_WCID_TX_INFO_TXPWR_ADJ,
+ wcid_tx_info);
+ nss = FIELD_GET(MT_WCID_TX_INFO_NSS, wcid_tx_info);
} else {
txwi->rate = mt76x02_mac_tx_rate_val(dev, rate, &nss);
max_txpwr_adj = mt76x02_tx_get_max_txpwr_adj(dev, rate);
}
- spin_unlock_bh(&dev->mt76.lock);
txpwr_adj = mt76x02_tx_get_txpwr_adj(dev, dev->mt76.txpower_conf,
max_txpwr_adj);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
index 284dae65cdf1..f3558c108b17 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
@@ -572,7 +572,6 @@ void mt76x02_sta_rate_tbl_update(struct ieee80211_hw *hw,
rate.idx = rates->rate[0].idx;
rate.flags = rates->rate[0].flags;
mt76x02_mac_wcid_set_rate(dev, &msta->wcid, &rate);
- msta->wcid.max_txpwr_adj = mt76x02_tx_get_max_txpwr_adj(dev, &rate);
}
EXPORT_SYMBOL_GPL(mt76x02_sta_rate_tbl_update);
diff --git a/drivers/net/wireless/mediatek/mt76/tx.c b/drivers/net/wireless/mediatek/mt76/tx.c
index 48f588726b3f..dd0c583ab5b1 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -266,7 +266,7 @@ mt76_tx(struct mt76_dev *dev, struct ieee80211_sta *sta,
skb_set_queue_mapping(skb, qid);
}
- if (!wcid->tx_rate_set)
+ if (!(wcid->tx_info & MT_WCID_TX_INFO_SET))
ieee80211_get_tx_rates(info->control.vif, sta, skb,
info->control.rates, 1);
@@ -412,7 +412,7 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
}
info = IEEE80211_SKB_CB(skb);
- if (!wcid->tx_rate_set)
+ if (!(wcid->tx_info & MT_WCID_TX_INFO_SET))
ieee80211_get_tx_rates(txq->vif, txq->sta, skb,
info->control.rates, 1);
tx_rate = info->control.rates[0];
--
2.17.0
^ permalink raw reply related
* [PATCH] brcmfmac: allocate msgbuf pktid from 1 to pktids array size
From: Wright Feng @ 2019-04-25 8:04 UTC (permalink / raw)
To: arend.vanspriel@broadcom.com, franky.lin@broadcom.com,
hante.meuleman@broadcom.com, kvalo@codeaurora.org, Chi-Hsien Lin
Cc: Wright Feng, linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com
Some PCIE firmwares drop txstatus if pktid is 0 and make packet held in
host side and not be released. If that packet type is 802.1x, the
pend_8021x_cnt value will be always greater than 0 and show "Timed out
waiting for no pending 802.1x packets" error message when sending key to
dongle.
To be compatible with all firmwares, host should allocate msgbuf pktid
from 1 instead of from 0.
Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
index d3780ea..b2fab3c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.c
@@ -348,7 +348,7 @@ brcmf_msgbuf_alloc_pktid(struct device *dev,
do {
(*idx)++;
if (*idx == pktids->array_size)
- *idx = 0;
+ *idx = 1;
if (array[*idx].allocated.counter == 0)
if (atomic_cmpxchg(&array[*idx].allocated, 0, 1) == 0)
break;
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v8 13/15] x86/split_lock: Enable split lock detection by default
From: Thomas Gleixner @ 2019-04-25 7:50 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-14-git-send-email-fenghua.yu@intel.com>
On Wed, 24 Apr 2019, Fenghua Yu wrote:
>
> +static void split_lock_update_msr(void)
> +{
> + /* Enable split lock detection */
> + msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> + this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
I'm pretty sure, that I told you to utilize the cache proper. Again:
> > Nothing in this file initializes msr_test_ctl_cache explicitely. Register
> > caching always requires to read the register and store it in the cache
> > before doing anything with it. Nothing guarantees that all bits in that MSR
> > are 0 by default forever.
> >
> > And once you do that _before_ calling split_lock_update_msr() then you can
> > spare the RMW in that function.
So you managed to fix the initializaiton part, but then you still do a
pointless RMW.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v8 12/15] kvm/vmx: Emulate MSR TEST_CTL
From: Thomas Gleixner @ 2019-04-25 7:42 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless, Xiaoyao Li
In-Reply-To: <1556134382-58814-13-git-send-email-fenghua.yu@intel.com>
On Wed, 24 Apr 2019, Fenghua Yu wrote:
>
> +static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
> +{
> + u64 host_msr_test_ctl;
> +
> + if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> + return;
Again: MSR_TST_CTL is not only about LOCK_DETECT. Check the control mask.
> + host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
> +
> + if (host_msr_test_ctl == vmx->msr_test_ctl) {
This still assumes that the only bit which can be set in the MSR is that
lock detect bit.
> + clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
> + } else {
> + add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
> + host_msr_test_ctl, false);
So what happens here is that if any other bit is set on the host, VMENTER
will happily clear it.
guest = (host & ~vmx->test_ctl_mask) | vmx->test_ctl;
That preserves any bits which are not exposed to the guest.
But the way more interesting question is why are you exposing the MSR and
the bit to the guest at all if the host has split lock detection enabled?
That does not make any sense as you basically allow the guest to switch it
off and then launch a slowdown attack. If the host has it enabled, then a
guest has to be treated like any other process and the #AC trap has to be
caught by the hypervisor which then kills the guest.
Only if the host has split lock detection disabled, then you can expose it
and allow the guest to turn it on and handle it on its own.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v2] brcmfmac: send mailbox interrupt twice for specific hardware device
From: Arend Van Spriel @ 2019-04-25 7:37 UTC (permalink / raw)
To: Wright Feng, franky.lin@broadcom.com, hante.meuleman@broadcom.com,
kvalo@codeaurora.org, Chi-Hsien Lin
Cc: linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com
In-Reply-To: <1556175939-174559-1-git-send-email-wright.feng@cypress.com>
On 4/25/2019 9:05 AM, Wright Feng wrote:
> For PCIE wireless device with core revision less than 14, device may miss
> PCIE to System Backplane Interrupt via PCIEtoSBMailbox. So add sending
> mail box interrupt twice as a hardware workaround.
Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Wright Feng <wright.feng@cypress.com>
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
^ permalink raw reply
* Re: [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
From: Thomas Gleixner @ 2019-04-25 7:29 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Paolo Bonzini,
Dave Hansen, Ashok Raj, Peter Zijlstra, Ravi V Shankar,
Xiaoyao Li, Christopherson Sean J, Kalle Valo, Michael Chan,
linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-11-git-send-email-fenghua.yu@intel.com>
On Wed, 24 Apr 2019, Fenghua Yu wrote:
> +void handle_split_lock_kernel_mode(void)
....
> +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
> +{
> + unsigned int trapnr = X86_TRAP_AC;
> + char str[] = "alignment check";
> + int signr = SIGBUS;
> +
> + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> + if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) ==
> + NOTIFY_STOP)
> + return;
> +
> + cond_local_irq_enable(regs);
> + if (!user_mode(regs) &&
> + static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
> + /*
> + * Only split lock can generate #AC from kernel at this point.
> + * Warn and disable split lock detection on this CPU. The
> + * faulting instruction will be executed without generating
> + * another #AC fault.
> + */
> + return handle_split_lock_kernel_mode();
return fun()? For some reason gcc will not complain about that, but for the
reader it's confusing at best.
Thanks,
tglx
^ permalink raw reply
* Re: brcmfmac: NULL pointer dereference during brcmf_detach() after firmware crash
From: Arend Van Spriel @ 2019-04-25 7:17 UTC (permalink / raw)
To: Rafał Miłecki
Cc: linux-wireless@vger.kernel.org,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
open list:BROADCOM BRCM80211 IEEE802.11n WIRELESS DRIVER,
brcm80211-dev-list, Aaron Blair
In-Reply-To: <CACna6rwF7rai_PMY3VtYXD=uBAT+jHsr8zrWP28vKjUx2S7Uyg@mail.gmail.com>
On 4/18/2019 1:55 PM, Rafał Miłecki wrote:
> On Fri, 15 Feb 2019 at 07:15, Rafał Miłecki <zajec5@gmail.com> wrote:
>> On Thu, 14 Feb 2019 at 23:37, Arend Van Spriel
>> <arend.vanspriel@broadcom.com> wrote:
>>> On 2/14/2019 11:30 PM, Rafał Miłecki wrote:
>>>> I've just found a well reproducible brcmfmac crash (NULL pointer
>>>> dereference).
>>>>
>>>> Steps:
>>>> 1. Wait for or trigger a FullMAC firmware crash
>>>> 2. Wait for some skb to get queued on a flowring
>>>> 3. Call rmmod brcmfmac
>>>>
>>>> Problem:
>>>> There is a NULL pointer dereference in one of the brcmf_detach() calls.
>>>>
>>>> Explanation:
>>>> brcmf_detach() first frees all "ifp"s and then deletes flowrings. If any
>>>> flowring has a skb it results in calling brcmf_txfinalize() which tries
>>>> to access "ifp" (struct brcmf_if) which is a NULL.
>>>
>>> Hi Rafał,
>>>
>>> Thanks for diving in. That was my suspicion. Does it mean you are
>>> working on a patch or shall I take care of it.
>>
>> It would be nice to have someone more experienced with detaching &
>> rings look at it. Is adding a simple
>> if (ifp)
>> enough? Or should that code get redesigned? Should we e.g. reorder detach order?
>
> Hi Arend, would you find a moment to look at that crash, please?
Hi Rafał,
Sorry for getting back on this so late. The driver tries to gracefully
teardown stuff by sending firmware commands to the device. I think that
makes no sense as that level of communication is not possible once our
driver .remove() callback is called. So I think upon calling
brcmf_detach() we should cleanup everything bottom-up. I will rework the
code and see how that goes.
Thanks,
Arend
^ permalink raw reply
* [PATCH v5.1] iwlwifi: mvm: fix merge damage in iwl_mvm_vif_dbgfs_register()
From: Luca Coelho @ 2019-04-25 7:14 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, Luca Coelho
From: Luca Coelho <luciano.coelho@intel.com>
When I rebased Greg's patch, I accidentally left the old if block that
was already there. Remove it.
Fixes: 154d4899e411 ("iwlwifi: mvm: properly check debugfs dentry before using it")
Signed-off-by: Luca Coelho <luciano.coelho@intel.com>
---
I am going to include this patch when I send my next pull request for
v5.1. This is not so important, but it's simple and clear and will
prevent further merge conflicts later.
drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c | 6 ------
1 file changed, 6 deletions(-)
diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
index 738eddb2e7ac..6925527d8457 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/debugfs-vif.c
@@ -780,12 +780,6 @@ void iwl_mvm_vif_dbgfs_register(struct iwl_mvm *mvm, struct ieee80211_vif *vif)
return;
}
- if (!mvmvif->dbgfs_dir) {
- IWL_ERR(mvm, "Failed to create debugfs directory under %pd\n",
- dbgfs_dir);
- return;
- }
-
if (iwlmvm_mod_params.power_scheme != IWL_POWER_SCHEME_CAM &&
((vif->type == NL80211_IFTYPE_STATION && !vif->p2p) ||
(vif->type == NL80211_IFTYPE_STATION && vif->p2p)))
--
2.20.1
^ permalink raw reply related
* [PATCH v2] brcmfmac: send mailbox interrupt twice for specific hardware device
From: Wright Feng @ 2019-04-25 7:05 UTC (permalink / raw)
To: arend.vanspriel@broadcom.com, franky.lin@broadcom.com,
hante.meuleman@broadcom.com, kvalo@codeaurora.org, Chi-Hsien Lin
Cc: Wright Feng, linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com
For PCIE wireless device with core revision less than 14, device may miss
PCIE to System Backplane Interrupt via PCIEtoSBMailbox. So add sending
mail box interrupt twice as a hardware workaround.
Signed-off-by: Wright Feng <wright.feng@cypress.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index fd3968f..66ee92b 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -698,7 +698,11 @@ brcmf_pcie_send_mb_data(struct brcmf_pciedev_info *devinfo, u32 htod_mb_data)
brcmf_pcie_write_tcm32(devinfo, addr, htod_mb_data);
pci_write_config_dword(devinfo->pdev, BRCMF_PCIE_REG_SBMBX, 1);
- pci_write_config_dword(devinfo->pdev, BRCMF_PCIE_REG_SBMBX, 1);
+
+ /* Send mailbox interrupt twice as a hardware workaround */
+ core = brcmf_chip_get_core(devinfo->ci, BCMA_CORE_PCIE2);
+ if (core->rev <= 13)
+ pci_write_config_dword(devinfo->pdev, BRCMF_PCIE_REG_SBMBX, 1);
return 0;
}
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v8 15/15] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Ingo Molnar @ 2019-04-25 6:31 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-16-git-send-email-fenghua.yu@intel.com>
* Fenghua Yu <fenghua.yu@intel.com> wrote:
> To workaround or debug a split lock issue, the administrator may need to
> disable or enable split lock detection during run time without rebooting
> the system.
>
> The interface /sys/device/system/cpu/split_lock_detect is added to allow
> the administrator to disable or enable split lock detection and show
> current split lock detection setting.
>
> Writing [yY1] or [oO][nN] to the file enables split lock detection and
> writing [nN0] or [oO][fF] disables split lock detection. Split lock
> detection is enabled or disabled on all CPUs.
>
> Reading the file returns current global split lock detection setting:
> 0: disabled
> 1: enabled
>
> Add an ABI document entry for /sys/devices/system/cpu/split_lock_detect.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> Not sure if the justification for the sysfs knob is valid. If not, this
> patch could be removed from this patch set.
>
> .../ABI/testing/sysfs-devices-system-cpu | 22 ++++++++
> arch/x86/kernel/cpu/intel.c | 52 ++++++++++++++++++-
> 2 files changed, 72 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-devices-system-cpu b/Documentation/ABI/testing/sysfs-devices-system-cpu
> index 9605dbd4b5b5..aad7b1698065 100644
> --- a/Documentation/ABI/testing/sysfs-devices-system-cpu
> +++ b/Documentation/ABI/testing/sysfs-devices-system-cpu
> @@ -67,6 +67,28 @@ Description: Discover NUMA node a CPU belongs to
> /sys/devices/system/cpu/cpu42/node2 -> ../../node/node2
>
>
> +What: /sys/devices/system/cpu/split_lock_detect
> +Date: March 2019
> +Contact: Linux kernel mailing list <linux-kernel@vger.kernel.org>
> +Description: (RW) Control split lock detection on Intel Tremont and
> + future CPUs
> +
> + Reads return split lock detection status:
> + 0: disabled
> + 1: enabled
> +
> + Writes enable or disable split lock detection:
> + The first character is one of 'Nn0' or [oO][fF] for off
> + disables the feature.
> + The first character is one of 'Yy1' or [oO][nN] for on
> + enables the feature.
> +
> + Please note the interface only shows or controls global setting.
> + During run time, split lock detection on one CPU may be
> + disabled if split lock operation in kernel code happens on
> + the CPU. The interface doesn't show or control split lock
> + detection on individual CPU.
I.e. implementation and possible actual state are out of sync. Why?
Also, if it's a global flag, why waste memory on putting a sysfs knob
into every CPU's sysfs file?
Finally, why is a debugging facility in sysfs, why not a debugfs knob?
Using a sysctl would solve the percpu vs. global confusion as well ...
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -35,6 +35,7 @@
> DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
>
> +static DEFINE_MUTEX(split_lock_detect_mutex);
> static bool split_lock_detect_enable;
'enable' is a verb in plain form - which we use for function names.
For variable names that denotes current state we typically use past
tense, i.e. 'enabled'.
(The only case where we'd us the split_lock_detect_enable name for a flag
if it's a flag to trigger some sort of enabling action - which this
isn't.)
Please review the whole series for various naming mishaps.
> + mutex_lock(&split_lock_detect_mutex);
> +
> + split_lock_detect_enable = val;
> +
> + /* Update the split lock detection setting in MSR on all online CPUs. */
> + on_each_cpu(split_lock_update_msr, NULL, 1);
> +
> + if (split_lock_detect_enable)
> + pr_info("enabled\n");
> + else
> + pr_info("disabled\n");
> +
> + mutex_unlock(&split_lock_detect_mutex);
Instead of a mutex, please just use the global atomic debug flag which
controls the warning printout. By using that flag both for the WARN()ing
and for controlling MSR state all the races are solved and the code is
simplified.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v8 09/15] x86/split_lock: Define MSR TEST_CTL register
From: Ingo Molnar @ 2019-04-25 6:21 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-10-git-send-email-fenghua.yu@intel.com>
* Fenghua Yu <fenghua.yu@intel.com> wrote:
> Setting bit 29 in MSR TEST_CTL (0x33) enables split lock detection and
> clearing the bit disables split lock detection.
>
> Define the MSR and the bit. The definitions will be used in enabling or
> disabling split lock detection.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> arch/x86/include/asm/msr-index.h | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index f65ef6f783d2..296eeb761ab6 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -39,6 +39,10 @@
>
> /* Intel MSRs. Some also available on other CPUs */
>
> +#define MSR_TEST_CTL 0x00000033
> +#define TEST_CTL_SPLIT_LOCK_DETECT_SHIFT 29
> +#define TEST_CTL_SPLIT_LOCK_DETECT BIT(29)
Three problems:
- Is MSR_TEST_CTL is not really a canonical MSR name... A quick look at
msr-index reveals the prevailing nomenclature:
dagon:~/tip> git grep -h 'define MSR' arch/x86/include/asm/msr-index.h | cut -d_ -f1-2 | sort -n | uniq -c | sort -n | tail -10
8 #define MSR_K8
8 #define MSR_MTRRfix4K
12 #define MSR_CORE
13 #define MSR_IDT
14 #define MSR_K7
16 #define MSR_PKG
19 #define MSR_F15H
33 #define MSR_AMD64
83 #define MSR_P4
163 #define MSR_IA32
I.e. this shouldn't this be something like MSR_IA32_TEST_CTL - or this
the name the Intel SDM uses? (I haven't checked.)
- The canonical way to define MSR capabilities is to use the MSR's name
as a prefix. I.e.:
MSR_TEST_CTL
MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT
MSR_TEST_CTL_SPLIT_LOCK_DETECT
etc.
Instead of the random mixture of MSR_ prefixed and non-prefixed
MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT and
TEST_CTL_SPLIT_LOCK_DETECT names.
- Finally, this is not how we define bits - the _SHIFT postfix is actively
confusing as we usually denote _SHIFT values with something that is
used in a bit-shift operation, which this isn't. Instead the proper
scheme is to postfix the bit number with _BIT and the mask with _MASK,
i.e. something like:
#define MSR_TEST_CTL 0x00000033
#define MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT 29
#define MSR_TEST_CTL_SPLIT_LOCK_DETECT BIT(MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT)
Note how this cleans up actual usage:
+ msr_set_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
+ this_cpu_or(msr_test_ctl_cache, TEST_CTL_SPLIT_LOCK_DETECT);
- msr_set_bit(MSR_TEST_CTL, MSR_TEST_CTL_SPLIT_LOCK_DETECT_BIT);
- this_cpu_or(msr_test_ctl_cache, MSR_TEST_CTL_SPLIT_LOCK_DETECT);
Frankly, this kind of disorganized code in a v8 submission is *really*
disappointing, it's not like it's hard to look up these patterns and
practices in existing code...
Sigh.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v8 10/15] x86/split_lock: Handle #AC exception for split lock
From: Ingo Molnar @ 2019-04-25 6:07 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-11-git-send-email-fenghua.yu@intel.com>
* Fenghua Yu <fenghua.yu@intel.com> wrote:
> There may be different considerations on how to handle #AC for split lock,
> e.g. how to handle system hang caused by split lock issue in firmware,
> how to emulate faulting instruction, etc. We use a simple method to
> handle user and kernel split lock and may extend the method in the future.
>
> When #AC exception for split lock is triggered from user process, the
> process is killed by SIGBUS. To execute the process properly, a user
> application developer needs to fix the split lock issue.
>
> When #AC exception for split lock is triggered from a kernel instruction,
> disable split lock detection on local CPU and warn the split lock issue.
> After the exception, the faulting instruction will be executed and kernel
> execution continues. Split lock detection is only disabled on the local
> CPU, not globally. It will be re-enabled if the CPU is offline and then
> online or through sysfs interface.
>
> A kernel/driver developer should check the warning, which contains helpful
> faulting address, context, and callstack info, and fix the split lock
> issues. Then further split lock issues may be captured and fixed.
>
> After bit 29 in MSR_TEST_CTL is set to 1 in kernel, firmware inherits
> the setting when firmware is executed in S4, S5, run time services, SMI,
> etc. If there is a split lock operation in firmware, it will triggers
> #AC and may hang the system depending on how firmware handles the #AC.
> It's up to a firmware developer to fix split lock issues in firmware.
>
> MSR TEST_CTL value is cached in per CPU msr_test_ctl_cache which will be
> used in virtualization to avoid costly MSR read.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> arch/x86/include/asm/cpu.h | 3 +++
> arch/x86/kernel/cpu/intel.c | 24 ++++++++++++++++++++++++
> arch/x86/kernel/traps.c | 31 ++++++++++++++++++++++++++++++-
> 3 files changed, 57 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> index 4e03f53fc079..5706461eb60f 100644
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -42,7 +42,10 @@ unsigned int x86_model(unsigned int sig);
> unsigned int x86_stepping(unsigned int sig);
> #ifdef CONFIG_CPU_SUP_INTEL
> void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c);
> +DECLARE_PER_CPU(u64, msr_test_ctl_cache);
> +void handle_split_lock_kernel_mode(void);
> #else
> static inline void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c) {}
> +static inline void handle_split_lock_kernel_mode(void) {}
> #endif
> #endif /* _ASM_X86_CPU_H */
> diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
> index d7e676c2aebf..2cc69217ca7c 100644
> --- a/arch/x86/kernel/cpu/intel.c
> +++ b/arch/x86/kernel/cpu/intel.c
> @@ -31,6 +31,9 @@
> #include <asm/apic.h>
> #endif
>
> +DEFINE_PER_CPU(u64, msr_test_ctl_cache);
> +EXPORT_PER_CPU_SYMBOL_GPL(msr_test_ctl_cache);
> +
> /*
> * Just in case our CPU detection goes bad, or you have a weird system,
> * allow a way to override the automatic disabling of MPX.
> @@ -654,6 +657,17 @@ static void init_intel_misc_features(struct cpuinfo_x86 *c)
> wrmsrl(MSR_MISC_FEATURES_ENABLES, msr);
> }
>
> +static void init_split_lock_detect(struct cpuinfo_x86 *c)
> +{
> + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
> + u64 test_ctl_val;
> +
> + /* Cache MSR TEST_CTL */
> + rdmsrl(MSR_TEST_CTL, test_ctl_val);
> + this_cpu_write(msr_test_ctl_cache, test_ctl_val);
> + }
> +}
> +
> static void init_intel(struct cpuinfo_x86 *c)
> {
> early_init_intel(c);
> @@ -766,6 +780,8 @@ static void init_intel(struct cpuinfo_x86 *c)
> init_intel_energy_perf(c);
>
> init_intel_misc_features(c);
> +
> + init_split_lock_detect(c);
> }
>
> #ifdef CONFIG_X86_32
> @@ -1060,3 +1076,11 @@ void __init cpu_set_core_cap_bits(struct cpuinfo_x86 *c)
> if (ia32_core_cap & CORE_CAP_SPLIT_LOCK_DETECT)
> set_split_lock_detect();
> }
> +
> +void handle_split_lock_kernel_mode(void)
> +{
> + /* Warn and disable split lock detection on this CPU */
> + msr_clear_bit(MSR_TEST_CTL, TEST_CTL_SPLIT_LOCK_DETECT_SHIFT);
> + this_cpu_and(msr_test_ctl_cache, ~TEST_CTL_SPLIT_LOCK_DETECT);
> + WARN_ONCE(1, "split lock operation detected\n");
Please name this more descriptively, such as x86_split_lock_disable() or
so.
Also, please reorganize the split lock detection namespace to be less
idiosynchratic, use a common x86_split_lock_ prefix and organize the new
namespace around that:
x86_split_lock_init() // was: init_split_lock_detect
x86_split_lock_enable() // was: set_split_lock_detect
x86_split_lock_disable() // was: handle_split_lock_kernel_mode
etc.
> +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
> +{
> + unsigned int trapnr = X86_TRAP_AC;
> + char str[] = "alignment check";
> + int signr = SIGBUS;
> +
> + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> +
> + if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) ==
> + NOTIFY_STOP)
Please do not break lines mid-line when it does absolutely nothing to
improve readablity. Just ignore checkpatch.
> + cond_local_irq_enable(regs);
> + if (!user_mode(regs) &&
> + static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
Ditto - in fact this doesn't even violate the col80 rule ...
> + /*
> + * Only split lock can generate #AC from kernel at this point.
> + * Warn and disable split lock detection on this CPU. The
> + * faulting instruction will be executed without generating
> + * another #AC fault.
> + */
How about explaining all this more clearly:
> + /*
> + * Only split locks can generate #AC from kernel mode.
> + *
> + * The split-lock detection feature is a one-shot
> + * debugging facility, so we disable it immediately and
> + * print a warning.
> + *
> + * This also solves the instruction restart problem: we
> + * return the faulting instruction right after this it
> + * will be executed without generating another #AC fault
> + * and getting into an infinite loop, instead it will
> + * continue without side effects to the interrupted
> + * execution conext.
> + *
> + * Split-lock detection will remain disabled permanently
> + * after this, until the next reboot.
> + */
?
Also, AFAICS this code will disable split-lock detection only on the
current CPU - all the other 4,096 CPUs hitting this same lock at the
exact same time will happily continue spamming the kernel log as they
encounter the same split lock, correct?
While the warning itself uses WARN_ONCE(), that and the underlying
BUGFLAG_ONCE mechanism is not an atomic facility.
Instead, please add an explicit, global split_lock_debug bit that the
first CPU hitting it disables, and only that CPU is allowed to print a
single warning. All other CPUs just disable split-lock debugging silently
and continue.
This also solves the race if the split-lock #AC fault is re-triggered by
NMI of perf context interrupting one split-lock warning execution while
the original WARN_ON() is executing.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH v8 05/15] x86/msr-index: Define MSR_IA32_CORE_CAPABILITY and split lock detection bit
From: Ingo Molnar @ 2019-04-25 5:45 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Paolo Bonzini, Dave Hansen, Ashok Raj, Peter Zijlstra,
Ravi V Shankar, Xiaoyao Li, Christopherson Sean J, Kalle Valo,
Michael Chan, linux-kernel, x86, kvm, netdev, linux-wireless
In-Reply-To: <1556134382-58814-6-git-send-email-fenghua.yu@intel.com>
* Fenghua Yu <fenghua.yu@intel.com> wrote:
> A new MSR_IA32_CORE_CAPABILITY (0xcf) is defined. Each bit in the MSR
> enumerates a model specific feature. Currently bit 5 enumerates split
> lock detection. When bit 5 is 1, split lock detection is supported.
> When the bit is 0, split lock detection is not supported.
>
> Please check the latest Intel 64 and IA-32 Architectures Software
> Developer's Manual for more detailed information on the MSR and the
> split lock detection bit.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> arch/x86/include/asm/msr-index.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
> index ca5bc0eacb95..f65ef6f783d2 100644
> --- a/arch/x86/include/asm/msr-index.h
> +++ b/arch/x86/include/asm/msr-index.h
> @@ -59,6 +59,9 @@
> #define MSR_PLATFORM_INFO_CPUID_FAULT_BIT 31
> #define MSR_PLATFORM_INFO_CPUID_FAULT BIT_ULL(MSR_PLATFORM_INFO_CPUID_FAULT_BIT)
>
> +#define MSR_IA32_CORE_CAPABILITY 0x000000cf
> +#define CORE_CAP_SPLIT_LOCK_DETECT BIT(5) /* Detect split lock */
Please don't put comments into definitions.
Thanks,
Ingo
^ permalink raw reply
* Re: [PATCH] brcmfmac: remove the duplicate line of sending mail box interrupt
From: Wright Feng @ 2019-04-25 3:37 UTC (permalink / raw)
To: Arend Van Spriel, franky.lin@broadcom.com,
hante.meuleman@broadcom.com, kvalo@codeaurora.org, Chi-Hsien Lin
Cc: linux-wireless@vger.kernel.org,
brcm80211-dev-list.pdl@broadcom.com
In-Reply-To: <164b59e8-633b-84cc-179f-9bdd7ec696a7@broadcom.com>
On 2019/4/24 下午 04:46, Arend Van Spriel wrote:
> On 4/24/2019 7:24 AM, Arend Van Spriel wrote:
>> On April 22, 2019 4:47:31 AM Wright Feng <Wright.Feng@cypress.com> wrote:
>>
>>> The line is duplicate so remove it from pcie.c. We don't need to send
>>> host to dongle mail box interrupt twice after writing data.
>>
>> Hi Wright,
>>
>> Thanks for the patch. I am a bit early as it is not uncommon that
>> hardware requires an extra kick. Might be the case here for some
>> chips. Will follow-up on it.
>
> Hi Wright,
>
> I checked in our dhd driver code and it also does a double write. Turns
> out to be a hardware workaround so we need to keep this. It is worth a
> comment though.
>
Hi Arend,
Thanks for the information.
NXP and I worked on suspend/resume stress test issue with 1FD(4359b1)
two months ago, and we found one of root cause is duplicate mailbox
interrupt.
I've checked the history of the hardware issue internally, and it seems that
the issue is only on PCIEGEN2 revision 1 to 13.
So is that okay we can skip the hardware workaround when PCIEGEN2
revision is greater than 13 and add the comment on it in my patch v2?
Regards,
Wright
> Thanks,
> Arend
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox