* [PATCH] carl9170: fix enum compare splat
From: Christian Lamparter @ 2019-06-08 14:49 UTC (permalink / raw)
To: linux-wireless; +Cc: Kalle Valo
This patch fixes a noisy warning triggered by -Wenum-compare
|main.c:1390:31: warning: comparison between ‘enum nl80211_ac’
| and ‘enum ar9170_txq’ [-Wenum-compare]
| BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
| ^
| [...]
This is a little bit unfortunate, since the number of queues
(hence NL80211_NUM_ACS) is a constant based on the IEEE 802.11
(much like IEEE80211_NUM_ACS) and __AR9170_NUM_TXQ is more or
less defined by the AR9170 hardware.
Signed-off-by: Christian Lamparter <chunkeey@gmail.com>
---
drivers/net/wireless/ath/carl9170/main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
index eaea08581cea..acc881bd7f9f 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1387,7 +1387,7 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
int ret;
BUILD_BUG_ON(ARRAY_SIZE(ar9170_qmap) != __AR9170_NUM_TXQ);
- BUILD_BUG_ON(NL80211_NUM_ACS > __AR9170_NUM_TXQ);
+ BUILD_BUG_ON((size_t)NL80211_NUM_ACS > (size_t)__AR9170_NUM_TXQ);
mutex_lock(&ar->mutex);
memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
ret = carl9170_set_qos(ar);
--
2.20.1
^ permalink raw reply related
* [PATCH] mt76: mt7615: simplify mt7615_mcu_set_sta_rec routine
From: Lorenzo Bianconi @ 2019-06-08 14:44 UTC (permalink / raw)
To: nbd; +Cc: lorenzo.bianconi, linux-wireless, ryder.lee, royluo
Move conn_type configuration directly in mt7615_mcu_set_sta_rec and
remove sta_rec_convert_vif_type since it is actually used just in
mt7615_mcu_set_sta_rec
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
.../net/wireless/mediatek/mt76/mt7615/mcu.c | 33 +++++++------------
1 file changed, 12 insertions(+), 21 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
index 76431d00a8ac..79baa455034c 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/mcu.c
@@ -1040,30 +1040,11 @@ int mt7615_mcu_set_sta_rec_bmc(struct mt7615_dev *dev,
&req, sizeof(req), true);
}
-static void sta_rec_convert_vif_type(enum nl80211_iftype type, u32 *conn_type)
-{
- switch (type) {
- case NL80211_IFTYPE_AP:
- case NL80211_IFTYPE_MESH_POINT:
- if (conn_type)
- *conn_type = CONNECTION_INFRA_STA;
- break;
- case NL80211_IFTYPE_STATION:
- if (conn_type)
- *conn_type = CONNECTION_INFRA_AP;
- break;
- default:
- WARN_ON(1);
- break;
- };
-}
-
int mt7615_mcu_set_sta_rec(struct mt7615_dev *dev, struct ieee80211_vif *vif,
struct ieee80211_sta *sta, bool en)
{
struct mt7615_vif *mvif = (struct mt7615_vif *)vif->drv_priv;
struct mt7615_sta *msta = (struct mt7615_sta *)sta->drv_priv;
- u32 conn_type = 0;
struct {
struct sta_req_hdr hdr;
@@ -1085,8 +1066,18 @@ int mt7615_mcu_set_sta_rec(struct mt7615_dev *dev, struct ieee80211_vif *vif,
};
memcpy(req.basic.peer_addr, sta->addr, ETH_ALEN);
- sta_rec_convert_vif_type(vif->type, &conn_type);
- req.basic.conn_type = cpu_to_le32(conn_type);
+ switch (vif->type) {
+ case NL80211_IFTYPE_AP:
+ case NL80211_IFTYPE_MESH_POINT:
+ req.basic.conn_type = cpu_to_le32(CONNECTION_INFRA_STA);
+ break;
+ case NL80211_IFTYPE_STATION:
+ req.basic.conn_type = cpu_to_le32(CONNECTION_INFRA_AP);
+ break;
+ default:
+ WARN_ON(1);
+ break;
+ };
if (en) {
req.basic.conn_state = CONN_STATE_PORT_SECURE;
--
2.21.0
^ permalink raw reply related
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Ard Biesheuvel @ 2019-06-08 14:37 UTC (permalink / raw)
To: Sandy Harris
Cc: Denis Kenzior, Eric Biggers, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <CACXcFm=2_1S75G7NWRCQjBS6gi+vDZFROzg6Ntjh-fAcPfYhyQ@mail.gmail.com>
On Sat, 8 Jun 2019 at 15:03, Sandy Harris <sandyinchina@gmail.com> wrote:
>
> First off, it is not clear we should implement WEP at all since it is
> fatally flawed. This has been known for about a decade, there have
> been at least two better algorithms added to the standards, & the only
> reason anyone would need WEP today would be to connect to an old
> router in an obviously insecure way.
> https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html
> https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html
>
> Twenty years ago the FreeS/WAN project implemented IPsec for Linux &
> deliberately did not include things like single DES which were known
> to be insecure:
> https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped
> I think a similar policy was would be a fine idea for the kernel today
> & WEP is hopelessly insecure.
>
It is actually pretty clear that we should implement WEP, simply
because we already do. We all know how broken it is, but that does not
mean we should be the ones policing its use. People may have good
reasons to stick with WEP in their particular use case, or maybe they
have bad reasons, but the bottom line is that it does not really
matter: if it works today, we can't just remove it.
What we can do is make the existing code less of an eyesore than it
already is, and in the context of what I want to achieve for the
crypto API, this involves moving it from the cipher API to something
else.
> > > As I am attempting to explain, ecb(arc4) does not implement this API correctly
> > > because it updates the *key* after each operation, not the IV. I doubt this is
> > > documented anywhere, but this can only be changed if people aren't relying on it
> > > already.
>
> It is more the case that the API does not apply to arc4, or more
> generally to stream ciphers, than that "ecb(arc4) does not implement
> this API correctly".
>
> ECB (electronic code book) is a mode of operation for block ciphers
> https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
> Stream ciphers do not have those modes.
>
This is exactly the point Eric was making. Our skcipher abstraction
deals with stream ciphers fine, but the way the arc4 code is exposed
as ecb(arc4) and updates the key in the process makes absolutely no
sense.
> For that matter, not all block cipher modes use an IV. The very common
> CBC mode -- the only mode used in IPsec, for example -- does, but
> others including ECB do not. I do not know of any mode that ever
> updates the IV. CBC uses the IV with the first block & on all other
> blocks uses the ciphertext from the previous block the same way; one
> might call that updating the IV I suppose, but I do not see why one
> would want to.
>
If you want to split up a CBC transformation into several invocations
of the underlying API, then the last ciphertext block of the first
call serves as the IV for the next call. Arguing that we should not be
calling this an IV serves little purpose, since the code already
treats it exactly the same. In fact, our CTS template relies on this
feature as well, so a CBC implementation that does not return the last
ciphertext block in the IV buffer is broken wrt our API requirements.
> > It sounds to me like it was broken and should be fixed. So our vote /
> > preference is to have ARC4 fixed to follow the proper semantics.
>
> As I see it, those are clearly not "he proper semantics" for a stream
> cipher & the question of forcing it into them should not even arise.
>
> One alternative would be to drop arc4. That would make sense if WEP is
> the only usage & we elect to drop WEP. One could also argue the arc4
> itself is insecure & should go, but I'm not sure that is accurate.
> Certainly there have been some published attacks & other stream
> ciphers are now generally preferrred, but I have not followed things
> closely enough to know if RC$ should be considered fatally flawed.
>
> A better choice might be to change the interface, defining a new
> interface for stream ciphers and/or generalising the interface so it
> works for either stream ciphers or block ciphers.
Dropping WEP is out of the question, and apparently, there are
userspace dependencies on the ecb(arc4) cipher as well, so
unfortunately, we have already painted ourselves into a corner here.
skcipher works fine for block ciphers wrapped in CTR mode, and for
chacha/salsa as well, so I don't think there is a problem with the API
for other stream ciphers we care about. Calling the rc4 skcipher
'ecb(arc4)' was obviously a mistake, but it seems we're stuck with
that as well :-(
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Sandy Harris @ 2019-06-08 13:03 UTC (permalink / raw)
To: Denis Kenzior
Cc: Eric Biggers, Ard Biesheuvel, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <61e1cd8a-4891-4e37-417e-1c31cd95a278@gmail.com>
First off, it is not clear we should implement WEP at all since it is
fatally flawed. This has been known for about a decade, there have
been at least two better algorithms added to the standards, & the only
reason anyone would need WEP today would be to connect to an old
router in an obviously insecure way.
https://www.schneier.com/blog/archives/2007/04/breaking_wep_in.html
https://www.tomshardware.com/reviews/wireless-security-hack,2981-4.html
Twenty years ago the FreeS/WAN project implemented IPsec for Linux &
deliberately did not include things like single DES which were known
to be insecure:
https://www.freeswan.org/freeswan_trees/freeswan-1.99/doc/compat.html#dropped
I think a similar policy was would be a fine idea for the kernel today
& WEP is hopelessly insecure.
> > As I am attempting to explain, ecb(arc4) does not implement this API correctly
> > because it updates the *key* after each operation, not the IV. I doubt this is
> > documented anywhere, but this can only be changed if people aren't relying on it
> > already.
It is more the case that the API does not apply to arc4, or more
generally to stream ciphers, than that "ecb(arc4) does not implement
this API correctly".
ECB (electronic code book) is a mode of operation for block ciphers
https://en.wikipedia.org/wiki/Block_cipher_mode_of_operation
Stream ciphers do not have those modes.
For that matter, not all block cipher modes use an IV. The very common
CBC mode -- the only mode used in IPsec, for example -- does, but
others including ECB do not. I do not know of any mode that ever
updates the IV. CBC uses the IV with the first block & on all other
blocks uses the ciphertext from the previous block the same way; one
might call that updating the IV I suppose, but I do not see why one
would want to.
> It sounds to me like it was broken and should be fixed. So our vote /
> preference is to have ARC4 fixed to follow the proper semantics.
As I see it, those are clearly not "he proper semantics" for a stream
cipher & the question of forcing it into them should not even arise.
One alternative would be to drop arc4. That would make sense if WEP is
the only usage & we elect to drop WEP. One could also argue the arc4
itself is insecure & should go, but I'm not sure that is accurate.
Certainly there have been some published attacks & other stream
ciphers are now generally preferrred, but I have not followed things
closely enough to know if RC$ should be considered fatally flawed.
A better choice might be to change the interface, defining a new
interface for stream ciphers and/or generalising the interface so it
works for either stream ciphers or block ciphers.
^ permalink raw reply
* rtw88: M.2 RTL8822BE not working - rfe 3 isn't supported
From: g.schlmm @ 2019-06-08 12:26 UTC (permalink / raw)
To: linux-wireless
my RTL8822BE M.2 card is not working with linux 5.2rc3
the staging r8822be driver in linux 5.1 was working for this card
from dmesg:
> [ 8.001186] rtw_pci 0000:04:00.0: rfe 3 isn't supported
> [ 8.003870] rtw_pci 0000:04:00.0: failed to setup chip efuse info
> [ 8.006405] rtw_pci 0000:04:00.0: failed to setup chip information
lspci:
> 04:00.0 Unassigned class [ff00]: Realtek Semiconductor Co., Ltd. RTL8822BE 802.11a/b/g/n/ac WiFi adapter
> Subsystem: Lenovo RTL8822BE 802.11a/b/g/n/ac WiFi adapter
> Flags: fast devsel, IRQ 19
> I/O ports at c000 [size=256]
> Memory at 81200000 (64-bit, non-prefetchable) [size=64K]
> Capabilities: [40] Power Management version 3
> Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+
> Capabilities: [70] Express Endpoint, MSI 00
> Capabilities: [100] Advanced Error Reporting
> Capabilities: [148] Device Serial Number 00-e0-4c-ff-fe-b8-22-01
> Capabilities: [158] Latency Tolerance Reporting
> Capabilities: [160] L1 PM Substates
> Kernel modules: rtwpci
^ permalink raw reply
* [PATCH AUTOSEL 5.1 07/70] staging: wilc1000: Fix some double unlock bugs in wilc_wlan_cleanup()
From: Sasha Levin @ 2019-06-08 11:38 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Dan Carpenter, Greg Kroah-Hartman, Sasha Levin, linux-wireless,
devel
In-Reply-To: <20190608113950.8033-1-sashal@kernel.org>
From: Dan Carpenter <dan.carpenter@oracle.com>
[ Upstream commit fea69916360468e364a4988db25a5afa835f3406 ]
If ->hif_read_reg() or ->hif_write_reg() fail then the code unlocks
and keeps executing. It should just return.
Fixes: c5c77ba18ea6 ("staging: wilc1000: Add SDIO/SPI 802.11 driver")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/staging/wilc1000/wilc_wlan.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/wilc1000/wilc_wlan.c b/drivers/staging/wilc1000/wilc_wlan.c
index c2389695fe20..70b1ab21f8a3 100644
--- a/drivers/staging/wilc1000/wilc_wlan.c
+++ b/drivers/staging/wilc1000/wilc_wlan.c
@@ -1076,13 +1076,17 @@ void wilc_wlan_cleanup(struct net_device *dev)
acquire_bus(wilc, WILC_BUS_ACQUIRE_AND_WAKEUP);
ret = wilc->hif_func->hif_read_reg(wilc, WILC_GP_REG_0, ®);
- if (!ret)
+ if (!ret) {
release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+ return;
+ }
ret = wilc->hif_func->hif_write_reg(wilc, WILC_GP_REG_0,
(reg | ABORT_INT));
- if (!ret)
+ if (!ret) {
release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
+ return;
+ }
release_bus(wilc, WILC_BUS_RELEASE_ALLOW_SLEEP);
wilc->hif_func->hif_deinit(NULL);
--
2.20.1
^ permalink raw reply related
* [PATCH] rtlwifi: rtl8188ee: remove redundant assignment to rtstatus
From: Colin King @ 2019-06-08 10:58 UTC (permalink / raw)
To: Ping-Ke Shih, Kalle Valo, David S . Miller, linux-wireless,
netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Variable rtstatus is being initialized with a value that is never read
as rtstatus is being re-assigned a little later on. The assignment is
redundant and hence can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
index 454bab38b165..f92e95f5494f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
@@ -1039,7 +1039,7 @@ int rtl88ee_hw_init(struct ieee80211_hw *hw)
struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
struct rtl_efuse *rtlefuse = rtl_efuse(rtl_priv(hw));
- bool rtstatus = true;
+ bool rtstatus;
int err = 0;
u8 tmp_u1b, u1byte;
unsigned long flags;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 4/4] mt76: mt76x02: fix tx reordering on rate control probing without a-mpdu
From: Felix Fietkau @ 2019-06-08 10:42 UTC (permalink / raw)
To: linux-wireless
In-Reply-To: <20190607164355.51876-4-nbd@nbd.name>
To avoid aggregating rate control probing packets with other traffic, and to
ensure that the probing rate gets used, probing packets get assigned a different
internal queueing priority.
This causes packets to be transmitted in a different order, which is compensated
by the receiver side reordering.
However, if A-MPDU is disabled, this reordering can become visible to upper
layers on the receiver side. Disable the priority change if A-MPDU is disabled.
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
v2: fix compile error
drivers/net/wireless/mediatek/mt76/mt76x02_txrx.c | 3 ++-
drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c | 3 ++-
2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_txrx.c b/drivers/net/wireless/mediatek/mt76/mt76x02_txrx.c
index 95c73049a68b..82acebb7f1a9 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_txrx.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_txrx.c
@@ -154,6 +154,7 @@ int mt76x02_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
struct mt76x02_dev *dev = container_of(mdev, struct mt76x02_dev, mt76);
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx_info->skb->data;
struct mt76x02_txwi *txwi = txwi_ptr;
+ bool ampdu = IEEE80211_SKB_CB(tx_info->skb)->flags & IEEE80211_TX_CTL_AMPDU;
int hdrlen, len, pid, qsel = MT_QSEL_EDCA;
if (qid == MT_TXQ_PSD && wcid && wcid->idx < 128)
@@ -171,7 +172,7 @@ int mt76x02_tx_prepare_skb(struct mt76_dev *mdev, void *txwi_ptr,
txwi->pktid = pid;
- if (mt76_is_skb_pktid(pid))
+ if (mt76_is_skb_pktid(pid) && ampdu)
qsel = MT_QSEL_MGMT;
tx_info->info = FIELD_PREP(MT_TXD_INFO_QSEL, qsel) |
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
index 2436f14ca24a..f3696afc1dde 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_usb_core.c
@@ -79,6 +79,7 @@ int mt76x02u_tx_prepare_skb(struct mt76_dev *mdev, void *data,
struct mt76x02_dev *dev = container_of(mdev, struct mt76x02_dev, mt76);
int pid, len = tx_info->skb->len, ep = q2ep(mdev->q_tx[qid].q->hw_idx);
struct mt76x02_txwi *txwi;
+ bool ampdu = IEEE80211_SKB_CB(tx_info->skb)->flags & IEEE80211_TX_CTL_AMPDU;
enum mt76_qsel qsel;
u32 flags;
@@ -96,7 +97,7 @@ int mt76x02u_tx_prepare_skb(struct mt76_dev *mdev, void *data,
txwi->pktid = pid;
- if (mt76_is_skb_pktid(pid) || ep == MT_EP_OUT_HCCA)
+ if ((mt76_is_skb_pktid(pid) && ampdu) || ep == MT_EP_OUT_HCCA)
qsel = MT_QSEL_MGMT;
else
qsel = MT_QSEL_EDCA;
--
2.17.0
^ permalink raw reply related
* Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook
From: Christoph Hellwig @ 2019-06-08 7:23 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Larry Finger, Aaro Koskinen, Christoph Hellwig,
Christian Zigotzky, Michael Ellerman, linuxppc-dev,
linux-wireless, linux-kernel
In-Reply-To: <7697a9d10777b28ae79fdffdde6d0985555f6310.camel@kernel.crashing.org>
On Sat, Jun 08, 2019 at 02:21:23PM +1000, Benjamin Herrenschmidt wrote:
>
> > Please try the attached patch. I'm not really pleased with it and I will
> > continue to determine why the fallback to a 30-bit mask fails, but at least this
> > one works for me.
>
> Your patch only makes sense if the device is indeed capable of
> addressing 31-bits.
>
> So either the driver is buggy and asks for a too small mask in which
> case your patch is ok, or it's not and you're just going to cause all
> sort of interesting random problems including possible memory
> corruption.
Well, my patches changes ZONE_DMA to be 30-bits instead of 31, and
thus should allow requesting a 30-bit mask to succeed.
^ permalink raw reply
* Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook
From: Benjamin Herrenschmidt @ 2019-06-08 4:21 UTC (permalink / raw)
To: Larry Finger, Aaro Koskinen, Christoph Hellwig,
Christian Zigotzky, Michael Ellerman
Cc: linuxppc-dev, linux-wireless, linux-kernel
In-Reply-To: <73da300c-871c-77ac-8a3a-deac226743ef@lwfinger.net>
> Please try the attached patch. I'm not really pleased with it and I will
> continue to determine why the fallback to a 30-bit mask fails, but at least this
> one works for me.
Your patch only makes sense if the device is indeed capable of
addressing 31-bits.
So either the driver is buggy and asks for a too small mask in which
case your patch is ok, or it's not and you're just going to cause all
sort of interesting random problems including possible memory
corruption.
Cheers,
Ben.
^ permalink raw reply
* [RESEND] brcmfmac support for BCM4359 sdio on arm64 ??
From: Christian Hewitt @ 2019-06-08 3:39 UTC (permalink / raw)
To: arend.vanspriel
Cc: linux-wireless, brcm80211-dev-list.pdl, brcm80211-dev-list,
Wright.Feng, Neil Armstrong, Christoph Muellner
Hello Arend,
Last October Christoph Müllner reported BCM4359 SDIO issues here: https://www.spinics.net/lists/linux-wireless/msg178783.html but the investigation stalled after the needs/timescale of his project forced a change to a different (working) module.
BCM4359 is being used in an increasing number of Amlogic devices the Kodi focussed distro LibreELEC supports. I’m one of the maintainers for the distro and I’d like to assist/resume the investigation.
To recap: using changes from Wright Feng that can be found here https://github.com/RobertCNelson/ti-linux-kernel-dev/blob/65f17112e1c883d3c9f3fa68837e5f9b5eb7cfad/patches/cypress/v4.14.52-2018_0928/cypress-patch/0050-brcmfmac-Add-support-for-BCM4359-SDIO-chipset.patch result in the BCM4359 device being identified but firmware/nvram loading fails:
[ 8.557929] brcmfmac: F1 signature read @0x18000000=0x17294359
[ 8.562087] brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4359-sdio for chip BCM4359/9
[ 8.775655] brcmfmac: brcmf_sdiod_ramrw: membytes transfer failed
[ 8.775667] brcmfmac: brcmf_sdio_verifymemory: error -84 on reading 2048 membytes at 0x0025f0c0
[ 8.775670] brcmfmac: brcmf_sdio_download_firmware: dongle nvram file download failed
See: http://ix.io/1KfY for the full dmesg output on 5.1-rc1 kernel including a splat that may or may not be related/relevant. I am using firmware and nvram files from https://github.com/LibreELEC/brcmfmac_sdio-firmware which match files found in several other github and public repo locations. The firmware/nvram are reported working in Android.
BCMDHD is also reported working with commits here: https://gitlab.com/baylibre/amlogic/atv/linux/commits/narmstrong/v5.1/aml/integ-5.1-bcmdhd but LibreELEC needs to support many different boards (with many different SDIO modules) from a single OS image, so BCMDHD is not the solution we need.
One additional patch I spotted mentioning BCM4359 (also from Wright Feng) was https://github.com/RobertCNelson/ti-linux-kernel-dev/blob/65f17112e1c883d3c9f3fa68837e5f9b5eb7cfad/patches/cypress/v4.14.52-2018_0928/cypress-patch/0073-non-upstream-reset-two-D11-cores-if-chip-has-two-D11.patch but it makes no difference (the dmesg log above is with this patch applied).
I don’t write code but am happy to build test kernels with suggested patches or explicit instructions. I’ve also CC’d LibreELEC colleague and linux-amlogic maintainer Neil Armstrong who can assist. NB: If direct access to hardware would help progress things I can easily organise remote access or get board samples shipped.
How can we resume the investigation?
Christian
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Denis Kenzior @ 2019-06-07 22:47 UTC (permalink / raw)
To: Eric Biggers
Cc: Ard Biesheuvel, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <20190607224040.GG648@sol.localdomain>
Hi Eric,
On 06/07/2019 05:40 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 04:54:04PM -0500, Denis Kenzior wrote:
>> Hi Eric,
>>
>> On 06/07/2019 04:41 PM, Eric Biggers wrote:
>>> On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
>>>> Hi Eric,
>>>>
>>>> On 06/07/2019 04:15 PM, Eric Biggers wrote:
>>>>> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>>>>>> Hi Ard,
>>>>>>
>>>>>>>
>>>>>>> Ah ok, good to know. That does imply that the driver is not entirely
>>>>>>> broken, which is good news I suppose.
>>>>>>>
>>>>>>
>>>>>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>>>>>> parallel encrypt/decrypt operations on the socket would result in invalid
>>>>>> behavior. Probably due to the issue Eric already pointed out.
>>>>>>
>>>>>> No such issue with any other ciphers that we use.
>>>>>>
>>>>>> Regards,
>>>>>> -Denis
>>>>>
>>>>> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
>>>>> we can't fix its name to be just "arc4". It's odd that someone would choose to
>>>>> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
>>>>>
>>>>> Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
>>>>> modern stream ciphers use a key + IV. To comply with the crypto API it would
>>>>> have to copy the key to a stack buffer for each encryption/decryption. But it
>>>>> doesn't; it just updates the key instead, making it non thread safe. If users
>>>>> are actually relying on that, we'll have to settle for adding a mutex instead.
>>>>
>>>> Well the issue isn't even about being thread safe. We run a single thread
>>>> in iwd. The details are a bit fuzzy now due to time elapsed, but if I
>>>> recall correctly, even behavior like:
>>>>
>>>> fd = socket();
>>>> bind(fd, ecb(arc4));
>>>> setsockopt(fd, ...key...);
>>>>
>>>> sendmsg(fd, OP_ENCRYPT, ...);
>>>> sendmsg(fd, OP_DECRYPT, ...);
>>>> sendmsg(fd, OP_ENCRYPT, ...);
>>>>
>>>> would produce different (incorrect) encrypted results compared to
>>>>
>>>> sendmsg(fd, OP_ENCRYPT, ...)
>>>> sendmsg(fd, OP_ENCRYPT, ...)
>>>>
>>>
>>> That's because currently each operation uses the next bytes from the keystream,
>>> and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
>>> There's no difference between ARC4 encryption and decryption; both just XOR the
>>> keystream with the data. Are you saying you expected each encryption to be a
>>> continuation of the previous encryption, but decryptions to be independent?
>>>
>>
>> From a userspace / api perspective, yes I would have expected the encrypt
>> and decrypt to work independently. No biggie now, but I remember being
>> surprised when this bit me as no other cipher had this behavior. E.g.
>> interleaving of operations seemed to only affect arc4 results.
>>
>> Are the exact semantics spelled out somewhere?
>>
>
> For all other skcipher algorithms, every operation is independent and depends
> only on the key which was set previously on the algorithm socket, plus the IV
> provided for the operation. There is no way to perform a single encryption or
> decryption incrementally in multiple parts, unless the algorithm supports it
> naturally by updating the IV (e.g. CBC mode).
Right, that is what I thought.
>
> As I am attempting to explain, ecb(arc4) does not implement this API correctly
> because it updates the *key* after each operation, not the IV. I doubt this is
> documented anywhere, but this can only be changed if people aren't relying on it
> already.
It sounds to me like it was broken and should be fixed. So our vote /
preference is to have ARC4 fixed to follow the proper semantics. We can
deal with the kernel behavioral change on our end easily enough; the
required workarounds are the worse evil.
Regards,
-Denis
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Eric Biggers @ 2019-06-07 22:40 UTC (permalink / raw)
To: Denis Kenzior
Cc: Ard Biesheuvel, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <78298612-a36b-deaa-1510-94cf0001af9d@gmail.com>
On Fri, Jun 07, 2019 at 04:54:04PM -0500, Denis Kenzior wrote:
> Hi Eric,
>
> On 06/07/2019 04:41 PM, Eric Biggers wrote:
> > On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
> > > Hi Eric,
> > >
> > > On 06/07/2019 04:15 PM, Eric Biggers wrote:
> > > > On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> > > > > Hi Ard,
> > > > >
> > > > > >
> > > > > > Ah ok, good to know. That does imply that the driver is not entirely
> > > > > > broken, which is good news I suppose.
> > > > > >
> > > > >
> > > > > Not entirely, but we did have to resort to using multiple sockets, otherwise
> > > > > parallel encrypt/decrypt operations on the socket would result in invalid
> > > > > behavior. Probably due to the issue Eric already pointed out.
> > > > >
> > > > > No such issue with any other ciphers that we use.
> > > > >
> > > > > Regards,
> > > > > -Denis
> > > >
> > > > Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
> > > > we can't fix its name to be just "arc4". It's odd that someone would choose to
> > > > use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
> > > >
> > > > Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
> > > > modern stream ciphers use a key + IV. To comply with the crypto API it would
> > > > have to copy the key to a stack buffer for each encryption/decryption. But it
> > > > doesn't; it just updates the key instead, making it non thread safe. If users
> > > > are actually relying on that, we'll have to settle for adding a mutex instead.
> > >
> > > Well the issue isn't even about being thread safe. We run a single thread
> > > in iwd. The details are a bit fuzzy now due to time elapsed, but if I
> > > recall correctly, even behavior like:
> > >
> > > fd = socket();
> > > bind(fd, ecb(arc4));
> > > setsockopt(fd, ...key...);
> > >
> > > sendmsg(fd, OP_ENCRYPT, ...);
> > > sendmsg(fd, OP_DECRYPT, ...);
> > > sendmsg(fd, OP_ENCRYPT, ...);
> > >
> > > would produce different (incorrect) encrypted results compared to
> > >
> > > sendmsg(fd, OP_ENCRYPT, ...)
> > > sendmsg(fd, OP_ENCRYPT, ...)
> > >
> >
> > That's because currently each operation uses the next bytes from the keystream,
> > and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
> > There's no difference between ARC4 encryption and decryption; both just XOR the
> > keystream with the data. Are you saying you expected each encryption to be a
> > continuation of the previous encryption, but decryptions to be independent?
> >
>
> From a userspace / api perspective, yes I would have expected the encrypt
> and decrypt to work independently. No biggie now, but I remember being
> surprised when this bit me as no other cipher had this behavior. E.g.
> interleaving of operations seemed to only affect arc4 results.
>
> Are the exact semantics spelled out somewhere?
>
For all other skcipher algorithms, every operation is independent and depends
only on the key which was set previously on the algorithm socket, plus the IV
provided for the operation. There is no way to perform a single encryption or
decryption incrementally in multiple parts, unless the algorithm supports it
naturally by updating the IV (e.g. CBC mode).
As I am attempting to explain, ecb(arc4) does not implement this API correctly
because it updates the *key* after each operation, not the IV. I doubt this is
documented anywhere, but this can only be changed if people aren't relying on it
already.
- Eric
^ permalink raw reply
* [PATCH v3 0/5] brcmfmac: sdio: Deal better w/ transmission errors related to idle
From: Douglas Anderson @ 2019-06-07 22:37 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, Linus Walleij, Shawn Lin, YueHaibing,
Rafał Miłecki, Hante Meuleman, Martin Blumenstingl,
Ritesh Harjani, Michael Trimarchi, Mathieu Malaterre,
Wolfram Sang, Franky Lin, Ondrej Jirman, Jiong Wu,
David S. Miller, Pan Bian, linux-kernel, Madhan Mohan R,
Tony Lindgren, Avri Altman, Pavel Machek
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 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 for temporarily disabling auto-retuning due to errors
brcmfmac: sdio: Disable auto-tuning around commands expected to fail
mmc: core: Export mmc_retune_hold_now() mmc_retune_release()
brcmfmac: sdio: Don't tune while the card is off
drivers/mmc/core/core.c | 19 +++++++++++++++++--
drivers/mmc/core/host.c | 7 +++++++
drivers/mmc/core/host.h | 7 -------
.../broadcom/brcm80211/brcmfmac/sdio.c | 18 +++++++++++++-----
include/linux/mmc/core.h | 4 ++++
include/linux/mmc/host.h | 1 +
6 files changed, 42 insertions(+), 14 deletions(-)
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply
* [PATCH v3 2/5] mmc: core: API for temporarily disabling auto-retuning due to errors
From: Douglas Anderson @ 2019-06-07 22:37 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, linux-mmc, linux-kernel, Shawn Lin,
Wolfram Sang, Avri Altman
In-Reply-To: <20190607223716.119277-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 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 card 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>
---
Note that are are a whole boatload of different ways that we could
provide an API for the Broadcom WiFi SDIO driver. This patch
illustrates one way but if maintainers feel strongly that this is too
ugly and have a better idea then I can give it a shot too. From a
purist point of view I kinda felt that the "expect errors" really
belonged as part of the mmc_request structure, but getting it into
there meant changing a whole pile of core SD/MMC APIs. Simply adding
it to the host seemed to match the current style better and was a less
intrusive change.
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 | 19 +++++++++++++++++--
include/linux/mmc/core.h | 2 ++
include/linux/mmc/host.h | 1 +
3 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 6db36dc870b5..bc109ec49406 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->expect_errors &&
(err == -EILSEQ || (mrq->sbc && mrq->sbc->error == -EILSEQ) ||
(mrq->data && mrq->data->error == -EILSEQ) ||
(mrq->stop && mrq->stop->error == -EILSEQ)))
@@ -2163,6 +2164,20 @@ int mmc_sw_reset(struct mmc_host *host)
}
EXPORT_SYMBOL(mmc_sw_reset);
+void mmc_expect_errors_begin(struct mmc_host *host)
+{
+ WARN_ON(host->expect_errors);
+ host->expect_errors = true;
+}
+EXPORT_SYMBOL_GPL(mmc_expect_errors_begin);
+
+void mmc_expect_errors_end(struct mmc_host *host)
+{
+ WARN_ON(!host->expect_errors);
+ host->expect_errors = false;
+}
+EXPORT_SYMBOL_GPL(mmc_expect_errors_end);
+
static int mmc_rescan_try_freq(struct mmc_host *host, unsigned freq)
{
host->f_init = freq;
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..8d553fb8c834 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 expect_errors:1; /* don't trigger retune upon errors */
int rescan_disable; /* disable card detection */
int rescan_entered; /* used with nonremovable devices */
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v3 1/5] Revert "brcmfmac: disable command decode in sdio_aos"
From: Douglas Anderson @ 2019-06-07 22:37 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,
David S. Miller, Franky Lin, linux-kernel,
Rafał Miłecki, Hante Meuleman, YueHaibing,
Michael Trimarchi
In-Reply-To: <20190607223716.119277-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 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 v3 5/5] brcmfmac: sdio: Don't tune while the card is off
From: Douglas Anderson @ 2019-06-07 22:37 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: <20190607223716.119277-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 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 4040aae1f9ed..98ffb4e90e15 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -670,6 +670,10 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
mmc_expect_errors_begin(bus->sdiodev->func1->card->host);
+ /* Cannot re-tune if device is asleep; defer till we're awake */
+ if (on)
+ mmc_retune_hold_now(bus->sdiodev->func1->card->host);
+
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);
@@ -730,6 +734,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)
+ mmc_retune_release(bus->sdiodev->func1->card->host);
+
mmc_expect_errors_end(bus->sdiodev->func1->card->host);
return err;
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v3 3/5] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Douglas Anderson @ 2019-06-07 22:37 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: <20190607223716.119277-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 v3:
- Expect errors for all of brcmf_sdio_kso_control() (Adrian).
Changes in v2: None
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index 4a750838d8cd..4040aae1f9ed 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -16,6 +16,7 @@
#include <linux/mmc/sdio_ids.h>
#include <linux/mmc/sdio_func.h>
#include <linux/mmc/card.h>
+#include <linux/mmc/core.h>
#include <linux/semaphore.h>
#include <linux/firmware.h>
#include <linux/module.h>
@@ -667,6 +668,8 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
brcmf_dbg(TRACE, "Enter: on=%d\n", on);
+ mmc_expect_errors_begin(bus->sdiodev->func1->card->host);
+
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 +730,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);
+ mmc_expect_errors_end(bus->sdiodev->func1->card->host);
+
return err;
}
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* [PATCH v3 4/5] mmc: core: Export mmc_retune_hold_now() mmc_retune_release()
From: Douglas Anderson @ 2019-06-07 22:37 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,
Martin Blumenstingl, Pan Bian, Linus Walleij, linux-mmc,
linux-kernel, Tony Lindgren, Mathieu Malaterre, Pavel Machek
In-Reply-To: <20190607223716.119277-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.
NOTE: We'll leave the calls with a mmc_ prefix following the lead of
the API call mmc_hw_reset(), which is also expected to be called
directly by SDIO cards.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Changes in v3:
- ("mmc: core: Export mmc_retune_hold_now() mmc_retune_release()") new for v3.
Changes in v2: None
drivers/mmc/core/host.c | 7 +++++++
drivers/mmc/core/host.h | 7 -------
include/linux/mmc/core.h | 2 ++
3 files changed, 9 insertions(+), 7 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 6a51f7a06ce7..361f4d151d20 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -111,6 +111,13 @@ void mmc_retune_hold(struct mmc_host *host)
host->hold_retune += 1;
}
+void mmc_retune_hold_now(struct mmc_host *host)
+{
+ host->retune_now = 0;
+ host->hold_retune += 1;
+}
+EXPORT_SYMBOL(mmc_retune_hold_now);
+
void mmc_retune_release(struct mmc_host *host)
{
if (host->hold_retune)
diff --git a/drivers/mmc/core/host.h b/drivers/mmc/core/host.h
index 4805438c02ff..3212afc6c9fe 100644
--- a/drivers/mmc/core/host.h
+++ b/drivers/mmc/core/host.h
@@ -19,17 +19,10 @@ void mmc_unregister_host_class(void);
void mmc_retune_enable(struct mmc_host *host);
void mmc_retune_disable(struct mmc_host *host);
void mmc_retune_hold(struct mmc_host *host);
-void mmc_retune_release(struct mmc_host *host);
int mmc_retune(struct mmc_host *host);
void mmc_retune_pause(struct mmc_host *host);
void mmc_retune_unpause(struct mmc_host *host);
-static inline void mmc_retune_hold_now(struct mmc_host *host)
-{
- host->retune_now = 0;
- host->hold_retune += 1;
-}
-
static inline void mmc_retune_recheck(struct mmc_host *host)
{
if (host->hold_retune <= 1)
diff --git a/include/linux/mmc/core.h b/include/linux/mmc/core.h
index 02a13abf0cda..53085245383c 100644
--- a/include/linux/mmc/core.h
+++ b/include/linux/mmc/core.h
@@ -181,5 +181,7 @@ 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);
+void mmc_retune_release(struct mmc_host *host);
+void mmc_retune_hold_now(struct mmc_host *host);
#endif /* LINUX_MMC_CORE_H */
--
2.22.0.rc2.383.gf4fbbf30c2-goog
^ permalink raw reply related
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Denis Kenzior @ 2019-06-07 21:54 UTC (permalink / raw)
To: Eric Biggers
Cc: Ard Biesheuvel, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <20190607214120.GE648@sol.localdomain>
Hi Eric,
On 06/07/2019 04:41 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
>> Hi Eric,
>>
>> On 06/07/2019 04:15 PM, Eric Biggers wrote:
>>> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>>>> Hi Ard,
>>>>
>>>>>
>>>>> Ah ok, good to know. That does imply that the driver is not entirely
>>>>> broken, which is good news I suppose.
>>>>>
>>>>
>>>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>>>> parallel encrypt/decrypt operations on the socket would result in invalid
>>>> behavior. Probably due to the issue Eric already pointed out.
>>>>
>>>> No such issue with any other ciphers that we use.
>>>>
>>>> Regards,
>>>> -Denis
>>>
>>> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
>>> we can't fix its name to be just "arc4". It's odd that someone would choose to
>>> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
>>>
>>> Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
>>> modern stream ciphers use a key + IV. To comply with the crypto API it would
>>> have to copy the key to a stack buffer for each encryption/decryption. But it
>>> doesn't; it just updates the key instead, making it non thread safe. If users
>>> are actually relying on that, we'll have to settle for adding a mutex instead.
>>
>> Well the issue isn't even about being thread safe. We run a single thread
>> in iwd. The details are a bit fuzzy now due to time elapsed, but if I
>> recall correctly, even behavior like:
>>
>> fd = socket();
>> bind(fd, ecb(arc4));
>> setsockopt(fd, ...key...);
>>
>> sendmsg(fd, OP_ENCRYPT, ...);
>> sendmsg(fd, OP_DECRYPT, ...);
>> sendmsg(fd, OP_ENCRYPT, ...);
>>
>> would produce different (incorrect) encrypted results compared to
>>
>> sendmsg(fd, OP_ENCRYPT, ...)
>> sendmsg(fd, OP_ENCRYPT, ...)
>>
>
> That's because currently each operation uses the next bytes from the keystream,
> and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
> There's no difference between ARC4 encryption and decryption; both just XOR the
> keystream with the data. Are you saying you expected each encryption to be a
> continuation of the previous encryption, but decryptions to be independent?
>
From a userspace / api perspective, yes I would have expected the
encrypt and decrypt to work independently. No biggie now, but I
remember being surprised when this bit me as no other cipher had this
behavior. E.g. interleaving of operations seemed to only affect arc4
results.
Are the exact semantics spelled out somewhere?
Regards,
-Denis
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Eric Biggers @ 2019-06-07 21:41 UTC (permalink / raw)
To: Denis Kenzior
Cc: Ard Biesheuvel, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <d394b421-799d-2019-fcf0-97ba0b2abb5f@gmail.com>
On Fri, Jun 07, 2019 at 04:28:59PM -0500, Denis Kenzior wrote:
> Hi Eric,
>
> On 06/07/2019 04:15 PM, Eric Biggers wrote:
> > On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> > > Hi Ard,
> > >
> > > >
> > > > Ah ok, good to know. That does imply that the driver is not entirely
> > > > broken, which is good news I suppose.
> > > >
> > >
> > > Not entirely, but we did have to resort to using multiple sockets, otherwise
> > > parallel encrypt/decrypt operations on the socket would result in invalid
> > > behavior. Probably due to the issue Eric already pointed out.
> > >
> > > No such issue with any other ciphers that we use.
> > >
> > > Regards,
> > > -Denis
> >
> > Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
> > we can't fix its name to be just "arc4". It's odd that someone would choose to
> > use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
> >
> > Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
> > modern stream ciphers use a key + IV. To comply with the crypto API it would
> > have to copy the key to a stack buffer for each encryption/decryption. But it
> > doesn't; it just updates the key instead, making it non thread safe. If users
> > are actually relying on that, we'll have to settle for adding a mutex instead.
>
> Well the issue isn't even about being thread safe. We run a single thread
> in iwd. The details are a bit fuzzy now due to time elapsed, but if I
> recall correctly, even behavior like:
>
> fd = socket();
> bind(fd, ecb(arc4));
> setsockopt(fd, ...key...);
>
> sendmsg(fd, OP_ENCRYPT, ...);
> sendmsg(fd, OP_DECRYPT, ...);
> sendmsg(fd, OP_ENCRYPT, ...);
>
> would produce different (incorrect) encrypted results compared to
>
> sendmsg(fd, OP_ENCRYPT, ...)
> sendmsg(fd, OP_ENCRYPT, ...)
>
That's because currently each operation uses the next bytes from the keystream,
and a new keystream is started only by setsockopt(..., ALG_SET_KEY, ...).
There's no difference between ARC4 encryption and decryption; both just XOR the
keystream with the data. Are you saying you expected each encryption to be a
continuation of the previous encryption, but decryptions to be independent?
- Eric
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Denis Kenzior @ 2019-06-07 21:28 UTC (permalink / raw)
To: Eric Biggers
Cc: Ard Biesheuvel, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <20190607211514.GD648@sol.localdomain>
Hi Eric,
On 06/07/2019 04:15 PM, Eric Biggers wrote:
> On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
>> Hi Ard,
>>
>>>
>>> Ah ok, good to know. That does imply that the driver is not entirely
>>> broken, which is good news I suppose.
>>>
>>
>> Not entirely, but we did have to resort to using multiple sockets, otherwise
>> parallel encrypt/decrypt operations on the socket would result in invalid
>> behavior. Probably due to the issue Eric already pointed out.
>>
>> No such issue with any other ciphers that we use.
>>
>> Regards,
>> -Denis
>
> Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
> we can't fix its name to be just "arc4". It's odd that someone would choose to
> use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
>
> Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
> modern stream ciphers use a key + IV. To comply with the crypto API it would
> have to copy the key to a stack buffer for each encryption/decryption. But it
> doesn't; it just updates the key instead, making it non thread safe. If users
> are actually relying on that, we'll have to settle for adding a mutex instead.
Well the issue isn't even about being thread safe. We run a single
thread in iwd. The details are a bit fuzzy now due to time elapsed, but
if I recall correctly, even behavior like:
fd = socket();
bind(fd, ecb(arc4));
setsockopt(fd, ...key...);
sendmsg(fd, OP_ENCRYPT, ...);
sendmsg(fd, OP_DECRYPT, ...);
sendmsg(fd, OP_ENCRYPT, ...);
would produce different (incorrect) encrypted results compared to
sendmsg(fd, OP_ENCRYPT, ...)
sendmsg(fd, OP_ENCRYPT, ...)
Regards,
-Denis
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Eric Biggers @ 2019-06-07 21:15 UTC (permalink / raw)
To: Denis Kenzior
Cc: Ard Biesheuvel, Marcel Holtmann,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <f40ad169-93b9-636f-9656-634ff331ee2b@gmail.com>
On Fri, Jun 07, 2019 at 03:45:45PM -0500, Denis Kenzior wrote:
> Hi Ard,
>
> >
> > Ah ok, good to know. That does imply that the driver is not entirely
> > broken, which is good news I suppose.
> >
>
> Not entirely, but we did have to resort to using multiple sockets, otherwise
> parallel encrypt/decrypt operations on the socket would result in invalid
> behavior. Probably due to the issue Eric already pointed out.
>
> No such issue with any other ciphers that we use.
>
> Regards,
> -Denis
Okay, that sucks, so we do have to keep "ecb(arc4)" in the crypto API then. And
we can't fix its name to be just "arc4". It's odd that someone would choose to
use AF_ALG over writing a 20 line arc4_crypt() in userspace, but whatever.
Yes, "ecb(arc4)" isn't currently thread safe. ARC4 uses a single key whereas
modern stream ciphers use a key + IV. To comply with the crypto API it would
have to copy the key to a stack buffer for each encryption/decryption. But it
doesn't; it just updates the key instead, making it non thread safe. If users
are actually relying on that, we'll have to settle for adding a mutex instead.
In any case, we can still remove the 'cipher' algorithm version as Ard is
suggesting, as well as possibly convert the in-kernel users to use an
arc4_crypt() library function and remove the hardware driver support.
- Eric
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Denis Kenzior @ 2019-06-07 20:45 UTC (permalink / raw)
To: Ard Biesheuvel, Marcel Holtmann
Cc: Eric Biggers, open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
Herbert Xu, Johannes Berg, open list:NFC SUBSYSTEM,
David S. Miller
In-Reply-To: <CAKv+Gu-ek4nK+cACx5QZTbp=ciQq_Fvtn9y3g-wFWSOabyczZg@mail.gmail.com>
Hi Ard,
>
> Ah ok, good to know. That does imply that the driver is not entirely
> broken, which is good news I suppose.
>
Not entirely, but we did have to resort to using multiple sockets,
otherwise parallel encrypt/decrypt operations on the socket would result
in invalid behavior. Probably due to the issue Eric already pointed out.
No such issue with any other ciphers that we use.
Regards,
-Denis
^ permalink raw reply
* Re: iwl_mvm_add_new_dqa_stream_wk BUG in lib/list_debug.c:56
From: Marc Haber @ 2019-06-07 20:44 UTC (permalink / raw)
To: Yussuf Khalil
Cc: linux-wireless, linux-kernel, netdev, Johannes Berg,
Emmanuel Grumbach, Luca Coelho, Intel Linux Wireless
In-Reply-To: <29401822-d7e9-430b-d284-706bf68acb8a@pp3345.net>
On Fri, Jun 07, 2019 at 10:20:56PM +0200, Yussuf Khalil wrote:
> CC'ing iwlwifi maintainers to get some attention for this issue.
>
> I am experiencing the very same bug on a ThinkPad T480s running 5.1.6 with
> Fedora 30. A friend is seeing it on his X1 Carbon 6th Gen, too. Both have an
> "Intel Corporation Wireless 8265 / 8275" card according to lspci.
I have an older 04:00.0 Network controller [0280]: Intel Corporation
Wireless 8260 [8086:24f3] (rev 3a) on a Thinkpad X260.
> Notably, in all cases I've observed it occurred right after roaming from one
> AP to another (though I can't guarantee this isn't a coincidence).
I also have multiple Access Points broadcasting the same SSID in my
house, and yes, I experience those issues often when I move from one
part of the hose to another. I have, however, also experienced it in a
hotel when I was using the mobile hotspot offered by my mobile, so that
was clearly not a roaming situation.
Greetings
Marc
--
-----------------------------------------------------------------------------
Marc Haber | "I don't trust Computers. They | Mailadresse im Header
Leimen, Germany | lose things." Winona Ryder | Fon: *49 6224 1600402
Nordisch by Nature | How to make an American Quilt | Fax: *49 6224 1600421
^ 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