* Re: Errant readings on LM81 with T2080 SoC
From: Guenter Roeck @ 2021-03-10 5:06 UTC (permalink / raw)
To: Chris Packham, jdelvare@suse.com
Cc: linux-hwmon@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org
In-Reply-To: <1aa0dc23-0706-5902-2f46-0767de0e3ad6@alliedtelesis.co.nz>
On 3/9/21 6:19 PM, Chris Packham wrote:
> On 9/03/21 9:27 am, Chris Packham wrote:
>> On 8/03/21 5:59 pm, Guenter Roeck wrote:
>>> Other than that, the only other real idea I have would be to monitor
>>> the i2c bus.
>> I am in the fortunate position of being able to go into the office and
>> even happen to have the expensive scope at the moment. Now I just need
>> to find a tame HW engineer so I don't burn myself trying to attach the
>> probes.
> One thing I see on the scope is that when there is a CPU load there
> appears to be some clock stretching going on (SCL is held low some
> times). I don't see it without the CPU load. It's hard to correlate a
> clock stretching event with a bad read or error but it is one area where
> the SMBUS spec has a maximum that might cause the device to give up waiting.
>
Do you have CONFIG_PREEMPT enabled in your kernel ? But even without
that it is possible that the hot loops at the beginning and end of
each operation mess up the driver and cause it to sleep longer
than intended. Did you try usleep_range() ?
On a side note, can you send me a register dump for the lm81 ?
It would be useful for my module test code.
Thanks,
Guenter
^ permalink raw reply
* Re: [PATCH v2] powerpc/kexec_file: Restore FDT size estimation for kdump kernel
From: Michael Ellerman @ 2021-03-10 5:03 UTC (permalink / raw)
To: Rob Herring, Thiago Jung Bauermann
Cc: kexec, linux-kernel@vger.kernel.org, Mimi Zohar,
Lakshmi Ramasubramanian, linuxppc-dev, Hari Bathini
In-Reply-To: <CAL_JsqJatxTwSbM8a0PB2ievO_fRFeYJ8ZtBN0pRfnEjSXyUkg@mail.gmail.com>
Rob Herring <robh@kernel.org> writes:
> On Fri, Feb 19, 2021 at 6:52 PM Thiago Jung Bauermann
> <bauerman@linux.ibm.com> wrote:
>>
>> Commit 2377c92e37fe ("powerpc/kexec_file: fix FDT size estimation for kdump
>> kernel") fixed how elf64_load() estimates the FDT size needed by the
>> crashdump kernel.
>>
>> At the same time, commit 130b2d59cec0 ("powerpc: Use common
>> of_kexec_alloc_and_setup_fdt()") changed the same code to use the generic
>> function of_kexec_alloc_and_setup_fdt() to calculate the FDT size. That
>> change made the code overestimate it a bit by counting twice the space
>> required for the kernel command line and /chosen properties.
>>
>> Therefore change kexec_fdt_totalsize_ppc64() to calculate just the extra
>> space needed by the kdump kernel, and change the function name so that it
>> better reflects what the function is now doing.
>>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> ---
>> arch/powerpc/include/asm/kexec.h | 2 +-
>> arch/powerpc/kexec/elf_64.c | 2 +-
>> arch/powerpc/kexec/file_load_64.c | 26 ++++++++------------------
>> 3 files changed, 10 insertions(+), 20 deletions(-)
>
> I ended up delaying the referenced series til 5.13, but have applied
> it now. Can I get an ack from the powerpc maintainers on this one?
> I'll fixup the commit log to make sense given the commit id's aren't
> valid.
Thanks for handling it.
Acked-by: Michael Ellerman <mpe@ellerman.id.au>
cheers
^ permalink raw reply
* Re: [PATCH 2/6] mm: Generalize SYS_SUPPORTS_HUGETLBFS (rename as ARCH_SUPPORTS_HUGETLBFS)
From: Palmer Dabbelt @ 2021-03-10 4:39 UTC (permalink / raw)
To: anshuman.khandual
Cc: dalias, linux-sh, linux-kernel, James.Bottomley, linux-mm, paulus,
linux-riscv, will, ysato, deller, linux, catalin.marinas, aou,
anshuman.khandual, viro, Paul Walmsley, linux-arm-kernel,
tsbogend, linux-parisc, linux-mips, linux-fsdevel, linuxppc-dev
In-Reply-To: <1615278790-18053-3-git-send-email-anshuman.khandual@arm.com>
On Tue, 09 Mar 2021 00:33:06 PST (-0800), anshuman.khandual@arm.com wrote:
> SYS_SUPPORTS_HUGETLBFS config has duplicate definitions on platforms that
> subscribe it. Instead, just make it a generic option which can be selected
> on applicable platforms. Also rename it as ARCH_SUPPORTS_HUGETLBFS instead.
> This reduces code duplication and makes it cleaner.
>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: "James E.J. Bottomley" <James.Bottomley@HansenPartnership.com>
> Cc: Helge Deller <deller@gmx.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Paul Walmsley <paul.walmsley@sifive.com>
> Cc: Palmer Dabbelt <palmer@dabbelt.com>
> Cc: Albert Ou <aou@eecs.berkeley.edu>
> Cc: Yoshinori Sato <ysato@users.sourceforge.jp>
> Cc: Rich Felker <dalias@libc.org>
> Cc: Alexander Viro <viro@zeniv.linux.org.uk>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-mips@vger.kernel.org
> Cc: linux-parisc@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-riscv@lists.infradead.org
> Cc: linux-sh@vger.kernel.org
> Cc: linux-fsdevel@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> arch/arm/Kconfig | 5 +----
> arch/arm64/Kconfig | 4 +---
> arch/mips/Kconfig | 6 +-----
> arch/parisc/Kconfig | 5 +----
> arch/powerpc/Kconfig | 3 ---
> arch/powerpc/platforms/Kconfig.cputype | 6 +++---
> arch/riscv/Kconfig | 5 +----
> arch/sh/Kconfig | 5 +----
> fs/Kconfig | 5 ++++-
> 9 files changed, 13 insertions(+), 31 deletions(-)
[...]
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 85d626b8ce5e..69954db3aca9 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -30,6 +30,7 @@ config RISCV
> select ARCH_HAS_STRICT_KERNEL_RWX if MMU
> select ARCH_OPTIONAL_KERNEL_RWX if ARCH_HAS_STRICT_KERNEL_RWX
> select ARCH_OPTIONAL_KERNEL_RWX_DEFAULT
> + select ARCH_SUPPORTS_HUGETLBFS if MMU
> select ARCH_WANT_DEFAULT_TOPDOWN_MMAP_LAYOUT if MMU
> select ARCH_WANT_FRAME_POINTERS
> select ARCH_WANT_HUGE_PMD_SHARE if 64BIT
> @@ -165,10 +166,6 @@ config ARCH_WANT_GENERAL_HUGETLB
> config ARCH_SUPPORTS_UPROBES
> def_bool y
>
> -config SYS_SUPPORTS_HUGETLBFS
> - depends on MMU
> - def_bool y
> -
> config STACKTRACE_SUPPORT
> def_bool y
Acked-by: Palmer Dabbelt <palmerdabbelt@google.com>
^ permalink raw reply
* Re: make alloc_anon_inode more useful
From: Matthew Wilcox @ 2021-03-10 4:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Minchan Kim, Alex Williamson, Nadav Amit, Al Viro,
Daniel Vetter, linux-fsdevel, Andrew Morton, linuxppc-dev,
Nitin Gupta
In-Reply-To: <20210309155348.974875-1-hch@lst.de>
On Tue, Mar 09, 2021 at 04:53:39PM +0100, Christoph Hellwig wrote:
> this series first renames the existing alloc_anon_inode to
> alloc_anon_inode_sb to clearly mark it as requiring a superblock.
>
> It then adds a new alloc_anon_inode that works on the anon_inode
> file system super block, thus removing tons of boilerplate code.
>
> The few remainig callers of alloc_anon_inode_sb all use alloc_file_pseudo
> later, but might also be ripe for some cleanup.
On a somewhat related note, could I get you to look at
drivers/video/fbdev/core/fb_defio.c?
As far as I can tell, there's no need for fb_deferred_io_aops to exist.
We could just set file->f_mapping->a_ops to NULL, and set_page_dirty()
would do the exact same thing this code does (except it would get the
return value correct).
But maybe that would make something else go wrong that distinguishes
between page->mapping being NULL and page->mapping->a_ops->foo being NULL?
Completely untested patch ...
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index a591d291b231..441ec31d3e4d 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -151,17 +151,6 @@ static const struct vm_operations_struct fb_deferred_io_vm_ops = {
.page_mkwrite = fb_deferred_io_mkwrite,
};
-static int fb_deferred_io_set_page_dirty(struct page *page)
-{
- if (!PageDirty(page))
- SetPageDirty(page);
- return 0;
-}
-
-static const struct address_space_operations fb_deferred_io_aops = {
- .set_page_dirty = fb_deferred_io_set_page_dirty,
-};
-
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma)
{
vma->vm_ops = &fb_deferred_io_vm_ops;
@@ -212,14 +201,6 @@ void fb_deferred_io_init(struct fb_info *info)
}
EXPORT_SYMBOL_GPL(fb_deferred_io_init);
-void fb_deferred_io_open(struct fb_info *info,
- struct inode *inode,
- struct file *file)
-{
- file->f_mapping->a_ops = &fb_deferred_io_aops;
-}
-EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-
void fb_deferred_io_cleanup(struct fb_info *info)
{
struct fb_deferred_io *fbdefio = info->fbdefio;
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 06f5805de2de..c4ba76359f22 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1415,10 +1415,7 @@ __releases(&info->lock)
if (res)
module_put(info->fbops->owner);
}
-#ifdef CONFIG_FB_DEFERRED_IO
- if (info->fbdefio)
- fb_deferred_io_open(info, inode, file);
-#endif
+ file->f_mapping->a_ops = NULL;
out:
unlock_fb_info(info);
if (res)
diff --git a/include/linux/fb.h b/include/linux/fb.h
index ecfbcc0553a5..a8dccd23c249 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -659,9 +659,6 @@ static inline void __fb_pad_aligned_buffer(u8 *dst, u32 d_pitch,
/* drivers/video/fb_defio.c */
int fb_deferred_io_mmap(struct fb_info *info, struct vm_area_struct *vma);
extern void fb_deferred_io_init(struct fb_info *info);
-extern void fb_deferred_io_open(struct fb_info *info,
- struct inode *inode,
- struct file *file);
extern void fb_deferred_io_cleanup(struct fb_info *info);
extern int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasync);
^ permalink raw reply related
* Re: PowerPC64 future proof kernel toc, revised for lld
From: Alan Modra @ 2021-03-10 3:48 UTC (permalink / raw)
To: linuxppc-dev; +Cc: alexey, ellerman
In-Reply-To: <20210309045638.GI6042@bubble.grove.modra.org>
This patch future-proofs the kernel against linker changes that might
put the toc pointer at some location other than .got+0x8000, by
replacing __toc_start+0x8000 with .TOC. throughout. If the kernel's
idea of the toc pointer doesn't agree with the linker, bad things
happen.
prom_init.c code relocating its toc is also changed so that a symbolic
__prom_init_toc_start toc-pointer relative address is calculated
rather than assuming that it is always at toc-pointer - 0x8000. The
length calculations loading values from the toc are also avoided.
It's a little incestuous to do that with unreloc_toc picking up
adjusted values (which is fine in practice, they both adjust by the
same amount if all goes well).
I've also changed the way .got is aligned in vmlinux.lds and
zImage.lds, mostly so that dumping out section info by objdump or
readelf plainly shows the alignment is 256. This linker script
feature was added 2005-09-27, available in FSF binutils releases from
2.17 onwards. Should be safe to use in the kernel, I think.
Finally, put *(.got) before the prom_init.o entry which only needs
*(.toc), so that the GOT header goes in the correct place. I don't
believe this makes any difference for the kernel as it would for
dynamic objects being loaded by ld.so. That change is just to stop
lusers who blindly copy kernel scripts being led astray. Of course,
this change needs the prom_init.c changes.
Some notes on .toc and .got.
.toc is a compiler generated section of addresses. .got is a linker
generated section of addresses, generally built when the linker sees
R_*_*GOT* relocations. In the case of powerpc64 ld.bfd, there are
multiple generated .got sections, one per input object file. So you
can somewhat reasonably write in a linker script an input section
statement like *prom_init.o(.got .toc) to mean "the .got and .toc
section for files matching *prom_init.o". On other architectures that
doesn't make sense, because the linker generally has just one .got
section. Even on powerpc64, note well that the GOT entries for
prom_init.o may be merged with GOT entries from other objects. That
means that if prom_init.o references, say, _end via some GOT
relocation, and some other object also references _end via a GOT
relocation, the GOT entry for _end may be in the range
__prom_init_toc_start to __prom_init_toc_end and if the kernel does
something special to GOT/TOC entries in that range then the value of
_end as seen by objects other than prom_init.o will be affected. On
the other hand the GOT entry for _end may not be in the range
__prom_init_toc_start to __prom_init_toc_end. Which way it turns out
is deterministic but a detail of linker operation that should not be
relied on.
A feature of ld.bfd is that input .toc (and .got) sections matching
one linker input section statement may be sorted, to put entries used
by small-model code first, near the toc base. This is why scripts for
powerpc64 normally use *(.got .toc) rather than *(.got) *(.toc), since
the first form allows more freedom to sort.
Another feature of ld.bfd is that indirect addressing sequences using
the GOT/TOC may be edited by the linker to relative addressing. In
many cases relative addressing would be emitted by gcc for
-mcmodel=medium if you appropriately decorate variable declarations
with non-default visibility.
Signed-off-by: Alan Modra <amodra@au1.ibm.com>
diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
index 1d83966f5ef6..e45907fe468f 100644
--- a/arch/powerpc/boot/crt0.S
+++ b/arch/powerpc/boot/crt0.S
@@ -28,7 +28,7 @@ p_etext: .8byte _etext
p_bss_start: .8byte __bss_start
p_end: .8byte _end
-p_toc: .8byte __toc_start + 0x8000 - p_base
+p_toc: .8byte .TOC. - p_base
p_dyn: .8byte __dynamic_start - p_base
p_rela: .8byte __rela_dyn_start - p_base
p_prom: .8byte 0
diff --git a/arch/powerpc/boot/zImage.lds.S b/arch/powerpc/boot/zImage.lds.S
index d6f072865627..d65cd55a6f38 100644
--- a/arch/powerpc/boot/zImage.lds.S
+++ b/arch/powerpc/boot/zImage.lds.S
@@ -36,12 +36,9 @@ SECTIONS
}
#ifdef CONFIG_PPC64_BOOT_WRAPPER
- . = ALIGN(256);
- .got :
+ .got : ALIGN(256)
{
- __toc_start = .;
- *(.got)
- *(.toc)
+ *(.got .toc)
}
#endif
diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
index 324d7b298ec3..e5a1eae11ed5 100644
--- a/arch/powerpc/include/asm/sections.h
+++ b/arch/powerpc/include/asm/sections.h
@@ -48,14 +48,18 @@ static inline int in_kernel_text(unsigned long addr)
static inline unsigned long kernel_toc_addr(void)
{
- /* Defined by the linker, see vmlinux.lds.S */
- extern unsigned long __toc_start;
-
- /*
- * The TOC register (r2) points 32kB into the TOC, so that 64kB of
- * the TOC can be addressed using a single machine instruction.
- */
- return (unsigned long)(&__toc_start) + 0x8000UL;
+#if 0
+ /* This version is appropriate if the kernel is never compiled
+ -mcmodel=small or the total .toc is always less than 64k. */
+ register unsigned long toc_ptr asm ("r2");
+ return toc_ptr;
+#else
+ /* Otherwise linker automatic multiple toc sections might be
+ required, and in that case r2 may be adjusted by a linker
+ stub. */
+ extern unsigned long __attribute__((visibility("hidden"))) toc asm (".TOC.");
+ return (unsigned long)&toc;
+#endif
}
static inline int overlaps_interrupt_vector_text(unsigned long start,
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index ece7f97bafff..9542d03b2efe 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -899,7 +899,7 @@ _GLOBAL(relative_toc)
blr
.balign 8
-p_toc: .8byte __toc_start + 0x8000 - 0b
+p_toc: .8byte .TOC. - 0b
/*
* This is where the main kernel code starts.
diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
index ccf77b985c8f..d309a7787652 100644
--- a/arch/powerpc/kernel/prom_init.c
+++ b/arch/powerpc/kernel/prom_init.c
@@ -3220,27 +3220,26 @@ static void unreloc_toc(void)
{
}
#else
-static void __reloc_toc(unsigned long offset, unsigned long nr_entries)
+static void __reloc_toc(unsigned long offset)
{
- unsigned long i;
unsigned long *toc_entry;
+ unsigned long *toc_start, *toc_end;
- /* Get the start of the TOC by using r2 directly. */
- asm volatile("addi %0,2,-0x8000" : "=b" (toc_entry));
+ asm("addis %0,2,__prom_init_toc_start@toc@ha\n\t"
+ "addi %0,%0,__prom_init_toc_start@toc@l" : "=b" (toc_start));
+ asm("addis %0,2,__prom_init_toc_end@toc@ha\n\t"
+ "addi %0,%0,__prom_init_toc_end@toc@l" : "=b" (toc_end));
- for (i = 0; i < nr_entries; i++) {
- *toc_entry = *toc_entry + offset;
- toc_entry++;
+ for (toc_entry = toc_start; toc_entry != toc_end; toc_entry++) {
+ *toc_entry += offset;
}
}
static void reloc_toc(void)
{
unsigned long offset = reloc_offset();
- unsigned long nr_entries =
- (__prom_init_toc_end - __prom_init_toc_start) / sizeof(long);
- __reloc_toc(offset, nr_entries);
+ __reloc_toc(offset);
mb();
}
@@ -3248,12 +3247,10 @@ static void reloc_toc(void)
static void unreloc_toc(void)
{
unsigned long offset = reloc_offset();
- unsigned long nr_entries =
- (__prom_init_toc_end - __prom_init_toc_start) / sizeof(long);
mb();
- __reloc_toc(-offset, nr_entries);
+ __reloc_toc(-offset);
}
#endif
#endif
diff --git a/arch/powerpc/kernel/vmlinux.lds.S b/arch/powerpc/kernel/vmlinux.lds.S
index 72fa3c00229a..3d2e6e2b81b5 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -326,15 +326,13 @@ SECTIONS
__end_opd = .;
}
- . = ALIGN(256);
- .got : AT(ADDR(.got) - LOAD_OFFSET) {
- __toc_start = .;
+ .got : AT(ADDR(.got) - LOAD_OFFSET) ALIGN(256) {
+ *(.got)
#ifndef CONFIG_RELOCATABLE
__prom_init_toc_start = .;
- arch/powerpc/kernel/prom_init.o*(.toc .got)
+ arch/powerpc/kernel/prom_init.o*(.toc)
__prom_init_toc_end = .;
#endif
- *(.got)
*(.toc)
}
#endif
--
Alan Modra
Australia Development Lab, IBM
^ permalink raw reply related
* Re: Errant readings on LM81 with T2080 SoC
From: Guenter Roeck @ 2021-03-10 3:29 UTC (permalink / raw)
To: Chris Packham, jdelvare@suse.com
Cc: linux-hwmon@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org
In-Reply-To: <b1ba3f34-cbcc-4bbd-ea84-aad21f513682@alliedtelesis.co.nz>
On 3/9/21 3:35 PM, Chris Packham wrote:
>
> On 8/03/21 1:31 pm, Guenter Roeck wrote:
>> On 3/7/21 2:52 PM, Chris Packham wrote:
>>> Fundamentally I think this is a problem with the fact that the LM81 is
>>> an SMBus device but the T2080 (and other Freescale SoCs) uses i2c and we
>>> emulate SMBus. I suspect the errant readings are when we don't get round
>>> to completing the read within the timeout specified by the SMBus
>>> specification. Depending on when that happens we either fail the
>>> transfer or interpret the result as all-1s.
>> That is quite unlikely. Many sensor chips are SMBus chips connected to
>> i2c busses. It is much more likely that there is a bug in the T2080 i2c driver,
>> that the chip doesn't like the bulk read command issued through regmap, that
>> the chip has problems with the i2c bus speed, or that the i2c bus is noisy.
> I have noticed that with the switch to regmap we end up using plain i2c
> instead of SMBUS. There appears to be no way of saying use SMBUS
> semantics if the i2c adapter reports I2C_FUNC_I2C.
>
The driver only really supports I2C; SMBUS functions are emulated.
I don't think that makes a real difference.
Guenter
^ permalink raw reply
* Re: [PATCH v2] powerpc/kexec_file: Restore FDT size estimation for kdump kernel
From: Rob Herring @ 2021-03-10 3:09 UTC (permalink / raw)
To: Lakshmi Ramasubramanian
Cc: kexec, linux-kernel@vger.kernel.org, Mimi Zohar,
Thiago Jung Bauermann, linuxppc-dev, Hari Bathini
In-Reply-To: <7d0c6062-ca73-f183-110d-f5b75ae91d10@linux.microsoft.com>
On Tue, Mar 9, 2021 at 7:31 PM Lakshmi Ramasubramanian
<nramas@linux.microsoft.com> wrote:
>
> On 3/9/21 6:08 PM, Rob Herring wrote:
>
> Hi Rob,
>
> > On Fri, Feb 19, 2021 at 6:52 PM Thiago Jung Bauermann
> > <bauerman@linux.ibm.com> wrote:
> >>
> >> Commit 2377c92e37fe ("powerpc/kexec_file: fix FDT size estimation for kdump
> >> kernel") fixed how elf64_load() estimates the FDT size needed by the
> >> crashdump kernel.
> >>
> >> At the same time, commit 130b2d59cec0 ("powerpc: Use common
> >> of_kexec_alloc_and_setup_fdt()") changed the same code to use the generic
> >> function of_kexec_alloc_and_setup_fdt() to calculate the FDT size. That
> >> change made the code overestimate it a bit by counting twice the space
> >> required for the kernel command line and /chosen properties.
> >>
> >> Therefore change kexec_fdt_totalsize_ppc64() to calculate just the extra
> >> space needed by the kdump kernel, and change the function name so that it
> >> better reflects what the function is now doing.
> >>
> >> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> >> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> >> ---
> >> arch/powerpc/include/asm/kexec.h | 2 +-
> >> arch/powerpc/kexec/elf_64.c | 2 +-
> >> arch/powerpc/kexec/file_load_64.c | 26 ++++++++------------------
> >> 3 files changed, 10 insertions(+), 20 deletions(-)
> >
> > I ended up delaying the referenced series til 5.13, but have applied
> > it now. Can I get an ack from the powerpc maintainers on this one?
> > I'll fixup the commit log to make sense given the commit id's aren't
> > valid.
>
> I checked the change applied in linux-next branch and also Device Tree's
> for-next branch - it looks like v1 of Thiago's patch has been applied.
> Could you please pick up the v2 patch?
Huh? This patch (v2) hasn't been applied to any tree AFAICT.
Rob
^ permalink raw reply
* Re: [PATCH v4 3/6] ASoC: dt-bindings: fsl_rpmsg: Add binding doc for rpmsg cpu dai driver
From: Rob Herring @ 2021-03-10 2:48 UTC (permalink / raw)
To: Shengjiu Wang
Cc: devicetree, alsa-devel, timur, lgirdwood, linuxppc-dev, Xiubo.Lee,
linux-kernel, tiwai, nicoleotsuka, broonie, perex, festevam
In-Reply-To: <1615209750-2357-4-git-send-email-shengjiu.wang@nxp.com>
On Mon, Mar 08, 2021 at 09:22:27PM +0800, Shengjiu Wang wrote:
> fsl_rpmsg cpu dai driver is driver for rpmsg audio, which is mainly used
Bindings describe h/w blocks, not drivers.
> for getting the user's configuration from device tree and configure the
> clocks which is used by Cortex-M core. So in this document define the
> needed property.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> .../devicetree/bindings/sound/fsl,rpmsg.yaml | 118 ++++++++++++++++++
> 1 file changed, 118 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml
>
> diff --git a/Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml b/Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml
> new file mode 100644
> index 000000000000..5731c1fbc0a6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/fsl,rpmsg.yaml
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/sound/fsl,rpmsg.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NXP Audio RPMSG CPU DAI Controller
> +
> +maintainers:
> + - Shengjiu Wang <shengjiu.wang@nxp.com>
> +
> +description: |
> + fsl_rpmsg cpu dai driver is virtual driver for rpmsg audio, which doesn't
> + touch hardware. It is mainly used for getting the user's configuration
> + from device tree and configure the clocks which is used by Cortex-M core.
> + So in this document define the needed property.
> +
> +properties:
> + compatible:
> + enum:
> + - fsl,imx7ulp-rpmsg
> + - fsl,imx8mn-rpmsg
> + - fsl,imx8mm-rpmsg
> + - fsl,imx8mp-rpmsg
> +
> + model:
> + $ref: /schemas/types.yaml#/definitions/string
> + description: User specified audio sound card name
> +
> + clocks:
> + items:
> + - description: Peripheral clock for register access
> + - description: Master clock
> + - description: DMA clock for DMA register access
> + - description: Parent clock for multiple of 8kHz sample rates
> + - description: Parent clock for multiple of 11kHz sample rates
> + minItems: 5
If this doesn't touch hardware, what are these clocks for?
You don't need 'minItems' unless it's less than the number of 'items'.
> +
> + clock-names:
> + items:
> + - const: ipg
> + - const: mclk
> + - const: dma
> + - const: pll8k
> + - const: pll11k
> + minItems: 5
> +
> + power-domains:
> + maxItems: 1
> +
> + fsl,audioindex:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1]
> + default: 0
> + description: Instance index for sound card in
> + M core side, which share one rpmsg
> + channel.
We don't do indexes in DT. What's this numbering tied to?
> +
> + fsl,version:
version of what?
This seems odd at best.
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [1, 2]
You're going to update this with every new firmware version?
> + default: 2
> + description: The version of M core image, which is
> + to make driver compatible with different image.
> +
> + fsl,buffer-size:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description: pre allocate dma buffer size
How can you have DMA, this doesn't touch h/w?
> +
> + fsl,enable-lpa:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: enable low power audio path.
> +
> + fsl,rpmsg-out:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
> + This is a boolean property. If present, the transmitting function
> + will be enabled.
> +
> + fsl,rpmsg-in:
> + $ref: /schemas/types.yaml#/definitions/flag
> + description: |
> + This is a boolean property. If present, the receiving function
> + will be enabled.
> +
> + fsl,codec-type:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + enum: [0, 1, 2]
> + default: 0
> + description: Sometimes the codec is registered by
> + driver not by the device tree, this items
> + can be used to distinguish codecs.
How does one decide what value to use?
> +
> + audio-codec:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: The phandle of the audio codec
The codec is controlled from the Linux side?
> +
> + memory-region:
> + $ref: /schemas/types.yaml#/definitions/phandle
> + description: phandle to the reserved memory nodes
> +
> +required:
> + - compatible
> + - fsl,audioindex
> + - fsl,version
> + - fsl,buffer-size
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + rpmsg_audio: rpmsg_audio {
> + compatible = "fsl,imx8mn-rpmsg";
> + fsl,audioindex = <0> ;
> + fsl,version = <2>;
> + fsl,buffer-size = <0x6000000>;
> + fsl,enable-lpa;
How does this work? Don't you need somewhere to put the 'rpmsg' data?
> + };
> --
> 2.27.0
>
^ permalink raw reply
* Re: [PATCH v2] powerpc/kexec_file: Restore FDT size estimation for kdump kernel
From: Lakshmi Ramasubramanian @ 2021-03-10 2:31 UTC (permalink / raw)
To: Rob Herring, Thiago Jung Bauermann
Cc: kexec, linux-kernel@vger.kernel.org, Mimi Zohar, linuxppc-dev,
Hari Bathini
In-Reply-To: <CAL_JsqJatxTwSbM8a0PB2ievO_fRFeYJ8ZtBN0pRfnEjSXyUkg@mail.gmail.com>
On 3/9/21 6:08 PM, Rob Herring wrote:
Hi Rob,
> On Fri, Feb 19, 2021 at 6:52 PM Thiago Jung Bauermann
> <bauerman@linux.ibm.com> wrote:
>>
>> Commit 2377c92e37fe ("powerpc/kexec_file: fix FDT size estimation for kdump
>> kernel") fixed how elf64_load() estimates the FDT size needed by the
>> crashdump kernel.
>>
>> At the same time, commit 130b2d59cec0 ("powerpc: Use common
>> of_kexec_alloc_and_setup_fdt()") changed the same code to use the generic
>> function of_kexec_alloc_and_setup_fdt() to calculate the FDT size. That
>> change made the code overestimate it a bit by counting twice the space
>> required for the kernel command line and /chosen properties.
>>
>> Therefore change kexec_fdt_totalsize_ppc64() to calculate just the extra
>> space needed by the kdump kernel, and change the function name so that it
>> better reflects what the function is now doing.
>>
>> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
>> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
>> ---
>> arch/powerpc/include/asm/kexec.h | 2 +-
>> arch/powerpc/kexec/elf_64.c | 2 +-
>> arch/powerpc/kexec/file_load_64.c | 26 ++++++++------------------
>> 3 files changed, 10 insertions(+), 20 deletions(-)
>
> I ended up delaying the referenced series til 5.13, but have applied
> it now. Can I get an ack from the powerpc maintainers on this one?
> I'll fixup the commit log to make sense given the commit id's aren't
> valid.
I checked the change applied in linux-next branch and also Device Tree's
for-next branch - it looks like v1 of Thiago's patch has been applied.
Could you please pick up the v2 patch?
thanks,
-lakshmi
^ permalink raw reply
* Re: Errant readings on LM81 with T2080 SoC
From: Chris Packham @ 2021-03-10 2:19 UTC (permalink / raw)
To: Guenter Roeck, jdelvare@suse.com
Cc: linux-hwmon@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org
In-Reply-To: <d6074923-ee7e-4499-0e54-383a607d3c41@alliedtelesis.co.nz>
On 9/03/21 9:27 am, Chris Packham wrote:
> On 8/03/21 5:59 pm, Guenter Roeck wrote:
>> Other than that, the only other real idea I have would be to monitor
>> the i2c bus.
> I am in the fortunate position of being able to go into the office and
> even happen to have the expensive scope at the moment. Now I just need
> to find a tame HW engineer so I don't burn myself trying to attach the
> probes.
One thing I see on the scope is that when there is a CPU load there
appears to be some clock stretching going on (SCL is held low some
times). I don't see it without the CPU load. It's hard to correlate a
clock stretching event with a bad read or error but it is one area where
the SMBUS spec has a maximum that might cause the device to give up waiting.
^ permalink raw reply
* Re: [PATCH v2] powerpc/kexec_file: Restore FDT size estimation for kdump kernel
From: Rob Herring @ 2021-03-10 2:08 UTC (permalink / raw)
To: Thiago Jung Bauermann
Cc: kexec, linux-kernel@vger.kernel.org, Mimi Zohar,
Lakshmi Ramasubramanian, linuxppc-dev, Hari Bathini
In-Reply-To: <20210220005204.1417200-1-bauerman@linux.ibm.com>
On Fri, Feb 19, 2021 at 6:52 PM Thiago Jung Bauermann
<bauerman@linux.ibm.com> wrote:
>
> Commit 2377c92e37fe ("powerpc/kexec_file: fix FDT size estimation for kdump
> kernel") fixed how elf64_load() estimates the FDT size needed by the
> crashdump kernel.
>
> At the same time, commit 130b2d59cec0 ("powerpc: Use common
> of_kexec_alloc_and_setup_fdt()") changed the same code to use the generic
> function of_kexec_alloc_and_setup_fdt() to calculate the FDT size. That
> change made the code overestimate it a bit by counting twice the space
> required for the kernel command line and /chosen properties.
>
> Therefore change kexec_fdt_totalsize_ppc64() to calculate just the extra
> space needed by the kdump kernel, and change the function name so that it
> better reflects what the function is now doing.
>
> Signed-off-by: Thiago Jung Bauermann <bauerman@linux.ibm.com>
> Reviewed-by: Lakshmi Ramasubramanian <nramas@linux.microsoft.com>
> ---
> arch/powerpc/include/asm/kexec.h | 2 +-
> arch/powerpc/kexec/elf_64.c | 2 +-
> arch/powerpc/kexec/file_load_64.c | 26 ++++++++------------------
> 3 files changed, 10 insertions(+), 20 deletions(-)
I ended up delaying the referenced series til 5.13, but have applied
it now. Can I get an ack from the powerpc maintainers on this one?
I'll fixup the commit log to make sense given the commit id's aren't
valid.
Rob
^ permalink raw reply
* Re: [PATCH v2 40/43] powerpc/64s: Make kuap_check_amr() and kuap_get_and_check_amr() generic
From: Nicholas Piggin @ 2021-03-10 1:37 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <7167aef44fb816f6df17f65d540ac07ca98c4af9.1615291474.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of March 9, 2021 10:10 pm:
> In preparation of porting powerpc32 to C syscall entry/exit,
> rename kuap_check_amr() and kuap_get_and_check_amr() as kuap_check()
> and kuap_get_and_check(), and move in the generic asm/kup.h the stub
> for when CONFIG_PPC_KUAP is not selected.
Looks pretty straightforward to me.
While you're renaming things, could kuap_check_amr() be changed to
kuap_assert_locked() or similar? Otherwise,
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/book3s/64/kup.h | 24 ++----------------------
> arch/powerpc/include/asm/kup.h | 10 +++++++++-
> arch/powerpc/kernel/interrupt.c | 12 ++++++------
> arch/powerpc/kernel/irq.c | 2 +-
> 4 files changed, 18 insertions(+), 30 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/64/kup.h b/arch/powerpc/include/asm/book3s/64/kup.h
> index 8bd905050896..d9b07e9998be 100644
> --- a/arch/powerpc/include/asm/book3s/64/kup.h
> +++ b/arch/powerpc/include/asm/book3s/64/kup.h
> @@ -287,7 +287,7 @@ static inline void kuap_kernel_restore(struct pt_regs *regs,
> */
> }
>
> -static inline unsigned long kuap_get_and_check_amr(void)
> +static inline unsigned long kuap_get_and_check(void)
> {
> if (mmu_has_feature(MMU_FTR_BOOK3S_KUAP)) {
> unsigned long amr = mfspr(SPRN_AMR);
> @@ -298,27 +298,7 @@ static inline unsigned long kuap_get_and_check_amr(void)
> return 0;
> }
>
> -#else /* CONFIG_PPC_PKEY */
> -
> -static inline void kuap_user_restore(struct pt_regs *regs)
> -{
> -}
> -
> -static inline void kuap_kernel_restore(struct pt_regs *regs, unsigned long amr)
> -{
> -}
> -
> -static inline unsigned long kuap_get_and_check_amr(void)
> -{
> - return 0;
> -}
> -
> -#endif /* CONFIG_PPC_PKEY */
> -
> -
> -#ifdef CONFIG_PPC_KUAP
> -
> -static inline void kuap_check_amr(void)
> +static inline void kuap_check(void)
> {
> if (IS_ENABLED(CONFIG_PPC_KUAP_DEBUG) && mmu_has_feature(MMU_FTR_BOOK3S_KUAP))
> WARN_ON_ONCE(mfspr(SPRN_AMR) != AMR_KUAP_BLOCKED);
> diff --git a/arch/powerpc/include/asm/kup.h b/arch/powerpc/include/asm/kup.h
> index 25671f711ec2..b7efa46b3109 100644
> --- a/arch/powerpc/include/asm/kup.h
> +++ b/arch/powerpc/include/asm/kup.h
> @@ -74,7 +74,15 @@ bad_kuap_fault(struct pt_regs *regs, unsigned long address, bool is_write)
> return false;
> }
>
> -static inline void kuap_check_amr(void) { }
> +static inline void kuap_check(void) { }
> +static inline void kuap_save_and_lock(struct pt_regs *regs) { }
> +static inline void kuap_user_restore(struct pt_regs *regs) { }
> +static inline void kuap_kernel_restore(struct pt_regs *regs, unsigned long amr) { }
> +
> +static inline unsigned long kuap_get_and_check(void)
> +{
> + return 0;
> +}
>
> /*
> * book3s/64/kup-radix.h defines these functions for the !KUAP case to flush
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 727b7848c9cc..40ed55064e54 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -76,7 +76,7 @@ notrace long system_call_exception(long r3, long r4, long r5,
> } else
> #endif
> #ifdef CONFIG_PPC64
> - kuap_check_amr();
> + kuap_check();
> #endif
>
> booke_restore_dbcr0();
> @@ -254,7 +254,7 @@ notrace unsigned long syscall_exit_prepare(unsigned long r3,
> CT_WARN_ON(ct_state() == CONTEXT_USER);
>
> #ifdef CONFIG_PPC64
> - kuap_check_amr();
> + kuap_check();
> #endif
>
> regs->result = r3;
> @@ -380,7 +380,7 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
> * AMR can only have been unlocked if we interrupted the kernel.
> */
> #ifdef CONFIG_PPC64
> - kuap_check_amr();
> + kuap_check();
> #endif
>
> local_irq_save(flags);
> @@ -451,7 +451,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
> unsigned long flags;
> unsigned long ret = 0;
> #ifdef CONFIG_PPC64
> - unsigned long amr;
> + unsigned long kuap;
> #endif
>
> if (!IS_ENABLED(CONFIG_BOOKE) && !IS_ENABLED(CONFIG_40x) &&
> @@ -467,7 +467,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
> CT_WARN_ON(ct_state() == CONTEXT_USER);
>
> #ifdef CONFIG_PPC64
> - amr = kuap_get_and_check_amr();
> + kuap = kuap_get_and_check();
> #endif
>
> if (unlikely(current_thread_info()->flags & _TIF_EMULATE_STACK_STORE)) {
> @@ -511,7 +511,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsign
> * value from the check above.
> */
> #ifdef CONFIG_PPC64
> - kuap_kernel_restore(regs, amr);
> + kuap_kernel_restore(regs, kuap);
> #endif
>
> return ret;
> diff --git a/arch/powerpc/kernel/irq.c b/arch/powerpc/kernel/irq.c
> index d71fd10a1dd4..3b18d2b2c702 100644
> --- a/arch/powerpc/kernel/irq.c
> +++ b/arch/powerpc/kernel/irq.c
> @@ -282,7 +282,7 @@ static inline void replay_soft_interrupts_irqrestore(void)
> * and re-locking AMR but we shouldn't get here in the first place,
> * hence the warning.
> */
> - kuap_check_amr();
> + kuap_check();
>
> if (kuap_state != AMR_KUAP_BLOCKED)
> set_kuap(AMR_KUAP_BLOCKED);
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH v2 36/43] powerpc/32: Set current->thread.regs in C interrupt entry
From: Nicholas Piggin @ 2021-03-10 1:33 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <8d523f9ecee1de0515cc31d43030c12ab171a670.1615291474.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of March 9, 2021 10:10 pm:
> No need to do that is assembly, do it in C.
Hmm. No issues with the patch as such, but why does ppc32 need this but
not 64? AFAIKS 64 sets this when a thread is created.
Thanks,
Nick
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/include/asm/interrupt.h | 4 +++-
> arch/powerpc/kernel/entry_32.S | 3 +--
> 2 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index 861e6eadc98c..e6d71c2e3aa2 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -33,8 +33,10 @@ static inline void interrupt_enter_prepare(struct pt_regs *regs, struct interrup
> if (!arch_irq_disabled_regs(regs))
> trace_hardirqs_off();
>
> - if (user_mode(regs))
> + if (user_mode(regs)) {
> + current->thread.regs = regs;
> account_cpu_user_entry();
> + }
> #endif
> /*
> * Book3E reconciles irq soft mask in asm
> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
> index 8fe1c3fdfa6e..815a4ff1ba76 100644
> --- a/arch/powerpc/kernel/entry_32.S
> +++ b/arch/powerpc/kernel/entry_32.S
> @@ -52,8 +52,7 @@
> prepare_transfer_to_handler:
> andi. r0,r9,MSR_PR
> addi r12, r2, THREAD
> - beq 2f /* if from user, fix up THREAD.regs */
> - stw r3,PT_REGS(r12)
> + beq 2f
> #ifdef CONFIG_PPC_BOOK3S_32
> kuep_lock r11, r12
> #endif
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH v2 28/43] powerpc/64e: Call bad_page_fault() from do_page_fault()
From: Nicholas Piggin @ 2021-03-10 1:29 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <b2878184d4c21faa8af55b60e52c83f391272112.1615291473.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of March 9, 2021 10:09 pm:
> book3e/64 is the last one calling __bad_page_fault()
> from assembly.
>
> Save non volatile registers before calling do_page_fault()
> and modify do_page_fault() to call __bad_page_fault()
> for all platforms.
>
> Then it can be refactored by the call of bad_page_fault()
> which avoids the duplication of the exception table search.
This can go in with the 64e change after your series. I think it should
be ready for the next merge window as well.
Thanks,
Nick
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/kernel/exceptions-64e.S | 8 +-------
> arch/powerpc/mm/fault.c | 17 ++++-------------
> 2 files changed, 5 insertions(+), 20 deletions(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64e.S b/arch/powerpc/kernel/exceptions-64e.S
> index e8eb9992a270..b60f89078a3f 100644
> --- a/arch/powerpc/kernel/exceptions-64e.S
> +++ b/arch/powerpc/kernel/exceptions-64e.S
> @@ -1010,15 +1010,9 @@ storage_fault_common:
> addi r3,r1,STACK_FRAME_OVERHEAD
> ld r14,PACA_EXGEN+EX_R14(r13)
> ld r15,PACA_EXGEN+EX_R15(r13)
> + bl save_nvgprs
> bl do_page_fault
> - cmpdi r3,0
> - bne- 1f
> b ret_from_except_lite
> -1: bl save_nvgprs
> - mr r4,r3
> - addi r3,r1,STACK_FRAME_OVERHEAD
> - bl __bad_page_fault
> - b ret_from_except
>
> /*
> * Alignment exception doesn't fit entirely in the 0x100 bytes so it
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 2e54bac99a22..7bcff3fca110 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -541,24 +541,15 @@ NOKPROBE_SYMBOL(___do_page_fault);
>
> static long __do_page_fault(struct pt_regs *regs)
> {
> - const struct exception_table_entry *entry;
> long err;
>
> err = ___do_page_fault(regs, regs->dar, regs->dsisr);
> if (likely(!err))
> - return err;
> -
> - entry = search_exception_tables(regs->nip);
> - if (likely(entry)) {
> - instruction_pointer_set(regs, extable_fixup(entry));
> return 0;
> - } else if (!IS_ENABLED(CONFIG_PPC_BOOK3E_64)) {
> - __bad_page_fault(regs, err);
> - return 0;
> - } else {
> - /* 32 and 64e handle the bad page fault in asm */
> - return err;
> - }
> +
> + bad_page_fault(regs, err);
> +
> + return 0;
> }
> NOKPROBE_SYMBOL(__do_page_fault);
>
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: [PATCH v2 02/43] powerpc/traps: Declare unrecoverable_exception() as __noreturn
From: Nicholas Piggin @ 2021-03-10 1:22 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <f097a1071254e8f6875588f8fb9771467824a569.1615291471.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of March 9, 2021 10:09 pm:
> unrecoverable_exception() is never expected to return, most callers
> have an infiniteloop in case it returns.
>
> Ensure it really never returns by terminating it with a BUG(), and
> declare it __no_return.
>
> It always GCC to really simplify functions calling it. In the exemple
> below, it avoids the stack frame in the likely fast path and avoids
> code duplication for the exit.
>
> With this patch:
[snip]
Nice.
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index a44a30b0688c..d5c9d9ddd186 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -2170,11 +2170,15 @@ DEFINE_INTERRUPT_HANDLER(SPEFloatingPointRoundException)
> * in the MSR is 0. This indicates that SRR0/1 are live, and that
> * we therefore lost state by taking this exception.
> */
> -void unrecoverable_exception(struct pt_regs *regs)
> +void __noreturn unrecoverable_exception(struct pt_regs *regs)
> {
> pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
> regs->trap, regs->nip, regs->msr);
> die("Unrecoverable exception", regs, SIGABRT);
> + /* die() should not return */
> + WARN(true, "die() unexpectedly returned");
> + for (;;)
> + ;
> }
I don't think the WARN should be added because that will cause another
interrupt after something is already badly wrong, so this might just
make it harder to debug.
For example if die() is falling through for some reason, we warn and
cause a program check here, and that might also be unrecoverable so it
might come through here and fall through again and warn again, etc.
Putting the infinite loop is good enough I think (and better than there
was previously).
Otherwise
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
Thanks,
Nick
^ permalink raw reply
* Re: [PATCH v2 01/43] powerpc/traps: unrecoverable_exception() is not an interrupt handler
From: Nicholas Piggin @ 2021-03-10 1:16 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Christophe Leroy, Michael Ellerman,
Paul Mackerras
Cc: linuxppc-dev, linux-kernel
In-Reply-To: <ae96c59fa2cb7f24a8929c58cfa2c909cb8ff1f1.1615291471.git.christophe.leroy@csgroup.eu>
Excerpts from Christophe Leroy's message of March 9, 2021 10:09 pm:
> unrecoverable_exception() is called from interrupt handlers or
> after an interrupt handler has failed.
>
> Make it a standard function to avoid doubling the actions
> performed on interrupt entry (e.g.: user time accounting).
>
> Fixes: 3a96570ffceb ("powerpc: convert interrupt handlers to use wrappers")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Nicholas Piggin <npiggin@gmail.com>
This should go in as a fix for this release I think.
> ---
> arch/powerpc/include/asm/interrupt.h | 3 ++-
> arch/powerpc/kernel/interrupt.c | 1 -
> arch/powerpc/kernel/traps.c | 2 +-
> 3 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/interrupt.h b/arch/powerpc/include/asm/interrupt.h
> index aedfba29e43a..e8d09a841373 100644
> --- a/arch/powerpc/include/asm/interrupt.h
> +++ b/arch/powerpc/include/asm/interrupt.h
> @@ -410,7 +410,6 @@ DECLARE_INTERRUPT_HANDLER(altivec_assist_exception);
> DECLARE_INTERRUPT_HANDLER(CacheLockingException);
> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointException);
> DECLARE_INTERRUPT_HANDLER(SPEFloatingPointRoundException);
> -DECLARE_INTERRUPT_HANDLER(unrecoverable_exception);
> DECLARE_INTERRUPT_HANDLER(WatchdogException);
> DECLARE_INTERRUPT_HANDLER(kernel_bad_stack);
>
> @@ -437,6 +436,8 @@ DECLARE_INTERRUPT_HANDLER_NMI(hmi_exception_realmode);
>
> DECLARE_INTERRUPT_HANDLER_ASYNC(TAUException);
>
> +void unrecoverable_exception(struct pt_regs *regs);
> +
> void replay_system_reset(void);
> void replay_soft_interrupts(void);
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index 398cd86b6ada..b8e7d25be31b 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -436,7 +436,6 @@ notrace unsigned long interrupt_exit_user_prepare(struct pt_regs *regs, unsigned
> return ret;
> }
>
> -void unrecoverable_exception(struct pt_regs *regs);
> void preempt_schedule_irq(void);
>
> notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs, unsigned long msr)
> diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
> index 1583fd1c6010..a44a30b0688c 100644
> --- a/arch/powerpc/kernel/traps.c
> +++ b/arch/powerpc/kernel/traps.c
> @@ -2170,7 +2170,7 @@ DEFINE_INTERRUPT_HANDLER(SPEFloatingPointRoundException)
> * in the MSR is 0. This indicates that SRR0/1 are live, and that
> * we therefore lost state by taking this exception.
> */
> -DEFINE_INTERRUPT_HANDLER(unrecoverable_exception)
> +void unrecoverable_exception(struct pt_regs *regs)
> {
> pr_emerg("Unrecoverable exception %lx at %lx (msr=%lx)\n",
> regs->trap, regs->nip, regs->msr);
> --
> 2.25.0
>
>
^ permalink raw reply
* Re: Errant readings on LM81 with T2080 SoC
From: Chris Packham @ 2021-03-09 23:35 UTC (permalink / raw)
To: Guenter Roeck, jdelvare@suse.com
Cc: linux-hwmon@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, linux-i2c@vger.kernel.org
In-Reply-To: <96d660bc-17ab-4e0e-9a94-bce1737a8da1@roeck-us.net>
On 8/03/21 1:31 pm, Guenter Roeck wrote:
> On 3/7/21 2:52 PM, Chris Packham wrote:
>> Fundamentally I think this is a problem with the fact that the LM81 is
>> an SMBus device but the T2080 (and other Freescale SoCs) uses i2c and we
>> emulate SMBus. I suspect the errant readings are when we don't get round
>> to completing the read within the timeout specified by the SMBus
>> specification. Depending on when that happens we either fail the
>> transfer or interpret the result as all-1s.
> That is quite unlikely. Many sensor chips are SMBus chips connected to
> i2c busses. It is much more likely that there is a bug in the T2080 i2c driver,
> that the chip doesn't like the bulk read command issued through regmap, that
> the chip has problems with the i2c bus speed, or that the i2c bus is noisy.
I have noticed that with the switch to regmap we end up using plain i2c
instead of SMBUS. There appears to be no way of saying use SMBUS
semantics if the i2c adapter reports I2C_FUNC_I2C.
^ permalink raw reply
* Re: [PATCH v1] powerpc: Include running function as first entry in save_stack_trace() and friends
From: Segher Boessenkool @ 2021-03-09 22:05 UTC (permalink / raw)
To: Mark Rutland
Cc: Marco Elver, Catalin Marinas, linuxppc-dev, LKML, kasan-dev,
broonie, Paul Mackerras, Will Deacon, linux-arm-kernel
In-Reply-To: <20210309160505.GA4979@C02TD0UTHF1T.local>
Hi!
On Tue, Mar 09, 2021 at 04:05:23PM +0000, Mark Rutland wrote:
> On Thu, Mar 04, 2021 at 03:54:48PM -0600, Segher Boessenkool wrote:
> > On Thu, Mar 04, 2021 at 02:57:30PM +0000, Mark Rutland wrote:
> > > It looks like GCC is happy to give us the function-entry-time FP if we use
> > > __builtin_frame_address(1),
> >
> > From the GCC manual:
> > Calling this function with a nonzero argument can have
> > unpredictable effects, including crashing the calling program. As
> > a result, calls that are considered unsafe are diagnosed when the
> > '-Wframe-address' option is in effect. Such calls should only be
> > made in debugging situations.
> >
> > It *does* warn (the warning is in -Wall btw), on both powerpc and
> > aarch64. Furthermore, using this builtin causes lousy code (it forces
> > the use of a frame pointer, which we normally try very hard to optimise
> > away, for good reason).
> >
> > And, that warning is not an idle warning. Non-zero arguments to
> > __builtin_frame_address can crash the program. It won't on simpler
> > functions, but there is no real definition of what a simpler function
> > *is*. It is meant for debugging, not for production use (this is also
> > why no one has bothered to make it faster).
> >
> > On Power it should work, but on pretty much any other arch it won't.
>
> I understand this is true generally, and cannot be relied upon in
> portable code. However as you hint here for Power, I believe that on
> arm64 __builtin_frame_address(1) shouldn't crash the program due to the
> way frame records work on arm64, but I'll go check with some local
> compiler folk. I agree that __builtin_frame_address(2) and beyond
> certainly can, e.g. by NULL dereference and similar.
I still do not know the aarch64 ABI well enough. If only I had time!
> For context, why do you think this would work on power specifically? I
> wonder if our rationale is similar.
On most 64-bit Power ABIs all stack frames are connected together as a
linked list (which is updated atomically, importantly). This makes it
possible to always find all previous stack frames.
> Are you aware of anything in particular that breaks using
> __builtin_frame_address(1) in non-portable code, or is this just a
> general sentiment of this not being a supported use-case?
It is not supported, and trying to do it anyway can crash: it can use
random stack contents as pointer! Not really "random" of course, but
where it thinks to find a pointer into the previous frame, which is not
something it can rely on (unless the ABI guarantees it somehow).
See gcc.gnu.org/PR60109 for example.
> > > Unless we can get some strong guarantees from compiler folk such that we
> > > can guarantee a specific function acts boundary for unwinding (and
> > > doesn't itself get split, etc), the only reliable way I can think to
> > > solve this requires an assembly trampoline. Whatever we do is liable to
> > > need some invasive rework.
> >
> > You cannot get such a guarantee, other than not letting the compiler
> > see into the routine at all, like with assembler code (not inline asm,
> > real assembler code).
>
> If we cannot reliably ensure this then I'm happy to go write an assembly
> trampoline to snapshot the state at a function call boundary (where our
> procedure call standard mandates the state of the LR, FP, and frame
> records pointed to by the FP).
Is the frame pointer required?!
> This'll require reworking a reasonable
> amount of code cross-architecture, so I'll need to get some more
> concrete justification (e.g. examples of things that can go wrong in
> practice).
Say you have a function that does dynamic stack allocation, then there
is usually no way to find the previous stack frame (without function-
specific knowledge). So __builtin_frame_address cannot work (it knows
nothing about frames further up).
Dynamic stack allocation (alloca, or variable length automatic arrays)
is just the most common and most convenient example; it is not the only
case you have problems here.
> > The real way forward is to bite the bullet and to no longer pretend you
> > can do a full backtrace from just the stack contents. You cannot.
>
> I think what you mean here is that there's no reliable way to handle the
> current/leaf function, right? If so I do agree.
No, I meant what I said.
There is the separate issue that you do not know where the return
address (etc.) is stored in a function that has not yet done a call
itself, sure. You cannot assume anything the ABI does not tell you you
can depend on.
> Beyond that I believe that arm64's frame records should be sufficient.
Do you have a simple linked list connecting all frames? The aarch64 GCC
port does not define anything special here (DYNAMIC_CHAIN_ADDRESS), so
the default will be used: every frame pointer has to point to the
previous one, no exceptions whatsoever.
Segher
^ permalink raw reply
* Re: [PATCH 1/9] fs: rename alloc_anon_inode to alloc_anon_inode_sb
From: Gao Xiang @ 2021-03-09 19:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Minchan Kim, Alex Williamson, Nadav Amit, Al Viro,
Daniel Vetter, linux-fsdevel, Andrew Morton, linuxppc-dev,
Nitin Gupta
In-Reply-To: <20210309155348.974875-2-hch@lst.de>
On Tue, Mar 09, 2021 at 04:53:40PM +0100, Christoph Hellwig wrote:
> Rename alloc_inode to free the name for a new variant that does not
> need boilerplate to create a super_block first.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
That is a nice idea as well to avoid sb by introducing an unique
fs...
Reviewed-by: Gao Xiang <hsiangkao@redhat.com>
Thanks,
Gao Xiang
^ permalink raw reply
* Re: [PATCH 2/9] fs: add an argument-less alloc_anon_inode
From: Gao Xiang @ 2021-03-09 19:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jason Gunthorpe, Michael S. Tsirkin, VMware, Inc.,
David Hildenbrand, linux-kernel, dri-devel, virtualization,
linux-mm, Minchan Kim, Alex Williamson, Nadav Amit, Al Viro,
Daniel Vetter, linux-fsdevel, Andrew Morton, linuxppc-dev,
Nitin Gupta
In-Reply-To: <20210309155348.974875-3-hch@lst.de>
On Tue, Mar 09, 2021 at 04:53:41PM +0100, Christoph Hellwig wrote:
> Add a new alloc_anon_inode helper that allocates an inode on
> the anon_inode file system.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Gao Xiang <hsiangkao@redhat.com>
Thanks,
Gao Xiang
^ permalink raw reply
* Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property
From: Daniel Henrique Barboza @ 2021-03-09 17:08 UTC (permalink / raw)
To: Cédric Le Goater, Greg Kurz; +Cc: linuxppc-dev
In-Reply-To: <8dd98e22-1f10-e87b-3fe3-e786bc9a8d71@kaod.org>
On 3/9/21 12:33 PM, Cédric Le Goater wrote:
> On 3/8/21 6:13 PM, Greg Kurz wrote:
>> On Wed, 3 Mar 2021 18:48:50 +0100
>> Cédric Le Goater <clg@kaod.org> wrote:
>>
>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
>>> target for a source located on the same chip when possible. This field
>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
>>> on pSeries under KVM when NUMA nodes are defined but it is undefined
>>
>> This sentence seems to have a syntax problem... like it is missing an
>> 'and' before 'on pSeries'.
>
> ah yes, or simply a comma.
>
>>> under PowerVM. The XIVE source structure has a similar field
>>> 'src_chip' which is only assigned on the PowerNV platform.
>>>
>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
>>> default node. It will also give us the opportunity to set the affinity
>>> of a source on pSeries when we can localize them.
>>>
>>
>> IIUC this relies on the fact that the NUMA node id is == to chip id
>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
>> with this change.
>
> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
> in Cc:)
That's correct. H_HOME_NODE_ASSOCIATIVITY returns not only the node_id, but
a list with the ibm,associativity domains of the CPU that "proc-no" (processor
identifier) is mapped to inside QEMU.
node_id in this case, considering that we're working with a reference-points
of size 4, is the 4th element of the returned list. The last element is
"procno" itself.
>
> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
> the node id. This value is built from the chip id in OPAL, so the
> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
> property are unlikely to be different.
>
> cpu_to_node(cpu) is used in many places to allocate the structures
> locally to the owning node. XIVE is not an exception (see below in the
> same patch), it is better to be consistent and get the same information
> (node id) using the same routine.
>
>
> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
> unifies the controllers of the system to only expose one the OS. This
> is problematic and should be changed but it's another topic.
>
>
>> On the other hand, you have the pSeries case under PowerVM that
>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.
>
> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
> chip id.
>
> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
> always correct btw)
If you have a way to reliably reproduce this, let me know and I'll fix it
up in QEMU.
Thanks,
DHB
>
>> It looks like the chip id is only used for localization purpose in
>> this case, right ?
>
> Yes and PAPR sources are not localized. So it's not used. MSI sources
> could be if we rewrote the MSI driver.
>
>> In this case, what about doing this change for pSeries only,
>> somewhere in spapr.c ?
>
> The IPI code is common to all platforms and all have the same issue.
> I rather not.
>
> Thanks,
>
> C.
>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>> arch/powerpc/sysdev/xive/common.c | 7 +------
>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>
>>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>>> index 595310e056f4..b8e456da28aa 100644
>>> --- a/arch/powerpc/sysdev/xive/common.c
>>> +++ b/arch/powerpc/sysdev/xive/common.c
>>> @@ -1335,16 +1335,11 @@ static int xive_prepare_cpu(unsigned int cpu)
>>>
>>> xc = per_cpu(xive_cpu, cpu);
>>> if (!xc) {
>>> - struct device_node *np;
>>> -
>>> xc = kzalloc_node(sizeof(struct xive_cpu),
>>> GFP_KERNEL, cpu_to_node(cpu));
>>> if (!xc)
>>> return -ENOMEM;
>>> - np = of_get_cpu_node(cpu, NULL);
>>> - if (np)
>>> - xc->chip_id = of_get_ibm_chip_id(np);
>>> - of_node_put(np);
>>> + xc->chip_id = cpu_to_node(cpu);
>>> xc->hw_ipi = XIVE_BAD_IRQ;
>>>
>>> per_cpu(xive_cpu, cpu) = xc;
>>
>
^ permalink raw reply
* Re: [PATCH] ethernet: ucc_geth: Use kmemdup instead of kmalloc and memcpy
From: Rasmus Villemoes @ 2021-03-09 21:41 UTC (permalink / raw)
To: angkery, leoyang.li, davem, kuba
Cc: netdev, linuxppc-dev, linux-kernel, Junlin Yang
In-Reply-To: <20210305142711.3022-1-angkery@163.com>
On 05/03/2021 15.27, angkery wrote:
> From: Junlin Yang <yangjunlin@yulong.com>
>
> Fixes coccicheck warnings:
> ./drivers/net/ethernet/freescale/ucc_geth.c:3594:11-18:
> WARNING opportunity for kmemdup
>
> Signed-off-by: Junlin Yang <yangjunlin@yulong.com>
> ---
> drivers/net/ethernet/freescale/ucc_geth.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> index ef4e2fe..2c079ad 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.c
> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> @@ -3591,10 +3591,9 @@ static int ucc_geth_probe(struct platform_device* ofdev)
> if ((ucc_num < 0) || (ucc_num > 7))
> return -ENODEV;
>
> - ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);
> + ug_info = kmemdup(&ugeth_primary_info, sizeof(*ug_info), GFP_KERNEL);
> if (ug_info == NULL)
> return -ENOMEM;
> - memcpy(ug_info, &ugeth_primary_info, sizeof(*ug_info));
>
> ug_info->uf_info.ucc_num = ucc_num;
>
>
Ah, yes, of course, I should have used that.
Acked-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
^ permalink raw reply
* Re: [PATCH v2 4/7] CMDLINE: powerpc: convert to generic builtin command line
From: Daniel Walker @ 2021-03-09 21:40 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rob Herring, Ruslan Ruslichenko, Ruslan Bilovol,
Daniel Gimpelevich, linuxppc-dev, x86, linux-mips, linux-kernel,
Paul Mackerras, xe-linux-external, Andrew Morton, Will Deacon
In-Reply-To: <c5c8b57e-7954-ec02-188a-7f85cb0af731@csgroup.eu>
On Tue, Mar 09, 2021 at 08:56:47AM +0100, Christophe Leroy wrote:
>
>
> Le 09/03/2021 à 01:02, Daniel Walker a écrit :
> > This updates the powerpc code to use the CONFIG_GENERIC_CMDLINE
> > option.
> >
> > Cc: xe-linux-external@cisco.com
> > Signed-off-by: Ruslan Ruslichenko <rruslich@cisco.com>
> > Signed-off-by: Ruslan Bilovol <rbilovol@cisco.com>
> > Signed-off-by: Daniel Walker <danielwa@cisco.com>
> > ---
> > arch/powerpc/Kconfig | 37 +--------------------------------
> > arch/powerpc/kernel/prom.c | 1 +
> > arch/powerpc/kernel/prom_init.c | 35 ++++++++++++++++++-------------
> > 3 files changed, 23 insertions(+), 50 deletions(-)
> >
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index 107bb4319e0e..276b06d5c961 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -167,6 +167,7 @@ config PPC
> > select EDAC_SUPPORT
> > select GENERIC_ATOMIC64 if PPC32
> > select GENERIC_CLOCKEVENTS_BROADCAST if SMP
> > + select GENERIC_CMDLINE
> > select GENERIC_CMOS_UPDATE
> > select GENERIC_CPU_AUTOPROBE
> > select GENERIC_CPU_VULNERABILITIES if PPC_BARRIER_NOSPEC
> > @@ -906,42 +907,6 @@ config PPC_DENORMALISATION
> > Add support for handling denormalisation of single precision
> > values. Useful for bare metal only. If unsure say Y here.
> > -config CMDLINE
> > - string "Initial kernel command string"
> > - default ""
> > - help
> > - On some platforms, there is currently no way for the boot loader to
> > - pass arguments to the kernel. For these platforms, you can supply
> > - some command-line options at build time by entering them here. In
> > - most cases you will need to specify the root device here.
> > -
> > -choice
> > - prompt "Kernel command line type" if CMDLINE != ""
> > - default CMDLINE_FROM_BOOTLOADER
> > -
> > -config CMDLINE_FROM_BOOTLOADER
> > - bool "Use bootloader kernel arguments if available"
> > - help
> > - Uses the command-line options passed by the boot loader. If
> > - the boot loader doesn't provide any, the default kernel command
> > - string provided in CMDLINE will be used.
> > -
> > -config CMDLINE_EXTEND
> > - bool "Extend bootloader kernel arguments"
> > - help
> > - The command-line arguments provided by the boot loader will be
> > - appended to the default kernel command string.
> > -
> > -config CMDLINE_FORCE
> > - bool "Always use the default kernel command string"
> > - help
> > - Always use the default kernel command string, even if the boot
> > - loader passes other arguments to the kernel.
> > - This is useful if you cannot or don't want to change the
> > - command-line options your boot loader passes to the kernel.
> > -
> > -endchoice
> > -
> > config EXTRA_TARGETS
> > string "Additional default image types"
> > help
> > diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> > index ae3c41730367..96d0a01be1b4 100644
> > --- a/arch/powerpc/kernel/prom.c
> > +++ b/arch/powerpc/kernel/prom.c
> > @@ -27,6 +27,7 @@
> > #include <linux/irq.h>
> > #include <linux/memblock.h>
> > #include <linux/of.h>
> > +#include <linux/cmdline.h>
>
> Why is this needed in prom.c ?
Must have been a mistake, I don't think it's needed.
> > #include <linux/of_fdt.h>
> > #include <linux/libfdt.h>
> > #include <linux/cpu.h>
> > diff --git a/arch/powerpc/kernel/prom_init.c b/arch/powerpc/kernel/prom_init.c
> > index e9d4eb6144e1..657241534d69 100644
> > --- a/arch/powerpc/kernel/prom_init.c
> > +++ b/arch/powerpc/kernel/prom_init.c
> > @@ -27,6 +27,7 @@
> > #include <linux/initrd.h>
> > #include <linux/bitops.h>
> > #include <linux/pgtable.h>
> > +#include <linux/cmdline.h>
> > #include <asm/prom.h>
> > #include <asm/rtas.h>
> > #include <asm/page.h>
> > @@ -242,15 +243,6 @@ static int __init prom_strcmp(const char *cs, const char *ct)
> > return 0;
> > }
> > -static char __init *prom_strcpy(char *dest, const char *src)
> > -{
> > - char *tmp = dest;
> > -
> > - while ((*dest++ = *src++) != '\0')
> > - /* nothing */;
> > - return tmp;
> > -}
> > -
>
> This game with prom_strcpy() should go a separate preceeding patch.
>
> Also, it looks like checkpatch.pl recommends to use strscpy() instead of strlcpy().
strscpy() is very large. I'm not sure it's compatible with this prom_init.c
environment.
> > static int __init prom_strncmp(const char *cs, const char *ct, size_t count)
> > {
> > unsigned char c1, c2;
> > @@ -276,6 +268,20 @@ static size_t __init prom_strlen(const char *s)
> > return sc - s;
> > }
> > +static size_t __init prom_strlcpy(char *dest, const char *src, size_t size)
> > +{
> > + size_t ret = prom_strlen(src);
> > +
> > + if (size) {
> > + size_t len = (ret >= size) ? size - 1 : ret;
> > +
> > + memcpy(dest, src, len);
> > + dest[len] = '\0';
> > + }
> > + return ret;
> > +}
> > +
> > +
> > static int __init prom_memcmp(const void *cs, const void *ct, size_t count)
> > {
> > const unsigned char *su1, *su2;
> > @@ -304,6 +310,7 @@ static char __init *prom_strstr(const char *s1, const char *s2)
> > return NULL;
> > }
> > +#ifdef GENERIC_CMDLINE_NEED_STRLCAT
> > static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
> > {
> > size_t dsize = prom_strlen(dest);
> > @@ -323,6 +330,7 @@ static size_t __init prom_strlcat(char *dest, const char *src, size_t count)
> > return res;
> > }
> > +#endif
> > #ifdef CONFIG_PPC_PSERIES
> > static int __init prom_strtobool(const char *s, bool *res)
> > @@ -775,12 +783,11 @@ static void __init early_cmdline_parse(void)
> > prom_cmd_line[0] = 0;
> > p = prom_cmd_line;
> > - if (!IS_ENABLED(CONFIG_CMDLINE_FORCE) && (long)prom.chosen > 0)
> > + if ((long)prom.chosen > 0)
> > l = prom_getprop(prom.chosen, "bootargs", p, COMMAND_LINE_SIZE-1);
> > - if (IS_ENABLED(CONFIG_CMDLINE_EXTEND) || l <= 0 || p[0] == '\0')
> > - prom_strlcat(prom_cmd_line, " " CONFIG_CMDLINE,
> > - sizeof(prom_cmd_line));
> > + cmdline_add_builtin_custom(prom_cmd_line, (l > 0 ? p : NULL), sizeof(prom_cmd_line),
> > + __prombss, prom_strlcpy, prom_strlcat);
>
> So we are referencing a function that doesn't exist (namely prom_strlcat).
> But it works because cmdline_add_builtin_custom() looks like a function but
> is in fact an obscure macro that doesn't use prom_strlcat() unless
> GENERIC_CMDLINE_NEED_STRLCAT is defined.
>
> IMHO that's awful for readability and code maintenance.
powerpc is a special case, there's no other users like this. The reason is
because of all the difficulty in this prom_init.c code. A lot of the generic
code has similar kind of changes to work across architectures.
> > prom_printf("command line: %s\n", prom_cmd_line);
> > @@ -2706,7 +2713,7 @@ static void __init flatten_device_tree(void)
> > /* Add "phandle" in there, we'll need it */
> > namep = make_room(&mem_start, &mem_end, 16, 1);
> > - prom_strcpy(namep, "phandle");
> > + prom_strlcpy(namep, "phandle", 8);
>
> Should be in a separate patch.
I can move it, I missed that from the first round.
Daniel
^ permalink raw reply
* Re: [PATCH v2 3/7] powerpc: convert config files to generic cmdline
From: Daniel Walker @ 2021-03-09 21:29 UTC (permalink / raw)
To: Christophe Leroy
Cc: Rob Herring, Daniel Gimpelevich, linuxppc-dev, x86, linux-mips,
linux-kernel, Paul Mackerras, xe-linux-external, Andrew Morton,
Will Deacon
In-Reply-To: <5f865584-09c9-d21f-ffb7-23cf07cf058e@csgroup.eu>
On Tue, Mar 09, 2021 at 08:47:09AM +0100, Christophe Leroy wrote:
>
>
> Le 09/03/2021 à 01:02, Daniel Walker a écrit :
> > This is a scripted mass convert of the config files to use
> > the new generic cmdline. There is a bit of a trim effect here.
> > It would seems that some of the config haven't been trimmed in
> > a while.
>
> If you do that in a separate patch, you loose bisectability.
>
> I think it would have been better to do things in a different way, more or less like I did in my series:
> 1/ Provide GENERIC cmdline at the same functionnality level as what is
> spread in the different architectures
> 2/ Convert architectures to the generic with least churn.
> 3/ Add new features to the generic
You have to have the churn eventually, no matter how you do it. The only way you
don't have churn is if you never upgrade the feature set.
> >
> > The bash script used to convert is as follows,
> >
> > if [[ -z "$1" || -z "$2" ]]; then
> > echo "Two arguments are needed."
> > exit 1
> > fi
> > mkdir $1
> > cp $2 $1/.config
> > sed -i 's/CONFIG_CMDLINE=/CONFIG_CMDLINE_BOOL=y\nCONFIG_CMDLINE_PREPEND=/g' $1/.config
>
> This is not correct.
>
> By default, on powerpc the provided command line is used only if the bootloader doesn't provide one.
>
> Otherwise:
> - the builtin command line is appended to the one provided by the bootloader
> if CONFIG_CMDLINE_EXTEND is selected
> - the builtin command line replaces to the one provided by the bootloader if
> CONFIG_CMDLINE_FORCE is selected
I think my changes maintain most of this due to the override of
CONFIG_CMDLINE_PREPEND. This is an upgrade and the inflexibility in powerpc is
an example of why these changes were created in the first place.
For example , say the default command line is "root=/dev/issblk0" from iss476
platform. And the bootloader adds "root=/dev/sda1"
The result is <prepend><bootloader><append>.
Then you have,
root=/dev/issblk0 root=/dev/sda1
and the bootloader has precedent over the default command line. So root= in the
above cases is defined by the bootloader.
The only issue would be if a person wants to override the default command line
with an unrelated bootloader command line. I don't know how many people do this,
but I doubt it's many. Can you think of any use cases like this?
I would imagine there are many more people who have to entirely duplicate the
default command line in the boot loader when they really just want to change a
single part of it like the root= device or console device or speed.
Daniel
^ permalink raw reply
* Re: [PATCH v2 1/8] powerpc/xive: Use cpu_to_node() instead of ibm,chip-id property
From: Cédric Le Goater @ 2021-03-09 17:26 UTC (permalink / raw)
To: Daniel Henrique Barboza, Greg Kurz
Cc: list@suse.de:PowerPC, linuxppc-dev, QEMU Developers, David Gibson
In-Reply-To: <3180b5c6-e61f-9c5f-3c80-f10e69dc5785@linux.ibm.com>
On 3/9/21 6:08 PM, Daniel Henrique Barboza wrote:
>
>
> On 3/9/21 12:33 PM, Cédric Le Goater wrote:
>> On 3/8/21 6:13 PM, Greg Kurz wrote:
>>> On Wed, 3 Mar 2021 18:48:50 +0100
>>> Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>>> The 'chip_id' field of the XIVE CPU structure is used to choose a
>>>> target for a source located on the same chip when possible. This field
>>>> is assigned on the PowerNV platform using the "ibm,chip-id" property
>>>> on pSeries under KVM when NUMA nodes are defined but it is undefined
>>>
>>> This sentence seems to have a syntax problem... like it is missing an
>>> 'and' before 'on pSeries'.
>>
>> ah yes, or simply a comma.
>>
>>>> under PowerVM. The XIVE source structure has a similar field
>>>> 'src_chip' which is only assigned on the PowerNV platform.
>>>>
>>>> cpu_to_node() returns a compatible value on all platforms, 0 being the
>>>> default node. It will also give us the opportunity to set the affinity
>>>> of a source on pSeries when we can localize them.
>>>>
>>>
>>> IIUC this relies on the fact that the NUMA node id is == to chip id
>>> on PowerNV, i.e. xc->chip_id which is passed to OPAL remain stable
>>> with this change.
>>
>> Linux sets the NUMA node in numa_setup_cpu(). On pseries, the hcall
>> H_HOME_NODE_ASSOCIATIVITY returns the node id if I am correct (Daniel
>> in Cc:)
>
> That's correct. H_HOME_NODE_ASSOCIATIVITY returns not only the node_id, but
> a list with the ibm,associativity domains of the CPU that "proc-no" (processor
> identifier) is mapped to inside QEMU.
>
> node_id in this case, considering that we're working with a reference-points
> of size 4, is the 4th element of the returned list. The last element is
> "procno" itself.
>
>
>>
>> On PowerNV, Linux uses "ibm,associativity" property of the CPU to find
>> the node id. This value is built from the chip id in OPAL, so the
>> value returned by cpu_to_node(cpu) and the value of the "ibm,chip-id"
>> property are unlikely to be different.
>>
>> cpu_to_node(cpu) is used in many places to allocate the structures
>> locally to the owning node. XIVE is not an exception (see below in the
>> same patch), it is better to be consistent and get the same information
>> (node id) using the same routine.
>>
>>
>> In Linux, "ibm,chip-id" is only used in low level PowerNV drivers :
>> LPC, XSCOM, RNG, VAS, NX. XIVE should be in that list also but skiboot
>> unifies the controllers of the system to only expose one the OS. This
>> is problematic and should be changed but it's another topic.
>>
>>
>>> On the other hand, you have the pSeries case under PowerVM that
>>> doesn't xc->chip_id, which isn't passed to any hcall AFAICT.
>>
>> yes "ibm,chip-id" is an OPAL concept unfortunately and it has no meaning
>> under PAPR. xc->chip_id on pseries (PowerVM) will contains an invalid
>> chip id.
>>
>> QEMU/KVM exposes "ibm,chip-id" but it's not used. (its value is not
>> always correct btw)
>
>
> If you have a way to reliably reproduce this, let me know and I'll fix it
> up in QEMU.
with :
-smp 4,cores=1,maxcpus=8 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
# dmesg | grep numa
[ 0.013106] numa: Node 0 CPUs: 0-1
[ 0.013136] numa: Node 1 CPUs: 2-3
# dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
ibm,chip-id = <0x01>;
ibm,chip-id = <0x02>;
ibm,chip-id = <0x00>;
ibm,chip-id = <0x03>;
with :
-smp 4,cores=4,maxcpus=8,threads=1 -object memory-backend-ram,id=ram-node0,size=2G -numa node,nodeid=0,cpus=0-1,cpus=4-5,memdev=ram-node0 -object memory-backend-ram,id=ram-node1,size=2G -numa node,nodeid=1,cpus=2-3,cpus=6-7,memdev=ram-node1
# dmesg | grep numa
[ 0.013106] numa: Node 0 CPUs: 0-1
[ 0.013136] numa: Node 1 CPUs: 2-3
# dtc -I fs /proc/device-tree/cpus/ -f | grep ibm,chip-id
ibm,chip-id = <0x00>;
ibm,chip-id = <0x00>;
ibm,chip-id = <0x00>;
ibm,chip-id = <0x00>;
I think we should simply remove "ibm,chip-id" since it's not used and
not in the PAPR spec.
Thanks,
C.
>
> Thanks,
>
>
> DHB
>
>
>>
>>> It looks like the chip id is only used for localization purpose in
>>> this case, right ?
>>
>> Yes and PAPR sources are not localized. So it's not used. MSI sources
>> could be if we rewrote the MSI driver.
>>
>>> In this case, what about doing this change for pSeries only,
>>> somewhere in spapr.c ?
>>
>> The IPI code is common to all platforms and all have the same issue.
>> I rather not.
>>
>> Thanks,
>>
>> C.
>>
>>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>>> ---
>>>> arch/powerpc/sysdev/xive/common.c | 7 +------
>>>> 1 file changed, 1 insertion(+), 6 deletions(-)
>>>>
>>>> diff --git a/arch/powerpc/sysdev/xive/common.c b/arch/powerpc/sysdev/xive/common.c
>>>> index 595310e056f4..b8e456da28aa 100644
>>>> --- a/arch/powerpc/sysdev/xive/common.c
>>>> +++ b/arch/powerpc/sysdev/xive/common.c
>>>> @@ -1335,16 +1335,11 @@ static int xive_prepare_cpu(unsigned int cpu)
>>>> xc = per_cpu(xive_cpu, cpu);
>>>> if (!xc) {
>>>> - struct device_node *np;
>>>> -
>>>> xc = kzalloc_node(sizeof(struct xive_cpu),
>>>> GFP_KERNEL, cpu_to_node(cpu));
>>>> if (!xc)
>>>> return -ENOMEM;
>>>> - np = of_get_cpu_node(cpu, NULL);
>>>> - if (np)
>>>> - xc->chip_id = of_get_ibm_chip_id(np);
>>>> - of_node_put(np);
>>>> + xc->chip_id = cpu_to_node(cpu);
>>>> xc->hw_ipi = XIVE_BAD_IRQ;
>>>> per_cpu(xive_cpu, cpu) = xc;
>>>
>>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox