[PATCH V6 1/7] mm/mmap: Add new config ARCH_HAS_VM_GET_PAGE_PROT
Add a new config ARCH_HAS_VM_GET_PAGE_PROT, which when subscribed enables a
given platform to define its own vm_get_page_prot() but still utilizing the
generic protection_map[] array.
Cc: Andrew Morton
Cc: linux...@kvack.org
Cc: linux-ker...@vger.kernel.org
Reviewed-by: Catalin Marinas
Suggested-by: Christoph Hellwig
Signed-off-by: Anshuman Khandual
---
mm/Kconfig | 3 +++
mm/mmap.c | 2 ++
2 files changed, 5 insertions(+)
diff --git a/mm/Kconfig b/mm/Kconfig
index 034d87953600..b1f7624276f8 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -765,6 +765,9 @@ config ARCH_HAS_CURRENT_STACK_POINTER
config ARCH_HAS_FILTER_PGPROT
bool
+config ARCH_HAS_VM_GET_PAGE_PROT
+ bool
+
config ARCH_HAS_PTE_DEVMAP
bool
diff --git a/mm/mmap.c b/mm/mmap.c
index 3aa839f81e63..87cb2eaf7e1a 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -106,6 +106,7 @@ pgprot_t protection_map[16] __ro_after_init = {
__S000, __S001, __S010, __S011, __S100, __S101, __S110, __S111
};
+#ifndef CONFIG_ARCH_HAS_VM_GET_PAGE_PROT
#ifndef CONFIG_ARCH_HAS_FILTER_PGPROT
static inline pgprot_t arch_filter_pgprot(pgprot_t prot)
{
@@ -122,6 +123,7 @@ pgprot_t vm_get_page_prot(unsigned long vm_flags)
return arch_filter_pgprot(ret);
}
EXPORT_SYMBOL(vm_get_page_prot);
+#endif /* CONFIG_ARCH_HAS_VM_GET_PAGE_PROT */
static pgprot_t vm_pgprot_modify(pgprot_t oldprot, unsigned long vm_flags)
{
--
2.25.1
[PATCH V6 0/7] mm/mmap: Drop arch_vm_get_page_prot() and arch_filter_pgprot()
protection_map[] is an array based construct that translates given vm_flags combination. This array contains page protection map, which is populated by the platform via [__S000 .. __S111] and [__P000 .. __P111] exported macros. Primary usage for protection_map[] is for vm_get_page_prot(), which is used to determine page protection value for a given vm_flags. vm_get_page_prot() implementation, could again call platform overrides arch_vm_get_page_prot() and arch_filter_pgprot(). Some platforms override protection_map[] that was originally built with __SXXX/__PXXX with different runtime values. Currently there are multiple layers of abstraction i.e __SXXX/__PXXX macros , protection_map[], arch_vm_get_page_prot() and arch_filter_pgprot() built between the platform and generic MM, finally defining vm_get_page_prot(). Hence this series proposes to drop later two abstraction levels and instead just move the responsibility of defining vm_get_page_prot() to the platform (still utilizing generic protection_map[] array) itself making it clean and simple. This first introduces ARCH_HAS_VM_GET_PAGE_PROT which enables the platforms to define custom vm_get_page_prot(). This starts converting platforms that define the overrides arch_filter_pgprot() or arch_vm_get_page_prot() which enables for those constructs to be dropped off completely. The series has been inspired from an earlier discuss with Christoph Hellwig https://lore.kernel.org/all/1632712920-8171-1-git-send-email-anshuman.khand...@arm.com/ This series applies on 5.18-rc2. This series has been cross built for multiple platforms. - Anshuman Cc: Christoph Hellwig Cc: Andrew Morton Cc: linuxppc-dev@lists.ozlabs.org Cc: linux-arm-ker...@lists.infradead.org Cc: sparcli...@vger.kernel.org Cc: linux...@kvack.org Cc: linux-a...@vger.kernel.org Cc: linux-ker...@vger.kernel.org Changes in V6: - Created single merged vm_get_page_prot() function per Christophe - Dropped local variable 'ret' in generic vm_get_page_prot() per Christophe - Dropped __pgprot(pgprot_val(x)) in generic vm_get_page_prot() per Christophe Changes in V5: https://lore.kernel.org/all/20220412043848.80464-2-anshuman.khand...@arm.com/ - Collected new tags on various patches in the series - Coalesced arm64_arch_vm_get_page_prot() into vm_get_page_prot() per Catalin - Modified powerpc's vm_get_page_prot() implementation per Christophe Changes in V4: https://lore.kernel.org/all/20220407103251.1209606-1-anshuman.khand...@arm.com/ - ARCH_HAS_VM_GET_PAGE_PROT now excludes generic protection_map[] - Changed platform's vm_get_page_prot() to use generic protection_map[] - Dropped all platform changes not enabling either arch_vm_get_page_prot() or arch_filter_pgprot() - Dropped all previous tags as code base has changed Changes in V3: https://lore.kernel.org/all/1646045273-9343-1-git-send-email-anshuman.khand...@arm.com/ - Dropped variable 'i' from sme_early_init() on x86 platform - Moved CONFIG_COLDFIRE vm_get_page_prot() inside arch/m68k/mm/mcfmmu.c - Moved CONFIG_SUN3 vm_get_page_prot() inside arch/m68k/mm/sun3mmu.c - Dropped cachebits for vm_get_page_prot() inside arch/m68k/mm/motorola.c - Dropped PAGE_XXX_C definitions from arch/m68k/include/asm/motorola_pgtable.h - Used PAGE_XXX instead for vm_get_page_prot() inside arch/m68k/mm/motorola.c - Dropped all references to protection_map[] in the tree - Replaced s/extensa/xtensa/ on the patch title - Moved left over comments from pgtable.h into init.c on nios2 platform Changes in V2: https://lore.kernel.org/all/1645425519-9034-1-git-send-email-anshuman.khand...@arm.com/ - Dropped the entire comment block in [PATCH 30/30] per Geert - Replaced __P010 (although commented) with __PAGE_COPY on arm platform - Replaced __P101 with PAGE_READONLY on um platform Changes in V1: https://lore.kernel.org/all/1644805853-21338-1-git-send-email-anshuman.khand...@arm.com/ - Add white spaces around the | operators - Moved powerpc_vm_get_page_prot() near vm_get_page_prot() on powerpc - Moved arm64_vm_get_page_prot() near vm_get_page_prot() on arm64 - Moved sparc_vm_get_page_prot() near vm_get_page_prot() on sparc - Compacted vm_get_page_prot() switch cases on all platforms - _PAGE_CACHE040 inclusion is dependent on CPU_IS_040_OR_060 - VM_SHARED case should return PAGE_NONE (not PAGE_COPY) on SH platform - Reorganized VM_SHARED, VM_EXEC, VM_WRITE, VM_READ - Dropped the last patch [RFC V1 31/31] which added macros for vm_flags combinations https://lore.kernel.org/all/1643029028-12710-32-git-send-email-anshuman.khand...@arm.com/ Changes in RFC: https://lore.kernel.org/all/1643029028-12710-1-git-send-email-anshuman.khand...@arm.com/ Anshuman Khandual (6): mm/mmap: Add new config ARCH_HAS_VM_GET_PAGE_PROT powerpc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT arm64/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT sparc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT mm/mmap: Drop arch_filter_pgprot() mm/mmap: Drop arch_vm_get_page_pgprot() Christoph Hellwig (1):
Re: [PATCH v2] powerpc/rtas: Keep MSR[RI] set when calling RTAS
Excerpts from Laurent Dufour's message of April 2, 2022 12:06 am: > RTAS runs in real mode (MSR[DR] and MSR[IR] unset) and in 32bits > mode (MSR[SF] unset). > > The change in MSR is done in enter_rtas() in a relatively complex way, > since the MSR value could be hardcoded. > > Furthermore, a panic has been reported when hitting the watchdog interrupt > while running in RTAS, this leads to the following stack trace: > > [69244.027433][ C24] watchdog: CPU 24 Hard LOCKUP > [69244.027442][ C24] watchdog: CPU 24 TB:997512652051031, last heartbeat > TB:997504470175378 (15980ms ago) > [69244.027451][ C24] Modules linked in: chacha_generic(E) libchacha(E) > xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) > libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) > algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) > fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) > cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) > algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) > rpcsec_gss_krb5(E) auth_rpcgss(E) > nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) > udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) > netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) > fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) > crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) > fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) > sd_mod(E) t10_pi(E) > [69244.027555][ C24] ibmvfc(EX) scsi_transport_fc(E) vmx_crypto(E) > gf128mul(E) btrfs(E) blake2b_generic(E) libcrc32c(E) crc32c_vpmsum(E) xor(E) > raid6_pq(E) dm_mirror(E) dm_region_hash(E) dm_log(E) sg(E) dm_multipath(E) > dm_mod(E) scsi_dh_rdac(E) scsi_dh_emc(E) scsi_dh_alua(E) scsi_mod(E) > [69244.027587][ C24] Supported: No, Unreleased kernel > [69244.027600][ C24] CPU: 24 PID: 87504 Comm: drmgr Kdump: loaded Tainted: > GE X5.14.21-150400.71.1.bz196362_2-default #1 SLE15-SP4 > (unreleased) 0d821077ef4faa8dfaf370efb5fdca1fa35f4e2c > [69244.027609][ C24] NIP: 1fb41050 LR: 1fb4104c CTR: > > [69244.027612][ C24] REGS: cfc33d60 TRAP: 0100 Tainted: G >E X (5.14.21-150400.71.1.bz196362_2-default) > [69244.027615][ C24] MSR: 82981000 CR: 4882 > XER: 20040020 > [69244.027625][ C24] CFAR: 011c IRQMASK: 1 > [69244.027625][ C24] GPR00: 0003 > 0001 50dc > [69244.027625][ C24] GPR04: 1ffb6100 0020 > 0001 1fb09010 > [69244.027625][ C24] GPR08: 2000 > > [69244.027625][ C24] GPR12: 8004072a40a8 cff8b680 > 0007 0034 > [69244.027625][ C24] GPR16: 1fbf6e94 1fbf6d84 > 1fbd1db0 1fb3f008 > [69244.027625][ C24] GPR20: 1fb41018 > 017f f68f > [69244.027625][ C24] GPR24: 1fb18fe8 1fb3e000 > 1fb1adc0 1fb1cf40 > [69244.027625][ C24] GPR28: 1fb26000 1fb460f0 > 1fb17f18 1fb17000 > [69244.027663][ C24] NIP [1fb41050] 0x1fb41050 > [69244.027696][ C24] LR [1fb4104c] 0x1fb4104c > [69244.027699][ C24] Call Trace: > [69244.027701][ C24] Instruction dump: > [69244.027723][ C24] > > [69244.027728][ C24] > > [69244.027762][T87504] Oops: Unrecoverable System Reset, sig: 6 [#1] > [69244.028044][T87504] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > [69244.028089][T87504] Modules linked in: chacha_generic(E) libchacha(E) > xxhash_generic(E) wp512(E) sha3_generic(E) rmd160(E) poly1305_generic(E) > libpoly1305(E) michael_mic(E) md4(E) crc32_generic(E) cmac(E) ccm(E) > algif_rng(E) twofish_generic(E) twofish_common(E) serpent_generic(E) > fcrypt(E) des_generic(E) libdes(E) cast6_generic(E) cast5_generic(E) > cast_common(E) camellia_generic(E) blowfish_generic(E) blowfish_common(E) > algif_skcipher(E) algif_hash(E) gcm(E) algif_aead(E) af_alg(E) tun(E) > rpcsec_gss_krb5(E) auth_rpcgss(E) > nfsv4(E) dns_resolver(E) rpadlpar_io(EX) rpaphp(EX) xsk_diag(E) tcp_diag(E) > udp_diag(E) raw_diag(E) inet_diag(E) unix_diag(E) af_packet_diag(E) > netlink_diag(E) nfsv3(E) nfs_acl(E) nfs(E) lockd(E) grace(E) sunrpc(E) > fscache(E) netfs(E) af_packet(E) rfkill(E) bonding(E) tls(E) ibmveth(EX) > crct10dif_vpmsum(E) rtc_generic(E) drm(E) drm_panel_orientation_quirks(E) > fuse(E) configfs(E) backlight(E) ip_tables(E) x_tables(E) dm_service_time(E) > sd_mod(E) t10_pi(E) > [69244.028171][T87504]
[no subject]
+Daniel, Thomas, Viresh
Subject: Re: rcu_sched self-detected stall on CPU
Excerpts from Michael Ellerman's message of April 9, 2022 12:42 am:
> Michael Ellerman writes:
>> "Paul E. McKenney" writes:
>>> On Wed, Apr 06, 2022 at 05:31:10PM +0800, Zhouyi Zhou wrote:
Hi
I can reproduce it in a ppc virtual cloud server provided by Oregon
State University. Following is what I do:
1) curl -l
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/snapshot/linux-5.18-rc1.tar.gz
-o linux-5.18-rc1.tar.gz
2) tar zxf linux-5.18-rc1.tar.gz
3) cp config linux-5.18-rc1/.config
4) cd linux-5.18-rc1
5) make vmlinux -j 8
6) qemu-system-ppc64 -kernel vmlinux -nographic -vga none -no-reboot
-smp 2 (QEMU 4.2.1)
7) after 12 rounds, the bug got reproduced:
(http://154.223.142.244/logs/20220406/qemu.log.txt)
>>>
>>> Just to make sure, are you both seeing the same thing? Last I knew,
>>> Zhouyi was chasing an RCU-tasks issue that appears only in kernels
>>> built with CONFIG_PROVE_RCU=y, which Miguel does not have set. Or did
>>> I miss something?
>>>
>>> Miguel is instead seeing an RCU CPU stall warning where RCU's grace-period
>>> kthread slept for three milliseconds, but did not wake up for more than
>>> 20 seconds. This kthread would normally have awakened on CPU 1, but
>>> CPU 1 looks to me to be very unhealthy, as can be seen in your console
>>> output below (but maybe my idea of what is healthy for powerpc systems
>>> is outdated). Please see also the inline annotations.
>>>
>>> Thoughts from the PPC guys?
>>
>> I haven't seen it in my testing. But using Miguel's config I can
>> reproduce it seemingly on every boot.
>>
>> For me it bisects to:
>>
>> 35de589cb879 ("powerpc/time: improve decrementer clockevent processing")
>>
>> Which seems plausible.
>>
>> Reverting that on mainline makes the bug go away.
>>
>> I don't see an obvious bug in the diff, but I could be wrong, or the old
>> code was papering over an existing bug?
>>
>> I'll try and work out what it is about Miguel's config that exposes
>> this vs our defconfig, that might give us a clue.
>
> It's CONFIG_HIGH_RES_TIMERS=n which triggers the stall.
>
> I can reproduce just with:
>
> $ make ppc64le_guest_defconfig
> $ ./scripts/config -d HIGH_RES_TIMERS
>
> We have no defconfigs that disable HIGH_RES_TIMERS, I didn't even
> realise you could disable it TBH :)
>
> The Rust CI has it disabled because I copied that from the x86 defconfig
> they were using back when I added the Rust support. I think that was
> meant to be a stripped down fast config for CI, but the result is it's
> just using a badly tested combination which is not helpful.
>
> So I'll send a patch to turn HIGH_RES_TIMERS on for the Rust CI, and we
> can debug this further without blocking them.
So we traced the problem down to possibly a misunderstanding between
decrementer clock event device and core code.
The decrementer is only oneshot*ish*. It actually needs to either be
reprogrammed or shut down otherwise it just continues to cause
interrupts.
Before commit 35de589cb879, it was sort of two-shot. The initial
interrupt at the programmed time would set its internal next_tb variable
to ~0 and call the ->event_handler(). If that did not set_next_event or
stop the timer, the interrupt will fire again immediately, notice
next_tb is ~0, and only then stop the decrementer interrupt.
So that was already kind of ugly, this patch just turned it into a hang.
The problem happens when the tick is stopped with an event still
pending, then tick_nohz_handler() is called, but it bails out because
tick_stopped == 1 so the device never gets programmed again, and so it
keeps firing.
How to fix it? Before commit a7cba02deced, powerpc's decrementer was
really oneshot, but we would like to avoid doing that because it requires
additional programming of the hardware on each timer interrupt. We have
the ONESHOT_STOPPED state which seems to be just about what we want.
Did the ONESHOT_STOPPED patch just miss this case, or is there a reason
we don't stop it here? This patch seems to fix the hang (not heavily
tested though).
Thanks,
Nick
---
diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
index 2d76c91b85de..7e13a55b6b71 100644
--- a/kernel/time/tick-sched.c
+++ b/kernel/time/tick-sched.c
@@ -1364,9 +1364,11 @@ static void tick_nohz_handler(struct clock_event_device
*dev)
tick_sched_do_timer(ts, now);
tick_sched_handle(ts, regs);
- /* No need to reprogram if we are running tickless */
- if (unlikely(ts->tick_stopped))
+ if (unlikely(ts->tick_stopped)) {
+ /* If we are tickless, change the clock event to stopped */
+ tick_program_event(KTIME_MAX, 1);
return;
+ }
hrtimer_forward(>sched_timer, now, TICK_NSEC);
tick_program_event(hrtimer_get_expires(>sched_timer), 1);
Re: [PATCH V5 2/7] powerpc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
On 4/12/22 17:57, Christophe Leroy wrote:
>
>
> Le 12/04/2022 à 06:38, Anshuman Khandual a écrit :
>> This defines and exports a platform specific custom vm_get_page_prot() via
>> subscribing ARCH_HAS_VM_GET_PAGE_PROT. While here, this also localizes
>> arch_vm_get_page_prot() as __vm_get_page_prot() and moves it near
>> vm_get_page_prot().
>>
>> Cc: Michael Ellerman
>> Cc: Paul Mackerras
>> Cc: linuxppc-dev@lists.ozlabs.org
>> Cc: linux-ker...@vger.kernel.org
>> Signed-off-by: Anshuman Khandual
>> ---
>> arch/powerpc/Kconfig | 1 +
>> arch/powerpc/include/asm/mman.h| 12
>> arch/powerpc/mm/book3s64/pgtable.c | 20
>> 3 files changed, 21 insertions(+), 12 deletions(-)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index 174edabb74fa..69e44358a235 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -140,6 +140,7 @@ config PPC
>> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
>> select ARCH_HAS_UACCESS_FLUSHCACHE
>> select ARCH_HAS_UBSAN_SANITIZE_ALL
>> +select ARCH_HAS_VM_GET_PAGE_PROTif PPC_BOOK3S_64
>> select ARCH_HAVE_NMI_SAFE_CMPXCHG
>> select ARCH_KEEP_MEMBLOCK
>> select ARCH_MIGHT_HAVE_PC_PARPORT
>> diff --git a/arch/powerpc/include/asm/mman.h
>> b/arch/powerpc/include/asm/mman.h
>> index 7cb6d18f5cd6..1b024e64c8ec 100644
>> --- a/arch/powerpc/include/asm/mman.h
>> +++ b/arch/powerpc/include/asm/mman.h
>> @@ -24,18 +24,6 @@ static inline unsigned long
>> arch_calc_vm_prot_bits(unsigned long prot,
>> }
>> #define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot,
>> pkey)
>>
>> -static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags)
>> -{
>> -#ifdef CONFIG_PPC_MEM_KEYS
>> -return (vm_flags & VM_SAO) ?
>> -__pgprot(_PAGE_SAO | vmflag_to_pte_pkey_bits(vm_flags)) :
>> -__pgprot(0 | vmflag_to_pte_pkey_bits(vm_flags));
>> -#else
>> -return (vm_flags & VM_SAO) ? __pgprot(_PAGE_SAO) : __pgprot(0);
>> -#endif
>> -}
>> -#define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
>> -
>> static inline bool arch_validate_prot(unsigned long prot, unsigned long
>> addr)
>> {
>> if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
>> diff --git a/arch/powerpc/mm/book3s64/pgtable.c
>> b/arch/powerpc/mm/book3s64/pgtable.c
>> index 052e6590f84f..d0319524e27f 100644
>> --- a/arch/powerpc/mm/book3s64/pgtable.c
>> +++ b/arch/powerpc/mm/book3s64/pgtable.c
>> @@ -7,6 +7,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> @@ -549,3 +550,22 @@ unsigned long memremap_compat_align(void)
>> }
>> EXPORT_SYMBOL_GPL(memremap_compat_align);
>> #endif
>> +
>> +static pgprot_t __vm_get_page_prot(unsigned long vm_flags)
>> +{
>> +#ifdef CONFIG_PPC_MEM_KEYS
>> +return (vm_flags & VM_SAO) ?
>> +__pgprot(_PAGE_SAO | vmflag_to_pte_pkey_bits(vm_flags)) :
>> +__pgprot(0 | vmflag_to_pte_pkey_bits(vm_flags));
>> +#else
>> +return (vm_flags & VM_SAO) ? __pgprot(_PAGE_SAO) : __pgprot(0);
>> +#endif
>> +}
>> +
>> +pgprot_t vm_get_page_prot(unsigned long vm_flags)
>> +{
>> +return __pgprot(pgprot_val(protection_map[vm_flags &
>> +(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
>> +pgprot_val(__vm_get_page_prot(vm_flags)));
>> +}
>> +EXPORT_SYMBOL(vm_get_page_prot);
>
> This looks functionnaly OK, but I think we could go in the same
> direction as ARM and try to integrate __vm_get_page_prot() inside
> vm_get_page_prot() to get something simpler and cleaner:
>
> Something like below (untested):
>
> pgprot_t vm_get_page_prot(unsigned long vm_flags)
> {
> unsigned long prot = pgprot_val(protection_map[vm_flags &
>(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]);
>
> if (vm_flags & VM_SAO)
> prot |= _PAGE_SAO
>
> #ifdef CONFIG_PPC_MEM_KEYS
> prot |= vmflag_to_pte_pkey_bits(vm_flags);
> #endif
>
> return __pgprot(prot);
> }
Okay, will integrate these functions into vm_get_page_prot() as suggested.
Re: [PATCH V5 6/7] mm/mmap: Drop arch_filter_pgprot()
On 4/12/22 17:59, Christophe Leroy wrote:
>
>
> Le 12/04/2022 à 06:38, Anshuman Khandual a écrit :
>> There are no platforms left which subscribe ARCH_HAS_FILTER_PGPROT. Hence
>> drop generic arch_filter_pgprot() and also config ARCH_HAS_FILTER_PGPROT.
>>
>> Cc: Andrew Morton
>> Cc: linux...@kvack.org
>> Cc: linux-ker...@vger.kernel.org
>> Reviewed-by: Catalin Marinas
>> Signed-off-by: Anshuman Khandual
>> ---
>> mm/Kconfig | 3 ---
>> mm/mmap.c | 9 +
>> 2 files changed, 1 insertion(+), 11 deletions(-)
>>
>> diff --git a/mm/Kconfig b/mm/Kconfig
>> index b1f7624276f8..3f7b6d7b69df 100644
>> --- a/mm/Kconfig
>> +++ b/mm/Kconfig
>> @@ -762,9 +762,6 @@ config ARCH_HAS_CURRENT_STACK_POINTER
>>register alias named "current_stack_pointer", this config can be
>>selected.
>>
>> -config ARCH_HAS_FILTER_PGPROT
>> -bool
>> -
>> config ARCH_HAS_VM_GET_PAGE_PROT
>> bool
>>
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index 87cb2eaf7e1a..edf2a5e38f4d 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -107,20 +107,13 @@ pgprot_t protection_map[16] __ro_after_init = {
>> };
>>
>> #ifndef CONFIG_ARCH_HAS_VM_GET_PAGE_PROT
>> -#ifndef CONFIG_ARCH_HAS_FILTER_PGPROT
>> -static inline pgprot_t arch_filter_pgprot(pgprot_t prot)
>> -{
>> -return prot;
>> -}
>> -#endif
>> -
>> pgprot_t vm_get_page_prot(unsigned long vm_flags)
>> {
>> pgprot_t ret = __pgprot(pgprot_val(protection_map[vm_flags &
>> (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
>> pgprot_val(arch_vm_get_page_prot(vm_flags)));
>>
>> -return arch_filter_pgprot(ret);
>> +return ret;
>
> You can drop 'ret' and directly do:
>
> return __pgprot(pgprot_val(protection_map[vm_flags &
> (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
> pgprot_val(arch_vm_get_page_prot(vm_flags)));
Sure, will do.
>
>
>> }
>> EXPORT_SYMBOL(vm_get_page_prot);
>> #endif /* CONFIG_ARCH_HAS_VM_GET_PAGE_PROT */
Re: [PATCH V5 7/7] mm/mmap: Drop arch_vm_get_page_pgprot()
On 4/12/22 18:00, Christophe Leroy wrote:
>
>
> Le 12/04/2022 à 06:38, Anshuman Khandual a écrit :
>> There are no platforms left which use arch_vm_get_page_prot(). Just drop
>> generic arch_vm_get_page_prot().
>>
>> Cc: Andrew Morton
>> Cc: linux...@kvack.org
>> Cc: linux-ker...@vger.kernel.org
>> Reviewed-by: Catalin Marinas
>> Signed-off-by: Anshuman Khandual
>> ---
>> include/linux/mman.h | 4
>> mm/mmap.c| 3 +--
>> 2 files changed, 1 insertion(+), 6 deletions(-)
>>
>> diff --git a/include/linux/mman.h b/include/linux/mman.h
>> index b66e91b8176c..58b3abd457a3 100644
>> --- a/include/linux/mman.h
>> +++ b/include/linux/mman.h
>> @@ -93,10 +93,6 @@ static inline void vm_unacct_memory(long pages)
>> #define arch_calc_vm_flag_bits(flags) 0
>> #endif
>>
>> -#ifndef arch_vm_get_page_prot
>> -#define arch_vm_get_page_prot(vm_flags) __pgprot(0)
>> -#endif
>> -
>> #ifndef arch_validate_prot
>> /*
>>* This is called from mprotect(). PROT_GROWSDOWN and PROT_GROWSUP have
>> diff --git a/mm/mmap.c b/mm/mmap.c
>> index edf2a5e38f4d..db7f33154206 100644
>> --- a/mm/mmap.c
>> +++ b/mm/mmap.c
>> @@ -110,8 +110,7 @@ pgprot_t protection_map[16] __ro_after_init = {
>> pgprot_t vm_get_page_prot(unsigned long vm_flags)
>> {
>> pgprot_t ret = __pgprot(pgprot_val(protection_map[vm_flags &
>> -(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
>> -pgprot_val(arch_vm_get_page_prot(vm_flags)));
>> +(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]));
>>
>> return ret;
>> }
>
> __pgprot(pgprot_val(x)) is a no-op.
>
> You can simply do:
>
> return protection_map[vm_flags &
> (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED);
Sure, will do.
Re: [PATCH] ASoC: fsl: using pm_runtime_resume_and_get instead of pm_runtime_get_sync
On Tue, Apr 12, 2022 at 4:30 PM wrote:
> From: Minghao Chi
>
> Using pm_runtime_resume_and_get is more appropriate
> for simplifing code
>
> Reported-by: Zeal Robot
> Signed-off-by: Minghao Chi
>
Acked-by: Shengjiu Wang
Best regards
Wang Shengjiu
> ---
> sound/soc/fsl/fsl_esai.c | 6 ++
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
> index ed444e8f1d6b..1a2bdf8e76f0 100644
> --- a/sound/soc/fsl/fsl_esai.c
> +++ b/sound/soc/fsl/fsl_esai.c
> @@ -1050,11 +1050,9 @@ static int fsl_esai_probe(struct platform_device
> *pdev)
> goto err_pm_disable;
> }
>
> - ret = pm_runtime_get_sync(>dev);
> - if (ret < 0) {
> - pm_runtime_put_noidle(>dev);
> + ret = pm_runtime_resume_and_get(>dev);
> + if (ret < 0)
> goto err_pm_get_sync;
> - }
>
> ret = fsl_esai_hw_init(esai_priv);
> if (ret)
> --
> 2.25.1
>
>
Re: [PATCH net-next v3 14/18] sfc: Remove usage of list iterator for list_add() after the loop body
On Tue, 12 Apr 2022 14:15:53 +0200 Jakob Koschel wrote: > - struct list_head *head = >rss_context.list; > + struct list_head *head = *pos = >rss_context.list; ENOTBUILT, please wait with the reposting. Since you posted two versions today I guess that's 2x 24h? :)
Re: [PATCH] ASoC: fsl: using pm_runtime_resume_and_get instead of pm_runtime_get_sync
On Tue, 12 Apr 2022 08:30:00 +, cgel@gmail.com wrote: > From: Minghao Chi > > Using pm_runtime_resume_and_get is more appropriate > for simplifing code > > Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next Thanks! [1/1] ASoC: fsl: using pm_runtime_resume_and_get instead of pm_runtime_get_sync commit: c721905c54d913db0102973dbcdfb48d91146a2d All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
Re: False positive kmemleak report for dtb properties names on powerpc
On Tue, Apr 12, 2022 at 04:47:47PM +1000, Michael Ellerman wrote:
> Christophe Leroy writes:
> > Hi Ariel
> >
> > Le 09/04/2022 à 15:47, Ariel Marcovitch a écrit :
> >> Hi Christophe, did you get the chance to look at this?
> >
> > I tested something this morning, it works for me, see below
> >
> >>
> >> On 23/03/2022 21:06, Mike Rapoport wrote:
> >>> Hi Catalin,
> >>>
> >>> On Wed, Mar 23, 2022 at 05:22:38PM +, Catalin Marinas wrote:
> Hi Ariel,
>
> On Fri, Feb 18, 2022 at 09:45:51PM +0200, Ariel Marcovitch wrote:
> > I was running a powerpc 32bit kernel (built using
> > qemu_ppc_mpc8544ds_defconfig
> > buildroot config, with enabling DEBUGFS+KMEMLEAK+HIGHMEM in the kernel
> > config)
> >
> > ...
> >
> > I don't suppose I can just shuffle the calls in setup_arch() around,
> > so I
> > wanted to hear your opinions first
> I think it's better if we change the logic than shuffling the calls.
> IIUC MEMBLOCK_ALLOC_ACCESSIBLE means that __va() works on the phys
> address return by memblock, so something like below (untested):
> >>> MEMBLOCK_ALLOC_ACCESSIBLE means "anywhere", see commit e63075a3c937
> >>> ("memblock: Introduce default allocation limit and use it to replace
> >>> explicit ones"), so it won't help to detect high memory.
> >>>
> >>> If I remember correctly, ppc initializes memblock *very* early, so
> >>> setting
> >>> max_low_pfn along with lowmem_end_addr in
> >>> arch/powerpc/mm/init_32::MMU_init() makes sense to me.
> >>>
> >>> Maybe ppc folks have other ideas...
> >>> I've added Christophe who works on ppc32 these days.
> >
> > I think memblock is already available at the end of MMU_init() on PPC32
> > and at the end of early_setup() on PPC64. It means it is ready when we
> > enter setup_arch().
> >
> > I tested the change below, it works for me, I don't get any kmemleak
> > report anymore.
> >
> > diff --git a/arch/powerpc/kernel/setup-common.c
> > b/arch/powerpc/kernel/setup-common.c
> > index 518ae5aa9410..9f4e50b176c9 100644
> > --- a/arch/powerpc/kernel/setup-common.c
> > +++ b/arch/powerpc/kernel/setup-common.c
> > @@ -840,6 +840,9 @@ void __init setup_arch(char **cmdline_p)
> > /* Set a half-reasonable default so udelay does something sensible */
> > loops_per_jiffy = 5 / HZ;
> >
> > + /* Parse memory topology */
> > + mem_topology_setup();
> > +
> > /* Unflatten the device-tree passed by prom_init or kexec */
> > unflatten_device_tree();
>
> The 64-bit/NUMA version of mem_topology_setup() requires the device tree
> to be unflattened, so I don't think that can work.
>
> Setting max_low_pfn etc in MMU_init() as Mike suggested seems more
> likely to work.
>
> But we might need to set it again in mem_topology_setup() though, so
> that things that change memblock_end_of_DRAM() are reflected, eg. memory
> limit or crash dump?
I don't think this can cause issues for kmemleak Ariel reported. The
kmemleak checks if there is a linear mapping for a PFN or that PFN is only
accessible via HIGHMEM. Memory limit or crash dump won't change the split,
or am I missing something?
> cheers
--
Sincerely yours,
Mike.
[PATCH V3 2/2] perf bench: Fix numa bench to fix usage of affinity for machines with #CPUs > 1K
The 'perf bench numa' testcase fails on systems with more than 1K CPUs.
Testcase: perf bench numa mem -p 1 -t 3 -P 512 -s 100 -zZ0qcm --thp 1
Snippet of code:
<<>>
perf: bench/numa.c:302: bind_to_node: Assertion `!(ret)' failed.
Aborted (core dumped)
<<>>
bind_to_node function uses "sched_getaffinity" to save the original
cpumask and this call is returning EINVAL ((invalid argument).
This happens because the default mask size in glibc is 1024. To
overcome this 1024 CPUs mask size limitation of cpu_set_t, change the
mask size using the CPU_*_S macros ie, use CPU_ALLOC to allocate
cpumask, CPU_ALLOC_SIZE for size.
Apart from fixing this for "orig_mask", apply same logic to "mask" as
well which is used to setaffinity so that mask size is large enough to
represent number of possible CPU's in the system.
sched_getaffinity is used in one more place in perf numa bench. It is in
"bind_to_cpu" function. Apply the same logic there also. Though
currently no failure is reported from there, it is ideal to change
getaffinity to work with such system configurations having CPU's more
than default mask size supported by glibc.
Also fix "sched_setaffinity" to use mask size which is large enough to
represent number of possible CPU's in the system.
Fixed all places where "bind_cpumask" which is part of "struct
thread_data" is used such that bind_cpumask works in all configuration.
Reported-by: Disha Goel
Signed-off-by: Athira Rajeev
---
Changelog
The first version was posted here:
https://lore.kernel.org/all/20220406175113.87881-5-atraj...@linux.vnet.ibm.com/
This v3 version separates patch 3 and addressing compilation
failure in some distro's. Change in V3 does initialization of cpu_set
to NULL in main to fix compilation warning for uninitialized pointer.
Also made changes in "bind_to_cpu" and "bind_to_node" to CPU_FREE
masks properly.
tools/perf/bench/numa.c | 128 +---
1 file changed, 95 insertions(+), 33 deletions(-)
diff --git a/tools/perf/bench/numa.c b/tools/perf/bench/numa.c
index c838c248aa2c..ffa83744ef99 100644
--- a/tools/perf/bench/numa.c
+++ b/tools/perf/bench/numa.c
@@ -55,7 +55,7 @@
struct thread_data {
int curr_cpu;
- cpu_set_t bind_cpumask;
+ cpu_set_t *bind_cpumask;
int bind_node;
u8 *process_data;
int process_nr;
@@ -267,71 +267,115 @@ static bool node_has_cpus(int node)
return ret;
}
-static cpu_set_t bind_to_cpu(int target_cpu)
+static cpu_set_t *bind_to_cpu(int target_cpu)
{
- cpu_set_t orig_mask, mask;
- int ret;
+ int nrcpus = numa_num_possible_cpus();
+ cpu_set_t *orig_mask, *mask;
+ size_t size;
- ret = sched_getaffinity(0, sizeof(orig_mask), _mask);
- BUG_ON(ret);
+ orig_mask = CPU_ALLOC(nrcpus);
+ BUG_ON(!orig_mask);
+ size = CPU_ALLOC_SIZE(nrcpus);
+ CPU_ZERO_S(size, orig_mask);
+
+ if (sched_getaffinity(0, size, orig_mask))
+ goto err_out;
+
+ mask = CPU_ALLOC(nrcpus);
+ if (!mask)
+ goto err_out;
- CPU_ZERO();
+ CPU_ZERO_S(size, mask);
if (target_cpu == -1) {
int cpu;
for (cpu = 0; cpu < g->p.nr_cpus; cpu++)
- CPU_SET(cpu, );
+ CPU_SET_S(cpu, size, mask);
} else {
- BUG_ON(target_cpu < 0 || target_cpu >= g->p.nr_cpus);
- CPU_SET(target_cpu, );
+ if (target_cpu < 0 || target_cpu >= g->p.nr_cpus)
+ goto err;
+
+ CPU_SET_S(target_cpu, size, mask);
}
- ret = sched_setaffinity(0, sizeof(mask), );
- BUG_ON(ret);
+ if (sched_setaffinity(0, size, mask))
+ goto err;
return orig_mask;
+
+err:
+ CPU_FREE(mask);
+err_out:
+ CPU_FREE(orig_mask);
+
+ /* BUG_ON due to failure in allocation of orig_mask/mask */
+ BUG_ON(-1);
}
-static cpu_set_t bind_to_node(int target_node)
+static cpu_set_t *bind_to_node(int target_node)
{
- cpu_set_t orig_mask, mask;
+ int nrcpus = numa_num_possible_cpus();
+ size_t size;
+ cpu_set_t *orig_mask, *mask;
int cpu;
- int ret;
- ret = sched_getaffinity(0, sizeof(orig_mask), _mask);
- BUG_ON(ret);
+ orig_mask = CPU_ALLOC(nrcpus);
+ BUG_ON(!orig_mask);
+ size = CPU_ALLOC_SIZE(nrcpus);
+ CPU_ZERO_S(size, orig_mask);
+
+ if (sched_getaffinity(0, size, orig_mask))
+ goto err_out;
+
+ mask = CPU_ALLOC(nrcpus);
+ if (!mask)
+ goto err_out;
- CPU_ZERO();
+ CPU_ZERO_S(size, mask);
if (target_node == NUMA_NO_NODE) {
for (cpu = 0; cpu < g->p.nr_cpus; cpu++)
- CPU_SET(cpu, );
+
[PATCH V3 1/2] tools/perf: Fix perf bench numa testcase to check if CPU used to bind task is online
Perf numa bench test fails with error:
Testcase:
./perf bench numa mem -p 2 -t 1 -P 1024 -C 0,8 -M 1,0 -s 20 -zZq
--thp 1 --no-data_rand_walk
Failure snippet:
<<>>
Running 'numa/mem' benchmark:
# Running main, "perf bench numa numa-mem -p 2 -t 1 -P 1024 -C 0,8
-M 1,0 -s 20 -zZq --thp 1 --no-data_rand_walk"
perf: bench/numa.c:333: bind_to_cpumask: Assertion `!(ret)' failed.
<<>>
The Testcases uses CPU's 0 and 8. In function "parse_setup_cpu_list",
There is check to see if cpu number is greater than max cpu's possible
in the system ie via "if (bind_cpu_0 >= g->p.nr_cpus ||
bind_cpu_1 >= g->p.nr_cpus) {". But it could happen that system has
say 48 CPU's, but only number of online CPU's is 0-7. Other CPU's
are offlined. Since "g->p.nr_cpus" is 48, so function will go ahead
and set bit for CPU 8 also in cpumask ( td->bind_cpumask).
bind_to_cpumask function is called to set affinity using
sched_setaffinity and the cpumask. Since the CPU8 is not present,
set affinity will fail here with EINVAL. Fix this issue by adding a
check to make sure that, CPU's provided in the input argument values
are online before proceeding further and skip the test. For this,
include new helper function "is_cpu_online" in
"tools/perf/util/header.c".
Since "BIT(x)" definition will get included from header.h, remove
that from bench/numa.c
Tested-by: Disha Goel
Signed-off-by: Athira Rajeev
Reported-by: Disha Goel
---
Changelog
The first version was posted here:
https://lore.kernel.org/all/20220406175113.87881-5-atraj...@linux.vnet.ibm.com/
This v3 version separates patch 4 and addressing review comments
from Arnaldo to use sysfs__read_str for reading sysfs file.
tools/perf/bench/numa.c | 8 +--
tools/perf/util/header.c | 51
tools/perf/util/header.h | 1 +
3 files changed, 58 insertions(+), 2 deletions(-)
diff --git a/tools/perf/bench/numa.c b/tools/perf/bench/numa.c
index f2640179ada9..c838c248aa2c 100644
--- a/tools/perf/bench/numa.c
+++ b/tools/perf/bench/numa.c
@@ -34,6 +34,7 @@
#include
#include
+#include "../util/header.h"
#include
#include
@@ -585,6 +586,11 @@ static int parse_setup_cpu_list(void)
return -1;
}
+ if (is_cpu_online(bind_cpu_0) != 1 || is_cpu_online(bind_cpu_1)
!= 1) {
+ printf("\nTest not applicable, bind_cpu_0 or bind_cpu_1
is offline\n");
+ return -1;
+ }
+
BUG_ON(bind_cpu_0 < 0 || bind_cpu_1 < 0);
BUG_ON(bind_cpu_0 > bind_cpu_1);
@@ -752,8 +758,6 @@ static int parse_nodes_opt(const struct option *opt
__maybe_unused,
return parse_node_list(arg);
}
-#define BIT(x) (1ul << x)
-
static inline uint32_t lfsr_32(uint32_t lfsr)
{
const uint32_t taps = BIT(1) | BIT(5) | BIT(6) | BIT(31);
diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
index d546ff724dbe..a27132e5a5ef 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -983,6 +983,57 @@ static int write_dir_format(struct feat_fd *ff,
return do_write(ff, >dir.version, sizeof(data->dir.version));
}
+/*
+ * Check whether a CPU is online
+ *
+ * Returns:
+ * 1 -> if CPU is online
+ * 0 -> if CPU is offline
+ *-1 -> error case
+ */
+int is_cpu_online(unsigned int cpu)
+{
+ char *str;
+ size_t strlen;
+ char buf[256];
+ int status = -1;
+ struct stat statbuf;
+
+ snprintf(buf, sizeof(buf),
+ "/sys/devices/system/cpu/cpu%d", cpu);
+ if (stat(buf, ) != 0)
+ return 0;
+
+ /*
+* Check if /sys/devices/system/cpu/cpux/online file
+* exists. Some cases cpu0 won't have online file since
+* it is not expected to be turned off generally.
+* In kernels without CONFIG_HOTPLUG_CPU, this
+* file won't exist
+*/
+ snprintf(buf, sizeof(buf),
+ "/sys/devices/system/cpu/cpu%d/online", cpu);
+ if (stat(buf, ) != 0)
+ return 1;
+
+ /*
+* Read online file using sysfs__read_str.
+* If read or open fails, return -1.
+* If read succeeds, return value from file
+* which gets stored in "str"
+*/
+ snprintf(buf, sizeof(buf),
+ "devices/system/cpu/cpu%d/online", cpu);
+
+ if (sysfs__read_str(buf, , ) < 0)
+ return status;
+
+ status = atoi(str);
+
+ free(str);
+ return status;
+}
+
#ifdef HAVE_LIBBPF_SUPPORT
static int write_bpf_prog_info(struct feat_fd *ff,
struct evlist *evlist __maybe_unused)
diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
index c9e3265832d9..0eb4bc29a5a4 100644
--- a/tools/perf/util/header.h
+++ b/tools/perf/util/header.h
@@ -158,6 +158,7 @@ int do_write(struct feat_fd *fd, const void *buf, size_t
size);
int write_padded(struct feat_fd *fd, const void *bf,
[PATCH V3 0/2] Fix perf bench numa to work with machines having #CPUs > 1K
The perf benchmark for collections: numa hits failure in system configuration with CPU's more than 1024. These benchmarks uses "sched_getaffinity" and "sched_setaffinity" in the code to work with affinity. Example snippet from numa benchmark: <<>> perf: bench/numa.c:302: bind_to_node: Assertion `!(ret)' failed. Aborted (core dumped) <<>> bind_to_node function uses "sched_getaffinity" to save the cpumask. This fails with EINVAL because the default mask size in glibc is 1024 To overcome this 1024 CPUs mask size limitation of cpu_set_t, change the mask size using the CPU_*_S macros ie, use CPU_ALLOC to allocate cpumask, CPU_ALLOC_SIZE for size, CPU_SET_S to set mask bit. Fix all the relevant places in the code to use mask size which is large enough to represent number of possible CPU's in the system. This patchset also address a fix for parse_setup_cpu_list function in numa bench to check if input CPU is online before binding task to that CPU. This is to fix failures where, though CPU number is within max CPU, it could happen that CPU is offline. Here, sched_setaffinity will result in failure when using cpumask having that cpu bit set in the mask. Patch 1 address fix for parse_setup_cpu_list to check if CPU used to bind task is online. Patch 2 has fix for bench numa to work with machines having #CPUs > 1K Athira Rajeev (2): tools/perf: Fix perf bench numa testcase to check if CPU used to bind task is online perf bench: Fix numa bench to fix usage of affinity for machines with #CPUs > 1K Changelog: v2 -> v3 Link to the v2 version : https://lore.kernel.org/all/20220406175113.87881-1-atraj...@linux.vnet.ibm.com/ - From the v2 version, patch 1 and patch 2 are now part of upstream. - This v3 version separates patch 3 and patch 4 to address review comments from arnaldo which includes using sysfs__read_str for reading sysfs file and fixing the compilation issues observed in debian tools/perf/bench/numa.c | 136 +-- tools/perf/util/header.c | 51 +++ tools/perf/util/header.h | 1 + 3 files changed, 153 insertions(+), 35 deletions(-) -- 2.35.1
Re: [PATCH v2 0/4] Fix perf bench numa, futex and epoll to work with machines having #CPUs > 1K
> On 09-Apr-2022, at 10:48 PM, Arnaldo Carvalho de Melo wrote: > > Em Sat, Apr 09, 2022 at 12:28:01PM -0300, Arnaldo Carvalho de Melo escreveu: >> Em Wed, Apr 06, 2022 at 11:21:09PM +0530, Athira Rajeev escreveu: >>> The perf benchmark for collections: numa, futex and epoll >>> hits failure in system configuration with CPU's more than 1024. >>> These benchmarks uses "sched_getaffinity" and "sched_setaffinity" >>> in the code to work with affinity. >> >> Applied 1-3, 4 needs some reworking and can wait for v5.19, the first 3 >> are fixes, so can go now. > > Now trying to fix this: > > 26 7.89 debian:9 : FAIL gcc version 6.3.0 20170516 > (Debian 6.3.0-18+deb9u1) >bench/numa.c: In function 'alloc_data': >bench/numa.c:359:6: error: 'orig_mask' may be used uninitialized in this > function [-Werror=maybe-uninitialized] > ret = sched_setaffinity(0, size, mask); > ^~ >bench/numa.c:409:13: note: 'orig_mask' was declared here > cpu_set_t *orig_mask; > ^ >cc1: all warnings being treated as errors >/git/perf-5.18.0-rc1/tools/build/Makefile.build:139: recipe for target > 'bench' failed >make[3]: *** [bench] Error 2 > > > Happened in several distros. Hi Arnaldo Thanks for pointing it. I could be able to recreate this compile error in Debian. The reason for this issue is variable orig_mask which is used and initialised in “alloc_data" function within if condition for "init_cpu0”. We can fix this issue by initialising it to NULL since it is accessed conditionally. I also made some changes to CPU_FREE the mask in other error paths. I will post a V3 with these changes. Athira > > - Arnaldo
[powerpc:merge] BUILD SUCCESS 83d8a0d166119de813cad27ae7d61f54f9aea707
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge branch HEAD: 83d8a0d166119de813cad27ae7d61f54f9aea707 Automatic merge of 'master' into merge (2022-04-11 23:56) elapsed time: 1324m configs tested: 125 configs skipped: 3 The following configs have been built successfully. More configs may be tested in the coming days. gcc tested configs: arm defconfig arm allmodconfig arm allyesconfig arm64 defconfig arm64allyesconfig i386 randconfig-c001-20220411 armlart_defconfig arc tb10x_defconfig arm gemini_defconfig i386defconfig sh alldefconfig sh kfr2r09-romimage_defconfig arm imx_v6_v7_defconfig arm pxa3xx_defconfig m68k m5475evb_defconfig xtensa iss_defconfig parisc64 alldefconfig armrealview_defconfig sh sdk7780_defconfig powerpc ep8248e_defconfig powerpc bamboo_defconfig powerpc stx_gp3_defconfig mips decstation_64_defconfig powerpc mpc83xx_defconfig xtensa cadence_csp_defconfig sh rsk7203_defconfig mips cobalt_defconfig nios2alldefconfig ia64 gensparse_defconfig powerpc mpc837x_rdb_defconfig arm jornada720_defconfig openrisc alldefconfig x86_64 randconfig-c001-20220411 arm randconfig-c002-20220411 ia64 allmodconfig ia64 allyesconfig ia64defconfig m68k allyesconfig m68k allmodconfig m68kdefconfig nios2 defconfig arc allyesconfig cskydefconfig nios2allyesconfig alpha defconfig alphaallyesconfig h8300allyesconfig xtensa allyesconfig arc defconfig sh allmodconfig s390defconfig s390 allmodconfig parisc defconfig parisc64defconfig parisc allyesconfig s390 allyesconfig sparc defconfig i386 allyesconfig sparcallyesconfig i386 debian-10.3-kselftests i386 debian-10.3 mips allyesconfig mips allmodconfig powerpc allyesconfig powerpc allnoconfig powerpc allmodconfig x86_64 randconfig-a016-20220411 x86_64 randconfig-a012-20220411 x86_64 randconfig-a013-20220411 x86_64 randconfig-a014-20220411 x86_64 randconfig-a015-20220411 x86_64 randconfig-a011-20220411 i386 randconfig-a015-20220411 i386 randconfig-a011-20220411 i386 randconfig-a016-20220411 i386 randconfig-a012-20220411 i386 randconfig-a013-20220411 i386 randconfig-a014-20220411 riscvrandconfig-r042-20220411 arc randconfig-r043-20220411 s390 randconfig-r044-20220411 riscvallmodconfig riscvnommu_k210_defconfig riscvnommu_virt_defconfig riscv allnoconfig riscv rv32_defconfig riscvallyesconfig riscv defconfig um i386_defconfig um x86_64_defconfig x86_64 rhel-8.3-func x86_64 kexec x86_64 defconfig x86_64 allyesconfig x86_64rhel-8.3-kselftests x86_64 rhel-8.3-kunit x86_64 rhel-8.3 clang tested configs: powerpc randconfig-c003-20220411 arm randconfig-c002-20220411 x86_64
Re: rcu_sched self-detected stall on CPU
On Tue, Apr 12, 2022 at 04:53:06PM +1000, Michael Ellerman wrote:
> "Paul E. McKenney" writes:
> > On Sun, Apr 10, 2022 at 09:33:43PM +1000, Michael Ellerman wrote:
> >> Zhouyi Zhou writes:
> >> > On Fri, Apr 8, 2022 at 10:07 PM Paul E. McKenney
> >> > wrote:
> >> >> On Fri, Apr 08, 2022 at 06:02:19PM +0800, Zhouyi Zhou wrote:
> >> >> > On Fri, Apr 8, 2022 at 3:23 PM Michael Ellerman
> >> >> > wrote:
> >> ...
> >> >> > > I haven't seen it in my testing. But using Miguel's config I can
> >> >> > > reproduce it seemingly on every boot.
> >> >> > >
> >> >> > > For me it bisects to:
> >> >> > >
> >> >> > > 35de589cb879 ("powerpc/time: improve decrementer clockevent
> >> >> > > processing")
> >> >> > >
> >> >> > > Which seems plausible.
> >> >> > I also bisect to 35de589cb879 ("powerpc/time: improve decrementer
> >> >> > clockevent processing")
> >> ...
> >> >>
> >> >> > > Reverting that on mainline makes the bug go away.
> >>
> >> >> > I also revert that on the mainline, and am currently doing a pressure
> >> >> > test (by repeatedly invoking qemu and checking the console.log) on PPC
> >> >> > VM in Oregon State University.
> >>
> >> > After 306 rounds of stress test on mainline without triggering the bug
> >> > (last for 4 hours and 27 minutes), I think the bug is indeed caused by
> >> > 35de589cb879 ("powerpc/time: improve decrementer clockevent
> >> > processing") and stop the test for now.
> >>
> >> Thanks for testing, that's pretty conclusive.
> >>
> >> I'm not inclined to actually revert it yet.
> >>
> >> We need to understand if there's actually a bug in the patch, or if it's
> >> just exposing some existing bug/bad behavior we have. The fact that it
> >> only appears with CONFIG_HIGH_RES_TIMERS=n is suspicious.
> >>
> >> Do we have some code that inadvertently relies on something enabled by
> >> HIGH_RES_TIMERS=y, or do we have a bug that is hidden by HIGH_RES_TIMERS=y
> >> ?
> >
> > For whatever it is worth, moderate rcutorture runs to completion without
> > errors with CONFIG_HIGH_RES_TIMERS=n on 64-bit x86.
>
> Thanks for testing that, I don't have any big x86 machines to test on :)
>
> > Also for whatever it is worth, I don't know of anything other than
> > microcontrollers or the larger IoT devices that would want their kernels
> > built with CONFIG_HIGH_RES_TIMERS=n. Which might be a failure of
> > imagination on my part, but so it goes.
>
> Yeah I agree, like I said before I wasn't even aware you could turn it
> off. So I think we'll definitely add a select HIGH_RES_TIMERS in future,
> but first I need to work out why we are seeing stalls with it disabled.
Good point, and fair enough!
Thanx, Paul
Re: [PATCH net-next v2 05/18] net: dsa: mv88e6xxx: remove redundant check in mv88e6xxx_port_vlan()
On Tue, 2022-04-12 at 13:37 +0200, Jakob Koschel wrote: > > > On 12. Apr 2022, at 13:27, Russell King (Oracle) > > wrote: > > > > On Tue, Apr 12, 2022 at 12:58:17PM +0200, Jakob Koschel wrote: > > > We know that "dev > dst->last_switch" in the "else" block. > > > In other words, that "dev - dst->last_switch" is > 0. > > > > > > dsa_port_bridge_num_get(dp) can be 0, but the check > > > "if (bridge_num + dst->last_switch != dev) continue", rewritten as > > > "if (bridge_num != dev - dst->last_switch) continue", aka > > > "if (bridge_num != something which cannot be 0) continue", > > > makes it redundant to have the extra "if (!bridge_num) continue" logic, > > > since a bridge_num of zero would have been skipped anyway. > > > > > > Signed-off-by: Jakob Koschel > > > Signed-off-by: Vladimir Oltean > > > > Isn't this Vladimir's patch? > > > > If so, it should be commited as Vladimir as the author, and Vladimir's > > sign-off should appear before yours. When sending out such patches, > > there should be a From: line for Vladimir as the first line in the body > > of the patch email. > > yes, you are right. I wasn't sure on how to send those commits, but > I guess if I just set the commit author correctly it should be fine. > > regarding the order: I thought I did it correctly doing bottom up but > I confused the order, wasn't on purpose. Sorry about that. > > I'll resend after verifying all the authors and sign-offs are correct. whoops, too late... Please, do wait at least 24h before reposting, as pointed out in the documentation: https://elixir.bootlin.com/linux/v5.18-rc2/source/Documentation/process/maintainer-netdev.rst#L148 Thanks, Paolo
Re: [RFC][PATCH] net: fs_enet: fix tx error handling
Le 17/03/2022 à 16:38, Mans Rullgard a écrit :
> In some cases, the TXE flag is apparently set without any error
> indication in the buffer descriptor status. When this happens, tx
> stalls until the tx_restart() function is called via the device
> watchdog which can take a long time.
Is there an errata from NXP about this ?
Did you report the issue to them ? What feedback did you get ?
>
> To fix this, check for TXE in the napi poll function and trigger a
> tx_restart() call as for errors reported in the buffer descriptor.
I'm not sure to understand. You change seems to do a lot more than that.
Especially it changes to location of the handling of errors. Previously
errors where handled in interrupt routine. Now it's handled in the napi
poll routine. You have to explain all that.
>
> This change makes the FCC based Ethernet controller on MPC82xx devices
> usable. It probably breaks the other modes (FEC, SCC) which I have no
> way of testing.
You should at least change mac-scc.h and mac-fec.h to match the changes
you did in mac-fcc.h
This would allow me to at least test the FEC one.
>
> Signed-off-by: Mans Rullgard
> ---
> .../ethernet/freescale/fs_enet/fs_enet-main.c | 47 +++
> .../net/ethernet/freescale/fs_enet/mac-fcc.c | 2 +-
> 2 files changed, 19 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> index 78e008b81374..4276becd07cf 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> @@ -94,14 +94,22 @@ static int fs_enet_napi(struct napi_struct *napi, int
> budget)
> int curidx;
> int dirtyidx, do_wake, do_restart;
> int tx_left = TX_RING_SIZE;
> + u32 int_events;
>
> spin_lock(>tx_lock);
> bdp = fep->dirty_tx;
> + do_wake = do_restart = 0;
> +
> + int_events = (*fep->ops->get_int_events)(dev);
> +
> + if (int_events & fep->ev_err) {
> + (*fep->ops->ev_error)(dev, int_events);
> + do_restart = 1;
> + }
>
> /* clear status bits for napi*/
> (*fep->ops->napi_clear_event)(dev);
>
> - do_wake = do_restart = 0;
> while (((sc = CBDR_SC(bdp)) & BD_ENET_TX_READY) == 0 && tx_left) {
> dirtyidx = bdp - fep->tx_bd_base;
>
> @@ -318,43 +326,24 @@ fs_enet_interrupt(int irq, void *dev_id)
> {
> struct net_device *dev = dev_id;
> struct fs_enet_private *fep;
> - const struct fs_platform_info *fpi;
> u32 int_events;
> - u32 int_clr_events;
> - int nr, napi_ok;
> - int handled;
>
> fep = netdev_priv(dev);
> - fpi = fep->fpi;
>
> - nr = 0;
> - while ((int_events = (*fep->ops->get_int_events)(dev)) != 0) {
> - nr++;
> + int_events = (*fep->ops->get_int_events)(dev);
> + if (!int_events)
> + return IRQ_NONE;
>
> - int_clr_events = int_events;
> - int_clr_events &= ~fep->ev_napi;
> + int_events &= ~fep->ev_napi;
>
> - (*fep->ops->clear_int_events)(dev, int_clr_events);
> -
> - if (int_events & fep->ev_err)
> - (*fep->ops->ev_error)(dev, int_events);
> -
> - if (int_events & fep->ev) {
> - napi_ok = napi_schedule_prep(>napi);
> -
> - (*fep->ops->napi_disable)(dev);
> - (*fep->ops->clear_int_events)(dev, fep->ev_napi);
> -
> - /* NOTE: it is possible for FCCs in NAPI mode*/
> - /* to submit a spurious interrupt while in poll */
> - if (napi_ok)
> - __napi_schedule(>napi);
> - }
> + (*fep->ops->clear_int_events)(dev, int_events);
>
> + if (napi_schedule_prep(>napi)) {
> + (*fep->ops->napi_disable)(dev);
> + __napi_schedule(>napi);
> }
>
> - handled = nr > 0;
> - return IRQ_RETVAL(handled);
> + return IRQ_HANDLED;
> }
>
> void fs_init_bds(struct net_device *dev)
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> index b47490be872c..66c8f82a8333 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> @@ -124,7 +124,7 @@ static int do_pd_setup(struct fs_enet_private *fep)
> return ret;
> }
>
> -#define FCC_NAPI_EVENT_MSK (FCC_ENET_RXF | FCC_ENET_RXB | FCC_ENET_TXB)
> +#define FCC_NAPI_EVENT_MSK (FCC_ENET_RXF | FCC_ENET_RXB | FCC_ENET_TXB |
> FCC_ENET_TXE)
> #define FCC_EVENT (FCC_ENET_RXF | FCC_ENET_TXB)
> #define FCC_ERR_EVENT_MSK (FCC_ENET_TXE)
>
Re: [PATCH V5 7/7] mm/mmap: Drop arch_vm_get_page_pgprot()
Le 12/04/2022 à 06:38, Anshuman Khandual a écrit :
> There are no platforms left which use arch_vm_get_page_prot(). Just drop
> generic arch_vm_get_page_prot().
>
> Cc: Andrew Morton
> Cc: linux...@kvack.org
> Cc: linux-ker...@vger.kernel.org
> Reviewed-by: Catalin Marinas
> Signed-off-by: Anshuman Khandual
> ---
> include/linux/mman.h | 4
> mm/mmap.c| 3 +--
> 2 files changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/include/linux/mman.h b/include/linux/mman.h
> index b66e91b8176c..58b3abd457a3 100644
> --- a/include/linux/mman.h
> +++ b/include/linux/mman.h
> @@ -93,10 +93,6 @@ static inline void vm_unacct_memory(long pages)
> #define arch_calc_vm_flag_bits(flags) 0
> #endif
>
> -#ifndef arch_vm_get_page_prot
> -#define arch_vm_get_page_prot(vm_flags) __pgprot(0)
> -#endif
> -
> #ifndef arch_validate_prot
> /*
>* This is called from mprotect(). PROT_GROWSDOWN and PROT_GROWSUP have
> diff --git a/mm/mmap.c b/mm/mmap.c
> index edf2a5e38f4d..db7f33154206 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -110,8 +110,7 @@ pgprot_t protection_map[16] __ro_after_init = {
> pgprot_t vm_get_page_prot(unsigned long vm_flags)
> {
> pgprot_t ret = __pgprot(pgprot_val(protection_map[vm_flags &
> - (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
> - pgprot_val(arch_vm_get_page_prot(vm_flags)));
> + (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]));
>
> return ret;
> }
__pgprot(pgprot_val(x)) is a no-op.
You can simply do:
return protection_map[vm_flags &
(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED);
Re: [PATCH V5 6/7] mm/mmap: Drop arch_filter_pgprot()
Le 12/04/2022 à 06:38, Anshuman Khandual a écrit :
> There are no platforms left which subscribe ARCH_HAS_FILTER_PGPROT. Hence
> drop generic arch_filter_pgprot() and also config ARCH_HAS_FILTER_PGPROT.
>
> Cc: Andrew Morton
> Cc: linux...@kvack.org
> Cc: linux-ker...@vger.kernel.org
> Reviewed-by: Catalin Marinas
> Signed-off-by: Anshuman Khandual
> ---
> mm/Kconfig | 3 ---
> mm/mmap.c | 9 +
> 2 files changed, 1 insertion(+), 11 deletions(-)
>
> diff --git a/mm/Kconfig b/mm/Kconfig
> index b1f7624276f8..3f7b6d7b69df 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -762,9 +762,6 @@ config ARCH_HAS_CURRENT_STACK_POINTER
> register alias named "current_stack_pointer", this config can be
> selected.
>
> -config ARCH_HAS_FILTER_PGPROT
> - bool
> -
> config ARCH_HAS_VM_GET_PAGE_PROT
> bool
>
> diff --git a/mm/mmap.c b/mm/mmap.c
> index 87cb2eaf7e1a..edf2a5e38f4d 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -107,20 +107,13 @@ pgprot_t protection_map[16] __ro_after_init = {
> };
>
> #ifndef CONFIG_ARCH_HAS_VM_GET_PAGE_PROT
> -#ifndef CONFIG_ARCH_HAS_FILTER_PGPROT
> -static inline pgprot_t arch_filter_pgprot(pgprot_t prot)
> -{
> - return prot;
> -}
> -#endif
> -
> pgprot_t vm_get_page_prot(unsigned long vm_flags)
> {
> pgprot_t ret = __pgprot(pgprot_val(protection_map[vm_flags &
> (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
> pgprot_val(arch_vm_get_page_prot(vm_flags)));
>
> - return arch_filter_pgprot(ret);
> + return ret;
You can drop 'ret' and directly do:
return __pgprot(pgprot_val(protection_map[vm_flags &
(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
pgprot_val(arch_vm_get_page_prot(vm_flags)));
> }
> EXPORT_SYMBOL(vm_get_page_prot);
> #endif /* CONFIG_ARCH_HAS_VM_GET_PAGE_PROT */
Re: [PATCH V5 2/7] powerpc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
Le 12/04/2022 à 06:38, Anshuman Khandual a écrit :
> This defines and exports a platform specific custom vm_get_page_prot() via
> subscribing ARCH_HAS_VM_GET_PAGE_PROT. While here, this also localizes
> arch_vm_get_page_prot() as __vm_get_page_prot() and moves it near
> vm_get_page_prot().
>
> Cc: Michael Ellerman
> Cc: Paul Mackerras
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-ker...@vger.kernel.org
> Signed-off-by: Anshuman Khandual
> ---
> arch/powerpc/Kconfig | 1 +
> arch/powerpc/include/asm/mman.h| 12
> arch/powerpc/mm/book3s64/pgtable.c | 20
> 3 files changed, 21 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index 174edabb74fa..69e44358a235 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -140,6 +140,7 @@ config PPC
> select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
> select ARCH_HAS_UACCESS_FLUSHCACHE
> select ARCH_HAS_UBSAN_SANITIZE_ALL
> + select ARCH_HAS_VM_GET_PAGE_PROTif PPC_BOOK3S_64
> select ARCH_HAVE_NMI_SAFE_CMPXCHG
> select ARCH_KEEP_MEMBLOCK
> select ARCH_MIGHT_HAVE_PC_PARPORT
> diff --git a/arch/powerpc/include/asm/mman.h b/arch/powerpc/include/asm/mman.h
> index 7cb6d18f5cd6..1b024e64c8ec 100644
> --- a/arch/powerpc/include/asm/mman.h
> +++ b/arch/powerpc/include/asm/mman.h
> @@ -24,18 +24,6 @@ static inline unsigned long
> arch_calc_vm_prot_bits(unsigned long prot,
> }
> #define arch_calc_vm_prot_bits(prot, pkey) arch_calc_vm_prot_bits(prot,
> pkey)
>
> -static inline pgprot_t arch_vm_get_page_prot(unsigned long vm_flags)
> -{
> -#ifdef CONFIG_PPC_MEM_KEYS
> - return (vm_flags & VM_SAO) ?
> - __pgprot(_PAGE_SAO | vmflag_to_pte_pkey_bits(vm_flags)) :
> - __pgprot(0 | vmflag_to_pte_pkey_bits(vm_flags));
> -#else
> - return (vm_flags & VM_SAO) ? __pgprot(_PAGE_SAO) : __pgprot(0);
> -#endif
> -}
> -#define arch_vm_get_page_prot(vm_flags) arch_vm_get_page_prot(vm_flags)
> -
> static inline bool arch_validate_prot(unsigned long prot, unsigned long
> addr)
> {
> if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
> diff --git a/arch/powerpc/mm/book3s64/pgtable.c
> b/arch/powerpc/mm/book3s64/pgtable.c
> index 052e6590f84f..d0319524e27f 100644
> --- a/arch/powerpc/mm/book3s64/pgtable.c
> +++ b/arch/powerpc/mm/book3s64/pgtable.c
> @@ -7,6 +7,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -549,3 +550,22 @@ unsigned long memremap_compat_align(void)
> }
> EXPORT_SYMBOL_GPL(memremap_compat_align);
> #endif
> +
> +static pgprot_t __vm_get_page_prot(unsigned long vm_flags)
> +{
> +#ifdef CONFIG_PPC_MEM_KEYS
> + return (vm_flags & VM_SAO) ?
> + __pgprot(_PAGE_SAO | vmflag_to_pte_pkey_bits(vm_flags)) :
> + __pgprot(0 | vmflag_to_pte_pkey_bits(vm_flags));
> +#else
> + return (vm_flags & VM_SAO) ? __pgprot(_PAGE_SAO) : __pgprot(0);
> +#endif
> +}
> +
> +pgprot_t vm_get_page_prot(unsigned long vm_flags)
> +{
> + return __pgprot(pgprot_val(protection_map[vm_flags &
> + (VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]) |
> + pgprot_val(__vm_get_page_prot(vm_flags)));
> +}
> +EXPORT_SYMBOL(vm_get_page_prot);
This looks functionnaly OK, but I think we could go in the same
direction as ARM and try to integrate __vm_get_page_prot() inside
vm_get_page_prot() to get something simpler and cleaner:
Something like below (untested):
pgprot_t vm_get_page_prot(unsigned long vm_flags)
{
unsigned long prot = pgprot_val(protection_map[vm_flags &
(VM_READ|VM_WRITE|VM_EXEC|VM_SHARED)]);
if (vm_flags & VM_SAO)
prot |= _PAGE_SAO
#ifdef CONFIG_PPC_MEM_KEYS
prot |= vmflag_to_pte_pkey_bits(vm_flags);
#endif
return __pgprot(prot);
}
[PATCH net-next v3 18/18] team: Remove use of list iterator variable for list_for_each_entry_from()
In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].
Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
To either continue iterating from that position or skip the iteration
(if the previous iteration was complete) list_prepare_entry() is used.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/team/team.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b07dde6f0abf..688c4393f099 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2425,17 +2425,17 @@ static int team_nl_send_options_get(struct team *team,
u32 portid, u32 seq,
int flags, team_nl_send_func_t *send_func,
struct list_head *sel_opt_inst_list)
{
+ struct team_option_inst *opt_inst, *tmp = NULL;
struct nlattr *option_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_option_inst *opt_inst;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- opt_inst = list_first_entry(sel_opt_inst_list,
- struct team_option_inst, tmp_list);
+ tmp = list_first_entry(sel_opt_inst_list,
+ struct team_option_inst, tmp_list);
start_again:
err = __send_and_alloc_skb(, team, portid, send_func);
@@ -2456,7 +2456,9 @@ static int team_nl_send_options_get(struct team *team,
u32 portid, u32 seq,
goto nla_put_failure;
i = 0;
+ opt_inst = list_prepare_entry(tmp, sel_opt_inst_list, tmp_list);
incomplete = false;
+ tmp = NULL;
list_for_each_entry_from(opt_inst, sel_opt_inst_list, tmp_list) {
err = team_nl_fill_one_option_get(skb, team, opt_inst);
if (err) {
@@ -2464,6 +2466,7 @@ static int team_nl_send_options_get(struct team *team,
u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = opt_inst;
break;
}
goto errout;
@@ -2707,14 +2710,14 @@ static int team_nl_send_port_list_get(struct team
*team, u32 portid, u32 seq,
struct nlattr *port_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_port *port;
+ struct team_port *port, *tmp = NULL;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- port = list_first_entry_or_null(>port_list,
- struct team_port, list);
+ tmp = list_first_entry_or_null(>port_list,
+ struct team_port, list);
start_again:
err = __send_and_alloc_skb(, team, portid, send_func);
@@ -2744,7 +2747,9 @@ static int team_nl_send_port_list_get(struct team *team,
u32 portid, u32 seq,
err = team_nl_fill_one_port_get(skb, one_port);
if (err)
goto errout;
- } else if (port) {
+ } else {
+ port = list_prepare_entry(tmp, >port_list, list);
+ tmp = NULL;
list_for_each_entry_from(port, >port_list, list) {
err = team_nl_fill_one_port_get(skb, port);
if (err) {
@@ -2752,6 +2757,7 @@ static int team_nl_send_port_list_get(struct team *team,
u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = port;
break;
}
goto errout;
--
2.25.1
[PATCH net-next v3 17/18] ipvlan: Remove usage of list iterator variable for the loop body
In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].
Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
To either continue iterating from that position or start a new
iteration (if the previous iteration was complete) list_prepare_entry()
is used.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ipvlan/ipvlan_main.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 696e245f6d00..063d7c30e944 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -9,7 +9,7 @@
static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
struct netlink_ext_ack *extack)
{
- struct ipvl_dev *ipvlan;
+ struct ipvl_dev *ipvlan, *tmp = NULL;
unsigned int flags;
int err;
@@ -26,8 +26,10 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16
nval,
flags & ~IFF_NOARP,
extack);
}
- if (unlikely(err))
+ if (unlikely(err)) {
+ tmp = ipvlan;
goto fail;
+ }
}
if (nval == IPVLAN_MODE_L3S) {
/* New mode is L3S */
@@ -43,6 +45,7 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16
nval,
return 0;
fail:
+ ipvlan = list_prepare_entry(tmp, >ipvlans, pnode);
/* Undo the flags changes that have been done so far. */
list_for_each_entry_continue_reverse(ipvlan, >ipvlans, pnode) {
flags = ipvlan->dev->flags;
--
2.25.1
[PATCH net-next v3 16/18] ps3_gelic: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
.../net/ethernet/toshiba/ps3_gelic_wireless.c | 30 +--
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
index dc14a66583ff..c8a016c902cd 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
@@ -1495,14 +1495,14 @@ static int gelic_wl_start_scan(struct gelic_wl_info
*wl, int always_scan,
*/
static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
{
+ struct gelic_wl_scan_info *target = NULL, *iter, *tmp;
struct gelic_eurus_cmd *cmd = NULL;
- struct gelic_wl_scan_info *target, *tmp;
struct gelic_wl_scan_info *oldest = NULL;
struct gelic_eurus_scan_info *scan_info;
unsigned int scan_info_size;
union iwreq_data data;
unsigned long this_time = jiffies;
- unsigned int data_len, i, found, r;
+ unsigned int data_len, i, r;
void *buf;
pr_debug("%s:start\n", __func__);
@@ -1539,14 +1539,14 @@ static void gelic_wl_scan_complete_event(struct
gelic_wl_info *wl)
wl->scan_stat = GELIC_WL_SCAN_STAT_GOT_LIST;
/* mark all entries are old */
- list_for_each_entry_safe(target, tmp, >network_list, list) {
- target->valid = 0;
+ list_for_each_entry_safe(iter, tmp, >network_list, list) {
+ iter->valid = 0;
/* expire too old entries */
- if (time_before(target->last_scanned + wl->scan_age,
+ if (time_before(iter->last_scanned + wl->scan_age,
this_time)) {
- kfree(target->hwinfo);
- target->hwinfo = NULL;
- list_move_tail(>list, >network_free_list);
+ kfree(iter->hwinfo);
+ iter->hwinfo = NULL;
+ list_move_tail(>list, >network_free_list);
}
}
@@ -1569,22 +1569,22 @@ static void gelic_wl_scan_complete_event(struct
gelic_wl_info *wl)
continue;
}
- found = 0;
+ target = NULL;
oldest = NULL;
- list_for_each_entry(target, >network_list, list) {
- if (ether_addr_equal(>hwinfo->bssid[2],
+ list_for_each_entry(iter, >network_list, list) {
+ if (ether_addr_equal(>hwinfo->bssid[2],
_info->bssid[2])) {
- found = 1;
+ target = iter;
pr_debug("%s: same BBS found scanned list\n",
__func__);
break;
}
if (!oldest ||
- (target->last_scanned < oldest->last_scanned))
- oldest = target;
+ (iter->last_scanned < oldest->last_scanned))
+ oldest = iter;
}
- if (!found) {
+ if (!target) {
/* not found in the list */
if (list_empty(>network_free_list)) {
/* expire oldest */
--
2.25.1
[PATCH net-next v3 15/18] net: netcp: Remove usage of list iterator for list_add() after loop body
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer pointing to the location
where the element should be inserted [1].
Before, the code implicitly used the head when no element was found
when using >list. The new 'pos' variable is set to the list head
by default and overwritten if the list exits early, marking the
insertion point for list_add().
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/ti/netcp_core.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/ti/netcp_core.c
b/drivers/net/ethernet/ti/netcp_core.c
index 16507bff652a..f25104b5a31b 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -471,6 +471,7 @@ struct netcp_hook_list {
int netcp_register_txhook(struct netcp_intf *netcp_priv, int order,
netcp_hook_rtn *hook_rtn, void *hook_data)
{
+ struct list_head *pos = _priv->txhook_list_head;
struct netcp_hook_list *entry;
struct netcp_hook_list *next;
unsigned long flags;
@@ -485,10 +486,12 @@ int netcp_register_txhook(struct netcp_intf *netcp_priv,
int order,
spin_lock_irqsave(_priv->lock, flags);
list_for_each_entry(next, _priv->txhook_list_head, list) {
- if (next->order > order)
+ if (next->order > order) {
+ pos = >list;
break;
+ }
}
- __list_add(>list, next->list.prev, >list);
+ list_add_tail(>list, pos);
spin_unlock_irqrestore(_priv->lock, flags);
return 0;
@@ -520,6 +523,7 @@ EXPORT_SYMBOL_GPL(netcp_unregister_txhook);
int netcp_register_rxhook(struct netcp_intf *netcp_priv, int order,
netcp_hook_rtn *hook_rtn, void *hook_data)
{
+ struct list_head *pos = _priv->rxhook_list_head;
struct netcp_hook_list *entry;
struct netcp_hook_list *next;
unsigned long flags;
@@ -534,10 +538,12 @@ int netcp_register_rxhook(struct netcp_intf *netcp_priv,
int order,
spin_lock_irqsave(_priv->lock, flags);
list_for_each_entry(next, _priv->rxhook_list_head, list) {
- if (next->order > order)
+ if (next->order > order) {
+ pos = >list;
break;
+ }
}
- __list_add(>list, next->list.prev, >list);
+ list_add_tail(>list, pos);
spin_unlock_irqrestore(_priv->lock, flags);
return 0;
--
2.25.1
[PATCH net-next v3 14/18] sfc: Remove usage of list iterator for list_add() after the loop body
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer pointing to the location
where the element should be inserted [1].
Before, the code implicitly used the head when no element was found
when using >list. The new 'pos' variable is set to the list head
by default and overwritten if the list exits early, marking the
insertion point for list_add().
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/sfc/rx_common.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sfc/rx_common.c
b/drivers/net/ethernet/sfc/rx_common.c
index 1b22c7be0088..80894a35ea79 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -555,7 +555,7 @@ efx_rx_packet_gro(struct efx_channel *channel, struct
efx_rx_buffer *rx_buf,
*/
struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
{
- struct list_head *head = >rss_context.list;
+ struct list_head *head = *pos = >rss_context.list;
struct efx_rss_context *ctx, *new;
u32 id = 1; /* Don't use zero, that refers to the master RSS context */
@@ -563,8 +563,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct
efx_nic *efx)
/* Search for first gap in the numbering */
list_for_each_entry(ctx, head, list) {
- if (ctx->user_id != id)
+ if (ctx->user_id != id) {
+ pos = >list;
break;
+ }
id++;
/* Check for wrap. If this happens, we have nearly 2^32
* allocated RSS contexts, which seems unlikely.
@@ -582,7 +584,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct
efx_nic *efx)
/* Insert the new entry into the gap */
new->user_id = id;
- list_add_tail(>list, >list);
+ list_add_tail(>list, pos);
return new;
}
--
2.25.1
[PATCH net-next v3 13/18] net: qede: Remove check of list iterator against head past the loop body
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
>member == head, using the iterator variable after the loop should
be avoided.
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 3010833ddde3..3d167e37e654 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -829,18 +829,21 @@ int qede_configure_vlan_filters(struct qede_dev *edev)
int qede_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
{
struct qede_dev *edev = netdev_priv(dev);
- struct qede_vlan *vlan;
+ struct qede_vlan *vlan = NULL;
+ struct qede_vlan *iter;
int rc = 0;
DP_VERBOSE(edev, NETIF_MSG_IFDOWN, "Removing vlan 0x%04x\n", vid);
/* Find whether entry exists */
__qede_lock(edev);
- list_for_each_entry(vlan, >vlan_list, list)
- if (vlan->vid == vid)
+ list_for_each_entry(iter, >vlan_list, list)
+ if (iter->vid == vid) {
+ vlan = iter;
break;
+ }
- if (list_entry_is_head(vlan, >vlan_list, list)) {
+ if (!vlan) {
DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
"Vlan isn't configured\n");
goto out;
--
2.25.1
[PATCH net-next v3 12/18] net: qede: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qede/qede_rdma.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_rdma.c
b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
index 6304514a6f2c..2eb03ffe2484 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_rdma.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
@@ -246,18 +246,17 @@ static void qede_rdma_change_mtu(struct qede_dev *edev)
static struct qede_rdma_event_work *
qede_rdma_get_free_event_node(struct qede_dev *edev)
{
- struct qede_rdma_event_work *event_node = NULL;
- bool found = false;
+ struct qede_rdma_event_work *event_node = NULL, *iter;
- list_for_each_entry(event_node, >rdma_info.rdma_event_list,
+ list_for_each_entry(iter, >rdma_info.rdma_event_list,
list) {
- if (!work_pending(_node->work)) {
- found = true;
+ if (!work_pending(>work)) {
+ event_node = iter;
break;
}
}
- if (!found) {
+ if (!event_node) {
event_node = kzalloc(sizeof(*event_node), GFP_ATOMIC);
if (!event_node) {
DP_NOTICE(edev,
--
2.25.1
[PATCH net-next v3 11/18] qed: Remove usage of list iterator variable after the loop
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
Since "found" and "p_ent" need to be equal, "found" should be used
consistently to limit the scope of "p_ent" to the list traversal in
the future.
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qed/qed_spq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c
b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index d01b9245f811..cbaa2abed660 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -934,10 +934,10 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
u8 fw_return_code,
union event_ring_data *p_data)
{
+ struct qed_spq_entry*found = NULL;
struct qed_spq *p_spq;
- struct qed_spq_entry*p_ent = NULL;
+ struct qed_spq_entry*p_ent;
struct qed_spq_entry*tmp;
- struct qed_spq_entry*found = NULL;
if (!p_hwfn)
return -EINVAL;
@@ -980,7 +980,7 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
DP_VERBOSE(p_hwfn, QED_MSG_SPQ,
"Complete EQE [echo %04x]: func %p cookie %p)\n",
le16_to_cpu(echo),
- p_ent->comp_cb.function, p_ent->comp_cb.cookie);
+ found->comp_cb.function, found->comp_cb.cookie);
if (found->comp_cb.function)
found->comp_cb.function(p_hwfn, found->comp_cb.cookie, p_data,
fw_return_code);
--
2.25.1
[PATCH net-next v3 10/18] qed: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 26 ++---
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index 1d1d4caad680..198c9321bf51 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -1630,38 +1630,36 @@ static struct qed_iwarp_listener *
qed_iwarp_get_listener(struct qed_hwfn *p_hwfn,
struct qed_iwarp_cm_info *cm_info)
{
- struct qed_iwarp_listener *listener = NULL;
+ struct qed_iwarp_listener *listener = NULL, *iter;
static const u32 ip_zero[4] = { 0, 0, 0, 0 };
- bool found = false;
- list_for_each_entry(listener,
+ list_for_each_entry(iter,
_hwfn->p_rdma_info->iwarp.listen_list,
list_entry) {
- if (listener->port == cm_info->local_port) {
- if (!memcmp(listener->ip_addr,
+ if (iter->port == cm_info->local_port) {
+ if (!memcmp(iter->ip_addr,
ip_zero, sizeof(ip_zero))) {
- found = true;
+ listener = iter;
break;
}
- if (!memcmp(listener->ip_addr,
+ if (!memcmp(iter->ip_addr,
cm_info->local_ip,
sizeof(cm_info->local_ip)) &&
- (listener->vlan == cm_info->vlan)) {
- found = true;
+ iter->vlan == cm_info->vlan) {
+ listener = iter;
break;
}
}
}
- if (found) {
+ if (listener)
DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener found = %p\n",
listener);
- return listener;
- }
+ else
+ DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- return NULL;
+ return listener;
}
static int
--
2.25.1
[PATCH net-next v3 09/18] qed: Use dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qed/qed_dev.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c
b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 672480c9d195..e920e7dcf66a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -174,7 +174,7 @@ int qed_db_recovery_add(struct qed_dev *cdev,
int qed_db_recovery_del(struct qed_dev *cdev,
void __iomem *db_addr, void *db_data)
{
- struct qed_db_recovery_entry *db_entry = NULL;
+ struct qed_db_recovery_entry *db_entry = NULL, *iter;
struct qed_hwfn *p_hwfn;
int rc = -EINVAL;
@@ -190,12 +190,13 @@ int qed_db_recovery_del(struct qed_dev *cdev,
/* Protect the list */
spin_lock_bh(_hwfn->db_recovery_info.lock);
- list_for_each_entry(db_entry,
+ list_for_each_entry(iter,
_hwfn->db_recovery_info.list, list_entry) {
/* search according to db_data addr since db_addr is not unique
(roce) */
- if (db_entry->db_data == db_data) {
- qed_db_recovery_dp_entry(p_hwfn, db_entry, "Deleting");
- list_del(_entry->list_entry);
+ if (iter->db_data == db_data) {
+ qed_db_recovery_dp_entry(p_hwfn, iter, "Deleting");
+ list_del(>list_entry);
+ db_entry = iter;
rc = 0;
break;
}
--
2.25.1
[PATCH net-next v3 08/18] net: sparx5: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
.../microchip/sparx5/sparx5_mactable.c| 25 +--
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
index a5837dbe0c7e..bb8d9ce79ac2 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
@@ -362,8 +362,7 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
unsigned char mac[ETH_ALEN],
u16 vid, u32 cfg2)
{
- struct sparx5_mact_entry *mact_entry;
- bool found = false;
+ struct sparx5_mact_entry *mact_entry = NULL, *iter;
u16 port;
if (LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_TYPE_GET(cfg2) !=
@@ -378,28 +377,28 @@ static void sparx5_mact_handle_entry(struct sparx5
*sparx5,
return;
mutex_lock(>mact_lock);
- list_for_each_entry(mact_entry, >mact_entries, list) {
- if (mact_entry->vid == vid &&
- ether_addr_equal(mac, mact_entry->mac)) {
- found = true;
- mact_entry->flags |= MAC_ENT_ALIVE;
- if (mact_entry->port != port) {
+ list_for_each_entry(iter, >mact_entries, list) {
+ if (iter->vid == vid &&
+ ether_addr_equal(mac, iter->mac)) {
+ iter->flags |= MAC_ENT_ALIVE;
+ if (iter->port != port) {
dev_warn(sparx5->dev, "Entry move: %d -> %d\n",
-mact_entry->port, port);
- mact_entry->port = port;
- mact_entry->flags |= MAC_ENT_MOVED;
+iter->port, port);
+ iter->port = port;
+ iter->flags |= MAC_ENT_MOVED;
}
/* Entry handled */
+ mact_entry = iter;
break;
}
}
mutex_unlock(>mact_lock);
- if (found && !(mact_entry->flags & MAC_ENT_MOVED))
+ if (mact_entry && !(mact_entry->flags & MAC_ENT_MOVED))
/* Present, not moved */
return;
- if (!found) {
+ if (!mact_entry) {
/* Entry not found - now add */
mact_entry = alloc_mact_entry(sparx5, mac, vid, port);
if (!mact_entry)
--
2.25.1
[PATCH net-next v3 07/18] net: dsa: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
net/dsa/dsa.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 89c6c86e746f..645522c4dd4a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -112,22 +112,21 @@ const struct dsa_device_ops
*dsa_find_tagger_by_name(const char *buf)
const struct dsa_device_ops *dsa_tag_driver_get(int tag_protocol)
{
- struct dsa_tag_driver *dsa_tag_driver;
+ struct dsa_tag_driver *dsa_tag_driver = NULL, *iter;
const struct dsa_device_ops *ops;
- bool found = false;
request_module("%s%d", DSA_TAG_DRIVER_ALIAS, tag_protocol);
mutex_lock(_tag_drivers_lock);
- list_for_each_entry(dsa_tag_driver, _tag_drivers_list, list) {
- ops = dsa_tag_driver->ops;
+ list_for_each_entry(iter, _tag_drivers_list, list) {
+ ops = iter->ops;
if (ops->proto == tag_protocol) {
- found = true;
+ dsa_tag_driver = iter;
break;
}
}
- if (found) {
+ if (dsa_tag_driver) {
if (!try_module_get(dsa_tag_driver->owner))
ops = ERR_PTR(-ENOPROTOOPT);
} else {
--
2.25.1
[PATCH net-next v3 06/18] net: dsa: mv88e6xxx: refactor mv88e6xxx_port_vlan()
From: Vladimir Oltean
To avoid bugs and speculative execution exploits due to type-confused
pointers at the end of a list_for_each_entry() loop, one measure is to
restrict code to not use the iterator variable outside the loop block.
In the case of mv88e6xxx_port_vlan(), this isn't a problem, as we never
let the loops exit through "natural causes" anyway, by using a "found"
variable and then using the last "dp" iterator prior to the break, which
is a safe thing to do.
Nonetheless, with the expected new syntax, this pattern will no longer
be possible.
Profit off of the occasion and break the two port finding methods into
smaller sub-functions. Somehow, returning a copy of the iterator pointer
is still accepted.
This change makes it redundant to have a "bool found", since the "dp"
from mv88e6xxx_port_vlan() now holds NULL if we haven't found what we
were looking for.
Signed-off-by: Vladimir Oltean
Signed-off-by: Jakob Koschel
---
drivers/net/dsa/mv88e6xxx/chip.c | 54 ++--
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b3aa0e5bc842..1f35e89053e6 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1378,42 +1378,50 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds,
int port,
return 0;
}
+static struct dsa_port *mv88e6xxx_find_port(struct dsa_switch_tree *dst,
+ int sw_index, int port)
+{
+ struct dsa_port *dp;
+
+ list_for_each_entry(dp, >ports, list)
+ if (dp->ds->index == sw_index && dp->index == port)
+ return dp;
+
+ return NULL;
+}
+
+static struct dsa_port *
+mv88e6xxx_find_port_by_bridge_num(struct dsa_switch_tree *dst,
+ unsigned int bridge_num)
+{
+ struct dsa_port *dp;
+
+ list_for_each_entry(dp, >ports, list)
+ if (dsa_port_bridge_num_get(dp) == bridge_num)
+ return dp;
+
+ return NULL;
+}
+
/* Mask of the local ports allowed to receive frames from a given fabric port
*/
static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
{
struct dsa_switch *ds = chip->ds;
struct dsa_switch_tree *dst = ds->dst;
struct dsa_port *dp, *other_dp;
- bool found = false;
u16 pvlan;
- /* dev is a physical switch */
if (dev <= dst->last_switch) {
- list_for_each_entry(dp, >ports, list) {
- if (dp->ds->index == dev && dp->index == port) {
- /* dp might be a DSA link or a user port, so it
-* might or might not have a bridge.
-* Use the "found" variable for both cases.
-*/
- found = true;
- break;
- }
- }
- /* dev is a virtual bridge */
+ /* dev is a physical switch */
+ dp = mv88e6xxx_find_port(dst, dev, port);
} else {
- list_for_each_entry(dp, >ports, list) {
- unsigned int bridge_num = dsa_port_bridge_num_get(dp);
-
- if (bridge_num + dst->last_switch != dev)
- continue;
-
- found = true;
- break;
- }
+ /* dev is a virtual bridge */
+ dp = mv88e6xxx_find_port_by_bridge_num(dst,
+ dev - dst->last_switch);
}
/* Prevent frames from unknown switch or virtual bridge */
- if (!found)
+ if (!dp)
return 0;
/* Frames from DSA links and CPU ports can egress any local port */
--
2.25.1
[PATCH net-next v3 05/18] net: dsa: mv88e6xxx: remove redundant check in mv88e6xxx_port_vlan()
From: Vladimir Oltean
We know that "dev > dst->last_switch" in the "else" block.
In other words, that "dev - dst->last_switch" is > 0.
dsa_port_bridge_num_get(dp) can be 0, but the check
"if (bridge_num + dst->last_switch != dev) continue", rewritten as
"if (bridge_num != dev - dst->last_switch) continue", aka
"if (bridge_num != something which cannot be 0) continue",
makes it redundant to have the extra "if (!bridge_num) continue" logic,
since a bridge_num of zero would have been skipped anyway.
Signed-off-by: Vladimir Oltean
Signed-off-by: Jakob Koschel
---
drivers/net/dsa/mv88e6xxx/chip.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd02902..b3aa0e5bc842 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1404,9 +1404,6 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip
*chip, int dev, int port)
list_for_each_entry(dp, >ports, list) {
unsigned int bridge_num = dsa_port_bridge_num_get(dp);
- if (!bridge_num)
- continue;
-
if (bridge_num + dst->last_switch != dev)
continue;
--
2.25.1
[PATCH net-next v3 03/18] net: dsa: sja1105: reorder sja1105_first_entry_longer_than with memory allocation
From: Vladimir Oltean
sja1105_first_entry_longer_than() does not make use of the full struct
sja1105_gate_entry *e, just of e->interval which is set from the passed
entry_time.
This means that if there is a gate conflict, we have allocated e for
nothing, just to free it later. Reorder the memory allocation and the
function call, to avoid that and simplify the error unwind path.
Signed-off-by: Vladimir Oltean
Signed-off-by: Jakob Koschel
---
drivers/net/dsa/sja1105/sja1105_vl.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
b/drivers/net/dsa/sja1105/sja1105_vl.c
index 369be2ac3587..e5ea8eb9ec4e 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -36,7 +36,11 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
{
struct sja1105_gate_entry *e;
struct list_head *pos;
- int rc;
+
+ pos = sja1105_first_entry_longer_than(_cfg->entries,
+ entry_time, extack);
+ if (IS_ERR(pos))
+ return PTR_ERR(pos);
e = kzalloc(sizeof(*e), GFP_KERNEL);
if (!e)
@@ -45,22 +49,11 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
e->rule = rule;
e->gate_state = gate_state;
e->interval = entry_time;
-
- pos = sja1105_first_entry_longer_than(_cfg->entries,
- e->interval, extack);
- if (IS_ERR(pos)) {
- rc = PTR_ERR(pos);
- goto err;
- }
-
list_add(>list, pos->prev);
gating_cfg->num_entries++;
return 0;
-err:
- kfree(e);
- return rc;
}
/* The gate entries contain absolute times in their e->interval field. Convert
--
2.25.1
[PATCH net-next v3 00/18] Remove use of list iterator after loop body
When the list iterator loop does not exit early the list iterator variable contains a type-confused pointer to a 'bogus' list element computed based on the head [1]. Often a 'found' variable is used to ensure the list iterator variable is only accessed after the loop body if the loop did exit early (using a break or goto). In other cases that list iterator variable is used in combination to access the list member which reverses the invocation of container_of() and brings back a "safe" pointer to the head of the list. Since, due to this code patten, there were quite a few bugs discovered [2], Linus concluded that the rule should be to never use the list iterator after the loop and introduce a dedicated pointer for that [3]. With the new gnu11 standard, it will now be possible to limit the scope of the list iterator variable to the traversal loop itself by defining the variable within the for loop. This, however, requires to remove all uses of the list iterator after the loop. Based on input from Paolo Abeni [4], Vinicius Costa Gomes [5], and Jakub Kicinski [6], I've splitted all the net-next related changes into two patch sets, where this is part 1. v2->v3: - fix commit authors and signed-off order regarding Vladimir's patches (Sorry about that, wasn't intentional.) v1->v2: - Fixed commit message for PATCH 14/18 and used dedicated variable pointing to the position (Edward Cree) - Removed redundant check in mv88e6xxx_port_vlan() (Vladimir Oltean) - Refactor mv88e6xxx_port_vlan() using separate list iterator functions (Vladimir Oltean) - Refactor sja1105_insert_gate_entry() to use separate list iterator functions (Vladimir Oltean) - Allow early return in sja1105_insert_gate_entry() if sja1105_first_entry_longer_than() didn't find any element (Vladimir Oltean) - Use list_add_tail() instead of list_add() in sja1105_insert_gate_entry() (Jakub Kicinski) - net: netcp: also use separate 'pos' variable instead of duplicating list_add() Link: https://lwn.net/Articles/887097/ [1] Link: https://lore.kernel.org/linux-kernel/20220217184829.1991035-4-jakobkosc...@gmail.com/ [2] Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/ [3] Link: https://lore.kernel.org/linux-kernel/7393b673c626fd75f2b4f8509faa5459254fb87c.ca...@redhat.com/ [4] Link: https://lore.kernel.org/linux-kernel/877d8a3sww@intel.com/ [5] Link: https://lore.kernel.org/linux-kernel/20220403205502.1b344...@kernel.org/ [6]
[PATCH net-next v3 04/18] net: dsa: sja1105: use list_add_tail(pos) instead of list_add(pos->prev)
From: Vladimir Oltean When passed a non-head list element, list_add_tail() actually adds the new element to its left, which is what we want. Despite the slightly confusing name, use the dedicated function which does the same thing as the open-coded list_add(pos->prev). Suggested-by: Jakub Kicinski Signed-off-by: Vladimir Oltean Signed-off-by: Jakob Koschel --- drivers/net/dsa/sja1105/sja1105_vl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c index e5ea8eb9ec4e..7fe9b18f1cbd 100644 --- a/drivers/net/dsa/sja1105/sja1105_vl.c +++ b/drivers/net/dsa/sja1105/sja1105_vl.c @@ -49,7 +49,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, e->rule = rule; e->gate_state = gate_state; e->interval = entry_time; - list_add(>list, pos->prev); + list_add_tail(>list, pos); gating_cfg->num_entries++; -- 2.25.1
[PATCH net-next v3 01/18] connector: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/connector/cn_queue.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 996f025eb63c..ed77599b0b25 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -92,20 +92,19 @@ int cn_queue_add_callback(struct cn_queue_dev *dev, const
char *name,
void cn_queue_del_callback(struct cn_queue_dev *dev, const struct cb_id *id)
{
- struct cn_callback_entry *cbq, *n;
- int found = 0;
+ struct cn_callback_entry *cbq = NULL, *iter, *n;
spin_lock_bh(>queue_lock);
- list_for_each_entry_safe(cbq, n, >queue_list, callback_entry) {
- if (cn_cb_equal(>id.id, id)) {
- list_del(>callback_entry);
- found = 1;
+ list_for_each_entry_safe(iter, n, >queue_list, callback_entry) {
+ if (cn_cb_equal(>id.id, id)) {
+ list_del(>callback_entry);
+ cbq = iter;
break;
}
}
spin_unlock_bh(>queue_lock);
- if (found)
+ if (cbq)
cn_queue_release_callback(cbq);
}
--
2.25.1
[PATCH net-next v3 02/18] net: dsa: sja1105: remove use of iterator after list_for_each_entry() loop
From: Vladimir Oltean
The link below explains that there is a desire to syntactically change
list_for_each_entry() and list_for_each() such that it becomes
impossible to use the iterator variable outside the scope of the loop.
Although sja1105_insert_gate_entry() makes legitimate use of the
iterator pointer when it breaks out, the pattern it uses may become
illegal, so it needs to change.
It is deemed acceptable to use a copy of the loop iterator, and
sja1105_insert_gate_entry() only needs to know the list_head element
before which the list insertion should be made. So let's profit from the
occasion and refactor the list iteration to a dedicated function.
An additional benefit is given by the fact that with the helper function
in place, we no longer need to special-case the empty list, since it is
equivalent to not having found any gating entry larger than the
specified interval in the list. We just need to insert at the tail of
that list (list_add vs list_add_tail on an empty list does the same
thing).
Link:
https://patchwork.kernel.org/project/netdevbpf/patch/20220407102900.3086255-3-jakobkosc...@gmail.com/#24810127
Signed-off-by: Vladimir Oltean
Signed-off-by: Jakob Koschel
---
drivers/net/dsa/sja1105/sja1105_vl.c | 46 ++--
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
b/drivers/net/dsa/sja1105/sja1105_vl.c
index b7e95d60a6e4..369be2ac3587 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -7,6 +7,27 @@
#define SJA1105_SIZE_VL_STATUS 8
+static struct list_head *
+sja1105_first_entry_longer_than(struct list_head *entries,
+ s64 interval,
+ struct netlink_ext_ack *extack)
+{
+ struct sja1105_gate_entry *p;
+
+ list_for_each_entry(p, entries, list) {
+ if (p->interval == interval) {
+ NL_SET_ERR_MSG_MOD(extack, "Gate conflict");
+ return ERR_PTR(-EBUSY);
+ }
+
+ if (interval < p->interval)
+ return >list;
+ }
+
+ /* Empty list, or specified interval is largest within the list */
+ return entries;
+}
+
/* Insert into the global gate list, sorted by gate action time. */
static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
struct sja1105_rule *rule,
@@ -14,6 +35,7 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
struct netlink_ext_ack *extack)
{
struct sja1105_gate_entry *e;
+ struct list_head *pos;
int rc;
e = kzalloc(sizeof(*e), GFP_KERNEL);
@@ -24,25 +46,15 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
e->gate_state = gate_state;
e->interval = entry_time;
- if (list_empty(_cfg->entries)) {
- list_add(>list, _cfg->entries);
- } else {
- struct sja1105_gate_entry *p;
-
- list_for_each_entry(p, _cfg->entries, list) {
- if (p->interval == e->interval) {
- NL_SET_ERR_MSG_MOD(extack,
- "Gate conflict");
- rc = -EBUSY;
- goto err;
- }
-
- if (e->interval < p->interval)
- break;
- }
- list_add(>list, p->list.prev);
+ pos = sja1105_first_entry_longer_than(_cfg->entries,
+ e->interval, extack);
+ if (IS_ERR(pos)) {
+ rc = PTR_ERR(pos);
+ goto err;
}
+ list_add(>list, pos->prev);
+
gating_cfg->num_entries++;
return 0;
--
2.25.1
Re: [PATCH net-next v2 05/18] net: dsa: mv88e6xxx: remove redundant check in mv88e6xxx_port_vlan()
> On 12. Apr 2022, at 13:27, Russell King (Oracle) > wrote: > > On Tue, Apr 12, 2022 at 12:58:17PM +0200, Jakob Koschel wrote: >> We know that "dev > dst->last_switch" in the "else" block. >> In other words, that "dev - dst->last_switch" is > 0. >> >> dsa_port_bridge_num_get(dp) can be 0, but the check >> "if (bridge_num + dst->last_switch != dev) continue", rewritten as >> "if (bridge_num != dev - dst->last_switch) continue", aka >> "if (bridge_num != something which cannot be 0) continue", >> makes it redundant to have the extra "if (!bridge_num) continue" logic, >> since a bridge_num of zero would have been skipped anyway. >> >> Signed-off-by: Jakob Koschel >> Signed-off-by: Vladimir Oltean > > Isn't this Vladimir's patch? > > If so, it should be commited as Vladimir as the author, and Vladimir's > sign-off should appear before yours. When sending out such patches, > there should be a From: line for Vladimir as the first line in the body > of the patch email. yes, you are right. I wasn't sure on how to send those commits, but I guess if I just set the commit author correctly it should be fine. regarding the order: I thought I did it correctly doing bottom up but I confused the order, wasn't on purpose. Sorry about that. I'll resend after verifying all the authors and sign-offs are correct. > > The same goes for the next mv88e6xxx patch in this series - I think > both of these are the patches Vladimir sent in his email: > > https://lore.kernel.org/r/20220408123101.p33jpynhqo67hebe@skbuf > > Thanks. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last! Thanks for pointing it out! Jakob
Re: [PATCH net-next v2 05/18] net: dsa: mv88e6xxx: remove redundant check in mv88e6xxx_port_vlan()
On Tue, Apr 12, 2022 at 12:58:17PM +0200, Jakob Koschel wrote: > We know that "dev > dst->last_switch" in the "else" block. > In other words, that "dev - dst->last_switch" is > 0. > > dsa_port_bridge_num_get(dp) can be 0, but the check > "if (bridge_num + dst->last_switch != dev) continue", rewritten as > "if (bridge_num != dev - dst->last_switch) continue", aka > "if (bridge_num != something which cannot be 0) continue", > makes it redundant to have the extra "if (!bridge_num) continue" logic, > since a bridge_num of zero would have been skipped anyway. > > Signed-off-by: Jakob Koschel > Signed-off-by: Vladimir Oltean Isn't this Vladimir's patch? If so, it should be commited as Vladimir as the author, and Vladimir's sign-off should appear before yours. When sending out such patches, there should be a From: line for Vladimir as the first line in the body of the patch email. The same goes for the next mv88e6xxx patch in this series - I think both of these are the patches Vladimir sent in his email: https://lore.kernel.org/r/20220408123101.p33jpynhqo67hebe@skbuf Thanks. -- RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
[PATCH net-next v2 18/18] team: Remove use of list iterator variable for list_for_each_entry_from()
In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].
Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
To either continue iterating from that position or skip the iteration
(if the previous iteration was complete) list_prepare_entry() is used.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/team/team.c | 20 +---
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index b07dde6f0abf..688c4393f099 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2425,17 +2425,17 @@ static int team_nl_send_options_get(struct team *team,
u32 portid, u32 seq,
int flags, team_nl_send_func_t *send_func,
struct list_head *sel_opt_inst_list)
{
+ struct team_option_inst *opt_inst, *tmp = NULL;
struct nlattr *option_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_option_inst *opt_inst;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- opt_inst = list_first_entry(sel_opt_inst_list,
- struct team_option_inst, tmp_list);
+ tmp = list_first_entry(sel_opt_inst_list,
+ struct team_option_inst, tmp_list);
start_again:
err = __send_and_alloc_skb(, team, portid, send_func);
@@ -2456,7 +2456,9 @@ static int team_nl_send_options_get(struct team *team,
u32 portid, u32 seq,
goto nla_put_failure;
i = 0;
+ opt_inst = list_prepare_entry(tmp, sel_opt_inst_list, tmp_list);
incomplete = false;
+ tmp = NULL;
list_for_each_entry_from(opt_inst, sel_opt_inst_list, tmp_list) {
err = team_nl_fill_one_option_get(skb, team, opt_inst);
if (err) {
@@ -2464,6 +2466,7 @@ static int team_nl_send_options_get(struct team *team,
u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = opt_inst;
break;
}
goto errout;
@@ -2707,14 +2710,14 @@ static int team_nl_send_port_list_get(struct team
*team, u32 portid, u32 seq,
struct nlattr *port_list;
struct nlmsghdr *nlh;
void *hdr;
- struct team_port *port;
+ struct team_port *port, *tmp = NULL;
int err;
struct sk_buff *skb = NULL;
bool incomplete;
int i;
- port = list_first_entry_or_null(>port_list,
- struct team_port, list);
+ tmp = list_first_entry_or_null(>port_list,
+ struct team_port, list);
start_again:
err = __send_and_alloc_skb(, team, portid, send_func);
@@ -2744,7 +2747,9 @@ static int team_nl_send_port_list_get(struct team *team,
u32 portid, u32 seq,
err = team_nl_fill_one_port_get(skb, one_port);
if (err)
goto errout;
- } else if (port) {
+ } else {
+ port = list_prepare_entry(tmp, >port_list, list);
+ tmp = NULL;
list_for_each_entry_from(port, >port_list, list) {
err = team_nl_fill_one_port_get(skb, port);
if (err) {
@@ -2752,6 +2757,7 @@ static int team_nl_send_port_list_get(struct team *team,
u32 portid, u32 seq,
if (!i)
goto errout;
incomplete = true;
+ tmp = port;
break;
}
goto errout;
--
2.25.1
[PATCH net-next v2 17/18] ipvlan: Remove usage of list iterator variable for the loop body
In preparation to limit the scope of the list iterator variable to the
list traversal loop, use a dedicated pointer to iterate through the
list [1].
Since that variable should not be used past the loop iteration, a
separate variable is used to 'remember the current location within the
loop'.
To either continue iterating from that position or start a new
iteration (if the previous iteration was complete) list_prepare_entry()
is used.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ipvlan/ipvlan_main.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/net/ipvlan/ipvlan_main.c b/drivers/net/ipvlan/ipvlan_main.c
index 696e245f6d00..063d7c30e944 100644
--- a/drivers/net/ipvlan/ipvlan_main.c
+++ b/drivers/net/ipvlan/ipvlan_main.c
@@ -9,7 +9,7 @@
static int ipvlan_set_port_mode(struct ipvl_port *port, u16 nval,
struct netlink_ext_ack *extack)
{
- struct ipvl_dev *ipvlan;
+ struct ipvl_dev *ipvlan, *tmp = NULL;
unsigned int flags;
int err;
@@ -26,8 +26,10 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16
nval,
flags & ~IFF_NOARP,
extack);
}
- if (unlikely(err))
+ if (unlikely(err)) {
+ tmp = ipvlan;
goto fail;
+ }
}
if (nval == IPVLAN_MODE_L3S) {
/* New mode is L3S */
@@ -43,6 +45,7 @@ static int ipvlan_set_port_mode(struct ipvl_port *port, u16
nval,
return 0;
fail:
+ ipvlan = list_prepare_entry(tmp, >ipvlans, pnode);
/* Undo the flags changes that have been done so far. */
list_for_each_entry_continue_reverse(ipvlan, >ipvlans, pnode) {
flags = ipvlan->dev->flags;
--
2.25.1
[PATCH net-next v2 16/18] ps3_gelic: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
.../net/ethernet/toshiba/ps3_gelic_wireless.c | 30 +--
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
index dc14a66583ff..c8a016c902cd 100644
--- a/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
+++ b/drivers/net/ethernet/toshiba/ps3_gelic_wireless.c
@@ -1495,14 +1495,14 @@ static int gelic_wl_start_scan(struct gelic_wl_info
*wl, int always_scan,
*/
static void gelic_wl_scan_complete_event(struct gelic_wl_info *wl)
{
+ struct gelic_wl_scan_info *target = NULL, *iter, *tmp;
struct gelic_eurus_cmd *cmd = NULL;
- struct gelic_wl_scan_info *target, *tmp;
struct gelic_wl_scan_info *oldest = NULL;
struct gelic_eurus_scan_info *scan_info;
unsigned int scan_info_size;
union iwreq_data data;
unsigned long this_time = jiffies;
- unsigned int data_len, i, found, r;
+ unsigned int data_len, i, r;
void *buf;
pr_debug("%s:start\n", __func__);
@@ -1539,14 +1539,14 @@ static void gelic_wl_scan_complete_event(struct
gelic_wl_info *wl)
wl->scan_stat = GELIC_WL_SCAN_STAT_GOT_LIST;
/* mark all entries are old */
- list_for_each_entry_safe(target, tmp, >network_list, list) {
- target->valid = 0;
+ list_for_each_entry_safe(iter, tmp, >network_list, list) {
+ iter->valid = 0;
/* expire too old entries */
- if (time_before(target->last_scanned + wl->scan_age,
+ if (time_before(iter->last_scanned + wl->scan_age,
this_time)) {
- kfree(target->hwinfo);
- target->hwinfo = NULL;
- list_move_tail(>list, >network_free_list);
+ kfree(iter->hwinfo);
+ iter->hwinfo = NULL;
+ list_move_tail(>list, >network_free_list);
}
}
@@ -1569,22 +1569,22 @@ static void gelic_wl_scan_complete_event(struct
gelic_wl_info *wl)
continue;
}
- found = 0;
+ target = NULL;
oldest = NULL;
- list_for_each_entry(target, >network_list, list) {
- if (ether_addr_equal(>hwinfo->bssid[2],
+ list_for_each_entry(iter, >network_list, list) {
+ if (ether_addr_equal(>hwinfo->bssid[2],
_info->bssid[2])) {
- found = 1;
+ target = iter;
pr_debug("%s: same BBS found scanned list\n",
__func__);
break;
}
if (!oldest ||
- (target->last_scanned < oldest->last_scanned))
- oldest = target;
+ (iter->last_scanned < oldest->last_scanned))
+ oldest = iter;
}
- if (!found) {
+ if (!target) {
/* not found in the list */
if (list_empty(>network_free_list)) {
/* expire oldest */
--
2.25.1
[PATCH net-next v2 15/18] net: netcp: Remove usage of list iterator for list_add() after loop body
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer pointing to the location
where the element should be inserted [1].
Before, the code implicitly used the head when no element was found
when using >list. The new 'pos' variable is set to the list head
by default and overwritten if the list exits early, marking the
insertion point for list_add().
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/ti/netcp_core.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/ti/netcp_core.c
b/drivers/net/ethernet/ti/netcp_core.c
index 16507bff652a..f25104b5a31b 100644
--- a/drivers/net/ethernet/ti/netcp_core.c
+++ b/drivers/net/ethernet/ti/netcp_core.c
@@ -471,6 +471,7 @@ struct netcp_hook_list {
int netcp_register_txhook(struct netcp_intf *netcp_priv, int order,
netcp_hook_rtn *hook_rtn, void *hook_data)
{
+ struct list_head *pos = _priv->txhook_list_head;
struct netcp_hook_list *entry;
struct netcp_hook_list *next;
unsigned long flags;
@@ -485,10 +486,12 @@ int netcp_register_txhook(struct netcp_intf *netcp_priv,
int order,
spin_lock_irqsave(_priv->lock, flags);
list_for_each_entry(next, _priv->txhook_list_head, list) {
- if (next->order > order)
+ if (next->order > order) {
+ pos = >list;
break;
+ }
}
- __list_add(>list, next->list.prev, >list);
+ list_add_tail(>list, pos);
spin_unlock_irqrestore(_priv->lock, flags);
return 0;
@@ -520,6 +523,7 @@ EXPORT_SYMBOL_GPL(netcp_unregister_txhook);
int netcp_register_rxhook(struct netcp_intf *netcp_priv, int order,
netcp_hook_rtn *hook_rtn, void *hook_data)
{
+ struct list_head *pos = _priv->rxhook_list_head;
struct netcp_hook_list *entry;
struct netcp_hook_list *next;
unsigned long flags;
@@ -534,10 +538,12 @@ int netcp_register_rxhook(struct netcp_intf *netcp_priv,
int order,
spin_lock_irqsave(_priv->lock, flags);
list_for_each_entry(next, _priv->rxhook_list_head, list) {
- if (next->order > order)
+ if (next->order > order) {
+ pos = >list;
break;
+ }
}
- __list_add(>list, next->list.prev, >list);
+ list_add_tail(>list, pos);
spin_unlock_irqrestore(_priv->lock, flags);
return 0;
--
2.25.1
[PATCH net-next v2 14/18] sfc: Remove usage of list iterator for list_add() after the loop body
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer pointing to the location
where the element should be inserted [1].
Before, the code implicitly used the head when no element was found
when using >list. The new 'pos' variable is set to the list head
by default and overwritten if the list exits early, marking the
insertion point for list_add().
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/sfc/rx_common.c | 8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/sfc/rx_common.c
b/drivers/net/ethernet/sfc/rx_common.c
index 1b22c7be0088..80894a35ea79 100644
--- a/drivers/net/ethernet/sfc/rx_common.c
+++ b/drivers/net/ethernet/sfc/rx_common.c
@@ -555,7 +555,7 @@ efx_rx_packet_gro(struct efx_channel *channel, struct
efx_rx_buffer *rx_buf,
*/
struct efx_rss_context *efx_alloc_rss_context_entry(struct efx_nic *efx)
{
- struct list_head *head = >rss_context.list;
+ struct list_head *head = *pos = >rss_context.list;
struct efx_rss_context *ctx, *new;
u32 id = 1; /* Don't use zero, that refers to the master RSS context */
@@ -563,8 +563,10 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct
efx_nic *efx)
/* Search for first gap in the numbering */
list_for_each_entry(ctx, head, list) {
- if (ctx->user_id != id)
+ if (ctx->user_id != id) {
+ pos = >list;
break;
+ }
id++;
/* Check for wrap. If this happens, we have nearly 2^32
* allocated RSS contexts, which seems unlikely.
@@ -582,7 +584,7 @@ struct efx_rss_context *efx_alloc_rss_context_entry(struct
efx_nic *efx)
/* Insert the new entry into the gap */
new->user_id = id;
- list_add_tail(>list, >list);
+ list_add_tail(>list, pos);
return new;
}
--
2.25.1
[PATCH net-next v2 13/18] net: qede: Remove check of list iterator against head past the loop body
When list_for_each_entry() completes the iteration over the whole list
without breaking the loop, the iterator value will be a bogus pointer
computed based on the head element.
While it is safe to use the pointer to determine if it was computed
based on the head element, either with list_entry_is_head() or
>member == head, using the iterator variable after the loop should
be avoided.
In preparation to limit the scope of a list iterator to the list
traversal loop, use a dedicated pointer to point to the found element [1].
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qede/qede_filter.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_filter.c
b/drivers/net/ethernet/qlogic/qede/qede_filter.c
index 3010833ddde3..3d167e37e654 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_filter.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_filter.c
@@ -829,18 +829,21 @@ int qede_configure_vlan_filters(struct qede_dev *edev)
int qede_vlan_rx_kill_vid(struct net_device *dev, __be16 proto, u16 vid)
{
struct qede_dev *edev = netdev_priv(dev);
- struct qede_vlan *vlan;
+ struct qede_vlan *vlan = NULL;
+ struct qede_vlan *iter;
int rc = 0;
DP_VERBOSE(edev, NETIF_MSG_IFDOWN, "Removing vlan 0x%04x\n", vid);
/* Find whether entry exists */
__qede_lock(edev);
- list_for_each_entry(vlan, >vlan_list, list)
- if (vlan->vid == vid)
+ list_for_each_entry(iter, >vlan_list, list)
+ if (iter->vid == vid) {
+ vlan = iter;
break;
+ }
- if (list_entry_is_head(vlan, >vlan_list, list)) {
+ if (!vlan) {
DP_VERBOSE(edev, (NETIF_MSG_IFUP | NETIF_MSG_IFDOWN),
"Vlan isn't configured\n");
goto out;
--
2.25.1
[PATCH net-next v2 12/18] net: qede: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qede/qede_rdma.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_rdma.c
b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
index 6304514a6f2c..2eb03ffe2484 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_rdma.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_rdma.c
@@ -246,18 +246,17 @@ static void qede_rdma_change_mtu(struct qede_dev *edev)
static struct qede_rdma_event_work *
qede_rdma_get_free_event_node(struct qede_dev *edev)
{
- struct qede_rdma_event_work *event_node = NULL;
- bool found = false;
+ struct qede_rdma_event_work *event_node = NULL, *iter;
- list_for_each_entry(event_node, >rdma_info.rdma_event_list,
+ list_for_each_entry(iter, >rdma_info.rdma_event_list,
list) {
- if (!work_pending(_node->work)) {
- found = true;
+ if (!work_pending(>work)) {
+ event_node = iter;
break;
}
}
- if (!found) {
+ if (!event_node) {
event_node = kzalloc(sizeof(*event_node), GFP_ATOMIC);
if (!event_node) {
DP_NOTICE(edev,
--
2.25.1
[PATCH net-next v2 11/18] qed: Remove usage of list iterator variable after the loop
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
Since "found" and "p_ent" need to be equal, "found" should be used
consistently to limit the scope of "p_ent" to the list traversal in
the future.
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qed/qed_spq.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_spq.c
b/drivers/net/ethernet/qlogic/qed/qed_spq.c
index d01b9245f811..cbaa2abed660 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_spq.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_spq.c
@@ -934,10 +934,10 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
u8 fw_return_code,
union event_ring_data *p_data)
{
+ struct qed_spq_entry*found = NULL;
struct qed_spq *p_spq;
- struct qed_spq_entry*p_ent = NULL;
+ struct qed_spq_entry*p_ent;
struct qed_spq_entry*tmp;
- struct qed_spq_entry*found = NULL;
if (!p_hwfn)
return -EINVAL;
@@ -980,7 +980,7 @@ int qed_spq_completion(struct qed_hwfn *p_hwfn,
DP_VERBOSE(p_hwfn, QED_MSG_SPQ,
"Complete EQE [echo %04x]: func %p cookie %p)\n",
le16_to_cpu(echo),
- p_ent->comp_cb.function, p_ent->comp_cb.cookie);
+ found->comp_cb.function, found->comp_cb.cookie);
if (found->comp_cb.function)
found->comp_cb.function(p_hwfn, found->comp_cb.cookie, p_data,
fw_return_code);
--
2.25.1
[PATCH net-next v2 10/18] qed: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 26 ++---
1 file changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
index 1d1d4caad680..198c9321bf51 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_iwarp.c
@@ -1630,38 +1630,36 @@ static struct qed_iwarp_listener *
qed_iwarp_get_listener(struct qed_hwfn *p_hwfn,
struct qed_iwarp_cm_info *cm_info)
{
- struct qed_iwarp_listener *listener = NULL;
+ struct qed_iwarp_listener *listener = NULL, *iter;
static const u32 ip_zero[4] = { 0, 0, 0, 0 };
- bool found = false;
- list_for_each_entry(listener,
+ list_for_each_entry(iter,
_hwfn->p_rdma_info->iwarp.listen_list,
list_entry) {
- if (listener->port == cm_info->local_port) {
- if (!memcmp(listener->ip_addr,
+ if (iter->port == cm_info->local_port) {
+ if (!memcmp(iter->ip_addr,
ip_zero, sizeof(ip_zero))) {
- found = true;
+ listener = iter;
break;
}
- if (!memcmp(listener->ip_addr,
+ if (!memcmp(iter->ip_addr,
cm_info->local_ip,
sizeof(cm_info->local_ip)) &&
- (listener->vlan == cm_info->vlan)) {
- found = true;
+ iter->vlan == cm_info->vlan) {
+ listener = iter;
break;
}
}
}
- if (found) {
+ if (listener)
DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener found = %p\n",
listener);
- return listener;
- }
+ else
+ DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- DP_VERBOSE(p_hwfn, QED_MSG_RDMA, "listener not found\n");
- return NULL;
+ return listener;
}
static int
--
2.25.1
[PATCH net-next v2 09/18] qed: Use dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable [1].
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/net/ethernet/qlogic/qed/qed_dev.c | 11 ++-
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qed/qed_dev.c
b/drivers/net/ethernet/qlogic/qed/qed_dev.c
index 672480c9d195..e920e7dcf66a 100644
--- a/drivers/net/ethernet/qlogic/qed/qed_dev.c
+++ b/drivers/net/ethernet/qlogic/qed/qed_dev.c
@@ -174,7 +174,7 @@ int qed_db_recovery_add(struct qed_dev *cdev,
int qed_db_recovery_del(struct qed_dev *cdev,
void __iomem *db_addr, void *db_data)
{
- struct qed_db_recovery_entry *db_entry = NULL;
+ struct qed_db_recovery_entry *db_entry = NULL, *iter;
struct qed_hwfn *p_hwfn;
int rc = -EINVAL;
@@ -190,12 +190,13 @@ int qed_db_recovery_del(struct qed_dev *cdev,
/* Protect the list */
spin_lock_bh(_hwfn->db_recovery_info.lock);
- list_for_each_entry(db_entry,
+ list_for_each_entry(iter,
_hwfn->db_recovery_info.list, list_entry) {
/* search according to db_data addr since db_addr is not unique
(roce) */
- if (db_entry->db_data == db_data) {
- qed_db_recovery_dp_entry(p_hwfn, db_entry, "Deleting");
- list_del(_entry->list_entry);
+ if (iter->db_data == db_data) {
+ qed_db_recovery_dp_entry(p_hwfn, iter, "Deleting");
+ list_del(>list_entry);
+ db_entry = iter;
rc = 0;
break;
}
--
2.25.1
[PATCH net-next v2 08/18] net: sparx5: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
.../microchip/sparx5/sparx5_mactable.c| 25 +--
1 file changed, 12 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
index a5837dbe0c7e..bb8d9ce79ac2 100644
--- a/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
+++ b/drivers/net/ethernet/microchip/sparx5/sparx5_mactable.c
@@ -362,8 +362,7 @@ static void sparx5_mact_handle_entry(struct sparx5 *sparx5,
unsigned char mac[ETH_ALEN],
u16 vid, u32 cfg2)
{
- struct sparx5_mact_entry *mact_entry;
- bool found = false;
+ struct sparx5_mact_entry *mact_entry = NULL, *iter;
u16 port;
if (LRN_MAC_ACCESS_CFG_2_MAC_ENTRY_ADDR_TYPE_GET(cfg2) !=
@@ -378,28 +377,28 @@ static void sparx5_mact_handle_entry(struct sparx5
*sparx5,
return;
mutex_lock(>mact_lock);
- list_for_each_entry(mact_entry, >mact_entries, list) {
- if (mact_entry->vid == vid &&
- ether_addr_equal(mac, mact_entry->mac)) {
- found = true;
- mact_entry->flags |= MAC_ENT_ALIVE;
- if (mact_entry->port != port) {
+ list_for_each_entry(iter, >mact_entries, list) {
+ if (iter->vid == vid &&
+ ether_addr_equal(mac, iter->mac)) {
+ iter->flags |= MAC_ENT_ALIVE;
+ if (iter->port != port) {
dev_warn(sparx5->dev, "Entry move: %d -> %d\n",
-mact_entry->port, port);
- mact_entry->port = port;
- mact_entry->flags |= MAC_ENT_MOVED;
+iter->port, port);
+ iter->port = port;
+ iter->flags |= MAC_ENT_MOVED;
}
/* Entry handled */
+ mact_entry = iter;
break;
}
}
mutex_unlock(>mact_lock);
- if (found && !(mact_entry->flags & MAC_ENT_MOVED))
+ if (mact_entry && !(mact_entry->flags & MAC_ENT_MOVED))
/* Present, not moved */
return;
- if (!found) {
+ if (!mact_entry) {
/* Entry not found - now add */
mact_entry = alloc_mact_entry(sparx5, mac, vid, port);
if (!mact_entry)
--
2.25.1
[PATCH net-next v2 07/18] net: dsa: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
net/dsa/dsa.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 89c6c86e746f..645522c4dd4a 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -112,22 +112,21 @@ const struct dsa_device_ops
*dsa_find_tagger_by_name(const char *buf)
const struct dsa_device_ops *dsa_tag_driver_get(int tag_protocol)
{
- struct dsa_tag_driver *dsa_tag_driver;
+ struct dsa_tag_driver *dsa_tag_driver = NULL, *iter;
const struct dsa_device_ops *ops;
- bool found = false;
request_module("%s%d", DSA_TAG_DRIVER_ALIAS, tag_protocol);
mutex_lock(_tag_drivers_lock);
- list_for_each_entry(dsa_tag_driver, _tag_drivers_list, list) {
- ops = dsa_tag_driver->ops;
+ list_for_each_entry(iter, _tag_drivers_list, list) {
+ ops = iter->ops;
if (ops->proto == tag_protocol) {
- found = true;
+ dsa_tag_driver = iter;
break;
}
}
- if (found) {
+ if (dsa_tag_driver) {
if (!try_module_get(dsa_tag_driver->owner))
ops = ERR_PTR(-ENOPROTOOPT);
} else {
--
2.25.1
[PATCH net-next v2 06/18] net: dsa: mv88e6xxx: refactor mv88e6xxx_port_vlan()
To avoid bugs and speculative execution exploits due to type-confused
pointers at the end of a list_for_each_entry() loop, one measure is to
restrict code to not use the iterator variable outside the loop block.
In the case of mv88e6xxx_port_vlan(), this isn't a problem, as we never
let the loops exit through "natural causes" anyway, by using a "found"
variable and then using the last "dp" iterator prior to the break, which
is a safe thing to do.
Nonetheless, with the expected new syntax, this pattern will no longer
be possible.
Profit off of the occasion and break the two port finding methods into
smaller sub-functions. Somehow, returning a copy of the iterator pointer
is still accepted.
This change makes it redundant to have a "bool found", since the "dp"
from mv88e6xxx_port_vlan() now holds NULL if we haven't found what we
were looking for.
Signed-off-by: Jakob Koschel
Signed-off-by: Vladimir Oltean
---
drivers/net/dsa/mv88e6xxx/chip.c | 54 ++--
1 file changed, 31 insertions(+), 23 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index b3aa0e5bc842..1f35e89053e6 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1378,42 +1378,50 @@ static int mv88e6xxx_set_mac_eee(struct dsa_switch *ds,
int port,
return 0;
}
+static struct dsa_port *mv88e6xxx_find_port(struct dsa_switch_tree *dst,
+ int sw_index, int port)
+{
+ struct dsa_port *dp;
+
+ list_for_each_entry(dp, >ports, list)
+ if (dp->ds->index == sw_index && dp->index == port)
+ return dp;
+
+ return NULL;
+}
+
+static struct dsa_port *
+mv88e6xxx_find_port_by_bridge_num(struct dsa_switch_tree *dst,
+ unsigned int bridge_num)
+{
+ struct dsa_port *dp;
+
+ list_for_each_entry(dp, >ports, list)
+ if (dsa_port_bridge_num_get(dp) == bridge_num)
+ return dp;
+
+ return NULL;
+}
+
/* Mask of the local ports allowed to receive frames from a given fabric port
*/
static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip *chip, int dev, int port)
{
struct dsa_switch *ds = chip->ds;
struct dsa_switch_tree *dst = ds->dst;
struct dsa_port *dp, *other_dp;
- bool found = false;
u16 pvlan;
- /* dev is a physical switch */
if (dev <= dst->last_switch) {
- list_for_each_entry(dp, >ports, list) {
- if (dp->ds->index == dev && dp->index == port) {
- /* dp might be a DSA link or a user port, so it
-* might or might not have a bridge.
-* Use the "found" variable for both cases.
-*/
- found = true;
- break;
- }
- }
- /* dev is a virtual bridge */
+ /* dev is a physical switch */
+ dp = mv88e6xxx_find_port(dst, dev, port);
} else {
- list_for_each_entry(dp, >ports, list) {
- unsigned int bridge_num = dsa_port_bridge_num_get(dp);
-
- if (bridge_num + dst->last_switch != dev)
- continue;
-
- found = true;
- break;
- }
+ /* dev is a virtual bridge */
+ dp = mv88e6xxx_find_port_by_bridge_num(dst,
+ dev - dst->last_switch);
}
/* Prevent frames from unknown switch or virtual bridge */
- if (!found)
+ if (!dp)
return 0;
/* Frames from DSA links and CPU ports can egress any local port */
--
2.25.1
[PATCH net-next v2 05/18] net: dsa: mv88e6xxx: remove redundant check in mv88e6xxx_port_vlan()
We know that "dev > dst->last_switch" in the "else" block.
In other words, that "dev - dst->last_switch" is > 0.
dsa_port_bridge_num_get(dp) can be 0, but the check
"if (bridge_num + dst->last_switch != dev) continue", rewritten as
"if (bridge_num != dev - dst->last_switch) continue", aka
"if (bridge_num != something which cannot be 0) continue",
makes it redundant to have the extra "if (!bridge_num) continue" logic,
since a bridge_num of zero would have been skipped anyway.
Signed-off-by: Jakob Koschel
Signed-off-by: Vladimir Oltean
---
drivers/net/dsa/mv88e6xxx/chip.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 64f4fdd02902..b3aa0e5bc842 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -1404,9 +1404,6 @@ static u16 mv88e6xxx_port_vlan(struct mv88e6xxx_chip
*chip, int dev, int port)
list_for_each_entry(dp, >ports, list) {
unsigned int bridge_num = dsa_port_bridge_num_get(dp);
- if (!bridge_num)
- continue;
-
if (bridge_num + dst->last_switch != dev)
continue;
--
2.25.1
[PATCH net-next v2 04/18] net: dsa: sja1105: use list_add_tail(pos) instead of list_add(pos->prev)
When passed a non-head list element, list_add_tail() actually adds the new element to its left, which is what we want. Despite the slightly confusing name, use the dedicated function which does the same thing as the open-coded list_add(pos->prev). Suggested-by: Jakub Kicinski Signed-off-by: Jakob Koschel Signed-off-by: Vladimir Oltean --- drivers/net/dsa/sja1105/sja1105_vl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c b/drivers/net/dsa/sja1105/sja1105_vl.c index e5ea8eb9ec4e..7fe9b18f1cbd 100644 --- a/drivers/net/dsa/sja1105/sja1105_vl.c +++ b/drivers/net/dsa/sja1105/sja1105_vl.c @@ -49,7 +49,7 @@ static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg, e->rule = rule; e->gate_state = gate_state; e->interval = entry_time; - list_add(>list, pos->prev); + list_add_tail(>list, pos); gating_cfg->num_entries++; -- 2.25.1
[PATCH net-next v2 00/18] Remove use of list iterator after loop body
When the list iterator loop does not exit early the list iterator variable contains a type-confused pointer to a 'bogus' list element computed based on the head [1]. Often a 'found' variable is used to ensure the list iterator variable is only accessed after the loop body if the loop did exit early (using a break or goto). In other cases that list iterator variable is used in combination to access the list member which reverses the invocation of container_of() and brings back a "safe" pointer to the head of the list. Since, due to this code patten, there were quite a few bugs discovered [2], Linus concluded that the rule should be to never use the list iterator after the loop and introduce a dedicated pointer for that [3]. With the new gnu11 standard, it will now be possible to limit the scope of the list iterator variable to the traversal loop itself by defining the variable within the for loop. This, however, requires to remove all uses of the list iterator after the loop. Based on input from Paolo Abeni [4], Vinicius Costa Gomes [5], and Jakub Kicinski [6], I've splitted all the net-next related changes into two patch sets, where this is part 1. v1->v2: - Fixed commit message for PATCH 14/18 and used dedicated variable pointing to the position (Edward Cree) - Removed redundant check in mv88e6xxx_port_vlan() (Vladimir Oltean) - Refactor mv88e6xxx_port_vlan() using separate list iterator functions (Vladimir Oltean) - Refactor sja1105_insert_gate_entry() to use separate list iterator functions (Vladimir Oltean) - Allow early return in sja1105_insert_gate_entry() if sja1105_first_entry_longer_than() didn't find any element (Vladimir Oltean) - Use list_add_tail() instead of list_add() in sja1105_insert_gate_entry() (Jakub Kicinski) - net: netcp: also use separate 'pos' variable instead of duplicating list_add() Link: https://lwn.net/Articles/887097/ [1] Link: https://lore.kernel.org/linux-kernel/20220217184829.1991035-4-jakobkosc...@gmail.com/ [2] Link: https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/ [3] Link: https://lore.kernel.org/linux-kernel/7393b673c626fd75f2b4f8509faa5459254fb87c.ca...@redhat.com/ [4] Link: https://lore.kernel.org/linux-kernel/877d8a3sww@intel.com/ [5] Link: https://lore.kernel.org/linux-kernel/20220403205502.1b344...@kernel.org/ [6] Jakob Koschel (18): connector: Replace usage of found with dedicated list iterator variable net: dsa: sja1105: remove use of iterator after list_for_each_entry() loop net: dsa: sja1105: reorder sja1105_first_entry_longer_than with memory allocation net: dsa: sja1105: use list_add_tail(pos) instead of list_add(pos->prev) net: dsa: mv88e6xxx: remove redundant check in mv88e6xxx_port_vlan() net: dsa: mv88e6xxx: refactor mv88e6xxx_port_vlan() net: dsa: Replace usage of found with dedicated list iterator variable net: sparx5: Replace usage of found with dedicated list iterator variable qed: Use dedicated list iterator variable qed: Replace usage of found with dedicated list iterator variable qed: Remove usage of list iterator variable after the loop net: qede: Replace usage of found with dedicated list iterator variable net: qede: Remove check of list iterator against head past the loop body sfc: Remove usage of list iterator for list_add() after the loop body net: netcp: Remove usage of list iterator for list_add() after loop body ps3_gelic: Replace usage of found with dedicated list iterator variable ipvlan: Remove usage of list iterator variable for the loop body team: Remove use of list iterator variable for list_for_each_entry_from() drivers/connector/cn_queue.c | 13 ++--- drivers/net/dsa/mv88e6xxx/chip.c | 57 ++- drivers/net/dsa/sja1105/sja1105_vl.c | 51 + .../microchip/sparx5/sparx5_mactable.c| 25 drivers/net/ethernet/qlogic/qed/qed_dev.c | 11 ++-- drivers/net/ethernet/qlogic/qed/qed_iwarp.c | 26 - drivers/net/ethernet/qlogic/qed/qed_spq.c | 6 +- .../net/ethernet/qlogic/qede/qede_filter.c| 11 ++-- drivers/net/ethernet/qlogic/qede/qede_rdma.c | 11 ++-- drivers/net/ethernet/sfc/rx_common.c | 8 ++- drivers/net/ethernet/ti/netcp_core.c | 14 +++-- .../net/ethernet/toshiba/ps3_gelic_wireless.c | 30 +- drivers/net/ipvlan/ipvlan_main.c | 7 ++- drivers/net/team/team.c | 20 --- net/dsa/dsa.c | 11 ++-- 15 files changed, 163 insertions(+), 138 deletions(-) base-commit: 3e732ebf7316ac83e8562db7e64cc68aec390a18 -- 2.25.1
[PATCH net-next v2 02/18] net: dsa: sja1105: remove use of iterator after list_for_each_entry() loop
The link below explains that there is a desire to syntactically change
list_for_each_entry() and list_for_each() such that it becomes
impossible to use the iterator variable outside the scope of the loop.
Although sja1105_insert_gate_entry() makes legitimate use of the
iterator pointer when it breaks out, the pattern it uses may become
illegal, so it needs to change.
It is deemed acceptable to use a copy of the loop iterator, and
sja1105_insert_gate_entry() only needs to know the list_head element
before which the list insertion should be made. So let's profit from the
occasion and refactor the list iteration to a dedicated function.
An additional benefit is given by the fact that with the helper function
in place, we no longer need to special-case the empty list, since it is
equivalent to not having found any gating entry larger than the
specified interval in the list. We just need to insert at the tail of
that list (list_add vs list_add_tail on an empty list does the same
thing).
Link:
https://patchwork.kernel.org/project/netdevbpf/patch/20220407102900.3086255-3-jakobkosc...@gmail.com/#24810127
Signed-off-by: Jakob Koschel
Signed-off-by: Vladimir Oltean
---
drivers/net/dsa/sja1105/sja1105_vl.c | 46 ++--
1 file changed, 29 insertions(+), 17 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
b/drivers/net/dsa/sja1105/sja1105_vl.c
index b7e95d60a6e4..369be2ac3587 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -7,6 +7,27 @@
#define SJA1105_SIZE_VL_STATUS 8
+static struct list_head *
+sja1105_first_entry_longer_than(struct list_head *entries,
+ s64 interval,
+ struct netlink_ext_ack *extack)
+{
+ struct sja1105_gate_entry *p;
+
+ list_for_each_entry(p, entries, list) {
+ if (p->interval == interval) {
+ NL_SET_ERR_MSG_MOD(extack, "Gate conflict");
+ return ERR_PTR(-EBUSY);
+ }
+
+ if (interval < p->interval)
+ return >list;
+ }
+
+ /* Empty list, or specified interval is largest within the list */
+ return entries;
+}
+
/* Insert into the global gate list, sorted by gate action time. */
static int sja1105_insert_gate_entry(struct sja1105_gating_config *gating_cfg,
struct sja1105_rule *rule,
@@ -14,6 +35,7 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
struct netlink_ext_ack *extack)
{
struct sja1105_gate_entry *e;
+ struct list_head *pos;
int rc;
e = kzalloc(sizeof(*e), GFP_KERNEL);
@@ -24,25 +46,15 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
e->gate_state = gate_state;
e->interval = entry_time;
- if (list_empty(_cfg->entries)) {
- list_add(>list, _cfg->entries);
- } else {
- struct sja1105_gate_entry *p;
-
- list_for_each_entry(p, _cfg->entries, list) {
- if (p->interval == e->interval) {
- NL_SET_ERR_MSG_MOD(extack,
- "Gate conflict");
- rc = -EBUSY;
- goto err;
- }
-
- if (e->interval < p->interval)
- break;
- }
- list_add(>list, p->list.prev);
+ pos = sja1105_first_entry_longer_than(_cfg->entries,
+ e->interval, extack);
+ if (IS_ERR(pos)) {
+ rc = PTR_ERR(pos);
+ goto err;
}
+ list_add(>list, pos->prev);
+
gating_cfg->num_entries++;
return 0;
--
2.25.1
[PATCH net-next v2 03/18] net: dsa: sja1105: reorder sja1105_first_entry_longer_than with memory allocation
sja1105_first_entry_longer_than() does not make use of the full struct
sja1105_gate_entry *e, just of e->interval which is set from the passed
entry_time.
This means that if there is a gate conflict, we have allocated e for
nothing, just to free it later. Reorder the memory allocation and the
function call, to avoid that and simplify the error unwind path.
Signed-off-by: Jakob Koschel
Signed-off-by: Vladimir Oltean
---
drivers/net/dsa/sja1105/sja1105_vl.c | 17 +
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
b/drivers/net/dsa/sja1105/sja1105_vl.c
index 369be2ac3587..e5ea8eb9ec4e 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -36,7 +36,11 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
{
struct sja1105_gate_entry *e;
struct list_head *pos;
- int rc;
+
+ pos = sja1105_first_entry_longer_than(_cfg->entries,
+ entry_time, extack);
+ if (IS_ERR(pos))
+ return PTR_ERR(pos);
e = kzalloc(sizeof(*e), GFP_KERNEL);
if (!e)
@@ -45,22 +49,11 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
e->rule = rule;
e->gate_state = gate_state;
e->interval = entry_time;
-
- pos = sja1105_first_entry_longer_than(_cfg->entries,
- e->interval, extack);
- if (IS_ERR(pos)) {
- rc = PTR_ERR(pos);
- goto err;
- }
-
list_add(>list, pos->prev);
gating_cfg->num_entries++;
return 0;
-err:
- kfree(e);
- return rc;
}
/* The gate entries contain absolute times in their e->interval field. Convert
--
2.25.1
[PATCH net-next v2 01/18] connector: Replace usage of found with dedicated list iterator variable
To move the list iterator variable into the list_for_each_entry_*()
macro in the future it should be avoided to use the list iterator
variable after the loop body.
To *never* use the list iterator variable after the loop it was
concluded to use a separate iterator variable instead of a
found boolean [1].
This removes the need to use a found variable and simply checking if
the variable was set, can determine if the break/goto was hit.
Link:
https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
[1]
Signed-off-by: Jakob Koschel
---
drivers/connector/cn_queue.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/connector/cn_queue.c b/drivers/connector/cn_queue.c
index 996f025eb63c..ed77599b0b25 100644
--- a/drivers/connector/cn_queue.c
+++ b/drivers/connector/cn_queue.c
@@ -92,20 +92,19 @@ int cn_queue_add_callback(struct cn_queue_dev *dev, const
char *name,
void cn_queue_del_callback(struct cn_queue_dev *dev, const struct cb_id *id)
{
- struct cn_callback_entry *cbq, *n;
- int found = 0;
+ struct cn_callback_entry *cbq = NULL, *iter, *n;
spin_lock_bh(>queue_lock);
- list_for_each_entry_safe(cbq, n, >queue_list, callback_entry) {
- if (cn_cb_equal(>id.id, id)) {
- list_del(>callback_entry);
- found = 1;
+ list_for_each_entry_safe(iter, n, >queue_list, callback_entry) {
+ if (cn_cb_equal(>id.id, id)) {
+ list_del(>callback_entry);
+ cbq = iter;
break;
}
}
spin_unlock_bh(>queue_lock);
- if (found)
+ if (cbq)
cn_queue_release_callback(cbq);
}
--
2.25.1
[PATCH] ASoC: fsl: using pm_runtime_resume_and_get instead of pm_runtime_get_sync
From: Minghao Chi
Using pm_runtime_resume_and_get is more appropriate
for simplifing code
Reported-by: Zeal Robot
Signed-off-by: Minghao Chi
---
sound/soc/fsl/fsl_esai.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/sound/soc/fsl/fsl_esai.c b/sound/soc/fsl/fsl_esai.c
index ed444e8f1d6b..1a2bdf8e76f0 100644
--- a/sound/soc/fsl/fsl_esai.c
+++ b/sound/soc/fsl/fsl_esai.c
@@ -1050,11 +1050,9 @@ static int fsl_esai_probe(struct platform_device *pdev)
goto err_pm_disable;
}
- ret = pm_runtime_get_sync(>dev);
- if (ret < 0) {
- pm_runtime_put_noidle(>dev);
+ ret = pm_runtime_resume_and_get(>dev);
+ if (ret < 0)
goto err_pm_get_sync;
- }
ret = fsl_esai_hw_init(esai_priv);
if (ret)
--
2.25.1
[Bug 215803] ppc64le(P9): BUG: Kernel NULL pointer dereference on read at 0x00000060 NIP: do_remove_conflicting_framebuffers+0x184/0x1d0
https://bugzilla.kernel.org/show_bug.cgi?id=215803 Michael Ellerman (mich...@ellerman.id.au) changed: What|Removed |Added Status|NEW |RESOLVED CC||mich...@ellerman.id.au Resolution|--- |CODE_FIX --- Comment #5 from Michael Ellerman (mich...@ellerman.id.au) --- This was reported to the patch author here: https://lore.kernel.org/all/YkHXO6LGHAN0p1pq@debian/ And there is a fix here: https://patchwork.freedesktop.org/patch/480648/ -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
Re: rcu_sched self-detected stall on CPU
"Paul E. McKenney" writes:
> On Sun, Apr 10, 2022 at 09:33:43PM +1000, Michael Ellerman wrote:
>> Zhouyi Zhou writes:
>> > On Fri, Apr 8, 2022 at 10:07 PM Paul E. McKenney
>> > wrote:
>> >> On Fri, Apr 08, 2022 at 06:02:19PM +0800, Zhouyi Zhou wrote:
>> >> > On Fri, Apr 8, 2022 at 3:23 PM Michael Ellerman
>> >> > wrote:
>> ...
>> >> > > I haven't seen it in my testing. But using Miguel's config I can
>> >> > > reproduce it seemingly on every boot.
>> >> > >
>> >> > > For me it bisects to:
>> >> > >
>> >> > > 35de589cb879 ("powerpc/time: improve decrementer clockevent
>> >> > > processing")
>> >> > >
>> >> > > Which seems plausible.
>> >> > I also bisect to 35de589cb879 ("powerpc/time: improve decrementer
>> >> > clockevent processing")
>> ...
>> >>
>> >> > > Reverting that on mainline makes the bug go away.
>>
>> >> > I also revert that on the mainline, and am currently doing a pressure
>> >> > test (by repeatedly invoking qemu and checking the console.log) on PPC
>> >> > VM in Oregon State University.
>>
>> > After 306 rounds of stress test on mainline without triggering the bug
>> > (last for 4 hours and 27 minutes), I think the bug is indeed caused by
>> > 35de589cb879 ("powerpc/time: improve decrementer clockevent
>> > processing") and stop the test for now.
>>
>> Thanks for testing, that's pretty conclusive.
>>
>> I'm not inclined to actually revert it yet.
>>
>> We need to understand if there's actually a bug in the patch, or if it's
>> just exposing some existing bug/bad behavior we have. The fact that it
>> only appears with CONFIG_HIGH_RES_TIMERS=n is suspicious.
>>
>> Do we have some code that inadvertently relies on something enabled by
>> HIGH_RES_TIMERS=y, or do we have a bug that is hidden by HIGH_RES_TIMERS=y ?
>
> For whatever it is worth, moderate rcutorture runs to completion without
> errors with CONFIG_HIGH_RES_TIMERS=n on 64-bit x86.
Thanks for testing that, I don't have any big x86 machines to test on :)
> Also for whatever it is worth, I don't know of anything other than
> microcontrollers or the larger IoT devices that would want their kernels
> built with CONFIG_HIGH_RES_TIMERS=n. Which might be a failure of
> imagination on my part, but so it goes.
Yeah I agree, like I said before I wasn't even aware you could turn it
off. So I think we'll definitely add a select HIGH_RES_TIMERS in future,
but first I need to work out why we are seeing stalls with it disabled.
cheers
Re: False positive kmemleak report for dtb properties names on powerpc
Christophe Leroy writes:
> Hi Ariel
>
> Le 09/04/2022 à 15:47, Ariel Marcovitch a écrit :
>> Hi Christophe, did you get the chance to look at this?
>
> I tested something this morning, it works for me, see below
>
>>
>> On 23/03/2022 21:06, Mike Rapoport wrote:
>>> Hi Catalin,
>>>
>>> On Wed, Mar 23, 2022 at 05:22:38PM +, Catalin Marinas wrote:
Hi Ariel,
On Fri, Feb 18, 2022 at 09:45:51PM +0200, Ariel Marcovitch wrote:
> I was running a powerpc 32bit kernel (built using
> qemu_ppc_mpc8544ds_defconfig
> buildroot config, with enabling DEBUGFS+KMEMLEAK+HIGHMEM in the kernel
> config)
>
> ...
>
> I don't suppose I can just shuffle the calls in setup_arch() around,
> so I
> wanted to hear your opinions first
I think it's better if we change the logic than shuffling the calls.
IIUC MEMBLOCK_ALLOC_ACCESSIBLE means that __va() works on the phys
address return by memblock, so something like below (untested):
>>> MEMBLOCK_ALLOC_ACCESSIBLE means "anywhere", see commit e63075a3c937
>>> ("memblock: Introduce default allocation limit and use it to replace
>>> explicit ones"), so it won't help to detect high memory.
>>>
>>> If I remember correctly, ppc initializes memblock *very* early, so
>>> setting
>>> max_low_pfn along with lowmem_end_addr in
>>> arch/powerpc/mm/init_32::MMU_init() makes sense to me.
>>>
>>> Maybe ppc folks have other ideas...
>>> I've added Christophe who works on ppc32 these days.
>
> I think memblock is already available at the end of MMU_init() on PPC32
> and at the end of early_setup() on PPC64. It means it is ready when we
> enter setup_arch().
>
> I tested the change below, it works for me, I don't get any kmemleak
> report anymore.
>
> diff --git a/arch/powerpc/kernel/setup-common.c
> b/arch/powerpc/kernel/setup-common.c
> index 518ae5aa9410..9f4e50b176c9 100644
> --- a/arch/powerpc/kernel/setup-common.c
> +++ b/arch/powerpc/kernel/setup-common.c
> @@ -840,6 +840,9 @@ void __init setup_arch(char **cmdline_p)
> /* Set a half-reasonable default so udelay does something sensible */
> loops_per_jiffy = 5 / HZ;
>
> + /* Parse memory topology */
> + mem_topology_setup();
> +
> /* Unflatten the device-tree passed by prom_init or kexec */
> unflatten_device_tree();
The 64-bit/NUMA version of mem_topology_setup() requires the device tree
to be unflattened, so I don't think that can work.
Setting max_low_pfn etc in MMU_init() as Mike suggested seems more
likely to work.
But we might need to set it again in mem_topology_setup() though, so
that things that change memblock_end_of_DRAM() are reflected, eg. memory
limit or crash dump?
cheers
Re: [PATCH AUTOSEL 5.17 40/49] powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit
Sasha Levin writes:
> From: Kefeng Wang
>
> [ Upstream commit ffa0b64e3be58519ae472ea29a1a1ad681e32f48 ]
>
> mpe: On 64-bit Book3E vmalloc space starts at 0x8000.
This cherry-pick is good, but can you also pick up the immediately
following commit:
1ff5c8e8c835 ("Revert "powerpc: Set max_mapnr correctly"")
For v5.16 and v5.17. Thanks.
cheers
Re: [PATCH 10/15] swiotlb: add a SWIOTLB_ANY flag to lift the low memory restriction
On Wed, Apr 06, 2022 at 08:25:32PM -0400, Konrad Rzeszutek Wilk wrote: > > diff --git a/arch/powerpc/platforms/pseries/svm.c > > b/arch/powerpc/platforms/pseries/svm.c > > index c5228f4969eb2..3b4045d508ec8 100644 > > --- a/arch/powerpc/platforms/pseries/svm.c > > +++ b/arch/powerpc/platforms/pseries/svm.c > > @@ -28,7 +28,7 @@ static int __init init_svm(void) > > * need to use the SWIOTLB buffer for DMA even if dma_capable() says > > * otherwise. > > */ > > - swiotlb_force = SWIOTLB_FORCE; > > + ppc_swiotlb_flags |= SWIOTLB_ANY | SWIOTLB_FORCE; > > This is the only place you set the ppc_swiotlb_flags.. so I wonder why > the '|=' instead of just '=' ? Preparing for setting others and not clobbering the value.
