* [PATCH 4/9] powerpc/bpf: Handle large branch ranges with BPF_EXIT
From: Naveen N. Rao @ 2021-10-01 21:14 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Daniel Borkmann,
Alexei Starovoitov, Christophe Leroy, Johan Almbladh
Cc: bpf, linuxppc-dev
In-Reply-To: <cover.1633104510.git.naveen.n.rao@linux.vnet.ibm.com>
In some scenarios, it is possible that the program epilogue is outside
the branch range for a BPF_EXIT instruction. Instead of rejecting such
programs, emit an indirect branch. We track the size of the bpf program
emitted after the initial run and do a second pass since BPF_EXIT can
end up emitting different number of instructions depending on the
program size.
Suggested-by: Jordan Niethe <jniethe5@gmail.com>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/net/bpf_jit.h | 3 +++
arch/powerpc/net/bpf_jit_comp.c | 22 +++++++++++++++++++++-
arch/powerpc/net/bpf_jit_comp32.c | 2 +-
arch/powerpc/net/bpf_jit_comp64.c | 2 +-
4 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 89bd744c2bffd4..4023de1698b9f5 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -126,6 +126,7 @@
#define SEEN_FUNC 0x20000000 /* might call external helpers */
#define SEEN_TAILCALL 0x40000000 /* uses tail calls */
+#define SEEN_BIG_PROG 0x80000000 /* large prog, >32MB */
#define SEEN_VREG_MASK 0x1ff80000 /* Volatile registers r3-r12 */
#define SEEN_NVREG_MASK 0x0003ffff /* Non volatile registers r14-r31 */
@@ -179,6 +180,8 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
void bpf_jit_build_prologue(u32 *image, struct codegen_context *ctx);
void bpf_jit_build_epilogue(u32 *image, struct codegen_context *ctx);
void bpf_jit_realloc_regs(struct codegen_context *ctx);
+int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx,
+ int tmp_reg, unsigned long exit_addr);
#endif
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index fcbf7a917c566e..3204872fbf2738 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -72,6 +72,21 @@ static int bpf_jit_fixup_subprog_calls(struct bpf_prog *fp, u32 *image,
return 0;
}
+int bpf_jit_emit_exit_insn(u32 *image, struct codegen_context *ctx,
+ int tmp_reg, unsigned long exit_addr)
+{
+ if (!(ctx->seen & SEEN_BIG_PROG) && is_offset_in_branch_range(exit_addr)) {
+ PPC_JMP(exit_addr);
+ } else {
+ ctx->seen |= SEEN_BIG_PROG;
+ PPC_FUNC_ADDR(tmp_reg, (unsigned long)image + exit_addr);
+ EMIT(PPC_RAW_MTCTR(tmp_reg));
+ EMIT(PPC_RAW_BCTR());
+ }
+
+ return 0;
+}
+
struct powerpc64_jit_data {
struct bpf_binary_header *header;
u32 *addrs;
@@ -155,12 +170,17 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
goto out_addrs;
}
+ if (!is_offset_in_branch_range((long)cgctx.idx * 4))
+ cgctx.seen |= SEEN_BIG_PROG;
+
/*
* If we have seen a tail call, we need a second pass.
* This is because bpf_jit_emit_common_epilogue() is called
* from bpf_jit_emit_tail_call() with a not yet stable ctx->seen.
+ * We also need a second pass if we ended up with too large
+ * a program so as to fix branches.
*/
- if (cgctx.seen & SEEN_TAILCALL) {
+ if (cgctx.seen & (SEEN_TAILCALL | SEEN_BIG_PROG)) {
cgctx.idx = 0;
if (bpf_jit_build_body(fp, 0, &cgctx, addrs, false)) {
fp = org_fp;
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
index a74d52204f8da2..d2a67574a23066 100644
--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c
@@ -852,7 +852,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
* we'll just fall through to the epilogue.
*/
if (i != flen - 1)
- PPC_JMP(exit_addr);
+ bpf_jit_emit_exit_insn(image, ctx, tmp_reg, exit_addr);
/* else fall through to the epilogue */
break;
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index f06c62089b1457..3351a866ef6207 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -761,7 +761,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
* we'll just fall through to the epilogue.
*/
if (i != flen - 1)
- PPC_JMP(exit_addr);
+ bpf_jit_emit_exit_insn(image, ctx, b2p[TMP_REG_1], exit_addr);
/* else fall through to the epilogue */
break;
--
2.33.0
^ permalink raw reply related
* [PATCH 3/9] powerpc/bpf: Remove unused SEEN_STACK
From: Naveen N. Rao @ 2021-10-01 21:14 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Daniel Borkmann,
Alexei Starovoitov, Christophe Leroy, Johan Almbladh
Cc: bpf, linuxppc-dev
In-Reply-To: <cover.1633104510.git.naveen.n.rao@linux.vnet.ibm.com>
From: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
SEEN_STACK is unused on PowerPC. Remove it. Also, have
SEEN_TAILCALL use 0x40000000.
Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
arch/powerpc/net/bpf_jit.h | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 7e9b978b768ed9..89bd744c2bffd4 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -125,8 +125,7 @@
#define COND_LE (CR0_GT | COND_CMP_FALSE)
#define SEEN_FUNC 0x20000000 /* might call external helpers */
-#define SEEN_STACK 0x40000000 /* uses BPF stack */
-#define SEEN_TAILCALL 0x80000000 /* uses tail calls */
+#define SEEN_TAILCALL 0x40000000 /* uses tail calls */
#define SEEN_VREG_MASK 0x1ff80000 /* Volatile registers r3-r12 */
#define SEEN_NVREG_MASK 0x0003ffff /* Non volatile registers r14-r31 */
--
2.33.0
^ permalink raw reply related
* [PATCH 2/9] powerpc/bpf: Validate branch ranges
From: Naveen N. Rao @ 2021-10-01 21:14 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Daniel Borkmann,
Alexei Starovoitov, Christophe Leroy, Johan Almbladh
Cc: bpf, linuxppc-dev
In-Reply-To: <cover.1633104510.git.naveen.n.rao@linux.vnet.ibm.com>
Add checks to ensure that we never emit branch instructions with
truncated branch offsets.
Suggested-by: Michael Ellerman <mpe@ellerman.id.au>
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/net/bpf_jit.h | 26 ++++++++++++++++++++------
arch/powerpc/net/bpf_jit_comp.c | 6 +++++-
arch/powerpc/net/bpf_jit_comp32.c | 8 ++++++--
arch/powerpc/net/bpf_jit_comp64.c | 8 ++++++--
4 files changed, 37 insertions(+), 11 deletions(-)
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 935ea95b66359e..7e9b978b768ed9 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -24,16 +24,30 @@
#define EMIT(instr) PLANT_INSTR(image, ctx->idx, instr)
/* Long jump; (unconditional 'branch') */
-#define PPC_JMP(dest) EMIT(PPC_INST_BRANCH | \
- (((dest) - (ctx->idx * 4)) & 0x03fffffc))
+#define PPC_JMP(dest) \
+ do { \
+ long offset = (long)(dest) - (ctx->idx * 4); \
+ if (!is_offset_in_branch_range(offset)) { \
+ pr_err_ratelimited("Branch offset 0x%lx (@%u) out of range\n", offset, ctx->idx); \
+ return -ERANGE; \
+ } \
+ EMIT(PPC_INST_BRANCH | (offset & 0x03fffffc)); \
+ } while (0)
+
/* blr; (unconditional 'branch' with link) to absolute address */
#define PPC_BL_ABS(dest) EMIT(PPC_INST_BL | \
(((dest) - (unsigned long)(image + ctx->idx)) & 0x03fffffc))
/* "cond" here covers BO:BI fields. */
-#define PPC_BCC_SHORT(cond, dest) EMIT(PPC_INST_BRANCH_COND | \
- (((cond) & 0x3ff) << 16) | \
- (((dest) - (ctx->idx * 4)) & \
- 0xfffc))
+#define PPC_BCC_SHORT(cond, dest) \
+ do { \
+ long offset = (long)(dest) - (ctx->idx * 4); \
+ if (!is_offset_in_cond_branch_range(offset)) { \
+ pr_err_ratelimited("Conditional branch offset 0x%lx (@%u) out of range\n", offset, ctx->idx); \
+ return -ERANGE; \
+ } \
+ EMIT(PPC_INST_BRANCH_COND | (((cond) & 0x3ff) << 16) | (offset & 0xfffc)); \
+ } while (0)
+
/* Sign-extended 32-bit immediate load */
#define PPC_LI32(d, i) do { \
if ((int)(uintptr_t)(i) >= -32768 && \
diff --git a/arch/powerpc/net/bpf_jit_comp.c b/arch/powerpc/net/bpf_jit_comp.c
index 53aefee3fe70be..fcbf7a917c566e 100644
--- a/arch/powerpc/net/bpf_jit_comp.c
+++ b/arch/powerpc/net/bpf_jit_comp.c
@@ -210,7 +210,11 @@ struct bpf_prog *bpf_int_jit_compile(struct bpf_prog *fp)
/* Now build the prologue, body code & epilogue for real. */
cgctx.idx = 0;
bpf_jit_build_prologue(code_base, &cgctx);
- bpf_jit_build_body(fp, code_base, &cgctx, addrs, extra_pass);
+ if (bpf_jit_build_body(fp, code_base, &cgctx, addrs, extra_pass)) {
+ bpf_jit_binary_free(bpf_hdr);
+ fp = org_fp;
+ goto out_addrs;
+ }
bpf_jit_build_epilogue(code_base, &cgctx);
if (bpf_jit_enable > 1)
diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c
index beb12cbc8c2994..a74d52204f8da2 100644
--- a/arch/powerpc/net/bpf_jit_comp32.c
+++ b/arch/powerpc/net/bpf_jit_comp32.c
@@ -200,7 +200,7 @@ void bpf_jit_emit_func_call_rel(u32 *image, struct codegen_context *ctx, u64 fun
}
}
-static void bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out)
+static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out)
{
/*
* By now, the eBPF program has already setup parameters in r3-r6
@@ -261,7 +261,9 @@ static void bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32
bpf_jit_emit_common_epilogue(image, ctx);
EMIT(PPC_RAW_BCTR());
+
/* out: */
+ return 0;
}
/* Assemble the body code between the prologue & epilogue */
@@ -1090,7 +1092,9 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
*/
case BPF_JMP | BPF_TAIL_CALL:
ctx->seen |= SEEN_TAILCALL;
- bpf_jit_emit_tail_call(image, ctx, addrs[i + 1]);
+ ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1]);
+ if (ret < 0)
+ return ret;
break;
default:
diff --git a/arch/powerpc/net/bpf_jit_comp64.c b/arch/powerpc/net/bpf_jit_comp64.c
index b87a63dba9c8fb..f06c62089b1457 100644
--- a/arch/powerpc/net/bpf_jit_comp64.c
+++ b/arch/powerpc/net/bpf_jit_comp64.c
@@ -206,7 +206,7 @@ void bpf_jit_emit_func_call_rel(u32 *image, struct codegen_context *ctx, u64 fun
EMIT(PPC_RAW_BCTRL());
}
-static void bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out)
+static int bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32 out)
{
/*
* By now, the eBPF program has already setup parameters in r3, r4 and r5
@@ -267,7 +267,9 @@ static void bpf_jit_emit_tail_call(u32 *image, struct codegen_context *ctx, u32
bpf_jit_emit_common_epilogue(image, ctx);
EMIT(PPC_RAW_BCTR());
+
/* out: */
+ return 0;
}
/* Assemble the body code between the prologue & epilogue */
@@ -993,7 +995,9 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context *
*/
case BPF_JMP | BPF_TAIL_CALL:
ctx->seen |= SEEN_TAILCALL;
- bpf_jit_emit_tail_call(image, ctx, addrs[i + 1]);
+ ret = bpf_jit_emit_tail_call(image, ctx, addrs[i + 1]);
+ if (ret < 0)
+ return ret;
break;
default:
--
2.33.0
^ permalink raw reply related
* [PATCH 1/9] powerpc/lib: Add helper to check if offset is within conditional branch range
From: Naveen N. Rao @ 2021-10-01 21:14 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Daniel Borkmann,
Alexei Starovoitov, Christophe Leroy, Johan Almbladh
Cc: bpf, linuxppc-dev
In-Reply-To: <cover.1633104510.git.naveen.n.rao@linux.vnet.ibm.com>
Add a helper to check if a given offset is within the branch range for a
powerpc conditional branch instruction, and update some sites to use the
new helper.
Signed-off-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
---
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/lib/code-patching.c | 7 ++++++-
arch/powerpc/net/bpf_jit.h | 7 +------
3 files changed, 8 insertions(+), 7 deletions(-)
diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index a95f63788c6b14..4ba834599c4d4c 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -23,6 +23,7 @@
#define BRANCH_ABSOLUTE 0x2
bool is_offset_in_branch_range(long offset);
+bool is_offset_in_cond_branch_range(long offset);
int create_branch(struct ppc_inst *instr, const u32 *addr,
unsigned long target, int flags);
int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index f9a3019e37b43c..e2342b9a1ab9c9 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -228,6 +228,11 @@ bool is_offset_in_branch_range(long offset)
return (offset >= -0x2000000 && offset <= 0x1fffffc && !(offset & 0x3));
}
+bool is_offset_in_cond_branch_range(long offset)
+{
+ return offset >= -0x8000 && offset <= 0x7FFF && !(offset & 0x3);
+}
+
/*
* Helper to check if a given instruction is a conditional branch
* Derived from the conditional checks in analyse_instr()
@@ -280,7 +285,7 @@ int create_cond_branch(struct ppc_inst *instr, const u32 *addr,
offset = offset - (unsigned long)addr;
/* Check we can represent the target in the instruction format */
- if (offset < -0x8000 || offset > 0x7FFF || offset & 0x3)
+ if (!is_offset_in_cond_branch_range(offset))
return 1;
/* Mask out the flags and target, so they don't step on each other. */
diff --git a/arch/powerpc/net/bpf_jit.h b/arch/powerpc/net/bpf_jit.h
index 99fad093f43ec1..935ea95b66359e 100644
--- a/arch/powerpc/net/bpf_jit.h
+++ b/arch/powerpc/net/bpf_jit.h
@@ -78,11 +78,6 @@
#define PPC_FUNC_ADDR(d,i) do { PPC_LI32(d, i); } while(0)
#endif
-static inline bool is_nearbranch(int offset)
-{
- return (offset < 32768) && (offset >= -32768);
-}
-
/*
* The fly in the ointment of code size changing from pass to pass is
* avoided by padding the short branch case with a NOP. If code size differs
@@ -91,7 +86,7 @@ static inline bool is_nearbranch(int offset)
* state.
*/
#define PPC_BCC(cond, dest) do { \
- if (is_nearbranch((dest) - (ctx->idx * 4))) { \
+ if (is_offset_in_cond_branch_range((long)(dest) - (ctx->idx * 4))) { \
PPC_BCC_SHORT(cond, dest); \
EMIT(PPC_RAW_NOP()); \
} else { \
--
2.33.0
^ permalink raw reply related
* [PATCH 0/9] powerpc/bpf: Various fixes
From: Naveen N. Rao @ 2021-10-01 21:14 UTC (permalink / raw)
To: Michael Ellerman, Nicholas Piggin, Daniel Borkmann,
Alexei Starovoitov, Christophe Leroy, Johan Almbladh
Cc: bpf, linuxppc-dev
Various fixes to the eBPF JIT for powerpc, thanks to some new tests
added by Johan. This series fixes all failures in test_bpf on powerpc64.
There are still some failures on powerpc32 to be looked into.
- Naveen
Naveen N. Rao (8):
powerpc/lib: Add helper to check if offset is within conditional
branch range
powerpc/bpf: Validate branch ranges
powerpc/bpf: Handle large branch ranges with BPF_EXIT
powerpc/bpf: Fix BPF_MOD when imm == 1
powerpc/bpf: Fix BPF_SUB when imm == 0x80000000
powerpc/bpf: Limit 'ldbrx' to processors compliant with ISA v2.06
powerpc/security: Add a helper to query stf_barrier type
powerpc/bpf: Emit stf barrier instruction sequences for BPF_NOSPEC
Ravi Bangoria (1):
powerpc/bpf: Remove unused SEEN_STACK
arch/powerpc/include/asm/code-patching.h | 1 +
arch/powerpc/include/asm/ppc-opcode.h | 1 +
arch/powerpc/include/asm/security_features.h | 5 +
arch/powerpc/kernel/security.c | 5 +
arch/powerpc/lib/code-patching.c | 7 +-
arch/powerpc/net/bpf_jit.h | 39 ++++---
arch/powerpc/net/bpf_jit64.h | 8 +-
arch/powerpc/net/bpf_jit_comp.c | 28 ++++-
arch/powerpc/net/bpf_jit_comp32.c | 10 +-
arch/powerpc/net/bpf_jit_comp64.c | 113 ++++++++++++++-----
10 files changed, 167 insertions(+), 50 deletions(-)
base-commit: 044c2d99d9f43c6d6fde8bed00672517dd9a5a57
--
2.33.0
^ permalink raw reply
* Re: [PATCH 0/5] convert ifc binding to yaml and drop "simple-bus"
From: Li Yang @ 2021-10-01 19:51 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linuxppc-dev, lkml, Rob Herring, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <4697aa5c-35de-8331-e7a9-831837618477@canonical.com>
On Fri, Oct 1, 2021 at 4:46 AM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 01/10/2021 02:09, Li Yang wrote:
> > Convert the ifc binding to yaml schema, in the mean while remove the
> > "simple-bus" compatible from the binding to make sure ifc device probes
> > before any of the child devices. Update the driver and existing DTSes
> > accordingly.
> >
> > DTS changes should be merged together with the driver/binding changes
> > if DTS maintainer is ok with it or after the driver changes are applied.
> >
>
> It's discouraged to merge DTS along with drivers (e.g. soc folks don't
> accept such pull requests), so I propose to apply it in the next cycle.
Ok. Will separate the DTS changes in the next version.
>
> Best regards,
> Krzysztof
^ permalink raw reply
* Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance
From: Ricardo Neri @ 2021-10-01 17:43 UTC (permalink / raw)
To: Vincent Guittot
Cc: Juri Lelli, Rafael J . Wysocki, Srikar Dronamraju,
Ravi V. Shankar, Peter Zijlstra (Intel), Ricardo Neri, Ben Segall,
Srinivas Pandruvada, Joel Fernandes (Google), Ingo Molnar,
Aubrey Li, Steven Rostedt, Mel Gorman, Len Brown, linuxppc-dev,
Nicholas Piggin, Aubrey Li, Dietmar Eggemann, Tim Chen,
kernelci-results@groups.io, Quentin Perret, Guillaume Tucker,
linux-kernel, Daniel Bristot de Oliveira
In-Reply-To: <CAKfTPtD1xE0amjCSkgczBjN=KbdVhVdK=6wiP=P+ynfGojky=Q@mail.gmail.com>
On Fri, Oct 01, 2021 at 12:25:42PM +0200, Vincent Guittot wrote:
> Hi Guillaume,
>
> This patch and the patchset which includes this patch only impacts
> systems with hyperthreading which is not the case of rk3328-rock64
> AFAICT. So there is no reason that this code is used by the board. The
> only impact should be an increase of the binary for this platform.
> Could it be that it reached a limit ?
>
> Regards,
> Vincent
>
> On Fri, 1 Oct 2021 at 11:33, Guillaume Tucker
> <guillaume.tucker@collabora.com> wrote:
> >
> > Hi Ricardo,
> >
> > Please see the bisection report below about a boot failure on
> > rk3288-rock64.
> >
> > Reports aren't automatically sent to the public while we're
> > trialing new bisection features on kernelci.org but this one
> > looks valid.
> >
> > Some more details can be found here:
> >
> > https://linux.kernelci.org/test/case/id/6155a4b0836c79a98f99a31d/
> >
> > It looks like some i.MX arm64 platforms also regressed with
> > next-20210920 although it hasn't been verified yet whether that's
> > due to the same commit:
> >
> > https://linux.kernelci.org/test/job/next/branch/master/kernel/next-20210930/plan/baseline/
> >
> > The x86 platforms don't seem to be impacted though.
> >
> > Please let us know if you need help debugging the issue or if you
> > have a fix to try.
Hi Guillaume,
I accessed the bug report above. It does not seem to include any kernel
log or crash. Maybe it hangs very early at boot? As Vicent mention, this
code is specific to hyperthreading. Furthermore, for this code path to
be executed the scheduling domains must have the SD_ASYM_PACKING flag.
AFAIK, only powerpc and x86 use this flag. Also, by the time this code
is executed, we should be past early boot. At least some messages should
have been printed to the kernel console.
Maybe Vincent's idea on the binary size is the issue?
Thanks and BR,
Ricardo
> >
> > Best wishes,
> > Guillaume
> >
> >
> > GitHub: https://github.com/kernelci/kernelci-project/issues/65
> >
> > -------------------------------------------------------------------------------
> >
> >
> > * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> > * This automated bisection report was sent to you on the basis *
> > * that you may be involved with the breaking commit it has *
> > * found. No manual investigation has been done to verify it, *
> > * and the root cause of the problem may be somewhere else. *
> > * *
> > * If you do send a fix, please include this trailer: *
> > * Reported-by: "kernelci.org bot" <bot@kernelci.org> *
> > * *
> > * Hope this helps! *
> > * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> >
> > next/master bisection: baseline.login on rk3328-rock64
> >
> > Summary:
> > Start: 2d02a18f75fc Add linux-next specific files for 20210929
> > Plain log: https://storage.kernelci.org/next/master/next-20210929/arm64/defconfig+CONFIG_RANDOMIZE_BASE=y/gcc-8/lab-baylibre/baseline-rk3328-rock64.txt
> > HTML log: https://storage.kernelci.org/next/master/next-20210929/arm64/defconfig+CONFIG_RANDOMIZE_BASE=y/gcc-8/lab-baylibre/baseline-rk3328-rock64.html
> > Result: eac6f3841f1d sched/fair: Consider SMT in ASYM_PACKING load balance
> >
> > Checks:
> > revert: PASS
> > verify: PASS
> >
> > Parameters:
> > Tree: next
> > URL: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> > Branch: master
> > Target: rk3328-rock64
> > CPU arch: arm64
> > Lab: lab-baylibre
> > Compiler: gcc-8
> > Config: defconfig+CONFIG_RANDOMIZE_BASE=y
> > Test case: baseline.login
> >
> > Breaking commit found:
> >
> > -------------------------------------------------------------------------------
> > commit eac6f3841f1dac7b6f43002056b63f44cc1f1543
> > Author: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > Date: Fri Sep 10 18:18:19 2021 -0700
> >
> > sched/fair: Consider SMT in ASYM_PACKING load balance
> >
> >
> > On 11/09/2021 03:18, Ricardo Neri wrote:
> > > When deciding to pull tasks in ASYM_PACKING, it is necessary not only to
> > > check for the idle state of the destination CPU, dst_cpu, but also of
> > > its SMT siblings.
> > >
> > > If dst_cpu is idle but its SMT siblings are busy, performance suffers
> > > if it pulls tasks from a medium priority CPU that does not have SMT
> > > siblings.
> > >
> > > Implement asym_smt_can_pull_tasks() to inspect the state of the SMT
> > > siblings of both dst_cpu and the CPUs in the candidate busiest group.
> > >
> > > Cc: Aubrey Li <aubrey.li@intel.com>
> > > Cc: Ben Segall <bsegall@google.com>
> > > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > > Cc: Mel Gorman <mgorman@suse.de>
> > > Cc: Quentin Perret <qperret@google.com>
> > > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > > Cc: Steven Rostedt <rostedt@goodmis.org>
> > > Cc: Tim Chen <tim.c.chen@linux.intel.com>
> > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > Reviewed-by: Len Brown <len.brown@intel.com>
> > > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > > ---
> > > Changes since v4:
> > > * Use sg_lb_stats::sum_nr_running the idle state of a scheduling group.
> > > (Vincent, Peter)
> > > * Do not even idle CPUs in asym_smt_can_pull_tasks(). (Vincent)
> > > * Updated function documentation and corrected a typo.
> > >
> > > Changes since v3:
> > > * Removed the arch_asym_check_smt_siblings() hook. Discussions with the
> > > powerpc folks showed that this patch should not impact them. Also, more
> > > recent powerpc processor no longer use asym_packing. (PeterZ)
> > > * Removed unnecessary local variable in asym_can_pull_tasks(). (Dietmar)
> > > * Removed unnecessary check for local CPUs when the local group has zero
> > > utilization. (Joel)
> > > * Renamed asym_can_pull_tasks() as asym_smt_can_pull_tasks() to reflect
> > > the fact that it deals with SMT cases.
> > > * Made asym_smt_can_pull_tasks() return false for !CONFIG_SCHED_SMT so
> > > that callers can deal with non-SMT cases.
> > >
> > > Changes since v2:
> > > * Reworded the commit message to reflect updates in code.
> > > * Corrected misrepresentation of dst_cpu as the CPU doing the load
> > > balancing. (PeterZ)
> > > * Removed call to arch_asym_check_smt_siblings() as it is now called in
> > > sched_asym().
> > >
> > > Changes since v1:
> > > * Don't bailout in update_sd_pick_busiest() if dst_cpu cannot pull
> > > tasks. Instead, reclassify the candidate busiest group, as it
> > > may still be selected. (PeterZ)
> > > * Avoid an expensive and unnecessary call to cpumask_weight() when
> > > determining if a sched_group is comprised of SMT siblings.
> > > (PeterZ).
> > > ---
> > > kernel/sched/fair.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 94 insertions(+)
> > >
> > > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > > index 26db017c14a3..8d763dd0174b 100644
> > > --- a/kernel/sched/fair.c
> > > +++ b/kernel/sched/fair.c
> > > @@ -8597,10 +8597,98 @@ group_type group_classify(unsigned int imbalance_pct,
> > > return group_has_spare;
> > > }
> > >
> > > +/**
> > > + * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks
> > > + * @dst_cpu: Destination CPU of the load balancing
> > > + * @sds: Load-balancing data with statistics of the local group
> > > + * @sgs: Load-balancing statistics of the candidate busiest group
> > > + * @sg: The candidate busiest group
> > > + *
> > > + * Check the state of the SMT siblings of both @sds::local and @sg and decide
> > > + * if @dst_cpu can pull tasks.
> > > + *
> > > + * If @dst_cpu does not have SMT siblings, it can pull tasks if two or more of
> > > + * the SMT siblings of @sg are busy. If only one CPU in @sg is busy, pull tasks
> > > + * only if @dst_cpu has higher priority.
> > > + *
> > > + * If both @dst_cpu and @sg have SMT siblings, and @sg has exactly one more
> > > + * busy CPU than @sds::local, let @dst_cpu pull tasks if it has higher priority.
> > > + * Bigger imbalances in the number of busy CPUs will be dealt with in
> > > + * update_sd_pick_busiest().
> > > + *
> > > + * If @sg does not have SMT siblings, only pull tasks if all of the SMT siblings
> > > + * of @dst_cpu are idle and @sg has lower priority.
> > > + */
> > > +static bool asym_smt_can_pull_tasks(int dst_cpu, struct sd_lb_stats *sds,
> > > + struct sg_lb_stats *sgs,
> > > + struct sched_group *sg)
> > > +{
> > > +#ifdef CONFIG_SCHED_SMT
> > > + bool local_is_smt, sg_is_smt;
> > > + int sg_busy_cpus;
> > > +
> > > + local_is_smt = sds->local->flags & SD_SHARE_CPUCAPACITY;
> > > + sg_is_smt = sg->flags & SD_SHARE_CPUCAPACITY;
> > > +
> > > + sg_busy_cpus = sgs->group_weight - sgs->idle_cpus;
> > > +
> > > + if (!local_is_smt) {
> > > + /*
> > > + * If we are here, @dst_cpu is idle and does not have SMT
> > > + * siblings. Pull tasks if candidate group has two or more
> > > + * busy CPUs.
> > > + */
> > > + if (sg_is_smt && sg_busy_cpus >= 2)
> > > + return true;
> > > +
> > > + /*
> > > + * @dst_cpu does not have SMT siblings. @sg may have SMT
> > > + * siblings and only one is busy. In such case, @dst_cpu
> > > + * can help if it has higher priority and is idle (i.e.,
> > > + * it has no running tasks).
> > > + */
> > > + return !sds->local_stat.sum_nr_running &&
> > > + sched_asym_prefer(dst_cpu, sg->asym_prefer_cpu);
> > > + }
> > > +
> > > + /* @dst_cpu has SMT siblings. */
> > > +
> > > + if (sg_is_smt) {
> > > + int local_busy_cpus = sds->local->group_weight -
> > > + sds->local_stat.idle_cpus;
> > > + int busy_cpus_delta = sg_busy_cpus - local_busy_cpus;
> > > +
> > > + if (busy_cpus_delta == 1)
> > > + return sched_asym_prefer(dst_cpu,
> > > + sg->asym_prefer_cpu);
> > > +
> > > + return false;
> > > + }
> > > +
> > > + /*
> > > + * @sg does not have SMT siblings. Ensure that @sds::local does not end
> > > + * up with more than one busy SMT sibling and only pull tasks if there
> > > + * are not busy CPUs (i.e., no CPU has running tasks).
> > > + */
> > > + if (!sds->local_stat.sum_nr_running)
> > > + return sched_asym_prefer(dst_cpu, sg->asym_prefer_cpu);
> > > +
> > > + return false;
> > > +#else
> > > + /* Always return false so that callers deal with non-SMT cases. */
> > > + return false;
> > > +#endif
> > > +}
> > > +
> > > static inline bool
> > > sched_asym(struct lb_env *env, struct sd_lb_stats *sds, struct sg_lb_stats *sgs,
> > > struct sched_group *group)
> > > {
> > > + /* Only do SMT checks if either local or candidate have SMT siblings */
> > > + if ((sds->local->flags & SD_SHARE_CPUCAPACITY) ||
> > > + (group->flags & SD_SHARE_CPUCAPACITY))
> > > + return asym_smt_can_pull_tasks(env->dst_cpu, sds, sgs, group);
> > > +
> > > return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
> > > }
> > >
> > > @@ -9606,6 +9694,12 @@ static struct rq *find_busiest_queue(struct lb_env *env,
> > > nr_running == 1)
> > > continue;
> > >
> > > + /* Make sure we only pull tasks from a CPU of lower priority */
> > > + if ((env->sd->flags & SD_ASYM_PACKING) &&
> > > + sched_asym_prefer(i, env->dst_cpu) &&
> > > + nr_running == 1)
> > > + continue;
> > > +
> > > switch (env->migration_type) {
> > > case migrate_load:
> > > /*
> > >
> >
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: memory: fsl: convert ifc binding to yaml schema
From: Rob Herring @ 2021-10-01 16:35 UTC (permalink / raw)
To: Li Yang
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linuxppc-dev, lkml, Krzysztof Kozlowski, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <CADRPPNQBR63pS60nmfUQx02GbBoWEbgroU5Zw-iY62TodmB91Q@mail.gmail.com>
On Fri, Oct 1, 2021 at 11:29 AM Li Yang <leoyang.li@nxp.com> wrote:
>
> On Fri, Oct 1, 2021 at 8:18 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, 30 Sep 2021 19:09:20 -0500, Li Yang wrote:
> > > Convert the txt binding to yaml format and add description. Drop the
> > > "simple-bus" compatible string from the example and not allowed by the
> > > binding any more. This will help to enforce the correct probe order
> > > between parent device and child devices, but will require the ifc driver
> > > to probe the child devices to work properly.
> > >
> > > Signed-off-by: Li Yang <leoyang.li@nxp.com>
> > > ---
> > > updates from previous submission:
> > > - Drop "simple-bus" from binding and only "fsl,ifc" as compatible
> > > - Fix one identiation problem of "reg"
> > > - Add type restriction to "little-endian" property
> > >
> > > .../bindings/memory-controllers/fsl/ifc.txt | 82 -----------
> > > .../bindings/memory-controllers/fsl/ifc.yaml | 137 ++++++++++++++++++
> > > 2 files changed, 137 insertions(+), 82 deletions(-)
> > > delete mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> > > create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
> > >
> >
> > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> > on your patch (DT_CHECKER_FLAGS is new in v5.13):
> >
> > yamllint warnings/errors:
> >
> > dtschema/dtc warnings/errors:
> > Documentation/devicetree/bindings/memory-controllers/fsl/ifc.example.dt.yaml:0:0: /example-0/soc/ifc@ffe1e000/flash@1,0: failed to match any schema with compatible: ['fsl,ifc-nand']
> > Documentation/devicetree/bindings/memory-controllers/fsl/ifc.example.dt.yaml:0:0: /example-0/soc/ifc@ffe1e000/cpld@3,0: failed to match any schema with compatible: ['fsl,p1010rdb-cpld']
>
> These are defined in other bindings, but unfortunately they are not
> converted to yaml format yet.
Yes, I know. I'm trying to turn on this check by default and adding
more cases here doesn't help. And often, when those other bindings get
converted, it's the example here that has errors and has to get fixed.
So either convert those bindings too or drop them from the example.
Rob
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: memory: fsl: convert ifc binding to yaml schema
From: Li Yang @ 2021-10-01 16:29 UTC (permalink / raw)
To: Rob Herring
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linuxppc-dev, lkml, Krzysztof Kozlowski, Rob Herring, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <1633094217.843390.3666440.nullmailer@robh.at.kernel.org>
On Fri, Oct 1, 2021 at 8:18 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, 30 Sep 2021 19:09:20 -0500, Li Yang wrote:
> > Convert the txt binding to yaml format and add description. Drop the
> > "simple-bus" compatible string from the example and not allowed by the
> > binding any more. This will help to enforce the correct probe order
> > between parent device and child devices, but will require the ifc driver
> > to probe the child devices to work properly.
> >
> > Signed-off-by: Li Yang <leoyang.li@nxp.com>
> > ---
> > updates from previous submission:
> > - Drop "simple-bus" from binding and only "fsl,ifc" as compatible
> > - Fix one identiation problem of "reg"
> > - Add type restriction to "little-endian" property
> >
> > .../bindings/memory-controllers/fsl/ifc.txt | 82 -----------
> > .../bindings/memory-controllers/fsl/ifc.yaml | 137 ++++++++++++++++++
> > 2 files changed, 137 insertions(+), 82 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> > create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
> >
>
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
>
> yamllint warnings/errors:
>
> dtschema/dtc warnings/errors:
> Documentation/devicetree/bindings/memory-controllers/fsl/ifc.example.dt.yaml:0:0: /example-0/soc/ifc@ffe1e000/flash@1,0: failed to match any schema with compatible: ['fsl,ifc-nand']
> Documentation/devicetree/bindings/memory-controllers/fsl/ifc.example.dt.yaml:0:0: /example-0/soc/ifc@ffe1e000/cpld@3,0: failed to match any schema with compatible: ['fsl,p1010rdb-cpld']
These are defined in other bindings, but unfortunately they are not
converted to yaml format yet.
>
> doc reference errors (make refcheckdocs):
>
> See https://patchwork.ozlabs.org/patch/1535102
>
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
>
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
>
> pip3 install dtschema --upgrade
>
> Please check and re-submit.
>
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: memory: fsl: convert ifc binding to yaml schema
From: Li Yang @ 2021-10-01 16:17 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linuxppc-dev, lkml, Rob Herring, Shawn Guo,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <db751cb1-9107-3857-7576-644bde4c28e5@canonical.com>
On Fri, Oct 1, 2021 at 5:01 AM Krzysztof Kozlowski
<krzysztof.kozlowski@canonical.com> wrote:
>
> On 01/10/2021 02:09, Li Yang wrote:
> > Convert the txt binding to yaml format and add description. Drop the
> > "simple-bus" compatible string from the example and not allowed by the
> > binding any more. This will help to enforce the correct probe order
> > between parent device and child devices, but will require the ifc driver
> > to probe the child devices to work properly.
> >
> > Signed-off-by: Li Yang <leoyang.li@nxp.com>
> > ---
> > updates from previous submission:
> > - Drop "simple-bus" from binding and only "fsl,ifc" as compatible
> > - Fix one identiation problem of "reg"
> > - Add type restriction to "little-endian" property
> >
> > .../bindings/memory-controllers/fsl/ifc.txt | 82 -----------
> > .../bindings/memory-controllers/fsl/ifc.yaml | 137 ++++++++++++++++++
> > 2 files changed, 137 insertions(+), 82 deletions(-)
> > delete mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> > create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> > deleted file mode 100644
> > index 89427b018ba7..000000000000
> > --- a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> > +++ /dev/null
> > @@ -1,82 +0,0 @@
> > -Integrated Flash Controller
> > -
> > -Properties:
> > -- name : Should be ifc
> > -- compatible : should contain "fsl,ifc". The version of the integrated
> > - flash controller can be found in the IFC_REV register at
> > - offset zero.
> > -
> > -- #address-cells : Should be either two or three. The first cell is the
> > - chipselect number, and the remaining cells are the
> > - offset into the chipselect.
> > -- #size-cells : Either one or two, depending on how large each chipselect
> > - can be.
> > -- reg : Offset and length of the register set for the device
> > -- interrupts: IFC may have one or two interrupts. If two interrupt
> > - specifiers are present, the first is the "common"
> > - interrupt (CM_EVTER_STAT), and the second is the NAND
> > - interrupt (NAND_EVTER_STAT). If there is only one,
> > - that interrupt reports both types of event.
> > -
> > -- little-endian : If this property is absent, the big-endian mode will
> > - be in use as default for registers.
> > -
> > -- ranges : Each range corresponds to a single chipselect, and covers
> > - the entire access window as configured.
> > -
> > -Child device nodes describe the devices connected to IFC such as NOR (e.g.
> > -cfi-flash) and NAND (fsl,ifc-nand). There might be board specific devices
> > -like FPGAs, CPLDs, etc.
> > -
> > -Example:
> > -
> > - ifc@ffe1e000 {
> > - compatible = "fsl,ifc", "simple-bus";
> > - #address-cells = <2>;
> > - #size-cells = <1>;
> > - reg = <0x0 0xffe1e000 0 0x2000>;
> > - interrupts = <16 2 19 2>;
> > - little-endian;
> > -
> > - /* NOR, NAND Flashes and CPLD on board */
> > - ranges = <0x0 0x0 0x0 0xee000000 0x02000000
> > - 0x1 0x0 0x0 0xffa00000 0x00010000
> > - 0x3 0x0 0x0 0xffb00000 0x00020000>;
> > -
> > - flash@0,0 {
> > - #address-cells = <1>;
> > - #size-cells = <1>;
> > - compatible = "cfi-flash";
> > - reg = <0x0 0x0 0x2000000>;
> > - bank-width = <2>;
> > - device-width = <1>;
> > -
> > - partition@0 {
> > - /* 32MB for user data */
> > - reg = <0x0 0x02000000>;
> > - label = "NOR Data";
> > - };
> > - };
> > -
> > - flash@1,0 {
> > - #address-cells = <1>;
> > - #size-cells = <1>;
> > - compatible = "fsl,ifc-nand";
> > - reg = <0x1 0x0 0x10000>;
> > -
> > - partition@0 {
> > - /* This location must not be altered */
> > - /* 1MB for u-boot Bootloader Image */
> > - reg = <0x0 0x00100000>;
> > - label = "NAND U-Boot Image";
> > - read-only;
> > - };
> > - };
> > -
> > - cpld@3,0 {
> > - #address-cells = <1>;
> > - #size-cells = <1>;
> > - compatible = "fsl,p1010rdb-cpld";
> > - reg = <0x3 0x0 0x000001f>;
> > - };
> > - };
> > diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
> > new file mode 100644
> > index 000000000000..19871ce39fe3
>
> Thanks for the patch.
>
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
> > @@ -0,0 +1,137 @@
> > +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
>
> Checkpatch should scream here. If it doesn't, maybe you work on some old
> tree, which would also explain why you send it to my old address (not
> the one from get_maintainers). Please use both checkpatch and
> get_maintainers.
>
> You basically relicense bindings from GPL-2.0 only to new license,
> including GPL-3.0.
Ok. Will update the license.
>
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/memory-controllers/fsl/ifc.yaml#
>
> File name should be "fsl,ifc.yaml"
Ok. But probably it is a little bit redundant as the upper level
folder also has fsl.
>
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: FSL/NXP Integrated Flash Controller
> > +
> > +maintainers:
> > + - Li Yang <leoyang.li@nxp.com>
> > +
> > +description: |
> > + NXP's integrated flash controller (IFC) is an advanced version of the
> > + enhanced local bus controller which includes similar programming and signal
> > + interfaces with an extended feature set. The IFC provides access to multiple
> > + external memory types, such as NAND flash (SLC and MLC), NOR flash, EPROM,
> > + SRAM and other memories where address and data are shared on a bus.
> > +
> > +properties:
> > + $nodename:
> > + pattern: "^ifc@[0-9a-f]+$"
>
> Nodes should be generic, so this looks like "memory-controller".
Ok.
>
> > +
> > + compatible:
> > + const: fsl,ifc
> > +
> > + "#address-cells":
> > + enum: [2, 3]
> > + description: |
> > + Should be either two or three. The first cell is the chipselect
> > + number, and the remaining cells are the offset into the chipselect.
> > +
> > + "#size-cells":
> > + enum: [1, 2]
> > + description: |
> > + Either one or two, depending on how large each chipselect can be.
> > +
> > + reg:
> > + maxItems: 1
> > + description: |
> > + Offset and length of the register set for the device.
>
> Skip the description, it's obvious.
Ok.
>
> > +
> > + interrupts:
> > + minItems: 1
> > + maxItems: 2
> > + description: |
> > + IFC may have one or two interrupts. If two interrupt specifiers are
> > + present, the first is the "common" interrupt (CM_EVTER_STAT), and the
> > + second is the NAND interrupt (NAND_EVTER_STAT). If there is only one,
> > + that interrupt reports both types of event.
> > +
> > + little-endian:
> > + $ref: '/schemas/types.yaml#/definitions/flag'
>
> type: boolean
It will not have a true or false value, but only present or not. Is
the boolean type taking care of this too?
>
>
> Best regards,
> Krzysztof
^ permalink raw reply
* Re: [PATCH kernel] powerpc/iommu: Report the correct most efficient DMA mask for PCI devices
From: Michael Ellerman @ 2021-10-01 14:36 UTC (permalink / raw)
To: Alexey Kardashevskiy, linuxppc-dev; +Cc: Carol L Soto, iommu, Christoph Hellwig
In-Reply-To: <20210930034454.95794-1-aik@ozlabs.ru>
On Thu, 30 Sep 2021 13:44:54 +1000, Alexey Kardashevskiy wrote:
> According to dma-api.rst, the dma_get_required_mask() helper should return
> "the mask that the platform requires to operate efficiently". Which in
> the case of PPC64 means the bypass mask and not a mask from an IOMMU table
> which is shorter and slower to use due to map/unmap operations (especially
> expensive on "pseries").
>
> However the existing implementation ignores the possibility of bypassing
> and returns the IOMMU table mask on the pseries platform which makes some
> drivers (mpt3sas is one example) choose 32bit DMA even though bypass is
> supported. The powernv platform sort of handles it by having a bigger
> default window with a mask >=40 but it only works as drivers choose
> 63/64bit if the required mask is >32 which is rather pointless.
>
> [...]
Applied to powerpc/fixes.
[1/1] powerpc/iommu: Report the correct most efficient DMA mask for PCI devices
https://git.kernel.org/powerpc/c/23c216b335d1fbd716076e8263b54a714ea3cf0e
cheers
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: memory: fsl: convert ifc binding to yaml schema
From: Rob Herring @ 2021-10-01 13:16 UTC (permalink / raw)
To: Li Yang
Cc: devicetree, linuxppc-dev, linux-kernel, Krzysztof Kozlowski,
Rob Herring, Shawn Guo, linux-arm-kernel
In-Reply-To: <20211001000924.15421-2-leoyang.li@nxp.com>
On Thu, 30 Sep 2021 19:09:20 -0500, Li Yang wrote:
> Convert the txt binding to yaml format and add description. Drop the
> "simple-bus" compatible string from the example and not allowed by the
> binding any more. This will help to enforce the correct probe order
> between parent device and child devices, but will require the ifc driver
> to probe the child devices to work properly.
>
> Signed-off-by: Li Yang <leoyang.li@nxp.com>
> ---
> updates from previous submission:
> - Drop "simple-bus" from binding and only "fsl,ifc" as compatible
> - Fix one identiation problem of "reg"
> - Add type restriction to "little-endian" property
>
> .../bindings/memory-controllers/fsl/ifc.txt | 82 -----------
> .../bindings/memory-controllers/fsl/ifc.yaml | 137 ++++++++++++++++++
> 2 files changed, 137 insertions(+), 82 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
>
My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):
yamllint warnings/errors:
dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/memory-controllers/fsl/ifc.example.dt.yaml:0:0: /example-0/soc/ifc@ffe1e000/flash@1,0: failed to match any schema with compatible: ['fsl,ifc-nand']
Documentation/devicetree/bindings/memory-controllers/fsl/ifc.example.dt.yaml:0:0: /example-0/soc/ifc@ffe1e000/cpld@3,0: failed to match any schema with compatible: ['fsl,p1010rdb-cpld']
doc reference errors (make refcheckdocs):
See https://patchwork.ozlabs.org/patch/1535102
This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.
If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:
pip3 install dtschema --upgrade
Please check and re-submit.
^ permalink raw reply
* Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance
From: Guillaume Tucker @ 2021-10-01 9:33 UTC (permalink / raw)
To: Ricardo Neri, Peter Zijlstra (Intel), Ingo Molnar, Juri Lelli,
Vincent Guittot
Cc: Len Brown, Rafael J . Wysocki, Srikar Dronamraju,
kernelci-results@groups.io, Ravi V. Shankar, linuxppc-dev,
Aubrey Li, Nicholas Piggin, Ricardo Neri, Steven Rostedt,
Quentin Perret, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Srinivas Pandruvada,
Joel Fernandes (Google), Tim Chen, Dietmar Eggemann, linux-kernel,
Aubrey Li
In-Reply-To: <20210911011819.12184-7-ricardo.neri-calderon@linux.intel.com>
Hi Ricardo,
Please see the bisection report below about a boot failure on
rk3288-rock64.
Reports aren't automatically sent to the public while we're
trialing new bisection features on kernelci.org but this one
looks valid.
Some more details can be found here:
https://linux.kernelci.org/test/case/id/6155a4b0836c79a98f99a31d/
It looks like some i.MX arm64 platforms also regressed with
next-20210920 although it hasn't been verified yet whether that's
due to the same commit:
https://linux.kernelci.org/test/job/next/branch/master/kernel/next-20210930/plan/baseline/
The x86 platforms don't seem to be impacted though.
Please let us know if you need help debugging the issue or if you
have a fix to try.
Best wishes,
Guillaume
GitHub: https://github.com/kernelci/kernelci-project/issues/65
-------------------------------------------------------------------------------
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
* This automated bisection report was sent to you on the basis *
* that you may be involved with the breaking commit it has *
* found. No manual investigation has been done to verify it, *
* and the root cause of the problem may be somewhere else. *
* *
* If you do send a fix, please include this trailer: *
* Reported-by: "kernelci.org bot" <bot@kernelci.org> *
* *
* Hope this helps! *
* * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
next/master bisection: baseline.login on rk3328-rock64
Summary:
Start: 2d02a18f75fc Add linux-next specific files for 20210929
Plain log: https://storage.kernelci.org/next/master/next-20210929/arm64/defconfig+CONFIG_RANDOMIZE_BASE=y/gcc-8/lab-baylibre/baseline-rk3328-rock64.txt
HTML log: https://storage.kernelci.org/next/master/next-20210929/arm64/defconfig+CONFIG_RANDOMIZE_BASE=y/gcc-8/lab-baylibre/baseline-rk3328-rock64.html
Result: eac6f3841f1d sched/fair: Consider SMT in ASYM_PACKING load balance
Checks:
revert: PASS
verify: PASS
Parameters:
Tree: next
URL: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
Branch: master
Target: rk3328-rock64
CPU arch: arm64
Lab: lab-baylibre
Compiler: gcc-8
Config: defconfig+CONFIG_RANDOMIZE_BASE=y
Test case: baseline.login
Breaking commit found:
-------------------------------------------------------------------------------
commit eac6f3841f1dac7b6f43002056b63f44cc1f1543
Author: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
Date: Fri Sep 10 18:18:19 2021 -0700
sched/fair: Consider SMT in ASYM_PACKING load balance
On 11/09/2021 03:18, Ricardo Neri wrote:
> When deciding to pull tasks in ASYM_PACKING, it is necessary not only to
> check for the idle state of the destination CPU, dst_cpu, but also of
> its SMT siblings.
>
> If dst_cpu is idle but its SMT siblings are busy, performance suffers
> if it pulls tasks from a medium priority CPU that does not have SMT
> siblings.
>
> Implement asym_smt_can_pull_tasks() to inspect the state of the SMT
> siblings of both dst_cpu and the CPUs in the candidate busiest group.
>
> Cc: Aubrey Li <aubrey.li@intel.com>
> Cc: Ben Segall <bsegall@google.com>
> Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> Cc: Mel Gorman <mgorman@suse.de>
> Cc: Quentin Perret <qperret@google.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Tim Chen <tim.c.chen@linux.intel.com>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Reviewed-by: Len Brown <len.brown@intel.com>
> Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> ---
> Changes since v4:
> * Use sg_lb_stats::sum_nr_running the idle state of a scheduling group.
> (Vincent, Peter)
> * Do not even idle CPUs in asym_smt_can_pull_tasks(). (Vincent)
> * Updated function documentation and corrected a typo.
>
> Changes since v3:
> * Removed the arch_asym_check_smt_siblings() hook. Discussions with the
> powerpc folks showed that this patch should not impact them. Also, more
> recent powerpc processor no longer use asym_packing. (PeterZ)
> * Removed unnecessary local variable in asym_can_pull_tasks(). (Dietmar)
> * Removed unnecessary check for local CPUs when the local group has zero
> utilization. (Joel)
> * Renamed asym_can_pull_tasks() as asym_smt_can_pull_tasks() to reflect
> the fact that it deals with SMT cases.
> * Made asym_smt_can_pull_tasks() return false for !CONFIG_SCHED_SMT so
> that callers can deal with non-SMT cases.
>
> Changes since v2:
> * Reworded the commit message to reflect updates in code.
> * Corrected misrepresentation of dst_cpu as the CPU doing the load
> balancing. (PeterZ)
> * Removed call to arch_asym_check_smt_siblings() as it is now called in
> sched_asym().
>
> Changes since v1:
> * Don't bailout in update_sd_pick_busiest() if dst_cpu cannot pull
> tasks. Instead, reclassify the candidate busiest group, as it
> may still be selected. (PeterZ)
> * Avoid an expensive and unnecessary call to cpumask_weight() when
> determining if a sched_group is comprised of SMT siblings.
> (PeterZ).
> ---
> kernel/sched/fair.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 94 insertions(+)
>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 26db017c14a3..8d763dd0174b 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8597,10 +8597,98 @@ group_type group_classify(unsigned int imbalance_pct,
> return group_has_spare;
> }
>
> +/**
> + * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks
> + * @dst_cpu: Destination CPU of the load balancing
> + * @sds: Load-balancing data with statistics of the local group
> + * @sgs: Load-balancing statistics of the candidate busiest group
> + * @sg: The candidate busiest group
> + *
> + * Check the state of the SMT siblings of both @sds::local and @sg and decide
> + * if @dst_cpu can pull tasks.
> + *
> + * If @dst_cpu does not have SMT siblings, it can pull tasks if two or more of
> + * the SMT siblings of @sg are busy. If only one CPU in @sg is busy, pull tasks
> + * only if @dst_cpu has higher priority.
> + *
> + * If both @dst_cpu and @sg have SMT siblings, and @sg has exactly one more
> + * busy CPU than @sds::local, let @dst_cpu pull tasks if it has higher priority.
> + * Bigger imbalances in the number of busy CPUs will be dealt with in
> + * update_sd_pick_busiest().
> + *
> + * If @sg does not have SMT siblings, only pull tasks if all of the SMT siblings
> + * of @dst_cpu are idle and @sg has lower priority.
> + */
> +static bool asym_smt_can_pull_tasks(int dst_cpu, struct sd_lb_stats *sds,
> + struct sg_lb_stats *sgs,
> + struct sched_group *sg)
> +{
> +#ifdef CONFIG_SCHED_SMT
> + bool local_is_smt, sg_is_smt;
> + int sg_busy_cpus;
> +
> + local_is_smt = sds->local->flags & SD_SHARE_CPUCAPACITY;
> + sg_is_smt = sg->flags & SD_SHARE_CPUCAPACITY;
> +
> + sg_busy_cpus = sgs->group_weight - sgs->idle_cpus;
> +
> + if (!local_is_smt) {
> + /*
> + * If we are here, @dst_cpu is idle and does not have SMT
> + * siblings. Pull tasks if candidate group has two or more
> + * busy CPUs.
> + */
> + if (sg_is_smt && sg_busy_cpus >= 2)
> + return true;
> +
> + /*
> + * @dst_cpu does not have SMT siblings. @sg may have SMT
> + * siblings and only one is busy. In such case, @dst_cpu
> + * can help if it has higher priority and is idle (i.e.,
> + * it has no running tasks).
> + */
> + return !sds->local_stat.sum_nr_running &&
> + sched_asym_prefer(dst_cpu, sg->asym_prefer_cpu);
> + }
> +
> + /* @dst_cpu has SMT siblings. */
> +
> + if (sg_is_smt) {
> + int local_busy_cpus = sds->local->group_weight -
> + sds->local_stat.idle_cpus;
> + int busy_cpus_delta = sg_busy_cpus - local_busy_cpus;
> +
> + if (busy_cpus_delta == 1)
> + return sched_asym_prefer(dst_cpu,
> + sg->asym_prefer_cpu);
> +
> + return false;
> + }
> +
> + /*
> + * @sg does not have SMT siblings. Ensure that @sds::local does not end
> + * up with more than one busy SMT sibling and only pull tasks if there
> + * are not busy CPUs (i.e., no CPU has running tasks).
> + */
> + if (!sds->local_stat.sum_nr_running)
> + return sched_asym_prefer(dst_cpu, sg->asym_prefer_cpu);
> +
> + return false;
> +#else
> + /* Always return false so that callers deal with non-SMT cases. */
> + return false;
> +#endif
> +}
> +
> static inline bool
> sched_asym(struct lb_env *env, struct sd_lb_stats *sds, struct sg_lb_stats *sgs,
> struct sched_group *group)
> {
> + /* Only do SMT checks if either local or candidate have SMT siblings */
> + if ((sds->local->flags & SD_SHARE_CPUCAPACITY) ||
> + (group->flags & SD_SHARE_CPUCAPACITY))
> + return asym_smt_can_pull_tasks(env->dst_cpu, sds, sgs, group);
> +
> return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
> }
>
> @@ -9606,6 +9694,12 @@ static struct rq *find_busiest_queue(struct lb_env *env,
> nr_running == 1)
> continue;
>
> + /* Make sure we only pull tasks from a CPU of lower priority */
> + if ((env->sd->flags & SD_ASYM_PACKING) &&
> + sched_asym_prefer(i, env->dst_cpu) &&
> + nr_running == 1)
> + continue;
> +
> switch (env->migration_type) {
> case migrate_load:
> /*
>
^ permalink raw reply
* Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance
From: Guillaume Tucker @ 2021-10-01 9:40 UTC (permalink / raw)
To: Ricardo Neri, Peter Zijlstra (Intel), Ingo Molnar, Juri Lelli,
Vincent Guittot
Cc: Len Brown, Rafael J . Wysocki, Srikar Dronamraju,
kernelci-results@groups.io, Ravi V. Shankar, linuxppc-dev,
Aubrey Li, Nicholas Piggin, Ricardo Neri, Steven Rostedt,
Quentin Perret, Ben Segall, Mel Gorman,
Daniel Bristot de Oliveira, Srinivas Pandruvada,
Joel Fernandes (Google), Tim Chen, Dietmar Eggemann, linux-kernel,
Aubrey Li
In-Reply-To: <78608a82-93b8-8036-2bf0-65f53f2f5120@collabora.com>
On 01/10/2021 11:33, Guillaume Tucker wrote:
> Please see the bisection report below about a boot failure on
> rk3288-rock64.
Sorry, I meant rk3328-rock64.
Guillaume
^ permalink raw reply
* Re: [V2 2/4] tools/perf: Refactor the code definition of perf reg extended mask in tools side header file
From: Michael Ellerman @ 2021-10-01 11:29 UTC (permalink / raw)
To: Daniel Axtens, Athira Rajeev, acme, jolsa
Cc: kjain, maddy, linuxppc-dev, rnsastry
In-Reply-To: <87o889mfl3.fsf@linkitivity.dja.id.au>
Daniel Axtens <dja@axtens.net> writes:
> Hi Athira,
>
>> PERF_REG_PMU_MASK_300 and PERF_REG_PMU_MASK_31 defines the mask
>> value for extended registers. Current definition of these mask values
>> uses hex constant and does not use registers by name, making it less
>> readable. Patch refactor the macro values in perf tools side header file
>> by or'ing together the actual register value constants.
>>
> This looks like a good simplification.
>
>> -/* Exclude MMCR3, SIER2, SIER3 for CPU_FTR_ARCH_300 */
>> -#define PERF_EXCLUDE_REG_EXT_300 (7ULL << PERF_REG_POWERPC_MMCR3)
>
> This file is uAPI - are we allowed to remove a define? Could a program
> built against these headers now fail to compile because we've removed it?
Yeah that's true.
In this case I think I'd rather we remove it though, because:
- it was never meant to be part of the uapi, it was just meant for use
in the construction of PERF_REG_PMU_MASK_300, and is no longer needed
for that.
- it's only been in the header since v5.12, so I think the chance of
anything using it is essentially zero.
cheers
^ permalink raw reply
* Re: [PATCH v5 6/6] sched/fair: Consider SMT in ASYM_PACKING load balance
From: Vincent Guittot @ 2021-10-01 10:25 UTC (permalink / raw)
To: Guillaume Tucker
Cc: Juri Lelli, Rafael J . Wysocki, Srikar Dronamraju,
Ravi V. Shankar, Peter Zijlstra (Intel), Ricardo Neri, Ben Segall,
Srinivas Pandruvada, Joel Fernandes (Google), Ingo Molnar,
Aubrey Li, Steven Rostedt, Mel Gorman, Len Brown, Ricardo Neri,
Nicholas Piggin, Aubrey Li, Dietmar Eggemann, Tim Chen,
kernelci-results@groups.io, Quentin Perret, linuxppc-dev,
linux-kernel, Daniel Bristot de Oliveira
In-Reply-To: <78608a82-93b8-8036-2bf0-65f53f2f5120@collabora.com>
Hi Guillaume,
This patch and the patchset which includes this patch only impacts
systems with hyperthreading which is not the case of rk3328-rock64
AFAICT. So there is no reason that this code is used by the board. The
only impact should be an increase of the binary for this platform.
Could it be that it reached a limit ?
Regards,
Vincent
On Fri, 1 Oct 2021 at 11:33, Guillaume Tucker
<guillaume.tucker@collabora.com> wrote:
>
> Hi Ricardo,
>
> Please see the bisection report below about a boot failure on
> rk3288-rock64.
>
> Reports aren't automatically sent to the public while we're
> trialing new bisection features on kernelci.org but this one
> looks valid.
>
> Some more details can be found here:
>
> https://linux.kernelci.org/test/case/id/6155a4b0836c79a98f99a31d/
>
> It looks like some i.MX arm64 platforms also regressed with
> next-20210920 although it hasn't been verified yet whether that's
> due to the same commit:
>
> https://linux.kernelci.org/test/job/next/branch/master/kernel/next-20210930/plan/baseline/
>
> The x86 platforms don't seem to be impacted though.
>
> Please let us know if you need help debugging the issue or if you
> have a fix to try.
>
> Best wishes,
> Guillaume
>
>
> GitHub: https://github.com/kernelci/kernelci-project/issues/65
>
> -------------------------------------------------------------------------------
>
>
> * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
> * This automated bisection report was sent to you on the basis *
> * that you may be involved with the breaking commit it has *
> * found. No manual investigation has been done to verify it, *
> * and the root cause of the problem may be somewhere else. *
> * *
> * If you do send a fix, please include this trailer: *
> * Reported-by: "kernelci.org bot" <bot@kernelci.org> *
> * *
> * Hope this helps! *
> * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * * *
>
> next/master bisection: baseline.login on rk3328-rock64
>
> Summary:
> Start: 2d02a18f75fc Add linux-next specific files for 20210929
> Plain log: https://storage.kernelci.org/next/master/next-20210929/arm64/defconfig+CONFIG_RANDOMIZE_BASE=y/gcc-8/lab-baylibre/baseline-rk3328-rock64.txt
> HTML log: https://storage.kernelci.org/next/master/next-20210929/arm64/defconfig+CONFIG_RANDOMIZE_BASE=y/gcc-8/lab-baylibre/baseline-rk3328-rock64.html
> Result: eac6f3841f1d sched/fair: Consider SMT in ASYM_PACKING load balance
>
> Checks:
> revert: PASS
> verify: PASS
>
> Parameters:
> Tree: next
> URL: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
> Branch: master
> Target: rk3328-rock64
> CPU arch: arm64
> Lab: lab-baylibre
> Compiler: gcc-8
> Config: defconfig+CONFIG_RANDOMIZE_BASE=y
> Test case: baseline.login
>
> Breaking commit found:
>
> -------------------------------------------------------------------------------
> commit eac6f3841f1dac7b6f43002056b63f44cc1f1543
> Author: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> Date: Fri Sep 10 18:18:19 2021 -0700
>
> sched/fair: Consider SMT in ASYM_PACKING load balance
>
>
> On 11/09/2021 03:18, Ricardo Neri wrote:
> > When deciding to pull tasks in ASYM_PACKING, it is necessary not only to
> > check for the idle state of the destination CPU, dst_cpu, but also of
> > its SMT siblings.
> >
> > If dst_cpu is idle but its SMT siblings are busy, performance suffers
> > if it pulls tasks from a medium priority CPU that does not have SMT
> > siblings.
> >
> > Implement asym_smt_can_pull_tasks() to inspect the state of the SMT
> > siblings of both dst_cpu and the CPUs in the candidate busiest group.
> >
> > Cc: Aubrey Li <aubrey.li@intel.com>
> > Cc: Ben Segall <bsegall@google.com>
> > Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
> > Cc: Dietmar Eggemann <dietmar.eggemann@arm.com>
> > Cc: Mel Gorman <mgorman@suse.de>
> > Cc: Quentin Perret <qperret@google.com>
> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Tim Chen <tim.c.chen@linux.intel.com>
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > Reviewed-by: Len Brown <len.brown@intel.com>
> > Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com>
> > ---
> > Changes since v4:
> > * Use sg_lb_stats::sum_nr_running the idle state of a scheduling group.
> > (Vincent, Peter)
> > * Do not even idle CPUs in asym_smt_can_pull_tasks(). (Vincent)
> > * Updated function documentation and corrected a typo.
> >
> > Changes since v3:
> > * Removed the arch_asym_check_smt_siblings() hook. Discussions with the
> > powerpc folks showed that this patch should not impact them. Also, more
> > recent powerpc processor no longer use asym_packing. (PeterZ)
> > * Removed unnecessary local variable in asym_can_pull_tasks(). (Dietmar)
> > * Removed unnecessary check for local CPUs when the local group has zero
> > utilization. (Joel)
> > * Renamed asym_can_pull_tasks() as asym_smt_can_pull_tasks() to reflect
> > the fact that it deals with SMT cases.
> > * Made asym_smt_can_pull_tasks() return false for !CONFIG_SCHED_SMT so
> > that callers can deal with non-SMT cases.
> >
> > Changes since v2:
> > * Reworded the commit message to reflect updates in code.
> > * Corrected misrepresentation of dst_cpu as the CPU doing the load
> > balancing. (PeterZ)
> > * Removed call to arch_asym_check_smt_siblings() as it is now called in
> > sched_asym().
> >
> > Changes since v1:
> > * Don't bailout in update_sd_pick_busiest() if dst_cpu cannot pull
> > tasks. Instead, reclassify the candidate busiest group, as it
> > may still be selected. (PeterZ)
> > * Avoid an expensive and unnecessary call to cpumask_weight() when
> > determining if a sched_group is comprised of SMT siblings.
> > (PeterZ).
> > ---
> > kernel/sched/fair.c | 94 +++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 94 insertions(+)
> >
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index 26db017c14a3..8d763dd0174b 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -8597,10 +8597,98 @@ group_type group_classify(unsigned int imbalance_pct,
> > return group_has_spare;
> > }
> >
> > +/**
> > + * asym_smt_can_pull_tasks - Check whether the load balancing CPU can pull tasks
> > + * @dst_cpu: Destination CPU of the load balancing
> > + * @sds: Load-balancing data with statistics of the local group
> > + * @sgs: Load-balancing statistics of the candidate busiest group
> > + * @sg: The candidate busiest group
> > + *
> > + * Check the state of the SMT siblings of both @sds::local and @sg and decide
> > + * if @dst_cpu can pull tasks.
> > + *
> > + * If @dst_cpu does not have SMT siblings, it can pull tasks if two or more of
> > + * the SMT siblings of @sg are busy. If only one CPU in @sg is busy, pull tasks
> > + * only if @dst_cpu has higher priority.
> > + *
> > + * If both @dst_cpu and @sg have SMT siblings, and @sg has exactly one more
> > + * busy CPU than @sds::local, let @dst_cpu pull tasks if it has higher priority.
> > + * Bigger imbalances in the number of busy CPUs will be dealt with in
> > + * update_sd_pick_busiest().
> > + *
> > + * If @sg does not have SMT siblings, only pull tasks if all of the SMT siblings
> > + * of @dst_cpu are idle and @sg has lower priority.
> > + */
> > +static bool asym_smt_can_pull_tasks(int dst_cpu, struct sd_lb_stats *sds,
> > + struct sg_lb_stats *sgs,
> > + struct sched_group *sg)
> > +{
> > +#ifdef CONFIG_SCHED_SMT
> > + bool local_is_smt, sg_is_smt;
> > + int sg_busy_cpus;
> > +
> > + local_is_smt = sds->local->flags & SD_SHARE_CPUCAPACITY;
> > + sg_is_smt = sg->flags & SD_SHARE_CPUCAPACITY;
> > +
> > + sg_busy_cpus = sgs->group_weight - sgs->idle_cpus;
> > +
> > + if (!local_is_smt) {
> > + /*
> > + * If we are here, @dst_cpu is idle and does not have SMT
> > + * siblings. Pull tasks if candidate group has two or more
> > + * busy CPUs.
> > + */
> > + if (sg_is_smt && sg_busy_cpus >= 2)
> > + return true;
> > +
> > + /*
> > + * @dst_cpu does not have SMT siblings. @sg may have SMT
> > + * siblings and only one is busy. In such case, @dst_cpu
> > + * can help if it has higher priority and is idle (i.e.,
> > + * it has no running tasks).
> > + */
> > + return !sds->local_stat.sum_nr_running &&
> > + sched_asym_prefer(dst_cpu, sg->asym_prefer_cpu);
> > + }
> > +
> > + /* @dst_cpu has SMT siblings. */
> > +
> > + if (sg_is_smt) {
> > + int local_busy_cpus = sds->local->group_weight -
> > + sds->local_stat.idle_cpus;
> > + int busy_cpus_delta = sg_busy_cpus - local_busy_cpus;
> > +
> > + if (busy_cpus_delta == 1)
> > + return sched_asym_prefer(dst_cpu,
> > + sg->asym_prefer_cpu);
> > +
> > + return false;
> > + }
> > +
> > + /*
> > + * @sg does not have SMT siblings. Ensure that @sds::local does not end
> > + * up with more than one busy SMT sibling and only pull tasks if there
> > + * are not busy CPUs (i.e., no CPU has running tasks).
> > + */
> > + if (!sds->local_stat.sum_nr_running)
> > + return sched_asym_prefer(dst_cpu, sg->asym_prefer_cpu);
> > +
> > + return false;
> > +#else
> > + /* Always return false so that callers deal with non-SMT cases. */
> > + return false;
> > +#endif
> > +}
> > +
> > static inline bool
> > sched_asym(struct lb_env *env, struct sd_lb_stats *sds, struct sg_lb_stats *sgs,
> > struct sched_group *group)
> > {
> > + /* Only do SMT checks if either local or candidate have SMT siblings */
> > + if ((sds->local->flags & SD_SHARE_CPUCAPACITY) ||
> > + (group->flags & SD_SHARE_CPUCAPACITY))
> > + return asym_smt_can_pull_tasks(env->dst_cpu, sds, sgs, group);
> > +
> > return sched_asym_prefer(env->dst_cpu, group->asym_prefer_cpu);
> > }
> >
> > @@ -9606,6 +9694,12 @@ static struct rq *find_busiest_queue(struct lb_env *env,
> > nr_running == 1)
> > continue;
> >
> > + /* Make sure we only pull tasks from a CPU of lower priority */
> > + if ((env->sd->flags & SD_ASYM_PACKING) &&
> > + sched_asym_prefer(i, env->dst_cpu) &&
> > + nr_running == 1)
> > + continue;
> > +
> > switch (env->migration_type) {
> > case migrate_load:
> > /*
> >
>
^ permalink raw reply
* Re: [V2 2/4] tools/perf: Refactor the code definition of perf reg extended mask in tools side header file
From: Athira Rajeev @ 2021-10-01 10:18 UTC (permalink / raw)
To: Daniel Axtens
Cc: maddy, rnsastry, Arnaldo Carvalho de Melo, jolsa, kjain,
linuxppc-dev
In-Reply-To: <87o889mfl3.fsf@linkitivity.dja.id.au>
> On 01-Oct-2021, at 11:50 AM, Daniel Axtens <dja@axtens.net> wrote:
>
> Hi Athira,
>
>> PERF_REG_PMU_MASK_300 and PERF_REG_PMU_MASK_31 defines the mask
>> value for extended registers. Current definition of these mask values
>> uses hex constant and does not use registers by name, making it less
>> readable. Patch refactor the macro values in perf tools side header file
>> by or'ing together the actual register value constants.
>>
> This looks like a good simplification.
>
>> -/* Exclude MMCR3, SIER2, SIER3 for CPU_FTR_ARCH_300 */
>> -#define PERF_EXCLUDE_REG_EXT_300 (7ULL << PERF_REG_POWERPC_MMCR3)
>
> This file is uAPI - are we allowed to remove a define? Could a program
> built against these headers now fail to compile because we've removed it?
Hi Daniel,
Thanks for the review.
My bad, you are right. I will correct this in version3 by retaining this define and refactoring the macro.
>
>> -
>> /*
>> * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300
>> * includes 9 SPRS from MMCR0 to PMC6 excluding the
>> - * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300.
>> + * unsupported SPRS MMCR3, SIER2 and SIER3.
>> */
>> -#define PERF_REG_PMU_MASK_300 ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300)
>> +#define PERF_REG_PMU_MASK_300 \
>> + ((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \
>> + (1ul << PERF_REG_POWERPC_MMCR2) | (1ul << PERF_REG_POWERPC_PMC1) | \
>> + (1ul << PERF_REG_POWERPC_PMC2) | (1ul << PERF_REG_POWERPC_PMC3) | \
>> + (1ul << PERF_REG_POWERPC_PMC4) | (1ul << PERF_REG_POWERPC_PMC5) | \
>> + (1ul << PERF_REG_POWERPC_PMC6))
>>
>> /*
>> * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31
>> * includes 12 SPRs from MMCR0 to PMC6.
>> */
>> -#define PERF_REG_PMU_MASK_31 (0xfffULL << PERF_REG_POWERPC_MMCR0)
>> +#define PERF_REG_PMU_MASK_31 \
>> + (PERF_REG_PMU_MASK_300 | (1ul << PERF_REG_POWERPC_MMCR3) | \
>> + (1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3))
>>
>> -#define PERF_REG_EXTENDED_MAX (PERF_REG_POWERPC_PMC6 + 1)
>
> Likewise for this define?
>
> I think this might also be an issue for some of your other patches which
> change an include/uapi/ file.
Though I am removing PERF_REG_EXTENDED_MAX define from end of this uapi file, it is moved along with enum definition of perf_event_powerpc_regs.
So we should be good with moving this define from this place.
Thanks
Athira
>
> Kind regards,
> Daniel
>
>> --
>> 2.30.1 (Apple Git-130)
^ permalink raw reply
* Re: [PATCH v2 6/9] PCI: Add pci_find_dvsec_capability to find designated VSEC
From: Jonathan Cameron @ 2021-10-01 9:42 UTC (permalink / raw)
To: Ben Widawsky
Cc: Andrew Donnellan, linux-pci, linux-cxl, Frederic Barrat, iommu,
Bjorn Helgaas, David E . Box, Bjorn Helgaas, Dan Williams,
Kan Liang, linuxppc-dev, David Woodhouse, Lu Baolu
In-Reply-To: <20210923172647.72738-7-ben.widawsky@intel.com>
On Thu, 23 Sep 2021 10:26:44 -0700
Ben Widawsky <ben.widawsky@intel.com> wrote:
> Add pci_find_dvsec_capability to locate a Designated Vendor-Specific
> Extended Capability with the specified DVSEC ID.
>
> The Designated Vendor-Specific Extended Capability (DVSEC) allows one or
> more vendor specific capabilities that aren't tied to the vendor ID of
> the PCI component.
>
> DVSEC is critical for both the Compute Express Link (CXL) driver as well
> as the driver for OpenCAPI coherent accelerator (OCXL).
>
> Cc: David E. Box <david.e.box@linux.intel.com>
> Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Dan Williams <dan.j.williams@intel.com>
> Cc: linux-pci@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: Andrew Donnellan <ajd@linux.ibm.com>
> Cc: Lu Baolu <baolu.lu@linux.intel.com>
> Reviewed-by: Frederic Barrat <fbarrat@linux.ibm.com>
> Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
Great to see this cleaned up.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> ---
> drivers/pci/pci.c | 32 ++++++++++++++++++++++++++++++++
> include/linux/pci.h | 1 +
> 2 files changed, 33 insertions(+)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ce2ab62b64cf..94ac86ff28b0 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -732,6 +732,38 @@ u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap)
> }
> EXPORT_SYMBOL_GPL(pci_find_vsec_capability);
>
> +/**
> + * pci_find_dvsec_capability - Find DVSEC for vendor
> + * @dev: PCI device to query
> + * @vendor: Vendor ID to match for the DVSEC
> + * @dvsec: Designated Vendor-specific capability ID
> + *
> + * If DVSEC has Vendor ID @vendor and DVSEC ID @dvsec return the capability
> + * offset in config space; otherwise return 0.
> + */
> +u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec)
> +{
> + int pos;
> +
> + pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DVSEC);
> + if (!pos)
> + return 0;
> +
> + while (pos) {
> + u16 v, id;
> +
> + pci_read_config_word(dev, pos + PCI_DVSEC_HEADER1, &v);
> + pci_read_config_word(dev, pos + PCI_DVSEC_HEADER2, &id);
> + if (vendor == v && dvsec == id)
> + return pos;
> +
> + pos = pci_find_next_ext_capability(dev, pos, PCI_EXT_CAP_ID_DVSEC);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_find_dvsec_capability);
> +
> /**
> * pci_find_parent_resource - return resource region of parent bus of given
> * region
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cd8aa6fce204..c93ccfa4571b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1130,6 +1130,7 @@ u16 pci_find_ext_capability(struct pci_dev *dev, int cap);
> u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 pos, int cap);
> struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
> u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap);
> +u16 pci_find_dvsec_capability(struct pci_dev *dev, u16 vendor, u16 dvsec);
>
> u64 pci_get_dsn(struct pci_dev *dev);
>
^ permalink raw reply
* Re: [PATCH 1/5] dt-bindings: memory: fsl: convert ifc binding to yaml schema
From: Krzysztof Kozlowski @ 2021-10-01 9:59 UTC (permalink / raw)
To: Li Yang, Shawn Guo, Rob Herring, devicetree, linux-arm-kernel,
Michael Ellerman, linuxppc-dev, linux-kernel
In-Reply-To: <20211001000924.15421-2-leoyang.li@nxp.com>
On 01/10/2021 02:09, Li Yang wrote:
> Convert the txt binding to yaml format and add description. Drop the
> "simple-bus" compatible string from the example and not allowed by the
> binding any more. This will help to enforce the correct probe order
> between parent device and child devices, but will require the ifc driver
> to probe the child devices to work properly.
>
> Signed-off-by: Li Yang <leoyang.li@nxp.com>
> ---
> updates from previous submission:
> - Drop "simple-bus" from binding and only "fsl,ifc" as compatible
> - Fix one identiation problem of "reg"
> - Add type restriction to "little-endian" property
>
> .../bindings/memory-controllers/fsl/ifc.txt | 82 -----------
> .../bindings/memory-controllers/fsl/ifc.yaml | 137 ++++++++++++++++++
> 2 files changed, 137 insertions(+), 82 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> create mode 100644 Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
>
> diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> deleted file mode 100644
> index 89427b018ba7..000000000000
> --- a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.txt
> +++ /dev/null
> @@ -1,82 +0,0 @@
> -Integrated Flash Controller
> -
> -Properties:
> -- name : Should be ifc
> -- compatible : should contain "fsl,ifc". The version of the integrated
> - flash controller can be found in the IFC_REV register at
> - offset zero.
> -
> -- #address-cells : Should be either two or three. The first cell is the
> - chipselect number, and the remaining cells are the
> - offset into the chipselect.
> -- #size-cells : Either one or two, depending on how large each chipselect
> - can be.
> -- reg : Offset and length of the register set for the device
> -- interrupts: IFC may have one or two interrupts. If two interrupt
> - specifiers are present, the first is the "common"
> - interrupt (CM_EVTER_STAT), and the second is the NAND
> - interrupt (NAND_EVTER_STAT). If there is only one,
> - that interrupt reports both types of event.
> -
> -- little-endian : If this property is absent, the big-endian mode will
> - be in use as default for registers.
> -
> -- ranges : Each range corresponds to a single chipselect, and covers
> - the entire access window as configured.
> -
> -Child device nodes describe the devices connected to IFC such as NOR (e.g.
> -cfi-flash) and NAND (fsl,ifc-nand). There might be board specific devices
> -like FPGAs, CPLDs, etc.
> -
> -Example:
> -
> - ifc@ffe1e000 {
> - compatible = "fsl,ifc", "simple-bus";
> - #address-cells = <2>;
> - #size-cells = <1>;
> - reg = <0x0 0xffe1e000 0 0x2000>;
> - interrupts = <16 2 19 2>;
> - little-endian;
> -
> - /* NOR, NAND Flashes and CPLD on board */
> - ranges = <0x0 0x0 0x0 0xee000000 0x02000000
> - 0x1 0x0 0x0 0xffa00000 0x00010000
> - 0x3 0x0 0x0 0xffb00000 0x00020000>;
> -
> - flash@0,0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "cfi-flash";
> - reg = <0x0 0x0 0x2000000>;
> - bank-width = <2>;
> - device-width = <1>;
> -
> - partition@0 {
> - /* 32MB for user data */
> - reg = <0x0 0x02000000>;
> - label = "NOR Data";
> - };
> - };
> -
> - flash@1,0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "fsl,ifc-nand";
> - reg = <0x1 0x0 0x10000>;
> -
> - partition@0 {
> - /* This location must not be altered */
> - /* 1MB for u-boot Bootloader Image */
> - reg = <0x0 0x00100000>;
> - label = "NAND U-Boot Image";
> - read-only;
> - };
> - };
> -
> - cpld@3,0 {
> - #address-cells = <1>;
> - #size-cells = <1>;
> - compatible = "fsl,p1010rdb-cpld";
> - reg = <0x3 0x0 0x000001f>;
> - };
> - };
> diff --git a/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
> new file mode 100644
> index 000000000000..19871ce39fe3
Thanks for the patch.
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/memory-controllers/fsl/ifc.yaml
> @@ -0,0 +1,137 @@
> +# SPDX-License-Identifier: (GPL-2.0+ OR BSD-3-Clause)
Checkpatch should scream here. If it doesn't, maybe you work on some old
tree, which would also explain why you send it to my old address (not
the one from get_maintainers). Please use both checkpatch and
get_maintainers.
You basically relicense bindings from GPL-2.0 only to new license,
including GPL-3.0.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/memory-controllers/fsl/ifc.yaml#
File name should be "fsl,ifc.yaml"
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: FSL/NXP Integrated Flash Controller
> +
> +maintainers:
> + - Li Yang <leoyang.li@nxp.com>
> +
> +description: |
> + NXP's integrated flash controller (IFC) is an advanced version of the
> + enhanced local bus controller which includes similar programming and signal
> + interfaces with an extended feature set. The IFC provides access to multiple
> + external memory types, such as NAND flash (SLC and MLC), NOR flash, EPROM,
> + SRAM and other memories where address and data are shared on a bus.
> +
> +properties:
> + $nodename:
> + pattern: "^ifc@[0-9a-f]+$"
Nodes should be generic, so this looks like "memory-controller".
> +
> + compatible:
> + const: fsl,ifc
> +
> + "#address-cells":
> + enum: [2, 3]
> + description: |
> + Should be either two or three. The first cell is the chipselect
> + number, and the remaining cells are the offset into the chipselect.
> +
> + "#size-cells":
> + enum: [1, 2]
> + description: |
> + Either one or two, depending on how large each chipselect can be.
> +
> + reg:
> + maxItems: 1
> + description: |
> + Offset and length of the register set for the device.
Skip the description, it's obvious.
> +
> + interrupts:
> + minItems: 1
> + maxItems: 2
> + description: |
> + IFC may have one or two interrupts. If two interrupt specifiers are
> + present, the first is the "common" interrupt (CM_EVTER_STAT), and the
> + second is the NAND interrupt (NAND_EVTER_STAT). If there is only one,
> + that interrupt reports both types of event.
> +
> + little-endian:
> + $ref: '/schemas/types.yaml#/definitions/flag'
type: boolean
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH 0/5] convert ifc binding to yaml and drop "simple-bus"
From: Krzysztof Kozlowski @ 2021-10-01 9:45 UTC (permalink / raw)
To: Li Yang, Shawn Guo, Rob Herring, devicetree, linux-arm-kernel,
Michael Ellerman, linuxppc-dev, linux-kernel
In-Reply-To: <20211001000924.15421-1-leoyang.li@nxp.com>
On 01/10/2021 02:09, Li Yang wrote:
> Convert the ifc binding to yaml schema, in the mean while remove the
> "simple-bus" compatible from the binding to make sure ifc device probes
> before any of the child devices. Update the driver and existing DTSes
> accordingly.
>
> DTS changes should be merged together with the driver/binding changes
> if DTS maintainer is ok with it or after the driver changes are applied.
>
It's discouraged to merge DTS along with drivers (e.g. soc folks don't
accept such pull requests), so I propose to apply it in the next cycle.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 2/4] mm: Make generic arch_is_kernel_initmem_freed() do what it says
From: Daniel Axtens @ 2021-10-01 7:14 UTC (permalink / raw)
To: Christophe Leroy, Andrew Morton, arnd
Cc: linux-arch, linux-s390, Kefeng Wang, linux-kernel, linux-mm,
Gerald Schaefer, linuxppc-dev
In-Reply-To: <1d40783e676e07858be97d881f449ee7ea8adfb1.1633001016.git.christophe.leroy@csgroup.eu>
> #ifdef __KERNEL__
> +/*
> + * Check if an address is part of freed initmem. After initmem is freed,
> + * memory can be allocated from it, and such allocations would then have
> + * addresses within the range [_stext, _end].
> + */
> +#ifndef arch_is_kernel_initmem_freed
> +static int arch_is_kernel_initmem_freed(unsigned long addr)
> +{
> + if (system_state < SYSTEM_FREEING_INITMEM)
> + return 0;
> +
> + return init_section_contains((void *)addr, 1);
Is init_section_contains sufficient here?
include/asm-generic/sections.h says:
* [__init_begin, __init_end]: contains .init.* sections, but .init.text.*
* may be out of this range on some architectures.
* [_sinittext, _einittext]: contains .init.text.* sections
init_section_contains only checks __init_*:
static inline bool init_section_contains(void *virt, size_t size)
{
return memory_contains(__init_begin, __init_end, virt, size);
}
Do we need to check against _sinittext and _einittext?
Your proposed generic code will work for powerpc and s390 because those
archs only test against __init_* anyway. I don't know if any platform
actually does place .init.text outside of __init_begin=>__init_end, but
the comment seems to suggest that they could.
Kind regards,
Daniel
^ permalink raw reply
* Re: [V2 3/4] powerpc/perf: Expose instruction and data address registers as part of extended regs
From: Daniel Axtens @ 2021-10-01 6:40 UTC (permalink / raw)
To: Athira Rajeev, mpe, acme, jolsa; +Cc: kjain, maddy, linuxppc-dev, rnsastry
In-Reply-To: <20210930122055.1390-4-atrajeev@linux.vnet.ibm.com>
Athira Rajeev <atrajeev@linux.vnet.ibm.com> writes:
> Patch adds support to include Sampled Instruction Address Register
This is a nit and doesn't require a new revision, but I think this
should read "Include Sampled Instruction Address ...", not "Patch adds
support to include Sampled Instruction ..." - see
https://www.kernel.org/doc/html/v5.11/process/submitting-patches.html#describe-your-changes
> (SIAR) and Sampled Data Address Register (SDAR) SPRs as part of extended
> registers. Update the definition of PERF_REG_PMU_MASK_300/31 and
> PERF_REG_EXTENDED_MAX to include these SPR's.
> diff --git a/arch/powerpc/perf/perf_regs.c b/arch/powerpc/perf/perf_regs.c
> index b931eed482c9..51d31b65e423 100644
> --- a/arch/powerpc/perf/perf_regs.c
> +++ b/arch/powerpc/perf/perf_regs.c
> @@ -90,7 +90,11 @@ static u64 get_ext_regs_value(int idx)
> return mfspr(SPRN_SIER2);
> case PERF_REG_POWERPC_SIER3:
> return mfspr(SPRN_SIER3);
> + case PERF_REG_POWERPC_SDAR:
> + return mfspr(SPRN_SDAR);
> #endif
> + case PERF_REG_POWERPC_SIAR:
> + return mfspr(SPRN_SIAR);
I was initially confused about why SIAR was outside the CONFIG_PPC64
block and SDAR was inside. But it turns out that SIAR is also defined
for a 32 bit platform, so that makes sense.
I'm not an expert on how the perf subsystem works, but this all seems
consistent with the surrounding code and it seems to do what the commit
message says, so on that limited basis:
Reviewed-by: Daniel Axtens <dja@axtens.net>
Kind regards,
Daniel
^ permalink raw reply
* Re: [V2 2/4] tools/perf: Refactor the code definition of perf reg extended mask in tools side header file
From: Daniel Axtens @ 2021-10-01 6:20 UTC (permalink / raw)
To: Athira Rajeev, mpe, acme, jolsa; +Cc: kjain, maddy, linuxppc-dev, rnsastry
In-Reply-To: <20210930122055.1390-3-atrajeev@linux.vnet.ibm.com>
Hi Athira,
> PERF_REG_PMU_MASK_300 and PERF_REG_PMU_MASK_31 defines the mask
> value for extended registers. Current definition of these mask values
> uses hex constant and does not use registers by name, making it less
> readable. Patch refactor the macro values in perf tools side header file
> by or'ing together the actual register value constants.
>
This looks like a good simplification.
> -/* Exclude MMCR3, SIER2, SIER3 for CPU_FTR_ARCH_300 */
> -#define PERF_EXCLUDE_REG_EXT_300 (7ULL << PERF_REG_POWERPC_MMCR3)
This file is uAPI - are we allowed to remove a define? Could a program
built against these headers now fail to compile because we've removed it?
> -
> /*
> * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_300
> * includes 9 SPRS from MMCR0 to PMC6 excluding the
> - * unsupported SPRS in PERF_EXCLUDE_REG_EXT_300.
> + * unsupported SPRS MMCR3, SIER2 and SIER3.
> */
> -#define PERF_REG_PMU_MASK_300 ((0xfffULL << PERF_REG_POWERPC_MMCR0) - PERF_EXCLUDE_REG_EXT_300)
> +#define PERF_REG_PMU_MASK_300 \
> + ((1ul << PERF_REG_POWERPC_MMCR0) | (1ul << PERF_REG_POWERPC_MMCR1) | \
> + (1ul << PERF_REG_POWERPC_MMCR2) | (1ul << PERF_REG_POWERPC_PMC1) | \
> + (1ul << PERF_REG_POWERPC_PMC2) | (1ul << PERF_REG_POWERPC_PMC3) | \
> + (1ul << PERF_REG_POWERPC_PMC4) | (1ul << PERF_REG_POWERPC_PMC5) | \
> + (1ul << PERF_REG_POWERPC_PMC6))
>
> /*
> * PERF_REG_EXTENDED_MASK value for CPU_FTR_ARCH_31
> * includes 12 SPRs from MMCR0 to PMC6.
> */
> -#define PERF_REG_PMU_MASK_31 (0xfffULL << PERF_REG_POWERPC_MMCR0)
> +#define PERF_REG_PMU_MASK_31 \
> + (PERF_REG_PMU_MASK_300 | (1ul << PERF_REG_POWERPC_MMCR3) | \
> + (1ul << PERF_REG_POWERPC_SIER2) | (1ul << PERF_REG_POWERPC_SIER3))
>
> -#define PERF_REG_EXTENDED_MAX (PERF_REG_POWERPC_PMC6 + 1)
Likewise for this define?
I think this might also be an issue for some of your other patches which
change an include/uapi/ file.
Kind regards,
Daniel
> --
> 2.30.1 (Apple Git-130)
^ permalink raw reply
* Re: [PATCH] powerpc/powernv/prd: Unregister OPAL_MSG_PRD2 notifier during module unload
From: Daniel Axtens @ 2021-10-01 6:10 UTC (permalink / raw)
To: Vasant Hegde, linuxppc-dev; +Cc: Vasant Hegde
In-Reply-To: <20210928120933.196571-1-hegdevasant@linux.vnet.ibm.com>
Hi Vasant,
> Commit 587164cd, introduced new opal message type (OPAL_MSG_PRD2) and added
> opal notifier. But I missed to unregister the notifier during module unload
> path. This results in below call trace if you try to unload and load
> opal_prd module.
>
> Fixes: 587164cd ("powerpc/powernv: Add new opal message type")
In reviewing this patch, I've become a bit worried the underlying patch
has another issue that we should fix.
Consider opal_prd_probe and the opal_prd_event_nb:
static struct notifier_block opal_prd_event_nb = {
.notifier_call = opal_prd_msg_notifier,
.next = NULL,
.priority = 0,
};
static int opal_prd_probe(struct platform_device *pdev)
{
...
rc = opal_message_notifier_register(OPAL_MSG_PRD, &opal_prd_event_nb);
if (rc) { ... }
rc = opal_message_notifier_register(OPAL_MSG_PRD2, &opal_prd_event_nb);
if (rc) { ... }
I don't think you can reuse a single notifier block for multiple
notifier_register calls. opal_message_notify_register calls
atomic_notifier_chain_register which takes a spinlock and calls
notifer_chain_register which reads:
static int notifier_chain_register(struct notifier_block **nl,
struct notifier_block *n)
{
while ((*nl) != NULL) {
// ... skip some error detection
// ... find the right spot in the list to add this
if (n->priority > (*nl)->priority)
break;
nl = &((*nl)->next);
}
n->next = *nl; // <-- mutate the notifier block!!
rcu_assign_pointer(*nl, n);
return 0;
}
So we have the same notifier block getting inserted into two different
linked lists. This is unlikely to break at the moment because nothing
else registers an MSG_PRD/PRD2 notifier, but nonetheless I think you
need to use a different notifer_block struct for each list you insert
your notifier into.
Likewise this could cause other problems during removal, if there were
other entries in either notifier list.
Kind regards,
Daniel
^ permalink raw reply
* Re: Add Apple M1 support to PASemi i2c driver
From: Christian Zigotzky @ 2021-10-01 5:49 UTC (permalink / raw)
To: Sven Peter
Cc: Darren Stevens, Arnd Bergmann, Hector Martin,
Linux Kernel Mailing List, linux-i2c, Paul Mackerras,
Alyssa Rosenzweig, R.T.Dickinson, Olof Johansson,
mohamed.mediouni, Matthew Leaman, Mark Kettenis, linuxppc-dev,
R.T.Dickinson, linux-arm-kernel, Stan Skowronek
In-Reply-To: <9c1f5c48-bf1a-0ecc-e769-773d2935c66c@xenosoft.de>
Typo: Damian
Correct: Damien
On 01 October 2021 at 06:47 am, Christian Zigotzky wrote:
> On 27 September 2021 at 07:39 am, Sven Peter wrote:
> > Hi Christian,
> >
> > Thanks already for volunteering to test this!
> >
> Hello Sven,
>
> Damian (Hypex) has successfully tested the RC3 of kernel 5.15 with
> your modified i2c driver on his Nemo board yesterday. [1]
>
> @Darren
> Could you also please check Sven's i2c modifications? He has also
> modified your source code a little bit. [2]
>
> @Olof
> Are these i2c modifications OK? Do these work on your P.A. Semi board?
>
> Thanks,
> Christian
>
> [1] https://forum.hyperion-entertainment.com/viewtopic.php?p=54098#p54098
> [2]
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-January/153195.html
> [3] Further information about the Nemo board:
> https://en.wikipedia.org/wiki/AmigaOne_X1000
> [4] Kernel patches for the Nemo board:
> https://lists.ozlabs.org/pipermail/linuxppc-dev/2018-January/167288.html
^ 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