* Re: [PATCH 5/7] mmc: sdio: Don't re-initialize powered-on removable SDIO cards at resume
From: Doug Anderson @ 2019-07-09 21:26 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, Arend Van Spriel,
linux-wireless
In-Reply-To: <20190618153448.27145-6-ulf.hansson@linaro.org>
Hi,
On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> It looks like the original idea behind always doing a re-initialization of
> a removable SDIO card during system resume in mmc_sdio_resume(), is to try
> to play safe to detect whether the card has been removed.
>
> However, this seems like a really a bad idea as it will most likely screw
> things up, especially when the card is expected to remain powered on during
> system suspend by the SDIO func driver.
>
> Let's fix this, simply by trusting that the detect work checks if the card
> is alive and inserted, which is being scheduled at the PM_POST_SUSPEND
> notification anyway.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/sdio.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
I'm not sure if it's even worth mentioning since the device I tested
was rk3288-veyron (both WiFi variants) and we have a non-removable,
powered-in-suspend, wakeup-disabled card. That means it isn't
affected at all by your change. ...but I suppose I can at least
confirm that your change didn't break me if that's worth anything.
:-P
Tested-by: Douglas Anderson <dianders@chromium.org>
I would also say that, though I don't have any history here, your
patch seems reasonable to me. So you can add a:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 3/7] mmc: sdio: Move comment about re-initialization to mmc_sdio_reinit_card()
From: Doug Anderson @ 2019-07-09 21:27 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, Arend Van Spriel,
linux-wireless
In-Reply-To: <20190618153448.27145-4-ulf.hansson@linaro.org>
Hi,
On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The comment in mmc_sdio_power_restore() belongs in mmc_sdio_reinit_card(),
> which was created during a previous commit that re-factored some code. Fix
> this by moving the comment into mmc_sdio_reinit_card().
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/sdio.c | 34 +++++++++++++++++-----------------
> 1 file changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 6/7] mmc: sdio: Drop unused in-parameter to mmc_sdio_reinit_card()
From: Doug Anderson @ 2019-07-09 21:29 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, Arend Van Spriel,
linux-wireless
In-Reply-To: <20190618153448.27145-7-ulf.hansson@linaro.org>
Hi,
On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The "powered_resume" in-parameter to mmc_sdio_reinit_card() has now become
> redundant as all callers set it to 0. Therefore let's just drop it.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/sdio.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 7/7] mmc: sdio: Drop unused in-parameter from mmc_sdio_init_card()
From: Doug Anderson @ 2019-07-09 21:31 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, Arend Van Spriel,
linux-wireless
In-Reply-To: <20190618153448.27145-8-ulf.hansson@linaro.org>
Hi,
On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> The "powered_resume" in-parameter to mmc_sdio_init_card() has now become
> redundant as all callers set it to 0. Therefore let's just drop it.
>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
> drivers/mmc/core/sdio.c | 20 +++++++++-----------
> 1 file changed, 9 insertions(+), 11 deletions(-)
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Tested-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: [PATCH 4/7] mmc: sdio: Drop powered-on re-init at runtime resume and HW reset
From: Doug Anderson @ 2019-07-09 23:35 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, Arend Van Spriel,
linux-wireless
In-Reply-To: <CAPDyKFpN38G-Oj-+cKcxQ8z8K7M_0sA6_CTSKuxvez2s3Td+xw@mail.gmail.com>
Hi,
On Tue, Jul 9, 2019 at 5:02 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Mon, 8 Jul 2019 at 23:12, Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Mon, Jul 8, 2019 at 3:54 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Thu, 4 Jul 2019 at 02:02, Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Jun 18, 2019 at 8:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > To use the so called powered-on re-initialization of an SDIO card, the
> > > > > power to the card must obviously have stayed on. If not, the initialization
> > > > > will simply fail.
> > > > >
> > > > > In the runtime suspend case, the card is always powered off. Hence, let's
> > > > > drop the support for powered-on re-initialization during runtime resume, as
> > > > > it doesn't make sense.
> > > > >
> > > > > Moreover, during a HW reset, the point is to cut the power to the card and
> > > > > then do fresh re-initialization. Therefore drop the support for powered-on
> > > > > re-initialization during HW reset.
> > > > >
> > > > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > > > ---
> > > > > drivers/mmc/core/sdio.c | 8 +-------
> > > > > 1 file changed, 1 insertion(+), 7 deletions(-)
> > > >
> > > > This has been on my list of things to test for a while but I never
> > > > quite got to it...
> > > >
> > > > ...and then, today, I spent time bisecting why the "reset"
> > > > functionality of miwfiex is broken on my 4.19 kernel [1]. AKA, this
> > > > is broken:
> > > >
> > > > cd /sys/kernel/debug/mwifiex/mlan0
> > > > echo 1 > reset
> > > >
> > > > I finally bisected the problem and tracked it down to commit
> > > > ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when SDIO IRQs
> > > > are enabled"), which embarrassingly has my Tested-by on it. I guess I
> > > > never tested the Marvell reset call. :-/
> > > >
> > > > I dug a little and found that when the Marvell code did its reset we
> > > > ended up getting a call to dw_mci_enable_sdio_irq(enb=0) and never saw
> > > > a dw_mci_enable_sdio_irq(enb=1) after. I tracked it down further and
> > > > found that specifically it was the call to mmc_signal_sdio_irq() in
> > > > mmc_sdio_power_restore() that was making the call. The call stack
> > > > shown for the "enb=0" call:
> > > >
> > > > [<c071a290>] (dw_mci_enable_sdio_irq) from [<c070a960>]
> > > > (mmc_sdio_power_restore+0x98/0xc0)
> > > > [<c070a960>] (mmc_sdio_power_restore) from [<c070a9b4>]
> > > > (mmc_sdio_reset+0x2c/0x30)
> > > > [<c070a9b4>] (mmc_signal_sdio_irq) from [<c06ff160>] (mmc_hw_reset+0xbc/0x138)
> > > > [<c06ff160>] (mmc_hw_reset) from [<bf1bbad8>]
> > > > (mwifiex_sdio_work+0x5d4/0x678 [mwifiex_sdio])
> > > > [<bf1bbad8>] (mwifiex_sdio_work [mwifiex_sdio]) from [<c0247cd0>]
> > > > (process_one_work+0x290/0x4b4)
> > > >
> > > > I picked your patch here (which gets rid of the call to
> > > > mmc_signal_sdio_irq()) and magically the problem went away because
> > > > there is no more call to mmc_signal_sdio_irq().
> > > >
> > > > I personally don't have lots of history about the whole
> > > > "powered_resume" code path. I checked and mmc_card_keep_power() was 0
> > > > in my test case of getting called from hw_reset, so the rest of this
> > > > patch doesn't affect me at all. This surprised me a little since I
> > > > saw "MMC_PM_KEEP_POWER" being set in mwifiex but then I realized that
> > > > it was only set for the duration of suspend and then cleared by the
> > > > core. ;-)
> > > >
> > > > I will also say that I don't have any test case or knowledge of how
> > > > SDIO runtime suspend/resume is supposed to work since on dw_mmc SDIO
> > > > cards are currently not allowed to runtime suspend anyway. ;-)
> > > >
> > > >
> > > > So I guess the result of all that long-winded reply is that for on
> > > > rk3288-veyron-jerry:
> > > >
> > > > Fixes: ca8971ca5753 ("mmc: dw_mmc: Prevent runtime PM suspend when
> > > > SDIO IRQs are enabled")
> > > > Tested-by: Douglas Anderson <dianders@chromium.org>
> > >
> > > Thanks a lot for testing and for your detailed feedback. I have
> > > amended the patch by adding your tags above.
> >
> > Sure! I'm going to try to do some detailed testing on the next patch
> > too to confirm it's OK, but I have a few other tasks to get to first.
> > ;-)
> >
> >
> > > Moreover, we seems to need this for stable as well, but I am leaving
> > > that to be managed as a separate task. We may even consider the hole
> > > series for stable, but that requires more testing first.
> >
> > Sure, makes sense. We'll pick it to the Chrome OS 4.19 kernel
> > directly but it's usually nice to get fixes like this into stable so
> > everyone can benefit.
> >
> >
> > > > One last note is that, though Marvell WiFi works after a reset after
> > > > this commit, Marvell Bluetooth (on the same SDIO module) doesn't. I
> > > > guess next week it'll be another bisect...
> > >
> > > Is the Bluetooth connected to the same SDIO interface, thus the
> > > Bluetooth driver is an SDIO func driver?
> >
> > Yes, it's a SDIO func driver connected to the same interface. So far
> > I've managed to confirm the problem on:
> >
> > v4.4 (plus 76ae3e26ea43 mwifiex: add debugfs file for testing reset of card)
> > v4.9
> > next-20190708
> >
> > ...so it seems like it's not a "regression", it's just never worked.
> > :-P I guess I'll have to see if I can figure out what's different in
> > our chromeos-3.14 kernel. Ah, I see. In 3.14 we had this solution:
> >
> > pr_err("Resetting card...\n");
> > mmc_remove_host(reset_host);
> > /* 200ms delay is based on experiment with sdhci controller */
> > mdelay(200);
> > reset_host->rescan_entered = 0;
> > mmc_add_host(reset_host);
> >
> > ...I think that didn't fly upstream. ...but I can confirm that this works fine:
> >
> > cd /sys/bus/platform/drivers/dwmmc_rockchip
> > echo ff0d0000.dwmmc > unbind
> > sleep .5
> > echo ff0d0000.dwmmc > bind
> >
> > ...so I guess this boils down to: how does the mwifiex reset code not
> > behave like a full removal and re-insertion of the card? Oh, but
> > maybe that's obvious. We're doing all the reset / re-init from the
> > WiFi side of things (mwifiex_sdio_card_reset_work) but I don't think
> > anything is communicated to the Bluetooth side of things. Presumably
> > this is just totally broken for everyone? ...or am I confused?
>
> Nope, that is most likely what is happening.
>
> I am not sure what is the best method to deal with this. Perhaps we
> should invent some callback the SDIO core code can call, for each
> active SDIO func on the particular SDIO card that becomes reset.
>
> Or is there a better way you think?
I didn't get a chance to fully dig today, but I keep thinking that the
cleanest way would be if I could somehow tell the MMC core to pretend
to unplug the card and then re-plug it in. Then we could go through
all the standard code paths. I remember the last time I looked for a
nice way to do that I couldn't find one.
...barring that I wonder if it's enough to just remove and re-add all
the active SDIO funcs?
-Doug
^ permalink raw reply
* [5.2 regression] rtwpci + amd iommu
From: Ján Veselý @ 2019-07-10 4:38 UTC (permalink / raw)
To: linux-wireless; +Cc: yhchuang
Hi,
after updating to 5.2 the wi-fi driver stopped working when the iommu
is enabled.
It fails to list the available wi-fi networks or connect to a known one.
booting with amd_iommu=off works around the problem.
The staging version in 5.1 and older worked OK with the iommu enabled
The device is:
04:00.0 Network controller: Realtek Semiconductor Co., Ltd. RTL8822BE
802.11a/b/g/n/ac WiFi adapter
I do see an IOMMU error in dmesg, but the originating device does not
match the nics pci location:
Jul 08 15:03:14 host kernel: rtw_pci 0000:04:00.0: start vif
xx:xx:xx:xx:xx:xx on port 0
Jul 08 15:03:14 host kernel: iommu ivhd0: AMD-Vi: Event logged
[INVALID_DEVICE_REQUEST device=00:00.1 pasid=0x00000
address=0xfffffffdf8140200 flags=0x0a00]
Jul 08 15:03:14 host kernel: rtw_pci 0000:04:00.0: stop vif
xx:xx:xx:xx:xx:xx on port 0
Jul 08 15:03:14 host NetworkManager[790]: <info> [1562612594.8992]
device (wlp4s0): set-hw-addr: set MAC address to yy:yy:yy:yy:yy:yy
(scanning)
Jul 08 15:03:15 host kernel: rtw_pci 0000:04:00.0: start vif
yy:yy:yy:yy:yy:yy on port 0
let me know if I should provide any further info.
thanks,
Jan
^ permalink raw reply
* [PATCH 09/12] rtw88: Fix misuse of GENMASK macro
From: Joe Perches @ 2019-07-10 5:04 UTC (permalink / raw)
To: Andrew Morton, Yan-Hsuan Chuang
Cc: Kalle Valo, David S. Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <cover.1562734889.git.joe@perches.com>
Arguments are supposed to be ordered high then low.
Signed-off-by: Joe Perches <joe@perches.com>
---
drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.c b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
index 1172f6c0605b..d61d534396c7 100644
--- a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
+++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
@@ -997,7 +997,7 @@ static void rtw8822b_do_iqk(struct rtw_dev *rtwdev)
rtw_write_rf(rtwdev, RF_PATH_A, RF_DTXLOK, RFREG_MASK, 0x0);
reload = !!rtw_read32_mask(rtwdev, REG_IQKFAILMSK, BIT(16));
- iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK, GENMASK(0, 7));
+ iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK, GENMASK(7, 0));
rtw_dbg(rtwdev, RTW_DBG_PHY,
"iqk counter=%d reload=%d do_iqk_cnt=%d n_iqk_fail(mask)=0x%02x\n",
counter, reload, ++do_iqk_cnt, iqk_fail_mask);
--
2.15.0
^ permalink raw reply related
* [PATCH 00/12] treewide: Fix GENMASK misuses
From: Joe Perches @ 2019-07-10 5:04 UTC (permalink / raw)
To: 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
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
clocksource/drivers/npcm: Fix misuse of GENMASK macro
drm: aspeed_gfx: Fix misuse of GENMASK macro
iio: adc: max9611: Fix misuse of GENMASK macro
irqchip/gic-v3-its: Fix misuse of GENMASK macro
mmc: meson-mx-sdio: Fix misuse of GENMASK macro
net: ethernet: mediatek: Fix misuses of GENMASK macro
net: stmmac: Fix misuses of GENMASK macro
rtw88: Fix misuse of GENMASK macro
phy: amlogic: G12A: Fix misuse of GENMASK macro
staging: media: cedrus: Fix misuse of GENMASK macro
ASoC: wcd9335: Fix misuse of GENMASK macro
drivers/clocksource/timer-npcm7xx.c | 2 +-
drivers/gpu/drm/aspeed/aspeed_gfx.h | 2 +-
drivers/iio/adc/max9611.c | 2 +-
drivers/irqchip/irq-gic-v3-its.c | 2 +-
drivers/mmc/host/meson-mx-sdio.c | 2 +-
drivers/net/ethernet/mediatek/mtk_eth_soc.h | 2 +-
drivers/net/ethernet/mediatek/mtk_sgmii.c | 2 +-
drivers/net/ethernet/stmicro/stmmac/descs.h | 2 +-
drivers/net/ethernet/stmicro/stmmac/dwmac-sun8i.c | 4 ++--
drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
drivers/phy/amlogic/phy-meson-g12a-usb2.c | 2 +-
drivers/staging/media/sunxi/cedrus/cedrus_regs.h | 2 +-
scripts/checkpatch.pl | 15 +++++++++++++++
sound/soc/codecs/wcd-clsh-v2.c | 2 +-
14 files changed, 29 insertions(+), 14 deletions(-)
--
2.15.0
^ permalink raw reply
* RE: [PATCH 09/12] rtw88: Fix misuse of GENMASK macro
From: Tony Chuang @ 2019-07-10 5:07 UTC (permalink / raw)
To: Joe Perches, Andrew Morton
Cc: Kalle Valo, David S. Miller, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org, Andy Huang
In-Reply-To: <0de52d891d7925b02f4f0fe2c750d076e55434d9.1562734889.git.joe@perches.com>
> Subject: [PATCH 09/12] rtw88: Fix misuse of GENMASK macro
>
> Arguments are supposed to be ordered high then low.
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> drivers/net/wireless/realtek/rtw88/rtw8822b.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
> b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
> index 1172f6c0605b..d61d534396c7 100644
> --- a/drivers/net/wireless/realtek/rtw88/rtw8822b.c
> +++ b/drivers/net/wireless/realtek/rtw88/rtw8822b.c
> @@ -997,7 +997,7 @@ static void rtw8822b_do_iqk(struct rtw_dev *rtwdev)
> rtw_write_rf(rtwdev, RF_PATH_A, RF_DTXLOK, RFREG_MASK, 0x0);
>
> reload = !!rtw_read32_mask(rtwdev, REG_IQKFAILMSK, BIT(16));
> - iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK,
> GENMASK(0, 7));
> + iqk_fail_mask = rtw_read32_mask(rtwdev, REG_IQKFAILMSK,
> GENMASK(7, 0));
> rtw_dbg(rtwdev, RTW_DBG_PHY,
> "iqk counter=%d reload=%d do_iqk_cnt=%d
> n_iqk_fail(mask)=0x%02x\n",
> counter, reload, ++do_iqk_cnt, iqk_fail_mask);
> --
That's correct. Thanks.
Acked-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
Yan-Hsuan
^ permalink raw reply
* RE: [5.2 regression] rtwpci + amd iommu
From: Tony Chuang @ 2019-07-10 5:12 UTC (permalink / raw)
To: Ján Veselý, linux-wireless
In-Reply-To: <CA+K+NcRWLeE3-vah=urveMVxcgXYO0yFHYD=WNeuX_TdZ9+8-A@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1390 bytes --]
> Hi,
>
> after updating to 5.2 the wi-fi driver stopped working when the iommu
> is enabled.
> It fails to list the available wi-fi networks or connect to a known one.
> booting with amd_iommu=off works around the problem.
> The staging version in 5.1 and older worked OK with the iommu enabled
>
> The device is:
> 04:00.0 Network controller: Realtek Semiconductor Co., Ltd. RTL8822BE
> 802.11a/b/g/n/ac WiFi adapter
>
> I do see an IOMMU error in dmesg, but the originating device does not
> match the nics pci location:
> Jul 08 15:03:14 host kernel: rtw_pci 0000:04:00.0: start vif
> xx:xx:xx:xx:xx:xx on port 0
> Jul 08 15:03:14 host kernel: iommu ivhd0: AMD-Vi: Event logged
> [INVALID_DEVICE_REQUEST device=00:00.1 pasid=0x00000
> address=0xfffffffdf8140200 flags=0x0a00]
> Jul 08 15:03:14 host kernel: rtw_pci 0000:04:00.0: stop vif
> xx:xx:xx:xx:xx:xx on port 0
> Jul 08 15:03:14 host NetworkManager[790]: <info> [1562612594.8992]
> device (wlp4s0): set-hw-addr: set MAC address to yy:yy:yy:yy:yy:yy
> (scanning)
> Jul 08 15:03:15 host kernel: rtw_pci 0000:04:00.0: start vif
> yy:yy:yy:yy:yy:yy on port 0
>
> let me know if I should provide any further info.
> thanks,
> Jan
>
Hi,
I am not sure if enabling MSI interrupt is going to help.
You can try the patch attached, if it works, I will send it to upstream.
Thanks
Yan-Hsuan
[-- Attachment #2: 0001-rtw88-pci-add-MSI-interrupt-support.patch --]
[-- Type: application/octet-stream, Size: 5082 bytes --]
From 570d6ed8585855a8c6aee20e6d319829ebef6bc7 Mon Sep 17 00:00:00 2001
From: Yu-Yen Ting <steventing@realtek.com>
Date: Wed, 29 May 2019 16:31:27 +0800
Subject: [PATCH] rtw88: pci: add MSI interrupt support
The MSI interrupt should be enabled on certain platform.
Add the module parameter disable_msi to force disable the MSI interrupt
Signed-off-by: Yu-Yen Ting <steventing@realtek.com>
Signed-off-by: Yan-Hsuan Chuang <yhchuang@realtek.com>
---
drivers/net/wireless/realtek/rtw88/main.c | 5 ++++
drivers/net/wireless/realtek/rtw88/main.h | 1 +
drivers/net/wireless/realtek/rtw88/pci.c | 47 +++++++++++++++++++++++++++++--
drivers/net/wireless/realtek/rtw88/pci.h | 1 +
4 files changed, 51 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw88/main.c b/drivers/net/wireless/realtek/rtw88/main.c
index b8e0519..d86e05e 100644
--- a/drivers/net/wireless/realtek/rtw88/main.c
+++ b/drivers/net/wireless/realtek/rtw88/main.c
@@ -15,13 +15,18 @@
static bool rtw_fw_support_lps;
unsigned int rtw_debug_mask;
+bool rtw_disable_msi;
+
EXPORT_SYMBOL(rtw_debug_mask);
+EXPORT_SYMBOL(rtw_disable_msi);
module_param_named(support_lps, rtw_fw_support_lps, bool, 0644);
module_param_named(debug_mask, rtw_debug_mask, uint, 0644);
+module_param_named(disable_msi, rtw_disable_msi, bool, 0644);
MODULE_PARM_DESC(support_lps, "Set Y to enable Leisure Power Save support, to turn radio off between beacons");
MODULE_PARM_DESC(debug_mask, "Debugging mask");
+MODULE_PARM_DESC(disable_msi, "Set Y to disable MSI interrupt support");
static struct ieee80211_channel rtw_channeltable_2g[] = {
{.center_freq = 2412, .hw_value = 1,},
diff --git a/drivers/net/wireless/realtek/rtw88/main.h b/drivers/net/wireless/realtek/rtw88/main.h
index 92730507..ca603ba 100644
--- a/drivers/net/wireless/realtek/rtw88/main.h
+++ b/drivers/net/wireless/realtek/rtw88/main.h
@@ -28,6 +28,7 @@
#define HW_FEATURE_LEN 13
extern unsigned int rtw_debug_mask;
+extern bool rtw_disable_msi;
extern const struct ieee80211_ops rtw_ops;
extern struct rtw_chip_info rtw8822b_hw_spec;
extern struct rtw_chip_info rtw8822c_hw_spec;
diff --git a/drivers/net/wireless/realtek/rtw88/pci.c b/drivers/net/wireless/realtek/rtw88/pci.c
index 353871c..817b4ab 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.c
+++ b/drivers/net/wireless/realtek/rtw88/pci.c
@@ -861,6 +861,7 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev)
if (!rtwpci->irq_enabled)
goto out;
+ rtw_pci_disable_interrupt(rtwdev, rtwpci);
rtw_pci_irq_recognized(rtwdev, rtwpci, irq_status);
if (irq_status[0] & IMR_MGNTDOK)
@@ -880,6 +881,8 @@ static irqreturn_t rtw_pci_interrupt_handler(int irq, void *dev)
if (irq_status[0] & IMR_ROK)
rtw_pci_rx_isr(rtwdev, rtwpci, RTW_RX_QUEUE_MPDU);
+ rtw_pci_enable_interrupt(rtwdev, rtwpci);
+
out:
spin_unlock(&rtwpci->irq_lock);
@@ -1090,6 +1093,45 @@ static struct rtw_hci_ops rtw_pci_ops = {
.write_data_h2c = rtw_pci_write_data_h2c,
};
+static int rtw_pci_request_irq(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+ struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+ int ret;
+
+ if (!rtw_disable_msi) {
+ ret = pci_enable_msi(pdev);
+ if (ret) {
+ rtw_warn(rtwdev, "failed to enable msi, using legacy irq\n");
+ } else {
+ rtw_warn(rtwdev, "pci msi enabled\n");
+ rtwpci->msi_enabled = true;
+ }
+ }
+
+ ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler, IRQF_SHARED,
+ KBUILD_MODNAME, rtwdev);
+ if (ret) {
+ rtw_err(rtwdev, "failed to request irq\n");
+ if (rtwpci->msi_enabled) {
+ pci_disable_msi(pdev);
+ rtwpci->msi_enabled = false;
+ }
+ }
+
+ return ret;
+}
+
+static void rtw_pci_free_irq(struct rtw_dev *rtwdev, struct pci_dev *pdev)
+{
+ struct rtw_pci *rtwpci = (struct rtw_pci *)rtwdev->priv;
+
+ free_irq(pdev->irq, rtwdev);
+ if (rtwpci->msi_enabled) {
+ pci_disable_msi(pdev);
+ rtwpci->msi_enabled = false;
+ }
+}
+
static int rtw_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
@@ -1144,8 +1186,7 @@ static int rtw_pci_probe(struct pci_dev *pdev,
goto err_destroy_pci;
}
- ret = request_irq(pdev->irq, &rtw_pci_interrupt_handler,
- IRQF_SHARED, KBUILD_MODNAME, rtwdev);
+ ret = rtw_pci_request_irq(rtwdev, pdev);
if (ret) {
ieee80211_unregister_hw(hw);
goto err_destroy_pci;
@@ -1184,7 +1225,7 @@ static void rtw_pci_remove(struct pci_dev *pdev)
rtw_pci_disable_interrupt(rtwdev, rtwpci);
rtw_pci_destroy(rtwdev, pdev);
rtw_pci_declaim(rtwdev, pdev);
- free_irq(rtwpci->pdev->irq, rtwdev);
+ rtw_pci_free_irq(rtwdev, pdev);
rtw_core_deinit(rtwdev);
ieee80211_free_hw(hw);
}
diff --git a/drivers/net/wireless/realtek/rtw88/pci.h b/drivers/net/wireless/realtek/rtw88/pci.h
index 87824a4..a8e369c 100644
--- a/drivers/net/wireless/realtek/rtw88/pci.h
+++ b/drivers/net/wireless/realtek/rtw88/pci.h
@@ -186,6 +186,7 @@ struct rtw_pci {
spinlock_t irq_lock;
u32 irq_mask[4];
bool irq_enabled;
+ bool msi_enabled;
u16 rx_tag;
struct rtw_pci_tx_ring tx_rings[RTK_MAX_TX_QUEUE_NUM];
--
2.7.4
^ permalink raw reply related
* Re: [5.2 regression] rtwpci + amd iommu
From: Ján Veselý @ 2019-07-10 6:09 UTC (permalink / raw)
To: Tony Chuang; +Cc: linux-wireless
In-Reply-To: <F7CD281DE3E379468C6D07993EA72F84D1864503@RTITMBSVM04.realtek.com.tw>
Hi,
thanks for a prompt reply.
the patch applies with minor changes (the LPS mod param message and -2
lines offsets), but it works.
The system now lists available wi-fi networks and connects to known
networks all while the iommu is enabled.
feel free to add my tested-by,
regards,
Jan
On Wed, Jul 10, 2019 at 1:12 AM Tony Chuang <yhchuang@realtek.com> wrote:
>
> > Hi,
> >
> > after updating to 5.2 the wi-fi driver stopped working when the iommu
> > is enabled.
> > It fails to list the available wi-fi networks or connect to a known one.
> > booting with amd_iommu=off works around the problem.
> > The staging version in 5.1 and older worked OK with the iommu enabled
> >
> > The device is:
> > 04:00.0 Network controller: Realtek Semiconductor Co., Ltd. RTL8822BE
> > 802.11a/b/g/n/ac WiFi adapter
> >
> > I do see an IOMMU error in dmesg, but the originating device does not
> > match the nics pci location:
> > Jul 08 15:03:14 host kernel: rtw_pci 0000:04:00.0: start vif
> > xx:xx:xx:xx:xx:xx on port 0
> > Jul 08 15:03:14 host kernel: iommu ivhd0: AMD-Vi: Event logged
> > [INVALID_DEVICE_REQUEST device=00:00.1 pasid=0x00000
> > address=0xfffffffdf8140200 flags=0x0a00]
> > Jul 08 15:03:14 host kernel: rtw_pci 0000:04:00.0: stop vif
> > xx:xx:xx:xx:xx:xx on port 0
> > Jul 08 15:03:14 host NetworkManager[790]: <info> [1562612594.8992]
> > device (wlp4s0): set-hw-addr: set MAC address to yy:yy:yy:yy:yy:yy
> > (scanning)
> > Jul 08 15:03:15 host kernel: rtw_pci 0000:04:00.0: start vif
> > yy:yy:yy:yy:yy:yy on port 0
> >
> > let me know if I should provide any further info.
> > thanks,
> > Jan
> >
>
>
> Hi,
>
> I am not sure if enabling MSI interrupt is going to help.
> You can try the patch attached, if it works, I will send it to upstream.
> Thanks
>
> Yan-Hsuan
^ permalink raw reply
* [net-next] mwifiex: use eth_broadcast_addr() to assign broadcast address
From: Mao Wenan @ 2019-07-10 7:25 UTC (permalink / raw)
To: amitkarwar, nishants, gbhat, huxinming820, kvalo
Cc: linux-wireless, kernel-janitors, Mao Wenan
This patch is to use eth_broadcast_addr() to assign broadcast address
insetad of memcpy().
Signed-off-by: Mao Wenan <maowenan@huawei.com>
---
drivers/net/wireless/marvell/mwifiex/tdls.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/tdls.c b/drivers/net/wireless/marvell/mwifiex/tdls.c
index 18e654d..0931304 100644
--- a/drivers/net/wireless/marvell/mwifiex/tdls.c
+++ b/drivers/net/wireless/marvell/mwifiex/tdls.c
@@ -731,7 +731,6 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
u16 status_code, struct sk_buff *skb)
{
struct ieee80211_mgmt *mgmt;
- u8 bc_addr[] = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff};
int ret;
u16 capab;
struct ieee80211_ht_cap *ht_cap;
@@ -765,7 +764,7 @@ mwifiex_construct_tdls_action_frame(struct mwifiex_private *priv,
memmove(pos + ETH_ALEN, &mgmt->u.action.category,
sizeof(mgmt->u.action.u.tdls_discover_resp));
/* init address 4 */
- memcpy(pos, bc_addr, ETH_ALEN);
+ eth_broadcast_addr(pos);
ret = mwifiex_tdls_append_rates_ie(priv, skb);
if (ret) {
--
2.7.4
^ permalink raw reply related
* RE: [PATCH v2 1/2] rtw88: pci: Rearrange the memory usage for skb in RX ISR
From: Tony Chuang @ 2019-07-10 8:36 UTC (permalink / raw)
To: Jian-Hong Pan, Kalle Valo, David S . Miller, Larry Finger,
David Laight
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: <20190709102059.7036-1-jian-hong@endlessm.com>
> 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
^ permalink raw reply
* [PATCH v3 1/2] rtw88: pci: Rearrange the memory usage for skb 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>
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.
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
+ */
+ 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
* [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
* 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
* 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 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 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
* [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 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
* 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 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
* [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
* [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
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