* [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
* Re: iwl_mvm_add_new_dqa_stream_wk BUG in lib/list_debug.c:56
From: Yussuf Khalil @ 2019-06-07 20:20 UTC (permalink / raw)
To: Marc Haber, linux-wireless
Cc: linux-kernel, netdev, Johannes Berg, Emmanuel Grumbach,
Luca Coelho, Intel Linux Wireless
In-Reply-To: <20190602134842.GC3249@torres.zugschlus.de>
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.
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).
Here is the dmesg output from my machine:
> Jun 02 00:38:25 pp3345-laptop kernel: wlp61s0: disconnect from AP 9c:1c:12:82:5a:b0 for new auth to ac:a3:1e:d9:5e:d0
> Jun 02 00:38:25 pp3345-laptop kernel: wlp61s0: authenticate with ac:a3:1e:d9:5e:d0
> Jun 02 00:38:25 pp3345-laptop kernel: wlp61s0: send auth to ac:a3:1e:d9:5e:d0 (try 1/3)
> Jun 02 00:38:25 pp3345-laptop kernel: wlp61s0: send auth to ac:a3:1e:d9:5e:d0 (try 2/3)
> Jun 02 00:38:25 pp3345-laptop kernel: wlp61s0: send auth to ac:a3:1e:d9:5e:d0 (try 3/3)
> Jun 02 00:38:25 pp3345-laptop kernel: wlp61s0: authentication with ac:a3:1e:d9:5e:d0 timed out
> Jun 02 00:38:26 pp3345-laptop kernel: wlp61s0: authenticate with 9c:1c:12:82:5a:b0
> Jun 02 00:38:26 pp3345-laptop kernel: wlp61s0: send auth to 9c:1c:12:82:5a:b0 (try 1/3)
> Jun 02 00:38:26 pp3345-laptop kernel: wlp61s0: authenticated
> Jun 02 00:38:26 pp3345-laptop kernel: wlp61s0: associate with 9c:1c:12:82:5a:b0 (try 1/3)
> Jun 02 00:38:26 pp3345-laptop kernel: wlp61s0: RX AssocResp from 9c:1c:12:82:5a:b0 (capab=0x1 status=0 aid=4)
> Jun 02 00:38:26 pp3345-laptop kernel: wlp61s0: associated
> Jun 02 00:38:26 pp3345-laptop kernel: list_del corruption. next->prev should be ffff9a9f64e527f8, but was ffff9a9f64e52108
> Jun 02 00:38:26 pp3345-laptop kernel: ------------[ cut here ]------------
> Jun 02 00:38:26 pp3345-laptop kernel: kernel BUG at lib/list_debug.c:54!
> Jun 02 00:38:26 pp3345-laptop kernel: invalid opcode: 0000 [#1] SMP PTI
> Jun 02 00:38:26 pp3345-laptop kernel: CPU: 5 PID: 2128 Comm: kworker/5:3 Not tainted 5.1.6-300.fc30.x86_64 #1
> Jun 02 00:38:26 pp3345-laptop kernel: Hardware name: LENOVO 20L8S02E00/20L8S02E00, BIOS N22ET54W (1.31 ) 04/22/2019
> Jun 02 00:38:26 pp3345-laptop kernel: Workqueue: events iwl_mvm_add_new_dqa_stream_wk [iwlmvm]
> Jun 02 00:38:26 pp3345-laptop kernel: RIP: 0010:__list_del_entry_valid.cold+0x1d/0x55
> Jun 02 00:38:26 pp3345-laptop kernel: Code: c7 c7 18 55 11 b8 e8 f5 0b c7 ff 0f 0b 48 89 fe 48 c7 c7 a8 55 11 b8 e8 e4 0b c7 ff 0f 0b 48 c7 c7 58 56 11 b8 e8 d6 0b c7 ff <0f> 0b 48 89 f2 48 89 fe 48 c7 c7 18 56 11 b8 e8 c2 0b c7 ff 0f 0b
> Jun 02 00:38:26 pp3345-laptop kernel: RSP: 0000:ffffa8ec4ad73dc8 EFLAGS: 00010246
> Jun 02 00:38:26 pp3345-laptop kernel: RAX: 0000000000000054 RBX: ffff9a9f64e527f8 RCX: 0000000000000000
> Jun 02 00:38:26 pp3345-laptop kernel: RDX: 0000000000000000 RSI: ffff9a9fcf3568c8 RDI: ffff9a9fcf3568c8
> Jun 02 00:38:26 pp3345-laptop kernel: RBP: ffff9a9f64e56898 R08: ffff9a9fcf3568c8 R09: 0000000000000c6f
> Jun 02 00:38:26 pp3345-laptop kernel: R10: ffffffffb89f930c R11: 0000000000000003 R12: 0000000000000004
> Jun 02 00:38:26 pp3345-laptop kernel: R13: ffff9a9fc879a388 R14: 0000000000000000 R15: ffff9a9fc8799608
> Jun 02 00:38:26 pp3345-laptop kernel: FS: 0000000000000000(0000) GS:ffff9a9fcf340000(0000) knlGS:0000000000000000
> Jun 02 00:38:26 pp3345-laptop kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Jun 02 00:38:26 pp3345-laptop kernel: CR2: 00001d3a916bf000 CR3: 0000000487c94003 CR4: 00000000003606e0
> Jun 02 00:38:26 pp3345-laptop kernel: Call Trace:
> Jun 02 00:38:26 pp3345-laptop kernel: iwl_mvm_add_new_dqa_stream_wk+0x2f3/0x870 [iwlmvm]
> Jun 02 00:38:26 pp3345-laptop kernel: ? __switch_to+0x40/0x4c0
> Jun 02 00:38:26 pp3345-laptop kernel: process_one_work+0x19d/0x380
> Jun 02 00:38:26 pp3345-laptop kernel: worker_thread+0x50/0x3b0
> Jun 02 00:38:26 pp3345-laptop kernel: kthread+0xfb/0x130
> Jun 02 00:38:26 pp3345-laptop kernel: ? process_one_work+0x380/0x380
> Jun 02 00:38:26 pp3345-laptop kernel: ? kthread_park+0x90/0x90
> Jun 02 00:38:26 pp3345-laptop kernel: ret_from_fork+0x35/0x40
> Jun 02 00:38:26 pp3345-laptop kernel: Modules linked in: xt_CHECKSUM ipt_MASQUERADE tun bridge stp llc nf_conntrack_netbios_ns nf_conntrack_broadcast xt_CT ip6t_rpfilter ip6t_REJECT nf_reject_ipv6 ipt_REJECT nf_reject_ipv4 xt_conntrack ebtable_nat ip6table_nat ip6table_mangle ip6table_raw ip6table_security iptable_nat nf_nat iptable_mangle iptable_raw iptable_security nf_conntrack nf_defrag_ipv6 nf_defrag_ipv4 libcrc32c ip_set nfnetlink ebtable_filter ebtables ip6table_filter ip6_tables iptable_filter ip_tables bnep sunrpc vfat fat arc4 elan_i2c iwlmvm mac80211 mei_hdcp iTCO_wdt iTCO_vendor_support snd_soc_skl snd_soc_hdac_hda snd_hda_ext_core snd_soc_skl_ipc snd_soc_sst_ipc intel_rapl snd_soc_sst_dsp iwlwifi snd_soc_acpi_intel_match snd_soc_acpi uvcvideo snd_hda_codec_hdmi x86_pkg_temp_thermal intel_powerclamp btusb coretemp btrtl btbcm snd_soc_core btintel kvm_intel videobuf2_vmalloc bluetooth snd_hda_codec_realtek videobuf2_memops videobuf2_v4l2 videobuf2_common snd_hda_codec_generic videodev snd_compress
> Jun 02 00:38:26 pp3345-laptop kernel: ac97_bus snd_pcm_dmaengine joydev snd_hda_intel kvm cfg80211 snd_hda_codec intel_wmi_thunderbolt wmi_bmof thunderbolt media snd_hda_core ecdh_generic snd_hwdep irqbypass snd_seq intel_cstate intel_uncore snd_seq_device intel_rapl_perf snd_pcm snd_timer thinkpad_acpi mei_me intel_xhci_usb_role_switch idma64 i2c_i801 mei roles ledtrig_audio ucsi_acpi snd intel_lpss_pci typec_ucsi intel_lpss processor_thermal_device intel_pch_thermal typec intel_soc_dts_iosf soundcore rfkill int3403_thermal int340x_thermal_zone int3400_thermal acpi_pad acpi_thermal_rel pcc_cpufreq dm_crypt i915 nouveau mxm_wmi ttm crct10dif_pclmul crc32_pclmul i2c_algo_bit crc32c_intel drm_kms_helper nvme drm nvme_core e1000e ghash_clmulni_intel uas usb_storage serio_raw wmi video fuse
> Jun 02 00:38:26 pp3345-laptop kernel: ---[ end trace 718b0122155852c5 ]---
> Jun 02 00:38:26 pp3345-laptop kernel: RIP: 0010:__list_del_entry_valid.cold+0x1d/0x55
> Jun 02 00:38:26 pp3345-laptop kernel: Code: c7 c7 18 55 11 b8 e8 f5 0b c7 ff 0f 0b 48 89 fe 48 c7 c7 a8 55 11 b8 e8 e4 0b c7 ff 0f 0b 48 c7 c7 58 56 11 b8 e8 d6 0b c7 ff <0f> 0b 48 89 f2 48 89 fe 48 c7 c7 18 56 11 b8 e8 c2 0b c7 ff 0f 0b
> Jun 02 00:38:26 pp3345-laptop kernel: RSP: 0000:ffffa8ec4ad73dc8 EFLAGS: 00010246
> Jun 02 00:38:26 pp3345-laptop kernel: RAX: 0000000000000054 RBX: ffff9a9f64e527f8 RCX: 0000000000000000
> Jun 02 00:38:26 pp3345-laptop kernel: RDX: 0000000000000000 RSI: ffff9a9fcf3568c8 RDI: ffff9a9fcf3568c8
> Jun 02 00:38:26 pp3345-laptop kernel: RBP: ffff9a9f64e56898 R08: ffff9a9fcf3568c8 R09: 0000000000000c6f
> Jun 02 00:38:26 pp3345-laptop kernel: R10: ffffffffb89f930c R11: 0000000000000003 R12: 0000000000000004
> Jun 02 00:38:26 pp3345-laptop kernel: R13: ffff9a9fc879a388 R14: 0000000000000000 R15: ffff9a9fc8799608
> Jun 02 00:38:26 pp3345-laptop kernel: FS: 0000000000000000(0000) GS:ffff9a9fcf340000(0000) knlGS:0000000000000000
> Jun 02 00:38:26 pp3345-laptop kernel: CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> Jun 02 00:38:26 pp3345-laptop kernel: CR2: 00001d3a916bf000 CR3: 0000000487c94003 CR4: 00000000003606e0
Am 02.06.19 um 15:48 schrieb Marc Haber:
> On Thu, May 30, 2019 at 10:12:57AM +0200, Marc Haber wrote:
>> on my primary notebook, a Lenovo X260, with an Intel Wireless 8260
>> (8086:24f3), running Debian unstable, I have started to see network
>> hangs since upgrading to kernel 5.1. In this situation, I cannot
>> restart Network-Manager (the call just hangs), I can log out of X, but
>> the system does not cleanly shut down and I need to Magic SysRq myself
>> out of the running system. This happens about once every two days.
>
> The issue is also present in 5.1.5 and 5.1.6.
>
> Greetings
> Marc
>
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Ard Biesheuvel @ 2019-06-07 20:27 UTC (permalink / raw)
To: 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: <97BB95F6-4A4C-4984-9EAB-6069E19B4A4F@holtmann.org>
On Fri, 7 Jun 2019 at 22:24, Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Eric,
>
> >> One of the issues that I would like to see addressed in the crypto API
> >> is they way the cipher abstraction is used. In general, a cipher should
> >> never be used directly, and so it would be much better to clean up the
> >> existing uses of ciphers outside of the crypto subsystem itself, so that
> >> we can make the cipher abstraction part of the internal API, only to
> >> be used by templates or crypto drivers that require them as a callback.
> >>
> >> As a first step, this series moves all users of the 'arc4' cipher to
> >> the ecb(arc4) skcipher, which happens to be implemented by the same
> >> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> >> actually evaluates to 1.
> >>
> >> Next step would be to switch the users of the 'des' and 'aes' ciphers
> >> to other interfaces that are more appropriate, either ecb(...) or a
> >> library interface, which may be more appropriate in some cases. In any
> >> case, the end result should be that ciphers are no longer used outside
> >> of crypto/ and drivers/crypto/
> >>
> >> This series is presented as an RFC, since I am mostly interested in
> >> discussing the above, but I prefer to do so in the context of actual
> >> patches rather than an abstract discussion.
> >>
> >> Ard Biesheuvel (3):
> >> net/mac80211: switch to skcipher interface for arc4
> >> lib80211/tkip: switch to skcipher interface for arc4
> >> lib80211/wep: switch to skcipher interface for arc4
> >>
> >
> > The way the crypto API exposes ARC4 is definitely broken. It treats it as a
> > block cipher (with a block size of 1 byte...), when it's actually a stream
> > cipher. Also, it violates the API by modifying the key during each encryption.
> >
> > Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
> > using, and the users call it on virtual addresses, perhaps we should instead
> > remove it from the crypto API and provide a library function arc4_crypt()? We'd
> > lose support for ARC4 in three hardware drivers, but are there real users who
> > really are using ARC4 and need those to get acceptable performance? Note that
> > they aren't being used in the cases where the 'cipher' API is currently being
> > used, so it would only be the current 'skcipher' users that might matter.
> >
> > Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
> > seems unlikely…
>
> that is not unlikely, we use ecb(arc4) via AF_ALG in iwd. It is what the WiFi standard defines to be used.
>
Ah ok, good to know. That does imply that the driver is not entirely
broken, which is good news I suppose.
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Marcel Holtmann @ 2019-06-07 20:24 UTC (permalink / raw)
To: Eric Biggers
Cc: Ard Biesheuvel, linux-crypto, Herbert Xu, Johannes Berg,
open list:NFC SUBSYSTEM, David S. Miller
In-Reply-To: <20190607175947.GB648@sol.localdomain>
Hi Eric,
>> One of the issues that I would like to see addressed in the crypto API
>> is they way the cipher abstraction is used. In general, a cipher should
>> never be used directly, and so it would be much better to clean up the
>> existing uses of ciphers outside of the crypto subsystem itself, so that
>> we can make the cipher abstraction part of the internal API, only to
>> be used by templates or crypto drivers that require them as a callback.
>>
>> As a first step, this series moves all users of the 'arc4' cipher to
>> the ecb(arc4) skcipher, which happens to be implemented by the same
>> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
>> actually evaluates to 1.
>>
>> Next step would be to switch the users of the 'des' and 'aes' ciphers
>> to other interfaces that are more appropriate, either ecb(...) or a
>> library interface, which may be more appropriate in some cases. In any
>> case, the end result should be that ciphers are no longer used outside
>> of crypto/ and drivers/crypto/
>>
>> This series is presented as an RFC, since I am mostly interested in
>> discussing the above, but I prefer to do so in the context of actual
>> patches rather than an abstract discussion.
>>
>> Ard Biesheuvel (3):
>> net/mac80211: switch to skcipher interface for arc4
>> lib80211/tkip: switch to skcipher interface for arc4
>> lib80211/wep: switch to skcipher interface for arc4
>>
>
> The way the crypto API exposes ARC4 is definitely broken. It treats it as a
> block cipher (with a block size of 1 byte...), when it's actually a stream
> cipher. Also, it violates the API by modifying the key during each encryption.
>
> Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
> using, and the users call it on virtual addresses, perhaps we should instead
> remove it from the crypto API and provide a library function arc4_crypt()? We'd
> lose support for ARC4 in three hardware drivers, but are there real users who
> really are using ARC4 and need those to get acceptable performance? Note that
> they aren't being used in the cases where the 'cipher' API is currently being
> used, so it would only be the current 'skcipher' users that might matter.
>
> Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
> seems unlikely…
that is not unlikely, we use ecb(arc4) via AF_ALG in iwd. It is what the WiFi standard defines to be used.
Regards
Marcel
^ permalink raw reply
* Re: pull-request: wireless-drivers 2019-06-07
From: David Miller @ 2019-06-07 19:41 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <87a7etsqg8.fsf@kamboji.qca.qualcomm.com>
From: Kalle Valo <kvalo@codeaurora.org>
Date: Fri, 07 Jun 2019 12:19:19 +0300
> here's a pull request to net tree for 5.2, more info below. Please let
> me know if there are any problems.
Pulled, thanks Kalle.
^ permalink raw reply
* [PATCH][next] qtnfmac: Use struct_size() in kzalloc()
From: Gustavo A. R. Silva @ 2019-06-07 19:17 UTC (permalink / raw)
To: Igor Mitsyanko, Avinash Patil, Sergey Matyukevich, Kalle Valo,
David S. Miller
Cc: linux-wireless, netdev, linux-kernel, Gustavo A. R. Silva
One of the more common cases of allocation size calculations is finding
the size of a structure that has a zero-sized array at the end, along
with memory for some number of elements for that array. For example:
struct ieee80211_regdomain {
...
struct ieee80211_reg_rule reg_rules[];
};
instance = kzalloc(sizeof(*mac->rd) +
sizeof(struct ieee80211_reg_rule) *
count, GFP_KERNEL);
Instead of leaving these open-coded and prone to type mistakes, we can
now use the new struct_size() helper:
instance = kzalloc(struct_size(instance, reg_rules, count), GFP_KERNEL);
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/net/wireless/quantenna/qtnfmac/commands.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/quantenna/qtnfmac/commands.c b/drivers/net/wireless/quantenna/qtnfmac/commands.c
index 459f6b81d2eb..dc0c7244b60e 100644
--- a/drivers/net/wireless/quantenna/qtnfmac/commands.c
+++ b/drivers/net/wireless/quantenna/qtnfmac/commands.c
@@ -1011,9 +1011,8 @@ qtnf_parse_variable_mac_info(struct qtnf_wmac *mac,
if (WARN_ON(resp->n_reg_rules > NL80211_MAX_SUPP_REG_RULES))
return -E2BIG;
- mac->rd = kzalloc(sizeof(*mac->rd) +
- sizeof(struct ieee80211_reg_rule) *
- resp->n_reg_rules, GFP_KERNEL);
+ mac->rd = kzalloc(struct_size(mac->rd, reg_rules, resp->n_reg_rules),
+ GFP_KERNEL);
if (!mac->rd)
return -ENOMEM;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v2 3/3] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Arend Van Spriel @ 2019-06-07 18:56 UTC (permalink / raw)
To: Doug Anderson
Cc: Adrian Hunter, Ulf Hansson, Kalle Valo, brcm80211-dev-list.pdl,
open list:ARM/Rockchip SoC..., Double Lo, Brian Norris,
linux-wireless, Naveen Gupta, Madhan Mohan R, Matthias Kaehlcke,
Wright Feng, Chi-Hsien Lin, netdev, brcm80211-dev-list,
David S. Miller, Franky Lin, LKML, Hante Meuleman, YueHaibing,
Michael Trimarchi
In-Reply-To: <CAD=FV=XVmCYWe9rtTFakq8yu67R-97EPyAHWck+o3dRXzHCchQ@mail.gmail.com>
On June 7, 2019 8:06:30 PM Doug Anderson <dianders@chromium.org> wrote:
> Hi,
>
> On Fri, Jun 7, 2019 at 6:32 AM Arend Van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>>
>> Right. I know it supports initial tuning, but I'm not sure about subsequent
>> retuning initiated by the host controller.
>
> My evidence says that it supports subsequent tuning. In fact, without
> this series my logs would be filled with:
>
> dwmmc_rockchip ff0d0000.dwmmc: Successfully tuned phase to XYZ
>
> ...where the phase varied by a few degrees each time. AKA: it was
> retuning over and over again and getting sane results which implies
> that the tuning was working just fine.
Ok. Thanks for confirming this.
Regards,
Arend
^ permalink raw reply
* Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook
From: Larry Finger @ 2019-06-07 18:50 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Aaro Koskinen, Christian Zigotzky, Michael Ellerman, linux-kernel,
linux-wireless, linuxppc-dev
In-Reply-To: <20190607172902.GA8183@lst.de>
On 6/7/19 12:29 PM, Christoph Hellwig wrote:
> I don't think we should work around this in the driver, we need to fix
> it in the core. I'm curious why my previous patch didn't work. Can
> you throw in a few printks what failed? I.e. did dma_direct_supported
> return false? Did the actual allocation fail?
I agree that that patch should not be sent upstream. I posted it only so that
anyone running into the problem would have a work around.
I will try to see why your patch failed.
Larry
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Ard Biesheuvel @ 2019-06-07 18:08 UTC (permalink / raw)
To: Eric Biggers
Cc: open list:HARDWARE RANDOM NUMBER GENERATOR CORE, Herbert Xu,
Johannes Berg, <linux-wireless@vger.kernel.org>,
David S. Miller
In-Reply-To: <20190607175947.GB648@sol.localdomain>
On Fri, 7 Jun 2019 at 19:59, Eric Biggers <ebiggers@kernel.org> wrote:
>
> On Fri, Jun 07, 2019 at 04:49:41PM +0200, Ard Biesheuvel wrote:
> > One of the issues that I would like to see addressed in the crypto API
> > is they way the cipher abstraction is used. In general, a cipher should
> > never be used directly, and so it would be much better to clean up the
> > existing uses of ciphers outside of the crypto subsystem itself, so that
> > we can make the cipher abstraction part of the internal API, only to
> > be used by templates or crypto drivers that require them as a callback.
> >
> > As a first step, this series moves all users of the 'arc4' cipher to
> > the ecb(arc4) skcipher, which happens to be implemented by the same
> > driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> > actually evaluates to 1.
> >
> > Next step would be to switch the users of the 'des' and 'aes' ciphers
> > to other interfaces that are more appropriate, either ecb(...) or a
> > library interface, which may be more appropriate in some cases. In any
> > case, the end result should be that ciphers are no longer used outside
> > of crypto/ and drivers/crypto/
> >
> > This series is presented as an RFC, since I am mostly interested in
> > discussing the above, but I prefer to do so in the context of actual
> > patches rather than an abstract discussion.
> >
> > Ard Biesheuvel (3):
> > net/mac80211: switch to skcipher interface for arc4
> > lib80211/tkip: switch to skcipher interface for arc4
> > lib80211/wep: switch to skcipher interface for arc4
> >
>
> The way the crypto API exposes ARC4 is definitely broken. It treats it as a
> block cipher (with a block size of 1 byte...), when it's actually a stream
> cipher. Also, it violates the API by modifying the key during each encryption.
>
> Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
> using, and the users call it on virtual addresses, perhaps we should instead
> remove it from the crypto API and provide a library function arc4_crypt()? We'd
> lose support for ARC4 in three hardware drivers, but are there real users who
> really are using ARC4 and need those to get acceptable performance? Note that
> they aren't being used in the cases where the 'cipher' API is currently being
> used, so it would only be the current 'skcipher' users that might matter.
>
In fact, this is what I started out doing, i.e., factor out the core
arc4 code into crypto/arc4_lib.c, and make the existing driver a thin
wrapper around it, so that we can invoke the library directly.
> Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
> seems unlikely...
>
Yes, that seems highly unlikely.
> As for removing the "cipher" API entirely, we'd have to consider how to convert
> all the current users, not just ARC4, so that would be a somewhat different
> discussion. How do you propose to handle dm-crypt and fscrypt which use the
> cipher API to do ESSIV?
>
Without having looked in too much detail, ESSIV seems like something
that could be moved into the crypto subsystem, and be implemented as a
template.
^ permalink raw reply
* Re: [PATCH v2 3/3] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Doug Anderson @ 2019-06-07 18:06 UTC (permalink / raw)
To: Arend Van Spriel
Cc: Adrian Hunter, Ulf Hansson, Kalle Valo, brcm80211-dev-list.pdl,
open list:ARM/Rockchip SoC..., Double Lo, Brian Norris,
linux-wireless, Naveen Gupta, Madhan Mohan R, Matthias Kaehlcke,
Wright Feng, Chi-Hsien Lin, netdev, brcm80211-dev-list,
David S. Miller, Franky Lin, LKML, Hante Meuleman, YueHaibing,
Michael Trimarchi
In-Reply-To: <16b3223dea0.2764.9b12b7fc0a3841636cfb5e919b41b954@broadcom.com>
Hi,
On Fri, Jun 7, 2019 at 6:32 AM Arend Van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On June 7, 2019 2:40:04 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> > On 7/06/19 8:12 AM, Arend Van Spriel wrote:
> >> On June 6, 2019 11:37:22 PM Doug Anderson <dianders@chromium.org> wrote:
> >>>
> >>> In the case of dw_mmc, which I'm most familiar with, we don't have any
> >>> sort of automated or timed-based retuning. ...so we'll only re-tune
> >>> when we see the CRC error. If I'm understanding things correctly then
> >>> that for dw_mmc my solution and yours behave the same. That means the
> >>> difference is how we deal with other retuning requests, either ones
> >>> that come about because of an interrupt that the host controller
> >>> provided or because of a timer. Did I get that right?
> >>
> >> Right.
> >>
> >>> ...and I guess the reason we have to deal specially with these cases
> >>> is because any time that SDIO card is "sleeping" we don't want to
> >>> retune because it won't work. Right? NOTE: the solution that would
> >>> come to my mind first to solve this would be to hold the retuning for
> >>> the whole time that the card was sleeping and then release it once the
> >>> card was awake again. ...but I guess we don't truly need to do that
> >>> because tuning only happens as a side effect of sending a command to
> >>> the card and the only command we send to the card is the "wake up"
> >>> command. That's why your solution to hold tuning while sending the
> >>> "wake up" command works, right?
> >>
> >> Yup.
> >>
> >>> ---
> >>>
> >>> OK, so assuming all the above is correct, I feel like we're actually
> >>> solving two problems and in fact I believe we actually need both our
> >>> approaches to solve everything correctly. With just your patch in
> >>> place there's a problem because we will clobber any external retuning
> >>> requests that happened while we were waking up the card. AKA, imagine
> >>> this:
> >>>
> >>> A) brcmf_sdio_kso_control(on=True) gets called; need_retune starts as 0
> >>>
> >>> B) We call sdio_retune_hold_now()
> >>>
> >>> C) A retuning timer goes off or the SD Host controller tells us to retune
> >>>
> >>> D) We get to the end of brcmf_sdio_kso_control() and clear the "retune
> >>> needed" since need_retune was 0 at the start.
> >>>
> >>> ...so we dropped the retuning request from C), right?
> >>>
> >>>
> >>> What we truly need is:
> >>>
> >>> 1. CRC errors shouldn't trigger a retuning request when we're in
> >>> brcmf_sdio_kso_control()
> >>>
> >>> 2. A separate patch that holds any retuning requests while the SDIO
> >>> card is off. This patch _shouldn't_ do any clearing of retuning
> >>> requests, just defer them.
> >>>
> >>>
> >>> Does that make sense to you? If so, I can try to code it up...
> >>
> >> FWIW it does make sense to me. However, I am still not sure if our sdio
> >> hardware supports retuning. Have to track down an asic designer who can tell
> >> or dive into vhdl myself.
> >
> > The card supports re-tuning if is handles CMD19, which it does. It is not
> > the card that does any tuning, only the host. The card just helps by
> > providing a known data pattern in response to CMD19. It can be that a card
> > provides good enough signals that the host should not need to re-tune. I
> > don't know if that can be affected by the board design though.
>
> Right. I know it supports initial tuning, but I'm not sure about subsequent
> retuning initiated by the host controller.
My evidence says that it supports subsequent tuning. In fact, without
this series my logs would be filled with:
dwmmc_rockchip ff0d0000.dwmmc: Successfully tuned phase to XYZ
...where the phase varied by a few degrees each time. AKA: it was
retuning over and over again and getting sane results which implies
that the tuning was working just fine.
The whole point of this series is not that the retuning was actually
broken or anything it was just pointless and blocking the bus while it
happened. On rk3288 dw_mmc ports we also currently do pretty
extensive tuning, trying _lots_ of phases. Thus the re-tuning was
blocking the bus for a significant amount of time.
-Doug
^ permalink raw reply
* Re: [PATCH v2 3/3] brcmfmac: sdio: Disable auto-tuning around commands expected to fail
From: Doug Anderson @ 2019-06-07 18:00 UTC (permalink / raw)
To: Adrian Hunter
Cc: Ulf Hansson, Kalle Valo, Arend van Spriel, brcm80211-dev-list.pdl,
open list:ARM/Rockchip SoC..., Double Lo, Brian Norris,
linux-wireless, Naveen Gupta, Madhan Mohan R, Matthias Kaehlcke,
Wright Feng, Chi-Hsien Lin, netdev, brcm80211-dev-list,
David S. Miller, Franky Lin, LKML, Hante Meuleman, YueHaibing,
Michael Trimarchi
In-Reply-To: <2e9f80af-aa26-5590-9ff0-9889400068d6@intel.com>
Hi,
On Fri, Jun 7, 2019 at 5:29 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> >> @@ -711,8 +720,16 @@ brcmf_sdio_kso_control(struct brcmf_sdio *bus, bool on)
> >> err_cnt = 0;
> >> }
> >> /* bail out upon subsequent access errors */
> >> - if (err && (err_cnt++ > BRCMF_SDIO_MAX_ACCESS_ERRORS))
> >> - break;
> >> + if (err && (err_cnt++ > BRCMF_SDIO_MAX_ACCESS_ERRORS)) {
> >> + if (!retune_release)
> >> + break;
> >> + /*
> >> + * Allow one more retry with re-tuning released in case
> >> + * it helps.
> >> + */
> >> + sdio_retune_release(bus->sdiodev->func1);
> >> + retune_release = false;
> >
> > I would be tempted to wait before adding this logic until we actually
> > see that it's needed. Sure, doing one more transfer probably won't
> > really hurt, but until we know that it actually helps it seems like
> > we're just adding extra complexity?
>
> Depends, what is the downside of unnecessarily returning an error from
> brcmf_sdio_kso_control() in that case?
I'm not aware of any downside, but without any example of it being
needed it's just untested code that might or might not fix a problem.
For now I'm going to leave it out of my patch and if someone later
finds it needed or if you're convinced that we really need it we can
add it as a patch atop.
-Doug
^ permalink raw reply
* Re: [RFC PATCH 0/3] move WEP implementation to skcipher interface
From: Eric Biggers @ 2019-06-07 17:59 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: linux-crypto, herbert, johannes, linux-wireless, davem
In-Reply-To: <20190607144944.13485-1-ard.biesheuvel@linaro.org>
On Fri, Jun 07, 2019 at 04:49:41PM +0200, Ard Biesheuvel wrote:
> One of the issues that I would like to see addressed in the crypto API
> is they way the cipher abstraction is used. In general, a cipher should
> never be used directly, and so it would be much better to clean up the
> existing uses of ciphers outside of the crypto subsystem itself, so that
> we can make the cipher abstraction part of the internal API, only to
> be used by templates or crypto drivers that require them as a callback.
>
> As a first step, this series moves all users of the 'arc4' cipher to
> the ecb(arc4) skcipher, which happens to be implemented by the same
> driver, and is already a stream cipher, given that ARC4_BLOCK_SIZE
> actually evaluates to 1.
>
> Next step would be to switch the users of the 'des' and 'aes' ciphers
> to other interfaces that are more appropriate, either ecb(...) or a
> library interface, which may be more appropriate in some cases. In any
> case, the end result should be that ciphers are no longer used outside
> of crypto/ and drivers/crypto/
>
> This series is presented as an RFC, since I am mostly interested in
> discussing the above, but I prefer to do so in the context of actual
> patches rather than an abstract discussion.
>
> Ard Biesheuvel (3):
> net/mac80211: switch to skcipher interface for arc4
> lib80211/tkip: switch to skcipher interface for arc4
> lib80211/wep: switch to skcipher interface for arc4
>
The way the crypto API exposes ARC4 is definitely broken. It treats it as a
block cipher (with a block size of 1 byte...), when it's actually a stream
cipher. Also, it violates the API by modifying the key during each encryption.
Since ARC4 is fast in software and is "legacy" crypto that people shouldn't be
using, and the users call it on virtual addresses, perhaps we should instead
remove it from the crypto API and provide a library function arc4_crypt()? We'd
lose support for ARC4 in three hardware drivers, but are there real users who
really are using ARC4 and need those to get acceptable performance? Note that
they aren't being used in the cases where the 'cipher' API is currently being
used, so it would only be the current 'skcipher' users that might matter.
Someone could theoretically be using "ecb(arc4)" via AF_ALG or dm-crypt, but it
seems unlikely...
As for removing the "cipher" API entirely, we'd have to consider how to convert
all the current users, not just ARC4, so that would be a somewhat different
discussion. How do you propose to handle dm-crypt and fscrypt which use the
cipher API to do ESSIV?
- Eric
^ permalink raw reply
* [PATCH] wlcore/wl18xx: Add invert-irq OF property for physically inverted IRQ
From: Eugeniu Rosca @ 2019-06-07 17:29 UTC (permalink / raw)
To: Geert Uytterhoeven, Simon Horman, Kalle Valo, David S. Miller,
Greg Kroah-Hartman, Randy Dunlap, Tony Lindgren, Ulf Hansson,
John Stultz, linux-wireless, netdev, linux-kernel,
Spyridon Papageorgiou, Joshua Frkuska, George G . Davis,
Andrey Gusakov, Linux-Renesas
Cc: Eugeniu Rosca, Eugeniu Rosca
The wl1837mod datasheet [1] says about the WL_IRQ pin:
---8<---
SDIO available, interrupt out. Active high. [..]
Set to rising edge (active high) on powerup.
---8<---
That's the reason of seeing the interrupt configured as:
- IRQ_TYPE_EDGE_RISING on HiKey 960/970
- IRQ_TYPE_LEVEL_HIGH on a number of i.MX6 platforms
We assert that all those platforms have the WL_IRQ pin connected
to the SoC _directly_ (confirmed on HiKey 970 [2]).
That's not the case for R-Car Kingfisher extension target, which carries
a WL1837MODGIMOCT IC. There is an SN74LV1T04DBVR inverter present
between the WLAN_IRQ pin of the WL18* chip and the SoC, effectively
reversing the requirement quoted from [1]. IOW, in Kingfisher DTS
configuration we would need to use IRQ_TYPE_EDGE_FALLING or
IRQ_TYPE_LEVEL_LOW.
Unfortunately, v4.2-rc1 commit bd763482c82ea2 ("wl18xx: wlan_irq:
support platform dependent interrupt types") made a special case out
of these interrupt types. After this commit, it is impossible to provide
an IRQ configuration via DTS which would describe an inverter present
between the WL18* chip and the SoC, generating the need for workarounds
like [3].
Create a boolean OF property, called "invert-irq" to specify that
the WLAN_IRQ pin of WL18* is connected to the SoC via an inverter.
This solution has been successfully tested on R-Car H3ULCB-KF-M06 using
the DTS configuration [4] combined with the "invert-irq" property.
[1] http://www.ti.com/lit/ds/symlink/wl1837mod.pdf
[2] https://www.96boards.org/documentation/consumer/hikey/hikey970/hardware-docs/
[3] https://github.com/CogentEmbedded/meta-rcar/blob/289fbd4f8354/meta-rcar-gen3-adas/recipes-kernel/linux/linux-renesas/0024-wl18xx-do-not-invert-IRQ-on-WLxxxx-side.patch
[4] https://patchwork.kernel.org/patch/10895879/
("arm64: dts: ulcb-kf: Add support for TI WL1837")
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
drivers/net/wireless/ti/wl18xx/main.c | 5 +++++
drivers/net/wireless/ti/wlcore/sdio.c | 2 ++
drivers/net/wireless/ti/wlcore/wlcore_i.h | 4 ++++
3 files changed, 11 insertions(+)
diff --git a/drivers/net/wireless/ti/wl18xx/main.c b/drivers/net/wireless/ti/wl18xx/main.c
index 496b9b63cea1..cea91d1aee98 100644
--- a/drivers/net/wireless/ti/wl18xx/main.c
+++ b/drivers/net/wireless/ti/wl18xx/main.c
@@ -877,6 +877,8 @@ static int wl18xx_pre_boot(struct wl1271 *wl)
static int wl18xx_pre_upload(struct wl1271 *wl)
{
+ struct platform_device *pdev = wl->pdev;
+ struct wlcore_platdev_data *pdata = dev_get_platdata(&pdev->dev);
u32 tmp;
int ret;
u16 irq_invert;
@@ -932,6 +934,9 @@ static int wl18xx_pre_upload(struct wl1271 *wl)
if (ret < 0)
goto out;
+ if (pdata->invert_irq)
+ goto out;
+
ret = irq_get_trigger_type(wl->irq);
if ((ret == IRQ_TYPE_LEVEL_LOW) || (ret == IRQ_TYPE_EDGE_FALLING)) {
wl1271_info("using inverted interrupt logic: %d", ret);
diff --git a/drivers/net/wireless/ti/wlcore/sdio.c b/drivers/net/wireless/ti/wlcore/sdio.c
index 4d4b07701149..581f56b0b6a2 100644
--- a/drivers/net/wireless/ti/wlcore/sdio.c
+++ b/drivers/net/wireless/ti/wlcore/sdio.c
@@ -266,6 +266,8 @@ static int wlcore_probe_of(struct device *dev, int *irq, int *wakeirq,
of_property_read_u32(np, "tcxo-clock-frequency",
&pdev_data->tcxo_clock_freq);
+ pdev_data->invert_irq = of_property_read_bool(np, "invert-irq");
+
return 0;
}
#else
diff --git a/drivers/net/wireless/ti/wlcore/wlcore_i.h b/drivers/net/wireless/ti/wlcore/wlcore_i.h
index 32ec121ccac2..01679f9d7170 100644
--- a/drivers/net/wireless/ti/wlcore/wlcore_i.h
+++ b/drivers/net/wireless/ti/wlcore/wlcore_i.h
@@ -213,6 +213,10 @@ struct wlcore_platdev_data {
u32 ref_clock_freq; /* in Hertz */
u32 tcxo_clock_freq; /* in Hertz, tcxo is always XTAL */
bool pwr_in_suspend;
+ bool invert_irq; /* specify if there is a physical IRQ inverter
+ * between WL chip and SoC, like SN74LV1T04DBVR
+ * in case of R-Car Kingfisher board
+ */
};
#define MAX_NUM_KEYS 14
--
2.21.0
^ permalink raw reply related
* Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook
From: Christoph Hellwig @ 2019-06-07 17:29 UTC (permalink / raw)
To: Larry Finger
Cc: Aaro Koskinen, Christoph Hellwig, Christian Zigotzky,
Michael Ellerman, linux-kernel, linux-wireless, linuxppc-dev
In-Reply-To: <73da300c-871c-77ac-8a3a-deac226743ef@lwfinger.net>
I don't think we should work around this in the driver, we need to fix
it in the core. I'm curious why my previous patch didn't work. Can
you throw in a few printks what failed? I.e. did dma_direct_supported
return false? Did the actual allocation fail?
^ permalink raw reply
* Re: [BISECTED REGRESSION] b43legacy broken on G4 PowerBook
From: Larry Finger @ 2019-06-07 17:25 UTC (permalink / raw)
To: Aaro Koskinen, Christoph Hellwig, Christian Zigotzky,
Michael Ellerman
Cc: linux-kernel, linux-wireless, linuxppc-dev
In-Reply-To: <20190605225059.GA9953@darkstar.musicnaut.iki.fi>
[-- Attachment #1: Type: text/plain, Size: 1169 bytes --]
On 6/5/19 5:50 PM, Aaro Koskinen wrote:
> Hi,
>
> When upgrading from v5.0 -> v5.1 on G4 PowerBook, I noticed WLAN does
> not work anymore:
>
> [ 42.004303] b43legacy-phy0: Loading firmware version 0x127, patch level 14 (2005-04-18 02:36:27)
> [ 42.184837] b43legacy-phy0 debug: Chip initialized
> [ 42.184873] b43legacy-phy0 ERROR: The machine/kernel does not support the required 30-bit DMA mask
>
> The same happens with the current mainline.
>
> Bisected to:
>
> commit 65a21b71f948406201e4f62e41f06513350ca390
> Author: Christoph Hellwig <hch@lst.de>
> Date: Wed Feb 13 08:01:26 2019 +0100
>
> powerpc/dma: remove dma_nommu_dma_supported
>
> This function is largely identical to the generic version used
> everywhere else. Replace it with the generic version.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Tested-by: Christian Zigotzky <chzigotzky@xenosoft.de>
> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au>
Aaro,
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.
Larry
[-- Attachment #2: 0001-b43legacy-Fix-DMA-breakage-from-commit-commit-65a21b.patch --]
[-- Type: text/x-patch, Size: 2674 bytes --]
From 25e2f50273e785598b6bd9a8aee28cf825d3fe9f Mon Sep 17 00:00:00 2001
From: Larry Finger <Larry.Finger@lwfinger.net>
Date: Fri, 7 Jun 2019 12:04:16 -0500
Subject: [PATCH] b43legacy: Fix DMA breakage from commit commit 65a21b71f948
To: kvalo@codeaurora.org
Cc: linux-wireless@vger.kernel.org,
pkshih@realtek.com
Following commit 65a21b71f948 ("powerpc/dma: remove dma_nommu_dma_supported"),
this driver errors with a message that "The machine/kernel does not
support the required 30-bit DMA mask." Indeed, the hardware only allows
31-bit masks. This solution is to change the fallback mask from 30-
to 31-bits for 32-bit PPC systems.
Fixes: 65a21b71f948 ("powerpc/dma: remove dma_nommu_dma_supported")
Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
drivers/net/wireless/broadcom/b43legacy/dma.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43legacy/dma.c b/drivers/net/wireless/broadcom/b43legacy/dma.c
index 1cc25f44dd9a..75613f516e50 100644
--- a/drivers/net/wireless/broadcom/b43legacy/dma.c
+++ b/drivers/net/wireless/broadcom/b43legacy/dma.c
@@ -27,6 +27,15 @@
#include <linux/slab.h>
#include <net/dst.h>
+/* Special handling for PPC32 - The maximum DMA mask is 31 bits, and
+ * the fallback to 30 bits fails. Set the fallback at 31.
+ */
+#ifdef CONFIG_PPC32
+#define FB_DMA 31
+#else
+#define FB_DMA 30
+#endif
+
/* 32bit DMA ops. */
static
struct b43legacy_dmadesc32 *op32_idx2desc(struct b43legacy_dmaring *ring,
@@ -418,7 +427,7 @@ static bool b43legacy_dma_mapping_error(struct b43legacy_dmaring *ring,
switch (ring->type) {
case B43legacy_DMA_30BIT:
- if ((u64)addr + buffersize > (1ULL << 30))
+ if ((u64)addr + buffersize > (1ULL << FB_DMA))
goto address_error;
break;
case B43legacy_DMA_32BIT:
@@ -617,12 +626,12 @@ static u64 supported_dma_mask(struct b43legacy_wldev *dev)
if (tmp & B43legacy_DMA32_TXADDREXT_MASK)
return DMA_BIT_MASK(32);
- return DMA_BIT_MASK(30);
+ return DMA_BIT_MASK(FB_DMA);
}
static enum b43legacy_dmatype dma_mask_to_engine_type(u64 dmamask)
{
- if (dmamask == DMA_BIT_MASK(30))
+ if (dmamask == DMA_BIT_MASK(FB_DMA))
return B43legacy_DMA_30BIT;
if (dmamask == DMA_BIT_MASK(32))
return B43legacy_DMA_32BIT;
@@ -802,7 +811,7 @@ static int b43legacy_dma_set_mask(struct b43legacy_wldev *dev, u64 mask)
continue;
}
if (mask == DMA_BIT_MASK(32)) {
- mask = DMA_BIT_MASK(30);
+ mask = DMA_BIT_MASK(FB_DMA);
fallback = true;
continue;
}
--
2.21.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox