* Re: [RFC PATCH V2 3/4] media: platform: Add Mediatek FD driver KConfig
From: Jerry-ch Chen @ 2019-09-05 14:14 UTC (permalink / raw)
To: Laurent Pinchart
Cc: devicetree@vger.kernel.org, Sean Cheng (鄭昇弘),
laurent.pinchart+renesas@ideasonboard.com,
Rynn Wu (吳育恩),
Christie Yu (游雅惠), srv_heupstream,
Po-Yang Huang (黃柏陽), suleiman@chromium.org,
shik@chromium.org, tfiga@chromium.org,
Jungo Lin (林明俊),
Sj Huang (黃信璋), yuzhao@chromium.org,
hans.verkuil@cisco.com, zwisler@chromium.org,
Frederic Chen (陳俊元), matthias.bgg@gmail.com,
linux-mediatek@lists.infradead.org, mchehab@kernel.org,
linux-arm-kernel@lists.infradead.org, linux-media@vger.kernel.org
In-Reply-To: <20190905123054.GL5035@pendragon.ideasonboard.com>
Hi Laurent,
On Thu, 2019-09-05 at 20:30 +0800, Laurent Pinchart wrote:
> Hi Jerry,
>
> Thank you for the patch.
>
> On Tue, Jul 09, 2019 at 04:41:11PM +0800, Jerry-ch Chen wrote:
> > From: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
> >
> > This patch adds KConfig for Mediatek Face Detection driver (FD).
> > FD is embedded in Mediatek SoCs. It can provide hardware
> > accelerated face detection function.
> >
> > Signed-off-by: Jerry-ch Chen <jerry-ch.chen@mediatek.com>
>
> You can squash this patch with 4/4, there's no need to keep it separate.
>
I appreciate your comments,
Ok, I will squash it.
> > ---
> > drivers/media/platform/Kconfig | 2 ++
> > drivers/media/platform/mtk-isp/fd/Kconfig | 17 +++++++++++++++++
> > 2 files changed, 19 insertions(+)
> > create mode 100644 drivers/media/platform/mtk-isp/fd/Kconfig
> >
> > diff --git a/drivers/media/platform/Kconfig b/drivers/media/platform/Kconfig
> > index a505e9f..ae99258e 100644
> > --- a/drivers/media/platform/Kconfig
> > +++ b/drivers/media/platform/Kconfig
> > @@ -32,6 +32,8 @@ source "drivers/media/platform/davinci/Kconfig"
> >
> > source "drivers/media/platform/omap/Kconfig"
> >
> > +source "drivers/media/platform/mtk-isp/fd/Kconfig"
> > +
> > config VIDEO_ASPEED
> > tristate "Aspeed AST2400 and AST2500 Video Engine driver"
> > depends on VIDEO_V4L2
> > diff --git a/drivers/media/platform/mtk-isp/fd/Kconfig b/drivers/media/platform/mtk-isp/fd/Kconfig
> > new file mode 100644
> > index 0000000..0c5eaf0
> > --- /dev/null
> > +++ b/drivers/media/platform/mtk-isp/fd/Kconfig
> > @@ -0,0 +1,17 @@
> > +config VIDEO_MEDIATEK_FD
> > + bool "Mediatek face detection processing function"
> > + select DMA_SHARED_BUFFER
> > + select VIDEOBUF2_DMA_CONTIG
> > + select VIDEOBUF2_CORE
> > + select VIDEOBUF2_V4L2
> > + select VIDEOBUF2_MEMOPS
> > + select VIDEOBUF2_VMALLOC
>
> Do you need both VIDEOBUF2_DMA_CONTIG and VIDEOBUF2_VMALLOC ? The driver
> doesn't seem to make use of VIDEOBUF2_VMALLOC.
>
No, I should remove it. and also would like to update as following:
depends on VIDEO_V4L2
depends on ARCH_MEDIATEK || COMPILE_TEST
select VIDEOBUF2_DMA_CONTIG
select VIDEOBUF2_CORE
select VIDEOBUF2_V4L2
select VIDEOBUF2_MEMOPS
select MEDIA_CONTROLLER
select MTK_SCP
> > + select MEDIA_CONTROLLER
> > +
> > + default n
> > + help
> > + Support the Face Detectioin (FD) feature.
>
> s/Detectioin/Detection/
>
Typo fixed.
> Maybe "... feature found in the Mediatek <list of SoCs> SoCs." ?
I will refine as:
Support the Face Detection (FD) feature in the Mediatek mt8183 Soc.
Thanks and best regards,
Jerry
>
> > +
> > + FD driver is a V4L2 memory-to-memory device driver which
> > + provides hardware accelerated face detection function,
> > + it can detect different sizes of faces in a raw image.
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] pinctrl: at91-pio4: implement .get_multiple and .set_multiple
From: Alexandre Belloni @ 2019-09-05 14:13 UTC (permalink / raw)
To: Linus Walleij
Cc: Alexandre Belloni, linux-kernel, linux-gpio, Ludovic Desroches,
linux-arm-kernel
Implement .get_multiple and .set_multiple to allow reading or setting
multiple pins simultaneously. Pins in the same bank will all be switched at
the same time, improving synchronization and performances.
Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
---
drivers/pinctrl/pinctrl-at91-pio4.c | 60 +++++++++++++++++++++++++++++
1 file changed, 60 insertions(+)
diff --git a/drivers/pinctrl/pinctrl-at91-pio4.c b/drivers/pinctrl/pinctrl-at91-pio4.c
index d6de4d360cd4..488a302a60d4 100644
--- a/drivers/pinctrl/pinctrl-at91-pio4.c
+++ b/drivers/pinctrl/pinctrl-at91-pio4.c
@@ -328,6 +328,35 @@ static int atmel_gpio_get(struct gpio_chip *chip, unsigned offset)
return !!(reg & BIT(pin->line));
}
+static int atmel_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
+ unsigned int bank;
+
+ bitmap_zero(bits, atmel_pioctrl->npins);
+
+ for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
+ unsigned int word = bank;
+ unsigned int offset = 0;
+ unsigned int reg;
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+ word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
+ offset = bank * ATMEL_PIO_NPINS_PER_BANK % BITS_PER_LONG;
+#endif
+ if (!mask[word])
+ continue;
+
+ reg = atmel_gpio_read(atmel_pioctrl, bank, ATMEL_PIO_PDSR);
+ bits[word] |= mask[word] & (reg << offset);
+
+ pr_err("ABE: %d %08x\n", bank, bits[word]);
+ }
+
+ return 0;
+}
+
static int atmel_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
int value)
{
@@ -358,11 +387,42 @@ static void atmel_gpio_set(struct gpio_chip *chip, unsigned offset, int val)
BIT(pin->line));
}
+static void atmel_gpio_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+ unsigned long *bits)
+{
+ struct atmel_pioctrl *atmel_pioctrl = gpiochip_get_data(chip);
+ unsigned int bank;
+
+ for (bank = 0; bank < atmel_pioctrl->nbanks; bank++) {
+ unsigned int bitmask;
+ unsigned int word = bank;
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+ word = BIT_WORD(bank * ATMEL_PIO_NPINS_PER_BANK);
+#endif
+ if (!mask[word])
+ continue;
+
+ bitmask = mask[word] & bits[word];
+ atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_SODR, bitmask);
+
+ bitmask = mask[word] & ~bits[word];
+ atmel_gpio_write(atmel_pioctrl, bank, ATMEL_PIO_CODR, bitmask);
+
+#if ATMEL_PIO_NPINS_PER_BANK != BITS_PER_LONG
+ mask[word] >>= ATMEL_PIO_NPINS_PER_BANK;
+ bits[word] >>= ATMEL_PIO_NPINS_PER_BANK;
+#endif
+ }
+}
+
static struct gpio_chip atmel_gpio_chip = {
.direction_input = atmel_gpio_direction_input,
.get = atmel_gpio_get,
+ .get_multiple = atmel_gpio_get_multiple,
.direction_output = atmel_gpio_direction_output,
.set = atmel_gpio_set,
+ .set_multiple = atmel_gpio_set_multiple,
.to_irq = atmel_gpio_to_irq,
.base = 0,
};
--
2.21.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v5 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd
From: Dave Martin @ 2019-09-05 14:20 UTC (permalink / raw)
To: Cristian Marussi
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <365b527e-793e-ad83-47a8-5d8692ed50c4@arm.com>
On Thu, Sep 05, 2019 at 02:32:09PM +0100, Cristian Marussi wrote:
> On 05/09/2019 13:39, Dave Martin wrote:
> > On Thu, Sep 05, 2019 at 01:15:58PM +0100, Cristian Marussi wrote:
> >> Hi
> >>
> >> On 04/09/2019 12:49, Dave Martin wrote:
> >>> On Mon, Sep 02, 2019 at 12:29:30pm +0100, Cristian Marussi wrote:
> >>>> Add a simple fake_sigreturn testcase which builds a ucontext_t with
> >>>> an anomalous additional fpsimd_context and place it onto the stack.
> >>>> Expects a SIGSEGV on test PASS.
> >>>>
> >>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >>>> ---
> >>>> v3 --> v4
> >>>> - fix commit
> >>>> - missing include
> >>>> - using new get_starting_head() helper
> >>>> - added test description
> >>>> ---
> >>>> .../fake_sigreturn_duplicated_fpsimd.c | 52 +++++++++++++++++++
> >>>> 1 file changed, 52 insertions(+)
> >>>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> >>>>
> >>>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> >>>> new file mode 100644
> >>>> index 000000000000..c7122c44f53f
> >>>> --- /dev/null
> >>>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
> >>>> @@ -0,0 +1,52 @@
> >>>> +// SPDX-License-Identifier: GPL-2.0
> >>>> +/*
> >>>> + * Copyright (C) 2019 ARM Limited
> >>>> + *
> >>>> + * Place a fake sigframe on the stack including an additional FPSIMD
> >>>> + * record: on sigreturn Kernel must spot this attempt and the test
> >>>> + * case is expected to be terminated via SEGV.
> >>>> + */
> >>>> +
> >>>> +#include <signal.h>
> >>>> +#include <ucontext.h>
> >>>> +
> >>>> +#include "test_signals_utils.h"
> >>>> +#include "testcases.h"
> >>>> +
> >>>> +struct fake_sigframe sf;
> >>>> +
> >>>> +static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,
> >>>> + siginfo_t *si, ucontext_t *uc)
> >>>> +{
> >>>> + size_t resv_sz, need_sz;
> >>>> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
> >>>> +
> >>>> + /* just to fill the ucontext_t with something real */
> >>>> + if (!get_current_context(td, &sf.uc))
> >>>> + return 1;
> >>>> +
> >>>> + resv_sz = GET_SF_RESV_SIZE(sf);
> >>>> + need_sz = HDR_SZ + sizeof(struct fpsimd_context);
> >>>
> >>> Nit: Maybe write this sum in the same order as the records we're going
> >>> o append, i.e.:
> >>>
> >>> need_sz = sizeof(struct fpsimd_context) + HDR_SZ; /* for terminator */
> >>
> >> Ok
> >>
> >>>
> >>> Also, maybe fail this test if there is no fpsimd_context in the initial
> >>> frame from get_current_context(): that would be a kernel bug, but we
> >>> wouldn't be giving fake_sigreturn() duplicate fpsimd_contexts in that
> >>> case -- so this test wouldn't test the thing it's supposed to test.
> >>>
> >>
> >> Any context grabbed by get_current_context() is verified at first to be sane
> >> when is copied in the handler by ASSERT_GOOD_CONTEXT()
> >>
> >>> } else if (signum == sig_copyctx && current->live_uc) {
> >>> memcpy(current->live_uc, uc, current->live_sz);
> >>> ASSERT_GOOD_CONTEXT(current->live_uc);
> >>> current->live_uc_valid = 1;
> >>
> >> A missing fpsimd in the original sigframe would lead to an abort()
> >> straight away while preparing the test, and the test will fail.
> >
> > OK, but there is no abort() in ASSERT_GOOD_CONTEXT(), only assert(0).
> > Can you add an abort() after the assert() in there in patch 2?
> >
> Ok yes, I meant the abort coming from assert(0) in fact....I'll review all
> the critical asserts for additional aborts in V6
OK -- I guess this only matters for things that should be reported as
test failures. Things that are purely bugs in the test are less of a
concern.
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v5 01/11] kselftest: arm64: add skeleton Makefile
From: Dave Martin @ 2019-09-05 14:18 UTC (permalink / raw)
To: Cristian Marussi
Cc: amit.kachhap, andreyknvl, shuah, linux-kselftest,
linux-arm-kernel
In-Reply-To: <4e7f583f-df36-1d7b-7a41-160abc60a296@arm.com>
On Thu, Sep 05, 2019 at 02:45:39PM +0100, Cristian Marussi wrote:
> On 04/09/2019 12:47, Dave Martin wrote:
> > On Mon, Sep 02, 2019 at 12:29:22pm +0100, Cristian Marussi wrote:
> >> Add a new arm64-specific empty subsystem amongst TARGETS of KSFT build
> >> framework; keep these new arm64 KSFT testcases separated into distinct
> >
> > Nit: this isn't true any more, since the tags tests already added the
> > arm64 subsystem here.
>
> Ok
> >
> >> subdirs inside tools/testing/selftests/arm64/ depending on the specific
> >> subsystem targeted.
> >>
> >> Add into toplevel arm64 KSFT Makefile a mechanism to guess the effective
> >> location of Kernel headers as installed by KSFT framework.
> >
> > This:
> >
> >> Merge with
> >>
> >> commit 9ce1263033cd ("selftests, arm64: add a selftest for passing
> >> tagged pointers to kernel")
> >>
> >> while moving such KSFT tags tests inside their own subdirectory
> >> (arm64/tags).
> >
> > ...could be put under the tearoff, but it doesn't really belong in the
> > commit message IMHO.
> >
> > I suggest rewriting the commit message to reflect the current
> > situation (but it can be kept brief).
> >
> > Basically, what this patch now seems to do is to prepare for adding
> > more arm64 tests, by moving the tags tests into their own subdirectory
> > and extending the existing skeleton Makefile as appropriate.
> >
>
> Ok
> >> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
> >> ---
> >> v4 --> v5
> >> - rebased on arm64/for-next/core
> >> - merged this patch with KSFT arm64 tags patch, while moving the latter
> >> into its own subdir
> >> - moved kernel header includes search mechanism from KSFT arm64
> >> SIGNAL Makefile
> >> - export proper top_srcdir ENV for lib.mk
> >> v3 --> v4
> >> - comment reword
> >> - simplified documentation in README
> >> - dropped README about standalone
> >> ---
> >
> > [...]
> >
> >> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
> >> index a61b2e743e99..5dbb0ffdfc9a 100644
> >> --- a/tools/testing/selftests/arm64/Makefile
> >> +++ b/tools/testing/selftests/arm64/Makefile
> >> @@ -1,11 +1,69 @@
> >> # SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (C) 2019 ARM Limited
> >
> > Change of copyright? This isn't pure Arm IP upstream IIUC.
> >
> > Maybe just drop it: Makefiles don't usually contain significant IP, so
> > many have no copyright message anyway.
> >
> Right. I'll drop.
> >> -# ARCH can be overridden by the user for cross compiling
> >> -ARCH ?= $(shell uname -m 2>/dev/null || echo not)
> >> +# When ARCH not overridden for crosscompiling, lookup machine
> >> +ARCH ?= $(shell uname -m)
> >> +ARCH := $(shell echo $(ARCH) | sed -e s/aarch64/arm64/)
> >>
> >> -ifneq (,$(filter $(ARCH),aarch64 arm64))
> >> -TEST_GEN_PROGS := tags_test
> >> -TEST_PROGS := run_tags_test.sh
> >> +ifeq ("x$(ARCH)", "xarm64")
> >> +SUBDIRS := tags
> >> +else
> >> +SUBDIRS :=
> >> endif
> >>
> >> -include ../lib.mk
> >> +CFLAGS := -Wall -O2 -g
> >> +
> >> +# A proper top_srcdir is needed by KSFT(lib.mk)
> >> +top_srcdir = ../../../../..
> >> +
> >> +# Additional include paths needed by kselftest.h and local headers
> >> +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/
> >> +
> >> +# Guessing where the Kernel headers could have been installed
> >> +# depending on ENV config
> >> +ifeq ($(KBUILD_OUTPUT),)
> >> +khdr_dir = $(top_srcdir)/usr/include
> >> +else
> >> +# the KSFT preferred location when KBUILD_OUTPUT is set
> >> +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include
> >> +endif
> >
> > Looking at this, can we just pass the directory in from the toplevel
> > "all" rule instead of guessing?
> >
> Do you mean toplevel in KSFT ?
> I think it's how should be done at the end, but I was trying to keep this series on
> arm64/ lands only. (also maybe I'm missing something obvious in KSFT handling of this
> situation....even though many other KSFT use built CFLAGS like: -I../../../usr/include
> or similar)
>
> > Maybe don't churn this for now though. It's something that could be
> > looked at later.
> >
>
> Ok. I'll leave here and fix it to avoid relative paths...which could be problematic
> when exported to lower level Makefiles.
>
> Cheers
>
> Cristian
Sounds reasonable
Cheers
---Dave
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] bus: ti-sysc: Fix clock handling for no-idle quirks
From: Grygorii Strashko @ 2019-09-05 14:17 UTC (permalink / raw)
To: Tony Lindgren, linux-omap
Cc: Nishanth Menon, Tero Kristo, Vignesh Raghavendra, Dave Gerlach,
Greg Kroah-Hartman, linux-kernel, Peter Ujfalusi, Faiz Abbas,
Keerthy, linux-arm-kernel, Roger Quadros
In-Reply-To: <20190905140337.19373-1-tony@atomide.com>
On 05/09/2019 17:03, Tony Lindgren wrote:
> NFSroot can fail on dra7 when cpsw is probed using ti-sysc interconnect
> target module driver as reported by Keerthy.
>
> Device clocks and the interconnect target module may or may not be
> enabled by the bootloader on init, but we currently assume the clocks
> and module are on from the bootloader for "ti,no-idle" and
> "ti,no-idle-on-init" quirks as reported by Grygorii Strashko.
>
> Let's fix the issue by always enabling clocks init, and
> never disable them for "ti,no-idle" quirk. For "ti,no-idle-on-init"
> quirk, we must decrement the usage count later on to allow PM
> runtime to idle the module if requested.
>
> Fixes: 1a5cd7c23cc5 ("bus: ti-sysc: Enable all clocks directly during init to read revision")
> Cc: Keerthy <j-keerthy@ti.com>
> Cc: Vignesh Raghavendra <vigneshr@ti.com>
> Reported-by: Keerthy <j-keerthy@ti.com>
> Reported-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Tony Lindgren <tony@atomide.com>
> ---
> drivers/bus/ti-sysc.c | 48 +++++++++++++++++++++++++++++++++----------
> 1 file changed, 37 insertions(+), 11 deletions(-)
>
Reviewed-by: Grygorii Strashko <grygorii.strashko@ti.com>
Thank you, Tony.
--
Best regards,
grygorii
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] pinctrl: bcm: remove redundant assignment to pointer log
From: Colin King @ 2019-09-05 14:09 UTC (permalink / raw)
To: Linus Walleij, Ray Jui, Scott Branden, bcm-kernel-feedback-list,
linux-gpio, linux-arm-kernel
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The pointer log is being initialized with a value that is never read
and is being re-assigned a little later on. The assignment is
redundant and hence can be removed.
Addresses-Coverity: ("Unused value")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/pinctrl/bcm/pinctrl-cygnus-mux.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c b/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c
index 44df35942a43..dcab2204c60c 100644
--- a/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c
+++ b/drivers/pinctrl/bcm/pinctrl-cygnus-mux.c
@@ -923,7 +923,6 @@ static int cygnus_mux_log_init(struct cygnus_pinctrl *pinctrl)
if (!pinctrl->mux_log)
return -ENOMEM;
- log = pinctrl->mux_log;
for (i = 0; i < CYGNUS_NUM_IOMUX_REGS; i++) {
for (j = 0; j < CYGNUS_NUM_MUX_PER_REG; j++) {
log = &pinctrl->mux_log[i * CYGNUS_NUM_MUX_PER_REG
--
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 related
* [PATCH] bus: ti-sysc: Fix clock handling for no-idle quirks
From: Tony Lindgren @ 2019-09-05 14:03 UTC (permalink / raw)
To: linux-omap
Cc: Nishanth Menon, Tero Kristo, Grygorii Strashko,
Vignesh Raghavendra, Dave Gerlach, Greg Kroah-Hartman,
linux-kernel, Peter Ujfalusi, Faiz Abbas, Keerthy,
linux-arm-kernel, Roger Quadros
NFSroot can fail on dra7 when cpsw is probed using ti-sysc interconnect
target module driver as reported by Keerthy.
Device clocks and the interconnect target module may or may not be
enabled by the bootloader on init, but we currently assume the clocks
and module are on from the bootloader for "ti,no-idle" and
"ti,no-idle-on-init" quirks as reported by Grygorii Strashko.
Let's fix the issue by always enabling clocks init, and
never disable them for "ti,no-idle" quirk. For "ti,no-idle-on-init"
quirk, we must decrement the usage count later on to allow PM
runtime to idle the module if requested.
Fixes: 1a5cd7c23cc5 ("bus: ti-sysc: Enable all clocks directly during init to read revision")
Cc: Keerthy <j-keerthy@ti.com>
Cc: Vignesh Raghavendra <vigneshr@ti.com>
Reported-by: Keerthy <j-keerthy@ti.com>
Reported-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Tony Lindgren <tony@atomide.com>
---
drivers/bus/ti-sysc.c | 48 +++++++++++++++++++++++++++++++++----------
1 file changed, 37 insertions(+), 11 deletions(-)
diff --git a/drivers/bus/ti-sysc.c b/drivers/bus/ti-sysc.c
--- a/drivers/bus/ti-sysc.c
+++ b/drivers/bus/ti-sysc.c
@@ -1632,17 +1632,19 @@ static int sysc_init_module(struct sysc *ddata)
if (error)
return error;
- if (manage_clocks) {
- sysc_clkdm_deny_idle(ddata);
+ sysc_clkdm_deny_idle(ddata);
- error = sysc_enable_opt_clocks(ddata);
- if (error)
- return error;
+ /*
+ * Always enable clocks. The bootloader may or may not have enabled
+ * the related clocks.
+ */
+ error = sysc_enable_opt_clocks(ddata);
+ if (error)
+ return error;
- error = sysc_enable_main_clocks(ddata);
- if (error)
- goto err_opt_clocks;
- }
+ error = sysc_enable_main_clocks(ddata);
+ if (error)
+ goto err_opt_clocks;
if (!(ddata->cfg.quirks & SYSC_QUIRK_NO_RESET_ON_INIT)) {
error = sysc_rstctrl_reset_deassert(ddata, true);
@@ -1660,7 +1662,7 @@ static int sysc_init_module(struct sysc *ddata)
goto err_main_clocks;
}
- if (!ddata->legacy_mode && manage_clocks) {
+ if (!ddata->legacy_mode) {
error = sysc_enable_module(ddata->dev);
if (error)
goto err_main_clocks;
@@ -1677,6 +1679,7 @@ static int sysc_init_module(struct sysc *ddata)
if (manage_clocks)
sysc_disable_main_clocks(ddata);
err_opt_clocks:
+ /* No re-enable of clockdomain autoidle to prevent module autoidle */
if (manage_clocks) {
sysc_disable_opt_clocks(ddata);
sysc_clkdm_allow_idle(ddata);
@@ -2357,6 +2360,28 @@ static void ti_sysc_idle(struct work_struct *work)
ddata = container_of(work, struct sysc, idle_work.work);
+ /*
+ * One time decrement of clock usage counts if left on from init.
+ * Note that we disable opt clocks unconditionally in this case
+ * as they are enabled unconditionally during init without
+ * considering sysc_opt_clks_needed() at that point.
+ */
+ if (ddata->cfg.quirks & (SYSC_QUIRK_NO_IDLE |
+ SYSC_QUIRK_NO_IDLE_ON_INIT)) {
+ sysc_clkdm_deny_idle(ddata);
+ sysc_disable_main_clocks(ddata);
+ sysc_disable_opt_clocks(ddata);
+ sysc_clkdm_allow_idle(ddata);
+ }
+
+ /* Keep permanent PM runtime usage count for SYSC_QUIRK_NO_IDLE */
+ if (ddata->cfg.quirks & SYSC_QUIRK_NO_IDLE)
+ return;
+
+ /*
+ * Decrement PM runtime usage count for SYSC_QUIRK_NO_IDLE_ON_INIT
+ * and SYSC_QUIRK_NO_RESET_ON_INIT
+ */
if (pm_runtime_active(ddata->dev))
pm_runtime_put_sync(ddata->dev);
}
@@ -2445,7 +2470,8 @@ static int sysc_probe(struct platform_device *pdev)
INIT_DELAYED_WORK(&ddata->idle_work, ti_sysc_idle);
/* At least earlycon won't survive without deferred idle */
- if (ddata->cfg.quirks & (SYSC_QUIRK_NO_IDLE_ON_INIT |
+ if (ddata->cfg.quirks & (SYSC_QUIRK_NO_IDLE |
+ SYSC_QUIRK_NO_IDLE_ON_INIT |
SYSC_QUIRK_NO_RESET_ON_INIT)) {
schedule_delayed_work(&ddata->idle_work, 3000);
} else {
--
2.23.0
_______________________________________________
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] sched/rt: avoid contend with CFS task
From: Vincent Guittot @ 2019-09-05 14:01 UTC (permalink / raw)
To: Jing-Ting Wu
Cc: wsd_upstream, Peter Zijlstra, linux-kernel, Valentin Schneider,
linux-mediatek, Matthias Brugger, Qais Yousef, LAK
In-Reply-To: <1567689999.2389.5.camel@mtkswgap22>
Hi Jing-Ting,
On Thu, 5 Sep 2019 at 15:26, Jing-Ting Wu <jing-ting.wu@mediatek.com> wrote:
>
> On Fri, 2019-08-30 at 15:55 +0100, Qais Yousef wrote:
> > On 08/29/19 11:38, Valentin Schneider wrote:
> > > On 29/08/2019 04:15, Jing-Ting Wu wrote:
> > > > At original linux design, RT & CFS scheduler are independent.
> > > > Current RT task placement policy will select the first cpu in
> > > > lowest_mask, even if the first CPU is running a CFS task.
> > > > This may put RT task to a running cpu and let CFS task runnable.
> > > >
> > > > So we select idle cpu in lowest_mask first to avoid preempting
> > > > CFS task.
> > > >
> > >
> > > Regarding the RT & CFS thing, that's working as intended. RT is a whole
> > > class above CFS, it shouldn't have to worry about CFS.
> > >
> > > On the other side of things, CFS does worry about RT. We have the concept
> > > of RT-pressure in the CFS scheduler, where RT tasks will reduce a CPU's
> > > capacity (see fair.c::scale_rt_capacity()).
> > >
> > > CPU capacity is looked at on CFS wakeup (see wake_cap() and
> > > find_idlest_cpu()), and the periodic load balancer tries to spread load
> > > over capacity, so it'll tend to put less things on CPUs that are also
> > > running RT tasks.
> > >
> > > If RT were to start avoiding rqs with CFS tasks, we'd end up with a nasty
> > > situation were both are avoiding each other. It's even more striking when
> > > you see that RT pressure is done with a rq-wide RT util_avg, which
> > > *doesn't* get migrated when a RT task migrates. So if you decide to move
> > > a RT task to an idle CPU "B" because CPU "A" had runnable CFS tasks, the
> > > CFS scheduler will keep seeing CPU "B" as not significantly RT-pressured
> > > while that util_avg signal ramps up, whereas it would correctly see CPU
> > > "A" as RT-pressured if the RT task previously ran there.
> > >
> > > So overall I think this is the wrong approach.
> >
> > I like the idea, but yeah tend to agree the current approach might not be
> > enough.
> >
> > I think the major problem here is that on generic systems where CFS is a first
> > class citizen, RT tasks can be hostile to them - not always necessarily for a
> > good reason.
> >
> > To further complicate the matter, even among CFS tasks we can't tell which are
> > more important than the others - though hopefully latency-nice proposal will
> > make the situation better.
> >
> > So I agree we have a problem here, but I think this patch is just a temporary
> > band aid and we need to do better. Though I have no concrete suggestion yet on
> > how to do that.
> >
> > Another thing I couldn't quantify yet how common and how severe this problem is
> > yet. Jing-Ting, if you can share the details of your use case that'd be great.
> >
> > Cheers
> >
> > --
> > Qais Yousef
>
>
> I agree that the nasty situation will happen.The current approach and this patch might not be enough.
RT task should not harm its cache hotness and responsiveness for the
benefit of a CFS task
> But for requirement of performance, I think it is better to differentiate between idle CPU and CPU has CFS task.
>
> For example, we use rt-app to evaluate runnable time on non-patched environment.
> There are (NR_CPUS-1) heavy CFS tasks and 1 RT Task. When a CFS task is running, the RT task wakes up and choose the same CPU.
> The CFS task will be preempted and keep runnable until it is migrated to another cpu by load balance.
> But load balance is not triggered immediately, it will be triggered until timer tick hits with some condition satisfied(ex. rq->next_balance).
Yes you will have to wait for the next tick that will trigger an idle
load balance because you have an idle cpu and 2 runnable tack (1 RT +
1CFS) on the same CPU. But you should not wait for more than 1 tick
The current load_balance doesn't handle correctly the situation of 1
CFS and 1 RT task on same CPU while 1 CPU is idle. There is a rework
of the load_balance that is under review on the mailing list that
fixes this problem and your CFS task should migrate to the idle CPU
faster than now
> CFS tasks may be runnable for a long time. In this test case, it increase 332.091 ms runnable time for CFS task.
>
> The detailed log is shown as following, CFS task(thread1-6580) is preempted by RT task(thread0-6674) about 332ms:
332ms is quite long and is probably not an idle load blanace but a
busy load balance
> thread1-6580 [003] dnh2 94.452898: sched_wakeup: comm=thread0 pid=6674 prio=89 target_cpu=003
> thread1-6580 [003] d..2 94.452916: sched_switch: prev_comm=thread1 prev_pid=6580 prev_prio=120 prev_state=R ==> next_comm=thread0 next_pid=6674 next_prio=89
> .... 332.091ms
> krtatm-1930 [001] d..2 94.785007: sched_migrate_task: comm=thread1 pid=6580 prio=120 orig_cpu=3 dest_cpu=1
> krtatm-1930 [001] d..2 94.785020: sched_switch: prev_comm=krtatm prev_pid=1930 prev_prio=100 prev_state=S ==> next_comm=thread1 next_pid=6580 next_prio=120
your CFS task has not moved on the idle CPU but has replaced another task
Regards,
Vincent
>
> So I think choose idle CPU at RT wake up flow could reduce the CFS runnable time.
>
>
> Best regards,
> Jing-Ting Wu
>
>
_______________________________________________
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] i2c: qcom-geni: Provide an option to disable DMA processing
From: Peter Rosin @ 2019-09-05 13:58 UTC (permalink / raw)
To: Wolfram Sang, Lee Jones
Cc: mark.rutland@arm.com, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, agross@kernel.org,
robh+dt@kernel.org, bjorn.andersson@linaro.org, vkoul@kernel.org,
alokc@codeaurora.org, linux-i2c@vger.kernel.org,
linux-arm-msm@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190905134941.GG1157@kunai>
On 2019-09-05 15:49, Wolfram Sang wrote:
> Hi Lee,
>
> I understand you are in a hurry, but please double check before
> sending...
Linus indicated that an rc8 is coming up, which should provide an extra week.
https://lwn.net/Articles/798152/
> On Thu, Sep 05, 2019 at 11:22:47AM +0100, Lee Jones wrote:
>> We have a production-level laptop (Lenovo Yoga C630) which is exhibiting
>> a rather horrific bug. When I2C HID devices are being scanned for at
>> boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.
>> When it does, the laptop reboots and the user never sees the OS.
>>
>> The beautiful thing about this approach is that, *if* the Geni SE DMA
>> 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* will not work - but they do not work now anyway.
>
> ... becasue this paragraph doesn't fit anymore. Needs to be reworded.
>
>>
>> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
>
> As said in the other thread, I don't get it, but this is not a show
> stopper for me.
WAG: because ACPI made some driver load at all, and when it
did it something started happening which crashed some machines.
Cheers,
Peter
_______________________________________________
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] i2c: qcom-geni: Provide an option to disable DMA processing
From: Wolfram Sang @ 2019-09-05 13:49 UTC (permalink / raw)
To: Lee Jones
Cc: mark.rutland, devicetree, linux-kernel, agross, robh+dt,
bjorn.andersson, vkoul, alokc, linux-i2c, linux-arm-msm,
linux-arm-kernel
In-Reply-To: <20190905102247.27583-1-lee.jones@linaro.org>
[-- Attachment #1.1: Type: text/plain, Size: 1273 bytes --]
Hi Lee,
I understand you are in a hurry, but please double check before
sending...
On Thu, Sep 05, 2019 at 11:22:47AM +0100, Lee Jones wrote:
> We have a production-level laptop (Lenovo Yoga C630) which is exhibiting
> a rather horrific bug. When I2C HID devices are being scanned for at
> boot-time the QCom Geni based I2C (Serial Engine) attempts to use DMA.
> When it does, the laptop reboots and the user never sees the OS.
>
> The beautiful thing about this approach is that, *if* the Geni SE DMA
> 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* will not work - but they do not work now anyway.
... becasue this paragraph doesn't fit anymore. Needs to be reworded.
>
> Fixes: 8bc529b25354 ("soc: qcom: geni: Add support for ACPI")
As said in the other thread, I don't get it, but this is not a show
stopper for me.
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> Reviewed-by: Vinod Koul <vkoul@kernel.org>
I'd like Vinod to resend his review. Because IMO the change since v2 was
not trivial, so the old rev-by has to be dropped.
Other than that, the code looks good to me!
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 v5 01/11] kselftest: arm64: add skeleton Makefile
From: Cristian Marussi @ 2019-09-05 13:45 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-arm-kernel,
linux-kselftest
In-Reply-To: <20190904114734.GR27757@arm.com>
On 04/09/2019 12:47, Dave Martin wrote:
> On Mon, Sep 02, 2019 at 12:29:22pm +0100, Cristian Marussi wrote:
>> Add a new arm64-specific empty subsystem amongst TARGETS of KSFT build
>> framework; keep these new arm64 KSFT testcases separated into distinct
>
> Nit: this isn't true any more, since the tags tests already added the
> arm64 subsystem here.
Ok
>
>> subdirs inside tools/testing/selftests/arm64/ depending on the specific
>> subsystem targeted.
>>
>> Add into toplevel arm64 KSFT Makefile a mechanism to guess the effective
>> location of Kernel headers as installed by KSFT framework.
>
> This:
>
>> Merge with
>>
>> commit 9ce1263033cd ("selftests, arm64: add a selftest for passing
>> tagged pointers to kernel")
>>
>> while moving such KSFT tags tests inside their own subdirectory
>> (arm64/tags).
>
> ...could be put under the tearoff, but it doesn't really belong in the
> commit message IMHO.
>
> I suggest rewriting the commit message to reflect the current
> situation (but it can be kept brief).
>
> Basically, what this patch now seems to do is to prepare for adding
> more arm64 tests, by moving the tags tests into their own subdirectory
> and extending the existing skeleton Makefile as appropriate.
>
Ok
>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>> ---
>> v4 --> v5
>> - rebased on arm64/for-next/core
>> - merged this patch with KSFT arm64 tags patch, while moving the latter
>> into its own subdir
>> - moved kernel header includes search mechanism from KSFT arm64
>> SIGNAL Makefile
>> - export proper top_srcdir ENV for lib.mk
>> v3 --> v4
>> - comment reword
>> - simplified documentation in README
>> - dropped README about standalone
>> ---
>
> [...]
>
>> diff --git a/tools/testing/selftests/arm64/Makefile b/tools/testing/selftests/arm64/Makefile
>> index a61b2e743e99..5dbb0ffdfc9a 100644
>> --- a/tools/testing/selftests/arm64/Makefile
>> +++ b/tools/testing/selftests/arm64/Makefile
>> @@ -1,11 +1,69 @@
>> # SPDX-License-Identifier: GPL-2.0
>> +# Copyright (C) 2019 ARM Limited
>
> Change of copyright? This isn't pure Arm IP upstream IIUC.
>
> Maybe just drop it: Makefiles don't usually contain significant IP, so
> many have no copyright message anyway.
>
Right. I'll drop.
>> -# ARCH can be overridden by the user for cross compiling
>> -ARCH ?= $(shell uname -m 2>/dev/null || echo not)
>> +# When ARCH not overridden for crosscompiling, lookup machine
>> +ARCH ?= $(shell uname -m)
>> +ARCH := $(shell echo $(ARCH) | sed -e s/aarch64/arm64/)
>>
>> -ifneq (,$(filter $(ARCH),aarch64 arm64))
>> -TEST_GEN_PROGS := tags_test
>> -TEST_PROGS := run_tags_test.sh
>> +ifeq ("x$(ARCH)", "xarm64")
>> +SUBDIRS := tags
>> +else
>> +SUBDIRS :=
>> endif
>>
>> -include ../lib.mk
>> +CFLAGS := -Wall -O2 -g
>> +
>> +# A proper top_srcdir is needed by KSFT(lib.mk)
>> +top_srcdir = ../../../../..
>> +
>> +# Additional include paths needed by kselftest.h and local headers
>> +CFLAGS += -I$(top_srcdir)/tools/testing/selftests/
>> +
>> +# Guessing where the Kernel headers could have been installed
>> +# depending on ENV config
>> +ifeq ($(KBUILD_OUTPUT),)
>> +khdr_dir = $(top_srcdir)/usr/include
>> +else
>> +# the KSFT preferred location when KBUILD_OUTPUT is set
>> +khdr_dir = $(KBUILD_OUTPUT)/kselftest/usr/include
>> +endif
>
> Looking at this, can we just pass the directory in from the toplevel
> "all" rule instead of guessing?
>
Do you mean toplevel in KSFT ?
I think it's how should be done at the end, but I was trying to keep this series on
arm64/ lands only. (also maybe I'm missing something obvious in KSFT handling of this
situation....even though many other KSFT use built CFLAGS like: -I../../../usr/include
or similar)
> Maybe don't churn this for now though. It's something that could be
> looked at later.
>
Ok. I'll leave here and fix it to avoid relative paths...which could be problematic
when exported to lower level Makefiles.
Cheers
Cristian
> [...]
>
> Apart from the comments above, the patch looks reasonable to me.
>
> Cheers
> ---Dave
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/2] i2c: qcom-geni: Provide an option to disable DMA processing
From: Wolfram Sang @ 2019-09-05 13:43 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: <20190905092816.GD26880@dell>
[-- Attachment #1.1: Type: text/plain, Size: 742 bytes --]
On Thu, Sep 05, 2019 at 10:28:16AM +0100, Lee Jones wrote:
> On Thu, 05 Sep 2019, Wolfram Sang wrote:
>
> >
> > > 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.
>
> This patch should have been part of the commit, or at the very least,
> part of the set, alluded to above. Unfortunately, I was carrying
> Bjorn's hack which simply returned early from geni_se_rx_dma_prep()
> with an error, so it masked the issue.
I still don't see why this basic ACPI enabling code (not touching DMA
but only clocks and pinctrl) causes and additional handling for DMA. Am
I overlooking something obvious?
[-- 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 v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-05 13:40 UTC (permalink / raw)
To: Christian Brauner
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: <20190905110544.d6c5t7rx25kvywmi@wittgenstein>
[-- Attachment #1.1: Type: text/plain, Size: 1978 bytes --]
On 2019-09-05, Christian Brauner <christian.brauner@ubuntu.com> wrote:
> On Thu, Sep 05, 2019 at 06:19:22AM +1000, 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>
[...]
> > + if (unlikely(!access_ok(src, usize)))
> > + return -EFAULT;
> > +
> > + /* Deal with trailing bytes. */
> > + if (usize < ksize)
> > + memset(dst + size, 0, rest);
[...]
> That's a change in behavior for clone3() and sched at least, no? Unless
> - which I guess you might have done - you have moved the "error out when
> the struct is too small" part before the call to copy_struct_from_user()
> for them.
Yes, I've put the minimum size check to the callers in all of the
cases (in the case of clone3() I've #define'd a CLONE_ARGS_SIZE_VER0 to
match the others -- see patch 2 of the series).
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 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/2] i2c: qcom-geni: Provide an option to select FIFO processing
From: Wolfram Sang @ 2019-09-05 13:37 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: <20190905093444.GE26880@dell>
[-- Attachment #1.1: Type: text/plain, Size: 1279 bytes --]
> > So, are there investigations running why this reboot happens?
>
> Yes, but they have been running for months, literally.
I see. This is good to know. Just so I also know where we are with this.
> > Which is a clear disadvantage of that solution. It won't fix older
> > kernels. My suggestion above should fix them, too.
>
> Not sure how this is possible. Unless you mean LTS?
Why not? Using of_machine_is_compatible() makes the patch 100% self
contained (no extra binding needed). It will work wherever the machine
description fits.
> > Unless we know why the reboot happens on your platform, I'd be careful
> > with saying "work obviously well" on other platforms.
>
> Someone must have tested it? Surely ... ;)
It seems to work mostly, I won't deny that. But we don't know if the
buggy situation can be triggered on these platforms as well by something
else later. We just don't know.
> > 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?
>
> Sounds okay to me. Let me code that up.
Glad you like it.
[-- 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 v12 01/12] lib: introduce copy_struct_{to,from}_user helpers
From: Aleksa Sarai @ 2019-09-05 13:35 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-ia64, linux-sh, Alexander Shishkin, Rasmus Villemoes,
Alexei Starovoitov, linux-kernel, David Howells, linux-kselftest,
sparclinux, Jiri Olsa, linux-arch, linux-s390, Tycho Andersen,
Aleksa Sarai, Shuah Khan, 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: <20190905094305.GJ2349@hirez.programming.kicks-ass.net>
[-- Attachment #1.1: Type: text/plain, Size: 5535 bytes --]
On 2019-09-05, Peter Zijlstra <peterz@infradead.org> wrote:
> On Thu, Sep 05, 2019 at 07:26:22PM +1000, Aleksa Sarai wrote:
> > On 2019-09-05, Peter Zijlstra <peterz@infradead.org> wrote:
> > > On Thu, Sep 05, 2019 at 06:19:22AM +1000, Aleksa Sarai wrote:
> > > > +/**
> > > > + * 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.
> > > > + *
> > > > + * Copies a struct from kernel space to user 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, struct foo __user *, uarg, size_t, usize)
> > > > + * {
> > > > + * int err;
> > > > + * struct foo karg = {};
> > > > + *
> > > > + * // do something with karg
> > > > + *
> > > > + * err = copy_struct_to_user(uarg, usize, &karg, sizeof(karg));
> > > > + * if (err)
> > > > + * return err;
> > > > + *
> > > > + * // ...
> > > > + * }
> > > > + *
> > > > + * There are three cases to consider:
> > > > + * * If @usize == @ksize, then it's copied verbatim.
> > > > + * * If @usize < @ksize, then kernel space is "returning" a newer struct to an
> > > > + * older user space. In order to avoid user space getting incomplete
> > > > + * information (new fields might be important), all trailing bytes in @src
> > > > + * (@ksize - @usize) must be zerored
> > >
> > > s/zerored/zero/, right?
> >
> > It should've been "zeroed".
>
> That reads wrong to me; that way it reads like this function must take
> that action and zero out the 'rest'; which is just wrong.
>
> This function must verify those bytes are zero, not make them zero.
Right, in my head I was thinking "must have been zeroed" which isn't
what it says. I'll switch to "zero".
> > > > , otherwise -EFBIG is returned.
> > >
> > > 'Funny' that, copy_struct_from_user() below seems to use E2BIG.
> >
> > This is a copy of the semantics that sched_[sg]etattr(2) uses -- E2BIG for
> > a "too big" struct passed to the kernel, and EFBIG for a "too big"
> > struct passed to user-space. I would personally have preferred EMSGSIZE
> > instead of EFBIG, but felt using the existing error codes would be less
> > confusing.
>
> Sadly a recent commit:
>
> 1251201c0d34 ("sched/core: Fix uclamp ABI bug, clean up and robustify sched_read_attr() ABI logic and code")
>
> Made the situation even 'worse'.
I hadn't seen this patch before, and I have a few questions taking a
look at it:
* An error code for a particular behaviour was changed (EFBIG ->
E2BIG). Is this not a userspace breakage (I know Linus went ballistic
about something similar a while ago[1]), or did I misunderstand what
the issue was in [1]?
* At the risk of bike-shedding -- of we are changing it, wouldn't
-EMSGSIZE be more appropriate? To be fair, picking errno values has
always been more of an art than a science, but to my ears "Argument
list too long" doesn't make too much sense in the context of
"returning" a struct back to userspace (and the cause of the error
is that the argument passed by user space *isn't big enough*). If
there was an E2SMALL that would also work. ;)
* Do you want me to write a patch based on that, to switch it to
copy_struct_to_user()?
* That patch removes the "are there non-zero bytes in the tail that
userspace won't know about" check (which I have included in mine). I
understand that this caused issues specifically with sched_getattr(2)
due to the default value not being zero -- how should we rectify that
(given that we'd hopefully want to port everyone who uses that
interface to copy_struct_{to,from}_user())?
* Given that the [uk]attr->size construct is pretty important to the
usability of the sched and perf interfaces, should we require (or
encourage) it for all struct-extension syscall setups?
> > > > + 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] = {};
> > >
> > > Isn't that too big for on-stack?
> >
> > Is a 64-byte buffer too big? I picked the number "at random" to be the
> > size of a cache line, but I could shrink it down to 32 bytes if the size
> > is an issue (I wanted to avoid needless allocations -- hence it being
> > on-stack).
>
> Ah, my ctags gave me a definition of BUFFER_SIZE that was 512. I suppose
> 64 should be OK.
Good to know, though I'll rename it to avoid confusion.
[1]: https://lore.kernel.org/lkml/CA+55aFy98A+LJK4+GWMcbzaa1zsPBRo76q+ioEjbx-uaMKH6Uw@mail.gmail.com/
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 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: [RFC PATCH V2 4/4] platform: mtk-isp: Add Mediatek FD driver
From: Jerry-ch Chen @ 2019-09-05 13:36 UTC (permalink / raw)
To: Tomasz Figa
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: <CAAFQd5D2ketE19RPr20BVYGhqg2Lh2ZNTtAr5J2GoWU9RiSAsA@mail.gmail.com>
On Thu, 2019-09-05 at 16:52 +0800, Tomasz Figa wrote:
> 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.
>
Ok, I will send v3 soon.
Thanks and best regards,
Jerry
> 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 v5 09/11] kselftest: arm64: fake_sigreturn_duplicated_fpsimd
From: Cristian Marussi @ 2019-09-05 13:32 UTC (permalink / raw)
To: Dave Martin
Cc: amit.kachhap, andreyknvl, shuah, linux-kselftest,
linux-arm-kernel
In-Reply-To: <20190905123904.GD27757@arm.com>
On 05/09/2019 13:39, Dave Martin wrote:
> On Thu, Sep 05, 2019 at 01:15:58PM +0100, Cristian Marussi wrote:
>> Hi
>>
>> On 04/09/2019 12:49, Dave Martin wrote:
>>> On Mon, Sep 02, 2019 at 12:29:30pm +0100, Cristian Marussi wrote:
>>>> Add a simple fake_sigreturn testcase which builds a ucontext_t with
>>>> an anomalous additional fpsimd_context and place it onto the stack.
>>>> Expects a SIGSEGV on test PASS.
>>>>
>>>> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
>>>> ---
>>>> v3 --> v4
>>>> - fix commit
>>>> - missing include
>>>> - using new get_starting_head() helper
>>>> - added test description
>>>> ---
>>>> .../fake_sigreturn_duplicated_fpsimd.c | 52 +++++++++++++++++++
>>>> 1 file changed, 52 insertions(+)
>>>> create mode 100644 tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
>>>>
>>>> diff --git a/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
>>>> new file mode 100644
>>>> index 000000000000..c7122c44f53f
>>>> --- /dev/null
>>>> +++ b/tools/testing/selftests/arm64/signal/testcases/fake_sigreturn_duplicated_fpsimd.c
>>>> @@ -0,0 +1,52 @@
>>>> +// SPDX-License-Identifier: GPL-2.0
>>>> +/*
>>>> + * Copyright (C) 2019 ARM Limited
>>>> + *
>>>> + * Place a fake sigframe on the stack including an additional FPSIMD
>>>> + * record: on sigreturn Kernel must spot this attempt and the test
>>>> + * case is expected to be terminated via SEGV.
>>>> + */
>>>> +
>>>> +#include <signal.h>
>>>> +#include <ucontext.h>
>>>> +
>>>> +#include "test_signals_utils.h"
>>>> +#include "testcases.h"
>>>> +
>>>> +struct fake_sigframe sf;
>>>> +
>>>> +static int fake_sigreturn_duplicated_fpsimd_run(struct tdescr *td,
>>>> + siginfo_t *si, ucontext_t *uc)
>>>> +{
>>>> + size_t resv_sz, need_sz;
>>>> + struct _aarch64_ctx *shead = GET_SF_RESV_HEAD(sf), *head;
>>>> +
>>>> + /* just to fill the ucontext_t with something real */
>>>> + if (!get_current_context(td, &sf.uc))
>>>> + return 1;
>>>> +
>>>> + resv_sz = GET_SF_RESV_SIZE(sf);
>>>> + need_sz = HDR_SZ + sizeof(struct fpsimd_context);
>>>
>>> Nit: Maybe write this sum in the same order as the records we're going
>>> o append, i.e.:
>>>
>>> need_sz = sizeof(struct fpsimd_context) + HDR_SZ; /* for terminator */
>>
>> Ok
>>
>>>
>>> Also, maybe fail this test if there is no fpsimd_context in the initial
>>> frame from get_current_context(): that would be a kernel bug, but we
>>> wouldn't be giving fake_sigreturn() duplicate fpsimd_contexts in that
>>> case -- so this test wouldn't test the thing it's supposed to test.
>>>
>>
>> Any context grabbed by get_current_context() is verified at first to be sane
>> when is copied in the handler by ASSERT_GOOD_CONTEXT()
>>
>>> } else if (signum == sig_copyctx && current->live_uc) {
>>> memcpy(current->live_uc, uc, current->live_sz);
>>> ASSERT_GOOD_CONTEXT(current->live_uc);
>>> current->live_uc_valid = 1;
>>
>> A missing fpsimd in the original sigframe would lead to an abort()
>> straight away while preparing the test, and the test will fail.
>
> OK, but there is no abort() in ASSERT_GOOD_CONTEXT(), only assert(0).
> Can you add an abort() after the assert() in there in patch 2?
>
Ok yes, I meant the abort coming from assert(0) in fact....I'll review all
the critical asserts for additional aborts in V6
> People probably aren't going to be building the tests with -DNDEBUG, but
> we should avoid unnecessary surprises...
>
> Cheers
> ---Dave
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/1] sched/rt: avoid contend with CFS task
From: Jing-Ting Wu @ 2019-09-05 13:26 UTC (permalink / raw)
To: Qais Yousef
Cc: wsd_upstream, Peter Zijlstra, linux-kernel, linux-mediatek,
Matthias Brugger, Valentin Schneider, linux-arm-kernel
In-Reply-To: <20190830145501.zadfv2ffuu7j46ft@e107158-lin.cambridge.arm.com>
On Fri, 2019-08-30 at 15:55 +0100, Qais Yousef wrote:
> On 08/29/19 11:38, Valentin Schneider wrote:
> > On 29/08/2019 04:15, Jing-Ting Wu wrote:
> > > At original linux design, RT & CFS scheduler are independent.
> > > Current RT task placement policy will select the first cpu in
> > > lowest_mask, even if the first CPU is running a CFS task.
> > > This may put RT task to a running cpu and let CFS task runnable.
> > >
> > > So we select idle cpu in lowest_mask first to avoid preempting
> > > CFS task.
> > >
> >
> > Regarding the RT & CFS thing, that's working as intended. RT is a whole
> > class above CFS, it shouldn't have to worry about CFS.
> >
> > On the other side of things, CFS does worry about RT. We have the concept
> > of RT-pressure in the CFS scheduler, where RT tasks will reduce a CPU's
> > capacity (see fair.c::scale_rt_capacity()).
> >
> > CPU capacity is looked at on CFS wakeup (see wake_cap() and
> > find_idlest_cpu()), and the periodic load balancer tries to spread load
> > over capacity, so it'll tend to put less things on CPUs that are also
> > running RT tasks.
> >
> > If RT were to start avoiding rqs with CFS tasks, we'd end up with a nasty
> > situation were both are avoiding each other. It's even more striking when
> > you see that RT pressure is done with a rq-wide RT util_avg, which
> > *doesn't* get migrated when a RT task migrates. So if you decide to move
> > a RT task to an idle CPU "B" because CPU "A" had runnable CFS tasks, the
> > CFS scheduler will keep seeing CPU "B" as not significantly RT-pressured
> > while that util_avg signal ramps up, whereas it would correctly see CPU
> > "A" as RT-pressured if the RT task previously ran there.
> >
> > So overall I think this is the wrong approach.
>
> I like the idea, but yeah tend to agree the current approach might not be
> enough.
>
> I think the major problem here is that on generic systems where CFS is a first
> class citizen, RT tasks can be hostile to them - not always necessarily for a
> good reason.
>
> To further complicate the matter, even among CFS tasks we can't tell which are
> more important than the others - though hopefully latency-nice proposal will
> make the situation better.
>
> So I agree we have a problem here, but I think this patch is just a temporary
> band aid and we need to do better. Though I have no concrete suggestion yet on
> how to do that.
>
> Another thing I couldn't quantify yet how common and how severe this problem is
> yet. Jing-Ting, if you can share the details of your use case that'd be great.
>
> Cheers
>
> --
> Qais Yousef
I agree that the nasty situation will happen.The current approach and this patch might not be enough.
But for requirement of performance, I think it is better to differentiate between idle CPU and CPU has CFS task.
For example, we use rt-app to evaluate runnable time on non-patched environment.
There are (NR_CPUS-1) heavy CFS tasks and 1 RT Task. When a CFS task is running, the RT task wakes up and choose the same CPU.
The CFS task will be preempted and keep runnable until it is migrated to another cpu by load balance.
But load balance is not triggered immediately, it will be triggered until timer tick hits with some condition satisfied(ex. rq->next_balance).
CFS tasks may be runnable for a long time. In this test case, it increase 332.091 ms runnable time for CFS task.
The detailed log is shown as following, CFS task(thread1-6580) is preempted by RT task(thread0-6674) about 332ms:
thread1-6580 [003] dnh2 94.452898: sched_wakeup: comm=thread0 pid=6674 prio=89 target_cpu=003
thread1-6580 [003] d..2 94.452916: sched_switch: prev_comm=thread1 prev_pid=6580 prev_prio=120 prev_state=R ==> next_comm=thread0 next_pid=6674 next_prio=89
.... 332.091ms
krtatm-1930 [001] d..2 94.785007: sched_migrate_task: comm=thread1 pid=6580 prio=120 orig_cpu=3 dest_cpu=1
krtatm-1930 [001] d..2 94.785020: sched_switch: prev_comm=krtatm prev_pid=1930 prev_prio=100 prev_state=S ==> next_comm=thread1 next_pid=6580 next_prio=120
So I think choose idle CPU at RT wake up flow could reduce the CFS runnable time.
Best regards,
Jing-Ting Wu
_______________________________________________
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] KVM: arm64: vgic-v4: Move the GICv4 residency flow to be driven by vcpu_load/put
From: Marc Zyngier @ 2019-09-05 13:26 UTC (permalink / raw)
To: Andrew Murray
Cc: kvm, Suzuki K Poulose, Andre Przywara, Eric Auger, James Morse,
linux-arm-kernel, kvmarm, Julien Thierry
In-Reply-To: <20190905130410.GA9720@e119886-lin.cambridge.arm.com>
On 05/09/2019 14:04, Andrew Murray wrote:
> Hi Marc,
>
> Some feedback below, but mostly questions to aid my understanding...
>
> On Tue, Sep 03, 2019 at 04:57:47PM +0100, Marc Zyngier wrote:
>> When the VHE code was reworked, a lot of the vgic stuff was moved around,
>> but the GICv4 residency code did stay untouched, meaning that we come
>> in and out of residency on each flush/sync, which is obviously suboptimal.
>>
>> To address this, let's move things around a bit:
>>
>> - Residency entry (flush) moves to vcpu_load
>> - Residency exit (sync) moves to vcpu_put
>> - On blocking (entry to WFI), we "put"
>> - On unblocking (exit from WFI, we "load"
>>
>> Because these can nest (load/block/put/load/unblock/put, for example),
>> we now have per-VPE tracking of the residency state.
>>
>> Additionally, vgic_v4_put gains a "need doorbell" parameter, which only
>> gets set to true when blocking because of a WFI. This allows a finer
>> control of the doorbell, which now also gets disabled as soon as
>> it gets signaled.
>>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> drivers/irqchip/irq-gic-v4.c | 7 +++-
>> include/kvm/arm_vgic.h | 4 +--
>> include/linux/irqchip/arm-gic-v4.h | 2 ++
>> virt/kvm/arm/arm.c | 12 ++++---
>> virt/kvm/arm/vgic/vgic-v3.c | 4 +++
>> virt/kvm/arm/vgic/vgic-v4.c | 55 ++++++++++++++----------------
>> virt/kvm/arm/vgic/vgic.c | 4 ---
>> virt/kvm/arm/vgic/vgic.h | 2 --
>> 8 files changed, 48 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
>> index 563e87ed0766..45969927cc81 100644
>> --- a/drivers/irqchip/irq-gic-v4.c
>> +++ b/drivers/irqchip/irq-gic-v4.c
>> @@ -141,12 +141,17 @@ static int its_send_vpe_cmd(struct its_vpe *vpe, struct its_cmd_info *info)
>> int its_schedule_vpe(struct its_vpe *vpe, bool on)
>> {
>> struct its_cmd_info info;
>> + int ret;
>>
>> WARN_ON(preemptible());
>>
>> info.cmd_type = on ? SCHEDULE_VPE : DESCHEDULE_VPE;
>>
>> - return its_send_vpe_cmd(vpe, &info);
>> + ret = its_send_vpe_cmd(vpe, &info);
>> + if (!ret)
>> + vpe->resident = on;
>> +
>
> We make an assumption here that its_schedule_vpe is the only caller of
> its_send_vpe_cmd where we may pass SCHEDULE_VPE. I guess this is currently
> the case.
It is, and it is intended to stay that way.
> Why do we also set the residency flag for DESCHEDULE_VPE?
We don't.
> And by residency we mean that interrupts are delivered to VM, instead of
> doorbell?
Interrupts are always delivered to the VPE, whether it is resident or
not. Residency is defined as the VPE that is currently programmed in the
redistributor (by virtue of programming the VPROPBASER and VPENDBASER
registers).
>
>> + return ret;
>> }
>>
>> int its_invall_vpe(struct its_vpe *vpe)
>> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
>> index af4f09c02bf1..4dc58d7a0010 100644
>> --- a/include/kvm/arm_vgic.h
>> +++ b/include/kvm/arm_vgic.h
>> @@ -396,7 +396,7 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int irq,
>> int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int irq,
>> struct kvm_kernel_irq_routing_entry *irq_entry);
>>
>> -void kvm_vgic_v4_enable_doorbell(struct kvm_vcpu *vcpu);
>> -void kvm_vgic_v4_disable_doorbell(struct kvm_vcpu *vcpu);
>> +int vgic_v4_load(struct kvm_vcpu *vcpu);
>> +int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db);
>>
>> #endif /* __KVM_ARM_VGIC_H */
>> diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
>> index e6b155713b47..ab1396afe08a 100644
>> --- a/include/linux/irqchip/arm-gic-v4.h
>> +++ b/include/linux/irqchip/arm-gic-v4.h
>> @@ -35,6 +35,8 @@ struct its_vpe {
>> /* Doorbell interrupt */
>> int irq;
>> irq_hw_number_t vpe_db_lpi;
>> + /* VPE resident */
>> + bool resident;
>> /* VPE proxy mapping */
>> int vpe_proxy_event;
>> /*
>> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
>> index 35a069815baf..4e69268621b6 100644
>> --- a/virt/kvm/arm/arm.c
>> +++ b/virt/kvm/arm/arm.c
>> @@ -321,20 +321,24 @@ void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu)
>> /*
>> * If we're about to block (most likely because we've just hit a
>> * WFI), we need to sync back the state of the GIC CPU interface
>> - * so that we have the lastest PMR and group enables. This ensures
>> + * so that we have the latest PMR and group enables. This ensures
>> * that kvm_arch_vcpu_runnable has up-to-date data to decide
>> * whether we have pending interrupts.
>> + *
>> + * For the same reason, we want to tell GICv4 that we need
>> + * doorbells to be signalled, should an interrupt become pending.
>
> As I understand, and as indicated by removal of kvm_vgic_v4_enable_doorbell
> below, we've now abstracted enabling the doorbell behind the concept of a
> v4_put.
>
> Why then, do we break that abstraction by adding this comment? Surely we just
> want to indicate that we're done with ITS for now - do whatever you need to do.
Well, I don't think you can realistically pretend that KVM doesn't know
about the intricacies of GICv4. They are intimately linked.
> This would have made more sense to me if the comment above was removed in this
> patch rather than added.
I disagree. The very reason we to a put on GICv4 is to get a doorbell.
If we didn't need one, we'd just let schedule() do a non
doorbell-generating vcpu_put.
>> */
>> preempt_disable();
>> kvm_vgic_vmcr_sync(vcpu);
>> + vgic_v4_put(vcpu, true);
>> preempt_enable();
>> -
>> - kvm_vgic_v4_enable_doorbell(vcpu);
>> }
>>
>> void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
>> {
>> - kvm_vgic_v4_disable_doorbell(vcpu);
>> + preempt_disable();
>> + vgic_v4_load(vcpu);
>> + preempt_enable();
>> }
>>
>> int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
>> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
>> index 8d69f007dd0c..48307a9eb1d8 100644
>> --- a/virt/kvm/arm/vgic/vgic-v3.c
>> +++ b/virt/kvm/arm/vgic/vgic-v3.c
>> @@ -664,6 +664,8 @@ void vgic_v3_load(struct kvm_vcpu *vcpu)
>>
>> if (has_vhe())
>> __vgic_v3_activate_traps(vcpu);
>> +
>> + WARN_ON(vgic_v4_load(vcpu));
>> }
>>
>> void vgic_v3_vmcr_sync(struct kvm_vcpu *vcpu)
>> @@ -676,6 +678,8 @@ void vgic_v3_vmcr_sync(struct kvm_vcpu *vcpu)
>>
>> void vgic_v3_put(struct kvm_vcpu *vcpu)
>> {
>> + WARN_ON(vgic_v4_put(vcpu, false));
>> +
>> vgic_v3_vmcr_sync(vcpu);
>>
>> kvm_call_hyp(__vgic_v3_save_aprs, vcpu);
>> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
>> index 477af6aebb97..3a8a28854b13 100644
>> --- a/virt/kvm/arm/vgic/vgic-v4.c
>> +++ b/virt/kvm/arm/vgic/vgic-v4.c
>> @@ -85,6 +85,10 @@ static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
>> {
>> struct kvm_vcpu *vcpu = info;
>>
>> + /* We got the message, no need to fire again */
>> + if (!irqd_irq_disabled(&irq_to_desc(irq)->irq_data))
>> + disable_irq_nosync(irq);
>> +
>> vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
>> kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
>> kvm_vcpu_kick(vcpu);
>
> This is because the doorbell will fire each time any guest device interrupts,
> however we only need to tell the guest just once that something has happened
> right?
Not for any guest interrupt. Only for VLPIs. And yes, there is no need
to get multiple doorbells. Once you got one, you know you're runnable
and don't need to be told another 50k times...
>
>> @@ -192,20 +196,30 @@ void vgic_v4_teardown(struct kvm *kvm)
>> its_vm->vpes = NULL;
>> }
>>
>> -int vgic_v4_sync_hwstate(struct kvm_vcpu *vcpu)
>> +int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db)
>> {
>> - if (!vgic_supports_direct_msis(vcpu->kvm))
>> + struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
>> + struct irq_desc *desc = irq_to_desc(vpe->irq);
>> +
>> + if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
>> return 0;
>
> Are we using !vpe->resident to avoid pointlessly doing work we've
> already done?
And also to avoid corrupting the state that we've saved by re-reading
what could potentially be an invalid state.
>
>>
>> - return its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, false);
>> + /*
>> + * If blocking, a doorbell is required. Undo the nested
>> + * disable_irq() calls...
>> + */
>> + while (need_db && irqd_irq_disabled(&desc->irq_data))
>> + enable_irq(vpe->irq);
>> +
>> + return its_schedule_vpe(vpe, false);
>> }
>>
>> -int vgic_v4_flush_hwstate(struct kvm_vcpu *vcpu)
>> +int vgic_v4_load(struct kvm_vcpu *vcpu)
>> {
>> - int irq = vcpu->arch.vgic_cpu.vgic_v3.its_vpe.irq;
>> + struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
>> int err;
>>
>> - if (!vgic_supports_direct_msis(vcpu->kvm))
>> + if (!vgic_supports_direct_msis(vcpu->kvm) || vpe->resident)
>> return 0;
>>
>> /*
>> @@ -214,11 +228,14 @@ int vgic_v4_flush_hwstate(struct kvm_vcpu *vcpu)
>> * doc in drivers/irqchip/irq-gic-v4.c to understand how this
>> * turns into a VMOVP command at the ITS level.
>> */
>> - err = irq_set_affinity(irq, cpumask_of(smp_processor_id()));
>> + err = irq_set_affinity(vpe->irq, cpumask_of(smp_processor_id()));
>> if (err)
>> return err;
>>
>> - err = its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, true);
>> + /* Disabled the doorbell, as we're about to enter the guest */
>> + disable_irq(vpe->irq);
>> +
>> + err = its_schedule_vpe(vpe, true);
>> if (err)
>> return err;
>
> Given that the doorbell corresponds with vpe residency, it could make sense
> to add a helper here that calls its_schedule_vpe and [disable,enable]_irq.
> Though I see that vgic_v3_put calls vgic_v4_put with need_db=false. I wonder
> what effect setting that to true would be for vgic_v3_put? Could it be known
> that v3 won't set need_db to true?
There is no doorbells for GICv3.
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: Heinrich Schuchardt @ 2019-09-05 13:25 UTC (permalink / raw)
To: Christoffer Dall, Peter Maydell
Cc: Daniel P. Berrangé, Marc Zyngier, lkml - Kernel Mailing List,
Stefan Hajnoczi, kvmarm, arm-mail-list
In-Reply-To: <20190905092223.GC4320@e113682-lin.lund.arm.com>
On 9/5/19 11:22 AM, Christoffer Dall wrote:
> On Thu, Sep 05, 2019 at 09:56:44AM +0100, Peter Maydell 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...
>>
>>> 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.
>>
>
> How about something like the following (completely untested, liable for
> ABI discussions etc. etc., but for illustration purposes).
>
> I think it raises the question (and likely many other) of whether we can
> break the existing 'ABI' and change behavior for missing ISV
> retrospectively for legacy user space when the issue has occurred?
>
> Someone might have written code that reacts to the -ENOSYS, so I've
> taken the conservative approach for this for the time being.
>
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 8a37c8e89777..19a92c49039c 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -76,6 +76,14 @@ struct kvm_arch {
>
> /* Mandated version of PSCI */
> u32 psci_version;
> +
> + /*
> + * If we encounter a data abort without valid instruction syndrome
> + * information, report this to user space. User space can (and
> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
> + * supported.
> + */
> + bool return_nisv_io_abort_to_user;
> };
>
> #define KVM_NR_MEM_OBJS 40
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f656169db8c3..019bc560edc1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -83,6 +83,14 @@ struct kvm_arch {
>
> /* Mandated version of PSCI */
> u32 psci_version;
> +
> + /*
> + * If we encounter a data abort without valid instruction syndrome
> + * information, report this to user space. User space can (and
> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
> + * supported.
> + */
> + bool return_nisv_io_abort_to_user;
How about 32bit ARM?
> };
>
> #define KVM_NR_MEM_OBJS 40
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 5e3f12d5359e..a4dd004d0db9 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -235,6 +235,7 @@ struct kvm_hyperv_exit {
> #define KVM_EXIT_S390_STSI 25
> #define KVM_EXIT_IOAPIC_EOI 26
> #define KVM_EXIT_HYPERV 27
> +#define KVM_EXIT_ARM_NISV 28
>
> /* For KVM_EXIT_INTERNAL_ERROR */
> /* Emulate instruction failed. */
> @@ -996,6 +997,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
> #define KVM_CAP_ARM_PTRAUTH_GENERIC 172
> #define KVM_CAP_PMU_EVENT_FILTER 173
> +#define KVM_CAP_ARM_NISV_TO_USER 174
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 35a069815baf..2ce94bd9d4a9 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -98,6 +98,26 @@ int kvm_arch_check_processor_compat(void)
> return 0;
> }
>
> +int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> + struct kvm_enable_cap *cap)
This overrides the weak implementation in virt/kvm/kvm_main.c. OK.
> +{
> + int r;
> +
> + if (cap->flags)
> + return -EINVAL;
> +
> + switch (cap->cap) {
> + case KVM_CAP_ARM_NISV_TO_USER:
> + r = 0;
> + kvm->arch.return_nisv_io_abort_to_user = true;
> + break;
> + default:
> + r = -EINVAL;
> + break;
> + }
> +
> + return r;
> +}
>
> /**
> * kvm_arch_init_vm - initializes a VM data structure
> @@ -196,6 +216,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_MP_STATE:
> case KVM_CAP_IMMEDIATE_EXIT:
> case KVM_CAP_VCPU_EVENTS:
> + case KVM_CAP_ARM_NISV_TO_USER:
> r = 1;
> break;
> case KVM_CAP_ARM_SET_DEVICE_ADDR:
> @@ -673,6 +694,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> if (ret)
> return ret;
> + } else if (run->exit_reason == KVM_EXIT_ARM_NISV) {
> + kvm_inject_undefined(vcpu);
So QEMU can try to enable the feature via IOCTL. And here you would
raise the 'undefined instruction' exception which QEMU will have to
handle in the loop calling KVM either by trying to make sense of the
instruction or by passing it on to the guest.
Conceptually this looks good to me and meets the requirements of my
application.
Thanks a lot for your suggestion.
Regards
Heinrich
> }
>
> if (run->immediate_exit)
> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> index 6af5c91337f2..62e6ef47a6de 100644
> --- a/virt/kvm/arm/mmio.c
> +++ b/virt/kvm/arm/mmio.c
> @@ -167,8 +167,15 @@ 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;
> + if (vcpu->kvm->arch.return_nisv_io_abort_to_user) {
> + run->exit_reason = KVM_EXIT_ARM_NISV;
> + run->mmio.phys_addr = fault_ipa;
> + vcpu->stat.mmio_exit_user++;
> + return 0;
> + } else {
> + kvm_info("encountered data abort without syndrome info\n");
> + return -ENOSYS;
> + }
> }
>
> rt = vcpu->arch.mmio_decode.rt;
>
>
> Thanks,
>
> Christoffer
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v1] watchdog: imx_sc: this patch just fixes whitespaces
From: Guenter Roeck @ 2019-09-05 13:24 UTC (permalink / raw)
To: Oliver Graute, oliver.graute@gmail.com
Cc: Mark Rutland, Aisheng Dong, Ulf Hansson,
linux-watchdog@vger.kernel.org, devicetree@vger.kernel.org,
Shawn Guo, Sascha Hauer, linux-kernel@vger.kernel.org,
Daniel Baluta, Rob Herring, NXP Linux Team,
Pengutronix Kernel Team, Wim Van Sebroeck, Fabio Estevam,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20190905073730.22258-1-oliver.graute@kococonnector.com>
On 9/5/19 12:44 AM, Oliver Graute wrote:
> Fix only whitespace errors in imx_sc_wdt_probe()
>
> Signed-off-by: Oliver Graute <oliver.graute@kococonnector.com>
This patch no longer applies due to commit "watchdog: imx_sc: Remove
unnecessary error log".
Guenter
> ---
> drivers/watchdog/imx_sc_wdt.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/watchdog/imx_sc_wdt.c b/drivers/watchdog/imx_sc_wdt.c
> index 78eaaf75a263..94db949042c9 100644
> --- a/drivers/watchdog/imx_sc_wdt.c
> +++ b/drivers/watchdog/imx_sc_wdt.c
> @@ -175,12 +175,12 @@ static int imx_sc_wdt_probe(struct platform_device *pdev)
> watchdog_stop_on_unregister(wdog);
>
> ret = devm_watchdog_register_device(dev, wdog);
> -
> - if (ret) {
> - dev_err(dev, "Failed to register watchdog device\n");
> - return ret;
> - }
> -
> +
> + if (ret) {
> + dev_err(dev, "Failed to register watchdog device\n");
> + return ret;
> + }
> +
> ret = imx_scu_irq_group_enable(SC_IRQ_GROUP_WDOG,
> SC_IRQ_WDOG,
> true);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [alsa-devel] [PATCH -next] ASoC: SOF: imx8: Fix COMPILE_TEST error
From: Pierre-Louis Bossart @ 2019-09-05 13:14 UTC (permalink / raw)
To: YueHaibing, lgirdwood, broonie, perex, tiwai, shawnguo, s.hauer,
kernel, festevam, linux-imx, daniel.baluta
Cc: alsa-devel, linux-kernel, linux-arm-kernel
In-Reply-To: <20190905064400.24800-1-yuehaibing@huawei.com>
On 9/5/19 1:44 AM, YueHaibing wrote:
> When do compile test, if SND_SOC_SOF_OF is not set, we get:
>
> sound/soc/sof/imx/imx8.o: In function `imx8_dsp_handle_request':
> imx8.c:(.text+0xb0): undefined reference to `snd_sof_ipc_msgs_rx'
> sound/soc/sof/imx/imx8.o: In function `imx8_ipc_msg_data':
> imx8.c:(.text+0xf4): undefined reference to `sof_mailbox_read'
> sound/soc/sof/imx/imx8.o: In function `imx8_dsp_handle_reply':
> imx8.c:(.text+0x160): undefined reference to `sof_mailbox_read'
>
> Make SND_SOC_SOF_IMX_TOPLEVEL always depends on SND_SOC_SOF_OF
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 202acc565a1f ("ASoC: SOF: imx: Add i.MX8 HW support")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
> sound/soc/sof/imx/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/sound/soc/sof/imx/Kconfig b/sound/soc/sof/imx/Kconfig
> index fd73d84..5acae75 100644
> --- a/sound/soc/sof/imx/Kconfig
> +++ b/sound/soc/sof/imx/Kconfig
> @@ -2,7 +2,8 @@
>
> config SND_SOC_SOF_IMX_TOPLEVEL
> bool "SOF support for NXP i.MX audio DSPs"
> - depends on ARM64 && SND_SOC_SOF_OF || COMPILE_TEST
> + depends on ARM64|| COMPILE_TEST
> + depends on SND_SOC_SOF_OF
> help
> This adds support for Sound Open Firmware for NXP i.MX platforms.
> Say Y if you have such a device.
>
_______________________________________________
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 13:09 UTC (permalink / raw)
To: Christoffer Dall, Peter Maydell
Cc: Daniel P. Berrangé, Heinrich Schuchardt,
lkml - Kernel Mailing List, Stefan Hajnoczi, kvmarm,
arm-mail-list
In-Reply-To: <20190905092223.GC4320@e113682-lin.lund.arm.com>
On 05/09/2019 10:22, Christoffer Dall wrote:
> On Thu, Sep 05, 2019 at 09:56:44AM +0100, Peter Maydell 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...
>>
>>> 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.
>>
>
> How about something like the following (completely untested, liable for
> ABI discussions etc. etc., but for illustration purposes).
>
> I think it raises the question (and likely many other) of whether we can
> break the existing 'ABI' and change behavior for missing ISV
> retrospectively for legacy user space when the issue has occurred?
>
> Someone might have written code that reacts to the -ENOSYS, so I've
> taken the conservative approach for this for the time being.
>
>
> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
> index 8a37c8e89777..19a92c49039c 100644
> --- a/arch/arm/include/asm/kvm_host.h
> +++ b/arch/arm/include/asm/kvm_host.h
> @@ -76,6 +76,14 @@ struct kvm_arch {
>
> /* Mandated version of PSCI */
> u32 psci_version;
> +
> + /*
> + * If we encounter a data abort without valid instruction syndrome
> + * information, report this to user space. User space can (and
> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
> + * supported.
> + */
> + bool return_nisv_io_abort_to_user;
> };
>
> #define KVM_NR_MEM_OBJS 40
> diff --git a/arch/arm64/include/asm/kvm_host.h b/arch/arm64/include/asm/kvm_host.h
> index f656169db8c3..019bc560edc1 100644
> --- a/arch/arm64/include/asm/kvm_host.h
> +++ b/arch/arm64/include/asm/kvm_host.h
> @@ -83,6 +83,14 @@ struct kvm_arch {
>
> /* Mandated version of PSCI */
> u32 psci_version;
> +
> + /*
> + * If we encounter a data abort without valid instruction syndrome
> + * information, report this to user space. User space can (and
> + * should) opt in to this feature if KVM_CAP_ARM_NISV_TO_USER is
> + * supported.
> + */
> + bool return_nisv_io_abort_to_user;
> };
>
> #define KVM_NR_MEM_OBJS 40
> diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
> index 5e3f12d5359e..a4dd004d0db9 100644
> --- a/include/uapi/linux/kvm.h
> +++ b/include/uapi/linux/kvm.h
> @@ -235,6 +235,7 @@ struct kvm_hyperv_exit {
> #define KVM_EXIT_S390_STSI 25
> #define KVM_EXIT_IOAPIC_EOI 26
> #define KVM_EXIT_HYPERV 27
> +#define KVM_EXIT_ARM_NISV 28
>
> /* For KVM_EXIT_INTERNAL_ERROR */
> /* Emulate instruction failed. */
> @@ -996,6 +997,7 @@ struct kvm_ppc_resize_hpt {
> #define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
> #define KVM_CAP_ARM_PTRAUTH_GENERIC 172
> #define KVM_CAP_PMU_EVENT_FILTER 173
> +#define KVM_CAP_ARM_NISV_TO_USER 174
>
> #ifdef KVM_CAP_IRQ_ROUTING
>
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 35a069815baf..2ce94bd9d4a9 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -98,6 +98,26 @@ int kvm_arch_check_processor_compat(void)
> return 0;
> }
>
> +int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
> + struct kvm_enable_cap *cap)
> +{
> + int r;
> +
> + if (cap->flags)
> + return -EINVAL;
> +
> + switch (cap->cap) {
> + case KVM_CAP_ARM_NISV_TO_USER:
> + r = 0;
> + kvm->arch.return_nisv_io_abort_to_user = true;
> + break;
> + default:
> + r = -EINVAL;
> + break;
> + }
> +
> + return r;
> +}
>
> /**
> * kvm_arch_init_vm - initializes a VM data structure
> @@ -196,6 +216,7 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
> case KVM_CAP_MP_STATE:
> case KVM_CAP_IMMEDIATE_EXIT:
> case KVM_CAP_VCPU_EVENTS:
> + case KVM_CAP_ARM_NISV_TO_USER:
> r = 1;
> break;
> case KVM_CAP_ARM_SET_DEVICE_ADDR:
> @@ -673,6 +694,8 @@ int kvm_arch_vcpu_ioctl_run(struct kvm_vcpu *vcpu, struct kvm_run *run)
> ret = kvm_handle_mmio_return(vcpu, vcpu->run);
> if (ret)
> return ret;
> + } else if (run->exit_reason == KVM_EXIT_ARM_NISV) {
> + kvm_inject_undefined(vcpu);
Just to make sure I understand: Is the expectation here that userspace
could clear the exit reason if it managed to handle the exit? And
otherwise we'd inject an UNDEF on reentry?
> }
>
> if (run->immediate_exit)
> diff --git a/virt/kvm/arm/mmio.c b/virt/kvm/arm/mmio.c
> index 6af5c91337f2..62e6ef47a6de 100644
> --- a/virt/kvm/arm/mmio.c
> +++ b/virt/kvm/arm/mmio.c
> @@ -167,8 +167,15 @@ 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;
> + if (vcpu->kvm->arch.return_nisv_io_abort_to_user) {
> + run->exit_reason = KVM_EXIT_ARM_NISV;
> + run->mmio.phys_addr = fault_ipa;
We could also record whether that's a read or a write (WnR should still
be valid). Actually, we could store a sanitized version of the ESR.
> + vcpu->stat.mmio_exit_user++;
> + return 0;
> + } else {
> + kvm_info("encountered data abort without syndrome info\n");
My only issue with this is that the previous message has been sort of
documented...
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/2] clk: imx8mm: Move 1443X/1416X PLL clock structure to common place
From: Leonard Crestez @ 2019-09-05 13:05 UTC (permalink / raw)
To: Anson Huang, Abel Vesa, Jacky Bai
Cc: Aisheng Dong, S.j. Wang, Peng Fan, sfr@canb.auug.org.au,
sboyd@kernel.org, shawnguo@kernel.org, mturquette@baylibre.com,
linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org,
dl-linux-imx, kernel@pengutronix.de, Fancy Fang,
festevam@gmail.com, s.hauer@pengutronix.de,
linux-arm-kernel@lists.infradead.org, l.stach@pengutronix.de
In-Reply-To: <1567720699-23514-1-git-send-email-Anson.Huang@nxp.com>
On 05.09.2019 12:59, Anson Huang wrote:
> Many i.MX8M SoCs use same 1443X/1416X PLL, such as i.MX8MM,
> i.MX8MN and later i.MX8M SoCs, moving these PLL definitions
> to common place can save a lot of duplicated code on each
> platform.
There are lots of similarities between imx8m clocks, do you plan to do
combine them further?
> Meanwhile, no need to define PLL clock structure for every
> module which uses same type of PLL, e.g., audio/video/dram use
> 1443X PLL, arm/gpu/vpu/sys use 1416X PLL, define 2 PLL clock
> structure for each group is enough.
> diff --git a/drivers/clk/imx/clk.c b/drivers/clk/imx/clk.c
> +const struct imx_pll14xx_rate_table imx_pll1416x_tbl[] = {
> + PLL_1416X_RATE(1800000000U, 225, 3, 0),
> + PLL_1416X_RATE(1600000000U, 200, 3, 0),
> + PLL_1416X_RATE(1200000000U, 300, 3, 1),
> + PLL_1416X_RATE(1000000000U, 250, 3, 1),
> + PLL_1416X_RATE(800000000U, 200, 3, 1),
> + PLL_1416X_RATE(750000000U, 250, 2, 2),
> + PLL_1416X_RATE(700000000U, 350, 3, 2),
> + PLL_1416X_RATE(600000000U, 300, 3, 2),
> +};
> +
> +const struct imx_pll14xx_rate_table imx_pll1443x_tbl[] = {
> + PLL_1443X_RATE(650000000U, 325, 3, 2, 0),
> + PLL_1443X_RATE(594000000U, 198, 2, 2, 0),
> + PLL_1443X_RATE(393216000U, 262, 2, 3, 9437),
> + PLL_1443X_RATE(361267200U, 361, 3, 3, 17511),
> +};
> +
> +struct imx_pll14xx_clk imx_1443x_pll = {
> + .type = PLL_1443X,
> + .rate_table = imx_pll1443x_tbl,
> + .rate_count = ARRAY_SIZE(imx_pll1443x_tbl),
> +};
> +
> +struct imx_pll14xx_clk imx_1416x_pll = {
> + .type = PLL_1416X,
> + .rate_table = imx_pll1416x_tbl,
> + .rate_count = ARRAY_SIZE(imx_pll1416x_tbl),
> +};
Perhaps these consts should be in clk-pll14xx.c? That way they won't be
compiled for imx6 as well.
--
Regards,
Leonard
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] KVM: arm64: vgic-v4: Move the GICv4 residency flow to be driven by vcpu_load/put
From: Andrew Murray @ 2019-09-05 13:04 UTC (permalink / raw)
To: Marc Zyngier
Cc: kvm, Suzuki K Poulose, Andre Przywara, Eric Auger, James Morse,
linux-arm-kernel, kvmarm, Julien Thierry
In-Reply-To: <20190903155747.219802-1-maz@kernel.org>
Hi Marc,
Some feedback below, but mostly questions to aid my understanding...
On Tue, Sep 03, 2019 at 04:57:47PM +0100, Marc Zyngier wrote:
> When the VHE code was reworked, a lot of the vgic stuff was moved around,
> but the GICv4 residency code did stay untouched, meaning that we come
> in and out of residency on each flush/sync, which is obviously suboptimal.
>
> To address this, let's move things around a bit:
>
> - Residency entry (flush) moves to vcpu_load
> - Residency exit (sync) moves to vcpu_put
> - On blocking (entry to WFI), we "put"
> - On unblocking (exit from WFI, we "load"
>
> Because these can nest (load/block/put/load/unblock/put, for example),
> we now have per-VPE tracking of the residency state.
>
> Additionally, vgic_v4_put gains a "need doorbell" parameter, which only
> gets set to true when blocking because of a WFI. This allows a finer
> control of the doorbell, which now also gets disabled as soon as
> it gets signaled.
>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
> drivers/irqchip/irq-gic-v4.c | 7 +++-
> include/kvm/arm_vgic.h | 4 +--
> include/linux/irqchip/arm-gic-v4.h | 2 ++
> virt/kvm/arm/arm.c | 12 ++++---
> virt/kvm/arm/vgic/vgic-v3.c | 4 +++
> virt/kvm/arm/vgic/vgic-v4.c | 55 ++++++++++++++----------------
> virt/kvm/arm/vgic/vgic.c | 4 ---
> virt/kvm/arm/vgic/vgic.h | 2 --
> 8 files changed, 48 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v4.c b/drivers/irqchip/irq-gic-v4.c
> index 563e87ed0766..45969927cc81 100644
> --- a/drivers/irqchip/irq-gic-v4.c
> +++ b/drivers/irqchip/irq-gic-v4.c
> @@ -141,12 +141,17 @@ static int its_send_vpe_cmd(struct its_vpe *vpe, struct its_cmd_info *info)
> int its_schedule_vpe(struct its_vpe *vpe, bool on)
> {
> struct its_cmd_info info;
> + int ret;
>
> WARN_ON(preemptible());
>
> info.cmd_type = on ? SCHEDULE_VPE : DESCHEDULE_VPE;
>
> - return its_send_vpe_cmd(vpe, &info);
> + ret = its_send_vpe_cmd(vpe, &info);
> + if (!ret)
> + vpe->resident = on;
> +
We make an assumption here that its_schedule_vpe is the only caller of
its_send_vpe_cmd where we may pass SCHEDULE_VPE. I guess this is currently
the case.
Why do we also set the residency flag for DESCHEDULE_VPE?
And by residency we mean that interrupts are delivered to VM, instead of
doorbell?
> + return ret;
> }
>
> int its_invall_vpe(struct its_vpe *vpe)
> diff --git a/include/kvm/arm_vgic.h b/include/kvm/arm_vgic.h
> index af4f09c02bf1..4dc58d7a0010 100644
> --- a/include/kvm/arm_vgic.h
> +++ b/include/kvm/arm_vgic.h
> @@ -396,7 +396,7 @@ int kvm_vgic_v4_set_forwarding(struct kvm *kvm, int irq,
> int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int irq,
> struct kvm_kernel_irq_routing_entry *irq_entry);
>
> -void kvm_vgic_v4_enable_doorbell(struct kvm_vcpu *vcpu);
> -void kvm_vgic_v4_disable_doorbell(struct kvm_vcpu *vcpu);
> +int vgic_v4_load(struct kvm_vcpu *vcpu);
> +int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db);
>
> #endif /* __KVM_ARM_VGIC_H */
> diff --git a/include/linux/irqchip/arm-gic-v4.h b/include/linux/irqchip/arm-gic-v4.h
> index e6b155713b47..ab1396afe08a 100644
> --- a/include/linux/irqchip/arm-gic-v4.h
> +++ b/include/linux/irqchip/arm-gic-v4.h
> @@ -35,6 +35,8 @@ struct its_vpe {
> /* Doorbell interrupt */
> int irq;
> irq_hw_number_t vpe_db_lpi;
> + /* VPE resident */
> + bool resident;
> /* VPE proxy mapping */
> int vpe_proxy_event;
> /*
> diff --git a/virt/kvm/arm/arm.c b/virt/kvm/arm/arm.c
> index 35a069815baf..4e69268621b6 100644
> --- a/virt/kvm/arm/arm.c
> +++ b/virt/kvm/arm/arm.c
> @@ -321,20 +321,24 @@ void kvm_arch_vcpu_blocking(struct kvm_vcpu *vcpu)
> /*
> * If we're about to block (most likely because we've just hit a
> * WFI), we need to sync back the state of the GIC CPU interface
> - * so that we have the lastest PMR and group enables. This ensures
> + * so that we have the latest PMR and group enables. This ensures
> * that kvm_arch_vcpu_runnable has up-to-date data to decide
> * whether we have pending interrupts.
> + *
> + * For the same reason, we want to tell GICv4 that we need
> + * doorbells to be signalled, should an interrupt become pending.
As I understand, and as indicated by removal of kvm_vgic_v4_enable_doorbell
below, we've now abstracted enabling the doorbell behind the concept of a
v4_put.
Why then, do we break that abstraction by adding this comment? Surely we just
want to indicate that we're done with ITS for now - do whatever you need to do.
This would have made more sense to me if the comment above was removed in this
patch rather than added.
> */
> preempt_disable();
> kvm_vgic_vmcr_sync(vcpu);
> + vgic_v4_put(vcpu, true);
> preempt_enable();
> -
> - kvm_vgic_v4_enable_doorbell(vcpu);
> }
>
> void kvm_arch_vcpu_unblocking(struct kvm_vcpu *vcpu)
> {
> - kvm_vgic_v4_disable_doorbell(vcpu);
> + preempt_disable();
> + vgic_v4_load(vcpu);
> + preempt_enable();
> }
>
> int kvm_arch_vcpu_init(struct kvm_vcpu *vcpu)
> diff --git a/virt/kvm/arm/vgic/vgic-v3.c b/virt/kvm/arm/vgic/vgic-v3.c
> index 8d69f007dd0c..48307a9eb1d8 100644
> --- a/virt/kvm/arm/vgic/vgic-v3.c
> +++ b/virt/kvm/arm/vgic/vgic-v3.c
> @@ -664,6 +664,8 @@ void vgic_v3_load(struct kvm_vcpu *vcpu)
>
> if (has_vhe())
> __vgic_v3_activate_traps(vcpu);
> +
> + WARN_ON(vgic_v4_load(vcpu));
> }
>
> void vgic_v3_vmcr_sync(struct kvm_vcpu *vcpu)
> @@ -676,6 +678,8 @@ void vgic_v3_vmcr_sync(struct kvm_vcpu *vcpu)
>
> void vgic_v3_put(struct kvm_vcpu *vcpu)
> {
> + WARN_ON(vgic_v4_put(vcpu, false));
> +
> vgic_v3_vmcr_sync(vcpu);
>
> kvm_call_hyp(__vgic_v3_save_aprs, vcpu);
> diff --git a/virt/kvm/arm/vgic/vgic-v4.c b/virt/kvm/arm/vgic/vgic-v4.c
> index 477af6aebb97..3a8a28854b13 100644
> --- a/virt/kvm/arm/vgic/vgic-v4.c
> +++ b/virt/kvm/arm/vgic/vgic-v4.c
> @@ -85,6 +85,10 @@ static irqreturn_t vgic_v4_doorbell_handler(int irq, void *info)
> {
> struct kvm_vcpu *vcpu = info;
>
> + /* We got the message, no need to fire again */
> + if (!irqd_irq_disabled(&irq_to_desc(irq)->irq_data))
> + disable_irq_nosync(irq);
> +
> vcpu->arch.vgic_cpu.vgic_v3.its_vpe.pending_last = true;
> kvm_make_request(KVM_REQ_IRQ_PENDING, vcpu);
> kvm_vcpu_kick(vcpu);
This is because the doorbell will fire each time any guest device interrupts,
however we only need to tell the guest just once that something has happened
right?
> @@ -192,20 +196,30 @@ void vgic_v4_teardown(struct kvm *kvm)
> its_vm->vpes = NULL;
> }
>
> -int vgic_v4_sync_hwstate(struct kvm_vcpu *vcpu)
> +int vgic_v4_put(struct kvm_vcpu *vcpu, bool need_db)
> {
> - if (!vgic_supports_direct_msis(vcpu->kvm))
> + struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
> + struct irq_desc *desc = irq_to_desc(vpe->irq);
> +
> + if (!vgic_supports_direct_msis(vcpu->kvm) || !vpe->resident)
> return 0;
Are we using !vpe->resident to avoid pointlessly doing work we've
already done?
>
> - return its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, false);
> + /*
> + * If blocking, a doorbell is required. Undo the nested
> + * disable_irq() calls...
> + */
> + while (need_db && irqd_irq_disabled(&desc->irq_data))
> + enable_irq(vpe->irq);
> +
> + return its_schedule_vpe(vpe, false);
> }
>
> -int vgic_v4_flush_hwstate(struct kvm_vcpu *vcpu)
> +int vgic_v4_load(struct kvm_vcpu *vcpu)
> {
> - int irq = vcpu->arch.vgic_cpu.vgic_v3.its_vpe.irq;
> + struct its_vpe *vpe = &vcpu->arch.vgic_cpu.vgic_v3.its_vpe;
> int err;
>
> - if (!vgic_supports_direct_msis(vcpu->kvm))
> + if (!vgic_supports_direct_msis(vcpu->kvm) || vpe->resident)
> return 0;
>
> /*
> @@ -214,11 +228,14 @@ int vgic_v4_flush_hwstate(struct kvm_vcpu *vcpu)
> * doc in drivers/irqchip/irq-gic-v4.c to understand how this
> * turns into a VMOVP command at the ITS level.
> */
> - err = irq_set_affinity(irq, cpumask_of(smp_processor_id()));
> + err = irq_set_affinity(vpe->irq, cpumask_of(smp_processor_id()));
> if (err)
> return err;
>
> - err = its_schedule_vpe(&vcpu->arch.vgic_cpu.vgic_v3.its_vpe, true);
> + /* Disabled the doorbell, as we're about to enter the guest */
> + disable_irq(vpe->irq);
> +
> + err = its_schedule_vpe(vpe, true);
> if (err)
> return err;
Given that the doorbell corresponds with vpe residency, it could make sense
to add a helper here that calls its_schedule_vpe and [disable,enable]_irq.
Though I see that vgic_v3_put calls vgic_v4_put with need_db=false. I wonder
what effect setting that to true would be for vgic_v3_put? Could it be known
that v3 won't set need_db to true?
Thanks,
Andrew Murray
>
> @@ -226,9 +243,7 @@ int vgic_v4_flush_hwstate(struct kvm_vcpu *vcpu)
> * Now that the VPE is resident, let's get rid of a potential
> * doorbell interrupt that would still be pending.
> */
> - err = irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, false);
> -
> - return err;
> + return irq_set_irqchip_state(vpe->irq, IRQCHIP_STATE_PENDING, false);
> }
>
> static struct vgic_its *vgic_get_its(struct kvm *kvm,
> @@ -335,21 +350,3 @@ int kvm_vgic_v4_unset_forwarding(struct kvm *kvm, int virq,
> mutex_unlock(&its->its_lock);
> return ret;
> }
> -
> -void kvm_vgic_v4_enable_doorbell(struct kvm_vcpu *vcpu)
> -{
> - if (vgic_supports_direct_msis(vcpu->kvm)) {
> - int irq = vcpu->arch.vgic_cpu.vgic_v3.its_vpe.irq;
> - if (irq)
> - enable_irq(irq);
> - }
> -}
> -
> -void kvm_vgic_v4_disable_doorbell(struct kvm_vcpu *vcpu)
> -{
> - if (vgic_supports_direct_msis(vcpu->kvm)) {
> - int irq = vcpu->arch.vgic_cpu.vgic_v3.its_vpe.irq;
> - if (irq)
> - disable_irq(irq);
> - }
> -}
> diff --git a/virt/kvm/arm/vgic/vgic.c b/virt/kvm/arm/vgic/vgic.c
> index 45a870cb63f5..99b02ca730a8 100644
> --- a/virt/kvm/arm/vgic/vgic.c
> +++ b/virt/kvm/arm/vgic/vgic.c
> @@ -857,8 +857,6 @@ void kvm_vgic_sync_hwstate(struct kvm_vcpu *vcpu)
> {
> struct vgic_cpu *vgic_cpu = &vcpu->arch.vgic_cpu;
>
> - WARN_ON(vgic_v4_sync_hwstate(vcpu));
> -
> /* An empty ap_list_head implies used_lrs == 0 */
> if (list_empty(&vcpu->arch.vgic_cpu.ap_list_head))
> return;
> @@ -882,8 +880,6 @@ static inline void vgic_restore_state(struct kvm_vcpu *vcpu)
> /* Flush our emulation state into the GIC hardware before entering the guest. */
> void kvm_vgic_flush_hwstate(struct kvm_vcpu *vcpu)
> {
> - WARN_ON(vgic_v4_flush_hwstate(vcpu));
> -
> /*
> * If there are no virtual interrupts active or pending for this
> * VCPU, then there is no work to do and we can bail out without
> diff --git a/virt/kvm/arm/vgic/vgic.h b/virt/kvm/arm/vgic/vgic.h
> index 83066a81b16a..c7fefd6b1c80 100644
> --- a/virt/kvm/arm/vgic/vgic.h
> +++ b/virt/kvm/arm/vgic/vgic.h
> @@ -316,7 +316,5 @@ void vgic_its_invalidate_cache(struct kvm *kvm);
> bool vgic_supports_direct_msis(struct kvm *kvm);
> int vgic_v4_init(struct kvm *kvm);
> void vgic_v4_teardown(struct kvm *kvm);
> -int vgic_v4_sync_hwstate(struct kvm_vcpu *vcpu);
> -int vgic_v4_flush_hwstate(struct kvm_vcpu *vcpu);
>
> #endif
> --
> 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
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