Re: [PATCH V4 2/7] powerpc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
Le 11/04/2022 à 07:31, Anshuman Khandual a écrit :
>
>
> On 4/9/22 17:06, Christophe Leroy wrote:
>>
>>
>> Le 08/04/2022 à 14:53, Christophe Leroy a écrit :
>>>
>>>
>>> Le 07/04/2022 à 12:32, 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 powerpc_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/mmap.c | 26 ++
3 files changed, 27 insertions(+), 12 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 174edabb74fa..eb9b6ddbf92f 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_PROT
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/mmap.c b/arch/powerpc/mm/mmap.c
index c475cf810aa8..cd17bd6fa36b 100644
--- a/arch/powerpc/mm/mmap.c
+++ b/arch/powerpc/mm/mmap.c
@@ -254,3 +254,29 @@ void arch_pick_mmap_layout(struct mm_struct *mm,
struct rlimit *rlim_stack)
mm->get_unmapped_area = arch_get_unmapped_area_topdown;
}
}
+
+#ifdef CONFIG_PPC64
+static pgprot_t powerpc_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
+}
+#else
+static pgprot_t powerpc_vm_get_page_prot(unsigned long vm_flags)
+{
+ return __pgprot(0);
+}
+#endif /* CONFIG_PPC64 */
>>>
>>> Can we reduce this forest of #ifdefs and make it more readable ?
>>>
>>> mm/mmap.c is going away with patch
>>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/d6d849621f821af253e777a24eda4c648814a76e.1646847562.git.christophe.le...@csgroup.eu/
>>>
>>> So it would be better to add two versions of vm_get_page_prot(), for
>>> instance one in mm/pgtable_64.c and one in mm/pgtable_32.c
>>
>> Indeed, you don't need anything at all for PPC32. All you need to do is
>>
>> select ARCH_HAS_VM_GET_PAGE_PROT if PPC64
>>
>> And in fact it could even be PPC_BOOK3S_64 instead of PPC64 because
>> CONFIG_PPC_MEM_KEYS depends on PPC_BOOK3S_64 and _PAGE_SAO is 0 on nohash/64.
>>
>> So you can then put it into arch/powerpc/mm/book3s64/pgtable.c
>
> Would something like the following change work ?
Yes that looks good to me.
Thanks
Christophe
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index eb9b6ddbf92f..69e44358a235 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -140,7 +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_PROT
> + select ARCH_HAS_VM_GET_PAGE_PROTif PPC_BOOK3S_64
> select ARCH_HAVE_NMI_SAFE_CMPXCHG
> select ARCH_KEEP_MEMBLOCK
> select
Re: [PATCH V4 2/7] powerpc/mm: Enable ARCH_HAS_VM_GET_PAGE_PROT
On 4/9/22 17:06, Christophe Leroy wrote:
>
>
> Le 08/04/2022 à 14:53, Christophe Leroy a écrit :
>>
>>
>> Le 07/04/2022 à 12:32, 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 powerpc_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/mmap.c | 26 ++
>>> 3 files changed, 27 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>> index 174edabb74fa..eb9b6ddbf92f 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_PROT
>>> 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/mmap.c b/arch/powerpc/mm/mmap.c
>>> index c475cf810aa8..cd17bd6fa36b 100644
>>> --- a/arch/powerpc/mm/mmap.c
>>> +++ b/arch/powerpc/mm/mmap.c
>>> @@ -254,3 +254,29 @@ void arch_pick_mmap_layout(struct mm_struct *mm,
>>> struct rlimit *rlim_stack)
>>> mm->get_unmapped_area = arch_get_unmapped_area_topdown;
>>> }
>>> }
>>> +
>>> +#ifdef CONFIG_PPC64
>>> +static pgprot_t powerpc_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
>>> +}
>>> +#else
>>> +static pgprot_t powerpc_vm_get_page_prot(unsigned long vm_flags)
>>> +{
>>> + return __pgprot(0);
>>> +}
>>> +#endif /* CONFIG_PPC64 */
>>
>> Can we reduce this forest of #ifdefs and make it more readable ?
>>
>> mm/mmap.c is going away with patch
>> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/d6d849621f821af253e777a24eda4c648814a76e.1646847562.git.christophe.le...@csgroup.eu/
>>
>> So it would be better to add two versions of vm_get_page_prot(), for
>> instance one in mm/pgtable_64.c and one in mm/pgtable_32.c
>
> Indeed, you don't need anything at all for PPC32. All you need to do is
>
> select ARCH_HAS_VM_GET_PAGE_PROT if PPC64
>
> And in fact it could even be PPC_BOOK3S_64 instead of PPC64 because
> CONFIG_PPC_MEM_KEYS depends on PPC_BOOK3S_64 and _PAGE_SAO is 0 on nohash/64.
>
> So you can then put it into arch/powerpc/mm/book3s64/pgtable.c
Would something like the following change work ?
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index eb9b6ddbf92f..69e44358a235 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -140,7 +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_PROT
+ 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/mm/book3s64/pgtable.c
b/arch/powerpc/mm/book3s64/pgtable.c
index 052e6590f84f..59d235519b44 100644
--- a/arch/powerpc/mm/book3s64/pgtable.c
+++ b/arch/powerpc/mm/book3s64/pgtable.c
@@ -7,6 +7,7 @@
#include
#include
#include
Re: rcu_sched self-detected stall on CPU
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.
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.
Thanx, Paul
[powerpc:merge] BUILD SUCCESS 80a1583efa182af91e6a278e82b5431ec9fc8282
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git merge branch HEAD: 80a1583efa182af91e6a278e82b5431ec9fc8282 Automatic merge of 'fixes' into merge (2022-04-10 22:29) elapsed time: 726m configs tested: 167 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 arm64allyesconfig arm64 defconfig powerpc randconfig-c003-20220410 i386 randconfig-c001 sh urquell_defconfig powerpc mpc8540_ads_defconfig mips tb0226_defconfig arcnsimosci_defconfig armrealview_defconfig m68k bvme6000_defconfig shedosk7705_defconfig xtensa virt_defconfig powerpc taishan_defconfig powerpc currituck_defconfig nios2alldefconfig m68k apollo_defconfig powerpcadder875_defconfig arm imxrt_defconfig powerpc mpc85xx_cds_defconfig powerpc chrp32_defconfig powerpc pasemi_defconfig s390defconfig powerpc motionpro_defconfig arm badge4_defconfig powerpc ppc40x_defconfig armzeus_defconfig x86_64 defconfig armoxnas_v6_defconfig sparc sparc64_defconfig sh sh7724_generic_defconfig arm h3600_defconfig arm multi_v4t_defconfig arm pxa910_defconfig powerpc tqm8541_defconfig mips capcella_defconfig nios2 defconfig m68kmvme16x_defconfig xtensa iss_defconfig mips maltasmvp_eva_defconfig m68k m5249evb_defconfig xtensa defconfig armxcep_defconfig sh ecovec24_defconfig h8300allyesconfig m68k m5275evb_defconfig powerpcmpc7448_hpc2_defconfig sh r7780mp_defconfig xtensasmp_lx200_defconfig sh se7619_defconfig mips allyesconfig shhp6xx_defconfig arm randconfig-c002-20220411 arm randconfig-c002-20220410 i386 randconfig-c001-20220411 x86_64 randconfig-c001-20220411 x86_64randconfig-c001 ia64 allmodconfig ia64 allyesconfig ia64defconfig m68k allyesconfig m68k allmodconfig m68kdefconfig cskydefconfig nios2allyesconfig alpha defconfig alphaallyesconfig xtensa allyesconfig arc defconfig sh allmodconfig arc allyesconfig s390 allmodconfig parisc defconfig parisc64defconfig parisc allyesconfig s390 allyesconfig sparc defconfig i386 allyesconfig sparcallyesconfig i386defconfig i386 debian-10.3-kselftests i386 debian-10.3 mips allmodconfig powerpc allyesconfig powerpc allnoconfig powerpc allmodconfig x86_64randconfig-a013 x86_64randconfig-a011 x86_64randconfig-a015 i386 randconfig-a015-20220411 i386 randconfig-a011-20220411 i386 randconfig-a016-20220411 i386 randconfig-a012-20220411 i386 randconfig-a013-20220411 i386 randconfig-a014-20220411 i386 randconfig-a012 i386 randconfig-a014 i386 randconfig-a016 x86_64
[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 --- Comment #4 from Daniel Kolesa (li...@octaforge.org) --- Also, just to be clear, reverting the commit I linked above does fix the problem for me. Here is a patch you can quickly test: https://gist.github.com/q66/da01b4baecfdc24cd8fa3253d4e7f05a -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[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 --- Comment #3 from Daniel Kolesa (li...@octaforge.org) --- It does not panic in my case though; I merely get stuck with the offb framebuffer console instead of it switching modes to the right thing -- You may reply to this email to add a comment. You are receiving this mail because: You are watching the assignee of the bug.
[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 Daniel Kolesa (li...@octaforge.org) changed: What|Removed |Added CC||li...@octaforge.org --- Comment #2 from Daniel Kolesa (li...@octaforge.org) --- This now hits 5.15.33. I noticed this when virtio-gpu failed to come up. Commit: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/video/fbdev/core?h=linux-5.15.y=c894ac44786cfed383a6c6b20c1bfb12eb96018a More detailed backtrace: https://gist.github.com/q66/6ffc1bd18cf241e6ad894dc4409a2f72 This is also on a ppc64le system. However, I think this bug may not be ppc64 specific... -- 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: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop
On Sun, Apr 10, 2022 at 10:30:31PM +0200, Jakob Koschel wrote:
> > On 10. Apr 2022, at 22:02, Vladimir Oltean wrote:
> >
> > On Sun, Apr 10, 2022 at 08:24:37PM +0200, Jakob Koschel wrote:
> >> Btw, I just realized that the if (!pos) is not necessary. This should
> >> simply do it:
> >>
> >> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
> >> b/drivers/net/dsa/sja1105/sja1105_vl.c
> >> index b7e95d60a6e4..2d59e75a9e3d 100644
> >> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
> >> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
> >> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct
> >> sja1105_gating_config *gating_cfg,
> >>list_add(>list, _cfg->entries);
> >>} else {
> >> + struct list_head *pos = _cfg->entries;
> >>struct sja1105_gate_entry *p;
> >>
> >>list_for_each_entry(p, _cfg->entries, list) {
> >>if (p->interval == e->interval) {
> >> @@ -37,10 +38,12 @@ static int sja1105_insert_gate_entry(struct
> >> sja1105_gating_config *gating_cfg,
> >>goto err;
> >>}
> >>
> >> - if (e->interval < p->interval)
> >> + if (e->interval < p->interval) {
> >> + pos = >list;
> >>break;
> >> + }
> >>}
> >> - list_add(>list, p->list.prev);
> >> + list_add(>list, pos->prev);
> >>}
> >>
> >>gating_cfg->num_entries++;
> >> --
> >> 2.25.1
> >
> > I think we can give this a turn that is actually beneficial for the driver.
> > I've prepared and tested 3 patches on this function, see below.
> > Concrete improvements:
> > - that thing with list_for_each_entry() and list_for_each()
> > - no more special-casing of an empty list
> > - simplifying the error path
> > - that thing with list_add_tail()
> >
> > What do you think about the changes below?
>
> Thanks for all the good cooperation and help. The changes look great.
> I'll include them in v2 unless you want to do that separately, then I'll
> just remove them from the patch series.
I think it's less of a synchronization hassle if you send them along
with your list of others. Good luck.
Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop
> On 10. Apr 2022, at 22:02, Vladimir Oltean wrote:
>
> On Sun, Apr 10, 2022 at 08:24:37PM +0200, Jakob Koschel wrote:
>> Btw, I just realized that the if (!pos) is not necessary. This should simply
>> do it:
>>
>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
>> b/drivers/net/dsa/sja1105/sja1105_vl.c
>> index b7e95d60a6e4..2d59e75a9e3d 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct
>> sja1105_gating_config *gating_cfg,
>> list_add(>list, _cfg->entries);
>> } else {
>> +struct list_head *pos = _cfg->entries;
>> struct sja1105_gate_entry *p;
>>
>> list_for_each_entry(p, _cfg->entries, list) {
>> if (p->interval == e->interval) {
>> @@ -37,10 +38,12 @@ static int sja1105_insert_gate_entry(struct
>> sja1105_gating_config *gating_cfg,
>> goto err;
>> }
>>
>> -if (e->interval < p->interval)
>> +if (e->interval < p->interval) {
>> +pos = >list;
>> break;
>> +}
>> }
>> -list_add(>list, p->list.prev);
>> +list_add(>list, pos->prev);
>> }
>>
>> gating_cfg->num_entries++;
>> --
>> 2.25.1
>
> I think we can give this a turn that is actually beneficial for the driver.
> I've prepared and tested 3 patches on this function, see below.
> Concrete improvements:
> - that thing with list_for_each_entry() and list_for_each()
> - no more special-casing of an empty list
> - simplifying the error path
> - that thing with list_add_tail()
>
> What do you think about the changes below?
Thanks for all the good cooperation and help. The changes look great.
I'll include them in v2 unless you want to do that separately, then I'll
just remove them from the patch series.
>
> From 5b952b75e239cbe96729cf78c17e8d9725c9617c Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean
> Date: Sun, 10 Apr 2022 22:21:41 +0300
> Subject: [PATCH 1/3] net: dsa: sja1105: remove use of iterator after
> list_for_each_entry() loop
>
> Jakob Koschel explains in the link below 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
> ---
> 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_STATUS8
>
> +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
Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop
On Sun, Apr 10, 2022 at 08:24:37PM +0200, Jakob Koschel wrote:
> Btw, I just realized that the if (!pos) is not necessary. This should simply
> do it:
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
> b/drivers/net/dsa/sja1105/sja1105_vl.c
> index b7e95d60a6e4..2d59e75a9e3d 100644
> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct
> sja1105_gating_config *gating_cfg,
> list_add(>list, _cfg->entries);
> } else {
> + struct list_head *pos = _cfg->entries;
> struct sja1105_gate_entry *p;
>
> list_for_each_entry(p, _cfg->entries, list) {
> if (p->interval == e->interval) {
> @@ -37,10 +38,12 @@ static int sja1105_insert_gate_entry(struct
> sja1105_gating_config *gating_cfg,
> goto err;
> }
>
> - if (e->interval < p->interval)
> + if (e->interval < p->interval) {
> + pos = >list;
> break;
> + }
> }
> - list_add(>list, p->list.prev);
> + list_add(>list, pos->prev);
> }
>
> gating_cfg->num_entries++;
> --
> 2.25.1
I think we can give this a turn that is actually beneficial for the driver.
I've prepared and tested 3 patches on this function, see below.
Concrete improvements:
- that thing with list_for_each_entry() and list_for_each()
- no more special-casing of an empty list
- simplifying the error path
- that thing with list_add_tail()
What do you think about the changes below?
>From 5b952b75e239cbe96729cf78c17e8d9725c9617c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean
Date: Sun, 10 Apr 2022 22:21:41 +0300
Subject: [PATCH 1/3] net: dsa: sja1105: remove use of iterator after
list_for_each_entry() loop
Jakob Koschel explains in the link below 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
---
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 {
-
Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop
> On 10. Apr 2022, at 14:39, Jakob Koschel wrote:
>
>
>
>> On 10. Apr 2022, at 13:05, Vladimir Oltean wrote:
>>
>> On Sun, Apr 10, 2022 at 12:51:56PM +0200, Jakob Koschel wrote:
>>> I've just looked at this again in a bit more detail while integrating it
>>> into the patch series.
>>>
>>> I realized that this just shifts the 'problem' to using the 'pos' iterator
>>> variable after the loop.
>>> If the scope of the list iterator would be lowered to the list traversal
>>> loop it would also make sense
>>> to also do it for list_for_each().
>>
>> Yes, but list_for_each() was never formulated as being problematic in
>> the same way as list_for_each_entry(), was it? I guess I'm starting to
>> not understand what is the true purpose of the changes.
>
> Sorry for having caused the confusion. Let me elaborate a bit to give more
> context.
>
> There are two main benefits of this entire effort.
>
> 1) fix the architectural bugs and avoid any missuse of the list iterator
> after the loop
> by construction. This only concerns the list_for_each_entry_*() macros and
> your change
> will allow lowering the scope for all of those. It might be debatable that it
> would be
> more consistent to lower the scope for list_for_each() as well, but it
> wouldn't be
> strictly necessary.
>
> 2) fix *possible* speculative bugs. In our project, Kasper [1], we were able
> to show
> that this can be an issue for the list traversal macros (and this is how the
> entire
> effort started).
> The reason is that the processor might run an additional loop iteration in
> speculative
> execution with the iterator variable computed based on the head element. This
> can
> (and we have verified this) happen if the CPU incorrectly
> assumes !list_entry_is_head(pos, head, member).
>
> If this happens, all memory accesses based on the iterator variable
> *potentially* open
> the chance for spectre [2] gadgets. The proposed mitigation was setting the
> iterator variable
> to NULL when the terminating condition is reached (in speculative safe way).
> Then,
> the additional speculative list iteration would still execute but won't
> access any
> potential secret data.
>
> And this would also be required for list_for_each() since combined with the
> list_entry()
> within the loop it basically is semantically identical to
> list_for_each_entry()
> for the additional speculative iteration.
>
> Now, I have no strong opinion on going all the way and since 2) is not the
> main motivation
> for this I'm also fine with sticking to your proposed solution, but it would
> mean that implementing
> a "speculative safe" list_for_each() will be more difficult in the future
> since it is using
> the iterator of list_for_each() past the loop.
>
> I hope this explains the background a bit better.
>
>>
>>> What do you think about doing it this way:
>>>
>>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
>>> b/drivers/net/dsa/sja1105/sja1105_vl.c
>>> index b7e95d60a6e4..f5b0502c1098 100644
>>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>>> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct
>>> sja1105_gating_config *gating_cfg,
>>> list_add(>list, _cfg->entries);
>>> } else {
>>> struct sja1105_gate_entry *p;
>>> + struct list_head *pos = NULL;
>>>
>>> list_for_each_entry(p, _cfg->entries, list) {
>>> if (p->interval == e->interval) {
>>> @@ -37,10 +38,14 @@ static int sja1105_insert_gate_entry(struct
>>> sja1105_gating_config *gating_cfg,
>>> goto err;
>>> }
>>>
>>> - if (e->interval < p->interval)
>>> + if (e->interval < p->interval) {
>>> + pos = >list;
>>> break;
>>> + }
>>> }
>>> - list_add(>list, p->list.prev);
>>> + if (!pos)
>>> + pos = _cfg->entries;
>>> + list_add(>list, pos->prev);
>>> }
>>>
>>> gating_cfg->num_entries++;
>>> --
>>>
Thanks for the suggestion.
> }
>
> gating_cfg->num_entries++;
> -[ cut here ]-
[1]
https://lore.kernel.org/linux-kernel/20220407102900.3086255-12-jakobkosc...@gmail.com/
Jakob
>>>
>>> Thanks,
>>> Jakob
>
> Thanks,
> Jakob
>
> [1] https://www.vusec.net/projects/kasper/
> [2] https://spectreattack.com/spectre.pdf
Btw, I just realized that the if (!pos) is not necessary. This should simply do
it:
diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
b/drivers/net/dsa/sja1105/sja1105_vl.c
index b7e95d60a6e4..2d59e75a9e3d 100644
--- a/drivers/net/dsa/sja1105/sja1105_vl.c
+++ b/drivers/net/dsa/sja1105/sja1105_vl.c
@@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct
sja1105_gating_config *gating_cfg,
list_add(>list, _cfg->entries);
} else {
+ struct list_head *pos = _cfg->entries;
struct sja1105_gate_entry *p;
list_for_each_entry(p, _cfg->entries, list) {
if (p->interval == e->interval) {
@@ -37,10
Re: [GIT PULL] Please pull powerpc/linux.git powerpc-5.18-2 tag
The pull request you sent on Sun, 10 Apr 2022 23:23:35 +1000: > https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git > tags/powerpc-5.18-2 has been merged into torvalds/linux.git: https://git.kernel.org/torvalds/c/4ea3c6425269d33da53c79d539ce9554117cf4d4 Thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/prtracker.html
[PATCH v2] macintosh: via-pmu and via-cuda need RTC_LIB
Fix build when RTC_LIB is not set/enabled.
Eliminates these build errors:
m68k-linux-ld: drivers/macintosh/via-pmu.o: in function `pmu_set_rtc_time':
drivers/macintosh/via-pmu.c:1769: undefined reference to `rtc_tm_to_time64'
m68k-linux-ld: drivers/macintosh/via-cuda.o: in function `cuda_set_rtc_time':
drivers/macintosh/via-cuda.c:797: undefined reference to `rtc_tm_to_time64'
Fixes: 0792a2c8e0bb ("macintosh: Use common code to access RTC")
Signed-off-by: Randy Dunlap
Reported-by: kernel test robot
Suggested-by: Christophe Leroy
Cc: Benjamin Herrenschmidt
Cc: Michael Ellerman
Cc: Kees Cook
Cc: Arnd Bergmann
Cc: Finn Thain
Cc: Geert Uytterhoeven
Cc: Nathan Chancellor
Cc: Nick Desaulniers
Cc: linuxppc-dev@lists.ozlabs.org
---
v2: use RTC_LIB instead of open-coding the call to rtc_tm_to_time64()
drivers/macintosh/Kconfig |2 ++
1 file changed, 2 insertions(+)
--- a/drivers/macintosh/Kconfig
+++ b/drivers/macintosh/Kconfig
@@ -44,6 +44,7 @@ config ADB_IOP
config ADB_CUDA
bool "Support for Cuda/Egret based Macs and PowerMacs"
depends on (ADB || PPC_PMAC) && !PPC_PMAC64
+ select RTC_LIB
help
This provides support for Cuda/Egret based Macintosh and
Power Macintosh systems. This includes most m68k based Macs,
@@ -57,6 +58,7 @@ config ADB_CUDA
config ADB_PMU
bool "Support for PMU based PowerMacs and PowerBooks"
depends on PPC_PMAC || MAC
+ select RTC_LIB
help
On PowerBooks, iBooks, and recent iMacs and Power Macintoshes, the
PMU is an embedded microprocessor whose primary function is to
Re: [PATCH] macintosh: fix via-pmu and via-cuda build without RTC_CLASS
Hi--
On 4/10/22 00:03, Christophe Leroy wrote:
>
>
> Le 09/04/2022 à 04:08, Randy Dunlap a écrit :
>> Fix build when RTC_CLASS is not set/enabled.
>> Eliminates these build errors:
>>
>> m68k-linux-ld: drivers/macintosh/via-pmu.o: in function `pmu_set_rtc_time':
>> drivers/macintosh/via-pmu.c:1769: undefined reference to `rtc_tm_to_time64'
>> m68k-linux-ld: drivers/macintosh/via-cuda.o: in function `cuda_set_rtc_time':
>> drivers/macintosh/via-cuda.c:797: undefined reference to `rtc_tm_to_time64'
>
> You don't need RTC_CLASS for that. All you need is RTC_LIB I think.
Oh good.
> What about selecting RTC_LIB from m68k Kconfig just like powerpc and a
> few architectures do ?
Sounds good. I'll test that now.
> See
> https://elixir.bootlin.com/linux/v5.18-rc1/source/arch/powerpc/Kconfig#L269
>
> Otherwise, I think it would be better to move (and rename)
> rtc_tm_to_time64() into kernel/time/time.c instead of opencoding it twice.
>
Yeah, I didn't like that either.
>>
>> Fixes: 0792a2c8e0bb ("macintosh: Use common code to access RTC")
>> Signed-off-by: Randy Dunlap
>> Reported-by: kernel test robot
>> Cc: Benjamin Herrenschmidt
>> Cc: Michael Ellerman
>> Cc: Christophe Leroy
>> Cc: Kees Cook
>> Cc: Arnd Bergmann
>> Cc: Finn Thain
>> Cc: Geert Uytterhoeven
>> Cc: Nathan Chancellor
>> Cc: Nick Desaulniers
>> Cc: linuxppc-dev@lists.ozlabs.org
>> ---
>> drivers/macintosh/via-cuda.c |5 -
>> drivers/macintosh/via-pmu.c |5 -
>> 2 files changed, 8 insertions(+), 2 deletions(-)
thanks.
--
~Randy
[GIT PULL] Please pull powerpc/linux.git powerpc-5.18-2 tag
-BEGIN PGP SIGNED MESSAGE- Hash: SHA256 Hi Linus, Please pull powerpc fixes for 5.18: The following changes since commit f82da161ea75dc4db21b2499e4b1facd36dab275: powerpc: restore removed #endif (2022-03-27 15:31:16 -0700) are available in the git repository at: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git tags/powerpc-5.18-2 for you to fetch changes up to 1ff5c8e8c835e8a81c0868e3050c76563dd56a2c: Revert "powerpc: Set max_mapnr correctly" (2022-04-07 08:54:35 +1000) - -- powerpc fixes for 5.18 #2 - Fix KVM "lost kick" race, where an attempt to pull a vcpu out of the guest could be lost (or delayed until the next guest exit). - Disable SCV (system call vectored) when PR KVM guests could be run. - Fix KVM PR guests using SCV, by disallowing AIL != 0 for KVM PR guests. - Add a new KVM CAP to indicate if AIL == 3 is supported. - Fix a regression when hotplugging a CPU to a memoryless/cpuless node. - Make virt_addr_valid() stricter for 64-bit Book3E & 32-bit, which fixes crashes seen due to hardened usercopy. - Revert a change to max_mapnr which broke HIGHMEM. Thanks to: Christophe Leroy, Fabiano Rosas, Kefeng Wang, Nicholas Piggin, Srikar Dronamraju. - -- Christophe Leroy (1): powerpc/64: Fix build failure with allyesconfig in book3s_64_entry.S Kefeng Wang (2): powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit Revert "powerpc: Set max_mapnr correctly" Michael Ellerman (3): Merge branch 'kvm-ppc-cap-210' of https://git.kernel.org/pub/scm/virt/kvm/kvm into topic/ppc-kvm Merge branch 'topic/ppc-kvm' into next KVM: PPC: Move kvmhv_on_pseries() into kvm_ppc.h Nicholas Piggin (4): KVM: PPC: Book3S HV P9: Fix "lost kick" race KVM: PPC: Book3S PR: Disable SCV when AIL could be disabled KVM: PPC: Book3S PR: Disallow AIL != 0 KVM: PPC: Use KVM_CAP_PPC_AIL_MODE_3 Srikar Dronamraju (1): powerpc/numa: Handle partially initialized numa nodes arch/powerpc/include/asm/kvm_book3s_64.h | 12 -- arch/powerpc/include/asm/kvm_ppc.h | 12 ++ arch/powerpc/include/asm/page.h | 6 ++- arch/powerpc/include/asm/setup.h | 2 + arch/powerpc/kernel/exceptions-64s.S | 4 ++ arch/powerpc/kernel/setup_64.c | 28 + arch/powerpc/kvm/Kconfig | 9 + arch/powerpc/kvm/book3s_64_entry.S | 10 - arch/powerpc/kvm/book3s_hv.c | 41 arch/powerpc/kvm/book3s_pr.c | 26 - arch/powerpc/kvm/book3s_pr_papr.c| 20 ++ arch/powerpc/kvm/powerpc.c | 17 arch/powerpc/mm/mem.c| 2 +- arch/powerpc/mm/numa.c | 2 +- arch/powerpc/platforms/pseries/setup.c | 13 ++- 15 files changed, 169 insertions(+), 35 deletions(-) -BEGIN PGP SIGNATURE- iQIzBAEBCAAdFiEEJFGtCPCthwEv2Y/bUevqPMjhpYAFAmJS2fgACgkQUevqPMjh pYBt1BAAgsfryhBqA9GD/hEgNpk08Pb214OAAGkKF4uStVs3kAlVhuVeDUThGt5B z3OpvAL8lnOOkQ6S/fl0FXMAU5URJhdXTNwmS3g557VIWAVblWEc+oGMOeq2aJIi hmlFoLUd7Ge31I03+OZ5IIlOuOc0DM8Qe6zpF25feg3Xw0oQzMHCKgPfnsWbQz+Q 116NhWE8r35eCmrZEsamaTaUBb8ox9aklVaNNyYm/fJgUUuZg9ROb+pmf4s4y5uP x+DQW5RawMV6S7+co7Je8wJB4wGDG0aO/edqUPmdkVW8jksUv5aTD3jr3aQeSA8l YwICU8wIZBd9Ps3W8/1LWVfNCyfdcbafOCztirrIAHpD7BdKUizvo8zyQCMTATqB m2mHI+EPOGcE/P8HRhLENnjBz6CTU0ZFuSaxpmyDTCOewDP9zzIGkEL5qwZ9EOcY 7XlqEEK0PiS/rBmigN8R5bv+H9WsJ6Lb2mOVkoSo18ujM5oclhyWw6OlAVmpLybU ZauoiM31SINO5LHX+M9P0o8FHtauRLPCHYxmB6akhnTPqYhwfqi+fWxrqbmBtEba dr+MTnAVMEFIt4ZdzFOPtZcM+nSGUX/CNfUO4DiCW9MgB9F24kx/lmy+MljQ0gvS 2jxt0ylnTUVxb1XeGZMVHD1CCz0vZhntmAxewbkoltuZuMufJdo= =WTqS -END PGP SIGNATURE-
Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop
> On 10. Apr 2022, at 13:05, Vladimir Oltean wrote:
>
> On Sun, Apr 10, 2022 at 12:51:56PM +0200, Jakob Koschel wrote:
>> I've just looked at this again in a bit more detail while integrating it
>> into the patch series.
>>
>> I realized that this just shifts the 'problem' to using the 'pos' iterator
>> variable after the loop.
>> If the scope of the list iterator would be lowered to the list traversal
>> loop it would also make sense
>> to also do it for list_for_each().
>
> Yes, but list_for_each() was never formulated as being problematic in
> the same way as list_for_each_entry(), was it? I guess I'm starting to
> not understand what is the true purpose of the changes.
Sorry for having caused the confusion. Let me elaborate a bit to give more
context.
There are two main benefits of this entire effort.
1) fix the architectural bugs and avoid any missuse of the list iterator after
the loop
by construction. This only concerns the list_for_each_entry_*() macros and your
change
will allow lowering the scope for all of those. It might be debatable that it
would be
more consistent to lower the scope for list_for_each() as well, but it wouldn't
be
strictly necessary.
2) fix *possible* speculative bugs. In our project, Kasper [1], we were able to
show
that this can be an issue for the list traversal macros (and this is how the
entire
effort started).
The reason is that the processor might run an additional loop iteration in
speculative
execution with the iterator variable computed based on the head element. This
can
(and we have verified this) happen if the CPU incorrectly
assumes !list_entry_is_head(pos, head, member).
If this happens, all memory accesses based on the iterator variable
*potentially* open
the chance for spectre [2] gadgets. The proposed mitigation was setting the
iterator variable
to NULL when the terminating condition is reached (in speculative safe way).
Then,
the additional speculative list iteration would still execute but won't access
any
potential secret data.
And this would also be required for list_for_each() since combined with the
list_entry()
within the loop it basically is semantically identical to list_for_each_entry()
for the additional speculative iteration.
Now, I have no strong opinion on going all the way and since 2) is not the main
motivation
for this I'm also fine with sticking to your proposed solution, but it would
mean that implementing
a "speculative safe" list_for_each() will be more difficult in the future since
it is using
the iterator of list_for_each() past the loop.
I hope this explains the background a bit better.
>
>> What do you think about doing it this way:
>>
>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
>> b/drivers/net/dsa/sja1105/sja1105_vl.c
>> index b7e95d60a6e4..f5b0502c1098 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct
>> sja1105_gating_config *gating_cfg,
>>list_add(>list, _cfg->entries);
>>} else {
>>struct sja1105_gate_entry *p;
>> + struct list_head *pos = NULL;
>>
>>list_for_each_entry(p, _cfg->entries, list) {
>>if (p->interval == e->interval) {
>> @@ -37,10 +38,14 @@ static int sja1105_insert_gate_entry(struct
>> sja1105_gating_config *gating_cfg,
>>goto err;
>>}
>>
>> - if (e->interval < p->interval)
>> + if (e->interval < p->interval) {
>> + pos = >list;
>>break;
>> + }
>>}
>> - list_add(>list, p->list.prev);
>> + if (!pos)
>> + pos = _cfg->entries;
>> + list_add(>list, pos->prev);
>>}
>>
>>gating_cfg->num_entries++;
>> --
>>
>>>
>>> Thanks for the suggestion.
>>>
}
gating_cfg->num_entries++;
-[ cut here ]-
>>>
>>> [1]
>>> https://lore.kernel.org/linux-kernel/20220407102900.3086255-12-jakobkosc...@gmail.com/
>>>
>>> Jakob
>>
>> Thanks,
>> Jakob
Thanks,
Jakob
[1] https://www.vusec.net/projects/kasper/
[2] https://spectreattack.com/spectre.pdf
Re: [PATCH] powerpc/numa: Handle partially initialized numa nodes
On Wed, 30 Mar 2022 19:21:23 +0530, Srikar Dronamraju wrote:
> With commit 09f49dca570a ("mm: handle uninitialized numa nodes
> gracefully") NODE_DATA for even a memoryless/cpuless node is partially
> initialized at boot time.
>
> Before onlining the node, current Powerpc code checks for NODE_DATA to
> be NULL. However since NODE_DATA is partially initialized, this check
> will end up always being false.
>
> [...]
Applied to powerpc/fixes.
[1/1] powerpc/numa: Handle partially initialized numa nodes
https://git.kernel.org/powerpc/c/e4ff77598a109bd36789ad5e80aba66fc53d0ffb
cheers
Re: [PATCH 1/6] powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit
On Thu, 7 Apr 2022 00:57:57 +1000, Michael Ellerman wrote: > From: Kefeng Wang > > mpe: On 64-bit Book3E vmalloc space starts at 0x8000. > > Because of the way __pa() works we have: > __pa(0x8000) == 0, and therefore > virt_to_pfn(0x8000) == 0, and therefore > virt_addr_valid(0x8000) == true > > [...] Patches 1 & 2 applied to powerpc/fixes. [1/6] powerpc: Fix virt_addr_valid() for 64-bit Book3E & 32-bit https://git.kernel.org/powerpc/c/ffa0b64e3be58519ae472ea29a1a1ad681e32f48 [2/6] Revert "powerpc: Set max_mapnr correctly" https://git.kernel.org/powerpc/c/1ff5c8e8c835e8a81c0868e3050c76563dd56a2c cheers
Re: [PATCH] powerpc/64: Fix build failure with allyesconfig in book3s_64_entry.S
On Sun, 27 Mar 2022 09:32:26 +0200, Christophe Leroy wrote: > Using conditional branches between two files is hasardous, > they may get linked to far from each other. > > arch/powerpc/kvm/book3s_64_entry.o:(.text+0x3ec): relocation truncated > to fit: R_PPC64_REL14 (stub) against symbol `system_reset_common' > defined in .text section in arch/powerpc/kernel/head_64.o > > [...] Applied to powerpc/fixes. [1/1] powerpc/64: Fix build failure with allyesconfig in book3s_64_entry.S https://git.kernel.org/powerpc/c/af41d2866f7d75bbb38d487f6ec7770425d70e45 cheers
Re: rcu_sched self-detected stall on CPU
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 ?
cheers
Re: [PATCH] powerpc/numa: Handle partially initialized numa nodes
Srikar Dronamraju writes:
> * Oscar Salvador [2022-04-06 18:19:00]:
>
>> On Wed, Mar 30, 2022 at 07:21:23PM +0530, Srikar Dronamraju wrote:
>> > arch/powerpc/mm/numa.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/arch/powerpc/mm/numa.c b/arch/powerpc/mm/numa.c
>> > index b9b7fefbb64b..13022d734951 100644
>> > --- a/arch/powerpc/mm/numa.c
>> > +++ b/arch/powerpc/mm/numa.c
>> > @@ -1436,7 +1436,7 @@ int find_and_online_cpu_nid(int cpu)
>> >if (new_nid < 0 || !node_possible(new_nid))
>> >new_nid = first_online_node;
>> >
>> > - if (NODE_DATA(new_nid) == NULL) {
>> > + if (!node_online(new_nid)) {
>> > #ifdef CONFIG_MEMORY_HOTPLUG
>> >/*
>> > * Need to ensure that NODE_DATA is initialized for a node from
>>
>> Because of this fix, I wanted to check whether we might have any more
>> fallouts due
>> to ("mm: handle uninitialized numa nodes gracefully"), and it made me look
>> closer
>> as to why powerpc is the only platform that special cases try_online_node(),
>> while all others rely on cpu_up()->try_online_node() to do the right thing.
>>
>> So, I had a look.
>> Let us rewind a bit:
>>
>> The commit that sets find_and_online_cpu_nid() in dlpar_online_cpu was
>> e67e02a544e9 ("powerpc/pseries: Fix cpu hotplug crash with memoryless
>> nodes").
>>
>> In there, it says that we have the following picture:
>>
>> partition_sched_domains
>> arch_update_cpu_topology
>> numa_update_cpu_topology
>>find_and_online_cpu_nid
>>
>> and by the time find_and_online_cpu_nid() gets called to online the node, it
>> might be
>> too late as we might have referenced some NODE_DATA() fields.
>> Note that this happens at a much later stage in cpuhp.
>>
>> Also note that at a much earlier stage, we do already have a
>> try_online_node() in cpu_up(),
>> which should allocate-and-online the node and prevent accessing garbage.
>> But the problem is that, on powerpc, all possible cpus have the same node
>> set at boot stage
>> (see arch/powerpc/mm/numa.c:mem_topology_setup),
>> so cpu_to_node() returns the same thing until it the mapping gets update
>> (which happens in
>> start_secondary()->set_numa_node()), and so, the try_online_node() from
>> cpu_up() does not
>> apply on the right node, because it still holds the not-up-to-date mapping
>> node <-> cpu.
>>
>> (e.g: in my test case, when onlining a CPU belongin to node1,
>> cpu_up()->try_online_node()
>> tries to online node0, or whatever old mapping numa<->cpu is there).
>>
>> So, we have something like:
>>
>> dlpar_online_cpu
>> device_online
>> dev->bus->online
>>cpu_subsys_online
>> cpu_device_up
>> cpu_up
>> try_online_node (old mapping nid <-> cpu )
>> ...
>> ...
>> cphp_callbacks
>>sched_cpu_activate
>> cpuset_update_active_cpus
>> schedule_work(_hotplug_work)
>> cpuset_hotplug_work
>>partition_sched_domains
>> arch_update_cpu_topology
>> numa_update_cpu_topology
>> find_and_online_cpu_nid (online new_nid)
>>
>>
>> So, yeah, the real onlining in
>> numa_update_cpu_topology()->find_and_online_cpu_nid()
>> happens too late, that is why adding find_and_online_cpu_nid() back in
>> dlpar_online_cpu()
>> fixed the issue, but we should not need this special casing at all.
>>
>> We do already know the numa<->cpu associativity in
>> dlpar_online_cpu()->find_and_online_cpu_nid(), so we should just be able to
>> update the numa<->cpu mapping, and let the try_online_node() do the right
>> thing.
>>
>> With this in mind, I came up with the following patch, where I carried out a
>> battery
>> of tests for CPU hotplug stuff and it worked as expected.
>> But I am not familiar with all powerpc pitfalls, e.g: dedicated vs shared
>> cpus etc, so
>> I would like to hear from more familiar people.
>>
>> The patch is:
>>
>> From: Oscar Salvador
>> Date: Wed, 6 Apr 2022 14:39:15 +0200
>> Subject: [PATCH] powerpc/numa: Associate numa node to its cpu earlier
>>
>> powerpc is the only platform that do not rely on
>> cpu_up()->try_online_node() to bring up a numa node,
>> and special cases it, instead, deep in its own machinery:
>>
>> dlpar_online_cpu
>> find_and_online_cpu_nid
>> try_online_node
>>
>> This should not be needed, but the thing is that the try_online_node()
>> from cpu_up() will not apply on the right node, because cpu_to_node()
>> will return the old mapping numa<->cpu that gets set on boot stage
>> for all possible cpus.
>>
>> That can be seen easily if we try to print out the numa node passed
>> to try_online_node() in cpu_up().
>>
>> The thing is that the numa<->cpu mapping does not get updated till a much
>> later stage in start_secondary:
>>
>> start_secondary:
>> set_numa_node(numa_cpu_lookup_table[cpu])
>>
>> But we do not really care, as we already now the
>> CPU <-> NUMA associativity back in find_and_online_cpu_nid(),
>>
Re: [PATCH v2 1/2] powerpc/powernv: Get L1D flush requirements from device-tree
Joel Stanley writes:
> On Tue, 5 Apr 2022 at 06:13, Michael Ellerman wrote:
>>
>> Joel Stanley writes:
>> > On Mon, 4 Apr 2022 at 10:15, Russell Currey wrote:
>> >>
>> >> The device-tree properties no-need-l1d-flush-msr-pr-1-to-0 and
>> >> no-need-l1d-flush-kernel-on-user-access are the equivalents of
>> >> H_CPU_BEHAV_NO_L1D_FLUSH_ENTRY and H_CPU_BEHAV_NO_L1D_FLUSH_UACCESS
>> >> from the H_GET_CPU_CHARACTERISTICS hcall on pseries respectively.
>> >>
>> >> In commit d02fa40d759f ("powerpc/powernv: Remove POWER9 PVR version
>> >> check for entry and uaccess flushes") the condition for disabling the
>> >> L1D flush on kernel entry and user access was changed from any non-P9
>> >> CPU to only checking P7 and P8. Without the appropriate device-tree
>> >> checks for newer processors on powernv, these flushes are unnecessarily
>> >> enabled on those systems. This patch corrects this.
>> >>
>> >> Fixes: d02fa40d759f ("powerpc/powernv: Remove POWER9 PVR version check
>> >> for entry and uaccess flushes")
>> >> Reported-by: Joel Stanley
>> >> Signed-off-by: Russell Currey
>> >
>> > I booted both patches in this series on a power10 powernv machine,
>> > applied on top of v5.18-rc1:
>> >
>> > $ dmesg |grep -i flush
>> > [0.00] rfi-flush: fallback displacement flush available
>> > [0.00] rfi-flush: patched 12 locations (no flush)
>> > [0.00] count-cache-flush: flush disabled.
>> > [0.00] link-stack-flush: flush disabled.
>> >
>> > $ grep . /sys/devices/system/cpu/vulnerabilities/*
>> > /sys/devices/system/cpu/vulnerabilities/itlb_multihit:Not affected
>> > /sys/devices/system/cpu/vulnerabilities/l1tf:Not affected
>> > /sys/devices/system/cpu/vulnerabilities/mds:Not affected
>> > /sys/devices/system/cpu/vulnerabilities/meltdown:Not affected
>> > /sys/devices/system/cpu/vulnerabilities/spec_store_bypass:Not affected
>> > /sys/devices/system/cpu/vulnerabilities/spectre_v1:Mitigation: __user
>> > pointer sanitization, ori31 speculation barrier enabled
>> > /sys/devices/system/cpu/vulnerabilities/spectre_v2:Mitigation:
>> > Software count cache flush (hardware accelerated), Software link stack
>> > flush
>> > /sys/devices/system/cpu/vulnerabilities/srbds:Not affected
>> > /sys/devices/system/cpu/vulnerabilities/tsx_async_abort:Not affected
>> >
>> > Does that match what we expect?
>>
>> AFAIK yes. Happy for ruscur to correct me though.
>>
>> Can you also try running the kernel selftests under
>> tools/testing/selftests/powerpc/security/ ?
>
> Here's the results when booted with no_spectrev2 (which I keep on
> doing by accident, as this machine has it in it's nvram):
>
> make[1]: Entering directory
> '/home/joel/dev/kernels/linus/tools/testing/selftests/powerpc/security'
> TAP version 13
> 1..5
> # selftests: powerpc/security: rfi_flush
> # test: rfi_flush_test
> # tags: git_version:v5.18-rc1-0-g312310928417
> # PASS (L1D misses with rfi_flush=0: 63101900 < 9500) [10/10 pass]
> # PASS (L1D misses with rfi_flush=1: 196001003 > 19000) [10/10 pass]
> # success: rfi_flush_test
> ok 1 selftests: powerpc/security: rfi_flush
> # selftests: powerpc/security: entry_flush
> # test: entry_flush_test
> # tags: git_version:v5.18-rc1-0-g312310928417
> # PASS (L1D misses with entry_flush=0: 52766044 < 9500) [10/10 pass]
> # PASS (L1D misses with entry_flush=1: 196082833 > 19000) [10/10 pass]
> # success: entry_flush_test
> ok 2 selftests: powerpc/security: entry_flush
> # selftests: powerpc/security: uaccess_flush
> # test: uaccess_flush_test
> # tags: git_version:v5.18-rc1-0-g312310928417
> # PASS (L1D misses with uaccess_flush=0: 68646267 < 9500) [10/10 pass]
> # PASS (L1D misses with uaccess_flush=1: 194177355 > 19000) [10/10 pass]
> # success: uaccess_flush_test
> ok 3 selftests: powerpc/security: uaccess_flush
> # selftests: powerpc/security: spectre_v2
> # test: spectre_v2
> # tags: git_version:v5.18-rc1-0-g312310928417
> # sysfs reports: 'Vulnerable'
> # PM_BR_PRED_CCACHE: result 0 running/enabled 2090650862
> # PM_BR_MPRED_CCACHE: result 1 running/enabled 2090648016
> # Miss percent 0 %
> # OK - Measured branch prediction rates match reported spectre v2 mitigation.
> # success: spectre_v2
> ok 4 selftests: powerpc/security: spectre_v2
> # selftests: powerpc/security: mitigation-patching.sh
> # Spawned threads enabling/disabling mitigations ...
> # Waiting for timeout ...
> # OK
> ok 5 selftests: powerpc/security: mitigation-patching.sh
>
>
> Here's the same thing without the command line option set:
Thanks.
> # test: spectre_v2
> # tags: git_version:v5.18-rc1-0-g312310928417
> # sysfs reports: 'Mitigation: Software count cache flush (hardware
> accelerated), Software link stack flush'
> # PM_BR_PRED_CCACHE: result 0 running/enabled 2016985490
> # PM_BR_MPRED_CCACHE: result 1 running/enabled 2016981520
> # Miss percent 0 %
> # OK - Measured branch prediction rates match reported spectre v2 mitigation.
> # success: spectre_v2
Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop
On Sun, Apr 10, 2022 at 12:51:56PM +0200, Jakob Koschel wrote:
> I've just looked at this again in a bit more detail while integrating it into
> the patch series.
>
> I realized that this just shifts the 'problem' to using the 'pos' iterator
> variable after the loop.
> If the scope of the list iterator would be lowered to the list traversal loop
> it would also make sense
> to also do it for list_for_each().
Yes, but list_for_each() was never formulated as being problematic in
the same way as list_for_each_entry(), was it? I guess I'm starting to
not understand what is the true purpose of the changes.
> What do you think about doing it this way:
>
> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
> b/drivers/net/dsa/sja1105/sja1105_vl.c
> index b7e95d60a6e4..f5b0502c1098 100644
> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
> @@ -28,6 +28,7 @@ static int sja1105_insert_gate_entry(struct
> sja1105_gating_config *gating_cfg,
> list_add(>list, _cfg->entries);
> } else {
> struct sja1105_gate_entry *p;
> + struct list_head *pos = NULL;
>
> list_for_each_entry(p, _cfg->entries, list) {
> if (p->interval == e->interval) {
> @@ -37,10 +38,14 @@ static int sja1105_insert_gate_entry(struct
> sja1105_gating_config *gating_cfg,
> goto err;
> }
>
> - if (e->interval < p->interval)
> + if (e->interval < p->interval) {
> + pos = >list;
> break;
> + }
> }
> - list_add(>list, p->list.prev);
> + if (!pos)
> + pos = _cfg->entries;
> + list_add(>list, pos->prev);
> }
>
> gating_cfg->num_entries++;
> --
>
> >
> > Thanks for the suggestion.
> >
> >>}
> >>
> >>gating_cfg->num_entries++;
> >> -[ cut here ]-
> >
> > [1]
> > https://lore.kernel.org/linux-kernel/20220407102900.3086255-12-jakobkosc...@gmail.com/
> >
> > Jakob
>
> Thanks,
> Jakob
Re: [PATCH net-next 02/15] net: dsa: sja1105: Remove usage of iterator for list_add() after loop
Hey Vladimir,
> On 9. Apr 2022, at 01:54, Jakob Koschel wrote:
>
> Hello Vladimir,
>
>> On 8. Apr 2022, at 13:41, Vladimir Oltean wrote:
>>
>> Hello Jakob,
>>
>> On Thu, Apr 07, 2022 at 12:28:47PM +0200, Jakob Koschel wrote:
>>> 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].
>>>
>>> Before, the code implicitly used the head when no element was found
>>> when using >list. Since the new variable is only set if an
>>> element was found, the list_add() is performed within the loop
>>> and only done after the loop if it is done on the list head directly.
>>>
>>> Link:
>>> https://lore.kernel.org/all/CAHk-=wgRr_D8CB-D9Kg-c=ehreask5sqxpwr9y7k9sa6cwx...@mail.gmail.com/
>>> [1]
>>> Signed-off-by: Jakob Koschel
>>> ---
>>> drivers/net/dsa/sja1105/sja1105_vl.c | 14 +-
>>> 1 file changed, 9 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
>>> b/drivers/net/dsa/sja1105/sja1105_vl.c
>>> index b7e95d60a6e4..cfcae4d19eef 100644
>>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>>> @@ -27,20 +27,24 @@ static int sja1105_insert_gate_entry(struct
>>> sja1105_gating_config *gating_cfg,
>>> if (list_empty(_cfg->entries)) {
>>> list_add(>list, _cfg->entries);
>>> } else {
>>> - struct sja1105_gate_entry *p;
>>> + struct sja1105_gate_entry *p = NULL, *iter;
>>>
>>> - list_for_each_entry(p, _cfg->entries, list) {
>>> - if (p->interval == e->interval) {
>>> + list_for_each_entry(iter, _cfg->entries, list) {
>>> + if (iter->interval == e->interval) {
>>> NL_SET_ERR_MSG_MOD(extack,
>>> "Gate conflict");
>>> rc = -EBUSY;
>>> goto err;
>>> }
>>>
>>> - if (e->interval < p->interval)
>>> + if (e->interval < iter->interval) {
>>> + p = iter;
>>> + list_add(>list, iter->list.prev);
>>> break;
>>> + }
>>> }
>>> - list_add(>list, p->list.prev);
>>> + if (!p)
>>> + list_add(>list, gating_cfg->entries.prev);
>>> }
>>>
>>> gating_cfg->num_entries++;
>>> --
>>> 2.25.1
>>>
>>
>> I apologize in advance if I've misinterpreted the end goal of your patch.
>> I do have a vague suspicion I understand what you're trying to achieve,
>> and in that case, would you mind using this patch instead of yours?
>
> I think you are very much spot on!
>
>> I think it still preserves the intention of the code in a clean manner.
>>
>> -[ cut here ]-
>> From 7aed740750d1bc3bff6e85fd33298f5905bb4e01 Mon Sep 17 00:00:00 2001
>> From: Vladimir Oltean
>> Date: Fri, 8 Apr 2022 13:55:14 +0300
>> Subject: [PATCH] net: dsa: sja1105: avoid use of type-confused pointer in
>> sja1105_insert_gate_entry()
>>
>> It appears that list_for_each_entry() leaks a type-confused pointer when
>> the iteration loop ends with no early break, since "*p" will no longer
>> point to a "struct sja1105_gate_entry", but rather to some memory in
>> front of "gating_cfg->entries".
>>
>> This isn't actually a problem here, because if the element we insert has
>> the highest interval, therefore we never exit the loop early, "p->list"
>> (which is all that we use outside the loop) will in fact point to
>> "gating_cfg->entries" even though "p" itself is invalid.
>>
>> Nonetheless, there are preparations to increase the safety of
>> list_for_each_entry() by making it impossible to use the encapsulating
>> structure of the iterator element outside the loop. So something needs
>> to change here before those preparations go in, even though this
>> constitutes legitimate use.
>>
>> Make it clear that we are not dereferencing members of the encapsulating
>> "struct sja1105_gate_entry" outside the loop, by using the regular
>> list_for_each() iterator, and dereferencing the struct sja1105_gate_entry
>> only within the loop.
>>
>> With list_for_each(), the iterator element at the end of the loop does
>> have a sane value in all cases, and we can just use that as the "head"
>> argument of list_add().
>>
>> Signed-off-by: Vladimir Oltean
>> ---
>> drivers/net/dsa/sja1105/sja1105_vl.c | 12 +---
>> 1 file changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/dsa/sja1105/sja1105_vl.c
>> b/drivers/net/dsa/sja1105/sja1105_vl.c
>> index c0e45b393fde..fe93c80fe5ef 100644
>> --- a/drivers/net/dsa/sja1105/sja1105_vl.c
>> +++ b/drivers/net/dsa/sja1105/sja1105_vl.c
>> @@ -27,9 +27,15 @@ static int sja1105_insert_gate_entry(struct
>> sja1105_gating_config *gating_cfg,
>> if
Re: [PATCH] macintosh: fix via-pmu and via-cuda build without RTC_CLASS
Le 09/04/2022 à 04:08, Randy Dunlap a écrit :
> Fix build when RTC_CLASS is not set/enabled.
> Eliminates these build errors:
>
> m68k-linux-ld: drivers/macintosh/via-pmu.o: in function `pmu_set_rtc_time':
> drivers/macintosh/via-pmu.c:1769: undefined reference to `rtc_tm_to_time64'
> m68k-linux-ld: drivers/macintosh/via-cuda.o: in function `cuda_set_rtc_time':
> drivers/macintosh/via-cuda.c:797: undefined reference to `rtc_tm_to_time64'
You don't need RTC_CLASS for that. All you need is RTC_LIB I think.
What about selecting RTC_LIB from m68k Kconfig just like powerpc and a
few architectures do ?
See
https://elixir.bootlin.com/linux/v5.18-rc1/source/arch/powerpc/Kconfig#L269
Otherwise, I think it would be better to move (and rename)
rtc_tm_to_time64() into kernel/time/time.c instead of opencoding it twice.
Christophe
>
> Fixes: 0792a2c8e0bb ("macintosh: Use common code to access RTC")
> Signed-off-by: Randy Dunlap
> Reported-by: kernel test robot
> Cc: Benjamin Herrenschmidt
> Cc: Michael Ellerman
> Cc: Christophe Leroy
> Cc: Kees Cook
> Cc: Arnd Bergmann
> Cc: Finn Thain
> Cc: Geert Uytterhoeven
> Cc: Nathan Chancellor
> Cc: Nick Desaulniers
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
> drivers/macintosh/via-cuda.c |5 -
> drivers/macintosh/via-pmu.c |5 -
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> --- a/drivers/macintosh/via-cuda.c
> +++ b/drivers/macintosh/via-cuda.c
> @@ -794,7 +794,10 @@ int cuda_set_rtc_time(struct rtc_time *t
> u32 now;
> struct adb_request req;
>
> - now = lower_32_bits(rtc_tm_to_time64(tm) + RTC_OFFSET);
> + now = lower_32_bits(mktime64(((unsigned int)tm->tm_year + 1900),
> + tm->tm_mon + 1, tm->tm_mday, tm->tm_hour,
> + tm->tm_min, tm->tm_sec)
> + + RTC_OFFSET);
> if (cuda_request(, NULL, 6, CUDA_PACKET, CUDA_SET_TIME,
>now >> 24, now >> 16, now >> 8, now) < 0)
> return -ENXIO;
> --- a/drivers/macintosh/via-pmu.c
> +++ b/drivers/macintosh/via-pmu.c
> @@ -1766,7 +1766,10 @@ int pmu_set_rtc_time(struct rtc_time *tm
> u32 now;
> struct adb_request req;
>
> - now = lower_32_bits(rtc_tm_to_time64(tm) + RTC_OFFSET);
> + now = lower_32_bits(mktime64(((unsigned int)tm->tm_year + 1900),
> + tm->tm_mon + 1, tm->tm_mday, tm->tm_hour,
> + tm->tm_min, tm->tm_sec)
> + + RTC_OFFSET);
> if (pmu_request(, NULL, 5, PMU_SET_RTC,
> now >> 24, now >> 16, now >> 8, now) < 0)
> return -ENXIO;
