* Re: [PATCH 0/6] stricter netlink validation
From: Johannes Berg @ 2019-04-05 15:13 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, pablo, dsa
In-Reply-To: <d423d283c91a48f75a509ee74255327cfe92fe97.camel@sipsolutions.net>
On Fri, 2019-04-05 at 13:47 +0200, Johannes Berg wrote:
>
> I've also pushed some very much WIP code to the netlink-policy-export
> branch there that exposes the policies to userspace, there at least for
> generic netlink now.
Seems to more or less work now, userspace gets things like (for
nl80211):
(ID 0x18 is the nl80211 genl family)
ID: 0x18 policy[0]:attr[1]: type=U32
[...]
ID: 0x18 policy[0]:attr[87]: type=U32
ID: 0x18 policy[0]:attr[88]: type=U64
ID: 0x18 policy[0]:attr[89]: type=U8
ID: 0x18 policy[0]:attr[90]: type=NESTED
ID: 0x18 policy[0]:attr[91]: type=BINARY
[...]
ID: 0x18 policy[0]:attr[270]: type=NESTED policy:1
[...]
ID: 0x18 policy[0]:attr[273]: type=NESTED policy:2
[...]
ID: 0x18 policy[1]:attr[1]: type=FLAG
ID: 0x18 policy[1]:attr[2]: type=BINARY
ID: 0x18 policy[1]:attr[3]: type=BINARY
[...]
ID: 0x18 policy[2]:attr[1]: type=REJECT
ID: 0x18 policy[2]:attr[2]: type=REJECT
ID: 0x18 policy[2]:attr[3]: type=REJECT
ID: 0x18 policy[2]:attr[4]: type=REJECT
ID: 0x18 policy[2]:attr[5]: type=NESTED_ARRAY policy:3
[...]
ID: 0x18 policy[3]:attr[3]: type=NESTED policy:4
etc.
See net/wireless/nl80211.c nl80211_policy[] for the original data, it's
unchanged over current net-next.
Policy 0 is - by convention - the top-level policy, but once I fix the
recursion issue in validate_nla() it's possible that a nested attribute
refers back to the top-level policy.
There are some bugs, like it generating an almost-empty message for when
the type is NLA_UNSPEC rather than eliding it entirely, and I haven't
implemented a bunch of things yet:
/* TODO advertise range (min/max) */
/* TODO advertise min/max len */
/* TODO show reject string if any */
Also, I haven't hooked it up to anything that's not generic netlink, but
the API should be general enough for anyone:
int netlink_policy_dump_start(const struct nla_policy *policy,
unsigned int maxtype,
unsigned long *state);
bool netlink_policy_dump_loop(unsigned long *state);
int netlink_policy_dump_write(struct sk_buff *skb, unsigned long state);
(*state/state is &cb->args[n]/cb->args[n] for the netlink dump, it will
generate one message per type. That may be overkill, but it lets us
include the potentially long reject string etc. without worrying about
any message size limitations.)
It feels like it's working, and so I'd like to propose formal patches
soon.
Pablo, what do you think? It seems to me that this type of thing would
address most if not all what you did with the object/bus description
stuff, while not writing any new code, the info is taken straight from
the policy.
johannes
^ permalink raw reply
* Re: [PATCH] ath10k: Don't log a traceback on invalid event IDs.
From: Ben Greear @ 2019-04-05 14:30 UTC (permalink / raw)
To: Peter Jones, Kalle Valo
Cc: David S. Miller, ath10k, linux-wireless, netdev, linux-kernel
In-Reply-To: <20190405100508.12679-1-pjones@redhat.com>
On 04/05/2019 03:05 AM, Peter Jones wrote:
> Very frequently upon resume from suspend, we see tracebacks like:
>
> ath10k_pci 0000:02:00.0: unsupported HTC service id: 1536
> ath10k_pci 0000:02:00.0: Unknown eventid: 118809
> WARNING: CPU: 1 PID: 23135 at drivers/net/wireless/ath/ath10k/mac.c:5650 ath10k_bss_info_changed+0xe8e/0xfd0 [ath10k_core]
From what I can tell below, you did not actually remove the WARN_ON?
I think it is worth keeping a one-line logging message about these unknown events.
They can indicate firmware API mismatch. They could be made to only print once
per boot if that helped.
And, if you are changing the logging messages, maybe put in extra info about what
code path it is taking, for instance:
+ ath10k_info(ar, ATH10K_DBG_WMI, "Unknown TLV op eventid: %d\n", id);
Thanks,
Ben
> Modules linked in: ...
> CPU: 1 PID: 23135 Comm: kworker/u16:54 Not tainted 4.20.15-200.fc29.x86_64 #1
> Hardware name: ...
> Workqueue: events_unbound async_run_entry_fn
> RIP: 0010:ath10k_bss_info_changed+0xe8e/0xfd0 [ath10k_core]
> Code: 89 c1 89 f2 4c 89 d7 48 c7 c6 60 7e ed c0 4c 89 4c 24 10 89 44 24 08 e8 e0 35 00 00 8b 44 24 08 4c 8b 4c 24 10 e9 22 f8 ff ff <0f> 0b e9 7a f4 ff ff 8b b3 70 01 00 00 89 f2 89 c1 48 c7 c6 20 7e
> RSP: 0018:ffffaba5456b7cc0 EFLAGS: 00010246
> RAX: 0000000000000000 RBX: ffff8d9b612a33a0 RCX: 0000000000000000
> RDX: 0000000000000001 RSI: 00000000fffffe01 RDI: ffffffffc0cca4ae
> RBP: 00000000020674ff R08: 0000000000200000 R09: 0000000000000000
> R10: ffff8d9a6d490700 R11: 0000000039a6c800 R12: ffff8d9b612a33a8
> R13: ffff8d9b4bd31580 R14: ffff8d9b4bd31580 R15: ffff8d9b612a3500
> FS: 0000000000000000(0000) GS:ffff8d9b66440000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 0000556cca653b66 CR3: 000000027920a001 CR4: 00000000003606e0
> Call Trace:
> ? ath10k_conf_tx+0x7a/0x4b0 [ath10k_core]
> ieee80211_bss_info_change_notify+0xab/0x1a0 [mac80211]
> ieee80211_reconfig+0x66c/0x1300 [mac80211]
> ? __switch_to_asm+0x34/0x70
> ? __switch_to_asm+0x34/0x70
> ? __switch_to_asm+0x40/0x70
> wiphy_resume+0x7f/0x120 [cfg80211]
> ? index_show+0x20/0x20 [cfg80211]
> dpm_run_callback+0x59/0x150
> device_resume+0xac/0x1f0
> async_resume+0x19/0x30
> async_run_entry_fn+0x39/0x160
> process_one_work+0x1a1/0x3a0
> worker_thread+0x30/0x380
> ? process_one_work+0x3a0/0x3a0
> kthread+0x112/0x130
> ? kthread_create_on_node+0x60/0x60
> ret_from_fork+0x35/0x40
> ---[ end trace 0afe382792dba858 ]---
>
> These errors are completely transient, and the card goes back to working
> perfectly fine afterwards. I suspect the driver is just reading an
> uninitialized buffer as the card comes back up.
>
> In any case, there's no point in treating this as a major problem and
> logging a full register dump and a traceback; just logginng an INFO
> message saying that it happened is enough. This patch does that.
>
> Signed-off-by: Peter Jones <pjones@redhat.com>
> ---
> drivers/net/wireless/ath/ath10k/htc.c | 4 ++--
> drivers/net/wireless/ath/ath10k/sdio.c | 2 +-
> drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 +-
> drivers/net/wireless/ath/ath10k/wmi.c | 8 ++++----
> 4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
> index 805a7f8a04f..c244889b72e 100644
> --- a/drivers/net/wireless/ath/ath10k/htc.c
> +++ b/drivers/net/wireless/ath/ath10k/htc.c
> @@ -799,7 +799,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
> &ep->ul_pipe_id,
> &ep->dl_pipe_id);
> if (status) {
> - ath10k_warn(ar, "unsupported HTC service id: %d\n",
> + ath10k_info(ar, "unsupported HTC service id: %d\n",
> ep->service_id);
> return status;
> }
> @@ -877,7 +877,7 @@ static bool ath10k_htc_pktlog_svc_supported(struct ath10k *ar)
> &ul_pipe_id,
> &dl_pipe_id);
> if (status) {
> - ath10k_warn(ar, "unsupported HTC service id: %d\n",
> + ath10k_info(ar, "unsupported HTC service id: %d\n",
> ATH10K_HTC_SVC_ID_HTT_LOG_MSG);
>
> return false;
> diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
> index fae56c67766..a265225216e 100644
> --- a/drivers/net/wireless/ath/ath10k/sdio.c
> +++ b/drivers/net/wireless/ath/ath10k/sdio.c
> @@ -1871,7 +1871,7 @@ static int ath10k_sdio_hif_map_service_to_pipe(struct ath10k *ar,
> ar_sdio->mbox_addr[eid], ar_sdio->mbox_size[eid]);
> break;
> default:
> - ath10k_warn(ar, "unsupported HTC service id: %d\n",
> + ath10k_info(ar, "unsupported HTC service id: %d\n",
> service_id);
> return -EINVAL;
> }
> diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> index 582fb11f648..ca990c8d306 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
> @@ -614,7 +614,7 @@ static void ath10k_wmi_tlv_op_rx(struct ath10k *ar, struct sk_buff *skb)
> ath10k_wmi_event_mgmt_tx_bundle_compl(ar, skb);
> break;
> default:
> - ath10k_dbg(ar, ATH10K_DBG_WMI, "Unknown eventid: %d\n", id);
> + ath10k_info(ar, ATH10K_DBG_WMI, "Unknown eventid: %d\n", id);
> break;
> }
>
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index 98a90e49d66..f4fa406d9fe 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -5850,7 +5850,7 @@ static void ath10k_wmi_op_rx(struct ath10k *ar, struct sk_buff *skb)
> ath10k_wmi_event_service_available(ar, skb);
> break;
> default:
> - ath10k_warn(ar, "Unknown eventid: %d\n", id);
> + ath10k_info(ar, "Unknown eventid: %d\n", id);
> break;
> }
>
> @@ -5981,7 +5981,7 @@ static void ath10k_wmi_10_1_op_rx(struct ath10k *ar, struct sk_buff *skb)
> /* ignore utf events */
> break;
> default:
> - ath10k_warn(ar, "Unknown eventid: %d\n", id);
> + ath10k_info(ar, "Unknown eventid: %d\n", id);
> break;
> }
>
> @@ -6130,7 +6130,7 @@ static void ath10k_wmi_10_2_op_rx(struct ath10k *ar, struct sk_buff *skb)
> ath10k_wmi_event_peer_sta_ps_state_chg(ar, skb);
> break;
> default:
> - ath10k_warn(ar, "Unknown eventid: %d\n", id);
> + ath10k_info(ar, "Unknown eventid: %d\n", id);
> break;
> }
>
> @@ -6250,7 +6250,7 @@ static void ath10k_wmi_10_4_op_rx(struct ath10k *ar, struct sk_buff *skb)
> ath10k_wmi_event_peer_sta_ps_state_chg(ar, skb);
> break;
> default:
> - ath10k_warn(ar, "Unknown eventid: %d\n", id);
> + ath10k_info(ar, "Unknown eventid: %d\n", id);
> break;
> }
>
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* [mac80211-next:netlink-policy-export 11/11] include/linux/kern_levels.h:5:18: warning: format '%s' expects argument of type 'char *', but argument 2 has type 'void *'
From: kbuild test robot @ 2019-04-05 14:06 UTC (permalink / raw)
To: Johannes Berg; +Cc: kbuild-all, linux-wireless
[-- Attachment #1: Type: text/plain, Size: 3702 bytes --]
tree: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git netlink-policy-export
head: 334240a63c7e6da8be21a0c469751afe72e73ef0
commit: 334240a63c7e6da8be21a0c469751afe72e73ef0 [11/11] netlink: add infrastructure to expose policies to userspace
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 334240a63c7e6da8be21a0c469751afe72e73ef0
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=xtensa
All warnings (new ones prefixed by >>):
In file included from include/linux/printk.h:7,
from include/linux/kernel.h:15,
from include/linux/list.h:9,
from include/linux/module.h:9,
from net//netlink/genetlink.c:10:
net//netlink/genetlink.c: In function 'ctrl_dumppolicy':
>> include/linux/kern_levels.h:5:18: warning: format '%s' expects argument of type 'char *', but argument 2 has type 'void *' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^~~~~~
include/linux/kern_levels.h:15:20: note: in expansion of macro 'KERN_SOH'
#define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
^~~~~~~~
net//netlink/genetlink.c:983:11: note: in expansion of macro 'KERN_DEBUG'
printk(KERN_DEBUG "find by name '%s'\n", nla_data(tb[CTRL_ATTR_FAMILY_NAME]));
^~~~~~~~~~
net//netlink/genetlink.c:983:38: note: format string is defined here
printk(KERN_DEBUG "find by name '%s'\n", nla_data(tb[CTRL_ATTR_FAMILY_NAME]));
~^
%p
--
In file included from include/linux/printk.h:7,
from include/linux/kernel.h:15,
from include/linux/list.h:9,
from include/linux/module.h:9,
from net/netlink/genetlink.c:10:
net/netlink/genetlink.c: In function 'ctrl_dumppolicy':
>> include/linux/kern_levels.h:5:18: warning: format '%s' expects argument of type 'char *', but argument 2 has type 'void *' [-Wformat=]
#define KERN_SOH "\001" /* ASCII Start Of Header */
^~~~~~
include/linux/kern_levels.h:15:20: note: in expansion of macro 'KERN_SOH'
#define KERN_DEBUG KERN_SOH "7" /* debug-level messages */
^~~~~~~~
net/netlink/genetlink.c:983:11: note: in expansion of macro 'KERN_DEBUG'
printk(KERN_DEBUG "find by name '%s'\n", nla_data(tb[CTRL_ATTR_FAMILY_NAME]));
^~~~~~~~~~
net/netlink/genetlink.c:983:38: note: format string is defined here
printk(KERN_DEBUG "find by name '%s'\n", nla_data(tb[CTRL_ATTR_FAMILY_NAME]));
~^
%p
vim +5 include/linux/kern_levels.h
314ba352 Joe Perches 2012-07-30 4
04d2c8c8 Joe Perches 2012-07-30 @5 #define KERN_SOH "\001" /* ASCII Start Of Header */
04d2c8c8 Joe Perches 2012-07-30 6 #define KERN_SOH_ASCII '\001'
04d2c8c8 Joe Perches 2012-07-30 7
:::::: The code at line 5 was first introduced by commit
:::::: 04d2c8c83d0e3ac5f78aeede51babb3236200112 printk: convert the format for KERN_<LEVEL> to a 2 byte pattern
:::::: TO: Joe Perches <joe@perches.com>
:::::: CC: Linus Torvalds <torvalds@linux-foundation.org>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 56829 bytes --]
^ permalink raw reply
* Re: [PATCH 3/5] mt76: fix a leaked reference by adding a missing of_node_put
From: Markus Elfring @ 2019-04-05 13:15 UTC (permalink / raw)
To: Wen Yang, linux-arm-kernel, linux-mediatek, linux-wireless,
netdev
Cc: UNGLinuxDriver, Alexandre Belloni, Andrew Lunn, Anirudha Sarangi,
David S. Miller, Florian Fainelli, Kalle Valo, Linus Walleij,
Lorenzo Bianconi, Matthias Brugger, Michal Simek, Felix Fietkau,
vivien.didelot, John Linn, Yi Wang, linux-kernel
In-Reply-To: <1550819742-32155-3-git-send-email-wen.yang99@zte.com.cn>
> @@ -54,22 +54,30 @@ mt76_get_of_eeprom(struct mt76_dev *dev, int len)
> part = np->name;
>
> mtd = get_mtd_device_nm(part);
…
> + if (retlen < len) {
> + ret = -EINVAL;
> + goto out_put_node;
I find a jump to an immediately following source code place unnecessary.
Would you like to delete it?
> + }
>
> - return 0;
> +out_put_node:
> + of_node_put(np);
> + return ret;
> #else
Can another bit of fine-tuning matter here?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/net/wireless/mediatek/mt76/eeprom.c?id=34e022d8b780a03902d82fb3997ba7c7b1f40c81#n73
Regards,
Markus
^ permalink raw reply
* Re: [PATCH v6 12/20] kvm/vmx: Emulate MSR TEST_CTL
From: Thomas Gleixner @ 2019-04-05 12:30 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Dave Hansen,
Paolo Bonzini, Ashok Raj, Peter Zijlstra, Kalle Valo, Xiaoyao Li,
Michael Chan, Ravi V Shankar, linux-kernel, x86, linux-wireless,
netdev, kvm, Xiaoyao Li
In-Reply-To: <1554326526-172295-13-git-send-email-fenghua.yu@intel.com>
On Wed, 3 Apr 2019, Fenghua Yu wrote:
> @@ -1663,6 +1663,12 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> u32 index;
>
> switch (msr_info->index) {
> + case MSR_TEST_CTL:
> + if (!msr_info->host_initiated &&
> + !(vcpu->arch.core_capability & CORE_CAP_SPLIT_LOCK_DETECT))
Errm? If the MSR_TEST_CTL is exposed to the guest via CPUID then the
rdmsr() in the guest is not supposed to fail just because
CORE_CAP_SPLIT_LOCK_DETECT is not set.
vmx->msr_test_ctl holds the proper value, which is either 0 or
CORE_CAP_SPLIT_LOCK_DETECT until more bits are supported.
So the chek needs to be guest_cpuid_has(X86_FEATURE_CORE_CAPABILITY).
> + return 1;
> + msr_info->data = vmx->msr_test_ctl;
> + break;
> #ifdef CONFIG_X86_64
> case MSR_FS_BASE:
> msr_info->data = vmcs_readl(GUEST_FS_BASE);
> @@ -1797,6 +1803,14 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> u32 index;
>
> switch (msr_index) {
> + case MSR_TEST_CTL:
> + if (!(vcpu->arch.core_capability & CORE_CAP_SPLIT_LOCK_DETECT))
> + return 1;
Again, this wants to be a check for the availability of the MSR in the
guest cpuid and not to the CORE_CAP_SPLIT_LOCK_DETECT magic bit.
> +
> + if (data & ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT)
> + return 1;
and this one wants to be:
if (data & vmx->msr_test_ctl_mask)
so additional bits can be enabled later on at exactly one point in the
code, i.e. during vm init.
> + vmx->msr_test_ctl = data;
> + break;
>
> +static void atomic_switch_msr_test_ctl(struct vcpu_vmx *vmx)
> +{
> + u64 host_msr_test_ctl;
> +
> + /* if TEST_CTL MSR doesn't exist on the hardware, do nothing */
> + if (rdmsrl_safe(MSR_TEST_CTL, &host_msr_test_ctl))
> + return;
Yikes. So on hosts which do not have MSR_TEST_CTL this takes a fault on
every VM enter. The host knows whether it has this MSR or not.
Even if it exists there is no point to do the rdmsr on every vmenter. The
value should be cached per cpu. It only changes when:
1) #AC triggers in kernel space
2) The sysfs knob is flipped
#1 It happens either _BEFORE_ or _AFTER_ atomic_switch_msr_test_ctl(). In
both cases the cached value is correct in atomic_switch_msr_test_ctl().
If it happens _AFTER_ atomic_switch_msr_test_ctl() then the VMEXIT will
restore the enabled bit, but there is nothing you can do about that.
#2 CANNOT happen in the context of vcpu_run().
So this wants to be:
host_msr_test_ctl = this_cpu_read(msr_test_ctl_cache);
> + if (host_msr_test_ctl == vmx->msr_test_ctl)
> + clear_atomic_switch_msr(vmx, MSR_TEST_CTL);
> + else
> + add_atomic_switch_msr(vmx, MSR_TEST_CTL, vmx->msr_test_ctl,
> + host_msr_test_ctl, false);
> +}
> +
> u64 kvm_get_core_capability(void)
> {
> - return 0;
> + u64 data;
> +
> + rdmsrl_safe(MSR_IA32_CORE_CAPABILITY, &data);
if (!boot_cpu_has(X86_FEATURE_CORE_CAPABILITY))
return 0;
> + /* mask non-virtualizable functions */
> + data &= CORE_CAP_SPLIT_LOCK_DETECT;
Why?
> + /*
> + * There will be a list of FMS values that have split lock detection
> + * but lack the CORE CAPABILITY MSR. In this case, set
> + * CORE_CAP_SPLIT_LOCK_DETECT since we emulate MSR CORE_CAPABILITY.
That's completely wrong. X86_FEATURE_SPLIT_LOCK_DETECT is set when the
capability is enumerated in CPUID or it's set via the FMS quirk.
So:
data = 0;
> + if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> + data |= CORE_CAP_SPLIT_LOCK_DETECT;
> +
> + return data;
is absolutely sufficient.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v6 11/20] kvm/x86: Emulate MSR IA32_CORE_CAPABILITY
From: Thomas Gleixner @ 2019-04-05 12:00 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Dave Hansen,
Paolo Bonzini, Ashok Raj, Peter Zijlstra, Kalle Valo, Xiaoyao Li,
Michael Chan, Ravi V Shankar, linux-kernel, x86, linux-wireless,
netdev, kvm, Xiaoyao Li
In-Reply-To: <1554326526-172295-12-git-send-email-fenghua.yu@intel.com>
On Wed, 3 Apr 2019, Fenghua Yu wrote:
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>
> +u64 kvm_get_core_capability(void)
> +{
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(kvm_get_core_capability);
Why is this global and exported? The only users are in this very file.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 0/6] stricter netlink validation
From: Johannes Berg @ 2019-04-05 11:47 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, pablo, dsa
In-Reply-To: <20190404.102850.631122626573856353.davem@davemloft.net>
On Thu, 2019-04-04 at 10:28 -0700, David Miller wrote:
> > Here's a version that has passed build testing ;-)
Umm, I sent out the wrong branch!
(Didn't even realize I had two ... oops)
The generic netlink bits are completely broken here, I was passing a
stack pointer to dump control, which obviously doesn't work
I've pushed out the right version to mac80211-next netlink-validation
branch as David Ahern requested I don't resend for now.
I've also pushed some very much WIP code to the netlink-policy-export
branch there that exposes the policies to userspace, there at least for
generic netlink now.
johannes
^ permalink raw reply
* [PATCH 5.1] mt76x02: avoid status_list.lock and sta->rate_ctrl_lock dependency
From: Stanislaw Gruszka @ 2019-04-05 11:42 UTC (permalink / raw)
To: linux-wireless; +Cc: Felix Fietkau, Lorenzo Bianconi
Move ieee80211_tx_status_ext() outside of status_list lock section
in order to avoid locking dependency and possible deadlock reposed by
LOCKDEP in below warning.
Also do mt76_tx_status_lock() just before it's needed.
[ 440.224832] WARNING: possible circular locking dependency detected
[ 440.224833] 5.1.0-rc2+ #22 Not tainted
[ 440.224834] ------------------------------------------------------
[ 440.224835] kworker/u16:28/2362 is trying to acquire lock:
[ 440.224836] 0000000089b8cacf (&(&q->lock)->rlock#2){+.-.}, at: mt76_wake_tx_queue+0x4c/0xb0 [mt76]
[ 440.224842]
but task is already holding lock:
[ 440.224842] 000000002cfedc59 (&(&sta->lock)->rlock){+.-.}, at: ieee80211_stop_tx_ba_cb+0x32/0x1f0 [mac80211]
[ 440.224863]
which lock already depends on the new lock.
[ 440.224863]
the existing dependency chain (in reverse order) is:
[ 440.224864]
-> #3 (&(&sta->lock)->rlock){+.-.}:
[ 440.224869] _raw_spin_lock_bh+0x34/0x40
[ 440.224880] ieee80211_start_tx_ba_session+0xe4/0x3d0 [mac80211]
[ 440.224894] minstrel_ht_get_rate+0x45c/0x510 [mac80211]
[ 440.224906] rate_control_get_rate+0xc1/0x140 [mac80211]
[ 440.224918] ieee80211_tx_h_rate_ctrl+0x195/0x3c0 [mac80211]
[ 440.224930] ieee80211_xmit_fast+0x26d/0xa50 [mac80211]
[ 440.224942] __ieee80211_subif_start_xmit+0xfc/0x310 [mac80211]
[ 440.224954] ieee80211_subif_start_xmit+0x38/0x390 [mac80211]
[ 440.224956] dev_hard_start_xmit+0xb8/0x300
[ 440.224957] __dev_queue_xmit+0x7d4/0xbb0
[ 440.224968] ip6_finish_output2+0x246/0x860 [ipv6]
[ 440.224978] mld_sendpack+0x1bd/0x360 [ipv6]
[ 440.224987] mld_ifc_timer_expire+0x1a4/0x2f0 [ipv6]
[ 440.224989] call_timer_fn+0x89/0x2a0
[ 440.224990] run_timer_softirq+0x1bd/0x4d0
[ 440.224992] __do_softirq+0xdb/0x47c
[ 440.224994] irq_exit+0xfa/0x100
[ 440.224996] smp_apic_timer_interrupt+0x9a/0x220
[ 440.224997] apic_timer_interrupt+0xf/0x20
[ 440.224999] cpuidle_enter_state+0xc1/0x470
[ 440.225000] do_idle+0x21a/0x260
[ 440.225001] cpu_startup_entry+0x19/0x20
[ 440.225004] start_secondary+0x135/0x170
[ 440.225006] secondary_startup_64+0xa4/0xb0
[ 440.225007]
-> #2 (&(&sta->rate_ctrl_lock)->rlock){+.-.}:
[ 440.225009] _raw_spin_lock_bh+0x34/0x40
[ 440.225022] rate_control_tx_status+0x4f/0xb0 [mac80211]
[ 440.225031] ieee80211_tx_status_ext+0x142/0x1a0 [mac80211]
[ 440.225035] mt76x02_send_tx_status+0x2e4/0x340 [mt76x02_lib]
[ 440.225037] mt76x02_tx_status_data+0x31/0x40 [mt76x02_lib]
[ 440.225040] mt76u_tx_status_data+0x51/0xa0 [mt76_usb]
[ 440.225042] process_one_work+0x237/0x5d0
[ 440.225043] worker_thread+0x3c/0x390
[ 440.225045] kthread+0x11d/0x140
[ 440.225046] ret_from_fork+0x3a/0x50
[ 440.225047]
-> #1 (&(&list->lock)->rlock#8){+.-.}:
[ 440.225049] _raw_spin_lock_bh+0x34/0x40
[ 440.225052] mt76_tx_status_skb_add+0x51/0x100 [mt76]
[ 440.225054] mt76x02u_tx_prepare_skb+0xbd/0x116 [mt76x02_usb]
[ 440.225056] mt76u_tx_queue_skb+0x5f/0x180 [mt76_usb]
[ 440.225058] mt76_tx+0x93/0x190 [mt76]
[ 440.225070] ieee80211_tx_frags+0x148/0x210 [mac80211]
[ 440.225081] __ieee80211_tx+0x75/0x1b0 [mac80211]
[ 440.225092] ieee80211_tx+0xde/0x110 [mac80211]
[ 440.225105] __ieee80211_tx_skb_tid_band+0x72/0x90 [mac80211]
[ 440.225122] ieee80211_send_auth+0x1f3/0x360 [mac80211]
[ 440.225141] ieee80211_auth.cold.40+0x6c/0x100 [mac80211]
[ 440.225156] ieee80211_mgd_auth.cold.50+0x132/0x15f [mac80211]
[ 440.225171] cfg80211_mlme_auth+0x149/0x360 [cfg80211]
[ 440.225181] nl80211_authenticate+0x273/0x2e0 [cfg80211]
[ 440.225183] genl_family_rcv_msg+0x196/0x3a0
[ 440.225184] genl_rcv_msg+0x47/0x8e
[ 440.225185] netlink_rcv_skb+0x3a/0xf0
[ 440.225187] genl_rcv+0x24/0x40
[ 440.225188] netlink_unicast+0x16d/0x210
[ 440.225189] netlink_sendmsg+0x204/0x3b0
[ 440.225191] sock_sendmsg+0x36/0x40
[ 440.225193] ___sys_sendmsg+0x259/0x2b0
[ 440.225194] __sys_sendmsg+0x47/0x80
[ 440.225196] do_syscall_64+0x60/0x1f0
[ 440.225197] entry_SYSCALL_64_after_hwframe+0x49/0xbe
[ 440.225198]
-> #0 (&(&q->lock)->rlock#2){+.-.}:
[ 440.225200] lock_acquire+0xb9/0x1a0
[ 440.225202] _raw_spin_lock_bh+0x34/0x40
[ 440.225204] mt76_wake_tx_queue+0x4c/0xb0 [mt76]
[ 440.225215] ieee80211_agg_start_txq+0xe8/0x2b0 [mac80211]
[ 440.225225] ieee80211_stop_tx_ba_cb+0xb8/0x1f0 [mac80211]
[ 440.225235] ieee80211_ba_session_work+0x1c1/0x2f0 [mac80211]
[ 440.225236] process_one_work+0x237/0x5d0
[ 440.225237] worker_thread+0x3c/0x390
[ 440.225239] kthread+0x11d/0x140
[ 440.225240] ret_from_fork+0x3a/0x50
[ 440.225240]
other info that might help us debug this:
[ 440.225241] Chain exists of:
&(&q->lock)->rlock#2 --> &(&sta->rate_ctrl_lock)->rlock --> &(&sta->lock)->rlock
[ 440.225243] Possible unsafe locking scenario:
[ 440.225244] CPU0 CPU1
[ 440.225244] ---- ----
[ 440.225245] lock(&(&sta->lock)->rlock);
[ 440.225245] lock(&(&sta->rate_ctrl_lock)->rlock);
[ 440.225246] lock(&(&sta->lock)->rlock);
[ 440.225247] lock(&(&q->lock)->rlock#2);
[ 440.225248]
*** DEADLOCK ***
[ 440.225249] 5 locks held by kworker/u16:28/2362:
[ 440.225250] #0: 0000000048fcd291 ((wq_completion)phy0){+.+.}, at: process_one_work+0x1b5/0x5d0
[ 440.225252] #1: 00000000f1c6828f ((work_completion)(&sta->ampdu_mlme.work)){+.+.}, at: process_one_work+0x1b5/0x5d0
[ 440.225254] #2: 00000000433d2b2c (&sta->ampdu_mlme.mtx){+.+.}, at: ieee80211_ba_session_work+0x5c/0x2f0 [mac80211]
[ 440.225265] #3: 000000002cfedc59 (&(&sta->lock)->rlock){+.-.}, at: ieee80211_stop_tx_ba_cb+0x32/0x1f0 [mac80211]
[ 440.225276] #4: 000000009d7b9a44 (rcu_read_lock){....}, at: ieee80211_agg_start_txq+0x33/0x2b0 [mac80211]
[ 440.225286]
stack backtrace:
[ 440.225288] CPU: 2 PID: 2362 Comm: kworker/u16:28 Not tainted 5.1.0-rc2+ #22
[ 440.225289] Hardware name: LENOVO 20KGS23S0P/20KGS23S0P, BIOS N23ET55W (1.30 ) 08/31/2018
[ 440.225300] Workqueue: phy0 ieee80211_ba_session_work [mac80211]
[ 440.225301] Call Trace:
[ 440.225304] dump_stack+0x85/0xc0
[ 440.225306] print_circular_bug.isra.38.cold.58+0x15c/0x195
[ 440.225307] check_prev_add.constprop.48+0x5f0/0xc00
[ 440.225309] ? check_prev_add.constprop.48+0x39d/0xc00
[ 440.225311] ? __lock_acquire+0x41d/0x1100
[ 440.225312] __lock_acquire+0xd98/0x1100
[ 440.225313] ? __lock_acquire+0x41d/0x1100
[ 440.225315] lock_acquire+0xb9/0x1a0
[ 440.225317] ? mt76_wake_tx_queue+0x4c/0xb0 [mt76]
[ 440.225319] _raw_spin_lock_bh+0x34/0x40
[ 440.225321] ? mt76_wake_tx_queue+0x4c/0xb0 [mt76]
[ 440.225323] mt76_wake_tx_queue+0x4c/0xb0 [mt76]
[ 440.225334] ieee80211_agg_start_txq+0xe8/0x2b0 [mac80211]
[ 440.225344] ieee80211_stop_tx_ba_cb+0xb8/0x1f0 [mac80211]
[ 440.225354] ieee80211_ba_session_work+0x1c1/0x2f0 [mac80211]
[ 440.225356] process_one_work+0x237/0x5d0
[ 440.225358] worker_thread+0x3c/0x390
[ 440.225359] ? wq_calc_node_cpumask+0x70/0x70
[ 440.225360] kthread+0x11d/0x140
[ 440.225362] ? kthread_create_on_node+0x40/0x40
[ 440.225363] ret_from_fork+0x3a/0x50
Cc: stable@vger.kernel.org
Fixes: 88046b2c9f6d ("mt76: add support for reporting tx status with skb")
Signed-off-by: Stanislaw Gruszka <sgruszka@redhat.com>
---
drivers/net/wireless/mediatek/mt76/mt76x02_mac.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
index 1eb669ccd5a7..4a77d509a86b 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
@@ -466,7 +466,6 @@ void mt76x02_send_tx_status(struct mt76x02_dev *dev,
return;
rcu_read_lock();
- mt76_tx_status_lock(mdev, &list);
if (stat->wcid < ARRAY_SIZE(dev->mt76.wcid))
wcid = rcu_dereference(dev->mt76.wcid[stat->wcid]);
@@ -479,6 +478,8 @@ void mt76x02_send_tx_status(struct mt76x02_dev *dev,
drv_priv);
}
+ mt76_tx_status_lock(mdev, &list);
+
if (wcid) {
if (stat->pktid >= MT_PACKET_ID_FIRST)
status.skb = mt76_tx_status_skb_get(mdev, wcid,
@@ -498,7 +499,9 @@ void mt76x02_send_tx_status(struct mt76x02_dev *dev,
if (*update == 0 && stat_val == stat_cache &&
stat->wcid == msta->status.wcid && msta->n_frames < 32) {
msta->n_frames++;
- goto out;
+ mt76_tx_status_unlock(mdev, &list);
+ rcu_read_unlock();
+ return;
}
mt76x02_mac_fill_tx_status(dev, status.info, &msta->status,
@@ -514,11 +517,10 @@ void mt76x02_send_tx_status(struct mt76x02_dev *dev,
if (status.skb)
mt76_tx_status_skb_done(mdev, status.skb, &list);
- else
- ieee80211_tx_status_ext(mt76_hw(dev), &status);
-
-out:
mt76_tx_status_unlock(mdev, &list);
+
+ if (!status.skb)
+ ieee80211_tx_status_ext(mt76_hw(dev), &status);
rcu_read_unlock();
}
--
2.20.1
^ permalink raw reply related
* [PATCH] ath10k: Don't log a traceback on invalid event IDs.
From: Peter Jones @ 2019-04-05 10:05 UTC (permalink / raw)
To: Kalle Valo
Cc: David S. Miller, ath10k, linux-wireless, netdev, linux-kernel,
Peter Jones
Very frequently upon resume from suspend, we see tracebacks like:
ath10k_pci 0000:02:00.0: unsupported HTC service id: 1536
ath10k_pci 0000:02:00.0: Unknown eventid: 118809
WARNING: CPU: 1 PID: 23135 at drivers/net/wireless/ath/ath10k/mac.c:5650 ath10k_bss_info_changed+0xe8e/0xfd0 [ath10k_core]
Modules linked in: ...
CPU: 1 PID: 23135 Comm: kworker/u16:54 Not tainted 4.20.15-200.fc29.x86_64 #1
Hardware name: ...
Workqueue: events_unbound async_run_entry_fn
RIP: 0010:ath10k_bss_info_changed+0xe8e/0xfd0 [ath10k_core]
Code: 89 c1 89 f2 4c 89 d7 48 c7 c6 60 7e ed c0 4c 89 4c 24 10 89 44 24 08 e8 e0 35 00 00 8b 44 24 08 4c 8b 4c 24 10 e9 22 f8 ff ff <0f> 0b e9 7a f4 ff ff 8b b3 70 01 00 00 89 f2 89 c1 48 c7 c6 20 7e
RSP: 0018:ffffaba5456b7cc0 EFLAGS: 00010246
RAX: 0000000000000000 RBX: ffff8d9b612a33a0 RCX: 0000000000000000
RDX: 0000000000000001 RSI: 00000000fffffe01 RDI: ffffffffc0cca4ae
RBP: 00000000020674ff R08: 0000000000200000 R09: 0000000000000000
R10: ffff8d9a6d490700 R11: 0000000039a6c800 R12: ffff8d9b612a33a8
R13: ffff8d9b4bd31580 R14: ffff8d9b4bd31580 R15: ffff8d9b612a3500
FS: 0000000000000000(0000) GS:ffff8d9b66440000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000556cca653b66 CR3: 000000027920a001 CR4: 00000000003606e0
Call Trace:
? ath10k_conf_tx+0x7a/0x4b0 [ath10k_core]
ieee80211_bss_info_change_notify+0xab/0x1a0 [mac80211]
ieee80211_reconfig+0x66c/0x1300 [mac80211]
? __switch_to_asm+0x34/0x70
? __switch_to_asm+0x34/0x70
? __switch_to_asm+0x40/0x70
wiphy_resume+0x7f/0x120 [cfg80211]
? index_show+0x20/0x20 [cfg80211]
dpm_run_callback+0x59/0x150
device_resume+0xac/0x1f0
async_resume+0x19/0x30
async_run_entry_fn+0x39/0x160
process_one_work+0x1a1/0x3a0
worker_thread+0x30/0x380
? process_one_work+0x3a0/0x3a0
kthread+0x112/0x130
? kthread_create_on_node+0x60/0x60
ret_from_fork+0x35/0x40
---[ end trace 0afe382792dba858 ]---
These errors are completely transient, and the card goes back to working
perfectly fine afterwards. I suspect the driver is just reading an
uninitialized buffer as the card comes back up.
In any case, there's no point in treating this as a major problem and
logging a full register dump and a traceback; just logginng an INFO
message saying that it happened is enough. This patch does that.
Signed-off-by: Peter Jones <pjones@redhat.com>
---
drivers/net/wireless/ath/ath10k/htc.c | 4 ++--
drivers/net/wireless/ath/ath10k/sdio.c | 2 +-
drivers/net/wireless/ath/ath10k/wmi-tlv.c | 2 +-
drivers/net/wireless/ath/ath10k/wmi.c | 8 ++++----
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/htc.c b/drivers/net/wireless/ath/ath10k/htc.c
index 805a7f8a04f..c244889b72e 100644
--- a/drivers/net/wireless/ath/ath10k/htc.c
+++ b/drivers/net/wireless/ath/ath10k/htc.c
@@ -799,7 +799,7 @@ int ath10k_htc_connect_service(struct ath10k_htc *htc,
&ep->ul_pipe_id,
&ep->dl_pipe_id);
if (status) {
- ath10k_warn(ar, "unsupported HTC service id: %d\n",
+ ath10k_info(ar, "unsupported HTC service id: %d\n",
ep->service_id);
return status;
}
@@ -877,7 +877,7 @@ static bool ath10k_htc_pktlog_svc_supported(struct ath10k *ar)
&ul_pipe_id,
&dl_pipe_id);
if (status) {
- ath10k_warn(ar, "unsupported HTC service id: %d\n",
+ ath10k_info(ar, "unsupported HTC service id: %d\n",
ATH10K_HTC_SVC_ID_HTT_LOG_MSG);
return false;
diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index fae56c67766..a265225216e 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -1871,7 +1871,7 @@ static int ath10k_sdio_hif_map_service_to_pipe(struct ath10k *ar,
ar_sdio->mbox_addr[eid], ar_sdio->mbox_size[eid]);
break;
default:
- ath10k_warn(ar, "unsupported HTC service id: %d\n",
+ ath10k_info(ar, "unsupported HTC service id: %d\n",
service_id);
return -EINVAL;
}
diff --git a/drivers/net/wireless/ath/ath10k/wmi-tlv.c b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
index 582fb11f648..ca990c8d306 100644
--- a/drivers/net/wireless/ath/ath10k/wmi-tlv.c
+++ b/drivers/net/wireless/ath/ath10k/wmi-tlv.c
@@ -614,7 +614,7 @@ static void ath10k_wmi_tlv_op_rx(struct ath10k *ar, struct sk_buff *skb)
ath10k_wmi_event_mgmt_tx_bundle_compl(ar, skb);
break;
default:
- ath10k_dbg(ar, ATH10K_DBG_WMI, "Unknown eventid: %d\n", id);
+ ath10k_info(ar, ATH10K_DBG_WMI, "Unknown eventid: %d\n", id);
break;
}
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 98a90e49d66..f4fa406d9fe 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -5850,7 +5850,7 @@ static void ath10k_wmi_op_rx(struct ath10k *ar, struct sk_buff *skb)
ath10k_wmi_event_service_available(ar, skb);
break;
default:
- ath10k_warn(ar, "Unknown eventid: %d\n", id);
+ ath10k_info(ar, "Unknown eventid: %d\n", id);
break;
}
@@ -5981,7 +5981,7 @@ static void ath10k_wmi_10_1_op_rx(struct ath10k *ar, struct sk_buff *skb)
/* ignore utf events */
break;
default:
- ath10k_warn(ar, "Unknown eventid: %d\n", id);
+ ath10k_info(ar, "Unknown eventid: %d\n", id);
break;
}
@@ -6130,7 +6130,7 @@ static void ath10k_wmi_10_2_op_rx(struct ath10k *ar, struct sk_buff *skb)
ath10k_wmi_event_peer_sta_ps_state_chg(ar, skb);
break;
default:
- ath10k_warn(ar, "Unknown eventid: %d\n", id);
+ ath10k_info(ar, "Unknown eventid: %d\n", id);
break;
}
@@ -6250,7 +6250,7 @@ static void ath10k_wmi_10_4_op_rx(struct ath10k *ar, struct sk_buff *skb)
ath10k_wmi_event_peer_sta_ps_state_chg(ar, skb);
break;
default:
- ath10k_warn(ar, "Unknown eventid: %d\n", id);
+ ath10k_info(ar, "Unknown eventid: %d\n", id);
break;
}
--
2.20.1
^ permalink raw reply related
* RE: [PATCH v6 04/20] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-05 9:23 UTC (permalink / raw)
To: 'Thomas Gleixner'
Cc: 'Fenghua Yu', 'Ingo Molnar',
'Borislav Petkov', 'H Peter Anvin',
'Dave Hansen', 'Paolo Bonzini',
'Ashok Raj', 'Peter Zijlstra',
'Kalle Valo', 'Xiaoyao Li ',
'Michael Chan', 'Ravi V Shankar',
'linux-kernel', 'x86',
'linux-wireless@vger.kernel.org',
'netdev@vger.kernel.org', 'kvm@vger.kernel.org'
In-Reply-To: <alpine.DEB.2.21.1904041833480.1802@nanos.tec.linutronix.de>
From: Thomas Gleixner [mailto:tglx@linutronix.de]
>
> On Thu, 4 Apr 2019, David Laight wrote:
...
> bitops operate on unsigned long arrays, so the RMW on the affected array
> member has to be atomic vs. other RMW operations on the same array
> member. If we make the bitops 32bit wide on x86/64 we break that.
>
> So selecting 64bit access (REX.W prefix) is correct and has to stay.
Even if the lock had a fine granularity it shouldn't matter whether
two RMW operations on different parts of the same 'long' happened
at the same time.
ISTR some of the 'constant bit offset' are done with byte sized
locked transfers.
The problem here is that the bitmap is defined as __u32[] not
unsigned long[].
- __u32 x86_capability[NCAPINTS + NBUGINTS];
+ /* Unsigned long alignment to avoid split lock in atomic bitmap ops */
+ __u32 x86_capability[NCAPINTS + NBUGINTS]
+ __aligned(sizeof(unsigned long));
Somewhere there is a cast.....
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH 0/6] stricter netlink validation
From: Johannes Berg @ 2019-04-05 7:09 UTC (permalink / raw)
To: David Ahern, David Miller; +Cc: linux-wireless, netdev, pablo
In-Reply-To: <db599b6b-94a5-3123-5613-9e349fcee702@cumulusnetworks.com>
On Thu, 2019-04-04 at 20:44 -0600, David Ahern wrote:
> Hit a compile issue right out of the gate:
>
> $ make O=kbuild/perf -j 24 -s
> /home/dsa/kernel-2.git/net/openvswitch/flow_netlink.c: In function
> ‘validate_and_copy_check_pkt_len’:
> /home/dsa/kernel-2.git/net/openvswitch/flow_netlink.c:2887:8: error:
> implicit declaration of function ‘nla_parse_deprecated_strict’
> [-Werror=implicit-function-declaration]
> err = nla_parse_deprecated_strict(a, OVS_CHECK_PKT_LEN_ATTR_MAX,
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> You should do an allmodconfig build to check for any others. I disabled
> ovs to continue.
Ugh, yes. The one change I made because I rebased on the latest net-next
after the build testing ...
Sorry about that, I'll fix it in v2 after more reviews.
johannes
^ permalink raw reply
* Re: [PATCH] ath6kl: remove redundant check of status != 0
From: Mukesh Ojha @ 2019-04-05 6:50 UTC (permalink / raw)
To: Colin King, Kalle Valo, David S . Miller, linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
In-Reply-To: <20190404134723.24667-1-colin.king@canonical.com>
On 4/4/2019 7:17 PM, Colin King wrote:
> From: Colin Ian King <colin.king@canonical.com>
>
> The check on status not being zero is redundant as previous code
> paths that set status to an error value break out of the while
> loop and hence status is never non-zero at the check. Remove
> this redundant code.
>
> Addresses-Coverity: ("Logically dead code")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Mukesh Ojha <mojha@codeaurora.org>
Cheers,
-Mukesh
> ---
> drivers/net/wireless/ath/ath6kl/htc_pipe.c | 3 ---
> 1 file changed, 3 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/htc_pipe.c b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> index 434b66829646..c68848819a52 100644
> --- a/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> +++ b/drivers/net/wireless/ath/ath6kl/htc_pipe.c
> @@ -898,9 +898,6 @@ static int htc_process_trailer(struct htc_target *target, u8 *buffer,
> break;
> }
>
> - if (status != 0)
> - break;
> -
> /* advance buffer past this record for next time around */
> buffer += record->len;
> len -= record->len;
^ permalink raw reply
* Re: [PATCH] mwifiex: Make resume actually do something useful again on SDIO cards
From: Kalle Valo @ 2019-04-05 3:35 UTC (permalink / raw)
To: Brian Norris
Cc: Douglas Anderson, open list:ARM/Rockchip SoC...,
Matthias Kaehlcke, Ryan Case, Heiko Stuebner, David S. Miller,
Ganapathi Bhat, linux-wireless, Amitkumar Karwar, Linux Kernel,
Nishant Sarmukadam, <netdev@vger.kernel.org>, Xinming Hu
In-Reply-To: <CA+ASDXNfOUmMEWYwts8PuiDaEjuoPK+tZ0BhDSSte5u5toydkA@mail.gmail.com>
Brian Norris <briannorris@chromium.org> writes:
> On Wed, Apr 3, 2019 at 10:20 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>> Douglas Anderson <dianders@chromium.org> writes:
>> > Fixes: fc3a2fcaa1ba ("mwifiex: use atomic bitops to represent
>> > adapter status variables")
>> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>
>> I'm planning to queue this for 5.1.
>
> With an explicit
>
> Cc: <stable@vger.kernel.org>
>
> please? Somebody would likely pick it up anyway, but it's nice to be
> explicit.
Ok, I'll add that.
> Not that it really matters, but also:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
This patchwork picks up automatically so I don't have to do anything.
But I so wish it would do the same for Cc and Fixes tags :)
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 0/6] stricter netlink validation
From: David Ahern @ 2019-04-05 2:44 UTC (permalink / raw)
To: David Miller, johannes; +Cc: linux-wireless, netdev, pablo
In-Reply-To: <20190404.102850.631122626573856353.davem@davemloft.net>
On 4/4/19 11:28 AM, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 4 Apr 2019 08:54:02 +0200
>
>> Here's a version that has passed build testing ;-)
>
> :-)
>
> I really like the approach taken here, and done in such a way that
> new attributes added get strict checking by default.
>
> I'll let David Ahern et al. have time to review this.
>
Hit a compile issue right out of the gate:
$ make O=kbuild/perf -j 24 -s
/home/dsa/kernel-2.git/net/openvswitch/flow_netlink.c: In function
‘validate_and_copy_check_pkt_len’:
/home/dsa/kernel-2.git/net/openvswitch/flow_netlink.c:2887:8: error:
implicit declaration of function ‘nla_parse_deprecated_strict’
[-Werror=implicit-function-declaration]
err = nla_parse_deprecated_strict(a, OVS_CHECK_PKT_LEN_ATTR_MAX,
^~~~~~~~~~~~~~~~~~~~~~~~~~~
You should do an allmodconfig build to check for any others. I disabled
ovs to continue.
^ permalink raw reply
* Re: [PATCH v6 10/20] x86/split_lock: Handle #AC exception for split lock
From: Fenghua Yu @ 2019-04-04 22:49 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Dave Hansen,
Paolo Bonzini, Ashok Raj, Peter Zijlstra, Kalle Valo, Xiaoyao Li,
Michael Chan, Ravi V Shankar, linux-kernel, x86, linux-wireless,
netdev, kvm
In-Reply-To: <alpine.DEB.2.21.1904041921460.1802@nanos.tec.linutronix.de>
On Thu, Apr 04, 2019 at 07:31:59PM +0200, Thomas Gleixner wrote:
> On Wed, 3 Apr 2019, Fenghua Yu wrote:
> > +dotraplinkage void do_alignment_check(struct pt_regs *regs, long error_code)
> > +{
> > + unsigned int trapnr = X86_TRAP_AC;
> > + char str[] = "alignment check";
> > + int signr = SIGBUS;
> > +
> > + RCU_LOCKDEP_WARN(!rcu_is_watching(), "entry code didn't wake RCU");
> > +
> > + /*
> > + * WARN*()s end up here; fix them up before we call the
> > + * notifier chain.
> > + */
>
> How exactly is WARN*() ending up here?
>
> > + if (!user_mode(regs) && fixup_bug(regs, trapnr))
>
> And that fixup_bug() check does what?
>
> int fixup_bug(struct pt_regs *regs, int trapnr)
> {
> if (trapnr != X86_TRAP_UD)
> return 0;
>
> Copy and paste from do_error_trap() ....
As you can see, do_alignment_check() is copied from do_error_trap().
But seems this part of code is irrelevant to #AC handler.
So I will remove the "if (!user_mode(regs) && fixup_bug(regs, trapnr))"
and surrounding code, right?
>
> > + return;
> > +
> > + if (notify_die(DIE_TRAP, str, regs, error_code, trapnr, signr) ==
> > + NOTIFY_STOP)
> > + return;
> > +
> > + cond_local_irq_enable(regs);
> > + if (!user_mode(regs) &&
> > + static_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT)) {
> > + /*
> > + * Only split lock can generate #AC from kernel at this point.
> > + * Warn and disable split lock detection on this CPU. The
> > + * faulting instruction will be executed without generating
> > + * another #AC fault. User needs to check the warning and
> > + * fix the split lock issue in the faulting instruction.
>
> "User needs to check the warning and fix the issue ..."
>
> I'm looking forward to all the fixes from Joe Users.
>
> Please remove that sentence. It's useless. Users report warnings if at all
> and the kernel developers who actually look at them surely don't need an
> advice like that.
Sure. Will do this.
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH] mwifiex: Make resume actually do something useful again on SDIO cards
From: Brian Norris @ 2019-04-04 20:47 UTC (permalink / raw)
To: Kalle Valo
Cc: Douglas Anderson, open list:ARM/Rockchip SoC...,
Matthias Kaehlcke, Ryan Case, Heiko Stuebner, David S. Miller,
Ganapathi Bhat, linux-wireless, Amitkumar Karwar, Linux Kernel,
Nishant Sarmukadam, <netdev@vger.kernel.org>, Xinming Hu
In-Reply-To: <874l7egxdj.fsf@codeaurora.org>
On Wed, Apr 3, 2019 at 10:20 PM Kalle Valo <kvalo@codeaurora.org> wrote:
> Douglas Anderson <dianders@chromium.org> writes:
> > Fixes: fc3a2fcaa1ba ("mwifiex: use atomic bitops to represent adapter status variables")
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> I'm planning to queue this for 5.1.
With an explicit
Cc: <stable@vger.kernel.org>
please? Somebody would likely pick it up anyway, but it's nice to be explicit.
Not that it really matters, but also:
Reviewed-by: Brian Norris <briannorris@chromium.org>
Thanks,
Brian
^ permalink raw reply
* Re: ath10k and min_gcd + adhoc issues in 4.20 kernel
From: Ben Greear @ 2019-04-04 20:45 UTC (permalink / raw)
To: linux-wireless@vger.kernel.org
In-Reply-To: <3dfff3d9-a070-f3a3-723c-d1442ec06bc5@candelatech.com>
Hello,
Any comments on this below?
Thanks,
Ben
On 1/15/19 3:29 PM, Ben Greear wrote:
> I just updated to 4.20 and ported forward my ath10k patches. Now the driver
> will not register with mac80211 because this warning hits in net/wireless/core.c:
>
> /*
> * This isn't well-defined right now. If you have an
> * IBSS interface, then its beacon interval may change
> * by joining other networks, and nothing prevents it
> * from doing that.
> * So technically we probably shouldn't even allow AP
> * and IBSS in the same interface, but it seems that
> * some drivers support that, possibly only with fixed
> * beacon intervals for IBSS.
> */
> if (WARN_ON(types & BIT(NL80211_IFTYPE_ADHOC) &&
> c->beacon_int_min_gcd)) {
> return -EINVAL;
> }
>
>
> It looks like this was triggered by:
>
> Commit 0c317a02ca982ca093e71bf07cb562265ba40032
> Author: Purushottam Kushwaha <pkushwah@qti.qualcomm.com>
> Date: Wed Oct 12 18:26:51 2016 +0530
>
> cfg80211: support virtual interfaces with different beacon intervals
>
> and
>
> commit 8ebee73b574ad3dd1f14d461f65ceaffbd637650
> Author: Anilkumar Kolli <akolli@codeaurora.org>
> Date: Wed Mar 28 12:19:40 2018 +0300
>
> ath10k: advertize beacon_int_min_gcd
>
>
> To be honest, I don't see why that check for beacon_int_min_gcd is in the registration
> logic. Having it be '1' means that the driver/nic/firmware is flexible on pretty much
> any combination of beacon interval, so why should that trigger the WARN_ON case?
>
> Thanks,
> Ben
>
--
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc http://www.candelatech.com
^ permalink raw reply
* Re: [PATCH 0/6] stricter netlink validation
From: Johannes Berg @ 2019-04-04 20:20 UTC (permalink / raw)
To: David Miller; +Cc: linux-wireless, netdev, pablo, dsa
In-Reply-To: <20190404.102850.631122626573856353.davem@davemloft.net>
On Thu, 2019-04-04 at 10:28 -0700, David Miller wrote:
> From: Johannes Berg <johannes@sipsolutions.net>
> Date: Thu, 4 Apr 2019 08:54:02 +0200
>
> > Here's a version that has passed build testing ;-)
>
> :-)
Actually it passed more than that - I did test the nl80211 bits etc.,
but I hadn't build-tested everything before so some missing function
renames were caught by the full build testing.
> I really like the approach taken here, and done in such a way that
> new attributes added get strict checking by default.
It's two things really
* new commands (aka new instances of nla_parse/nlmsg_parse and friends)
--> strict checking for everything, including existing attributes
because we reason that you're writing some new userspace code,
and even if that might use some existing functionality, which
might even be wrong, you're going to fix it here
* new attributes on existing commands (in the policy)
--> can be set up (with the strict_start_type from patch 4) to be
strictly checked
> I'll let David Ahern et al. have time to review this.
Sure.
FWIW, I wasn't really entirely sure I liked doing a cross-tree rename,
but ultimately I felt that we should discourage uses of what I now
called *_deprecated() and *_strict_deprecated() APIs, and having sort of
the "default" names do the thing we believe is right (strict checking)
helps with that - in a sort of 'social engineering' way, people will not
want to type out "_deprecated" all the time ;-)
I do realize that this may be a bit controversial and am certainly open
to other suggestions on this.
Similarly, I engineered the generic netlink stuff in a way that adding
non-strict behaviour needs extra work, so that hopefully new stuff will
not do that extra work.
Also, both of these are then easier to see in reviews, since you can see
"deprecated" in the function names, or "DONT_VALIDATE" in the generic
netlink things.
johannes
^ permalink raw reply
* Re: [PATCH v6 13/20] x86/split_lock: Enable split lock detection by default
From: Thomas Gleixner @ 2019-04-04 19:44 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Dave Hansen,
Paolo Bonzini, Ashok Raj, Peter Zijlstra, Kalle Valo, Xiaoyao Li,
Michael Chan, Ravi V Shankar, linux-kernel, x86, linux-wireless,
netdev, kvm
In-Reply-To: <20190404192306.GA178488@romley-ivt3.sc.intel.com>
On Thu, 4 Apr 2019, Fenghua Yu wrote:
> On Thu, Apr 04, 2019 at 08:07:57PM +0200, Thomas Gleixner wrote:
> > On Wed, 3 Apr 2019, Fenghua Yu wrote:
> > > +static DEFINE_MUTEX(split_lock_detect_mutex);
> > > +static int split_lock_detect_val;
> >
> > detect_val? What value is that?
>
> According to previous discussions, I was told to call this split lock feature
> as "split lock detection" instead of "#AC for split lock". So I use
> "split_lock_detect..." in variable names or function names, call feature flag
> as "split_lock_detect", and call the feature as "split lock detection" in
> descriptions.
>
> If you don't agree to name feature as "split lock detection", I can change
> variable names/function names/feature flag/descriptions etc back to previous
> names "ac_split_lock...", "#AC for split lock", etc.
>
> The variable split_lock_detect_val is either 0 or 1. It stores current
> enable/disable status of split lock detection feature. By default it's
> one after the feature is enumerated. Then sysadmin can change it to 0 or 1
> to enable or disable the feature during run time.
> > static unsigned int ac_split_lock_enable;
>
> If you agree to name the split lock feature as "split lock detection" feature,
> can I change this variable to static unsigned int split_lock_detect_enable?
I don't care much whether it's ac_split_lock or split_lock_detect, but _val
is a completely bogus and unintuitive name. The variable tells whether the
functionality is enabled or not. Then do not name it $prefix_val, which can
mean anything. Name it $prefix_enable, which makes it entirely clear what
this is about.
And please make it type bool so you don't need any of these defines either.
> > > +static u32 new_sp_test_ctl_val(u32 test_ctl_val)
> > > +{
> > > + /* Change the split lock setting. */
> > > + if (READ_ONCE(split_lock_detect_val) == DISABLE_SPLIT_LOCK_DETECT)
> >
> > That READ_ONCE() is required because?
>
> Ok. Will remove READ_ONCE().
>
> >
> > > + test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> > > + else
> > > + test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> > > +
> > > + return test_ctl_val;
> > > +}
> >
> > Aside of that do we really need a misnomed function which replaces the
> > simple inline code at the call site:
> >
> > rdmsr(l, h)
> > l &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> > l |= ac_split_lock_enable << TEST_CTL_ENABLE_SPLIT_LOCK_DETECT_SHIFT;
> > wrmrs(...)
> >
> > or the even more simple
> >
> > if (ac_split_lock_enable)
> > msr_set_bit(...)
> > else
> > msr_clear_nit(...)
> >
> > Hmm?
>
> The function new_sp_test_ctrl_val() will be called twice: here when
> initializing split lock detection and in split_lock_detect_store()
> when enabling/disabling the feature through the sysfs interface in
> patch 0014.
It's still pointless.
> So can I still keep this function and name it as get_new_test_ctrl_val()?
No. The function you want to share between init code and sysfs is
split_lock_update_msr()
{
if (split_lock_enable)
msr_set_bit(...)
else
msr_clear_nit(...)
}
That's all. No duplicated code. No convoluted helper function,
nothing. Simple straight forward readable code.
> > > +static inline void show_split_lock_detection_info(void)
> > > +{
> > > + if (READ_ONCE(split_lock_detect_val))
> >
> > That READ_ONCE() is required because?
>
> Ok. Will remove READ_ONCE().
>
> >
> > > + pr_info_once("x86/split_lock: split lock detection enabled\n");
> > > + else
> > > + pr_info_once("x86/split_lock: split lock detection disabled\n");
> >
> > pr_fmt exists for a reason and having 'split lock' repeated several times
> > in the same line is not making it more readable.
>
> Ok. I will change the string to "x86/split_lock_detection: enabled\n",
> is it ok?
Care to read carefully what I wrote? Hint: pr_fmt
> > Oh well. You add defines on top of the file and then you don't use them.
>
> Will fix this.
What about the init / feature detection sequence which you snipped from the
reply?
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v6 13/20] x86/split_lock: Enable split lock detection by default
From: Fenghua Yu @ 2019-04-04 19:23 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Dave Hansen,
Paolo Bonzini, Ashok Raj, Peter Zijlstra, Kalle Valo, Xiaoyao Li,
Michael Chan, Ravi V Shankar, linux-kernel, x86, linux-wireless,
netdev, kvm
In-Reply-To: <alpine.DEB.2.21.1904041932500.1802@nanos.tec.linutronix.de>
On Thu, Apr 04, 2019 at 08:07:57PM +0200, Thomas Gleixner wrote:
> On Wed, 3 Apr 2019, Fenghua Yu wrote:
>
> > A split locked access locks bus and degrades overall memory access
> > performance. When split lock detection feature is enumerated, enable
> > the feature by default to find any split lock issue and then fix
> > the issue.
>
> Enabling the feature allows to find the issues, but does not automagically
> fix them. Come on.
Ok. I will remove the "and then fix the issue".
>
> > +#define DISABLE_SPLIT_LOCK_DETECT 0
> > +#define ENABLE_SPLIT_LOCK_DETECT 1
>
> If those defines have a value at all, please start with the facility not
> with functionality, i.e. AC_SPLIT_LOCK_ENABLE....
OK.
>
> > +
> > +static DEFINE_MUTEX(split_lock_detect_mutex);
> > +static int split_lock_detect_val;
>
> detect_val? What value is that?
According to previous discussions, I was told to call this split lock feature
as "split lock detection" instead of "#AC for split lock". So I use
"split_lock_detect..." in variable names or function names, call feature flag
as "split_lock_detect", and call the feature as "split lock detection" in
descriptions.
If you don't agree to name feature as "split lock detection", I can change
variable names/function names/feature flag/descriptions etc back to previous
names "ac_split_lock...", "#AC for split lock", etc.
The variable split_lock_detect_val is either 0 or 1. It stores current
enable/disable status of split lock detection feature. By default it's
one after the feature is enumerated. Then sysadmin can change it to 0 or 1
to enable or disable the feature during run time.
> Its supposed to hold those magic defines
> above. So something like
>
> static unsigned int ac_split_lock_enable;
If you agree to name the split lock feature as "split lock detection" feature,
can I change this variable to static unsigned int split_lock_detect_enable?
> > /*
> > * Just in case our CPU detection goes bad, or you have a weird system,
> > * allow a way to override the automatic disabling of MPX.
> > @@ -161,10 +167,45 @@ static bool bad_spectre_microcode(struct cpuinfo_x86 *c)
> > return false;
> > }
> >
> > +static u32 new_sp_test_ctl_val(u32 test_ctl_val)
> > +{
> > + /* Change the split lock setting. */
> > + if (READ_ONCE(split_lock_detect_val) == DISABLE_SPLIT_LOCK_DETECT)
>
> That READ_ONCE() is required because?
Ok. Will remove READ_ONCE().
>
> > + test_ctl_val &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> > + else
> > + test_ctl_val |= TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> > +
> > + return test_ctl_val;
> > +}
>
> Aside of that do we really need a misnomed function which replaces the
> simple inline code at the call site:
>
> rdmsr(l, h)
> l &= ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT;
> l |= ac_split_lock_enable << TEST_CTL_ENABLE_SPLIT_LOCK_DETECT_SHIFT;
> wrmrs(...)
>
> or the even more simple
>
> if (ac_split_lock_enable)
> msr_set_bit(...)
> else
> msr_clear_nit(...)
>
> Hmm?
The function new_sp_test_ctrl_val() will be called twice: here when
initializing split lock detection and in split_lock_detect_store()
when enabling/disabling the feature through the sysfs interface in
patch 0014.
So can I still keep this function and name it as get_new_test_ctrl_val()?
>
> > +
> > +static inline void show_split_lock_detection_info(void)
> > +{
> > + if (READ_ONCE(split_lock_detect_val))
>
> That READ_ONCE() is required because?
Ok. Will remove READ_ONCE().
>
> > + pr_info_once("x86/split_lock: split lock detection enabled\n");
> > + else
> > + pr_info_once("x86/split_lock: split lock detection disabled\n");
>
> pr_fmt exists for a reason and having 'split lock' repeated several times
> in the same line is not making it more readable.
Ok. I will change the string to "x86/split_lock_detection: enabled\n",
is it ok?
>
>
> > +
> > /* Unmask CPUID levels if masked: */
> > if (c->x86 > 6 || (c->x86 == 6 && c->x86_model >= 0xd)) {
> > if (msr_clear_bit(MSR_IA32_MISC_ENABLE,
> > @@ -1032,6 +1073,7 @@ cpu_dev_register(intel_cpu_dev);
> > static void __init set_split_lock_detect(void)
> > {
> > setup_force_cpu_cap(X86_FEATURE_SPLIT_LOCK_DETECT);
> > + split_lock_detect_val = 1;
>
> Oh well. You add defines on top of the file and then you don't use them.
Will fix this.
Thanks.
-Fenghua
^ permalink raw reply
* Re: [PATCH v6 14/20] x86/split_lock: Add a sysfs interface to enable/disable split lock detection during run time
From: Thomas Gleixner @ 2019-04-04 19:11 UTC (permalink / raw)
To: Fenghua Yu
Cc: Ingo Molnar, Borislav Petkov, H Peter Anvin, Dave Hansen,
Paolo Bonzini, Ashok Raj, Peter Zijlstra, Kalle Valo, Xiaoyao Li,
Michael Chan, Ravi V Shankar, linux-kernel, x86, linux-wireless,
netdev, kvm
In-Reply-To: <1554326526-172295-15-git-send-email-fenghua.yu@intel.com>
On Wed, 3 Apr 2019, Fenghua Yu wrote:
> +
> +static ssize_t
> +split_lock_detect_show(struct device *dev, struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%u\n", READ_ONCE(split_lock_detect_val));
Please stop sprinkling READ_ONCE all over the place or can you explain why
this is in any way useful? You know what READ/WRITE_ONCE() is for, right?
> +}
> +
> +static ssize_t
> +split_lock_detect_store(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + u32 val, l, h;
> + int cpu, ret;
> +
> + ret = kstrtou32(buf, 10, &val);
> + if (ret)
> + return ret;
> +
> + if (val != DISABLE_SPLIT_LOCK_DETECT && val != ENABLE_SPLIT_LOCK_DETECT)
> + return -EINVAL;
As this is really a simple boolean you can just use strtobool() and be done
with it.
> +
> + /*
> + * Since split lock could be disabled by kernel #AC handler or user
> + * may directly change bit 29 in MSR_TEST_CTL, split lock setting on
The user can change bit 29 in that MSR? If you talk about /dev/msr then I
really do not care. That interface should die.
Aside of that your usage of the term 'user' is really misleading and
inconsistent all over the place.
> + * each CPU may be different from global setting split_lock_detect_val
> + * by now. Update MSR on each CPU, so all of CPUs will have same split
> + * lock setting.
That helps in which way? If #AC was detected in the kernel then
1) It's likely to be switched off again right away
2) The WARN_ONCE() already triggered and will not warn again.
So what's the point here, really? If the kernel triggers #AC, game
over. Fix the kernel first. If your kernel is clean, then why do you need
that knob at all?
> + */
> + mutex_lock(&split_lock_detect_mutex);
> +
> + WRITE_ONCE(split_lock_detect_val, val);
Oh well.
> + /*
> + * Get MSR_TEST_CTL on this CPU, assuming all CPUs have same value
> + * in the MSR except split lock detection bit (bit 29).
And some day in the future this breaks because MRS_TEST_CTL has some other
shiny bits.
> + */
> + rdmsr(MSR_TEST_CTL, l, h);
> + l = new_sp_test_ctl_val(l);
> + /* Update the split lock detection setting on all online CPUs. */
> + for_each_online_cpu(cpu)
And what exactly protects the online cpu mask?
> + wrmsr_on_cpu(cpu, MSR_TEST_CTL, l, h);
Oh well. Instead of just having a function which does:
fun()
if (ac_...enabled)
msr_set_bit()
else
msr_clear_bit()
and invoke that from cpu init code and from here via on_each_cpu() or such?
> + mutex_unlock(&split_lock_detect_mutex);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR_RW(split_lock_detect);
> +
> +static int __init split_lock_init(void)
> +{
> + int ret;
> +
> + if (!boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> + return -ENODEV;
> +
> + ret = device_create_file(cpu_subsys.dev_root,
> + &dev_attr_split_lock_detect);
> + if (ret)
> + return ret;
> +
> + return 0;
What's wrong with:
return device_create_file();
??? Not hard enough to read, right?
> +}
> +
Pointless empty line.
> +subsys_initcall(split_lock_init);
Thanks,
tglx
^ permalink raw reply
* [PATCH] b43: Remove empty function lpphy_papd_cal()
From: Larry Finger @ 2019-04-04 18:49 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, pkshih, Larry Finger, Arnd Bergmann
In commit d825db346270e ("b43: shut up clang -Wuninitialized variable
warning"), the message noted that function lpphy_papd_cal() was empty
and had an old TODO regarding its implementation. As the reverse
engineering project that created the LP-PHY version of this driver
has not been active for some time, it is safe to remove this empty
function.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
Cc: Arnd Bergmann <arnd@arndb.de>
---
drivers/net/wireless/broadcom/b43/phy_lp.c | 11 -----------
1 file changed, 11 deletions(-)
diff --git a/drivers/net/wireless/broadcom/b43/phy_lp.c b/drivers/net/wireless/broadcom/b43/phy_lp.c
index aedee026c5e2..6b7f0238723f 100644
--- a/drivers/net/wireless/broadcom/b43/phy_lp.c
+++ b/drivers/net/wireless/broadcom/b43/phy_lp.c
@@ -1826,12 +1826,6 @@ static void lpphy_stop_tx_tone(struct b43_wldev *dev)
}
-static void lpphy_papd_cal(struct b43_wldev *dev, struct lpphy_tx_gains gains,
- int mode, bool useindex, u8 index)
-{
- //TODO
-}
-
static void lpphy_papd_cal_txpwr(struct b43_wldev *dev)
{
struct b43_phy_lp *lpphy = dev->phy.lp;
@@ -1848,11 +1842,6 @@ static void lpphy_papd_cal_txpwr(struct b43_wldev *dev)
lpphy_set_tx_power_control(dev, B43_LPPHY_TXPCTL_OFF);
- if (dev->dev->chip_id == 0x4325 && dev->dev->chip_rev == 0)
- lpphy_papd_cal(dev, oldgains, 0, 1, 30);
- else
- lpphy_papd_cal(dev, oldgains, 0, 1, 65);
-
if (old_afe_ovr)
lpphy_set_tx_gains(dev, oldgains);
lpphy_set_bb_mult(dev, old_bbmult);
--
2.21.0
^ permalink raw reply related
* [PATCH v2 2/2] rtlwifi: Convert the wake_match variable to local
From: Larry Finger @ 2019-04-04 18:29 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, pkshih, Larry Finger
In-Reply-To: <20190404182931.9477-1-Larry.Finger@lwfinger.net>
In five of the drivers, the contents of bits 29-31 of one of the RX
descriptors is used to set bits in a variable that is used to save the
wakeup condition for output in a debugging statement. The resulting
variable is not used anywhere else even though it is stored in a struct
and could be available in other routines. This variable is changed to be
local.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
V2 - Fixed Merge problem
---
.../net/wireless/realtek/rtlwifi/rtl8188ee/trx.c | 15 ++++++++-------
.../wireless/realtek/rtlwifi/rtl8188ee/trx.c.rej | 10 ++++++++++
.../net/wireless/realtek/rtlwifi/rtl8192ee/trx.c | 13 +++++++------
.../net/wireless/realtek/rtlwifi/rtl8723be/trx.c | 14 +++++++-------
.../net/wireless/realtek/rtlwifi/rtl8821ae/trx.c | 14 +++++++-------
drivers/net/wireless/realtek/rtlwifi/wifi.h | 1 -
6 files changed, 39 insertions(+), 28 deletions(-)
create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c.rej
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
index 2dd01696c014..483dc8bdc555 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
@@ -372,8 +372,9 @@ bool rtl88ee_rx_query_desc(struct ieee80211_hw *hw,
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rx_fwinfo_88e *p_drvinfo;
struct ieee80211_hdr *hdr;
-
+ u8 wake_match;
u32 phystatus = GET_RX_DESC_PHYST(pdesc);
+
status->packet_report_type = (u8)GET_RX_STATUS_DESC_RPT_SEL(pdesc);
if (status->packet_report_type == TX_REPORT2)
status->length = (u16)GET_RX_RPT2_DESC_PKT_LEN(pdesc);
@@ -400,17 +401,17 @@ bool rtl88ee_rx_query_desc(struct ieee80211_hw *hw,
status->macid = GET_RX_DESC_MACID(pdesc);
if (GET_RX_STATUS_DESC_PATTERN_MATCH(pdesc))
- status->wake_match = BIT(2);
+ wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
- status->wake_match = BIT(1);
+ wake_match = BIT(1);
else if (GET_RX_STATUS_DESC_UNICAST_MATCH(pdesc))
- status->wake_match = BIT(0);
+ wake_match = BIT(0);
else
- status->wake_match = 0;
- if (status->wake_match)
+ wake_match = 0;
+ if (wake_match)
RT_TRACE(rtlpriv, COMP_RXDESC, DBG_LOUD,
"GGGGGGGGGGGGGet Wakeup Packet!! WakeMatch=%d\n",
- status->wake_match);
+ wake_match);
rx_status->freq = hw->conf.chandef.chan->center_freq;
rx_status->band = hw->conf.chandef.chan->band;
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c.rej b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c.rej
new file mode 100644
index 000000000000..aa03d4605d8c
--- /dev/null
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c.rej
@@ -0,0 +1,10 @@
+--- drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
++++ drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
+@@ -373,6 +373,7 @@ bool rtl88ee_rx_query_desc(struct ieee80211_hw *hw,
+ struct rx_fwinfo_88e *p_drvinfo;
+ struct ieee80211_hdr *hdr;
+ u32 phystatus = GET_RX_DESC_PHYST(pdesc);
++ u8 wake_match;
+
+ status->packet_report_type = (u8)GET_RX_STATUS_DESC_RPT_SEL(pdesc);
+ if (status->packet_report_type == TX_REPORT2)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c
index 49a4c84d193a..d297cfc0fd2b 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c
@@ -331,6 +331,7 @@ bool rtl92ee_rx_query_desc(struct ieee80211_hw *hw,
struct rx_fwinfo *p_drvinfo;
struct ieee80211_hdr *hdr;
u32 phystatus = GET_RX_DESC_PHYST(pdesc);
+ u8 wake_match;
if (GET_RX_STATUS_DESC_RPT_SEL(pdesc) == 0)
status->packet_report_type = NORMAL_RX;
@@ -351,17 +352,17 @@ bool rtl92ee_rx_query_desc(struct ieee80211_hw *hw,
status->macid = GET_RX_DESC_MACID(pdesc);
if (GET_RX_STATUS_DESC_PATTERN_MATCH(pdesc))
- status->wake_match = BIT(2);
+ wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
- status->wake_match = BIT(1);
+ wake_match = BIT(1);
else if (GET_RX_STATUS_DESC_UNICAST_MATCH(pdesc))
- status->wake_match = BIT(0);
+ wake_match = BIT(0);
else
- status->wake_match = 0;
- if (status->wake_match)
+ wake_match = 0;
+ if (wake_match)
RT_TRACE(rtlpriv, COMP_RXDESC, DBG_LOUD,
"GGGGGGGGGGGGGet Wakeup Packet!! WakeMatch=%d\n",
- status->wake_match);
+ wake_match);
rx_status->freq = hw->conf.chandef.chan->center_freq;
rx_status->band = hw->conf.chandef.chan->band;
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c
index a382cdc668ed..d87ba03fe78f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c
@@ -300,7 +300,7 @@ bool rtl8723be_rx_query_desc(struct ieee80211_hw *hw,
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rx_fwinfo_8723be *p_drvinfo;
struct ieee80211_hdr *hdr;
-
+ u8 wake_match;
u32 phystatus = GET_RX_DESC_PHYST(pdesc);
status->length = (u16)GET_RX_DESC_PKT_LEN(pdesc);
@@ -330,17 +330,17 @@ bool rtl8723be_rx_query_desc(struct ieee80211_hw *hw,
if (GET_RX_STATUS_DESC_PATTERN_MATCH(pdesc))
- status->wake_match = BIT(2);
+ wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
- status->wake_match = BIT(1);
+ wake_match = BIT(1);
else if (GET_RX_STATUS_DESC_UNICAST_MATCH(pdesc))
- status->wake_match = BIT(0);
+ wake_match = BIT(0);
else
- status->wake_match = 0;
- if (status->wake_match)
+ wake_match = 0;
+ if (wake_match)
RT_TRACE(rtlpriv, COMP_RXDESC, DBG_LOUD,
"GGGGGGGGGGGGGet Wakeup Packet!! WakeMatch=%d\n",
- status->wake_match);
+ wake_match);
rx_status->freq = hw->conf.chandef.chan->center_freq;
rx_status->band = hw->conf.chandef.chan->band;
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
index db5e628b17ed..7b6faf38e09c 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/trx.c
@@ -436,7 +436,7 @@ bool rtl8821ae_rx_query_desc(struct ieee80211_hw *hw,
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rx_fwinfo_8821ae *p_drvinfo;
struct ieee80211_hdr *hdr;
-
+ u8 wake_match;
u32 phystatus = GET_RX_DESC_PHYST(pdesc);
status->length = (u16)GET_RX_DESC_PKT_LEN(pdesc);
@@ -473,18 +473,18 @@ bool rtl8821ae_rx_query_desc(struct ieee80211_hw *hw,
status->packet_report_type = NORMAL_RX;
if (GET_RX_STATUS_DESC_PATTERN_MATCH(pdesc))
- status->wake_match = BIT(2);
+ wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
- status->wake_match = BIT(1);
+ wake_match = BIT(1);
else if (GET_RX_STATUS_DESC_UNICAST_MATCH(pdesc))
- status->wake_match = BIT(0);
+ wake_match = BIT(0);
else
- status->wake_match = 0;
+ wake_match = 0;
- if (status->wake_match)
+ if (wake_match)
RT_TRACE(rtlpriv, COMP_RXDESC, DBG_LOUD,
"GGGGGGGGGGGGGet Wakeup Packet!! WakeMatch=%d\n",
- status->wake_match);
+ wake_match);
rx_status->freq = hw->conf.chandef.chan->center_freq;
rx_status->band = hw->conf.chandef.chan->band;
diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
index e32e9ffa3192..518aaa875361 100644
--- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
+++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
@@ -2138,7 +2138,6 @@ struct rtl_stats {
u8 packet_report_type;
u32 macid;
- u8 wake_match;
u32 bt_rx_rssi_percentage;
u32 macid_valid_entry[2];
};
--
2.21.0
^ permalink raw reply related
* [PATCH v2 1/2] rtlwifi: Fix duplicate tests of one of the RX descriptors
From: Larry Finger @ 2019-04-04 18:29 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, pkshih, Larry Finger
In-Reply-To: <20190404182931.9477-1-Larry.Finger@lwfinger.net>
In drivers rtl8188ee, rtl8821ae, rtl8723be, and rtl8192ee, the reason
for a wake-up is returned in the fourth RX descriptor in bits 29-31. Due
to typographical errors, all but rtl8821ae test bit 31 twice and fail to
test bit 29.
This error causes no problems as the tests are only used to set bits in
the output of an optional debugging statement.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
V2 - Fixed Merge problem
---
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c | 2 +-
drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c | 2 +-
drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
index 106011a24827..2dd01696c014 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c
@@ -399,7 +399,7 @@ bool rtl88ee_rx_query_desc(struct ieee80211_hw *hw,
status->is_cck = RTL8188_RX_HAL_IS_CCK_RATE(status->rate);
status->macid = GET_RX_DESC_MACID(pdesc);
- if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
+ if (GET_RX_STATUS_DESC_PATTERN_MATCH(pdesc))
status->wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
status->wake_match = BIT(1);
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c
index 09cf8180e4ff..49a4c84d193a 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8192ee/trx.c
@@ -350,7 +350,7 @@ bool rtl92ee_rx_query_desc(struct ieee80211_hw *hw,
status->is_cck = RTL92EE_RX_HAL_IS_CCK_RATE(status->rate);
status->macid = GET_RX_DESC_MACID(pdesc);
- if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
+ if (GET_RX_STATUS_DESC_PATTERN_MATCH(pdesc))
status->wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
status->wake_match = BIT(1);
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c
index 9ada9a06c6ea..a382cdc668ed 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/trx.c
@@ -329,7 +329,7 @@ bool rtl8723be_rx_query_desc(struct ieee80211_hw *hw,
status->packet_report_type = NORMAL_RX;
- if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
+ if (GET_RX_STATUS_DESC_PATTERN_MATCH(pdesc))
status->wake_match = BIT(2);
else if (GET_RX_STATUS_DESC_MAGIC_MATCH(pdesc))
status->wake_match = BIT(1);
--
2.21.0
^ permalink raw reply related
* [PATCH v2 0/2] Modify the handling of the reason for a wakeup
From: Larry Finger @ 2019-04-04 18:29 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, pkshih, Larry Finger
Some of the drivers test the wrong bit in an RX descriptor. That is
fixed. In a second patch, that variable is moved out of a struct and
made local to the routine that uses it.
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
V2 - Fixed Merge problem
Larry Finger (2):
rtlwifi: Fix duplicate tests of one of the RX descriptors
rtlwifi: Convert the wake_match variable to local
.../wireless/realtek/rtlwifi/rtl8188ee/trx.c | 17 +++++++++--------
.../realtek/rtlwifi/rtl8188ee/trx.c.rej | 10 ++++++++++
.../wireless/realtek/rtlwifi/rtl8192ee/trx.c | 15 ++++++++-------
.../wireless/realtek/rtlwifi/rtl8723be/trx.c | 16 ++++++++--------
.../wireless/realtek/rtlwifi/rtl8821ae/trx.c | 14 +++++++-------
drivers/net/wireless/realtek/rtlwifi/wifi.h | 1 -
6 files changed, 42 insertions(+), 31 deletions(-)
create mode 100644 drivers/net/wireless/realtek/rtlwifi/rtl8188ee/trx.c.rej
--
2.21.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox