* Re: [PATCH] ARM: Add support for Realtek SOC
From: Arnd Bergmann @ 2019-09-05 8:31 UTC (permalink / raw)
To: jamestai.sky
Cc: Rob Herring, Jason A . Donenfeld, Thierry Reding, james.tai,
Ard Biesheuvel, CY_Huang, Linus Walleij, Nicolas Pitre,
Nick Desaulniers, linux-kernel@vger.kernel.org, Stefan Agner,
Russell King, Masahiro Yamada, Paul Burton, Benjamin Gaignard,
Andreas Färber, Mauro Carvalho Chehab, Doug Anderson,
Mike Rapoport, Phinex Hung, Linux ARM
In-Reply-To: <20190905054647.1235-1-james.tai@realtek.com>
On Thu, Sep 5, 2019 at 7:48 AM <jamestai.sky@gmail.com> wrote:
>
> From: "james.tai" <james.tai@realtek.com>
>
> This patch adds the basic machine file for
> the Realtek RTD16XX platform.
>
> Signed-off-by: james.tai <james.tai@realtek.com>
Hi James,
Thanks a lot for your submission! I'm glad to see interest in upstream
support for this SoC family. I have a few small comments on
details, mostly where I would either like to see an explanation
in the patch description, or things that looks like they can be
left out from the patch.
> index 33b00579beff..c7c9a3662eb7 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -836,6 +836,8 @@ source "arch/arm/mach-zx/Kconfig"
>
> source "arch/arm/mach-zynq/Kconfig"
>
> +source "arch/arm/mach-realtek/Kconfig"
> +
> diff --git a/arch/arm/mach-realtek/Kconfig b/arch/arm/mach-realtek/Kconfig
> @@ -225,6 +226,7 @@ machine-$(CONFIG_ARCH_VT8500) += vt8500
> machine-$(CONFIG_ARCH_W90X900) += w90x900
> machine-$(CONFIG_ARCH_ZX) += zx
> machine-$(CONFIG_ARCH_ZYNQ) += zynq
> +machine-$(CONFIG_ARCH_REALTEK) += realtek
> machine-$(CONFIG_PLAT_SPEAR) += spear
>
> # Platform directory name. This list is sorted alphanumerically
Please keep these lists in alphabetical order.
> # ARMv7-M architecture
> config ARCH_EFM32
> bool "Energy Micro efm32"
> diff --git a/arch/arm/Makefile b/arch/arm/Makefile
> index c3624ca6c0bc..1f0926449d47 100644
> --- a/arch/arm/Makefile
> +++ b/arch/arm/Makefile
> @@ -148,6 +148,7 @@ endif
> textofs-$(CONFIG_ARCH_MSM8X60) := 0x00208000
> textofs-$(CONFIG_ARCH_MSM8960) := 0x00208000
> textofs-$(CONFIG_ARCH_MESON) := 0x00208000
> +textofs-$(CONFIG_ARCH_REALTEK) := 0x00208000
> textofs-$(CONFIG_ARCH_AXXIA) := 0x00308000
Can you explain why this is needed for your platform?
> # Machine directory name. This list is sorted alphanumerically
> new file mode 100644
> index 000000000000..a8269964dbdb
> --- /dev/null
> +++ b/arch/arm/mach-realtek/Kconfig
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +menuconfig ARCH_REALTEK
> + bool "Realtek SoCs"
Please add "depends on ARCH_MULTI_V7" to avoid
compile time issues when selecting it on an earlier
architecture.
> + select ARM_GLOBAL_TIMER
> + select CLKDEV_LOOKUP
> + select HAVE_SMP
> + select HAVE_MACH_CLKDEV
> + select GENERIC_CLOCKEVENTS
> + select HAVE_SCHED_CLOCK
> + select ARCH_HAS_CPUFREQ
> + select CLKSRC_OF
> + select ARCH_REQUIRE_GPIOLIB
> + select GENERIC_IRQ_CHIP
> + select IRQ_DOMAIN
> + select PINCTRL
> + select COMMON_CLK
> + select ARCH_HAS_BARRIERS
> + select SPARSE_IRQ
> + select PM_OPP
> + select ARM_HAS_SG_CHAIN
> + select ARM_PATCH_PHYS_VIRT
> + select AUTO_ZRELADDR
> + select MIGHT_HAVE_PCI
> + select MULTI_IRQ_HANDLER
> + select PCI_DOMAINS if PCI
> + select USE_OF
Almost all of the symbols above are implied by
ARCH_MULTI_V7 and should not be selected
separately.
> +config ARCH_RTD16XX
> + bool "Enable support for RTD1619"
> + depends on ARCH_REALTEK
> + select ARM_GIC_V3
> + select ARM_PSCI
As I understand, this chip uses a Cortex-A55. What is the reason
for adding support only to the 32-bit ARM architecture rather than
64-bit?
Most 64-bit SoCs are only supported with arch/arm64, but generally
speaking that is not a requirement. My rule of thumb is that on
systems with 1GB of RAM or more, one would want to run a 64-bit
kernel, while systems with less than that are better off with a 32-bit
one, but that is clearly not the only reason for picking one over the
other.
> +
> +static int rtk_boot_secondary(unsigned int cpu, struct task_struct *idle)
> +{
> + unsigned long entry_pa = __pa_symbol(secondary_startup);
> +
> + writel_relaxed(entry_pa | (cpu << CPUID), cpu_release_virt);
> +
> + arch_send_wakeup_ipi_mask(cpumask_of(cpu));
> +
> + return 0;
> +}
It's very unusual to see custom smp operations on an ARMv8
system, as we normally use PSCI here. Can you explain what
is going on here? Are you able to use a boot wrapper that implements
these in psci instead?
> +
> +#include "platsmp.h"
> +
> +#define RBUS_BASE_PHYS (0x98000000)
> +#define RBUS_BASE_VIRT (0xfe000000)
> +#define RBUS_BASE_SIZE (0x00100000)
> +
> +static struct map_desc rtk_io_desc[] __initdata = {
> + {
> + .virtual = (unsigned long) IOMEM(RBUS_BASE_VIRT),
> + .pfn = __phys_to_pfn(RBUS_BASE_PHYS),
> + .length = RBUS_BASE_SIZE,
> + .type = MT_DEVICE,
> + },
> +};
This needs a comment: Why do you require a static mapping for
"RBUS_BASE_PHYS"? Normally device drivers should just use
ioremap() for mapping whichever registers they want to access.
> +static void __init rtk_dt_init(void)
> +{
> + of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
> +}
This should be taken care of by the
of_platform_default_populate_init() and can be dropped.
> +static void __init rtk_timer_init(void)
> +{
> +#ifdef CONFIG_COMMON_CLK
> + of_clk_init(NULL);
> +#endif
COMMON_CLK is implied by ARCH_MULTI_V7, so the
#ifdef can be dropped.
> + timer_probe();
> + tick_setup_hrtimer_broadcast();
> +}
What do you need tick_setup_hrtimer_broadcast() for? I don't
see any other platform calling this.
> +bool __init rtk_smp_init_ops(void)
> +{
> + smp_set_ops(smp_ops(rtk_smp_ops));
> +
> + return true;
> +}
I think this can also be dropped, as you set the smp_ops in the
machine descriptor.
Arnd
_______________________________________________
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Peter Maydell @ 2019-09-05 8:32 UTC (permalink / raw)
To: Christoffer Dall
Cc: Daniel P . Berrangé, Marc Zyngier,
lkml - Kernel Mailing List, Stefan Hajnoczi, Heinrich Schuchardt,
kvmarm, arm-mail-list
In-Reply-To: <20190905082503.GB4320@e113682-lin.lund.arm.com>
On Thu, 5 Sep 2019 at 09:25, Christoffer Dall <christoffer.dall@arm.com> wrote:
>
> On Thu, Sep 05, 2019 at 09:16:54AM +0100, Peter Maydell wrote:
> > This is true, but the problem is that barfing out to userspace
> > makes it harder to debug the guest because it means that
> > the VM is immediately destroyed, whereas AIUI if we
> > inject some kind of exception then (assuming you're set up
> > to do kernel-debug via gdbstub) you can actually examine
> > the offending guest code with a debugger because at least
> > your VM is still around to inspect...
> >
>
> Is it really going to be easier to debug a guest that sees behavior
> which may not be architecturally correct? For example, seeing a data
> abort on an access to an MMIO region because the guest used a strange
> instruction?
Yeah, a data abort is not ideal. You could UNDEF the insn, which
probably is more likely to result in getting control in the
debugger I suppose.
As for whether it's going to be easier to debug, for the
user who reported this in the first place it certainly was.
(Consider even a simple Linux guest not under a debugger --
if we UNDEF the insn the guest kernel will print a helpful
backtrace so you can tell where the problem is; at the moment
we just print a register dump from the host kernel, which is a
lot less informative.)
> I appreaciate that the current way we handle this is confusing and has
> led many people down a rabbit hole, so we should do better.
>
> Would a better approach not be to return to userspace saying, "we can't
> handle this in the kernel, you decide", without printing the dubious
> kernel error message.
Printing the message in the kernel is the best clue we give
the user at the moment that they've run into this problem;
I would be wary of removing it (even if we decide to also
do something else).
> Then user space could suspend the VM and print a
> lenghty explanation of all the possible problems there could be, or
> re-inject something back into the guest, or whatever, for a particular
> environment.
In theory I guess so. In practice that's not what userspace
currently in the wild does, and injecting an exception from
userspace is a bit awkward (I dunno if kvmtool does it,
QEMU only needs to in really obscure circumstances and
was buggy in how it tried to do it until very recently)...
thanks
-- PMM
_______________________________________________
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Heinrich Schuchardt @ 2019-09-05 8:28 UTC (permalink / raw)
To: Marc Zyngier
Cc: Peter Maydell, Daniel P . Berrangé, Suzuki K Pouloze,
Julien Thierry, linux-kernel, James Morse, Stefan Hajnoczi,
kvmarm, linux-arm-kernel
In-Reply-To: <86r24vrwyh.wl-maz@kernel.org>
On 9/5/19 10:03 AM, Marc Zyngier wrote:
> [Please use my kernel.org address. My arm.com address will disappear shortly]
>
> On Wed, 04 Sep 2019 19:07:36 +0100,
> Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> If an application tries to access memory that is not mapped, an error
>> ENOSYS, "load/store instruction decoding not implemented" may occur.
>> QEMU will hang with a register dump.
>>
>> Instead create a data abort that can be handled gracefully by the
>> application running in the virtual environment.
>>
>> Now the virtual machine can react to the event in the most appropriate
>> way - by recovering, by writing an informative log, or by rebooting.
>>
>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>> virt/kvm/arm/mmio.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
>> index a8a6a0c883f1..0cbed7d6a0f4 100644
>> --- a/virt/kvm/arm/mmio.c
>> +++ b/virt/kvm/arm/mmio.c
>> @@ -161,8 +161,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
>> if (ret)
>> return ret;
>> } else {
>> - kvm_err("load/store instruction decoding not implemented\n");
>> - return -ENOSYS;
>> + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
>> + return 1;
>
> How can you tell that the access would fault? You have no idea at that
> stage (the kernel doesn't know about the MMIO ranges that userspace
> handles). All you know is that you're faced with a memory access that
> you cannot emulate in the kernel. Injecting a data abort at that stage
> is not something that the architecture allows.
>
> If you want to address this, consider forwarding the access to
> userspace. You'll only need an instruction decoder (supporting T1, T2,
> A32 and A64) and a S1 page table walker (one per page table format,
> all three of them) to emulate the access (having taken care of
> stopping all the other vcpus to make sure there is no concurrent
> modification of the page tables). You'll then be able to return the
> result of the access back to the kernel.
If I understand you right, the problem has to be fixed in QEMU and not
in KVM.
Is there an example of such a forwarding already available in QEMU?
>
> Of course, the best thing would be to actually fix the guest so that
> it doesn't use non-emulatable MMIO accesses. In general, that the sign
> of a bug in low-level accessors.
My problem was to find out where in my guest (U-Boot running UEFI SCT)
the problem occurred. With this patch U-Boot gave me the relative
address in Shell.efi and I was able to locate what was wrong in U-Boot's
UEFI implementation.
Thanks for reviewing.
Heinrich
_______________________________________________
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] mmc: block: make the card_busy_detect() more generic
From: Avri Altman @ 2019-09-05 8:43 UTC (permalink / raw)
To: Chaotian Jing, Ulf Hansson
Cc: Jens Axboe, Chris Boot, srv_heupstream@mediatek.com,
linux-mmc@vger.kernel.org, Zachary Hays, YueHaibing,
linux-kernel@vger.kernel.org, Ming Lei, Wolfram Sang,
linux-mediatek@lists.infradead.org, Hannes Reinecke,
Matthias Brugger, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190905075318.15554-2-chaotian.jing@mediatek.com>
>
> to use the card_busy_detect() to wait card levae the programming state,
> there may be do not have the "struct request *" argument.
Maybe reword the commit log to make it more clear:
A tad optimization, removing the "struct request *" argument from card_busy_detect().
It's not really needed there, and will prove its worth in the next patch,
Where we'll use it in __mmc_blk_ioctl_cmd where struct request is not available.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
_______________________________________________
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 v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Rasmus Villemoes @ 2019-09-05 8:43 UTC (permalink / raw)
To: Aleksa Sarai, Al Viro, Jeff Layton, J. Bruce Fields,
Arnd Bergmann, David Howells, Shuah Khan, Shuah Khan, Ingo Molnar,
Peter Zijlstra, Christian Brauner
Cc: linux-ia64, linux-sh, Alexander Shishkin, Alexei Starovoitov,
linux-kernel, linux-kselftest, sparclinux, Jiri Olsa, linux-arch,
linux-s390, Tycho Andersen, Aleksa Sarai, linux-mips,
linux-xtensa, Kees Cook, Jann Horn, linuxppc-dev, linux-m68k,
Andy Lutomirski, Namhyung Kim, David Drysdale, linux-arm-kernel,
linux-parisc, linux-api, Chanho Min, Oleg Nesterov,
Eric Biederman, linux-alpha, linux-fsdevel, Andrew Morton,
Linus Torvalds, containers
In-Reply-To: <20190904201933.10736-2-cyphar@cyphar.com>
On 04/09/2019 22.19, Aleksa Sarai wrote:
> A common pattern for syscall extensions is increasing the size of a
> struct passed from userspace, such that the zero-value of the new fields
> result in the old kernel behaviour (allowing for a mix of userspace and
> kernel vintages to operate on one another in most cases). This is done
> in both directions -- hence two helpers -- though it's more common to
> have to copy user space structs into kernel space.
>
> Previously there was no common lib/ function that implemented
> the necessary extension-checking semantics (and different syscalls
> implemented them slightly differently or incompletely[1]). A future
> patch replaces all of the common uses of this pattern to use the new
> copy_struct_{to,from}_user() helpers.
>
> [1]: For instance {sched_setattr,perf_event_open,clone3}(2) all do do
> similar checks to copy_struct_from_user() while rt_sigprocmask(2)
> always rejects differently-sized struct arguments.
>
> Suggested-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
> ---
> diff --git a/lib/struct_user.c b/lib/struct_user.c
> new file mode 100644
> index 000000000000..7301ab1bbe98
> --- /dev/null
> +++ b/lib/struct_user.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2019 SUSE LLC
> + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/export.h>
> +#include <linux/uaccess.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +
> +#define BUFFER_SIZE 64
> +
> +/*
> + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> + * checked access_ok(p, size).
> + */
Isn't this __clear_user() exactly (perhaps except for the return value)?
Perhaps not every arch has that?
> +static int __memzero_user(void __user *p, size_t s)
> +{
> + const char zeros[BUFFER_SIZE] = {};
> + while (s > 0) {
> + size_t n = min(s, sizeof(zeros));
> +
> + if (__copy_to_user(p, zeros, n))
> + return -EFAULT;
> +
> + p += n;
> + s -= n;
> + }
> + return 0;
> +}
> +
> +/**
> + * copy_struct_to_user: copy a struct to user space
> + * @dst: Destination address, in user space.
> + * @usize: Size of @dst struct.
> + * @src: Source address, in kernel space.
> + * @ksize: Size of @src struct.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -EFBIG: (@usize < @ksize) and there are non-zero trailing bytes in @src.
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_to_user(void __user *dst, size_t usize,
> + const void *src, size_t ksize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
Eh, I'd avoid abs() here due to the funkiness of the implicit type
conversions - ksize-usize has type size_t, then that's coerced to an int
(or a long maybe?), the abs is applied which return an int/long (or
unsigned versions?). Something like "rest = max(ksize, usize) - size;"
is more obviously correct and doesn't fall into any
narrowing/widening/sign extending traps.
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
Please don't. That is a restriction on all future extensions - once a
kernel is shipped with a syscall using this helper with that arbitrary
restriction in place, that syscall is forever prevented from extending
its arg struct beyond PAGE_SIZE (which is arch-dependent anyway). Sure,
it's hard to imagine, but who'd have thought 32 O_* or CLONE_* bits
weren't enough for everybody?
This is only for future compatibility, and if someone runs an app
compiled against 7.3 headers on a 5.4 kernel, they probably don't care
about performance, but they would like their app to run.
[If we ever create such a large ABI struct that doesn't fit on stack,
we'd have to extend our API a little to create a dup_struct_from_user()
that does the kmalloc() for us and then calls copy_struct_from_user() -
but we might want that long before we hit PAGE_SIZE structs].
> + if (unlikely(!access_ok(dst, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize) {
> + if (memchr_inv(src + size, 0, rest))
> + return -EFBIG;
> + } else if (usize > ksize) {
> + if (__memzero_user(dst + size, rest))
> + return -EFAULT;
I think that could simply be __clear_user().
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_to_user(dst, src, size))
> + return -EFAULT;
I think I understand why you put this last instead of handling the
buffer in the "natural" order. However,
I'm wondering whether we should actually do this copy before checking
that the extra kernel bytes are 0 - the user will still be told that
there was some extra information via the -EFBIG/-E2BIG return, but maybe
in some cases the part he understands is good enough. But I also guess
we have to look to existing users to see whether that would prevent them
from being converted to using this helper.
linux-api folks, WDYT?
> + return 0;
Maybe more useful to "return size;", some users might want to know/pass
on how much was actually copied.
> +}
> +EXPORT_SYMBOL(copy_struct_to_user);
Can't we wait with this until a modular user shows up? The primary users
are syscalls, which can't be modular AFAIK.
> +/**
> + * copy_struct_from_user: copy a struct from user space
> + * @dst: Destination address, in kernel space. This buffer must be @ksize
> + * bytes long.
> + * @ksize: Size of @dst struct.
> + * @src: Source address, in user space.
> + * @usize: (Alleged) size of @src struct.
> + *
> + * Copies a struct from user space to kernel space, in a way that guarantees
> + * backwards-compatibility for struct syscall arguments (as long as future
> + * struct extensions are made such that all new fields are *appended* to the
> + * old struct, and zeroed-out new fields have the same meaning as the old
> + * struct).
> + *
> + * @ksize is just sizeof(*dst), and @usize should've been passed by user space.
> + * The recommended usage is something like the following:
> + *
> + * SYSCALL_DEFINE2(foobar, const struct foo __user *, uarg, size_t, usize)
> + * {
> + * int err;
> + * struct foo karg = {};
> + *
> + * err = copy_struct_from_user(&karg, sizeof(karg), uarg, size);
> + * if (err)
> + * return err;
> + *
> + * // ...
> + * }
> + *
> + * There are three cases to consider:
> + * * If @usize == @ksize, then it's copied verbatim.
> + * * If @usize < @ksize, then the user space has passed an old struct to a
> + * newer kernel. The rest of the trailing bytes in @dst (@ksize - @usize)
> + * are to be zero-filled.
> + * * If @usize > @ksize, then the user space has passed a new struct to an
> + * older kernel. The trailing bytes unknown to the kernel (@usize - @ksize)
> + * are checked to ensure they are zeroed, otherwise -E2BIG is returned.
> + *
> + * Returns (in all cases, some data may have been copied):
> + * * -E2BIG: (@usize > @ksize) and there are non-zero trailing bytes in @src.
> + * * -E2BIG: @usize is "too big" (at time of writing, >PAGE_SIZE).
> + * * -EFAULT: access to user space failed.
> + */
> +int copy_struct_from_user(void *dst, size_t ksize,
> + const void __user *src, size_t usize)
> +{
> + size_t size = min(ksize, usize);
> + size_t rest = abs(ksize - usize);
As above.
> + if (unlikely(usize > PAGE_SIZE))
> + return -EFAULT;
As above.
> + if (unlikely(!access_ok(src, usize)))
> + return -EFAULT;
> +
> + /* Deal with trailing bytes. */
> + if (usize < ksize)
> + memset(dst + size, 0, rest);
> + else if (usize > ksize) {
> + const void __user *addr = src + size;
> + char buffer[BUFFER_SIZE] = {};
> +
> + while (rest > 0) {
> + size_t bufsize = min(rest, sizeof(buffer));
> +
> + if (__copy_from_user(buffer, addr, bufsize))
> + return -EFAULT;
> + if (memchr_inv(buffer, 0, bufsize))
> + return -E2BIG;
> +
> + addr += bufsize;
> + rest -= bufsize;
> + }
I'd create a __user_is_zero() helper for this - that way the two
branches in the two helpers become nicely symmetric, each just calling a
single helper that deals appropriately with the tail. And we can discuss
how to implement __user_is_zero() in another bikeshed.
> + }
> + /* Copy the interoperable parts of the struct. */
> + if (__copy_from_user(dst, src, size))
> + return -EFAULT;
If you do move up the __copy_to_user(), please move this as well - on
the kernel side, we certainly don't care that we copied some bytes to a
local buffer which we then ignore because the user had a non-zero tail.
But if __copy_to_user() is kept last in copy_struct_to_user(), this
should stay for symmetry.
> + return 0;
As above.
> +}
> +EXPORT_SYMBOL(copy_struct_from_user);
As above.
Rasmus
_______________________________________________
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] mmc: block: add CMD13 polling for ioctl() cmd with R1B response
From: Avri Altman @ 2019-09-05 8:44 UTC (permalink / raw)
To: Chaotian Jing, Ulf Hansson
Cc: Jens Axboe, Chris Boot, srv_heupstream@mediatek.com,
linux-mmc@vger.kernel.org, Zachary Hays, YueHaibing,
linux-kernel@vger.kernel.org, Ming Lei, Wolfram Sang,
linux-mediatek@lists.infradead.org, Hannes Reinecke,
Matthias Brugger, linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190905075318.15554-3-chaotian.jing@mediatek.com>
>
>
> currently there is no CMD13 polling and other code to wait card
> change to transfer state after R1B command completed. and this
> polling operation cannot do in user space, because other request
> may coming before the CMD13 from user space.
>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
Reviewed-by: Avri Altman <avri.altman@wdc.com>
_______________________________________________
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: config: Add Realtek RTD16XX defconfig
From: Arnd Bergmann @ 2019-09-05 8:46 UTC (permalink / raw)
To: jamestai.sky
Cc: james.tai, CY_Huang, linux-kernel@vger.kernel.org, Russell King,
Phinex Hung, Linux ARM
In-Reply-To: <20190905081140.1428-1-james.tai@realtek.com>
On Thu, Sep 5, 2019 at 10:14 AM <jamestai.sky@gmail.com> wrote:
>
> From: "james.tai" <james.tai@realtek.com>
>
> Add a defconfig for Realtek RTD16XX platform.
>
> Signed-off-by: james.tai <james.tai@realtek.com>
> ---
> arch/arm/configs/rtd16xx_defconfig | 427 +++++++++++++++++++++++++++++
We usually try to have one defconfig per vendor. Expecting that
there will be other Realtek SoCs in the future that we may add
here, I would name this 'rtd_defconfig' or 'realtek_defconfig'.
Please also add the set of options you want to multi_v7_defconfig
so you are able to boot with that.
> 1 file changed, 427 insertions(+)
> create mode 100644 arch/arm/configs/rtd16xx_defconfig
>
> diff --git a/arch/arm/configs/rtd16xx_defconfig b/arch/arm/configs/rtd16xx_defconfig
> new file mode 100644
> index 000000000000..49bcbe6c6af8
> --- /dev/null
> +++ b/arch/arm/configs/rtd16xx_defconfig
> @@ -0,0 +1,427 @@
> +CONFIG_SYSVIPC=y
> +CONFIG_NO_HZ=y
> +CONFIG_HIGH_RES_TIMERS=y
> +CONFIG_CGROUPS=y
> +CONFIG_BLK_DEV_INITRD=y
> +CONFIG_EMBEDDED=y
I normally would not turn on CONFIG_EMBEDDED, this is only
needed to change some rare options.
> +CONFIG_PERF_EVENTS=y
> +CONFIG_ARCH_REALTEK=y
> +CONFIG_ARCH_RTD16XX=y
> +CONFIG_ARM_THUMBEE=y
ThumbEE is deprecated in ARMv8, and one usually should not
rely on it. If you don't actually need it, just turn it off.
(note: this is unrelated to regular thumb execution, which
is enabled by default)
> +# CONFIG_CACHE_L2X0 is not set
> +# CONFIG_ARM_ERRATA_643719 is not set
> +CONFIG_ARM_ERRATA_814220=y
> +CONFIG_SMP=y
> +CONFIG_SCHED_MC=y
> +CONFIG_SCHED_SMT=y
If you don't have SMT in the CPU, there is no need ot enable this.
> +CONFIG_HAVE_ARM_ARCH_TIMER=y
> +CONFIG_MCPM=y
> +CONFIG_NR_CPUS=6
> +CONFIG_HZ_250=y
> +CONFIG_OABI_COMPAT=y
It seems unlikely you want OABI_COMPAT
> +CONFIG_HIGHMEM=y
> +CONFIG_FORCE_MAX_ZONEORDER=12
> +CONFIG_SECCOMP=y
> +CONFIG_ARM_APPENDED_DTB=y
> +CONFIG_ARM_ATAG_DTB_COMPAT=y
> +CONFIG_KEXEC=y
> +CONFIG_EFI=y
What method do you actually use for booting? New platforms
should generally not require CONFIG_ARM_APPENDED_DTB
or CONFIG_ARM_ATAG_DTB_COMPAT, and I suspect you
don't use EFI.
> +CONFIG_CPUFREQ_DT=y
> +CONFIG_QORIQ_CPUFREQ=y
QORIQ_CPUFREQ is a platform specific option that you
won't need.
> +CONFIG_NET_DSA=m
> +CONFIG_CAN=y
> +CONFIG_CAN_FLEXCAN=m
> +CONFIG_CAN_RCAR=m
> +CONFIG_BT=m
> +CONFIG_BT_HCIUART=m
> +CONFIG_BT_HCIUART_BCM=y
> +CONFIG_BT_MRVL=m
> +CONFIG_BT_MRVL_SDIO=m
Many more hardware specific drivers here that you should turn off
> +CONFIG_MTD=y
> +CONFIG_MTD_CMDLINE_PARTS=y
> +CONFIG_MTD_BLOCK=y
> +CONFIG_MTD_CFI=y
> +CONFIG_MTD_PHYSMAP=y
> +CONFIG_MTD_PHYSMAP_OF=y
> +CONFIG_MTD_RAW_NAND=y
> +CONFIG_MTD_NAND_DENALI_DT=y
> +CONFIG_MTD_NAND_BRCMNAND=y
and here.
> +CONFIG_BLK_DEV_LOOP=y
> +CONFIG_BLK_DEV_RAM=y
> +CONFIG_BLK_DEV_RAM_SIZE=65536
Do you require BLK_DEV_RAM for initrd? Normally one uses
initramfs instead or tmpfs instead.
> +# CONFIG_NET_VENDOR_3COM is not set
> +# CONFIG_NET_VENDOR_ADAPTEC is not set
> +# CONFIG_NET_VENDOR_AGERE is not set
> +# CONFIG_NET_VENDOR_ALACRITECH is not set
> +# CONFIG_NET_VENDOR_ALTEON is not set
I would trim the list here, just leave all network device vendors
enabled, they don't hurt.
> +CONFIG_USB_PEGASUS=y
> +CONFIG_USB_RTL8152=m
> +CONFIG_USB_LAN78XX=m
> +CONFIG_USB_USBNET=y
> +CONFIG_USB_NET_SMSC75XX=y
> +CONFIG_USB_NET_SMSC95XX=y
> +CONFIG_BRCMFMAC=m
> +CONFIG_MWIFIEX=m
> +CONFIG_MWIFIEX_SDIO=m
> +CONFIG_RT2X00=m
> +CONFIG_RT2800USB=m
Do you need all of the above? It's no problem to enable
them if you do, it just seems unusual.
Arnd
_______________________________________________
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Heinrich Schuchardt @ 2019-09-05 8:48 UTC (permalink / raw)
To: Peter Maydell, Marc Zyngier
Cc: Daniel P . Berrangé, Suzuki K Pouloze, Julien Thierry,
lkml - Kernel Mailing List, James Morse, Stefan Hajnoczi, kvmarm,
arm-mail-list
In-Reply-To: <CAFEAcA-mc6cLmRGdGNOBR0PC1f_VBjvTdAL6xYtKjApx3NoPgQ@mail.gmail.com>
On 9/5/19 10:16 AM, Peter Maydell wrote:
> On Thu, 5 Sep 2019 at 09:04, Marc Zyngier <maz@kernel.org> wrote:
>> How can you tell that the access would fault? You have no idea at that
>> stage (the kernel doesn't know about the MMIO ranges that userspace
>> handles). All you know is that you're faced with a memory access that
>> you cannot emulate in the kernel. Injecting a data abort at that stage
>> is not something that the architecture allows.
>
> To be fair, locking up the whole CPU (which is effectively
> what the kvm_err/ENOSYS is going to do to the VM) isn't
> something the architecture allows either :-)
>
>> Of course, the best thing would be to actually fix the guest so that
>> it doesn't use non-emulatable MMIO accesses. In general, that the sign
>> of a bug in low-level accessors.
>
> This is true, but the problem is that barfing out to userspace
> makes it harder to debug the guest because it means that
> the VM is immediately destroyed, whereas AIUI if we
> inject some kind of exception then (assuming you're set up
> to do kernel-debug via gdbstub) you can actually examine
> the offending guest code with a debugger because at least
> your VM is still around to inspect...
Stopping the CPU and debugging is not what I am interested in. I want
the QEMU guest to be able to react to an incorrect memory access.
Imagine Apollo 11's computer not restarting when hitting an exception.
They would never have reached the moon. - I think allowing an emulation
guest to react to an exception, e.g. by resetting, is a necessity.
In my case U-Boot as a guest creates an output like the one below when a
data abort occurs:
"Synchronous Abort" handler, esr 0x02000000
elr: fffffffffdeac19c lr : fffffffffdeac19c (reloc)
elr: 000000007ddd719c lr : 000000007ddd719c
x0 : 0000000000000000 x1 : 000000007ffbc000
x2 : 000000000000000a x3 : 000000007ffbcd80
x4 : 0000000000002800 x5 : 000000007ffbcdb0
x6 : 0000000000000001 x7 : 000000007eef8b80
x8 : 000000000000003f x9 : 0000000000000004
x10: 0000000000000001 x11: 000000000000000d
x12: 0000000000000006 x13: 000000000001869f
x14: 0000000047f00000 x15: 0000000000000000
x16: 000000007ff5b194 x17: 0000000000000000
x18: 0000000000000000 x19: 000000007ffbcd30
x20: 0000000000000000 x21: 000000007ffeb000
x22: 0000000000000009 x23: 000000007eef5cf0
x24: 0000000000000000 x25: 000000007ffa7806
x26: 000000007ffa7834 x27: 0000000000000024
x28: 000000007dddd040 x29: 000000007ede9990
UEFI image [0x000000007ddd7000:0x000000007ddd749f] pc=0x19c '/bug.efi'
Resetting CPU ...
With this information I see that the problem occurred at 0x019C from the
start of the loaded binary bug.efi. Next thing is to look at the map
file of bug.efi to find out in which instruction the problem occurred.
After providing the dump U-Boot continues to reset the system.
When U-Boot is running the EDK II SCT (a test suite for UEFI firmware),
SCT will log that a restart occurred (indicating that a test failed) and
continue to run the next test.
Best regards
Heinrich
_______________________________________________
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Marc Zyngier @ 2019-09-05 8:52 UTC (permalink / raw)
To: Peter Maydell
Cc: "Daniel P . Berrangé", Suzuki K Pouloze,
Heinrich Schuchardt, Julien Thierry, lkml - Kernel Mailing List,
James Morse, Stefan Hajnoczi, kvmarm, arm-mail-list
In-Reply-To: <CAFEAcA-mc6cLmRGdGNOBR0PC1f_VBjvTdAL6xYtKjApx3NoPgQ@mail.gmail.com>
On Thu, 05 Sep 2019 09:16:54 +0100,
Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 5 Sep 2019 at 09:04, Marc Zyngier <maz@kernel.org> wrote:
> > How can you tell that the access would fault? You have no idea at that
> > stage (the kernel doesn't know about the MMIO ranges that userspace
> > handles). All you know is that you're faced with a memory access that
> > you cannot emulate in the kernel. Injecting a data abort at that stage
> > is not something that the architecture allows.
>
> To be fair, locking up the whole CPU (which is effectively
> what the kvm_err/ENOSYS is going to do to the VM) isn't
> something the architecture allows either :-)
Hey, I didn't say things were good as they are now! ;-)
I'm definitely willing to change things in that area, but I also don't
want anyone to start relying on things that are not specified anywhere.
>
> > Of course, the best thing would be to actually fix the guest so that
> > it doesn't use non-emulatable MMIO accesses. In general, that the sign
> > of a bug in low-level accessors.
>
> This is true, but the problem is that barfing out to userspace
> makes it harder to debug the guest because it means that
> the VM is immediately destroyed, whereas AIUI if we
> inject some kind of exception then (assuming you're set up
> to do kernel-debug via gdbstub) you can actually examine
> the offending guest code with a debugger because at least
> your VM is still around to inspect...
To Christoffer's point, I find the benefit a bit dubious. Yes, you get
an exception, but the instruction that caused it may be completely
legal (store with post-increment, for example), leading to an even
more puzzled developer (that exception should never have been
delivered the first place).
I'm far more in favour of dumping the state of the access in the run
structure (much like we do for a MMIO access) and let userspace do
something about it (such as dumping information on the console or
breaking). It could even inject an exception *if* the user has asked
for it.
Thanks,
M.
--
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: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Tomasz Figa @ 2019-09-05 8:52 UTC (permalink / raw)
To: Jerry-ch Chen
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
laurent.pinchart+renesas@ideasonboard.com,
Rynn Wu (吳育恩), srv_heupstream,
Po-Yang Huang (黃柏陽), mchehab@kernel.org,
suleiman@chromium.org, shik@chromium.org,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
linux-mediatek@lists.infradead.org, zwisler@chromium.org,
matthias.bgg@gmail.com, Christie Yu (游雅惠),
Frederic Chen (陳俊元), hans.verkuil@cisco.com,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <1567671418.22453.41.camel@mtksdccf07>
On Thu, Sep 5, 2019 at 5:17 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
>
> Hi Tomasz,
>
> On Thu, 2019-09-05 at 15:13 +0800, Tomasz Figa wrote:
> > On Thu, Sep 5, 2019 at 4:02 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Wed, 2019-09-04 at 21:19 +0800, Jerry-ch Chen wrote:
> > > > On Wed, 2019-09-04 at 21:12 +0800, Tomasz Figa wrote:
> > > > > On Wed, Sep 4, 2019 at 6:26 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > >
> > > > > > Hi Tomasz,
> > > > > >
> > > > > > On Wed, 2019-09-04 at 17:03 +0800, Tomasz Figa wrote:
> > > > > > > On Wed, Sep 4, 2019 at 6:02 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > >
> > > > > > > > Hi Tomasz,
> > > > > > > >
> > > > > > > > On Wed, 2019-09-04 at 16:25 +0800, Tomasz Figa wrote:
> > > > > > > > > On Wed, Sep 4, 2019 at 5:09 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Tomasz,
> > > > > > > > > >
> > > > > > > > > > On Wed, 2019-09-04 at 14:34 +0800, Tomasz Figa wrote:
> > > > > > > > > > > On Wed, Sep 4, 2019 at 3:09 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, 2019-09-04 at 12:15 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > On Wed, Sep 4, 2019 at 12:38 PM Jerry-ch Chen
> > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Tue, 2019-09-03 at 20:05 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 8:46 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 15:04 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > > > On Tue, Sep 3, 2019 at 3:44 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > On Tue, 2019-09-03 at 13:19 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > > > > > On Mon, Sep 2, 2019 at 8:47 PM Jerry-ch Chen <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-30 at 16:33 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > > > > > > > On Wed, Aug 28, 2019 at 11:00 AM Jerry-ch Chen
> > > > > > > > > > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > On Mon, 2019-08-26 at 14:36 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > > > > > > > > > Hi Jerry,
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > On Sun, Aug 25, 2019 at 6:18 PM Jerry-ch Chen
> > > > > > > > > > > > > > > > > > > > > > > <Jerry-ch.Chen@mediatek.com> wrote:
> > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > Hi Tomasz,
> > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > On Fri, 2019-08-02 at 16:28 +0800, Tomasz Figa wrote:
> > > > > > > > > > > > > > > > > > > > > > > > > Hi Jerry,
> > > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > > On Tue, Jul 09, 2019 at 04:41:12PM +0800, Jerry-ch Chen wrote:
> > > > > > [snip]
> > > > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > > > > > > > > {
> > > > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > > > > > struct vb2_v4l2_buffer *vb;
> > > > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx;
> > > > > > > > > > u32 ret;
> > > > > > > > > >
> > > > > > > > > > if (!fd->fd_irq_done.done)
> > > > > > > > >
> > > > > > > > > We shouldn't access internal fields of completion.
> > > > > > > > >
> > > > > > > > > > ret = wait_for_completion_timeout(&fd->fd_irq_done,
> > > > > > > > > > msecs_to_jiffies(
> > > > > > > > > > MTK_FD_HW_TIMEOUT));
> > > > > > > > > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ?
> > > > > > > > > > &m2m_ctx->out_q_ctx :
> > > > > > > > > > &m2m_ctx->cap_q_ctx;
> > > > > > > > > > while ((vb = v4l2_m2m_buf_remove(queue_ctx)))
> > > > > > > > > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > > > > > > > > >
> > > > > > > > > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> > > > > > > > > > mtk_fd_hw_disconnect(fd);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > I've also tried to wait completion unconditionally for both queues and
> > > > > > > > > > the second time will wait until timeout, as a result, it takes longer to
> > > > > > > > > > swap the camera every time and close the camera app.
> > > > > > > > >
> > > > > > > > > I think it should work better if we call complete_all() instead of complete().
> > > > > > > > >
> > > > > > > > Thanks,
> > > > > > > >
> > > > > > > > I use complete_all(), and it works fine now.
> > > > > > > >
> > > > > > > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > > > > > > {
> > > > > > > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > > > > > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > > > > > > struct vb2_v4l2_buffer *vb;
> > > > > > > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > > > > > > struct v4l2_m2m_queue_ctx *queue_ctx;
> > > > > > > >
> > > > > > > > wait_for_completion_timeout(&fd->fd_irq_done,
> > > > > > > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT));
> > > > > > >
> > > > > > > Shouldn't we still send some command to the hardware to stop? Like a
> > > > > > > reset. Otherwise we don't know if it isn't still accessing the memory.
> > > > > > >
> > > > > > I thought no more jobs will be enqueued here when stop_streaming so we
> > > > > > don't need it.
> > > > >
> > > > > That's true for the case when the wait completed successfully, but we
> > > > > also need to ensure the hardware is stopped even if a timeout happens.
> > > > >
> > > > > > We still could send an ipi command to reset the HW, and wait for it's
> > > > > > callback or we could set the register MTK_FD_REG_OFFSET_HW_ENABLE to
> > > > > > zero to disable the HW.
> > > > >
> > > > > Since it's for handling a timeout, a reset should be more likely to
> > > > > bring the hardware back to a reasonable state.
> > > > >
> > > >
> > > > Ok, I will send the ipi command to reset the HW.
> > > >
> > > > Thanks and best regards,
> > > > Jerry
> > > I've tested and will refine as following:
> > >
> > > static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> > > {
> > > struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> > > struct mtk_fd_dev *fd = ctx->fd_dev;
> > > struct vb2_v4l2_buffer *vb;
> > > struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> > > struct v4l2_m2m_queue_ctx *queue_ctx;
> > > u32 ret;
> > >
> > > ret = wait_for_completion_timeout(&fd->fd_irq_done,
> > > msecs_to_jiffies(MTK_FD_HW_TIMEOUT));
> > > /* Disable FD HW */
> > > if(!ret) {
> > > struct ipi_message fd_ipi_msg;
> > >
> > > fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET;
> > > ret = scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> > > sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT);
> > > if (ret)
> > > dev_err(fd->dev, "FD Reset HW error\n");
> > > }
> >
> > Would you also put the same code in suspend handler? If so, perhaps
> > it's better to keep this in a helper function (mtk_fd_job_abort()) as
> > we had before?
> >
>
> Ok, done, It will reset the HW and return ETIMEOUT if the last job is
> timeout, the return value will be used in suspend for further action.
>
> static int mtk_fd_job_abort(struct mtk_fd_dev *fd)
> {
> u32 ret;
>
> ret = wait_for_completion_timeout(&fd->fd_irq_done,
> msecs_to_jiffies(MTK_FD_HW_TIMEOUT));
> /* Reset FD HW */
> if (!ret) {
> struct ipi_message fd_ipi_msg;
>
> fd_ipi_msg.cmd_id = MTK_FD_IPI_CMD_RESET;
> if (scp_ipi_send(fd->scp_pdev, SCP_IPI_FD_CMD, &fd_ipi_msg,
> sizeof(fd_ipi_msg), MTK_FD_IPI_SEND_TIMEOUT))
> dev_err(fd->dev, "FD Reset HW error\n");
> return -ETIMEDOUT;
> }
> return 0;
> }
>
> static void mtk_fd_vb2_stop_streaming(struct vb2_queue *vq)
> {
> struct mtk_fd_ctx *ctx = vb2_get_drv_priv(vq);
> struct mtk_fd_dev *fd = ctx->fd_dev;
> struct vb2_v4l2_buffer *vb;
> struct v4l2_m2m_ctx *m2m_ctx = ctx->fh.m2m_ctx;
> struct v4l2_m2m_queue_ctx *queue_ctx;
>
> mtk_fd_job_abort(fd);
> queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ?
> &m2m_ctx->out_q_ctx :
> &m2m_ctx->cap_q_ctx;
> while ((vb = v4l2_m2m_buf_remove(queue_ctx)))
> v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
>
> if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> mtk_fd_hw_disconnect(fd);
> }
>
> static int mtk_fd_suspend(struct device *dev)
> {
> struct mtk_fd_dev *fd = dev_get_drvdata(dev);
>
> if (pm_runtime_suspended(dev))
> return 0;
>
> if (fd->fd_stream_count)
> if (mtk_fd_job_abort(fd))
> mtk_fd_hw_job_finish(fd, VB2_BUF_STATE_ERROR);
Wouldn't this cause the next job to be run?
>
> /* suspend FD HW */
> writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_INT_EN);
> writel(0x0, fd->fd_base + MTK_FD_REG_OFFSET_HW_ENABLE);
> clk_disable_unprepare(fd->fd_clk);
> dev_dbg(dev, "%s:disable clock\n", __func__);
>
> return 0;
> }
>
> > > queue_ctx = V4L2_TYPE_IS_OUTPUT(vq->type) ?
> > > &m2m_ctx->out_q_ctx :
> > > &m2m_ctx->cap_q_ctx;
> > > while ((vb = v4l2_m2m_buf_remove(queue_ctx)))
> > > v4l2_m2m_buf_done(vb, VB2_BUF_STATE_ERROR);
> > >
> > > if (vq->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
> > > mtk_fd_hw_disconnect(fd);
> > > }
> > >
> > > If there is no other concern, may I send the RFC v3 patch for review?
> >
> > Thanks, technically it looks good now. Just one comment about avoiding
> > code duplication above.
> >
>
> Thanks,
>
> I will send the v3 if the above fix-up is accepted,
I think there is a bigger issue here actually, related to how the m2m
helpers work. Let's just keep the code as you proposed and post v3.
We can continue the discussion there.
Best regards,
Tomasz
_______________________________________________
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Peter Maydell @ 2019-09-05 8:56 UTC (permalink / raw)
To: Marc Zyngier
Cc: Daniel P . Berrangé, Suzuki K Pouloze, Heinrich Schuchardt,
Julien Thierry, lkml - Kernel Mailing List, James Morse,
Stefan Hajnoczi, kvmarm, arm-mail-list
In-Reply-To: <86mufjrup7.wl-maz@kernel.org>
On Thu, 5 Sep 2019 at 09:52, Marc Zyngier <maz@kernel.org> wrote:
>
> On Thu, 05 Sep 2019 09:16:54 +0100,
> Peter Maydell <peter.maydell@linaro.org> wrote:
> > This is true, but the problem is that barfing out to userspace
> > makes it harder to debug the guest because it means that
> > the VM is immediately destroyed, whereas AIUI if we
> > inject some kind of exception then (assuming you're set up
> > to do kernel-debug via gdbstub) you can actually examine
> > the offending guest code with a debugger because at least
> > your VM is still around to inspect...
>
> To Christoffer's point, I find the benefit a bit dubious. Yes, you get
> an exception, but the instruction that caused it may be completely
> legal (store with post-increment, for example), leading to an even
> more puzzled developer (that exception should never have been
> delivered the first place).
Right, but the combination of "host kernel prints a message
about an unsupported load/store insn" and "within-guest debug
dump/stack trace/etc" is much more useful than just having
"host kernel prints message" and "QEMU exits"; and it requires
about 3 lines of code change...
> I'm far more in favour of dumping the state of the access in the run
> structure (much like we do for a MMIO access) and let userspace do
> something about it (such as dumping information on the console or
> breaking). It could even inject an exception *if* the user has asked
> for it.
...whereas this requires agreement on a kernel-userspace API,
larger changes in the kernel, somebody to implement the userspace
side of things, and the user to update both the kernel and QEMU.
It's hard for me to see that the benefit here over the 3-line
approach really outweighs the extra effort needed. In practice
saying "we should do this" is saying "we're going to do nothing",
based on the historical record.
thanks
-- PMM
_______________________________________________
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 1/1] mm/pgtable/debug: Add test validating architecture page table helpers
From: Kirill A. Shutemov @ 2019-09-05 8:59 UTC (permalink / raw)
To: Anshuman Khandual
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Tetsuo Handa, Heiko Carstens, Michal Hocko, linux-mm, Dave Hansen,
Paul Mackerras, sparclinux, Thomas Gleixner, linux-s390,
Michael Ellerman, x86, Russell King - ARM Linux, Matthew Wilcox,
Steven Price, Jason Gunthorpe, linux-arm-kernel, linux-snps-arc,
Kees Cook, Masahiro Yamada, Mark Brown, Dan Williams,
Vlastimil Babka, Sri Krishna chowdary, Ard Biesheuvel,
Greg Kroah-Hartman, linux-mips, Ralf Baechle, linux-kernel,
Paul Burton, Mike Rapoport, Vineet Gupta, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <6d4b989d-8eaa-d26e-6068-4b0e4d7a52f9@arm.com>
On Thu, Sep 05, 2019 at 01:48:27PM +0530, Anshuman Khandual wrote:
> >> +#define VADDR_TEST (PGDIR_SIZE + PUD_SIZE + PMD_SIZE + PAGE_SIZE)
> >
> > What is special about this address? How do you know if it is not occupied
> > yet?
>
> We are creating the page table from scratch after allocating an mm_struct
> for a given random virtual address 'VADDR_TEST'. Hence nothing is occupied
> just yet. There is nothing special about this address, just that it tries
> to ensure the page table entries are being created with some offset from
> beginning of respective page table page at all levels ? The idea is to
> have a more representative form of page table structure for test.
Why P4D_SIZE is missing?
Are you sure it will not land into kernel address space on any arch?
I think more robust way to deal with this would be using
get_unmapped_area() instead of fixed address.
> This makes sense for runtime cases but there is a problem here.
>
> On arm64, pgd_populate() which takes (pud_t *) as last argument instead of
> (p4d_t *) will fail to build when not wrapped in !__PAGETABLE_P4D_FOLDED
> on certain configurations.
>
> ./arch/arm64/include/asm/pgalloc.h:81:75: note:
> expected ‘pud_t *’ {aka ‘struct <anonymous> *’}
> but argument is of type ‘pgd_t *’ {aka ‘struct <anonymous> *’}
> static inline void pgd_populate(struct mm_struct *mm, pgd_t *pgdp, pud_t *pudp)
> ~~~~~~~^~~~
> Wondering if this is something to be fixed on arm64 or its more general
> problem. Will look into this further.
I think you need wrap this into #ifndef __ARCH_HAS_5LEVEL_HACK.
> >> + pmd_populate_tests(mm, pmdp, (pgtable_t) page);
> >
> > This is not correct for architectures that defines pgtable_t as pte_t
> > pointer, not struct page pointer.
>
> Right, a grep on the source confirms that.
>
> These platforms define pgtable_t as struct page *
>
> arch/alpha/include/asm/page.h:typedef struct page *pgtable_t;
> arch/arm/include/asm/page.h:typedef struct page *pgtable_t;
> arch/arm64/include/asm/page.h:typedef struct page *pgtable_t;
> arch/csky/include/asm/page.h:typedef struct page *pgtable_t;
> arch/hexagon/include/asm/page.h:typedef struct page *pgtable_t;
> arch/ia64/include/asm/page.h: typedef struct page *pgtable_t;
> arch/ia64/include/asm/page.h: typedef struct page *pgtable_t;
> arch/m68k/include/asm/page.h:typedef struct page *pgtable_t;
> arch/microblaze/include/asm/page.h:typedef struct page *pgtable_t;
> arch/mips/include/asm/page.h:typedef struct page *pgtable_t;
> arch/nds32/include/asm/page.h:typedef struct page *pgtable_t;
> arch/nios2/include/asm/page.h:typedef struct page *pgtable_t;
> arch/openrisc/include/asm/page.h:typedef struct page *pgtable_t;
> arch/parisc/include/asm/page.h:typedef struct page *pgtable_t;
> arch/riscv/include/asm/page.h:typedef struct page *pgtable_t;
> arch/sh/include/asm/page.h:typedef struct page *pgtable_t;
> arch/sparc/include/asm/page_32.h:typedef struct page *pgtable_t;
> arch/um/include/asm/page.h:typedef struct page *pgtable_t;
> arch/unicore32/include/asm/page.h:typedef struct page *pgtable_t;
> arch/x86/include/asm/pgtable_types.h:typedef struct page *pgtable_t;
> arch/xtensa/include/asm/page.h:typedef struct page *pgtable_t;
>
> These platforms define pgtable_t as pte_t *
>
> arch/arc/include/asm/page.h:typedef pte_t * pgtable_t;
> arch/powerpc/include/asm/mmu.h:typedef pte_t *pgtable_t;
> arch/s390/include/asm/page.h:typedef pte_t *pgtable_t;
> arch/sparc/include/asm/page_64.h:typedef pte_t *pgtable_t;
>
> Should we need have two pmd_populate_tests() definitions with
> different arguments (struct page pointer or pte_t pointer) and then
> call either one after detecting the given platform ?
Use pte_alloc_one() instead of alloc_mapped_page() to allocate the page
table.
> >> + pud_populate_tests(mm, pudp, pmdp);
> >> + p4d_populate_tests(mm, p4dp, pudp);
> >> + pgd_populate_tests(mm, pgdp, p4dp);
> >
> > This is wrong. All p?dp points to the second entry in page table entry.
> > This is not valid pointer for page table and triggers p?d_bad() on x86.
>
> Yeah these are second entries because of the way we create the page table.
> But I guess its applicable only to the second argument in all these above
> cases because the first argument can be any valid entry on previous page
> table level.
Yes:
@@ -397,9 +396,9 @@ static int __init arch_pgtable_tests_init(void)
pgd_clear_tests(pgdp);
pmd_populate_tests(mm, pmdp, (pgtable_t) page);
- pud_populate_tests(mm, pudp, pmdp);
- p4d_populate_tests(mm, p4dp, pudp);
- pgd_populate_tests(mm, pgdp, p4dp);
+ pud_populate_tests(mm, pudp, saved_pmdp);
+ p4d_populate_tests(mm, p4dp, saved_pudp);
+ pgd_populate_tests(mm, pgdp, saved_p4dp);
p4d_free(mm, saved_p4dp);
pud_free(mm, saved_pudp);
--
Kirill A. Shutemov
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [GIT PULL] Renesas ARM Based SoC Fixes for v5.3
From: Simon Horman @ 2019-09-05 9:08 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Kevin Hilman, Magnus Damm, Linux-Renesas, arm-soc, Olof Johansson,
Linux ARM
In-Reply-To: <CAK8P3a0rQgEj9gQh-jyPOtoj+QVT2eeXz-vF0v5aKfnzWXP35g@mail.gmail.com>
On Wed, Sep 04, 2019 at 02:40:36PM +0200, Arnd Bergmann wrote:
> On Fri, Aug 16, 2019 at 3:33 PM Simon Horman <horms+renesas@verge.net.au> wrote:
> >
> > are available in the git repository at:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas.git tags/renesas-fixes-for-v5.3
> >
> > for you to fetch changes up to 45f5d5a9e34d3fe4140a9a3b5f7ebe86c252440a:
> >
> > arm64: dts: renesas: r8a77995: draak: Fix backlight regulator name (2019-08-09 11:58:17 -0700)
> >
> > ----------------------------------------------------------------
> > Renesas ARM Based SoC Fixes for v5.3
> >
> > * R-Car D3 (r8a77995) based Draak Board
> > - Correct backlight regulator name in device tree
>
> I just found this pull request in the arm@kernel.org inbox, sorry for missing
> it earlier. The 5.4 pull requests that Geert sent in the meantime are all
> merged as they went to the new soc@kernel.org address.
>
> Pulled this now into arm/fixes.
Thanks, and sorry for using the old email address.
I do have one more fix which I plan to post later today.
_______________________________________________
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] arm64: dts: mt8183: Add node for the Mali GPU
From: Rob Herring @ 2019-09-05 9:09 UTC (permalink / raw)
To: Nicolas Boichat
Cc: Mark Rutland, devicetree, linux-kernel@vger.kernel.org,
Boris Brezillon, moderated list:ARM/Mediatek SoC support,
Alyssa Rosenzweig, Matthias Brugger, Nick Fan,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20190905081546.42716-1-drinkcat@chromium.org>
On Thu, Sep 5, 2019 at 9:16 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> Add a basic GPU node and opp table for mt8183.
>
> The binding we use with out-of-tree Mali drivers includes more
> clocks, I assume this would be required eventually if we have an
> in-tree driver:
We have an in-tree driver...
> clocks =
> <&topckgen CLK_TOP_MFGPLL_CK>,
> <&topckgen CLK_TOP_MUX_MFG>,
> <&clk26m>,
> <&mfgcfg CLK_MFG_BG3D>;
> clock-names =
> "clk_main_parent",
> "clk_mux",
> "clk_sub_parent",
> "subsys_mfg_cg";
>
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>
> ---
> Upstreaming what matches existing bindings from our Chromium OS tree:
> https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.19/arch/arm64/boot/dts/mediatek/mt8183.dtsi#1348
>
> The evb part of this change depends on this patch to add PMIC dtsi:
> https://patchwork.kernel.org/patch/10928161/
>
> arch/arm64/boot/dts/mediatek/mt8183-evb.dts | 7 ++
> arch/arm64/boot/dts/mediatek/mt8183.dtsi | 103 ++++++++++++++++++++
> 2 files changed, 110 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> index 1fb195c683c3d01..200d8e65a6368a1 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> +++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
> @@ -7,6 +7,7 @@
>
> /dts-v1/;
> #include "mt8183.dtsi"
> +#include "mt6358.dtsi"
>
> / {
> model = "MediaTek MT8183 evaluation board";
> @@ -30,6 +31,12 @@
> status = "okay";
> };
>
> +&gpu {
> + supply-names = "mali", "mali_sram";
> + mali-supply = <&mt6358_vgpu_reg>;
> + mali_sram-supply = <&mt6358_vsram_gpu_reg>;
Not documented. Just 'sram-supply' is enough.
Note that the binding doc queued up for 5.4 has been converted to DT schema.
> +};
> +
> &i2c0 {
> pinctrl-names = "default";
> pinctrl-0 = <&i2c_pins_0>;
> diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> index 97f84aa9fc6e1c1..8ea548a762ea252 100644
> --- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> +++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
> @@ -579,6 +579,109 @@
> #clock-cells = <1>;
> };
>
> + gpu: mali@13040000 {
gpu@...
> + compatible = "mediatek,mt8183-mali", "arm,mali-bifrost";
You need to add this compatible string too.
> + reg = <0 0x13040000 0 0x4000>;
> + interrupts =
> + <GIC_SPI 280 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 279 IRQ_TYPE_LEVEL_LOW>,
> + <GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
> + interrupt-names = "job", "mmu", "gpu";
> +
> + clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
> + power-domains =
> + <&scpsys MT8183_POWER_DOMAIN_MFG_CORE0>,
> + <&scpsys MT8183_POWER_DOMAIN_MFG_CORE1>,
> + <&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
This needs to be documented too.
> +
> + operating-points-v2 = <&gpu_opp_table>;
> + };
> +
> + gpu_opp_table: opp_table0 {
> + compatible = "operating-points-v2";
> + opp-shared;
> +
> + opp-300000000 {
> + opp-hz = /bits/ 64 <300000000>;
> + opp-microvolt = <625000>, <850000>;
> + };
> +
> + opp-320000000 {
> + opp-hz = /bits/ 64 <320000000>;
> + opp-microvolt = <631250>, <850000>;
> + };
> +
> + opp-340000000 {
> + opp-hz = /bits/ 64 <340000000>;
> + opp-microvolt = <637500>, <850000>;
> + };
> +
> + opp-360000000 {
> + opp-hz = /bits/ 64 <360000000>;
> + opp-microvolt = <643750>, <850000>;
> + };
> +
> + opp-380000000 {
> + opp-hz = /bits/ 64 <380000000>;
> + opp-microvolt = <650000>, <850000>;
> + };
> +
> + opp-400000000 {
> + opp-hz = /bits/ 64 <400000000>;
> + opp-microvolt = <656250>, <850000>;
> + };
> +
> + opp-420000000 {
> + opp-hz = /bits/ 64 <420000000>;
> + opp-microvolt = <662500>, <850000>;
> + };
> +
> + opp-460000000 {
> + opp-hz = /bits/ 64 <460000000>;
> + opp-microvolt = <675000>, <850000>;
> + };
> +
> + opp-500000000 {
> + opp-hz = /bits/ 64 <500000000>;
> + opp-microvolt = <687500>, <850000>;
> + };
> +
> + opp-540000000 {
> + opp-hz = /bits/ 64 <540000000>;
> + opp-microvolt = <700000>, <850000>;
> + };
> +
> + opp-580000000 {
> + opp-hz = /bits/ 64 <580000000>;
> + opp-microvolt = <712500>, <850000>;
> + };
> +
> + opp-620000000 {
> + opp-hz = /bits/ 64 <620000000>;
> + opp-microvolt = <725000>, <850000>;
> + };
> +
> + opp-653000000 {
> + opp-hz = /bits/ 64 <653000000>;
> + opp-microvolt = <743750>, <850000>;
> + };
> +
> + opp-698000000 {
> + opp-hz = /bits/ 64 <698000000>;
> + opp-microvolt = <768750>, <868750>;
> + };
> +
> + opp-743000000 {
> + opp-hz = /bits/ 64 <743000000>;
> + opp-microvolt = <793750>, <893750>;
> + };
> +
> + opp-800000000 {
> + opp-hz = /bits/ 64 <800000000>;
> + opp-microvolt = <825000>, <925000>;
> + };
Okay, but I seriously doubt the OPP selection logic is sophisticated
enough or will ever be to use all these levels...
> + };
> +
> mmsys: syscon@14000000 {
> compatible = "mediatek,mt8183-mmsys", "syscon";
> reg = <0 0x14000000 0 0x1000>;
> --
> 2.23.0.187.g17f5b7556c-goog
>
_______________________________________________
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 v12 01/12] lib: introduce copy_struct_{to, from}_user helpers
From: Andreas Schwab @ 2019-09-05 9:09 UTC (permalink / raw)
To: Aleksa Sarai
Cc: linux-ia64, linux-sh, Peter Zijlstra, Rasmus Villemoes,
Alexei Starovoitov, linux-kernel, David Howells, linux-kselftest,
sparclinux, Shuah Khan, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Jiri Olsa, Alexander Shishkin, Ingo Molnar,
linux-arm-kernel, linux-mips, linux-xtensa, Kees Cook,
Arnd Bergmann, Jann Horn, linuxppc-dev, linux-m68k, Al Viro,
Andy Lutomirski, Shuah Khan, Namhyung Kim, David Drysdale,
Christian Brauner, J. Bruce Fields, linux-parisc, linux-api,
Chanho Min, Jeff Layton, Oleg Nesterov, Eric Biederman,
linux-alpha, linux-fsdevel, Andrew Morton, Linus Torvalds,
containers
In-Reply-To: <20190904201933.10736-2-cyphar@cyphar.com>
On Sep 05 2019, Aleksa Sarai <cyphar@cyphar.com> wrote:
> diff --git a/lib/struct_user.c b/lib/struct_user.c
> new file mode 100644
> index 000000000000..7301ab1bbe98
> --- /dev/null
> +++ b/lib/struct_user.c
> @@ -0,0 +1,182 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2019 SUSE LLC
> + * Copyright (C) 2019 Aleksa Sarai <cyphar@cyphar.com>
> + */
> +
> +#include <linux/types.h>
> +#include <linux/export.h>
> +#include <linux/uaccess.h>
> +#include <linux/kernel.h>
> +#include <linux/string.h>
> +
> +#define BUFFER_SIZE 64
> +
> +/*
> + * "memset(p, 0, size)" but for user space buffers. Caller must have already
> + * checked access_ok(p, size).
> + */
> +static int __memzero_user(void __user *p, size_t s)
> +{
> + const char zeros[BUFFER_SIZE] = {};
Perhaps make that static?
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
_______________________________________________
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Marc Zyngier @ 2019-09-05 9:11 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Peter Maydell, "Daniel P . Berrangé",
Suzuki K Pouloze, Julien Thierry, linux-kernel, James Morse,
Stefan Hajnoczi, kvmarm, linux-arm-kernel
In-Reply-To: <754fb77a-aace-e0aa-a5bb-a6c6bcff9890@gmx.de>
On Thu, 05 Sep 2019 09:28:42 +0100,
Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 9/5/19 10:03 AM, Marc Zyngier wrote:
> > [Please use my kernel.org address. My arm.com address will disappear shortly]
> >
> > On Wed, 04 Sep 2019 19:07:36 +0100,
> > Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> If an application tries to access memory that is not mapped, an error
> >> ENOSYS, "load/store instruction decoding not implemented" may occur.
> >> QEMU will hang with a register dump.
> >>
> >> Instead create a data abort that can be handled gracefully by the
> >> application running in the virtual environment.
> >>
> >> Now the virtual machine can react to the event in the most appropriate
> >> way - by recovering, by writing an informative log, or by rebooting.
> >>
> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >> ---
> >> virt/kvm/arm/mmio.c | 4 ++--
> >> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> >> index a8a6a0c883f1..0cbed7d6a0f4 100644
> >> --- a/virt/kvm/arm/mmio.c
> >> +++ b/virt/kvm/arm/mmio.c
> >> @@ -161,8 +161,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> >> if (ret)
> >> return ret;
> >> } else {
> >> - kvm_err("load/store instruction decoding not implemented\n");
> >> - return -ENOSYS;
> >> + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> >> + return 1;
> >
> > How can you tell that the access would fault? You have no idea at that
> > stage (the kernel doesn't know about the MMIO ranges that userspace
> > handles). All you know is that you're faced with a memory access that
> > you cannot emulate in the kernel. Injecting a data abort at that stage
> > is not something that the architecture allows.
> >
> > If you want to address this, consider forwarding the access to
> > userspace. You'll only need an instruction decoder (supporting T1, T2,
> > A32 and A64) and a S1 page table walker (one per page table format,
> > all three of them) to emulate the access (having taken care of
> > stopping all the other vcpus to make sure there is no concurrent
> > modification of the page tables). You'll then be able to return the
> > result of the access back to the kernel.
>
> If I understand you right, the problem has to be fixed in QEMU and not
> in KVM.
It is a shared responsibility.
> Is there an example of such a forwarding already available in QEMU?
Yes. That's how we ask userspace (QEMU) to perform a MMIO access on
behalf of the guest (see how the run structure is populated and the
vcpu thread returns to userspace).
> >
> > Of course, the best thing would be to actually fix the guest so that
> > it doesn't use non-emulatable MMIO accesses. In general, that the sign
> > of a bug in low-level accessors.
>
> My problem was to find out where in my guest (U-Boot running UEFI SCT)
> the problem occurred. With this patch U-Boot gave me the relative
> address in Shell.efi and I was able to locate what was wrong in U-Boot's
> UEFI implementation.
I understand that there is a need for more precise information. I'm
just wary of adding debug features that become something that people
rely on, despite being in contradiction with the architecture.
I can help with the kernel side of the reporting, should someone want
to tackle the userspace side of it.
Thanks,
M.
--
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Marc Zyngier @ 2019-09-05 9:15 UTC (permalink / raw)
To: Peter Maydell
Cc: "Daniel P . Berrangé", Suzuki K Pouloze,
Heinrich Schuchardt, Julien Thierry, lkml - Kernel Mailing List,
James Morse, Stefan Hajnoczi, kvmarm, arm-mail-list
In-Reply-To: <CAFEAcA9qkqkOTqSVrhTpt-NkZSNXomSBNiWo_D6Kr=QKYRRf=w@mail.gmail.com>
On Thu, 05 Sep 2019 09:56:44 +0100,
Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Thu, 5 Sep 2019 at 09:52, Marc Zyngier <maz@kernel.org> wrote:
> >
> > On Thu, 05 Sep 2019 09:16:54 +0100,
> > Peter Maydell <peter.maydell@linaro.org> wrote:
> > > This is true, but the problem is that barfing out to userspace
> > > makes it harder to debug the guest because it means that
> > > the VM is immediately destroyed, whereas AIUI if we
> > > inject some kind of exception then (assuming you're set up
> > > to do kernel-debug via gdbstub) you can actually examine
> > > the offending guest code with a debugger because at least
> > > your VM is still around to inspect...
> >
> > To Christoffer's point, I find the benefit a bit dubious. Yes, you get
> > an exception, but the instruction that caused it may be completely
> > legal (store with post-increment, for example), leading to an even
> > more puzzled developer (that exception should never have been
> > delivered the first place).
>
> Right, but the combination of "host kernel prints a message
> about an unsupported load/store insn" and "within-guest debug
> dump/stack trace/etc" is much more useful than just having
> "host kernel prints message" and "QEMU exits"; and it requires
> about 3 lines of code change...
Which is wrong, and creates a new behaviour that isn't specified
anywhere.
>
> > I'm far more in favour of dumping the state of the access in the run
> > structure (much like we do for a MMIO access) and let userspace do
> > something about it (such as dumping information on the console or
> > breaking). It could even inject an exception *if* the user has asked
> > for it.
>
> ...whereas this requires agreement on a kernel-userspace API,
> larger changes in the kernel, somebody to implement the userspace
> side of things, and the user to update both the kernel and QEMU.
> It's hard for me to see that the benefit here over the 3-line
> approach really outweighs the extra effort needed.
3 lines that already require the host kernel to be updated, and create
a legacy that we'll never be able to get rid of.
> In practice saying "we should do this" is saying "we're going to do
> nothing", based on the historical record.
Thanks for the vote of confidence...
M.
--
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: [GIT PULL 2/4] DaVinci defconfig updates for v5.4
From: Arnd Bergmann @ 2019-09-05 9:15 UTC (permalink / raw)
To: Sekhar Nori
Cc: Bartosz Golaszewski, ARM-SoC Maintainers, Linux ARM Mailing List
In-Reply-To: <937b7ee1-ebdd-220d-f43a-da5e40ddfdf3@ti.com>
On Thu, Sep 5, 2019 at 8:18 AM Sekhar Nori <nsekhar@ti.com> wrote:
> On 04/09/19 8:21 PM, Arnd Bergmann wrote:
> > On Wed, Aug 28, 2019 at 5:18 PM Sekhar Nori <nsekhar@ti.com> wrote:
> > Some lines are just moved around but these ones
> > are completely removed:
> >
> > -# CONFIG_IOSCHED_DEADLINE is not set
> > -# CONFIG_IOSCHED_CFQ is not set
> > -CONFIG_PREEMPT=y
> > -CONFIG_SND_SOC_TLV320AIC3X=m
> > -CONFIG_SND_SOC_DAVINCI_MCASP=m
> > -CONFIG_LEDS_TRIGGERS=y
> > -CONFIG_TI_EDMA=y
> > -# CONFIG_ARM_UNWIND is not set
> >
> > I think most of these are ok, but I don't see any explanation
> > about why you turn off CONFIG_PREEMPT.
>
> So this came because with commit a50a3f4b6a31 ("sched/rt, Kconfig:
> Introduce CONFIG_PREEMPT_RT") PREEMPT lost its prompt string. As
> suggested by that commit, davinci_all_defconfig should be enabling
> CONFIG_PREEMPT_LL=y to retain functionality.
This got changed back with b8d3349803ba ("sched/rt, Kconfig: Unbreak
def/oldconfig with CONFIG_PREEMPT=y")
> I can respin the branch with the preempt fix first and then the
> defconfig refresh. Or I can skip the refresh altogether.
I think the best way to do these is to split up the refresh into two
or more patches:
- one patch to reorder the options that just moved around
- one patch that removes lines that are no longer needed,
explaining why there is no replacement
- separate patches for things that are now called something
else, or got replaced with a different option.
Arnd
_______________________________________________
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 1/2] i2c: qcom-geni: Provide an option to select FIFO processing
From: Wolfram Sang @ 2019-09-05 9:16 UTC (permalink / raw)
To: Lee Jones
Cc: mark.rutland, devicetree, linux-kernel, linux-arm-msm, agross,
robh+dt, Bjorn Andersson, alokc, linux-i2c, linux-arm-kernel
In-Reply-To: <20190905071103.GX26880@dell>
[-- Attachment #1.1: Type: text/plain, Size: 1963 bytes --]
Hi Lee,
> > It looks like a workaround to me. It would be interesting to hear which
> > I2C client breaks with DMA and if it's driver can't be fixed somehow
> > instead. But even if we agree on a workaround short term, adding a
So, are there investigations running why this reboot happens?
> > Is there no other way to disable DMA which is local to this driver so we
> > can easily revert the workaround later?
>
> This is the most local low-impact solution (nomenclature aside).
I disagree. You could use of_machine_is_compatible() and disable DMA for
that machine. Less impact because we save the workaround binding.
> The beautiful thing about this approach is that, *if* the Geni SE DMA
I'd say 'advantage' instead of 'beautiful' ;)
> ever starts working, we can remove the C code and any old properties
> left in older DTs just become NOOP. Older kernels with newer DTs
> (less of a priority) *still* won't work, but they don't work now
> anyway.
Which is a clear disadvantage of that solution. It won't fix older
kernels. My suggestion above should fix them, too.
> The offending line can be found at [0]. There is no obvious bug to
> fix and this code obviously works well on some of the hardware
> platforms using it. But on our platform (Lenovo Yoga C630 - QCom
> SMD850) that final command, which initiates the DMA transaction, ends
> up rebooting the machine.
Unless we know why the reboot happens on your platform, I'd be careful
with saying "work obviously well" on other platforms.
> With regards to the nomenclature, my original suggestion was
> 'qcom,geni-se-no-dma'. Would that better suit your request?
My suggestion:
For 5.3, use of_machine_is_compatible() and we backport that. For later,
try to find out the root cause and fix it. If that can't be done, try to
set up a generic "disable-dma" property and use it.
What do you think about that?
Kind regards,
Wolfram
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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 1/4] ACPI: Support Generic Initiator only domains
From: Jonathan Cameron @ 2019-09-05 9:16 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: ACPI Devel Maling List, Rafael J . Wysocki,
Linux Kernel Mailing List, Linuxarm, Keith Busch,
Linux Memory Management List, Jerome Glisse, Dan Williams,
Andrew Morton, Linux ARM
In-Reply-To: <CAJZ5v0ie8s-Ye7PD=xj0nXL228WDqhjJPCs+eV3n6_SAeaQowg@mail.gmail.com>
On Mon, 2 Sep 2019 23:26:16 +0200
"Rafael J. Wysocki" <rafael@kernel.org> wrote:
> On Wed, Aug 21, 2019 at 4:53 PM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > Generic Initiators are a new ACPI concept that allows for the
> > description of proximity domains that contain a device which
> > performs memory access (such as a network card) but neither
> > host CPU nor Memory.
> >
> > This patch has the parsing code and provides the infrastructure
> > for an architecture to associate these new domains with their
> > nearest memory processing node.
> >
> > Signed-off-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
> Dan, Keith, any comments?
>
> AFAICS this clashes with the series from Dan that rearranges the ACPI
> NUMA related code.
Seems that one is going forwards now which is great. I'll rebase this on
top of Dan's series and send a v5 sometime soon.
Thanks,
Jonathan
>
> > ---
> > drivers/acpi/numa.c | 62 +++++++++++++++++++++++++++++++++-
> > drivers/base/node.c | 3 ++
> > include/asm-generic/topology.h | 3 ++
> > include/linux/nodemask.h | 1 +
> > include/linux/topology.h | 7 ++++
> > 5 files changed, 75 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/numa.c b/drivers/acpi/numa.c
> > index eadbf90e65d1..fe34315a9234 100644
> > --- a/drivers/acpi/numa.c
> > +++ b/drivers/acpi/numa.c
> > @@ -170,6 +170,38 @@ acpi_table_print_srat_entry(struct acpi_subtable_header *header)
> > }
> > break;
> >
> > + case ACPI_SRAT_TYPE_GENERIC_AFFINITY:
> > + {
> > + struct acpi_srat_generic_affinity *p =
> > + (struct acpi_srat_generic_affinity *)header;
> > + char name[9] = {};
> > +
> > + if (p->device_handle_type == 0) {
> > + /*
> > + * For pci devices this may be the only place they
> > + * are assigned a proximity domain
> > + */
> > + pr_debug("SRAT Generic Initiator(Seg:%u BDF:%u) in proximity domain %d %s\n",
> > + *(u16 *)(&p->device_handle[0]),
> > + *(u16 *)(&p->device_handle[2]),
> > + p->proximity_domain,
> > + (p->flags & ACPI_SRAT_GENERIC_AFFINITY_ENABLED) ?
> > + "enabled" : "disabled");
> > + } else {
> > + /*
> > + * In this case we can rely on the device having a
> > + * proximity domain reference
> > + */
> > + memcpy(name, p->device_handle, 8);
> > + pr_info("SRAT Generic Initiator(HID=%.8s UID=%.4s) in proximity domain %d %s\n",
> > + (char *)(&p->device_handle[0]),
> > + (char *)(&p->device_handle[8]),
> > + p->proximity_domain,
> > + (p->flags & ACPI_SRAT_GENERIC_AFFINITY_ENABLED) ?
> > + "enabled" : "disabled");
> > + }
> > + }
> > + break;
> > default:
> > pr_warn("Found unsupported SRAT entry (type = 0x%x)\n",
> > header->type);
> > @@ -378,6 +410,32 @@ acpi_parse_gicc_affinity(union acpi_subtable_headers *header,
> > return 0;
> > }
> >
> > +static int __init
> > +acpi_parse_gi_affinity(union acpi_subtable_headers *header,
> > + const unsigned long end)
> > +{
> > + struct acpi_srat_generic_affinity *gi_affinity;
> > + int node;
> > +
> > + gi_affinity = (struct acpi_srat_generic_affinity *)header;
> > + if (!gi_affinity)
> > + return -EINVAL;
> > + acpi_table_print_srat_entry(&header->common);
> > +
> > + if (!(gi_affinity->flags & ACPI_SRAT_GENERIC_AFFINITY_ENABLED))
> > + return -EINVAL;
> > +
> > + node = acpi_map_pxm_to_node(gi_affinity->proximity_domain);
> > + if (node == NUMA_NO_NODE || node >= MAX_NUMNODES) {
> > + pr_err("SRAT: Too many proximity domains.\n");
> > + return -EINVAL;
> > + }
> > + node_set(node, numa_nodes_parsed);
> > + node_set_state(node, N_GENERIC_INITIATOR);
> > +
> > + return 0;
> > +}
> > +
> > static int __initdata parsed_numa_memblks;
> >
> > static int __init
> > @@ -433,7 +491,7 @@ int __init acpi_numa_init(void)
> >
> > /* SRAT: System Resource Affinity Table */
> > if (!acpi_table_parse(ACPI_SIG_SRAT, acpi_parse_srat)) {
> > - struct acpi_subtable_proc srat_proc[3];
> > + struct acpi_subtable_proc srat_proc[4];
> >
> > memset(srat_proc, 0, sizeof(srat_proc));
> > srat_proc[0].id = ACPI_SRAT_TYPE_CPU_AFFINITY;
> > @@ -442,6 +500,8 @@ int __init acpi_numa_init(void)
> > srat_proc[1].handler = acpi_parse_x2apic_affinity;
> > srat_proc[2].id = ACPI_SRAT_TYPE_GICC_AFFINITY;
> > srat_proc[2].handler = acpi_parse_gicc_affinity;
> > + srat_proc[3].id = ACPI_SRAT_TYPE_GENERIC_AFFINITY;
> > + srat_proc[3].handler = acpi_parse_gi_affinity;
> >
> > acpi_table_parse_entries_array(ACPI_SIG_SRAT,
> > sizeof(struct acpi_table_srat),
> > diff --git a/drivers/base/node.c b/drivers/base/node.c
> > index 75b7e6f6535b..6f60689af5f8 100644
> > --- a/drivers/base/node.c
> > +++ b/drivers/base/node.c
> > @@ -980,6 +980,8 @@ static struct node_attr node_state_attr[] = {
> > #endif
> > [N_MEMORY] = _NODE_ATTR(has_memory, N_MEMORY),
> > [N_CPU] = _NODE_ATTR(has_cpu, N_CPU),
> > + [N_GENERIC_INITIATOR] = _NODE_ATTR(has_generic_initiator,
> > + N_GENERIC_INITIATOR),
> > };
> >
> > static struct attribute *node_state_attrs[] = {
> > @@ -991,6 +993,7 @@ static struct attribute *node_state_attrs[] = {
> > #endif
> > &node_state_attr[N_MEMORY].attr.attr,
> > &node_state_attr[N_CPU].attr.attr,
> > + &node_state_attr[N_GENERIC_INITIATOR].attr.attr,
> > NULL
> > };
> >
> > diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
> > index 238873739550..54d0b4176a45 100644
> > --- a/include/asm-generic/topology.h
> > +++ b/include/asm-generic/topology.h
> > @@ -71,6 +71,9 @@
> > #ifndef set_cpu_numa_mem
> > #define set_cpu_numa_mem(cpu, node)
> > #endif
> > +#ifndef set_gi_numa_mem
> > +#define set_gi_numa_mem(gi, node)
> > +#endif
> >
> > #endif /* !CONFIG_NUMA || !CONFIG_HAVE_MEMORYLESS_NODES */
> >
> > diff --git a/include/linux/nodemask.h b/include/linux/nodemask.h
> > index 27e7fa36f707..1aebf766fb52 100644
> > --- a/include/linux/nodemask.h
> > +++ b/include/linux/nodemask.h
> > @@ -399,6 +399,7 @@ enum node_states {
> > #endif
> > N_MEMORY, /* The node has memory(regular, high, movable) */
> > N_CPU, /* The node has one or more cpus */
> > + N_GENERIC_INITIATOR, /* The node is a GI only node */
> > NR_NODE_STATES
> > };
> >
> > diff --git a/include/linux/topology.h b/include/linux/topology.h
> > index 47a3e3c08036..2f97754e0508 100644
> > --- a/include/linux/topology.h
> > +++ b/include/linux/topology.h
> > @@ -125,6 +125,13 @@ static inline void set_numa_mem(int node)
> > }
> > #endif
> >
> > +#ifndef set_gi_numa_mem
> > +static inline void set_gi_numa_mem(int gi, int node)
> > +{
> > + _node_numa_mem_[gi] = node;
> > +}
> > +#endif
> > +
> > #ifndef node_to_mem_node
> > static inline int node_to_mem_node(int node)
> > {
> > --
> > 2.20.1
> >
>
_______________________________________________
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 2/2] i2c: qcom-geni: Provide an option to disable DMA processing
From: Wolfram Sang @ 2019-09-05 9:18 UTC (permalink / raw)
To: Lee Jones
Cc: mark.rutland, devicetree, linux-kernel, linux-arm-msm, agross,
robh+dt, bjorn.andersson, vkoul, alokc, linux-i2c,
linux-arm-kernel
In-Reply-To: <20190905075213.13260-2-lee.jones@linaro.org>
[-- Attachment #1.1: Type: text/plain, Size: 1967 bytes --]
> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
Are you sure? From visual inspection, I don't see a correlation between
this commit and the fix here.
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
> ---
> drivers/i2c/busses/i2c-qcom-geni.c | 14 ++++++++++----
> 1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index a89bfce5388e..8822dea82980 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -353,13 +353,16 @@ static void geni_i2c_tx_fsm_rst(struct geni_i2c_dev *gi2c)
> static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> u32 m_param)
> {
> + struct device_node *np = gi2c->se.dev->of_node;
> dma_addr_t rx_dma;
> unsigned long time_left;
> - void *dma_buf;
> + void *dma_buf = NULL;
> struct geni_se *se = &gi2c->se;
> size_t len = msg->len;
>
> - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> + if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))
> + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> +
> if (dma_buf)
> geni_se_select_mode(se, GENI_SE_DMA);
> else
> @@ -392,13 +395,16 @@ static int geni_i2c_rx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> u32 m_param)
> {
> + struct device_node *np = gi2c->se.dev->of_node;
> dma_addr_t tx_dma;
> unsigned long time_left;
> - void *dma_buf;
> + void *dma_buf = NULL;
> struct geni_se *se = &gi2c->se;
> size_t len = msg->len;
>
> - dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> + if (!of_property_read_bool(np, "qcom,geni-se-no-dma"))
> + dma_buf = i2c_get_dma_safe_msg_buf(msg, 32);
> +
> if (dma_buf)
> geni_se_select_mode(se, GENI_SE_DMA);
> else
> --
> 2.17.1
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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 1/1] mm/pgtable/debug: Add test validating architecture page table helpers
From: Anshuman Khandual @ 2019-09-05 9:18 UTC (permalink / raw)
To: Gerald Schaefer
Cc: Mark Rutland, linux-ia64, linux-sh, Peter Zijlstra, James Hogan,
Tetsuo Handa, Heiko Carstens, Michal Hocko, linux-mm, Dave Hansen,
Paul Mackerras, sparclinux, Thomas Gleixner, linux-s390,
Michael Ellerman, x86, Russell King - ARM Linux, Matthew Wilcox,
Steven Price, Jason Gunthorpe, linux-arm-kernel, linux-snps-arc,
Kees Cook, Masahiro Yamada, Mark Brown, Dan Williams,
Vlastimil Babka, Sri Krishna chowdary, Ard Biesheuvel,
Greg Kroah-Hartman, linux-mips, Ralf Baechle, linux-kernel,
Paul Burton, Mike Rapoport, Vineet Gupta, Martin Schwidefsky,
Andrew Morton, linuxppc-dev, David S. Miller
In-Reply-To: <20190904221618.1b624a98@thinkpad>
On 09/05/2019 01:46 AM, Gerald Schaefer wrote:
> On Tue, 3 Sep 2019 13:31:46 +0530
> Anshuman Khandual <anshuman.khandual@arm.com> wrote:
>
>> This adds a test module which will validate architecture page table helpers
>> and accessors regarding compliance with generic MM semantics expectations.
>> This will help various architectures in validating changes to the existing
>> page table helpers or addition of new ones.
>>
>> Test page table and memory pages creating it's entries at various level are
>> all allocated from system memory with required alignments. If memory pages
>> with required size and alignment could not be allocated, then all depending
>> individual tests are skipped.
>
> This looks very useful, thanks. Of course, s390 is quite special and does
> not work nicely with this patch (yet), mostly because of our dynamic page
> table levels/folding. Still need to figure out what can be fixed in the arch
Hmm.
> code and what would need to be changed in the test module. See below for some
> generic comments/questions.
Sure.
>
> At least one real bug in the s390 code was already revealed by this, which
> is very nice. In pmd/pud_bad(), we also check large pmds/puds for sanity,
> instead of reporting them as bad, which is apparently not how it is expected.
Hmm, so it has already started being useful :)
>
> [...]
>> +/*
>> + * Basic operations
>> + *
>> + * mkold(entry) = An old and not a young entry
>> + * mkyoung(entry) = A young and not an old entry
>> + * mkdirty(entry) = A dirty and not a clean entry
>> + * mkclean(entry) = A clean and not a dirty entry
>> + * mkwrite(entry) = A write and not a write protected entry
>> + * wrprotect(entry) = A write protected and not a write entry
>> + * pxx_bad(entry) = A mapped and non-table entry
>> + * pxx_same(entry1, entry2) = Both entries hold the exact same value
>> + */
>> +#define VADDR_TEST (PGDIR_SIZE + PUD_SIZE + PMD_SIZE + PAGE_SIZE)
>
> Why is P4D_SIZE missing in the VADDR_TEST calculation?
This was a random possible virtual address to generate a representative
page table structure for the test. As there is a default (PGDIR_SIZE) for
P4D_SIZE on platforms which really do not have P4D level, it should be okay
to add P4D_SIZE in the above calculation.
>
> [...]
>> +
>> +#if !defined(__PAGETABLE_PMD_FOLDED) && !defined(__ARCH_HAS_4LEVEL_HACK)
>> +static void pud_clear_tests(pud_t *pudp)
>> +{
>> + memset(pudp, RANDOM_NZVALUE, sizeof(pud_t));
>> + pud_clear(pudp);
>> + WARN_ON(!pud_none(READ_ONCE(*pudp)));
>> +}
>
> For pgd/p4d/pud_clear(), we only clear if the page table level is present
> and not folded. The memset() here overwrites the table type bits, so
> pud_clear() will not clear anything on s390 and the pud_none() check will
> fail.
> Would it be possible to OR a (larger) random value into the table, so that
> the lower 12 bits would be preserved?
So the suggestion is instead of doing memset() on entry with RANDOM_NZVALUE,
it should OR a large random value preserving lower 12 bits. Hmm, this should
still do the trick for other platforms, they just need non zero value. So on
s390, the lower 12 bits on the page table entry already has valid value while
entering this function which would make sure that pud_clear() really does
clear the entry ?
>
>> +
>> +static void pud_populate_tests(struct mm_struct *mm, pud_t *pudp, pmd_t *pmdp)
>> +{
>> + /*
>> + * This entry points to next level page table page.
>> + * Hence this must not qualify as pud_bad().
>> + */
>> + pmd_clear(pmdp);
>> + pud_clear(pudp);
>> + pud_populate(mm, pudp, pmdp);
>> + WARN_ON(pud_bad(READ_ONCE(*pudp)));
>> +}
>
> This will populate the pud with a pmd pointer that does not point to the
> beginning of the pmd table, but to the second entry (because of how
> VADDR_TEST is constructed). This will result in failing pud_bad() check
> on s390. Not sure why/how it works on other archs, but would it be possible
> to align pmdp down to the beginning of the pmd table (and similar for the
> other pxd_populate_tests)?
Right, that was a problem. Will fix it by using the saved entries used for
freeing the page table pages at the end, which always point to the beginning
of a page table page.
>
> [...]
>> +
>> + p4d_free(mm, saved_p4dp);
>> + pud_free(mm, saved_pudp);
>> + pmd_free(mm, saved_pmdp);
>> + pte_free(mm, (pgtable_t) virt_to_page(saved_ptep));
>
> pgtable_t is arch-specific, and on s390 it is not a struct page pointer,
> but a pte pointer. So this will go wrong, also on all other archs (if any)
> where pgtable_t is not struct page.
> Would it be possible to use pte_free_kernel() instead, and just pass
> saved_ptep directly?
It makes sense, will change.
_______________________________________________
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 1/1] KVM: inject data abort if instruction cannot be decoded
From: Stefan Hajnoczi @ 2019-09-05 9:20 UTC (permalink / raw)
To: Heinrich Schuchardt
Cc: Peter Maydell, Daniel P . Berrangé, Suzuki K Pouloze,
Marc Zyngier, Julien Thierry, linux-kernel, James Morse, kvmarm,
linux-arm-kernel
In-Reply-To: <20190904180736.29009-1-xypron.glpk@gmx.de>
[-- Attachment #1.1: Type: text/plain, Size: 1691 bytes --]
On Wed, Sep 04, 2019 at 08:07:36PM +0200, Heinrich Schuchardt wrote:
> If an application tries to access memory that is not mapped, an error
> ENOSYS, "load/store instruction decoding not implemented" may occur.
> QEMU will hang with a register dump.
>
> Instead create a data abort that can be handled gracefully by the
> application running in the virtual environment.
>
> Now the virtual machine can react to the event in the most appropriate
> way - by recovering, by writing an informative log, or by rebooting.
>
> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
> virt/kvm/arm/mmio.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> index a8a6a0c883f1..0cbed7d6a0f4 100644
> --- a/virt/kvm/arm/mmio.c
> +++ b/virt/kvm/arm/mmio.c
> @@ -161,8 +161,8 @@ int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> if (ret)
> return ret;
> } else {
> - kvm_err("load/store instruction decoding not implemented\n");
> - return -ENOSYS;
> + kvm_inject_dabt(vcpu, kvm_vcpu_get_hfar(vcpu));
> + return 1;
I see this more as a temporary debugging hack than something to merge.
It sounds like in your case the guest environment provided good
debugging information and you preferred it over debugging this from the
host side. That's fine, but allowing the guest to continue running in
the general case makes it much harder to track down the root cause of a
problem because many guest CPU instructions may be executed after the
original problem occurs. Other guest software may fail silently in
weird ways. IMO it's best to fail early.
Stefan
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
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 1/1] arm64: dts: qcom: Add Lenovo Yoga C630
From: Sudeep Holla @ 2019-09-05 9:21 UTC (permalink / raw)
To: Lee Jones
Cc: mark.rutland, devicetree, linux-arm-msm, linux-kernel, robh+dt,
bjorn.andersson, agross, Sudeep Holla, linux-arm-kernel
In-Reply-To: <20190904121606.17474-1-lee.jones@linaro.org>
On Wed, Sep 04, 2019 at 01:16:06PM +0100, Lee Jones wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> The Lenovo Yoga C630 is built on the SDM850 from Qualcomm, but this seem
> to be similar enough to the SDM845 that we can reuse the sdm845.dtsi.
>
> Supported by this patch is: keyboard, battery monitoring, UFS storage,
> USB host and Bluetooth.
>
FWIW :),
Acked-by: Sudeep Holla <sudeep.holla@arm.com>
--
Regards,
Sudeep
_______________________________________________
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] dt-bindings: arm: Convert Realtek board/soc bindings to json-schema
From: Rob Herring @ 2019-09-05 9:21 UTC (permalink / raw)
To: jamestai.sky
Cc: Mark Rutland, devicetree, james.tai, CY_Huang,
linux-kernel@vger.kernel.org, Phinex Hung, Andreas Färber,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE
In-Reply-To: <20190905081721.1548-1-james.tai@realtek.com>
On Thu, Sep 5, 2019 at 9:19 AM <jamestai.sky@gmail.com> wrote:
>
> From: "james.tai" <james.tai@realtek.com>
>
> Convert Realtek SoC bindings to DT schema format using json-schema.
>
> Signed-off-by: james.tai <james.tai@realtek.com>
> ---
> .../devicetree/bindings/arm/realtek.txt | 22 -------------------
> .../devicetree/bindings/arm/realtek.yaml | 17 ++++++++++++++
> 2 files changed, 17 insertions(+), 22 deletions(-)
> delete mode 100644 Documentation/devicetree/bindings/arm/realtek.txt
> create mode 100644 Documentation/devicetree/bindings/arm/realtek.yaml
I've already submitted a patch for this that's *still* waiting on
Andreas to apply or comment on the licensing.
Also, your patch isn't valid schema. Please check with 'make dt_binding_check'.
_______________________________________________
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