* Re: [patch V3 25/35] powerpc/pseries/msi: Let core code check for contiguous entries
From: Jason Gunthorpe @ 2021-12-13 14:05 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221814.720998720@linutronix.de>
On Fri, Dec 10, 2021 at 11:19:22PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Set the domain info flag and remove the check.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: "Cédric Le Goater" <clg@kaod.org>
> Cc: linuxppc-dev@lists.ozlabs.org
>
> ---
> V2: Remove it completely - Cedric
> ---
> arch/powerpc/platforms/pseries/msi.c | 33 ++++++++-------------------------
> 1 file changed, 8 insertions(+), 25 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [patch V3 06/35] powerpc/pseries/msi: Use PCI device properties
From: Jason Gunthorpe @ 2021-12-13 14:04 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.556202506@linutronix.de>
On Fri, Dec 10, 2021 at 11:18:52PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> instead of fiddling with MSI descriptors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
> V3: Use pci_dev->msix_enabled - Jason
> ---
> arch/powerpc/platforms/pseries/msi.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
> --- a/arch/powerpc/platforms/pseries/msi.c
> +++ b/arch/powerpc/platforms/pseries/msi.c
> @@ -448,8 +448,7 @@ static int pseries_msi_ops_prepare(struc
> int nvec, msi_alloc_info_t *arg)
> {
> struct pci_dev *pdev = to_pci_dev(dev);
> - struct msi_desc *desc = first_pci_msi_entry(pdev);
> - int type = desc->pci.msi_attrib.is_msix ? PCI_CAP_ID_MSIX : PCI_CAP_ID_MSI;
> + int type = pdev->msix_enabled ? PCI_CAP_ID_MSIX : PCI_CAP_ID_MSI;
Long term it probably makes sense to change the msi_domain_ops so that
it has PCI versions of the ops to use in places like this that hard
assume PCI is the only kind of MSI at all.
If the non-PCI op isn't provided then things like IMS would be denied
- and the PCI op can directly pass in a pci_dev * so we don't have all
these to_pci_devs() in drivers.
Jason
^ permalink raw reply
* Re: [patch V3 05/35] powerpc/cell/axon_msi: Use PCI device property
From: Jason Gunthorpe @ 2021-12-13 14:01 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.493922179@linutronix.de>
On Fri, Dec 10, 2021 at 11:18:51PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> instead of fiddling with MSI descriptors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: linuxppc-dev@lists.ozlabs.org
> ---
> V3: Use pci_dev property - Jason
> V2: Invoke the function with the correct number of arguments - Andy
> ---
> arch/powerpc/platforms/cell/axon_msi.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [patch V3 04/35] genirq/msi: Use PCI device property
From: Jason Gunthorpe @ 2021-12-13 13:59 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.434156196@linutronix.de>
On Fri, Dec 10, 2021 at 11:18:49PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> to determine whether this is MSI or MSIX instead of consulting MSI
> descriptors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V2: Use PCI device property - Jason
> ---
> kernel/irq/msi.c | 17 ++---------------
> 1 file changed, 2 insertions(+), 15 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [patch V3 03/35] x86/apic/msi: Use PCI device MSI property
From: Jason Gunthorpe @ 2021-12-13 13:59 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.372357371@linutronix.de>
On Fri, Dec 10, 2021 at 11:18:47PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> instead of fiddling with MSI descriptors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V3: Use pci_dev->msix_enabled - Jason
> ---
> arch/x86/kernel/apic/msi.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [patch V3 02/35] x86/pci/XEN: Use PCI device property
From: Jason Gunthorpe @ 2021-12-13 13:56 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.311410967@linutronix.de>
On Fri, Dec 10, 2021 at 11:18:46PM +0100, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> instead of fiddling with MSI descriptors.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Juergen Gross <jgross@suse.com>
> Cc: xen-devel@lists.xenproject.org
> ---
> V3: Use pci_dev->msix_enabled.
> ---
> arch/x86/pci/xen.c | 9 ++-------
> 1 file changed, 2 insertions(+), 7 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [patch V3 01/35] PCI/MSI: Set pci_dev::msi[x]_enabled early
From: Jason Gunthorpe @ 2021-12-13 13:37 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, iommu,
Peter Ujfalusi, Bjorn Helgaas, linux-arm-kernel, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, Vinod Koul, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221813.250049810@linutronix.de>
On Fri, Dec 10, 2021 at 11:18:44PM +0100, Thomas Gleixner wrote:
> There are quite some places which retrieve the first MSI descriptor to
> evaluate whether the setup is for MSI or MSI-X. That's required because
> pci_dev::msi[x]_enabled is only set when the setup completed successfully.
>
> There is no real reason why msi[x]_enabled can't be set at the beginning of
> the setup sequence and cleared in case of a failure.
>
> Implement that so the MSI descriptor evaluations can be converted to simple
> property queries.
>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V3: New patch
> ---
> drivers/pci/msi/msi.c | 23 +++++++++++++++++------
> 1 file changed, 17 insertions(+), 6 deletions(-)
Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>
Jason
^ permalink raw reply
* Re: [PATCH v4 06/10] powerpc/mm: Use generic_get_unmapped_area() and call it from arch_get_unmapped_area()
From: Michael Ellerman @ 2021-12-13 13:02 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
alex@ghiti.fr
Cc: linux-mm@kvack.org, akpm@linux-foundation.org,
linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
In-Reply-To: <8f54a8d097c402d808147b2044365ebfda2862dd.1638976229.git.christophe.leroy@csgroup.eu>
Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> Use the generic version of arch_get_unmapped_area() which
> is now available at all time instead of its copy
> radix__arch_get_unmapped_area()
>
> Instead of setting mm->get_unmapped_area() to either
> arch_get_unmapped_area() or generic_get_unmapped_area(),
> always set it to arch_get_unmapped_area() and call
> generic_get_unmapped_area() from there when radix is enabled.
>
> Do the same with radix__arch_get_unmapped_area_topdown()
>
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> arch/powerpc/mm/mmap.c | 127 ++---------------------------------------
> 1 file changed, 6 insertions(+), 121 deletions(-)
>
> diff --git a/arch/powerpc/mm/mmap.c b/arch/powerpc/mm/mmap.c
> index 9b0d6e395bc0..46781d0103d1 100644
> --- a/arch/powerpc/mm/mmap.c
> +++ b/arch/powerpc/mm/mmap.c
> @@ -81,115 +81,15 @@ static inline unsigned long mmap_base(unsigned long rnd,
> }
>
> #ifdef HAVE_ARCH_UNMAPPED_AREA
> -#ifdef CONFIG_PPC_RADIX_MMU
> -/*
> - * Same function as generic code used only for radix, because we don't need to overload
> - * the generic one. But we will have to duplicate, because hash select
> - * HAVE_ARCH_UNMAPPED_AREA
> - */
> -static unsigned long
> -radix__arch_get_unmapped_area(struct file *filp, unsigned long addr,
> - unsigned long len, unsigned long pgoff,
> - unsigned long flags)
> -{
> - struct mm_struct *mm = current->mm;
> - struct vm_area_struct *vma;
> - int fixed = (flags & MAP_FIXED);
> - unsigned long high_limit;
> - struct vm_unmapped_area_info info;
> -
> - high_limit = DEFAULT_MAP_WINDOW;
> - if (addr >= high_limit || (fixed && (addr + len > high_limit)))
> - high_limit = TASK_SIZE;
> -
> - if (len > high_limit)
> - return -ENOMEM;
There are some differences in the above vs the generic code, the generic
arch_get_unmapped_area_topdown() in mm/mmap.c does:
const unsigned long mmap_end = arch_get_mmap_end(addr);
if (len > mmap_end - mmap_min_addr)
return -ENOMEM;
if (flags & MAP_FIXED)
return addr;
Our current code adjusts high_limit for fixed mappings that span above
the default map window. We added that logic in:
35602f82d0c7 ("powerpc/64s/hash: Allow MAP_FIXED allocations to cross 128TB boundary")
That means a fixed mapping that crosses the 128T boundary will be
allowed by our code.
On the other hand the generic code will allow a fixed mapping to cross
the 128T boundary, but only if the size of the mapping is < ~128T.
(The actual size limit is (128T - mmap_min_addr), which is usually 4K or
64K, but is adjustable.)
It's unlikely that any apps are doing fixed mappings larger than 128T
that cross the 128T boundary, but I think we need to allow it. 128T
seems like a lot, but is not compared to the entire 4PB address space.
So I think we need to fix that in the generic code.
The easiest option is probably to pass flags to arch_get_mmap_end(), and
then the arches can decide whether to adjust the return value based on
flags.
Then there's also the extra check we have here:
> - if (fixed) {
> - if (addr > high_limit - len)
> - return -ENOMEM;
> - return addr;
> - }
I think we can drop that when converting to the generic version, the
only case in which it matters is when high_limit == TASK_SIZE, and
get_unmapped_area() already does that check after calling us:
if (addr > TASK_SIZE - len)
return -ENOMEM;
cheers
^ permalink raw reply
* Re: [PATCH v2] of/fdt: Rework early_init_dt_scan_memory() to call directly
From: Michael Ellerman @ 2021-12-13 12:46 UTC (permalink / raw)
To: Rob Herring, John Crispin, Thomas Bogendoerfer,
Benjamin Herrenschmidt, Paul Mackerras, Frank Rowand
Cc: devicetree, linuxppc-dev, linux-mips, linux-kernel
In-Reply-To: <20211208155839.4084795-1-robh@kernel.org>
Rob Herring <robh@kernel.org> writes:
> Use of the of_scan_flat_dt() function predates libfdt and is discouraged
> as libfdt provides a nicer set of APIs. Rework
> early_init_dt_scan_memory() to be called directly and use libfdt.
...
> diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
> index 6e1a106f02eb..63762a3b75e8 100644
> --- a/arch/powerpc/kernel/prom.c
> +++ b/arch/powerpc/kernel/prom.c
> @@ -532,19 +532,19 @@ static int __init early_init_drmem_lmb(struct drmem_lmb *lmb,
> }
> #endif /* CONFIG_PPC_PSERIES */
>
> -static int __init early_init_dt_scan_memory_ppc(unsigned long node,
> - const char *uname,
> - int depth, void *data)
> +static int __init early_init_dt_scan_memory_ppc(void)
> {
> #ifdef CONFIG_PPC_PSERIES
> - if (depth == 1 &&
> - strcmp(uname, "ibm,dynamic-reconfiguration-memory") == 0) {
> + const void *fdt = initial_boot_params;
> + int node = fdt_path_offset(fdt, "/ibm,dynamic-reconfiguration-memory");
> +
> + if (node > 0) {
> walk_drmem_lmbs_early(node, NULL, early_init_drmem_lmb);
> return 0;
> }
> #endif
>
> - return early_init_dt_scan_memory(node, uname, depth, data);
> + return early_init_dt_scan_memory();
> }
>
> /*
> @@ -749,7 +749,7 @@ void __init early_init_devtree(void *params)
>
> /* Scan memory nodes and rebuild MEMBLOCKs */
> early_init_dt_scan_root();
> - of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> + early_init_dt_scan_memory_ppc();
>
> parse_early_param();
>
> @@ -858,7 +858,7 @@ void __init early_get_first_memblock_info(void *params, phys_addr_t *size)
> */
> add_mem_to_memblock = 0;
> early_init_dt_scan_root();
> - of_scan_flat_dt(early_init_dt_scan_memory_ppc, NULL);
> + early_init_dt_scan_memory_ppc();
> add_mem_to_memblock = 1;
>
> if (size)
This blows up one of my machines with:
[ 0.000000][ T0] printk: bootconsole [udbg0] enabled
-> early_setup(), dt_ptr: 0x1ec90000
[ 0.000000][ T0] ------------[ cut here ]------------
[ 0.000000][ T0] kernel BUG at arch/powerpc/mm/book3s64/hash_utils.c:2117!
[ 0.000000][ T0] Oops: Exception in kernel mode, sig: 5 [#1]
[ 0.000000][ T0] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA
[ 0.000000][ T0] Modules linked in:
[ 0.000000][ T0] CPU: 0 PID: 0 Comm: swapper Not tainted 5.16.0-rc4-00073-g81291383ffde-dirty #69
[ 0.000000][ T0] NIP: c0000000000924d8 LR: c000000002009764 CTR: c0000000000924d0
[ 0.000000][ T0] REGS: c000000002833bc0 TRAP: 0700 Not tainted (5.16.0-rc4-00073-g81291383ffde-dirty)
[ 0.000000][ T0] MSR: 8000000000021003 <SF,ME,RI,LE> CR: 24000244 XER: 20000001
[ 0.000000][ T0] CFAR: 0000000000000730 IRQMASK: 1
[ 0.000000][ T0] GPR00: c000000002009764 c000000002833e60 c000000002834100 ffffffffffffffff
[ 0.000000][ T0] GPR04: 0000000000000000 c000000002080866 0000000000000000 0000000000000000
[ 0.000000][ T0] GPR08: c000000002080864 0000000000000001 c0000000028d4100 c000000000ffe598
[ 0.000000][ T0] GPR12: c0000000000924d0 c000000002082200 0000000000000000 0000000000000000
[ 0.000000][ T0] GPR16: 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[ 0.000000][ T0] GPR20: 0000000000000001 0000000010004604 0000000000000000 0000000010004bfc
[ 0.000000][ T0] GPR24: 0000000000000000 c000000000000000 0000000002970000 c00000000008a480
[ 0.000000][ T0] GPR28: c0000000028e19f8 c00000001ec90000 c000000002865af8 000000001ec90000
[ 0.000000][ T0] NIP [c0000000000924d8] hash__setup_initial_memory_limit+0x18/0x110
[ 0.000000][ T0] LR [c000000002009764] early_init_devtree+0x13c/0x4ec
[ 0.000000][ T0] Call Trace:
[ 0.000000][ T0] [c000000002833e60] [c0000000020096fc] early_init_devtree+0xd4/0x4ec (unreliable)
[ 0.000000][ T0] [c000000002833f10] [c00000000200b008] early_setup+0xc8/0x22c
[ 0.000000][ T0] [c000000002833f90] [000000000000d368] 0xd368
[ 0.000000][ T0] Instruction dump:
[ 0.000000][ T0] 4bffff0c eaa10028 4bffff44 60000000 60000000 60420000 3c4c027a 38421c40
[ 0.000000][ T0] 7c0802a6 4bfe2e5d 3123ffff 7d291910 <0b090000> 3d220003 392919f8 e9290000
[ 0.000000][ T0] random: get_random_bytes called from oops_exit+0x54/0xa0 with crng_init=0
[ 0.000000][ T0] ---[ end trace 0000000000000000 ]---
It's complaining about memstart_addr being 0, which implies
early_init_dt_add_memory_arch() was never called.
Will try and get some more debug tomorrow.
cheers
^ permalink raw reply
* Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
From: Geert Uytterhoeven @ 2021-12-13 9:23 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Catalin Marinas, Linus Walleij,
Dave Hansen, Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Guo Ren, Pavel Machek, H. Peter Anvin, Dmitry Osipenko,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar, xen-devel,
linux-mips, Guenter Roeck, Len Brown, Albert Ou, Lee Jones,
Michał Mirosław, linux-m68k, Mark Brown,
Borislav Petkov, Greentime Hu, Paul Walmsley, linux-tegra,
Thomas Gleixner, Andy Shevchenko, Linux ARM, Juergen Gross,
Thomas Bogendoerfer, Daniel Lezcano, linux-parisc, Linux PM,
Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, linux-riscv, Palmer Dabbelt,
Philipp Zabel, Paul Mackerras, Andrew Morton, linuxppc-dev,
Joshua Thompson
In-Reply-To: <CAJZ5v0ieTwnBVjW8R_VTdPFH3yr5AwLc+ZEG5N3KrpTH+j8qZw@mail.gmail.com>
On Fri, Dec 10, 2021 at 8:14 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> > 10.12.2021 21:27, Rafael J. Wysocki пишет:
> > > On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> > >> 29.11.2021 03:26, Michał Mirosław пишет:
> > >>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> > >>>> 28.11.2021 03:28, Michał Mirosław пишет:
> > >>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> > >>>>>> Add sanity check which ensures that there are no two restart handlers
> > >>>>>> registered with the same priority. Normally it's a direct sign of a
> > >>>>>> problem if two handlers use the same priority.
> > >>>>>
> > >>>>> The patch doesn't ensure the property that there are no duplicated-priority
> > >>>>> entries on the chain.
> > >>>>
> > >>>> It's not the exact point of this patch.
> > >>>>
> > >>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
> > >>>>> -EBUSY or something istead of adding an entry with duplicate priority.
> > >>>>> That way it would need only one list traversal unless you want to
> > >>>>> register the duplicate anyway (then you would call the older
> > >>>>> atomic_notifier_chain_register() after reporting the error).
> > >>>>
> > >>>> The point of this patch is to warn developers about the problem that
> > >>>> needs to be fixed. We already have such troubling drivers in mainline.
> > >>>>
> > >>>> It's not critical to register different handlers with a duplicated
> > >>>> priorities, but such cases really need to be corrected. We shouldn't
> > >>>> break users' machines during transition to the new API, meanwhile
> > >>>> developers should take action of fixing theirs drivers.
> > >>>>
> > >>>>> (Or you could return > 0 when a duplicate is registered in
> > >>>>> atomic_notifier_chain_register() if the callers are prepared
> > >>>>> for that. I don't really like this way, though.)
> > >>>>
> > >>>> I had a similar thought at some point before and decided that I'm not in
> > >>>> favor of this approach. It's nicer to have a dedicated function that
> > >>>> verifies the uniqueness, IMO.
> > >>>
> > >>> I don't like the part that it traverses the list second time to check
> > >>> the uniqueness. But actually you could avoid that if
> > >>> notifier_chain_register() would always add equal-priority entries in
> > >>> reverse order:
> > >>>
> > >>> static int notifier_chain_register(struct notifier_block **nl,
> > >>> struct notifier_block *n)
> > >>> {
> > >>> while ((*nl) != NULL) {
> > >>> if (unlikely((*nl) == n)) {
> > >>> WARN(1, "double register detected");
> > >>> return 0;
> > >>> }
> > >>> - if (n->priority > (*nl)->priority)
> > >>> + if (n->priority >= (*nl)->priority)
> > >>> break;
> > >>> nl = &((*nl)->next);
> > >>> }
> > >>> n->next = *nl;
> > >>> rcu_assign_pointer(*nl, n);
> > >>> return 0;
> > >>> }
> > >>>
> > >>> Then the check for uniqueness after adding would be:
> > >>>
> > >>> WARN(nb->next && nb->priority == nb->next->priority);
> > >>
> > >> We can't just change the registration order because invocation order of
> > >> the call chain depends on the registration order
> > >
> > > It doesn't if unique priorities are required and isn't that what you want?
> > >
> > >> and some of current
> > >> users may rely on that order. I'm pretty sure that changing the order
> > >> will have unfortunate consequences.
> > >
> > > Well, the WARN() doesn't help much then.
> > >
> > > Either you can make all of the users register with unique priorities,
> > > and then you can make the registration reject non-unique ones, or you
> > > cannot assume them to be unique.
> >
> > There is no strong requirement for priorities to be unique, the reboot.c
> > code will work properly.
>
> In which case adding the WARN() is not appropriate IMV.
>
> Also I've looked at the existing code and at least in some cases the
> order in which the notifiers run doesn't matter. I'm not sure what
> the purpose of this patch is TBH.
>
> > The potential problem is on the user's side and the warning is intended
> > to aid the user.
>
> Unless somebody has the panic_on_warn mentioned previously set and
> really the user need not understand what the WARN() is about. IOW,
> WARN() helps developers, not users.
Do panic_on_warn and reboot_on_panic play well with having a WARN()
in the reboot notifier handling?
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* [PATCH] arch: powerpc: add missing of_node_put before break
From: Qing Wang @ 2021-12-13 9:42 UTC (permalink / raw)
To: Benjamin Herrenschmidt, Michael Ellerman, Paul Mackerras,
linuxppc-dev, linux-kernel
Cc: Wang Qing
From: Wang Qing <wangqing@vivo.com>
Fix following coccicheck warning:
WARNING: Function "for_each_node_by_name"
should have of_node_put() before return.
Early exits from for_each_node_by_name should decrement the
node reference counter.
Signed-off-by: Wang Qing <wangqing@vivo.com>
---
arch/powerpc/platforms/powermac/smp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/platforms/powermac/smp.c b/arch/powerpc/platforms/powermac/smp.c
index 3256a31..c9a044a
--- a/arch/powerpc/platforms/powermac/smp.c
+++ b/arch/powerpc/platforms/powermac/smp.c
@@ -598,8 +598,10 @@ static void __init smp_core99_setup_i2c_hwsync(int ncpus)
name = "Pulsar";
break;
}
- if (pmac_tb_freeze != NULL)
+ if (pmac_tb_freeze != NULL) {
+ of_node_put(cc);
break;
+ }
}
if (pmac_tb_freeze != NULL) {
/* Open i2c bus for synchronous access */
--
2.7.4
^ permalink raw reply related
* RE: [PATCH v2 0/2] kdump: simplify code
From: David Laight @ 2021-12-13 8:30 UTC (permalink / raw)
To: 'Matthew Wilcox'
Cc: linux-ia64@vger.kernel.org, Baoquan He, linux-sh@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org, x86@kernel.org,
kexec@lists.infradead.org, linux-mips@vger.kernel.org,
linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, Xuefeng Li,
linux-fsdevel@vger.kernel.org, Andrew Morton, Dave Young,
'Tiezhu Yang', Vivek Goyal
In-Reply-To: <YbXhVxRJfjvKw++W@casper.infradead.org>
From: Matthew Wilcox
> Sent: 12 December 2021 11:48
>
> On Sat, Dec 11, 2021 at 05:53:46PM +0000, David Laight wrote:
> > From: Tiezhu Yang
> > > Sent: 11 December 2021 03:33
> > >
> > > v2:
> > > -- add copy_to_user_or_kernel() in lib/usercopy.c
> > > -- define userbuf as bool type
> >
> > Instead of having a flag to indicate whether the buffer is user or kernel,
> > would it be better to have two separate buffer pointers.
> > One for a user space buffer, the other for a kernel space buffer.
> > Exactly one of the buffers should always be NULL.
>
> No. You should be using an iov_iter instead. See
> https://lore.kernel.org/all/Ya4bdB0UBJCZhUSo@casper.infradead.org/
> for a start on this.
iov_iter gets horribly expensive...
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
^ permalink raw reply
* Re: ppc64le STRICT_MODULE_RWX and livepatch apply_relocate_add() crashes
From: Christophe Leroy @ 2021-12-13 7:42 UTC (permalink / raw)
To: Joe Lawrence, Russell Currey, live-patching@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org
Cc: Peter Zijlstra, Jordan Niethe, Jessica Yu, Josh Poimboeuf
In-Reply-To: <f8a96ac1-fda3-92da-cf27-0992a43a2f3f@redhat.com>
Le 01/11/2021 à 14:48, Joe Lawrence a écrit :
> On 11/1/21 5:20 AM, Russell Currey wrote:
>> I'm looking into this now, will update when there's progress. I
>> personally wasn't aware but Jordan flagged this as an issue back in
>> August [0]. Are the selftests in the klp-convert tree sufficient for
>> testing? I'm not especially familiar with livepatching & haven't used
>> the userspace tools.
>>
>
> Hi Russell, thanks for taking a look.
>
> Testing with that klp-convert tree is probably the quickest and easiest
> way to verify the late relocations.
>
> I'm happy to setup and test additional tools (ie, kpatch-build) with any
> potential changes as I know they take longer to config and run.
>
Hello Joe,
I'm implementing LIVEPATCH on PPC32 and I wanted to test with
STRICT_MODULE_RWX enabled so I took your branch as suggested, but I'm
getting the following errors on build. What shall I do ?
CALL scripts/checksyscalls.sh
CALL scripts/atomic/check-atomics.sh
CHK include/generated/compile.h
KLP lib/livepatch/test_klp_convert1.ko
klp-convert: section .rela.klp.module_relocs.test_klp_convert_mod length
beyond nr_entries
klp-convert: Unable to load user-provided sympos
make[2]: *** [scripts/Makefile.modfinal:79:
lib/livepatch/test_klp_convert1.ko] Error 255
KLP lib/livepatch/test_klp_convert2.ko
klp-convert: section .rela.klp.module_relocs.test_klp_convert_mod length
beyond nr_entries
klp-convert: Unable to load user-provided sympos
make[2]: *** [scripts/Makefile.modfinal:79:
lib/livepatch/test_klp_convert2.ko] Error 255
KLP lib/livepatch/test_klp_convert_sections.ko
klp-convert: section .rela.klp.module_relocs.test_klp_convert_mod length
beyond nr_entries
klp-convert: Unable to load user-provided sympos
make[2]: *** [scripts/Makefile.modfinal:79:
lib/livepatch/test_klp_convert_sections.ko] Error 255
make[2]: Target '__modfinal' not remade because of errors.
make[1]: *** [scripts/Makefile.modpost:145: __modpost] Error 2
make: *** [Makefile:1770: modules] Error 2
Christophe
^ permalink raw reply
* Re: [PATCH v2 2/2] powerpc/module_64: Use patch_memory() to apply relocations to loaded modules
From: Russell Currey @ 2021-12-13 6:07 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org
Cc: jniethe5@gmail.com, naveen.n.rao@linux.vnet.ibm.com,
joe.lawrence@redhat.com, joel@jms.id.au
In-Reply-To: <12d81cfc-1c76-705f-af83-1b424eff64a0@csgroup.eu>
On Sun, 2021-12-12 at 10:41 +0000, Christophe Leroy wrote:
>
>
> Le 12/12/2021 à 02:03, Russell Currey a écrit :
> > Livepatching a loaded module involves applying relocations through
> > apply_relocate_add(), which attempts to write to read-only memory
> > when
> > CONFIG_STRICT_MODULE_RWX=y. Work around this by performing these
> > writes through the text poke area by using patch_memory().
> >
> > Similar to x86 and s390 implementations, apply_relocate_add() now
> > chooses to use patch_memory() or memcpy() depending on if the
> > module
> > is loaded or not. Without STRICT_KERNEL_RWX, patch_memory() is
> > just
> > memcpy(), so there should be no performance impact.
> >
> > While many relocation types may not be applied in a livepatch
> > context, comprehensively handling them prevents any issues in
> > future,
> > with no performance penalty as the text poke area is only used when
> > necessary.
> >
> > create_stub() and create_ftrace_stub() are modified to first write
> > to the stack so that the ppc64_stub_entry struct only takes one
> > write() to modify, saving several map/unmap/flush operations
> > when use of patch_memory() is necessary.
> >
> > This patch also contains some trivial whitespace fixes.
> >
> > Fixes: c35717c71e98 ("powerpc: Set ARCH_HAS_STRICT_MODULE_RWX")
> > Reported-by: Joe Lawrence <joe.lawrence@redhat.com>
> > Signed-off-by: Russell Currey <ruscur@russell.cc>
> > ---
> > v2: No changes.
> >
> > Some discussion here:https://github.com/linuxppc/issues/issues/375
> > for-stable version using patch_instruction():
> > https://lore.kernel.org/linuxppc-dev/20211123081520.18843-1-ruscur@russell.cc/
> >
> > arch/powerpc/kernel/module_64.c | 157 +++++++++++++++++++++------
> > -----
> > 1 file changed, 104 insertions(+), 53 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/module_64.c
> > b/arch/powerpc/kernel/module_64.c
> > index 6baa676e7cb6..2a146750fa6f 100644
> > --- a/arch/powerpc/kernel/module_64.c
> > +++ b/arch/powerpc/kernel/module_64.c
> > @@ -350,11 +350,11 @@ static u32 stub_insns[] = {
> > */
> > static inline int create_ftrace_stub(struct ppc64_stub_entry
> > *entry,
> > unsigned long addr,
> > - struct module *me)
> > + struct module *me,
> > + void *(*write)(void *,
> > const void *, size_t))
>
> I really dislike this write() parameter to the function.
>
> I think it would be better to define a static sub-function that takes
> write()'s parameters plus the 'struct module *me' and have it call
> either patch_memory() or memcpy() based on me->state.
I don't like it much either, I was just going off prior art from x86
and s390. I like your idea better, and that function could just be
memcpy() if !CONFIG_STRICT_MODULE_RWX, removing the need to check the
state in that case.
>
> > {
> > long reladdr;
> > -
> > - memcpy(entry->jump, stub_insns, sizeof(stub_insns));
> > + struct ppc64_stub_entry tmp_entry;
> >
> > /* Stub uses address relative to kernel toc (from the paca)
> > */
> > reladdr = addr - kernel_toc_addr();
> > @@ -364,12 +364,20 @@ static inline int create_ftrace_stub(struct
> > ppc64_stub_entry *entry,
> > return 0;
> > }
> >
> > - entry->jump[1] |= PPC_HA(reladdr);
> > - entry->jump[2] |= PPC_LO(reladdr);
> > + /*
> > + * In case @entry is write-protected, make our changes on
> > the stack
> > + * so we can update the whole struct in one write().
> > + */
> > + memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
>
> That copy seems unnecessary, entry is a struct with three fields and
> you
> are setting all three field below.
Oops, you're right.
> >
> > + memcpy(&tmp_entry.jump, stub_insns, sizeof(stub_insns));
> > + tmp_entry.jump[1] |= PPC_HA(reladdr);
> > + tmp_entry.jump[2] |= PPC_LO(reladdr);
> > /* Eventhough we don't use funcdata in the stub, it's
> > needed elsewhere. */
> > - entry->funcdata = func_desc(addr);
> > - entry->magic = STUB_MAGIC;
> > + tmp_entry.funcdata = func_desc(addr);
> > + tmp_entry.magic = STUB_MAGIC;
> > +
> > + write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
> >
> > return 1;
> > }
> > @@ -392,7 +400,8 @@ static bool is_mprofile_ftrace_call(const char
> > *name)
> > #else
> > static inline int create_ftrace_stub(struct ppc64_stub_entry
> > *entry,
> > unsigned long addr,
> > - struct module *me)
> > + struct module *me,
> > + void *(*write)(void *,
> > const void *, size_t))
> > {
> > return 0;
> > }
> > @@ -419,14 +428,14 @@ static inline int create_stub(const
> > Elf64_Shdr *sechdrs,
> > struct ppc64_stub_entry *entry,
> > unsigned long addr,
> > struct module *me,
> > - const char *name)
> > + const char *name,
> > + void *(*write)(void *, const void *,
> > size_t))
> > {
> > long reladdr;
> > + struct ppc64_stub_entry tmp_entry;
> >
> > if (is_mprofile_ftrace_call(name))
> > - return create_ftrace_stub(entry, addr, me);
> > -
> > - memcpy(entry->jump, ppc64_stub_insns,
> > sizeof(ppc64_stub_insns));
> > + return create_ftrace_stub(entry, addr, me, write);
> >
> > /* Stub uses address relative to r2. */
> > reladdr = (unsigned long)entry - my_r2(sechdrs, me);
> > @@ -437,10 +446,19 @@ static inline int create_stub(const
> > Elf64_Shdr *sechdrs,
> > }
> > pr_debug("Stub %p get data from reladdr %li\n", entry,
> > reladdr);
> >
> > - entry->jump[0] |= PPC_HA(reladdr);
> > - entry->jump[1] |= PPC_LO(reladdr);
> > - entry->funcdata = func_desc(addr);
> > - entry->magic = STUB_MAGIC;
> > + /*
> > + * In case @entry is write-protected, make our changes on
> > the stack
> > + * so we can update the whole struct in one write().
> > + */
> > + memcpy(&tmp_entry, entry, sizeof(struct ppc64_stub_entry));
> > +
> > + memcpy(&tmp_entry.jump, ppc64_stub_insns,
> > sizeof(ppc64_stub_insns));
> > + tmp_entry.jump[0] |= PPC_HA(reladdr);
> > + tmp_entry.jump[1] |= PPC_LO(reladdr);
> > + tmp_entry.funcdata = func_desc(addr);
> > + tmp_entry.magic = STUB_MAGIC;
> > +
> > + write(entry, &tmp_entry, sizeof(struct ppc64_stub_entry));
> >
> > return 1;
> > }
> > @@ -450,7 +468,8 @@ static inline int create_stub(const Elf64_Shdr
> > *sechdrs,
> > static unsigned long stub_for_addr(const Elf64_Shdr *sechdrs,
> > unsigned long addr,
> > struct module *me,
> > - const char *name)
> > + const char *name,
> > + void *(*write)(void *, const
> > void *, size_t))
> > {
> > struct ppc64_stub_entry *stubs;
> > unsigned int i, num_stubs;
> > @@ -467,7 +486,7 @@ static unsigned long stub_for_addr(const
> > Elf64_Shdr *sechdrs,
> > return (unsigned long)&stubs[i];
> > }
> >
> > - if (!create_stub(sechdrs, &stubs[i], addr, me, name))
> > + if (!create_stub(sechdrs, &stubs[i], addr, me, name,
> > write))
> > return 0;
> >
> > return (unsigned long)&stubs[i];
> > @@ -496,15 +515,20 @@ static int restore_r2(const char *name, u32
> > *instruction, struct module *me)
> > return 0;
> > }
> > /* ld r2,R2_STACK_OFFSET(r1) */
> > - *instruction = PPC_INST_LD_TOC;
> > + if (me->state == MODULE_STATE_UNFORMED)
> > + *instruction = PPC_INST_LD_TOC;
> > + else
> > + patch_instruction(instruction,
> > ppc_inst(PPC_INST_LD_TOC));
> > +
>
> Would be better if that hunk was following the same approach as other
> places.
It's not great that it's different, but I do like using
patch_instruction() for instructions.
>
> > return 1;
> > }
> >
> > -int apply_relocate_add(Elf64_Shdr *sechdrs,
> > - const char *strtab,
> > - unsigned int symindex,
> > - unsigned int relsec,
> > - struct module *me)
> > +static int __apply_relocate_add(Elf64_Shdr *sechdrs,
> > + const char *strtab,
> > + unsigned int symindex,
> > + unsigned int relsec,
> > + struct module *me,
> > + void *(*write)(void *dest, const
> > void *src, size_t len))
> > {
> > unsigned int i;
> > Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
> > @@ -544,16 +568,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > switch (ELF64_R_TYPE(rela[i].r_info)) {
> > case R_PPC64_ADDR32:
> > /* Simply set it */
> > - *(u32 *)location = value;
> > + write(location, &value, 4);
> > break;
> >
> > case R_PPC64_ADDR64:
> > /* Simply set it */
> > - *(unsigned long *)location = value;
> > + write(location, &value, 8);
> > break;
> >
> > case R_PPC64_TOC:
> > - *(unsigned long *)location = my_r2(sechdrs,
> > me);
> > + value = my_r2(sechdrs, me);
> > + write(location, &value, 8);
> > break;
> >
> > case R_PPC64_TOC16:
> > @@ -564,17 +589,17 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > me->name, value);
> > return -ENOEXEC;
> > }
> > - *((uint16_t *) location)
> > - = (*((uint16_t *) location) &
> > ~0xffff)
> > + value = (*((uint16_t *) location) &
> > ~0xffff)
> > | (value & 0xffff);
> > + write(location, &value, 2);
> > break;
> >
> > case R_PPC64_TOC16_LO:
> > /* Subtract TOC pointer */
> > value -= my_r2(sechdrs, me);
> > - *((uint16_t *) location)
> > - = (*((uint16_t *) location) &
> > ~0xffff)
> > + value = (*((uint16_t *) location) &
> > ~0xffff)
> > | (value & 0xffff);
> > + write(location, &value, 2);
> > break;
> >
> > case R_PPC64_TOC16_DS:
> > @@ -585,9 +610,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > me->name, value);
> > return -ENOEXEC;
> > }
> > - *((uint16_t *) location)
> > - = (*((uint16_t *) location) &
> > ~0xfffc)
> > + value = (*((uint16_t *) location) &
> > ~0xfffc)
> > | (value & 0xfffc);
> > + write(location, &value, 2);
> > break;
> >
> > case R_PPC64_TOC16_LO_DS:
> > @@ -598,18 +623,18 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > me->name, value);
> > return -ENOEXEC;
> > }
> > - *((uint16_t *) location)
> > - = (*((uint16_t *) location) &
> > ~0xfffc)
> > + value = (*((uint16_t *) location) &
> > ~0xfffc)
> > | (value & 0xfffc);
> > + write(location, &value, 2);
> > break;
> >
> > case R_PPC64_TOC16_HA:
> > /* Subtract TOC pointer */
> > value -= my_r2(sechdrs, me);
> > value = ((value + 0x8000) >> 16);
> > - *((uint16_t *) location)
> > - = (*((uint16_t *) location) &
> > ~0xffff)
> > + value = (*((uint16_t *) location) &
> > ~0xffff)
> > | (value & 0xffff);
> > + write(location, &value, 2);
> > break;
> >
> > case R_PPC_REL24:
> > @@ -618,14 +643,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > sym->st_shndx == SHN_LIVEPATCH) {
> > /* External: go via stub */
> > value = stub_for_addr(sechdrs,
> > value, me,
> > - strtab + sym-
> > >st_name);
> > + strtab + sym-
> > >st_name, write);
> > if (!value)
> > return -ENOENT;
> > if (!restore_r2(strtab + sym-
> > >st_name,
> > (u32
> > *)location + 1, me))
> > return -ENOEXEC;
> > - } else
> > + } else {
> > value += local_entry_offset(sym);
> > + }
> >
> > /* Convert value to relative */
> > value -= (unsigned long)location;
> > @@ -636,14 +662,15 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > }
> >
> > /* Only replace bits 2 through 26 */
> > - *(uint32_t *)location
> > - = (*(uint32_t *)location &
> > ~0x03fffffc)
> > + value = (*(uint32_t *)location &
> > ~0x03fffffc)
> > | (value & 0x03fffffc);
> > + write(location, &value, 4);
> > break;
> >
> > case R_PPC64_REL64:
> > /* 64 bits relative (used by features
> > fixups) */
> > - *location = value - (unsigned
> > long)location;
> > + value -= (unsigned long)location;
> > + write(location, &value, 8);
> > break;
> >
> > case R_PPC64_REL32:
> > @@ -655,7 +682,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > me->name, (long int)value);
> > return -ENOEXEC;
> > }
> > - *(u32 *)location = value;
> > + write(location, &value, 4);
> > break;
> >
> > case R_PPC64_TOCSAVE:
> > @@ -676,7 +703,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > break;
> > /*
> > * Check for the large code model prolog
> > sequence:
> > - * ld r2, ...(r12)
> > + * ld r2, ...(r12)
> > * add r2, r2, r12
> > */
> > if ((((uint32_t *)location)[0] & ~0xfffc)
> > != PPC_RAW_LD(_R2, _R12, 0))
> > @@ -688,25 +715,27 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > * addis r2, r12, (.TOC.-func)@ha
> > * addi r2, r2, (.TOC.-func)@l
> > */
> > - ((uint32_t *)location)[0] =
> > PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value));
> > - ((uint32_t *)location)[1] =
> > PPC_RAW_ADDI(_R2, _R2, PPC_LO(value));
> > + patch_instruction(&((uint32_t
> > *)location)[0],
> > +
> > ppc_inst(PPC_RAW_ADDIS(_R2, _R12, PPC_HA(value))));
> > + patch_instruction(&((uint32_t
> > *)location)[1],
> > +
> > ppc_inst(PPC_RAW_ADDI(_R2, _R2, PPC_LO(value))));
>
> Shouldn't you do like restore_r2() ?
Yeah, I probably did it this way to reduce code size in this already
very ugly section. I can solve both problems with a static helper.
>
> > break;
> >
> > case R_PPC64_REL16_HA:
> > /* Subtract location pointer */
> > value -= (unsigned long)location;
> > value = ((value + 0x8000) >> 16);
> > - *((uint16_t *) location)
> > - = (*((uint16_t *) location) &
> > ~0xffff)
> > + value = (*((uint16_t *) location) &
> > ~0xffff)
> > | (value & 0xffff);
> > + write(location, &value, 2);
> > break;
> >
> > case R_PPC64_REL16_LO:
> > /* Subtract location pointer */
> > value -= (unsigned long)location;
> > - *((uint16_t *) location)
> > - = (*((uint16_t *) location) &
> > ~0xffff)
> > + value = (*((uint16_t *) location) &
> > ~0xffff)
> > | (value & 0xffff);
> > + write(location, &value, 2);
> > break;
> >
> > default:
> > @@ -720,6 +749,20 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> > return 0;
> > }
> >
> > +int apply_relocate_add(Elf64_Shdr *sechdrs,
> > + const char *strtab,
> > + unsigned int symindex,
> > + unsigned int relsec,
> > + struct module *me)
> > +{
> > + void *(*write)(void *, const void *, size_t) = memcpy;
> > + bool early = me->state == MODULE_STATE_UNFORMED;
> > +
> > + if (!early)
> > + write = patch_memory;
> > +
> > + return __apply_relocate_add(sechdrs, strtab, symindex,
> > relsec, me, write);
> > +}
>
> I really dislike this stuff with the write() function as a parameter.
> We
> have 'me', it should be enough for the callee to know what to do, see
> my
> first comment at the top of this email.
I'll rework it. No love lost, I had to look up function pointer syntax
for this.
Thanks for the feedback.
- Russell
>
> > #ifdef CONFIG_DYNAMIC_FTRACE
> > int module_trampoline_target(struct module *mod, unsigned long
> > addr,
> > unsigned long *target)
> > @@ -749,7 +792,7 @@ int module_trampoline_target(struct module
> > *mod, unsigned long addr,
> > if (copy_from_kernel_nofault(&funcdata, &stub->funcdata,
> > sizeof(funcdata))) {
> > pr_err("%s: fault reading funcdata for stub %lx for
> > %s\n", __func__, addr, mod->name);
> > - return -EFAULT;
> > + return -EFAULT;
> > }
> >
> > *target = stub_func_addr(funcdata);
> > @@ -759,15 +802,23 @@ int module_trampoline_target(struct module
> > *mod, unsigned long addr,
> >
> > int module_finalize_ftrace(struct module *mod, const Elf_Shdr
> > *sechdrs)
> > {
> > + void *(*write)(void *, const void *, size_t) = memcpy;
> > + bool early = mod->state == MODULE_STATE_UNFORMED;
> > +
> > + if (!early)
> > + write = patch_memory;
> > +
> > mod->arch.tramp = stub_for_addr(sechdrs,
> > (unsigned
> > long)ftrace_caller,
> > mod,
> > - "ftrace_caller");
> > + "ftrace_caller",
> > + write);
> > #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> > mod->arch.tramp_regs = stub_for_addr(sechdrs,
> > (unsigned
> > long)ftrace_regs_caller,
> > mod,
> > - "ftrace_regs_caller");
> > + "ftrace_regs_caller",
> > + write);
> > if (!mod->arch.tramp_regs)
> > return -ENOENT;
> > #endif
> >
>
> Christophe
^ permalink raw reply
* Re: [PATCH v2 1/2] powerpc/code-patching: add patch_memory() for writing RO text
From: Russell Currey @ 2021-12-13 5:51 UTC (permalink / raw)
To: Christophe Leroy, linuxppc-dev@lists.ozlabs.org
Cc: jniethe5@gmail.com, naveen.n.rao@linux.vnet.ibm.com,
joe.lawrence@redhat.com, joel@jms.id.au
In-Reply-To: <9a3a5e5d-ee90-4290-abc9-31be2716e6a1@csgroup.eu>
On Sun, 2021-12-12 at 09:08 +0000, Christophe Leroy wrote:
> Le 12/12/2021 à 02:03, Russell Currey a écrit :
> > +static int do_patch_memory(void *dest, const void *src, size_t
> > size, unsigned long poke_addr)
> > +{
> > + unsigned long patch_addr = poke_addr +
> > offset_in_page(dest);
> > +
> > + if (map_patch_area(dest, poke_addr)) {
> > + pr_warn("failed to map %lx\n", poke_addr);
>
> It isn't worth a warning here. If that happens before slab is
> available,
> it will panic in early_alloc_pgtable().
>
> If it happens after, you will already get a pile of messages dumping
> the
> memory state etc ...
>
> During the last few years, pr_ messages have been removed from most
> places where ENOMEM is returned.
That's good to know, thanks.
>
> > + return -1;
> > + }
>
> I have a series reworking error handling at
> https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=274823&state=*
>
> Especially this one handles map_patch_area() :
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/85259d894069e47f915ea580b169e1adbeec7a61.1638446239.git.christophe.leroy@csgroup.eu/
>
> Would be good if you could rebase your series on top of it.
>
I've rebased on top of your series (patchwork 274258 & 274823).
> > +
> > + memcpy((u8 *)patch_addr, src, size);
>
> Shouldn't we use copy_to_kernel_nofault(), so that we survive from a
> fault just like patch_instruction() ?
Yes we should.
> > +
> > + flush_icache_range(patch_addr, size);
> > +
> > + if (unmap_patch_area(poke_addr)) {
> > + pr_warn("failed to unmap %lx\n", poke_addr);
> > + return -1;
> > + }
>
> I have changed unmap_page_area() to a void in
> https://patchwork.ozlabs.org/project/linuxppc-dev/patch/299804b117fae35c786c827536c91f25352e279b.1638446239.git.christophe.leroy@csgroup.eu/
>
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * patch_memory - write data using the text poke area
> > + *
> > + * @dest: destination address
> > + * @src: source address
> > + * @size: size in bytes
> > + *
> > + * like memcpy(), but using the text poke area. No atomicity
> > guarantees.
> > + * Do not use for instructions, use patch_instruction() instead.
> > + * Handles crossing page boundaries, though you shouldn't need to.
> > + *
> > + * Return value:
> > + * @dest
> > + **/
> > +void *patch_memory(void *dest, const void *src, size_t size)
> > +{
> > + size_t bytes_written, write_size;
> > + unsigned long text_poke_addr;
> > + unsigned long flags;
> > +
> > + // If the poke area isn't set up, it's early boot and we
> > can just memcpy.
> > + if (!this_cpu_read(text_poke_area))
> > + return memcpy(dest, src, size);
> > +
> > + local_irq_save(flags);
>
> Do we want to do such potentially big copies with interrupts disabled
> ?
Probably not. This should never actually get used for big copies - the
problem it was written to solve never copies more than 40 bytes, and is
very unlikely to ever cross a page boundary.
I could disable and re-enable interrupts per-page (per call of
do_patch_memory()) so there's a preemption window on longer operations.
>
> > + text_poke_addr = (unsigned
> > long)__this_cpu_read(text_poke_area)->addr;
> > +
> > + for (bytes_written = 0;
> > + bytes_written < size;
> > + bytes_written += write_size) {
>
> I recommend you to read
> https://www.kernel.org/doc/html/latest/process/coding-style.html?highlight=coding%20style#naming
>
> As explained there, local variable names should be short. Using long
> names is non-productive.
>
> You could just call it "written", it would allow you to keep the
> for()
> on a single line, that would be a lot more readable.
I am aware of the coding style, my brain somehow didn't consider
"written" as a better option, which is quite silly.
> > + // Write as much as possible without crossing a
> > page boundary.
> > + write_size = min_t(size_t,
> > + size - bytes_written,
> > + PAGE_SIZE - offset_in_page(dest
> > + bytes_written));
>
> Reduce the size of you variable names and keep it on a single line.
> > +
> > + if (do_patch_memory(dest + bytes_written,
> > + src + bytes_written,
> > + write_size,
> > + text_poke_addr))
>
> Same, keep a single line as much as possible.
>
> > + break;
> > + }
> > +
> > + local_irq_restore(flags);
> > +
> > + return dest;
>
> Maybe it would be better to return ERR_PTR() of the error returned by
> do_page_memory().
That is indeed much better.
Thanks for the feedback.
- Russell
>
> > +}
> > #else /* !CONFIG_STRICT_KERNEL_RWX */
> >
> > static int do_patch_instruction(u32 *addr, struct ppc_inst instr)
> > @@ -185,6 +254,11 @@ static int do_patch_instruction(u32 *addr,
> > struct ppc_inst instr)
> > return raw_patch_instruction(addr, instr);
> > }
> >
> > +void *patch_memory(void *dest, const void *src, size_t size)
> > +{
> > + return memcpy(dest, src, size);
> > +}
> > +
> > #endif /* CONFIG_STRICT_KERNEL_RWX */
> >
> > int patch_instruction(u32 *addr, struct ppc_inst instr)
> >
>
> Christophe
^ permalink raw reply
* Re: [patch V3 34/35] soc: ti: ti_sci_inta_msi: Get rid of ti_sci_inta_msi_get_virq()
From: Vinod Koul @ 2021-12-13 5:15 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Peter Ujfalusi, Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya,
Bjorn Helgaas, linux-arm-kernel, Jason Gunthorpe, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, iommu, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221815.269468319@linutronix.de>
On 10-12-21, 23:19, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Just use the core function msi_get_virq().
Acked-By: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [patch V3 35/35] dmaengine: qcom_hidma: Cleanup MSI handling
From: Vinod Koul @ 2021-12-13 5:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, Peter Ujfalusi,
Bjorn Helgaas, linux-arm-kernel, Jason Gunthorpe, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, iommu, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221815.329792721@linutronix.de>
On 10-12-21, 23:19, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> There is no reason to walk the MSI descriptors to retrieve the interrupt
> number for a device. Use msi_get_virq() instead.
Acked-By: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [patch V3 29/35] dmaengine: mv_xor_v2: Get rid of msi_desc abuse
From: Vinod Koul @ 2021-12-13 5:14 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Nishanth Menon, Mark Rutland, Stuart Yoder, Will Deacon,
Ashok Raj, Joerg Roedel, Jassi Brar, Sinan Kaya, Peter Ujfalusi,
Bjorn Helgaas, linux-arm-kernel, Jason Gunthorpe, linux-pci,
xen-devel, Kevin Tian, Arnd Bergmann, Robin Murphy,
Alex Williamson, Cedric Le Goater, Santosh Shilimkar,
Bjorn Helgaas, Megha Dey, Laurentiu Tudor, Juergen Gross,
Tero Kristo, Greg Kroah-Hartman, LKML, iommu, Marc Zygnier,
dmaengine, linuxppc-dev
In-Reply-To: <20211210221814.970099984@linutronix.de>
On 10-12-21, 23:19, Thomas Gleixner wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
>
> Storing a pointer to the MSI descriptor just to keep track of the Linux
> interrupt number is daft. Use msi_get_virq() instead.
Acked-By: Vinod Koul <vkoul@kernel.org>
--
~Vinod
^ permalink raw reply
* Re: [PATCH] powerpc: Add set_memory_{p/np}() and remove set_memory_attr()
From: Russell Currey @ 2021-12-13 4:36 UTC (permalink / raw)
To: Christophe Leroy, Benjamin Herrenschmidt, Paul Mackerras,
Michael Ellerman
Cc: Maxime Bizon, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org, stable@vger.kernel.org
In-Reply-To: <715cc0c2f801ef3b39b91233be44d328a91c30bc.1639123757.git.christophe.leroy@csgroup.eu>
On Fri, 2021-12-10 at 08:09 +0000, Christophe Leroy wrote:
> set_memory_attr() was implemented by commit 4d1755b6a762
> ("powerpc/mm:
> implement set_memory_attr()") because the set_memory_xx() couldn't
> be used at that time to modify memory "on the fly" as explained it
> the commit.
>
> But set_memory_attr() uses set_pte_at() which leads to warnings when
> CONFIG_DEBUG_VM is selected, because set_pte_at() is unexpected for
> updating existing page table entries.
>
> The check could be bypassed by using __set_pte_at() instead,
> as it was the case before commit c988cfd38e48 ("powerpc/32:
> use set_memory_attr()") but since commit 9f7853d7609d ("powerpc/mm:
> Fix set_memory_*() against concurrent accesses") it is now possible
> to use set_memory_xx() functions to update page table entries
> "on the fly" because the update is now atomic.
>
> For DEBUG_PAGEALLOC we need to clear and set back _PAGE_PRESENT.
> Add set_memory_np() and set_memory_p() for that.
>
> Replace all uses of set_memory_attr() by the relevant set_memory_xx()
> and remove set_memory_attr().
>
> Reported-by: Maxime Bizon <mbizon@freebox.fr>
> Fixes: c988cfd38e48 ("powerpc/32: use set_memory_attr()")
> Cc: stable@vger.kernel.org
> Depends-on: 9f7853d7609d ("powerpc/mm: Fix set_memory_*() against
> concurrent accesses")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Russell Currey <ruscur@russell.cc>
One comment below:
> diff --git a/arch/powerpc/include/asm/set_memory.h
> b/arch/powerpc/include/asm/set_memory.h
> index b040094f7920..061f1766a8a4 100644
> --- a/arch/powerpc/include/asm/set_memory.h
> +++ b/arch/powerpc/include/asm/set_memory.h
> @@ -6,6 +6,8 @@
> #define SET_MEMORY_RW 1
> #define SET_MEMORY_NX 2
> #define SET_MEMORY_X 3
> +#define SET_MEMORY_NP 4
> +#define SET_MEMORY_P 5
It might be nice to have a comment somewhere in set_memory.h explaining
that {p/np} = present/not present. RO/RW/NX/X are commonly used, "p"
as shorthand for "present" is less obvious. x86's set_memory.h has a
nice comment covering everything as an example.
> int change_memory_attr(unsigned long addr, int numpages, long
> action);
>
> @@ -29,6 +31,14 @@ static inline int set_memory_x(unsigned long addr,
> int numpages)
> return change_memory_attr(addr, numpages, SET_MEMORY_X);
> }
>
> -int set_memory_attr(unsigned long addr, int numpages, pgprot_t
> prot);
> +static inline int set_memory_np(unsigned long addr, int numpages)
> +{
> + return change_memory_attr(addr, numpages, SET_MEMORY_NP);
> +}
> +
> +static inline int set_memory_p(unsigned long addr, int numpages)
> +{
> + return change_memory_attr(addr, numpages, SET_MEMORY_P);
> +}
>
> #endif
^ permalink raw reply
* Re: [PATCH] powerpc: mpc52xx_gpt: fix a potential memory leak
From: xkernel.wang @ 2021-12-13 2:26 UTC (permalink / raw)
To: mpe, agust; +Cc: paulus, linuxppc-dev, linux-kernel
Michael Ellerman <mpe@ellerman.id.au> wrote:
> > When some internal memory errors happend in of_iomap(), we should free
> > gpt to prevent memory leak.
>
> But it's allocated with devm_kzalloc(), so the devres core is meant to
> free it for us isn't it?
Yes, maybe you are right. I did that as I mentioned when gpt-regs is NULL, it
will return -ENOMEM, which is the same when gpt is NULL. So I suppose to
free it in time is better:
> gpt = devm_kzalloc(&ofdev->dev, sizeof *gpt, GFP_KERNEL);
> if (!gpt)
> return -ENOMEM;
cheers
^ permalink raw reply
* Re: [PATCH] powerpc: mpc52xx_gpt: fix a potential memory leak
From: Michael Ellerman @ 2021-12-13 2:07 UTC (permalink / raw)
To: xkernel, agust; +Cc: xkernel, paulus, linuxppc-dev, linux-kernel
In-Reply-To: <tencent_765F05E486793F9790A388C2289C5429F705@qq.com>
xkernel <xkernel.wang@foxmail.com> writes:
> When some internal memory errors happend in of_iomap(), we should free
> gpt to prevent memory leak.
But it's allocated with devm_kzalloc(), so the devres core is meant to
free it for us isn't it?
cheers
> diff --git a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> index f862b48..c506cfd 100644
> --- a/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> +++ b/arch/powerpc/platforms/52xx/mpc52xx_gpt.c
> @@ -722,8 +722,10 @@ static int mpc52xx_gpt_probe(struct platform_device *ofdev)
> gpt->dev = &ofdev->dev;
> gpt->ipb_freq = mpc5xxx_get_bus_frequency(ofdev->dev.of_node);
> gpt->regs = of_iomap(ofdev->dev.of_node, 0);
> - if (!gpt->regs)
> + if (!gpt->regs) {
> + devm_kfree(&ofdev->dev, gpt);
> return -ENOMEM;
> + }
>
> dev_set_drvdata(&ofdev->dev, gpt);
>
> --
^ permalink raw reply
* Re: [PATCH v2 2/6] powerpc/kexec_file: Add KEXEC_SIG support.
From: Nayna @ 2021-12-13 0:46 UTC (permalink / raw)
To: Michal Suchanek, keyrings
Cc: Mimi Zohar, David Howells, Paul Mackerras, Alexander Gordeev,
linux-s390, Herbert Xu, Baoquan He, Christian Borntraeger,
James Morris, Lakshmi Ramasubramanian, Christian Borntraeger,
Serge E. Hallyn, Vasily Gorbik, Rob Herring, Heiko Carstens,
linux-crypto, Dmitry Kasatkin, Hari Bathini, Daniel Axtens,
Philipp Rudo, Frank van der Linden, kexec, linux-kernel,
Luis Chamberlain, Sven Schnelle, linux-security-module,
Jessica Yu, linux-integrity, linuxppc-dev, David S. Miller,
Thiago Jung Bauermann, buendgen
In-Reply-To: <8b30a3c6a4e845eb77f276298424811897efdebf.1637862358.git.msuchanek@suse.de>
On 11/25/21 13:02, Michal Suchanek wrote:
> Copy the code from s390x
>
> Signed-off-by: Michal Suchanek <msuchanek@suse.de>
> ---
> arch/powerpc/Kconfig | 11 +++++++++++
> arch/powerpc/kexec/elf_64.c | 36 ++++++++++++++++++++++++++++++++++++
> 2 files changed, 47 insertions(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index ac0c515552fd..ecc1227a77f1 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -561,6 +561,17 @@ config KEXEC_FILE
> config ARCH_HAS_KEXEC_PURGATORY
> def_bool KEXEC_FILE
>
> +config KEXEC_SIG
> + bool "Verify kernel signature during kexec_file_load() syscall"
> + depends on KEXEC_FILE && MODULE_SIG_FORMAT
> + help
> + This option makes kernel signature verification mandatory for
> + the kexec_file_load() syscall.
> +
Resending my last response as looks like it didn't go through mailing
list because of some wrong formatting. My apologies to those who are
receiving it twice.
Since powerpc also supports IMA_ARCH_POLICY for kernel image signature
verification, please include the following:
"An alternative implementation for the powerpc arch is IMA_ARCH_POLICY.
It verifies the appended kernel image signature and additionally
includes both the signed and unsigned file hashes in the IMA measurement
list, extends the IMA PCR in the TPM, and prevents blacklisted binary
kernel images from being kexec'd."
Thanks & Regards,
- Nayna
^ permalink raw reply
* [PATCH v5 21/21] reboot: Remove pm_power_off_prepare()
From: Dmitry Osipenko @ 2021-12-12 21:03 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Russell King, Catalin Marinas,
Will Deacon, Guo Ren, Geert Uytterhoeven, Greg Ungerer,
Joshua Thompson, Thomas Bogendoerfer, Sebastian Reichel,
Linus Walleij, Philipp Zabel, Greentime Hu, Vincent Chen,
James E.J. Bottomley, Helge Deller, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Yoshinori Sato, Rich Felker,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Boris Ostrovsky, Juergen Gross,
Stefano Stabellini, Rafael J. Wysocki, Len Brown,
Santosh Shilimkar, Krzysztof Kozlowski, Liam Girdwood, Mark Brown,
Pavel Machek, Lee Jones, Andrew Morton, Guenter Roeck,
Daniel Lezcano, Andy Shevchenko, Ulf Hansson, alankao,
K . C . Kuen-Chern Lin, Michał Mirosław
Cc: linux-ia64, linux-parisc, linux-sh, linux-pm, linux-kernel,
linux-csky, linux-mips, linux-acpi, linux-m68k, linux-tegra,
xen-devel, linux-riscv, linuxppc-dev
In-Reply-To: <20211212210309.9851-1-digetx@gmail.com>
All pm_power_off_prepare() users were converted to sys-off handler API.
Remove the obsolete callback.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
include/linux/pm.h | 1 -
kernel/reboot.c | 11 -----------
2 files changed, 12 deletions(-)
diff --git a/include/linux/pm.h b/include/linux/pm.h
index 1d8209c09686..d9bf1426f81e 100644
--- a/include/linux/pm.h
+++ b/include/linux/pm.h
@@ -20,7 +20,6 @@
* Callbacks for platform drivers to implement.
*/
extern void (*pm_power_off)(void);
-extern void (*pm_power_off_prepare)(void);
struct device; /* we have a circular dep with device.h */
#ifdef CONFIG_VT_CONSOLE_SLEEP
diff --git a/kernel/reboot.c b/kernel/reboot.c
index 3085873a876f..2f79d4f7cfaa 100644
--- a/kernel/reboot.c
+++ b/kernel/reboot.c
@@ -48,13 +48,6 @@ int reboot_cpu;
enum reboot_type reboot_type = BOOT_ACPI;
int reboot_force;
-/*
- * If set, this is used for preparing the system to power off.
- */
-
-void (*pm_power_off_prepare)(void);
-EXPORT_SYMBOL_GPL(pm_power_off_prepare);
-
/**
* emergency_restart - reboot the system
*
@@ -829,10 +822,6 @@ void do_kernel_power_off(void)
static void do_kernel_power_off_prepare(void)
{
- /* legacy pm_power_off_prepare() is unchained and has highest priority */
- if (pm_power_off_prepare)
- return pm_power_off_prepare();
-
blocking_notifier_call_chain(&power_off_handler_list, POWEROFF_PREPARE,
NULL);
}
--
2.33.1
^ permalink raw reply related
* [PATCH v5 20/21] regulator: pfuze100: Use devm_register_sys_off_handler()
From: Dmitry Osipenko @ 2021-12-12 21:03 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Russell King, Catalin Marinas,
Will Deacon, Guo Ren, Geert Uytterhoeven, Greg Ungerer,
Joshua Thompson, Thomas Bogendoerfer, Sebastian Reichel,
Linus Walleij, Philipp Zabel, Greentime Hu, Vincent Chen,
James E.J. Bottomley, Helge Deller, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Yoshinori Sato, Rich Felker,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Boris Ostrovsky, Juergen Gross,
Stefano Stabellini, Rafael J. Wysocki, Len Brown,
Santosh Shilimkar, Krzysztof Kozlowski, Liam Girdwood, Mark Brown,
Pavel Machek, Lee Jones, Andrew Morton, Guenter Roeck,
Daniel Lezcano, Andy Shevchenko, Ulf Hansson, alankao,
K . C . Kuen-Chern Lin, Michał Mirosław
Cc: linux-ia64, linux-parisc, linux-sh, linux-pm, linux-kernel,
linux-csky, linux-mips, linux-acpi, linux-m68k, linux-tegra,
xen-devel, linux-riscv, linuxppc-dev
In-Reply-To: <20211212210309.9851-1-digetx@gmail.com>
Use devm_register_sys_off_handler() that replaces global
pm_power_off_prepare variable and allows to register multiple
power-off handlers.
Acked-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/regulator/pfuze100-regulator.c | 38 ++++++++++----------------
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/drivers/regulator/pfuze100-regulator.c b/drivers/regulator/pfuze100-regulator.c
index d60d7d1b7fa2..2eca8d43a097 100644
--- a/drivers/regulator/pfuze100-regulator.c
+++ b/drivers/regulator/pfuze100-regulator.c
@@ -10,6 +10,7 @@
#include <linux/of_device.h>
#include <linux/regulator/of_regulator.h>
#include <linux/platform_device.h>
+#include <linux/reboot.h>
#include <linux/regulator/driver.h>
#include <linux/regulator/machine.h>
#include <linux/regulator/pfuze100.h>
@@ -76,6 +77,7 @@ struct pfuze_chip {
struct pfuze_regulator regulator_descs[PFUZE100_MAX_REGULATOR];
struct regulator_dev *regulators[PFUZE100_MAX_REGULATOR];
struct pfuze_regulator *pfuze_regulators;
+ struct sys_off_handler sys_off;
};
static const int pfuze100_swbst[] = {
@@ -569,10 +571,10 @@ static inline struct device_node *match_of_node(int index)
return pfuze_matches[index].of_node;
}
-static struct pfuze_chip *syspm_pfuze_chip;
-
-static void pfuze_power_off_prepare(void)
+static void pfuze_power_off_prepare(struct power_off_prep_data *data)
{
+ struct pfuze_chip *syspm_pfuze_chip = data->cb_data;
+
dev_info(syspm_pfuze_chip->dev, "Configure standby mode for power off");
/* Switch from default mode: APS/APS to APS/Off */
@@ -611,24 +613,23 @@ static void pfuze_power_off_prepare(void)
static int pfuze_power_off_prepare_init(struct pfuze_chip *pfuze_chip)
{
+ int err;
+
if (pfuze_chip->chip_id != PFUZE100) {
dev_warn(pfuze_chip->dev, "Requested pm_power_off_prepare handler for not supported chip\n");
return -ENODEV;
}
- if (pm_power_off_prepare) {
- dev_warn(pfuze_chip->dev, "pm_power_off_prepare is already registered.\n");
- return -EBUSY;
- }
+ pfuze_chip->sys_off.power_off_prepare_cb = pfuze_power_off_prepare;
+ pfuze_chip->sys_off.cb_data = pfuze_chip;
- if (syspm_pfuze_chip) {
- dev_warn(pfuze_chip->dev, "syspm_pfuze_chip is already set.\n");
- return -EBUSY;
+ err = devm_register_sys_off_handler(pfuze_chip->dev, &pfuze_chip->sys_off);
+ if (err) {
+ dev_err(pfuze_chip->dev,
+ "failed to register sys-off handler: %d\n", err);
+ return err;
}
- syspm_pfuze_chip = pfuze_chip;
- pm_power_off_prepare = pfuze_power_off_prepare;
-
return 0;
}
@@ -837,23 +838,12 @@ static int pfuze100_regulator_probe(struct i2c_client *client,
return 0;
}
-static int pfuze100_regulator_remove(struct i2c_client *client)
-{
- if (syspm_pfuze_chip) {
- syspm_pfuze_chip = NULL;
- pm_power_off_prepare = NULL;
- }
-
- return 0;
-}
-
static struct i2c_driver pfuze_driver = {
.driver = {
.name = "pfuze100-regulator",
.of_match_table = pfuze_dt_ids,
},
.probe = pfuze100_regulator_probe,
- .remove = pfuze100_regulator_remove,
};
module_i2c_driver(pfuze_driver);
--
2.33.1
^ permalink raw reply related
* [PATCH v5 19/21] ACPI: power: Switch to sys-off handler API
From: Dmitry Osipenko @ 2021-12-12 21:03 UTC (permalink / raw)
To: Thierry Reding, Jonathan Hunter, Russell King, Catalin Marinas,
Will Deacon, Guo Ren, Geert Uytterhoeven, Greg Ungerer,
Joshua Thompson, Thomas Bogendoerfer, Sebastian Reichel,
Linus Walleij, Philipp Zabel, Greentime Hu, Vincent Chen,
James E.J. Bottomley, Helge Deller, Michael Ellerman,
Benjamin Herrenschmidt, Paul Mackerras, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Yoshinori Sato, Rich Felker,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Boris Ostrovsky, Juergen Gross,
Stefano Stabellini, Rafael J. Wysocki, Len Brown,
Santosh Shilimkar, Krzysztof Kozlowski, Liam Girdwood, Mark Brown,
Pavel Machek, Lee Jones, Andrew Morton, Guenter Roeck,
Daniel Lezcano, Andy Shevchenko, Ulf Hansson, alankao,
K . C . Kuen-Chern Lin, Michał Mirosław
Cc: linux-ia64, linux-parisc, linux-sh, linux-pm, linux-kernel,
linux-csky, linux-mips, linux-acpi, linux-m68k, linux-tegra,
xen-devel, linux-riscv, linuxppc-dev
In-Reply-To: <20211212210309.9851-1-digetx@gmail.com>
Switch to sys-off API that replaces legacy pm_power_off callbacks.
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
drivers/acpi/sleep.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index eaa47753b758..2e613fddd614 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -47,19 +47,11 @@ static void acpi_sleep_tts_switch(u32 acpi_state)
}
}
-static int tts_notify_reboot(struct notifier_block *this,
- unsigned long code, void *x)
+static void tts_reboot_prepare(struct reboot_prep_data *data)
{
acpi_sleep_tts_switch(ACPI_STATE_S5);
- return NOTIFY_DONE;
}
-static struct notifier_block tts_notifier = {
- .notifier_call = tts_notify_reboot,
- .next = NULL,
- .priority = 0,
-};
-
static int acpi_sleep_prepare(u32 acpi_state)
{
#ifdef CONFIG_ACPI_SLEEP
@@ -1020,7 +1012,7 @@ static void acpi_sleep_hibernate_setup(void)
static inline void acpi_sleep_hibernate_setup(void) {}
#endif /* !CONFIG_HIBERNATION */
-static void acpi_power_off_prepare(void)
+static void acpi_power_off_prepare(struct power_off_prep_data *data)
{
/* Prepare to power off the system */
acpi_sleep_prepare(ACPI_STATE_S5);
@@ -1028,7 +1020,7 @@ static void acpi_power_off_prepare(void)
acpi_os_wait_events_complete();
}
-static void acpi_power_off(void)
+static void acpi_power_off(struct power_off_data *data)
{
/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
pr_debug("%s called\n", __func__);
@@ -1036,6 +1028,11 @@ static void acpi_power_off(void)
acpi_enter_sleep_state(ACPI_STATE_S5);
}
+static struct sys_off_handler acpi_sys_off_handler = {
+ .power_off_priority = POWEROFF_PRIO_FIRMWARE,
+ .reboot_prepare_cb = tts_reboot_prepare,
+};
+
int __init acpi_sleep_init(void)
{
char supported[ACPI_S_STATE_COUNT * 3 + 1];
@@ -1052,8 +1049,8 @@ int __init acpi_sleep_init(void)
if (acpi_sleep_state_supported(ACPI_STATE_S5)) {
sleep_states[ACPI_STATE_S5] = 1;
- pm_power_off_prepare = acpi_power_off_prepare;
- pm_power_off = acpi_power_off;
+ acpi_sys_off_handler.power_off_cb = acpi_power_off;
+ acpi_sys_off_handler.power_off_prepare_cb = acpi_power_off_prepare;
} else {
acpi_no_s5 = true;
}
@@ -1069,6 +1066,6 @@ int __init acpi_sleep_init(void)
* Register the tts_notifier to reboot notifier list so that the _TTS
* object can also be evaluated when the system enters S5.
*/
- register_reboot_notifier(&tts_notifier);
+ register_sys_off_handler(&acpi_sys_off_handler);
return 0;
}
--
2.33.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox