* [PATCH 03/14] arm64: Add per-cpu infrastructure to call ARCH_WORKAROUND_2
From: Julien Grall @ 2018-05-23 10:03 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-4-marc.zyngier@arm.com>
Hi Marc,
On 05/22/2018 04:06 PM, Marc Zyngier wrote:
> In a heterogeneous system, we can end up with both affected and
> unaffected CPUs. Let's check their status before calling into the
> firmware.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
> arch/arm64/kernel/cpu_errata.c | 2 ++
> arch/arm64/kernel/entry.S | 11 +++++++----
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 46b3aafb631a..0288d6cf560e 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -233,6 +233,8 @@ enable_smccc_arch_workaround_1(const struct arm64_cpu_capabilities *entry)
> #endif /* CONFIG_HARDEN_BRANCH_PREDICTOR */
>
> #ifdef CONFIG_ARM64_SSBD
> +DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);
> +
> void __init arm64_update_smccc_conduit(struct alt_instr *alt,
> __le32 *origptr, __le32 *updptr,
> int nr_inst)
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index f33e6aed3037..29ad672a6abd 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -140,8 +140,10 @@ alternative_else_nop_endif
>
> // This macro corrupts x0-x3. It is the caller's duty
> // to save/restore them if required.
> - .macro apply_ssbd, state
> + .macro apply_ssbd, state, targ, tmp1, tmp2
> #ifdef CONFIG_ARM64_SSBD
> + ldr_this_cpu \tmp2, arm64_ssbd_callback_required, \tmp1
> + cbz \tmp2, \targ
> mov w0, #ARM_SMCCC_ARCH_WORKAROUND_2
> mov w1, #\state
> alternative_cb arm64_update_smccc_conduit
> @@ -176,12 +178,13 @@ alternative_cb_end
> ldr x19, [tsk, #TSK_TI_FLAGS] // since we can unmask debug
> disable_step_tsk x19, x20 // exceptions when scheduling.
>
> - apply_ssbd 1
> + apply_ssbd 1, 1f, x22, x23
>
> #ifdef CONFIG_ARM64_SSBD
> ldp x0, x1, [sp, #16 * 0]
> ldp x2, x3, [sp, #16 * 1]
> #endif
> +1:
>
> mov x29, xzr // fp pointed to user-space
> .else
> @@ -323,8 +326,8 @@ alternative_if ARM64_WORKAROUND_845719
> alternative_else_nop_endif
> #endif
> 3:
> - apply_ssbd 0
> -
> + apply_ssbd 0, 5f, x0, x1
> +5:
> .endif
>
> msr elr_el1, x21 // set up the return data
>
--
Julien Grall
^ permalink raw reply
* [PATCH 04/14] arm64: Add ARCH_WORKAROUND_2 probing
From: Julien Grall @ 2018-05-23 10:06 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-5-marc.zyngier@arm.com>
Hi Marc,
On 05/22/2018 04:06 PM, Marc Zyngier wrote:
> As for Spectre variant-2, we rely on SMCCC 1.1 to provide the
> discovery mechanism for detecting the SSBD mitigation.
>
> A new capability is also allocated for that purpose, and a
> config option.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
> ---
> arch/arm64/Kconfig | 9 ++++++
> arch/arm64/include/asm/cpucaps.h | 3 +-
> arch/arm64/kernel/cpu_errata.c | 69 ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index eb2cf4938f6d..b2103b4df467 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -938,6 +938,15 @@ config HARDEN_EL2_VECTORS
>
> If unsure, say Y.
>
> +config ARM64_SSBD
> + bool "Speculative Store Bypass Disable" if EXPERT
> + default y
> + help
> + This enables mitigation of the bypassing of previous stores
> + by speculative loads.
> +
> + If unsure, say Y.
> +
> menuconfig ARMV8_DEPRECATED
> bool "Emulate deprecated/obsolete ARMv8 instructions"
> depends on COMPAT
> diff --git a/arch/arm64/include/asm/cpucaps.h b/arch/arm64/include/asm/cpucaps.h
> index bc51b72fafd4..5b2facf786ba 100644
> --- a/arch/arm64/include/asm/cpucaps.h
> +++ b/arch/arm64/include/asm/cpucaps.h
> @@ -48,7 +48,8 @@
> #define ARM64_HAS_CACHE_IDC 27
> #define ARM64_HAS_CACHE_DIC 28
> #define ARM64_HW_DBM 29
> +#define ARM64_SSBD 30
NIT: Could you indent 30 the same way as the other number?
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheers,
--
Julien Grall
^ permalink raw reply
* [PATCH 05/14] arm64: Add 'ssbd' command-line option
From: Julien Grall @ 2018-05-23 10:08 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-6-marc.zyngier@arm.com>
Hi Marc,
On 05/22/2018 04:06 PM, Marc Zyngier wrote:
> On a system where the firmware implements ARCH_WORKAROUND_2,
> it may be useful to either permanently enable or disable the
> workaround for cases where the user decides that they'd rather
> not get a trap overhead, and keep the mitigation permanently
> on or off instead of switching it on exception entry/exit.
>
> In any case, default to the mitigation being enabled.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
> Documentation/admin-guide/kernel-parameters.txt | 17 ++++
> arch/arm64/include/asm/cpufeature.h | 6 ++
> arch/arm64/kernel/cpu_errata.c | 102 ++++++++++++++++++++----
> 3 files changed, 109 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index f2040d46f095..646e112c6f63 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4092,6 +4092,23 @@
> expediting. Set to zero to disable automatic
> expediting.
>
> + ssbd= [ARM64,HW]
> + Speculative Store Bypass Disable control
> +
> + On CPUs that are vulnerable to the Speculative
> + Store Bypass vulnerability and offer a
> + firmware based mitigation, this parameter
> + indicates how the mitigation should be used:
> +
> + force-on: Unconditionnaly enable mitigation for
> + for both kernel and userspace
> + force-off: Unconditionnaly disable mitigation for
> + for both kernel and userspace
> + kernel: Always enable mitigation in the
> + kernel, and offer a prctl interface
> + to allow userspace to register its
> + interest in being mitigated too.
> +
> stack_guard_gap= [MM]
> override the default stack gap protection. The value
> is in page units and it defines how many pages prior
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 09b0f2a80c8f..9bc548e22784 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -537,6 +537,12 @@ static inline u64 read_zcr_features(void)
> return zcr;
> }
>
> +#define ARM64_SSBD_UNKNOWN -1
> +#define ARM64_SSBD_FORCE_DISABLE 0
> +#define ARM64_SSBD_EL1_ENTRY 1
> +#define ARM64_SSBD_FORCE_ENABLE 2
> +#define ARM64_SSBD_MITIGATED 3
> +
> #endif /* __ASSEMBLY__ */
>
> #endif
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 7fd6d5b001f5..f1d4e75b0ddd 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -235,6 +235,38 @@ enable_smccc_arch_workaround_1(const struct arm64_cpu_capabilities *entry)
> #ifdef CONFIG_ARM64_SSBD
> DEFINE_PER_CPU_READ_MOSTLY(u64, arm64_ssbd_callback_required);
>
> +int ssbd_state __read_mostly = ARM64_SSBD_EL1_ENTRY;
> +
> +static const struct ssbd_options {
> + const char *str;
> + int state;
> +} ssbd_options[] = {
> + { "force-on", ARM64_SSBD_FORCE_ENABLE, },
> + { "force-off", ARM64_SSBD_FORCE_DISABLE, },
> + { "kernel", ARM64_SSBD_EL1_ENTRY, },
> +};
> +
> +static int __init ssbd_cfg(char *buf)
> +{
> + int i;
> +
> + if (!buf || !buf[0])
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(ssbd_options); i++) {
> + int len = strlen(ssbd_options[i].str);
> +
> + if (strncmp(buf, ssbd_options[i].str, len))
> + continue;
> +
> + ssbd_state = ssbd_options[i].state;
> + return 0;
> + }
> +
> + return -EINVAL;
> +}
> +early_param("ssbd", ssbd_cfg);
> +
> void __init arm64_update_smccc_conduit(struct alt_instr *alt,
> __le32 *origptr, __le32 *updptr,
> int nr_inst)
> @@ -272,44 +304,82 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,
> int scope)
> {
> struct arm_smccc_res res;
> - bool supported = true;
> + bool required = true;
> + s32 val;
>
> WARN_ON(scope != SCOPE_LOCAL_CPU || preemptible());
>
> - if (psci_ops.smccc_version == SMCCC_VERSION_1_0)
> + if (psci_ops.smccc_version == SMCCC_VERSION_1_0) {
> + ssbd_state = ARM64_SSBD_UNKNOWN;
> return false;
> + }
>
> - /*
> - * The probe function return value is either negative
> - * (unsupported or mitigated), positive (unaffected), or zero
> - * (requires mitigation). We only need to do anything in the
> - * last case.
> - */
> switch (psci_ops.conduit) {
> case PSCI_CONDUIT_HVC:
> arm_smccc_1_1_hvc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> ARM_SMCCC_ARCH_WORKAROUND_2, &res);
> - if ((int)res.a0 != 0)
> - supported = false;
> break;
>
> case PSCI_CONDUIT_SMC:
> arm_smccc_1_1_smc(ARM_SMCCC_ARCH_FEATURES_FUNC_ID,
> ARM_SMCCC_ARCH_WORKAROUND_2, &res);
> - if ((int)res.a0 != 0)
> - supported = false;
> break;
>
> default:
> - supported = false;
> + ssbd_state = ARM64_SSBD_UNKNOWN;
> + return false;
> }
>
> - if (supported) {
> - __this_cpu_write(arm64_ssbd_callback_required, 1);
> + val = (s32)res.a0;
> +
> + switch (val) {
> + case SMCCC_RET_NOT_SUPPORTED:
> + ssbd_state = ARM64_SSBD_UNKNOWN;
> + return false;
> +
> + case SMCCC_RET_NOT_REQUIRED:
> + ssbd_state = ARM64_SSBD_MITIGATED;
> + return false;
> +
> + case SMCCC_RET_SUCCESS:
> + required = true;
> + break;
> +
> + case 1: /* Mitigation not required on this CPU */
> + required = false;
> + break;
> +
> + default:
> + WARN_ON(1);
> + return false;
> + }
> +
> + switch (ssbd_state) {
> + case ARM64_SSBD_FORCE_DISABLE:
> + pr_info_once("%s disabled from command-line\n", entry->desc);
> + do_ssbd(false);
> + required = false;
> + break;
> +
> + case ARM64_SSBD_EL1_ENTRY:
> + if (required) {
> + __this_cpu_write(arm64_ssbd_callback_required, 1);
> + do_ssbd(true);
> + }
> + break;
> +
> + case ARM64_SSBD_FORCE_ENABLE:
> + pr_info_once("%s forced from command-line\n", entry->desc);
> do_ssbd(true);
> + required = true;
> + break;
> +
> + default:
> + WARN_ON(1);
> + break;
> }
>
> - return supported;
> + return required;
> }
> #endif /* CONFIG_ARM64_SSBD */
>
>
--
Julien Grall
^ permalink raw reply
* [PATCH 06/14] arm64: ssbd: Add global mitigation state accessor
From: Julien Grall @ 2018-05-23 10:11 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-7-marc.zyngier@arm.com>
Hi Marc,
On 05/22/2018 04:06 PM, Marc Zyngier wrote:
> We're about to need the mitigation state in various parts of the
> kernel in order to do the right thing for userspace and guests.
>
> Let's expose an accessor that will let other subsystems know
> about the state.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
> arch/arm64/include/asm/cpufeature.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 9bc548e22784..1bacdf57f0af 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -543,6 +543,16 @@ static inline u64 read_zcr_features(void)
> #define ARM64_SSBD_FORCE_ENABLE 2
> #define ARM64_SSBD_MITIGATED 3
>
> +static inline int arm64_get_ssbd_state(void)
> +{
> +#ifdef CONFIG_ARM64_SSBD
> + extern int ssbd_state;
> + return ssbd_state;
> +#else
> + return ARM64_SSBD_UNKNOWN;
> +#endif
> +}
> +
> #endif /* __ASSEMBLY__ */
>
> #endif
>
--
Julien Grall
^ permalink raw reply
* [PATCH 07/14] arm64: ssbd: Skip apply_ssbd if not using dynamic mitigation
From: Julien Grall @ 2018-05-23 10:13 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-8-marc.zyngier@arm.com>
Hi Marc,
On 05/22/2018 04:06 PM, Marc Zyngier wrote:
> In order to avoid checking arm64_ssbd_callback_required on each
> kernel entry/exit even if no mitigation is required, let's
> add yet another alternative that by default jumps over the mitigation,
> and that gets nop'ed out if we're doing dynamic mitigation.
>
> Think of it as a poor man's static key...
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
> arch/arm64/kernel/cpu_errata.c | 14 ++++++++++++++
> arch/arm64/kernel/entry.S | 3 +++
> 2 files changed, 17 insertions(+)
>
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index f1d4e75b0ddd..8f686f39b9c1 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -283,6 +283,20 @@ void __init arm64_update_smccc_conduit(struct alt_instr *alt,
> *updptr = cpu_to_le32(insn);
> }
>
> +void __init arm64_enable_wa2_handling(struct alt_instr *alt,
> + __le32 *origptr, __le32 *updptr,
> + int nr_inst)
> +{
> + BUG_ON(nr_inst != 1);
> + /*
> + * Only allow mitigation on EL1 entry/exit and guest
> + * ARCH_WORKAROUND_2 handling if the SSBD state allows it to
> + * be flipped.
> + */
> + if (arm64_get_ssbd_state() == ARM64_SSBD_EL1_ENTRY)
> + *updptr = cpu_to_le32(aarch64_insn_gen_nop());
> +}
> +
> static void do_ssbd(bool state)
> {
> switch (psci_ops.conduit) {
> diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> index 29ad672a6abd..e6f6e2339b22 100644
> --- a/arch/arm64/kernel/entry.S
> +++ b/arch/arm64/kernel/entry.S
> @@ -142,6 +142,9 @@ alternative_else_nop_endif
> // to save/restore them if required.
> .macro apply_ssbd, state, targ, tmp1, tmp2
> #ifdef CONFIG_ARM64_SSBD
> +alternative_cb arm64_enable_wa2_handling
> + b \targ
> +alternative_cb_end
> ldr_this_cpu \tmp2, arm64_ssbd_callback_required, \tmp1
> cbz \tmp2, \targ
> mov w0, #ARM_SMCCC_ARCH_WORKAROUND_2
>
--
Julien Grall
^ permalink raw reply
* [PATCH] gpio: zynq: Setup chip->base based on alias ID
From: Michal Simek @ 2018-05-23 10:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdbCsUBMcHg+xGOQCBmhoJsqh9VjK+4pjhG5Hf6Fjzwjww@mail.gmail.com>
>> If you take a look at
>> arch/arm64/boot/dts/xilinx/zynqmp-zcu100-revC.dts
>> which is Ultra96 board gpio-line-names are filled there for the whole PS
>> part. Definitely take a look and let know if you find out any issue there.
>
> It looks good. I even managed to boot my Ultra96 board which
> was sent over as price in some competition (great hardware!)
Wonderful that you get hw available to be closer to issues.
Maybe it would be worth to give you some guidance what to do with PL and
how to add more stuff there especially in connection to GPIO.
We have pinctrl driver in vendor tree but my colleague needs to upstream
firmware interface first.
>> zynq/zynqmp gpio controller contains PS pins (hard part) and PL pins
>> coming to logic.
>>
>> I can't describe PL gpio pins because it can be whatever even I have
>> done that for one fixed hw design.
>>
>> Interesting part on that sha1 you shared is how "NC" pin is described.
>>
>> gpio pin 35 I have on zcu100 as "" but it should be maybe TP_PAD which
>> is really just a pad on real board. And the rest of "" gpio names are
>> connected to PL.
>
> How to handle anything routed to/from programmable logic is
> in a bit of mess right now, I understand this work isn't the
> easiest :/
I am happy to discuss this to find out a way how this can be handled.
There are several things together.
On arm64 how to disable/enable access for NS software.
Maybe also separate pins PS pins from PL pins. Right now all are
together and not sure if this is ideal in sense of DT overal for
example. Separate nodes would enable better flexibility. Also if you
look at partial reconfiguration you can just route just part of pins
which suggest list of gpios used there.
Thanks,
Michal
^ permalink raw reply
* [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd
From: Jon Hunter @ 2018-05-23 10:22 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CAPDyKFoHwujfL4vpB8kq+rxDP4cThW4xB4nwb8Qc6_bZwTPbzA@mail.gmail.com>
On 23/05/18 10:47, Ulf Hansson wrote:
> On 23 May 2018 at 11:45, Jon Hunter <jonathanh@nvidia.com> wrote:
>>
>> On 23/05/18 10:33, Ulf Hansson wrote:
>>>
>>> On 23 May 2018 at 11:27, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>>>
>>>>
>>>>
>>>> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>>>>
>>>>>
>>>>> On 23/05/18 07:12, Ulf Hansson wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>>>>>> Thanks for sending this. Believe it or not this has still been on
>>>>>>>>>> my to-do list
>>>>>>>>>> and so we definitely need a solution for Tegra.
>>>>>>>>>>
>>>>>>>>>> Looking at the above it appears that additional power-domains
>>>>>>>>>> exposed as devices
>>>>>>>>>> to the client device. So I assume that this means that the drivers
>>>>>>>>>> for devices
>>>>>>>>>> with multiple power-domains will need to call RPM APIs for each of
>>>>>>>>>> these
>>>>>>>>>> additional power-domains. Is that correct?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> They can, but should not!
>>>>>>>>>
>>>>>>>>> Instead, the driver shall use device_link_add() and
>>>>>>>>> device_link_del(),
>>>>>>>>> dynamically, depending on what PM domain that their original device
>>>>>>>>> needs for the current running use case.
>>>>>>>>>
>>>>>>>>> In that way, they keep existing runtime PM deployment, operating on
>>>>>>>>> its original device.
>>>>>>>>
>>>>>>>>
>>>>>>>> OK, sounds good. Any reason why the linking cannot be handled by the
>>>>>>>> above API? Is there a use-case where you would not want it linked?
>>>>>>>
>>>>>>>
>>>>>>> I am guessing the linking is what would give the driver the ability to
>>>>>>> decide which subset of powerdomains it actually wants to control
>>>>>>> at any point using runtime PM. If we have cases wherein the driver
>>>>>>> would want to turn on/off _all_ its associated powerdomains _always_
>>>>>>> then a default linking of all would help.
>>>>>>
>>>>>>
>>>>>> First, I think we need to decide on *where* the linking should be
>>>>>> done, not at both places, as that would just mess up synchronization
>>>>>> of who is responsible for calling the device_link_del() at detach.
>>>>>>
>>>>>> Second, It would in principle be fine to call device_link_add() and
>>>>>> device_link_del() as a part of the attach/detach APIs. However, there
>>>>>> is a downside to such solution, which would be that the driver then
>>>>>> needs call the detach API, just to do device_link_del(). Of course
>>>>>> then it would also needs to call the attach API later if/when needed.
>>>>>> Doing this adds unnecessary overhead - comparing to just let the
>>>>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>>>>> would put less burden on the drivers as it then only needs to care
>>>>>> about using one set of functions.
>>>>>>
>>>>>> Which solution do you prefer?
>>>>>
>>>>>
>>>>> Any reason why we could not add a 'boolean' argument to the API to
>>>>> indicate whether the new device should be linked? I think that I prefer the
>>>>> API handles it, but I can see there could be instances where drivers may
>>>>> wish to handle it themselves.
>>>>>
>>>>> Rajendra, do you have a use-case right now where the driver would want
>>>>> to handle the linking?
>>>>
>>>>
>>>> So if I understand this right, any driver which does want to control
>>>> individual powerdomain state would
>>>> need to do the linking itself right?
>>>>
>>>> What I am saying is, if I have device A, with powerdomains X and Y, and
>>>> if I want to turn on only X,
>>>> then I would want only X to be linked with A, and at a later point if I
>>>> want both X and Y to be turned on,
>>>> I would then go ahead and link both X and Y to A? Is that correct or did
>>>> I get it all wrong?
>>>
>>>
>>> Correct!
>>>
>>>>
>>>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe
>>>> powerdoamins, which can be
>>>> turned on one at a time (depending on what resolution needs to be
>>>> supported) or both together if we
>>>> really need very high resolution using both vfe modules.
>>>
>>>
>>> I think this is also the case for the Tegra XUSB subsystem.
>>>
>>> The usb device is always attached to one PM domain, but depending on
>>> if super-speed mode is used, another PM domain for that logic needs to
>>> be powered on as well.
>>>
>>> Jon, please correct me if I am wrong!
>>
>>
>> Yes this is technically correct, however, in reality I think we are always
>> going to enable the superspeed domain if either the host or device domain is
>> enabled. So we would probably always link the superspeed with the host and
>> device devices.
>
> Why? Wouldn't that waste power if the superspeed mode isn't used?
Simply to reduce complexity.
Jon
--
nvpublic
^ permalink raw reply
* [PATCH v1] dma: imx-sdma: add virt-dma support
From: Robin Gong @ 2018-05-23 10:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522100948.6xsq3unhyyyd7zz3@pengutronix.de>
On ?, 2018-05-22 at 12:09 +0200, Sascha Hauer wrote:
> Hi Robin,
>
> Several comments inside.
>
> Sascha
>
> On Fri, Mar 23, 2018 at 12:18:19AM +0800, Robin Gong wrote:
> >
> > The legacy sdma driver has below limitations or drawbacks:
> > ? 1. Hardcode the max BDs number as "PAGE_SIZE / sizeof(*)", and
> > alloc
> > ?????one page size for one channel regardless of only few BDs
> > needed
> > ?????most time. But in few cases, the max PAGE_SIZE maybe not
> > enough.
> > ? 2. One SDMA channel can't stop immediatley once channel disabled
> > which
> > ?????means SDMA interrupt may come in after this channel
> > terminated.There
> > ?????are some patches for this corner case such as commit
> > "2746e2c389f9",
> > ?????but not cover non-cyclic.
> >
> > The common virt-dma overcomes the above limitations. It can alloc
> > bd
> > dynamically and free bd once this tx transfer done. No memory
> > wasted or
> > maximum limititation here, only depends on how many memory can be
> > requested
> > from kernel. For No.2, such issue can be workaround by checking if
> > there
> > is available descript("sdmac->desc") now once the unwanted
> > interrupt
> > coming. At last the common virt-dma is easier for sdma driver
> > maintain.
> >
> > Signed-off-by: Robin Gong <yibin.gong@nxp.com>
> > ---
> > ?drivers/dma/Kconfig????|???1 +
> > ?drivers/dma/imx-sdma.c | 395 +++++++++++++++++++++++++++++++----
> > --------------
> > ?2 files changed, 253 insertions(+), 143 deletions(-)
> >
> > diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
> > index 27df3e2..c4ce43c 100644
> > --- a/drivers/dma/Kconfig
> > +++ b/drivers/dma/Kconfig
> > @@ -247,6 +247,7 @@ config IMX_SDMA
> > ? tristate "i.MX SDMA support"
> > ? depends on ARCH_MXC
> > ? select DMA_ENGINE
> > + select DMA_VIRTUAL_CHANNELS
> > ? help
> > ? ??Support the i.MX SDMA engine. This engine is integrated
> > into
> > ? ??Freescale i.MX25/31/35/51/53/6 chips.
> > diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
> > index ccd03c3..df79e73 100644
> > --- a/drivers/dma/imx-sdma.c
> > +++ b/drivers/dma/imx-sdma.c
> > @@ -48,6 +48,7 @@
> > ?#include <linux/mfd/syscon/imx6q-iomuxc-gpr.h>
> > ?
> > ?#include "dmaengine.h"
> > +#include "virt-dma.h"
> > ?
> > ?/* SDMA registers */
> > ?#define SDMA_H_C0PTR 0x000
> > @@ -291,10 +292,19 @@ struct sdma_context_data {
> > ? u32??scratch7;
> > ?} __attribute__ ((packed));
> > ?
> > -#define NUM_BD (int)(PAGE_SIZE / sizeof(struct
> > sdma_buffer_descriptor))
> > -
> > ?struct sdma_engine;
> > ?
> > +struct sdma_desc {
> > + struct virt_dma_desc vd;
> > + struct list_head node;
> > + unsigned int num_bd;
> > + dma_addr_t bd_phys;
> > + unsigned int buf_tail;
> > + unsigned int buf_ptail;
> > + struct sdma_channel *sdmac;
> > + struct sdma_buffer_descriptor *bd;
> > +};
> > +
> > ?/**
> > ? * struct sdma_channel - housekeeping for a SDMA channel
> > ? *
> > @@ -310,19 +320,17 @@ struct sdma_engine;
> > ? * @num_bd max NUM_BD. number of descriptors
> > currently handling
> > ? */
> > ?struct sdma_channel {
> > + struct virt_dma_chan vc;
> > + struct list_head pending;
> > ? struct sdma_engine *sdma;
> > + struct sdma_desc *desc;
> > ? unsigned int channel;
> > ? enum dma_transfer_direction direction;
> > ? enum sdma_peripheral_type peripheral_type;
> > ? unsigned int event_id0;
> > ? unsigned int event_id1;
> > ? enum dma_slave_buswidth word_size;
> > - unsigned int buf_tail;
> > - unsigned int buf_ptail;
> > - unsigned int num_bd;
> > ? unsigned int period_len;
> > - struct sdma_buffer_descriptor *bd;
> > - dma_addr_t bd_phys;
> > ? unsigned int pc_from_device,
> > pc_to_device;
> > ? unsigned int device_to_device;
> > ? unsigned long flags;
> > @@ -330,15 +338,12 @@ struct sdma_channel {
> > ? unsigned long event_mask[2];
> > ? unsigned long watermark_level;
> > ? u32 shp_addr, per_addr;
> > - struct dma_chan chan;
> > - spinlock_t lock;
> > - struct dma_async_tx_descriptor desc;
> > ? enum dma_status status;
> > ? unsigned int chn_count;
> > ? unsigned int chn_real_count;
> > - struct tasklet_struct tasklet;
> > ? struct imx_dma_data data;
> > ? bool enabled;
> Usage of this variable is removed in this patch, but not the variable
> itself.
Yes, will remove the usless 'enabled' in v2.
>
> >
> > + u32 bd_size_sum;
> This variable is never used for anything.
Yes, it's not for significative use but debug to see how many current
bds used.
>
> >
> > ?};
> > ?
> > ?#define IMX_DMA_SG_LOOP BIT(0)
> > @@ -398,6 +403,9 @@ struct sdma_engine {
> > ? u32 spba_start_addr;
> > ? u32 spba_end_addr;
> > ? unsigned int irq;
> > + /* channel0 bd */
> > + dma_addr_t bd0_phys;
> > + struct sdma_buffer_descriptor *bd0;
> > ?};
> > ?
> > ?static struct sdma_driver_data sdma_imx31 = {
> > @@ -553,6 +561,8 @@ MODULE_DEVICE_TABLE(of, sdma_dt_ids);
> > ?#define SDMA_H_CONFIG_ACR BIT(4)??/* indicates if AHB freq
> > /core freq = 2 or 1 */
> > ?#define SDMA_H_CONFIG_CSM (3)???????/* indicates which
> > context switch mode is selected*/
> > ?
> > +static void sdma_start_desc(struct sdma_channel *sdmac);
> > +
> > ?static inline u32 chnenbl_ofs(struct sdma_engine *sdma, unsigned
> > int event)
> > ?{
> > ? u32 chnenbl0 = sdma->drvdata->chnenbl0;
> > @@ -597,14 +607,7 @@ static int sdma_config_ownership(struct
> > sdma_channel *sdmac,
> > ?
> > ?static void sdma_enable_channel(struct sdma_engine *sdma, int
> > channel)
> > ?{
> > - unsigned long flags;
> > - struct sdma_channel *sdmac = &sdma->channel[channel];
> > -
> > ? writel(BIT(channel), sdma->regs + SDMA_H_START);
> > -
> > - spin_lock_irqsave(&sdmac->lock, flags);
> > - sdmac->enabled = true;
> > - spin_unlock_irqrestore(&sdmac->lock, flags);
> > ?}
> > ?
> > ?/*
> > @@ -632,7 +635,7 @@ static int sdma_run_channel0(struct sdma_engine
> > *sdma)
> > ?static int sdma_load_script(struct sdma_engine *sdma, void *buf,
> > int size,
> > ? u32 address)
> > ?{
> > - struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd;
> > + struct sdma_buffer_descriptor *bd0 = sdma->bd0;
> This change seems to be an orthogonal change. Please make this a
> separate patch.
It's something related with virtual dma support, because in virtual
dma framework, all bds should be allocated dynamically if they used.
but bd0 is a specail case since it's must and basic for load sdma
firmware and context for other channels. So here alloc 'bd0' for other
channels.
>
> >
> > ? void *buf_virt;
> > ? dma_addr_t buf_phys;
> > ? int ret;
> > @@ -691,23 +694,16 @@ static void sdma_event_disable(struct
> > sdma_channel *sdmac, unsigned int event)
> > ?static void sdma_update_channel_loop(struct sdma_channel *sdmac)
> > ?{
> > ? struct sdma_buffer_descriptor *bd;
> > + struct sdma_desc *desc = sdmac->desc;
> > ? int error = 0;
> > ? enum dma_status old_status = sdmac->status;
> > - unsigned long flags;
> > -
> > - spin_lock_irqsave(&sdmac->lock, flags);
> > - if (!sdmac->enabled) {
> > - spin_unlock_irqrestore(&sdmac->lock, flags);
> > - return;
> > - }
> > - spin_unlock_irqrestore(&sdmac->lock, flags);
> > ?
> > ? /*
> > ? ?* loop mode. Iterate over descriptors, re-setup them and
> > ? ?* call callback function.
> > ? ?*/
> > - while (1) {
> > - bd = &sdmac->bd[sdmac->buf_tail];
> > + while (desc) {
> 'desc' seems to be used as a loop counter here, but this variable is
> never assigned another value, so I assume it's just another way to
> say
> "skip the loop if desc is NULL". When 'desc' NULL you won't get into
> this function at all though, so this check for desc seems rather
> pointless.
Good catch, should check 'sdmac->desc' here instead of 'desc' since in
the following 'sdmac->desc' may be set to NULL by sdma_terminate_all
during folllowing spin_unlock and spin_lock narrow window. Will improve
it in V2.
>
> >
> > + bd = &desc->bd[desc->buf_tail];
> > ?
> > ? if (bd->mode.status & BD_DONE)
> > ? break;
> > @@ -726,8 +722,8 @@ static void sdma_update_channel_loop(struct
> > sdma_channel *sdmac)
> > ? sdmac->chn_real_count = bd->mode.count;
> > ? bd->mode.status |= BD_DONE;
> > ? bd->mode.count = sdmac->period_len;
> > - sdmac->buf_ptail = sdmac->buf_tail;
> > - sdmac->buf_tail = (sdmac->buf_tail + 1) % sdmac-
> > >num_bd;
> > + desc->buf_ptail = desc->buf_tail;
> > + desc->buf_tail = (desc->buf_tail + 1) % desc-
> > >num_bd;
> > ?
> > ? /*
> > ? ?* The callback is called from the interrupt
> > context in order
> > @@ -735,15 +731,16 @@ static void sdma_update_channel_loop(struct
> > sdma_channel *sdmac)
> > ? ?* SDMA transaction status by the time the client
> > tasklet is
> > ? ?* executed.
> > ? ?*/
> > -
> > - dmaengine_desc_get_callback_invoke(&sdmac->desc,
> > NULL);
> > + spin_unlock(&sdmac->vc.lock);
> > + dmaengine_desc_get_callback_invoke(&desc->vd.tx,
> > NULL);
> > + spin_lock(&sdmac->vc.lock);
> > ?
> > ? if (error)
> > ? sdmac->status = old_status;
> > ? }
> > ?}
> > ?
> > -static void mxc_sdma_handle_channel_normal(unsigned long data)
> > +static void mxc_sdma_handle_channel_normal(struct sdma_channel
> > *data)
> > ?{
> > ? struct sdma_channel *sdmac = (struct sdma_channel *) data;
> > ? struct sdma_buffer_descriptor *bd;
> > @@ -754,8 +751,8 @@ static void
> > mxc_sdma_handle_channel_normal(unsigned long data)
> > ? ?* non loop mode. Iterate over all descriptors, collect
> > ? ?* errors and call callback function
> > ? ?*/
> > - for (i = 0; i < sdmac->num_bd; i++) {
> > - bd = &sdmac->bd[i];
> > + for (i = 0; i < sdmac->desc->num_bd; i++) {
> > + bd = &sdmac->desc->bd[i];
> > ?
> > ? ?if (bd->mode.status & (BD_DONE | BD_RROR))
> > ? error = -EIO;
> > @@ -766,10 +763,6 @@ static void
> > mxc_sdma_handle_channel_normal(unsigned long data)
> > ? sdmac->status = DMA_ERROR;
> > ? else
> > ? sdmac->status = DMA_COMPLETE;
> > -
> > - dma_cookie_complete(&sdmac->desc);
> > -
> > - dmaengine_desc_get_callback_invoke(&sdmac->desc, NULL);
> > ?}
> > ?
> > ?static irqreturn_t sdma_int_handler(int irq, void *dev_id)
> > @@ -785,13 +778,24 @@ static irqreturn_t sdma_int_handler(int irq,
> > void *dev_id)
> > ? while (stat) {
> > ? int channel = fls(stat) - 1;
> > ? struct sdma_channel *sdmac = &sdma-
> > >channel[channel];
> > -
> > - if (sdmac->flags & IMX_DMA_SG_LOOP)
> > - sdma_update_channel_loop(sdmac);
> > - else
> > - tasklet_schedule(&sdmac->tasklet);
> > + struct sdma_desc *desc;
> > +
> > + spin_lock(&sdmac->vc.lock);
> > + desc = sdmac->desc;
> > + if (desc) {
> > + if (sdmac->flags & IMX_DMA_SG_LOOP) {
> > + sdma_update_channel_loop(sdmac);
> > + } else {
> > + mxc_sdma_handle_channel_normal(sdm
> > ac);
> > + vchan_cookie_complete(&desc->vd);
> > + if (!list_empty(&sdmac->pending))
> > + list_del(&desc->node);
> What does this list_empty check protect you from? It looks like when
> the
> list really is empty then it's a bug in your internal driver logic.
Yes, no need here check local sdmac->pending since I directly start
setup next desc flowing in isr instead of local tasklet and virt_dma
framework will handle all lists such as desc_issued/desc_completed etc.
Will remove sdmac->pending in V2.
>
> >
> > + ?sdma_start_desc(sdmac);
> Whitespace damage here.
Will fix in V2.
>
> >
> > + }
> > + }
> > ?
> > ? __clear_bit(channel, &stat);
> > + spin_unlock(&sdmac->vc.lock);
> > ? }
> > ?
> > ? return IRQ_HANDLED;
> > @@ -897,7 +901,7 @@ static int sdma_load_context(struct
> > sdma_channel *sdmac)
> > ? int channel = sdmac->channel;
> > ? int load_address;
> > ? struct sdma_context_data *context = sdma->context;
> > - struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd;
> > + struct sdma_buffer_descriptor *bd0 = sdma->bd0;
> > ? int ret;
> > ? unsigned long flags;
> > ?
> > @@ -946,7 +950,7 @@ static int sdma_load_context(struct
> > sdma_channel *sdmac)
> > ?
> > ?static struct sdma_channel *to_sdma_chan(struct dma_chan *chan)
> > ?{
> > - return container_of(chan, struct sdma_channel, chan);
> > + return container_of(chan, struct sdma_channel, vc.chan);
> > ?}
> > ?
> > ?static int sdma_disable_channel(struct dma_chan *chan)
> > @@ -954,15 +958,10 @@ static int sdma_disable_channel(struct
> > dma_chan *chan)
> > ? struct sdma_channel *sdmac = to_sdma_chan(chan);
> > ? struct sdma_engine *sdma = sdmac->sdma;
> > ? int channel = sdmac->channel;
> > - unsigned long flags;
> > ?
> > ? writel_relaxed(BIT(channel), sdma->regs +
> > SDMA_H_STATSTOP);
> > ? sdmac->status = DMA_ERROR;
> > ?
> > - spin_lock_irqsave(&sdmac->lock, flags);
> > - sdmac->enabled = false;
> > - spin_unlock_irqrestore(&sdmac->lock, flags);
> > -
> > ? return 0;
> > ?}
> > ?
> > @@ -1097,42 +1096,101 @@ static int
> > sdma_set_channel_priority(struct sdma_channel *sdmac,
> > ? return 0;
> > ?}
> > ?
> > -static int sdma_request_channel(struct sdma_channel *sdmac)
> > +static int sdma_alloc_bd(struct sdma_desc *desc)
> > ?{
> > - struct sdma_engine *sdma = sdmac->sdma;
> > - int channel = sdmac->channel;
> > - int ret = -EBUSY;
> > + u32 bd_size = desc->num_bd * sizeof(struct
> > sdma_buffer_descriptor);
> > + int ret = 0;
> > + unsigned long flags;
> > ?
> > - sdmac->bd = dma_zalloc_coherent(NULL, PAGE_SIZE, &sdmac-
> > >bd_phys,
> > + desc->bd = dma_zalloc_coherent(NULL, bd_size, &desc-
> > >bd_phys,
> > ? GFP_KERNEL);
> > - if (!sdmac->bd) {
> > + if (!desc->bd) {
> > ? ret = -ENOMEM;
> > ? goto out;
> > ? }
> > ?
> > - sdma->channel_control[channel].base_bd_ptr = sdmac-
> > >bd_phys;
> > - sdma->channel_control[channel].current_bd_ptr = sdmac-
> > >bd_phys;
> > + spin_lock_irqsave(&desc->sdmac->vc.lock, flags);
> > + desc->sdmac->bd_size_sum += bd_size;
> > + spin_unlock_irqrestore(&desc->sdmac->vc.lock, flags);
> > ?
> > - sdma_set_channel_priority(sdmac,
> > MXC_SDMA_DEFAULT_PRIORITY);
> > - return 0;
> > ?out:
> > -
> > ? return ret;
> > ?}
> > ?
> > -static dma_cookie_t sdma_tx_submit(struct dma_async_tx_descriptor
> > *tx)
> > +static void sdma_free_bd(struct sdma_desc *desc)
> > ?{
> > + u32 bd_size = desc->num_bd * sizeof(struct
> > sdma_buffer_descriptor);
> > ? unsigned long flags;
> > - struct sdma_channel *sdmac = to_sdma_chan(tx->chan);
> > - dma_cookie_t cookie;
> > ?
> > - spin_lock_irqsave(&sdmac->lock, flags);
> > + if (desc->bd) {
> > + dma_free_coherent(NULL, bd_size, desc->bd, desc-
> > >bd_phys);
> > +
> > + spin_lock_irqsave(&desc->sdmac->vc.lock, flags);
> > + desc->sdmac->bd_size_sum -= bd_size;
> > + spin_unlock_irqrestore(&desc->sdmac->vc.lock,
> > flags);
> > + }
> > +}
> > +
> > +static int sdma_request_channel0(struct sdma_engine *sdma)
> > +{
> > + int ret = 0;
> > +
> > + sdma->bd0 = dma_zalloc_coherent(NULL, PAGE_SIZE, &sdma-
> > >bd0_phys,
> > + GFP_KERNEL);
> > + if (!sdma->bd0) {
> > + ret = -ENOMEM;
> > + goto out;
> > + }
> > ?
> > - cookie = dma_cookie_assign(tx);
> > + sdma->channel_control[0].base_bd_ptr = sdma->bd0_phys;
> > + sdma->channel_control[0].current_bd_ptr = sdma->bd0_phys;
> > ?
> > - spin_unlock_irqrestore(&sdmac->lock, flags);
> > + sdma_set_channel_priority(&sdma->channel[0],
> > MXC_SDMA_DEFAULT_PRIORITY);
> > +out:
> > ?
> > - return cookie;
> > + return ret;
> > +}
> > +
> > +static struct sdma_desc *to_sdma_desc(struct
> > dma_async_tx_descriptor *t)
> > +{
> > + return container_of(t, struct sdma_desc, vd.tx);
> > +}
> > +
> > +static void sdma_desc_free(struct virt_dma_desc *vd)
> > +{
> > + struct sdma_desc *desc = container_of(vd, struct
> > sdma_desc, vd);
> > +
> > + if (desc) {
> Depending on the position of 'vd' in struct sdma_desc 'desc' will
> always
> be non-NULL, even if 'vd' is NULL.
>
> I think this test is unnecessary since this function should never be
> called with an invalid pointer. If it is, then the caller really
> deserved the resulting crash.
Yes, will remove it.
>
> >
> > + sdma_free_bd(desc);
> > + kfree(desc);
> > + }
> > +}
> > +
> > +static int sdma_terminate_all(struct dma_chan *chan)
> > +{
> > + struct sdma_channel *sdmac = to_sdma_chan(chan);
> > + unsigned long flags;
> > + LIST_HEAD(head);
> > +
> > + spin_lock_irqsave(&sdmac->vc.lock, flags);
> > + vchan_get_all_descriptors(&sdmac->vc, &head);
> > + while (!list_empty(&sdmac->pending)) {
> > + struct sdma_desc *desc = list_first_entry(&sdmac-
> > >pending,
> > + struct sdma_desc, node);
> > +
> > + ?list_del(&desc->node);
> > + ?spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > + ?sdmac->vc.desc_free(&desc->vd);
> > + ?spin_lock_irqsave(&sdmac->vc.lock, flags);
> > + }
> list_for_each_entry_safe?
Will remove here all while(sdmac->pending) checking.No need here.?
>
> >
> > +
> > + if (sdmac->desc)
> > + sdmac->desc = NULL;
> The test is unnecesary.
This 'NULL' is meaningful in case that dma done interrupt come after
terminate as you know sdma will actually stop after current transfer
done.
> >
> > + spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > + vchan_dma_desc_free_list(&sdmac->vc, &head);
> > + sdma_disable_channel_with_delay(chan);
> > +
> > + return 0;
> > ?}
> > ?
> > ?static int sdma_alloc_chan_resources(struct dma_chan *chan)
> > @@ -1168,18 +1226,11 @@ static int sdma_alloc_chan_resources(struct
> > dma_chan *chan)
> > ? if (ret)
> > ? goto disable_clk_ipg;
> > ?
> > - ret = sdma_request_channel(sdmac);
> > - if (ret)
> > - goto disable_clk_ahb;
> > -
> > ? ret = sdma_set_channel_priority(sdmac, prio);
> > ? if (ret)
> > ? goto disable_clk_ahb;
> > ?
> > - dma_async_tx_descriptor_init(&sdmac->desc, chan);
> > - sdmac->desc.tx_submit = sdma_tx_submit;
> > - /* txd.flags will be overwritten in prep funcs */
> > - sdmac->desc.flags = DMA_CTRL_ACK;
> > + sdmac->bd_size_sum = 0;
> > ?
> > ? return 0;
> > ?
> > @@ -1195,7 +1246,7 @@ static void sdma_free_chan_resources(struct
> > dma_chan *chan)
> > ? struct sdma_channel *sdmac = to_sdma_chan(chan);
> > ? struct sdma_engine *sdma = sdmac->sdma;
> > ?
> > - sdma_disable_channel(chan);
> > + sdma_terminate_all(chan);
> > ?
> > ? if (sdmac->event_id0)
> > ? sdma_event_disable(sdmac, sdmac->event_id0);
> > @@ -1207,12 +1258,43 @@ static void sdma_free_chan_resources(struct
> > dma_chan *chan)
> > ?
> > ? sdma_set_channel_priority(sdmac, 0);
> > ?
> > - dma_free_coherent(NULL, PAGE_SIZE, sdmac->bd, sdmac-
> > >bd_phys);
> > -
> > ? clk_disable(sdma->clk_ipg);
> > ? clk_disable(sdma->clk_ahb);
> > ?}
> > ?
> > +static struct sdma_desc *sdma_transfer_init(struct sdma_channel
> > *sdmac,
> > + enum dma_transfer_direction
> > direction, u32 bds)
> > +{
> > + struct sdma_desc *desc;
> > +
> > + desc = kzalloc((sizeof(*desc)), GFP_KERNEL);
> > + if (!desc)
> > + goto err_out;
> > +
> > + sdmac->status = DMA_IN_PROGRESS;
> > + sdmac->direction = direction;
> > + sdmac->flags = 0;
> > + sdmac->chn_count = 0;
> > + sdmac->chn_real_count = 0;
> > +
> > + desc->sdmac = sdmac;
> > + desc->num_bd = bds;
> > + INIT_LIST_HEAD(&desc->node);
> > +
> > + if (sdma_alloc_bd(desc))
> > + goto err_desc_out;
> > +
> > + if (sdma_load_context(sdmac))
> > + goto err_desc_out;
> > +
> > + return desc;
> > +
> > +err_desc_out:
> > + kfree(desc);
> > +err_out:
> > + return NULL;
> > +}
> > +
> > ?static struct dma_async_tx_descriptor *sdma_prep_slave_sg(
> > ? struct dma_chan *chan, struct scatterlist *sgl,
> > ? unsigned int sg_len, enum dma_transfer_direction
> > direction,
> > @@ -1223,35 +1305,24 @@ static struct dma_async_tx_descriptor
> > *sdma_prep_slave_sg(
> > ? int ret, i, count;
> > ? int channel = sdmac->channel;
> > ? struct scatterlist *sg;
> > + struct sdma_desc *desc;
> > ?
> > - if (sdmac->status == DMA_IN_PROGRESS)
> > + if (!chan)
> > ? return NULL;
> > - sdmac->status = DMA_IN_PROGRESS;
> > -
> > - sdmac->flags = 0;
> > ?
> > - sdmac->buf_tail = 0;
> > - sdmac->buf_ptail = 0;
> > - sdmac->chn_real_count = 0;
> > + desc = sdma_transfer_init(sdmac, direction, sg_len);
> > + if (!desc)
> > + goto err_out;
> > ?
> > ? dev_dbg(sdma->dev, "setting up %d entries for channel
> > %d.\n",
> > ? sg_len, channel);
> > ?
> > - sdmac->direction = direction;
> > ? ret = sdma_load_context(sdmac);
> > ? if (ret)
> > ? goto err_out;
> > ?
> > - if (sg_len > NUM_BD) {
> > - dev_err(sdma->dev, "SDMA channel %d: maximum
> > number of sg exceeded: %d > %d\n",
> > - channel, sg_len, NUM_BD);
> > - ret = -EINVAL;
> > - goto err_out;
> > - }
> > -
> > - sdmac->chn_count = 0;
> > ? for_each_sg(sgl, sg, sg_len, i) {
> > - struct sdma_buffer_descriptor *bd = &sdmac->bd[i];
> > + struct sdma_buffer_descriptor *bd = &desc->bd[i];
> > ? int param;
> > ?
> > ? bd->buffer_addr = sg->dma_address;
> > @@ -1262,7 +1333,7 @@ static struct dma_async_tx_descriptor
> > *sdma_prep_slave_sg(
> > ? dev_err(sdma->dev, "SDMA channel %d:
> > maximum bytes for sg entry exceeded: %d > %d\n",
> > ? channel, count, 0xffff);
> > ? ret = -EINVAL;
> > - goto err_out;
> > + goto err_bd_out;
> > ? }
> > ?
> > ? bd->mode.count = count;
> > @@ -1307,10 +1378,11 @@ static struct dma_async_tx_descriptor
> > *sdma_prep_slave_sg(
> > ? bd->mode.status = param;
> > ? }
> > ?
> > - sdmac->num_bd = sg_len;
> > - sdma->channel_control[channel].current_bd_ptr = sdmac-
> > >bd_phys;
> > + return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
> > ?
> > - return &sdmac->desc;
> > +err_bd_out:
> > + sdma_free_bd(desc);
> > + kfree(desc);
> > ?err_out:
> > ? sdmac->status = DMA_ERROR;
> > ? return NULL;
> > @@ -1326,39 +1398,32 @@ static struct dma_async_tx_descriptor
> > *sdma_prep_dma_cyclic(
> > ? int num_periods = buf_len / period_len;
> > ? int channel = sdmac->channel;
> > ? int ret, i = 0, buf = 0;
> > + struct sdma_desc *desc;
> > ?
> > ? dev_dbg(sdma->dev, "%s channel: %d\n", __func__, channel);
> > ?
> > - if (sdmac->status == DMA_IN_PROGRESS)
> > - return NULL;
> > -
> > - sdmac->status = DMA_IN_PROGRESS;
> > + /* Now allocate and setup the descriptor. */
> > + desc = sdma_transfer_init(sdmac, direction, num_periods);
> > + if (!desc)
> > + goto err_out;
> > ?
> > - sdmac->buf_tail = 0;
> > - sdmac->buf_ptail = 0;
> > - sdmac->chn_real_count = 0;
> > + desc->buf_tail = 0;
> > + desc->buf_ptail = 0;
> > ? sdmac->period_len = period_len;
> > -
> > ? sdmac->flags |= IMX_DMA_SG_LOOP;
> > - sdmac->direction = direction;
> > +
> > ? ret = sdma_load_context(sdmac);
> > ? if (ret)
> > ? goto err_out;
> > ?
> > - if (num_periods > NUM_BD) {
> > - dev_err(sdma->dev, "SDMA channel %d: maximum
> > number of sg exceeded: %d > %d\n",
> > - channel, num_periods, NUM_BD);
> > - goto err_out;
> > - }
> > -
> > ? if (period_len > 0xffff) {
> > ? dev_err(sdma->dev, "SDMA channel %d: maximum
> > period size exceeded: %zu > %d\n",
> > ? channel, period_len, 0xffff);
> > - goto err_out;
> > + goto err_bd_out;
> > ? }
> > ?
> > ? while (buf < buf_len) {
> > - struct sdma_buffer_descriptor *bd = &sdmac->bd[i];
> > + struct sdma_buffer_descriptor *bd = &desc->bd[i];
> > ? int param;
> > ?
> > ? bd->buffer_addr = dma_addr;
> > @@ -1366,7 +1431,7 @@ static struct dma_async_tx_descriptor
> > *sdma_prep_dma_cyclic(
> > ? bd->mode.count = period_len;
> > ?
> > ? if (sdmac->word_size > DMA_SLAVE_BUSWIDTH_4_BYTES)
> > - goto err_out;
> > + goto err_bd_out;
> > ? if (sdmac->word_size ==
> > DMA_SLAVE_BUSWIDTH_4_BYTES)
> > ? bd->mode.command = 0;
> > ? else
> > @@ -1389,10 +1454,10 @@ static struct dma_async_tx_descriptor
> > *sdma_prep_dma_cyclic(
> > ? i++;
> > ? }
> > ?
> > - sdmac->num_bd = num_periods;
> > - sdma->channel_control[channel].current_bd_ptr = sdmac-
> > >bd_phys;
> > -
> > - return &sdmac->desc;
> > + return vchan_tx_prep(&sdmac->vc, &desc->vd, flags);
> > +err_bd_out:
> > + sdma_free_bd(desc);
> > + kfree(desc);
> > ?err_out:
> > ? sdmac->status = DMA_ERROR;
> > ? return NULL;
> > @@ -1432,26 +1497,74 @@ static enum dma_status
> > sdma_tx_status(struct dma_chan *chan,
> > ?{
> > ? struct sdma_channel *sdmac = to_sdma_chan(chan);
> > ? u32 residue;
> > + struct virt_dma_desc *vd;
> > + struct sdma_desc *desc;
> > + enum dma_status ret;
> > + unsigned long flags;
> > ?
> > - if (sdmac->flags & IMX_DMA_SG_LOOP)
> > - residue = (sdmac->num_bd - sdmac->buf_ptail) *
> > + ret = dma_cookie_status(chan, cookie, txstate);
> > + if (ret == DMA_COMPLETE && txstate) {
> > + residue = sdmac->chn_count - sdmac-
> > >chn_real_count;
> > + return ret;
> > + }
> > +
> > + spin_lock_irqsave(&sdmac->vc.lock, flags);
> > + vd = vchan_find_desc(&sdmac->vc, cookie);
> > + desc = to_sdma_desc(&vd->tx);
> You should use 'vd' only after you have made sure it is valid (though
> I
> see it causes no harm in this case, but let's be nice to the readers
> of
> this code)
Ok, will move desc = to_sdma_desc(&vd->tx) into the below if(vd)...?
>
> >
> > + if (vd) {
> > + if (sdmac->flags & IMX_DMA_SG_LOOP)
> > + residue = (desc->num_bd - desc->buf_ptail)
> > *
> > ? ???sdmac->period_len - sdmac-
> > >chn_real_count;
> > - else
> > + else
> > + residue = sdmac->chn_count - sdmac-
> > >chn_real_count;
> > + } else if (sdmac->desc && sdmac->desc->vd.tx.cookie ==
> > cookie) {
> > ? residue = sdmac->chn_count - sdmac-
> > >chn_real_count;
> > + } else {
> > + residue = 0;
> > + }
> > + ret = sdmac->status;
> > + spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > ?
> > ? dma_set_tx_state(txstate, chan->completed_cookie, chan-
> > >cookie,
> > ? ?residue);
> > ?
> > - return sdmac->status;
> > + return ret;
> > +}
> > +
> > +static void sdma_start_desc(struct sdma_channel *sdmac)
> > +{
> > + struct virt_dma_desc *vd = vchan_next_desc(&sdmac->vc);
> > + struct sdma_desc *desc;
> > + struct sdma_engine *sdma = sdmac->sdma;
> > + int channel = sdmac->channel;
> > +
> > + if (!vd) {
> > + sdmac->desc = NULL;
> > + return;
> > + }
> > + sdmac->desc = desc = to_sdma_desc(&vd->tx);
> > + /*
> > + ?* Do not delete the node in desc_issued list in cyclic
> > mode, otherwise
> > + ?* the desc alloced will never be freed in
> > vchan_dma_desc_free_list
> > + ?*/
> > + if (!(sdmac->flags & IMX_DMA_SG_LOOP)) {
> > + list_add_tail(&sdmac->desc->node, &sdmac-
> > >pending);
> > + list_del(&vd->node);
> > + }
> > + sdma->channel_control[channel].base_bd_ptr = desc-
> > >bd_phys;
> > + sdma->channel_control[channel].current_bd_ptr = desc-
> > >bd_phys;
> > + sdma_enable_channel(sdma, sdmac->channel);
> > ?}
> > ?
> > ?static void sdma_issue_pending(struct dma_chan *chan)
> > ?{
> > ? struct sdma_channel *sdmac = to_sdma_chan(chan);
> > - struct sdma_engine *sdma = sdmac->sdma;
> > + unsigned long flags;
> > ?
> > - if (sdmac->status == DMA_IN_PROGRESS)
> > - sdma_enable_channel(sdma, sdmac->channel);
> > + spin_lock_irqsave(&sdmac->vc.lock, flags);
> > + if (vchan_issue_pending(&sdmac->vc) && !sdmac->desc)
> > + sdma_start_desc(sdmac);
> > + spin_unlock_irqrestore(&sdmac->vc.lock, flags);
> > ?}
> > ?
> > ?#define SDMA_SCRIPT_ADDRS_ARRAY_SIZE_V1 34
> > @@ -1657,7 +1770,7 @@ static int sdma_init(struct sdma_engine
> > *sdma)
> > ? for (i = 0; i < MAX_DMA_CHANNELS; i++)
> > ? writel_relaxed(0, sdma->regs + SDMA_CHNPRI_0 + i *
> > 4);
> > ?
> > - ret = sdma_request_channel(&sdma->channel[0]);
> > + ret = sdma_request_channel0(sdma);
> > ? if (ret)
> > ? goto err_dma_alloc;
> > ?
> > @@ -1819,22 +1932,17 @@ static int sdma_probe(struct
> > platform_device *pdev)
> > ? struct sdma_channel *sdmac = &sdma->channel[i];
> > ?
> > ? sdmac->sdma = sdma;
> > - spin_lock_init(&sdmac->lock);
> > -
> > - sdmac->chan.device = &sdma->dma_device;
> > - dma_cookie_init(&sdmac->chan);
> > ? sdmac->channel = i;
> > -
> > - tasklet_init(&sdmac->tasklet,
> > mxc_sdma_handle_channel_normal,
> > - ?????(unsigned long) sdmac);
> > + sdmac->status = DMA_IN_PROGRESS;
> > + sdmac->vc.desc_free = sdma_desc_free;
> > + INIT_LIST_HEAD(&sdmac->pending);
> > ? /*
> > ? ?* Add the channel to the DMAC list. Do not add
> > channel 0 though
> > ? ?* because we need it internally in the SDMA
> > driver. This also means
> > ? ?* that channel 0 in dmaengine counting matches
> > sdma channel 1.
> > ? ?*/
> > ? if (i)
> > - list_add_tail(&sdmac->chan.device_node,
> > - &sdma-
> > >dma_device.channels);
> > + vchan_init(&sdmac->vc, &sdma->dma_device);
> > ? }
> > ?
> > ? ret = sdma_init(sdma);
> > @@ -1879,7 +1987,7 @@ static int sdma_probe(struct platform_device
> > *pdev)
> > ? sdma->dma_device.device_prep_slave_sg =
> > sdma_prep_slave_sg;
> > ? sdma->dma_device.device_prep_dma_cyclic =
> > sdma_prep_dma_cyclic;
> > ? sdma->dma_device.device_config = sdma_config;
> > - sdma->dma_device.device_terminate_all =
> > sdma_disable_channel_with_delay;
> > + sdma->dma_device.device_terminate_all =
> > sdma_terminate_all;
> > ? sdma->dma_device.src_addr_widths = SDMA_DMA_BUSWIDTHS;
> > ? sdma->dma_device.dst_addr_widths = SDMA_DMA_BUSWIDTHS;
> > ? sdma->dma_device.directions = SDMA_DMA_DIRECTIONS;
> > @@ -1939,7 +2047,8 @@ static int sdma_remove(struct platform_device
> > *pdev)
> > ? for (i = 0; i < MAX_DMA_CHANNELS; i++) {
> > ? struct sdma_channel *sdmac = &sdma->channel[i];
> > ?
> > - tasklet_kill(&sdmac->tasklet);
> > + tasklet_kill(&sdmac->vc.task);
> > + sdma_free_chan_resources(&sdmac->vc.chan);
> > ? }
> > ?
> > ? platform_set_drvdata(pdev, NULL);
> > --?
> > 2.7.4
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel at lists.infradead.org
> > https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fl
> > ists.infradead.org%2Fmailman%2Flistinfo%2Flinux-arm-
> > kernel&data=02%7C01%7Cyibin.gong%40nxp.com%7C3323f6aae75e45a3155f08
> > d5bfcc314f%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63662580608
> > 2347660&sdata=5eDirTg4boJfw0zu320d9GZTeeDwnfCPfHFY8HXt1nI%3D&reserv
> > ed=0
> >
^ permalink raw reply
* [PATCH] gpio: zynq: Setup chip->base based on alias ID
From: Michal Simek @ 2018-05-23 10:26 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CACRpkdb6pN=XYgWN7rPBGDwgpMjNLhgxVk_=QRTrHb=DZuU-xQ@mail.gmail.com>
On 23.5.2018 11:44, Linus Walleij wrote:
> On Tue, May 15, 2018 at 3:26 PM, Michal Simek <michal.simek@xilinx.com> wrote:
>
>> Have you done any decision about this patch?
>
> Yeah I applied the patch. It's a bit of "choose the lesser evil" approach
> but what can I do...
Thanks,
Michal
^ permalink raw reply
* [PATCH v3] arm64: allwinner: a64: Add Amarula A64-Relic initial support
From: Jagan Teki @ 2018-05-23 10:27 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180523081830.4ly6sj34reabbttn@flea>
On Wed, May 23, 2018 at 1:48 PM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> On Wed, May 23, 2018 at 11:44:56AM +0530, Jagan Teki wrote:
>> On Tue, May 22, 2018 at 8:00 PM, Maxime Ripard
>> <maxime.ripard@bootlin.com> wrote:
>> > On Tue, May 22, 2018 at 06:52:28PM +0530, Jagan Teki wrote:
>> >> Amarula A64-Relic is Allwinner A64 based IoT device, which support
>> >> - Allwinner A64 Cortex-A53
>> >> - Mali-400MP2 GPU
>> >> - AXP803 PMIC
>> >> - 1GB DDR3 RAM
>> >> - 8GB eMMC
>> >> - AP6330 Wifi/BLE
>> >> - MIPI-DSI
>> >> - CSI: OV5640 sensor
>> >> - USB OTG
>> >
>> > You claim that this is doing OTG...
>> >
>> > [..]
>> >
>> >> +&usb_otg {
>> >> + dr_mode = "peripheral";
>> >> + status = "okay";
>> >> +};
>> >
>> > ... and yet you're setting it as peripheral...
>>
>> Though it claims OTG, board doesn't have any USB ports to operate(not
>> even Mini-AB) the only way to use the board as peripheral to transfer
>> images from host.
>
> I'm not sure what you mean here. If there's no USB connector, why do
> you even enable it?
I'm saying there is no host port on board. Board has connector
header[1] comes with few pins includes USB D+, D-, ID, VBUS, GROUND,
UART0 TX, RX etc. we have to connect wires to these pins to make
pluggable USB to host pc. and here USB pins used for transferring data
from host pc to target board like in peripheral mode.
Hope you understand.
[1] https://www.digikey.com/product-detail/en/molex-llc/5016451420/WM6074-ND/1787775
Jagan.
--
Jagan Teki
Senior Linux Kernel Engineer | Amarula Solutions
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.
^ permalink raw reply
* [PATCH v3] arm64: fault: Don't leak data in ESR context for user fault on kernel VA
From: Dave Martin @ 2018-05-23 10:31 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522161120.12798-1-peter.maydell@linaro.org>
On Tue, May 22, 2018 at 05:11:20PM +0100, Peter Maydell wrote:
> If userspace faults on a kernel address, handing them the raw ESR
> value on the sigframe as part of the delivered signal can leak data
Nit: on -> in
(not worth it unless you respin the patch though).
> useful to attackers who are using information about the underlying hardware
> fault type (e.g. translation vs permission) as a mechanism to defeat KASLR.
>
> However there are also legitimate uses for the information provided
> in the ESR -- notably the GCC and LLVM sanitizers use this to report
> whether wild pointer accesses by the application are reads or writes
> (since a wild write is a more serious bug than a wild read), so we
> don't want to drop the ESR information entirely.
>
> For faulting addresses in the kernel, sanitize the ESR. We choose
> to present userspace with the illusion that there is nothing mapped
> in the kernel's part of the address space at all, by reporting all
> faults as level 0 translation faults taken to EL1.
Did the discussion about faultin on user addresses go anywhere?
i.e., we tell userspace which page table level a fault occurred
at, and other things that are really none of userspace's business to
know.
This is not obviously a problem, but still a bit odd. It could be
addressed separately.
> These fields are safe to pass through to userspace as they depend
> only on the instruction that userspace used to provoke the fault:
> EC IL (always)
> ISV CM WNR (for all data aborts)
> All the other fields in ESR except DFSC are architecturally RES0
> for an L0 translation fault taken to EL1, so can be zeroed out
> without confusing userspace.
>
> The illusion is not entirely perfect, as there is a tiny wrinkle
> where we will report an alignment fault that was not due to the memory
> type (for instance a LDREX to an unaligned address) as a translation
> fault, whereas if you do this on real unmapped memory the alignment
> fault takes precedence. This is not likely to trip anybody up in
> practice, as the only users we know of for the ESR information who
> care about the behaviour for kernel addresses only really want to
> know about the WnR bit.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Modulo the above (which may be out of scope anyway)
Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Cheers
---Dave
> ---
> This patch is an alternative proposal to Will's patch
> https://patchwork.kernel.org/patch/10258781/
> which simply removed the ESR record entirely for kernel addresses.
>
> Changes v1->v2:
> * rebased on master
> * commit message tweak
> * DABT_CUR and IABT_CUR moved to "can't happen" default case
> * explicitly clear the bits which are RES0 if ISV == 0
> * comment text tweaks
> Changes v2->v3:
> * remove the support for reporting ESRs with ISV == 1 (this
> can't happen, and we probably don't want to tell userspace
> that the exception was taken to EL2 if in some hypothetical
> future that becomes possible)
> * rebased on 4.17-rc6
> ---
> arch/arm64/mm/fault.c | 51 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 51 insertions(+)
>
> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
> index 4165485e8b6e..2af3dd89bcdb 100644
> --- a/arch/arm64/mm/fault.c
> +++ b/arch/arm64/mm/fault.c
> @@ -293,6 +293,57 @@ static void __do_kernel_fault(unsigned long addr, unsigned int esr,
> static void __do_user_fault(struct siginfo *info, unsigned int esr)
> {
> current->thread.fault_address = (unsigned long)info->si_addr;
> +
> + /*
> + * If the faulting address is in the kernel, we must sanitize the ESR.
> + * From userspace's point of view, kernel-only mappings don't exist
> + * at all, so we report them as level 0 translation faults.
> + * (This is not quite the way that "no mapping there at all" behaves:
> + * an alignment fault not caused by the memory type would take
> + * precedence over translation fault for a real access to empty
> + * space. Unfortunately we can't easily distinguish "alignment fault
> + * not caused by memory type" from "alignment fault caused by memory
> + * type", so we ignore this wrinkle and just return the translation
> + * fault.)
> + */
> + if (current->thread.fault_address >= TASK_SIZE) {
> + switch (ESR_ELx_EC(esr)) {
> + case ESR_ELx_EC_DABT_LOW:
> + /*
> + * These bits provide only information about the
> + * faulting instruction, which userspace knows already.
> + * We explicitly clear bits which are architecturally
> + * RES0 in case they are given meanings in future.
> + * We always report the ESR as if the fault was taken
> + * to EL1 and so ISV and the bits in ISS[23:14] are
> + * clear. (In fact it always will be a fault to EL1.)
> + */
> + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL |
> + ESR_ELx_CM | ESR_ELx_WNR;
> + esr |= ESR_ELx_FSC_FAULT;
> + break;
> + case ESR_ELx_EC_IABT_LOW:
> + /*
> + * Claim a level 0 translation fault.
> + * All other bits are architecturally RES0 for faults
> + * reported with that DFSC value, so we clear them.
> + */
> + esr &= ESR_ELx_EC_MASK | ESR_ELx_IL;
> + esr |= ESR_ELx_FSC_FAULT;
> + break;
> + default:
> + /*
> + * This should never happen (entry.S only brings us
> + * into this code for insn and data aborts from a lower
> + * exception level). Fail safe by not providing an ESR
> + * context record at all.
> + */
> + WARN(1, "ESR 0x%x is not DABT or IABT from EL0\n", esr);
> + esr = 0;
> + break;
> + }
> + }
> +
> current->thread.fault_code = esr;
> arm64_force_sig_info(info, esr_to_fault_info(esr)->name, current);
> }
> --
> 2.17.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] arm64: Kconfig: Enable LSE atomics by default
From: Catalin Marinas @ 2018-05-23 10:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1526926462-19214-1-git-send-email-will.deacon@arm.com>
On Mon, May 21, 2018 at 07:14:22PM +0100, Will Deacon wrote:
> Now that we're seeing CPUs shipping with LSE atomics, default them to
> 'on' in Kconfig. CPUs without the instructions will continue to use
> LDXR/STXR-based sequences, but they will be placed out-of-line by the
> compiler.
>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Queued for 4.18. Thanks.
--
Catalin
^ permalink raw reply
* [PATCH] mtd: nand: raw: atmel: add module param to avoid using dma
From: Boris Brezillon @ 2018-05-23 10:42 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <498f4a40-0f4b-fb32-2d6c-032b5341f2b0@axentia.se>
On Tue, 22 May 2018 20:03:41 +0200
Peter Rosin <peda@axentia.se> wrote:
> On 2018-04-11 17:34, Nicolas Ferre wrote:
> > On 11/04/2018 at 16:44, Peter Rosin wrote:
> >> Hi Nicolas,
> >>
> >> Boris asked for your input on this (the datasheet difference appears to
> >> have no bearing on the issue) elsewhere in the tree of messages. It's
> >> now been a week or so and I'm starting to wonder if you missed this
> >> altogether or if you are simply out of office or something?
> >
> > Hi Peter,
> >
> > I didn't dig into this issue with matrix datasheet reset values and your
> > findings below. I'll try to move forward with your detailed explanation
> > and with my contacts within the "product" team internally.
>
> Hi again, just checking in to see if there is any progress? If not, maybe
> it's time to just take the patch as-is, warts and imperfections included,
> and even if we all hate it. But what to do...
Well, it's not really that the fix is ugly, but I'm pretty sure it's
actually not fixing the problem, just make it less likely to happen.
And, as soon as you'll have more traffic going through the DRAM
controller you'll experience the same problem again.
^ permalink raw reply
* [PATCH 0/2] arm64: fatal kernel fault reporting improvements
From: Catalin Marinas @ 2018-05-23 10:47 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180521131451.41040-1-mark.rutland@arm.com>
On Mon, May 21, 2018 at 02:14:49PM +0100, Mark Rutland wrote:
> Catalin noticed that do_page_fault() can die() for some faults without
> reporting the faulting address, which is unfortunate. We also don't
> decode the ESR, or attempt to dump the page tables.
>
> These patches unify the handling of those faults with the ones we handle
> in __do_kernel_fault(), making things a little more consistent.
>
> Thanks,
> Mark.
>
> Mark Rutland (2):
> arm64: make is_permission_fault() name clearer
> arm64: Unify kernel fault reporting
Queued for 4.18. Thanks.
--
Catalin
^ permalink raw reply
* [PATCH 13/14] ARM: KVM: Add SMCCC_ARCH_WORKAROUND_1 fast handling
From: Marc Zyngier @ 2018-05-23 10:50 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <E1fKjFo-00064i-Hc@rmk-PC.armlinux.org.uk>
On 21/05/18 12:45, Russell King wrote:
> We want SMCCC_ARCH_WORKAROUND_1 to be fast. As fast as possible.
> So let's intercept it as early as we can by testing for the
> function call number as soon as we've identified a HVC call
> coming from the guest.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
> arch/arm/kvm/hyp/hyp-entry.S | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/kvm/hyp/hyp-entry.S b/arch/arm/kvm/hyp/hyp-entry.S
> index 918a05dd2d63..67de45685e29 100644
> --- a/arch/arm/kvm/hyp/hyp-entry.S
> +++ b/arch/arm/kvm/hyp/hyp-entry.S
> @@ -16,6 +16,7 @@
> * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
> */
>
> +#include <linux/arm-smccc.h>
> #include <linux/linkage.h>
> #include <asm/kvm_arm.h>
> #include <asm/kvm_asm.h>
> @@ -202,7 +203,7 @@ ENDPROC(__hyp_do_panic)
> lsr r2, r2, #16
> and r2, r2, #0xff
> cmp r2, #0
> - bne guest_trap @ Guest called HVC
> + bne guest_hvc_trap @ Guest called HVC
>
> /*
> * Getting here means host called HVC, we shift parameters and branch
> @@ -253,6 +254,16 @@ THUMB( orr lr, #1)
> pop {r2, lr}
> eret
>
> +guest_hvc_trap:
> + movw ip, #:lower16:ARM_SMCCC_ARCH_WORKAROUND_1
> + movt ip, #:upper16:ARM_SMCCC_ARCH_WORKAROUND_1
r12 is a live guest register, and only r0-r2 are saved at that stage.
You could additionally corrupt r3 though, once you've identified that
you're in the context of an SMCCC 1.1 call.
You should be able to replace r12 with r2.
> + ldr r0, [sp] @ Guest's r0
> + teq r0, ip
> + bne guest_trap
> + pop {r0, r1, r2}
You could replace this slightly more efficient
add sp, sp, #12
since we don't need to restore those registers to the guest. r2 would be
left containing ARM_SMCCC_ARCH_WORKAROUND_1 (harmless), and r1 has the
HSR value (perfectly predictable).
> + mov r0, #0
> + eret
> +
> guest_trap:
> load_vcpu r0 @ Load VCPU pointer to r0
>
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply
* [PATCH] arm64: Make sure permission updates happen for pmd/pud
From: Will Deacon @ 2018-05-23 10:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522235049.23400-1-labbott@redhat.com>
Hi Laura,
On Tue, May 22, 2018 at 04:50:49PM -0700, Laura Abbott wrote:
> Commit 15122ee2c515 ("arm64: Enforce BBM for huge IO/VMAP mappings")
> disallowed block mappings for ioremap since that code does not honor
> break-before-make. The same APIs are also used for permission updating
> though and the extra checks prevent the permission updates from happening,
> even though this should be permitted. This results in read-only permissions
> not being fully applied. Visibly, this can occasionaly be seen as a failure
> on the built in rodata test when the test data ends up in a section or
> as an odd RW gap on the page table dump. Fix this by keeping the check
> for the top level p*d_set_huge APIs but using separate functions for the
> update APIs.
>
> Reported-by: Peter Robinson <pbrobinson@gmail.com>
> Fixes: 15122ee2c515 ("arm64: Enforce BBM for huge IO/VMAP mappings")
> Signed-off-by: Laura Abbott <labbott@redhat.com>
> ---
> arch/arm64/mm/mmu.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
Thanks for sending the fix. One thing below...
> diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
> index 2dbb2c9f1ec1..57517ad86910 100644
> --- a/arch/arm64/mm/mmu.c
> +++ b/arch/arm64/mm/mmu.c
> @@ -66,6 +66,9 @@ static pte_t bm_pte[PTRS_PER_PTE] __page_aligned_bss;
> static pmd_t bm_pmd[PTRS_PER_PMD] __page_aligned_bss __maybe_unused;
> static pud_t bm_pud[PTRS_PER_PUD] __page_aligned_bss __maybe_unused;
>
> +static void __pmd_set_huge(pmd_t *pmdp, phys_addr_t phys, pgprot_t prot);
> +static void __pud_set_huge(pud_t *pudp, phys_addr_t phys, pgprot_t prot);
> +
> pgprot_t phys_mem_access_prot(struct file *file, unsigned long pfn,
> unsigned long size, pgprot_t vma_prot)
> {
> @@ -200,7 +203,7 @@ static void init_pmd(pud_t *pudp, unsigned long addr, unsigned long end,
> /* try section mapping first */
> if (((addr | next | phys) & ~SECTION_MASK) == 0 &&
> (flags & NO_BLOCK_MAPPINGS) == 0) {
> - pmd_set_huge(pmdp, phys, prot);
> + __pmd_set_huge(pmdp, phys, prot);
Given that there is ongoing work to fix the core ioremap code, it would
be nice to avoid adding '__' versions if we can help it. Would it work
if we replaced the pXd_present check with a call to pgattr_change_is_safe
instead?
Will
^ permalink raw reply
* [PATCH 08/14] arm64: ssbd: Disable mitigation on CPU resume if required by user
From: Julien Grall @ 2018-05-23 10:52 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20180522150648.28297-9-marc.zyngier@arm.com>
Hi,
On 05/22/2018 04:06 PM, Marc Zyngier wrote:
> On a system where firmware can dynamically change the state of the
> mitigation, the CPU will always come up with the mitigation enabled,
> including when coming back from suspend.
>
> If the user has requested "no mitigation" via a command line option,
> let's enforce it by calling into the firmware again to disable it.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
Reviewed-by: Julien Grall <julien.grall@arm.com>
Cheers,
> ---
> arch/arm64/include/asm/cpufeature.h | 6 ++++++
> arch/arm64/kernel/cpu_errata.c | 8 ++++----
> arch/arm64/kernel/suspend.c | 8 ++++++++
> 3 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
> index 1bacdf57f0af..d9dcb683259e 100644
> --- a/arch/arm64/include/asm/cpufeature.h
> +++ b/arch/arm64/include/asm/cpufeature.h
> @@ -553,6 +553,12 @@ static inline int arm64_get_ssbd_state(void)
> #endif
> }
>
> +#ifdef CONFIG_ARM64_SSBD
> +void arm64_set_ssbd_mitigation(bool state);
> +#else
> +static inline void arm64_set_ssbd_mitigation(bool state) {}
> +#endif
> +
> #endif /* __ASSEMBLY__ */
>
> #endif
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index 8f686f39b9c1..b4c12e9140f0 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -297,7 +297,7 @@ void __init arm64_enable_wa2_handling(struct alt_instr *alt,
> *updptr = cpu_to_le32(aarch64_insn_gen_nop());
> }
>
> -static void do_ssbd(bool state)
> +void arm64_set_ssbd_mitigation(bool state)
> {
> switch (psci_ops.conduit) {
> case PSCI_CONDUIT_HVC:
> @@ -371,20 +371,20 @@ static bool has_ssbd_mitigation(const struct arm64_cpu_capabilities *entry,
> switch (ssbd_state) {
> case ARM64_SSBD_FORCE_DISABLE:
> pr_info_once("%s disabled from command-line\n", entry->desc);
> - do_ssbd(false);
> + arm64_set_ssbd_mitigation(false);
> required = false;
> break;
>
> case ARM64_SSBD_EL1_ENTRY:
> if (required) {
> __this_cpu_write(arm64_ssbd_callback_required, 1);
> - do_ssbd(true);
> + arm64_set_ssbd_mitigation(true);
> }
> break;
>
> case ARM64_SSBD_FORCE_ENABLE:
> pr_info_once("%s forced from command-line\n", entry->desc);
> - do_ssbd(true);
> + arm64_set_ssbd_mitigation(true);
> required = true;
> break;
>
> diff --git a/arch/arm64/kernel/suspend.c b/arch/arm64/kernel/suspend.c
> index a307b9e13392..70c283368b64 100644
> --- a/arch/arm64/kernel/suspend.c
> +++ b/arch/arm64/kernel/suspend.c
> @@ -62,6 +62,14 @@ void notrace __cpu_suspend_exit(void)
> */
> if (hw_breakpoint_restore)
> hw_breakpoint_restore(cpu);
> +
> + /*
> + * On resume, firmware implementing dynamic mitigation will
> + * have turned the mitigation on. If the user has forcefully
> + * disabled it, make sure their wishes are obeyed.
> + */
> + if (arm64_get_ssbd_state() == ARM64_SSBD_FORCE_DISABLE)
> + arm64_set_ssbd_mitigation(false);
> }
>
> /*
>
--
Julien Grall
^ permalink raw reply
* [PATCH v3 01/13] dt-bindings: power: add RK3036 SoCs header for power-domain
From: Heiko Stübner @ 2018-05-23 10:55 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058129-10260-2-git-send-email-zhangqing@rock-chips.com>
Am Mittwoch, 23. Mai 2018, 08:48:37 CEST schrieb Elaine Zhang:
> From: Caesar Wang <wxt@rock-chips.com>
>
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
applied for 4.18 or 4.19 (not sure yet) after adding
Rob's Review-tag from v2.
Thanks
Heiko
^ permalink raw reply
* [PATCH v3 02/13] dt-bindings: add binding for rk3036 power domains
From: Heiko Stübner @ 2018-05-23 10:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058129-10260-3-git-send-email-zhangqing@rock-chips.com>
Am Mittwoch, 23. Mai 2018, 08:48:38 CEST schrieb Elaine Zhang:
> From: Caesar Wang <wxt@rock-chips.com>
>
> Add binding documentation for the power domains
> found on Rockchip RK3036 SoCs.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Acked-by: Rob Herring <robh@kernel.org>
applied for 4.18 or 4.19 (not sure yet).
Thanks
Heiko
^ permalink raw reply
* [PATCH v1] dma: imx-sdma: add virt-dma support
From: s.hauer at pengutronix.de @ 2018-05-23 10:56 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527099973.3990.37.camel@nxp.com>
On Wed, May 23, 2018 at 10:26:23AM +0000, Robin Gong wrote:
> >
> > >
> > > + u32 bd_size_sum;
> > This variable is never used for anything.
> Yes, it's not for significative use but debug to see how many current
> bds used.
I am not convinced this is useful. The variable could easily be added back
by someone who debugs this driver. The code has to be changed anyway to
make use of this variable.
> > > @@ -632,7 +635,7 @@ static int sdma_run_channel0(struct sdma_engine
> > > *sdma)
> > > ?static int sdma_load_script(struct sdma_engine *sdma, void *buf,
> > > int size,
> > > ? u32 address)
> > > ?{
> > > - struct sdma_buffer_descriptor *bd0 = sdma->channel[0].bd;
> > > + struct sdma_buffer_descriptor *bd0 = sdma->bd0;
> > This change seems to be an orthogonal change. Please make this a
> > separate patch.
> It's something related with virtual dma support, because in virtual
> dma framework, all bds should be allocated dynamically if they used.
> but bd0 is a specail case since it's must and basic for load sdma
> firmware and context for other channels. So here alloc 'bd0' for other
> channels.
Well, it's somewhat related to virtual dma support, but that's not my
point. My point is that this patch is quite big and thus hard to review.
If we find ways to make it smaller and to split it up in multiple
patches then we should do so, because it makes it easier to review and
in case you break something here we raise the chance that a "git bisect"
lands on a smaller patch which is easier to understand.
Please try and make that a separate change. I haven't really looked into
it and it may not be possible due to reasons I haven't seen, but please
at least give it a try.
> >
> > >
> > > +
> > > + if (sdmac->desc)
> > > + sdmac->desc = NULL;
> > The test is unnecesary.
> This 'NULL' is meaningful in case that dma done interrupt come after
> terminate as you know sdma will actually stop after current transfer
> done.
The setting of the variable to NULL is ok, but the test is useless.
if (sdmac->desc)
sdmac->desc = NULL;
is equivalent to:
sdmac->desc = NULL;
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH v3 03/13] Soc: rockchip: power-domain: add power domain support for rk3036
From: Heiko Stübner @ 2018-05-23 10:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058129-10260-4-git-send-email-zhangqing@rock-chips.com>
Am Mittwoch, 23. Mai 2018, 08:48:39 CEST schrieb Elaine Zhang:
> From: Caesar Wang <wxt@rock-chips.com>
>
> This driver is modified to support RK3036 SoC.
>
> Signed-off-by: Caesar Wang <wxt@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> @@ -102,6 +103,14 @@ struct rockchip_pmu {
> .ack_mask = (ack >= 0) ? BIT(ack) : 0, \
> .active_wakeup = wakeup, \
> }
I've added a blank line here, made "Soc" in the subject lower case and
applied it for 4.18 or 4.19 (not sure yet).
Thanks
Heiko
> +#define DOMAIN_RK3036(req, ack, idle, wakeup) \
> +{ \
> + .req_mask = (req >= 0) ? BIT(req) : 0, \
> + .req_w_mask = (req >= 0) ? BIT(req + 16) : 0, \
> + .ack_mask = (ack >= 0) ? BIT(ack) : 0, \
> + .idle_mask = (idle >= 0) ? BIT(idle) : 0, \
> + .active_wakeup = wakeup, \
> +}
^ permalink raw reply
* [PATCH 1/5] Documentation: DT: Add optional 'timeout-sec' property for sp805
From: Robin Murphy @ 2018-05-23 10:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527014840-21236-2-git-send-email-ray.jui@broadcom.com>
On 22/05/18 19:47, Ray Jui wrote:
> Update the SP805 binding document to add optional 'timeout-sec'
> devicetree property
>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> ---
> Documentation/devicetree/bindings/watchdog/sp805-wdt.txt | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
> index edc4f0e..f898a86 100644
> --- a/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
> +++ b/Documentation/devicetree/bindings/watchdog/sp805-wdt.txt
> @@ -19,6 +19,8 @@ Required properties:
>
> Optional properties:
> - interrupts : Should specify WDT interrupt number.
> +- timeout-sec : Should specify default WDT timeout in seconds. If unset, the
> + default timeout is 30 seconds
According to the SP805 TRM, the default interval is dependent on the
rate of WDOGCLK, but would typically be a lot longer than that :/
On a related note, anyone have any idea why we seem to have two subtly
different SP805 bindings defined?
Robin.
>
> Examples:
>
>
^ permalink raw reply
* [PATCH v3 04/13] soc: rockchip: power-domain: Fix wrong value when power up pd
From: Heiko Stübner @ 2018-05-23 10:57 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058129-10260-5-git-send-email-zhangqing@rock-chips.com>
Am Mittwoch, 23. Mai 2018, 08:48:40 CEST schrieb Elaine Zhang:
> From: Finley Xiao <finley.xiao@rock-chips.com>
>
> Solve the pd could only ever turn off but never turn them on again,
> If the pd registers have the writemask bits.
>
> Fix up the code error for commit:
> commit 79bb17ce8edb3141339b5882e372d0ec7346217c
> Author: Elaine Zhang <zhangqing@rock-chips.com>
> Date: Fri Dec 23 11:47:52 2016 +0800
>
> soc: rockchip: power-domain: Support domain control in hiword-registers
>
> New Rockchips SoCs may have their power-domain control in registers
> using a writemask-based access scheme (upper 16bit being the write
> mask). So add a DOMAIN_M type and handle this case accordingly.
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
>
> Signed-off-by: Finley Xiao <finley.xiao@rock-chips.com>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
I've already applied that patch from v2.
Heiko
^ permalink raw reply
* [PATCH v3 05/13] dt-bindings: power: add RK3128 SoCs header for power-domain
From: Heiko Stübner @ 2018-05-23 11:16 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058231-10319-1-git-send-email-zhangqing@rock-chips.com>
Am Mittwoch, 23. Mai 2018, 08:50:31 CEST schrieb Elaine Zhang:
> According to a description from TRM, add all the power domains.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
applied for 4.18 or 4.19 (not sure yet) after adding
Rob's Review-tag from v2.
Thanks
Heiko
^ permalink raw reply
* [PATCH v3 06/13] dt-bindings: add binding for rk3128 power domains
From: Heiko Stübner @ 2018-05-23 11:17 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1527058248-10366-1-git-send-email-zhangqing@rock-chips.com>
Am Mittwoch, 23. Mai 2018, 08:50:48 CEST schrieb Elaine Zhang:
> Add binding documentation for the power domains
> found on Rockchip RK3128 SoCs.
>
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> Acked-by: Rob Herring <robh@kernel.org>
applied for 4.18 or 4.19 (not sure yet).
^ 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