* Re: [PATCH][next] qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
From: Sergey Matyukevich @ 2019-04-04 13:32 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Igor Mitsyanko, Avinash Patil, Sergey Matyukevich, Kalle Valo,
David S. Miller, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190403170236.GA31637@embeddedor>
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> struct boo entry[];
> };
>
> size = sizeof(struct foo) + count * sizeof(struct boo);
> instance = kzalloc(size, GFP_KERNEL)
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> size = struct_size(instance, entry, count);
>
> or
>
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL)
>
> Based on the above, replace qtnf_cmd_acl_data_size() with the
> new struct_size() helper.
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> ---
> drivers/net/wireless/quantenna/qtnfmac/commands.c | 15 ++++-----------
> 1 file changed, 4 insertions(+), 11 deletions(-)
Hi Gustavo,
Thanks for the patch! By the way, it does not apply cleanly, so it needs
to be rebased on top of the up-to-date wireless-drivers-next tree. Let
me know if you would prefer me to care about rebase. Then I will
add this patch to the upcoming series of qtnfmac fixes.
Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
Regards,
Sergey
^ permalink raw reply
* [PATCH] ath6kl: remove redundant check of status != 0
From: Colin King @ 2019-04-04 13:47 UTC (permalink / raw)
To: Kalle Valo, David S . Miller, linux-wireless, netdev
Cc: kernel-janitors, linux-kernel
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>
---
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;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] ath6kl: remove redundant check of status != 0
From: Kalle Valo @ 2019-04-04 14:33 UTC (permalink / raw)
To: Colin King
Cc: David S . Miller, linux-wireless, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20190404134723.24667-1-colin.king@canonical.com>
Colin King <colin.king@canonical.com> writes:
> 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")
Is the coverity id missing?
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v6 01/20] x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long
From: Borislav Petkov @ 2019-04-04 14:39 UTC (permalink / raw)
To: Fenghua Yu
Cc: Thomas Gleixner, Ingo Molnar, 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-2-git-send-email-fenghua.yu@intel.com>
On Wed, Apr 03, 2019 at 02:21:47PM -0700, Fenghua Yu wrote:
> cpu_caps_cleared and cpu_caps_set may not be aligned to unsigned long.
> Atomic operations (i.e. set_bit and clear_bit) on the bitmaps may access
Please end function names with parentheses.
> two cache lines (a.k.a. split lock) and lock the bus to block all memory
not "lock the bus" but "cause the CPU to do a bus lock... "
> accesses from other processors to ensure atomicity.
>
> To avoid the overall performance degradation from the bus locking, align
> the two variables to unsigned long.
>
> Defining the variables as unsigned long may also fix the issue because
> they are naturally aligned to unsigned long. But that needs additional
> code changes. Adding __aligned(unsigned long) is a simpler fix.
>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> arch/x86/kernel/cpu/common.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
With the above nitpicks addressed:
Reviewed-by: Borislav Petkov <bp@suse.de>
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
^ permalink raw reply
* RE: [PATCH v6 04/20] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-04 14:44 UTC (permalink / raw)
To: 'Fenghua Yu', Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H Peter Anvin, Dave Hansen, Paolo Bonzini,
Ashok Raj, Peter Zijlstra, Kalle Valo, Xiaoyao Li , Michael Chan,
Ravi V Shankar
Cc: linux-kernel, x86, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, kvm@vger.kernel.org
In-Reply-To: <1554326526-172295-5-git-send-email-fenghua.yu@intel.com>
From: Fenghua Yu
> Sent: 03 April 2019 22:22
> set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> operate on bitmap defined in x86_capability.
>
> Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> the location is at:
> base address of x86_capability + (bit offset in x86_capability / 64) * 8
>
> Since base address of x86_capability may not be aligned to unsigned long,
> the single unsigned long location may cross two cache lines and
> accessing the location by locked BTS/BTR introductions will trigger #AC.
That is not true.
The BTS/BTR instructions access the memory word that contains the
expected bit.
The 'operand size' only affects the size of the register use for the
bit offset.
If the 'operand size' is 16 bits wide (+/- 32k bit offset) the cpu might
do an aligned 16bit memory access, otherwise (32 or 64bit bit offset) it
might do an aligned 32 bit access.
It should never do an 64bit access and never a misaligned one (even if
the base address is misaligned).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v6 12/20] kvm/vmx: Emulate MSR TEST_CTL
From: Sean Christopherson @ 2019-04-04 14:44 UTC (permalink / raw)
To: Fenghua Yu, Paolo Bonzini
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, H Peter Anvin,
Dave Hansen, 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, Apr 03, 2019 at 02:21:58PM -0700, Fenghua Yu wrote:
> From: Xiaoyao Li <xiaoyao.li@linux.intel.com>
>
> A control bit (bit 29) in TEST_CTL MSR 0x33 will be introduced in
> future x86 processors. When bit 29 is set, the processor causes #AC
> exception for split locked accesses at all CPL.
>
> Please check the latest Intel 64 and IA-32 Architectures Software
> Developer's Manual for more detailed information on the MSR and
> the split lock bit.
>
> This patch emulate MSR TEST_CTL with vmx->msr_test_ctl and does the
> following:
> 1. As MSR TEST_CTL of guest is emulated, enable the related bits
> in CORE_CAPABILITY to corretly report this feature to guest.
s/corretly/correctly
>
> 2. Differentiate MSR TEST_CTL between host and guest.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@linux.intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Acked-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/kvm/vmx/vmx.c | 35 +++++++++++++++++++++++++++++++++++
> arch/x86/kvm/vmx/vmx.h | 1 +
> arch/x86/kvm/x86.c | 17 ++++++++++++++++-
> 3 files changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index ab432a930ae8..309ccf593f0d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -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))
> + 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;
> +
> + if (data & ~TEST_CTL_ENABLE_SPLIT_LOCK_DETECT)
> + return 1;
> + vmx->msr_test_ctl = data;
> + break;
> case MSR_EFER:
> ret = kvm_set_msr_common(vcpu, msr_info);
> break;
> @@ -4077,6 +4091,9 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
> ++vmx->nmsrs;
> }
>
> + /* disable AC split lock by default */
> + vmx->msr_test_ctl = 0;
> +
> vm_exit_controls_init(vmx, vmx_vmexit_ctrl());
>
> /* 22.2.1, 20.8.1 */
> @@ -4114,6 +4131,7 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
>
> vmx->rmode.vm86_active = 0;
> vmx->spec_ctrl = 0;
> + vmx->msr_test_ctl = 0;
>
> vcpu->arch.microcode_version = 0x100000000ULL;
> vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val();
> @@ -6313,6 +6331,21 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
> msrs[i].host, false);
> }
>
> +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;
This adds a RDMSR on every VM-Enter, and a fault on CPUs that don't
support MSR_TEST_CTL. Ideally the kernel would cache MSR_TEST_CTL and
expose a helper that returns a boolean to indicate the existence of the
MSRs along with the current value. Racing with split_lock_detect_store()
is ok since this code runs with interrupts disabled, i.e. will block
split_lock_detect_store() until after VM-Exit.
Paolo, can you weigh in with your thoughts? I'm surprised you acked
this patch given your earlier comment:
https://patchwork.kernel.org/patch/10413779/#21892723
> +
> + 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);
> +}
> +
> static void vmx_arm_hv_timer(struct vcpu_vmx *vmx, u32 val)
> {
> vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, val);
> @@ -6419,6 +6452,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
>
> atomic_switch_perf_msrs(vmx);
>
> + atomic_switch_msr_test_ctl(vmx);
> +
> vmx_update_hv_timer(vcpu);
>
> /*
> diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> index a1e00d0a2482..6091a8b9de74 100644
> --- a/arch/x86/kvm/vmx/vmx.h
> +++ b/arch/x86/kvm/vmx/vmx.h
> @@ -190,6 +190,7 @@ struct vcpu_vmx {
> u64 msr_guest_kernel_gs_base;
> #endif
>
> + u64 msr_test_ctl;
> u64 spec_ctrl;
>
> u32 vm_entry_controls_shadow;
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 4459115eb0ec..e93c2f620cdb 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1229,7 +1229,22 @@ EXPORT_SYMBOL_GPL(kvm_get_arch_capabilities);
>
> u64 kvm_get_core_capability(void)
> {
> - return 0;
> + u64 data;
> +
> + rdmsrl_safe(MSR_IA32_CORE_CAPABILITY, &data);
> +
> + /* mask non-virtualizable functions */
> + data &= CORE_CAP_SPLIT_LOCK_DETECT;
> +
> + /*
> + * 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.
> + */
> + if (boot_cpu_has(X86_FEATURE_SPLIT_LOCK_DETECT))
> + data |= CORE_CAP_SPLIT_LOCK_DETECT;
> +
> + return data;
> }
> EXPORT_SYMBOL_GPL(kvm_get_core_capability);
>
> --
> 2.19.1
>
^ permalink raw reply
* Re: [PATCH][next] qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
From: Gustavo A. R. Silva @ 2019-04-04 16:01 UTC (permalink / raw)
To: Sergey Matyukevich
Cc: Igor Mitsyanko, Avinash Patil, Sergey Matyukevich, Kalle Valo,
David S. Miller, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <20190404133234.2uatru2vlw3ntrmv@bars>
On 4/4/19 8:32 AM, Sergey Matyukevich wrote:
>> One of the more common cases of allocation size calculations is finding
>> the size of a structure that has a zero-sized array at the end, along
>> with memory for some number of elements for that array. For example:
>>
>> struct foo {
>> int stuff;
>> struct boo entry[];
>> };
>>
>> size = sizeof(struct foo) + count * sizeof(struct boo);
>> instance = kzalloc(size, GFP_KERNEL)
>>
>> Instead of leaving these open-coded and prone to type mistakes, we can
>> now use the new struct_size() helper:
>>
>> size = struct_size(instance, entry, count);
>>
>> or
>>
>> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL)
>>
>> Based on the above, replace qtnf_cmd_acl_data_size() with the
>> new struct_size() helper.
>>
>> This code was detected with the help of Coccinelle.
>>
>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>> ---
>> drivers/net/wireless/quantenna/qtnfmac/commands.c | 15 ++++-----------
>> 1 file changed, 4 insertions(+), 11 deletions(-)
>
> Hi Gustavo,
>
Hey Sergey,
> Thanks for the patch! By the way, it does not apply cleanly, so it needs
> to be rebased on top of the up-to-date wireless-drivers-next tree. Let
> me know if you would prefer me to care about rebase. Then I will
> add this patch to the upcoming series of qtnfmac fixes.
>
Don't worry. I'll do it and send this again.
> Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
>
I'll add your RB.
Thanks!
--
Gustavo
^ permalink raw reply
* Re: [PATCH v6 01/20] x86/common: Align cpu_caps_cleared and cpu_caps_set to unsigned long
From: Fenghua Yu @ 2019-04-04 15:54 UTC (permalink / raw)
To: Borislav Petkov
Cc: Thomas Gleixner, Ingo Molnar, 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: <20190404143942.GD22539@zn.tnic>
On Thu, Apr 04, 2019 at 04:39:43PM +0200, Borislav Petkov wrote:
> On Wed, Apr 03, 2019 at 02:21:47PM -0700, Fenghua Yu wrote:
> > cpu_caps_cleared and cpu_caps_set may not be aligned to unsigned long.
> > Atomic operations (i.e. set_bit and clear_bit) on the bitmaps may access
>
> Please end function names with parentheses.
Sure.
>
> > two cache lines (a.k.a. split lock) and lock the bus to block all memory
>
> not "lock the bus" but "cause the CPU to do a bus lock... "
Sure.
>
> > accesses from other processors to ensure atomicity.
> >
> > To avoid the overall performance degradation from the bus locking, align
> > the two variables to unsigned long.
> >
> > Defining the variables as unsigned long may also fix the issue because
> > they are naturally aligned to unsigned long. But that needs additional
> > code changes. Adding __aligned(unsigned long) is a simpler fix.
> >
> > Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> > ---
> > arch/x86/kernel/cpu/common.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
>
> With the above nitpicks addressed:
>
> Reviewed-by: Borislav Petkov <bp@suse.de>
Thank you for your review!
-Fenghua
^ permalink raw reply
* RE: [PATCH v6 04/20] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: David Laight @ 2019-04-04 16:24 UTC (permalink / raw)
To: David Laight, 'Fenghua Yu', 'Thomas Gleixner',
'Ingo Molnar', 'Borislav Petkov',
'H Peter Anvin', 'Dave Hansen',
'Paolo Bonzini', 'Ashok Raj',
'Peter Zijlstra', 'Kalle Valo',
'Xiaoyao Li ', 'Michael Chan',
'Ravi V Shankar'
Cc: 'linux-kernel', 'x86',
'linux-wireless@vger.kernel.org',
'netdev@vger.kernel.org', 'kvm@vger.kernel.org'
In-Reply-To: <73ecc9de54c3424da3cddd1a34cb8701@AcuMS.aculab.com>
From: David Laight
> Sent: 04 April 2019 15:45
>
> From: Fenghua Yu
> > Sent: 03 April 2019 22:22
> > set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> > operate on bitmap defined in x86_capability.
> >
> > Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> > the location is at:
> > base address of x86_capability + (bit offset in x86_capability / 64) * 8
> >
> > Since base address of x86_capability may not be aligned to unsigned long,
> > the single unsigned long location may cross two cache lines and
> > accessing the location by locked BTS/BTR introductions will trigger #AC.
>
> That is not true.
> The BTS/BTR instructions access the memory word that contains the
> expected bit.
> The 'operand size' only affects the size of the register use for the
> bit offset.
> If the 'operand size' is 16 bits wide (+/- 32k bit offset) the cpu might
> do an aligned 16bit memory access, otherwise (32 or 64bit bit offset) it
> might do an aligned 32 bit access.
> It should never do an 64bit access and never a misaligned one (even if
> the base address is misaligned).
Hmmm... I may have misread things slightly.
The accessed address is 'Effective Address + (4 ∗ (BitOffset DIV 32))'.
However nothing suggests that it ever does 64bit accesses.
If it does do 64bit accesses when the operand size is 64 bits then the
asm stubs ought to be changed to only specify 32bit operand size.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: [PATCH v6 04/20] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: Sean Christopherson @ 2019-04-04 16:35 UTC (permalink / raw)
To: David Laight
Cc: 'Fenghua Yu', 'Thomas Gleixner',
'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: <c0d43317cce34389ac4a1e91e97d909b@AcuMS.aculab.com>
On Thu, Apr 04, 2019 at 04:24:15PM +0000, David Laight wrote:
> From: David Laight
> > Sent: 04 April 2019 15:45
> >
> > From: Fenghua Yu
> > > Sent: 03 April 2019 22:22
> > > set_cpu_cap() calls locked BTS and clear_cpu_cap() calls locked BTR to
> > > operate on bitmap defined in x86_capability.
> > >
> > > Locked BTS/BTR accesses a single unsigned long location. In 64-bit mode,
> > > the location is at:
> > > base address of x86_capability + (bit offset in x86_capability / 64) * 8
> > >
> > > Since base address of x86_capability may not be aligned to unsigned long,
> > > the single unsigned long location may cross two cache lines and
> > > accessing the location by locked BTS/BTR introductions will trigger #AC.
> >
> > That is not true.
> > The BTS/BTR instructions access the memory word that contains the
> > expected bit.
> > The 'operand size' only affects the size of the register use for the
> > bit offset.
> > If the 'operand size' is 16 bits wide (+/- 32k bit offset) the cpu might
> > do an aligned 16bit memory access, otherwise (32 or 64bit bit offset) it
> > might do an aligned 32 bit access.
> > It should never do an 64bit access and never a misaligned one (even if
> > the base address is misaligned).
>
> Hmmm... I may have misread things slightly.
> The accessed address is 'Effective Address + (4 ∗ (BitOffset DIV 32))'.
> However nothing suggests that it ever does 64bit accesses.
>
> If it does do 64bit accesses when the operand size is 64 bits then the
> asm stubs ought to be changed to only specify 32bit operand size.
Heh, we had this discussion before[1], the op size dictates the size of
the memory access and can generate unaligned accesses.
[1] https://lkml.kernel.org/r/20181127195153.GE27075@linux.intel.com
^ permalink raw reply
* Re: [PATCH][next] qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
From: Kalle Valo @ 2019-04-04 16:38 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Igor Mitsyanko, Avinash Patil, Sergey Matyukevich,
David S. Miller, linux-wireless, netdev, linux-kernel,
Gustavo A. R. Silva
In-Reply-To: <20190403170236.GA31637@embeddedor>
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> struct boo entry[];
> };
>
> size = sizeof(struct foo) + count * sizeof(struct boo);
> instance = kzalloc(size, GFP_KERNEL)
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> size = struct_size(instance, entry, count);
>
> or
>
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL)
>
> Based on the above, replace qtnf_cmd_acl_data_size() with the
> new struct_size() helper.
>
> This code was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Reviewed-by: Sergey Matyukevich <sergey.matyukevich.os@quantenna.com>
Patch applied to wireless-drivers-next.git, thanks.
95336d4cb588 qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
--
https://patchwork.kernel.org/patch/10884311/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH][next] qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
From: Kalle Valo @ 2019-04-04 16:40 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Sergey Matyukevich, Igor Mitsyanko, Avinash Patil,
Sergey Matyukevich, David S. Miller,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <3baa19c1-a213-3dcf-2f03-23b058ec419a@embeddedor.com>
"Gustavo A. R. Silva" <gustavo@embeddedor.com> writes:
> On 4/4/19 11:01 AM, Gustavo A. R. Silva wrote:
>>
>>
>> On 4/4/19 8:32 AM, Sergey Matyukevich wrote:
>>>> One of the more common cases of allocation size calculations is finding
>>>> the size of a structure that has a zero-sized array at the end, along
>>>> with memory for some number of elements for that array. For example:
>>>>
>>>> struct foo {
>>>> int stuff;
>>>> struct boo entry[];
>>>> };
>>>>
>>>> size = sizeof(struct foo) + count * sizeof(struct boo);
>>>> instance = kzalloc(size, GFP_KERNEL)
>>>>
>>>> Instead of leaving these open-coded and prone to type mistakes, we can
>>>> now use the new struct_size() helper:
>>>>
>>>> size = struct_size(instance, entry, count);
>>>>
>>>> or
>>>>
>>>> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL)
>>>>
>>>> Based on the above, replace qtnf_cmd_acl_data_size() with the
>>>> new struct_size() helper.
>>>>
>>>> This code was detected with the help of Coccinelle.
>>>>
>>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>>> ---
>>>> drivers/net/wireless/quantenna/qtnfmac/commands.c | 15 ++++-----------
>>>> 1 file changed, 4 insertions(+), 11 deletions(-)
>>>
>>> Hi Gustavo,
>>>
>>
>> Hey Sergey,
>>
>>> Thanks for the patch! By the way, it does not apply cleanly, so it needs
>>> to be rebased on top of the up-to-date wireless-drivers-next tree. Let
>>> me know if you would prefer me to care about rebase. Then I will
>>> add this patch to the upcoming series of qtnfmac fixes.
>>>
>>
>> Don't worry. I'll do it and send this again.
>>
>
> Hmm... I just applied it cleanly on top of wireless-drivers-next/master:
>
> 973a99be7943 (HEAD) qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
> 38bb0baea310 (wireless-drivers-next/master) rtlwifi: move spin_lock_bh
> to spin_lock in tasklet
> 60209d482b97 rtlwifi: fix potential NULL pointer dereference
> 765976285a8c rtlwifi: fix a potential NULL pointer dereference
>
> Do you see any issues with this?
Yeah, I also was able to apply it without problems. So it's in w-d-next
now :)
--
Kalle Valo
^ permalink raw reply
* RE: [PATCH v6 04/20] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: Thomas Gleixner @ 2019-04-04 16:52 UTC (permalink / raw)
To: David Laight
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: <c0d43317cce34389ac4a1e91e97d909b@AcuMS.aculab.com>
[-- Attachment #1: Type: text/plain, Size: 1261 bytes --]
On Thu, 4 Apr 2019, David Laight wrote:
> From: David Laight Sent: 04 April 2019 15:45
> > From: Fenghua Yu Sent: 03 April 2019 22:22
> > That is not true.
> > The BTS/BTR instructions access the memory word that contains the
> > expected bit.
> > The 'operand size' only affects the size of the register use for the
> > bit offset.
> > If the 'operand size' is 16 bits wide (+/- 32k bit offset) the cpu might
> > do an aligned 16bit memory access, otherwise (32 or 64bit bit offset) it
> > might do an aligned 32 bit access.
> > It should never do an 64bit access and never a misaligned one (even if
> > the base address is misaligned).
>
> Hmmm... I may have misread things slightly.
> The accessed address is 'Effective Address + (4 ∗ (BitOffset DIV 32))'.
> However nothing suggests that it ever does 64bit accesses.
>
> If it does do 64bit accesses when the operand size is 64 bits then the
> asm stubs ought to be changed to only specify 32bit operand size.
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.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH][next] qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
From: Gustavo A. R. Silva @ 2019-04-04 16:33 UTC (permalink / raw)
To: Sergey Matyukevich
Cc: Igor Mitsyanko, Avinash Patil, Sergey Matyukevich, Kalle Valo,
David S. Miller, linux-wireless@vger.kernel.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <e56579c6-da6e-4cb2-90b1-89e7774debf9@embeddedor.com>
On 4/4/19 11:01 AM, Gustavo A. R. Silva wrote:
>
>
> On 4/4/19 8:32 AM, Sergey Matyukevich wrote:
>>> One of the more common cases of allocation size calculations is finding
>>> the size of a structure that has a zero-sized array at the end, along
>>> with memory for some number of elements for that array. For example:
>>>
>>> struct foo {
>>> int stuff;
>>> struct boo entry[];
>>> };
>>>
>>> size = sizeof(struct foo) + count * sizeof(struct boo);
>>> instance = kzalloc(size, GFP_KERNEL)
>>>
>>> Instead of leaving these open-coded and prone to type mistakes, we can
>>> now use the new struct_size() helper:
>>>
>>> size = struct_size(instance, entry, count);
>>>
>>> or
>>>
>>> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL)
>>>
>>> Based on the above, replace qtnf_cmd_acl_data_size() with the
>>> new struct_size() helper.
>>>
>>> This code was detected with the help of Coccinelle.
>>>
>>> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
>>> ---
>>> drivers/net/wireless/quantenna/qtnfmac/commands.c | 15 ++++-----------
>>> 1 file changed, 4 insertions(+), 11 deletions(-)
>>
>> Hi Gustavo,
>>
>
> Hey Sergey,
>
>> Thanks for the patch! By the way, it does not apply cleanly, so it needs
>> to be rebased on top of the up-to-date wireless-drivers-next tree. Let
>> me know if you would prefer me to care about rebase. Then I will
>> add this patch to the upcoming series of qtnfmac fixes.
>>
>
> Don't worry. I'll do it and send this again.
>
Hmm... I just applied it cleanly on top of wireless-drivers-next/master:
973a99be7943 (HEAD) qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
38bb0baea310 (wireless-drivers-next/master) rtlwifi: move spin_lock_bh to spin_lock in tasklet
60209d482b97 rtlwifi: fix potential NULL pointer dereference
765976285a8c rtlwifi: fix a potential NULL pointer dereference
Do you see any issues with this?
Thanks
--
Gustavo
^ permalink raw reply
* Re: [PATCH][next] qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
From: Gustavo A. R. Silva @ 2019-04-04 16:45 UTC (permalink / raw)
To: Kalle Valo
Cc: Sergey Matyukevich, Igor Mitsyanko, Avinash Patil,
Sergey Matyukevich, David S. Miller,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <87imvtsp09.fsf@kamboji.qca.qualcomm.com>
On 4/4/19 11:40 AM, Kalle Valo wrote:
[..]
>>>>
>>>> Hi Gustavo,
>>>>
>>>
>>> Hey Sergey,
>>>
>>>> Thanks for the patch! By the way, it does not apply cleanly, so it needs
>>>> to be rebased on top of the up-to-date wireless-drivers-next tree. Let
>>>> me know if you would prefer me to care about rebase. Then I will
>>>> add this patch to the upcoming series of qtnfmac fixes.
>>>>
>>>
>>> Don't worry. I'll do it and send this again.
>>>
>>
>> Hmm... I just applied it cleanly on top of wireless-drivers-next/master:
>>
>> 973a99be7943 (HEAD) qtnfmac: replace qtnf_cmd_acl_data_size() with struct_size()
>> 38bb0baea310 (wireless-drivers-next/master) rtlwifi: move spin_lock_bh
>> to spin_lock in tasklet
>> 60209d482b97 rtlwifi: fix potential NULL pointer dereference
>> 765976285a8c rtlwifi: fix a potential NULL pointer dereference
>>
>> Do you see any issues with this?
>
> Yeah, I also was able to apply it without problems. So it's in w-d-next
> now :)
>
Awesome. :)
Thanks, Kalle.
--
Gustavo
^ permalink raw reply
* Re: [PATCH 0/6] stricter netlink validation
From: David Miller @ 2019-04-04 17:28 UTC (permalink / raw)
To: johannes; +Cc: linux-wireless, netdev, pablo, dsa
In-Reply-To: <20190404065408.5864-1-johannes@sipsolutions.net>
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.
^ permalink raw reply
* Re: [PATCH v6 04/20] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: Paolo Bonzini @ 2019-04-04 17:29 UTC (permalink / raw)
To: Thomas Gleixner, David Laight
Cc: 'Fenghua Yu', 'Ingo Molnar',
'Borislav Petkov', 'H Peter Anvin',
'Dave Hansen', '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>
On 04/04/19 18:52, Thomas Gleixner wrote:
> On Thu, 4 Apr 2019, David Laight wrote:
>> From: David Laight Sent: 04 April 2019 15:45
>>> From: Fenghua Yu Sent: 03 April 2019 22:22
>>> That is not true.
>>> The BTS/BTR instructions access the memory word that contains the
>>> expected bit.
>>> The 'operand size' only affects the size of the register use for the
>>> bit offset.
>>> If the 'operand size' is 16 bits wide (+/- 32k bit offset) the cpu might
>>> do an aligned 16bit memory access, otherwise (32 or 64bit bit offset) it
>>> might do an aligned 32 bit access.
>>> It should never do an 64bit access and never a misaligned one (even if
>>> the base address is misaligned).
>>
>> Hmmm... I may have misread things slightly.
>> The accessed address is 'Effective Address + (4 ∗ (BitOffset DIV 32))'.
>> However nothing suggests that it ever does 64bit accesses.
>>
>> If it does do 64bit accesses when the operand size is 64 bits then the
>> asm stubs ought to be changed to only specify 32bit operand size.
>
> 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.
Aren't bitops always atomic with respect to the whole cache line(s)? We
regularly rely on cmpxchgl being atomic with respect to movb.
Paolo
^ permalink raw reply
* Re: [PATCH v6 10/20] x86/split_lock: Handle #AC exception for split lock
From: Thomas Gleixner @ 2019-04-04 17:31 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-11-git-send-email-fenghua.yu@intel.com>
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() ....
> + 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.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v6 13/20] x86/split_lock: Enable split lock detection by default
From: Thomas Gleixner @ 2019-04-04 18:07 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-14-git-send-email-fenghua.yu@intel.com>
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.
> +#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....
> +
> +static DEFINE_MUTEX(split_lock_detect_mutex);
> +static int split_lock_detect_val;
detect_val? What value is that? Its supposed to hold those magic defines
above. So something like
static unsigned int ac_split_lock_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?
> + 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?
> +
> +static inline void show_split_lock_detection_info(void)
> +{
> + if (READ_ONCE(split_lock_detect_val))
That READ_ONCE() is required because?
> + 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.
> +}
> +
> +static void init_split_lock_detect(struct cpuinfo_x86 *c)
> +{
> + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
> + u32 l, h;
> +
> + mutex_lock(&split_lock_detect_mutex);
> + rdmsr(MSR_TEST_CTL, l, h);
> + l = new_sp_test_ctl_val(l);
> + wrmsr(MSR_TEST_CTL, l, h);
> + show_split_lock_detection_info();
> + mutex_unlock(&split_lock_detect_mutex);
> + }
> +}
> +
> static void early_init_intel(struct cpuinfo_x86 *c)
> {
> u64 misc_enable;
>
> + init_split_lock_detect(c);
so we have in early boot:
early_cpu_init()
early_identify_cpu()
this_cpu->c_early_init(c)
early_init_intel() {
init_split_lock_detect();
}
....
cpu_set_core_cap_bits(c)
set(FEATURE_SPLIT_LOCK)
I don't have to understand how init_split_lock_detect() will magically see
the feature bit which gets set afterwards, right?
> +
> /* 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.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v6 04/20] x86/split_lock: Align x86_capability to unsigned long to avoid split locked access
From: Thomas Gleixner @ 2019-04-04 18:11 UTC (permalink / raw)
To: Paolo Bonzini
Cc: David Laight, 'Fenghua Yu', 'Ingo Molnar',
'Borislav Petkov', 'H Peter Anvin',
'Dave Hansen', '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: <d6c47549-586c-29ab-d4a7-7ab18ed7a486@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1750 bytes --]
On Thu, 4 Apr 2019, Paolo Bonzini wrote:
> On 04/04/19 18:52, Thomas Gleixner wrote:
> > On Thu, 4 Apr 2019, David Laight wrote:
> >> From: David Laight Sent: 04 April 2019 15:45
> >>> From: Fenghua Yu Sent: 03 April 2019 22:22
> >>> That is not true.
> >>> The BTS/BTR instructions access the memory word that contains the
> >>> expected bit.
> >>> The 'operand size' only affects the size of the register use for the
> >>> bit offset.
> >>> If the 'operand size' is 16 bits wide (+/- 32k bit offset) the cpu might
> >>> do an aligned 16bit memory access, otherwise (32 or 64bit bit offset) it
> >>> might do an aligned 32 bit access.
> >>> It should never do an 64bit access and never a misaligned one (even if
> >>> the base address is misaligned).
> >>
> >> Hmmm... I may have misread things slightly.
> >> The accessed address is 'Effective Address + (4 ∗ (BitOffset DIV 32))'.
> >> However nothing suggests that it ever does 64bit accesses.
> >>
> >> If it does do 64bit accesses when the operand size is 64 bits then the
> >> asm stubs ought to be changed to only specify 32bit operand size.
> >
> > 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.
>
> Aren't bitops always atomic with respect to the whole cache line(s)? We
> regularly rely on cmpxchgl being atomic with respect to movb.
Yes, but if your long goes across a cacheline then you have lost due to the
requirement to lock both cachelines. Same problem as with bitops and I
rather catch all of those than just some.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v6 13/20] x86/split_lock: Enable split lock detection by default
From: Thomas Gleixner @ 2019-04-04 18:14 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: <alpine.DEB.2.21.1904041932500.1802@nanos.tec.linutronix.de>
On Thu, 4 Apr 2019, Thomas Gleixner wrote:
> On Wed, 3 Apr 2019, Fenghua Yu wrote:
> > +static void init_split_lock_detect(struct cpuinfo_x86 *c)
> > +{
> > + if (cpu_has(c, X86_FEATURE_SPLIT_LOCK_DETECT)) {
> > + u32 l, h;
> > +
> > + mutex_lock(&split_lock_detect_mutex);
The mutex protects what exactly in the cpu bringup code?
Thanks,
tglx
^ permalink raw reply
* [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
* [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 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] 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
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