* [PATCH 3/7] brcmfmac: avoid firmware command in brcmf_netdev_open() when bus is down
From: Arend van Spriel @ 2019-07-11 9:05 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Arend van Spriel
In-Reply-To: <1562835912-1404-1-git-send-email-arend.vanspriel@broadcom.com>
No point in sending a firmware command when bus is down so make it
conditional checking the state.
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
index 80d54d2..705b8cc 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
@@ -579,7 +579,8 @@ static int brcmf_netdev_stop(struct net_device *ndev)
brcmf_cfg80211_down(ndev);
- brcmf_fil_iovar_data_set(ifp, "arp_hostip_clear", NULL, 0);
+ if (ifp->drvr->bus_if->state == BRCMF_BUS_UP)
+ brcmf_fil_iovar_data_set(ifp, "arp_hostip_clear", NULL, 0);
brcmf_net_setcarrier(ifp, false);
--
1.9.1
^ permalink raw reply related
* [PATCH 0/7] brcmfmac: rework probe/attach sequence
From: Arend van Spriel @ 2019-07-11 9:05 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Arend van Spriel
The brcmfmac driver spews some error message upon unloading and Stefan
Wahren was wondering whether it could be cleaned up. Related to this
was a recent fix for NULL pointer deref. That fix introduced a construct
that added to the itch to rework the probe sequence. So this series
reverts commit 5cdb0ef6144f ("brcmfmac: fix NULL pointer derefence during
USB disconnect").
The changes in this series are:
* reorder brcmf_detach() code.
* avoid firmware interaction when bus is down.
* remove strlcpy() before issueing firmware version iovar.
This series applies to the master branch of the wireless-drivers-next
repository.
Arend van Spriel (7):
Revert "brcmfmac: fix NULL pointer derefence during USB disconnect"
brcmfmac: change the order of things in brcmf_detach()
brcmfmac: avoid firmware command in brcmf_netdev_open() when bus is
down
brcmfmac: clear events in brcmf_fweh_detach() will always fail
brcmfmac: avoid firmware commands when bus is down
brcmfmac: simply remove flowring if bus is down
brcmfmac: remove unnecessary strlcpy() upon obtaining "ver" iovar
.../wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 11 ++------
.../wireless/broadcom/brcm80211/brcmfmac/bcdc.h | 6 ++---
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 23 +++++++++--------
.../wireless/broadcom/brcm80211/brcmfmac/common.c | 1 -
.../wireless/broadcom/brcm80211/brcmfmac/core.c | 30 +++++++++++-----------
.../wireless/broadcom/brcm80211/brcmfmac/fweh.c | 9 -------
.../broadcom/brcm80211/brcmfmac/fwsignal.c | 16 +++---------
.../broadcom/brcm80211/brcmfmac/fwsignal.h | 3 +--
.../wireless/broadcom/brcm80211/brcmfmac/msgbuf.c | 7 +++++
.../wireless/broadcom/brcm80211/brcmfmac/proto.c | 10 ++------
.../wireless/broadcom/brcm80211/brcmfmac/proto.h | 3 +--
11 files changed, 47 insertions(+), 72 deletions(-)
--
1.9.1
^ permalink raw reply
* [PATCH 2/3] brcmfmac: enable DFS_OFFLOAD extended feature if supported
From: Arend van Spriel @ 2019-07-11 8:45 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Arend van Spriel
In-Reply-To: <1562834732-31508-1-git-send-email-arend.vanspriel@broadcom.com>
If the firmware supports 802.11h and the device can operate in 5GHz
band we can enable DFS_OFFLOAD extended feature.
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 5 +++++
drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c | 1 +
drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h | 4 +++-
3 files changed, 9 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 5168d42..3f72dc1 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -6733,6 +6733,11 @@ static int brcmf_setup_wiphy(struct wiphy *wiphy, struct brcmf_if *ifp)
}
}
+ if (wiphy->bands[NL80211_BAND_5GHZ] &&
+ brcmf_feat_is_enabled(ifp, BRCMF_FEAT_DOT11H))
+ wiphy_ext_feature_set(wiphy,
+ NL80211_EXT_FEATURE_DFS_OFFLOAD);
+
wiphy_read_of_freq_limits(wiphy);
return 0;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
index 73aff4e..2c3526a 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
@@ -39,6 +39,7 @@ struct brcmf_feat_fwcap {
{ BRCMF_FEAT_P2P, "p2p" },
{ BRCMF_FEAT_MONITOR, "monitor" },
{ BRCMF_FEAT_MONITOR_FMT_RADIOTAP, "rtap" },
+ { BRCMF_FEAT_DOT11H, "802.11h" }
};
#ifdef DEBUG
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
index f127eb2..736a817 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.h
@@ -25,6 +25,7 @@
* MONITOR: firmware can pass monitor packets to host.
* MONITOR_FMT_RADIOTAP: firmware provides monitor packets with radiotap header
* MONITOR_FMT_HW_RX_HDR: firmware provides monitor packets with hw/ucode header
+ * DOT11H: firmware supports 802.11h
*/
#define BRCMF_FEAT_LIST \
BRCMF_FEAT_DEF(MBSS) \
@@ -43,7 +44,8 @@
BRCMF_FEAT_DEF(FWSUP) \
BRCMF_FEAT_DEF(MONITOR) \
BRCMF_FEAT_DEF(MONITOR_FMT_RADIOTAP) \
- BRCMF_FEAT_DEF(MONITOR_FMT_HW_RX_HDR)
+ BRCMF_FEAT_DEF(MONITOR_FMT_HW_RX_HDR) \
+ BRCMF_FEAT_DEF(DOT11H)
/*
* Quirks:
--
1.9.1
^ permalink raw reply related
* [PATCH 1/3] brcmfmac: add 160MHz in chandef_to_chanspec()
From: Arend van Spriel @ 2019-07-11 8:45 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Arend van Spriel
In-Reply-To: <1562834732-31508-1-git-send-email-arend.vanspriel@broadcom.com>
The function chandef_to_chanspec() was not handling 160MHz bandwidth
resulting in wrong encoding of the channel. That resulting in firmware
rejecting the provided channel specification.
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
.../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index b6d0df3..5168d42 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -276,8 +276,26 @@ static u16 chandef_to_chanspec(struct brcmu_d11inf *d11inf,
else
ch_inf.sb = BRCMU_CHAN_SB_UU;
break;
- case NL80211_CHAN_WIDTH_80P80:
case NL80211_CHAN_WIDTH_160:
+ ch_inf.bw = BRCMU_CHAN_BW_160;
+ if (primary_offset == -70)
+ ch_inf.sb = BRCMU_CHAN_SB_LLL;
+ else if (primary_offset == -50)
+ ch_inf.sb = BRCMU_CHAN_SB_LLU;
+ else if (primary_offset == -30)
+ ch_inf.sb = BRCMU_CHAN_SB_LUL;
+ else if (primary_offset == -10)
+ ch_inf.sb = BRCMU_CHAN_SB_LUU;
+ else if (primary_offset == 10)
+ ch_inf.sb = BRCMU_CHAN_SB_ULL;
+ else if (primary_offset == 30)
+ ch_inf.sb = BRCMU_CHAN_SB_ULU;
+ else if (primary_offset == 50)
+ ch_inf.sb = BRCMU_CHAN_SB_UUL;
+ else
+ ch_inf.sb = BRCMU_CHAN_SB_UUU;
+ break;
+ case NL80211_CHAN_WIDTH_80P80:
case NL80211_CHAN_WIDTH_5:
case NL80211_CHAN_WIDTH_10:
default:
@@ -296,6 +314,7 @@ static u16 chandef_to_chanspec(struct brcmu_d11inf *d11inf,
}
d11inf->encchspec(&ch_inf);
+ brcmf_dbg(TRACE, "chanspec: 0x%x\n", ch_inf.chspec);
return ch_inf.chspec;
}
--
1.9.1
^ permalink raw reply related
* [PATCH 3/3] brcmfmac: allow 160MHz in custom regulatory rules
From: Arend van Spriel @ 2019-07-11 8:45 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Arend van Spriel
In-Reply-To: <1562834732-31508-1-git-send-email-arend.vanspriel@broadcom.com>
The driver has custom regulatory rules which had maximum bandwidth
for 5GHz channels set to 80MHz. As a consequence the driver can
not use 160MHz in AP mode even when the device supports it. So
relax the rules allowing 160MHz. After wiphy_register() the channel
flags are updated according what the device actually supports.
Reviewed-by: Hante Meuleman <hante.meuleman@broadcom.com>
Reviewed-by: Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>
Reviewed-by: Franky Lin <franky.lin@broadcom.com>
Signed-off-by: Arend van Spriel <arend.vanspriel@broadcom.com>
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
index 3f72dc1..b692689 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c
@@ -189,9 +189,9 @@ static bool check_vif_up(struct brcmf_cfg80211_vif *vif)
*/
REG_RULE(2484-10, 2484+10, 20, 6, 20, 0),
/* IEEE 802.11a, channel 36..64 */
- REG_RULE(5150-10, 5350+10, 80, 6, 20, 0),
+ REG_RULE(5150-10, 5350+10, 160, 6, 20, 0),
/* IEEE 802.11a, channel 100..165 */
- REG_RULE(5470-10, 5850+10, 80, 6, 20, 0), }
+ REG_RULE(5470-10, 5850+10, 160, 6, 20, 0), }
};
/* Note: brcmf_cipher_suites is an array of int defining which cipher suites
--
1.9.1
^ permalink raw reply related
* [PATCH 0/3] brcmfmac: 160MHz fixes and DFS offload
From: Arend van Spriel @ 2019-07-11 8:45 UTC (permalink / raw)
To: Kalle Valo; +Cc: linux-wireless, Arend van Spriel
The 160MHz AP mode was not properly working and limited due to lack of
DFS offload. So this series include following changes:
* encode 160MHz channel definition to firmware format.
* adapt custom regulatory rule allowing 160MHz.
* enable DFS offloading for firmwares supporting it.
The series applies to the master branch of the wireless-drivers-next
repository.
Arend van Spriel (3):
brcmfmac: add 160MHz in chandef_to_chanspec()
brcmfmac: enable DFS_OFFLOAD extended feature if supported
brcmfmac: allow 160MHz in custom regulatory rules
.../broadcom/brcm80211/brcmfmac/cfg80211.c | 30 +++++++++++++++++++---
.../wireless/broadcom/brcm80211/brcmfmac/feature.c | 1 +
.../wireless/broadcom/brcm80211/brcmfmac/feature.h | 4 ++-
3 files changed, 31 insertions(+), 4 deletions(-)
--
1.9.1
^ permalink raw reply
* Re: [PATCH v4 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR
From: Jian-Hong Pan @ 2019-07-11 5:30 UTC (permalink / raw)
To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller, Larry Finger,
David Laight, Christoph Hellwig
Cc: linux-wireless, Linux Netdev List, Linux Kernel,
Linux Upstreaming Team, Daniel Drake, stable
In-Reply-To: <20190711052427.5582-2-jian-hong@endlessm.com>
Jian-Hong Pan <jian-hong@endlessm.com> 於 2019年7月11日 週四 下午1:25寫道：
>
> Since each skb in RX ring is reused instead of new allocation, we can
> treat the DMA in a more efficient way by DMA synchronization.
>
> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> Cc: <stable@vger.kernel.org>
> ---
Sorry, also forget to place the version difference here
v2:
- New patch by following [PATCH v3 1/2] rtw88: pci: Rearrange the
memory usage for skb in RX ISR.
v3:
- Remove rtw_pci_sync_rx_desc_cpu and call dma_sync_single_for_cpu in
rtw_pci_rx_isr directly.
- Remove the return value of rtw_pci_sync_rx_desc_device.
- Use DMA_FROM_DEVICE instead of PCI_DMA_FROMDEVICE.
v4:
- Same as v3.
> drivers/net/wireless/realtek/rtw88/pci.c | 24 +++++++++++++++++++++---
> 1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index c415f5e94fed..68fae52151dd 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -206,6 +206,23 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
> return 0;
> }
>
> +static void rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
> + struct rtw_pci_rx_ring *rx_ring,
> + u32 idx, u32 desc_sz)
> +{
> + struct device *dev = rtwdev->dev;
> + struct rtw_pci_rx_buffer_desc *buf_desc;
> + int buf_sz = RTK_PCI_RX_BUF_SIZE;
> +
> + dma_sync_single_for_device(dev, dma, buf_sz, DMA_FROM_DEVICE);
> +
> + buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
> + idx * desc_sz);
> + memset(buf_desc, 0, sizeof(*buf_desc));
> + buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
> + buf_desc->dma = cpu_to_le32(dma);
> +}
> +
> static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
> struct rtw_pci_rx_ring *rx_ring,
> u8 desc_size, u32 len)
> @@ -782,8 +799,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> rtw_pci_dma_check(rtwdev, ring, cur_rp);
> skb = ring->buf[cur_rp];
> dma = *((dma_addr_t *)skb->cb);
> - pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
> - PCI_DMA_FROMDEVICE);
> + dma_sync_single_for_cpu(rtwdev->dev, dma, RTK_PCI_RX_BUF_SIZE,
> + DMA_FROM_DEVICE);
> rx_desc = skb->data;
> chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
>
> @@ -818,7 +835,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
>
> next_rp:
> /* new skb delivered to mac80211, re-enable original skb DMA */
> - rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
> + rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
> + buf_desc_sz);
>
> /* host read next element in ring */
> if (++cur_rp >= ring->r.len)
> --
> 2.22.0
>
^ permalink raw reply
* Re: [PATCH v4 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: Jian-Hong Pan @ 2019-07-11 5:28 UTC (permalink / raw)
To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller, Larry Finger,
David Laight, Christoph Hellwig
Cc: linux-wireless, Linux Netdev List, Linux Kernel,
Linux Upstreaming Team, Daniel Drake, stable
In-Reply-To: <20190711052427.5582-1-jian-hong@endlessm.com>
Jian-Hong Pan <jian-hong@endlessm.com> 於 2019年7月11日 週四 下午1:25寫道：
>
> Testing with RTL8822BE hardware, when available memory is low, we
> frequently see a kernel panic and system freeze.
>
> First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed):
>
> rx routine starvation
> WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822 rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> [ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
>
> Then we see a variety of different error conditions and kernel panics,
> such as this one (trimmed):
>
> rtw_pci 0000:02:00.0: pci bus timeout, check dma status
> skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415 head:00000000d2880c6f data:000000007a02b1ea tail:0x1df end:0xc0 dev:<NULL>
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:105!
> invalid opcode: 0000 [#1] SMP NOPTI
> RIP: 0010:skb_panic+0x43/0x45
>
> When skb allocation fails and the "rx routine starvation" is hit, the
> function returns immediately without updating the RX ring. At this
> point, the RX ring may continue referencing an old skb which was already
> handed off to ieee80211_rx_irqsafe(). When it comes to be used again,
> bad things happen.
>
> This patch allocates a new, data-sized skb first in RX ISR. After
> copying the data in, we pass it to the upper layers. However, if skb
> allocation fails, we effectively drop the frame. In both cases, the
> original, full size ring skb is reused.
>
> In addition, to fixing the kernel crash, the RX routine should now
> generally behave better under low memory conditions.
>
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053
> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> Cc: <stable@vger.kernel.org>
> ---
Sorry, I forget to place the version difference here.
v2:
- Allocate new data-sized skb and put data into it, then pass it to
mac80211. Reuse the original skb in RX ring by DMA sync.
- Modify the commit message.
- Introduce following [PATCH v3 2/2] rtw88: pci: Use DMA sync instead
of remapping in RX ISR.
v3:
- Same as v2.
v4:
- Fix comment: allocate a new skb for this frame, discard the frame
if none available
> drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++-------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index cfe05ba7280d..c415f5e94fed 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> u32 pkt_offset;
> u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> u32 buf_desc_sz = chip->rx_buf_desc_sz;
> + u32 new_len;
> u8 *rx_desc;
> dma_addr_t dma;
>
> @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> pkt_stat.shift;
>
> - if (pkt_stat.is_c2h) {
> - /* keep rx_desc, halmac needs it */
> - skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> + /* allocate a new skb for this frame,
> + * discard the frame if none available
> + */
> + new_len = pkt_stat.pkt_len + pkt_offset;
> + new = dev_alloc_skb(new_len);
> + if (WARN_ONCE(!new, "rx routine starvation\n"))
> + goto next_rp;
> +
> + /* put the DMA data including rx_desc from phy to new skb */
> + skb_put_data(new, skb->data, new_len);
>
> - /* pass offset for further operation */
> - *((u32 *)skb->cb) = pkt_offset;
> - skb_queue_tail(&rtwdev->c2h_queue, skb);
> + if (pkt_stat.is_c2h) {
> + /* pass rx_desc & offset for further operation */
> + *((u32 *)new->cb) = pkt_offset;
> + skb_queue_tail(&rtwdev->c2h_queue, new);
> ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> } else {
> - /* remove rx_desc, maybe use skb_pull? */
> - skb_put(skb, pkt_stat.pkt_len);
> - skb_reserve(skb, pkt_offset);
> -
> - /* alloc a smaller skb to mac80211 */
> - new = dev_alloc_skb(pkt_stat.pkt_len);
> - if (!new) {
> - new = skb;
> - } else {
> - skb_put_data(new, skb->data, skb->len);
> - dev_kfree_skb_any(skb);
> - }
> - /* TODO: merge into rx.c */
> - rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> + /* remove rx_desc */
> + skb_pull(new, pkt_offset);
> +
> + rtw_rx_stats(rtwdev, pkt_stat.vif, new);
> memcpy(new->cb, &rx_status, sizeof(rx_status));
> ieee80211_rx_irqsafe(rtwdev->hw, new);
> }
>
> - /* skb delivered to mac80211, alloc a new one in rx ring */
> - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> - if (WARN(!new, "rx routine starvation\n"))
> - return;
> -
> - ring->buf[cur_rp] = new;
> - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
> +next_rp:
> + /* new skb delivered to mac80211, re-enable original skb DMA */
> + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
>
> /* host read next element in ring */
> if (++cur_rp >= ring->r.len)
> --
> 2.22.0
>
^ permalink raw reply
* [PATCH v4 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR
From: Jian-Hong Pan @ 2019-07-11 5:24 UTC (permalink / raw)
To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller, Larry Finger,
David Laight, Christoph Hellwig
Cc: linux-wireless, netdev, linux-kernel, linux, Daniel Drake,
Jian-Hong Pan, stable
In-Reply-To: <20190711052427.5582-1-jian-hong@endlessm.com>
Since each skb in RX ring is reused instead of new allocation, we can
treat the DMA in a more efficient way by DMA synchronization.
Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
Cc: <stable@vger.kernel.org>
---
drivers/net/wireless/realtek/rtw88/pci.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index c415f5e94fed..68fae52151dd 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -206,6 +206,23 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
return 0;
}
+static void rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
+ struct rtw_pci_rx_ring *rx_ring,
+ u32 idx, u32 desc_sz)
+{
+ struct device *dev = rtwdev->dev;
+ struct rtw_pci_rx_buffer_desc *buf_desc;
+ int buf_sz = RTK_PCI_RX_BUF_SIZE;
+
+ dma_sync_single_for_device(dev, dma, buf_sz, DMA_FROM_DEVICE);
+
+ buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+ idx * desc_sz);
+ memset(buf_desc, 0, sizeof(*buf_desc));
+ buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
+ buf_desc->dma = cpu_to_le32(dma);
+}
+
static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
struct rtw_pci_rx_ring *rx_ring,
u8 desc_size, u32 len)
@@ -782,8 +799,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
rtw_pci_dma_check(rtwdev, ring, cur_rp);
skb = ring->buf[cur_rp];
dma = *((dma_addr_t *)skb->cb);
- pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
- PCI_DMA_FROMDEVICE);
+ dma_sync_single_for_cpu(rtwdev->dev, dma, RTK_PCI_RX_BUF_SIZE,
+ DMA_FROM_DEVICE);
rx_desc = skb->data;
chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
@@ -818,7 +835,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
next_rp:
/* new skb delivered to mac80211, re-enable original skb DMA */
- rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
+ rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
+ buf_desc_sz);
/* host read next element in ring */
if (++cur_rp >= ring->r.len)
--
2.22.0
^ permalink raw reply related
* [PATCH v4 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: Jian-Hong Pan @ 2019-07-11 5:24 UTC (permalink / raw)
To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller, Larry Finger,
David Laight, Christoph Hellwig
Cc: linux-wireless, netdev, linux-kernel, linux, Daniel Drake,
Jian-Hong Pan, stable
In-Reply-To: <CAPpJ_edDcaBq+0DocPmS-yYM10B4MkWvBn=f6wwbYdqzSGmp_g@mail.gmail.com>
Testing with RTL8822BE hardware, when available memory is low, we
frequently see a kernel panic and system freeze.
First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed):
rx routine starvation
WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822 rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
[ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
Then we see a variety of different error conditions and kernel panics,
such as this one (trimmed):
rtw_pci 0000:02:00.0: pci bus timeout, check dma status
skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415 head:00000000d2880c6f data:000000007a02b1ea tail:0x1df end:0xc0 dev:<NULL>
------------[ cut here ]------------
kernel BUG at net/core/skbuff.c:105!
invalid opcode: 0000 [#1] SMP NOPTI
RIP: 0010:skb_panic+0x43/0x45
When skb allocation fails and the "rx routine starvation" is hit, the
function returns immediately without updating the RX ring. At this
point, the RX ring may continue referencing an old skb which was already
handed off to ieee80211_rx_irqsafe(). When it comes to be used again,
bad things happen.
This patch allocates a new, data-sized skb first in RX ISR. After
copying the data in, we pass it to the upper layers. However, if skb
allocation fails, we effectively drop the frame. In both cases, the
original, full size ring skb is reused.
In addition, to fixing the kernel crash, the RX routine should now
generally behave better under low memory conditions.
Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053
Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
Cc: <stable@vger.kernel.org>
---
drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++-------------
1 file changed, 22 insertions(+), 27 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index cfe05ba7280d..c415f5e94fed 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
u32 pkt_offset;
u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
u32 buf_desc_sz = chip->rx_buf_desc_sz;
+ u32 new_len;
u8 *rx_desc;
dma_addr_t dma;
@@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
pkt_stat.shift;
- if (pkt_stat.is_c2h) {
- /* keep rx_desc, halmac needs it */
- skb_put(skb, pkt_stat.pkt_len + pkt_offset);
+ /* allocate a new skb for this frame,
+ * discard the frame if none available
+ */
+ new_len = pkt_stat.pkt_len + pkt_offset;
+ new = dev_alloc_skb(new_len);
+ if (WARN_ONCE(!new, "rx routine starvation\n"))
+ goto next_rp;
+
+ /* put the DMA data including rx_desc from phy to new skb */
+ skb_put_data(new, skb->data, new_len);
- /* pass offset for further operation */
- *((u32 *)skb->cb) = pkt_offset;
- skb_queue_tail(&rtwdev->c2h_queue, skb);
+ if (pkt_stat.is_c2h) {
+ /* pass rx_desc & offset for further operation */
+ *((u32 *)new->cb) = pkt_offset;
+ skb_queue_tail(&rtwdev->c2h_queue, new);
ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
} else {
- /* remove rx_desc, maybe use skb_pull? */
- skb_put(skb, pkt_stat.pkt_len);
- skb_reserve(skb, pkt_offset);
-
- /* alloc a smaller skb to mac80211 */
- new = dev_alloc_skb(pkt_stat.pkt_len);
- if (!new) {
- new = skb;
- } else {
- skb_put_data(new, skb->data, skb->len);
- dev_kfree_skb_any(skb);
- }
- /* TODO: merge into rx.c */
- rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
+ /* remove rx_desc */
+ skb_pull(new, pkt_offset);
+
+ rtw_rx_stats(rtwdev, pkt_stat.vif, new);
memcpy(new->cb, &rx_status, sizeof(rx_status));
ieee80211_rx_irqsafe(rtwdev->hw, new);
}
- /* skb delivered to mac80211, alloc a new one in rx ring */
- new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
- if (WARN(!new, "rx routine starvation\n"))
- return;
-
- ring->buf[cur_rp] = new;
- rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
+next_rp:
+ /* new skb delivered to mac80211, re-enable original skb DMA */
+ rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
/* host read next element in ring */
if (++cur_rp >= ring->r.len)
--
2.22.0
^ permalink raw reply related
* Re: [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: Jian-Hong Pan @ 2019-07-11 3:50 UTC (permalink / raw)
To: David Laight
Cc: Yan-Hsuan Chuang, Kalle Valo, David S . Miller, Larry Finger,
Christoph Hellwig, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
linux@endlessm.com, Daniel Drake, stable@vger.kernel.org
In-Reply-To: <81a2b91c4b084617bab8656fca932f6d@AcuMS.aculab.com>
David Laight <David.Laight@aculab.com> 於 2019年7月10日 週三 下午4:57寫道：
>
> From: Jian-Hong Pan
> > Sent: 10 July 2019 09:38
> >
> > Testing with RTL8822BE hardware, when available memory is low, we
> > frequently see a kernel panic and system freeze.
> >
> > First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed):
> >
> > rx routine starvation
> > WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822
> > rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> > [ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> >
> > Then we see a variety of different error conditions and kernel panics,
> > such as this one (trimmed):
> >
> > rtw_pci 0000:02:00.0: pci bus timeout, check dma status
> > skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415 head:00000000d2880c6f
> > data:000000007a02b1ea tail:0x1df end:0xc0 dev:<NULL>
> > ------------[ cut here ]------------
> > kernel BUG at net/core/skbuff.c:105!
> > invalid opcode: 0000 [#1] SMP NOPTI
> > RIP: 0010:skb_panic+0x43/0x45
> >
> > When skb allocation fails and the "rx routine starvation" is hit, the
> > function returns immediately without updating the RX ring. At this
> > point, the RX ring may continue referencing an old skb which was already
> > handed off to ieee80211_rx_irqsafe(). When it comes to be used again,
> > bad things happen.
> >
> > This patch allocates a new, data-sized skb first in RX ISR. After
> > copying the data in, we pass it to the upper layers. However, if skb
> > allocation fails, we effectively drop the frame. In both cases, the
> > original, full size ring skb is reused.
> >
> > In addition, by fixing the kernel crash, the RX routine should now
> > generally behave better under low memory conditions.
>
> A couple of minor nits (see below).
> You may want to do a followup patch that changes the rx buffers
> (used by the hardware) to by just memory buffers.
> Nothing (probably) relies on them being skb with all the accociated
> baggage.
It is a good idea for later commit.
> David
>
> >
> > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053
> > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> > v2:
> > - Allocate new data-sized skb and put data into it, then pass it to
> > mac80211. Reuse the original skb in RX ring by DMA sync.
> > - Modify the commit message.
> > - Introduce following [PATCH v3 2/2] rtw88: pci: Use DMA sync instead
> > of remapping in RX ISR.
> >
> > v3:
> > - Same as v2.
> >
> > drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++-------------
> > 1 file changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> > index cfe05ba7280d..e9fe3ad896c8 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> > u32 pkt_offset;
> > u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> > u32 buf_desc_sz = chip->rx_buf_desc_sz;
> > + u32 new_len;
> > u8 *rx_desc;
> > dma_addr_t dma;
> >
> > @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> > pkt_stat.shift;
> >
> > - if (pkt_stat.is_c2h) {
> > - /* keep rx_desc, halmac needs it */
> > - skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> > + /* discard current skb if the new skb cannot be allocated as a
> > + * new one in rx ring later
> > + */
>
> That comment isn't quite right.
> maybe: "Allocate a new skb for this frame, discard if none available"
Thanks! I will tweak it.
> > + new_len = pkt_stat.pkt_len + pkt_offset;
> > + new = dev_alloc_skb(new_len);
> > + if (WARN_ONCE(!new, "rx routine starvation\n"))
>
> I think you should count these??
Larry has a different idea here. [1]
I agree with Larry that just need to know not enough memory here.
[1] https://lkml.org/lkml/2019/7/8/1049
Jian-Hong Pan
> > + goto next_rp;
> > +
> > + /* put the DMA data including rx_desc from phy to new skb */
> > + skb_put_data(new, skb->data, new_len);
> >
> > - /* pass offset for further operation */
> > - *((u32 *)skb->cb) = pkt_offset;
> > - skb_queue_tail(&rtwdev->c2h_queue, skb);
> > + if (pkt_stat.is_c2h) {
> > + /* pass rx_desc & offset for further operation */
> > + *((u32 *)new->cb) = pkt_offset;
> > + skb_queue_tail(&rtwdev->c2h_queue, new);
> > ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> > } else {
> > - /* remove rx_desc, maybe use skb_pull? */
> > - skb_put(skb, pkt_stat.pkt_len);
> > - skb_reserve(skb, pkt_offset);
> > -
> > - /* alloc a smaller skb to mac80211 */
> > - new = dev_alloc_skb(pkt_stat.pkt_len);
> > - if (!new) {
> > - new = skb;
> > - } else {
> > - skb_put_data(new, skb->data, skb->len);
> > - dev_kfree_skb_any(skb);
> > - }
> > - /* TODO: merge into rx.c */
> > - rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> > + /* remove rx_desc */
> > + skb_pull(new, pkt_offset);
> > +
> > + rtw_rx_stats(rtwdev, pkt_stat.vif, new);
> > memcpy(new->cb, &rx_status, sizeof(rx_status));
> > ieee80211_rx_irqsafe(rtwdev->hw, new);
> > }
> >
> > - /* skb delivered to mac80211, alloc a new one in rx ring */
> > - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> > - if (WARN(!new, "rx routine starvation\n"))
> > - return;
> > -
> > - ring->buf[cur_rp] = new;
> > - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
> > +next_rp:
> > + /* new skb delivered to mac80211, re-enable original skb DMA */
> > + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
> >
> > /* host read next element in ring */
> > if (++cur_rp >= ring->r.len)
> > --
> > 2.22.0
>
> -
> Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
> Registration No: 1397386 (Wales)
>
^ permalink raw reply
* Investment Transaction.......
From: Phil Moore @ 2019-07-08 23:53 UTC (permalink / raw)
To: linux-wireless
Greetings,
I have a business transaction i would like to discuss with you. contact me on skylinefin010@gmail.com for more details.
Yours Truly,
Phil Moore
^ permalink raw reply
* Re: [PATCH AUTOSEL 4.19 14/60] mwifiex: Abort at too short BSS descriptor element
From: Brian Norris @ 2019-07-10 21:12 UTC (permalink / raw)
To: Sasha Levin
Cc: Linux Kernel, stable, Takashi Iwai, Kalle Valo, linux-wireless,
<netdev@vger.kernel.org>
In-Reply-To: <20190710145112.GX10104@sasha-vm>
On Wed, Jul 10, 2019 at 7:51 AM Sasha Levin <sashal@kernel.org> wrote:
> I see that 63d7ef36103d didn't make it into 5.2, so I'll just drop this
> for now.
Yeah, I think it's stuck at net/master. Presumably it'll get into
5.3-rc somewhere.
Brian
^ permalink raw reply
* [RFC PATCH v3 1/2] cfg80211: refactor cfg80211_bss_update
From: Sergey Matyukevich @ 2019-07-10 17:37 UTC (permalink / raw)
To: linux-wireless@vger.kernel.org
Cc: Johannes Berg, Igor Mitsyanko, Mikhail Karpenko,
Sergey Matyukevich
In-Reply-To: <20190710173651.15770-1-sergey.matyukevich.os@quantenna.com>
This patch implements minor refactoring for cfg80211_bss_update function.
Code path for updating known BSS is extracted into dedicated
cfg80211_update_known_bss function.
Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
net/wireless/scan.c | 171 +++++++++++++++++++++++++++-------------------------
1 file changed, 89 insertions(+), 82 deletions(-)
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index d66e6d4b7555..9f21162f05e9 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1091,6 +1091,93 @@ struct cfg80211_non_tx_bss {
u8 bssid_index;
};
+static bool
+cfg80211_update_known_bss(struct cfg80211_registered_device *rdev,
+ struct cfg80211_internal_bss *known,
+ struct cfg80211_internal_bss *new,
+ bool signal_valid)
+{
+ lockdep_assert_held(&rdev->bss_lock);
+
+ /* Update IEs */
+ if (rcu_access_pointer(new->pub.proberesp_ies)) {
+ const struct cfg80211_bss_ies *old;
+
+ old = rcu_access_pointer(known->pub.proberesp_ies);
+
+ rcu_assign_pointer(known->pub.proberesp_ies,
+ new->pub.proberesp_ies);
+ /* Override possible earlier Beacon frame IEs */
+ rcu_assign_pointer(known->pub.ies,
+ new->pub.proberesp_ies);
+ if (old)
+ kfree_rcu((struct cfg80211_bss_ies *)old, rcu_head);
+ } else if (rcu_access_pointer(new->pub.beacon_ies)) {
+ const struct cfg80211_bss_ies *old;
+ struct cfg80211_internal_bss *bss;
+
+ if (known->pub.hidden_beacon_bss &&
+ !list_empty(&known->hidden_list)) {
+ const struct cfg80211_bss_ies *f;
+
+ /* The known BSS struct is one of the probe
+ * response members of a group, but we're
+ * receiving a beacon (beacon_ies in the new
+ * bss is used). This can only mean that the
+ * AP changed its beacon from not having an
+ * SSID to showing it, which is confusing so
+ * drop this information.
+ */
+
+ f = rcu_access_pointer(new->pub.beacon_ies);
+ kfree_rcu((struct cfg80211_bss_ies *)f, rcu_head);
+ return false;
+ }
+
+ old = rcu_access_pointer(known->pub.beacon_ies);
+
+ rcu_assign_pointer(known->pub.beacon_ies, new->pub.beacon_ies);
+
+ /* Override IEs if they were from a beacon before */
+ if (old == rcu_access_pointer(known->pub.ies))
+ rcu_assign_pointer(known->pub.ies, new->pub.beacon_ies);
+
+ /* Assign beacon IEs to all sub entries */
+ list_for_each_entry(bss, &known->hidden_list, hidden_list) {
+ const struct cfg80211_bss_ies *ies;
+
+ ies = rcu_access_pointer(bss->pub.beacon_ies);
+ WARN_ON(ies != old);
+
+ rcu_assign_pointer(bss->pub.beacon_ies,
+ new->pub.beacon_ies);
+ }
+
+ if (old)
+ kfree_rcu((struct cfg80211_bss_ies *)old, rcu_head);
+ }
+
+ known->pub.beacon_interval = new->pub.beacon_interval;
+
+ /* don't update the signal if beacon was heard on
+ * adjacent channel.
+ */
+ if (signal_valid)
+ known->pub.signal = new->pub.signal;
+ known->pub.capability = new->pub.capability;
+ known->ts = new->ts;
+ known->ts_boottime = new->ts_boottime;
+ known->parent_tsf = new->parent_tsf;
+ known->pub.chains = new->pub.chains;
+ memcpy(known->pub.chain_signal, new->pub.chain_signal,
+ IEEE80211_MAX_CHAINS);
+ ether_addr_copy(known->parent_bssid, new->parent_bssid);
+ known->pub.max_bssid_indicator = new->pub.max_bssid_indicator;
+ known->pub.bssid_index = new->pub.bssid_index;
+
+ return true;
+}
+
/* Returned bss is reference counted and must be cleaned up appropriately. */
struct cfg80211_internal_bss *
cfg80211_bss_update(struct cfg80211_registered_device *rdev,
@@ -1114,88 +1201,8 @@ cfg80211_bss_update(struct cfg80211_registered_device *rdev,
found = rb_find_bss(rdev, tmp, BSS_CMP_REGULAR);
if (found) {
- /* Update IEs */
- if (rcu_access_pointer(tmp->pub.proberesp_ies)) {
- const struct cfg80211_bss_ies *old;
-
- old = rcu_access_pointer(found->pub.proberesp_ies);
-
- rcu_assign_pointer(found->pub.proberesp_ies,
- tmp->pub.proberesp_ies);
- /* Override possible earlier Beacon frame IEs */
- rcu_assign_pointer(found->pub.ies,
- tmp->pub.proberesp_ies);
- if (old)
- kfree_rcu((struct cfg80211_bss_ies *)old,
- rcu_head);
- } else if (rcu_access_pointer(tmp->pub.beacon_ies)) {
- const struct cfg80211_bss_ies *old;
- struct cfg80211_internal_bss *bss;
-
- if (found->pub.hidden_beacon_bss &&
- !list_empty(&found->hidden_list)) {
- const struct cfg80211_bss_ies *f;
-
- /*
- * The found BSS struct is one of the probe
- * response members of a group, but we're
- * receiving a beacon (beacon_ies in the tmp
- * bss is used). This can only mean that the
- * AP changed its beacon from not having an
- * SSID to showing it, which is confusing so
- * drop this information.
- */
-
- f = rcu_access_pointer(tmp->pub.beacon_ies);
- kfree_rcu((struct cfg80211_bss_ies *)f,
- rcu_head);
- goto drop;
- }
-
- old = rcu_access_pointer(found->pub.beacon_ies);
-
- rcu_assign_pointer(found->pub.beacon_ies,
- tmp->pub.beacon_ies);
-
- /* Override IEs if they were from a beacon before */
- if (old == rcu_access_pointer(found->pub.ies))
- rcu_assign_pointer(found->pub.ies,
- tmp->pub.beacon_ies);
-
- /* Assign beacon IEs to all sub entries */
- list_for_each_entry(bss, &found->hidden_list,
- hidden_list) {
- const struct cfg80211_bss_ies *ies;
-
- ies = rcu_access_pointer(bss->pub.beacon_ies);
- WARN_ON(ies != old);
-
- rcu_assign_pointer(bss->pub.beacon_ies,
- tmp->pub.beacon_ies);
- }
-
- if (old)
- kfree_rcu((struct cfg80211_bss_ies *)old,
- rcu_head);
- }
-
- found->pub.beacon_interval = tmp->pub.beacon_interval;
- /*
- * don't update the signal if beacon was heard on
- * adjacent channel.
- */
- if (signal_valid)
- found->pub.signal = tmp->pub.signal;
- found->pub.capability = tmp->pub.capability;
- found->ts = tmp->ts;
- found->ts_boottime = tmp->ts_boottime;
- found->parent_tsf = tmp->parent_tsf;
- found->pub.chains = tmp->pub.chains;
- memcpy(found->pub.chain_signal, tmp->pub.chain_signal,
- IEEE80211_MAX_CHAINS);
- ether_addr_copy(found->parent_bssid, tmp->parent_bssid);
- found->pub.max_bssid_indicator = tmp->pub.max_bssid_indicator;
- found->pub.bssid_index = tmp->pub.bssid_index;
+ if (!cfg80211_update_known_bss(rdev, found, tmp, signal_valid))
+ goto drop;
} else {
struct cfg80211_internal_bss *new;
struct cfg80211_internal_bss *hidden;
--
2.11.0
^ permalink raw reply related
* [RFC PATCH v3 2/2] cfg80211: fix duplicated scan entries after channel switch
From: Sergey Matyukevich @ 2019-07-10 17:37 UTC (permalink / raw)
To: linux-wireless@vger.kernel.org
Cc: Johannes Berg, Igor Mitsyanko, Mikhail Karpenko,
Sergey Matyukevich
In-Reply-To: <20190710173651.15770-1-sergey.matyukevich.os@quantenna.com>
When associated BSS completes channel switch procedure, its channel record
needs to be updated. The existing mac80211 solution was extended to
cfg80211 in commit 5dc8cdce1d72 ("mac80211/cfg80211: update bss
channel on channel switch")
However that solution still appears to be incomplete as it may lead
to duplicated scan entries for associated BSS after channel switch.
The root cause of the problem is as follows. Each BSS entry is
included into the following data structures:
- bss list rdev->bss_list
- bss search tree rdev->bss_tree
Updating BSS channel record without rebuilding bss_tree may break
tree search since cmp_bss considers all of the following: channel,
bssid, ssid. When BSS channel is updated, but its location in bss_tree
is not updated, then subsequent search operations may fail to locate
this BSS since they will be traversing bss_tree in wrong direction.
As a result, for scan performed after associated BSS channel switch,
cfg80211_bss_update may add the second entry for the same BSS to both
bss_list and bss_tree, rather then update the existing one.
To summarize, if BSS channel needs to be updated, then bss_tree should
be rebuilt in order to put updated BSS entry into a proper location.
This commit suggests the following straightforward solution:
- if new entry has been already created for BSS after channel switch,
then use its IEs to update known BSS entry and then remove new
entry completely
- use rb_erase/rb_insert_bss reinstall updated BSS in bss_tree
- update channel and location in rb-tree for non-transmitting bss entries
Signed-off-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
---
Cover email is not attached to patchwork, so duplicate it here as well...
Suggested approach to handle non-transmitting BSS entries is simplified in the
following sense. If new entries have been already created after channel switch,
only transmitting bss will be updated using IEs of new entry for the same
transmitting bss. Non-transmitting bss entries will be updated as soon as
new mgmt frames are received. Updating non-transmitting bss entries seems
too expensive: nested nontrans_list traversing is needed since we can not
rely on the same order of old and new non-transmitting entries.
Basic use-case tested using both iwlwifi and qtnfmac. However multi-BSSID
support has not yet been tested.
v1 -> v2
- use IEs of new BSS entry to update known BSS entry
for this purpose extract BSS update code from cfg80211_bss_update
into a separate function cfg80211_update_known_bss
v2 -> v3
- minor cleanup according to review comments
- split cfg80211_update_known_bss function into a separate patch
- update channel and location in rb-tree for nontransmit bss entries
Regards,
Sergey
---
net/wireless/core.h | 2 ++
net/wireless/nl80211.c | 2 +-
net/wireless/scan.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 82 insertions(+), 1 deletion(-)
diff --git a/net/wireless/core.h b/net/wireless/core.h
index ee8388fe4a92..77556c58d9ac 100644
--- a/net/wireless/core.h
+++ b/net/wireless/core.h
@@ -306,6 +306,8 @@ void ieee80211_set_bitrate_flags(struct wiphy *wiphy);
void cfg80211_bss_expire(struct cfg80211_registered_device *rdev);
void cfg80211_bss_age(struct cfg80211_registered_device *rdev,
unsigned long age_secs);
+void cfg80211_update_assoc_bss_entry(struct wireless_dev *wdev,
+ struct ieee80211_channel *channel);
/* IBSS */
int __cfg80211_join_ibss(struct cfg80211_registered_device *rdev,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index fc83dd179c1a..6ebb427883d0 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -16092,7 +16092,7 @@ void cfg80211_ch_switch_notify(struct net_device *dev,
if (wdev->iftype == NL80211_IFTYPE_STATION &&
!WARN_ON(!wdev->current_bss))
- wdev->current_bss->pub.channel = chandef->chan;
+ cfg80211_update_assoc_bss_entry(wdev, chandef->chan);
nl80211_ch_switch_notify(rdev, dev, chandef, GFP_KERNEL,
NL80211_CMD_CH_SWITCH_NOTIFY, 0);
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index 9f21162f05e9..30932b955ebc 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -2002,6 +2002,85 @@ void cfg80211_bss_iter(struct wiphy *wiphy,
}
EXPORT_SYMBOL(cfg80211_bss_iter);
+void cfg80211_update_assoc_bss_entry(struct wireless_dev *wdev,
+ struct ieee80211_channel *chan)
+{
+ struct wiphy *wiphy = wdev->wiphy;
+ struct cfg80211_registered_device *rdev = wiphy_to_rdev(wiphy);
+ struct cfg80211_internal_bss *cbss = wdev->current_bss;
+ struct cfg80211_internal_bss *new = NULL;
+ struct cfg80211_internal_bss *bss;
+ struct cfg80211_bss *nontrans_bss;
+ struct cfg80211_bss *tmp;
+
+ spin_lock_bh(&rdev->bss_lock);
+
+ if (WARN_ON(cbss->pub.channel == chan))
+ goto done;
+
+ /* use transmitting bss */
+ if (cbss->pub.transmitted_bss)
+ cbss = container_of(cbss->pub.transmitted_bss,
+ struct cfg80211_internal_bss,
+ pub);
+
+ cbss->pub.channel = chan;
+
+ list_for_each_entry(bss, &rdev->bss_list, list) {
+ if (!cfg80211_bss_type_match(bss->pub.capability,
+ bss->pub.channel->band,
+ wdev->conn_bss_type))
+ continue;
+
+ if (bss == cbss)
+ continue;
+
+ if (!cmp_bss(&bss->pub, &cbss->pub, BSS_CMP_REGULAR)) {
+ new = bss;
+ break;
+ }
+ }
+
+ if (new) {
+ /* to save time, update IEs for trasmitted bss only */
+ if (cfg80211_update_known_bss(rdev, cbss, new, false)) {
+ new->pub.proberesp_ies = NULL;
+ new->pub.beacon_ies = NULL;
+ }
+
+ list_for_each_entry_safe(nontrans_bss, tmp,
+ &new->pub.nontrans_list,
+ nontrans_list) {
+ bss = container_of(nontrans_bss,
+ struct cfg80211_internal_bss, pub);
+ if (__cfg80211_unlink_bss(rdev, bss))
+ rdev->bss_generation++;
+ }
+
+ WARN_ON(atomic_read(&new->hold));
+ if (!WARN_ON(!__cfg80211_unlink_bss(rdev, new)))
+ rdev->bss_generation++;
+ }
+
+ rb_erase(&cbss->rbn, &rdev->bss_tree);
+ rb_insert_bss(rdev, cbss);
+ rdev->bss_generation++;
+
+ list_for_each_entry_safe(nontrans_bss, tmp,
+ &cbss->pub.nontrans_list,
+ nontrans_list) {
+ bss = container_of(nontrans_bss,
+ struct cfg80211_internal_bss, pub);
+ bss->pub.channel = chan;
+ rb_erase(&bss->rbn, &rdev->bss_tree);
+ rb_insert_bss(rdev, bss);
+ rdev->bss_generation++;
+ }
+
+done:
+ spin_unlock_bh(&rdev->bss_lock);
+}
+
#ifdef CONFIG_CFG80211_WEXT
static struct cfg80211_registered_device *
cfg80211_get_dev_from_ifindex(struct net *net, int ifindex)
--
2.11.0
^ permalink raw reply related
* [RFC PATCH v3 0/2] cfg80211: fix duplicated scan entries after channel switch
From: Sergey Matyukevich @ 2019-07-10 17:36 UTC (permalink / raw)
To: linux-wireless@vger.kernel.org
Cc: Johannes Berg, Igor Mitsyanko, Mikhail Karpenko,
Sergey Matyukevich
Hi Johannes and all,
This is v3 of RFC patch aimed at fixing duplicated scan entries after channel
switch. The major change is updating non-transmitting bss entries. Since such
a bss cannot change channel without its transmitting bss (and vice versa),
the whole hierarchy of transmitting bss is updated, including channel and
location in rb-tree.
Suggested approach to handle non-transmitting BSS entries is simplified in the
following sense. If new entries have been already created after channel switch,
only transmitting bss will be updated using IEs of new entry for the same
transmitting bss. Non-transmitting bss entries will be updated as soon as
new mgmt frames are received. Updating non-transmitting bss entries seems
too expensive: nested nontrans_list traversing is needed since we can not
rely on the same order of old and new non-transmitting entries.
Basic use-case tested using both iwlwifi and qtnfmac.
However multi-BSSID support has not yet been tested.
Regards,
Sergey
v1 -> v2
- use IEs of new BSS entry to update known BSS entry
for this purpose extract BSS update code from cfg80211_bss_update
into a separate function cfg80211_update_known_bss
v2 -> v3
- minor cleanup according to review comments
- split cfg80211_update_known_bss function into a separate patch
- update channel and location in rb-tree for nontransmit bss entries
Sergey Matyukevich (2):
cfg80211: refactor cfg80211_bss_update
cfg80211: fix duplicated scan entries after channel switch
net/wireless/core.h | 2 +
net/wireless/nl80211.c | 2 +-
net/wireless/scan.c | 250 +++++++++++++++++++++++++++++++++----------------
3 files changed, 171 insertions(+), 83 deletions(-)
--
2.11.0
^ permalink raw reply
* Re: [PATCH 00/12] treewide: Fix GENMASK misuses
From: Joe Perches @ 2019-07-10 16:01 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Johannes Berg
Cc: Andrew Morton, Patrick Venture, Nancy Yuen, Benjamin Fair,
Andrew Jeffery, openbmc, linux-kernel, linux-aspeed,
linux-arm-kernel, linux-amlogic, netdev, linux-mediatek,
linux-stm32, linux-wireless, linux-media, linux-iio, devel,
alsa-devel, linux-mmc, dri-devel
In-Reply-To: <b9c3b83c9be50286062ae8cefd5d38e2baa0fb22.camel@perches.com>
On Wed, 2019-07-10 at 08:45 -0700, Joe Perches wrote:
> On Wed, 2019-07-10 at 10:43 +0100, Russell King - ARM Linux admin wrote:
> > On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote:
> > > On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote:
> > > > These GENMASK uses are inverted argument order and the
> > > > actual masks produced are incorrect. Fix them.
> > > >
> > > > Add checkpatch tests to help avoid more misuses too.
> > > >
> > > > Joe Perches (12):
> > > > checkpatch: Add GENMASK tests
> > >
> > > IMHO this doesn't make a lot of sense as a checkpatch test - just throw
> > > in a BUILD_BUG_ON()?
>
> I tried that.
>
> It'd can't be done as it's used in declarations
> and included in asm files and it uses the UL()
> macro.
>
> I also tried just making it do the right thing
> whatever the argument order.
I forgot.
I also made all those arguments when it was
introduced in 2013.
https://lore.kernel.org/patchwork/patch/414248/
> Oh well.
yeah.
^ permalink raw reply
* Re: [PATCH 00/12] treewide: Fix GENMASK misuses
From: Joe Perches @ 2019-07-10 15:45 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Johannes Berg
Cc: Andrew Morton, Patrick Venture, Nancy Yuen, Benjamin Fair,
Andrew Jeffery, openbmc, linux-kernel, linux-aspeed,
linux-arm-kernel, linux-amlogic, netdev, linux-mediatek,
linux-stm32, linux-wireless, linux-media, linux-iio, devel,
alsa-devel, linux-mmc, dri-devel
In-Reply-To: <20190710094337.wf2lftxzfjq2etro@shell.armlinux.org.uk>
On Wed, 2019-07-10 at 10:43 +0100, Russell King - ARM Linux admin wrote:
> On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote:
> > On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote:
> > > These GENMASK uses are inverted argument order and the
> > > actual masks produced are incorrect. Fix them.
> > >
> > > Add checkpatch tests to help avoid more misuses too.
> > >
> > > Joe Perches (12):
> > > checkpatch: Add GENMASK tests
> >
> > IMHO this doesn't make a lot of sense as a checkpatch test - just throw
> > in a BUILD_BUG_ON()?
I tried that.
It'd can't be done as it's used in declarations
and included in asm files and it uses the UL()
macro.
I also tried just making it do the right thing
whatever the argument order.
Oh well.
> My personal take on this is that GENMASK() is really not useful, it's
> just pure obfuscation and leads to exactly these kinds of mistakes.
>
> Yes, I fully understand the argument that you can just specify the
> start and end bits, and it _in theory_ makes the code more readable.
>
> However, the problem is when writing code. GENMASK(a, b). Is a the
> starting bit or ending bit? Is b the number of bits? It's confusing
> and causes mistakes resulting in incorrect code. A BUILD_BUG_ON()
> can catch some of the cases, but not all of them.
It's a horrid little macro and I agree with Russell.
I also think if it existed at all it should have been
GENMASK(low, high) not GENMASK(high, low).
I
^ permalink raw reply
* Re: [PATCH AUTOSEL 4.19 14/60] mwifiex: Abort at too short BSS descriptor element
From: Sasha Levin @ 2019-07-10 14:51 UTC (permalink / raw)
To: Brian Norris
Cc: Linux Kernel, stable, Takashi Iwai, Kalle Valo, linux-wireless,
<netdev@vger.kernel.org>
In-Reply-To: <CA+ASDXPyGECiq9gZmFj8TU6Gmt2epQtuBqnGqRWad79DJT589w@mail.gmail.com>
On Fri, Jun 28, 2019 at 03:58:49PM -0700, Brian Norris wrote:
>On Wed, Jun 26, 2019 at 5:49 PM Sasha Levin <sashal@kernel.org> wrote:
>>
>> From: Takashi Iwai <tiwai@suse.de>
>>
>> [ Upstream commit 685c9b7750bfacd6fc1db50d86579980593b7869 ]
>>
>> Currently mwifiex_update_bss_desc_with_ie() implicitly assumes that
>> the source descriptor entries contain the enough size for each type
>> and performs copying without checking the source size. This may lead
>> to read over boundary.
>>
>> Fix this by putting the source size check in appropriate places.
>>
>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
>> Signed-off-by: Sasha Levin <sashal@kernel.org>
>
>For the record, this fixup is still aiming for 5.2, correcting some
>potential mistakes in this patch:
>
>63d7ef36103d mwifiex: Don't abort on small, spec-compliant vendor IEs
>
>So you might want to hold off a bit, and grab them both.
I see that 63d7ef36103d didn't make it into 5.2, so I'll just drop this
for now.
--
Thanks,
Sasha
^ permalink raw reply
* [PATCH] libertas: Add missing sentinel at end of if_usb.c fw_table
From: Kevin Easton @ 2019-07-10 13:31 UTC (permalink / raw)
To: linux-wireless
Cc: andreyknvl, davem, kvalo, libertas-dev, linux-kernel, syzbot,
netdev, syzkaller-bugs
This sentinel tells the firmware loading process when to stop.
Reported-and-tested-by: syzbot+98156c174c5a2cad9f8f@syzkaller.appspotmail.com
Signed-off-by: Kevin Easton <kevin@guarana.org>
---
drivers/net/wireless/marvell/libertas/if_usb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/wireless/marvell/libertas/if_usb.c
index f1622f0ff8c9..fe3142d85d1e 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -50,7 +50,8 @@ static const struct lbs_fw_table fw_table[] = {
{ MODEL_8388, "libertas/usb8388_v5.bin", NULL },
{ MODEL_8388, "libertas/usb8388.bin", NULL },
{ MODEL_8388, "usb8388.bin", NULL },
- { MODEL_8682, "libertas/usb8682.bin", NULL }
+ { MODEL_8682, "libertas/usb8682.bin", NULL },
+ { 0, NULL, NULL }
};
static const struct usb_device_id if_usb_table[] = {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 00/12] treewide: Fix GENMASK misuses
From: Russell King - ARM Linux admin @ 2019-07-10 9:43 UTC (permalink / raw)
To: Johannes Berg
Cc: Joe Perches, Andrew Morton, Patrick Venture, Nancy Yuen,
Benjamin Fair, Andrew Jeffery, openbmc, linux-kernel,
linux-aspeed, linux-arm-kernel, linux-amlogic, netdev,
linux-mediatek, linux-stm32, linux-wireless, linux-media,
linux-iio, devel, alsa-devel, linux-mmc, dri-devel
In-Reply-To: <5fa1fa6998332642c49e2d5209193ffe2713f333.camel@sipsolutions.net>
On Wed, Jul 10, 2019 at 11:17:31AM +0200, Johannes Berg wrote:
> On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote:
> > These GENMASK uses are inverted argument order and the
> > actual masks produced are incorrect. Fix them.
> >
> > Add checkpatch tests to help avoid more misuses too.
> >
> > Joe Perches (12):
> > checkpatch: Add GENMASK tests
>
> IMHO this doesn't make a lot of sense as a checkpatch test - just throw
> in a BUILD_BUG_ON()?
My personal take on this is that GENMASK() is really not useful, it's
just pure obfuscation and leads to exactly these kinds of mistakes.
Yes, I fully understand the argument that you can just specify the
start and end bits, and it _in theory_ makes the code more readable.
However, the problem is when writing code. GENMASK(a, b). Is a the
starting bit or ending bit? Is b the number of bits? It's confusing
and causes mistakes resulting in incorrect code. A BUILD_BUG_ON()
can catch some of the cases, but not all of them.
For example:
GENMASK(6, 2)
would satisify the requirement that a > b, so a BUILD_BUG_ON() will
not trigger, but was the author meaning 0x3c or 0xc0?
Personally, I've decided I am _not_ going to use GENMASK() in my code
because I struggle to get the macro arguments correct - I'm _much_
happier, and it is way more reliable for me to write the mask in hex
notation.
I think this is where use of a ternary operator would come in use. The
normal way of writing a number of bits tends to be "a:b", so if GENMASK
took something like GENMASK(6:2), then I'd have less issue with it,
because it's argument is then in a familiar notation.
Yes, I'm sure that someone will point out that the GENMASK arguments
are just in the same order, but that doesn't prevent _me_ frequently
getting it wrong - and that's the point. The macro seems to me to
cause more problems than it solves.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH 00/12] treewide: Fix GENMASK misuses
From: Johannes Berg @ 2019-07-10 9:17 UTC (permalink / raw)
To: Joe Perches, Andrew Morton, Patrick Venture, Nancy Yuen,
Benjamin Fair, Andrew Jeffery, openbmc, linux-kernel,
linux-aspeed, linux-arm-kernel, linux-amlogic, netdev,
linux-mediatek, linux-stm32, linux-wireless, linux-media
Cc: dri-devel, linux-iio, linux-mmc, devel, alsa-devel
In-Reply-To: <cover.1562734889.git.joe@perches.com>
On Tue, 2019-07-09 at 22:04 -0700, Joe Perches wrote:
> These GENMASK uses are inverted argument order and the
> actual masks produced are incorrect. Fix them.
>
> Add checkpatch tests to help avoid more misuses too.
>
> Joe Perches (12):
> checkpatch: Add GENMASK tests
IMHO this doesn't make a lot of sense as a checkpatch test - just throw
in a BUILD_BUG_ON()?
johannes
^ permalink raw reply
* RE: [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: David Laight @ 2019-07-10 8:57 UTC (permalink / raw)
To: 'Jian-Hong Pan', Yan-Hsuan Chuang, Kalle Valo,
David S . Miller, Larry Finger, Christoph Hellwig
Cc: linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux@endlessm.com, Daniel Drake,
stable@vger.kernel.org
In-Reply-To: <20190710083825.7115-1-jian-hong@endlessm.com>
From: Jian-Hong Pan
> Sent: 10 July 2019 09:38
>
> Testing with RTL8822BE hardware, when available memory is low, we
> frequently see a kernel panic and system freeze.
>
> First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed):
>
> rx routine starvation
> WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822
> rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> [ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
>
> Then we see a variety of different error conditions and kernel panics,
> such as this one (trimmed):
>
> rtw_pci 0000:02:00.0: pci bus timeout, check dma status
> skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415 head:00000000d2880c6f
> data:000000007a02b1ea tail:0x1df end:0xc0 dev:<NULL>
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:105!
> invalid opcode: 0000 [#1] SMP NOPTI
> RIP: 0010:skb_panic+0x43/0x45
>
> When skb allocation fails and the "rx routine starvation" is hit, the
> function returns immediately without updating the RX ring. At this
> point, the RX ring may continue referencing an old skb which was already
> handed off to ieee80211_rx_irqsafe(). When it comes to be used again,
> bad things happen.
>
> This patch allocates a new, data-sized skb first in RX ISR. After
> copying the data in, we pass it to the upper layers. However, if skb
> allocation fails, we effectively drop the frame. In both cases, the
> original, full size ring skb is reused.
>
> In addition, by fixing the kernel crash, the RX routine should now
> generally behave better under low memory conditions.
A couple of minor nits (see below).
You may want to do a followup patch that changes the rx buffers
(used by the hardware) to by just memory buffers.
Nothing (probably) relies on them being skb with all the accociated
baggage.
David
>
> Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053
> Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> Cc: <stable@vger.kernel.org>
> ---
> v2:
> - Allocate new data-sized skb and put data into it, then pass it to
> mac80211. Reuse the original skb in RX ring by DMA sync.
> - Modify the commit message.
> - Introduce following [PATCH v3 2/2] rtw88: pci: Use DMA sync instead
> of remapping in RX ISR.
>
> v3:
> - Same as v2.
>
> drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++-------------
> 1 file changed, 22 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
> index cfe05ba7280d..e9fe3ad896c8 100644
> --- a/drivers/net/wireless/realtek/rtw88/pci.c
> +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> u32 pkt_offset;
> u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> u32 buf_desc_sz = chip->rx_buf_desc_sz;
> + u32 new_len;
> u8 *rx_desc;
> dma_addr_t dma;
>
> @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
> pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> pkt_stat.shift;
>
> - if (pkt_stat.is_c2h) {
> - /* keep rx_desc, halmac needs it */
> - skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> + /* discard current skb if the new skb cannot be allocated as a
> + * new one in rx ring later
> + */
That comment isn't quite right.
maybe: "Allocate a new skb for this frame, discard if none available"
> + new_len = pkt_stat.pkt_len + pkt_offset;
> + new = dev_alloc_skb(new_len);
> + if (WARN_ONCE(!new, "rx routine starvation\n"))
I think you should count these??
> + goto next_rp;
> +
> + /* put the DMA data including rx_desc from phy to new skb */
> + skb_put_data(new, skb->data, new_len);
>
> - /* pass offset for further operation */
> - *((u32 *)skb->cb) = pkt_offset;
> - skb_queue_tail(&rtwdev->c2h_queue, skb);
> + if (pkt_stat.is_c2h) {
> + /* pass rx_desc & offset for further operation */
> + *((u32 *)new->cb) = pkt_offset;
> + skb_queue_tail(&rtwdev->c2h_queue, new);
> ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> } else {
> - /* remove rx_desc, maybe use skb_pull? */
> - skb_put(skb, pkt_stat.pkt_len);
> - skb_reserve(skb, pkt_offset);
> -
> - /* alloc a smaller skb to mac80211 */
> - new = dev_alloc_skb(pkt_stat.pkt_len);
> - if (!new) {
> - new = skb;
> - } else {
> - skb_put_data(new, skb->data, skb->len);
> - dev_kfree_skb_any(skb);
> - }
> - /* TODO: merge into rx.c */
> - rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> + /* remove rx_desc */
> + skb_pull(new, pkt_offset);
> +
> + rtw_rx_stats(rtwdev, pkt_stat.vif, new);
> memcpy(new->cb, &rx_status, sizeof(rx_status));
> ieee80211_rx_irqsafe(rtwdev->hw, new);
> }
>
> - /* skb delivered to mac80211, alloc a new one in rx ring */
> - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> - if (WARN(!new, "rx routine starvation\n"))
> - return;
> -
> - ring->buf[cur_rp] = new;
> - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
> +next_rp:
> + /* new skb delivered to mac80211, re-enable original skb DMA */
> + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
>
> /* host read next element in ring */
> if (++cur_rp >= ring->r.len)
> --
> 2.22.0
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: Jian-Hong Pan @ 2019-07-10 8:54 UTC (permalink / raw)
To: Tony Chuang
Cc: Kalle Valo, David S . Miller, Larry Finger, David Laight,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux@endlessm.com, Daniel Drake,
stable@vger.kernel.org
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D1864779@RTITMBSVM04.realtek.com.tw>
Tony Chuang <yhchuang@realtek.com> 於 2019年7月10日 週三 下午4:37寫道：
>
> > Subject: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in
> > RX ISR
> >
> > Testing with RTL8822BE hardware, when available memory is low, we
> > frequently see a kernel panic and system freeze.
> >
> > First, rtw_pci_rx_isr encounters a memory allocation failure (trimmed):
> >
> > rx routine starvation
> > WARNING: CPU: 7 PID: 9871 at drivers/net/wireless/realtek/rtw88/pci.c:822
> > rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> > [ 2356.580313] RIP: 0010:rtw_pci_rx_isr.constprop.25+0x35a/0x370 [rtwpci]
> >
> > Then we see a variety of different error conditions and kernel panics,
> > such as this one (trimmed):
> >
> > rtw_pci 0000:02:00.0: pci bus timeout, check dma status
> > skbuff: skb_over_panic: text:00000000091b6e66 len:415 put:415
> > head:00000000d2880c6f data:000000007a02b1ea tail:0x1df end:0xc0
> > dev:<NULL>
> > ------------[ cut here ]------------
> > kernel BUG at net/core/skbuff.c:105!
> > invalid opcode: 0000 [#1] SMP NOPTI
> > RIP: 0010:skb_panic+0x43/0x45
> >
> > When skb allocation fails and the "rx routine starvation" is hit, the
> > function returns immediately without updating the RX ring. At this
> > point, the RX ring may continue referencing an old skb which was already
> > handed off to ieee80211_rx_irqsafe(). When it comes to be used again,
> > bad things happen.
> >
> > This patch allocates a new, data-sized skb first in RX ISR. After
> > copying the data in, we pass it to the upper layers. However, if skb
> > allocation fails, we effectively drop the frame. In both cases, the
> > original, full size ring skb is reused.
> >
> > In addition, to fixing the kernel crash, the RX routine should now
> > generally behave better under low memory conditions.
> >
> > Buglink: https://bugzilla.kernel.org/show_bug.cgi?id=204053
> > Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
> > Cc: <stable@vger.kernel.org>
> > ---
> > drivers/net/wireless/realtek/rtw88/pci.c | 49 +++++++++++-------------
> > 1 file changed, 22 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/net/wireless/realtek/rtw88/pci.c
> > b/drivers/net/wireless/realtek/rtw88/pci.c
> > index cfe05ba7280d..e9fe3ad896c8 100644
> > --- a/drivers/net/wireless/realtek/rtw88/pci.c
> > +++ b/drivers/net/wireless/realtek/rtw88/pci.c
> > @@ -763,6 +763,7 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev,
> > struct rtw_pci *rtwpci,
> > u32 pkt_offset;
> > u32 pkt_desc_sz = chip->rx_pkt_desc_sz;
> > u32 buf_desc_sz = chip->rx_buf_desc_sz;
> > + u32 new_len;
> > u8 *rx_desc;
> > dma_addr_t dma;
> >
> > @@ -790,40 +791,34 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev,
> > struct rtw_pci *rtwpci,
> > pkt_offset = pkt_desc_sz + pkt_stat.drv_info_sz +
> > pkt_stat.shift;
> >
> > - if (pkt_stat.is_c2h) {
> > - /* keep rx_desc, halmac needs it */
> > - skb_put(skb, pkt_stat.pkt_len + pkt_offset);
> > + /* discard current skb if the new skb cannot be allocated as a
> > + * new one in rx ring later
> > + */
> > + new_len = pkt_stat.pkt_len + pkt_offset;
> > + new = dev_alloc_skb(new_len);
> > + if (WARN_ONCE(!new, "rx routine starvation\n"))
> > + goto next_rp;
> > +
> > + /* put the DMA data including rx_desc from phy to new skb */
> > + skb_put_data(new, skb->data, new_len);
> >
> > - /* pass offset for further operation */
> > - *((u32 *)skb->cb) = pkt_offset;
> > - skb_queue_tail(&rtwdev->c2h_queue, skb);
> > + if (pkt_stat.is_c2h) {
> > + /* pass rx_desc & offset for further operation */
> > + *((u32 *)new->cb) = pkt_offset;
> > + skb_queue_tail(&rtwdev->c2h_queue, new);
> > ieee80211_queue_work(rtwdev->hw, &rtwdev->c2h_work);
> > } else {
> > - /* remove rx_desc, maybe use skb_pull? */
> > - skb_put(skb, pkt_stat.pkt_len);
> > - skb_reserve(skb, pkt_offset);
> > -
> > - /* alloc a smaller skb to mac80211 */
> > - new = dev_alloc_skb(pkt_stat.pkt_len);
> > - if (!new) {
> > - new = skb;
> > - } else {
> > - skb_put_data(new, skb->data, skb->len);
> > - dev_kfree_skb_any(skb);
> > - }
> > - /* TODO: merge into rx.c */
> > - rtw_rx_stats(rtwdev, pkt_stat.vif, skb);
> > + /* remove rx_desc */
> > + skb_pull(new, pkt_offset);
> > +
> > + rtw_rx_stats(rtwdev, pkt_stat.vif, new);
> > memcpy(new->cb, &rx_status, sizeof(rx_status));
> > ieee80211_rx_irqsafe(rtwdev->hw, new);
> > }
> >
> > - /* skb delivered to mac80211, alloc a new one in rx ring */
> > - new = dev_alloc_skb(RTK_PCI_RX_BUF_SIZE);
> > - if (WARN(!new, "rx routine starvation\n"))
> > - return;
> > -
> > - ring->buf[cur_rp] = new;
> > - rtw_pci_reset_rx_desc(rtwdev, new, ring, cur_rp, buf_desc_sz);
> > +next_rp:
> > + /* new skb delivered to mac80211, re-enable original skb DMA */
> > + rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
> >
> > /* host read next element in ring */
> > if (++cur_rp >= ring->r.len)
> > --
> > 2.22.0
>
> Now it looks good to me. Thanks.
>
> Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
>
> Yan-Hsuan
Uh! Thanks for your ack.
But I just sent version 3 patches (including [PATCH v3 2/2] rtw88:
pci: Use DMA sync instead of remapping in RX ISR) by following
Christoph's comment. [1]
Could you please also review the 2 patches of version 3? Thank you.
[1]: https://lkml.org/lkml/2019/7/9/507
Jian-Hong Pan
^ permalink raw reply
* [PATCH v3 2/2] rtw88: pci: Use DMA sync instead of remapping in RX ISR
From: Jian-Hong Pan @ 2019-07-10 8:38 UTC (permalink / raw)
To: Yan-Hsuan Chuang, Kalle Valo, David S . Miller, Larry Finger,
David Laight, Christoph Hellwig
Cc: linux-wireless, netdev, linux-kernel, linux, Daniel Drake,
Jian-Hong Pan, stable
In-Reply-To: <20190709161550.GA8703@infradead.org>
Since each skb in RX ring is reused instead of new allocation, we can
treat the DMA in a more efficient way by DMA synchronization.
Signed-off-by: Jian-Hong Pan <jian-hong@endlessm.com>
Cc: <stable@vger.kernel.org>
---
v2:
- New patch by following [PATCH v3 1/2] rtw88: pci: Rearrange the
memory usage for skb in RX ISR.
v3:
- Remove rtw_pci_sync_rx_desc_cpu and call dma_sync_single_for_cpu in
rtw_pci_rx_isr directly.
- Remove the return value of rtw_pci_sync_rx_desc_device.
- Use DMA_FROM_DEVICE instead of PCI_DMA_FROMDEVICE.
drivers/net/wireless/realtek/rtw88/pci.c | 24 +++++++++++++++++++++---
1 file changed, 21 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index e9fe3ad896c8..485d30c06935 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -206,6 +206,23 @@ static int rtw_pci_reset_rx_desc(struct rtw_dev *rtwdev, struct sk_buff *skb,
return 0;
}
+static void rtw_pci_sync_rx_desc_device(struct rtw_dev *rtwdev, dma_addr_t dma,
+ struct rtw_pci_rx_ring *rx_ring,
+ u32 idx, u32 desc_sz)
+{
+ struct device *dev = rtwdev->dev;
+ struct rtw_pci_rx_buffer_desc *buf_desc;
+ int buf_sz = RTK_PCI_RX_BUF_SIZE;
+
+ dma_sync_single_for_device(dev, dma, buf_sz, DMA_FROM_DEVICE);
+
+ buf_desc = (struct rtw_pci_rx_buffer_desc *)(rx_ring->r.head +
+ idx * desc_sz);
+ memset(buf_desc, 0, sizeof(*buf_desc));
+ buf_desc->buf_size = cpu_to_le16(RTK_PCI_RX_BUF_SIZE);
+ buf_desc->dma = cpu_to_le32(dma);
+}
+
static int rtw_pci_init_rx_ring(struct rtw_dev *rtwdev,
struct rtw_pci_rx_ring *rx_ring,
u8 desc_size, u32 len)
@@ -782,8 +799,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
rtw_pci_dma_check(rtwdev, ring, cur_rp);
skb = ring->buf[cur_rp];
dma = *((dma_addr_t *)skb->cb);
- pci_unmap_single(rtwpci->pdev, dma, RTK_PCI_RX_BUF_SIZE,
- PCI_DMA_FROMDEVICE);
+ dma_sync_single_for_cpu(rtwdev->dev, dma, RTK_PCI_RX_BUF_SIZE,
+ DMA_FROM_DEVICE);
rx_desc = skb->data;
chip->ops->query_rx_desc(rtwdev, rx_desc, &pkt_stat, &rx_status);
@@ -818,7 +835,8 @@ static void rtw_pci_rx_isr(struct rtw_dev *rtwdev, struct rtw_pci *rtwpci,
next_rp:
/* new skb delivered to mac80211, re-enable original skb DMA */
- rtw_pci_reset_rx_desc(rtwdev, skb, ring, cur_rp, buf_desc_sz);
+ rtw_pci_sync_rx_desc_device(rtwdev, dma, ring, cur_rp,
+ buf_desc_sz);
/* host read next element in ring */
if (++cur_rp >= ring->r.len)
--
2.22.0
^ permalink raw reply related
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