Re: [PATCH v2 00/19] prevent bounds-check bypass via speculative execution
On Thu, Jan 11, 2018 at 5:19 PM, Linus Torvalds
wrote:
> Should the array access in entry_SYSCALL_64_fastpath be made to use
> the masking approach?
That one has a bounds check for an inline constant.
cmpq$__NR_syscall_max, %rax
so should be safe.
The classic Spectre variant #1 code sequence is:
int array_size;
if (x < array_size) {
something with array[x]
}
which runs into problems because the array_size variable may not
be in cache, and while the CPU core is waiting for the value it
speculates inside the "if" body.
The syscall entry is more like:
#define ARRAY_SIZE 10
if (x < ARRAY_SIZE) {
something with array[x]
}
Here there isn't any reason for speculation. The core has the
value of 'x' in a register and the upper bound encoded into the
"cmp" instruction. Both are right there, no waiting, no speculation.
-Tony
Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution
On Fri, Jan 12, 2018 at 12:01 PM, Christian Lamparter
wrote:
> On Friday, January 12, 2018 7:39:50 PM CET Dan Williams wrote:
>> On Fri, Jan 12, 2018 at 6:42 AM, Christian Lamparter
>> wrote:
>> > On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
>> >> Static analysis reports that 'queue' may be a user controlled value that
>> >> is used as a data dependency to read from the 'ar9170_qmap' array. In
>> >> order to avoid potential leaks of kernel memory values, block
>> >> speculative execution of the instruction stream that could issue reads
>> >> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
>> >> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
>> >> 'ar->edcf' array.
>> >>
>> >> Based on an original patch by Elena Reshetova.
>> >>
>> >> Cc: Christian Lamparter
>> >> Cc: Kalle Valo
>> >> Cc: linux-wireless@vger.kernel.org
>> >> Cc: net...@vger.kernel.org
>> >> Signed-off-by: Elena Reshetova
>> >> Signed-off-by: Dan Williams
>> >> ---
>> > This patch (and p54, cw1200) look like the same patch?!
>> > Can you tell me what happend to:
>> >
>> > On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
>> >> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter
>> >> wrote:
>> >> > And Furthermore a invalid queue (param->ac) would cause a crash in
>> >> > this line in mac80211 before it even reaches the driver [1]:
>> >> > | sdata->tx_conf[params->ac] = p;
>> >> > |
>> >> > | if (drv_conf_tx(local, sdata, params->ac , &p)) {
>> >> > |^^ (this is a wrapper for the *_op_conf_tx)
>> >> >
>> >> > I don't think these chin-up exercises are needed.
>> >>
>> >> Quite the contrary, you've identified a better place in the call stack
>> >> to sanitize the input and disable speculation. Then we can kill the
>> >> whole class of the wireless driver reports at once it seems.
>> >
>>
>> I didn't see where ac is being validated against the driver specific
>> 'queues' value in that earlier patch.
> The link to the check is right there in the earlier post. It's in
> parse_txq_params():
>
> | if (txq_params->ac >= NL80211_NUM_ACS)
> | return -EINVAL;
>
> NL80211_NUM_ACS is 4
>
>
> This check was added ever since mac80211's ieee80211_set_txq_params():
> | sdata->tx_conf[params->ac] = p;
>
> For cw1200: the driver just sets the dev->queue to 4.
> In carl9170 dev->queues is set to __AR9170_NUM_TXQ and
> p54 uses P54_QUEUE_AC_NUM.
>
> Both __AR9170_NUM_TXQ and P54_QUEUE_AC_NUM are 4.
> And this is not going to change since all drivers
> have to follow mac80211's queue API:
>
>
> Some background:
> In the old days (linux 2.6 and early 3.x), the parse_txq_params() function did
> not verify the "queue" value. That's why these drivers had to do it.
>
> Here's the relevant code from 2.6.39:
>
> You'll notice that the check is missing there.
> Here's mac80211's ieee80211_set_txq_params for reference:
>
>
> However over time, the check in the driver has become redundant.
>
Excellent, thank you for pointing that out and the background so clearly.
What this tells me though is that we want to inject an ifence() at
this input validation point, i.e.:
if (txq_params->ac >= NL80211_NUM_ACS) {
ifence();
return -EINVAL;
}
...but the kernel, in these patches, only has ifence() defined for
x86. The only way we can sanitize the 'txq_params->ac' value without
ifence() is to do it at array access time, but then we're stuck
touching all drivers when standard kernel development practice says
'refactor common code out of drivers'.
Ugh, the bigger concern is that this driver is being flagged and not
that initial bounds check. Imagine if cw1200 and p54 had already been
converted to assume that they can just trust 'queue'. It would never
have been flagged.
I think we should focus on the get_user path and __fcheck_files for v3.
[PATCH 1/2] Revert "mwifiex: cancel pcie/sdio work in remove/shutdown handler"
This reverts commit b713bbf1471b56b572ce26bd02b81a85c2b007f4.
The "fix" in question does not actually fix all related problems, and it
also introduces new deadlock possibilities. Since commit b014e96d1abb
("PCI: Protect pci_error_handlers->reset_notify() usage with
device_lock()"), the race in question is actually resolved (PCIe reset
cannot happen at the same time as remove()). Instead, this "fix" just
introduces a deadlock where mwifiex_pcie_card_reset_work() is waiting on
device_lock, which is held by PCIe device remove(), which is waiting
on...mwifiex_pcie_card_reset_work().
The proper thing to do is just to fix the deadlock. Patch for this will
come separately.
Cc: Signed-off-by: Xinming Hu
Signed-off-by: Brian Norris
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 2 --
drivers/net/wireless/marvell/mwifiex/sdio.c | 2 --
2 files changed, 4 deletions(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index 23209c5cab05..f666cb2ea7e0 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -310,8 +310,6 @@ static void mwifiex_pcie_remove(struct pci_dev *pdev)
mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
}
- cancel_work_sync(&card->work);
-
mwifiex_remove_card(adapter);
}
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c
b/drivers/net/wireless/marvell/mwifiex/sdio.c
index 248858723753..a82880132af4 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -399,8 +399,6 @@ mwifiex_sdio_remove(struct sdio_func *func)
mwifiex_init_shutdown_fw(priv, MWIFIEX_FUNC_SHUTDOWN);
}
- cancel_work_sync(&card->work);
-
mwifiex_remove_card(adapter);
}
--
2.16.0.rc1.238.g530d649a79-goog
[PATCH 2/2] mwifiex: resolve reset vs. remove()/shutdown() deadlocks
Commit b014e96d1abb ("PCI: Protect pci_error_handlers->reset_notify()
usage with device_lock()") resolves races between driver reset and
removal, but it introduces some new deadlock problems. If we see a
timeout while we've already started suspending, removing, or shutting
down the driver, we might see:
(a) a worker thread, running mwifiex_pcie_work() ->
mwifiex_pcie_card_reset_work() -> pci_reset_function()
(b) a removal thread, running mwifiex_pcie_remove() ->
mwifiex_free_adapter() -> mwifiex_unregister() ->
mwifiex_cleanup_pcie() -> cancel_work_sync(&card->work)
Unfortunately, mwifiex_pcie_remove() already holds the device lock that
pci_reset_function() is now requesting, and so we see a deadlock.
It's necessary to cancel and synchronize our outstanding work before
tearing down the driver, so we can't have this work wait indefinitely
for the lock.
It's reasonable to only "try" to reset here, since this will mostly
happen for cases where it's already difficult to reset the firmware
anyway (e.g., while we're suspending or powering off the system). And if
reset *really* needs to happen, we can always try again later.
Fixes: b014e96d1abb ("PCI: Protect pci_error_handlers->reset_notify() usage
with device_lock()")
Cc:
Cc: Xinming Hu
Signed-off-by: Brian Norris
---
drivers/net/wireless/marvell/mwifiex/pcie.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c
b/drivers/net/wireless/marvell/mwifiex/pcie.c
index f666cb2ea7e0..97a6199692ab 100644
--- a/drivers/net/wireless/marvell/mwifiex/pcie.c
+++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
@@ -2786,7 +2786,10 @@ static void mwifiex_pcie_card_reset_work(struct
mwifiex_adapter *adapter)
{
struct pcie_service_card *card = adapter->card;
- pci_reset_function(card->dev);
+ /* We can't afford to wait here; remove() might be waiting on us. If we
+* can't grab the device lock, maybe we'll get another chance later.
+*/
+ pci_try_reset_function(card->dev);
}
static void mwifiex_pcie_work(struct work_struct *work)
--
2.16.0.rc1.238.g530d649a79-goog
Re: brcmfmac4329-sdio firmware load failed.
Thank you Arend. I updated to master again, v4.15-rc7+, and applied your patch. All log snips are grabbed with dmesg|grep -E 'mmc0|brcm', as the sdio device is on mmc0. Without patch [1], for reference... [0.00] Kernel command line: console=tty0 selinux=0 video=1280x800 root=/dev/mmcblk1p1 brcmfmac.bebug=0x2 [3.952561] mmc0: Invalid maximum block size, assuming 512 bytes [4.010466] mmc0: SDHCI controller on c800.sdhci [c800.sdhci] using ADMA [4.338545] mmc0: queuing unknown CIS tuple 0x80 (50 bytes) [4.347098] mmc0: queuing unknown CIS tuple 0x80 (7 bytes) [4.350099] mmc0: queuing unknown CIS tuple 0x80 (7 bytes) [4.378430] mmc0: queuing unknown CIS tuple 0x02 (1 bytes) [4.388387] mmc0: new SDIO card at address 0001 [4.401773] brcmfmac: F1 signature read @0x1800=0x9934329 [4.407624] brcmfmac: brcmf_sdiod_regrw_helper: failed to read data F1@0x080ac, err: -84 [4.410159] brcmfmac: brcmf_fw_map_chip_to_name: using brcm/brcmfmac4329-sdio.bin for chip 0x004329(17193) rev 0x03 [4.781374] brcmfmac mmc0:0001:1: Direct firmware load for brcm/brcmfmac4329-sdio.clm_blob failed with error -2 [4.781491] brcmfmac mmc0:0001:1: Falling back to user helper [ 69.601645] brcmfmac: brcmf_c_process_clm_blob: request CLM blob file failed (-11) [ 69.601668] brcmfmac: brcmf_c_preinit_dcmds: download CLM blob file failed, -11 [ 69.601685] brcmfmac: brcmf_bus_started: failed: -11 [ 69.601728] brcmfmac: brcmf_sdio_firmware_callback: dongle is not responding With patch [1]... [0.00] Kernel command line: console=tty0 selinux=0 video=1280x800 root=/dev/mmcblk1p1 brcmfmac.bebug=0x2 [3.954628] mmc0: Invalid maximum block size, assuming 512 bytes [4.010608] mmc0: SDHCI controller on c800.sdhci [c800.sdhci] using ADMA [4.341727] mmc0: queuing unknown CIS tuple 0x80 (50 bytes) [4.349595] mmc0: queuing unknown CIS tuple 0x80 (7 bytes) [4.352695] mmc0: queuing unknown CIS tuple 0x80 (7 bytes) [4.381292] mmc0: queuing unknown CIS tuple 0x02 (1 bytes) [4.387377] mmc0: new SDIO card at address 0001 [4.399393] brcmfmac: F1 signature read @0x1800=0x9934329 [4.405883] brcmfmac: brcmf_sdiod_regrw_helper: failed to read data F1@0x080ac, err: -84 [4.407207] brcmfmac: brcmf_fw_map_chip_to_name: using brcm/brcmfmac4329-sdio.bin for chip 0x004329(17193) rev 0x03 [4.709436] brcmfmac mmc0:0001:1: Direct firmware load for brcm/brcmfmac4329-sdio.clm_blob failed with error -2 [4.709517] brcmfmac mmc0:0001:1: Falling back to user helper [ 69.710122] brcmfmac mmc0:0001:1: Direct firmware load for brcm/brcmfmac4329-sdio.clm_blob failed with error -2 [ 69.710137] brcmfmac mmc0:0001:1: Falling back to user helper [ 131.054899] brcmfmac: brcmf_c_preinit_dcmds: Firmware version = wl0: Sep 2 2011 14:48:19 version 4.220.48 [ 131.072561] brcmfmac: brcmf_setup_wiphybands: rxchain error (-23) [ 131.388384] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 131.392390] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 131.920861] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 131.924183] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 132.593074] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 133.135973] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 133.138223] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 133.152149] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 134.311983] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 134.458577] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 134.461640] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 [ 134.555048] brcmfmac: _brcmf_set_multicast_list: Setting BRCMF_C_SET_PROMISC failed, -23 I can now connect to an AP with the following caveats: 1) It takes about two minutes before the wlan device is available. For a long while I didn't think it was working because I would check dmesg and check ifconfig -a before the wlan would be present. 2) After reboot I get this nasty error... [0.00] Kernel command line: console=tty0 selinux=0 video=1280x800 root=/dev/mmcblk1p1 brcmfmac.bebug=0x2 [2.269750] mmc0: Invalid maximum block size, assuming 512 bytes [2.330010] mmc0: SDHCI controller on c800.sdhci [c800.sdhci] using ADMA [2.645242] mmc0: error -110 whilst initialising SDIO card Regarding the patch, brcmfmac tries to load a firmware file twice for a device which the firmware file does not exist. Is there a know list of devices which do not need it that we can use to bypass brcmf_c_process_clm_blob()? Thanks, Kyle On 01/10/2018 03:47 A
Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution
On Friday, January 12, 2018 7:39:50 PM CET Dan Williams wrote:
> On Fri, Jan 12, 2018 at 6:42 AM, Christian Lamparter
> wrote:
> > On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
> >> Static analysis reports that 'queue' may be a user controlled value that
> >> is used as a data dependency to read from the 'ar9170_qmap' array. In
> >> order to avoid potential leaks of kernel memory values, block
> >> speculative execution of the instruction stream that could issue reads
> >> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
> >> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
> >> 'ar->edcf' array.
> >>
> >> Based on an original patch by Elena Reshetova.
> >>
> >> Cc: Christian Lamparter
> >> Cc: Kalle Valo
> >> Cc: linux-wireless@vger.kernel.org
> >> Cc: net...@vger.kernel.org
> >> Signed-off-by: Elena Reshetova
> >> Signed-off-by: Dan Williams
> >> ---
> > This patch (and p54, cw1200) look like the same patch?!
> > Can you tell me what happend to:
> >
> > On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
> >> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter
> >> wrote:
> >> > And Furthermore a invalid queue (param->ac) would cause a crash in
> >> > this line in mac80211 before it even reaches the driver [1]:
> >> > | sdata->tx_conf[params->ac] = p;
> >> > |
> >> > | if (drv_conf_tx(local, sdata, params->ac , &p)) {
> >> > |^^ (this is a wrapper for the *_op_conf_tx)
> >> >
> >> > I don't think these chin-up exercises are needed.
> >>
> >> Quite the contrary, you've identified a better place in the call stack
> >> to sanitize the input and disable speculation. Then we can kill the
> >> whole class of the wireless driver reports at once it seems.
> >
>
> I didn't see where ac is being validated against the driver specific
> 'queues' value in that earlier patch.
The link to the check is right there in the earlier post. It's in
parse_txq_params():
| if (txq_params->ac >= NL80211_NUM_ACS)
| return -EINVAL;
NL80211_NUM_ACS is 4
This check was added ever since mac80211's ieee80211_set_txq_params():
| sdata->tx_conf[params->ac] = p;
For cw1200: the driver just sets the dev->queue to 4.
In carl9170 dev->queues is set to __AR9170_NUM_TXQ and
p54 uses P54_QUEUE_AC_NUM.
Both __AR9170_NUM_TXQ and P54_QUEUE_AC_NUM are 4.
And this is not going to change since all drivers
have to follow mac80211's queue API:
Some background:
In the old days (linux 2.6 and early 3.x), the parse_txq_params() function did
not verify the "queue" value. That's why these drivers had to do it.
Here's the relevant code from 2.6.39:
You'll notice that the check is missing there.
Here's mac80211's ieee80211_set_txq_params for reference:
However over time, the check in the driver has become redundant.
> > Anyway, I think there's an easy way to solve this: remove the
> > "if (queue < ar->hw->queues)" check altogether. It's no longer needed
> > anymore as the "queue" value is validated long before the driver code
> > gets called.
> >
> > And from my understanding, this will fix the "In this case
> > the value of 'ar9170_qmap[queue]' is immediately reused as an index to
> > the 'ar->edcf' array." gripe your tool complains about.
> >
> > This is here's a quick test-case for carl9170.:
> > ---
> > diff --git a/drivers/net/wireless/ath/carl9170/main.c
> > b/drivers/net/wireless/ath/carl9170/main.c
> > index 988c8857d78c..2d3afb15bb62 100644
> > --- a/drivers/net/wireless/ath/carl9170/main.c
> > +++ b/drivers/net/wireless/ath/carl9170/main.c
> > @@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw
> > *hw,
> > int ret;
> >
> > mutex_lock(&ar->mutex);
> > - if (queue < ar->hw->queues) {
> > - memcpy(&ar->edcf[ar9170_qmap[queue]], param,
> > sizeof(*param));
> > - ret = carl9170_set_qos(ar);
> > - } else {
> > - ret = -EINVAL;
> > - }
> > -
> > + memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
> > + ret = carl9170_set_qos(ar);
> > mutex_unlock(&ar->mutex);
> > return ret;
> > }
> > ---
> > What does your tool say about this?
>
> If you take away the 'if' then I don't the tool will report on this.
>
> > (If necessary, the "queue" value could be even sanitized with a
> > queue %= ARRAY_SIZE(ar9170_qmap); before the mutex_lock.)
>
> That is what array_ptr() is doing in a more sophisticated w
Re: [PATCH v2 01/10] rtlwifi: Use mutex to replace spin_lock to protect IPS and LPS
On 01/11/2018 01:09 AM, pks...@realtek.com wrote:
From: Ping-Ke Shih
Enter/leavel IPS and LPS are large critical section, and they can't use
sleep function because running in atomic-context, which own a spin_lock.
In commit ba9f93f82aba ("rtlwifi: Fix enter/exit power_save"), it moves
LPS functions to thread-context, so this commit can simply change LPS's
spin lock to mutex.
Considering IPS functions, rtl_ips_nic_on() may be called by TX tasklet
(softirq-context) that check whether packet is auth frame. Fortunately,
current mac80211 will ask driver to leave IPS using op_config with
changed flag IEEE80211_CONF_CHANGE_IDLE, before issuing auth frame, so
IPS functions can run in thread-context and use mutex to protect critical
section, too.
Also, this commit removes some useless spin locks.
Signed-off-by: Ping-Ke Shih
Acked-by: Larry Finger
---
drivers/net/wireless/realtek/rtlwifi/base.c | 6 ++
drivers/net/wireless/realtek/rtlwifi/ps.c | 24 ++--
drivers/net/wireless/realtek/rtlwifi/usb.c | 1 -
drivers/net/wireless/realtek/rtlwifi/wifi.h | 10 ++
4 files changed, 14 insertions(+), 27 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c
b/drivers/net/wireless/realtek/rtlwifi/base.c
index 0ba9c0cc95e1..89ec318598ea 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -551,7 +551,8 @@ int rtl_init_core(struct ieee80211_hw *hw)
/* <4> locks */
mutex_init(&rtlpriv->locks.conf_mutex);
- spin_lock_init(&rtlpriv->locks.ips_lock);
+ mutex_init(&rtlpriv->locks.ips_mutex);
+ mutex_init(&rtlpriv->locks.lps_mutex);
spin_lock_init(&rtlpriv->locks.irq_th_lock);
spin_lock_init(&rtlpriv->locks.h2c_lock);
spin_lock_init(&rtlpriv->locks.rf_ps_lock);
@@ -561,9 +562,7 @@ int rtl_init_core(struct ieee80211_hw *hw)
spin_lock_init(&rtlpriv->locks.c2hcmd_lock);
spin_lock_init(&rtlpriv->locks.scan_list_lock);
spin_lock_init(&rtlpriv->locks.cck_and_rw_pagea_lock);
- spin_lock_init(&rtlpriv->locks.check_sendpkt_lock);
spin_lock_init(&rtlpriv->locks.fw_ps_lock);
- spin_lock_init(&rtlpriv->locks.lps_lock);
spin_lock_init(&rtlpriv->locks.iqk_lock);
/* <5> init list */
INIT_LIST_HEAD(&rtlpriv->entry_list);
@@ -1229,7 +1228,6 @@ bool rtl_tx_mgmt_proc(struct ieee80211_hw *hw, struct
sk_buff *skb)
}
if (ieee80211_is_auth(fc)) {
RT_TRACE(rtlpriv, COMP_SEND, DBG_DMESG, "MAC80211_LINKING\n");
- rtl_ips_nic_on(hw);
mac->link_state = MAC80211_LINKING;
/* Dul mac */
diff --git a/drivers/net/wireless/realtek/rtlwifi/ps.c
b/drivers/net/wireless/realtek/rtlwifi/ps.c
index 24c87fae5382..6a4008845f49 100644
--- a/drivers/net/wireless/realtek/rtlwifi/ps.c
+++ b/drivers/net/wireless/realtek/rtlwifi/ps.c
@@ -289,7 +289,7 @@ void rtl_ips_nic_on(struct ieee80211_hw *hw)
cancel_delayed_work(&rtlpriv->works.ips_nic_off_wq);
- spin_lock(&rtlpriv->locks.ips_lock);
+ mutex_lock(&rtlpriv->locks.ips_mutex);
if (ppsc->inactiveps) {
rtstate = ppsc->rfpwr_state;
@@ -306,7 +306,7 @@ void rtl_ips_nic_on(struct ieee80211_hw *hw)
ppsc->inactive_pwrstate);
}
}
- spin_unlock(&rtlpriv->locks.ips_lock);
+ mutex_unlock(&rtlpriv->locks.ips_mutex);
}
EXPORT_SYMBOL_GPL(rtl_ips_nic_on);
@@ -415,7 +415,6 @@ static void rtl_lps_enter_core(struct ieee80211_hw *hw)
struct rtl_mac *mac = rtl_mac(rtl_priv(hw));
struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
struct rtl_priv *rtlpriv = rtl_priv(hw);
- unsigned long flag;
if (!ppsc->fwctrl_lps)
return;
@@ -436,7 +435,7 @@ static void rtl_lps_enter_core(struct ieee80211_hw *hw)
if (mac->link_state != MAC80211_LINKED)
return;
- spin_lock_irqsave(&rtlpriv->locks.lps_lock, flag);
+ mutex_lock(&rtlpriv->locks.lps_mutex);
/* Don't need to check (ppsc->dot11_psmode == EACTIVE), because
* bt_ccoexist may ask to enter lps.
@@ -446,7 +445,7 @@ static void rtl_lps_enter_core(struct ieee80211_hw *hw)
"Enter 802.11 power save mode...\n");
rtl_lps_set_psmode(hw, EAUTOPS);
- spin_unlock_irqrestore(&rtlpriv->locks.lps_lock, flag);
+ mutex_unlock(&rtlpriv->locks.lps_mutex);
}
/* Interrupt safe routine to leave the leisure power save mode.*/
@@ -455,9 +454,8 @@ static void rtl_lps_leave_core(struct ieee80211_hw *hw)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_ps_ctl *ppsc = rtl_psc(rtl_priv(hw));
struct rtl_hal *rtlhal = rtl_hal(rtl_priv(hw));
- unsigned long flag;
- spin_lock_irqsave(&rtlpriv->locks.lps_lock, flag);
+ mutex_lock(&rtlpriv->locks.lps_mutex);
if (ppsc->fwctrl_lps)
Re: Re: Re: [PATCH] mwifiex: cancel pcie/sdio work in remove/shutdown handler
On Thu, Jan 11, 2018 at 06:25:09PM -0800, Brian Norris wrote: > Anyway, I'll do my own testing and then submit my patch properly. OK, so I definitely confirmed: if your patch does anything, it introduces a new deadlock possibility. Just trigger a Wifi timeout or reset from within remove(), and you'll see the work event get stuck in pci_reset_function(), while remove() gets stuck at cancel_work_sync(). I also confirmed that my patch resolves this problem. I'll send the revert + my patch now. Brian
Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution
On Fri, Jan 12, 2018 at 6:42 AM, Christian Lamparter wrote:
> On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
>> Static analysis reports that 'queue' may be a user controlled value that
>> is used as a data dependency to read from the 'ar9170_qmap' array. In
>> order to avoid potential leaks of kernel memory values, block
>> speculative execution of the instruction stream that could issue reads
>> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
>> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
>> 'ar->edcf' array.
>>
>> Based on an original patch by Elena Reshetova.
>>
>> Cc: Christian Lamparter
>> Cc: Kalle Valo
>> Cc: linux-wireless@vger.kernel.org
>> Cc: net...@vger.kernel.org
>> Signed-off-by: Elena Reshetova
>> Signed-off-by: Dan Williams
>> ---
> This patch (and p54, cw1200) look like the same patch?!
> Can you tell me what happend to:
>
> On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
>> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter
>> wrote:
>> > And Furthermore a invalid queue (param->ac) would cause a crash in
>> > this line in mac80211 before it even reaches the driver [1]:
>> > | sdata->tx_conf[params->ac] = p;
>> > |
>> > | if (drv_conf_tx(local, sdata, params->ac , &p)) {
>> > |^^ (this is a wrapper for the *_op_conf_tx)
>> >
>> > I don't think these chin-up exercises are needed.
>>
>> Quite the contrary, you've identified a better place in the call stack
>> to sanitize the input and disable speculation. Then we can kill the
>> whole class of the wireless driver reports at once it seems.
>
I didn't see where ac is being validated against the driver specific
'queues' value in that earlier patch.
>
> Anyway, I think there's an easy way to solve this: remove the
> "if (queue < ar->hw->queues)" check altogether. It's no longer needed
> anymore as the "queue" value is validated long before the driver code
> gets called.
Can you point me to where that validation happens?
> And from my understanding, this will fix the "In this case
> the value of 'ar9170_qmap[queue]' is immediately reused as an index to
> the 'ar->edcf' array." gripe your tool complains about.
>
> This is here's a quick test-case for carl9170.:
> ---
> diff --git a/drivers/net/wireless/ath/carl9170/main.c
> b/drivers/net/wireless/ath/carl9170/main.c
> index 988c8857d78c..2d3afb15bb62 100644
> --- a/drivers/net/wireless/ath/carl9170/main.c
> +++ b/drivers/net/wireless/ath/carl9170/main.c
> @@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
> int ret;
>
> mutex_lock(&ar->mutex);
> - if (queue < ar->hw->queues) {
> - memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
> - ret = carl9170_set_qos(ar);
> - } else {
> - ret = -EINVAL;
> - }
> -
> + memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
> + ret = carl9170_set_qos(ar);
> mutex_unlock(&ar->mutex);
> return ret;
> }
> ---
> What does your tool say about this?
If you take away the 'if' then I don't the tool will report on this.
> (If necessary, the "queue" value could be even sanitized with a
> queue %= ARRAY_SIZE(ar9170_qmap); before the mutex_lock.)
That is what array_ptr() is doing in a more sophisticated way.
[PATCH] ath10k: remove redundant -ve check against u32 integer size
From: Colin Ian King
Variable section_table.size is a u32 and so cannot be less than
zero, hence the less than zero check is redundant and can be
removed.
Detected by CoverityScan, CID#1463855 ("Unsigned compared against 0")
Signed-off-by: Colin Ian King
---
drivers/net/wireless/ath/ath10k/pci.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c
b/drivers/net/wireless/ath/ath10k/pci.c
index f6a23f2d0335..355db6a0fcf3 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1478,9 +1478,6 @@ static int ath10k_pci_dump_memory_section(struct ath10k
*ar,
if (!mem_region || !buf)
return 0;
- if (mem_region->section_table.size < 0)
- return 0;
-
cur_section = &mem_region->section_table.sections[0];
if (mem_region->start > cur_section->start) {
--
2.15.1
Re: [PATCH v2 15/19] carl9170: prevent bounds-check bypass via speculative execution
On Friday, January 12, 2018 1:47:46 AM CET Dan Williams wrote:
> Static analysis reports that 'queue' may be a user controlled value that
> is used as a data dependency to read from the 'ar9170_qmap' array. In
> order to avoid potential leaks of kernel memory values, block
> speculative execution of the instruction stream that could issue reads
> based on an invalid result of 'ar9170_qmap[queue]'. In this case the
> value of 'ar9170_qmap[queue]' is immediately reused as an index to the
> 'ar->edcf' array.
>
> Based on an original patch by Elena Reshetova.
>
> Cc: Christian Lamparter
> Cc: Kalle Valo
> Cc: linux-wireless@vger.kernel.org
> Cc: net...@vger.kernel.org
> Signed-off-by: Elena Reshetova
> Signed-off-by: Dan Williams
> ---
This patch (and p54, cw1200) look like the same patch?!
Can you tell me what happend to:
On Saturday, January 6, 2018 5:34:03 PM CET Dan Williams wrote:
> On Sat, Jan 6, 2018 at 6:23 AM, Christian Lamparter
> wrote:
> > And Furthermore a invalid queue (param->ac) would cause a crash in
> > this line in mac80211 before it even reaches the driver [1]:
> > | sdata->tx_conf[params->ac] = p;
> > |
> > | if (drv_conf_tx(local, sdata, params->ac , &p)) {
> > |^^ (this is a wrapper for the *_op_conf_tx)
> >
> > I don't think these chin-up exercises are needed.
>
> Quite the contrary, you've identified a better place in the call stack
> to sanitize the input and disable speculation. Then we can kill the
> whole class of the wireless driver reports at once it seems.
Anyway, I think there's an easy way to solve this: remove the
"if (queue < ar->hw->queues)" check altogether. It's no longer needed
anymore as the "queue" value is validated long before the driver code
gets called. And from my understanding, this will fix the "In this case
the value of 'ar9170_qmap[queue]' is immediately reused as an index to
the 'ar->edcf' array." gripe your tool complains about.
This is here's a quick test-case for carl9170.:
---
diff --git a/drivers/net/wireless/ath/carl9170/main.c
b/drivers/net/wireless/ath/carl9170/main.c
index 988c8857d78c..2d3afb15bb62 100644
--- a/drivers/net/wireless/ath/carl9170/main.c
+++ b/drivers/net/wireless/ath/carl9170/main.c
@@ -1387,13 +1387,8 @@ static int carl9170_op_conf_tx(struct ieee80211_hw *hw,
int ret;
mutex_lock(&ar->mutex);
- if (queue < ar->hw->queues) {
- memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
- ret = carl9170_set_qos(ar);
- } else {
- ret = -EINVAL;
- }
-
+ memcpy(&ar->edcf[ar9170_qmap[queue]], param, sizeof(*param));
+ ret = carl9170_set_qos(ar);
mutex_unlock(&ar->mutex);
return ret;
}
---
What does your tool say about this?
(If necessary, the "queue" value could be even sanitized with a
queue %= ARRAY_SIZE(ar9170_qmap); before the mutex_lock.)
[PATCH] ssb: Prevent build of PCI host features in module
Attempting to build ssb.ko with CONFIG_SSB_DRIVER_PCICORE=y results in a build error due to use of symbols not exported from vmlinux: ERROR: "pcibios_enable_device" [drivers/ssb/ssb.ko] undefined! ERROR: "register_pci_controller" [drivers/ssb/ssb.ko] undefined! make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1 To prevent this, don't allow the host mode feature to be built if CONFIG_SSB=m Signed-off-by: Matt Redfearn --- drivers/ssb/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/ssb/Kconfig b/drivers/ssb/Kconfig index d8e4219c2324..3e1c814e7331 100644 --- a/drivers/ssb/Kconfig +++ b/drivers/ssb/Kconfig @@ -118,7 +118,7 @@ config SSB_SERIAL config SSB_DRIVER_PCICORE_POSSIBLE bool - depends on SSB_PCIHOST + depends on SSB_PCIHOST && SSB = y default y config SSB_DRIVER_PCICORE -- 2.7.4
[PATCH] bcma: Prevent build of PCI host features in module
Attempting to build bcma.ko with BCMA_DRIVER_PCI_HOSTMODE=y results in a build error due to use of symbols not exported from vmlinux: ERROR: "pcibios_enable_device" [drivers/bcma/bcma.ko] undefined! ERROR: "register_pci_controller" [drivers/bcma/bcma.ko] undefined! make[1]: *** [scripts/Makefile.modpost:92: __modpost] Error 1 To prevent this, don't allow the host mode feature to be built if CONFIG_BCMA=m Signed-off-by: Matt Redfearn --- drivers/bcma/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bcma/Kconfig b/drivers/bcma/Kconfig index 02d78f6cecbb..4294784b9cf1 100644 --- a/drivers/bcma/Kconfig +++ b/drivers/bcma/Kconfig @@ -55,7 +55,7 @@ config BCMA_DRIVER_PCI config BCMA_DRIVER_PCI_HOSTMODE bool "Driver for PCI core working in hostmode" - depends on MIPS && BCMA_DRIVER_PCI + depends on MIPS && BCMA_DRIVER_PCI && BCMA = y help PCI core hostmode operation (external PCI bus). -- 2.7.4
Re: [PATCH 0/5] iwlwifi: updates intended for v4.16 2017-12-23
+ emmanuel Luca Coelho writes: > From: Luca Coelho > > Hi, > > Here's the fourth and probably last batch of patches intended for > 4.16. Nothing major, just continued development, some cleanups and > small fixes here and there. > > * Fix a UBSAN warning; > * Improvement in the net-stack/driver log syncing > * An RCU lock fix in the new rate-scaling code; > * Small cleanups; > > As usual, I'm pushing this to a pending branch, for kbuild bot, and > will send a pull-request later. > > FYI, I'll be on holidays/vacations for the next few weeks, so there > will probably not be much activity in the iwlwifi driver code. > > Please review. > > Cheers, > Luca. > > > Luca Coelho (1): > iwlwifi: mvm: check if mac80211_queue is valid in iwl_mvm_disable_txq > > Mordechay Goodstein (1): > iwlwifi: set default timstamp marker cmd > > Sara Sharon (3): > iwlwifi: mvm: flip AMSDU addresses only for 9000 family > iwlwifi: mvm: take RCU lock before dereferencing > iwlwifi: mvm: move TSO segment to a separate function Most likely the merge window starts in 9 days so I need to start getting patches ready. As Luca is away should I apply these patches directly to wireless-drivers-next? -- Kalle Valo
Re: [PATCH 02/10] qtnfmac: pass complete channel data between driver and firmware
Sergey Matyukevich writes:
>> > +/**
>> > * struct qlink_chandef - qlink channel definition
>> > *
>> > + * @chan: primary channel definition
>> > * @center_freq1: center frequency of first segment
>> > * @center_freq2: center frequency of second segment (80+80 only)
>> > * @width: channel width, one of @enum qlink_channel_width
>> > */
>> > struct qlink_chandef {
>> > + struct qlink_channel chan;
>> > __le16 center_freq1;
>> > __le16 center_freq2;
>> > u8 width;
>> > - u8 rsvd[3];
>> > + u8 rsvd;
>> > } __packed;
>>
>> Doesn't this break backwards compatibility with the older firmware? The
>> basic princinple is that old firmware images continue to work with newer
>> driver (or there will be a firmware image with new name, eg. fw-2.bin).
>> You can check how iwlwifi does that.
>
> Yes, it breaks. That is why we increment qlink protocol version in each
> change affecting backwards compatibility. So driver is going to work only
> with matching firmware. This is a very simplistic approach, but it looks
> reasonable for current stage of development since we keep adding features.
Everyone are always adding new features, that's no excuse to break
backwards compatibility with user space. In the future you really need
to come up a way to handle the firmware interface breaks gracefully,
just like iwlwifi does.
Related to this, any progress on getting the firmware image to
linux-firmware?
--
Kalle Valo
Re: [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
Arend van Spriel writes:
> On 1/12/2018 8:44 AM, Wright Feng wrote:
>> For legacy chips without CLM blob files, kernel with user helper function
>> returns -EAGAIN when we request_firmware() for blob file.
_Why_ is the -EAGAIN returned? Is it because of user space, due to
timing when loading the brcmfmac module or what? You should explain the
problem in detail in the commit log and why this is the right approach
to fix the problem.
Based on the commit log to me this still looks like a random attempt to
workaround a bug, not a proper fix.
>> In this case, brcmf_bus_started gets error and failed to bring up
>> legacy chips. Because of that, we should continue with CLM data
>> currently present in firmware if getting -EAGAIN when doing
>> request_firmware().
>>
>> Signed-off-by: Wright Feng
>> ---
>> v2: remove retry from patch v1
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> index 6a59d06..0baab4c 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
>> @@ -182,7 +182,7 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
>>
>> err = request_firmware(&clm, clm_name, dev);
>> if (err) {
>> -if (err == -ENOENT) {
>> +if (err == -ENOENT || err == -EAGAIN) {
>> brcmf_dbg(INFO, "continue with CLM data currently
>> present in firmware\n");
>> return 0;
>> }
>
> Why don't we just fall-back to "CLM in firmware" regardless of the
> error code?
Indeed, I was thinking the same.
--
Kalle Valo
Re: [PATCH v2] brcmfmac: fix CLM load error for legacy chips when user helper is enabled
On 1/12/2018 8:44 AM, Wright Feng wrote:
For legacy chips without CLM blob files, kernel with user helper function
returns -EAGAIN when we request_firmware() for blob file. In this case,
brcmf_bus_started gets error and failed to bring up legacy chips.
Because of that, we should continue with CLM data currently present in
firmware if getting -EAGAIN when doing request_firmware().
Signed-off-by: Wright Feng
---
v2: remove retry from patch v1
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
index 6a59d06..0baab4c 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c
@@ -182,7 +182,7 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp)
err = request_firmware(&clm, clm_name, dev);
if (err) {
- if (err == -ENOENT) {
+ if (err == -ENOENT || err == -EAGAIN) {
brcmf_dbg(INFO, "continue with CLM data currently present in
firmware\n");
return 0;
}
Hi Wright,
Why don't we just fall-back to "CLM in firmware" regardless of the error
code? Also it might be better to use brcmf_info() instead of
brcmf_dbg(INFO, ..).
Regards,
Arend
Re: [PATCH v2 00/19] prevent bounds-check bypass via speculative execution
Do you think that the appropriate patches could be copied to the
appropriate people please?
On Thu, Jan 11, 2018 at 04:46:24PM -0800, Dan Williams wrote:
> Changes since v1 [1]:
> * fixup the ifence definition to use alternative_2 per recent AMD
> changes in tip/x86/pti (Tom)
>
> * drop 'nospec_ptr' (Linus, Mark)
>
> * rename 'nospec_array_ptr' to 'array_ptr' (Alexei)
>
> * rename 'nospec_barrier' to 'ifence' (Peter, Ingo)
>
> * clean up occasions of 'variable assignment in if()' (Sergei, Stephen)
>
> * make 'array_ptr' use a mask instead of an architectural ifence by
> default (Linus, Alexei)
>
> * provide a command line and compile-time opt-in to the ifence
> mechanism, if an architecture provides 'ifence_array_ptr'.
>
> * provide an optimized mask generation helper, 'array_ptr_mask', for
> x86 (Linus)
>
> * move 'get_user' hardening from '__range_not_ok' to '__uaccess_begin'
> (Linus)
>
> * drop "Thermal/int340x: prevent bounds-check..." since userspace does
> not have arbitrary control over the 'trip' index (Srinivas)
>
> * update the changelog of "net: mpls: prevent bounds-check..." and keep
> it in the series to continue the debate about Spectre hygiene patches.
> (Eric).
>
> * record a reviewed-by from Laurent on "[media] uvcvideo: prevent
> bounds-check..."
>
> * update the cover letter
>
> [1]: https://lwn.net/Articles/743376/
>
> ---
>
> Quoting Mark's original RFC:
>
> "Recently, Google Project Zero discovered several classes of attack
> against speculative execution. One of these, known as variant-1, allows
> explicit bounds checks to be bypassed under speculation, providing an
> arbitrary read gadget. Further details can be found on the GPZ blog [2]
> and the Documentation patch in this series."
>
> This series incorporates Mark Rutland's latest ARM changes and adds
> the x86 specific implementation of 'ifence_array_ptr'. That ifence
> based approach is provided as an opt-in fallback, but the default
> mitigation, '__array_ptr', uses a 'mask' approach that removes
> conditional branches instructions, and otherwise aims to redirect
> speculation to use a NULL pointer rather than a user controlled value.
>
> The mask is generated by the following from Alexei, and Linus:
>
> mask = ~(long)(_i | (_s - 1 - _i)) >> (BITS_PER_LONG - 1);
>
> ...and Linus provided an optimized mask generation helper for x86:
>
> asm ("cmpq %1,%2; sbbq %0,%0;"
> :"=r" (mask)
> :"r"(sz),"r" (idx)
> :"cc");
>
> The 'array_ptr' mechanism can be switched between 'mask' and 'ifence'
> via the spectre_v1={mask,ifence} command line option, and the
> compile-time default is set by selecting either CONFIG_SPECTRE1_MASK or
> CONFIG_SPECTRE1_IFENCE.
>
> The 'array_ptr' infrastructure is the primary focus this patch set. The
> individual patches that perform 'array_ptr' conversions are a point in
> time (i.e. earlier kernel, early analysis tooling, x86 only etc...)
> start at finding some of these gadgets.
>
> Another consideration for reviewing these patches is the 'hygiene'
> argument. When a patch refers to hygiene it is concerned with stopping
> speculation on an unconstrained or insufficiently constrained pointer
> value under userspace control. That by itself is not sufficient for
> attack (per current understanding) [3], but it is a necessary
> pre-condition. So 'hygiene' refers to cleaning up those suspect
> pointers regardless of whether they are usable as a gadget.
>
> These patches are also be available via the 'nospec-v2' git branch
> here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/djbw/linux nospec-v2
>
> Note that the BPF fix for Spectre variant1 is merged in the bpf.git
> tree [4], and is not included in this branch.
>
> [2]:
> https://googleprojectzero.blogspot.co.uk/2018/01/reading-privileged-memory-with-side.html
> [3]: https://spectreattack.com/spectre.pdf
> [4]:
> https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git/commit/?id=b2157399cc98
>
> ---
>
> Dan Williams (16):
> x86: implement ifence()
> x86: implement ifence_array_ptr() and array_ptr_mask()
> asm-generic/barrier: mask speculative execution flows
> x86: introduce __uaccess_begin_nospec and ASM_IFENCE
> x86: use __uaccess_begin_nospec and ASM_IFENCE in get_user paths
> ipv6: prevent bounds-check bypass via speculative execution
> ipv4: prevent bounds-check bypass via speculative execution
> vfs, fdtable: prevent bounds-check bypass via speculative execution
> userns: prevent bounds-check bypass via speculative execution
> udf: prevent bounds-check bypass via speculative execution
> [media] uvcvideo: prevent bounds-check bypass via speculative execution
> carl9170: prevent bounds-check bypass via speculative execution
> p54: prevent bounds-check bypass via speculative execution
> qla2xxx: prevent bounds-check bypass via speculative execution
> cw1
[PATCH][next] ath10k: fix spelling mistake: "addrress" -> "address"
From: Colin Ian King
Trivial fix to spelling mistake in warning message text.
Signed-off-by: Colin Ian King
---
drivers/net/wireless/ath/ath10k/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/pci.c
b/drivers/net/wireless/ath/ath10k/pci.c
index 8abaccc25227..f6a23f2d0335 100644
--- a/drivers/net/wireless/ath/ath10k/pci.c
+++ b/drivers/net/wireless/ath/ath10k/pci.c
@@ -1484,7 +1484,7 @@ static int ath10k_pci_dump_memory_section(struct ath10k
*ar,
cur_section = &mem_region->section_table.sections[0];
if (mem_region->start > cur_section->start) {
- ath10k_warn(ar, "incorrect memdump region 0x%x with section
start addrress 0x%x.\n",
+ ath10k_warn(ar, "incorrect memdump region 0x%x with section
start address 0x%x.\n",
mem_region->start, cur_section->start);
return 0;
}
--
2.15.1
Re: [PATCH v3 00/27] kill devm_ioremap_nocache
Hi Christophe ，
On 2018/1/4 16:05, Christophe LEROY wrote:
>
>
> Le 25/12/2017 à 02:34, Yisheng Xie a écrit :
>>
>>
>> On 2017/12/24 17:05, christophe leroy wrote:
>>>
>>>
>>> Le 23/12/2017 à 14:48, Greg KH a écrit :
On Sat, Dec 23, 2017 at 06:55:25PM +0800, Yisheng Xie wrote:
> Hi all,
>
> When I tried to use devm_ioremap function and review related code, I found
> devm_ioremap and devm_ioremap_nocache is almost the same with each other,
> except one use ioremap while the other use ioremap_nocache.
For all arches? Really? Look at MIPS, and x86, they have different
functions.
> While ioremap's
> default function is ioremap_nocache, so devm_ioremap_nocache also have the
> same function with devm_ioremap, which can just be killed to reduce the
> size
> of devres.o(from 20304 bytes to 18992 bytes in my compile environment).
>
> I have posted two versions, which use macro instead of function for
> devm_ioremap_nocache[1] or devm_ioremap[2]. And Greg suggest me to kill
> devm_ioremap_nocache for no need to keep a macro around for the duplicate
> thing. So here comes v3 and please help to review.
I don't think this can be done, what am I missing? These functions are
not identical, sorry for missing that before.
>>>
>>> devm_ioremap() and devm_ioremap_nocache() are quite similar, both use
>>> devm_ioremap_release() for the release, why not just defining:
>>>
>>> static void __iomem *__devm_ioremap(struct device *dev, resource_size_t
>>> offset,
>>> resource_size_t size, bool nocache)
>>> {
>>> [...]
>>> if (nocache)
>>> addr = ioremap_nocache(offset, size);
>>> else
>>> addr = ioremap(offset, size);
>>> [...]
>>> }
>>>
>>> then in include/linux/io.h
>>>
>>> static inline void __iomem *devm_ioremap(struct device *dev,
>>> resource_size_t offset,
>>> resource_size_t size)
>>> {return __devm_ioremap(dev, offset, size, false);}
>>>
>>> static inline void __iomem *devm_ioremap_nocache(struct device *dev,
>>> resource_size_t offset,
>>> resource_size_t size);
>>> {return __devm_ioremap(dev, offset, size, true);}
>>
>> Yeah, this seems good to me, right now we have devm_ioremap,
>> devm_ioremap_wc, devm_ioremap_nocache
>> May be we can use an enum like:
>> typedef enum {
>> DEVM_IOREMAP = 0,
>> DEVM_IOREMAP_NOCACHE,
>> DEVM_IOREMAP_WC,
>> } devm_ioremap_type;
>>
>> static inline void __iomem *devm_ioremap(struct device *dev, resource_size_t
>> offset,
>> resource_size_t size)
>> {return __devm_ioremap(dev, offset, size, DEVM_IOREMAP);}
>>
>> static inline void __iomem *devm_ioremap_nocache(struct device *dev,
>> resource_size_t offset,
>> resource_size_t size);
>> {return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_NOCACHE);}
>>
>> static inline void __iomem *devm_ioremap_wc(struct device *dev,
>> resource_size_t offset,
>> resource_size_t size);
>> {return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_WC);}
>>
>> static void __iomem *__devm_ioremap(struct device *dev, resource_size_t
>> offset,
>> resource_size_t size, devm_ioremap_type type)
>> {
>> void __iomem **ptr, *addr = NULL;
>> [...]
>> switch (type){
>> case DEVM_IOREMAP:
>> addr = ioremap(offset, size);
>> break;
>> case DEVM_IOREMAP_NOCACHE:
>> addr = ioremap_nocache(offset, size);
>> break;
>> case DEVM_IOREMAP_WC:
>> addr = ioremap_wc(offset, size);
>> break;
>> }
>> [...]
>> }
>
>
> That looks good to me, will you submit a v4 ?
Sorry for late response. And I will submit the v4 as your suggestion.
Thanks
Yisheng
>
> Christophe
>
>>
