* Re: [PATCH v6 02/11] kselftest: arm64: mangle_pstate_invalid_compat_toggle and common utils
From: Dave Martin @ 2019-09-17 16:05 UTC (permalink / raw)
To: Cristian Marussi
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190910123111.33478-3-cristian.marussi@arm.com>
On Tue, Sep 10, 2019 at 01:31:02pm +0100, Cristian Marussi wrote:
> Add some arm64/signal specific boilerplate and utility code to help
> further testcases' development.
>
> Introduce also one simple testcase mangle_pstate_invalid_compat_toggle
> and some related helpers: it is a simple mangle testcase which messes
> with the ucontext_t from within the signal handler, trying to toggle
> PSTATE state bits to switch the system between 32bit/64bit execution
> state. Expects SIGSEGV on test PASS.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v5 --> v6
> - fix commit msg
> - feat_names is char const *const
> - better supported options check and reporting
> - removed critical asserts to avoid issues with NDEBUG
> - more robust get_header
> - fix validation for ESR_CONTEXT size
> - add more explicit comment in GET_RESV_NEXT_HEAD() macro
> - refactored default_handler()
> - feats_ok() now public
> - call always test_results() no matter the outcome of test_run()
[...]
> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
[...]
> +static int test_init(struct tdescr *td)
> +{
> + td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
> + if (!td->minsigstksz)
> + td->minsigstksz = MINSIGSTKSZ;
> + fprintf(stderr, "Detected MINSTKSIGSZ:%d\n", td->minsigstksz);
> +
> + if (td->feats_required) {
> + td->feats_supported = 0;
> + /*
> + * Checking for CPU required features using both the
> + * auxval and the arm64 MRS Emulation to read sysregs.
> + */
> + if (getauxval(AT_HWCAP) & HWCAP_SSBS)
> + td->feats_supported |= FEAT_SSBS;
> + if (getauxval(AT_HWCAP) & HWCAP_CPUID) {
> + uint64_t val = 0;
> +
> + /* Uses MRS emulation to check capability */
> + get_regval(SYS_ID_AA64MMFR1_EL1, val);
> + if (ID_AA64MMFR1_EL1_PAN_SUPPORTED(val))
> + td->feats_supported |= FEAT_PAN;
> + /* Uses MRS emulation to check capability */
> + get_regval(SYS_ID_AA64MMFR2_EL1, val);
> + if (ID_AA64MMFR2_EL1_UAO_SUPPORTED(val))
> + td->feats_supported |= FEAT_UAO;
> + } else {
> + fprintf(stderr,
> + "HWCAP_CPUID NOT available. Mark ALL feats UNSUPPORTED.\n");
Nit: this message isn't strictly correct now: SSBS may still be detected
even if HWCAP_CPUID isn't present.
For simplicity I suggest to drop this fprintf() (and the containing
else { }, which is otherwise empty).
The following code reports what features are supported in any case, so
the user will be able to see what was detected.
The rest looks reasonable to me now, so with the above nit fixed:
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
[...]
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 05/11] kselftest: arm64: mangle_pstate_ssbs_regs
From: Dave Martin @ 2019-09-17 16:05 UTC (permalink / raw)
To: Cristian Marussi
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190910123111.33478-6-cristian.marussi@arm.com>
On Tue, Sep 10, 2019 at 01:31:05pm +0100, Cristian Marussi wrote:
> Add a simple mangle testcase which messes with the ucontext_t from within
> the signal handler, trying to set the PSTATE SSBS bit and verify that
> SSBS bit set is preserved across sigreturn.
> When available, use MRS SBSS support to set/get SSBS bit, otherwise lookup
> PSTATE.SSBS directly.
>
> Additionally, in order to support this test specific needs:
> - extend signal testing framework to allow the definition of a custom
> per test initialization function to be run at the end of test setup
> and before test run routine.
> - introduce a set_regval() helper to set system register values in a
> toolchain independent way.
> - introduce also a new common utility function: get_current_context()
> which can be used to grab a ucontext without the help of libc, and
> detect if such ucontext has been actively used to jump back.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v5 --> v6
> - using SIGTRAP as sig_copyctx for get_current_context()
> - get_current_context() is now __always_inline
> - last minute check for SSBS cleared
> - restore volatile usage, dropping useless DSB
> - output clobber on *dest_uc
> - no abort() on SSSB not cleared
> - refactored/relocated test_init() call to be after test_setup()
> [to catch early SIGILL while initializing]
> - avoid MRS SSBS when !feats_ok()...use instead PSTATE.SSBS
> - refactored SIG_COPYCTX usage to fit new splitted-by-signal layout
> v3 --> v4
> - fix commit message
> - missing include signal.h
> - added .init per-test init-func
> - added set_regval() helper
> - added SSBS clear to 0 custom .init function
> - removed volatile qualifier associated with sig_atomic_t data
> - added dsb inside handler to ensure the writes related to the
> grabbed ucontext have completed
> - added test description
> ---
> .../selftests/arm64/signal/test_signals.c | 2 +-
> .../selftests/arm64/signal/test_signals.h | 54 ++++++----
> .../arm64/signal/test_signals_utils.c | 41 +++++++-
> .../arm64/signal/test_signals_utils.h | 99 +++++++++++++++++++
> .../testcases/mangle_pstate_ssbs_regs.c | 88 +++++++++++++++++
> 5 files changed, 258 insertions(+), 26 deletions(-)
> create mode 100644 tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
>
> diff --git a/tools/testing/selftests/arm64/signal/test_signals.c b/tools/testing/selftests/arm64/signal/test_signals.c
> index cb970346b280..6e4da9c920d4 100644
> --- a/tools/testing/selftests/arm64/signal/test_signals.c
> +++ b/tools/testing/selftests/arm64/signal/test_signals.c
> @@ -19,7 +19,7 @@ int main(int argc, char *argv[])
> current = &tde;
>
> ksft_print_msg("%s :: %s\n", current->name, current->descr);
> - if (test_setup(current)) {
> + if (test_setup(current) && test_init(current)) {
> test_run(current);
> test_result(current);
> test_cleanup(current);
> diff --git a/tools/testing/selftests/arm64/signal/test_signals.h b/tools/testing/selftests/arm64/signal/test_signals.h
> index 0dd71700ff67..753fe64cbe11 100644
> --- a/tools/testing/selftests/arm64/signal/test_signals.h
> +++ b/tools/testing/selftests/arm64/signal/test_signals.h
> @@ -27,6 +27,14 @@
> : "memory"); \
> }
>
> +#define set_regval(regname, in) \
> +{ \
> + asm volatile("msr " __stringify(regname) ", %0" \
> + : \
> + : "r" (in) \
> + : "memory"); \
> +}
> +
> /* Regs encoding and masks naming copied in from sysreg.h */
> #define SYS_ID_AA64MMFR1_EL1 S3_0_C0_C7_1 /* MRS Emulated */
> #define SYS_ID_AA64MMFR2_EL1 S3_0_C0_C7_2 /* MRS Emulated */
> @@ -62,39 +70,47 @@ enum {
> */
> struct tdescr {
> /* KEEP THIS FIELD FIRST for easier lookup from assembly */
> - void *token;
> + void *token;
All the whitespace changes here make it difficult to review this patch.
Can you please fold the indentation changes into the patches that added
the lines in the first place -- patches 2 and 5, maybe others?
(Assuming you make no other changes, feel free to keep my Reviewed-by on
those patches.)
> /* when disabled token based sanity checking is skipped in handler */
> - bool sanity_disabled;
> + bool sanity_disabled;
> /* just a name for the test-case; manadatory field */
> - char *name;
> - char *descr;
> - unsigned long feats_required;
> + char *name;
> + char *descr;
> + unsigned long feats_required;
> /* bitmask of effectively supported feats: populated at run-time */
> - unsigned long feats_supported;
> - bool initialized;
> - unsigned int minsigstksz;
> + unsigned long feats_supported;
> + bool initialized;
> + unsigned int minsigstksz;
> /* signum used as a test trigger. Zero if no trigger-signal is used */
> - int sig_trig;
> + int sig_trig;
> /*
> * signum considered as a successful test completion.
> * Zero when no signal is expected on success
> */
> - int sig_ok;
> + int sig_ok;
> /* signum expected on unsupported CPU features. */
> - int sig_unsupp;
> + int sig_unsupp;
> /* a timeout in second for test completion */
> - unsigned int timeout;
> - bool triggered;
> - bool pass;
> + unsigned int timeout;
> + bool triggered;
> + bool pass;
> /* optional sa_flags for the installed handler */
> - int sa_flags;
> - ucontext_t saved_uc;
> + int sa_flags;
> + ucontext_t saved_uc;
> + /* used by get_current_ctx() */
> + size_t live_sz;
> + ucontext_t *live_uc;
>
> - /* a custom setup function to be called before test starts */
> + volatile sig_atomic_t live_uc_valid;
> + /* optional test private data */
> + void *priv;
> + /* a custom setup: called alternatively to default_setup */
> int (*setup)(struct tdescr *td);
> + /* a custom init: called by default test init after test_setup */
> + void (*init)(struct tdescr *td);
> /* a custom cleanup function called before test exits */
> void (*cleanup)(struct tdescr *td);
> - /* an optional function to be used as a trigger for test starting */
> + /* an optional function to be used as a trigger for starting test */
> int (*trigger)(struct tdescr *td);
> /*
> * the actual test-core: invoked differently depending on the
> @@ -103,8 +119,6 @@ struct tdescr {
> int (*run)(struct tdescr *td, siginfo_t *si, ucontext_t *uc);
> /* an optional function for custom results' processing */
> void (*check_result)(struct tdescr *td);
> -
> - void *priv;
> };
>
> extern struct tdescr tde;
> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.c b/tools/testing/selftests/arm64/signal/test_signals_utils.c
> index 7324e8a7f47f..133105755683 100644
> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.c
> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.c
> @@ -4,19 +4,22 @@
> #include <stdio.h>
> #include <stdlib.h>
> #include <signal.h>
> -#include <string.h>
Is this intentional? This file still uses memcpy().
> #include <unistd.h>
> #include <assert.h>
> #include <sys/auxv.h>
> #include <linux/auxvec.h>
> #include <ucontext.h>
>
> +#include <asm/unistd.h>
> +
> #include "test_signals.h"
> #include "test_signals_utils.h"
> #include "testcases/testcases.h"
>
> extern struct tdescr *current;
>
> +static int sig_copyctx = SIGTRAP;
> +
> static char const *const feats_names[FMAX_END] = {
> " SSBS ",
> " PAN ",
> @@ -148,6 +151,20 @@ static bool handle_signal_ok(struct tdescr *td,
> return true;
> }
>
> +static bool handle_signal_copyctx(struct tdescr *td,
> + siginfo_t *si, void *uc)
> +{
> + /* Mangling PC to avoid loops on original BRK instr */
> + ((ucontext_t *)uc)->uc_mcontext.pc += 4;
> + memcpy(td->live_uc, uc, td->live_sz);
> + ASSERT_GOOD_CONTEXT(td->live_uc);
> + td->live_uc_valid = 1;
> + fprintf(stderr,
> + "GOOD CONTEXT grabbed from sig_copyctx handler\n");
> +
> + return true;
> +}
> +
> static void default_handler(int signum, siginfo_t *si, void *uc)
> {
> if (current->sig_unsupp && signum == current->sig_unsupp &&
> @@ -159,6 +176,9 @@ static void default_handler(int signum, siginfo_t *si, void *uc)
> } else if (current->sig_ok && signum == current->sig_ok &&
> handle_signal_ok(current, si, uc)) {
> fprintf(stderr, "Handled SIG_OK\n");
> + } else if (signum == sig_copyctx && current->live_uc &&
> + handle_signal_copyctx(current, si, uc)) {
> + fprintf(stderr, "Handled SIG_COPYCTX\n");
> } else {
> if (signum == SIGALRM && current->timeout) {
> fprintf(stderr, "-- Timeout !\n");
> @@ -211,8 +231,17 @@ static inline int default_trigger(struct tdescr *td)
> return !raise(td->sig_trig);
> }
>
> -static int test_init(struct tdescr *td)
> +int test_init(struct tdescr *td)
> {
> + if (td->sig_trig == sig_copyctx) {
> + fprintf(stdout,
> + "Signal %d is RESERVED, cannot be used as a trigger. Aborting\n",
> + sig_copyctx);
> + return 0;
> + }
> + /* just in case */
> + unblock_signal(sig_copyctx);
> +
> td->minsigstksz = getauxval(AT_MINSIGSTKSZ);
> if (!td->minsigstksz)
> td->minsigstksz = MINSIGSTKSZ;
> @@ -253,7 +282,12 @@ static int test_init(struct tdescr *td)
> ~td->feats_supported));
> }
>
> + /* Perform test specific additional initialization */
> + if (td->init)
> + td->init(td);
> td->initialized = 1;
> + fprintf(stderr, "Testcase initialized.\n");
> +
> return 1;
> }
>
> @@ -265,9 +299,6 @@ int test_setup(struct tdescr *td)
> assert(td->name);
> assert(td->run);
>
> - if (!test_init(td))
> - return 0;
> -
> if (td->setup)
> return td->setup(td);
> else
> diff --git a/tools/testing/selftests/arm64/signal/test_signals_utils.h b/tools/testing/selftests/arm64/signal/test_signals_utils.h
> index 47a7592b7c53..3ad062af1fa7 100644
> --- a/tools/testing/selftests/arm64/signal/test_signals_utils.h
> +++ b/tools/testing/selftests/arm64/signal/test_signals_utils.h
> @@ -4,8 +4,13 @@
> #ifndef __TEST_SIGNALS_UTILS_H__
> #define __TEST_SIGNALS_UTILS_H__
>
> +#include <assert.h>
> +#include <stdio.h>
> +#include <string.h>
> +
> #include "test_signals.h"
>
> +int test_init(struct tdescr *td);
> int test_setup(struct tdescr *td);
> void test_cleanup(struct tdescr *td);
> int test_run(struct tdescr *td);
> @@ -16,4 +21,98 @@ static inline bool feats_ok(struct tdescr *td)
> return (td->feats_required & td->feats_supported) == td->feats_required;
> }
>
> +/*
> + * Obtaining a valid and full-blown ucontext_t from userspace is tricky:
> + * libc getcontext does() not save all the regs and messes with some of
> + * them (pstate value in particular is not reliable).
> + *
> + * Here we use a service signal to grab the ucontext_t from inside a
> + * dedicated signal handler, since there, it is populated by Kernel
> + * itself in setup_sigframe(). The grabbed context is then stored and
> + * made available in td->live_uc.
> + *
> + * As service-signal is used a SIGTRAP induced by a 'brk' instruction,
> + * because here we have to avoid syscalls to trigger the signal since
> + * they would cause any SVE sigframe content (if any) to be removed.
> + *
> + * Anyway this function really serves a dual purpose:
> + *
> + * 1. grab a valid sigcontext into td->live_uc for result analysis: in
> + * such case it returns 1.
> + *
> + * 2. detect if, somehow, a previously grabbed live_uc context has been
> + * used actively with a sigreturn: in such a case the execution would have
> + * magically resumed in the middle of this function itself (seen_already==1):
> + * in such a case return 0, since in fact we have not just simply grabbed
> + * the context.
> + *
> + * This latter case is useful to detect when a fake_sigreturn test-case has
> + * unexpectedly survived without hitting a SEGV.
> + *
> + * Note that the case of runtime dynamically sized sigframes (like in SVE
> + * context) is still NOT addressed: sigframe size is supposed to be fixed
> + * at sizeof(ucontext_t).
> + */
> +static __always_inline bool get_current_context(struct tdescr *td,
> + ucontext_t *dest_uc)
> +{
> + static volatile bool seen_already;
> +
> + assert(td && dest_uc);
> + /* it's a genuine invocation..reinit */
> + seen_already = 0;
Nit: can we have "= 0" as an initializer in the declaration above?
> + td->live_uc_valid = 0;
> + td->live_sz = sizeof(*dest_uc);
> + memset(dest_uc, 0x00, td->live_sz);
> + td->live_uc = dest_uc;
> + /*
> + * Grab ucontext_t triggering a SIGTRAP.
> + *
> + * Note that:
> + * - live_uc_valid is declared volatile sig_atomic_t in
> + * struct tdescr since it will be changed inside the
> + * sig_copyctx handler
> + * - the additional 'memory' clobber is there to avoid possible
> + * compiler's assumption on live_uc_valid, seen-already and
I could be wrong about this, but I'm not sure that the "memory" clobber
is sufficient for for seen_already, because of the way that variable is
scoped to this function; however, you declare seen_already volatile
anyway which should be sufficient.
I suggest you just omit seen_already from this comment.
> + * the content pointed by dest_uc, which are all changed inside
> + * the signal handler
> + * - BRK causes a debug exception which is handled by the Kernel
> + * and finally causes the SIGTRAP signal to be delivered to this
> + * test thread. Since such delivery happens on the ret_to_user()
> + * /do_notify_resume() debug exception return-path, we are sure
> + * that the registered SIGTRAP handler has been run to completion
> + * before the execution path is restored here: as a consequence
> + * we can be sure that the volatile sig_atomic_t live_uc_valid
> + * carries a meaningful result. Being in a single thread context
> + * we'll also be sure that any access to memory modified by the
> + * handler (namely ucontext_t) will be visible once returned.
> + * - note that since we are using a breakpoint instruction here
> + * to cause a SIGTRAP, the ucontext_t grabbed from the signal
> + * handler would naturally contain a PC pointing exactly to this
> + * BRK line, which means that, on return from the signal handler,
> + * or if we place the ucontext_t on the stack to fake a sigreturn,
> + * we'll end up in an infinite loop of BRK-SIGTRAP-handler.
> + * For this reason we take care to artificially move forward the
> + * PC to the next instruction while inside the signal handler.
> + */
> + asm volatile ("brk #666"
> + : "=m" (*dest_uc)
Make this an input-output argument ("+m")? The memset() needs to
take effect happen before the asm.
> + :
> + : "memory");
> +
> + /*
> + * If we get here with seen_already==1 it implies the td->live_uc
> + * context has been used to get back here....this probably means
> + * a test has failed to cause a SEGV...anyway live_uc does not
> + * point to a just acquired copy of ucontext_t...so return 0
> + */
> + if (seen_already) {
> + fprintf(stdout,
> + "Unexpected successful sigreturn detected: live_uc is stale !\n");
> + return 0;
> + }
> + seen_already = 1;
> +
> + return td->live_uc_valid;
> +}
> #endif
> diff --git a/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
> new file mode 100644
> index 000000000000..e2b87ea4c11f
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/mangle_pstate_ssbs_regs.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 ARM Limited
> + *
> + * Try to mangle the ucontext from inside a signal handler, setting the
> + * SSBS bit to 1 and veryfing that such modification is preserved.
> + */
> +
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <signal.h>
> +#include <ucontext.h>
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +static void mangle_invalid_pstate_ssbs_init(struct tdescr *td)
> +{
> + if (feats_ok(td)) {
> + fprintf(stderr, "Clearing SSBS to 0\n");
> + set_regval(SSBS_SYSREG, 0);
> + }
> +}
> +
> +static int mangle_invalid_pstate_ssbs_run(struct tdescr *td,
> + siginfo_t *si, ucontext_t *uc)
> +{
> + ASSERT_GOOD_CONTEXT(uc);
> +
> + /*
> + * If HW_SSBS is supported but we weren't able to clear SSBS during
> + * test_init, or if something has reset SSBS in the meantime, abort.
What is "HW_SSBS" ?
> + */
> + if (feats_ok(td) && (uc->uc_mcontext.pstate & PSR_SSBS_BIT)) {
> + fprintf(stderr,
> + "SSBS unexpectedly NOT zeroed ! Something's wrong. Abort\n");
> + abort();
> + }
> +
> + /* set bit value ... should NOT be cleared by Kernel on sigreturn */
> + uc->uc_mcontext.pstate |= PSR_SSBS_BIT;
> + fprintf(stderr, "SSBS set to 1 -- PSTATE: 0x%016llX\n",
> + uc->uc_mcontext.pstate);
> + /* Save after mangling...it should be preserved */
> + td->saved_uc = *uc;
> +
> + return 1;
> +}
> +
> +static void pstate_ssbs_bit_checks(struct tdescr *td)
> +{
> + uint64_t val = 0;
> + ucontext_t uc;
> +
> + /* This check reports some result even if MRS SSBS unsupported */
> + if (get_current_context(td, &uc))
> + fprintf(stderr,
> + "INFO: live_uc - got PSTATE: 0x%016llX -> SSBS %s\n",
> + uc.uc_mcontext.pstate,
> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT) ==
> + (uc.uc_mcontext.pstate & PSR_SSBS_BIT) ?
> + "PRESERVED" : "CLEARED");
Don't we subsequently refer to uc even if get_current_context() failed
here?
> + /* Choose check method depending of supported features */
> + if (feats_ok(td)) {
> + fprintf(stderr, "Checking with MRS SSBS...\n");
> + get_regval(SSBS_SYSREG, val);
> + fprintf(stderr, "INFO: MRS SSBS - got: 0x%016lX\n", val);
> + } else {
> + fprintf(stderr, "Checking with PSTATE.SSBS...\n");
> + val = uc.uc_mcontext.pstate;
> + }
> + /* pass when preserved */
> + td->pass = (val & PSR_SSBS_BIT) ==
> + (td->saved_uc.uc_mcontext.pstate & PSR_SSBS_BIT);
Does this mean the test fails when SSBS isn't supported at all? That
doesn't seem right.
I think trying to handle both levels of SSBS support within the same
test is complicating things here. The purpose of this is to check
that sigreturn doesn't mask out the SSBS bit when it shouldn't: we don't
care whether the SSBS architecture feature actually works.
So, would it be simpler to drop the MSR/MRS direct access to the SSBS
bit?
Instead, we could predicate this test on whether ID_AA64PFR1_EL1.SSBS
>= 1 instead of HWCAP_SSBS, and we could just check that run() can
successfully _toggle_ uc->uc_mcontext.pstate ^= PSR_SSBS_BIT, with the
change checked via a subsequent get_current_context().
In other words, we no longer try to initialise SSBS to a particular
value. Instead, we just check the we can change the bit.
Does that make sense?
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 06/11] kselftest: arm64: fake_sigreturn_bad_magic
From: Dave Martin @ 2019-09-17 16:06 UTC (permalink / raw)
To: Cristian Marussi
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190910123111.33478-7-cristian.marussi@arm.com>
On Tue, Sep 10, 2019 at 01:31:06pm +0100, Cristian Marussi wrote:
> Add a simple fake_sigreturn testcase which builds a ucontext_t with a bad
> magic header and place it onto the stack. Expects a SIGSEGV on test PASS.
>
> Introduce a common utility assembly trampoline function to invoke a
> sigreturn while placing the provided sigframe at wanted alignment and
> also an helper to make space when needed inside the sigframe reserved
> area.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v5 --> v6
> - fake_sigreturn: cleaned up, avoiding excessive SP misalignments
> - fake_sigreturn: better formatting and prologue
> - get_starting_header: cleand up and commented
> - avoid timeout on failure
[...]
> diff --git a/tools/testing/selftests/arm64/signal/signals.S b/tools/testing/selftests/arm64/signal/signals.S
> new file mode 100644
> index 000000000000..e670f8f2c8de
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/signals.S
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Copyright (C) 2019 ARM Limited */
> +
> +#include <asm/unistd.h>
> +
> +.section .rodata, "a"
> +call_fmt:
> + .asciz "Calling sigreturn with fake sigframe sized:%zd at SP @%08lX\n"
> +
> +.text
> +
> +.globl fake_sigreturn
> +
> +/* fake_sigreturn x0:&sigframe, x1:sigframe_size, x2:misalign_bytes */
> +fake_sigreturn:
> + stp x29, x30, [sp, #-16]!
> + mov x29, sp
> +
> + mov x20, x0
> + mov x21, x1
> + mov x22, x2
> +
> + /* create space on the stack for fake sigframe 16 bytes-aligned */
> + add x0, x21, x22
> + add x0, x0, #16
> + bic x0, x0, #15 /* round_up(sigframe_size + misalign_bytes, 16) */
If I've figured this out right, x0 as computed here actually looks
like round_up(sigframe_size + misalign_bytes + 1, 16) - 1.
(n + (m - 1)) & ~(m - 1) is the classic way to round up when m is a
power of 2.
That's why I originally suggested to add 15. Your code works, but I
think it always allocates at least one byte more than needed (?)
This is not a huge deal, and better than allocating one byte to few,
but it would be good to understand whether this behaviour was
intentional or not.
> + sub sp, sp, x0
> + add x23, sp, x22 /* new sigframe base with misaligment if any */
> +
> + ldr x0, =call_fmt
> + mov x1, x21
> + mov x2, x23
> + bl printf
> +
> + /* memcpy the provided content, while still keeping SP aligned */
> + mov x0, x23
> + mov x1, x20
> + mov x2, x21
> + bl memcpy
> +
> + /*
> + * Here saving a last minute SP to current->token acts as a marker:
> + * if we got here, we are successfully faking a sigreturn; in other
> + * words we are sure no bad fatal signal has been raised till now
> + * for unrelated reasons, so we should consider the possibly observed
> + * fatal signal like SEGV coming from Kernel restore_sigframe() and
> + * triggered as expected from our test-case.
> + * For simplicity this assumes that current field 'token' is laid out
> + * as first in struct tdescr
> + */
> + ldr x0, current
> + str x23, [x0]
> + /* finally move SP to misaligned address...if any requested */
> + mov sp, x23
> +
> + mov x8, #__NR_rt_sigreturn
> + svc #0
> +
> + /*
> + * Above sigreturn should not return...looping here leads to a timeout
> + * and ensure proper and clean test failure, instead of jumping around
> + * on a potentially corrupted stack.
> + */
> + b .
[...]
> diff --git a/tools/testing/selftests/arm64/signal/testcases/testcases.c b/tools/testing/selftests/arm64/signal/testcases/testcases.c
> index 1914a01222a1..e3521949b800 100644
> --- a/tools/testing/selftests/arm64/signal/testcases/testcases.c
> +++ b/tools/testing/selftests/arm64/signal/testcases/testcases.c
> @@ -148,3 +148,49 @@ bool validate_reserved(ucontext_t *uc, size_t resv_sz, char **err)
>
> return true;
> }
> +
> +/*
> + * This function walks through the records inside the provided reserved area
> + * trying to find enough space to fit @need_sz bytes: if not enough space is
> + * available and an extra_context record is present, it throws away the
> + * extra_context record.
> + *
> + * It returns a pointer to a new header where it is possible to start storing
> + * our need_sz bytes.
> + *
> + * @shead: points to the start of reserved area
> + * @need_sz: needed bytes
> + * @resv_sz: reserved area size in bytes
> + * @offset: if not null, this will be filled with the offset of the return
> + * head pointer from @shead
> + *
> + * @return: pointer to a new head where to start storing need_sz bytes, or
> + * NULL if space could not be made available.
> + */
That's much clearer now, thanks for that.
> +struct _aarch64_ctx *get_starting_head(struct _aarch64_ctx *shead,
> + size_t need_sz, size_t resv_sz,
> + size_t *offset)
> +{
[...]
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd
From: Dave Martin @ 2019-09-17 16:06 UTC (permalink / raw)
To: Cristian Marussi
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190910123111.33478-10-cristian.marussi@arm.com>
On Tue, Sep 10, 2019 at 01:31:09pm +0100, Cristian Marussi wrote:
> Add a simple fake_sigreturn testcase which builds a ucontext_t with
> an anomalous additional fpsimd_context and place it onto the stack.
> Expects a SIGSEGV on test PASS.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> ---
> v5 --> v6
> - removed unneeded locals
> - avoid timeout on failure
> v3 --> v4
> - fix commit
> - missing include
> - using new get_starting_head() helper
> - added test description
> ---
> .../fake_sigreturn_duplicated_fpsimd.c | 50 +++++++++++++++++++
> 1 file changed, 50 insertions(+)
> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
>
> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> new file mode 100644
> index 000000000000..f0cc34dac47c
> --- /dev/null
> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> @@ -0,0 +1,50 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2019 ARM Limited
> + *
> + * Place a fake sigframe on the stack including an additional FPSIMD
> + * record: on sigreturn Kernel must spot this attempt and the test
> + * case is expected to be terminated via SEGV.
> + */
> +
> +#include <signal.h>
> +#include <ucontext.h>
> +
> +#include "test_signals_utils.h"
> +#include "testcases.h"
> +
> +struct fake_sigframe sf;
> +
> +static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,
> + siginfo_t *si, ucontext_t *uc)
> +{
> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
> +
> + /* just to fill the ucontext_t with something real */
> + if (!get_current_context(td, &sf.uc))
> + return 1;
> +
> + head = get_starting_head(shead, sizeof(struct fpsimd_context) + HDR_SZ,
> + GET_SF_RESV_SIZE(sf), NULL);
> + if (!head)
> + return 0;
> +
> + /* Add a spurios fpsimd_context */
Nit: spurious
With that,
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
[...]
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile
From: shuah @ 2019-09-17 16:16 UTC (permalink / raw)
To: Cristian Marussi, Anders Roxell
Cc: andreyknvl, linux-kselftest, amit.kachhap, shuah, dave.martin,
linux-arm-kernel
In-Reply-To: <883054a8-7d56-9f8b-ed35-892582bf7599@arm.com>
On 9/17/19 9:58 AM, Cristian Marussi wrote:
> On 17/09/2019 16:29, shuah wrote:
>> On 9/17/19 9:17 AM, Cristian Marussi wrote:
>>> Hi Anders
>>>
>>> thanks for the review.
>>>
>>> On 17/09/2019 14:42, Anders Roxell wrote:
>>>> On 2019-09-10 13:31, Cristian Marussi wrote:
>>>>> Modify KSFT arm64 toplevel Makefile to maintain arm64 kselftests organized
>>>>> by subsystem, keeping them into distinct subdirectories under arm64 custom
>>>>> KSFT directory: tools/testing/selftests/arm64/
>>>>>
>>>>> Add to such toplevel Makefile a mechanism to guess the effective location
>>>>> of Kernel headers as installed by KSFT framework.
>>>>>
>>>>> Fit existing arm64 tags kselftest into this new schema moving them into
>>>>> their own subdirectory (arm64/tags).
>>>>>
>>>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>>>> ---
>>>>> Based on:
>>>>> commit 9ce1263033cd ("selftests, arm64: add a selftest for passing
>>>>> tagged pointers to kernel")
>>>>> ---
>>>>> v5 --> v6
>>>>> - using realpath to avoid passing down relative paths
>>>>> - fix commit msg & Copyright
>>>>> - removed unneded Makefile export
>>>>> - added SUBTARGETS specification, to allow building specific only some
>>>>> arm64 test subsystems
>>>>> v4 --> v5
>>>>> - rebased on arm64/for-next/core
>>>>> - merged this patch with KSFT arm64 tags patch, while moving the latter
>>>>> into its own subdir
>>>>> - moved kernel header includes search mechanism from KSFT arm64
>>>>> SIGNAL Makefile
>>>>> - export proper top_srcdir ENV for lib.mk
>>>>> v3 --> v4
>>>>> - comment reword
>>>>> - simplified documentation in README
>>>>> - dropped README about standalone
>>>>> ---
>>>>> tools/testing/selftests/Makefile | 1 +
>>>>> tools/testing/selftests/arm64/Makefile | 63 +++++++++++++++++--
>>>>> tools/testing/selftests/arm64/README | 25 ++++++++
>>>>> tools/testing/selftests/arm64/tags/Makefile | 6 ++
>>>>> .../arm64/{ => tags}/run_tags_test.sh | 0
>>>>> .../selftests/arm64/{ => tags}/tags_test.c | 0
>>>>> 6 files changed, 91 insertions(+), 4 deletions(-)
>>>>> create mode 100644 tools/testing/selftests/arm64/README
>>>>> create mode 100644 tools/testing/selftests/arm64/tags/Makefile
>>>>> rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%)
>>>>> rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)
>>>>>
>>>>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>>>>> index 25b43a8c2b15..1722dae9381a 100644
>>>>> --- a/tools/testing/selftests/Makefile
>>>>> +++ b/tools/testing/selftests/Makefile
>>>>> @@ -1,5 +1,6 @@
>>>>> # SPDX-License-Identifier: GPL-2.0
>>>>> TARGETS = android
>>>>> +TARGETS += arm64
>>>>> TARGETS += bpf
>>>>> TARGETS += breakpoints
>>>>> TARGETS += capabilities
>>>>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>>>>> index a61b2e743e99..cbb2a5a9e3fc 100644
>>>>> --- a/tools/testing/selftests/arm64/Makefile
>>>>> +++ b/tools/testing/selftests/arm64/Makefile
>>>>> @@ -1,11 +1,66 @@
>>>>> # SPDX-License-Identifier: GPL-2.0
>>>>>
>>>>> -# ARCH can be overridden by the user for cross compiling
>>>>> +# When ARCH not overridden for crosscompiling, lookup machine
>>>>> ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>>>>
>>>>> ifneq (,$(filter $(ARCH),aarch64 arm64))
>>>>> -TEST_GEN_PROGS := tags_test
>>>>> -TEST_PROGS := run_tags_test.sh
>>>>> +SUBTARGETS ?= tags
>>>>> +else
>>>>> +SUBTARGETS :=
>>>>> endif
>>>>>
>>>>> -include ../lib.mk
>>>>> +CFLAGS := -Wall -O2 -g
>>>>> +
>>>>> +# A proper top_srcdir is needed by KSFT(lib.mk)
>>>>> +top_srcdir = $(realpath ../../../../)
>>>>> +
>>>>> +# Additional include paths needed by kselftest.h and local headers
>>>>> +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/
>>>>> +
>>>>> +# Guessing where the Kernel headers could have been installed
>>>>> +# depending on ENV config
>>>>> +ifeq ($(KBUILD_OUTPUT),)
>>>>> +khdr_dir = $(top_srcdir)/usr/include
>>>>> +else
>>>>> +# the KSFT preferred location when KBUILD_OUTPUT is set
>>>>> +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include
>>>>> +endif
>>>>> +
>>>>> +CFLAGS += -I$(khdr_dir)
>>>>> +
>>>>> +export CFLAGS
>>>>> +export top_srcdir
>>>>> +
>>>>> +all:
>>>>> + @for DIR in $(SUBTARGETS); do \
>>>>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>>>>> + mkdir -p $$BUILD_TARGET; \
>>>>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>>>>> + done
>>>>> +
>>>>> +install: all
>>>>> + @for DIR in $(SUBTARGETS); do \
>>>>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>>>>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>>>>> + done
>>>>> +
>>>>> +run_tests: all
>>>>> + @for DIR in $(SUBTARGETS); do \
>>>>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>>>>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>>>>> + done
>>>>> +
>>>>> +# Avoid any output on non arm64 on emit_tests
>>>>> +emit_tests: all
>>>>> + @for DIR in $(SUBTARGETS); do \
>>>>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>>>>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>>>>> + done
>>>>> +
>>>>> +clean:
>>>>> + @for DIR in $(SUBTARGETS); do \
>>>>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>>>>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>>>>> + done
>>>>> +
>>>>> +.PHONY: all clean install run_tests emit_tests
>>>>> diff --git a/tools/testing/selftests/arm64/README b/tools/testing/selftests/arm64/README
>>>>> new file mode 100644
>>>>> index 000000000000..cc1e51796fee
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/arm64/README
>>>>> @@ -0,0 +1,25 @@
>>>>> +KSelfTest ARM64
>>>>> +===============
>>>>> +
>>>>> +- These tests are arm64 specific and so not built or run but just skipped
>>>>> + completely when env-variable ARCH is found to be different than 'arm64'
>>>>> + and `uname -m` reports other than 'aarch64'.
>>>>> +
>>>>> +- Holding true the above, ARM64 KSFT tests can be run within the KSelfTest
>>>>> + framework using standard Linux top-level-makefile targets:
>>>>> +
>>>>> + $ make TARGETS=arm64 kselftest-clean
>>>>> + $ make TARGETS=arm64 kselftest
>>>>> +
>>>>> + or
>>>>> +
>>>>> + $ make -C tools/testing/selftests TARGETS=arm64 \
>>>>> + INSTALL_PATH=<your-installation-path> install
>>>>> +
>>>>> + or, alternatively, only specific arm64/ subtargets can be picked:
>>>>> +
>>>>> + $ make -C tools/testing/selftests TARGETS=arm64 SUBTARGETS="tags signal" \
>>>>> + INSTALL_PATH=<your-installation-path> install
>>>>> +
>>>>> + Further details on building and running KFST can be found in:
>>>>> + Documentation/dev-tools/kselftest.rst
>>>>> diff --git a/tools/testing/selftests/arm64/tags/Makefile b/tools/testing/selftests/arm64/tags/Makefile
>>>>> new file mode 100644
>>>>> index 000000000000..dcc8b0467b68
>>>>> --- /dev/null
>>>>> +++ b/tools/testing/selftests/arm64/tags/Makefile
>>>>> @@ -0,0 +1,6 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +TEST_GEN_PROGS := tags_test
>>>>
>>>> This should be TEST_GEN_FILES, since its used by run_tags_test.sh.
>>>> If its TEST_GEN_PROGS it will be added to the script run_kselftest.sh,
>>>> and I don't think thats the intent, even though it looked like that
>>>> before.
>>>>
>>>
>>> In fact I saw the tags tests running twice (via ./tags_test and via ./run_tags_test.sh) when called
>>> via run_kselftest.sh....but since it was already like that in the original patch so I did not want to
>>> fix it in the context of this series (where tags tests are simply relocated into their own directory)
>>>
>>> I could add a separate fix on top of this series if it could make sense.
>>>
>>
>> We are still in review phase I would think. It would make sense to fix
>> the original patch and not as a separate fix patch.
>
> The original code for:
>
>>>>> +++ b/tools/testing/selftests/arm64/tags/Makefile
>>>>> @@ -0,0 +1,6 @@
>>>>> +# SPDX-License-Identifier: GPL-2.0
>>>>> +
>>>>> +TEST_GEN_PROGS := tags_test
>
> has not been introduced in this series (under review), and that's merged already I think:
>
> https://lore.kernel.org/linux-kselftest/0999c80cd639b78ae27c0674069d552833227564.1561386715.git.andreyknvl@google.com/
>
> This patch only moves the original tags tests (introduced with the above commit) from arm64/ into their own arm64/tags/
> directory and integrate with the new arm64/signal tests by this series.
>
> Here I have just moved down the original code including the bug, that's why I'm saying I could push a fix on top of this series.
>
> I thought I had to keep the two series distinct give that I'm integrating someone else commit (and eventually fix later): but if
> not I can alternatively fix the above tags tests issue in the next v7 02/11 within this series.
>
Thanks. Yeah if it is already in a tree then, let's fix it in a separate
patch and add Fixes tag to make it easier to track them.
thanks,
-- Shuah
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 01/11] kselftest: arm64: extend toplevel skeleton Makefile
From: shuah @ 2019-09-17 16:18 UTC (permalink / raw)
To: Dave Martin, Cristian Marussi
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190917160534.GK27757@arm.com>
On 9/17/19 10:05 AM, Dave Martin wrote:
> On Tue, Sep 10, 2019 at 01:31:01pm +0100, Cristian Marussi wrote:
>> Modify KSFT arm64 toplevel Makefile to maintain arm64 kselftests organized
>> by subsystem, keeping them into distinct subdirectories under arm64 custom
>> KSFT directory: tools/testing/selftests/arm64/
>>
>> Add to such toplevel Makefile a mechanism to guess the effective location
>> of Kernel headers as installed by KSFT framework.
>>
>> Fit existing arm64 tags kselftest into this new schema moving them into
>> their own subdirectory (arm64/tags).
>>
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> Based on:
>> commit 9ce1263033cd ("selftests, arm64: add a selftest for passing
>> tagged pointers to kernel")
>> ---
>> v5 --> v6
>> - using realpath to avoid passing down relative paths
>> - fix commit msg & Copyright
>> - removed unneded Makefile export
>> - added SUBTARGETS specification, to allow building specific only some
>> arm64 test subsystems
>> v4 --> v5
>> - rebased on arm64/for-next/core
>> - merged this patch with KSFT arm64 tags patch, while moving the latter
>> into its own subdir
>> - moved kernel header includes search mechanism from KSFT arm64
>> SIGNAL Makefile
>> - export proper top_srcdir ENV for lib.mk
>> v3 --> v4
>> - comment reword
>> - simplified documentation in README
>> - dropped README about standalone
>> ---
>> tools/testing/selftests/Makefile | 1 +
>> tools/testing/selftests/arm64/Makefile | 63 +++++++++++++++++--
>> tools/testing/selftests/arm64/README | 25 ++++++++
>> tools/testing/selftests/arm64/tags/Makefile | 6 ++
>> .../arm64/{ => tags}/run_tags_test.sh | 0
>> .../selftests/arm64/{ => tags}/tags_test.c | 0
>> 6 files changed, 91 insertions(+), 4 deletions(-)
>> create mode 100644 tools/testing/selftests/arm64/README
>> create mode 100644 tools/testing/selftests/arm64/tags/Makefile
>> rename tools/testing/selftests/arm64/{ => tags}/run_tags_test.sh (100%)
>> rename tools/testing/selftests/arm64/{ => tags}/tags_test.c (100%)
>>
>> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
>> index 25b43a8c2b15..1722dae9381a 100644
>> --- a/tools/testing/selftests/Makefile
>> +++ b/tools/testing/selftests/Makefile
>> @@ -1,5 +1,6 @@
>> # SPDX-License-Identifier: GPL-2.0
>> TARGETS = android
>> +TARGETS += arm64
>> TARGETS += bpf
>> TARGETS += breakpoints
>> TARGETS += capabilities
>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>> index a61b2e743e99..cbb2a5a9e3fc 100644
>> --- a/tools/testing/selftests/arm64/Makefile
>> +++ b/tools/testing/selftests/arm64/Makefile
>> @@ -1,11 +1,66 @@
>> # SPDX-License-Identifier: GPL-2.0
>>
>> -# ARCH can be overridden by the user for cross compiling
>> +# When ARCH not overridden for crosscompiling, lookup machine
>> ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>>
>> ifneq (,$(filter $(ARCH),aarch64 arm64))
>> -TEST_GEN_PROGS := tags_test
>> -TEST_PROGS := run_tags_test.sh
>> +SUBTARGETS ?= tags
>> +else
>> +SUBTARGETS :=
>> endif
>>
>> -include ../lib.mk
>> +CFLAGS := -Wall -O2 -g
>> +
>> +# A proper top_srcdir is needed by KSFT(lib.mk)
>> +top_srcdir = $(realpath ../../../../)
>> +
>> +# Additional include paths needed by kselftest.h and local headers
>> +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/
>> +
>> +# Guessing where the Kernel headers could have been installed
>> +# depending on ENV config
>> +ifeq ($(KBUILD_OUTPUT),)
>> +khdr_dir = $(top_srcdir)/usr/include
>> +else
>> +# the KSFT preferred location when KBUILD_OUTPUT is set
>> +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include
>> +endif
>
> I still tend to think that for now we should just do what all the other
> tests do.
>
> Most tests use
>
> CFLAGS += -I../../../../usr/include/
>
> in their Makefiles.
>
> For us, the test Makefiles are nested one level deeper, so I guess
> we would put
>
> CFLAGS += -I../../../../../usr/include/
>
> in each.
>
>
> This will break in some cases, but only in the same cases where
> kselftest is already broken.
>
> Ideally we would fix this globally, but can that instead be done
> independently of this series?
>
> Fixing only arm64, by pasting some arbitrary logic from
> selftests/Makefile doesn't seem like a future-proof approach.
>
>
> Or did I miss something?
>
>> +
>> +CFLAGS += -I$(khdr_dir)
>> +
>> +export CFLAGS
>> +export top_srcdir
>> +
>> +all:
>> + @for DIR in $(SUBTARGETS); do \
>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>> + mkdir -p $$BUILD_TARGET; \
>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>> + done
>> +
>> +install: all
>> + @for DIR in $(SUBTARGETS); do \
>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>> + done
>> +
>> +run_tests: all
>> + @for DIR in $(SUBTARGETS); do \
>> + BUILD_TARGET=$(OUTPUT)/$$DIR; \
>> + make OUTPUT=$$BUILD_TARGET -C $$DIR $@; \
>> + done
>> +
>> +# Avoid any output on non arm64 on emit_tests
>
> This comment can be dropped: the whole file does nothing for
> non-arm64, and it achieves it in the same way as other arch-specific
> Makefiles, so it's odd to have the comment here specifically (?)
>
>
> With or without the above changes, I'm happy to give
>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
>
> (but Shuah or someone will need to give a view on how this integrates
> with kselftest overall).
>
I am reviewing the series this week. I will provide comments in a
day or two.
thanks,
-- Shuah
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH V3 0/4] clk: imx8m: fix glitch/mux
From: Stephen Boyd @ 2019-09-17 16:28 UTC (permalink / raw)
To: festevam@gmail.com, mturquette@baylibre.com,
s.hauer@pengutronix.de, shawnguo@kernel.org, Peng Fan
Cc: Abel Vesa, Anson Huang, linux-kernel@vger.kernel.org,
dl-linux-imx, kernel@pengutronix.de, Leonard Crestez,
linux-clk@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
Jacky Bai
In-Reply-To: <AM0PR04MB4481A31DD68C3C3409E95339888F0@AM0PR04MB4481.eurprd04.prod.outlook.com>
Quoting Peng Fan (2019-09-16 23:20:15)
> Hi Stephen, Shawn,
>
> > Subject: [PATCH V3 0/4] clk: imx8m: fix glitch/mux
>
> Sorry to ping early. Is there a chance to land this patchset in 5.3 release?
>
No, it won't be in 5.3 because that version is released. Shawn already
sent the PR for 5.4 too so this will most likely be in v5.5 at the
earliest.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/7] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram_alt/apb
From: Stephen Boyd @ 2019-09-17 16:32 UTC (permalink / raw)
To: Leonard Crestez, Shawn Guo
Cc: Mark Rutland, Artur Świgoń, Jacky Bai, Viresh Kumar,
Michael Turquette, Alexandre Bailon, linux-clk@vger.kernel.org,
Abel Vesa, Saravana Kannan, Krzysztof Kozlowski, Chanwoo Choi,
MyungJoo Ham, dl-linux-imx, devicetree@vger.kernel.org,
linux-pm@vger.kernel.org, Rob Herring,
linux-arm-kernel@lists.infradead.org, Aisheng Dong, Anson Huang,
Kyungmin Park, kernel@pengutronix.de, Fabio Estevam,
Georgi Djakov
In-Reply-To: <VI1PR04MB70239F26BE42AD5E6FC3639DEE8C0@VI1PR04MB7023.eurprd04.prod.outlook.com>
Quoting Leonard Crestez (2019-09-16 16:03:53)
> On 2019-09-16 11:33 PM, Stephen Boyd wrote:
> > Quoting Leonard Crestez (2019-08-20 08:45:06)
> >> Dram frequency changes required modifying these clocks outside the
> >> control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
> >> rates are always read back from registers.
> >
> > Why can't we control the clks from the clk framework? Please add that
> > information in the commit text here.
>
> OK, I will update commit message and comments
>
> These clocks are only modified for DRAM frequency switches during which
> DRAM is briefly inaccessible. The switch is performed with a SMC call to
> by TF-A which runs from a SRAM area. Upon returning to linux several
> clocks bits are modified and we need to update them.
>
> For rate bits an easy solution is to just mark with
> CLK_GET_RATE_NOCACHE, muxes are handled explicitly.
Is there any reason to expose or control these clks from Linux then? It
might be easier to just make any children clks of the DRAM frequency clk
"root" clks and then ignore any frequency that they might have.
Similarly, because the SMC call is used to change the frequency, it may
be simpler to handle that completely outside of the clk framework (it
may already be this way in this patch series but I haven't read
everything here).
>
> Linux code performing the SMC call is also part of this series:
>
> https://patchwork.kernel.org/patch/11104145/
>
> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
> >> ---
> >> drivers/clk/imx/clk-imx8mm.c | 6 ++++--
> >> drivers/clk/imx/clk-imx8mn.c | 6 ++++--
> >> drivers/clk/imx/clk-imx8mq.c | 7 ++++---
> >> 3 files changed, 12 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/clk/imx/clk-imx8mm.c b/drivers/clk/imx/clk-imx8mm.c
> >> index 4ead3ea2713c..6cac80550f43 100644
> >> --- a/drivers/clk/imx/clk-imx8mm.c
> >> +++ b/drivers/clk/imx/clk-imx8mm.c
> >> @@ -526,12 +526,14 @@ static int imx8mm_clocks_probe(struct platform_device *pdev)
> >> /* IPG */
> >> clks[IMX8MM_CLK_IPG_ROOT] = imx_clk_divider2("ipg_root", "ahb", base + 0x9080, 0, 1);
> >> clks[IMX8MM_CLK_IPG_AUDIO_ROOT] = imx_clk_divider2("ipg_audio_root", "audio_ahb", base + 0x9180, 0, 1);
> >>
> >> /* IP */
> >> - clks[IMX8MM_CLK_DRAM_ALT] = imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000);
> >> - clks[IMX8MM_CLK_DRAM_APB] = imx8m_clk_composite_critical("dram_apb", imx8mm_dram_apb_sels, base + 0xa080);
> >> + clks[IMX8MM_CLK_DRAM_ALT] = __imx8m_clk_composite("dram_alt", imx8mm_dram_alt_sels, base + 0xa000,
> >> + CLK_GET_RATE_NOCACHE);
> >> + clks[IMX8MM_CLK_DRAM_APB] = __imx8m_clk_composite("dram_apb", imx8mm_dram_apb_sels, base + 0xa080,
> >> + CLK_IS_CRITICAL | CLK_GET_RATE_NOCACHE);
> >
> > Also, add a comment to this effect about why it can't be done from the
> > clk framework wherever the CLK_GET_RATE_NOCACHE flag is set. Basically
> > this flag is a hack and is an example of something that we need to fix.
>
> DRAM freq switch requires multiple clk changes to be performed
> atomically while DRAM itself is not accessible so it's not something to
> "fix".
Ok. Fix may be the wrong word.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/7] clk: actions: Fix factor clk struct member access
From: Stephen Boyd @ 2019-09-17 16:34 UTC (permalink / raw)
To: Manivannan Sadhasivam, afaerber, robh+dt, ulf.hansson
Cc: devicetree, linux-mmc, linus.walleij, linux-actions, linux-kernel,
thomas.liau, Manivannan Sadhasivam, linux-clk, linux-arm-kernel
In-Reply-To: <20190916154546.24982-2-manivannan.sadhasivam@linaro.org>
Quoting Manivannan Sadhasivam (2019-09-16 08:45:40)
> Since the helper "owl_factor_helper_round_rate" is shared between factor
> and composite clocks, using the factor clk specific helper function
> like "hw_to_owl_factor" to access its members will create issues when
> called from composite clk specific code. Hence, pass the "factor_hw"
> struct pointer directly instead of fetching it using factor clk specific
> helpers.
>
> This issue has been observed when a composite clock like "sd0_clk" tried
> to call "owl_factor_helper_round_rate" resulting in pointer dereferencing
> error.
>
> While we are at it, let's rename the "clk_val_best" function to
> "owl_clk_val_best" since this is an owl SoCs specific helper.
>
> Fixes: 4bb78fc9744a ("clk: actions: Add factor clock support")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> ---
I can apply this to clk-next?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s max-speed
From: tinywrkb @ 2019-09-17 16:34 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Mark Rutland, Andrew Lunn, Baruch Siach,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sascha Hauer, open list, Rob Herring,
NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
linux-arm-kernel
In-Reply-To: <20190917153027.GW25745@shell.armlinux.org.uk>
On Tue, Sep 17, 2019 at 04:30:27PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Sep 17, 2019 at 04:17:07PM +0100, Russell King - ARM Linux admin wrote:
> > On Tue, Sep 17, 2019 at 02:39:43PM +0100, Russell King - ARM Linux admin wrote:
> > > On Tue, Sep 17, 2019 at 04:32:53PM +0300, tinywrkb wrote:
> > > > On Tue, Sep 17, 2019 at 02:54:34PM +0200, Andrew Lunn wrote:
> > > > > On Tue, Sep 17, 2019 at 03:41:01PM +0300, tinywrkb wrote:
> > > > > > On Sun, Sep 15, 2019 at 03:56:52PM +0200, Andrew Lunn wrote:
> > > > > > > > Tinywrkb confirmed to me in private communication that revert of
> > > > > > > > 5502b218e001 fixes Ethernet for him on effected system.
> > > > > > > >
> > > > > > > > He also referred me to an old Cubox-i spec that lists 10/100 Ethernet
> > > > > > > > only for i.MX6 Solo/DualLite variants of Cubox-i. It turns out that
> > > > > > > > there was a plan to use a different 10/100 PHY for Solo/DualLite
> > > > > > > > SOMs. This plan never materialized. All SolidRun i.MX6 SOMs use the same
> > > > > > > > AR8035 PHY that supports 1Gb.
> > > > > > > >
> > > > > > > > Commit 5502b218e001 might be triggering a hardware issue on the affected
> > > > > > > > Cubox-i. I could not reproduce the issue here with Cubox-i and a Dual
> > > > > > > > SOM variant running v5.3-rc8. I have no Solo/DualLite variant handy at
> > > > > > > > the moment.
> > > > > > >
> > > > > > > Could somebody with an affected device show us the output of ethtool
> > > > > > > with and without 5502b218e001. Does one show 1G has been negotiated,
> > > > > > > and the other 100Mbps? If this is true, how does it get 100Mbps
> > > > > > > without that patch? We are missing a piece of the puzzle.
> > > > > > >
> > > > > > > Andrew
> > > > > >
> > > > > > linux-test-5.1rc1-a2703de70942-without_bad_commit
> > > > > >
> > > > > > Settings for eth0:
> > > > > > Supported ports: [ TP MII ]
> > > > > > Supported link modes: 10baseT/Half 10baseT/Full
> > > > > > 100baseT/Half 100baseT/Full
> > > > > > 1000baseT/Full
> > > > >
> > > > > So this means the local device says it can do 1000Mbps.
> > > > >
> > > > >
> > > > > > Supported pause frame use: Symmetric
> > > > > > Supports auto-negotiation: Yes
> > > > > > Supported FEC modes: Not reported
> > > > > > Advertised link modes: 10baseT/Half 10baseT/Full
> > > > > > 100baseT/Half 100baseT/Full
> > > > > > 1000baseT/Full
> > > > >
> > > > > The link peer can also do 1000Mbps.
> > > > >
> > > > >
> > > > > > Advertised pause frame use: Symmetric
> > > > > > Advertised auto-negotiation: Yes
> > > > > > Advertised FEC modes: Not reported
> > > > > > Link partner advertised link modes: 10baseT/Half 10baseT/Full
> > > > > > 100baseT/Half 100baseT/Full
> > > > > > 1000baseT/Full
> > > > > > Link partner advertised pause frame use: Symmetric
> > > > > > Link partner advertised auto-negotiation: Yes
> > > > > > Link partner advertised FEC modes: Not reported
> > > > > > Speed: 100Mb/s
> > > > >
> > > > > Yet they have decided to do 100Mbps.
> > > > >
> > > > > We need to understand Why? The generic PHY driver would not do this on
> > > > > its own. So i'm thinking something has poked a PHY register with some
> > > > > value, and this patch is causing it to be over written.
> > > > >
> > > > > Please can you use mii-tool -v -v to dump the PHY registers in each
> > > > > case.
> > > > >
> > > > > Thanks
> > > > > Andrew
> > > >
> > > > Here's the output of # mii-tool -v -v eth0
> > > >
> > > > * linux-test-5.1rc1-a2703de70942-without_bad_commit
> > > >
> > > > Using SIOCGMIIPHY=0x8947
> > > > eth0: negotiated 100baseTx-FD flow-control, link ok
> > > > registers for MII PHY 0:
> > > > 3100 796d 004d d072 15e1 c5e1 000f 0000
> > > > 0000 0000 0800 0000 0000 0000 0000 a000
> > > > 0000 0000 0000 f420 082c 0000 04e8 0000
> > > > 3200 3000 0000 063d 0000 0000 0000 0000
> > > > product info: vendor 00:13:74, model 7 rev 2
> > > > basic mode: autonegotiation enabled
> > > > basic status: autonegotiation complete, link ok
> > > > capabilities: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> > > > advertising: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> > > > link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> > >
> > > This is *not* advertising 1000baseT modes (register 9).
> > >
> > > >
> > > >
> > > > * linux-test-5.1rc1-5502b218e001-with_bad_commit
> > > >
> > > > Using SIOCGMIIPHY=0x8947
> > > > eth0: negotiated 100baseTx-FD flow-control, link ok
> > > > registers for MII PHY 0:
> > > > 3100 796d 004d d072 15e1 c5e1 000d 0000
> > > > 0000 0000 0800 0000 0000 0000 0000 a000
> > > > 0000 0000 0000 0000 082c 0000 04e8 0000
> > > > 3200 3000 0000 063d 0000 0000 0000 0000
> > > > product info: vendor 00:13:74, model 7 rev 2
> > > > basic mode: autonegotiation enabled
> > > > basic status: autonegotiation complete, link ok
> > > > capabilities: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> > > > advertising: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> > > > link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> > >
> > > Neither is this.
> > >
> > > However, the kernel and phylib _thinks_ that it is. My guess is
> > > something has rewritten the PHY registers from userspace, rather
> > > than using ethtool to change the advertisment. The MAC is still
> > > trying to operate at 1000Mbps (since that is what phylib resolved)
> > > yet the link might be actually operating at 100Mbps - but for that
> > > to happen, we should've seen the link go down and up again.
> > >
> > > Odd.
> >
> > For reference, here I see:
> >
> > Using SIOCGMIIPHY=0x8947
> > eth0: negotiated 1000baseT-FD flow-control, link ok
> > registers for MII PHY 0:
> > 3100 796d 004d d072 15e1 c1e1 000d 0000
> > 0000 0200 3800 0000 0000 0000 0000 a000
> > 0000 0000 0000 0000 082c 0000 04e8 0000
> > 3200 3000 0000 060e 0000 0000 0000 0000
> > product info: vendor 00:13:74, model 7 rev 2
> > basic mode: autonegotiation enabled
> > basic status: autonegotiation complete, link ok
> > capabilities: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> > advertising: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> > link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> >
> > Note that userspace writing to register 9 (1G settings) will update
> > the physical PHY's advertisment for 1G speeds, but the kernel's idea
> > will not be updated, so things will become de-sync'd, and the kernel
> > _will_ incorrectly determine the speed.
> >
> > So, are you using mii-tool or mii-diag to change the negotiation
> > settings, rather than using ethtool?
> >
> > This is likely the case, as prior to 5502b218e001, we used to read
> > the advertisment registers afresh each time we evaluate the resulting
> > link mode. After 5502b218e001, we use the advertisment mask cached
> > in phydev->advertising, which, looking at phy_mii_ioctl(), will not
> > be updated if register 9 is written.
>
> Please try this patch - I haven't written a commit message for it yet,
> it's just to prove the above point.
>
> Thanks.
>
> drivers/net/phy/phy.c | 5 +++++
> include/linux/mii.h | 9 +++++++++
> 2 files changed, 14 insertions(+)
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 7af390d8bbdb..068a08a50064 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -458,6 +458,11 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
> val);
> change_autoneg = true;
> break;
> + case MII_CTRL1000:
> + mii_ctrl1000_mod_linkmode_adv_t(phydev->advertising,
> + val);
> + change_autoneg = true;
> + break;
> default:
> /* do nothing */
> break;
> diff --git a/include/linux/mii.h b/include/linux/mii.h
> index 5cd824c1c0ca..1249d32dbd63 100644
> --- a/include/linux/mii.h
> +++ b/include/linux/mii.h
> @@ -455,6 +455,15 @@ static inline void mii_lpa_mod_linkmode_lpa_t(unsigned long *lp_advertising,
> lp_advertising, lpa & LPA_LPACK);
> }
>
> +static inline void mii_ctrl1000_mod_linkmode_adv_t(unsigned long *advertising,
> + u32 ctrl1000)
> +{
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, advertising,
> + ctrl1000 & ADVERTISE_1000HALF);
> + linkmode_mod_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, advertising,
> + ctrl1000 & ADVERTISE_1000FULL);
> +}
> +
> /**
> * linkmode_adv_to_lcl_adv_t
> * @advertising:pointer to linkmode advertising
>
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
> According to speedtest.net: 11.9Mbps down 500kbps up
The patch didn't fix the issue.
# ethtool eth0
Settings for eth0:
Supported ports: [ TP MII ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supported pause frame use: Symmetric
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
Advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Advertised pause frame use: Symmetric
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
Link partner advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Link partner advertised pause frame use: Symmetric
Link partner advertised auto-negotiation: Yes
Link partner advertised FEC modes: Not reported
Speed: 1000Mb/s
Duplex: Full
Port: MII
PHYAD: 0
Transceiver: internal
Auto-negotiation: on
Supports Wake-on: d
Wake-on: d
Link detected: yes
# mii-tool -v -v eth0
Using SIOCGMIIPHY=0x8947
eth0: negotiated 100baseTx-FD flow-control, link ok
registers for MII PHY 0:
3100 796d 004d d072 15e1 c5e1 000f 0000
0000 0000 0800 0000 0000 0000 0000 a000
0000 0000 0000 f420 082c 0000 04e8 0000
3200 3000 0000 063d 0000 0000 0000 0000
product info: vendor 00:13:74, model 7 rev 2
basic mode: autonegotiation enabled
basic status: autonegotiation complete, link ok
capabilities: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
advertising: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
# journalctl -b | egrep -i 'phy|eth|fec'|grep -v usb
kernel: Booting Linux on physical CPU 0x0
kernel: libphy: Fixed MDIO Bus: probed
kernel: libphy: fec_enet_mii_bus: probed
kernel: fec 2188000.ethernet eth0: registered PHC device 0
kernel: dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY)
kernel: Generic PHY 2188000.ethernet-1:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=2188000.ethernet-1:00, irq=POLL)
kernel: fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
kernel: IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
systemd-networkd[242]: eth0: Gained carrier
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v4 1/7] clk: actions: Fix factor clk struct member access
From: Manivannan Sadhasivam @ 2019-09-17 16:36 UTC (permalink / raw)
To: Stephen Boyd
Cc: devicetree, ulf.hansson, linux-actions, linus.walleij, linux-mmc,
linux-kernel, thomas.liau, linux-clk, robh+dt, afaerber,
linux-arm-kernel
In-Reply-To: <20190917163419.4C4DD20665@mail.kernel.org>
On Tue, Sep 17, 2019 at 09:34:18AM -0700, Stephen Boyd wrote:
> Quoting Manivannan Sadhasivam (2019-09-16 08:45:40)
> > Since the helper "owl_factor_helper_round_rate" is shared between factor
> > and composite clocks, using the factor clk specific helper function
> > like "hw_to_owl_factor" to access its members will create issues when
> > called from composite clk specific code. Hence, pass the "factor_hw"
> > struct pointer directly instead of fetching it using factor clk specific
> > helpers.
> >
> > This issue has been observed when a composite clock like "sd0_clk" tried
> > to call "owl_factor_helper_round_rate" resulting in pointer dereferencing
> > error.
> >
> > While we are at it, let's rename the "clk_val_best" function to
> > "owl_clk_val_best" since this is an owl SoCs specific helper.
> >
> > Fixes: 4bb78fc9744a ("clk: actions: Add factor clock support")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > Reviewed-by: Stephen Boyd <sboyd@kernel.org>
> > ---
>
> I can apply this to clk-next?
Yes, please :-) Rest can go through Ulf's tree.
Thanks,
Mani
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/7] clk: imx8m: Set CLK_GET_RATE_NOCACHE on dram_alt/apb
From: Leonard Crestez @ 2019-09-17 16:59 UTC (permalink / raw)
To: Stephen Boyd, Shawn Guo
Cc: Mark Rutland, Artur Świgoń, Jacky Bai, Viresh Kumar,
Michael Turquette, Alexandre Bailon, linux-clk@vger.kernel.org,
Abel Vesa, Saravana Kannan, Krzysztof Kozlowski, Chanwoo Choi,
MyungJoo Ham, dl-linux-imx, devicetree@vger.kernel.org,
linux-pm@vger.kernel.org, Rob Herring,
linux-arm-kernel@lists.infradead.org, Aisheng Dong, Anson Huang,
Kyungmin Park, kernel@pengutronix.de, Fabio Estevam,
Georgi Djakov
In-Reply-To: <20190917163215.A838020665@mail.kernel.org>
On 2019-09-17 7:32 PM, Stephen Boyd wrote:
> Quoting Leonard Crestez (2019-09-16 16:03:53)
>> On 2019-09-16 11:33 PM, Stephen Boyd wrote:
>>> Quoting Leonard Crestez (2019-08-20 08:45:06)
>>>> Dram frequency changes required modifying these clocks outside the
>>>> control of clk framework. Mark them as CLK_GET_RATE_NOCACHE so that
>>>> rates are always read back from registers.
>>>
>>> Why can't we control the clks from the clk framework? Please add that
>>> information in the commit text here.
>>
>> OK, I will update commit message and comments
>>
>> These clocks are only modified for DRAM frequency switches during which
>> DRAM is briefly inaccessible. The switch is performed with a SMC call to
>> by TF-A which runs from a SRAM area. Upon returning to linux several
>> clocks bits are modified and we need to update them.
>>
>> For rate bits an easy solution is to just mark with
>> CLK_GET_RATE_NOCACHE, muxes are handled explicitly.
>
> Is there any reason to expose or control these clks from Linux then? It
> might be easier to just make any children clks of the DRAM frequency clk
> "root" clks and then ignore any frequency that they might have.
> Similarly, because the SMC call is used to change the frequency, it may
> be simpler to handle that completely outside of the clk framework (it
> may already be this way in this patch series but I haven't read
> everything here).
The dram alt/apb clocks are real imx8m composite clocks with the same HW
implementation as used for peripherals. They also have mux parents which
are under the control of the clock framework so the freq switching code
takes care to properly enable the new parents before calling SMC.
See imx_ddrc_set_freq: https://patchwork.kernel.org/patch/11104145/
Removing dram alt/apb clocks from the tree would still require keeping
possible parents enabled somehow. It wouldn't be simpler but a lot uglier.
--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s max-speed
From: Russell King - ARM Linux admin @ 2019-09-17 17:04 UTC (permalink / raw)
To: tinywrkb
Cc: Mark Rutland, Andrew Lunn, Baruch Siach,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Shawn Guo, Sascha Hauer, open list, Rob Herring, NXP Linux Team,
Pengutronix Kernel Team, Fabio Estevam, linux-arm-kernel
In-Reply-To: <20190917163427.GA1475935@arch-dsk-01>
On Tue, Sep 17, 2019 at 07:34:27PM +0300, tinywrkb wrote:
> The patch didn't fix the issue.
>
> # ethtool eth0
>
> Settings for eth0:
> Supported ports: [ TP MII ]
> Supported link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Supported pause frame use: Symmetric
> Supports auto-negotiation: Yes
> Supported FEC modes: Not reported
> Advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Advertised pause frame use: Symmetric
> Advertised auto-negotiation: Yes
> Advertised FEC modes: Not reported
> Link partner advertised link modes: 10baseT/Half 10baseT/Full
> 100baseT/Half 100baseT/Full
> 1000baseT/Full
> Link partner advertised pause frame use: Symmetric
> Link partner advertised auto-negotiation: Yes
> Link partner advertised FEC modes: Not reported
> Speed: 1000Mb/s
> Duplex: Full
> Port: MII
> PHYAD: 0
> Transceiver: internal
> Auto-negotiation: on
> Supports Wake-on: d
> Wake-on: d
> Link detected: yes
>
> # mii-tool -v -v eth0
>
> Using SIOCGMIIPHY=0x8947
> eth0: negotiated 100baseTx-FD flow-control, link ok
> registers for MII PHY 0:
> 3100 796d 004d d072 15e1 c5e1 000f 0000
> 0000 0000 0800 0000 0000 0000 0000 a000
> 0000 0000 0000 f420 082c 0000 04e8 0000
> 3200 3000 0000 063d 0000 0000 0000 0000
> product info: vendor 00:13:74, model 7 rev 2
> basic mode: autonegotiation enabled
> basic status: autonegotiation complete, link ok
> capabilities: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> advertising: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
>
> # journalctl -b | egrep -i 'phy|eth|fec'|grep -v usb
>
> kernel: Booting Linux on physical CPU 0x0
> kernel: libphy: Fixed MDIO Bus: probed
> kernel: libphy: fec_enet_mii_bus: probed
> kernel: fec 2188000.ethernet eth0: registered PHC device 0
> kernel: dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY)
> kernel: Generic PHY 2188000.ethernet-1:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=2188000.ethernet-1:00, irq=POLL)
> kernel: fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> kernel: IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> systemd-networkd[242]: eth0: Gained carrier
Okay, so this is getting weird.
ethtool still shows that 1000baseT/Full is being advertised, yet the
PHY disagrees:
3100 796d 004d d072 15e1 c5e1 000f 0000
0000 0000 0800 0000 0000 0000 0000 a000
^^^^
Gigabit control register, bits 9 should be set, but it's clear.
Looking at the following registers, brings up another possibility what
is going on:
0000 0000 0000 f420 082c 0000 04e8 0000
^^^^ ^^^^
These two registers may provide a hint. Of the first register, which
is the interrupt status register, bit 5 is set, indicating that a
"smartspeed downgrade occurred". The second register is the smartspeed
configuration, which basically says that the feature is enabled.
Smartspeed is designed to allow the link to come up if two-pair CAT5
cable is used (are you using a 4-pair or 2-pair cable?) by making the
link fall back to 100mbit, or with CAT3 cable, 10mbit speeds. What
isn't specified is whether it does this by clearing bits in the various
advertisement registers.
Given what you've said so far, I'd suggest that this is indeed the
case - when smartspeed is triggered, advertisement bits are cleared by
the PHY without the kernel's knowledge, leading to the kernel getting
the speed resolution incorrect after 5502b218e001.
There's another issue here - if smartspeed clears advertisement bits,
then if you connect a 4-pair cable after having used a 2-pair cable,
you'd still be limited to 100mbit. The ethtool output will be just
as confusing.
The only thing I can think we should do is to read-back the
advertisement from the PHY whenever we read the rest of the status
and update the phy->advertising mask, just like we do with the link
partner advertisement.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/6] Timer: expose monotonic clock and counter value
From: Marc Zyngier @ 2019-09-17 17:10 UTC (permalink / raw)
To: Jianyong Wu, netdev, pbonzini, sean.j.christopherson,
richardcochran, Mark.Rutland, Will.Deacon, suzuki.poulose
Cc: justin.he, Steve.Capper, linux-kernel, Kaly.Xin, nd,
linux-arm-kernel
In-Reply-To: <20190917112430.45680-4-jianyong.wu@arm.com>
On 17/09/2019 12:24, Jianyong Wu wrote:
> A number of PTP drivers (such as ptp-kvm) are assuming what the
> current clock source is, which could lead to interesting effects on
> systems where the clocksource can change depending on external events.
>
> For this purpose, add a new API that retrives both the current
> monotonic clock as well as its counter value.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Jianyong Wu <jianyong.wu@arm.com>
There must be something wrong with the way you've taken this patch in
your tree. My authorship is gone (not that I deeply care about it, but
it is good practice to keep attributions), and the subject line has been
rewritten.
I'd appreciate it if you could fix this in a future revision of this
series. For reference, the original patch is here[1].
> ---
> include/linux/timekeeping.h | 3 +++
> kernel/time/timekeeping.c | 13 +++++++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index a8ab0f143ac4..a5389adaa8bc 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -247,6 +247,9 @@ extern int get_device_system_crosststamp(
> struct system_time_snapshot *history,
> struct system_device_crosststamp *xtstamp);
>
> +/* Obtain current monotonic clock and its counter value */
> +extern void get_current_counterval(struct system_counterval_t *sc);
> +
> /*
> * Simultaneously snapshot realtime and monotonic raw clocks
> */
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 44b726bab4bd..07a0969625b1 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1098,6 +1098,19 @@ static bool cycle_between(u64 before, u64 test, u64 after)
> return false;
> }
>
> +/**
> + * get_current_counterval - Snapshot the current clocksource and counter value
> + * @sc: Pointer to a struct containing the current clocksource and its value
> + */
> +void get_current_counterval(struct system_counterval_t *sc)
> +{
> + struct timekeeper *tk = &tk_core.timekeeper;
> +
> + sc->cs = READ_ONCE(tk->tkr_mono.clock);
> + sc->cycles = sc->cs->read(sc->cs);
> +}
> +EXPORT_SYMBOL_GPL(get_current_counterval);
This export wasn't in my original patch. I guess you need it because
your ptp driver builds as a module? It'd be good to mention it in the
commit log.
> +
> /**
> * get_device_system_crosststamp - Synchronously capture system/device timestamp
> * @get_time_fn: Callback to get simultaneous device time and
>
Thanks,
M.
[1]
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=timer/counterval&id=a6e8abce025691b6a55e1c195878d7f76bfeb9d1
--
Jazz is not dead, it just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s max-speed
From: Russell King - ARM Linux admin @ 2019-09-17 17:19 UTC (permalink / raw)
To: tinywrkb
Cc: Mark Rutland, Andrew Lunn, Baruch Siach,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Fabio Estevam, Sascha Hauer, open list, Rob Herring,
NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
linux-arm-kernel
In-Reply-To: <20190917170419.GX25745@shell.armlinux.org.uk>
On Tue, Sep 17, 2019 at 06:04:19PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Sep 17, 2019 at 07:34:27PM +0300, tinywrkb wrote:
> > The patch didn't fix the issue.
> >
> > # ethtool eth0
> >
> > Settings for eth0:
> > Supported ports: [ TP MII ]
> > Supported link modes: 10baseT/Half 10baseT/Full
> > 100baseT/Half 100baseT/Full
> > 1000baseT/Full
> > Supported pause frame use: Symmetric
> > Supports auto-negotiation: Yes
> > Supported FEC modes: Not reported
> > Advertised link modes: 10baseT/Half 10baseT/Full
> > 100baseT/Half 100baseT/Full
> > 1000baseT/Full
> > Advertised pause frame use: Symmetric
> > Advertised auto-negotiation: Yes
> > Advertised FEC modes: Not reported
> > Link partner advertised link modes: 10baseT/Half 10baseT/Full
> > 100baseT/Half 100baseT/Full
> > 1000baseT/Full
> > Link partner advertised pause frame use: Symmetric
> > Link partner advertised auto-negotiation: Yes
> > Link partner advertised FEC modes: Not reported
> > Speed: 1000Mb/s
> > Duplex: Full
> > Port: MII
> > PHYAD: 0
> > Transceiver: internal
> > Auto-negotiation: on
> > Supports Wake-on: d
> > Wake-on: d
> > Link detected: yes
> >
> > # mii-tool -v -v eth0
> >
> > Using SIOCGMIIPHY=0x8947
> > eth0: negotiated 100baseTx-FD flow-control, link ok
> > registers for MII PHY 0:
> > 3100 796d 004d d072 15e1 c5e1 000f 0000
> > 0000 0000 0800 0000 0000 0000 0000 a000
> > 0000 0000 0000 f420 082c 0000 04e8 0000
> > 3200 3000 0000 063d 0000 0000 0000 0000
> > product info: vendor 00:13:74, model 7 rev 2
> > basic mode: autonegotiation enabled
> > basic status: autonegotiation complete, link ok
> > capabilities: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD
> > advertising: 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> > link partner: 1000baseT-FD 100baseTx-FD 100baseTx-HD 10baseT-FD 10baseT-HD flow-control
> >
> > # journalctl -b | egrep -i 'phy|eth|fec'|grep -v usb
> >
> > kernel: Booting Linux on physical CPU 0x0
> > kernel: libphy: Fixed MDIO Bus: probed
> > kernel: libphy: fec_enet_mii_bus: probed
> > kernel: fec 2188000.ethernet eth0: registered PHC device 0
> > kernel: dwhdmi-imx 120000.hdmi: Detected HDMI TX controller v1.31a with HDCP (DWC HDMI 3D TX PHY)
> > kernel: Generic PHY 2188000.ethernet-1:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=2188000.ethernet-1:00, irq=POLL)
> > kernel: fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control rx/tx
> > kernel: IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> > systemd-networkd[242]: eth0: Gained carrier
>
> Okay, so this is getting weird.
>
> ethtool still shows that 1000baseT/Full is being advertised, yet the
> PHY disagrees:
>
> 3100 796d 004d d072 15e1 c5e1 000f 0000
> 0000 0000 0800 0000 0000 0000 0000 a000
> ^^^^
> Gigabit control register, bits 9 should be set, but it's clear.
>
> Looking at the following registers, brings up another possibility what
> is going on:
>
> 0000 0000 0000 f420 082c 0000 04e8 0000
> ^^^^ ^^^^
>
> These two registers may provide a hint. Of the first register, which
> is the interrupt status register, bit 5 is set, indicating that a
> "smartspeed downgrade occurred". The second register is the smartspeed
> configuration, which basically says that the feature is enabled.
>
> Smartspeed is designed to allow the link to come up if two-pair CAT5
> cable is used (are you using a 4-pair or 2-pair cable?) by making the
> link fall back to 100mbit, or with CAT3 cable, 10mbit speeds. What
> isn't specified is whether it does this by clearing bits in the various
> advertisement registers.
>
> Given what you've said so far, I'd suggest that this is indeed the
> case - when smartspeed is triggered, advertisement bits are cleared by
> the PHY without the kernel's knowledge, leading to the kernel getting
> the speed resolution incorrect after 5502b218e001.
>
> There's another issue here - if smartspeed clears advertisement bits,
> then if you connect a 4-pair cable after having used a 2-pair cable,
> you'd still be limited to 100mbit. The ethtool output will be just
> as confusing.
>
> The only thing I can think we should do is to read-back the
> advertisement from the PHY whenever we read the rest of the status
> and update the phy->advertising mask, just like we do with the link
> partner advertisement.
This patch will disable smartspeed - could you see what happens, and
whether you can get the link to come up at all. You might need to see
whether wiggling the RJ45 helps (I've had that sort of thing with some
cables.)
You might also need "ethtool -s eth0 advertise ffcf" after trying that
if it doesn't work to take the gigabit speeds out of the advertisement.
Thanks.
drivers/net/phy/at803x.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index b3893347804d..85cf4a4a5e81 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -296,6 +296,11 @@ static int at803x_config_init(struct phy_device *phydev)
if (ret < 0)
return ret;
+ /* Disable smartspeed */
+ ret = phy_modify(phydev, 0x14, BIT(5), 0);
+ if (ret < 0)
+ return ret;
+
/* The RX and TX delay default is:
* after HW reset: RX delay enabled and TX delay disabled
* after SW reset: RX delay enabled, while TX delay retains the
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 5/7] iommu/arm-smmu: Support DOMAIN_ATTR_SPLIT_TABLES
From: Will Deacon @ 2019-09-17 17:20 UTC (permalink / raw)
To: Jordan Crouse
Cc: linux-arm-msm, Joerg Roedel, linux-kernel, iommu, freedreno,
linux-arm-kernel, Robin Murphy
In-Reply-To: <1566327992-362-6-git-send-email-jcrouse@codeaurora.org>
On Tue, Aug 20, 2019 at 01:06:30PM -0600, Jordan Crouse wrote:
> Support the DOMAIN_ATTR_SPLIT_TABLES attribute to let the leaf driver
> know if split pagetables are enabled for the domain.
>
> Signed-off-by: Jordan Crouse <jcrouse@codeaurora.org>
> ---
>
> drivers/iommu/arm-smmu.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 3f41cf7..6a512ff 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1442,6 +1442,9 @@ static int arm_smmu_domain_get_attr(struct iommu_domain *domain,
> case DOMAIN_ATTR_NESTING:
> *(int *)data = (smmu_domain->stage == ARM_SMMU_DOMAIN_NESTED);
> return 0;
> + case DOMAIN_ATTR_SPLIT_TABLES:
> + *(int *)data = !!(smmu_domain->split_pagetables);
> + return 0;
Hmm. Could you move the setting of this attribute into
arm_smmu_domain_set_attr() and reject it if the ias != 48 in there? That way
the user of the domain can request this feature, rather than us enforcing it
based on the compatible string.
I'd also prefer to call it DOMAIN_ATTR_USE_TTBR1 instead, since it's pretty
ARM specific at this point.
Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: clock: mediatek: add pericfg for MT8183
From: Stephen Boyd @ 2019-09-17 17:21 UTC (permalink / raw)
To: Chunfeng Yun, Rob Herring
Cc: Mark Rutland, devicetree, Ryder Lee, Weiyi Lu, Michael Turquette,
linux-kernel, linux-clk, Chunfeng Yun, Nicolas Boichat,
linux-mediatek, Matthias Brugger, Erin Lo, linux-arm-kernel
In-Reply-To: <1566980533-28282-1-git-send-email-chunfeng.yun@mediatek.com>
Quoting Chunfeng Yun (2019-08-28 01:22:12)
> This patch adds binding of pericfg for MT8183.
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
Applied to clk-next
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 2/2] clk: mediatek: add pericfg clocks for MT8183
From: Stephen Boyd @ 2019-09-17 17:21 UTC (permalink / raw)
To: Chunfeng Yun, Rob Herring
Cc: Mark Rutland, devicetree, Ryder Lee, Weiyi Lu, Michael Turquette,
linux-kernel, linux-clk, Chunfeng Yun, Nicolas Boichat,
linux-mediatek, Matthias Brugger, Erin Lo, linux-arm-kernel
In-Reply-To: <1566980533-28282-2-git-send-email-chunfeng.yun@mediatek.com>
Quoting Chunfeng Yun (2019-08-28 01:22:13)
> Add pericfg clocks for MT8183, it's used when support USB
> remote wakeup
>
> Cc: Weiyi Lu <weiyi.lu@mediatek.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
> ---
Applied to clk-next
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1 1/1] arm64: dts: Fix gpio to pinmux mapping
From: Florian Fainelli @ 2019-09-17 17:22 UTC (permalink / raw)
To: bcm-kernel-feedback-list, Rayagonda Kokatanur, Ray Jui,
Scott Branden, Rob Herring, Mark Rutland, linux-arm-kernel,
devicetree, linux-kernel
In-Reply-To: <1568018127-26730-1-git-send-email-rayagonda.kokatanur@broadcom.com>
On Mon, 9 Sep 2019 14:05:27 +0530, Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com> wrote:
> There are total of 151 non-secure gpio (0-150) and four
> pins of pinmux (91, 92, 93 and 94) are not mapped to any
> gpio pin, hence update same in DT.
>
> Fixes: 8aa428cc1e2e ("arm64: dts: Add pinctrl DT nodes for Stingray SOC")
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
Applied to devicetree-arm64/fixes, thanks!
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RESEND PATCH v1 1/3] clk: mediatek: Register clock gate with device
From: Stephen Boyd @ 2019-09-17 17:23 UTC (permalink / raw)
To: Matthias Brugger, Nicolas Boichat, Rob Herring, Weiyi Lu
Cc: James Liao, Weiyi Lu, srv_heupstream, linux-kernel, Fan Chen,
linux-mediatek, CK Hu, linux-clk, linux-arm-kernel
In-Reply-To: <1567414859-3244-2-git-send-email-weiyi.lu@mediatek.com>
Quoting Weiyi Lu (2019-09-02 02:00:57)
> Allow those clocks under a power domain to do the runtime pm operation
> by forwarding the struct device pointer from clock provider.
>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
Applied to clk-next
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RESEND PATCH v1 2/3] clk: mediatek: Runtime PM support for MT8183 mcucfg clock provider
From: Stephen Boyd @ 2019-09-17 17:23 UTC (permalink / raw)
To: Matthias Brugger, Nicolas Boichat, Rob Herring, Weiyi Lu
Cc: James Liao, Weiyi Lu, srv_heupstream, linux-kernel, Fan Chen,
linux-mediatek, CK Hu, linux-clk, linux-arm-kernel
In-Reply-To: <1567414859-3244-3-git-send-email-weiyi.lu@mediatek.com>
Quoting Weiyi Lu (2019-09-02 02:00:58)
> Enable the runtime PM support and forward the struct device pointer for
> registration of MT8183 mcucfg clocks.
>
> Signed-off-by: Weiyi Lu <weiyi.lu@mediatek.com>
> ---
Applied to clk-next
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s max-speed
From: Andrew Lunn @ 2019-09-17 17:26 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Baruch Siach, Fabio Estevam, Sascha Hauer, tinywrkb, open list,
Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
linux-arm-kernel
In-Reply-To: <20190917171913.GY25745@shell.armlinux.org.uk>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index b3893347804d..85cf4a4a5e81 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
Hi Russell
This won't work. In the kernel logs, you see
kernel: Generic PHY 2188000.ethernet-1:00: attached PHY driver [Generic PHY]
The generic PHY driver is being used, not the at803x driver.
But i do like your idea, it does fit the problem description.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH V6 1/2] dt-bindings: mailbox: add binding doc for the ARM SMC/HVC mailbox
From: Andre Przywara @ 2019-09-17 17:31 UTC (permalink / raw)
To: Peng Fan
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
f.fainelli@gmail.com, jassisinghbrar@gmail.com,
linux-kernel@vger.kernel.org, robh+dt@kernel.org, dl-linux-imx,
sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <1568626884-5189-2-git-send-email-peng.fan@nxp.com>
On Mon, 16 Sep 2019 09:44:37 +0000
Peng Fan <peng.fan@nxp.com> wrote:
Hi,
> From: Peng Fan <peng.fan@nxp.com>
>
> The ARM SMC/HVC mailbox binding describes a firmware interface to trigger
> actions in software layers running in the EL2 or EL3 exception levels.
> The term "ARM" here relates to the SMC instruction as part of the ARM
> instruction set, not as a standard endorsed by ARM Ltd.
>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> .../devicetree/bindings/mailbox/arm-smc.yaml | 96 ++++++++++++++++++++++
> 1 file changed, 96 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mailbox/arm-smc.yaml
>
> diff --git a/Documentation/devicetree/bindings/mailbox/arm-smc.yaml b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> new file mode 100644
> index 000000000000..bf01bec035fc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mailbox/arm-smc.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mailbox/arm-smc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ARM SMC Mailbox Interface
> +
> +maintainers:
> + - Peng Fan <peng.fan@nxp.com>
> +
> +description: |
> + This mailbox uses the ARM smc (secure monitor call) and hvc (hypervisor
I think "or" instead of "and" is less confusing.
> + call) instruction to trigger a mailbox-connected activity in firmware,
> + executing on the very same core as the caller. The value of r0/w0/x0
> + the firmware returns after the smc call is delivered as a received
> + message to the mailbox framework, so synchronous communication can be
> + established. The exact meaning of the action the mailbox triggers as
> + well as the return value is defined by their users and is not subject
> + to this binding.
> +
> + One use case of this mailbox is the SCMI interface, which uses shared
One example use case of this mailbox ...
(to make it more obvious that it's not restricted to this)
> + memory to transfer commands and parameters, and a mailbox to trigger a
> + function call. This allows SoCs without a separate management processor
> + (or when such a processor is not available or used) to use this
> + standardized interface anyway.
> +
> + This binding describes no hardware, but establishes a firmware interface.
> + Upon receiving an SMC using one of the described SMC function identifiers,
... the described SMC function identifier,
> + the firmware is expected to trigger some mailbox connected functionality.
> + The communication follows the ARM SMC calling convention.
> + Firmware expects an SMC function identifier in r0 or w0. The supported
> + identifiers are passed from consumers,
identifier
"passed from consumers": How? Where?
But I want to repeat: We should not allow this. This is a binding for a mailbox controller driver, not a generic firmware backdoor.
We should be as strict as possible to avoid any security issues.
The firmware certainly knows the function ID it implements. The firmware controls the DT. So it is straight-forward to put the ID into the DT. The firmware could even do this at boot time, dynamically, before passing on the DT to the non-secure world (bootloader or kernel).
What would be the use case of this functionality?
> or listed in the the arm,func-ids
arm,func-id
> + properties as described below. The firmware can return one value in
property
> + the first SMC result register, it is expected to be an error value,
> + which shall be propagated to the mailbox client.
> +
> + Any core which supports the SMC or HVC instruction can be used, as long
> + as a firmware component running in EL3 or EL2 is handling these calls.
> +
> +properties:
> + compatible:
> + oneOf:
> + - description:
> + For implementations using ARM SMC instruction.
> + const: arm,smc-mbox
> +
> + - description:
> + For implementations using ARM HVC instruction.
> + const: arm,hvc-mbox
I am not particularly happy with this, but well ...
> +
> + "#mbox-cells":
> + const: 1
Why is this "1"? What is this number used for? It used to be the channel ID, but since you are describing a single channel controller only, this should be 0 now.
> +
> + arm,func-id:
> + description: |
> + An 32-bit value specifying the function ID used by the mailbox.
A single 32-bit value ...
> + The function ID follow the ARM SMC calling convention standard [1].
follows
> + $ref: /schemas/types.yaml#/definitions/uint32
> +
> +required:
> + - compatible
> + - "#mbox-cells"
> +
> +examples:
> + - |
> + sram@93f000 {
> + compatible = "mmio-sram";
> + reg = <0x0 0x93f000 0x0 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x93f000 0x1000>;
> +
> + cpu_scp_lpri: scp-shmem@0 {
> + compatible = "arm,scmi-shmem";
> + reg = <0x0 0x200>;
> + };
> + };
> +
> + smc_tx_mbox: tx_mbox {
> + #mbox-cells = <1>;
As mentioned above, should be 0.
> + compatible = "arm,smc-mbox";
> + /* optional */
First: having "optional" in a specific example is not helpful, just confusing.
Second: It is actually *not* optional in this case, as there is no other way of propagating the function ID. The SCMI driver as the mailbox client has certainly no clue about this.
I think I said this previously: Relying on the mailbox client to pass the function ID sounds broken, as this is a property of the mailbox controller driver. The mailbox client does not care about this mailbox communication detail, it just wants to trigger the mailbox.
> + arm,func-id = <0xc20000fe>;
> + };
> +
> + firmware {
> + scmi {
> + compatible = "arm,scmi";
> + mboxes = <&smc_tx_mbox 0>;
... and here just <&smc_tx_mbox>; would suffice.
> + mbox-names = "tx";
> + shmem = <&cpu_scp_lpri>;
> + };
> + };
> +
> +...
Cheers,
Andre.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s max-speed
From: Russell King - ARM Linux admin @ 2019-09-17 17:37 UTC (permalink / raw)
To: Andrew Lunn
Cc: Mark Rutland,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
Baruch Siach, Fabio Estevam, Sascha Hauer, tinywrkb, open list,
Rob Herring, NXP Linux Team, Pengutronix Kernel Team, Shawn Guo,
linux-arm-kernel
In-Reply-To: <20190917172658.GB9591@lunn.ch>
On Tue, Sep 17, 2019 at 07:26:58PM +0200, Andrew Lunn wrote:
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index b3893347804d..85cf4a4a5e81 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
>
> Hi Russell
>
> This won't work. In the kernel logs, you see
>
> kernel: Generic PHY 2188000.ethernet-1:00: attached PHY driver [Generic PHY]
>
> The generic PHY driver is being used, not the at803x driver.
Well, the _correct_ driver needs to be used for the PHY specific
features to be properly controlled. Using the generic driver
in this situation will not be guaranteed to work.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH V6 2/2] mailbox: introduce ARM SMC based mailbox
From: Andre Przywara @ 2019-09-17 17:38 UTC (permalink / raw)
To: Peng Fan
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
f.fainelli@gmail.com, jassisinghbrar@gmail.com,
linux-kernel@vger.kernel.org, robh+dt@kernel.org, dl-linux-imx,
sudeep.holla@arm.com, linux-arm-kernel@lists.infradead.org
In-Reply-To: <1568626884-5189-3-git-send-email-peng.fan@nxp.com>
On Mon, 16 Sep 2019 09:44:41 +0000
Peng Fan <peng.fan@nxp.com> wrote:
Hi,
looks quite good now, some smaller comments below.
I think the only thing left is the "function ID passed by the client" topic.
Have you tried using this interface using hvc, for instance in KVM or Xen? For instance to provide access to a clock for a passed-through platform device?
Another use case that pops up from time to time is GPIO for guests. This sounds like a use case for using the register interface, also we could use the hvc return value.
> From: Peng Fan <peng.fan@nxp.com>
>
> This mailbox driver implements a mailbox which signals transmitted data
> via an ARM smc (secure monitor call) instruction. The mailbox receiver
> is implemented in firmware and can synchronously return data when it
> returns execution to the non-secure world again.
> An asynchronous receive path is not implemented.
> This allows the usage of a mailbox to trigger firmware actions on SoCs
> which either don't have a separate management processor or on which such
> a core is not available. A user of this mailbox could be the SCP
> interface.
>
> Modified from Andre Przywara's v2 patch
> https://lore.kernel.org/patchwork/patch/812999/
>
> Cc: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> drivers/mailbox/Kconfig | 7 ++
> drivers/mailbox/Makefile | 2 +
> drivers/mailbox/arm-smc-mailbox.c | 167 ++++++++++++++++++++++++++++++++++++++
> 3 files changed, 176 insertions(+)
> create mode 100644 drivers/mailbox/arm-smc-mailbox.c
>
> diff --git a/drivers/mailbox/Kconfig b/drivers/mailbox/Kconfig
> index ab4eb750bbdd..7707ee26251a 100644
> --- a/drivers/mailbox/Kconfig
> +++ b/drivers/mailbox/Kconfig
> @@ -16,6 +16,13 @@ config ARM_MHU
> The controller has 3 mailbox channels, the last of which can be
> used in Secure mode only.
>
> +config ARM_SMC_MBOX
> + tristate "Generic ARM smc mailbox"
> + depends on OF && HAVE_ARM_SMCCC
> + help
> + Generic mailbox driver which uses ARM smc calls to call into
> + firmware for triggering mailboxes.
> +
> config IMX_MBOX
> tristate "i.MX Mailbox"
> depends on ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/mailbox/Makefile b/drivers/mailbox/Makefile
> index c22fad6f696b..93918a84c91b 100644
> --- a/drivers/mailbox/Makefile
> +++ b/drivers/mailbox/Makefile
> @@ -7,6 +7,8 @@ obj-$(CONFIG_MAILBOX_TEST) += mailbox-test.o
>
> obj-$(CONFIG_ARM_MHU) += arm_mhu.o
>
> +obj-$(CONFIG_ARM_SMC_MBOX) += arm-smc-mailbox.o
> +
> obj-$(CONFIG_IMX_MBOX) += imx-mailbox.o
>
> obj-$(CONFIG_ARMADA_37XX_RWTM_MBOX) += armada-37xx-rwtm-mailbox.o
> diff --git a/drivers/mailbox/arm-smc-mailbox.c b/drivers/mailbox/arm-smc-mailbox.c
> new file mode 100644
> index 000000000000..c84aef39c8d9
> --- /dev/null
> +++ b/drivers/mailbox/arm-smc-mailbox.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2016,2017 ARM Ltd.
> + * Copyright 2019 NXP
> + */
> +
> +#include <linux/arm-smccc.h>
> +#include <linux/device.h>
> +#include <linux/kernel.h>
> +#include <linux/interrupt.h>
> +#include <linux/mailbox_controller.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +
> +struct arm_smc_chan_data {
> + unsigned int function_id;
> +};
> +
> +struct arm_smccc_mbox_cmd {
> + unsigned int function_id;
> + union {
> + unsigned int args_smccc32[6];
> + unsigned long args_smccc64[6];
Shouldn't this be u32 and u64 here, as the data type has this explicit length in the SMCCC?
> + };
> +};
If this is the data structure that this mailbox controller uses, I would expect this to be documented somewhere, or even exported.
And again, I don't like the idea of having the function ID in here.
> +
> +typedef unsigned long (smc_mbox_fn)(unsigned int, unsigned long,
> + unsigned long, unsigned long,
> + unsigned long, unsigned long,
> + unsigned long);
> +static smc_mbox_fn *invoke_smc_mbox_fn;
> +
> +static int arm_smc_send_data(struct mbox_chan *link, void *data)
> +{
> + struct arm_smc_chan_data *chan_data = link->con_priv;
> + struct arm_smccc_mbox_cmd *cmd = data;
> + unsigned long ret;
> + u32 function_id;
> +
> + function_id = chan_data->function_id;
> + if (!function_id)
> + function_id = cmd->function_id;
> +
> + if (function_id & BIT(30)) {
if (ARM_SMCCC_IS_64(function_id)) {
> + ret = invoke_smc_mbox_fn(function_id, cmd->args_smccc64[0],
> + cmd->args_smccc64[1],
> + cmd->args_smccc64[2],
> + cmd->args_smccc64[3],
> + cmd->args_smccc64[4],
> + cmd->args_smccc64[5]);
> + } else {
> + ret = invoke_smc_mbox_fn(function_id, cmd->args_smccc32[0],
> + cmd->args_smccc32[1],
> + cmd->args_smccc32[2],
> + cmd->args_smccc32[3],
> + cmd->args_smccc32[4],
> + cmd->args_smccc32[5]);
> + }
> +
> + mbox_chan_received_data(link, (void *)ret);
> +
> + return 0;
> +}
> +
> +static unsigned long __invoke_fn_hvc(unsigned int function_id,
> + unsigned long arg0, unsigned long arg1,
> + unsigned long arg2, unsigned long arg3,
> + unsigned long arg4, unsigned long arg5)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_hvc(function_id, arg0, arg1, arg2, arg3, arg4,
> + arg5, 0, &res);
> + return res.a0;
> +}
> +
> +static unsigned long __invoke_fn_smc(unsigned int function_id,
> + unsigned long arg0, unsigned long arg1,
> + unsigned long arg2, unsigned long arg3,
> + unsigned long arg4, unsigned long arg5)
> +{
> + struct arm_smccc_res res;
> +
> + arm_smccc_smc(function_id, arg0, arg1, arg2, arg3, arg4,
> + arg5, 0, &res);
> + return res.a0;
> +}
> +
> +static const struct mbox_chan_ops arm_smc_mbox_chan_ops = {
> + .send_data = arm_smc_send_data,
> +};
> +
> +static int arm_smc_mbox_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mbox_controller *mbox;
> + struct arm_smc_chan_data *chan_data;
> + int ret;
> + u32 function_id = 0;
> +
> + if (of_device_is_compatible(dev->of_node, "arm,smc-mbox"))
> + invoke_smc_mbox_fn = __invoke_fn_smc;
> + else
> + invoke_smc_mbox_fn = __invoke_fn_hvc;
> +
> + mbox = devm_kzalloc(dev, sizeof(*mbox), GFP_KERNEL);
> + if (!mbox)
> + return -ENOMEM;
> +
> + mbox->num_chans = 1;
> + mbox->chans = devm_kzalloc(dev, sizeof(*mbox->chans), GFP_KERNEL);
> + if (!mbox->chans)
> + return -ENOMEM;
> +
> + chan_data = devm_kzalloc(dev, sizeof(*chan_data), GFP_KERNEL);
> + if (!chan_data)
> + return -ENOMEM;
> +
> + of_property_read_u32(dev->of_node, "arm,func-id", &function_id);
> + chan_data->function_id = function_id;
> +
> + mbox->chans->con_priv = chan_data;
> +
> + mbox->txdone_poll = false;
> + mbox->txdone_irq = false;
Don't we need to provide something to confirm reception to the client? In our case we can set this as soon as the smc/hvc returns.
Cheers,
Andre.
> + mbox->ops = &arm_smc_mbox_chan_ops;
> + mbox->dev = dev;
> +
> + platform_set_drvdata(pdev, mbox);
> +
> + ret = devm_mbox_controller_register(dev, mbox);
> + if (ret)
> + return ret;
> +
> + dev_info(dev, "ARM SMC mailbox enabled.\n");
> +
> + return ret;
> +}
> +
> +static int arm_smc_mbox_remove(struct platform_device *pdev)
> +{
> + struct mbox_controller *mbox = platform_get_drvdata(pdev);
> +
> + mbox_controller_unregister(mbox);
> + return 0;
> +}
> +
> +static const struct of_device_id arm_smc_mbox_of_match[] = {
> + { .compatible = "arm,smc-mbox", },
> + { .compatible = "arm,hvc-mbox", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, arm_smc_mbox_of_match);
> +
> +static struct platform_driver arm_smc_mbox_driver = {
> + .driver = {
> + .name = "arm-smc-mbox",
> + .of_match_table = arm_smc_mbox_of_match,
> + },
> + .probe = arm_smc_mbox_probe,
> + .remove = arm_smc_mbox_remove,
> +};
> +module_platform_driver(arm_smc_mbox_driver);
> +
> +MODULE_AUTHOR("Peng Fan <peng.fan@nxp.com>");
> +MODULE_DESCRIPTION("Generic ARM smc mailbox driver");
> +MODULE_LICENSE("GPL v2");
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ 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