* [PATCH v4 2/2] powerpc/powernv: Re-enable imc trace-mode in kernel
From: Anju T Sudhakar @ 2020-03-13 5:52 UTC (permalink / raw)
To: mpe; +Cc: harihare, maddy, linuxppc-dev, anju
In-Reply-To: <20200313055238.8656-1-anju@linux.vnet.ibm.com>
commit <249fad734a25> ""powerpc/perf: Disable trace_imc pmu"
disables IMC(In-Memory Collection) trace-mode in kernel, since frequent
mode switching between accumulation mode and trace mode via the spr LDBAR
in the hardware can trigger a checkstop(system crash).
Patch to re-enable imc-trace mode in kernel.
The previous patch(1/2) in this series will address the mode switching issue
by implementing a global lock, and will restrict the usage of
accumulation and trace-mode at a time.
Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
---
arch/powerpc/platforms/powernv/opal-imc.c | 9 +--------
1 file changed, 1 insertion(+), 8 deletions(-)
diff --git a/arch/powerpc/platforms/powernv/opal-imc.c b/arch/powerpc/platforms/powernv/opal-imc.c
index 968b9a4d1cd9..7824cc364bc4 100644
--- a/arch/powerpc/platforms/powernv/opal-imc.c
+++ b/arch/powerpc/platforms/powernv/opal-imc.c
@@ -268,14 +268,7 @@ static int opal_imc_counters_probe(struct platform_device *pdev)
domain = IMC_DOMAIN_THREAD;
break;
case IMC_TYPE_TRACE:
- /*
- * FIXME. Using trace_imc events to monitor application
- * or KVM thread performance can cause a checkstop
- * (system crash).
- * Disable it for now.
- */
- pr_info_once("IMC: disabling trace_imc PMU\n");
- domain = -1;
+ domain = IMC_DOMAIN_TRACE;
break;
default:
pr_warn("IMC Unknown Device type \n");
--
2.20.1
^ permalink raw reply related
* [PATCH v4 1/2] powerpc/perf: Implement a global lock to avoid races between trace, core and thread imc events.
From: Anju T Sudhakar @ 2020-03-13 5:52 UTC (permalink / raw)
To: mpe; +Cc: harihare, maddy, linuxppc-dev, anju
IMC(In-memory Collection Counters) does performance monitoring in
two different modes, i.e accumulation mode(core-imc and thread-imc events),
and trace mode(trace-imc events). A cpu thread can either be in
accumulation-mode or trace-mode at a time and this is done via the LDBAR
register in POWER architecture. The current design does not address the
races between thread-imc and trace-imc events.
Patch implements a global id and lock to avoid the races between
core, trace and thread imc events. With this global id-lock
implementation, the system can either run core, thread or trace imc
events at a time. i.e. to run any core-imc events, thread/trace imc events
should not be enabled/monitored.
Signed-off-by: Anju T Sudhakar <anju@linux.vnet.ibm.com>
---
Changes from v3->v4:
- Added mutex lock for thread, core and trace imc cpu offline path.
Changes from v2->v3:
- Addressed the off-line comments from Michael Ellerman
- Optimized the *_event_init code path for trace, core and thread imc
- Handled the global refc in cpuhotplug scenario
- Re-order the patch series
- Removed the selftest patches and will send as a follow up patch
Changes from v1 -> v2:
- Added self test patches to the series.
---
arch/powerpc/perf/imc-pmu.c | 173 +++++++++++++++++++++++++++++++-----
1 file changed, 149 insertions(+), 24 deletions(-)
diff --git a/arch/powerpc/perf/imc-pmu.c b/arch/powerpc/perf/imc-pmu.c
index cb50a9e1fd2d..eb82dda884e5 100644
--- a/arch/powerpc/perf/imc-pmu.c
+++ b/arch/powerpc/perf/imc-pmu.c
@@ -44,6 +44,16 @@ static DEFINE_PER_CPU(u64 *, trace_imc_mem);
static struct imc_pmu_ref *trace_imc_refc;
static int trace_imc_mem_size;
+/*
+ * Global data structure used to avoid races between thread,
+ * core and trace-imc
+ */
+static struct imc_pmu_ref imc_global_refc = {
+ .lock = __MUTEX_INITIALIZER(imc_global_refc.lock),
+ .id = 0,
+ .refc = 0,
+};
+
static struct imc_pmu *imc_event_to_pmu(struct perf_event *event)
{
return container_of(event->pmu, struct imc_pmu, pmu);
@@ -698,6 +708,16 @@ static int ppc_core_imc_cpu_offline(unsigned int cpu)
return -EINVAL;
ref->refc = 0;
+ /*
+ * Reduce the global reference count, if this is the
+ * last cpu in this core and core-imc event running
+ * in this cpu.
+ */
+ mutex_lock(&imc_global_refc.lock);
+ if (imc_global_refc.id == IMC_DOMAIN_CORE)
+ imc_global_refc.refc--;
+
+ mutex_unlock(&imc_global_refc.lock);
}
return 0;
}
@@ -710,6 +730,23 @@ static int core_imc_pmu_cpumask_init(void)
ppc_core_imc_cpu_offline);
}
+static void reset_global_refc(struct perf_event *event)
+{
+ mutex_lock(&imc_global_refc.lock);
+ imc_global_refc.refc--;
+
+ /*
+ * If no other thread is running any
+ * event for this domain(thread/core/trace),
+ * set the global id to zero.
+ */
+ if (imc_global_refc.refc <= 0) {
+ imc_global_refc.refc = 0;
+ imc_global_refc.id = 0;
+ }
+ mutex_unlock(&imc_global_refc.lock);
+}
+
static void core_imc_counters_release(struct perf_event *event)
{
int rc, core_id;
@@ -759,6 +796,8 @@ static void core_imc_counters_release(struct perf_event *event)
ref->refc = 0;
}
mutex_unlock(&ref->lock);
+
+ reset_global_refc(event);
}
static int core_imc_event_init(struct perf_event *event)
@@ -819,6 +858,29 @@ static int core_imc_event_init(struct perf_event *event)
++ref->refc;
mutex_unlock(&ref->lock);
+ /*
+ * Since the system can run either in accumulation or trace-mode
+ * of IMC at a time, core-imc events are allowed only if no other
+ * trace/thread imc events are enabled/monitored.
+ *
+ * Take the global lock, and check the refc.id
+ * to know whether any other trace/thread imc
+ * events are running.
+ */
+ mutex_lock(&imc_global_refc.lock);
+ if (imc_global_refc.id == 0 || imc_global_refc.id == IMC_DOMAIN_CORE) {
+ /*
+ * No other trace/thread imc events are running in
+ * the system, so set the refc.id to core-imc.
+ */
+ imc_global_refc.id = IMC_DOMAIN_CORE;
+ imc_global_refc.refc++;
+ } else {
+ mutex_unlock(&imc_global_refc.lock);
+ return -EBUSY;
+ }
+ mutex_unlock(&imc_global_refc.lock);
+
event->hw.event_base = (u64)pcmi->vbase + (config & IMC_EVENT_OFFSET_MASK);
event->destroy = core_imc_counters_release;
return 0;
@@ -877,7 +939,23 @@ static int ppc_thread_imc_cpu_online(unsigned int cpu)
static int ppc_thread_imc_cpu_offline(unsigned int cpu)
{
- mtspr(SPRN_LDBAR, 0);
+ /*
+ * Set the bit 0 of LDBAR to zero.
+ *
+ * If bit 0 of LDBAR is unset, it will stop posting
+ * the counter data to memory.
+ * For thread-imc, bit 0 of LDBAR will be set to 1 in the
+ * event_add function. So reset this bit here, to stop the updates
+ * to memory in the cpu_offline path.
+ */
+ mtspr(SPRN_LDBAR, (mfspr(SPRN_LDBAR) & (~(1UL << 63))));
+
+ /* Reduce the refc if thread-imc event running on this cpu */
+ mutex_lock(&imc_global_refc.lock);
+ if (imc_global_refc.id == IMC_DOMAIN_THREAD)
+ imc_global_refc.refc--;
+ mutex_unlock(&imc_global_refc.lock);
+
return 0;
}
@@ -916,7 +994,22 @@ static int thread_imc_event_init(struct perf_event *event)
if (!target)
return -EINVAL;
+ mutex_lock(&imc_global_refc.lock);
+ /*
+ * Check if any other trace/core imc events are running in the
+ * system, if not set the global id to thread-imc.
+ */
+ if (imc_global_refc.id == 0 || imc_global_refc.id == IMC_DOMAIN_THREAD) {
+ imc_global_refc.id = IMC_DOMAIN_THREAD;
+ imc_global_refc.refc++;
+ } else {
+ mutex_unlock(&imc_global_refc.lock);
+ return -EBUSY;
+ }
+ mutex_unlock(&imc_global_refc.lock);
+
event->pmu->task_ctx_nr = perf_sw_context;
+ event->destroy = reset_global_refc;
return 0;
}
@@ -1063,10 +1156,12 @@ static void thread_imc_event_del(struct perf_event *event, int flags)
int core_id;
struct imc_pmu_ref *ref;
- mtspr(SPRN_LDBAR, 0);
-
core_id = smp_processor_id() / threads_per_core;
ref = &core_imc_refc[core_id];
+ if (!ref) {
+ pr_debug("imc: Failed to get event reference count\n");
+ return;
+ }
mutex_lock(&ref->lock);
ref->refc--;
@@ -1082,6 +1177,10 @@ static void thread_imc_event_del(struct perf_event *event, int flags)
ref->refc = 0;
}
mutex_unlock(&ref->lock);
+
+ /* Set bit 0 of LDBAR to zero, to stop posting updates to memory */
+ mtspr(SPRN_LDBAR, (mfspr(SPRN_LDBAR) & (~(1UL << 63))));
+
/*
* Take a snapshot and calculate the delta and update
* the event counter values.
@@ -1133,7 +1232,18 @@ static int ppc_trace_imc_cpu_online(unsigned int cpu)
static int ppc_trace_imc_cpu_offline(unsigned int cpu)
{
- mtspr(SPRN_LDBAR, 0);
+ /*
+ * No need to set bit 0 of LDBAR to zero, as
+ * it is set to zero for imc trace-mode
+ *
+ * Reduce the refc if any trace-imc event running
+ * on this cpu.
+ */
+ mutex_lock(&imc_global_refc.lock);
+ if (imc_global_refc.id == IMC_DOMAIN_TRACE)
+ imc_global_refc.refc--;
+ mutex_unlock(&imc_global_refc.lock);
+
return 0;
}
@@ -1226,15 +1336,14 @@ static int trace_imc_event_add(struct perf_event *event, int flags)
local_mem = get_trace_imc_event_base_addr();
ldbar_value = ((u64)local_mem & THREAD_IMC_LDBAR_MASK) | TRACE_IMC_ENABLE;
- if (core_imc_refc)
- ref = &core_imc_refc[core_id];
+ /* trace-imc reference count */
+ if (trace_imc_refc)
+ ref = &trace_imc_refc[core_id];
if (!ref) {
- /* If core-imc is not enabled, use trace-imc reference count */
- if (trace_imc_refc)
- ref = &trace_imc_refc[core_id];
- if (!ref)
- return -EINVAL;
+ pr_debug("imc: Failed to get the event reference count\n");
+ return -EINVAL;
}
+
mtspr(SPRN_LDBAR, ldbar_value);
mutex_lock(&ref->lock);
if (ref->refc == 0) {
@@ -1242,13 +1351,11 @@ static int trace_imc_event_add(struct perf_event *event, int flags)
get_hard_smp_processor_id(smp_processor_id()))) {
mutex_unlock(&ref->lock);
pr_err("trace-imc: Unable to start the counters for core %d\n", core_id);
- mtspr(SPRN_LDBAR, 0);
return -EINVAL;
}
}
++ref->refc;
mutex_unlock(&ref->lock);
-
return 0;
}
@@ -1274,16 +1381,13 @@ static void trace_imc_event_del(struct perf_event *event, int flags)
int core_id = smp_processor_id() / threads_per_core;
struct imc_pmu_ref *ref = NULL;
- if (core_imc_refc)
- ref = &core_imc_refc[core_id];
+ if (trace_imc_refc)
+ ref = &trace_imc_refc[core_id];
if (!ref) {
- /* If core-imc is not enabled, use trace-imc reference count */
- if (trace_imc_refc)
- ref = &trace_imc_refc[core_id];
- if (!ref)
- return;
+ pr_debug("imc: Failed to get event reference count\n");
+ return;
}
- mtspr(SPRN_LDBAR, 0);
+
mutex_lock(&ref->lock);
ref->refc--;
if (ref->refc == 0) {
@@ -1297,6 +1401,7 @@ static void trace_imc_event_del(struct perf_event *event, int flags)
ref->refc = 0;
}
mutex_unlock(&ref->lock);
+
trace_imc_event_stop(event, flags);
}
@@ -1314,10 +1419,30 @@ static int trace_imc_event_init(struct perf_event *event)
if (event->attr.sample_period == 0)
return -ENOENT;
+ /*
+ * Take the global lock, and make sure
+ * no other thread is running any core/thread imc
+ * events
+ */
+ mutex_lock(&imc_global_refc.lock);
+ if (imc_global_refc.id == 0 || imc_global_refc.id == IMC_DOMAIN_TRACE) {
+ /*
+ * No core/thread imc events are running in the
+ * system, so set the refc.id to trace-imc.
+ */
+ imc_global_refc.id = IMC_DOMAIN_TRACE;
+ imc_global_refc.refc++;
+ } else {
+ mutex_unlock(&imc_global_refc.lock);
+ return -EBUSY;
+ }
+ mutex_unlock(&imc_global_refc.lock);
+
event->hw.idx = -1;
target = event->hw.target;
event->pmu->task_ctx_nr = perf_hw_context;
+ event->destroy = reset_global_refc;
return 0;
}
@@ -1429,10 +1554,10 @@ static void cleanup_all_core_imc_memory(void)
static void thread_imc_ldbar_disable(void *dummy)
{
/*
- * By Zeroing LDBAR, we disable thread-imc
- * updates.
+ * By setting 0th bit of LDBAR to zero, we disable thread-imc
+ * updates to memory.
*/
- mtspr(SPRN_LDBAR, 0);
+ mtspr(SPRN_LDBAR, (mfspr(SPRN_LDBAR) & (~(1UL << 63))));
}
void thread_imc_disable(void)
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] powerpc/32: Fix missing NULL pmd check in virt_to_kpte()
From: Nathan Chancellor @ 2020-03-13 3:35 UTC (permalink / raw)
To: Christophe Leroy; +Cc: ndesaulniers, linux-kernel, Paul Mackerras, linuxppc-dev
In-Reply-To: <b1177cdfc6af74a3e277bba5d9e708c4b3315ebe.1583575707.git.christophe.leroy@c-s.fr>
On Sat, Mar 07, 2020 at 10:09:15AM +0000, Christophe Leroy wrote:
> Commit 2efc7c085f05 ("powerpc/32: drop get_pteptr()"),
> replaced get_pteptr() by virt_to_kpte(). But virt_to_kpte() lacks a
> NULL pmd check and returns an invalid non NULL pointer when there
> is no page table.
>
> Reported-by: Nick Desaulniers <ndesaulniers@google.com>
> Fixes: 2efc7c085f05 ("powerpc/32: drop get_pteptr()")
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
> arch/powerpc/include/asm/pgtable.h | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/include/asm/pgtable.h b/arch/powerpc/include/asm/pgtable.h
> index b80bfd41828d..b1f1d5339735 100644
> --- a/arch/powerpc/include/asm/pgtable.h
> +++ b/arch/powerpc/include/asm/pgtable.h
> @@ -54,7 +54,9 @@ static inline pmd_t *pmd_ptr_k(unsigned long va)
>
> static inline pte_t *virt_to_kpte(unsigned long vaddr)
> {
> - return pte_offset_kernel(pmd_ptr_k(vaddr), vaddr);
> + pmd_t *pmd = pmd_ptr_k(vaddr);
> +
> + return pmd_none(*pmd) ? NULL : pte_offset_kernel(pmd, vaddr);
> }
> #endif
>
> --
> 2.25.0
>
With QEMU 4.2.0, I can confirm this fixes the panic:
Tested-by: Nathan Chancellor <natechancellor@gmail.com>
^ permalink raw reply
* Re: [PATCHv3 2/2] pseries/scm: buffer pmem's bound addr in dt for kexec kernel
From: Oliver O'Halloran @ 2020-03-13 3:17 UTC (permalink / raw)
To: Pingfan Liu
Cc: Andrew Donnellan, Frank Rowand, kexec, Rob Herring,
Paul Mackerras, Aneesh Kumar K . V, Dan Williams, linuxppc-dev,
Hari Bathini
In-Reply-To: <1583311651-29310-3-git-send-email-kernelfans@gmail.com>
On Wed, Mar 4, 2020 at 7:50 PM Pingfan Liu <kernelfans@gmail.com> wrote:
>
> At present, plpar_hcall(H_SCM_BIND_MEM, ...) takes a very long time, so
> if dumping to fsdax, it will take a very long time.
>
> Take a closer look, during the papr_scm initialization, the only
> configuration is through drc_pmem_bind()-> plpar_hcall(H_SCM_BIND_MEM,
> ...), which helps to set up the bound address.
>
> On pseries, for kexec -l/-p kernel, there is no reset of hardware, and this
> step can be stepped around to save times. So the pmem bound address can be
> passed to the 2nd kernel through a dynamic added property "bound-addr" in
> dt node 'ibm,pmemory'.
>
> Signed-off-by: Pingfan Liu <kernelfans@gmail.com>
> To: linuxppc-dev@lists.ozlabs.org
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Hari Bathini <hbathini@linux.ibm.com>
> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com>
> Cc: Oliver O'Halloran <oohall@gmail.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: Andrew Donnellan <ajd@linux.ibm.com>
> Cc: Christophe Leroy <christophe.leroy@c-s.fr>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Frank Rowand <frowand.list@gmail.com>
> Cc: kexec@lists.infradead.org
> ---
> note: This patch has not been tested since I can not get such a pseries with pmem.
> Please kindly to give some suggestion, thanks.
There was some qemu patches to implement the Hcall interface floating
around a while ago. I'm not sure they ever made it into upstream qemu
though.
> ---
> arch/powerpc/platforms/pseries/of_helpers.c | 1 +
> arch/powerpc/platforms/pseries/papr_scm.c | 33 ++++++++++++++++++++---------
> drivers/of/base.c | 1 +
> 3 files changed, 25 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/platforms/pseries/of_helpers.c b/arch/powerpc/platforms/pseries/of_helpers.c
> index 1022e0f..2c7bab4 100644
> --- a/arch/powerpc/platforms/pseries/of_helpers.c
> +++ b/arch/powerpc/platforms/pseries/of_helpers.c
> @@ -34,6 +34,7 @@ struct property *new_property(const char *name, const int length,
> kfree(new);
> return NULL;
> }
> +EXPORT_SYMBOL(new_property);
>
> /**
> * pseries_of_derive_parent - basically like dirname(1)
> diff --git a/arch/powerpc/platforms/pseries/papr_scm.c b/arch/powerpc/platforms/pseries/papr_scm.c
> index 0b4467e..54ae903 100644
> --- a/arch/powerpc/platforms/pseries/papr_scm.c
> +++ b/arch/powerpc/platforms/pseries/papr_scm.c
> @@ -14,6 +14,7 @@
> #include <linux/delay.h>
>
> #include <asm/plpar_wrappers.h>
> +#include "of_helpers.h"
>
> #define BIND_ANY_ADDR (~0ul)
>
> @@ -383,7 +384,7 @@ static int papr_scm_probe(struct platform_device *pdev)
> {
> struct device_node *dn = pdev->dev.of_node;
> u32 drc_index, metadata_size;
> - u64 blocks, block_size;
> + u64 blocks, block_size, bound_addr = 0;
> struct papr_scm_priv *p;
> const char *uuid_str;
> u64 uuid[2];
> @@ -440,17 +441,29 @@ static int papr_scm_probe(struct platform_device *pdev)
> p->metadata_size = metadata_size;
> p->pdev = pdev;
>
> - /* request the hypervisor to bind this region to somewhere in memory */
> - rc = drc_pmem_bind(p);
> + of_property_read_u64(dn, "bound-addr", &bound_addr);
> + if (bound_addr) {
> + p->bound_addr = bound_addr;
> + } else {
> + struct property *property;
> + u64 big;
>
> - /* If phyp says drc memory still bound then force unbound and retry */
> - if (rc == H_OVERLAP)
> - rc = drc_pmem_query_n_bind(p);
> + /* request the hypervisor to bind this region to somewhere in memory */
> + rc = drc_pmem_bind(p);
>
> - if (rc != H_SUCCESS) {
> - dev_err(&p->pdev->dev, "bind err: %d\n", rc);
> - rc = -ENXIO;
> - goto err;
> + /* If phyp says drc memory still bound then force unbound and retry */
> + if (rc == H_OVERLAP)
> + rc = drc_pmem_query_n_bind(p);
> +
> + if (rc != H_SUCCESS) {
> + dev_err(&p->pdev->dev, "bind err: %d\n", rc);
> + rc = -ENXIO;
> + goto err;
> + }
> + big = cpu_to_be64(p->bound_addr);
> + property = new_property("bound-addr", sizeof(u64), (const unsigned char *)&big,
> + NULL);
That should probably be "linux,bound-addr"
The other thing that stands out to me is that you aren't removing the
property when the region is unbound. As a general rule I'd prefer we
didn't hack the DT at runtime, but if we are going to then we should
make sure we're not putting anything wrong in there.
> + of_add_property(dn, property);
> }
>
> /* setup the resource for the newly bound range */
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index ae03b12..602d2a9 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -1817,6 +1817,7 @@ int of_add_property(struct device_node *np, struct property *prop)
>
> return rc;
> }
> +EXPORT_SYMBOL_GPL(of_add_property);
>
> int __of_remove_property(struct device_node *np, struct property *prop)
> {
> --
> 2.7.5
>
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record
From: Michael Ellerman @ 2020-03-13 2:29 UTC (permalink / raw)
To: Nathan Lynch; +Cc: Tyrel Datwyler, mwb, msuchanek, linuxppc-dev
In-Reply-To: <87r1xx36u5.fsf@linux.ibm.com>
Nathan Lynch <nathanl@linux.ibm.com> writes:
> Michael Ellerman <mpe@ellerman.id.au> writes:
>>
>> It would also be *really* nice if we had some unit tests for this
>> parsing code, it's demonstrably very bug prone.
>
> Can you say more about what form unit tests could take? Like some self
> tests that run at boot time, or is there a way to run the code in a user
> space test harness?
It depends.
A userspace selftest is ideal as it's the easiest option for people to
run, ie. they just have to build a user program. There's also CI systems
setup to run the kernel selftests automatically.
See eg. tools/testing/selftests/powerpc/vphn/vphn.c
We also have boot time tests, they are good for code that we want to
test in the kernel proper, or that are hard to extract into a userspace
program. Most of those are configurable, so testing them requires
someone to enable the appropriate CONFIG_FOO and build & boot that
kernel. That reduces coverage obviously.
See eg. arch/powerpc/lib/code-patching.c
These days there's also kunit, which is a "proper" way to do kernel unit
tests. They get built into the kernel but then there's some support for
running those like a user program using UML on x86. On powerpc we'd have
to boot the kunit enabled kernel on hardware or in qemu to exercise the
tests. So they're essentially just boot time tests but with some nicer
infrastructure vs doing it all by hand like we used to.
In this case the actual function we want to test could be trivially
built into a userspace program, but the underlying device tree helpers
probably can't be.
eg. drivers/of/property.c is quite large and contains quite a few
things, we'd need to shim quite a bit to get that building in userspace
I suspect, which then becomes a maintenance burden.
So this is probably a good candidate for a kunit test, though I haven't
personally written/converted any tests to kunit so I can't say exactly
how easy that is.
cheers
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record
From: Michael Ellerman @ 2020-03-13 2:16 UTC (permalink / raw)
To: Tyrel Datwyler, Nathan Lynch; +Cc: mwb, msuchanek, linuxppc-dev
In-Reply-To: <3a3950ed-a7fb-3458-dce0-0dd6e45f93eb@linux.ibm.com>
Tyrel Datwyler <tyreld@linux.ibm.com> writes:
> On 3/11/20 10:43 PM, Michael Ellerman wrote:
>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>> On 3/10/20 10:25 AM, Nathan Lynch wrote:
>>>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>>>> The expectation is that when calling of_read_drc_info_cell()
>>>>> repeatedly to parse multiple drc-info records that the in/out curval
>>>>> parameter points at the start of the next record on return. However,
>>>>> the current behavior has curval still pointing at the final value of
>>>>> the record just parsed. The result of which is that if the
>>>>> ibm,drc-info property contains multiple properties the parsed value
>>>>> of the drc_type for any record after the first has the power_domain
>>>>> value of the previous record appended to the type string.
>>>>>
>>>>> Ex: observed the following 0xffffffff prepended to PHB
>>>>>
>>>>> [ 69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>>>>> [ 69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>>>>> [ 69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>>>>
>>>>> Fix by incrementing curval past the power_domain value to point at
>>>>> drc_type string of next record.
>>>>>
>>>>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
>>>>
>>>> I have a different commit hash for that:
>>>> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes
>>>
>>> Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
>>> the correct upstream commit.
>>>
>>> Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")
>>>
>>> Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
>>> tag on your end?
>>
>> I can update the Fixes tag.
>>
>> What's the practical effect of this bug? It seems like it should break
>> all the hotplug stuff, but presumably it doesn't in practice for some
>> reason?
>>
>> It would also be *really* nice if we had some unit tests for this
>> parsing code, it's demonstrably very bug prone.
>
> In practice PHBs are the only type of connector in the ibm,drc-info property
> that has multiple records. So, it breaks PHB hotplug, but by chance not pci,
> cpu, slot, or mem because they happen to only ever be a single record.
Thanks. I folded that into the change log.
cheers
^ permalink raw reply
* Re: [PATCH v5 1/7] ASoC: dt-bindings: fsl_asrc: Add new property fsl, asrc-format
From: Shengjiu Wang @ 2020-03-13 1:58 UTC (permalink / raw)
To: Nicolin Chen, Rob Herring
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Linux-ALSA, Timur Tabi, Xiubo Li, Fabio Estevam, Shengjiu Wang,
Takashi Iwai, Liam Girdwood, Mark Brown, linuxppc-dev,
linux-kernel
In-Reply-To: <20200309211943.GB11333@Asurada-Nvidia.nvidia.com>
Hi Rob
On Tue, Mar 10, 2020 at 5:20 AM Nicolin Chen <nicoleotsuka@gmail.com> wrote:
>
> 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.
Could you please share your comments or proposal about
Nicolin's concern?
best regards
wang shengjiu
^ permalink raw reply
* Re: [PATCH v3 24/27] powerpc/powernv/pmem: Expose SMART data via ndctl
From: Alastair D'Silva @ 2020-03-12 23:14 UTC (permalink / raw)
To: Andrew Donnellan
Cc: Madhavan Srinivasan, Alexey Kardashevskiy, Masahiro Yamada,
Oliver O'Halloran, Mauro Carvalho Chehab, Ira Weiny,
Thomas Gleixner, Rob Herring, Dave Jiang, linux-nvdimm,
Aneesh Kumar K . V, Krzysztof Kozlowski, Anju T Sudhakar,
Mahesh Salgaonkar, Arnd Bergmann, Greg Kurz, Nicholas Piggin,
Cédric Le Goater, Dan Williams, Hari Bathini, linux-mm,
Greg Kroah-Hartman, linux-kernel, Vishal Verma, Frederic Barrat,
Paul Mackerras, Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <e68c9064-cb7f-2b5e-9a7b-70fd5367270a@linux.ibm.com>
On Thu, 2020-03-05 at 14:36 +1100, Andrew Donnellan wrote:
> On 21/2/20 2:27 pm, Alastair D'Silva wrote:
> > +static int ndctl_smart(struct ocxlpmem *ocxlpmem, struct
> > nd_cmd_pkg *pkg)
> > +{
> > + u32 length, i;
> > + struct nd_ocxl_smart *out;
> > + int rc;
> > +
> > + mutex_lock(&ocxlpmem->admin_command.lock);
> > +
> > + rc = admin_command_request(ocxlpmem, ADMIN_COMMAND_SMART);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_execute(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = admin_command_complete_timeout(ocxlpmem,
> > ADMIN_COMMAND_SMART);
> > + if (rc < 0) {
> > + dev_err(&ocxlpmem->dev, "SMART timeout\n");
> > + goto out;
> > + }
> > +
> > + rc = admin_response(ocxlpmem);
> > + if (rc < 0)
> > + goto out;
> > + if (rc != STATUS_SUCCESS) {
> > + warn_status(ocxlpmem, "Unexpected status from SMART",
> > rc);
> > + goto out;
> > + }
> > +
> > + rc = smart_header_parse(ocxlpmem, &length);
> > + if (rc)
> > + goto out;
> > +
> > + pkg->nd_fw_size = length;
> > +
> > + length = min(length, pkg->nd_size_out); // bytes
> > + out = (struct nd_ocxl_smart *)pkg->nd_payload;
> > + // Each SMART attribute is 2 * 64 bits
> > + out->count = length / (2 * sizeof(u64)); // attributes
>
> From what I can tell - 8 bytes of nd_ocxl_smart are taken up for
> the
> count + reserved bytes, so this is going to potentially overrun the
> user
> buffer.
Ok
>
> > +
> > + for (i = 0; i < length; i += sizeof(u64)) {
>
> It might be neater to make i count up by 1 and then multiply by
> sizeof(u64) later.
>
I tried, it doesn't make much difference to the complexity, and makes
it less clear that we are stepping in 64bit chunks.
> > + rc = ocxl_global_mmio_read64(ocxlpmem->ocxl_afu,
> > + ocxlpmem-
> > >admin_command.data_offset + sizeof(u64) + i,
>
> + 0x08 rather than + sizeof(u64) for consistency.
>
We use sizeof(u64) in the rest of this function.
> > + OCXL_LITTLE_ENDIAN,
> > + &out-
> > >attribs[i/sizeof(u64)]);
> > + if (rc)
> > + goto out;
> > + }
> > +
> > + rc = admin_response_handled(ocxlpmem);
> > + if (rc)
> > + goto out;
> > +
> > + rc = 0;
> > + goto out;
> > +
> > +out:
> > + mutex_unlock(&ocxlpmem->admin_command.lock);
> > + return rc;
> > +}
> > +
> > +static int ndctl_call(struct ocxlpmem *ocxlpmem, void *buf,
> > unsigned int buf_len)
> > +{
> > + struct nd_cmd_pkg *pkg = buf;
> > +
> > + if (buf_len < sizeof(struct nd_cmd_pkg)) {
> > + dev_err(&ocxlpmem->dev, "Invalid ND_CALL size=%u\n",
> > buf_len);
> > + return -EINVAL;
> > + }
> > +
> > + if (pkg->nd_family != NVDIMM_FAMILY_OCXL) {
> > + dev_err(&ocxlpmem->dev, "Invalid ND_CALL
> > family=0x%llx\n", pkg->nd_family);
> > + return -EINVAL;
> > + }
> > +
> > + switch (pkg->nd_command) {
> > + case ND_CMD_OCXL_SMART:
> > + ndctl_smart(ocxlpmem, pkg);
>
> Did you intend to dispose of the return code here?
Whoops.
>
> > + break;
> > +
> > + default:
> > + dev_err(&ocxlpmem->dev, "Invalid ND_CALL
> > command=0x%llx\n", pkg->nd_command);
> > + return -EINVAL;
> > + }
> > +
> > +
> > + return 0;
> > +}
> > +
> > static int ndctl(struct nvdimm_bus_descriptor *nd_desc,
> > struct nvdimm *nvdimm,
> > unsigned int cmd, void *buf, unsigned int buf_len, int
> > *cmd_rc)
> > @@ -88,6 +211,10 @@ static int ndctl(struct nvdimm_bus_descriptor
> > *nd_desc,
> > struct ocxlpmem *ocxlpmem = container_of(nd_desc, struct
> > ocxlpmem, bus_desc);
> >
> > switch (cmd) {
> > + case ND_CMD_CALL:
> > + *cmd_rc = ndctl_call(ocxlpmem, buf, buf_len);
> > + return 0;
> > +
> > case ND_CMD_GET_CONFIG_SIZE:
> > *cmd_rc = ndctl_config_size(buf);
> > return 0;
> > @@ -171,6 +298,7 @@ static int register_lpc_mem(struct ocxlpmem
> > *ocxlpmem)
> > set_bit(ND_CMD_GET_CONFIG_SIZE, &nvdimm_cmd_mask);
> > set_bit(ND_CMD_GET_CONFIG_DATA, &nvdimm_cmd_mask);
> > set_bit(ND_CMD_SET_CONFIG_DATA, &nvdimm_cmd_mask);
> > + set_bit(ND_CMD_CALL, &nvdimm_cmd_mask);
> >
> > set_bit(NDD_ALIASING, &nvdimm_flags);
> >
> > diff --git a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> > b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> > index 927690f4888f..0eb7a35d24ae 100644
> > --- a/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> > +++ b/arch/powerpc/platforms/powernv/pmem/ocxl_internal.h
> > @@ -7,6 +7,7 @@
> > #include <linux/libnvdimm.h>
> > #include <uapi/nvdimm/ocxl-pmem.h>
> > #include <linux/mm.h>
> > +#include <linux/ndctl.h>
> >
> > #define LABEL_AREA_SIZE (1UL << PA_SECTION_SHIFT)
> > #define DEFAULT_TIMEOUT 100
> > @@ -98,6 +99,23 @@ struct ocxlpmem_function0 {
> > struct ocxl_fn *ocxl_fn;
> > };
> >
> > +struct nd_ocxl_smart {
> > + __u8 count;
> > + __u8 reserved[7];
> > + __u64 attribs[0];
> > +} __packed;
> > +
> > +struct nd_pkg_ocxl {
> > + struct nd_cmd_pkg gen;
> > + union {
> > + struct nd_ocxl_smart smart;
> > + };
> > +};
> > +
> > +enum nd_cmd_ocxl {
> > + ND_CMD_OCXL_SMART = 1,
> > +};
> > +
> > struct ocxlpmem {
> > struct device dev;
> > struct pci_dev *pdev;
> > diff --git a/include/uapi/linux/ndctl.h
> > b/include/uapi/linux/ndctl.h
> > index de5d90212409..2885052e7f40 100644
> > --- a/include/uapi/linux/ndctl.h
> > +++ b/include/uapi/linux/ndctl.h
> > @@ -244,6 +244,7 @@ struct nd_cmd_pkg {
> > #define NVDIMM_FAMILY_HPE2 2
> > #define NVDIMM_FAMILY_MSFT 3
> > #define NVDIMM_FAMILY_HYPERV 4
> > +#define NVDIMM_FAMILY_OCXL 6
> >
> > #define ND_IOCTL_CALL _IOWR(ND_IOCTL,
> > ND_CMD_CALL,\
> > struct nd_cmd_pkg)
> >
--
Alastair D'Silva
Open Source Developer
Linux Technology Centre, IBM Australia
mob: 0423 762 819
^ permalink raw reply
* Re: [RFC 00/11] perf: Enhancing perf to export processor hazard information
From: Kim Phillips @ 2020-03-12 22:38 UTC (permalink / raw)
To: Ravi Bangoria
Cc: Mark Rutland, Andi Kleen, maddy, Peter Zijlstra, Jiri Olsa, LKML,
Stephane Eranian, Adrian Hunter, Alexander Shishkin, yao.jin,
Ingo Molnar, Paul Mackerras, Arnaldo Carvalho de Melo,
Robert Richter, Namhyung Kim, linuxppc-dev, Alexey Budankov,
Liang, Kan
In-Reply-To: <8a4d966c-acc9-b2b7-8ab7-027aefab201c@linux.ibm.com>
On 3/11/20 11:00 AM, Ravi Bangoria wrote:
> Hi Kim,
Hi Ravi,
> On 3/6/20 3:36 AM, Kim Phillips wrote:
>>> On 3/3/20 3:55 AM, Kim Phillips wrote:
>>>> On 3/2/20 2:21 PM, Stephane Eranian wrote:
>>>>> On Mon, Mar 2, 2020 at 2:13 AM Peter Zijlstra <peterz@infradead.org> wrote:
>>>>>>
>>>>>> On Mon, Mar 02, 2020 at 10:53:44AM +0530, Ravi Bangoria wrote:
>>>>>>> Modern processors export such hazard data in Performance
>>>>>>> Monitoring Unit (PMU) registers. Ex, 'Sampled Instruction Event
>>>>>>> Register' on IBM PowerPC[1][2] and 'Instruction-Based Sampling' on
>>>>>>> AMD[3] provides similar information.
>>>>>>>
>>>>>>> Implementation detail:
>>>>>>>
>>>>>>> A new sample_type called PERF_SAMPLE_PIPELINE_HAZ is introduced.
>>>>>>> If it's set, kernel converts arch specific hazard information
>>>>>>> into generic format:
>>>>>>>
>>>>>>> struct perf_pipeline_haz_data {
>>>>>>> /* Instruction/Opcode type: Load, Store, Branch .... */
>>>>>>> __u8 itype;
>>>>>>> /* Instruction Cache source */
>>>>>>> __u8 icache;
>>>>>>> /* Instruction suffered hazard in pipeline stage */
>>>>>>> __u8 hazard_stage;
>>>>>>> /* Hazard reason */
>>>>>>> __u8 hazard_reason;
>>>>>>> /* Instruction suffered stall in pipeline stage */
>>>>>>> __u8 stall_stage;
>>>>>>> /* Stall reason */
>>>>>>> __u8 stall_reason;
>>>>>>> __u16 pad;
>>>>>>> };
>>>>>>
>>>>>> Kim, does this format indeed work for AMD IBS?
>>>>
>>>> It's not really 1:1, we don't have these separations of stages
>>>> and reasons, for example: we have missed in L2 cache, for example.
>>>> So IBS output is flatter, with more cycle latency figures than
>>>> IBM's AFAICT.
>>>
>>> AMD IBS captures pipeline latency data incase Fetch sampling like the
>>> Fetch latency, tag to retire latency, completion to retire latency and
>>> so on. Yes, Ops sampling do provide more data on load/store centric
>>> information. But it also captures more detailed data for Branch instructions.
>>> And we also looked at ARM SPE, which also captures more details pipeline
>>> data and latency information.
>>>
>>>>> Personally, I don't like the term hazard. This is too IBM Power
>>>>> specific. We need to find a better term, maybe stall or penalty.
>>>>
>>>> Right, IBS doesn't have a filter to only count stalled or otherwise
>>>> bad events. IBS' PPR descriptions has one occurrence of the
>>>> word stall, and no penalty. The way I read IBS is it's just
>>>> reporting more sample data than just the precise IP: things like
>>>> hits, misses, cycle latencies, addresses, types, etc., so words
>>>> like 'extended', or the 'auxiliary' already used today even
>>>> are more appropriate for IBS, although I'm the last person to
>>>> bikeshed.
>>>
>>> We are thinking of using "pipeline" word instead of Hazard.
>>
>> Hm, the word 'pipeline' occurs 0 times in IBS documentation.
>
> NP. We thought pipeline is generic hw term so we proposed "pipeline"
> word. We are open to term which can be generic enough.
>
>>
>> I realize there are a couple of core pipeline-specific pieces
>> of information coming out of it, but the vast majority
>> are addresses, latencies of various components in the memory
>> hierarchy, and various component hit/miss bits.
>
> Yes. we should capture core pipeline specific details. For example,
> IBS generates Branch unit information(IbsOpData1) and Icahce related
> data(IbsFetchCtl) which is something that shouldn't be extended as
> part of perf-mem, IMO.
Sure, IBS Op-side output is more 'perf mem' friendly, and so it
should populate perf_mem_data_src fields, just like POWER9 can:
union perf_mem_data_src {
...
__u64 mem_rsvd:24,
mem_snoopx:2, /* snoop mode, ext */
mem_remote:1, /* remote */
mem_lvl_num:4, /* memory hierarchy level number */
mem_dtlb:7, /* tlb access */
mem_lock:2, /* lock instr */
mem_snoop:5, /* snoop mode */
mem_lvl:14, /* memory hierarchy level */
mem_op:5; /* type of opcode */
E.g., SIER[LDST] SIER[A_XLATE_SRC] can be used to populate
mem_lvl[_num], SIER_TYPE can be used to populate 'mem_op',
'mem_lock', and the Reload Bus Source Encoding bits can
be used to populate mem_snoop, right?
For IBS, I see PERF_SAMPLE_ADDR and PERF_SAMPLE_PHYS_ADDR can be
used for the ld/st target addresses, too.
>> What's needed here is a vendor-specific extended
>> sample information that all these technologies gather,
>> of which things like e.g., 'L1 TLB cycle latency' we
>> all should have in common.
>
> Yes. We will include fields to capture the latency cycles (like Issue
> latency, Instruction completion latency etc..) along with other pipeline
> details in the proposed structure.
Latency figures are just an example, and from what I
can tell, struct perf_sample_data already has a 'weight' member,
used with PERF_SAMPLE_WEIGHT, that is used by intel-pt to
transfer memory access latency figures. Granted, that's
a bad name given all other vendors don't call latency
'weight'.
I didn't see any latency figures coming out of POWER9,
and do not expect this patchseries to implement those
of other vendors, e.g., AMD's IBS; leave each vendor
to amend perf to suit their own h/w output please.
My main point there, however, was that each vendor should
use streamlined record-level code to just copy the data
in the proprietary format that their hardware produces,
and then then perf tooling can synthesize the events
from the raw data at report/script/etc. time.
>> I'm not sure why a new PERF_SAMPLE_PIPELINE_HAZ is needed
>> either. Can we use PERF_SAMPLE_AUX instead?
>
> We took a look at PERF_SAMPLE_AUX. IIUC, PERF_SAMPLE_AUX is intended when
> large volume of data needs to be captured as part of perf.data without
> frequent PMIs. But proposed type is to address the capture of pipeline
SAMPLE_AUX shouldn't care whether the volume is large, or how frequent
PMIs are, even though it may be used in those environments.
> information on each sample using PMI at periodic intervals. Hence proposing
> PERF_SAMPLE_PIPELINE_HAZ.
And that's fine for any extra bits that POWER9 has to convey
to its users beyond things already represented by other sample
types like PERF_SAMPLE_DATA_SRC, but the capturing of both POWER9
and other vendor e.g., AMD IBS data can be made vendor-independent
at record time by using SAMPLE_AUX, or SAMPLE_RAW even, which is
what IBS currently uses.
>> Take a look at
>> commit 98dcf14d7f9c "perf tools: Add kernel AUX area sampling
>> definitions". The sample identifier can be used to determine
>> which vendor's sampling IP's data is in it, and events can
>> be recorded just by copying the content of the SIER, etc.
>> registers, and then events get synthesized from the aux
>> sample at report/inject/annotate etc. time. This allows
>> for less sample recording overhead, and moves all the vendor
>> specific decoding and common event conversions for userspace
>> to figure out.
>
> When AUX buffer data is structured, tool side changes added to present the
> pipeline data can be re-used.
Not sure I understand: AUX data would be structured on
each vendor's raw h/w register formats.
Thanks,
Kim
>>>>> Also worth considering is the support of ARM SPE (Statistical
>>>>> Profiling Extension) which is their version of IBS.
>>>>> Whatever gets added need to cover all three with no limitations.
>>>>
>>>> I thought Intel's various LBR, PEBS, and PT supported providing
>>>> similar sample data in perf already, like with perf mem/c2c?
>>>
>>> perf-mem is more of data centric in my opinion. It is more towards
>>> memory profiling. So proposal here is to expose pipeline related
>>> details like stalls and latencies.
>>
>> Like I said, I don't see it that way, I see it as "any particular
>> vendor's event's extended details', and these pipeline details
>> have overlap with existing infrastructure within perf, e.g., L2
>> cache misses.
>>
>> Kim
>>
>
^ permalink raw reply
* [PATCH 6/6] soc: fsl: qe: fix sparse warnings for ucc_slow.c
From: Li Yang @ 2020-03-12 22:28 UTC (permalink / raw)
To: Rasmus Villemoes, Timur Tabi, Zhao Qiang
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, Li Yang
In-Reply-To: <20200312222827.17409-1-leoyang.li@nxp.com>
Fixes the following sparse warnings:
drivers/soc/fsl/qe/ucc_slow.c:78:17: warning: incorrect type in assignment (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:78:17: expected struct ucc_slow *us_regs
drivers/soc/fsl/qe/ucc_slow.c:78:17: got struct ucc_slow [noderef] <asn:2> *us_regs
drivers/soc/fsl/qe/ucc_slow.c:81:18: warning: incorrect type in argument 1 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:81:18: expected void const volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:81:18: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:90:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:90:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:90:9: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:99:17: warning: incorrect type in assignment (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:99:17: expected struct ucc_slow *us_regs
drivers/soc/fsl/qe/ucc_slow.c:99:17: got struct ucc_slow [noderef] <asn:2> *us_regs
drivers/soc/fsl/qe/ucc_slow.c:102:18: warning: incorrect type in argument 1 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:102:18: expected void const volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:102:18: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:111:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:111:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:111:9: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:172:28: warning: Using plain integer as NULL pointer
drivers/soc/fsl/qe/ucc_slow.c:174:25: warning: cast removes address space '<asn:2>' of expression
drivers/soc/fsl/qe/ucc_slow.c:175:25: warning: cast removes address space '<asn:2>' of expression
drivers/soc/fsl/qe/ucc_slow.c:194:23: warning: incorrect type in assignment (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:194:23: expected struct ucc_slow_pram *us_pram
drivers/soc/fsl/qe/ucc_slow.c:194:23: got void [noderef] <asn:2> *
drivers/soc/fsl/qe/ucc_slow.c:204:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:204:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:204:9: got restricted __be16 *
drivers/soc/fsl/qe/ucc_slow.c:229:41: warning: incorrect type in assignment (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:229:41: expected struct qe_bd *tx_bd
drivers/soc/fsl/qe/ucc_slow.c:229:41: got void [noderef] <asn:2> *
drivers/soc/fsl/qe/ucc_slow.c:232:17: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:232:17: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:232:17: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:234:17: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:234:17: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:234:17: got unsigned int [usertype] *
drivers/soc/fsl/qe/ucc_slow.c:238:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:238:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:238:9: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:239:9: warning: cast from restricted __be32
drivers/soc/fsl/qe/ucc_slow.c:239:9: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/ucc_slow.c:239:9: expected unsigned int [usertype] val
drivers/soc/fsl/qe/ucc_slow.c:239:9: got restricted __be32 [usertype]
drivers/soc/fsl/qe/ucc_slow.c:239:9: warning: cast from restricted __be32
drivers/soc/fsl/qe/ucc_slow.c:239:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:239:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:239:9: got unsigned int [usertype] *
drivers/soc/fsl/qe/ucc_slow.c:242:26: warning: incorrect type in assignment (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:242:26: expected struct qe_bd *rx_bd
drivers/soc/fsl/qe/ucc_slow.c:242:26: got void [noderef] <asn:2> *
drivers/soc/fsl/qe/ucc_slow.c:245:17: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:245:17: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:245:17: got unsigned int [usertype] *
drivers/soc/fsl/qe/ucc_slow.c:247:17: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:247:17: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:247:17: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:251:9: warning: cast from restricted __be32
drivers/soc/fsl/qe/ucc_slow.c:251:9: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/ucc_slow.c:251:9: expected unsigned int [usertype] val
drivers/soc/fsl/qe/ucc_slow.c:251:9: got restricted __be32 [usertype]
drivers/soc/fsl/qe/ucc_slow.c:251:9: warning: cast from restricted __be32
drivers/soc/fsl/qe/ucc_slow.c:251:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:251:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:251:9: got unsigned int [usertype] *
drivers/soc/fsl/qe/ucc_slow.c:252:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:252:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:252:9: got restricted __be32 *
drivers/soc/fsl/qe/ucc_slow.c:276:39: warning: mixing different enum types:
drivers/soc/fsl/qe/ucc_slow.c:276:39: unsigned int enum ucc_slow_tx_oversampling_rate
drivers/soc/fsl/qe/ucc_slow.c:276:39: unsigned int enum ucc_slow_rx_oversampling_rate
drivers/soc/fsl/qe/ucc_slow.c:296:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:296:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:296:9: got restricted __be16 *
drivers/soc/fsl/qe/ucc_slow.c:297:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc_slow.c:297:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc_slow.c:297:9: got restricted __be16 *
Also removed the unneccessary clearing for kzalloc'ed structure.
Signed-off-by: Li Yang <leoyang.li@nxp.com>
---
drivers/soc/fsl/qe/ucc_slow.c | 33 +++++++++++++--------------------
include/soc/fsl/qe/ucc_slow.h | 13 ++++++-------
2 files changed, 19 insertions(+), 27 deletions(-)
diff --git a/drivers/soc/fsl/qe/ucc_slow.c b/drivers/soc/fsl/qe/ucc_slow.c
index 274d34449846..7e11be41ab62 100644
--- a/drivers/soc/fsl/qe/ucc_slow.c
+++ b/drivers/soc/fsl/qe/ucc_slow.c
@@ -72,7 +72,7 @@ EXPORT_SYMBOL(ucc_slow_restart_tx);
void ucc_slow_enable(struct ucc_slow_private * uccs, enum comm_dir mode)
{
- struct ucc_slow *us_regs;
+ struct ucc_slow __iomem *us_regs;
u32 gumr_l;
us_regs = uccs->us_regs;
@@ -93,7 +93,7 @@ EXPORT_SYMBOL(ucc_slow_enable);
void ucc_slow_disable(struct ucc_slow_private * uccs, enum comm_dir mode)
{
- struct ucc_slow *us_regs;
+ struct ucc_slow __iomem *us_regs;
u32 gumr_l;
us_regs = uccs->us_regs;
@@ -122,7 +122,7 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
u32 i;
struct ucc_slow __iomem *us_regs;
u32 gumr;
- struct qe_bd *bd;
+ struct qe_bd __iomem *bd;
u32 id;
u32 command;
int ret = 0;
@@ -168,16 +168,9 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
return -ENOMEM;
}
- uccs->saved_uccm = 0;
- uccs->p_rx_frame = 0;
us_regs = uccs->us_regs;
- uccs->p_ucce = (u16 *) & (us_regs->ucce);
- uccs->p_uccm = (u16 *) & (us_regs->uccm);
-#ifdef STATISTICS
- uccs->rx_frames = 0;
- uccs->tx_frames = 0;
- uccs->rx_discarded = 0;
-#endif /* STATISTICS */
+ uccs->p_ucce = &us_regs->ucce;
+ uccs->p_uccm = &us_regs->uccm;
/* Get PRAM base */
uccs->us_pram_offset =
@@ -231,24 +224,24 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
/* clear bd buffer */
qe_iowrite32be(0, &bd->buf);
/* set bd status and length */
- qe_iowrite32be(0, (u32 *)bd);
+ qe_iowrite32be(0, (u32 __iomem *)bd);
bd++;
}
/* for last BD set Wrap bit */
qe_iowrite32be(0, &bd->buf);
- qe_iowrite32be(cpu_to_be32(T_W), (u32 *)bd);
+ qe_iowrite32be(T_W, (u32 __iomem *)bd);
/* Init Rx bds */
bd = uccs->rx_bd = qe_muram_addr(uccs->rx_base_offset);
for (i = 0; i < us_info->rx_bd_ring_len - 1; i++) {
/* set bd status and length */
- qe_iowrite32be(0, (u32 *)bd);
+ qe_iowrite32be(0, (u32 __iomem *)bd);
/* clear bd buffer */
qe_iowrite32be(0, &bd->buf);
bd++;
}
/* for last BD set Wrap bit */
- qe_iowrite32be(cpu_to_be32(R_W), (u32 *)bd);
+ qe_iowrite32be(R_W, (u32 __iomem *)bd);
qe_iowrite32be(0, &bd->buf);
/* Set GUMR (For more details see the hardware spec.). */
@@ -273,8 +266,8 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
qe_iowrite32be(gumr, &us_regs->gumr_h);
/* gumr_l */
- gumr = us_info->tdcr | us_info->rdcr | us_info->tenc | us_info->renc |
- us_info->diag | us_info->mode;
+ gumr = (u32)us_info->tdcr | (u32)us_info->rdcr | (u32)us_info->tenc |
+ (u32)us_info->renc | (u32)us_info->diag | (u32)us_info->mode;
if (us_info->tci)
gumr |= UCC_SLOW_GUMR_L_TCI;
if (us_info->rinv)
@@ -289,8 +282,8 @@ int ucc_slow_init(struct ucc_slow_info * us_info, struct ucc_slow_private ** ucc
/* if the data is in cachable memory, the 'global' */
/* in the function code should be set. */
- uccs->us_pram->tbmr = UCC_BMR_BO_BE;
- uccs->us_pram->rbmr = UCC_BMR_BO_BE;
+ qe_iowrite8(UCC_BMR_BO_BE, &uccs->us_pram->tbmr);
+ qe_iowrite8(UCC_BMR_BO_BE, &uccs->us_pram->rbmr);
/* rbase, tbase are offsets from MURAM base */
qe_iowrite16be(uccs->rx_base_offset, &uccs->us_pram->rbase);
diff --git a/include/soc/fsl/qe/ucc_slow.h b/include/soc/fsl/qe/ucc_slow.h
index d187a6be83bc..11a216e4e919 100644
--- a/include/soc/fsl/qe/ucc_slow.h
+++ b/include/soc/fsl/qe/ucc_slow.h
@@ -184,7 +184,7 @@ struct ucc_slow_info {
struct ucc_slow_private {
struct ucc_slow_info *us_info;
struct ucc_slow __iomem *us_regs; /* Ptr to memory map of UCC regs */
- struct ucc_slow_pram *us_pram; /* a pointer to the parameter RAM */
+ struct ucc_slow_pram __iomem *us_pram; /* a pointer to the parameter RAM */
s32 us_pram_offset;
int enabled_tx; /* Whether channel is enabled for Tx (ENT) */
int enabled_rx; /* Whether channel is enabled for Rx (ENR) */
@@ -196,13 +196,12 @@ struct ucc_slow_private {
and length for first BD in a frame */
s32 tx_base_offset; /* first BD in Tx BD table offset (In MURAM) */
s32 rx_base_offset; /* first BD in Rx BD table offset (In MURAM) */
- struct qe_bd *confBd; /* next BD for confirm after Tx */
- struct qe_bd *tx_bd; /* next BD for new Tx request */
- struct qe_bd *rx_bd; /* next BD to collect after Rx */
+ struct qe_bd __iomem *confBd; /* next BD for confirm after Tx */
+ struct qe_bd __iomem *tx_bd; /* next BD for new Tx request */
+ struct qe_bd __iomem *rx_bd; /* next BD to collect after Rx */
void *p_rx_frame; /* accumulating receive frame */
- u16 *p_ucce; /* a pointer to the event register in memory.
- */
- u16 *p_uccm; /* a pointer to the mask register in memory */
+ __be16 __iomem *p_ucce; /* a pointer to the event register in memory */
+ __be16 __iomem *p_uccm; /* a pointer to the mask register in memory */
u16 saved_uccm; /* a saved mask for the RX Interrupt bits */
#ifdef STATISTICS
u32 tx_frames; /* Transmitted frames counters */
--
2.17.1
^ permalink raw reply related
* [PATCH 5/6] soc: fsl: qe: fix sparse warnings for ucc_fast.c
From: Li Yang @ 2020-03-12 22:28 UTC (permalink / raw)
To: Rasmus Villemoes, Timur Tabi, Zhao Qiang
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, Li Yang
In-Reply-To: <20200312222827.17409-1-leoyang.li@nxp.com>
Fixes the following sparse warnings:
drivers/soc/fsl/qe/ucc_fast.c:218:22: warning: incorrect type in assignment (different base types)
drivers/soc/fsl/qe/ucc_fast.c:218:22: expected unsigned int [noderef] [usertype] <asn:2> *p_ucce
drivers/soc/fsl/qe/ucc_fast.c:218:22: got restricted __be32 [noderef] <asn:2> *
drivers/soc/fsl/qe/ucc_fast.c:219:22: warning: incorrect type in assignment (different base types)
drivers/soc/fsl/qe/ucc_fast.c:219:22: expected unsigned int [noderef] [usertype] <asn:2> *p_uccm
drivers/soc/fsl/qe/ucc_fast.c:219:22: got restricted __be32 [noderef] <asn:2> *
Signed-off-by: Li Yang <leoyang.li@nxp.com>
---
include/soc/fsl/qe/ucc_fast.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/include/soc/fsl/qe/ucc_fast.h b/include/soc/fsl/qe/ucc_fast.h
index ba0e838f962a..dc4e79468094 100644
--- a/include/soc/fsl/qe/ucc_fast.h
+++ b/include/soc/fsl/qe/ucc_fast.h
@@ -178,10 +178,10 @@ struct ucc_fast_info {
struct ucc_fast_private {
struct ucc_fast_info *uf_info;
struct ucc_fast __iomem *uf_regs; /* a pointer to the UCC regs. */
- u32 __iomem *p_ucce; /* a pointer to the event register in memory. */
- u32 __iomem *p_uccm; /* a pointer to the mask register in memory. */
+ __be32 __iomem *p_ucce; /* a pointer to the event register in memory. */
+ __be32 __iomem *p_uccm; /* a pointer to the mask register in memory. */
#ifdef CONFIG_UGETH_TX_ON_DEMAND
- u16 __iomem *p_utodr; /* pointer to the transmit on demand register */
+ __be16 __iomem *p_utodr;/* pointer to the transmit on demand register */
#endif
int enabled_tx; /* Whether channel is enabled for Tx (ENT) */
int enabled_rx; /* Whether channel is enabled for Rx (ENR) */
--
2.17.1
^ permalink raw reply related
* [PATCH 1/6] soc: fsl: qe: fix sparse warnings for qe.c
From: Li Yang @ 2020-03-12 22:28 UTC (permalink / raw)
To: Rasmus Villemoes, Timur Tabi, Zhao Qiang
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, Li Yang
In-Reply-To: <20200312222827.17409-1-leoyang.li@nxp.com>
Fixes the following sparse warnings:
drivers/soc/fsl/qe/qe.c:426:9: warning: cast to restricted __be32
drivers/soc/fsl/qe/qe.c:528:41: warning: incorrect type in assignment (different base types)
drivers/soc/fsl/qe/qe.c:528:41: expected unsigned long long static [addressable] [toplevel] [usertype] extended_modes
drivers/soc/fsl/qe/qe.c:528:41: got restricted __be64 const [usertype] extended_modes
Signed-off-by: Li Yang <leoyang.li@nxp.com>
---
drivers/soc/fsl/qe/qe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index 96c2057d8d8e..447146861c2c 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -423,7 +423,7 @@ static void qe_upload_microcode(const void *base,
qe_iowrite32be(be32_to_cpu(code[i]), &qe_immr->iram.idata);
/* Set I-RAM Ready Register */
- qe_iowrite32be(be32_to_cpu(QE_IRAM_READY), &qe_immr->iram.iready);
+ qe_iowrite32be(QE_IRAM_READY, &qe_immr->iram.iready);
}
/*
@@ -525,7 +525,7 @@ int qe_upload_firmware(const struct qe_firmware *firmware)
*/
memset(&qe_firmware_info, 0, sizeof(qe_firmware_info));
strlcpy(qe_firmware_info.id, firmware->id, sizeof(qe_firmware_info.id));
- qe_firmware_info.extended_modes = firmware->extended_modes;
+ qe_firmware_info.extended_modes = be64_to_cpu(firmware->extended_modes);
memcpy(qe_firmware_info.vtraps, firmware->vtraps,
sizeof(firmware->vtraps));
--
2.17.1
^ permalink raw reply related
* [PATCH 3/6] soc: fsl: qe: fix sparse warnings for ucc.c
From: Li Yang @ 2020-03-12 22:28 UTC (permalink / raw)
To: Rasmus Villemoes, Timur Tabi, Zhao Qiang
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, Li Yang
In-Reply-To: <20200312222827.17409-1-leoyang.li@nxp.com>
Fixes the following sparse warnings:
drivers/soc/fsl/qe/ucc.c:637:20: warning: incorrect type in assignment (different address spaces)
drivers/soc/fsl/qe/ucc.c:637:20: expected struct qe_mux *qe_mux_reg
drivers/soc/fsl/qe/ucc.c:637:20: got struct qe_mux [noderef] <asn:2> *
drivers/soc/fsl/qe/ucc.c:652:9: warning: incorrect type in argument 1 (different address spaces)
drivers/soc/fsl/qe/ucc.c:652:9: expected void const volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc.c:652:9: got restricted __be32 *
drivers/soc/fsl/qe/ucc.c:652:9: warning: incorrect type in argument 2 (different address spaces)
drivers/soc/fsl/qe/ucc.c:652:9: expected void volatile [noderef] <asn:2> *addr
drivers/soc/fsl/qe/ucc.c:652:9: got restricted __be32 *
Signed-off-by: Li Yang <leoyang.li@nxp.com>
---
drivers/soc/fsl/qe/ucc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/qe/ucc.c b/drivers/soc/fsl/qe/ucc.c
index 90157acc5ba6..d6c93970df4d 100644
--- a/drivers/soc/fsl/qe/ucc.c
+++ b/drivers/soc/fsl/qe/ucc.c
@@ -632,7 +632,7 @@ int ucc_set_tdm_rxtx_sync(u32 tdm_num, enum qe_clock clock,
{
int source;
u32 shift;
- struct qe_mux *qe_mux_reg;
+ struct qe_mux __iomem *qe_mux_reg;
qe_mux_reg = &qe_immr->qmx;
--
2.17.1
^ permalink raw reply related
* [PATCH 4/6] soc: fsl: qe: fix sparse warnings for qe_ic.c
From: Li Yang @ 2020-03-12 22:28 UTC (permalink / raw)
To: Rasmus Villemoes, Timur Tabi, Zhao Qiang
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, Li Yang
In-Reply-To: <20200312222827.17409-1-leoyang.li@nxp.com>
Fixes the following sparse warnings:
drivers/soc/fsl/qe/qe_ic.c:253:32: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/qe_ic.c:253:32: expected restricted __be32 [noderef] [usertype] <asn:2> *base
drivers/soc/fsl/qe/qe_ic.c:253:32: got unsigned int [noderef] [usertype] <asn:2> *regs
drivers/soc/fsl/qe/qe_ic.c:254:26: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/qe_ic.c:254:26: expected restricted __be32 [noderef] [usertype] <asn:2> *base
drivers/soc/fsl/qe/qe_ic.c:254:26: got unsigned int [noderef] [usertype] <asn:2> *regs
drivers/soc/fsl/qe/qe_ic.c:269:32: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/qe_ic.c:269:32: expected restricted __be32 [noderef] [usertype] <asn:2> *base
drivers/soc/fsl/qe/qe_ic.c:269:32: got unsigned int [noderef] [usertype] <asn:2> *regs
drivers/soc/fsl/qe/qe_ic.c:270:26: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/qe_ic.c:270:26: expected restricted __be32 [noderef] [usertype] <asn:2> *base
drivers/soc/fsl/qe/qe_ic.c:270:26: got unsigned int [noderef] [usertype] <asn:2> *regs
drivers/soc/fsl/qe/qe_ic.c:341:31: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/qe_ic.c:341:31: expected restricted __be32 [noderef] [usertype] <asn:2> *base
drivers/soc/fsl/qe/qe_ic.c:341:31: got unsigned int [noderef] [usertype] <asn:2> *regs
drivers/soc/fsl/qe/qe_ic.c:357:31: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/qe_ic.c:357:31: expected restricted __be32 [noderef] [usertype] <asn:2> *base
drivers/soc/fsl/qe/qe_ic.c:357:31: got unsigned int [noderef] [usertype] <asn:2> *regs
drivers/soc/fsl/qe/qe_ic.c:450:26: warning: incorrect type in argument 1 (different base types)
drivers/soc/fsl/qe/qe_ic.c:450:26: expected restricted __be32 [noderef] [usertype] <asn:2> *base
drivers/soc/fsl/qe/qe_ic.c:450:26: got unsigned int [noderef] [usertype] <asn:2> *regs
Signed-off-by: Li Yang <leoyang.li@nxp.com>
---
drivers/soc/fsl/qe/qe_ic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/qe/qe_ic.c b/drivers/soc/fsl/qe/qe_ic.c
index 0dd5bdb04a14..0390af999900 100644
--- a/drivers/soc/fsl/qe/qe_ic.c
+++ b/drivers/soc/fsl/qe/qe_ic.c
@@ -44,7 +44,7 @@
struct qe_ic {
/* Control registers offset */
- u32 __iomem *regs;
+ __be32 __iomem *regs;
/* The remapper for this QEIC */
struct irq_domain *irqhost;
--
2.17.1
^ permalink raw reply related
* [PATCH 2/6] soc: fsl: qe: fix sparse warning for qe_common.c
From: Li Yang @ 2020-03-12 22:28 UTC (permalink / raw)
To: Rasmus Villemoes, Timur Tabi, Zhao Qiang
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, Li Yang
In-Reply-To: <20200312222827.17409-1-leoyang.li@nxp.com>
Fixes the following sparse warning:
drivers/soc/fsl/qe/qe_common.c:75:48: warning: incorrect type in argument 2 (different base types)
drivers/soc/fsl/qe/qe_common.c:75:48: expected restricted __be32 const [usertype] *addr
drivers/soc/fsl/qe/qe_common.c:75:48: got unsigned int *
Signed-off-by: Li Yang <leoyang.li@nxp.com>
---
drivers/soc/fsl/qe/qe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/soc/fsl/qe/qe_common.c b/drivers/soc/fsl/qe/qe_common.c
index a81a1a79f1ca..75075591f630 100644
--- a/drivers/soc/fsl/qe/qe_common.c
+++ b/drivers/soc/fsl/qe/qe_common.c
@@ -46,7 +46,7 @@ int cpm_muram_init(void)
{
struct device_node *np;
struct resource r;
- u32 zero[OF_MAX_ADDR_CELLS] = {};
+ __be32 zero[OF_MAX_ADDR_CELLS] = {};
resource_size_t max = 0;
int i = 0;
int ret = 0;
--
2.17.1
^ permalink raw reply related
* [PATCH 0/6] Fix sparse warnings for common qe library code
From: Li Yang @ 2020-03-12 22:28 UTC (permalink / raw)
To: Rasmus Villemoes, Timur Tabi, Zhao Qiang
Cc: linuxppc-dev, linux-kernel, linux-arm-kernel, Li Yang
The QE code was previously only supported on big-endian PowerPC systems
that use the same endian as the QE device. The endian transfer code is
not really exercised. Recent updates extended the QE drivers to
little-endian ARM/ARM64 systems which makes the endian transfer really
meaningful and hence triggered more sparse warnings for the endian
mismatch. Some of these endian issues are real issues that need to be
fixed.
While at it, fixed some direct de-references of IO memory space and
suppressed other __iomem address space mismatch issues by adding correct
address space attributes.
Li Yang (6):
soc: fsl: qe: fix sparse warnings for qe.c
soc: fsl: qe: fix sparse warning for qe_common.c
soc: fsl: qe: fix sparse warnings for ucc.c
soc: fsl: qe: fix sparse warnings for qe_ic.c
soc: fsl: qe: fix sparse warnings for ucc_fast.c
soc: fsl: qe: fix sparse warnings for ucc_slow.c
drivers/soc/fsl/qe/qe.c | 4 ++--
drivers/soc/fsl/qe/qe_common.c | 2 +-
drivers/soc/fsl/qe/qe_ic.c | 2 +-
drivers/soc/fsl/qe/ucc.c | 2 +-
drivers/soc/fsl/qe/ucc_slow.c | 33 +++++++++++++--------------------
include/soc/fsl/qe/ucc_fast.h | 6 +++---
include/soc/fsl/qe/ucc_slow.h | 13 ++++++-------
7 files changed, 27 insertions(+), 35 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record
From: Tyrel Datwyler @ 2020-03-12 21:34 UTC (permalink / raw)
To: Michael Ellerman, Nathan Lynch; +Cc: mwb, msuchanek, linuxppc-dev
In-Reply-To: <87imjai0wp.fsf@mpe.ellerman.id.au>
On 3/11/20 10:43 PM, Michael Ellerman wrote:
> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>> On 3/10/20 10:25 AM, Nathan Lynch wrote:
>>> Tyrel Datwyler <tyreld@linux.ibm.com> writes:
>>>> The expectation is that when calling of_read_drc_info_cell()
>>>> repeatedly to parse multiple drc-info records that the in/out curval
>>>> parameter points at the start of the next record on return. However,
>>>> the current behavior has curval still pointing at the final value of
>>>> the record just parsed. The result of which is that if the
>>>> ibm,drc-info property contains multiple properties the parsed value
>>>> of the drc_type for any record after the first has the power_domain
>>>> value of the previous record appended to the type string.
>>>>
>>>> Ex: observed the following 0xffffffff prepended to PHB
>>>>
>>>> [ 69.485037] drc-info: type: \xff\xff\xff\xffPHB, prefix: PHB , index_start: 0x20000001
>>>> [ 69.485038] drc-info: suffix_start: 1, sequential_elems: 3072, sequential_inc: 1
>>>> [ 69.485038] drc-info: power-domain: 0xffffffff, last_index: 0x20000c00
>>>>
>>>> Fix by incrementing curval past the power_domain value to point at
>>>> drc_type string of next record.
>>>>
>>>> Fixes: a29396653b8bf ("pseries/drc-info: Search DRC properties for CPU indexes")
>>>
>>> I have a different commit hash for that:
>>> e83636ac3334 pseries/drc-info: Search DRC properties for CPU indexes
>>
>> Oof, looks like I grabbed the commit hash from the SLES 15 SP1 branch. You have
>> the correct upstream commit.
>>
>> Fixes: e83636ac3334 ("pseries/drc-info: Search DRC properties for CPU indexes")
>>
>> Michael, let me know if you want me to resubmit, or if you will fixup the Fixes
>> tag on your end?
>
> I can update the Fixes tag.
>
> What's the practical effect of this bug? It seems like it should break
> all the hotplug stuff, but presumably it doesn't in practice for some
> reason?
>
> It would also be *really* nice if we had some unit tests for this
> parsing code, it's demonstrably very bug prone.
>
> cheers
>
In practice PHBs are the only type of connector in the ibm,drc-info property
that has multiple records. So, it breaks PHB hotplug, but by chance not pci,
cpu, slot, or mem because they happen to only ever be a single record.
I have a follow up patch to at least add some pr_debug at the end of this
parsing function to dump each record as its read.
-Tyrel
^ permalink raw reply
* Re: [PATCH -next] PCI: rpaphp: remove set but not used variable 'value'
From: Tyrel Datwyler @ 2020-03-12 21:36 UTC (permalink / raw)
To: Bjorn Helgaas, Chen Zhou; +Cc: paulus, linuxppc-dev, linux-kernel, linux-pci
In-Reply-To: <20200312144157.GA110750@google.com>
On 3/12/20 7:41 AM, Bjorn Helgaas wrote:
> On Thu, Mar 12, 2020 at 09:38:02AM -0500, Bjorn Helgaas wrote:
>> On Thu, Mar 12, 2020 at 10:04:12PM +0800, Chen Zhou wrote:
>>> Fixes gcc '-Wunused-but-set-variable' warning:
>>>
>>> drivers/pci/hotplug/rpaphp_core.c: In function is_php_type:
>>> drivers/pci/hotplug/rpaphp_core.c:291:16: warning:
>>> variable value set but not used [-Wunused-but-set-variable]
>>>
>>> Reported-by: Hulk Robot <hulkci@huawei.com>
>>> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
>>
>> Michael, if you want this:
>>
>> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>>
>> If you don't mind, edit the subject to follow the convention, e.g.,
>>
>> PCI: rpaphp: Remove unused variable 'value'
>>
>> Apparently simple_strtoul() is deprecated and we're supposed to use
>> kstrtoul() instead. Looks like kstrtoul() might simplify the code a
>> little, too, e.g.,
>>
>> if (kstrtoul(drc_type, 0, &value) == 0)
>> return 1;
>>
>> return 0;
>
> I guess there are several other uses of simple_strtoul() in this file.
> Not sure if it's worth changing them all, just this one, or just the
> patch below as-is.
If we are going to change one might as well do them all at once. If the original
submitter wants to send the follow up that is fine, or I can send a patch when I
have a minute.
-Tyrel
>
>>> ---
>>> drivers/pci/hotplug/rpaphp_core.c | 3 +--
>>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
>>> index e408e40..5d871ef 100644
>>> --- a/drivers/pci/hotplug/rpaphp_core.c
>>> +++ b/drivers/pci/hotplug/rpaphp_core.c
>>> @@ -288,11 +288,10 @@ EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
>>>
>>> static int is_php_type(char *drc_type)
>>> {
>>> - unsigned long value;
>>> char *endptr;
>>>
>>> /* PCI Hotplug nodes have an integer for drc_type */
>>> - value = simple_strtoul(drc_type, &endptr, 10);
>>> + simple_strtoul(drc_type, &endptr, 10);
>>> if (endptr == drc_type)
>>> return 0;
>>>
>>> --
>>> 2.7.4
>>>
^ permalink raw reply
* Re: [PATCH v5 4/7] ASoC: fsl_asrc: rename asrc_priv to asrc
From: Mark Brown @ 2020-03-12 17:46 UTC (permalink / raw)
To: Nicolin Chen
Cc: mark.rutland, devicetree, alsa-devel, timur, Xiubo.Lee,
linuxppc-dev, Shengjiu Wang, tiwai, lgirdwood, perex, robh+dt,
festevam, linux-kernel
In-Reply-To: <20200309213016.GC11333@Asurada-Nvidia.nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 518 bytes --]
On Mon, Mar 09, 2020 at 02:30:17PM -0700, Nicolin Chen wrote:
> 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?
Yes, please. Or even just send it separately.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [5.6.0-rc2-next-20200218/powerpc] Boot failure on POWER9
From: Sachin Sant @ 2020-03-12 16:51 UTC (permalink / raw)
To: Michael Ellerman
Cc: Srikar Dronamraju, Michal Hocko, Pekka Enberg,
Linux-Next Mailing List, Paul Mackerras, Vlastimil Babka,
David Rientjes, Christopher Lameter, linuxppc-dev, Joonsoo Kim,
Kirill Tkhai
In-Reply-To: <87a74lix5p.fsf@mpe.ellerman.id.au>
> The patch below might work. Sachin can you test this? I tried faking up
> a system with a memoryless node zero but couldn't get it to even start
> booting.
>
The patch did not help. The kernel crashed during
the boot with the same call trace.
BUG_ON() introduced with the patch was not triggered.
Thanks
-Sachin
> cheers
>
>
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index 9b4f5fb719e0..d1f11437f6c4 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -282,6 +282,9 @@ void __init mem_init(void)
> */
> BUILD_BUG_ON(MMU_PAGE_COUNT > 16);
>
> + BUG_ON(smp_processor_id() != boot_cpuid);
> + set_numa_mem(local_memory_node(numa_cpu_lookup_table[boot_cpuid]));
> +
> #ifdef CONFIG_SWIOTLB
> /*
> * Some platforms (e.g. 85xx) limit DMA-able memory way below
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/numa: Set numa_node for all possible cpus
From: Vlastimil Babka @ 2020-03-12 16:41 UTC (permalink / raw)
To: Srikar Dronamraju
Cc: Sachin Sant, linuxppc-dev, LKML, Michal Hocko, linux-mm,
Kirill Tkhai, Mel Gorman, Joonsoo Kim, Kirill A. Shutemov,
Andrew Morton, Linus Torvalds, Christopher Lameter
In-Reply-To: <20200312161310.GC3277@linux.vnet.ibm.com>
On 3/12/20 5:13 PM, Srikar Dronamraju wrote:
> * Vlastimil Babka <vbabka@suse.cz> [2020-03-12 14:51:38]:
>
>> > * Vlastimil Babka <vbabka@suse.cz> [2020-03-12 10:30:50]:
>> >
>> >> On 3/12/20 9:23 AM, Sachin Sant wrote:
>> >> >> On 12-Mar-2020, at 10:57 AM, Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:
>> >> >> * Michal Hocko <mhocko@kernel.org> [2020-03-11 12:57:35]:
>> >> >>> On Wed 11-03-20 16:32:35, Srikar Dronamraju wrote:
>> >> >>>> To ensure a cpuless, memoryless dummy node is not online, powerpc need
>> >> >>>> to make sure all possible but not present cpu_to_node are set to a
>> >> >>>> proper node.
>> >> >>>
>> >> >>> Just curious, is this somehow related to
>> >> >>> http://lkml.kernel.org/r/20200227182650.GG3771@dhcp22.suse.cz?
>> >> >>>
>> >> >>
>> >> >> The issue I am trying to fix is a known issue in Powerpc since many years.
>> >> >> So this surely not a problem after a75056fc1e7c (mm/memcontrol.c: allocate
>> >> >> shrinker_map on appropriate NUMA node").
>> >> >>
>> >
>> > While I am not an expert in the slub area, I looked at the patch
>> > a75056fc1e7c and had some thoughts on why this could be causing this issue.
>> >
>> > On the system where the crash happens, the possible number of nodes is much
>> > greater than the number of onlined nodes. The pdgat or the NODE_DATA is only
>> > available for onlined nodes.
>> >
>> > With a75056fc1e7c memcg_alloc_shrinker_maps, we end up calling kzalloc_node
>> > for all possible nodes and in ___slab_alloc we end up looking at the
>> > node_present_pages which is NODE_DATA(nid)->node_present_pages.
>> > i.e for a node whose pdgat struct is not allocated, we are trying to
>> > dereference.
>>
>> From what we saw, the pgdat does exist, the problem is that slab's per-node data
>> doesn't exist for a node that doesn't have present pages, as it would be a waste
>> of memory.
>
> Just to be clear
> Before my 3 patches to fix dummy node:
> srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/possible
> 0-31
> srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/online
> 0-1
OK
>>
>> Uh actually you are probably right, the NODE_DATA doesn't exist anymore? In
>> Sachin's first report [1] we have
>>
>> [ 0.000000] numa: NODE_DATA [mem 0x8bfedc900-0x8bfee3fff]
>> [ 0.000000] numa: NODE_DATA(0) on node 1
>> [ 0.000000] numa: NODE_DATA [mem 0x8bfed5200-0x8bfedc8ff]
>>
>
> So even if pgdat would exist for nodes 0 and 1, there is no pgdat for the
> rest 30 nodes.
I see. Perhaps node_present_pages(node) is not safe in SLUB then and it should
check online first, as you suggested.
>> But in this thread, with your patches Sachin reports:
>
> and with my patches
> srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/possible
> 0-31
> srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/online
> 1
>
>>
>> [ 0.000000] numa: NODE_DATA [mem 0x8bfedc900-0x8bfee3fff]
>>
>
> so we only see one pgdat.
>
>> So I assume it's just node 1. In that case, node_present_pages is really dangerous.
>>
>> [1]
>> https://lore.kernel.org/linux-next/3381CD91-AB3D-4773-BA04-E7A072A63968@linux.vnet.ibm.com/
>>
>> > Also for a memoryless/cpuless node or possible but not present nodes,
>> > node_to_mem_node(node) will still end up as node (atleast on powerpc).
>>
>> I think that's the place where this would be best to fix.
>>
>
> Maybe. I thought about it but the current set_numa_mem semantics are apt
> for memoryless cpu node and not for possible nodes. We could have upto 256
> possible nodes and only 2 nodes (1,2) with cpu and 1 node (1) with memory.
> node_to_mem_node seems to return what is set in set_numa_mem().
> set_numa_mem() seems to say set my numa_mem node for the current memoryless
> node to the param passed.
>
> But how do we set numa_mem for all the other 253 possible nodes, which
> probably will have 0 as default?
>
> Should we introduce another API such that we could update for all possible
> nodes?
If we want to rely on node_to_mem_node() to give us something safe for each
possible node, then probably it would have to be like that, yeah.
>> > I tried with this hunk below and it works.
>> >
>> > But I am not sure if we need to check at other places were
>> > node_present_pages is being called.
>>
>> I think this seems to defeat the purpose of node_to_mem_node()? Shouldn't it
>> return only nodes that are online with present memory?
>> CCing Joonsoo who seems to have introduced this in ad2c8144418c ("topology: add
>> support for node_to_mem_node() to determine the fallback node")
>>
>
> Agree
>
>> I think we do need well defined and documented rules around node_to_mem_node(),
>> cpu_to_node(), existence of NODE_DATA, various node_states bitmaps etc so
>> everyone handles it the same, safe way.
So let's try to brainstorm how this would look like? What I mean are some rules
like below, even if some details in my current understanding are most likely
incorrect:
with nid present in:
N_POSSIBLE - pgdat might not exist, node_to_mem_node() must return some online
node with memory so that we don't require everyone to search for it in slightly
different ways
N_ONLINE - pgdat must exist, there doesn't have to be present memory,
node_to_mem_node() still has to return something else (?)
N_NORMAL_MEMORY - there is present memory, node_to_mem_node() returns itself
N_HIGH_MEMORY - node has present high memory
>
> Other option would be to tweak Kirill Tkhai's patch such that we call
> kvmalloc_node()/kzalloc_node() if node is online and call kvmalloc/kvzalloc
> if the node is offline.
I really would like a solution that hides these ugly details from callers so
they don't have to workaround the APIs we provide. kvmalloc_node() really
shouldn't crash, and it should fallback automatically if we don't give it
__GFP_THISNODE
However, taking a step back, memcg_alloc_shrinker_maps() is probably rather
wasteful on systems with 256 possible nodes and only few present, by allocating
effectively dead structures for each memcg.
SLUB tries to be smart, so it allocates the per-node per-cache structures only
when the node goes online in slab_mem_going_online_callback(). This is why
there's a crash when such non-existing structures are accessed for a node that's
not online, and why they shouldn't be accessed.
Perhaps memcg should do the same on-demand allocation, if possible.
>> > diff --git a/mm/slub.c b/mm/slub.c
>> > index 626cbcbd977f..bddb93bed55e 100644
>> > --- a/mm/slub.c
>> > +++ b/mm/slub.c
>> > @@ -2571,9 +2571,13 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
>> > if (unlikely(!node_match(page, node))) {
>> > int searchnode = node;
>> >
>> > - if (node != NUMA_NO_NODE && !node_present_pages(node))
>> > - searchnode = node_to_mem_node(node);
>> > -
>> > + if (node != NUMA_NO_NODE) {
>> > + if (!node_online(node) || !node_present_pages(node)) {
>> > + searchnode = node_to_mem_node(node);
>> > + if (!node_online(searchnode))
>> > + searchnode = first_online_node;
>> > + }
>> > + }
>> > if (unlikely(!node_match(page, searchnode))) {
>> > stat(s, ALLOC_NODE_MISMATCH);
>> > deactivate_slab(s, page, c->freelist, c);
>
^ permalink raw reply
* Re: [RFC PATCH v1] pseries/drmem: don't cache node id in drmem_lmb struct
From: Scott Cheloha @ 2020-03-12 16:07 UTC (permalink / raw)
To: Michal Suchánek
Cc: Nathan Lynch, Nathan Fontenont, David Hildenbrand, Aneesh Kumar,
Paul Mackerras, linuxppc-dev, Rick Lindsley
In-Reply-To: <20200312050237.GP1776@kitsune.suse.cz>
Hi Michal,
On Thu, Mar 12, 2020 at 06:02:37AM +0100, Michal Suchánek wrote:
> On Wed, Mar 11, 2020 at 06:08:15PM -0500, Scott Cheloha wrote:
> > At memory hot-remove time we can retrieve an LMB's nid from its
> > corresponding memory_block. There is no need to store the nid
> > in multiple locations.
> >
> > Signed-off-by: Scott Cheloha <cheloha@linux.ibm.com>
> > ---
> > The linear search in powerpc's memory_add_physaddr_to_nid() has become a
> > bottleneck at boot on systems with many LMBs.
> >
> > As described in this patch here:
> >
> > https://lore.kernel.org/linuxppc-dev/20200221172901.1596249-2-cheloha@linux.ibm.com/
> >
> > the linear search seriously cripples drmem_init().
> >
> > The obvious solution (shown in that patch) is to just make the search
> > in memory_add_physaddr_to_nid() faster. An XArray seems well-suited
> > to the task of mapping an address range to an LMB object.
> >
> > The less obvious approach is to just call memory_add_physaddr_to_nid()
> > in fewer places.
> >
> > I'm not sure which approach is correct, hence the RFC.
>
> You basically revert the below which will likely cause the very error
> that was fixed there:
>
> commit b2d3b5ee66f2a04a918cc043cec0c9ed3de58f40
> Author: Nathan Fontenot <nfont@linux.vnet.ibm.com>
> Date: Tue Oct 2 10:35:59 2018 -0500
>
> powerpc/pseries: Track LMB nid instead of using device tree
>
> When removing memory we need to remove the memory from the node
> it was added to instead of looking up the node it should be in
> in the device tree.
>
> During testing we have seen scenarios where the affinity for a
> LMB changes due to a partition migration or PRRN event. In these
> cases the node the LMB exists in may not match the node the device
> tree indicates it belongs in. This can lead to a system crash
> when trying to DLPAR remove the LMB after a migration or PRRN
> event. The current code looks up the node in the device tree to
> remove the LMB from, the crash occurs when we try to offline this
> node and it does not have any data, i.e. node_data[nid] == NULL.
I'm aware of this patch and that this is a near-total revert.
I'm not reintroducing the original behavior, though. Instead of going
to the device tree to recompute the expected node id I'm retrieving it
from the LMB's corresponding memory_block.
That crucial difference is this chunk:
--- a/arch/powerpc/platforms/pseries/hotplug-memory.c
+++ b/arch/powerpc/platforms/pseries/hotplug-memory.c
@@ -376,25 +376,29 @@ static int dlpar_add_lmb(struct drmem_lmb *);
static int dlpar_remove_lmb(struct drmem_lmb *lmb)
{
+ struct memory_block *mem_block;
unsigned long block_sz;
int rc;
if (!lmb_is_removable(lmb))
return -EINVAL;
+ mem_block = lmb_to_memblock(lmb);
+ if (mem_block == NULL)
+ return -EINVAL;
+
rc = dlpar_offline_lmb(lmb);
if (rc)
return rc;
block_sz = pseries_memory_block_size();
- __remove_memory(lmb->nid, lmb->base_addr, block_sz);
+ __remove_memory(mem_block->nid, lmb->base_addr, block_sz);
/* Update memory regions for memory remove */
memblock_remove(lmb->base_addr, block_sz);
invalidate_lmb_associativity_index(lmb);
- lmb_clear_nid(lmb);
lmb->flags &= ~DRCONF_MEM_ASSIGNED;
return 0;
---
Unless it's possible that the call:
__add_memory(my_nid, my_addr, my_size);
does not yield the following:
memory_block.nid == my_nid
on success, then I think the solution in my patch is equivalent to and
simpler than the one introduced in the patch you quote.
Can you see a way the above would not hold?
Then again, maybe there's a good reason not to retrieve the node id in
this way. I'm thinking David Hildenbrand and/or Nathan Fontenont may
have some insight on that.
^ permalink raw reply
* Re: [PATCH] powerpc/pseries: fix of_read_drc_info_cell() to point at next record
From: Nathan Lynch @ 2020-03-12 15:56 UTC (permalink / raw)
To: Michael Ellerman; +Cc: Tyrel Datwyler, mwb, msuchanek, linuxppc-dev
In-Reply-To: <87imjai0wp.fsf@mpe.ellerman.id.au>
Michael Ellerman <mpe@ellerman.id.au> writes:
>
> It would also be *really* nice if we had some unit tests for this
> parsing code, it's demonstrably very bug prone.
Can you say more about what form unit tests could take? Like some self
tests that run at boot time, or is there a way to run the code in a user
space test harness?
^ permalink raw reply
* Re: [PATCH 1/3] powerpc/numa: Set numa_node for all possible cpus
From: Srikar Dronamraju @ 2020-03-12 16:13 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Sachin Sant, linuxppc-dev, LKML, Michal Hocko, linux-mm,
Kirill Tkhai, Mel Gorman, Joonsoo Kim, Kirill A. Shutemov,
Andrew Morton, Linus Torvalds, Christopher Lameter
In-Reply-To: <61437352-8b54-38fa-4471-044a65c9d05a@suse.cz>
* Vlastimil Babka <vbabka@suse.cz> [2020-03-12 14:51:38]:
> > * Vlastimil Babka <vbabka@suse.cz> [2020-03-12 10:30:50]:
> >
> >> On 3/12/20 9:23 AM, Sachin Sant wrote:
> >> >> On 12-Mar-2020, at 10:57 AM, Srikar Dronamraju <srikar@linux.vnet.ibm.com> wrote:
> >> >> * Michal Hocko <mhocko@kernel.org> [2020-03-11 12:57:35]:
> >> >>> On Wed 11-03-20 16:32:35, Srikar Dronamraju wrote:
> >> >>>> To ensure a cpuless, memoryless dummy node is not online, powerpc need
> >> >>>> to make sure all possible but not present cpu_to_node are set to a
> >> >>>> proper node.
> >> >>>
> >> >>> Just curious, is this somehow related to
> >> >>> http://lkml.kernel.org/r/20200227182650.GG3771@dhcp22.suse.cz?
> >> >>>
> >> >>
> >> >> The issue I am trying to fix is a known issue in Powerpc since many years.
> >> >> So this surely not a problem after a75056fc1e7c (mm/memcontrol.c: allocate
> >> >> shrinker_map on appropriate NUMA node").
> >> >>
> >
> > While I am not an expert in the slub area, I looked at the patch
> > a75056fc1e7c and had some thoughts on why this could be causing this issue.
> >
> > On the system where the crash happens, the possible number of nodes is much
> > greater than the number of onlined nodes. The pdgat or the NODE_DATA is only
> > available for onlined nodes.
> >
> > With a75056fc1e7c memcg_alloc_shrinker_maps, we end up calling kzalloc_node
> > for all possible nodes and in ___slab_alloc we end up looking at the
> > node_present_pages which is NODE_DATA(nid)->node_present_pages.
> > i.e for a node whose pdgat struct is not allocated, we are trying to
> > dereference.
>
> From what we saw, the pgdat does exist, the problem is that slab's per-node data
> doesn't exist for a node that doesn't have present pages, as it would be a waste
> of memory.
Just to be clear
Before my 3 patches to fix dummy node:
srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/possible
0-31
srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/online
0-1
>
> Uh actually you are probably right, the NODE_DATA doesn't exist anymore? In
> Sachin's first report [1] we have
>
> [ 0.000000] numa: NODE_DATA [mem 0x8bfedc900-0x8bfee3fff]
> [ 0.000000] numa: NODE_DATA(0) on node 1
> [ 0.000000] numa: NODE_DATA [mem 0x8bfed5200-0x8bfedc8ff]
>
So even if pgdat would exist for nodes 0 and 1, there is no pgdat for the
rest 30 nodes.
> But in this thread, with your patches Sachin reports:
and with my patches
srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/possible
0-31
srikar@ltc-zzci-2 /sys/devices/system/node $ cat $PWD/online
1
>
> [ 0.000000] numa: NODE_DATA [mem 0x8bfedc900-0x8bfee3fff]
>
so we only see one pgdat.
> So I assume it's just node 1. In that case, node_present_pages is really dangerous.
>
> [1]
> https://lore.kernel.org/linux-next/3381CD91-AB3D-4773-BA04-E7A072A63968@linux.vnet.ibm.com/
>
> > Also for a memoryless/cpuless node or possible but not present nodes,
> > node_to_mem_node(node) will still end up as node (atleast on powerpc).
>
> I think that's the place where this would be best to fix.
>
Maybe. I thought about it but the current set_numa_mem semantics are apt
for memoryless cpu node and not for possible nodes. We could have upto 256
possible nodes and only 2 nodes (1,2) with cpu and 1 node (1) with memory.
node_to_mem_node seems to return what is set in set_numa_mem().
set_numa_mem() seems to say set my numa_mem node for the current memoryless
node to the param passed.
But how do we set numa_mem for all the other 253 possible nodes, which
probably will have 0 as default?
Should we introduce another API such that we could update for all possible
nodes?
> > I tried with this hunk below and it works.
> >
> > But I am not sure if we need to check at other places were
> > node_present_pages is being called.
>
> I think this seems to defeat the purpose of node_to_mem_node()? Shouldn't it
> return only nodes that are online with present memory?
> CCing Joonsoo who seems to have introduced this in ad2c8144418c ("topology: add
> support for node_to_mem_node() to determine the fallback node")
>
Agree
> I think we do need well defined and documented rules around node_to_mem_node(),
> cpu_to_node(), existence of NODE_DATA, various node_states bitmaps etc so
> everyone handles it the same, safe way.
>
Other option would be to tweak Kirill Tkhai's patch such that we call
kvmalloc_node()/kzalloc_node() if node is online and call kvmalloc/kvzalloc
if the node is offline.
> > diff --git a/mm/slub.c b/mm/slub.c
> > index 626cbcbd977f..bddb93bed55e 100644
> > --- a/mm/slub.c
> > +++ b/mm/slub.c
> > @@ -2571,9 +2571,13 @@ static void *___slab_alloc(struct kmem_cache *s, gfp_t gfpflags, int node,
> > if (unlikely(!node_match(page, node))) {
> > int searchnode = node;
> >
> > - if (node != NUMA_NO_NODE && !node_present_pages(node))
> > - searchnode = node_to_mem_node(node);
> > -
> > + if (node != NUMA_NO_NODE) {
> > + if (!node_online(node) || !node_present_pages(node)) {
> > + searchnode = node_to_mem_node(node);
> > + if (!node_online(searchnode))
> > + searchnode = first_online_node;
> > + }
> > + }
> > if (unlikely(!node_match(page, searchnode))) {
> > stat(s, ALLOC_NODE_MISMATCH);
> > deactivate_slab(s, page, c->freelist, c);
--
Thanks and Regards
Srikar Dronamraju
^ permalink raw reply
* Re: [PATCH -next] PCI: rpaphp: remove set but not used variable 'value'
From: Bjorn Helgaas @ 2020-03-12 14:41 UTC (permalink / raw)
To: Chen Zhou; +Cc: tyreld, linux-pci, linux-kernel, paulus, linuxppc-dev
In-Reply-To: <20200312143800.GA109542@google.com>
On Thu, Mar 12, 2020 at 09:38:02AM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 12, 2020 at 10:04:12PM +0800, Chen Zhou wrote:
> > Fixes gcc '-Wunused-but-set-variable' warning:
> >
> > drivers/pci/hotplug/rpaphp_core.c: In function is_php_type:
> > drivers/pci/hotplug/rpaphp_core.c:291:16: warning:
> > variable value set but not used [-Wunused-but-set-variable]
> >
> > Reported-by: Hulk Robot <hulkci@huawei.com>
> > Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
>
> Michael, if you want this:
>
> Acked-by: Bjorn Helgaas <bhelgaas@google.com>
>
> If you don't mind, edit the subject to follow the convention, e.g.,
>
> PCI: rpaphp: Remove unused variable 'value'
>
> Apparently simple_strtoul() is deprecated and we're supposed to use
> kstrtoul() instead. Looks like kstrtoul() might simplify the code a
> little, too, e.g.,
>
> if (kstrtoul(drc_type, 0, &value) == 0)
> return 1;
>
> return 0;
I guess there are several other uses of simple_strtoul() in this file.
Not sure if it's worth changing them all, just this one, or just the
patch below as-is.
> > ---
> > drivers/pci/hotplug/rpaphp_core.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
> > index e408e40..5d871ef 100644
> > --- a/drivers/pci/hotplug/rpaphp_core.c
> > +++ b/drivers/pci/hotplug/rpaphp_core.c
> > @@ -288,11 +288,10 @@ EXPORT_SYMBOL_GPL(rpaphp_check_drc_props);
> >
> > static int is_php_type(char *drc_type)
> > {
> > - unsigned long value;
> > char *endptr;
> >
> > /* PCI Hotplug nodes have an integer for drc_type */
> > - value = simple_strtoul(drc_type, &endptr, 10);
> > + simple_strtoul(drc_type, &endptr, 10);
> > if (endptr == drc_type)
> > return 0;
> >
> > --
> > 2.7.4
> >
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox