* Re: [PATCH v2] mac80211: remove warning message
From: Johannes Berg @ 2019-06-14 7:22 UTC (permalink / raw)
To: Yibo Zhao; +Cc: Ben Greear, linux-wireless, ath10k, Zhi Chen
In-Reply-To: <143d044082b54b2be41ba3760390d97f@codeaurora.org>
On Fri, 2019-06-14 at 10:52 +0800, Yibo Zhao wrote:
>
> May I know if it is fine that WARN_ON_ONCE() to be applied in kernel in
> the future? If a separate patch for it is needed, please let me know so
> that I can raise a new one.
Please do send a new patch.
johannes
^ permalink raw reply
* [PATCH] nfc: Ensure presence of required attributes in the deactivate_target handler
From: Young Xiao @ 2019-06-14 7:13 UTC (permalink / raw)
To: sameo, davem, linux-wireless, netdev, linux-kernel; +Cc: Young Xiao
Check that the NFC_ATTR_TARGET_INDEX attributes (in addition to
NFC_ATTR_DEVICE_INDEX) are provided by the netlink client prior to
accessing them. This prevents potential unhandled NULL pointer dereference
exceptions which can be triggered by malicious user-mode programs,
if they omit one or both of these attributes.
Signed-off-by: Young Xiao <92siuyang@gmail.com>
---
net/nfc/netlink.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/net/nfc/netlink.c b/net/nfc/netlink.c
index 04a8e47..89d885d 100644
--- a/net/nfc/netlink.c
+++ b/net/nfc/netlink.c
@@ -923,7 +923,8 @@ static int nfc_genl_deactivate_target(struct sk_buff *skb,
u32 device_idx, target_idx;
int rc;
- if (!info->attrs[NFC_ATTR_DEVICE_INDEX])
+ if (!info->attrs[NFC_ATTR_DEVICE_INDEX] ||
+ !info->attrs[NFC_ATTR_TARGET_INDEX])
return -EINVAL;
device_idx = nla_get_u32(info->attrs[NFC_ATTR_DEVICE_INDEX]);
--
2.7.4
^ permalink raw reply related
* [PATCH] mac80211: no need to check return value of debugfs_create functions
From: Greg Kroah-Hartman @ 2019-06-14 6:59 UTC (permalink / raw)
To: Johannes Berg; +Cc: David S. Miller, linux-wireless, netdev
When calling debugfs functions, there is no need to ever check the
return value. The function can work or not, but the code logic should
never do something different based on this.
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: linux-wireless@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
net/mac80211/debugfs_key.c | 3 ---
net/mac80211/debugfs_netdev.c | 10 +++-------
net/mac80211/debugfs_sta.c | 2 --
3 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/net/mac80211/debugfs_key.c b/net/mac80211/debugfs_key.c
index a2ef95f16f11..1a25de4e7e78 100644
--- a/net/mac80211/debugfs_key.c
+++ b/net/mac80211/debugfs_key.c
@@ -342,9 +342,6 @@ void ieee80211_debugfs_key_add(struct ieee80211_key *key)
key->debugfs.dir = debugfs_create_dir(buf,
key->local->debugfs.keys);
- if (!key->debugfs.dir)
- return;
-
sta = key->sta;
if (sta) {
sprintf(buf, "../../netdev:%s/stations/%pM",
diff --git a/net/mac80211/debugfs_netdev.c b/net/mac80211/debugfs_netdev.c
index deb3faf08337..f6508cf67944 100644
--- a/net/mac80211/debugfs_netdev.c
+++ b/net/mac80211/debugfs_netdev.c
@@ -818,9 +818,8 @@ void ieee80211_debugfs_add_netdev(struct ieee80211_sub_if_data *sdata)
sprintf(buf, "netdev:%s", sdata->name);
sdata->vif.debugfs_dir = debugfs_create_dir(buf,
sdata->local->hw.wiphy->debugfsdir);
- if (sdata->vif.debugfs_dir)
- sdata->debugfs.subdir_stations = debugfs_create_dir("stations",
- sdata->vif.debugfs_dir);
+ sdata->debugfs.subdir_stations = debugfs_create_dir("stations",
+ sdata->vif.debugfs_dir);
add_files(sdata);
}
@@ -845,8 +844,5 @@ void ieee80211_debugfs_rename_netdev(struct ieee80211_sub_if_data *sdata)
return;
sprintf(buf, "netdev:%s", sdata->name);
- if (!debugfs_rename(dir->d_parent, dir, dir->d_parent, buf))
- sdata_err(sdata,
- "debugfs: failed to rename debugfs dir to %s\n",
- buf);
+ debugfs_rename(dir->d_parent, dir, dir->d_parent, buf);
}
diff --git a/net/mac80211/debugfs_sta.c b/net/mac80211/debugfs_sta.c
index 8e921281e0d5..b2542bb2814e 100644
--- a/net/mac80211/debugfs_sta.c
+++ b/net/mac80211/debugfs_sta.c
@@ -960,8 +960,6 @@ void ieee80211_sta_debugfs_add(struct sta_info *sta)
* dir might still be around.
*/
sta->debugfs_dir = debugfs_create_dir(mac, stations_dir);
- if (!sta->debugfs_dir)
- return;
DEBUGFS_ADD(flags);
DEBUGFS_ADD(aid);
--
2.22.0
^ permalink raw reply related
* Re: Cleanup of -Wunused-const-variable in drivers/net/wireless/ti/wl18xx/main.c
From: Kalle Valo @ 2019-06-14 5:25 UTC (permalink / raw)
To: Nathan Huckleberry
Cc: eliad, davem, netdev, linux-wireless, clang-built-linux
In-Reply-To: <CAJkfWY4WkeMv3Z+Nh4B0xtErTAi6mVCriURZTjd2Q__gMtaEqA@mail.gmail.com>
Nathan Huckleberry <nhuck@google.com> writes:
> I'm looking into cleaning up ignored warnings in the kernel so we can
> remove compiler flags to ignore warnings.
>
> There are two unused variables ('wl18xx_iface_ap_cl_limits' and
> 'wl18xx_iface_ap_go_limits') in drivers/net/wireless/ti/wl18xx/main.c.
> These appear to be limits when using p2p devices, yet they are never
> used.
>
> Wanted to reach out for the best course of action to fix the warning.
>
> https://github.com/ClangBuiltLinux/linux/issues/530
The the variables were added in this commit:
commit 7845af35e0deeb7537de759ebc69d6395d4123bf
Author: Eliad Peller <eliad@wizery.com>
AuthorDate: Thu Jul 30 22:38:22 2015 +0300
Commit: Kalle Valo <kvalo@codeaurora.org>
CommitDate: Mon Aug 10 22:16:34 2015 +0300
wlcore: add p2p device support
And even that commit didn't use them, no idea why. Just send a patch
removing them, if someone needs them later they can be added again.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v2] mac80211: remove warning message
From: Yibo Zhao @ 2019-06-14 2:52 UTC (permalink / raw)
To: Johannes Berg; +Cc: Ben Greear, linux-wireless, ath10k, Zhi Chen
In-Reply-To: <bbfd69ccb4289ba3f1767c1066f61ee1@codeaurora.org>
On 2019-05-20 21:56, Yibo Zhao wrote:
> On 2019-05-15 02:57, Johannes Berg wrote:
>> On Tue, 2019-05-14 at 11:54 -0700, Ben Greear wrote:
>>>
>>> Here is the info I have in my commit that changed this to
>>> WARN_ON_ONCE.
>>> I never posted it because I had to hack ath10k to get to this state,
>>> so maybe
>>> this is not a valid case to debug.
>>>
>>>
>>> Maybe Yibo Zhao has a better example.
>>>
>>> mac80211: don't spam kernel logs when chantx is null.
>>>
>>> I set up ath10k to be chandef based again in order to test
>>> WDS. My WDS stations are not very functional yet, and
>>> when ethtool stats are queried, there is a WARN_ON splat
>>> generated. Change this to WARN_ON_ONCE so that there is
>>> less kernel spam.
>>
>> I'm totally fine with WARN_ON_ONCE, FWIW.
>>
>> Sounds like different bugs though. You're talking about WDS here, and
>> Yibo was talking about something with AP interfaces prematurely
>> accepting frames or so.
>
> Yes, they might be different bugs that hit the same point. Looks like
> others found this too many warnings issue as well. Then I believe
> WARN_ON_ONCE() seems to be our solution for now.
>
Hi Johannes,
May I know if it is fine that WARN_ON_ONCE() to be applied in kernel in
the future? If a separate patch for it is needed, please let me know so
that I can raise a new one.
>>
>> johannes
--
Yibo
^ permalink raw reply
* Re: iwlwifi module crash
From: Balakrishnan Balasubramanian @ 2019-06-14 1:47 UTC (permalink / raw)
To: Emmanuel Grumbach; +Cc: Balakrishnan Balasubramanian, linux-wireless
In-Reply-To: <CANUX_P3jUBREM=cwLHs8LE+ZbUVKGZNsBWGGepbS-mcHq0w9ow@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 392 bytes --]
The issue occured again today. I tried to restart the module
> echo 1 > /sys/module/iwlwifi/devices/0000\:02\:00.0/remove
There is no folder 'devices'
zadesk% ls /sys/module/iwlwifi
coresize drivers holders initsize initstate notes parameters refcnt
sections srcversion taint uevent
> echo 1 > /sys/bus/pci/rescan
Attached the error when trying to rescan.
Thanks,
Bala
[-- Attachment #2: error --]
[-- Type: text/plain, Size: 367 bytes --]
Jun 13 21:41:56 zadesk kernel: iwlwifi 0000:02:00.0: Failed to wake NIC for hcmd
Jun 13 21:41:56 zadesk kernel: iwlwifi 0000:02:00.0: Error sending SCAN_OFFLOAD_REQUEST_CMD: enqueue_hcmd failed: -5
Jun 13 21:41:56 zadesk kernel: iwlwifi 0000:02:00.0: Scan failed! ret -5
Jun 13 21:41:56 zadesk iwd[483]: Received error during CMD_TRIGGER_SCAN: Input/output error (5)
^ permalink raw reply
* Re: [PATCH] mwifiex: avoid deleting uninitialized timer during USB cleanup
From: Brian Norris @ 2019-06-14 1:16 UTC (permalink / raw)
To: Ganapathi Bhat
Cc: linux-wireless, Cathy Luo, Zhiyuan Yang, James Cao, Rakesh Parmar,
Dmitry Vyukov
In-Reply-To: <1560354873-17182-1-git-send-email-gbhat@marvell.com>
Hi Ganapathi,
This looks kinda wrong, but I'm not totally sure, as I'm not very familiar with
your USB driver.
On Wed, Jun 12, 2019 at 09:24:33PM +0530, Ganapathi Bhat wrote:
> Driver calls del_timer_sync(hold_timer), in unregister_dev(), but
> there exists is a case when the timer is yet to be initialized. A
> restructure of init and cleanup is needed to synchronize timer
> creation and delee. Make use of init_if() / cleanup_if() handlers
s/delee/delete/
> to get this done.
>
> Reported-by: syzbot+373e6719b49912399d21@syzkaller.appspotmail.com
> Signed-off-by: Ganapathi Bhat <gbhat@marvell.com>
> ---
> drivers/net/wireless/marvell/mwifiex/usb.c | 32 +++++++++++++++++++++++-------
> 1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/usb.c b/drivers/net/wireless/marvell/mwifiex/usb.c
> index c2365ee..939f1e9 100644
> --- a/drivers/net/wireless/marvell/mwifiex/usb.c
> +++ b/drivers/net/wireless/marvell/mwifiex/usb.c
> @@ -1348,6 +1348,8 @@ static void mwifiex_usb_cleanup_tx_aggr(struct mwifiex_adapter *adapter)
>
> for (idx = 0; idx < MWIFIEX_TX_DATA_PORT; idx++) {
> port = &card->port[idx];
> + if (!port->tx_data_ep)
> + continue;
It's not clear to me what this is about. Are you sure you're not just cleaning
stuff up in the wrong order?
> if (adapter->bus_aggr.enable)
> while ((skb_tmp =
> skb_dequeue(&port->tx_aggr.aggr_list)))
...
> @@ -1584,7 +1580,29 @@ static void mwifiex_usb_submit_rem_rx_urbs(struct mwifiex_adapter *adapter)
> return 0;
> }
>
> +static int mwifiex_init_usb(struct mwifiex_adapter *adapter)
> +{
> + struct usb_card_rec *card = (struct usb_card_rec *)adapter->card;
> + int ret = 0;
> +
> + if (card->usb_boot_state == USB8XXX_FW_DNLD)
> + return 0;
This looks wrong. You don't want to skip your basic initialization just because
firmware isn't loaded yet. In fact, init_if() always gets called before FW
init, so haven't you basically stubbed out this function most of the time?
I guess the question is: is this step supposed to go before, or after firmware
initilization? Based on that answer, we can make an appropriate patch.
(The original code does this after FW initialization, and now you're only sort
of moving it before.)
> +
> + ret = mwifiex_usb_rx_init(adapter);
> + if (!ret)
> + ret = mwifiex_usb_tx_init(adapter);
> +
> + return ret;
> +}
Brian
^ permalink raw reply
* [PATCH v4 0/5] brcmfmac: sdio: Deal better w/ transmission errors related to idle
From: Douglas Anderson @ 2019-06-13 23:41 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter, Arend van Spriel
Cc: brcm80211-dev-list.pdl, linux-rockchip, Double Lo, briannorris,
linux-wireless, Naveen Gupta, Madhan Mohan R, mka, Wright Feng,
Chi-Hsien Lin, netdev, brcm80211-dev-list, Douglas Anderson,
Shawn Lin, YueHaibing, Allison Randal, Thomas Gleixner,
Hans de Goede, Hante Meuleman, Greg Kroah-Hartman, Ritesh Harjani,
Wolfram Sang, Franky Lin, Ondrej Jirman, Jiong Wu,
David S. Miller, linux-mmc, linux-kernel, Madhan Mohan R,
Avri Altman
This series attempts to deal better with the expected transmission
errors related to the idle states (handled by the Always-On-Subsystem
or AOS) on the SDIO-based WiFi on rk3288-veyron-minnie,
rk3288-veyron-speedy, and rk3288-veyron-mickey.
Some details about those errors can be found in
<https://crbug.com/960222>, but to summarize it here: if we try to
send the wakeup command to the WiFi card at the same time it has
decided to wake up itself then it will behave badly on the SDIO bus.
This can cause timeouts or CRC errors.
When I tested on 4.19 and 4.20 these CRC errors can be seen to cause
re-tuning. Since I am currently developing on 4.19 this was the
original problem I attempted to solve.
On mainline it turns out that you don't see the retuning errors but
you see tons of spam about timeouts trying to wakeup from sleep. I
tracked down the commit that was causing that and have partially
reverted it here. I have no real knowledge about Broadcom WiFi, but
the commit that was causing problems sounds (from the descriptioin) to
be a hack commit penalizing all Broadcom WiFi users because of a bug
in a Cypress SD controller. I will let others comment if this is
truly the case and, if so, what the right solution should be.
For v3 of this series I have added 2 patches to the end of the series
to address errors that would show up on systems with these same SDIO
WiFi cards when used on controllers that do periodic retuning. These
systems need an extra fix to prevent the retuning from happening when
the card is asleep.
Changes in v4:
- Moved to SDIO API only (Adrian, Ulf).
- Renamed to make it less generic, now retune_crc_disable (Ulf).
- Function header makes it clear host must be claimed (Ulf).
- No more WARN_ON (Ulf).
- Adjust to API rename (Adrian, Ulf).
- Moved retune hold/release to SDIO API (Adrian).
- Adjust to API rename (Adrian).
Changes in v3:
- Took out the spinlock since I believe this is all in one context.
- Expect errors for all of brcmf_sdio_kso_control() (Adrian).
- ("mmc: core: Export mmc_retune_hold_now() mmc_retune_release()") new for v3.
- ("brcmfmac: sdio: Don't tune while the card is off") new for v3.
Changes in v2:
- A full revert, not just a partial one (Arend). ...with explicit Cc.
- Updated commit message to clarify based on discussion of v1.
Douglas Anderson (5):
Revert "brcmfmac: disable command decode in sdio_aos"
mmc: core: API to temporarily disable retuning for SDIO CRC errors
brcmfmac: sdio: Disable auto-tuning around commands expected to fail
mmc: core: Add sdio_retune_hold_now() and sdio_retune_release()
brcmfmac: sdio: Don't tune while the card is off
drivers/mmc/core/core.c | 5 +-
drivers/mmc/core/sdio_io.c | 76 +++++++++++++++++++
.../broadcom/brcm80211/brcmfmac/sdio.c | 17 +++--
include/linux/mmc/core.h | 2 +
include/linux/mmc/host.h | 1 +
include/linux/mmc/sdio_func.h | 6 ++
6 files changed, 100 insertions(+), 7 deletions(-)
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply
* [PATCH v4 1/5] Revert "brcmfmac: disable command decode in sdio_aos"
From: Douglas Anderson @ 2019-06-13 23:41 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter, Arend van Spriel
Cc: brcm80211-dev-list.pdl, linux-rockchip, Double Lo, briannorris,
linux-wireless, Naveen Gupta, Madhan Mohan R, mka, Wright Feng,
Chi-Hsien Lin, netdev, brcm80211-dev-list, Douglas Anderson,
Franky Lin, linux-kernel, Hante Meuleman, Ondrej Jirman,
YueHaibing, David S. Miller
In-Reply-To: <20190613234153.59309-1-dianders@chromium.org>
This reverts commit 29f6589140a10ece8c1d73f58043ea5b3473ab3e.
After that patch landed I find that my kernel log on
rk3288-veyron-minnie and rk3288-veyron-speedy is filled with:
brcmfmac: brcmf_sdio_bus_sleep: error while changing bus sleep state -110
This seems to happen every time the Broadcom WiFi transitions out of
sleep mode. Reverting the commit fixes the problem for me, so that's
what this patch does.
Note that, in general, the justification in the original commit seemed
a little weak. It looked like someone was testing on a SD card
controller that would sometimes die if there were CRC errors on the
bus. This used to happen back in early days of dw_mmc (the controller
on my boards), but we fixed it. Disabling a feature on all boards
just because one SD card controller is broken seems bad.
Fixes: 29f6589140a1 ("brcmfmac: disable command decode in sdio_aos")
Cc: Wright Feng <wright.feng@cypress.com>
Cc: Double Lo <double.lo@cypress.com>
Cc: Madhan Mohan R <madhanmohan.r@cypress.com>
Cc: Chi-Hsien Lin <chi-hsien.lin@cypress.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
As far as I know this patch can land anytime.
Changes in v4: None
Changes in v3: None
Changes in v2:
- A full revert, not just a partial one (Arend). ...with explicit Cc.
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 4e15ea57d4f5..4a750838d8cd 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -3364,11 +3364,7 @@ static int brcmf_sdio_download_firmware(struct brcmf_sdio *bus,
static bool brcmf_sdio_aos_no_decode(struct brcmf_sdio *bus)
{
- if (bus->ci->chip == CY_CC_43012_CHIP_ID ||
- bus->ci->chip == CY_CC_4373_CHIP_ID ||
- bus->ci->chip == BRCM_CC_4339_CHIP_ID ||
- bus->ci->chip == BRCM_CC_4345_CHIP_ID ||
- bus->ci->chip == BRCM_CC_4354_CHIP_ID)
+ if (bus->ci->chip == CY_CC_43012_CHIP_ID)
return true;
else
return false;
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v4 3/5] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Douglas Anderson @ 2019-06-13 23:41 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter, Arend van Spriel
Cc: brcm80211-dev-list.pdl, linux-rockchip, Double Lo, briannorris,
linux-wireless, Naveen Gupta, Madhan Mohan R, mka, Wright Feng,
Chi-Hsien Lin, netdev, brcm80211-dev-list, Douglas Anderson,
Hans de Goede, Franky Lin, linux-kernel, Madhan Mohan R,
Hante Meuleman, YueHaibing, David S. Miller
In-Reply-To: <20190613234153.59309-1-dianders@chromium.org>
There are certain cases, notably when transitioning between sleep and
active state, when Broadcom SDIO WiFi cards will produce errors on the
SDIO bus. This is evident from the source code where you can see that
we try commands in a loop until we either get success or we've tried
too many times. The comment in the code reinforces this by saying
"just one write attempt may fail"
Unfortunately these failures sometimes end up causing an "-EILSEQ"
back to the core which triggers a retuning of the SDIO card and that
blocks all traffic to the card until it's done.
Let's disable retuning around the commands we expect might fail.
Fixes: bd11e8bd03ca ("mmc: core: Flag re-tuning is needed on CRC errors")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v4:
- Adjust to API rename (Adrian, Ulf).
Changes in v3:
- Expect errors for all of brcmf_sdio_kso_control() (Adrian).
Changes in v2: None
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 4a750838d8cd..ee76593259a7 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -667,6 +667,8 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
brcmf_dbg(TRACE, "Enter: on=%d\n", on);
+ sdio_retune_crc_disable(bus->sdiodev->func1);
+
wr_val = (on << SBSDIO_FUNC1_SLEEPCSR_KSO_SHIFT);
/* 1st KSO write goes to AOS wake up core if device is asleep */
brcmf_sdiod_writeb(bus->sdiodev, SBSDIO_FUNC1_SLEEPCSR, wr_val, &err);
@@ -727,6 +729,8 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
if (try_cnt > MAX_KSO_ATTEMPTS)
brcmf_err("max tries: rd_val=0x%x err=%d\n", rd_val, err);
+ sdio_retune_crc_enable(bus->sdiodev->func1);
+
return err;
}
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v4 2/5] mmc: core: API to temporarily disable retuning for SDIO CRC errors
From: Douglas Anderson @ 2019-06-13 23:41 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter, Arend van Spriel
Cc: brcm80211-dev-list.pdl, linux-rockchip, Double Lo, briannorris,
linux-wireless, Naveen Gupta, Madhan Mohan R, mka, Wright Feng,
Chi-Hsien Lin, netdev, brcm80211-dev-list, Douglas Anderson,
Jiong Wu, Ritesh Harjani, Allison Randal, linux-mmc, linux-kernel,
Thomas Gleixner, Greg Kroah-Hartman, Shawn Lin, Wolfram Sang,
Avri Altman
In-Reply-To: <20190613234153.59309-1-dianders@chromium.org>
Normally when the MMC core sees an "-EILSEQ" error returned by a host
controller then it will trigger a retuning of the card. This is
generally a good idea.
However, if a command is expected to sometimes cause transfer errors
then these transfer errors shouldn't cause a re-tuning. This
re-tuning will be a needless waste of time. One example case where a
transfer is expected to cause errors is when transitioning between
idle (sometimes referred to as "sleep" in Broadcom code) and active
state on certain Broadcom WiFi SDIO cards. Specifically if the card
was already transitioning between states when the command was sent it
could cause an error on the SDIO bus.
Let's add an API that the SDIO function drivers can call that will
temporarily disable the auto-tuning functionality. Then we can add a
call to this in the Broadcom WiFi driver and any other driver that
might have similar needs.
NOTE: this makes the assumption that the card is already tuned well
enough that it's OK to disable the auto-retuning during one of these
error-prone situations. Presumably the driver code performing the
error-prone transfer knows how to recover / retry from errors. ...and
after we can get back to a state where transfers are no longer
error-prone then we can enable the auto-retuning again. If we truly
find ourselves in a case where the card needs to be retuned sometimes
to handle one of these error-prone transfers then we can always try a
few transfers first without auto-retuning and then re-try with
auto-retuning if the first few fail.
Without this change on rk3288-veyron-minnie I periodically see this in
the logs of a machine just sitting there idle:
dwmmc_rockchip ff0d0000.dwmmc: Successfully tuned phase to XYZ
Fixes: bd11e8bd03ca ("mmc: core: Flag re-tuning is needed on CRC errors")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v4:
- Moved to SDIO API only (Adrian, Ulf).
- Renamed to make it less generic, now retune_crc_disable (Ulf).
- Function header makes it clear host must be claimed (Ulf).
- No more WARN_ON (Ulf).
Changes in v3:
- Took out the spinlock since I believe this is all in one context.
Changes in v2:
- Updated commit message to clarify based on discussion of v1.
drivers/mmc/core/core.c | 5 +++--
drivers/mmc/core/sdio_io.c | 36 +++++++++++++++++++++++++++++++++++
include/linux/mmc/core.h | 2 ++
include/linux/mmc/host.h | 1 +
include/linux/mmc/sdio_func.h | 3 +++
5 files changed, 45 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6db36dc870b5..9020cb2490f7 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -144,8 +144,9 @@ void mmc_request_done(struct mmc_host *host, struct mmc_request *mrq)
int err = cmd->error;
/* Flag re-tuning needed on CRC errors */
- if ((cmd->opcode != MMC_SEND_TUNING_BLOCK &&
- cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200) &&
+ if (cmd->opcode != MMC_SEND_TUNING_BLOCK &&
+ cmd->opcode != MMC_SEND_TUNING_BLOCK_HS200 &&
+ !host->retune_crc_disable &&
(err == -EILSEQ || (mrq->sbc && mrq->sbc->error == -EILSEQ) ||
(mrq->data && mrq->data->error == -EILSEQ) ||
(mrq->stop && mrq->stop->error == -EILSEQ)))
diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index f79f0b0caab8..f822a9630b0e 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -734,3 +734,39 @@ int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags)
return 0;
}
EXPORT_SYMBOL_GPL(sdio_set_host_pm_flags);
+
+/**
+ * sdio_retune_crc_disable - temporarily disable retuning on CRC errors
+ * @func: SDIO function attached to host
+ *
+ * If the SDIO card is known to be in a state where it might produce
+ * CRC errors on the bus in response to commands (like if we know it is
+ * transitioning between power states), an SDIO function driver can
+ * call this function to temporarily disable the SD/MMC core behavior of
+ * triggering an automatic retuning.
+ *
+ * This function should be called while the host is claimed and the host
+ * should remain claimed until sdio_retune_crc_enable() is called.
+ * Specifically, the expected sequence of calls is:
+ * - sdio_claim_host()
+ * - sdio_retune_crc_disable()
+ * - some number of calls like sdio_writeb() and sdio_readb()
+ * - sdio_release_host()
+ */
+void sdio_retune_crc_disable(struct sdio_func *func)
+{
+ func->card->host->retune_crc_disable = true;
+}
+EXPORT_SYMBOL_GPL(sdio_retune_crc_disable);
+
+/**
+ * sdio_retune_crc_enable - reneable retuning on CRC errors
+ * @func: SDIO function attached to host
+ *
+ * This is the compement to sdio_retune_crc_disable().
+ */
+void sdio_retune_crc_enable(struct sdio_func *func)
+{
+ func->card->host->retune_crc_disable = false;
+}
+EXPORT_SYMBOL_GPL(sdio_retune_crc_enable);
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 134a6483347a..02a13abf0cda 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -178,6 +178,8 @@ int mmc_wait_for_cmd(struct mmc_host *host, struct mmc_command *cmd,
int mmc_hw_reset(struct mmc_host *host);
int mmc_sw_reset(struct mmc_host *host);
+void mmc_expect_errors_begin(struct mmc_host *host);
+void mmc_expect_errors_end(struct mmc_host *host);
void mmc_set_data_timeout(struct mmc_data *data, const struct mmc_card *card);
#endif /* LINUX_MMC_CORE_H */
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 43d0f0c496f6..ecb7972e2423 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -398,6 +398,7 @@ struct mmc_host {
unsigned int retune_now:1; /* do re-tuning at next req */
unsigned int retune_paused:1; /* re-tuning is temporarily disabled */
unsigned int use_blk_mq:1; /* use blk-mq */
+ unsigned int retune_crc_disable:1; /* don't trigger retune upon crc */
int rescan_disable; /* disable card detection */
int rescan_entered; /* used with nonremovable devices */
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index e9dfdd501cd1..4820e6d09dac 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -167,4 +167,7 @@ extern void sdio_f0_writeb(struct sdio_func *func, unsigned char b,
extern mmc_pm_flag_t sdio_get_host_pm_caps(struct sdio_func *func);
extern int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags);
+extern void sdio_retune_crc_disable(struct sdio_func *func);
+extern void sdio_retune_crc_enable(struct sdio_func *func);
+
#endif /* LINUX_MMC_SDIO_FUNC_H */
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v4 5/5] brcmfmac: sdio: Don't tune while the card is off
From: Douglas Anderson @ 2019-06-13 23:41 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter, Arend van Spriel
Cc: brcm80211-dev-list.pdl, linux-rockchip, Double Lo, briannorris,
linux-wireless, Naveen Gupta, Madhan Mohan R, mka, Wright Feng,
Chi-Hsien Lin, netdev, brcm80211-dev-list, Douglas Anderson,
Franky Lin, linux-kernel, Madhan Mohan R, Hante Meuleman,
YueHaibing, David S. Miller
In-Reply-To: <20190613234153.59309-1-dianders@chromium.org>
When Broadcom SDIO cards are idled they go to sleep and a whole
separate subsystem takes over their SDIO communication. This is the
Always-On-Subsystem (AOS) and it can't handle tuning requests.
Specifically, as tested on rk3288-veyron-minnie (which reports having
BCM4354/1 in dmesg), if I force a retune in brcmf_sdio_kso_control()
when "on = 1" (aka we're transition from sleep to wake) by whacking:
bus->sdiodev->func1->card->host->need_retune = 1
...then I can often see tuning fail. In this case dw_mmc reports "All
phases bad!"). Note that I don't get 100% failure, presumably because
sometimes the card itself has already transitioned away from the AOS
itself by the time we try to wake it up. If I force retuning when "on
= 0" (AKA force retuning right before sending the command to go to
sleep) then retuning is always OK.
NOTE: we need _both_ this patch and the patch to avoid triggering
tuning due to CRC errors in the sleep/wake transition, AKA ("brcmfmac:
sdio: Disable auto-tuning around commands expected to fail"). Though
both patches handle issues with Broadcom's AOS, the problems are
distinct:
1. We want to defer (but not ignore) asynchronous (like
timer-requested) tuning requests till the card is awake. However,
we want to ignore CRC errors during the transition, we don't want
to queue deferred tuning request.
2. You could imagine that the AOS could implement retuning but we
could still get errors while transitioning in and out of the AOS.
Similarly you could imagine a seamless transition into and out of
the AOS (with no CRC errors) even if the AOS couldn't handle
tuning.
ALSO NOTE: presumably there is never a desperate need to retune in
order to wake up the card, since doing so is impossible. Luckily the
only way the card can get into sleep state is if we had a good enough
tuning to send it a sleep command, so presumably that "good enough"
tuning is enough to wake us up, at least with a few retries.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v4:
- Adjust to API rename (Adrian).
Changes in v3:
- ("brcmfmac: sdio: Don't tune while the card is off") new for v3.
Changes in v2: None
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index ee76593259a7..629140b6d7e2 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -669,6 +669,10 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
sdio_retune_crc_disable(bus->sdiodev->func1);
+ /* Cannot re-tune if device is asleep; defer till we're awake */
+ if (on)
+ sdio_retune_hold_now(bus->sdiodev->func1);
+
wr_val = (on << SBSDIO_FUNC1_SLEEPCSR_KSO_SHIFT);
/* 1st KSO write goes to AOS wake up core if device is asleep */
brcmf_sdiod_writeb(bus->sdiodev, SBSDIO_FUNC1_SLEEPCSR, wr_val, &err);
@@ -729,6 +733,9 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
if (try_cnt > MAX_KSO_ATTEMPTS)
brcmf_err("max tries: rd_val=0x%x err=%d\n", rd_val, err);
+ if (on)
+ sdio_retune_release(bus->sdiodev->func1);
+
sdio_retune_crc_enable(bus->sdiodev->func1);
return err;
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v4 4/5] mmc: core: Add sdio_retune_hold_now() and sdio_retune_release()
From: Douglas Anderson @ 2019-06-13 23:41 UTC (permalink / raw)
To: Ulf Hansson, Kalle Valo, Adrian Hunter, Arend van Spriel
Cc: brcm80211-dev-list.pdl, linux-rockchip, Double Lo, briannorris,
linux-wireless, Naveen Gupta, Madhan Mohan R, mka, Wright Feng,
Chi-Hsien Lin, netdev, brcm80211-dev-list, Douglas Anderson,
linux-mmc, linux-kernel, Thomas Gleixner, Greg Kroah-Hartman,
Avri Altman
In-Reply-To: <20190613234153.59309-1-dianders@chromium.org>
We want SDIO drivers to be able to temporarily stop retuning when the
driver knows that the SDIO card is not in a state where retuning will
work (maybe because the card is asleep). We'll move the relevant
functions to a place where drivers can call them.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v4:
- Moved retune hold/release to SDIO API (Adrian).
Changes in v3:
- ("mmc: core: Export mmc_retune_hold_now() mmc_retune_release()") new for v3.
Changes in v2: None
drivers/mmc/core/sdio_io.c | 40 +++++++++++++++++++++++++++++++++++
include/linux/mmc/sdio_func.h | 3 +++
2 files changed, 43 insertions(+)
diff --git a/drivers/mmc/core/sdio_io.c b/drivers/mmc/core/sdio_io.c
index f822a9630b0e..1b6fe737bd72 100644
--- a/drivers/mmc/core/sdio_io.c
+++ b/drivers/mmc/core/sdio_io.c
@@ -15,6 +15,7 @@
#include "sdio_ops.h"
#include "core.h"
#include "card.h"
+#include "host.h"
/**
* sdio_claim_host - exclusively claim a bus for a certain SDIO function
@@ -770,3 +771,42 @@ void sdio_retune_crc_enable(struct sdio_func *func)
func->card->host->retune_crc_disable = false;
}
EXPORT_SYMBOL_GPL(sdio_retune_crc_enable);
+
+/**
+ * sdio_retune_hold_now - start deferring retuning requests till release
+ * @func: SDIO function attached to host
+ *
+ * This function can be called if it's currently a bad time to do
+ * a retune of the SDIO card. Retune requests made during this time
+ * will be held and we'll actually do the retune sometime after the
+ * release.
+ *
+ * This function could be useful if an SDIO card is in a power state
+ * where it can respond to a small subset of commands that doesn't
+ * include the retuning command. Care should be taken when using
+ * this function since (presumably) the retuning request we might be
+ * deferring was made for a good reason.
+ *
+ * This function should be called while the host is claimed.
+ */
+void sdio_retune_hold_now(struct sdio_func *func)
+{
+ mmc_retune_hold_now(func->card->host);
+}
+EXPORT_SYMBOL_GPL(sdio_retune_hold_now);
+
+/**
+ * sdio_retune_release - signal that it's OK to retune now
+ * @func: SDIO function attached to host
+ *
+ * This is the complement to sdio_retune_hold_now(). Calling this
+ * function won't make a retune happen right away but will allow
+ * them to be scheduled normally.
+ *
+ * This function should be called while the host is claimed.
+ */
+void sdio_retune_release(struct sdio_func *func)
+{
+ mmc_retune_release(func->card->host);
+}
+EXPORT_SYMBOL_GPL(sdio_retune_release);
diff --git a/include/linux/mmc/sdio_func.h b/include/linux/mmc/sdio_func.h
index 4820e6d09dac..5a177f7a83c3 100644
--- a/include/linux/mmc/sdio_func.h
+++ b/include/linux/mmc/sdio_func.h
@@ -170,4 +170,7 @@ extern int sdio_set_host_pm_flags(struct sdio_func *func, mmc_pm_flag_t flags);
extern void sdio_retune_crc_disable(struct sdio_func *func);
extern void sdio_retune_crc_enable(struct sdio_func *func);
+extern void sdio_retune_hold_now(struct sdio_func *func);
+extern void sdio_retune_release(struct sdio_func *func);
+
#endif /* LINUX_MMC_SDIO_FUNC_H */
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
From: Lorenzo Bianconi @ 2019-06-13 21:43 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, nbd, lorenzo.bianconi, sgruszka
In-Reply-To: <cover.1560461404.git.lorenzo@kernel.org>
Set usb buffer size taking into account skb_shared_info in order to
not always copy the first part of received frames if A-MSDU is enabled
for SG capable devices. Moreover align usb buffer size to max_ep
boundaries and set buf_size to PAGE_SIZE even for sg case
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/mediatek/mt76/usb.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 1ee54a9b302e..2ee3f8fa1483 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -289,8 +289,10 @@ static int
mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
int nsgs, gfp_t gfp)
{
- int i;
+ int i, data_size;
+ data_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size),
+ dev->usb.in_ep[MT_EP_IN_PKT_RX].max_packet);
for (i = 0; i < nsgs; i++) {
struct page *page;
void *data;
@@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
page = virt_to_head_page(data);
offset = data - page_address(page);
- sg_set_page(&urb->sg[i], page, q->buf_size, offset);
+ sg_set_page(&urb->sg[i], page, data_size, offset);
}
if (i < nsgs) {
@@ -314,7 +316,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
}
urb->num_sgs = max_t(int, i, urb->num_sgs);
- urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
+ urb->transfer_buffer_length = urb->num_sgs * data_size;
sg_init_marker(urb->sg, urb->num_sgs);
return i ? : -ENOMEM;
@@ -611,8 +613,9 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
if (!q->entry)
return -ENOMEM;
- q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
q->ndesc = MT_NUM_RX_ENTRIES;
+ q->buf_size = PAGE_SIZE;
+
for (i = 0; i < q->ndesc; i++) {
err = mt76u_rx_urb_alloc(dev, &q->entry[i]);
if (err < 0)
--
2.21.0
^ permalink raw reply related
* [PATCH v3 wireless-drivers 2/3] mt76: mt76u: introduce mt76u_ep data structure
From: Lorenzo Bianconi @ 2019-06-13 21:43 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, nbd, lorenzo.bianconi, sgruszka
In-Reply-To: <cover.1560461404.git.lorenzo@kernel.org>
Introduce mt76u_ep data structure as a container for usb endpoint info.
This is a preliminary patch to compute proper usb buffer size and avoid
always copy the first part of received frames
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 16 ++++++++++------
drivers/net/wireless/mediatek/mt76/usb.c | 15 +++++++++------
2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 889b76deb703..1c51d6d48e60 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -382,6 +382,11 @@ enum mt76u_out_ep {
__MT_EP_OUT_MAX,
};
+struct mt76u_ep {
+ u16 max_packet;
+ u8 ep;
+};
+
#define MT_SG_MAX_SIZE 8
#define MT_NUM_TX_ENTRIES 256
#define MT_NUM_RX_ENTRIES 128
@@ -393,10 +398,8 @@ struct mt76_usb {
struct tasklet_struct rx_tasklet;
struct delayed_work stat_work;
- u8 out_ep[__MT_EP_OUT_MAX];
- u16 out_max_packet;
- u8 in_ep[__MT_EP_IN_MAX];
- u16 in_max_packet;
+ struct mt76u_ep out_ep[__MT_EP_OUT_MAX];
+ struct mt76u_ep in_ep[__MT_EP_IN_MAX];
bool sg_en;
struct mt76u_mcu {
@@ -786,9 +789,10 @@ mt76u_bulk_msg(struct mt76_dev *dev, void *data, int len, int *actual_len,
unsigned int pipe;
if (actual_len)
- pipe = usb_rcvbulkpipe(udev, usb->in_ep[MT_EP_IN_CMD_RESP]);
+ pipe = usb_rcvbulkpipe(udev, usb->in_ep[MT_EP_IN_CMD_RESP].ep);
else
- pipe = usb_sndbulkpipe(udev, usb->out_ep[MT_EP_OUT_INBAND_CMD]);
+ pipe = usb_sndbulkpipe(udev,
+ usb->out_ep[MT_EP_OUT_INBAND_CMD].ep);
return usb_bulk_msg(udev, pipe, data, len, actual_len, timeout);
}
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index 12d60d31cb51..1ee54a9b302e 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -260,19 +260,22 @@ mt76u_set_endpoints(struct usb_interface *intf,
struct usb_host_interface *intf_desc = intf->cur_altsetting;
struct usb_endpoint_descriptor *ep_desc;
int i, in_ep = 0, out_ep = 0;
+ struct mt76u_ep *ep;
for (i = 0; i < intf_desc->desc.bNumEndpoints; i++) {
ep_desc = &intf_desc->endpoint[i].desc;
if (usb_endpoint_is_bulk_in(ep_desc) &&
in_ep < __MT_EP_IN_MAX) {
- usb->in_ep[in_ep] = usb_endpoint_num(ep_desc);
- usb->in_max_packet = usb_endpoint_maxp(ep_desc);
+ ep = &usb->in_ep[in_ep];
+ ep->max_packet = usb_endpoint_maxp(ep_desc);
+ ep->ep = usb_endpoint_num(ep_desc);
in_ep++;
} else if (usb_endpoint_is_bulk_out(ep_desc) &&
out_ep < __MT_EP_OUT_MAX) {
- usb->out_ep[out_ep] = usb_endpoint_num(ep_desc);
- usb->out_max_packet = usb_endpoint_maxp(ep_desc);
+ ep = &usb->out_ep[out_ep];
+ ep->max_packet = usb_endpoint_maxp(ep_desc);
+ ep->ep = usb_endpoint_num(ep_desc);
out_ep++;
}
}
@@ -386,9 +389,9 @@ mt76u_fill_bulk_urb(struct mt76_dev *dev, int dir, int index,
unsigned int pipe;
if (dir == USB_DIR_IN)
- pipe = usb_rcvbulkpipe(udev, dev->usb.in_ep[index]);
+ pipe = usb_rcvbulkpipe(udev, dev->usb.in_ep[index].ep);
else
- pipe = usb_sndbulkpipe(udev, dev->usb.out_ep[index]);
+ pipe = usb_sndbulkpipe(udev, dev->usb.out_ep[index].ep);
urb->dev = udev;
urb->pipe = pipe;
--
2.21.0
^ permalink raw reply related
* [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
From: Lorenzo Bianconi @ 2019-06-13 21:43 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, nbd, lorenzo.bianconi, sgruszka
In-Reply-To: <cover.1560461404.git.lorenzo@kernel.org>
Commit f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes
for rx") breaks A-MSDU support. When A-MSDU is enable the device can
receive frames up to q->buf_size but they will be discarded in
mt76u_process_rx_entry since there is no enough room for
skb_shared_info. Fix the issue reallocating the skb and copying in the
linear area the first 128B of the received frames and in the frag_list
the remaining part.
Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for rx")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
drivers/net/wireless/mediatek/mt76/usb.c | 49 ++++++++++++++++++-----
2 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 8ecbf81a906f..889b76deb703 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -30,6 +30,7 @@
#define MT_TX_RING_SIZE 256
#define MT_MCU_RING_SIZE 32
#define MT_RX_BUF_SIZE 2048
+#define MT_SKB_HEAD_LEN 128
struct mt76_dev;
struct mt76_wcid;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c b/drivers/net/wireless/mediatek/mt76/usb.c
index bbaa1365bbda..12d60d31cb51 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)
return dma_len;
}
+static struct sk_buff *
+mt76u_build_rx_skb(u8 *data, int len, int buf_size)
+{
+ struct sk_buff *skb;
+
+ if (SKB_WITH_OVERHEAD(buf_size) < MT_DMA_HDR_LEN + len) {
+ struct page *page;
+ int offset;
+
+ /* slow path, not enough space for data and
+ * skb_shared_info
+ */
+ skb = alloc_skb(MT_SKB_HEAD_LEN, GFP_ATOMIC);
+ if (!skb)
+ return NULL;
+
+ skb_put_data(skb, data + MT_DMA_HDR_LEN, MT_SKB_HEAD_LEN);
+ data += (MT_SKB_HEAD_LEN + MT_DMA_HDR_LEN);
+ page = virt_to_head_page(data);
+ offset = data - (u8 *)page_address(page);
+
+ skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
+ page, offset, len - MT_SKB_HEAD_LEN,
+ buf_size);
+
+ return skb;
+ }
+
+ /* fast path */
+ skb = build_skb(data, buf_size);
+ if (!skb)
+ return NULL;
+
+ skb_reserve(skb, MT_DMA_HDR_LEN);
+ __skb_put(skb, len);
+
+ return skb;
+}
+
static int
mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
{
@@ -446,19 +485,11 @@ mt76u_process_rx_entry(struct mt76_dev *dev, struct urb *urb)
return 0;
data_len = min_t(int, len, data_len - MT_DMA_HDR_LEN);
- if (MT_DMA_HDR_LEN + data_len > SKB_WITH_OVERHEAD(q->buf_size)) {
- dev_err_ratelimited(dev->dev, "rx data too big %d\n", data_len);
- return 0;
- }
-
- skb = build_skb(data, q->buf_size);
+ skb = mt76u_build_rx_skb(data, data_len, q->buf_size);
if (!skb)
return 0;
- skb_reserve(skb, MT_DMA_HDR_LEN);
- __skb_put(skb, data_len);
len -= data_len;
-
while (len > 0 && nsgs < urb->num_sgs) {
data_len = min_t(int, len, urb->sg[nsgs].length);
skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
--
2.21.0
^ permalink raw reply related
* [PATCH v3 wireless-drivers 0/3] mt76: usb: fix A-MSDU support
From: Lorenzo Bianconi @ 2019-06-13 21:43 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, nbd, lorenzo.bianconi, sgruszka
Reallocate the skb if there is no enough space to manage the AMSDU rx packets.
Do not always copy the first part of received frames if A-MSDU is enabled
for SG capable devices
Changes since v2:
- simplify mt76u_build_rx_skb
- add patch 2/3: mt76u: introduce mt76u_ep data structure
- align usb buffer size to usb max endpoint length
- set buf_size to PAGE_SIZE even for sg case
Changes since v1:
- do not allocate multiple page buffers but rely on fragmented skbs
if there is no enough space to manage the AMSDU rx packets
Lorenzo Bianconi (3):
mt76: usb: fix rx A-MSDU support
mt76: mt76u: introduce mt76u_ep data structure
mt76: usb: do not always copy the first part of received frames
drivers/net/wireless/mediatek/mt76/mt76.h | 17 +++--
drivers/net/wireless/mediatek/mt76/usb.c | 75 +++++++++++++++++------
2 files changed, 67 insertions(+), 25 deletions(-)
--
2.21.0
^ permalink raw reply
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
From: Brian Norris @ 2019-06-13 20:26 UTC (permalink / raw)
To: Takashi Iwai
Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam,
Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer,
Marcus Meissner
In-Reply-To: <20190613183835.GA16432@google.com>
On Thu, Jun 13, 2019 at 11:38 AM Brian Norris <briannorris@chromium.org> wrote:
> So, I might say:
>
> /* Vendor IEs must at least contain the OUI. */
> if (total_ie_len < offsetof(struct ieee80211_vendor_ie, oui_type))
> return -EINVAL;
>
> /* If the IE still isn't long enough, it's not a match. */
> if (element_len < sizeof(wpa_oui))
> continue;
That would of course need to be break, not continue, to properly skip
to the next IE.
Brian
^ permalink raw reply
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
From: Brian Norris @ 2019-06-13 18:38 UTC (permalink / raw)
To: Takashi Iwai
Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam,
Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer,
Marcus Meissner
In-Reply-To: <s5hmuile4mg.wl-tiwai@suse.de>
On Thu, Jun 13, 2019 at 08:12:39PM +0200, Takashi Iwai wrote:
> On Thu, 13 Jun 2019 19:49:40 +0200,
> Brian Norris wrote:
> > On Wed, May 29, 2019 at 02:52:20PM +0200, Takashi Iwai wrote:
> > > --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> > > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> > > @@ -1269,6 +1269,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> > > break;
> > >
> > > case WLAN_EID_FH_PARAMS:
> > > + if (element_len + 2 < sizeof(*fh_param_set))
> >
> > "element_len + 2" would be much more readable as "total_ie_len". (Same for
> > several other usages in this patch.) I can send such a patch myself as a
> > follow-up I suppose.
>
> Yes, please.
I'll wait until we straighten out the other (potentially) real bug.
Don't want to make needless conflicts.
> > > + return -EINVAL;
> > > fh_param_set =
> > > (struct ieee_types_fh_param_set *) current_ptr;
> > > memcpy(&bss_entry->phy_param_set.fh_param_set,
> >
> > [...]
> >
> > > @@ -1349,6 +1361,9 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> > > break;
> > >
> > > case WLAN_EID_VENDOR_SPECIFIC:
> > > + if (element_len + 2 < sizeof(vendor_ie->vend_hdr))
> >
> > Why 'sizeof(vendor_ie->vend_hdr)'? The (mwifiex-specific compare with the
> > ieee80211.h generic struct ieee80211_vendor_ie) ieee_types_vendor_header struct
> > includes the 'oui_subtype' and 'version' fields, which are not standard
> > requirements for the vendor header (in fact, even the 4th byte of the
> > OUI -- "oui_type" -- doesn't appear to be in the 802.11 specification).
> > So it looks to me like you might be rejecting valid vendor headers (that
> > we should just be skipping) that might have vendor-specific content with
> > length 0 or 1 bytes.
> >
> > It seems like we should only be validating the standard pieces (e.g., up to the
> > length/OUI), and only after an appropriate OUI match, *then* validating the rest of
> > the vendor element (the pieces we'll use later).
>
> Hm, right, that looks too strict. Instead we need to check right
> before both memcmp()'s of OUI.
I think this is the right place for one check (the memcmp() is right
after this line anyway) -- the minimum length just should be smaller.
e.g.:
sizeof(struct ieee80211_vendor_ie) // this is still technically 1 byte too large I think
or
offsetof(struct ieee80211_vendor_ie, oui_type) // not sure if this is the cleanest...
If it's smaller than that, we can still say -EINVAL.
Then, we can go with:
if (element_len < sizeof(wpa_oui)
continue;
or similar.
So, I might say:
/* Vendor IEs must at least contain the OUI. */
if (total_ie_len < offsetof(struct ieee80211_vendor_ie, oui_type))
return -EINVAL;
/* If the IE still isn't long enough, it's not a match. */
if (element_len < sizeof(wpa_oui))
continue;
Brian
^ permalink raw reply
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
From: Takashi Iwai @ 2019-06-13 18:12 UTC (permalink / raw)
To: Brian Norris
Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam,
Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer,
Marcus Meissner
In-Reply-To: <20190613174938.GA260350@google.com>
On Thu, 13 Jun 2019 19:49:40 +0200,
Brian Norris wrote:
>
> Hi Takashi,
>
> On Wed, May 29, 2019 at 02:52:20PM +0200, Takashi Iwai wrote:
> > 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>
> > ---
> > drivers/net/wireless/marvell/mwifiex/scan.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
> > index 64ab6fe78c0d..c269a0de9413 100644
> > --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> > +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> > @@ -1269,6 +1269,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> > break;
> >
> > case WLAN_EID_FH_PARAMS:
> > + if (element_len + 2 < sizeof(*fh_param_set))
>
> "element_len + 2" would be much more readable as "total_ie_len". (Same for
> several other usages in this patch.) I can send such a patch myself as a
> follow-up I suppose.
Yes, please.
> > + return -EINVAL;
> > fh_param_set =
> > (struct ieee_types_fh_param_set *) current_ptr;
> > memcpy(&bss_entry->phy_param_set.fh_param_set,
>
> [...]
>
> > @@ -1349,6 +1361,9 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> > break;
> >
> > case WLAN_EID_VENDOR_SPECIFIC:
> > + if (element_len + 2 < sizeof(vendor_ie->vend_hdr))
>
> Why 'sizeof(vendor_ie->vend_hdr)'? The (mwifiex-specific compare with the
> ieee80211.h generic struct ieee80211_vendor_ie) ieee_types_vendor_header struct
> includes the 'oui_subtype' and 'version' fields, which are not standard
> requirements for the vendor header (in fact, even the 4th byte of the
> OUI -- "oui_type" -- doesn't appear to be in the 802.11 specification).
> So it looks to me like you might be rejecting valid vendor headers (that
> we should just be skipping) that might have vendor-specific content with
> length 0 or 1 bytes.
>
> It seems like we should only be validating the standard pieces (e.g., up to the
> length/OUI), and only after an appropriate OUI match, *then* validating the rest of
> the vendor element (the pieces we'll use later).
Hm, right, that looks too strict. Instead we need to check right
before both memcmp()'s of OUI.
thanks,
Takashi
^ permalink raw reply
* Re: [PATCH] mmc: core: Prevent processing SDIO IRQs when the card is suspended
From: Doug Anderson @ 2019-06-13 18:05 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, linux-wireless, # 4.0+
In-Reply-To: <CAPDyKFqR-xSKdYZYBTK5kKOt1dk7dx_BjedHiDOKs7-X4od=dg@mail.gmail.com>
Hi,
On Thu, Jun 13, 2019 at 2:30 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> > > @@ -937,6 +937,10 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host)
> > > */
> > > static int mmc_sdio_suspend(struct mmc_host *host)
> > > {
> > > + /* Prevent processing of SDIO IRQs in suspended state. */
> > > + mmc_card_set_suspended(host->card);
> >
> > Do you need to claim / release the host around the call to
> > mmc_card_set_suspended() to avoid races?
>
> The intent is that the races should be taken care of like this:
> 1) In MMC_CAP2_SDIO_IRQ_NOTHREAD case, the call to
> cancel_delayed_work_sync() below, will make sure that if there are any
> new work scheduled beyond that point, mmc_card_suspended() will be set
> and process_sdio_pending_irqs() will bail out.
>
> 2. In the non MMC_CAP2_SDIO_IRQ_NOTHREAD case, the call to
> mmc_claim_host() below will make sure if there is any new attempt to
> claim the host from the kthread, mmc_card_suspended() will be set and
> process_sdio_pending_irqs() bails out.
>
> Ideally in the long run and want to remove the SDIO kthread, so
> perhaps this is good enough for now, what do you think?
I was more worried about the safety of mmc_card_set_suspended()
itself. That is:
#define mmc_card_set_suspended(c) ((c)->state |= MMC_STATE_SUSPENDED)
...so it's doing a read-modify-write of "state". Is that safe to do
without any type of locking?
> BTW, the important point is that the call to
> cancel_delayed_work_sync(), must not be done while keeping the host
> claimed, as then it could deadlock.
Definitely. I was thinking of this sequence:
mmc_claim_host(host);
mmc_card_set_suspended(host->card);
mmc_release_host(host);
cancel_delayed_work_sync(&host->sdio_irq_work);
mmc_claim_host(host);
> > > if (!err && host->sdio_irqs) {
> > > if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
> > > wake_up_process(host->sdio_irq_thread);
> > > diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> > > index 931e6226c0b3..9f54a259a1b3 100644
> > > --- a/drivers/mmc/core/sdio_irq.c
> > > +++ b/drivers/mmc/core/sdio_irq.c
> > > @@ -34,6 +34,10 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> > > unsigned char pending;
> > > struct sdio_func *func;
> > >
> > > + /* Don't process SDIO IRQs if the card is suspended. */
> > > + if (mmc_card_suspended(card))
> > > + return 0;
> > > +
> >
> > Is it really OK to just return like this? I guess there are two
> > (somewhat opposite) worries I'd have. See A) and B) below:
>
> Let me comment on A) and B) below, for sure there are more problems to address.
>
> The main reason to why I think it's okay to bail out here, is because
> I think it still improves the current behavior a lot. So, rather than
> solving all problems at once, I wanted to take a step by step
> approach.
>
> >
> > A) Do we need to do anything extra to make sure we actually call the
> > interrupt handler after we've resumed? I guess we can't actually
> > "lose" the interrupt since it will be sitting asserted in CCCR_INTx
> > until we deal with it (right?), but maybe we need to do something to
> > ensure the handler gets called once we're done resuming?
>
> Good point!
>
> Although, it also depends on if we are going to power off the SDIO
> card or not. In other words, if the SDIO IRQ are configured as a
> system wakeup.
Even if it's not a system wakeup, we still don't want to drop the
interrupt on the ground though, do we? For instance, think about a
level-triggered GPIO interrupt that's _not_ a wakeup interrupt. If
that gets asserted in suspend then we won't wakeup the system, but as
soon as the system gets to a certain point in the resume sequence then
we should pass the interrupt on to the handler. If an edge triggered
(but non-wakeup) interrupt fires when the system is resuming then we
should similarly not drop it, should we?
> Moreover there is another related problem, if the SDIO IRQ are
> configured as a system wakeup, and if there is an IRQ raised during
> the system suspend process, the system suspend process should be
> aborted but it may not. This is another issue that currently isn't
> supported. The PM core helps to deals with this, but to take advantage
> of that, the host controller device device must be configured via the
> common wakeup interfaces, such as the device_init_wakeup(), for
> example.
As per earlier discussions I don't have any good examples of SDIO IRQs
being able to wakeup the device to poke at. ...but from GPIO-based
wakeups I'm used to the suspend code masking the interrupt (so it
doesn't fire anymore after the suspend call) but leaving it enabled
and configured as a wakeup. I guess we'd have to think about how that
translates. Your patch seems to be acting as a "mask" of the
interrupt, at least on my dw_mmc tests where the hardware presents the
interrupt like it was edge triggered. ...so it would work OK there
I'd guess.
> > A2): new MMC_CAP2_SDIO_IRQ_NOTHREAD case
> >
> > Should we do something to re-kick things? We could call
> > sdio_signal_irq() in mmc_sdio_resume() I guess? I was worried that
> > might conflict with those that call sdio_run_irqs() directly but it
> > seems like that's nobody as of commit 89f3c365f3e1 ("mmc: sdhci: Fix
> > SDIO IRQ thread deadlock").
>
> Good point!
>
> Again, whether we should re-kick things depends on if the SDIO IRQ is
> configured as wakeup, but in general using sdio_signal_irq() should
> work.
>
> The other part I am considering is to disable the SDIO irq, in case of
> "mmc_card_keep_power() && !mmc_card_wake_sdio_irq()".
>
> Moreover, if !mmc_card_keep_power(), then there really shouldn't be
> any IRQs registered so perhaps we should add a special check for that
> and return an error code.
I haven't looked through all the details here but I can dig if you
want. On other drivers it's generally OK to leave your interrupt
registered (just disabled and/or masked) across suspend/resume, but
maybe that's not OK for SDIO cards without keep power?
> In regards to other callers of sdio_run_irqs(). I have a patch that
> makes it this function static, as it really should not need to be used
> other than from the work queue path. Let me post it asap to cover that
> gap. Again, thanks for pointing this out!
Yeah, I was thinking of posting that too, but happy to have you do it! :-)
> > ...side note: overall looking at this code path, two additional
> > questions come up for me. One is why sdio_run_irqs() hardcodes
> > "sdio_irq_pending" as true. That means we won't _ever_ poll CCCR_INTx
> > in the 1-function case, right? That seems wrong. The other is why
>
> In the 1-function case, the idea is that we don't have to read the
> CCCR_INTx to find out what func number the IRQ belongs to.
>
> This is the same behavior consistent as with the kthread case, see
> mmc_signal_sdio_irq(), unless I am mistaken.
I think there's at least the bug that nothing will ever set
"sdio_irq_pending" to false in the MMC_CAP2_SDIO_IRQ_NOTHREAD case,
right? So we'll set it to true the first time and from then on out it
will never be false again?
> > mmc_sdio_resume() always calls host->ops->enable_sdio_irq(host, 1) at
> > resume time when nobody ever turned the IRQs off.
>
> That's correct and it leads to unbalanced calls of
> host->ops->enable_sdio_irq(). This needs to be fixed as well.
>
> >
> > ===
> >
> > B) Are there any instances where the interrupt will just keep firing
> > over and over again because we don't handle it?
> >
> > As per above, this _isn't_ happening on dw_mmc on my setup because
> > dw_mmc seems to treat the SDIO interrupt as edge triggered. ...but is
> > this true everywhere? If we were using SDIO in 1-bit mode on dw_mmc,
> > would the interrupt re-assert right away? If dw_mmc were configured
> > to use a dedicated pin would it re-assert right away? What about
> > other host controllers?
> >
> > If you're sure no host controllers will keep asserting the interrupt
> > over and over then I guess we don't need to worry about it?
> > ...otherwise we'd need to find some way to mask the interrupt and we'd
> > need to make sure whatever we do doesn't interfere with anyone who
> > supports the SDIO interrupt as a wake source, right?
>
> For the MMC_CAP2_SDIO_IRQ_NOTHREAD case, the expected behavior by the
> host driver is to prior calling sdio_signal_irq(), is should temporary
> disable the SDIO IRQ. Then, when the host->ops->ack_sdio_irq is called
> from the work, the IRQ has been processed, which tells the host driver
> to re-enable the SDIO IRQ.
So what I'm imagining is this:
1. mmc_sdio_suspend() starts; calls mmc_card_set_suspended() and
cancel_delayed_work_sync().
2. SDIO interrupt comes in; host controller calls sdio_signal_irq()
3. sdio_signal_irq() queues delayed work, which gets scheduled right away.
4. sdio_run_irqs() calls process_sdio_pending_irqs() which is a no-op
(because we're suspended)
5. sdio_run_irqs() calls host->ops->ack_sdio_irq(), which re-enables
more interrupts.
6. If SDIO interrupt was truly level triggered, we'll go straight back
to #2 because we never actually removed the true source of the
level-triggered interrupt by handling it.
We'll run steps #2 - #6 above ad nauseam until we finally manage to
get to the point in the suspend process where the system actually
masks/disables all driver interrupts. This happens sometime _after_
the host controller's suspend call happens. Technically this might
not really hurt anything (other than burning CPU cycles) because the
system workqueue isn't all that high priority so I think the suspend
can continue happening while we're looping. ...but it still doesn't
seem great.
We don't end up in the above situation in my tests because the SDIO
interrupt was acting as an edge triggered interrupt. ...and because,
as per below, we eventually turn the clock off.
> In the kthread case, this is managed by mmc_signal_sdio_irq() and the
> sdio_irq_thread() that calls host->ops->enable_sdio_irq() both to
> enable/disable the IRQ (but there are other problems with that).
>
> >
> > ======
> >
> > Overall, I can confirm that on my system your patch actually does
> > work. ...so if all of the above concerns are moot and won't cause
> > anyone else problems then I can say that they don't seem to cause any
> > problems on my system. On rk3288-veyron-jerry:
> >
> > - Before your patch, I got failures at iteration 18, then 32, then 55,
> > then 7, then 26.
> >
> > - After your patch I could do 100 iterations of suspend/resume with no
> > failures. I also put printouts to confirm your patch was having an
> > effect.
>
> Great news, thanks a lot for testing and sharing these result.
>
> One more thing to consider. After the system suspend callback have
> been called for the mmc host driver (assuming SDIO IRQ isn't
> configured as system wakeup), the host driver shouldn't really receive
> SDIO IRQs and nor should it signal them via sdio_signal_irq(), simply
> because it has suspended its device/controller and beyond that point,
> the behavior might be undefined. Can you check to see if this is
> happening, or possibly you already know that this is the case and that
> we are "lucky"?
It's happening fine as long as we're loose with the term "after". :-)
Most certainly when we just finished executing the last line of the
host controller's suspend call then the system can't have done
anything to prevent interrupts from going off. Even if the very next
thing that the core OS did was to disable interrupts there would still
be at least a few CPU instructions in there where we could have
finished the suspend call and interrupts were still enabled at the
system level.
It looks like the actual suspension of interrupts is in
suspend_device_irqs() which is called right before the "no irq" calls
are made. ...so in theory we could still get interrupts for quite a
while after the host controller's suspend call.
In practice it actually looks to be impossible for dw_mmc, though.
...part of dw_mmc's suspend call turns off both the ciu (card clock)
and biu (bus clock). I believe this means that the controller is
fully unclocked and there's no way it could give an interrupt.
In fact, the only time we actually get into trouble in dw_mmc is right
at the beginning of the resume code where we start re-initting the
host controller (and turning its clocks on) and then the interrupt
fires before we're quite ready.
-Doug
^ permalink raw reply
* Cleanup of -Wunused-const-variable in drivers/net/wireless/ti/wl18xx/main.c
From: Nathan Huckleberry @ 2019-06-13 18:00 UTC (permalink / raw)
To: eliad, kvalo, davem; +Cc: netdev, linux-wireless, clang-built-linux
Hey all,
I'm looking into cleaning up ignored warnings in the kernel so we can
remove compiler flags to ignore warnings.
There are two unused variables ('wl18xx_iface_ap_cl_limits' and
'wl18xx_iface_ap_go_limits') in drivers/net/wireless/ti/wl18xx/main.c.
These appear to be limits when using p2p devices, yet they are never
used.
Wanted to reach out for the best course of action to fix the warning.
https://github.com/ClangBuiltLinux/linux/issues/530
Thanks,
Nathan Huckleberry
^ permalink raw reply
* Re: [PATCH 2/2] mwifiex: Abort at too short BSS descriptor element
From: Brian Norris @ 2019-06-13 17:49 UTC (permalink / raw)
To: Takashi Iwai
Cc: linux-wireless, Amitkumar Karwar, Nishant Sarmukadam,
Ganapathi Bhat, Xinming Hu, Kalle Valo, huangwen, Solar Designer,
Marcus Meissner
In-Reply-To: <20190529125220.17066-3-tiwai@suse.de>
Hi Takashi,
On Wed, May 29, 2019 at 02:52:20PM +0200, Takashi Iwai wrote:
> 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>
> ---
> drivers/net/wireless/marvell/mwifiex/scan.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
> index 64ab6fe78c0d..c269a0de9413 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -1269,6 +1269,8 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> break;
>
> case WLAN_EID_FH_PARAMS:
> + if (element_len + 2 < sizeof(*fh_param_set))
"element_len + 2" would be much more readable as "total_ie_len". (Same for
several other usages in this patch.) I can send such a patch myself as a
follow-up I suppose.
> + return -EINVAL;
> fh_param_set =
> (struct ieee_types_fh_param_set *) current_ptr;
> memcpy(&bss_entry->phy_param_set.fh_param_set,
[...]
> @@ -1349,6 +1361,9 @@ int mwifiex_update_bss_desc_with_ie(struct mwifiex_adapter *adapter,
> break;
>
> case WLAN_EID_VENDOR_SPECIFIC:
> + if (element_len + 2 < sizeof(vendor_ie->vend_hdr))
Why 'sizeof(vendor_ie->vend_hdr)'? The (mwifiex-specific compare with the
ieee80211.h generic struct ieee80211_vendor_ie) ieee_types_vendor_header struct
includes the 'oui_subtype' and 'version' fields, which are not standard
requirements for the vendor header (in fact, even the 4th byte of the
OUI -- "oui_type" -- doesn't appear to be in the 802.11 specification).
So it looks to me like you might be rejecting valid vendor headers (that
we should just be skipping) that might have vendor-specific content with
length 0 or 1 bytes.
It seems like we should only be validating the standard pieces (e.g., up to the
length/OUI), and only after an appropriate OUI match, *then* validating the rest of
the vendor element (the pieces we'll use later).
Brian
> + return -EINVAL;
> +
> vendor_ie = (struct ieee_types_vendor_specific *)
> current_ptr;
>
> --
> 2.16.4
>
^ permalink raw reply
* Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook
From: Benjamin Herrenschmidt @ 2019-06-12 21:59 UTC (permalink / raw)
To: Larry Finger, Christoph Hellwig
Cc: Aaro Koskinen, linux-wireless, linux-kernel, Christian Zigotzky,
linuxppc-dev
In-Reply-To: <d6d82c0d-4a40-a191-0414-6b9a64547f65@lwfinger.net>
On Wed, 2019-06-12 at 14:41 -0500, Larry Finger wrote:
> On 6/12/19 1:55 AM, Christoph Hellwig wrote:
> >
> > Ooops, yes. But I think we could just enable ZONE_DMA on 32-bit
> > powerpc. Crude enablement hack below:
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 8c1c636308c8..1dd71a98b70c 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -372,7 +372,7 @@ config PPC_ADV_DEBUG_DAC_RANGE
> >
> > config ZONE_DMA
> > bool
> > - default y if PPC_BOOK3E_64
> > + default y
> >
> > config PGTABLE_LEVELS
> > int
> >
>
> With the patch for Kconfig above, and the original patch setting
> ARCH_ZONE_DMA_BITS to 30, everything works.
>
> Do you have any ideas on what should trigger the change in ARCH_ZONE_BITS?
> Should it be CONFIG_PPC32 defined, or perhaps CONFIG_G4_CPU defined?
I think CONFIG_PPC32 is fine
Ben.
^ permalink raw reply
* Re: [PATCH] mmc: core: Prevent processing SDIO IRQs when the card is suspended
From: Doug Anderson @ 2019-06-12 22:20 UTC (permalink / raw)
To: Ulf Hansson
Cc: Linux MMC List, Adrian Hunter, Brian Norris, Shawn Lin,
Guenter Roeck, Heiko Stuebner, Kalle Valo, linux-wireless, # 4.0+
In-Reply-To: <20190611123221.11580-1-ulf.hansson@linaro.org>
Hi,
On Tue, Jun 11, 2019 at 5:32 AM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> Processing of SDIO IRQs must obviously be prevented while the card is
> system suspended, otherwise we may end up trying to communicate with an
> uninitialized SDIO card.
>
> Reports throughout the years shows that this is not only a theoretical
> problem, but a real issue. So, let's finally fix this problem, by keeping
> track of the state for the card and bail out before processing the SDIO
> IRQ, in case the card is suspended.
>
> Cc: stable@vger.kernel.org
> Reported-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>
> This has only been compile tested so far, any help for real test on HW is
> greatly appreciated.
Thanks for sending this!
> Note that, this is only the initial part of what is needed to make power
> management of SDIO card more robust, but let's start somewhere and continue to
> improve things.
>
> The next step I am looking at right now, is to make sure the SDIO IRQ is turned
> off during system suspend, unless it's supported as a system wakeup (and enabled
> to be used).
My gut says that the partway solution is going to be a problem on some
controllers / systems, even though it seems to work OK on mine. See
my thoughts below and let me know what you think.
> ---
> drivers/mmc/core/sdio.c | 7 +++++++
> drivers/mmc/core/sdio_irq.c | 4 ++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/drivers/mmc/core/sdio.c b/drivers/mmc/core/sdio.c
> index d1aa1c7577bb..9951295d3220 100644
> --- a/drivers/mmc/core/sdio.c
> +++ b/drivers/mmc/core/sdio.c
> @@ -937,6 +937,10 @@ static int mmc_sdio_pre_suspend(struct mmc_host *host)
> */
> static int mmc_sdio_suspend(struct mmc_host *host)
> {
> + /* Prevent processing of SDIO IRQs in suspended state. */
> + mmc_card_set_suspended(host->card);
Do you need to claim / release the host around the call to
mmc_card_set_suspended() to avoid races?
> + cancel_delayed_work_sync(&host->sdio_irq_work);
> +
> mmc_claim_host(host);
>
> if (mmc_card_keep_power(host) && mmc_card_wake_sdio_irq(host))
> @@ -985,6 +989,9 @@ static int mmc_sdio_resume(struct mmc_host *host)
> err = sdio_enable_4bit_bus(host->card);
> }
>
> + /* Allow SDIO IRQs to be processed again. */
> + mmc_card_clr_suspended(host->card);
> +
Do you need to check for "!err" before calling
mmc_card_clr_suspended()? ...or add an "if (err) goto exit" type
thing and get rid of the "!err" check below?
> if (!err && host->sdio_irqs) {
> if (!(host->caps2 & MMC_CAP2_SDIO_IRQ_NOTHREAD))
> wake_up_process(host->sdio_irq_thread);
> diff --git a/drivers/mmc/core/sdio_irq.c b/drivers/mmc/core/sdio_irq.c
> index 931e6226c0b3..9f54a259a1b3 100644
> --- a/drivers/mmc/core/sdio_irq.c
> +++ b/drivers/mmc/core/sdio_irq.c
> @@ -34,6 +34,10 @@ static int process_sdio_pending_irqs(struct mmc_host *host)
> unsigned char pending;
> struct sdio_func *func;
>
> + /* Don't process SDIO IRQs if the card is suspended. */
> + if (mmc_card_suspended(card))
> + return 0;
> +
Is it really OK to just return like this? I guess there are two
(somewhat opposite) worries I'd have. See A) and B) below:
A) Do we need to do anything extra to make sure we actually call the
interrupt handler after we've resumed? I guess we can't actually
"lose" the interrupt since it will be sitting asserted in CCCR_INTx
until we deal with it (right?), but maybe we need to do something to
ensure the handler gets called once we're done resuming?
A1) old SDIO thread case
I think we'll be OK in the old SDIO thread case. We'll call
wake_up_process() after we clear the suspended state and then we'll
either see "sdio_irq_pending" was set to true or we'll poll CCCR_INTx.
--
A2): new MMC_CAP2_SDIO_IRQ_NOTHREAD case
Should we do something to re-kick things? We could call
sdio_signal_irq() in mmc_sdio_resume() I guess? I was worried that
might conflict with those that call sdio_run_irqs() directly but it
seems like that's nobody as of commit 89f3c365f3e1 ("mmc: sdhci: Fix
SDIO IRQ thread deadlock").
NOTE: I put a bunch of debug printouts and I'm fairly convinced that
this is a real problem. Sort of. Specifically I confirmed that in
dw_mmc the SDIO interrupt seems to be treated as an edge-triggered
interrupt. AKA: in dw_mci_interrupt() when we write to "RINTSTS" as
we're handling the interrupt the interrupt immediately stops
asserting. It doesn't actually fire again until the Marvell SDIO
resume functions run. I didn't dig enough to figure out what
specifically makes the interrupt fire again in the Marvell resume
functions, but it seems a little concerning that we're relying on
something in that driver to re-kick the host controller interrupt.
...side note: overall looking at this code path, two additional
questions come up for me. One is why sdio_run_irqs() hardcodes
"sdio_irq_pending" as true. That means we won't _ever_ poll CCCR_INTx
in the 1-function case, right? That seems wrong. The other is why
mmc_sdio_resume() always calls host->ops->enable_sdio_irq(host, 1) at
resume time when nobody ever turned the IRQs off.
===
B) Are there any instances where the interrupt will just keep firing
over and over again because we don't handle it?
As per above, this _isn't_ happening on dw_mmc on my setup because
dw_mmc seems to treat the SDIO interrupt as edge triggered. ...but is
this true everywhere? If we were using SDIO in 1-bit mode on dw_mmc,
would the interrupt re-assert right away? If dw_mmc were configured
to use a dedicated pin would it re-assert right away? What about
other host controllers?
If you're sure no host controllers will keep asserting the interrupt
over and over then I guess we don't need to worry about it?
...otherwise we'd need to find some way to mask the interrupt and we'd
need to make sure whatever we do doesn't interfere with anyone who
supports the SDIO interrupt as a wake source, right?
======
Overall, I can confirm that on my system your patch actually does
work. ...so if all of the above concerns are moot and won't cause
anyone else problems then I can say that they don't seem to cause any
problems on my system. On rk3288-veyron-jerry:
- Before your patch, I got failures at iteration 18, then 32, then 55,
then 7, then 26.
- After your patch I could do 100 iterations of suspend/resume with no
failures. I also put printouts to confirm your patch was having an
effect.
I also confirmed that rk3288-veyron-minnie (which has Broadcom WiFi) I
could still suspend/resume fine with your patch.
-Doug
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox