* [PATCH v6 6/7] powerpc/mm: implement set_memory_attr()
From: Russell Currey @ 2020-03-10 1:03 UTC (permalink / raw)
To: linuxppc-dev
Cc: ajd, kernel-hardening, npiggin, kbuild test robot, joel,
Russell Currey, dja
In-Reply-To: <20200310010338.21205-1-ruscur@russell.cc>
From: Christophe Leroy <christophe.leroy@c-s.fr>
In addition to the set_memory_xx() functions which allows to change
the memory attributes of not (yet) used memory regions, implement a
set_memory_attr() function to:
- set the final memory protection after init on currently used
kernel regions.
- enable/disable kernel memory regions in the scope of DEBUG_PAGEALLOC.
Unlike the set_memory_xx() which can act in three step as the regions
are unused, this function must modify 'on the fly' as the kernel is
executing from them. At the moment only PPC32 will use it and changing
page attributes on the fly is not an issue.
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
Reported-by: kbuild test robot <lkp@intel.com>
[ruscur: cast "data" to unsigned long instead of int]
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/include/asm/set_memory.h | 2 ++
arch/powerpc/mm/pageattr.c | 33 +++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
index 64011ea444b4..b040094f7920 100644
--- a/arch/powerpc/include/asm/set_memory.h
+++ b/arch/powerpc/include/asm/set_memory.h
@@ -29,4 +29,6 @@ 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);
+
#endif
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
index 748fa56d9db0..60139fedc6cc 100644
--- a/arch/powerpc/mm/pageattr.c
+++ b/arch/powerpc/mm/pageattr.c
@@ -77,3 +77,36 @@ int change_memory_attr(unsigned long addr, int numpages, long action)
return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action);
}
+
+/*
+ * Set the attributes of a page:
+ *
+ * This function is used by PPC32 at the end of init to set final kernel memory
+ * protection. It includes changing the maping of the page it is executing from
+ * and data pages it is using.
+ */
+static int set_page_attr(pte_t *ptep, unsigned long addr, void *data)
+{
+ pgprot_t prot = __pgprot((unsigned long)data);
+
+ spin_lock(&init_mm.page_table_lock);
+
+ set_pte_at(&init_mm, addr, ptep, pte_modify(*ptep, prot));
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+
+ spin_unlock(&init_mm.page_table_lock);
+
+ return 0;
+}
+
+int set_memory_attr(unsigned long addr, int numpages, pgprot_t prot)
+{
+ unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
+ unsigned long sz = numpages * PAGE_SIZE;
+
+ if (!numpages)
+ return 0;
+
+ return apply_to_page_range(&init_mm, start, sz, set_page_attr,
+ (void *)pgprot_val(prot));
+}
--
2.25.1
^ permalink raw reply related
* [PATCH v6 5/7] powerpc/configs: Enable STRICT_MODULE_RWX in skiroot_defconfig
From: Russell Currey @ 2020-03-10 1:03 UTC (permalink / raw)
To: linuxppc-dev
Cc: ajd, kernel-hardening, Joel Stanley, npiggin, joel,
Russell Currey, dja
In-Reply-To: <20200310010338.21205-1-ruscur@russell.cc>
skiroot_defconfig is the only powerpc defconfig with STRICT_KERNEL_RWX
enabled, and if you want memory protection for kernel text you'd want it
for modules too, so enable STRICT_MODULE_RWX there.
Acked-by: Joel Stanley <joel@joel.id.au>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/configs/skiroot_defconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/configs/skiroot_defconfig b/arch/powerpc/configs/skiroot_defconfig
index 1b6bdad36b13..66d20dbe67b7 100644
--- a/arch/powerpc/configs/skiroot_defconfig
+++ b/arch/powerpc/configs/skiroot_defconfig
@@ -51,6 +51,7 @@ CONFIG_CMDLINE="console=tty0 console=hvc0 ipr.fast_reboot=1 quiet"
# CONFIG_PPC_MEM_KEYS is not set
CONFIG_JUMP_LABEL=y
CONFIG_STRICT_KERNEL_RWX=y
+CONFIG_STRICT_MODULE_RWX=y
CONFIG_MODULES=y
CONFIG_MODULE_UNLOAD=y
CONFIG_MODULE_SIG_FORCE=y
--
2.25.1
^ permalink raw reply related
* [PATCH v6 4/7] powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
From: Russell Currey @ 2020-03-10 1:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, joel, Russell Currey, dja
In-Reply-To: <20200310010338.21205-1-ruscur@russell.cc>
To enable strict module RWX on powerpc, set:
CONFIG_STRICT_MODULE_RWX=y
You should also have CONFIG_STRICT_KERNEL_RWX=y set to have any real
security benefit.
ARCH_HAS_STRICT_MODULE_RWX is set to require ARCH_HAS_STRICT_KERNEL_RWX.
This is due to a quirk in arch/Kconfig and arch/powerpc/Kconfig that
makes STRICT_MODULE_RWX *on by default* in configurations where
STRICT_KERNEL_RWX is *unavailable*.
Since this doesn't make much sense, and module RWX without kernel RWX
doesn't make much sense, having the same dependencies as kernel RWX
works around this problem.
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index bd074246e34e..e1fc7fba10bf 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -131,6 +131,7 @@ config PPC
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
select ARCH_HAS_SET_MEMORY
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
+ select ARCH_HAS_STRICT_MODULE_RWX if ARCH_HAS_STRICT_KERNEL_RWX
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
select ARCH_HAS_UACCESS_MCSAFE if PPC64
--
2.25.1
^ permalink raw reply related
* [PATCH v6 1/7] powerpc/mm: Implement set_memory() routines
From: Russell Currey @ 2020-03-10 1:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, joel, Russell Currey, dja
In-Reply-To: <20200310010338.21205-1-ruscur@russell.cc>
The set_memory_{ro/rw/nx/x}() functions are required for STRICT_MODULE_RWX,
and are generally useful primitives to have. This implementation is
designed to be completely generic across powerpc's many MMUs.
It's possible that this could be optimised to be faster for specific
MMUs, but the focus is on having a generic and safe implementation for
now.
This implementation does not handle cases where the caller is attempting
to change the mapping of the page it is executing from, or if another
CPU is concurrently using the page being altered. These cases likely
shouldn't happen, but a more complex implementation with MMU-specific code
could safely handle them, so that is left as a TODO for now.
These functions do nothing if STRICT_KERNEL_RWX is not enabled.
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
v6: Merge patch 8/8 from v5, handling RWX not being enabled.
Add note to change_page_attr() in case it's ever made non-static
---
arch/powerpc/Kconfig | 1 +
arch/powerpc/include/asm/set_memory.h | 32 +++++++++++
arch/powerpc/mm/Makefile | 2 +-
arch/powerpc/mm/pageattr.c | 79 +++++++++++++++++++++++++++
4 files changed, 113 insertions(+), 1 deletion(-)
create mode 100644 arch/powerpc/include/asm/set_memory.h
create mode 100644 arch/powerpc/mm/pageattr.c
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 497b7d0b2d7e..bd074246e34e 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -129,6 +129,7 @@ config PPC
select ARCH_HAS_PTE_SPECIAL
select ARCH_HAS_MEMBARRIER_CALLBACKS
select ARCH_HAS_SCALED_CPUTIME if VIRT_CPU_ACCOUNTING_NATIVE && PPC_BOOK3S_64
+ select ARCH_HAS_SET_MEMORY
select ARCH_HAS_STRICT_KERNEL_RWX if ((PPC_BOOK3S_64 || PPC32) && !HIBERNATION)
select ARCH_HAS_TICK_BROADCAST if GENERIC_CLOCKEVENTS_BROADCAST
select ARCH_HAS_UACCESS_FLUSHCACHE
diff --git a/arch/powerpc/include/asm/set_memory.h b/arch/powerpc/include/asm/set_memory.h
new file mode 100644
index 000000000000..64011ea444b4
--- /dev/null
+++ b/arch/powerpc/include/asm/set_memory.h
@@ -0,0 +1,32 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _ASM_POWERPC_SET_MEMORY_H
+#define _ASM_POWERPC_SET_MEMORY_H
+
+#define SET_MEMORY_RO 0
+#define SET_MEMORY_RW 1
+#define SET_MEMORY_NX 2
+#define SET_MEMORY_X 3
+
+int change_memory_attr(unsigned long addr, int numpages, long action);
+
+static inline int set_memory_ro(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RO);
+}
+
+static inline int set_memory_rw(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_RW);
+}
+
+static inline int set_memory_nx(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_NX);
+}
+
+static inline int set_memory_x(unsigned long addr, int numpages)
+{
+ return change_memory_attr(addr, numpages, SET_MEMORY_X);
+}
+
+#endif
diff --git a/arch/powerpc/mm/Makefile b/arch/powerpc/mm/Makefile
index 5e147986400d..a998fdac52f9 100644
--- a/arch/powerpc/mm/Makefile
+++ b/arch/powerpc/mm/Makefile
@@ -5,7 +5,7 @@
ccflags-$(CONFIG_PPC64) := $(NO_MINIMAL_TOC)
-obj-y := fault.o mem.o pgtable.o mmap.o \
+obj-y := fault.o mem.o pgtable.o mmap.o pageattr.o \
init_$(BITS).o pgtable_$(BITS).o \
pgtable-frag.o ioremap.o ioremap_$(BITS).o \
init-common.o mmu_context.o drmem.o
diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
new file mode 100644
index 000000000000..748fa56d9db0
--- /dev/null
+++ b/arch/powerpc/mm/pageattr.c
@@ -0,0 +1,79 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * MMU-generic set_memory implementation for powerpc
+ *
+ * Copyright 2019, IBM Corporation.
+ */
+
+#include <linux/mm.h>
+#include <linux/set_memory.h>
+
+#include <asm/mmu.h>
+#include <asm/page.h>
+#include <asm/pgtable.h>
+
+
+/*
+ * Updates the attributes of a page in three steps:
+ *
+ * 1. invalidate the page table entry
+ * 2. flush the TLB
+ * 3. install the new entry with the updated attributes
+ *
+ * This is unsafe if the caller is attempting to change the mapping of the
+ * page it is executing from, or if another CPU is concurrently using the
+ * page being altered.
+ *
+ * TODO make the implementation resistant to this.
+ *
+ * NOTE: can be dangerous to call without STRICT_KERNEL_RWX
+ */
+static int change_page_attr(pte_t *ptep, unsigned long addr, void *data)
+{
+ long action = (long)data;
+ pte_t pte;
+
+ spin_lock(&init_mm.page_table_lock);
+
+ /* invalidate the PTE so it's safe to modify */
+ pte = ptep_get_and_clear(&init_mm, addr, ptep);
+ flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
+
+ /* modify the PTE bits as desired, then apply */
+ switch (action) {
+ case SET_MEMORY_RO:
+ pte = pte_wrprotect(pte);
+ break;
+ case SET_MEMORY_RW:
+ pte = pte_mkwrite(pte);
+ break;
+ case SET_MEMORY_NX:
+ pte = pte_exprotect(pte);
+ break;
+ case SET_MEMORY_X:
+ pte = pte_mkexec(pte);
+ break;
+ default:
+ break;
+ }
+
+ set_pte_at(&init_mm, addr, ptep, pte);
+ spin_unlock(&init_mm.page_table_lock);
+
+ return 0;
+}
+
+int change_memory_attr(unsigned long addr, int numpages, long action)
+{
+ unsigned long start = ALIGN_DOWN(addr, PAGE_SIZE);
+ unsigned long sz = numpages * PAGE_SIZE;
+
+ if (!IS_ENABLED(CONFIG_STRICT_KERNEL_RWX))
+ return 0;
+
+ if (!numpages)
+ return 0;
+
+ return apply_to_page_range(&init_mm, start, sz, change_page_attr, (void *)action);
+}
--
2.25.1
^ permalink raw reply related
* [PATCH v6 2/7] powerpc/kprobes: Mark newly allocated probes as RO
From: Russell Currey @ 2020-03-10 1:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, joel, Russell Currey, dja
In-Reply-To: <20200310010338.21205-1-ruscur@russell.cc>
With CONFIG_STRICT_KERNEL_RWX=y and CONFIG_KPROBES=y, there will be one
W+X page at boot by default. This can be tested with
CONFIG_PPC_PTDUMP=y and CONFIG_PPC_DEBUG_WX=y set, and checking the
kernel log during boot.
powerpc doesn't implement its own alloc() for kprobes like other
architectures do, but we couldn't immediately mark RO anyway since we do
a memcpy to the page we allocate later. After that, nothing should be
allowed to modify the page, and write permissions are removed well
before the kprobe is armed.
The memcpy() would fail if >1 probes were allocated, so use
patch_instruction() instead which is safe for RO.
Reviewed-by: Daniel Axtens <dja@axtens.net>
Signed-off-by: Russell Currey <ruscur@russell.cc>
Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
---
arch/powerpc/kernel/kprobes.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/kprobes.c b/arch/powerpc/kernel/kprobes.c
index 2d27ec4feee4..bfab91ded234 100644
--- a/arch/powerpc/kernel/kprobes.c
+++ b/arch/powerpc/kernel/kprobes.c
@@ -24,6 +24,8 @@
#include <asm/sstep.h>
#include <asm/sections.h>
#include <linux/uaccess.h>
+#include <linux/set_memory.h>
+#include <linux/vmalloc.h>
DEFINE_PER_CPU(struct kprobe *, current_kprobe) = NULL;
DEFINE_PER_CPU(struct kprobe_ctlblk, kprobe_ctlblk);
@@ -102,6 +104,16 @@ kprobe_opcode_t *kprobe_lookup_name(const char *name, unsigned int offset)
return addr;
}
+void *alloc_insn_page(void)
+{
+ void *page = vmalloc_exec(PAGE_SIZE);
+
+ if (page)
+ set_memory_ro((unsigned long)page, 1);
+
+ return page;
+}
+
int arch_prepare_kprobe(struct kprobe *p)
{
int ret = 0;
@@ -124,11 +136,8 @@ int arch_prepare_kprobe(struct kprobe *p)
}
if (!ret) {
- memcpy(p->ainsn.insn, p->addr,
- MAX_INSN_SIZE * sizeof(kprobe_opcode_t));
+ patch_instruction(p->ainsn.insn, *p->addr);
p->opcode = *p->addr;
- flush_icache_range((unsigned long)p->ainsn.insn,
- (unsigned long)p->ainsn.insn + sizeof(kprobe_opcode_t));
}
p->ainsn.boostable = 0;
--
2.25.1
^ permalink raw reply related
* [PATCH v6 3/7] powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
From: Russell Currey @ 2020-03-10 1:03 UTC (permalink / raw)
To: linuxppc-dev
Cc: ajd, Kees Cook, kernel-hardening, npiggin, joel, Russell Currey,
dja
In-Reply-To: <20200310010338.21205-1-ruscur@russell.cc>
Very rudimentary, just
echo 1 > [debugfs]/check_wx_pages
and check the kernel log. Useful for testing strict module RWX.
Updated the Kconfig entry to reflect this.
Also fixed a typo.
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: Russell Currey <ruscur@russell.cc>
---
arch/powerpc/Kconfig.debug | 6 ++++--
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++++++++++++++++++-
2 files changed, 24 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/Kconfig.debug b/arch/powerpc/Kconfig.debug
index 0b063830eea8..e37960ef68c6 100644
--- a/arch/powerpc/Kconfig.debug
+++ b/arch/powerpc/Kconfig.debug
@@ -370,7 +370,7 @@ config PPC_PTDUMP
If you are unsure, say N.
config PPC_DEBUG_WX
- bool "Warn on W+X mappings at boot"
+ bool "Warn on W+X mappings at boot & enable manual checks at runtime"
depends on PPC_PTDUMP && STRICT_KERNEL_RWX
help
Generate a warning if any W+X mappings are found at boot.
@@ -384,7 +384,9 @@ config PPC_DEBUG_WX
of other unfixed kernel bugs easier.
There is no runtime or memory usage effect of this option
- once the kernel has booted up - it's a one time check.
+ once the kernel has booted up, it only automatically checks once.
+
+ Enables the "check_wx_pages" debugfs entry for checking at runtime.
If in doubt, say "Y".
diff --git a/arch/powerpc/mm/ptdump/ptdump.c b/arch/powerpc/mm/ptdump/ptdump.c
index 206156255247..a15e19a3b14e 100644
--- a/arch/powerpc/mm/ptdump/ptdump.c
+++ b/arch/powerpc/mm/ptdump/ptdump.c
@@ -4,7 +4,7 @@
*
* This traverses the kernel pagetables and dumps the
* information about the used sections of memory to
- * /sys/kernel/debug/kernel_pagetables.
+ * /sys/kernel/debug/kernel_page_tables.
*
* Derived from the arm64 implementation:
* Copyright (c) 2014, The Linux Foundation, Laura Abbott.
@@ -413,6 +413,25 @@ void ptdump_check_wx(void)
else
pr_info("Checked W+X mappings: passed, no W+X pages found\n");
}
+
+static int check_wx_debugfs_set(void *data, u64 val)
+{
+ if (val != 1ULL)
+ return -EINVAL;
+
+ ptdump_check_wx();
+
+ return 0;
+}
+
+DEFINE_SIMPLE_ATTRIBUTE(check_wx_fops, NULL, check_wx_debugfs_set, "%llu\n");
+
+static int ptdump_check_wx_init(void)
+{
+ return debugfs_create_file("check_wx_pages", 0200, NULL,
+ NULL, &check_wx_fops) ? 0 : -ENOMEM;
+}
+device_initcall(ptdump_check_wx_init);
#endif
static int ptdump_init(void)
--
2.25.1
^ permalink raw reply related
* [PATCH v6 0/7] set_memory() routines and STRICT_MODULE_RWX
From: Russell Currey @ 2020-03-10 1:03 UTC (permalink / raw)
To: linuxppc-dev; +Cc: ajd, kernel-hardening, npiggin, joel, Russell Currey, dja
Back again, just minor changes.
v5: https://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=160869
Changes since v5:
[1/8]: Patch 8/8 squashed as suggested by Andrew Donnellan
Added a note to the comment of change_page_attr()
Rename size to sz to meet 90 chars without multiple lines
[8/8]: Removed, change_memory_attr() section squashed, rest dropped
as suggested by Christophe Leroy (since I just assumed it was
the right thing to do instead of actually checking)
Thanks for the feedback.
Christophe Leroy (2):
powerpc/mm: implement set_memory_attr()
powerpc/32: use set_memory_attr()
Russell Currey (5):
powerpc/mm: Implement set_memory() routines
powerpc/kprobes: Mark newly allocated probes as RO
powerpc/mm/ptdump: debugfs handler for W+X checks at runtime
powerpc: Set ARCH_HAS_STRICT_MODULE_RWX
powerpc/configs: Enable STRICT_MODULE_RWX in skiroot_defconfig
arch/powerpc/Kconfig | 2 +
arch/powerpc/Kconfig.debug | 6 +-
arch/powerpc/configs/skiroot_defconfig | 1 +
arch/powerpc/include/asm/set_memory.h | 34 ++++++++
arch/powerpc/kernel/kprobes.c | 17 +++-
arch/powerpc/mm/Makefile | 2 +-
arch/powerpc/mm/pageattr.c | 112 +++++++++++++++++++++++++
arch/powerpc/mm/pgtable_32.c | 95 +++------------------
arch/powerpc/mm/ptdump/ptdump.c | 21 ++++-
9 files changed, 197 insertions(+), 93 deletions(-)
create mode 100644 arch/powerpc/include/asm/set_memory.h
create mode 100644 arch/powerpc/mm/pageattr.c
--
2.25.1
^ permalink raw reply
* [PATCH net] ibmvnic: Do not process device remove during device reset
From: Juliet Kim @ 2020-03-10 0:02 UTC (permalink / raw)
To: netdev; +Cc: julietk, tlfalcon, linuxppc-dev
The ibmvnic driver does not check the device state when the device
is removed. If the device is removed while a device reset is being
processed, the remove may free structures needed by the reset,
causing an oops.
Fix this by checking the device state before processing device remove.
Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com>
---
drivers/net/ethernet/ibm/ibmvnic.c | 24 ++++++++++++++++++++++--
drivers/net/ethernet/ibm/ibmvnic.h | 6 +++++-
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index c75239d8820f..7ef1ae0d49bc 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2144,6 +2144,8 @@ static void __ibmvnic_reset(struct work_struct *work)
struct ibmvnic_adapter *adapter;
u32 reset_state;
int rc = 0;
+ unsigned long flags;
+ bool saved_state = false;
adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
@@ -2153,17 +2155,25 @@ static void __ibmvnic_reset(struct work_struct *work)
return;
}
- reset_state = adapter->state;
-
rwi = get_next_rwi(adapter);
while (rwi) {
+ spin_lock_irqsave(&adapter->state_lock, flags);
+
if (adapter->state == VNIC_REMOVING ||
adapter->state == VNIC_REMOVED) {
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
kfree(rwi);
rc = EBUSY;
break;
}
+ if (!saved_state) {
+ reset_state = adapter->state;
+ adapter->state = VNIC_RESETTING;
+ saved_state = true;
+ }
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
+
if (rwi->reset_reason == VNIC_RESET_CHANGE_PARAM) {
/* CHANGE_PARAM requestor holds rtnl_lock */
rc = do_change_param_reset(adapter, rwi, reset_state);
@@ -5091,6 +5101,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
__ibmvnic_delayed_reset);
INIT_LIST_HEAD(&adapter->rwi_list);
spin_lock_init(&adapter->rwi_lock);
+ spin_lock_init(&adapter->state_lock);
mutex_init(&adapter->fw_lock);
init_completion(&adapter->init_done);
init_completion(&adapter->fw_done);
@@ -5161,10 +5172,19 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
static int ibmvnic_remove(struct vio_dev *dev)
{
+ unsigned long flags;
struct net_device *netdev = dev_get_drvdata(&dev->dev);
struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+ spin_lock_irqsave(&adapter->state_lock, flags);
+ if (adapter->state == VNIC_RESETTING) {
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
+ return -EBUSY;
+ }
+
adapter->state = VNIC_REMOVING;
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
+
rtnl_lock();
unregister_netdevice(netdev);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 60eccaf91b12..971505e4cc52 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -941,7 +941,8 @@ enum vnic_state {VNIC_PROBING = 1,
VNIC_CLOSING,
VNIC_CLOSED,
VNIC_REMOVING,
- VNIC_REMOVED};
+ VNIC_REMOVED,
+ VNIC_RESETTING};
enum ibmvnic_reset_reason {VNIC_RESET_FAILOVER = 1,
VNIC_RESET_MOBILITY,
@@ -1090,4 +1091,7 @@ struct ibmvnic_adapter {
struct ibmvnic_tunables desired;
struct ibmvnic_tunables fallback;
+
+ /* lock to protect state field */
+ spinlock_t state_lock;
};
--
2.25.0
^ permalink raw reply related
* [PATCH] ibmvnic: Do not process device remove during device reset
From: Juliet Kim @ 2020-03-10 0:00 UTC (permalink / raw)
To: netdev; +Cc: julietk, tlfalcon, linuxppc-dev
The ibmvnic driver does not check the device state when the device
is removed. If the device is removed while a device reset is being
processed, the remove may free structures needed by the reset,
causing an oops.
Fix this by checking the device state before processing device remove.
Signed-off-by: Juliet Kim <julietk@linux.vnet.ibm.com>
---
drivers/net/ethernet/ibm/ibmvnic.c | 24 ++++++++++++++++++++++--
drivers/net/ethernet/ibm/ibmvnic.h | 6 +++++-
2 files changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index c75239d8820f..7ef1ae0d49bc 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -2144,6 +2144,8 @@ static void __ibmvnic_reset(struct work_struct *work)
struct ibmvnic_adapter *adapter;
u32 reset_state;
int rc = 0;
+ unsigned long flags;
+ bool saved_state = false;
adapter = container_of(work, struct ibmvnic_adapter, ibmvnic_reset);
@@ -2153,17 +2155,25 @@ static void __ibmvnic_reset(struct work_struct *work)
return;
}
- reset_state = adapter->state;
-
rwi = get_next_rwi(adapter);
while (rwi) {
+ spin_lock_irqsave(&adapter->state_lock, flags);
+
if (adapter->state == VNIC_REMOVING ||
adapter->state == VNIC_REMOVED) {
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
kfree(rwi);
rc = EBUSY;
break;
}
+ if (!saved_state) {
+ reset_state = adapter->state;
+ adapter->state = VNIC_RESETTING;
+ saved_state = true;
+ }
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
+
if (rwi->reset_reason == VNIC_RESET_CHANGE_PARAM) {
/* CHANGE_PARAM requestor holds rtnl_lock */
rc = do_change_param_reset(adapter, rwi, reset_state);
@@ -5091,6 +5101,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
__ibmvnic_delayed_reset);
INIT_LIST_HEAD(&adapter->rwi_list);
spin_lock_init(&adapter->rwi_lock);
+ spin_lock_init(&adapter->state_lock);
mutex_init(&adapter->fw_lock);
init_completion(&adapter->init_done);
init_completion(&adapter->fw_done);
@@ -5161,10 +5172,19 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
static int ibmvnic_remove(struct vio_dev *dev)
{
+ unsigned long flags;
struct net_device *netdev = dev_get_drvdata(&dev->dev);
struct ibmvnic_adapter *adapter = netdev_priv(netdev);
+ spin_lock_irqsave(&adapter->state_lock, flags);
+ if (adapter->state == VNIC_RESETTING) {
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
+ return -EBUSY;
+ }
+
adapter->state = VNIC_REMOVING;
+ spin_unlock_irqrestore(&adapter->state_lock, flags);
+
rtnl_lock();
unregister_netdevice(netdev);
diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index 60eccaf91b12..971505e4cc52 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h
@@ -941,7 +941,8 @@ enum vnic_state {VNIC_PROBING = 1,
VNIC_CLOSING,
VNIC_CLOSED,
VNIC_REMOVING,
- VNIC_REMOVED};
+ VNIC_REMOVED,
+ VNIC_RESETTING};
enum ibmvnic_reset_reason {VNIC_RESET_FAILOVER = 1,
VNIC_RESET_MOBILITY,
@@ -1090,4 +1091,7 @@ struct ibmvnic_adapter {
struct ibmvnic_tunables desired;
struct ibmvnic_tunables fallback;
+
+ /* lock to protect state field */
+ spinlock_t state_lock;
};
--
2.25.0
^ permalink raw reply related
* Re: [PATCH v5 7/7] ASoC: fsl_easrc: Add EASRC ASoC CPU DAI drivers
From: Nicolin Chen @ 2020-03-09 23:46 UTC (permalink / raw)
To: Shengjiu Wang
Cc: mark.rutland, devicetree, alsa-devel, timur, Xiubo.Lee,
linuxppc-dev, tiwai, lgirdwood, robh+dt, perex, broonie, festevam,
linux-kernel
In-Reply-To: <2616bfd81df982add337b169b2d424a8d50c6bda.1583725533.git.shengjiu.wang@nxp.com>
A few small comments -- trying to improve readability.
On Mon, Mar 09, 2020 at 11:58:34AM +0800, Shengjiu Wang wrote:
> EASRC (Enhanced Asynchronous Sample Rate Converter) is a new IP module
> found on i.MX8MN. It is different with old ASRC module.
>
> The primary features for the EASRC are as follows:
> - 4 Contexts - groups of channels with an independent time base
> - Fully independent and concurrent context control
> - Simultaneous processing of up to 32 audio channels
> - Programmable filter charachteristics for each context
> - 32, 24, 20, and 16-bit fixed point audio sample support
> - 32-bit floating point audio sample support
> - 8kHz to 384kHz sample rate
> - 1/16 to 8x sample rate conversion ratio
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> Signed-off-by: Cosmin-Gabriel Samoila <cosmin.samoila@nxp.com>
> ---
> sound/soc/fsl/Kconfig | 11 +
> sound/soc/fsl/Makefile | 2 +
> sound/soc/fsl/fsl_easrc.c | 2111 +++++++++++++++++++++++++++++++++++++
> sound/soc/fsl/fsl_easrc.h | 651 ++++++++++++
> 4 files changed, 2775 insertions(+)
> create mode 100644 sound/soc/fsl/fsl_easrc.c
> create mode 100644 sound/soc/fsl/fsl_easrc.h
> diff --git a/sound/soc/fsl/fsl_easrc.c b/sound/soc/fsl/fsl_easrc.c
> +static int fsl_easrc_resampler_config(struct fsl_asrc *easrc)
> +{
> + struct device *dev = &easrc->pdev->dev;
> + struct fsl_easrc_priv *easrc_priv = easrc->private;
> + struct asrc_firmware_hdr *hdr = easrc_priv->firmware_hdr;
> + struct interp_params *interp = easrc_priv->interp;
> + struct interp_params *selected_interp = NULL;
> + unsigned int num_coeff;
> + unsigned int i;
> + u64 *arr;
> + u32 *r;
> + int ret;
> +
> + if (!hdr) {
> + dev_err(dev, "firmware not loaded!\n");
> + return -ENODEV;
> + }
> +
> + for (i = 0; i < hdr->interp_scen; i++) {
> + if ((interp[i].num_taps - 1) ==
> + bits_taps_to_val(easrc_priv->rs_num_taps)) {
Could fit everything under 80 characters:
+ if ((interp[i].num_taps - 1) !=
+ bits_taps_to_val(easrc_priv->rs_num_taps))
+ continue;
+
+ arr = interp[i].coeff;
+ selected_interp = &interp[i];
+ dev_dbg(dev, "Selected interp_filter: %u taps - %u phases\n",
+ selected_interp->num_taps,
+ selected_interp->num_phases);
+ break;
> +static int fsl_easrc_normalize_filter(struct fsl_asrc *easrc,
> + u64 *infilter,
> + u64 *outfilter,
> + int shift)
> +{
> + struct device *dev = &easrc->pdev->dev;
> + u64 coef = *infilter;
> + s64 exp = (coef & 0x7ff0000000000000ll) >> 52;
Hmm...by masking 0x7ff0000000000000ll, MSB (sign bit) is gone.
Would the result still possibly be a negative value?
> + /*
> + * If exponent is zero (value == 0), or 7ff (value == NaNs)
> + * dont touch the content
> + */
> + if (((coef & 0x7ff0000000000000ll) == 0) ||
> + ((coef & 0x7ff0000000000000ll) == ((u64)0x7ff << 52))) {
You have extracted "exp" already:
+ if (exp == 0 || (u64)exp & 0x7ff == 0x7ff)
> + *outfilter = coef;
Could simply a bit by returning here:
+ return 0;
+ }
Then:
+ /* coef * 2^shift == exp + shift */
+ exp += shift;
+
+ if ((shift > 0 && exp >= 2047) || (shift < 0 && exp <= 0)) {
+ dev_err(dev, "coef out of range\n");
+ return -EINVAL;
+ }
+
+ outcoef = (u64)(coef & 0x800FFFFFFFFFFFFFll) + ((u64)exp << 52);
+ *outfilter = outcoef;
> +static int fsl_easrc_write_pf_coeff_mem(struct fsl_asrc *easrc, int ctx_id,
> + u64 *arr, int n_taps, int shift)
> +{
> + if (!arr) {
> + dev_err(dev, "NULL buffer\n");
Could it be slightly more specific?
> +static int fsl_easrc_prefilter_config(struct fsl_asrc *easrc,
> + unsigned int ctx_id)
> +{
> + ctx_priv->in_filled_sample = bits_taps_to_val(easrc_priv->rs_num_taps) / 2;
> + ctx_priv->out_missed_sample = ctx_priv->in_filled_sample *
> + ctx_priv->out_params.sample_rate /
> + ctx_priv->in_params.sample_rate;
There are quite a few references to sample_rate and sample_format
here, so we could use some local variables to cache them:
+ in_s_rate = ctx_priv->in_params.sample_rate;
+ out_s_rate = ctx_priv->out_params.sample_rate;
+ in_s_fmt = ctx_priv->in_params.sample_format;
+ out_s_fmt = ctx_priv->out_params.sample_format;
> +static int fsl_easrc_config_slot(struct fsl_asrc *easrc, unsigned int ctx_id)
> +{
> + struct fsl_easrc_priv *easrc_priv = easrc->private;
> + struct fsl_asrc_pair *ctx = easrc->pair[ctx_id];
> + int req_channels = ctx->channels;
> + int start_channel = 0, avail_channel;
> + struct fsl_easrc_slot *slot0, *slot1;
> + int i, ret;
> +
> + if (req_channels <= 0)
> + return -EINVAL;
> +
> + for (i = 0; i < EASRC_CTX_MAX_NUM; i++) {
> + slot0 = &easrc_priv->slot[i][0];
> + slot1 = &easrc_priv->slot[i][1];
> +
> + if (slot0->busy && slot1->busy)
> + continue;
> +
Could merge the duplication by doing:
+ struct fsl_easrc_slot *slot = NULL;
...
+ else if ((slot0->busy && slot0->ctx_index == ctx->index) ||
+ (slot1->busy && slot1->ctx_index == ctx->index))
+ continue;
+ else if (!slot0->busy)
+ slot = slot0;
+ else if (!slot1->busy)
+ slot = slot1;
+
+ if (!slot)
+ continue;
+
+ avail_channel = fsl_easrc_max_ch_for_slot(ctx, slot);
+ if (avail_channel <= 0)
+ continue;
+
+ slot->slot_index = 0;
+
+ ret = fsl_easrc_config_one_slot(ctx, slot, i, &req_channels,
+ &start_channel, &avail_channel);
+ if (ret)
+ return ret;
+
+ if (req_channels > 0)
+ continue;
+ else
+ break;
# Please double check before doing copy-n-paste.
> +int fsl_easrc_config_context(struct fsl_asrc *easrc, unsigned int ctx_id)
> +{
> + /* Context Input FIFO Watermark */
> + regmap_update_bits(easrc->regmap, REG_EASRC_CC(ctx_id),
> + EASRC_CC_FIFO_WTMK_MASK,
> + EASRC_CC_FIFO_WTMK(ctx_priv->in_params.fifo_wtmk));
> +
> + /* Context Output FIFO Watermark */
> + regmap_update_bits(easrc->regmap, REG_EASRC_COC(ctx_id),
> + EASRC_COC_FIFO_WTMK_MASK,
> + EASRC_COC_FIFO_WTMK(ctx_priv->out_params.fifo_wtmk - 1));
Why a "-1" here vs. no "-1" for input FIFO? Could probably put
the reason in the comments?
> +void fsl_easrc_release_context(struct fsl_asrc_pair *ctx)
> +{
> + unsigned long lock_flags;
> + struct fsl_asrc *easrc;
> + struct device *dev;
> + int ret;
> +
> + if (!ctx)
> + return;
> +
> + easrc = ctx->asrc;
> + dev = &easrc->pdev->dev;
> +
> + spin_lock_irqsave(&easrc->lock, lock_flags);
> +
> + ret = fsl_easrc_release_slot(easrc, ctx->index);
Where is this "ret" used?
> +
> + easrc->channel_avail += ctx->channels;
> + easrc->pair[ctx->index] = NULL;
> +
> + spin_unlock_irqrestore(&easrc->lock, lock_flags);
> +}
> +void fsl_easrc_dump_firmware(struct fsl_asrc *easrc)
Hmm..where is this function being used? From outside?
> +int fsl_easrc_get_firmware(struct fsl_asrc *easrc)
static?
If it's being called from an outsider, it might be safer to check
easrc->private pointer too?
> +{
> + struct fsl_easrc_priv *easrc_priv;
Could probably clean up some wrappings with:
+ struct firmware **fw_p;
> + u32 pnum, inum, offset;
+ u8 *data;
> + int ret;
> +
> + if (!easrc)
> + return -EINVAL;
> +
> + easrc_priv = easrc->private;
+ fw_p = &easrc_priv->fw;
> + ret = request_firmware(&easrc_priv->fw, easrc_priv->fw_name,
> + &easrc->pdev->dev);
+ ret = request_firmware(fw_p, easrc_priv->fw_name, &easrc->pdev->dev);
> + if (ret)
> + return ret;
+ data = easrc_priv->fw->data;
And replace all data references.
> +static int fsl_easrc_get_fifo_addr(u8 dir, enum asrc_pair_index index)
> +{
> + return REG_EASRC_FIFO(dir, index);
Maybe an inline type or simply a macro?
> +static int fsl_easrc_probe(struct platform_device *pdev)
> +{
: + irq = platform_get_irq(pdev, 0);
> + if (irq < 0) {
> + dev_err(&pdev->dev, "no irq for node %s\n",
> + dev_name(&pdev->dev));
Probably could save some wrappings in this function if we have a:
struct device *dev = &pdev->dev;
And dev_err() prints dev_name() actually, so it'd be better use:
+ dev_err(dev, "no irq for node %pOF\n", np);
> + ret = of_property_read_string(np,
> + "fsl,easrc-ram-script-name",
> + &easrc_priv->fw_name);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to get firmware name\n");
> + return ret;
> + }
Could we move this to the place where we parse DT bindings?
> +static int fsl_easrc_runtime_resume(struct device *dev)
> +{
> + for (i = ASRC_PAIR_A; i < EASRC_CTX_MAX_NUM; i++) {
> + ctx = easrc->pair[i];
> + if (ctx) {
Could do this to save some indentations from following lines:
+ if (!ctx)
+ continue;
> + ctx_priv = ctx->private;
> + fsl_easrc_set_rs_ratio(ctx);
> + ctx_priv->out_missed_sample = ctx_priv->in_filled_sample *
> + ctx_priv->out_params.sample_rate /
> + ctx_priv->in_params.sample_rate;
> + if (ctx_priv->in_filled_sample * ctx_priv->out_params.sample_rate
> + % ctx_priv->in_params.sample_rate != 0)
> + ctx_priv->out_missed_sample += 1;
> +
> + ret = fsl_easrc_write_pf_coeff_mem(easrc, i,
> + ctx_priv->st1_coeff,
> + ctx_priv->st1_num_taps,
> + ctx_priv->st1_addexp);
> + if (ret)
> + goto disable_mem_clk;
> +
> + ret = fsl_easrc_write_pf_coeff_mem(easrc, i,
> + ctx_priv->st2_coeff,
> + ctx_priv->st2_num_taps,
> + ctx_priv->st2_addexp);
> + if (ret)
> + goto disable_mem_clk;
> diff --git a/sound/soc/fsl/fsl_easrc.h b/sound/soc/fsl/fsl_easrc.h
> +struct fsl_easrc_slot {
> + bool busy;
> + int ctx_index;
> + int slot_index;
> + int num_channel; /*maximum is 8*/
Spaces for comments: /* maximum is 8 */
> +/**
> + * fsl_easrc_priv: EASRC private data
> + *
> + * @slot: slot setting
> + * @firmware_hdr: the header of firmware
> + * @interp: pointer to interpolation filter coeff
> + * @prefil: pointer to prefilter coeff
> + * @fw: firmware of coeff table
> + * @fw_name: firmware name
> + * @rs_num_taps: resample filter taps, 32, 64, or 128
> + * @bps_i2c958: bits per sample of IEC958
i2c or iec?
^ permalink raw reply
* Re: [PATCH -next] soc: fsl: dpio: remove set but not used variable 'addr_cena'
From: Li Yang @ 2020-03-09 22:33 UTC (permalink / raw)
To: YueHaibing, Youri Querry
Cc: Roy Pledge, linuxppc-dev, lkml,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20200221083637.13392-1-yuehaibing@huawei.com>
On Fri, Feb 21, 2020 at 2:38 AM YueHaibing <yuehaibing@huawei.com> wrote:
>
> commit 3b2abda7d28c ("soc: fsl: dpio: Replace QMAN array
> mode with ring mode enqueue") introduced this, but not
> used, so remove it.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/soc/fsl/dpio/qbman-portal.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/soc/fsl/dpio/qbman-portal.c b/drivers/soc/fsl/dpio/qbman-portal.c
> index 740ee0d..350de56 100644
> --- a/drivers/soc/fsl/dpio/qbman-portal.c
> +++ b/drivers/soc/fsl/dpio/qbman-portal.c
> @@ -658,7 +658,6 @@ int qbman_swp_enqueue_multiple_direct(struct qbman_swp *s,
> const uint32_t *cl = (uint32_t *)d;
> uint32_t eqcr_ci, eqcr_pi, half_mask, full_mask;
> int i, num_enqueued = 0;
> - uint64_t addr_cena;
>
> spin_lock(&s->access_spinlock);
> half_mask = (s->eqcr.pi_ci_mask>>1);
> @@ -711,7 +710,6 @@ int qbman_swp_enqueue_multiple_direct(struct qbman_swp *s,
>
> /* Flush all the cacheline without load/store in between */
> eqcr_pi = s->eqcr.pi;
> - addr_cena = (size_t)s->addr_cena;
> for (i = 0; i < num_enqueued; i++)
> eqcr_pi++;
> s->eqcr.pi = eqcr_pi & full_mask;
> @@ -822,7 +820,6 @@ int qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s,
> const uint32_t *cl;
> uint32_t eqcr_ci, eqcr_pi, half_mask, full_mask;
> int i, num_enqueued = 0;
> - uint64_t addr_cena;
>
> half_mask = (s->eqcr.pi_ci_mask>>1);
> full_mask = s->eqcr.pi_ci_mask;
> @@ -866,7 +863,6 @@ int qbman_swp_enqueue_multiple_desc_direct(struct qbman_swp *s,
>
> /* Flush all the cacheline without load/store in between */
> eqcr_pi = s->eqcr.pi;
> - addr_cena = (uint64_t)s->addr_cena;
Hi Youri,
Looks like this problem exposed another issue that you removed the
cacheline related code in the upstream version. Then the comment /*
Flush all the cacheline without load/store in between */ is no longer
true now, and probably the whole block can be replaced with a single
line to increase s->eqcr.pi? The same for the block above. Can you
provide a better fix for this issue?
Regards,
Leo
> for (i = 0; i < num_enqueued; i++)
> eqcr_pi++;
> s->eqcr.pi = eqcr_pi & full_mask;
> --
> 2.7.4
>
>
^ permalink raw reply
* Re: [PATCH] powerpc/64: BE option to use ELFv2 ABI for big endian kernels
From: Segher Boessenkool @ 2020-03-09 21:48 UTC (permalink / raw)
To: Nicholas Piggin; +Cc: linuxppc-dev
In-Reply-To: <1583542596.l37hgse8mc.astroid@bobo.none>
On Sat, Mar 07, 2020 at 10:58:54AM +1000, Nicholas Piggin wrote:
> Segher Boessenkool's on March 5, 2020 8:55 pm:
> > That name looks perfect to me. You'll have to update REs expecting the
> > arch at the end (like /le$/), but you had to already I think?
>
> le$ is still okay for testing ppc64le, unless you wanted me to add the
> -elfv2 suffix on there as well? If that was the case, for consistency
> we'd also have to add -elfv1 for the BE v1 case. I was just going to add
> -elfv2 for the new variant.
I was thinking that, yes, but your plan is better alright (there aren't
many supported configs at all, anyway!)
Segher
^ permalink raw reply
* Re: [PATCH v5 4/7] ASoC: fsl_asrc: rename asrc_priv to asrc
From: Nicolin Chen @ 2020-03-09 21:30 UTC (permalink / raw)
To: Shengjiu Wang
Cc: mark.rutland, devicetree, alsa-devel, timur, Xiubo.Lee,
linuxppc-dev, tiwai, lgirdwood, robh+dt, perex, broonie, festevam,
linux-kernel
In-Reply-To: <8282b290d39dd8dae5da02f5cbb3f647fa778aa0.1583725533.git.shengjiu.wang@nxp.com>
On Mon, Mar 09, 2020 at 11:58:31AM +0800, Shengjiu Wang wrote:
> In order to move common structure to fsl_asrc_common.h
> we change the name of asrc_priv to asrc, the asrc_priv
> will be used by new struct fsl_asrc_priv.
This actually could be a cleanup patch which comes as the
first one in this series, so that we could ack it and get
merged without depending on others. Maybe next version?
Thanks
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> sound/soc/fsl/fsl_asrc.c | 296 +++++++++++++++++------------------
> sound/soc/fsl/fsl_asrc.h | 4 +-
> sound/soc/fsl/fsl_asrc_dma.c | 24 +--
> 3 files changed, 162 insertions(+), 162 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl_asrc.c b/sound/soc/fsl/fsl_asrc.c
> index 11dfe3068b04..eea19e2b723b 100644
> --- a/sound/soc/fsl/fsl_asrc.c
> +++ b/sound/soc/fsl/fsl_asrc.c
> @@ -21,10 +21,10 @@
> #define IDEAL_RATIO_DECIMAL_DEPTH 26
>
> #define pair_err(fmt, ...) \
> - dev_err(&asrc_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__)
> + dev_err(&asrc->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__)
>
> #define pair_dbg(fmt, ...) \
> - dev_dbg(&asrc_priv->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__)
> + dev_dbg(&asrc->pdev->dev, "Pair %c: " fmt, 'A' + index, ##__VA_ARGS__)
>
> /* Corresponding to process_option */
> static unsigned int supported_asrc_rate[] = {
> @@ -157,15 +157,15 @@ static void fsl_asrc_sel_proc(int inrate, int outrate,
> int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
> {
> enum asrc_pair_index index = ASRC_INVALID_PAIR;
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> - struct device *dev = &asrc_priv->pdev->dev;
> + struct fsl_asrc *asrc = pair->asrc;
> + struct device *dev = &asrc->pdev->dev;
> unsigned long lock_flags;
> int i, ret = 0;
>
> - spin_lock_irqsave(&asrc_priv->lock, lock_flags);
> + spin_lock_irqsave(&asrc->lock, lock_flags);
>
> for (i = ASRC_PAIR_A; i < ASRC_PAIR_MAX_NUM; i++) {
> - if (asrc_priv->pair[i] != NULL)
> + if (asrc->pair[i] != NULL)
> continue;
>
> index = i;
> @@ -177,17 +177,17 @@ int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
> if (index == ASRC_INVALID_PAIR) {
> dev_err(dev, "all pairs are busy now\n");
> ret = -EBUSY;
> - } else if (asrc_priv->channel_avail < channels) {
> + } else if (asrc->channel_avail < channels) {
> dev_err(dev, "can't afford required channels: %d\n", channels);
> ret = -EINVAL;
> } else {
> - asrc_priv->channel_avail -= channels;
> - asrc_priv->pair[index] = pair;
> + asrc->channel_avail -= channels;
> + asrc->pair[index] = pair;
> pair->channels = channels;
> pair->index = index;
> }
>
> - spin_unlock_irqrestore(&asrc_priv->lock, lock_flags);
> + spin_unlock_irqrestore(&asrc->lock, lock_flags);
>
> return ret;
> }
> @@ -195,25 +195,25 @@ int fsl_asrc_request_pair(int channels, struct fsl_asrc_pair *pair)
> /**
> * Release ASRC pair
> *
> - * It clears the resource from asrc_priv and releases the occupied channels.
> + * It clears the resource from asrc and releases the occupied channels.
> */
> void fsl_asrc_release_pair(struct fsl_asrc_pair *pair)
> {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> enum asrc_pair_index index = pair->index;
> unsigned long lock_flags;
>
> /* Make sure the pair is disabled */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ASRCEi_MASK(index), 0);
>
> - spin_lock_irqsave(&asrc_priv->lock, lock_flags);
> + spin_lock_irqsave(&asrc->lock, lock_flags);
>
> - asrc_priv->channel_avail += pair->channels;
> - asrc_priv->pair[index] = NULL;
> + asrc->channel_avail += pair->channels;
> + asrc->pair[index] = NULL;
> pair->error = 0;
>
> - spin_unlock_irqrestore(&asrc_priv->lock, lock_flags);
> + spin_unlock_irqrestore(&asrc->lock, lock_flags);
> }
>
> /**
> @@ -221,10 +221,10 @@ void fsl_asrc_release_pair(struct fsl_asrc_pair *pair)
> */
> static void fsl_asrc_set_watermarks(struct fsl_asrc_pair *pair, u32 in, u32 out)
> {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> enum asrc_pair_index index = pair->index;
>
> - regmap_update_bits(asrc_priv->regmap, REG_ASRMCR(index),
> + regmap_update_bits(asrc->regmap, REG_ASRMCR(index),
> ASRMCRi_EXTTHRSHi_MASK |
> ASRMCRi_INFIFO_THRESHOLD_MASK |
> ASRMCRi_OUTFIFO_THRESHOLD_MASK,
> @@ -257,7 +257,7 @@ static u32 fsl_asrc_cal_asrck_divisor(struct fsl_asrc_pair *pair, u32 div)
> static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair *pair,
> int inrate, int outrate)
> {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> enum asrc_pair_index index = pair->index;
> unsigned long ratio;
> int i;
> @@ -286,8 +286,8 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair *pair,
> break;
> }
>
> - regmap_write(asrc_priv->regmap, REG_ASRIDRL(index), ratio);
> - regmap_write(asrc_priv->regmap, REG_ASRIDRH(index), ratio >> 24);
> + regmap_write(asrc->regmap, REG_ASRIDRL(index), ratio);
> + regmap_write(asrc->regmap, REG_ASRIDRH(index), ratio >> 24);
>
> return 0;
> }
> @@ -309,7 +309,7 @@ static int fsl_asrc_set_ideal_ratio(struct fsl_asrc_pair *pair,
> static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> {
> struct asrc_config *config = pair->config;
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> enum asrc_pair_index index = pair->index;
> enum asrc_word_width input_word_width;
> enum asrc_word_width output_word_width;
> @@ -392,11 +392,11 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> }
>
> /* Validate input and output clock sources */
> - clk_index[IN] = asrc_priv->clk_map[IN][config->inclk];
> - clk_index[OUT] = asrc_priv->clk_map[OUT][config->outclk];
> + clk_index[IN] = asrc->clk_map[IN][config->inclk];
> + clk_index[OUT] = asrc->clk_map[OUT][config->outclk];
>
> /* We only have output clock for ideal ratio mode */
> - clk = asrc_priv->asrck_clk[clk_index[ideal ? OUT : IN]];
> + clk = asrc->asrck_clk[clk_index[ideal ? OUT : IN]];
>
> clk_rate = clk_get_rate(clk);
> rem[IN] = do_div(clk_rate, inrate);
> @@ -417,7 +417,7 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
>
> div[IN] = min_t(u32, 1024, div[IN]);
>
> - clk = asrc_priv->asrck_clk[clk_index[OUT]];
> + clk = asrc->asrck_clk[clk_index[OUT]];
> clk_rate = clk_get_rate(clk);
> if (ideal && use_ideal_rate)
> rem[OUT] = do_div(clk_rate, IDEAL_RATIO_RATE);
> @@ -437,22 +437,22 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> /* Set the channel number */
> channels = config->channel_num;
>
> - if (asrc_priv->soc->channel_bits < 4)
> + if (asrc->soc->channel_bits < 4)
> channels /= 2;
>
> /* Update channels for current pair */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCNCR,
> - ASRCNCR_ANCi_MASK(index, asrc_priv->soc->channel_bits),
> - ASRCNCR_ANCi(index, channels, asrc_priv->soc->channel_bits));
> + regmap_update_bits(asrc->regmap, REG_ASRCNCR,
> + ASRCNCR_ANCi_MASK(index, asrc->soc->channel_bits),
> + ASRCNCR_ANCi(index, channels, asrc->soc->channel_bits));
>
> /* Default setting: Automatic selection for processing mode */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ATSi_MASK(index), ASRCTR_ATS(index));
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_USRi_MASK(index), 0);
>
> /* Set the input and output clock sources */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCSR,
> + regmap_update_bits(asrc->regmap, REG_ASRCSR,
> ASRCSR_AICSi_MASK(index) | ASRCSR_AOCSi_MASK(index),
> ASRCSR_AICS(index, clk_index[IN]) |
> ASRCSR_AOCS(index, clk_index[OUT]));
> @@ -462,19 +462,19 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> outdiv = fsl_asrc_cal_asrck_divisor(pair, div[OUT]);
>
> /* Suppose indiv and outdiv includes prescaler, so add its MASK too */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCDR(index),
> + regmap_update_bits(asrc->regmap, REG_ASRCDR(index),
> ASRCDRi_AOCPi_MASK(index) | ASRCDRi_AICPi_MASK(index) |
> ASRCDRi_AOCDi_MASK(index) | ASRCDRi_AICDi_MASK(index),
> ASRCDRi_AOCP(index, outdiv) | ASRCDRi_AICP(index, indiv));
>
> /* Implement word_width configurations */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRMCR1(index),
> + regmap_update_bits(asrc->regmap, REG_ASRMCR1(index),
> ASRMCR1i_OW16_MASK | ASRMCR1i_IWD_MASK,
> ASRMCR1i_OW16(output_word_width) |
> ASRMCR1i_IWD(input_word_width));
>
> /* Enable BUFFER STALL */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRMCR(index),
> + regmap_update_bits(asrc->regmap, REG_ASRMCR(index),
> ASRMCRi_BUFSTALLi_MASK, ASRMCRi_BUFSTALLi);
>
> /* Set default thresholds for input and output FIFO */
> @@ -486,18 +486,18 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> return 0;
>
> /* Clear ASTSx bit to use Ideal Ratio mode */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ATSi_MASK(index), 0);
>
> /* Enable Ideal Ratio mode */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_IDRi_MASK(index) | ASRCTR_USRi_MASK(index),
> ASRCTR_IDR(index) | ASRCTR_USR(index));
>
> fsl_asrc_sel_proc(inrate, outrate, &pre_proc, &post_proc);
>
> /* Apply configurations for pre- and post-processing */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCFG,
> + regmap_update_bits(asrc->regmap, REG_ASRCFG,
> ASRCFG_PREMODi_MASK(index) | ASRCFG_POSTMODi_MASK(index),
> ASRCFG_PREMOD(index, pre_proc) |
> ASRCFG_POSTMOD(index, post_proc));
> @@ -512,28 +512,28 @@ static int fsl_asrc_config_pair(struct fsl_asrc_pair *pair, bool use_ideal_rate)
> */
> static void fsl_asrc_start_pair(struct fsl_asrc_pair *pair)
> {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> enum asrc_pair_index index = pair->index;
> int reg, retry = 10, i;
>
> /* Enable the current pair */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ASRCEi_MASK(index), ASRCTR_ASRCE(index));
>
> /* Wait for status of initialization */
> do {
> udelay(5);
> - regmap_read(asrc_priv->regmap, REG_ASRCFG, ®);
> + regmap_read(asrc->regmap, REG_ASRCFG, ®);
> reg &= ASRCFG_INIRQi_MASK(index);
> } while (!reg && --retry);
>
> /* Make the input fifo to ASRC STALL level */
> - regmap_read(asrc_priv->regmap, REG_ASRCNCR, ®);
> + regmap_read(asrc->regmap, REG_ASRCNCR, ®);
> for (i = 0; i < pair->channels * 4; i++)
> - regmap_write(asrc_priv->regmap, REG_ASRDI(index), 0);
> + regmap_write(asrc->regmap, REG_ASRDI(index), 0);
>
> /* Enable overload interrupt */
> - regmap_write(asrc_priv->regmap, REG_ASRIER, ASRIER_AOLIE);
> + regmap_write(asrc->regmap, REG_ASRIER, ASRIER_AOLIE);
> }
>
> /**
> @@ -541,11 +541,11 @@ static void fsl_asrc_start_pair(struct fsl_asrc_pair *pair)
> */
> static void fsl_asrc_stop_pair(struct fsl_asrc_pair *pair)
> {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> enum asrc_pair_index index = pair->index;
>
> /* Stop the current pair */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ASRCEi_MASK(index), 0);
> }
>
> @@ -554,23 +554,23 @@ static void fsl_asrc_stop_pair(struct fsl_asrc_pair *pair)
> */
> struct dma_chan *fsl_asrc_get_dma_channel(struct fsl_asrc_pair *pair, bool dir)
> {
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> enum asrc_pair_index index = pair->index;
> char name[4];
>
> sprintf(name, "%cx%c", dir == IN ? 'r' : 't', index + 'a');
>
> - return dma_request_slave_channel(&asrc_priv->pdev->dev, name);
> + return dma_request_slave_channel(&asrc->pdev->dev, name);
> }
> EXPORT_SYMBOL_GPL(fsl_asrc_get_dma_channel);
>
> static int fsl_asrc_dai_startup(struct snd_pcm_substream *substream,
> struct snd_soc_dai *dai)
> {
> - struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> + struct fsl_asrc *asrc = snd_soc_dai_get_drvdata(dai);
>
> /* Odd channel number is not valid for older ASRC (channel_bits==3) */
> - if (asrc_priv->soc->channel_bits == 3)
> + if (asrc->soc->channel_bits == 3)
> snd_pcm_hw_constraint_step(substream->runtime, 0,
> SNDRV_PCM_HW_PARAM_CHANNELS, 2);
>
> @@ -583,7 +583,7 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
> struct snd_pcm_hw_params *params,
> struct snd_soc_dai *dai)
> {
> - struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> + struct fsl_asrc *asrc = snd_soc_dai_get_drvdata(dai);
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct fsl_asrc_pair *pair = runtime->private_data;
> unsigned int channels = params_channels(params);
> @@ -606,13 +606,13 @@ static int fsl_asrc_dai_hw_params(struct snd_pcm_substream *substream,
>
> if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> config.input_format = params_format(params);
> - config.output_format = asrc_priv->asrc_format;
> + config.output_format = asrc->asrc_format;
> config.input_sample_rate = rate;
> - config.output_sample_rate = asrc_priv->asrc_rate;
> + config.output_sample_rate = asrc->asrc_rate;
> } else {
> - config.input_format = asrc_priv->asrc_format;
> + config.input_format = asrc->asrc_format;
> config.output_format = params_format(params);
> - config.input_sample_rate = asrc_priv->asrc_rate;
> + config.input_sample_rate = asrc->asrc_rate;
> config.output_sample_rate = rate;
> }
>
> @@ -670,10 +670,10 @@ static const struct snd_soc_dai_ops fsl_asrc_dai_ops = {
>
> static int fsl_asrc_dai_probe(struct snd_soc_dai *dai)
> {
> - struct fsl_asrc *asrc_priv = snd_soc_dai_get_drvdata(dai);
> + struct fsl_asrc *asrc = snd_soc_dai_get_drvdata(dai);
>
> - snd_soc_dai_init_dma_data(dai, &asrc_priv->dma_params_tx,
> - &asrc_priv->dma_params_rx);
> + snd_soc_dai_init_dma_data(dai, &asrc->dma_params_tx,
> + &asrc->dma_params_rx);
>
> return 0;
> }
> @@ -852,30 +852,30 @@ static const struct regmap_config fsl_asrc_regmap_config = {
> /**
> * Initialize ASRC registers with a default configurations
> */
> -static int fsl_asrc_init(struct fsl_asrc *asrc_priv)
> +static int fsl_asrc_init(struct fsl_asrc *asrc)
> {
> /* Halt ASRC internal FP when input FIFO needs data for pair A, B, C */
> - regmap_write(asrc_priv->regmap, REG_ASRCTR, ASRCTR_ASRCEN);
> + regmap_write(asrc->regmap, REG_ASRCTR, ASRCTR_ASRCEN);
>
> /* Disable interrupt by default */
> - regmap_write(asrc_priv->regmap, REG_ASRIER, 0x0);
> + regmap_write(asrc->regmap, REG_ASRIER, 0x0);
>
> /* Apply recommended settings for parameters from Reference Manual */
> - regmap_write(asrc_priv->regmap, REG_ASRPM1, 0x7fffff);
> - regmap_write(asrc_priv->regmap, REG_ASRPM2, 0x255555);
> - regmap_write(asrc_priv->regmap, REG_ASRPM3, 0xff7280);
> - regmap_write(asrc_priv->regmap, REG_ASRPM4, 0xff7280);
> - regmap_write(asrc_priv->regmap, REG_ASRPM5, 0xff7280);
> + regmap_write(asrc->regmap, REG_ASRPM1, 0x7fffff);
> + regmap_write(asrc->regmap, REG_ASRPM2, 0x255555);
> + regmap_write(asrc->regmap, REG_ASRPM3, 0xff7280);
> + regmap_write(asrc->regmap, REG_ASRPM4, 0xff7280);
> + regmap_write(asrc->regmap, REG_ASRPM5, 0xff7280);
>
> /* Base address for task queue FIFO. Set to 0x7C */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRTFR1,
> + regmap_update_bits(asrc->regmap, REG_ASRTFR1,
> ASRTFR1_TF_BASE_MASK, ASRTFR1_TF_BASE(0xfc));
>
> /* Set the processing clock for 76KHz to 133M */
> - regmap_write(asrc_priv->regmap, REG_ASR76K, 0x06D6);
> + regmap_write(asrc->regmap, REG_ASR76K, 0x06D6);
>
> /* Set the processing clock for 56KHz to 133M */
> - return regmap_write(asrc_priv->regmap, REG_ASR56K, 0x0947);
> + return regmap_write(asrc->regmap, REG_ASR56K, 0x0947);
> }
>
> /**
> @@ -883,15 +883,15 @@ static int fsl_asrc_init(struct fsl_asrc *asrc_priv)
> */
> static irqreturn_t fsl_asrc_isr(int irq, void *dev_id)
> {
> - struct fsl_asrc *asrc_priv = (struct fsl_asrc *)dev_id;
> - struct device *dev = &asrc_priv->pdev->dev;
> + struct fsl_asrc *asrc = (struct fsl_asrc *)dev_id;
> + struct device *dev = &asrc->pdev->dev;
> enum asrc_pair_index index;
> u32 status;
>
> - regmap_read(asrc_priv->regmap, REG_ASRSTR, &status);
> + regmap_read(asrc->regmap, REG_ASRSTR, &status);
>
> /* Clean overload error */
> - regmap_write(asrc_priv->regmap, REG_ASRSTR, ASRSTR_AOLE);
> + regmap_write(asrc->regmap, REG_ASRSTR, ASRSTR_AOLE);
>
> /*
> * We here use dev_dbg() for all exceptions because ASRC itself does
> @@ -899,31 +899,31 @@ static irqreturn_t fsl_asrc_isr(int irq, void *dev_id)
> * interrupt would result a ridged conversion.
> */
> for (index = ASRC_PAIR_A; index < ASRC_PAIR_MAX_NUM; index++) {
> - if (!asrc_priv->pair[index])
> + if (!asrc->pair[index])
> continue;
>
> if (status & ASRSTR_ATQOL) {
> - asrc_priv->pair[index]->error |= ASRC_TASK_Q_OVERLOAD;
> + asrc->pair[index]->error |= ASRC_TASK_Q_OVERLOAD;
> dev_dbg(dev, "ASRC Task Queue FIFO overload\n");
> }
>
> if (status & ASRSTR_AOOL(index)) {
> - asrc_priv->pair[index]->error |= ASRC_OUTPUT_TASK_OVERLOAD;
> + asrc->pair[index]->error |= ASRC_OUTPUT_TASK_OVERLOAD;
> pair_dbg("Output Task Overload\n");
> }
>
> if (status & ASRSTR_AIOL(index)) {
> - asrc_priv->pair[index]->error |= ASRC_INPUT_TASK_OVERLOAD;
> + asrc->pair[index]->error |= ASRC_INPUT_TASK_OVERLOAD;
> pair_dbg("Input Task Overload\n");
> }
>
> if (status & ASRSTR_AODO(index)) {
> - asrc_priv->pair[index]->error |= ASRC_OUTPUT_BUFFER_OVERFLOW;
> + asrc->pair[index]->error |= ASRC_OUTPUT_BUFFER_OVERFLOW;
> pair_dbg("Output Data Buffer has overflowed\n");
> }
>
> if (status & ASRSTR_AIDU(index)) {
> - asrc_priv->pair[index]->error |= ASRC_INPUT_BUFFER_UNDERRUN;
> + asrc->pair[index]->error |= ASRC_INPUT_BUFFER_UNDERRUN;
> pair_dbg("Input Data Buffer has underflowed\n");
> }
> }
> @@ -934,7 +934,7 @@ static irqreturn_t fsl_asrc_isr(int irq, void *dev_id)
> static int fsl_asrc_probe(struct platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
> - struct fsl_asrc *asrc_priv;
> + struct fsl_asrc *asrc;
> struct resource *res;
> void __iomem *regs;
> int irq, ret, i;
> @@ -942,11 +942,11 @@ static int fsl_asrc_probe(struct platform_device *pdev)
> char tmp[16];
> u32 width;
>
> - asrc_priv = devm_kzalloc(&pdev->dev, sizeof(*asrc_priv), GFP_KERNEL);
> - if (!asrc_priv)
> + asrc = devm_kzalloc(&pdev->dev, sizeof(*asrc), GFP_KERNEL);
> + if (!asrc)
> return -ENOMEM;
>
> - asrc_priv->pdev = pdev;
> + asrc->pdev = pdev;
>
> /* Get the addresses and IRQ */
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -954,13 +954,13 @@ static int fsl_asrc_probe(struct platform_device *pdev)
> if (IS_ERR(regs))
> return PTR_ERR(regs);
>
> - asrc_priv->paddr = res->start;
> + asrc->paddr = res->start;
>
> - asrc_priv->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "mem", regs,
> - &fsl_asrc_regmap_config);
> - if (IS_ERR(asrc_priv->regmap)) {
> + asrc->regmap = devm_regmap_init_mmio_clk(&pdev->dev, "mem", regs,
> + &fsl_asrc_regmap_config);
> + if (IS_ERR(asrc->regmap)) {
> dev_err(&pdev->dev, "failed to init regmap\n");
> - return PTR_ERR(asrc_priv->regmap);
> + return PTR_ERR(asrc->regmap);
> }
>
> irq = platform_get_irq(pdev, 0);
> @@ -968,49 +968,49 @@ static int fsl_asrc_probe(struct platform_device *pdev)
> return irq;
>
> ret = devm_request_irq(&pdev->dev, irq, fsl_asrc_isr, 0,
> - dev_name(&pdev->dev), asrc_priv);
> + dev_name(&pdev->dev), asrc);
> if (ret) {
> dev_err(&pdev->dev, "failed to claim irq %u: %d\n", irq, ret);
> return ret;
> }
>
> - asrc_priv->mem_clk = devm_clk_get(&pdev->dev, "mem");
> - if (IS_ERR(asrc_priv->mem_clk)) {
> + asrc->mem_clk = devm_clk_get(&pdev->dev, "mem");
> + if (IS_ERR(asrc->mem_clk)) {
> dev_err(&pdev->dev, "failed to get mem clock\n");
> - return PTR_ERR(asrc_priv->mem_clk);
> + return PTR_ERR(asrc->mem_clk);
> }
>
> - asrc_priv->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> - if (IS_ERR(asrc_priv->ipg_clk)) {
> + asrc->ipg_clk = devm_clk_get(&pdev->dev, "ipg");
> + if (IS_ERR(asrc->ipg_clk)) {
> dev_err(&pdev->dev, "failed to get ipg clock\n");
> - return PTR_ERR(asrc_priv->ipg_clk);
> + return PTR_ERR(asrc->ipg_clk);
> }
>
> - asrc_priv->spba_clk = devm_clk_get(&pdev->dev, "spba");
> - if (IS_ERR(asrc_priv->spba_clk))
> + asrc->spba_clk = devm_clk_get(&pdev->dev, "spba");
> + if (IS_ERR(asrc->spba_clk))
> dev_warn(&pdev->dev, "failed to get spba clock\n");
>
> for (i = 0; i < ASRC_CLK_MAX_NUM; i++) {
> sprintf(tmp, "asrck_%x", i);
> - asrc_priv->asrck_clk[i] = devm_clk_get(&pdev->dev, tmp);
> - if (IS_ERR(asrc_priv->asrck_clk[i])) {
> + asrc->asrck_clk[i] = devm_clk_get(&pdev->dev, tmp);
> + if (IS_ERR(asrc->asrck_clk[i])) {
> dev_err(&pdev->dev, "failed to get %s clock\n", tmp);
> - return PTR_ERR(asrc_priv->asrck_clk[i]);
> + return PTR_ERR(asrc->asrck_clk[i]);
> }
> }
>
> - asrc_priv->soc = of_device_get_match_data(&pdev->dev);
> - if (!asrc_priv->soc) {
> + asrc->soc = of_device_get_match_data(&pdev->dev);
> + if (!asrc->soc) {
> dev_err(&pdev->dev, "failed to get soc data\n");
> return -ENODEV;
> }
>
> if (of_device_is_compatible(np, "fsl,imx35-asrc")) {
> - asrc_priv->clk_map[IN] = input_clk_map_imx35;
> - asrc_priv->clk_map[OUT] = output_clk_map_imx35;
> + asrc->clk_map[IN] = input_clk_map_imx35;
> + asrc->clk_map[OUT] = output_clk_map_imx35;
> } else if (of_device_is_compatible(np, "fsl,imx53-asrc")) {
> - asrc_priv->clk_map[IN] = input_clk_map_imx53;
> - asrc_priv->clk_map[OUT] = output_clk_map_imx53;
> + asrc->clk_map[IN] = input_clk_map_imx53;
> + asrc->clk_map[OUT] = output_clk_map_imx53;
> } else if (of_device_is_compatible(np, "fsl,imx8qm-asrc") ||
> of_device_is_compatible(np, "fsl,imx8qxp-asrc")) {
> ret = of_property_read_u32(np, "fsl,asrc-clk-map", &map_idx);
> @@ -1024,30 +1024,30 @@ static int fsl_asrc_probe(struct platform_device *pdev)
> return -EINVAL;
> }
> if (of_device_is_compatible(np, "fsl,imx8qm-asrc")) {
> - asrc_priv->clk_map[IN] = clk_map_imx8qm[map_idx];
> - asrc_priv->clk_map[OUT] = clk_map_imx8qm[map_idx];
> + asrc->clk_map[IN] = clk_map_imx8qm[map_idx];
> + asrc->clk_map[OUT] = clk_map_imx8qm[map_idx];
> } else {
> - asrc_priv->clk_map[IN] = clk_map_imx8qxp[map_idx];
> - asrc_priv->clk_map[OUT] = clk_map_imx8qxp[map_idx];
> + asrc->clk_map[IN] = clk_map_imx8qxp[map_idx];
> + asrc->clk_map[OUT] = clk_map_imx8qxp[map_idx];
> }
> }
>
> - ret = fsl_asrc_init(asrc_priv);
> + ret = fsl_asrc_init(asrc);
> if (ret) {
> dev_err(&pdev->dev, "failed to init asrc %d\n", ret);
> return ret;
> }
>
> - asrc_priv->channel_avail = 10;
> + asrc->channel_avail = 10;
>
> ret = of_property_read_u32(np, "fsl,asrc-rate",
> - &asrc_priv->asrc_rate);
> + &asrc->asrc_rate);
> if (ret) {
> dev_err(&pdev->dev, "failed to get output rate\n");
> return ret;
> }
>
> - ret = of_property_read_u32(np, "fsl,asrc-format", &asrc_priv->asrc_format);
> + ret = of_property_read_u32(np, "fsl,asrc-format", &asrc->asrc_format);
> if (ret) {
> ret = of_property_read_u32(np, "fsl,asrc-width", &width);
> if (ret) {
> @@ -1074,9 +1074,9 @@ static int fsl_asrc_probe(struct platform_device *pdev)
> asrc->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
> }
>
> - platform_set_drvdata(pdev, asrc_priv);
> + platform_set_drvdata(pdev, asrc);
> pm_runtime_enable(&pdev->dev);
> - spin_lock_init(&asrc_priv->lock);
> + spin_lock_init(&asrc->lock);
>
> ret = devm_snd_soc_register_component(&pdev->dev, &fsl_asrc_component,
> &fsl_asrc_dai, 1);
> @@ -1091,22 +1091,22 @@ static int fsl_asrc_probe(struct platform_device *pdev)
> #ifdef CONFIG_PM
> static int fsl_asrc_runtime_resume(struct device *dev)
> {
> - struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
> + struct fsl_asrc *asrc = dev_get_drvdata(dev);
> int i, ret;
>
> - ret = clk_prepare_enable(asrc_priv->mem_clk);
> + ret = clk_prepare_enable(asrc->mem_clk);
> if (ret)
> return ret;
> - ret = clk_prepare_enable(asrc_priv->ipg_clk);
> + ret = clk_prepare_enable(asrc->ipg_clk);
> if (ret)
> goto disable_mem_clk;
> - if (!IS_ERR(asrc_priv->spba_clk)) {
> - ret = clk_prepare_enable(asrc_priv->spba_clk);
> + if (!IS_ERR(asrc->spba_clk)) {
> + ret = clk_prepare_enable(asrc->spba_clk);
> if (ret)
> goto disable_ipg_clk;
> }
> for (i = 0; i < ASRC_CLK_MAX_NUM; i++) {
> - ret = clk_prepare_enable(asrc_priv->asrck_clk[i]);
> + ret = clk_prepare_enable(asrc->asrck_clk[i]);
> if (ret)
> goto disable_asrck_clk;
> }
> @@ -1115,27 +1115,27 @@ static int fsl_asrc_runtime_resume(struct device *dev)
>
> disable_asrck_clk:
> for (i--; i >= 0; i--)
> - clk_disable_unprepare(asrc_priv->asrck_clk[i]);
> - if (!IS_ERR(asrc_priv->spba_clk))
> - clk_disable_unprepare(asrc_priv->spba_clk);
> + clk_disable_unprepare(asrc->asrck_clk[i]);
> + if (!IS_ERR(asrc->spba_clk))
> + clk_disable_unprepare(asrc->spba_clk);
> disable_ipg_clk:
> - clk_disable_unprepare(asrc_priv->ipg_clk);
> + clk_disable_unprepare(asrc->ipg_clk);
> disable_mem_clk:
> - clk_disable_unprepare(asrc_priv->mem_clk);
> + clk_disable_unprepare(asrc->mem_clk);
> return ret;
> }
>
> static int fsl_asrc_runtime_suspend(struct device *dev)
> {
> - struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
> + struct fsl_asrc *asrc = dev_get_drvdata(dev);
> int i;
>
> for (i = 0; i < ASRC_CLK_MAX_NUM; i++)
> - clk_disable_unprepare(asrc_priv->asrck_clk[i]);
> - if (!IS_ERR(asrc_priv->spba_clk))
> - clk_disable_unprepare(asrc_priv->spba_clk);
> - clk_disable_unprepare(asrc_priv->ipg_clk);
> - clk_disable_unprepare(asrc_priv->mem_clk);
> + clk_disable_unprepare(asrc->asrck_clk[i]);
> + if (!IS_ERR(asrc->spba_clk))
> + clk_disable_unprepare(asrc->spba_clk);
> + clk_disable_unprepare(asrc->ipg_clk);
> + clk_disable_unprepare(asrc->mem_clk);
>
> return 0;
> }
> @@ -1144,37 +1144,37 @@ static int fsl_asrc_runtime_suspend(struct device *dev)
> #ifdef CONFIG_PM_SLEEP
> static int fsl_asrc_suspend(struct device *dev)
> {
> - struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
> + struct fsl_asrc *asrc = dev_get_drvdata(dev);
>
> - regmap_read(asrc_priv->regmap, REG_ASRCFG,
> - &asrc_priv->regcache_cfg);
> + regmap_read(asrc->regmap, REG_ASRCFG,
> + &asrc->regcache_cfg);
>
> - regcache_cache_only(asrc_priv->regmap, true);
> - regcache_mark_dirty(asrc_priv->regmap);
> + regcache_cache_only(asrc->regmap, true);
> + regcache_mark_dirty(asrc->regmap);
>
> return 0;
> }
>
> static int fsl_asrc_resume(struct device *dev)
> {
> - struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
> + struct fsl_asrc *asrc = dev_get_drvdata(dev);
> u32 asrctr;
>
> /* Stop all pairs provisionally */
> - regmap_read(asrc_priv->regmap, REG_ASRCTR, &asrctr);
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_read(asrc->regmap, REG_ASRCTR, &asrctr);
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ASRCEi_ALL_MASK, 0);
>
> /* Restore all registers */
> - regcache_cache_only(asrc_priv->regmap, false);
> - regcache_sync(asrc_priv->regmap);
> + regcache_cache_only(asrc->regmap, false);
> + regcache_sync(asrc->regmap);
>
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCFG,
> + regmap_update_bits(asrc->regmap, REG_ASRCFG,
> ASRCFG_NDPRi_ALL_MASK | ASRCFG_POSTMODi_ALL_MASK |
> - ASRCFG_PREMODi_ALL_MASK, asrc_priv->regcache_cfg);
> + ASRCFG_PREMODi_ALL_MASK, asrc->regcache_cfg);
>
> /* Restart enabled pairs */
> - regmap_update_bits(asrc_priv->regmap, REG_ASRCTR,
> + regmap_update_bits(asrc->regmap, REG_ASRCTR,
> ASRCTR_ASRCEi_ALL_MASK, asrctr);
>
> return 0;
> diff --git a/sound/soc/fsl/fsl_asrc.h b/sound/soc/fsl/fsl_asrc.h
> index 4940fa0a7542..3b0f156af2c3 100644
> --- a/sound/soc/fsl/fsl_asrc.h
> +++ b/sound/soc/fsl/fsl_asrc.h
> @@ -448,7 +448,7 @@ struct fsl_asrc_soc_data {
> /**
> * fsl_asrc_pair: ASRC Pair private data
> *
> - * @asrc_priv: pointer to its parent module
> + * @asrc: pointer to its parent module
> * @config: configuration profile
> * @error: error record
> * @index: pair index (ASRC_PAIR_A, ASRC_PAIR_B, ASRC_PAIR_C)
> @@ -460,7 +460,7 @@ struct fsl_asrc_soc_data {
> * @private: pair private area
> */
> struct fsl_asrc_pair {
> - struct fsl_asrc *asrc_priv;
> + struct fsl_asrc *asrc;
> struct asrc_config *config;
> unsigned int error;
>
> diff --git a/sound/soc/fsl/fsl_asrc_dma.c b/sound/soc/fsl/fsl_asrc_dma.c
> index af6b583aa71e..d58b2d4bad47 100644
> --- a/sound/soc/fsl/fsl_asrc_dma.c
> +++ b/sound/soc/fsl/fsl_asrc_dma.c
> @@ -135,7 +135,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
> struct snd_dmaengine_dai_dma_data *dma_params_be = NULL;
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct fsl_asrc_pair *pair = runtime->private_data;
> - struct fsl_asrc *asrc_priv = pair->asrc_priv;
> + struct fsl_asrc *asrc = pair->asrc;
> struct dma_slave_config config_fe, config_be;
> enum asrc_pair_index index = pair->index;
> struct device *dev = component->dev;
> @@ -170,7 +170,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
>
> /* Override dma_data of the Front-End and config its dmaengine */
> dma_params_fe = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
> - dma_params_fe->addr = asrc_priv->paddr + REG_ASRDx(!dir, index);
> + dma_params_fe->addr = asrc->paddr + REG_ASRDx(!dir, index);
> dma_params_fe->maxburst = dma_params_be->maxburst;
>
> pair->dma_chan[!dir] = fsl_asrc_get_dma_channel(pair, !dir);
> @@ -203,7 +203,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
> * need to configure dma_request and dma_request2, but get dma_chan via
> * dma_request_slave_channel directly with dma name of Front-End device
> */
> - if (!asrc_priv->soc->use_edma) {
> + if (!asrc->soc->use_edma) {
> /* Get DMA request of Back-End */
> tmp_chan = dma_request_slave_channel(dev_be, tx ? "tx" : "rx");
> tmp_data = tmp_chan->private;
> @@ -230,7 +230,7 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
> return -EINVAL;
> }
>
> - bits = snd_pcm_format_physical_width(asrc_priv->asrc_format);
> + bits = snd_pcm_format_physical_width(asrc->asrc_format);
> if (bits < 8 || bits > 64)
> return -EINVAL;
> else if (bits == 8)
> @@ -251,10 +251,10 @@ static int fsl_asrc_dma_hw_params(struct snd_soc_component *component,
> config_be.dst_maxburst = dma_params_be->maxburst;
>
> if (tx) {
> - config_be.src_addr = asrc_priv->paddr + REG_ASRDO(index);
> + config_be.src_addr = asrc->paddr + REG_ASRDO(index);
> config_be.dst_addr = dma_params_be->addr;
> } else {
> - config_be.dst_addr = asrc_priv->paddr + REG_ASRDI(index);
> + config_be.dst_addr = asrc->paddr + REG_ASRDI(index);
> config_be.src_addr = dma_params_be->addr;
> }
>
> @@ -297,7 +297,7 @@ static int fsl_asrc_dma_startup(struct snd_soc_component *component,
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct snd_dmaengine_dai_dma_data *dma_data;
> struct device *dev = component->dev;
> - struct fsl_asrc *asrc_priv = dev_get_drvdata(dev);
> + struct fsl_asrc *asrc = dev_get_drvdata(dev);
> struct fsl_asrc_pair *pair;
> struct dma_chan *tmp_chan = NULL;
> u8 dir = tx ? OUT : IN;
> @@ -315,7 +315,7 @@ static int fsl_asrc_dma_startup(struct snd_soc_component *component,
> if (!pair)
> return -ENOMEM;
>
> - pair->asrc_priv = asrc_priv;
> + pair->asrc = asrc;
>
> runtime->private_data = pair;
>
> @@ -370,15 +370,15 @@ static int fsl_asrc_dma_shutdown(struct snd_soc_component *component,
> {
> struct snd_pcm_runtime *runtime = substream->runtime;
> struct fsl_asrc_pair *pair = runtime->private_data;
> - struct fsl_asrc *asrc_priv;
> + struct fsl_asrc *asrc;
>
> if (!pair)
> return 0;
>
> - asrc_priv = pair->asrc_priv;
> + asrc = pair->asrc;
>
> - if (asrc_priv->pair[pair->index] == pair)
> - asrc_priv->pair[pair->index] = NULL;
> + if (asrc->pair[pair->index] == pair)
> + asrc->pair[pair->index] = NULL;
>
> kfree(pair);
>
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl,asrc-format
From: Nicolin Chen @ 2020-03-09 21:19 UTC (permalink / raw)
To: Shengjiu Wang
Cc: mark.rutland, devicetree, alsa-devel, timur, Xiubo.Lee,
linuxppc-dev, tiwai, lgirdwood, robh+dt, perex, broonie, festevam,
linux-kernel
In-Reply-To: <24f69c50925b93afd7a706bd888ee25d27247c78.1583725533.git.shengjiu.wang@nxp.com>
On Mon, Mar 09, 2020 at 11:58:28AM +0800, Shengjiu Wang wrote:
> In order to support new EASRC and simplify the code structure,
> We decide to share the common structure between them. This bring
> a problem that EASRC accept format directly from devicetree, but
> ASRC accept width from devicetree.
>
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, then driver
> can accept format from devicetree, don't need to convert it to
> format through width.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> Documentation/devicetree/bindings/sound/fsl,asrc.txt | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/sound/fsl,asrc.txt b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> index cb9a25165503..780455cf7f71 100644
> --- a/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> +++ b/Documentation/devicetree/bindings/sound/fsl,asrc.txt
> @@ -51,6 +51,11 @@ Optional properties:
> will be in use as default. Otherwise, the big endian
> mode will be in use for all the device registers.
>
> + - fsl,asrc-format : Defines a mutual sample format used by DPCM Back
> + Ends, which can replace the fsl,asrc-width.
> + The value is SNDRV_PCM_FORMAT_S16_LE, or
> + SNDRV_PCM_FORMAT_S24_LE
I am still holding the concern at the DT binding of this format,
as it uses values from ASoC header file instead of a dt-binding
header file -- not sure if we can do this. Let's wait for Rob's
comments.
^ permalink raw reply
* Re: [PATCH v5 2/7] ASoC: fsl-asoc-card: Support new property fsl,asrc-format
From: Nicolin Chen @ 2020-03-09 21:18 UTC (permalink / raw)
To: Shengjiu Wang
Cc: mark.rutland, devicetree, alsa-devel, timur, Xiubo.Lee,
linuxppc-dev, tiwai, lgirdwood, robh+dt, perex, broonie, festevam,
linux-kernel
In-Reply-To: <266dccc836c11165ad91a301f24fe4f7ad2557be.1583725533.git.shengjiu.wang@nxp.com>
On Mon, Mar 09, 2020 at 11:58:29AM +0800, Shengjiu Wang wrote:
> In order to align with new ESARC, we add new property fsl,asrc-format.
> The fsl,asrc-format can replace the fsl,asrc-width, driver
> can accept format from devicetree, don't need to convert it to
> format through width.
>
> Signed-off-by: Shengjiu Wang <shengjiu.wang@nxp.com>
> ---
> sound/soc/fsl/fsl-asoc-card.c | 20 +++++++++++---------
> 1 file changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/sound/soc/fsl/fsl-asoc-card.c b/sound/soc/fsl/fsl-asoc-card.c
> index 9ce55feaac22..32101b9a37b9 100644
> --- a/sound/soc/fsl/fsl-asoc-card.c
> +++ b/sound/soc/fsl/fsl-asoc-card.c
> @@ -680,17 +680,19 @@ static int fsl_asoc_card_probe(struct platform_device *pdev)
> goto asrc_fail;
> }
>
> - ret = of_property_read_u32(asrc_np, "fsl,asrc-width", &width);
> + ret = of_property_read_u32(asrc_np, "fsl,asrc-format", &priv->asrc_format);
> if (ret) {
> - dev_err(&pdev->dev, "failed to get output rate\n");
Nice that your patch fixed my copy-n-paste typo here :)
> - ret = -EINVAL;
> - goto asrc_fail;
> - }
It'd be nicer to have a line of comments:
/* Fallback to old binding; translate to asrc_format */
> + ret = of_property_read_u32(asrc_np, "fsl,asrc-width", &width);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to get output width\n");
> + return ret;
> + }
>
> - if (width == 24)
> - priv->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
> - else
> - priv->asrc_format = SNDRV_PCM_FORMAT_S16_LE;
> + if (width == 24)
> + priv->asrc_format = SNDRV_PCM_FORMAT_S24_LE;
> + else
> + priv->asrc_format = SNDRV_PCM_FORMAT_S16_LE;
> + }
^ permalink raw reply
* Re: [PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events
From: Arnaldo Melo @ 2020-03-09 11:22 UTC (permalink / raw)
To: Jiri Olsa, Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200309093506.GB67774@krava>
[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]
Sure, will do it today
On March 9, 2020 6:35:06 AM GMT-03:00, Jiri Olsa <jolsa@redhat.com> wrote:
>On Mon, Mar 09, 2020 at 11:55:44AM +0530, Kajol Jain wrote:
>> First patch of the patchset fix inconsistent results we are getting
>when
>> we run multiple 24x7 events.
>>
>> Patchset adds json file metric support for the hv_24x7 socket/chip
>level
>> events. "hv_24x7" pmu interface events needs system dependent
>parameter
>> like socket/chip/core. For example, hv_24x7 chip level events needs
>> specific chip-id to which the data is requested should be added as
>part
>> of pmu events.
>>
>> So to enable JSON file support to "hv_24x7" interface, patchset
>expose
>> total number of sockets and chips per-socket details in sysfs
>> files (sockets, chips) under "/sys/devices/hv_24x7/interface/".
>>
>> To get sockets and number of chips per sockets, patchset adds a rtas
>call
>> with token "PROCESSOR_MODULE_INFO" to get these details. Patchset
>also
>> handles partition migration case to re-init these system depended
>> parameters by adding proper calls in post_mobility_fixup()
>(mobility.c).
>>
>> Patch 6 & 8 of the patchset handles perf tool plumbing needed to
>replace
>> the "?" character in the metric expression to proper value and
>hv_24x7
>> json metric file for different Socket/chip resources.
>>
>> Patch set also enable Hz/hz prinitg for --metric-only option to print
>> metric data for bus frequency.
>>
>> Applied and tested all these patches cleanly on top of jiri's flex
>changes
>> with the changes done by Kan Liang for "Support metric group
>constraint"
>> patchset and made required changes.
>>
>> Changelog:
>> v3 -> v4
>> - Made changes suggested by jiri.
>
>could you please mention them next time? ;-)
>
>> - Apply these patch on top of Kan liang changes.
>
>Arnaldo, could you please pull the expr flex changes and Kan's
>metric group constraint changes? it's both prereq of this patchset
>
>thanks,
>jirka
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
[-- Attachment #2: Type: text/html, Size: 2560 bytes --]
^ permalink raw reply
* Re: [PATCH 8/8] powerpc/papr_scm: Implement support for DSM_PAPR_SCM_HEALTH
From: Aneesh Kumar K.V @ 2020-03-09 10:58 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Vaibhav Jain, Michael Ellerman, Oliver O'Halloran,
Alastair D'Silva
In-Reply-To: <20200220095805.197229-9-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> The DSM 'DSM_PAPR_SCM_HEALTH' should return a 'struct
> nd_papr_scm_dimm_health_stat' containing information in dimm health back
> to user space in response to ND_CMD_CALL. We implement this DSM by
> implementing a new function papr_scm_get_health() that queries the
> DIMM health information and then copies these bitmaps to the package
> payload whose layout is defined by 'struct papr_scm_ndctl_health'.
>
> The patch also handle cases where in future versions of 'struct
> papr_scm_ndctl_health' may want to return more health
> information. Such payload envelops will contain appropriate version
> information in 'struct nd_papr_scm_cmd_pkg.payload_version'. The patch
> takes care of only returning the sub-data corresponding to the payload
> version requested. Please see the comments in papr_scm_get_health()
> for how this is done.
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 73 +++++++++++++++++++++++
> 1 file changed, 73 insertions(+)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 29f38246c59f..bf81acb0bf3f 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -415,6 +415,74 @@ static int cmd_to_func(struct nvdimm *nvdimm, unsigned int cmd, void *buf,
> return pkg->hdr.nd_command;
> }
>
> +/*
> + * Fetch the DIMM health info and populate it in provided papr_scm package.
> + * Since the caller can request a different version of payload and each new
> + * version of struct nd_papr_scm_dimm_health_stat is a proper-subset of
> + * previous version hence we return a subset of the cached 'struct
> + * nd_papr_scm_dimm_health_stat' depending on the payload version requested.
> + */
> +static int papr_scm_get_health(struct papr_scm_priv *p,
> + struct nd_papr_scm_cmd_pkg *pkg)
> +{
> + int rc;
> + size_t copysize;
> + /* Map version to number of bytes to be copied to payload */
> + const size_t copysizes[] = {
> + [1] =
> + sizeof(struct nd_papr_scm_dimm_health_stat_v1),
> +
> + /* This should always be preset */
> + [ND_PAPR_SCM_DIMM_HEALTH_VERSION] =
> + sizeof(struct nd_papr_scm_dimm_health_stat),
> + };
We will not be able to determine that during build. For performance
hcall to run LPAR should be privileged. ie, even if the kernel supports v2
version of the health information, it may only be able to
return v1 version of the health because LPAR performance stat hcall
failed.
-aneesh
^ permalink raw reply
* Re: [PATCH 3/8] powerpc/papr_scm: Fetch dimm performance stats from PHYP
From: Aneesh Kumar K.V @ 2020-03-09 10:28 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Vaibhav Jain, Michael Ellerman, Oliver O'Halloran,
Alastair D'Silva
In-Reply-To: <20200220095805.197229-4-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Implement support for fetching dimm performance metrics via
> H_SCM_PERFORMANCE_HEALTH hcall as documented in Ref[1]. The hcall
> returns a structure as described in Ref[1] and defined as newly
> introduced 'struct papr_scm_perf_stats'. The struct has a header
> followed by key-value pairs of performance attributes. The 'key' part
> is a 8-byte char array naming the attribute encoded as a __be64
> integer. This makes the output buffer format for the hcall self
> describing and can be easily interpreted.
>
> This patch implements functionality to fetch these performance stats
> and reporting them via a nvdimm sysfs attribute named
> 'papr_perf_stats'.
>
> A new function drc_pmem_query_stats() is implemented that issues hcall
> H_SCM_PERFORMANCE_HEALTH ,requesting PHYP to store performance stats
> in pre-allocated 'struct papr_scm_perf_stats' buffer. During nvdimm
> initialization in papr_scm_nvdimm_init() this function is called with
> an empty buffer to know the max buffer size needed for issuing the
> H_SCM_PERFORMANCE_HEALTH hcall. The buffer size retrieved is stored in
> newly introduced 'struct papc_scm_priv.len_stat_buffer' for later
> retrival.
>
You are not using this hcall in the series? If so can you drop it from
the series and add it when you want to use hcall returned value.
> [1]: commit 58b278f568f0 ("powerpc: Provide initial documentation for
> PAPR hcalls")
>
-aneesh
^ permalink raw reply
* Re: [PATCH 7/8] powerpc/papr_scm: Re-implement 'papr_flags' using 'nd_papr_scm_dimm_health_stat'
From: Aneesh Kumar K.V @ 2020-03-09 10:27 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Vaibhav Jain, Michael Ellerman, Oliver O'Halloran,
Alastair D'Silva
In-Reply-To: <20200220095805.197229-8-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Previous commit [1] introduced 'struct nd_papr_scm_dimm_health_stat' for
> communicating health status of an nvdimm to libndctl. This struct
> however can also be used to cache the nvdimm health information in
> 'struct papr_scm_priv' instead of two '__be64' values. Benefit of this
> re-factoring will be apparent when support for libndctl being able to
> request nvdimm health stats is implemented where we can simply memcpy
> this struct over to the user-space provided payload envelope.
>
> Hence this patch introduces a new member 'struct papr_scm_priv.health'
> that caches the health information of a dimm. This member is populated
> inside drc_pmem_query_health() which checks for the various bit flags
> returned from H_SCM_HEALTH and sets them in this struct. We also
> re-factor 'papr_flags' sysfs attribute show function papr_flags_show()
> to use the flags in 'struct papr_scm_priv.health' to return
> appropriate status strings pertaining to dimm health.
>
> This patch shouldn't introduce any behavioral change.
This patch is undoing what is doing in PATCH 2. If you reorder things,
we could drop either this or PATCH2.?
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/platforms/pseries/papr_scm.c | 61 ++++++++++++++++-------
> 1 file changed, 44 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index d5eea2f38fa6..29f38246c59f 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -47,8 +47,7 @@ struct papr_scm_priv {
> struct mutex dimm_mutex;
>
> /* Health information for the dimm */
> - __be64 health_bitmap;
> - __be64 health_bitmap_valid;
> + struct nd_papr_scm_dimm_health_stat health;
>
> /* length of the stat buffer as expected by phyp */
> size_t len_stat_buffer;
> @@ -205,6 +204,7 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
> {
> unsigned long ret[PLPAR_HCALL_BUFSIZE];
> int64_t rc;
> + __be64 health;
>
> rc = plpar_hcall(H_SCM_HEALTH, ret, p->drc_index);
> if (rc != H_SUCCESS) {
> @@ -219,13 +219,41 @@ static int drc_pmem_query_health(struct papr_scm_priv *p)
> return rc;
>
> /* Store the retrieved health information in dimm platform data */
> - p->health_bitmap = ret[0];
> - p->health_bitmap_valid = ret[1];
> + health = ret[0] & ret[1];
>
> dev_dbg(&p->pdev->dev,
> "Queried dimm health info. Bitmap:0x%016llx Mask:0x%016llx\n",
> - be64_to_cpu(p->health_bitmap),
> - be64_to_cpu(p->health_bitmap_valid));
> + be64_to_cpu(ret[0]),
> + be64_to_cpu(ret[1]));
> +
> + memset(&p->health, 0, sizeof(p->health));
> +
> + /* Check for various masks in bitmap and set the buffer */
> + if (health & PAPR_SCM_DIMM_UNARMED_MASK)
> + p->health.dimm_unarmed = true;
> +
> + if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> + p->health.dimm_bad_shutdown = true;
> +
> + if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> + p->health.dimm_bad_restore = true;
> +
> + if (health & PAPR_SCM_DIMM_ENCRYPTED)
> + p->health.dimm_encrypted = true;
> +
> + if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED) {
> + p->health.dimm_locked = true;
> + p->health.dimm_scrubbed = true;
> + }
> +
> + if (health & PAPR_SCM_DIMM_HEALTH_UNHEALTHY)
> + p->health.dimm_health = DSM_PAPR_SCM_DIMM_UNHEALTHY;
> +
> + if (health & PAPR_SCM_DIMM_HEALTH_CRITICAL)
> + p->health.dimm_health = DSM_PAPR_SCM_DIMM_CRITICAL;
> +
> + if (health & PAPR_SCM_DIMM_HEALTH_FATAL)
> + p->health.dimm_health = DSM_PAPR_SCM_DIMM_FATAL;
>
> mutex_unlock(&p->dimm_mutex);
> return 0;
> @@ -513,7 +541,6 @@ static ssize_t papr_flags_show(struct device *dev,
> {
> struct nvdimm *dimm = to_nvdimm(dev);
> struct papr_scm_priv *p = nvdimm_provider_data(dimm);
> - __be64 health;
> int rc;
>
> rc = drc_pmem_query_health(p);
> @@ -525,26 +552,26 @@ static ssize_t papr_flags_show(struct device *dev,
> if (rc)
> return rc;
>
> - health = p->health_bitmap & p->health_bitmap_valid;
> -
> - /* Check for various masks in bitmap and set the buffer */
> - if (health & PAPR_SCM_DIMM_UNARMED_MASK)
> + if (p->health.dimm_unarmed)
> rc += sprintf(buf, "not_armed ");
>
> - if (health & PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK)
> + if (p->health.dimm_bad_shutdown)
> rc += sprintf(buf + rc, "save_fail ");
>
> - if (health & PAPR_SCM_DIMM_BAD_RESTORE_MASK)
> + if (p->health.dimm_bad_restore)
> rc += sprintf(buf + rc, "restore_fail ");
>
> - if (health & PAPR_SCM_DIMM_ENCRYPTED)
> + if (p->health.dimm_encrypted)
> rc += sprintf(buf + rc, "encrypted ");
>
> - if (health & PAPR_SCM_DIMM_SMART_EVENT_MASK)
> + if (p->health.dimm_health)
> rc += sprintf(buf + rc, "smart_notify ");
>
> - if (health & PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED)
> - rc += sprintf(buf + rc, "scrubbed locked ");
> + if (p->health.dimm_scrubbed)
> + rc += sprintf(buf + rc, "scrubbed ");
> +
> + if (p->health.dimm_locked)
> + rc += sprintf(buf + rc, "locked ");
>
> if (rc > 0)
> rc += sprintf(buf + rc, "\n");
> --
> 2.24.1
^ permalink raw reply
* Re: [PATCH 2/8] powerpc/papr_scm: Provide support for fetching dimm health information
From: Aneesh Kumar K.V @ 2020-03-09 10:24 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Vaibhav Jain, Michael Ellerman, Oliver O'Halloran,
Alastair D'Silva
In-Reply-To: <20200220095805.197229-3-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Implement support for fetching dimm health information via
> H_SCM_HEALTH hcall as documented in Ref[1]. The hcall returns a pair of
> 64-bit big-endian integers which are then stored in 'struct
> papr_scm_priv' and subsequently exposed to userspace via dimm
> attribute 'papr_flags'.
Can you add this as part of PATCH 1. While reviewing i had to go back
and forth between 1 and 2 to find different flags. IMHO it would be nice
to introduce new flags and users of the flag together.
>
> 'papr_flags' sysfs attribute reports space separated string flags
> indicating various health state an nvdimm can be. These are:
>
> * "not_armed" : Indicating that nvdimm contents wont survive a power
> cycle.
> * "save_fail" : Indicating that nvdimm contents couldn't be flushed
> during last shutdown event.
> * "restore_fail": Indicating that nvdimm contents couldn't be restored
> during dimm initialization.
> * "encrypted" : Dimm contents are encrypted.
> * "smart_notify": There is health event for the nvdimm.
Can you explain this more? This is suppose to be set when there is a
smart event? Currently you derive this based on
+/* Bit status indicators for smart event notification */
+#define PAPR_SCM_DIMM_SMART_EVENT_MASK (PAPR_SCM_DIMM_HEALTH_CRITICAL | \
+ PAPR_SCM_DIMM_HEALTH_FATAL | \
+ PAPR_SCM_DIMM_HEALTH_UNHEALTHY | \
+
> * "scrubbed" : Indicating that contents of the nvdimm have been
> scrubbed.
> * "locked" : Indicating that nvdimm contents cant be modified
> until next power cycle.
>
> [1]: commit 58b278f568f0 ("powerpc: Provide initial documentation for
> PAPR hcalls")
>
-aneesh
^ permalink raw reply
* Re: [PATCH 1/8] powerpc: Add asm header 'papr_scm.h' describing the papr-scm interface
From: Aneesh Kumar K.V @ 2020-03-09 10:07 UTC (permalink / raw)
To: Vaibhav Jain, linuxppc-dev
Cc: Vaibhav Jain, Michael Ellerman, Oliver O'Halloran,
Alastair D'Silva
In-Reply-To: <20200220095805.197229-2-vaibhav@linux.ibm.com>
Vaibhav Jain <vaibhav@linux.ibm.com> writes:
> Add a new powerpc specific asm header named 'papr-scm.h' that descibes
> the interface between PHYP and guest kernel running as an LPAR.
>
> The HCALLs specific to managing SCM are descibed in Ref[1]. The asm
> header introduced by this patch however describes the data structures
> exchanged between PHYP and kernel during those HCALLs.
>
> Future patches will use these structures to provide support for
> retriving nvdimm health and performance stats in papr_scm kernel
> module.
>
> [1]: commit 58b278f568f0 ("powerpc: Provide initial documentation for
> PAPR hcalls")
>
> Signed-off-by: Vaibhav Jain <vaibhav@linux.ibm.com>
> ---
> arch/powerpc/include/asm/papr_scm.h | 68 +++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
> create mode 100644 arch/powerpc/include/asm/papr_scm.h
>
> diff --git a/arch/powerpc/include/asm/papr_scm.h b/arch/powerpc/include/asm/papr_scm.h
> new file mode 100644
> index 000000000000..d893621063f3
> --- /dev/null
> +++ b/arch/powerpc/include/asm/papr_scm.h
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Structures and defines needed to manage nvdimms for spapr guests.
> + */
> +#ifndef _ASM_POWERPC_PAPR_SCM_H_
> +#define _ASM_POWERPC_PAPR_SCM_H_
> +
> +#include <linux/types.h>
> +#include <asm/bitsperlong.h>
> +#include <linux/stringify.h>
> +
> +/* DIMM health bitmap bitmap indicators */
> +/* SCM device is unable to persist memory contents */
> +#define PAPR_SCM_DIMM_UNARMED PPC_BIT(0)
> +/* SCM device failed to persist memory contents */
> +#define PAPR_SCM_DIMM_SHUTDOWN_DIRTY PPC_BIT(1)
> +/* SCM device contents are persisted from previous IPL */
> +#define PAPR_SCM_DIMM_SHUTDOWN_CLEAN PPC_BIT(2)
> +/* SCM device contents are not persisted from previous IPL */
> +#define PAPR_SCM_DIMM_EMPTY PPC_BIT(3)
> +/* SCM device memory life remaining is critically low */
> +#define PAPR_SCM_DIMM_HEALTH_CRITICAL PPC_BIT(4)
> +/* SCM device will be garded off next IPL due to failure */
> +#define PAPR_SCM_DIMM_HEALTH_FATAL PPC_BIT(5)
> +/* SCM contents cannot persist due to current platform health status */
> +#define PAPR_SCM_DIMM_HEALTH_UNHEALTHY PPC_BIT(6)
> +/* SCM device is unable to persist memory contents in certain conditions */
> +#define PAPR_SCM_DIMM_HEALTH_NON_CRITICAL PPC_BIT(7)
That is not really a bad health condition. That is an indication of
vpmem where we won't persist memory contents on CEC reboot.
> +/* SCM device is encrypted */
> +#define PAPR_SCM_DIMM_ENCRYPTED PPC_BIT(8)
> +/* SCM device has been scrubbed and locked */
> +#define PAPR_SCM_DIMM_SCRUBBED_AND_LOCKED PPC_BIT(9)
> +
> +/* Bits status indicators for health bitmap indicating unarmed dimm */
> +#define PAPR_SCM_DIMM_UNARMED_MASK (PAPR_SCM_DIMM_UNARMED | \
> + PAPR_SCM_DIMM_HEALTH_UNHEALTHY | \
> + PAPR_SCM_DIMM_HEALTH_NON_CRITICAL)
Based on the above, I guess you should remove PAPR_SCM_DIMM_HEALTH_NON_CRITICAL
from the above?
> +
> +/* Bits status indicators for health bitmap indicating unflushed dimm */
> +#define PAPR_SCM_DIMM_BAD_SHUTDOWN_MASK (PAPR_SCM_DIMM_SHUTDOWN_DIRTY)
> +
> +/* Bits status indicators for health bitmap indicating unrestored dimm */
> +#define PAPR_SCM_DIMM_BAD_RESTORE_MASK (PAPR_SCM_DIMM_EMPTY)
> +
> +/* Bit status indicators for smart event notification */
> +#define PAPR_SCM_DIMM_SMART_EVENT_MASK (PAPR_SCM_DIMM_HEALTH_CRITICAL | \
> + PAPR_SCM_DIMM_HEALTH_FATAL | \
> + PAPR_SCM_DIMM_HEALTH_UNHEALTHY | \
> + PAPR_SCM_DIMM_HEALTH_NON_CRITICAL)
> +
-aneesh
^ permalink raw reply
* Re: [PATCH v4 0/8] powerpc/perf: Add json file metric support for the hv_24x7 socket/chip level events
From: Jiri Olsa @ 2020-03-09 9:35 UTC (permalink / raw)
To: Kajol Jain
Cc: mark.rutland, maddy, peterz, yao.jin, mingo, kan.liang, ak,
alexander.shishkin, anju, mamatha4, sukadev, ravi.bangoria, acme,
jmario, namhyung, tglx, mpetlan, gregkh, linux-kernel,
linux-perf-users, jolsa, linuxppc-dev
In-Reply-To: <20200309062552.29911-1-kjain@linux.ibm.com>
On Mon, Mar 09, 2020 at 11:55:44AM +0530, Kajol Jain wrote:
> First patch of the patchset fix inconsistent results we are getting when
> we run multiple 24x7 events.
>
> Patchset adds json file metric support for the hv_24x7 socket/chip level
> events. "hv_24x7" pmu interface events needs system dependent parameter
> like socket/chip/core. For example, hv_24x7 chip level events needs
> specific chip-id to which the data is requested should be added as part
> of pmu events.
>
> So to enable JSON file support to "hv_24x7" interface, patchset expose
> total number of sockets and chips per-socket details in sysfs
> files (sockets, chips) under "/sys/devices/hv_24x7/interface/".
>
> To get sockets and number of chips per sockets, patchset adds a rtas call
> with token "PROCESSOR_MODULE_INFO" to get these details. Patchset also
> handles partition migration case to re-init these system depended
> parameters by adding proper calls in post_mobility_fixup() (mobility.c).
>
> Patch 6 & 8 of the patchset handles perf tool plumbing needed to replace
> the "?" character in the metric expression to proper value and hv_24x7
> json metric file for different Socket/chip resources.
>
> Patch set also enable Hz/hz prinitg for --metric-only option to print
> metric data for bus frequency.
>
> Applied and tested all these patches cleanly on top of jiri's flex changes
> with the changes done by Kan Liang for "Support metric group constraint"
> patchset and made required changes.
>
> Changelog:
> v3 -> v4
> - Made changes suggested by jiri.
could you please mention them next time? ;-)
> - Apply these patch on top of Kan liang changes.
Arnaldo, could you please pull the expr flex changes and Kan's
metric group constraint changes? it's both prereq of this patchset
thanks,
jirka
^ permalink raw reply
* [PATCH 15/15] powerpc/watchpoint/xmon: Support 2nd dawr
From: Ravi Bangoria @ 2020-03-09 8:58 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, Ravi Bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200309085806.155823-1-ravi.bangoria@linux.ibm.com>
Add support for 2nd DAWR in xmon. With this, we can have two
simultaneous breakpoints from xmon.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/xmon/xmon.c | 101 ++++++++++++++++++++++++++-------------
1 file changed, 69 insertions(+), 32 deletions(-)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index ac18fe3e4295..20adc83404c8 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -110,7 +110,7 @@ struct bpt {
#define NBPTS 256
static struct bpt bpts[NBPTS];
-static struct bpt dabr;
+static struct bpt dabr[HBP_NUM_MAX];
static struct bpt *iabr;
static unsigned bpinstr = 0x7fe00008; /* trap */
@@ -786,10 +786,17 @@ static int xmon_sstep(struct pt_regs *regs)
static int xmon_break_match(struct pt_regs *regs)
{
+ int i;
+
if ((regs->msr & (MSR_IR|MSR_PR|MSR_64BIT)) != (MSR_IR|MSR_64BIT))
return 0;
- if (dabr.enabled == 0)
- return 0;
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (dabr[i].enabled)
+ goto found;
+ }
+ return 0;
+
+found:
xmon_core(regs, 0);
return 1;
}
@@ -928,13 +935,16 @@ static void insert_bpts(void)
static void insert_cpu_bpts(void)
{
+ int i;
struct arch_hw_breakpoint brk;
- if (dabr.enabled) {
- brk.address = dabr.address;
- brk.type = (dabr.enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
- brk.len = DABR_MAX_LEN;
- __set_breakpoint(&brk, 0);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (dabr[i].enabled) {
+ brk.address = dabr[i].address;
+ brk.type = (dabr[i].enabled & HW_BRK_TYPE_DABR) | HW_BRK_TYPE_PRIV_ALL;
+ brk.len = 8;
+ __set_breakpoint(&brk, i);
+ }
}
if (iabr)
@@ -1348,6 +1358,35 @@ static long check_bp_loc(unsigned long addr)
return 1;
}
+static int free_data_bpt(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!dabr[i].enabled)
+ return i;
+ }
+ printf("Couldn't find free breakpoint register\n");
+ return -1;
+}
+
+static void print_data_bpts(void)
+{
+ int i;
+
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!dabr[i].enabled)
+ continue;
+
+ printf(" data "REG" [", dabr[i].address);
+ if (dabr[i].enabled & 1)
+ printf("r");
+ if (dabr[i].enabled & 2)
+ printf("w");
+ printf("]\n");
+ }
+}
+
static char *breakpoint_help_string =
"Breakpoint command usage:\n"
"b show breakpoints\n"
@@ -1381,10 +1420,9 @@ bpt_cmds(void)
printf("Hardware data breakpoint not supported on this cpu\n");
break;
}
- if (dabr.enabled) {
- printf("Couldn't find free breakpoint register\n");
+ i = free_data_bpt();
+ if (i < 0)
break;
- }
mode = 7;
cmd = inchar();
if (cmd == 'r')
@@ -1393,15 +1431,15 @@ bpt_cmds(void)
mode = 6;
else
termch = cmd;
- dabr.address = 0;
- dabr.enabled = 0;
- if (scanhex(&dabr.address)) {
- if (!is_kernel_addr(dabr.address)) {
+ dabr[i].address = 0;
+ dabr[i].enabled = 0;
+ if (scanhex(&dabr[i].address)) {
+ if (!is_kernel_addr(dabr[i].address)) {
printf(badaddr);
break;
}
- dabr.address &= ~HW_BRK_TYPE_DABR;
- dabr.enabled = mode | BP_DABR;
+ dabr[i].address &= ~HW_BRK_TYPE_DABR;
+ dabr[i].enabled = mode | BP_DABR;
}
force_enable_xmon();
@@ -1440,7 +1478,9 @@ bpt_cmds(void)
for (i = 0; i < NBPTS; ++i)
bpts[i].enabled = 0;
iabr = NULL;
- dabr.enabled = 0;
+ for (i = 0; i < nr_wp_slots(); i++)
+ dabr[i].enabled = 0;
+
printf("All breakpoints cleared\n");
break;
}
@@ -1474,14 +1514,7 @@ bpt_cmds(void)
if (xmon_is_ro || !scanhex(&a)) {
/* print all breakpoints */
printf(" type address\n");
- if (dabr.enabled) {
- printf(" data "REG" [", dabr.address);
- if (dabr.enabled & 1)
- printf("r");
- if (dabr.enabled & 2)
- printf("w");
- printf("]\n");
- }
+ print_data_bpts();
for (bp = bpts; bp < &bpts[NBPTS]; ++bp) {
if (!bp->enabled)
continue;
@@ -1941,8 +1974,13 @@ static void dump_207_sprs(void)
printf("hfscr = %.16lx dhdes = %.16lx rpr = %.16lx\n",
mfspr(SPRN_HFSCR), mfspr(SPRN_DHDES), mfspr(SPRN_RPR));
- printf("dawr = %.16lx dawrx = %.16lx ciabr = %.16lx\n",
- mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0), mfspr(SPRN_CIABR));
+ printf("dawr0 = %.16lx dawrx0 = %.16lx\n",
+ mfspr(SPRN_DAWR0), mfspr(SPRN_DAWRX0));
+ if (nr_wp_slots() > 1) {
+ printf("dawr1 = %.16lx dawrx1 = %.16lx\n",
+ mfspr(SPRN_DAWR1), mfspr(SPRN_DAWRX1));
+ }
+ printf("ciabr = %.16lx\n", mfspr(SPRN_CIABR));
#endif
}
@@ -3862,10 +3900,9 @@ static void clear_all_bpt(void)
bpts[i].enabled = 0;
/* Clear any data or iabr breakpoints */
- if (iabr || dabr.enabled) {
- iabr = NULL;
- dabr.enabled = 0;
- }
+ iabr = NULL;
+ for (i = 0; i < nr_wp_slots(); i++)
+ dabr[i].enabled = 0;
}
#ifdef CONFIG_DEBUG_FS
--
2.21.1
^ permalink raw reply related
* [PATCH 14/15] powerpc/watchpoint/xmon: Don't allow breakpoint overwriting
From: Ravi Bangoria @ 2020-03-09 8:58 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, Ravi Bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200309085806.155823-1-ravi.bangoria@linux.ibm.com>
Xmon allows overwriting breakpoints because it's supported by only
one dawr. But with multiple dawrs, overwriting becomes ambiguous
or unnecessary complicated. So let's not allow it.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/xmon/xmon.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/powerpc/xmon/xmon.c b/arch/powerpc/xmon/xmon.c
index 0ca0d29f99c6..ac18fe3e4295 100644
--- a/arch/powerpc/xmon/xmon.c
+++ b/arch/powerpc/xmon/xmon.c
@@ -1381,6 +1381,10 @@ bpt_cmds(void)
printf("Hardware data breakpoint not supported on this cpu\n");
break;
}
+ if (dabr.enabled) {
+ printf("Couldn't find free breakpoint register\n");
+ break;
+ }
mode = 7;
cmd = inchar();
if (cmd == 'r')
--
2.21.1
^ permalink raw reply related
* [PATCH 13/15] powerpc/watchpoint: Don't allow concurrent perf and ptrace events
From: Ravi Bangoria @ 2020-03-09 8:58 UTC (permalink / raw)
To: mpe, mikey
Cc: apopple, Ravi Bangoria, peterz, fweisbec, oleg, npiggin,
linux-kernel, paulus, jolsa, naveen.n.rao, linuxppc-dev, mingo
In-Reply-To: <20200309085806.155823-1-ravi.bangoria@linux.ibm.com>
ptrace and perf watchpoints on powerpc behaves differently. Ptrace
watchpoint works in one-shot mode and generates signal before executing
instruction. It's ptrace user's job to single-step the instruction and
re-enable the watchpoint. OTOH, in case of perf watchpoint, kernel
emulates/single-steps the instruction and then generates event. If perf
and ptrace creates two events with same or overlapping address ranges,
it's ambiguous to decide who should single-step the instruction. Because
of this issue ptrace and perf event can't coexist when the address range
overlaps.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
---
arch/powerpc/include/asm/hw_breakpoint.h | 2 +
arch/powerpc/kernel/hw_breakpoint.c | 220 +++++++++++++++++++++++
kernel/events/hw_breakpoint.c | 16 ++
3 files changed, 238 insertions(+)
diff --git a/arch/powerpc/include/asm/hw_breakpoint.h b/arch/powerpc/include/asm/hw_breakpoint.h
index ec61e2b7195c..6e1a19af5177 100644
--- a/arch/powerpc/include/asm/hw_breakpoint.h
+++ b/arch/powerpc/include/asm/hw_breakpoint.h
@@ -66,6 +66,8 @@ extern int hw_breakpoint_exceptions_notify(struct notifier_block *unused,
unsigned long val, void *data);
int arch_install_hw_breakpoint(struct perf_event *bp);
void arch_uninstall_hw_breakpoint(struct perf_event *bp);
+int arch_reserve_bp_slot(struct perf_event *bp);
+void arch_release_bp_slot(struct perf_event *bp);
void arch_unregister_hw_breakpoint(struct perf_event *bp);
void hw_breakpoint_pmu_read(struct perf_event *bp);
extern void flush_ptrace_hw_breakpoint(struct task_struct *tsk);
diff --git a/arch/powerpc/kernel/hw_breakpoint.c b/arch/powerpc/kernel/hw_breakpoint.c
index 2ac89b92590f..d8529d9151e8 100644
--- a/arch/powerpc/kernel/hw_breakpoint.c
+++ b/arch/powerpc/kernel/hw_breakpoint.c
@@ -123,6 +123,226 @@ static bool is_ptrace_bp(struct perf_event *bp)
return (bp->overflow_handler == ptrace_triggered);
}
+struct breakpoint {
+ struct list_head list;
+ struct perf_event *bp;
+ bool ptrace_bp;
+};
+
+static DEFINE_PER_CPU(struct breakpoint *, cpu_bps[HBP_NUM_MAX]);
+static LIST_HEAD(task_bps);
+
+static struct breakpoint *alloc_breakpoint(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ tmp = kzalloc(sizeof(*tmp), GFP_KERNEL);
+ if (!tmp)
+ return ERR_PTR(-ENOMEM);
+ tmp->bp = bp;
+ tmp->ptrace_bp = is_ptrace_bp(bp);
+ return tmp;
+}
+
+static bool bp_addr_range_overlap(struct perf_event *bp1, struct perf_event *bp2)
+{
+ __u64 bp1_saddr, bp1_eaddr, bp2_saddr, bp2_eaddr;
+
+ bp1_saddr = bp1->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
+ bp1_eaddr = (bp1->attr.bp_addr + bp1->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
+ bp2_saddr = bp2->attr.bp_addr & ~HW_BREAKPOINT_ALIGN;
+ bp2_eaddr = (bp2->attr.bp_addr + bp2->attr.bp_len - 1) | HW_BREAKPOINT_ALIGN;
+
+ return (bp1_saddr <= bp2_eaddr && bp1_eaddr >= bp2_saddr);
+}
+
+static bool alternate_infra_bp(struct breakpoint *b, struct perf_event *bp)
+{
+ return is_ptrace_bp(bp) ? !b->ptrace_bp : b->ptrace_bp;
+}
+
+static bool can_co_exist(struct breakpoint *b, struct perf_event *bp)
+{
+ return !(alternate_infra_bp(b, bp) && bp_addr_range_overlap(b->bp, bp));
+}
+
+static int task_bps_add(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ tmp = alloc_breakpoint(bp);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ list_add(&tmp->list, &task_bps);
+ return 0;
+}
+
+static void task_bps_remove(struct perf_event *bp)
+{
+ struct list_head *pos, *q;
+ struct breakpoint *tmp;
+
+ list_for_each_safe(pos, q, &task_bps) {
+ tmp = list_entry(pos, struct breakpoint, list);
+
+ if (tmp->bp == bp) {
+ list_del(&tmp->list);
+ kfree(tmp);
+ break;
+ }
+ }
+}
+
+/*
+ * If any task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool all_task_bps_check(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ list_for_each_entry(tmp, &task_bps, list) {
+ if (!can_co_exist(tmp, bp))
+ return true;
+ }
+ return false;
+}
+
+/*
+ * If same task has breakpoint from alternate infrastructure,
+ * return true. Otherwise return false.
+ */
+static bool same_task_bps_check(struct perf_event *bp)
+{
+ struct breakpoint *tmp;
+
+ list_for_each_entry(tmp, &task_bps, list) {
+ if (tmp->bp->hw.target == bp->hw.target &&
+ !can_co_exist(tmp, bp))
+ return true;
+ }
+ return false;
+}
+
+static int cpu_bps_add(struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ struct breakpoint *tmp;
+ int i = 0;
+
+ tmp = alloc_breakpoint(bp);
+ if (IS_ERR(tmp))
+ return PTR_ERR(tmp);
+
+ cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!cpu_bp[i]) {
+ cpu_bp[i] = tmp;
+ break;
+ }
+ }
+ return 0;
+}
+
+static void cpu_bps_remove(struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ int i = 0;
+
+ cpu_bp = per_cpu_ptr(cpu_bps, bp->cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (!cpu_bp[i])
+ continue;
+
+ if (cpu_bp[i]->bp == bp) {
+ kfree(cpu_bp[i]);
+ cpu_bp[i] = NULL;
+ break;
+ }
+ }
+}
+
+static bool cpu_bps_check(int cpu, struct perf_event *bp)
+{
+ struct breakpoint **cpu_bp;
+ int i;
+
+ cpu_bp = per_cpu_ptr(cpu_bps, cpu);
+ for (i = 0; i < nr_wp_slots(); i++) {
+ if (cpu_bp[i] && !can_co_exist(cpu_bp[i], bp))
+ return true;
+ }
+ return false;
+}
+
+static bool all_cpu_bps_check(struct perf_event *bp)
+{
+ int cpu;
+
+ for_each_online_cpu(cpu) {
+ if (cpu_bps_check(cpu, bp))
+ return true;
+ }
+ return false;
+}
+
+/*
+ * We don't use any locks to serialize accesses to cpu_bps or task_bps
+ * because are already inside nr_bp_mutex.
+ */
+int arch_reserve_bp_slot(struct perf_event *bp)
+{
+ int ret;
+
+ if (is_ptrace_bp(bp)) {
+ if (all_cpu_bps_check(bp))
+ return -ENOSPC;
+
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ return task_bps_add(bp);
+ } else {
+ if (is_kernel_addr(bp->attr.bp_addr))
+ return 0;
+
+ if (bp->hw.target && bp->cpu == -1) {
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ return task_bps_add(bp);
+ } else if (!bp->hw.target && bp->cpu != -1) {
+ if (all_task_bps_check(bp))
+ return -ENOSPC;
+
+ return cpu_bps_add(bp);
+ } else {
+ if (same_task_bps_check(bp))
+ return -ENOSPC;
+
+ ret = cpu_bps_add(bp);
+ if (ret)
+ return ret;
+ ret = task_bps_add(bp);
+ if (ret)
+ cpu_bps_remove(bp);
+
+ return ret;
+ }
+ }
+}
+
+void arch_release_bp_slot(struct perf_event *bp)
+{
+ if (!is_kernel_addr(bp->attr.bp_addr)) {
+ if (bp->hw.target)
+ task_bps_remove(bp);
+ if (bp->cpu != -1)
+ cpu_bps_remove(bp);
+ }
+}
+
/*
* Perform cleanup of arch-specific counters during unregistration
* of the perf-event
diff --git a/kernel/events/hw_breakpoint.c b/kernel/events/hw_breakpoint.c
index 3cc8416ec844..b48d7039a015 100644
--- a/kernel/events/hw_breakpoint.c
+++ b/kernel/events/hw_breakpoint.c
@@ -213,6 +213,15 @@ toggle_bp_slot(struct perf_event *bp, bool enable, enum bp_type_idx type,
list_del(&bp->hw.bp_list);
}
+__weak int arch_reserve_bp_slot(struct perf_event *bp)
+{
+ return 0;
+}
+
+__weak void arch_release_bp_slot(struct perf_event *bp)
+{
+}
+
/*
* Function to perform processor-specific cleanup during unregistration
*/
@@ -270,6 +279,7 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
struct bp_busy_slots slots = {0};
enum bp_type_idx type;
int weight;
+ int ret;
/* We couldn't initialize breakpoint constraints on boot */
if (!constraints_initialized)
@@ -294,6 +304,10 @@ static int __reserve_bp_slot(struct perf_event *bp, u64 bp_type)
if (slots.pinned + (!!slots.flexible) > nr_slots[type])
return -ENOSPC;
+ ret = arch_reserve_bp_slot(bp);
+ if (ret)
+ return ret;
+
toggle_bp_slot(bp, true, type, weight);
return 0;
@@ -317,6 +331,8 @@ static void __release_bp_slot(struct perf_event *bp, u64 bp_type)
enum bp_type_idx type;
int weight;
+ arch_release_bp_slot(bp);
+
type = find_slot_idx(bp_type);
weight = hw_breakpoint_weight(bp);
toggle_bp_slot(bp, false, type, weight);
--
2.21.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