[PATCH] Input: pxrc - fix leak of usb_device
pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev() anywhere in the driver. The patch adds one to error handling code and to pxrc_disconnect(). Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov --- drivers/input/joystick/pxrc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c index 07a0dbd3ced2..0a31de63ac8e 100644 --- a/drivers/input/joystick/pxrc.c +++ b/drivers/input/joystick/pxrc.c @@ -221,6 +221,7 @@ static int pxrc_probe(struct usb_interface *intf, usb_free_urb(pxrc->urb); error: + usb_put_dev(pxrc->udev); return retval; } @@ -229,6 +230,7 @@ static void pxrc_disconnect(struct usb_interface *intf) struct pxrc *pxrc = usb_get_intfdata(intf); usb_free_urb(pxrc->urb); + usb_put_dev(pxrc->udev); usb_set_intfdata(intf, NULL); } -- 2.7.4
[PATCH] Input: pxrc - fix leak of usb_device
pxrc_probe() calls usb_get_dev(), but there is no usb_put_dev() anywhere in the driver. The patch adds one to error handling code and to pxrc_disconnect(). Found by Linux Driver Verification project (linuxtesting.org). Signed-off-by: Alexey Khoroshilov --- drivers/input/joystick/pxrc.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/input/joystick/pxrc.c b/drivers/input/joystick/pxrc.c index 07a0dbd3ced2..0a31de63ac8e 100644 --- a/drivers/input/joystick/pxrc.c +++ b/drivers/input/joystick/pxrc.c @@ -221,6 +221,7 @@ static int pxrc_probe(struct usb_interface *intf, usb_free_urb(pxrc->urb); error: + usb_put_dev(pxrc->udev); return retval; } @@ -229,6 +230,7 @@ static void pxrc_disconnect(struct usb_interface *intf) struct pxrc *pxrc = usb_get_intfdata(intf); usb_free_urb(pxrc->urb); + usb_put_dev(pxrc->udev); usb_set_intfdata(intf, NULL); } -- 2.7.4
Re: [PATCH v4 1/2] leds: core: Introduce generic pattern interface
Hi Baolin,
Thank you for the update.
On 07/13/2018 08:21 AM, Baolin Wang wrote:
From: Bjorn Andersson
Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.
This adds a new optional operator that LED class drivers can implement
if they support such functionality as well as a new device attribute to
configure the pattern for a given LED.
[Baolin Wang did some improvements.]
Signed-off-by: Bjorn Andersson
Signed-off-by: Baolin Wang
---
Changes from v3:
- Move the check in pattern_show() to of_led_classdev_register().
- Add more documentation to explain how to set/clear one pattern.
Changes from v2:
- Change kernel version to 4.19.
- Force user to return error pointer if failed to issue pattern_get().
- Use strstrip() to trim trailing newline.
- Other optimization.
Changes from v1:
- Add some comments suggested by Pavel.
- Change 'delta_t' can be 0.
Note: I removed the pattern repeat check and will get the repeat number by
adding
one extra file named 'pattern_repeat' according to previous discussion.
---
Documentation/ABI/testing/sysfs-class-led | 20 +
drivers/leds/led-class.c | 118 +
include/linux/leds.h | 19 +
3 files changed, 157 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led
b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..f4b73ad 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,23 @@ Description:
gpio and backlight triggers. In case of the backlight trigger,
it is useful when driving a LED which is intended to indicate
a device in a standby like state.
+
+What: /sys/class/leds//pattern
+Date: July 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern for the LED, for LED hardware that support
+ altering the brightness as a function of time.
+
+ The pattern is given by a series of tuples, of brightness and
+ duration (ms). The LED is expected to traverse the series and
+ each brightness value for the specified duration. Duration of
+ 0 means brightness should immediately change to new value.
+
+ As LED hardware might have different capabilities and precision
+ the requested pattern might be slighly adjusted by the driver
+ and the resulting pattern of such operation should be returned
+ when this file is read.
+
+ Writing non-empty string to this file will active the pattern,
s/active/activate/
+ and empty string will disable the pattern.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..0992a0e 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,119 @@ static ssize_t max_brightness_show(struct device *dev,
}
static DEVICE_ATTR_RO(max_brightness);
+static ssize_t pattern_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_pattern *pattern;
+ size_t offset = 0;
+ int count, n, i;
+
+ pattern = led_cdev->pattern_get(led_cdev, );
+ if (IS_ERR(pattern))
+ return PTR_ERR(pattern);
+
+ for (i = 0; i < count; i++) {
+ n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
+pattern[i].brightness, pattern[i].delta_t);
+
+ if (offset + n >= PAGE_SIZE)
+ goto err_nospc;
+
+ offset += n;
+ }
+
+ buf[offset - 1] = '\n';
+
+ kfree(pattern);
+ return offset;
+
+err_nospc:
+ kfree(pattern);
+ return -ENOSPC;
+}
+
+static ssize_t pattern_store(struct device *dev,
+struct device_attribute *attr,
+const char *buf, size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_pattern *pattern = NULL;
+ char *sbegin, *elem, *s;
+ unsigned long val;
+ int ret = 0, len = 0;
+ bool odd = true;
+
+ sbegin = kstrndup(buf, size, GFP_KERNEL);
+ if (!sbegin)
+ return -ENOMEM;
+
+ /*
+* Trim trailing newline, if the remaining string is empty,
+* clear the pattern.
+*/
+ s = strstrip(sbegin);
+ if (!*s) {
+ if (led_cdev->pattern_clear)
+ ret = led_cdev->pattern_clear(led_cdev);
Why you don't require pattern_clear op to be initialized?
Without it there seems to be no other way to disable the pattern.
Please fail in led_classdev_register() if it is NULL, similarly
as when pattern_get is NULL.
+ goto out;
+ }
Re: [PATCH v4 1/2] leds: core: Introduce generic pattern interface
Hi Baolin,
Thank you for the update.
On 07/13/2018 08:21 AM, Baolin Wang wrote:
From: Bjorn Andersson
Some LED controllers have support for autonomously controlling
brightness over time, according to some preprogrammed pattern or
function.
This adds a new optional operator that LED class drivers can implement
if they support such functionality as well as a new device attribute to
configure the pattern for a given LED.
[Baolin Wang did some improvements.]
Signed-off-by: Bjorn Andersson
Signed-off-by: Baolin Wang
---
Changes from v3:
- Move the check in pattern_show() to of_led_classdev_register().
- Add more documentation to explain how to set/clear one pattern.
Changes from v2:
- Change kernel version to 4.19.
- Force user to return error pointer if failed to issue pattern_get().
- Use strstrip() to trim trailing newline.
- Other optimization.
Changes from v1:
- Add some comments suggested by Pavel.
- Change 'delta_t' can be 0.
Note: I removed the pattern repeat check and will get the repeat number by
adding
one extra file named 'pattern_repeat' according to previous discussion.
---
Documentation/ABI/testing/sysfs-class-led | 20 +
drivers/leds/led-class.c | 118 +
include/linux/leds.h | 19 +
3 files changed, 157 insertions(+)
diff --git a/Documentation/ABI/testing/sysfs-class-led
b/Documentation/ABI/testing/sysfs-class-led
index 5f67f7a..f4b73ad 100644
--- a/Documentation/ABI/testing/sysfs-class-led
+++ b/Documentation/ABI/testing/sysfs-class-led
@@ -61,3 +61,23 @@ Description:
gpio and backlight triggers. In case of the backlight trigger,
it is useful when driving a LED which is intended to indicate
a device in a standby like state.
+
+What: /sys/class/leds//pattern
+Date: July 2018
+KernelVersion: 4.19
+Description:
+ Specify a pattern for the LED, for LED hardware that support
+ altering the brightness as a function of time.
+
+ The pattern is given by a series of tuples, of brightness and
+ duration (ms). The LED is expected to traverse the series and
+ each brightness value for the specified duration. Duration of
+ 0 means brightness should immediately change to new value.
+
+ As LED hardware might have different capabilities and precision
+ the requested pattern might be slighly adjusted by the driver
+ and the resulting pattern of such operation should be returned
+ when this file is read.
+
+ Writing non-empty string to this file will active the pattern,
s/active/activate/
+ and empty string will disable the pattern.
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c7e348..0992a0e 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -74,6 +74,119 @@ static ssize_t max_brightness_show(struct device *dev,
}
static DEVICE_ATTR_RO(max_brightness);
+static ssize_t pattern_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_pattern *pattern;
+ size_t offset = 0;
+ int count, n, i;
+
+ pattern = led_cdev->pattern_get(led_cdev, );
+ if (IS_ERR(pattern))
+ return PTR_ERR(pattern);
+
+ for (i = 0; i < count; i++) {
+ n = snprintf(buf + offset, PAGE_SIZE - offset, "%d %d ",
+pattern[i].brightness, pattern[i].delta_t);
+
+ if (offset + n >= PAGE_SIZE)
+ goto err_nospc;
+
+ offset += n;
+ }
+
+ buf[offset - 1] = '\n';
+
+ kfree(pattern);
+ return offset;
+
+err_nospc:
+ kfree(pattern);
+ return -ENOSPC;
+}
+
+static ssize_t pattern_store(struct device *dev,
+struct device_attribute *attr,
+const char *buf, size_t size)
+{
+ struct led_classdev *led_cdev = dev_get_drvdata(dev);
+ struct led_pattern *pattern = NULL;
+ char *sbegin, *elem, *s;
+ unsigned long val;
+ int ret = 0, len = 0;
+ bool odd = true;
+
+ sbegin = kstrndup(buf, size, GFP_KERNEL);
+ if (!sbegin)
+ return -ENOMEM;
+
+ /*
+* Trim trailing newline, if the remaining string is empty,
+* clear the pattern.
+*/
+ s = strstrip(sbegin);
+ if (!*s) {
+ if (led_cdev->pattern_clear)
+ ret = led_cdev->pattern_clear(led_cdev);
Why you don't require pattern_clear op to be initialized?
Without it there seems to be no other way to disable the pattern.
Please fail in led_classdev_register() if it is NULL, similarly
as when pattern_get is NULL.
+ goto out;
+ }
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Fri, 13 Jul 2018 09:24:44 -0400 Pavel Tatashin
wrote:
> On 07/13/2018 09:17 AM, Oscar Salvador wrote:
> > On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
> >> +static void *sparsemap_buf __meminitdata;
> >> +static void *sparsemap_buf_end __meminitdata;
> >> +
> >> +void __init sparse_buffer_init(unsigned long size, int nid)
> >> +{
> >> + BUG_ON(sparsemap_buf);
> >
> > Why do we need a BUG_ON() here?
> > Looking at the code I cannot really see how we can end up with
> > sparsemap_buf being NULL.
> > Is it just for over-protection?
>
> This checks that we do not accidentally leak memory by calling
> sparse_buffer_init() consequently without sparse_buffer_fini() in-between.
A memory leak isn't serious enough to justify crashing the kernel.
Therefore
--- a/mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix-fix
+++ a/mm/sparse.c
@@ -469,7 +469,7 @@ static void *sparsemap_buf_end __meminit
void __init sparse_buffer_init(unsigned long size, int nid)
{
- BUG_ON(sparsemap_buf);
+ WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */
sparsemap_buf =
memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
_
Re: [PATCH v5 1/5] mm/sparse: abstract sparse buffer allocations
On Fri, 13 Jul 2018 09:24:44 -0400 Pavel Tatashin
wrote:
> On 07/13/2018 09:17 AM, Oscar Salvador wrote:
> > On Thu, Jul 12, 2018 at 04:37:26PM -0400, Pavel Tatashin wrote:
> >> +static void *sparsemap_buf __meminitdata;
> >> +static void *sparsemap_buf_end __meminitdata;
> >> +
> >> +void __init sparse_buffer_init(unsigned long size, int nid)
> >> +{
> >> + BUG_ON(sparsemap_buf);
> >
> > Why do we need a BUG_ON() here?
> > Looking at the code I cannot really see how we can end up with
> > sparsemap_buf being NULL.
> > Is it just for over-protection?
>
> This checks that we do not accidentally leak memory by calling
> sparse_buffer_init() consequently without sparse_buffer_fini() in-between.
A memory leak isn't serious enough to justify crashing the kernel.
Therefore
--- a/mm/sparse.c~mm-sparse-abstract-sparse-buffer-allocations-fix-fix
+++ a/mm/sparse.c
@@ -469,7 +469,7 @@ static void *sparsemap_buf_end __meminit
void __init sparse_buffer_init(unsigned long size, int nid)
{
- BUG_ON(sparsemap_buf);
+ WARN_ON(sparsemap_buf); /* forgot to call sparse_buffer_fini()? */
sparsemap_buf =
memblock_virt_alloc_try_nid_raw(size, PAGE_SIZE,
__pa(MAX_DMA_ADDRESS),
_
[GIT PULL] timer fixes
Linus,
Please pull the latest timers-urgent-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
timers-urgent-for-linus
# HEAD: 5e18e412973d6bb1804de1d4d30a891c774b006e clocksource:
arm_arch_timer: Set arch_mem_timer cpumask to cpu_possible_mask
A clocksource driver fix and a revert.
Thanks,
Ingo
-->
Sudeep Holla (2):
Revert "tick: Prefer a lower rating device only if it's CPU local device"
clocksource: arm_arch_timer: Set arch_mem_timer cpumask to
cpu_possible_mask
drivers/clocksource/arm_arch_timer.c | 2 +-
kernel/time/tick-common.c| 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index 57cb2f00fc07..d8c7f5750cdb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -735,7 +735,7 @@ static void __arch_timer_setup(unsigned type,
clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
clk->name = "arch_mem_timer";
clk->rating = 400;
- clk->cpumask = cpu_all_mask;
+ clk->cpumask = cpu_possible_mask;
if (arch_timer_mem_use_virtual) {
clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
clk->set_state_oneshot_stopped =
arch_timer_shutdown_virt_mem;
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index b7005dd21ec1..14de3727b18e 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -277,8 +277,7 @@ static bool tick_check_preferred(struct clock_event_device
*curdev,
*/
return !curdev ||
newdev->rating > curdev->rating ||
- (!cpumask_equal(curdev->cpumask, newdev->cpumask) &&
- !tick_check_percpu(curdev, newdev, smp_processor_id()));
+ !cpumask_equal(curdev->cpumask, newdev->cpumask);
}
/*
[GIT PULL] timer fixes
Linus,
Please pull the latest timers-urgent-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
timers-urgent-for-linus
# HEAD: 5e18e412973d6bb1804de1d4d30a891c774b006e clocksource:
arm_arch_timer: Set arch_mem_timer cpumask to cpu_possible_mask
A clocksource driver fix and a revert.
Thanks,
Ingo
-->
Sudeep Holla (2):
Revert "tick: Prefer a lower rating device only if it's CPU local device"
clocksource: arm_arch_timer: Set arch_mem_timer cpumask to
cpu_possible_mask
drivers/clocksource/arm_arch_timer.c | 2 +-
kernel/time/tick-common.c| 3 +--
2 files changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/clocksource/arm_arch_timer.c
b/drivers/clocksource/arm_arch_timer.c
index 57cb2f00fc07..d8c7f5750cdb 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -735,7 +735,7 @@ static void __arch_timer_setup(unsigned type,
clk->features |= CLOCK_EVT_FEAT_DYNIRQ;
clk->name = "arch_mem_timer";
clk->rating = 400;
- clk->cpumask = cpu_all_mask;
+ clk->cpumask = cpu_possible_mask;
if (arch_timer_mem_use_virtual) {
clk->set_state_shutdown = arch_timer_shutdown_virt_mem;
clk->set_state_oneshot_stopped =
arch_timer_shutdown_virt_mem;
diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
index b7005dd21ec1..14de3727b18e 100644
--- a/kernel/time/tick-common.c
+++ b/kernel/time/tick-common.c
@@ -277,8 +277,7 @@ static bool tick_check_preferred(struct clock_event_device
*curdev,
*/
return !curdev ||
newdev->rating > curdev->rating ||
- (!cpumask_equal(curdev->cpumask, newdev->cpumask) &&
- !tick_check_percpu(curdev, newdev, smp_processor_id()));
+ !cpumask_equal(curdev->cpumask, newdev->cpumask);
}
/*
[GIT PULL] perf fixes
Linus,
Please pull the latest perf-urgent-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
perf-urgent-for-linus
# HEAD: 6e1d33b24a2b4aebcb05782e937ebc9a7a4a3f50 Merge tag
'perf-urgent-for-mingo-4.18-20180711' of
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent
Misc tooling fixes: ptyhon3 related fixes, gcc8 fix, bashism fixes and some
other
smaller fixes.
Thanks,
Ingo
-->
Janne Huttunen (1):
perf script python: Fix dict reference counting
Jeremy Cline (7):
perf tools: Generate a Python script compatible with Python 2 and 3
perf scripts python: Add Python 3 support to Core.py
perf scripts python: Add Python 3 support to SchedGui.py
perf scripts python: Add Python 3 support to Util.py
perf scripts python: Add Python 3 support to sched-migration.py
perf scripts python: Add Python 3 support to EventClass.py
perf tools: Use python-config --includes rather than --cflags
Jiri Olsa (2):
perf tools: Fix compilation errors on gcc8
perf stat: Fix --interval_clear option
Kim Phillips (4):
perf test shell: Replace '|&' with '2>&1 |' to work with more shells
perf test shell: Make perf's inet_pton test more portable
perf llvm-utils: Remove bashism from kernel include fetch script
perf test shell: Prevent temporary editor files from being considered
test scripts
tools/perf/Makefile.config | 3 +-
tools/perf/arch/x86/util/perf_regs.c | 2 +-
tools/perf/builtin-stat.c | 2 +-
tools/perf/jvmti/jvmti_agent.c | 3 +-
.../python/Perf-Trace-Util/lib/Perf/Trace/Core.py | 40 +-
.../Perf-Trace-Util/lib/Perf/Trace/EventClass.py | 4 ++-
.../Perf-Trace-Util/lib/Perf/Trace/SchedGui.py | 2 +-
.../python/Perf-Trace-Util/lib/Perf/Trace/Util.py | 11 +++---
tools/perf/scripts/python/sched-migration.py | 14 +---
tools/perf/tests/builtin-test.c| 2 +-
.../tests/shell/record+probe_libc_inet_pton.sh | 37 +++-
tools/perf/tests/shell/trace+probe_vfs_getname.sh | 2 +-
tools/perf/util/llvm-utils.c | 6 ++--
.../util/scripting-engines/trace-event-python.c| 37 +---
14 files changed, 84 insertions(+), 81 deletions(-)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index b5ac356ba323..f5a3b402589e 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -207,8 +207,7 @@ ifdef PYTHON_CONFIG
PYTHON_EMBED_LDOPTS := $(shell $(PYTHON_CONFIG_SQ) --ldflags 2>/dev/null)
PYTHON_EMBED_LDFLAGS := $(call strip-libs,$(PYTHON_EMBED_LDOPTS))
PYTHON_EMBED_LIBADD := $(call grep-libs,$(PYTHON_EMBED_LDOPTS)) -lutil
- PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --cflags 2>/dev/null)
- PYTHON_EMBED_CCOPTS := $(filter-out -specs=%,$(PYTHON_EMBED_CCOPTS))
+ PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --includes 2>/dev/null)
FLAGS_PYTHON_EMBED := $(PYTHON_EMBED_CCOPTS) $(PYTHON_EMBED_LDOPTS)
endif
diff --git a/tools/perf/arch/x86/util/perf_regs.c
b/tools/perf/arch/x86/util/perf_regs.c
index 4b2caf6d48e7..fead6b3b4206 100644
--- a/tools/perf/arch/x86/util/perf_regs.c
+++ b/tools/perf/arch/x86/util/perf_regs.c
@@ -226,7 +226,7 @@ int arch_sdt_arg_parse_op(char *old_op, char **new_op)
else if (rm[2].rm_so != rm[2].rm_eo)
prefix[0] = '+';
else
- strncpy(prefix, "+0", 2);
+ scnprintf(prefix, sizeof(prefix), "+0");
}
/* Rename register */
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 22547a490e1f..05be023c3f0e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1742,7 +1742,7 @@ static void print_interval(char *prefix, struct timespec
*ts)
}
}
- if ((num_print_interval == 0 && metric_only) || interval_clear)
+ if ((num_print_interval == 0 || interval_clear) && metric_only)
print_metric_headers(" ", true);
if (++num_print_interval == 25)
num_print_interval = 0;
diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
index 0c6d1002b524..ac1bcdc17dae 100644
--- a/tools/perf/jvmti/jvmti_agent.c
+++ b/tools/perf/jvmti/jvmti_agent.c
@@ -35,6 +35,7 @@
#include
#include /* for gettid() */
#include
+#include
#include "jvmti_agent.h"
#include "../util/jitdump.h"
@@ -249,7 +250,7 @@ void *jvmti_open(void)
/*
* jitdump file name
*/
- snprintf(dump_path, PATH_MAX, "%s/jit-%i.dump", jit_path, getpid());
+ scnprintf(dump_path, PATH_MAX, "%s/jit-%i.dump", jit_path, getpid());
fd = open(dump_path, O_CREAT|O_TRUNC|O_RDWR, 0666);
if (fd == -1)
diff --git
[GIT PULL] perf fixes
Linus,
Please pull the latest perf-urgent-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
perf-urgent-for-linus
# HEAD: 6e1d33b24a2b4aebcb05782e937ebc9a7a4a3f50 Merge tag
'perf-urgent-for-mingo-4.18-20180711' of
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/urgent
Misc tooling fixes: ptyhon3 related fixes, gcc8 fix, bashism fixes and some
other
smaller fixes.
Thanks,
Ingo
-->
Janne Huttunen (1):
perf script python: Fix dict reference counting
Jeremy Cline (7):
perf tools: Generate a Python script compatible with Python 2 and 3
perf scripts python: Add Python 3 support to Core.py
perf scripts python: Add Python 3 support to SchedGui.py
perf scripts python: Add Python 3 support to Util.py
perf scripts python: Add Python 3 support to sched-migration.py
perf scripts python: Add Python 3 support to EventClass.py
perf tools: Use python-config --includes rather than --cflags
Jiri Olsa (2):
perf tools: Fix compilation errors on gcc8
perf stat: Fix --interval_clear option
Kim Phillips (4):
perf test shell: Replace '|&' with '2>&1 |' to work with more shells
perf test shell: Make perf's inet_pton test more portable
perf llvm-utils: Remove bashism from kernel include fetch script
perf test shell: Prevent temporary editor files from being considered
test scripts
tools/perf/Makefile.config | 3 +-
tools/perf/arch/x86/util/perf_regs.c | 2 +-
tools/perf/builtin-stat.c | 2 +-
tools/perf/jvmti/jvmti_agent.c | 3 +-
.../python/Perf-Trace-Util/lib/Perf/Trace/Core.py | 40 +-
.../Perf-Trace-Util/lib/Perf/Trace/EventClass.py | 4 ++-
.../Perf-Trace-Util/lib/Perf/Trace/SchedGui.py | 2 +-
.../python/Perf-Trace-Util/lib/Perf/Trace/Util.py | 11 +++---
tools/perf/scripts/python/sched-migration.py | 14 +---
tools/perf/tests/builtin-test.c| 2 +-
.../tests/shell/record+probe_libc_inet_pton.sh | 37 +++-
tools/perf/tests/shell/trace+probe_vfs_getname.sh | 2 +-
tools/perf/util/llvm-utils.c | 6 ++--
.../util/scripting-engines/trace-event-python.c| 37 +---
14 files changed, 84 insertions(+), 81 deletions(-)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config
index b5ac356ba323..f5a3b402589e 100644
--- a/tools/perf/Makefile.config
+++ b/tools/perf/Makefile.config
@@ -207,8 +207,7 @@ ifdef PYTHON_CONFIG
PYTHON_EMBED_LDOPTS := $(shell $(PYTHON_CONFIG_SQ) --ldflags 2>/dev/null)
PYTHON_EMBED_LDFLAGS := $(call strip-libs,$(PYTHON_EMBED_LDOPTS))
PYTHON_EMBED_LIBADD := $(call grep-libs,$(PYTHON_EMBED_LDOPTS)) -lutil
- PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --cflags 2>/dev/null)
- PYTHON_EMBED_CCOPTS := $(filter-out -specs=%,$(PYTHON_EMBED_CCOPTS))
+ PYTHON_EMBED_CCOPTS := $(shell $(PYTHON_CONFIG_SQ) --includes 2>/dev/null)
FLAGS_PYTHON_EMBED := $(PYTHON_EMBED_CCOPTS) $(PYTHON_EMBED_LDOPTS)
endif
diff --git a/tools/perf/arch/x86/util/perf_regs.c
b/tools/perf/arch/x86/util/perf_regs.c
index 4b2caf6d48e7..fead6b3b4206 100644
--- a/tools/perf/arch/x86/util/perf_regs.c
+++ b/tools/perf/arch/x86/util/perf_regs.c
@@ -226,7 +226,7 @@ int arch_sdt_arg_parse_op(char *old_op, char **new_op)
else if (rm[2].rm_so != rm[2].rm_eo)
prefix[0] = '+';
else
- strncpy(prefix, "+0", 2);
+ scnprintf(prefix, sizeof(prefix), "+0");
}
/* Rename register */
diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 22547a490e1f..05be023c3f0e 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -1742,7 +1742,7 @@ static void print_interval(char *prefix, struct timespec
*ts)
}
}
- if ((num_print_interval == 0 && metric_only) || interval_clear)
+ if ((num_print_interval == 0 || interval_clear) && metric_only)
print_metric_headers(" ", true);
if (++num_print_interval == 25)
num_print_interval = 0;
diff --git a/tools/perf/jvmti/jvmti_agent.c b/tools/perf/jvmti/jvmti_agent.c
index 0c6d1002b524..ac1bcdc17dae 100644
--- a/tools/perf/jvmti/jvmti_agent.c
+++ b/tools/perf/jvmti/jvmti_agent.c
@@ -35,6 +35,7 @@
#include
#include /* for gettid() */
#include
+#include
#include "jvmti_agent.h"
#include "../util/jitdump.h"
@@ -249,7 +250,7 @@ void *jvmti_open(void)
/*
* jitdump file name
*/
- snprintf(dump_path, PATH_MAX, "%s/jit-%i.dump", jit_path, getpid());
+ scnprintf(dump_path, PATH_MAX, "%s/jit-%i.dump", jit_path, getpid());
fd = open(dump_path, O_CREAT|O_TRUNC|O_RDWR, 0666);
if (fd == -1)
diff --git
Re: [PATCH] arm64: build with baremetal linker target instead of Linux when available
On Fri, Jul 13, 2018 at 12:21 PM, Laura Abbott wrote: > On 07/13/2018 08:30 AM, Olof Johansson wrote: >> >> Not all toolchains have the baremetal elf targets, RedHat/Fedora ones >> in particular. So, probe for whether it's available and use the previous >> (linux) targets if it isn't. >> > > > For the Fedora toolchains: > > Tested-by: Laura Abbott Thanks! -Olof
Re: [PATCH] arm64: build with baremetal linker target instead of Linux when available
On Fri, Jul 13, 2018 at 12:21 PM, Laura Abbott wrote: > On 07/13/2018 08:30 AM, Olof Johansson wrote: >> >> Not all toolchains have the baremetal elf targets, RedHat/Fedora ones >> in particular. So, probe for whether it's available and use the previous >> (linux) targets if it isn't. >> > > > For the Fedora toolchains: > > Tested-by: Laura Abbott Thanks! -Olof
[GIT PULL] EFI fix
Linus, Please pull the latest efi-urgent-for-linus git tree from: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-for-linus # HEAD: e296701800f30d260a66f8aa1971b5b1bc3d2f81 efi/x86: Fix mixed mode reboot loop by removing pointless call to PciIo->Attributes() Fix a UEFI mixed mode (64-bit kernel on 32-bit UEFI) reboot loop regression. Thanks, Ingo --> Ard Biesheuvel (1): efi/x86: Fix mixed mode reboot loop by removing pointless call to PciIo->Attributes() arch/x86/boot/compressed/eboot.c | 12 +++- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index e57665b4ba1c..e98522ea6f09 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -114,18 +114,12 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom) struct pci_setup_rom *rom = NULL; efi_status_t status; unsigned long size; - uint64_t attributes, romsize; + uint64_t romsize; void *romimage; - status = efi_call_proto(efi_pci_io_protocol, attributes, pci, - EfiPciIoAttributeOperationGet, 0ULL, - ); - if (status != EFI_SUCCESS) - return status; - /* -* Some firmware images contain EFI function pointers at the place where the -* romimage and romsize fields are supposed to be. Typically the EFI +* Some firmware images contain EFI function pointers at the place where +* the romimage and romsize fields are supposed to be. Typically the EFI * code is mapped at high addresses, translating to an unrealistically * large romsize. The UEFI spec limits the size of option ROMs to 16 * MiB so we reject any ROMs over 16 MiB in size to catch this.
[GIT PULL] EFI fix
Linus, Please pull the latest efi-urgent-for-linus git tree from: git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git efi-urgent-for-linus # HEAD: e296701800f30d260a66f8aa1971b5b1bc3d2f81 efi/x86: Fix mixed mode reboot loop by removing pointless call to PciIo->Attributes() Fix a UEFI mixed mode (64-bit kernel on 32-bit UEFI) reboot loop regression. Thanks, Ingo --> Ard Biesheuvel (1): efi/x86: Fix mixed mode reboot loop by removing pointless call to PciIo->Attributes() arch/x86/boot/compressed/eboot.c | 12 +++- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c index e57665b4ba1c..e98522ea6f09 100644 --- a/arch/x86/boot/compressed/eboot.c +++ b/arch/x86/boot/compressed/eboot.c @@ -114,18 +114,12 @@ __setup_efi_pci(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom) struct pci_setup_rom *rom = NULL; efi_status_t status; unsigned long size; - uint64_t attributes, romsize; + uint64_t romsize; void *romimage; - status = efi_call_proto(efi_pci_io_protocol, attributes, pci, - EfiPciIoAttributeOperationGet, 0ULL, - ); - if (status != EFI_SUCCESS) - return status; - /* -* Some firmware images contain EFI function pointers at the place where the -* romimage and romsize fields are supposed to be. Typically the EFI +* Some firmware images contain EFI function pointers at the place where +* the romimage and romsize fields are supposed to be. Typically the EFI * code is mapped at high addresses, translating to an unrealistically * large romsize. The UEFI spec limits the size of option ROMs to 16 * MiB so we reject any ROMs over 16 MiB in size to catch this.
Re: [PATCH v2] tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire
On Fri, Jul 13, 2018 at 06:42:39PM +0200, Peter Zijlstra wrote: > > Hi Michael, > > On Fri, Jul 13, 2018 at 11:15:26PM +1000, Michael Ellerman wrote: > > I reran some numbers today with some slightly updated tests. > > > > It varies quite a bit across machines and CPU revisions. > > > > On one I get: > > > > Lock/UnlockTime Time %Total Cycles Cycles Cycles > > Delta > > lwsync/lwsync 79,290,859,955 100.0 % 290,160,065,087 145 - > > lwsync/sync104,903,703,237 132.3 % 383,966,199,430 192 47 > > > > Another: > > > > Lock/UnlockTime Time %Total Cycles Cycles Cycles > > Delta > > lwsync/lwsync 71,662,395,722 100.0 % 252,403,777,715 126 - > > lwsync/sync84,932,987,977 118.5 % 299,141,951,285 150 23 > > > > > > So 18-32% slower, or 23-47 cycles. > > Very good info. Note that another option is to put the SYNC in lock() it > doesn't really matter which of the two primitives gets it. I don't > suppose it really matters for timing either way around. > > > Next week I can do some macro benchmarks, to see if it's actually > > detectable at all. > > > > The other question is how they behave on a heavily loaded system. > > > > > > My personal preference would be to switch to sync, we don't want to be > > the only arch finding (or not finding!) exotic ordering bugs. > > > > But we'd also rather not make our slow locks any slower than they have > > to be. > > I completely understand, but I'll get you beer (lots) if you do manage > to make SYNC happen :-) :-) One trivia about seems due: it's of course very easy to stick a full or a "tso" fence in one's spin_lock() implementation, or to tight the semantics of such a primitive; removing this fence, or weakening the semantics is another matter... (/me reminding about that spin_is_locked() discussion...) Andrea
Re: [PATCH v2] tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire
On Fri, Jul 13, 2018 at 06:42:39PM +0200, Peter Zijlstra wrote: > > Hi Michael, > > On Fri, Jul 13, 2018 at 11:15:26PM +1000, Michael Ellerman wrote: > > I reran some numbers today with some slightly updated tests. > > > > It varies quite a bit across machines and CPU revisions. > > > > On one I get: > > > > Lock/UnlockTime Time %Total Cycles Cycles Cycles > > Delta > > lwsync/lwsync 79,290,859,955 100.0 % 290,160,065,087 145 - > > lwsync/sync104,903,703,237 132.3 % 383,966,199,430 192 47 > > > > Another: > > > > Lock/UnlockTime Time %Total Cycles Cycles Cycles > > Delta > > lwsync/lwsync 71,662,395,722 100.0 % 252,403,777,715 126 - > > lwsync/sync84,932,987,977 118.5 % 299,141,951,285 150 23 > > > > > > So 18-32% slower, or 23-47 cycles. > > Very good info. Note that another option is to put the SYNC in lock() it > doesn't really matter which of the two primitives gets it. I don't > suppose it really matters for timing either way around. > > > Next week I can do some macro benchmarks, to see if it's actually > > detectable at all. > > > > The other question is how they behave on a heavily loaded system. > > > > > > My personal preference would be to switch to sync, we don't want to be > > the only arch finding (or not finding!) exotic ordering bugs. > > > > But we'd also rather not make our slow locks any slower than they have > > to be. > > I completely understand, but I'll get you beer (lots) if you do manage > to make SYNC happen :-) :-) One trivia about seems due: it's of course very easy to stick a full or a "tso" fence in one's spin_lock() implementation, or to tight the semantics of such a primitive; removing this fence, or weakening the semantics is another matter... (/me reminding about that spin_is_locked() discussion...) Andrea
Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
On 07/13/2018 12:40 PM, Arjan van de Ven wrote: > On 7/13/2018 12:19 PM, patrickg wrote: >> This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT >> calibration methods should the user desire to. >> >> The current ordering in ML x86 tsc is to calibrate in the order listed >> above; returning whenever there's a successful calibration. However there >> are certain BIOS/HW Designs for overclocking that cause the TSC to change >> along with the max core clock; and simple 'trusting' calibration >> methodologies will lead to the TSC running 'faster' and eventually, TSC >> instability. >> > > > that would be a real violation of the contract between cpu and OS: tsc is not > supposed to change for the duration of the boot With the methodology used; the TSC is still invariant; it's just running faster than the CPUID math calculates. > >> I only know that there's a use-case for me to want to be able to skip CPUID >> calibration, however I included args for skipping all the rest just so that >> all functionality is covered in the long run instead of just one use-case. > > wouldn't it be better to start the detailed calibration with the value from > CPUID instead; that way we also properly calibrate spread spectrum etc... > > I thought we switched to that recently to be honest... Are you referencing: 1bf8915ae5156dff439d2c65314bd8fdde1b83bf - x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID However since it's returning at CPUID calibration during native_calibrate_cpu(); it's not compared after-the-fact, leading to the TSC to use the 'slower' number returned by CPUID. Now keep in mind; I dunno if there was any reason to explicitly not want to utilize the PIT calib sequences on SKL. That'd be a factor for this. Would comparing the number after the fact; then if there's a significant difference between PIT and MSR/CPUID, defaulting to the 'faster' value be a better solution?
Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
On 07/13/2018 12:40 PM, Arjan van de Ven wrote: > On 7/13/2018 12:19 PM, patrickg wrote: >> This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT >> calibration methods should the user desire to. >> >> The current ordering in ML x86 tsc is to calibrate in the order listed >> above; returning whenever there's a successful calibration. However there >> are certain BIOS/HW Designs for overclocking that cause the TSC to change >> along with the max core clock; and simple 'trusting' calibration >> methodologies will lead to the TSC running 'faster' and eventually, TSC >> instability. >> > > > that would be a real violation of the contract between cpu and OS: tsc is not > supposed to change for the duration of the boot With the methodology used; the TSC is still invariant; it's just running faster than the CPUID math calculates. > >> I only know that there's a use-case for me to want to be able to skip CPUID >> calibration, however I included args for skipping all the rest just so that >> all functionality is covered in the long run instead of just one use-case. > > wouldn't it be better to start the detailed calibration with the value from > CPUID instead; that way we also properly calibrate spread spectrum etc... > > I thought we switched to that recently to be honest... Are you referencing: 1bf8915ae5156dff439d2c65314bd8fdde1b83bf - x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID However since it's returning at CPUID calibration during native_calibrate_cpu(); it's not compared after-the-fact, leading to the TSC to use the 'slower' number returned by CPUID. Now keep in mind; I dunno if there was any reason to explicitly not want to utilize the PIT calib sequences on SKL. That'd be a factor for this. Would comparing the number after the fact; then if there's a significant difference between PIT and MSR/CPUID, defaulting to the 'faster' value be a better solution?
Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
On 07/13/2018 12:40 PM, Arjan van de Ven wrote: > On 7/13/2018 12:19 PM, patrickg wrote: >> This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT >> calibration methods should the user desire to. >> >> The current ordering in ML x86 tsc is to calibrate in the order listed >> above; returning whenever there's a successful calibration. However there >> are certain BIOS/HW Designs for overclocking that cause the TSC to change >> along with the max core clock; and simple 'trusting' calibration >> methodologies will lead to the TSC running 'faster' and eventually, TSC >> instability. >> > > > that would be a real violation of the contract between cpu and OS: tsc is not > supposed to change for the duration of the boot With the methodology used; the TSC is still invariant; it's just running faster than the CPUID math calculates. > >> I only know that there's a use-case for me to want to be able to skip CPUID >> calibration, however I included args for skipping all the rest just so that >> all functionality is covered in the long run instead of just one use-case. > > wouldn't it be better to start the detailed calibration with the value from > CPUID instead; that way we also properly calibrate spread spectrum etc... > > I thought we switched to that recently to be honest... Are you referencing: 1bf8915ae5156dff439d2c65314bd8fdde1b83bf - x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID However since it's returning at CPUID calibration during native_calibrate_cpu(); it's not compared after-the-fact, leading to the TSC to use the 'slower' number returned by CPUID. Now keep in mind; I dunno if there was any reason to explicitly not want to utilize the PIT calib sequences on SKL. That'd be a factor for this. Would comparing the number after the fact; then if there's a significant difference between PIT and MSR/CPUID, defaulting to the 'faster' value be a better solution?
Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
On 07/13/2018 12:40 PM, Arjan van de Ven wrote: > On 7/13/2018 12:19 PM, patrickg wrote: >> This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT >> calibration methods should the user desire to. >> >> The current ordering in ML x86 tsc is to calibrate in the order listed >> above; returning whenever there's a successful calibration. However there >> are certain BIOS/HW Designs for overclocking that cause the TSC to change >> along with the max core clock; and simple 'trusting' calibration >> methodologies will lead to the TSC running 'faster' and eventually, TSC >> instability. >> > > > that would be a real violation of the contract between cpu and OS: tsc is not > supposed to change for the duration of the boot With the methodology used; the TSC is still invariant; it's just running faster than the CPUID math calculates. > >> I only know that there's a use-case for me to want to be able to skip CPUID >> calibration, however I included args for skipping all the rest just so that >> all functionality is covered in the long run instead of just one use-case. > > wouldn't it be better to start the detailed calibration with the value from > CPUID instead; that way we also properly calibrate spread spectrum etc... > > I thought we switched to that recently to be honest... Are you referencing: 1bf8915ae5156dff439d2c65314bd8fdde1b83bf - x86/tsc: Enumerate SKL cpu_khz and tsc_khz via CPUID However since it's returning at CPUID calibration during native_calibrate_cpu(); it's not compared after-the-fact, leading to the TSC to use the 'slower' number returned by CPUID. Now keep in mind; I dunno if there was any reason to explicitly not want to utilize the PIT calib sequences on SKL. That'd be a factor for this. Would comparing the number after the fact; then if there's a significant difference between PIT and MSR/CPUID, defaulting to the 'faster' value be a better solution?
[RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration
methods should the user desire to.
The current ordering in ML x86 tsc is to calibrate in the order listed above;
returning whenever there's a successful calibration. However there are certain
BIOS/HW Designs for overclocking that cause the TSC to change along with the
max core clock; and simple 'trusting' calibration methodologies will lead to
the TSC running 'faster' and eventually, TSC instability.
I only know that there's a use-case for me to want to be able to skip CPUID
calibration, however I included args for skipping all the rest just so that all
functionality is covered in the long run instead of just one use-case.
I included some noise; in the end it's probably not too necessary to have, but
it could be useful from a debugging standpoint to see if someone is utilizing
the flags.
---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..5a07d12 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -47,6 +47,13 @@ static DEFINE_STATIC_KEY_FALSE(__use_tsc);
int tsc_clocksource_reliable;
+/*
+ * TSC calibration sequence disablement
+ */
+int calibrate_cpuid_khz_disabled = 0;
+int calibrate_msr_disabled = 0;
+int calibrate_quick_disabled = 0;
+
static u32 art_to_tsc_numerator;
static u32 art_to_tsc_denominator;
static u64 art_to_tsc_offset;
@@ -281,6 +288,32 @@ static int __init tsc_setup(char *str)
__setup("tsc=", tsc_setup);
+static int __init setup_tsc_calibration_order(char *str)
+{
+ if (!str)
+ return -EINVAL;
+
+ while (*str) {
+ if (!strncmp(str, "nocpuid", 7)) {
+ calibrate_cpuid_khz_disabled = 1;
+ pr_info("TSC CPUID khz calibrate disabled\n");
+ } else if (!strncmp(str, "nomsr", 5)) {
+ calibrate_msr_disabled = 1;
+ pr_info("TSC msr calibrate disabled\n");
+ } else if (!strncmp(str, "noquick", 7)) {
+ calibrate_quick_disabled = 1;
+ pr_info("TSC quick calibrate disabled\n");
+ }
+
+ str += strcspn(str, ",");
+ while (*str == ',')
+ str++;
+ }
+ return 1;
+}
+
+__setup("tsc_calibrate=", setup_tsc_calibration_order);
+
#define MAX_RETRIES 5
#define SMI_TRESHOLD5
@@ -675,19 +708,25 @@ unsigned long native_calibrate_cpu(void)
unsigned long flags, latch, ms, fast_calibrate;
int hpet = is_hpet_enabled(), i, loopmin;
- fast_calibrate = cpu_khz_from_cpuid();
- if (fast_calibrate)
- return fast_calibrate;
+ if (!calibrate_cpuid_khz_disabled) {
+ fast_calibrate = cpu_khz_from_cpuid();
+ if (fast_calibrate)
+ return fast_calibrate;
+ }
- fast_calibrate = cpu_khz_from_msr();
- if (fast_calibrate)
- return fast_calibrate;
+ if (!calibrate_msr_disabled) {
+ fast_calibrate = cpu_khz_from_msr();
+ if (fast_calibrate)
+ return fast_calibrate;
+ }
- local_irq_save(flags);
- fast_calibrate = quick_pit_calibrate();
- local_irq_restore(flags);
- if (fast_calibrate)
- return fast_calibrate;
+ if (!calibrate_quick_disabled) {
+ local_irq_save(flags);
+ fast_calibrate = quick_pit_calibrate();
+ local_irq_restore(flags);
+ if (fast_calibrate)
+ return fast_calibrate;
+ }
/*
* Run 5 calibration loops to get the lowest frequency value
---
[RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration
methods should the user desire to.
The current ordering in ML x86 tsc is to calibrate in the order listed above;
returning whenever there's a successful calibration. However there are certain
BIOS/HW Designs for overclocking that cause the TSC to change along with the
max core clock; and simple 'trusting' calibration methodologies will lead to
the TSC running 'faster' and eventually, TSC instability.
I only know that there's a use-case for me to want to be able to skip CPUID
calibration, however I included args for skipping all the rest just so that all
functionality is covered in the long run instead of just one use-case.
I included some noise; in the end it's probably not too necessary to have, but
it could be useful from a debugging standpoint to see if someone is utilizing
the flags.
---
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 74392d9..5a07d12 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -47,6 +47,13 @@ static DEFINE_STATIC_KEY_FALSE(__use_tsc);
int tsc_clocksource_reliable;
+/*
+ * TSC calibration sequence disablement
+ */
+int calibrate_cpuid_khz_disabled = 0;
+int calibrate_msr_disabled = 0;
+int calibrate_quick_disabled = 0;
+
static u32 art_to_tsc_numerator;
static u32 art_to_tsc_denominator;
static u64 art_to_tsc_offset;
@@ -281,6 +288,32 @@ static int __init tsc_setup(char *str)
__setup("tsc=", tsc_setup);
+static int __init setup_tsc_calibration_order(char *str)
+{
+ if (!str)
+ return -EINVAL;
+
+ while (*str) {
+ if (!strncmp(str, "nocpuid", 7)) {
+ calibrate_cpuid_khz_disabled = 1;
+ pr_info("TSC CPUID khz calibrate disabled\n");
+ } else if (!strncmp(str, "nomsr", 5)) {
+ calibrate_msr_disabled = 1;
+ pr_info("TSC msr calibrate disabled\n");
+ } else if (!strncmp(str, "noquick", 7)) {
+ calibrate_quick_disabled = 1;
+ pr_info("TSC quick calibrate disabled\n");
+ }
+
+ str += strcspn(str, ",");
+ while (*str == ',')
+ str++;
+ }
+ return 1;
+}
+
+__setup("tsc_calibrate=", setup_tsc_calibration_order);
+
#define MAX_RETRIES 5
#define SMI_TRESHOLD5
@@ -675,19 +708,25 @@ unsigned long native_calibrate_cpu(void)
unsigned long flags, latch, ms, fast_calibrate;
int hpet = is_hpet_enabled(), i, loopmin;
- fast_calibrate = cpu_khz_from_cpuid();
- if (fast_calibrate)
- return fast_calibrate;
+ if (!calibrate_cpuid_khz_disabled) {
+ fast_calibrate = cpu_khz_from_cpuid();
+ if (fast_calibrate)
+ return fast_calibrate;
+ }
- fast_calibrate = cpu_khz_from_msr();
- if (fast_calibrate)
- return fast_calibrate;
+ if (!calibrate_msr_disabled) {
+ fast_calibrate = cpu_khz_from_msr();
+ if (fast_calibrate)
+ return fast_calibrate;
+ }
- local_irq_save(flags);
- fast_calibrate = quick_pit_calibrate();
- local_irq_restore(flags);
- if (fast_calibrate)
- return fast_calibrate;
+ if (!calibrate_quick_disabled) {
+ local_irq_save(flags);
+ fast_calibrate = quick_pit_calibrate();
+ local_irq_restore(flags);
+ if (fast_calibrate)
+ return fast_calibrate;
+ }
/*
* Run 5 calibration loops to get the lowest frequency value
---
Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
On 7/13/2018 12:19 PM, patrickg wrote: This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration methods should the user desire to. The current ordering in ML x86 tsc is to calibrate in the order listed above; returning whenever there's a successful calibration. However there are certain BIOS/HW Designs for overclocking that cause the TSC to change along with the max core clock; and simple 'trusting' calibration methodologies will lead to the TSC running 'faster' and eventually, TSC instability. that would be a real violation of the contract between cpu and OS: tsc is not supposed to change for the duration of the boot I only know that there's a use-case for me to want to be able to skip CPUID calibration, however I included args for skipping all the rest just so that all functionality is covered in the long run instead of just one use-case. wouldn't it be better to start the detailed calibration with the value from CPUID instead; that way we also properly calibrate spread spectrum etc... I thought we switched to that recently to be honest...
Re: [RFC] x86, tsc: Add kcmdline args for skipping tsc calibration sequences
On 7/13/2018 12:19 PM, patrickg wrote: This RFC patch is intended to allow bypass CPUID, MSR and QuickPIT calibration methods should the user desire to. The current ordering in ML x86 tsc is to calibrate in the order listed above; returning whenever there's a successful calibration. However there are certain BIOS/HW Designs for overclocking that cause the TSC to change along with the max core clock; and simple 'trusting' calibration methodologies will lead to the TSC running 'faster' and eventually, TSC instability. that would be a real violation of the contract between cpu and OS: tsc is not supposed to change for the duration of the boot I only know that there's a use-case for me to want to be able to skip CPUID calibration, however I included args for skipping all the rest just so that all functionality is covered in the long run instead of just one use-case. wouldn't it be better to start the detailed calibration with the value from CPUID instead; that way we also properly calibrate spread spectrum etc... I thought we switched to that recently to be honest...
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On 7/12/2018 1:41 AM, Brendan Higgins wrote:
On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo
wrote:
This patch adjusts spinlock scope to make it wrap the whole irq
handler using a single lock/unlock which covers both master and
slave handlers.
Signed-off-by: Jae Hyun Yoo
---
drivers/i2c/busses/i2c-aspeed.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 60e4d0e939a3..9f02aa959a3e 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
bool irq_handled = true;
u8 value;
- spin_lock(>lock);
if (!slave) {
irq_handled = false;
goto out;
@@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
out:
- spin_unlock(>lock);
return irq_handled;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus
*bus)
u8 recv_byte;
int ret;
- spin_lock(>lock);
irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Ack all interrupt bits. */
writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
@@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus
*bus)
dev_err(bus->dev,
"irq handled != irq. expected 0x%08x, but was
0x%08x\n",
irq_status, status_ack);
- spin_unlock(>lock);
return !!irq_status;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
+ bool ret;
+
+ spin_lock(>lock);
#if IS_ENABLED(CONFIG_I2C_SLAVE)
if (aspeed_i2c_slave_irq(bus)) {
dev_dbg(bus->dev, "irq handled by slave.\n");
- return IRQ_HANDLED;
+ ret = true;
+ goto out;
}
#endif /* CONFIG_I2C_SLAVE */
- return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE;
+ ret = aspeed_i2c_master_irq(bus);
+
+out:
+ spin_unlock(>lock);
+ return ret ? IRQ_HANDLED : IRQ_NONE;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.17.1
Reviewed-by: Brendan Higgins
Thanks!
Thanks Brendan!
BTW, to whom should I ask applying this patch? Should I send v2 after
adding your reviewed-by tag?
Thanks,
Jae
Re: [PATCH] i2c: aspeed: Adjust spinlock scope in the irq handler
On 7/12/2018 1:41 AM, Brendan Higgins wrote:
On Mon, Jul 2, 2018 at 2:40 PM Jae Hyun Yoo
wrote:
This patch adjusts spinlock scope to make it wrap the whole irq
handler using a single lock/unlock which covers both master and
slave handlers.
Signed-off-by: Jae Hyun Yoo
---
drivers/i2c/busses/i2c-aspeed.c | 16 ++--
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-aspeed.c b/drivers/i2c/busses/i2c-aspeed.c
index 60e4d0e939a3..9f02aa959a3e 100644
--- a/drivers/i2c/busses/i2c-aspeed.c
+++ b/drivers/i2c/busses/i2c-aspeed.c
@@ -234,7 +234,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
bool irq_handled = true;
u8 value;
- spin_lock(>lock);
if (!slave) {
irq_handled = false;
goto out;
@@ -325,7 +324,6 @@ static bool aspeed_i2c_slave_irq(struct aspeed_i2c_bus *bus)
writel(status_ack, bus->base + ASPEED_I2C_INTR_STS_REG);
out:
- spin_unlock(>lock);
return irq_handled;
}
#endif /* CONFIG_I2C_SLAVE */
@@ -389,7 +387,6 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus
*bus)
u8 recv_byte;
int ret;
- spin_lock(>lock);
irq_status = readl(bus->base + ASPEED_I2C_INTR_STS_REG);
/* Ack all interrupt bits. */
writel(irq_status, bus->base + ASPEED_I2C_INTR_STS_REG);
@@ -547,22 +544,29 @@ static bool aspeed_i2c_master_irq(struct aspeed_i2c_bus
*bus)
dev_err(bus->dev,
"irq handled != irq. expected 0x%08x, but was
0x%08x\n",
irq_status, status_ack);
- spin_unlock(>lock);
return !!irq_status;
}
static irqreturn_t aspeed_i2c_bus_irq(int irq, void *dev_id)
{
struct aspeed_i2c_bus *bus = dev_id;
+ bool ret;
+
+ spin_lock(>lock);
#if IS_ENABLED(CONFIG_I2C_SLAVE)
if (aspeed_i2c_slave_irq(bus)) {
dev_dbg(bus->dev, "irq handled by slave.\n");
- return IRQ_HANDLED;
+ ret = true;
+ goto out;
}
#endif /* CONFIG_I2C_SLAVE */
- return aspeed_i2c_master_irq(bus) ? IRQ_HANDLED : IRQ_NONE;
+ ret = aspeed_i2c_master_irq(bus);
+
+out:
+ spin_unlock(>lock);
+ return ret ? IRQ_HANDLED : IRQ_NONE;
}
static int aspeed_i2c_master_xfer(struct i2c_adapter *adap,
--
2.17.1
Reviewed-by: Brendan Higgins
Thanks!
Thanks Brendan!
BTW, to whom should I ask applying this patch? Should I send v2 after
adding your reviewed-by tag?
Thanks,
Jae
Re: [PATCH] arm64: build with baremetal linker target instead of Linux when available
On 07/13/2018 08:30 AM, Olof Johansson wrote: Not all toolchains have the baremetal elf targets, RedHat/Fedora ones in particular. So, probe for whether it's available and use the previous (linux) targets if it isn't. For the Fedora toolchains: Tested-by: Laura Abbott Reported-by: Laura Abbott Cc: Paul Kocialkowski Signed-off-by: Olof Johansson --- arch/arm64/Makefile| 9 + scripts/Kbuild.include | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index e7101b19d590..efe61a2e4b5e 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -60,15 +60,16 @@ ifeq ($(CONFIG_CPU_BIG_ENDIAN), y) KBUILD_CPPFLAGS += -mbig-endian CHECKFLAGS+= -D__AARCH64EB__ AS+= -EB -# We must use the linux target here, since distributions don't tend to package -# the ELF linker scripts with binutils, and this results in a build failure. -LDFLAGS+= -EB -maarch64linuxb +# Prefer the baremetal ELF build target, but not all toolchains include +# it so fall back to the standard linux version if needed. +LDFLAGS+= -EB $(call ld-option, -maarch64elfb, -maarch64linuxb) UTS_MACHINE := aarch64_be else KBUILD_CPPFLAGS += -mlittle-endian CHECKFLAGS+= -D__AARCH64EL__ AS+= -EL -LDFLAGS+= -EL -maarch64linux # See comment above +# Same as above, prefer ELF but fall back to linux target if needed. +LDFLAGS+= -EL $(call ld-option, -maarch64elf, -maarch64linux) UTS_MACHINE := aarch64 endif diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index c8156d61678c..1e13f502b42f 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -163,8 +163,8 @@ cc-ldoption = $(call try-run,\ $(CC) $(1) $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2)) # ld-option -# Usage: LDFLAGS += $(call ld-option, -X) -ld-option = $(call try-run, $(LD) $(LDFLAGS) $(1) -v,$(1),$(2)) +# Usage: LDFLAGS += $(call ld-option, -X, -Y) +ld-option = $(call try-run, $(LD) $(LDFLAGS) $(1) -v,$(1),$(2),$(3)) # ar-option # Usage: KBUILD_ARFLAGS := $(call ar-option,D)
Re: [PATCH] arm64: build with baremetal linker target instead of Linux when available
On 07/13/2018 08:30 AM, Olof Johansson wrote: Not all toolchains have the baremetal elf targets, RedHat/Fedora ones in particular. So, probe for whether it's available and use the previous (linux) targets if it isn't. For the Fedora toolchains: Tested-by: Laura Abbott Reported-by: Laura Abbott Cc: Paul Kocialkowski Signed-off-by: Olof Johansson --- arch/arm64/Makefile| 9 + scripts/Kbuild.include | 4 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/arch/arm64/Makefile b/arch/arm64/Makefile index e7101b19d590..efe61a2e4b5e 100644 --- a/arch/arm64/Makefile +++ b/arch/arm64/Makefile @@ -60,15 +60,16 @@ ifeq ($(CONFIG_CPU_BIG_ENDIAN), y) KBUILD_CPPFLAGS += -mbig-endian CHECKFLAGS+= -D__AARCH64EB__ AS+= -EB -# We must use the linux target here, since distributions don't tend to package -# the ELF linker scripts with binutils, and this results in a build failure. -LDFLAGS+= -EB -maarch64linuxb +# Prefer the baremetal ELF build target, but not all toolchains include +# it so fall back to the standard linux version if needed. +LDFLAGS+= -EB $(call ld-option, -maarch64elfb, -maarch64linuxb) UTS_MACHINE := aarch64_be else KBUILD_CPPFLAGS += -mlittle-endian CHECKFLAGS+= -D__AARCH64EL__ AS+= -EL -LDFLAGS+= -EL -maarch64linux # See comment above +# Same as above, prefer ELF but fall back to linux target if needed. +LDFLAGS+= -EL $(call ld-option, -maarch64elf, -maarch64linux) UTS_MACHINE := aarch64 endif diff --git a/scripts/Kbuild.include b/scripts/Kbuild.include index c8156d61678c..1e13f502b42f 100644 --- a/scripts/Kbuild.include +++ b/scripts/Kbuild.include @@ -163,8 +163,8 @@ cc-ldoption = $(call try-run,\ $(CC) $(1) $(KBUILD_CPPFLAGS) $(CC_OPTION_CFLAGS) -nostdlib -x c /dev/null -o "$$TMP",$(1),$(2)) # ld-option -# Usage: LDFLAGS += $(call ld-option, -X) -ld-option = $(call try-run, $(LD) $(LDFLAGS) $(1) -v,$(1),$(2)) +# Usage: LDFLAGS += $(call ld-option, -X, -Y) +ld-option = $(call try-run, $(LD) $(LDFLAGS) $(1) -v,$(1),$(2),$(3)) # ar-option # Usage: KBUILD_ARFLAGS := $(call ar-option,D)
RE: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields
>> Dell - Internal Use - Confidential >Really? To a public mailing list? odd... Ha yea sorry - I was so excited to show my support, that I jumped the gun :)
RE: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields
>> Dell - Internal Use - Confidential >Really? To a public mailing list? odd... Ha yea sorry - I was so excited to show my support, that I jumped the gun :)
Re: [PATCH v2] tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire
On Fri, Jul 13, 2018 at 10:16:48AM -0700, Linus Torvalds wrote:
> On Fri, Jul 13, 2018 at 2:34 AM Will Deacon wrote:
> >
> > And, since we're stating preferences, I'll reiterate my preference towards:
> >
> > * RCsc unlock/lock
> > * RCpc release/acquire
>
> Yes, I think this would be best. We *used* to have pretty heavy-weight
> locking rules for various reasons, and we relaxed them for reasons
> that weren't perhaps always the right ones.
>
> Locking is pretty heavy-weight in general, and meant to be the "I
> don't really have to think about this very much" option. Then not
> being serializing enough to confuse people when it allows odd behavior
> (on _some_ architectures) does not sound like a great idea.
>
> In contrast, when you do release/acquire or any of the other "I know
> what I'm doing" things, I think we want the minimal serialization
> implied by the very specialized op.
The changes under discussion are _not_ affecting uses such as:
P0:
spin_lock(s);
UPDATE data_struct
spin_unlock(s);
P1:
spin_lock(s);
UPDATE data_struct
spin_unlock(s);
[...]
(most common use case for locking?): these uses work just _fine_ with
the current implementations and in LKMM.
OTOH, these changes are going to affect uses where threads interact by
"mixing" locking and _other_ synchronization primitives such as in:
{ x = 0; y = 0; }
P0:
spin_lock(s);
WRITE_ONCE(x, 1);
spin_unlock(s);
P1:
spin_lock(s);
r0 = READ_ONCE(x);
WRITE_ONCE(y, 1);
spin_unlock(s);
P2:
r1 = smp_load_acquire();
r2 = READ_ONCE(x);
BUG_ON(r0 == 1 && r1 == 1 && r2 == 0)
and
{ x = 0; y = 0; z = 0; }
P0:
spin_lock(s);
WRITE_ONCE(x, 1);
r0 = READ_ONCE(y);
spin_unlock(s);
P1:
spin_lock(s);
WRITE_ONCE(y, 1);
r1 = READ_ONCE(z);
spin_unlock(s);
P2
WRITE_ONCE(z, 1);
smp_mb();
r2 = READ_ONCE(x);
BUG_ON(r0 == 0 && r1 == 0 && r2 == 0)
(inspired from __two__ uses in kernel/{sched,rcu}). Even if someone were
to tell me that locks serialize enough, I'd still be prompted to say "yes,
but do / can my BUG_ON()s fire?".
Actually, my very first reaction, before starting what does appear to be
indeed a long and complex conversation, would probably be to run/check the
above snippets against the (latest) LKMM, by using the associated tool.
Once "checked" with both people and automated models, I'd probably remain
suspicious about my "magic" code so that I most likely will be prompted to
dig into each single arch. implementation / reference manual...
... Time's up!
Andrea
Re: [PATCH v2] tools/memory-model: Add extra ordering for locks and remove it for ordinary release/acquire
On Fri, Jul 13, 2018 at 10:16:48AM -0700, Linus Torvalds wrote:
> On Fri, Jul 13, 2018 at 2:34 AM Will Deacon wrote:
> >
> > And, since we're stating preferences, I'll reiterate my preference towards:
> >
> > * RCsc unlock/lock
> > * RCpc release/acquire
>
> Yes, I think this would be best. We *used* to have pretty heavy-weight
> locking rules for various reasons, and we relaxed them for reasons
> that weren't perhaps always the right ones.
>
> Locking is pretty heavy-weight in general, and meant to be the "I
> don't really have to think about this very much" option. Then not
> being serializing enough to confuse people when it allows odd behavior
> (on _some_ architectures) does not sound like a great idea.
>
> In contrast, when you do release/acquire or any of the other "I know
> what I'm doing" things, I think we want the minimal serialization
> implied by the very specialized op.
The changes under discussion are _not_ affecting uses such as:
P0:
spin_lock(s);
UPDATE data_struct
spin_unlock(s);
P1:
spin_lock(s);
UPDATE data_struct
spin_unlock(s);
[...]
(most common use case for locking?): these uses work just _fine_ with
the current implementations and in LKMM.
OTOH, these changes are going to affect uses where threads interact by
"mixing" locking and _other_ synchronization primitives such as in:
{ x = 0; y = 0; }
P0:
spin_lock(s);
WRITE_ONCE(x, 1);
spin_unlock(s);
P1:
spin_lock(s);
r0 = READ_ONCE(x);
WRITE_ONCE(y, 1);
spin_unlock(s);
P2:
r1 = smp_load_acquire();
r2 = READ_ONCE(x);
BUG_ON(r0 == 1 && r1 == 1 && r2 == 0)
and
{ x = 0; y = 0; z = 0; }
P0:
spin_lock(s);
WRITE_ONCE(x, 1);
r0 = READ_ONCE(y);
spin_unlock(s);
P1:
spin_lock(s);
WRITE_ONCE(y, 1);
r1 = READ_ONCE(z);
spin_unlock(s);
P2
WRITE_ONCE(z, 1);
smp_mb();
r2 = READ_ONCE(x);
BUG_ON(r0 == 0 && r1 == 0 && r2 == 0)
(inspired from __two__ uses in kernel/{sched,rcu}). Even if someone were
to tell me that locks serialize enough, I'd still be prompted to say "yes,
but do / can my BUG_ON()s fire?".
Actually, my very first reaction, before starting what does appear to be
indeed a long and complex conversation, would probably be to run/check the
above snippets against the (latest) LKMM, by using the associated tool.
Once "checked" with both people and automated models, I'd probably remain
suspicious about my "magic" code so that I most likely will be prompted to
dig into each single arch. implementation / reference manual...
... Time's up!
Andrea
Re: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields
On Fri, Jul 13, 2018 at 06:49:04PM +, eugene@dell.com wrote: > Dell - Internal Use - Confidential Really? To a public mailing list? odd...
Re: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields
On Fri, Jul 13, 2018 at 06:49:04PM +, eugene@dell.com wrote: > Dell - Internal Use - Confidential Really? To a public mailing list? odd...
Re: [PATCH 38/39] x86/mm/pti: Add Warning when booting on a PCID capable CPU
On Wed, Jul 11, 2018 at 4:29 AM, Joerg Roedel wrote:
> From: Joerg Roedel
>
> Warn the user in case the performance can be significantly
> improved by switching to a 64-bit kernel.
...
> +#ifdef CONFIG_X86_32
> + if (boot_cpu_has(X86_FEATURE_PCID)) {
I'm a bit confused. Wouldn't the setup_clear_cpu_cap() call in
early_identify_cpu() prevent this from working?
Boris, do we have a straightforward way to ask "does the CPU advertise
this feature in CPUID regardless of whether we have it enabled right
now"?
--Andy
Re: [PATCH 38/39] x86/mm/pti: Add Warning when booting on a PCID capable CPU
On Wed, Jul 11, 2018 at 4:29 AM, Joerg Roedel wrote:
> From: Joerg Roedel
>
> Warn the user in case the performance can be significantly
> improved by switching to a 64-bit kernel.
...
> +#ifdef CONFIG_X86_32
> + if (boot_cpu_has(X86_FEATURE_PCID)) {
I'm a bit confused. Wouldn't the setup_clear_cpu_cap() call in
early_identify_cpu() prevent this from working?
Boris, do we have a straightforward way to ask "does the CPU advertise
this feature in CPUID regardless of whether we have it enabled right
now"?
--Andy
Re: [PATCH 2/6] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
On Fri, Jul 13, 2018 at 11:38:19AM -0700, Daniel Jordan wrote:
> On Fri, Jul 13, 2018 at 07:36:32AM +0800, Huang, Ying wrote:
> > @@ -1260,7 +1257,6 @@ static void swapcache_free(swp_entry_t entry)
> > }
> > }
> >
> > -#ifdef CONFIG_THP_SWAP
> > static void swapcache_free_cluster(swp_entry_t entry)
> > {
> > unsigned long offset = swp_offset(entry);
> > @@ -1271,6 +1267,9 @@ static void swapcache_free_cluster(swp_entry_t entry)
> > unsigned int i, free_entries = 0;
> > unsigned char val;
> >
> > + if (!IS_ENABLED(CONFIG_THP_SWAP))
> > + return;
> > +
>
> VM_WARN_ON_ONCE(1) here too?
Nevermind, this branch goes away later in the series.
Re: [PATCH 2/6] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
On Fri, Jul 13, 2018 at 11:38:19AM -0700, Daniel Jordan wrote:
> On Fri, Jul 13, 2018 at 07:36:32AM +0800, Huang, Ying wrote:
> > @@ -1260,7 +1257,6 @@ static void swapcache_free(swp_entry_t entry)
> > }
> > }
> >
> > -#ifdef CONFIG_THP_SWAP
> > static void swapcache_free_cluster(swp_entry_t entry)
> > {
> > unsigned long offset = swp_offset(entry);
> > @@ -1271,6 +1267,9 @@ static void swapcache_free_cluster(swp_entry_t entry)
> > unsigned int i, free_entries = 0;
> > unsigned char val;
> >
> > + if (!IS_ENABLED(CONFIG_THP_SWAP))
> > + return;
> > +
>
> VM_WARN_ON_ONCE(1) here too?
Nevermind, this branch goes away later in the series.
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On 7/13/2018 11:12 AM, Brendan Higgins wrote:
On Fri, Jul 13, 2018 at 10:22 AM Jae Hyun Yoo
wrote:
On 7/12/2018 11:21 AM, Jae Hyun Yoo wrote:
On 7/12/2018 2:33 AM, Brendan Higgins wrote:
On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
wrote:
+ for (;;) {
+ if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
+ (ASPEED_I2CD_BUS_BUSY_STS |
+ ASPEED_I2CD_XFER_MODE_STS_MASK)))
Is using the Transfer Mode State Machine bits necessary? The
documentation marks it as "for debugging purpose only," so relying on
it makes me nervous.
As you said, the documentation marks it as "for debugging purpose only."
but ASPEED also uses this way in their SDK code because it's the best
way for checking bus busy status which can cover both single and
multi-master use cases.
Well, it would also be really nice to have access to this bit if
someone wants
to implement MCTP. Could we maybe check with Aspeed what them meant by
"for
debugging purposes only" and document it here? It makes me nervous to
rely on
debugging functionality for normal usage.
Okay, I'll check it with Aspeed. Will let you know their response.
I've checked it with Gary Hsu and he confirmed
that the bits reflect real information and good to be used in practical
code.
Huh. For my own edification, could you ask them why they said "for debugging
purpose only" in the documentation? I am just really curious what they meant by
that. I would be satisfied if you just CC'ed me on your email thread with Gary,
and I can ask him myself.
I've already CC'ed Gary and Ryan in this thread.
Hi Gary,
Can you explain why the documentation says that the bit field is 'for
debugging purpose only'? Any plan to change the description?
Thanks,
Jae
I'll add a comment like below:
/*
* This is marked as 'for debugging purpose only' in datasheet but
* ASPEED confirmed that this reflects real information and good
* to be used in practical code.
*/
Is it acceptable then?
Yeah, that's fine.
Cheers
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On 7/13/2018 11:12 AM, Brendan Higgins wrote:
On Fri, Jul 13, 2018 at 10:22 AM Jae Hyun Yoo
wrote:
On 7/12/2018 11:21 AM, Jae Hyun Yoo wrote:
On 7/12/2018 2:33 AM, Brendan Higgins wrote:
On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
wrote:
+ for (;;) {
+ if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
+ (ASPEED_I2CD_BUS_BUSY_STS |
+ ASPEED_I2CD_XFER_MODE_STS_MASK)))
Is using the Transfer Mode State Machine bits necessary? The
documentation marks it as "for debugging purpose only," so relying on
it makes me nervous.
As you said, the documentation marks it as "for debugging purpose only."
but ASPEED also uses this way in their SDK code because it's the best
way for checking bus busy status which can cover both single and
multi-master use cases.
Well, it would also be really nice to have access to this bit if
someone wants
to implement MCTP. Could we maybe check with Aspeed what them meant by
"for
debugging purposes only" and document it here? It makes me nervous to
rely on
debugging functionality for normal usage.
Okay, I'll check it with Aspeed. Will let you know their response.
I've checked it with Gary Hsu and he confirmed
that the bits reflect real information and good to be used in practical
code.
Huh. For my own edification, could you ask them why they said "for debugging
purpose only" in the documentation? I am just really curious what they meant by
that. I would be satisfied if you just CC'ed me on your email thread with Gary,
and I can ask him myself.
I've already CC'ed Gary and Ryan in this thread.
Hi Gary,
Can you explain why the documentation says that the bit field is 'for
debugging purpose only'? Any plan to change the description?
Thanks,
Jae
I'll add a comment like below:
/*
* This is marked as 'for debugging purpose only' in datasheet but
* ASPEED confirmed that this reflects real information and good
* to be used in practical code.
*/
Is it acceptable then?
Yeah, that's fine.
Cheers
Re: [GIT PULL] arm64 fixes for 4.18-rc5
On Fri, Jul 13, 2018 at 6:13 AM Will Deacon wrote: > > Catalin's out enjoying the sunshine, so I'm sending the fixes for a couple > of weeks (although there hopefully won't be any more!). Never fear, I'm sure there won't be more than a couple of weeks of sunshine in the UK this summer. That's what you were trying to say, right? Linus
Re: [PATCH] get_maintainer.pl: Add optional .get_maintainer.MAINTAINERS override
On Fri, Jul 06, 2018 at 03:14:28PM -0700, Joe Perches wrote:
> On Fri, 2018-07-06 at 15:09 -0700, Joe Perches wrote:
> > On Fri, 2018-07-06 at 17:58 -0400, Don Zickus wrote:
> > > We have an internal use case of multiple MAINTAINER files, some folks have
> > > more rights to patches than others so they are not allowed to be cc'd
> > > (think
> > > embargoed stuff).
>
> How about:
Hi Joe,
You are probably busy with stuff, but wanted to softly poke you to see what
is going on with this patch and if there is anything we can help with?
Cheers,
Don
> ---
> scripts/get_maintainer.pl | 39 +--
> 1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index c87fa734e3e1..f7a7d46340a8 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -60,7 +60,7 @@ my $pattern_depth = 0;
> my $self_test = undef;
> my $version = 0;
> my $help = 0;
> -my $find_maintainer_files = 0;
> +my $find_maintainer_files;
>
> my $vcs_used = 0;
>
> @@ -262,7 +262,7 @@ if (!GetOptions(
> 'sections!' => \$sections,
> 'fe|file-emails!' => \$file_emails,
> 'f|file' => \$from_filename,
> - 'find-maintainer-files' => \$find_maintainer_files,
> + 'find-maintainer-files:s' => \$find_maintainer_files,
> 'self-test:s' => \$self_test,
> 'v|version' => \$version,
> 'h|help|usage' => \$help,
> @@ -384,26 +384,29 @@ sub find_ignore_git {
> read_all_maintainer_files();
>
> sub read_all_maintainer_files {
> -if (-d "${lk_path}MAINTAINERS") {
> -opendir(DIR, "${lk_path}MAINTAINERS") or die $!;
> -my @files = readdir(DIR);
> -closedir(DIR);
> -foreach my $file (@files) {
> -push(@mfiles, "${lk_path}MAINTAINERS/$file") if ($file !~ /^\./);
> -}
> -}
> -
> -if ($find_maintainer_files) {
> -find( { wanted => \_is_maintainer_file,
> -preprocess => \_ignore_git,
> -no_chdir => 1,
> -}, "${lk_path}");
> +my $path = defined $find_maintainer_files && $find_maintainer_files ne ""
> + ? $find_maintainer_files : $lk_path;
> +if (-d "${path}MAINTAINERS") {
> + opendir(DIR, "${path}MAINTAINERS") or die $!;
> + my @files = readdir(DIR);
> + closedir(DIR);
> + foreach my $file (@files) {
> + push(@mfiles, "${path}MAINTAINERS/$file") if ($file !~ /^\./);
> + }
> +}
> +
> +if (defined $find_maintainer_files && (-d $find_maintainer_files)) {
> + find( { wanted => \_is_maintainer_file,
> + preprocess => \_ignore_git,
> + no_chdir => 1,
> + }, "${path}");
> } else {
> -push(@mfiles, "${lk_path}MAINTAINERS") if -f "${lk_path}MAINTAINERS";
> + push(@mfiles, "${path}MAINTAINERS") if -f "${path}MAINTAINERS";
> }
>
> +die "$P: No MAINTAINER files found in $path\n" if (scalar(@mfiles) == 0);
> foreach my $file (@mfiles) {
> -read_maintainer_file("$file");
> + read_maintainer_file("$file");
> }
> }
>
Re: [GIT PULL] arm64 fixes for 4.18-rc5
On Fri, Jul 13, 2018 at 6:13 AM Will Deacon wrote: > > Catalin's out enjoying the sunshine, so I'm sending the fixes for a couple > of weeks (although there hopefully won't be any more!). Never fear, I'm sure there won't be more than a couple of weeks of sunshine in the UK this summer. That's what you were trying to say, right? Linus
Re: [PATCH] get_maintainer.pl: Add optional .get_maintainer.MAINTAINERS override
On Fri, Jul 06, 2018 at 03:14:28PM -0700, Joe Perches wrote:
> On Fri, 2018-07-06 at 15:09 -0700, Joe Perches wrote:
> > On Fri, 2018-07-06 at 17:58 -0400, Don Zickus wrote:
> > > We have an internal use case of multiple MAINTAINER files, some folks have
> > > more rights to patches than others so they are not allowed to be cc'd
> > > (think
> > > embargoed stuff).
>
> How about:
Hi Joe,
You are probably busy with stuff, but wanted to softly poke you to see what
is going on with this patch and if there is anything we can help with?
Cheers,
Don
> ---
> scripts/get_maintainer.pl | 39 +--
> 1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/scripts/get_maintainer.pl b/scripts/get_maintainer.pl
> index c87fa734e3e1..f7a7d46340a8 100755
> --- a/scripts/get_maintainer.pl
> +++ b/scripts/get_maintainer.pl
> @@ -60,7 +60,7 @@ my $pattern_depth = 0;
> my $self_test = undef;
> my $version = 0;
> my $help = 0;
> -my $find_maintainer_files = 0;
> +my $find_maintainer_files;
>
> my $vcs_used = 0;
>
> @@ -262,7 +262,7 @@ if (!GetOptions(
> 'sections!' => \$sections,
> 'fe|file-emails!' => \$file_emails,
> 'f|file' => \$from_filename,
> - 'find-maintainer-files' => \$find_maintainer_files,
> + 'find-maintainer-files:s' => \$find_maintainer_files,
> 'self-test:s' => \$self_test,
> 'v|version' => \$version,
> 'h|help|usage' => \$help,
> @@ -384,26 +384,29 @@ sub find_ignore_git {
> read_all_maintainer_files();
>
> sub read_all_maintainer_files {
> -if (-d "${lk_path}MAINTAINERS") {
> -opendir(DIR, "${lk_path}MAINTAINERS") or die $!;
> -my @files = readdir(DIR);
> -closedir(DIR);
> -foreach my $file (@files) {
> -push(@mfiles, "${lk_path}MAINTAINERS/$file") if ($file !~ /^\./);
> -}
> -}
> -
> -if ($find_maintainer_files) {
> -find( { wanted => \_is_maintainer_file,
> -preprocess => \_ignore_git,
> -no_chdir => 1,
> -}, "${lk_path}");
> +my $path = defined $find_maintainer_files && $find_maintainer_files ne ""
> + ? $find_maintainer_files : $lk_path;
> +if (-d "${path}MAINTAINERS") {
> + opendir(DIR, "${path}MAINTAINERS") or die $!;
> + my @files = readdir(DIR);
> + closedir(DIR);
> + foreach my $file (@files) {
> + push(@mfiles, "${path}MAINTAINERS/$file") if ($file !~ /^\./);
> + }
> +}
> +
> +if (defined $find_maintainer_files && (-d $find_maintainer_files)) {
> + find( { wanted => \_is_maintainer_file,
> + preprocess => \_ignore_git,
> + no_chdir => 1,
> + }, "${path}");
> } else {
> -push(@mfiles, "${lk_path}MAINTAINERS") if -f "${lk_path}MAINTAINERS";
> + push(@mfiles, "${path}MAINTAINERS") if -f "${path}MAINTAINERS";
> }
>
> +die "$P: No MAINTAINER files found in $path\n" if (scalar(@mfiles) == 0);
> foreach my $file (@mfiles) {
> -read_maintainer_file("$file");
> + read_maintainer_file("$file");
> }
> }
>
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On 7/13/2018 11:02 AM, Brendan Higgins wrote:
On Thu, Jul 12, 2018 at 11:21 AM Jae Hyun Yoo
wrote:
On 7/12/2018 2:33 AM, Brendan Higgins wrote:
On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
wrote:
+/* Timeout for bus busy checking */
+#define BUS_BUSY_CHECK_TIMEOUT 25 /* 250ms */
+#define BUS_BUSY_CHECK_INTERVAL1 /* 10ms
*/
Could you add a comment on where you got these values from?
These are coming from ASPEED SDK code. Actually, they use 100ms for
timeout and 10ms for interval but I increased the timeout value to
250ms so that it covers a various range of bus speed. I think, it
should be computed at run time based on the current bus speed, or
we could add these as device tree settings. How do you think about it?
This should definitely be a device tree setting. If one of the busses is being
used as a regular I2C bus, it could hold the bus for an unlimited amount of
time before sending a STOP. As for a default, 100ms is probably fine given
that, a) the limit will only apply to multi-master mode, and b) multi-master
mode will probably almost always be used with IPMB, or MCTP (MCTP actually
recommends a 100ms timeout for this purpose, see
https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
symbol PT2a). That being said, if you actually want to implement IPMB, or MCTP
arbitration logic, it is much more complicated.
Okay then, I think, we can fix the timeout value to 100ms and enable the
bus busy checking logic only when 'multi-master' is set in device tree.
My thought is, no additional arbitration logic is needed because
arbitration is performed in H/W level and H/W reports
ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
ARBIT_LOSS event is already being handled well by this driver code you
implemented.
I still think it would be best to provide an option to specify the timeout value
it in the device tree regardless of master mode or not. Also, I am talking about
fairness arbitration not the physical level arbitration provided by the I2C
spec. The physical arbitration that Aspeed provides just allows multiple masters
to operate on the same bus according to the specification; this bus arbitration
does not guarantee forward progress or even guarentee that the actual message
will be sent, which is what you are trying to do here.
Since you are planning on implementing IPMB, you will probably need to implement
fairness arbitration. I am not familiar with your BMC-ME channel. It sounds like
it pre-dates MCTP, so it must implement its own fairness arbitration on top of
the IPMB layer (more like you bake in some assumptions about what the possible
state is at anytime that guarentee fairness).
Are you using the Aspeed BMC on both sides of the connection? If so, you might
be further ahead to implement MCTP fairness arbitration which can be used in
conjunction with IPMB. This will require a bit of work to do, but everyone will
be much happier in the long term (assuming MCTP does eventually become a thing).
Okay. I'll add an additional option into device tree to set the timeout
value.
My thought is, the fairness arbitration you are saying should be
considered in IPMB or MCTP layer not in this driver code.
The intention of the code I added is, preventing state corruption in
this driver code. In multi-master environment, this driver side master
cannot know exactly when peer master send data to this side master so a
case can be happened that this master tries to send data through the
master_xfer function but slave data from peer master is still being
processed by this driver. Previous code treats this as an error and
tries recovering a bus which is definitely wrong. So the patch code
checks whether there is any ongoing slave operation or not and wait up
to the timeout duration before start master xfer.
>
#if IS_ENABLED(CONFIG_I2C_SLAVE)
- if (aspeed_i2c_slave_irq(bus)) {
- dev_dbg(bus->dev, "irq handled by slave.\n");
- return IRQ_HANDLED;
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ if (!aspeed_i2c_master_irq(bus))
Why do you check the slave if master fails (or vice versa)? I
understand that there are some status bits that have not been handled,
but it doesn't seem reasonable to assume that there is state that the
other should do something with; the only way this would happen is if
the state that you think you are in does not match the status bits you
have been given, but if this is the case, you are already hosed; I
don't think trying the other handler is likely to make things better,
unless there is something that I am missing.
In most of cases, interrupt bits are set one by one but there are also a
lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
with combining master and slave events using a single interrupt call. It
happens much in multi-master environment than
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On 7/13/2018 11:02 AM, Brendan Higgins wrote:
On Thu, Jul 12, 2018 at 11:21 AM Jae Hyun Yoo
wrote:
On 7/12/2018 2:33 AM, Brendan Higgins wrote:
On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
wrote:
+/* Timeout for bus busy checking */
+#define BUS_BUSY_CHECK_TIMEOUT 25 /* 250ms */
+#define BUS_BUSY_CHECK_INTERVAL1 /* 10ms
*/
Could you add a comment on where you got these values from?
These are coming from ASPEED SDK code. Actually, they use 100ms for
timeout and 10ms for interval but I increased the timeout value to
250ms so that it covers a various range of bus speed. I think, it
should be computed at run time based on the current bus speed, or
we could add these as device tree settings. How do you think about it?
This should definitely be a device tree setting. If one of the busses is being
used as a regular I2C bus, it could hold the bus for an unlimited amount of
time before sending a STOP. As for a default, 100ms is probably fine given
that, a) the limit will only apply to multi-master mode, and b) multi-master
mode will probably almost always be used with IPMB, or MCTP (MCTP actually
recommends a 100ms timeout for this purpose, see
https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
symbol PT2a). That being said, if you actually want to implement IPMB, or MCTP
arbitration logic, it is much more complicated.
Okay then, I think, we can fix the timeout value to 100ms and enable the
bus busy checking logic only when 'multi-master' is set in device tree.
My thought is, no additional arbitration logic is needed because
arbitration is performed in H/W level and H/W reports
ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
ARBIT_LOSS event is already being handled well by this driver code you
implemented.
I still think it would be best to provide an option to specify the timeout value
it in the device tree regardless of master mode or not. Also, I am talking about
fairness arbitration not the physical level arbitration provided by the I2C
spec. The physical arbitration that Aspeed provides just allows multiple masters
to operate on the same bus according to the specification; this bus arbitration
does not guarantee forward progress or even guarentee that the actual message
will be sent, which is what you are trying to do here.
Since you are planning on implementing IPMB, you will probably need to implement
fairness arbitration. I am not familiar with your BMC-ME channel. It sounds like
it pre-dates MCTP, so it must implement its own fairness arbitration on top of
the IPMB layer (more like you bake in some assumptions about what the possible
state is at anytime that guarentee fairness).
Are you using the Aspeed BMC on both sides of the connection? If so, you might
be further ahead to implement MCTP fairness arbitration which can be used in
conjunction with IPMB. This will require a bit of work to do, but everyone will
be much happier in the long term (assuming MCTP does eventually become a thing).
Okay. I'll add an additional option into device tree to set the timeout
value.
My thought is, the fairness arbitration you are saying should be
considered in IPMB or MCTP layer not in this driver code.
The intention of the code I added is, preventing state corruption in
this driver code. In multi-master environment, this driver side master
cannot know exactly when peer master send data to this side master so a
case can be happened that this master tries to send data through the
master_xfer function but slave data from peer master is still being
processed by this driver. Previous code treats this as an error and
tries recovering a bus which is definitely wrong. So the patch code
checks whether there is any ongoing slave operation or not and wait up
to the timeout duration before start master xfer.
>
#if IS_ENABLED(CONFIG_I2C_SLAVE)
- if (aspeed_i2c_slave_irq(bus)) {
- dev_dbg(bus->dev, "irq handled by slave.\n");
- return IRQ_HANDLED;
+ if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
+ if (!aspeed_i2c_master_irq(bus))
Why do you check the slave if master fails (or vice versa)? I
understand that there are some status bits that have not been handled,
but it doesn't seem reasonable to assume that there is state that the
other should do something with; the only way this would happen is if
the state that you think you are in does not match the status bits you
have been given, but if this is the case, you are already hosed; I
don't think trying the other handler is likely to make things better,
unless there is something that I am missing.
In most of cases, interrupt bits are set one by one but there are also a
lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
with combining master and slave events using a single interrupt call. It
happens much in multi-master environment than
RE: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields
Dell - Internal Use - Confidential +1 from someone using Nuvoton's BMC SoC -Original Message- From: Alexander Amelkin [mailto:a.amel...@yadro.com] Sent: Friday, July 13, 2018 10:14 AM To: Andrew Jeffery; Benjamin Herrenschmidt; Rob Herring Cc: Mark Rutland; devicet...@vger.kernel.org; Greg Kroah-Hartman; Cho, Eugene; linux-kernel@vger.kernel.org; Joel Stanley; stew...@linux.ibm.com; OpenBMC Maillist; moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE Subject: Re: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields Andrew, Ben, first of all let me thank you for bringing in this set of patches. From the discussion it looks to me like Rob is not familiar with specifics of BMC-managed servers and tries to apply to them the rules that have proven to be good for workstations and laptops. As someone using /dev/mem these days to configure those registers in BMCs, I'm all for this patch set as it will make BMC configuration less obscure. Writing 1 or 0 to a named node is way clearer than writing some magic value at some magic offset in /dev/mem. I like the idea of having it all configurable via DT as it allows for only having exported the nodes that are actually needed, thus reducing, as you have said, the foot-gun. So far I do not have any objections or constructive comments to the architecture of the proposed patches. So I'm writing this to support your position in this discussion and to let Rob and other reviewers know that this feature is actually needed. With best regards, Alexander Amelkin 13.07.2018 09:31, Andrew Jeffery wrote: > Hi Rob, Ben, > > I've replied to you both inline below, hopefully it's clear enough from the > context. > > On Fri, 13 Jul 2018, at 10:25, Benjamin Herrenschmidt wrote: >> On Thu, 2018-07-12 at 09:11 -0600, Rob Herring wrote: >>> On Wed, Jul 11, 2018 at 6:54 PM Andrew Jeffery wrote: Hi Rob, Thanks for the response. On Thu, 12 Jul 2018, at 05:34, Rob Herring wrote: > On Wed, Jul 11, 2018 at 03:01:19PM +0930, Andrew Jeffery wrote: >> Baseboard Management Controllers (BMCs) are embedded SoCs that >> exist to provide remote management of (primarily) server >> platforms. BMCs are often tightly coupled to the platform in >> terms of behaviour and provide many hardware features integral to >> booting and running the host system. >> >> Some of these hardware features are simple, for example scratch >> registers provided by the BMC that are exposed to both the host >> and the BMC. In other cases there's a single bit switch to enable >> or disable some of the provided functionality. >> >> The documentation defines bindings for fields in registers that >> do not integrate well into other driver models yet must be >> described to allow the BMC kernel to assume control of these features. > So we'll get a new binding when that happens? That will break > compatibility. Can you please expand on this? I'm not following. >>> If we have a subsystem in the future, then there would likely be an >>> associated binding which would be different. So if you update the >>> DT, then old kernels won't work with it. >> What kind of "subsystem" ? There is almost no way there could be one >> for that sort of BMC tunables. We've look at several BMC chips out >> there and requirements from several vendors, BIOS and system >> manufacturers and it's all over the place. > Right - This is the fundamental principle backing these patches: There will > never be a coherent subsystem catering to any of what we want to describe > with these bindings. > I feel like this is an argument of tradition. Maybe people have been dissuaded from doing so when they don't have a reasonable use- case? I'm not saying that what I'm proposing is unquestionably reasonable, but I don't want to dismiss it out of hand. > ... >>> It comes up with system controller type blocks too that just have a >>> bunch of random registers. > This matches the situation at hand. > >>> Those change in every SoC and not in any controlled or ordered way >>> that would make describing the individual sub-functions in DT >>> worthwhile. > "Not worthwhile" is what I'm pushing back against for our use cases. I think > they are narrow and limited enough to make it worthwhile. > > Obviously we want to avoid describing these things *badly* - you mentioned > the clock bindings - so I'm happy to hash out what the right representation > should be. But I struggle to think the solution is not describing some of our > hardware features at all. > >> So what's the alternative ? Because without something like what we >> propose, what's going to happen is /dev/mem ... that's what people do >> today. > Yep. And I've outlined in the cover letter what I think are the advantages of > what I'm proposing over /dev/mem. It's not an incredible gain, but has > several of
RE: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields
Dell - Internal Use - Confidential +1 from someone using Nuvoton's BMC SoC -Original Message- From: Alexander Amelkin [mailto:a.amel...@yadro.com] Sent: Friday, July 13, 2018 10:14 AM To: Andrew Jeffery; Benjamin Herrenschmidt; Rob Herring Cc: Mark Rutland; devicet...@vger.kernel.org; Greg Kroah-Hartman; Cho, Eugene; linux-kernel@vger.kernel.org; Joel Stanley; stew...@linux.ibm.com; OpenBMC Maillist; moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE Subject: Re: [RFC PATCH v2 1/4] dt-bindings: misc: Add bindings for misc. BMC control fields Andrew, Ben, first of all let me thank you for bringing in this set of patches. From the discussion it looks to me like Rob is not familiar with specifics of BMC-managed servers and tries to apply to them the rules that have proven to be good for workstations and laptops. As someone using /dev/mem these days to configure those registers in BMCs, I'm all for this patch set as it will make BMC configuration less obscure. Writing 1 or 0 to a named node is way clearer than writing some magic value at some magic offset in /dev/mem. I like the idea of having it all configurable via DT as it allows for only having exported the nodes that are actually needed, thus reducing, as you have said, the foot-gun. So far I do not have any objections or constructive comments to the architecture of the proposed patches. So I'm writing this to support your position in this discussion and to let Rob and other reviewers know that this feature is actually needed. With best regards, Alexander Amelkin 13.07.2018 09:31, Andrew Jeffery wrote: > Hi Rob, Ben, > > I've replied to you both inline below, hopefully it's clear enough from the > context. > > On Fri, 13 Jul 2018, at 10:25, Benjamin Herrenschmidt wrote: >> On Thu, 2018-07-12 at 09:11 -0600, Rob Herring wrote: >>> On Wed, Jul 11, 2018 at 6:54 PM Andrew Jeffery wrote: Hi Rob, Thanks for the response. On Thu, 12 Jul 2018, at 05:34, Rob Herring wrote: > On Wed, Jul 11, 2018 at 03:01:19PM +0930, Andrew Jeffery wrote: >> Baseboard Management Controllers (BMCs) are embedded SoCs that >> exist to provide remote management of (primarily) server >> platforms. BMCs are often tightly coupled to the platform in >> terms of behaviour and provide many hardware features integral to >> booting and running the host system. >> >> Some of these hardware features are simple, for example scratch >> registers provided by the BMC that are exposed to both the host >> and the BMC. In other cases there's a single bit switch to enable >> or disable some of the provided functionality. >> >> The documentation defines bindings for fields in registers that >> do not integrate well into other driver models yet must be >> described to allow the BMC kernel to assume control of these features. > So we'll get a new binding when that happens? That will break > compatibility. Can you please expand on this? I'm not following. >>> If we have a subsystem in the future, then there would likely be an >>> associated binding which would be different. So if you update the >>> DT, then old kernels won't work with it. >> What kind of "subsystem" ? There is almost no way there could be one >> for that sort of BMC tunables. We've look at several BMC chips out >> there and requirements from several vendors, BIOS and system >> manufacturers and it's all over the place. > Right - This is the fundamental principle backing these patches: There will > never be a coherent subsystem catering to any of what we want to describe > with these bindings. > I feel like this is an argument of tradition. Maybe people have been dissuaded from doing so when they don't have a reasonable use- case? I'm not saying that what I'm proposing is unquestionably reasonable, but I don't want to dismiss it out of hand. > ... >>> It comes up with system controller type blocks too that just have a >>> bunch of random registers. > This matches the situation at hand. > >>> Those change in every SoC and not in any controlled or ordered way >>> that would make describing the individual sub-functions in DT >>> worthwhile. > "Not worthwhile" is what I'm pushing back against for our use cases. I think > they are narrow and limited enough to make it worthwhile. > > Obviously we want to avoid describing these things *badly* - you mentioned > the clock bindings - so I'm happy to hash out what the right representation > should be. But I struggle to think the solution is not describing some of our > hardware features at all. > >> So what's the alternative ? Because without something like what we >> propose, what's going to happen is /dev/mem ... that's what people do >> today. > Yep. And I've outlined in the cover letter what I think are the advantages of > what I'm proposing over /dev/mem. It's not an incredible gain, but has > several of
How are you doing today? Please read my email and reply me!!
Dear Sir/Madam, How are you doing today? My name is Billy Wilfred. I am California, United States of America. I am a broker of Project Financing Firm who has cutting edge and group capital fund, they can finance any lucrative project and help you to enhance your business plan. Are you in need of a Loan to Finance and Fund your Project or Company? Are you an Investor, Real Estate developer, Construction Company, etc? or Do you need a loan to keep your investment or business going on in order to have a different look? Have you been trying to obtain a Loan from Banks or Loan Companies and got Ripped off and they have refused to grant you the Loan because of bad credit? do not close your company and stop your project because of bankruptcy, we are here for you for real, please be happy, rejoice and celebrate because your solution have come, we will end your financial worries now. Therefore come to us, we will grant you the loan you need without delay. We offer all types of non-recourse Loan and Funding at a low Interest Rate of The categories of Loan Financial Funding we offered include but not limited to: Business Loan, Personal Loan, Company Loan, Mortgage Loan, Debt Consolidation and Financial Funding for both Turnkey and mega projects etc from a minimum of Euro / US$1Million to Euro / US$5 Billion Max. Most importantly, Note that the loan company DO NOT charge any upfront fee or advance fee. This message is not scam for what so ever. This is 100% real, legal and legitimate loan company office in Turkey. Kindly get in touch for further details and procedures. Thanks for your cooperation with us. I will be waiting for your response. Further more details and directives contact me on my private email: bwilalessan...@gmail.com Thank you & best regards, Billy Wilfred. Contact me on my private email: bwilalessan...@gmail.com
How are you doing today? Please read my email and reply me!!
Dear Sir/Madam, How are you doing today? My name is Billy Wilfred. I am California, United States of America. I am a broker of Project Financing Firm who has cutting edge and group capital fund, they can finance any lucrative project and help you to enhance your business plan. Are you in need of a Loan to Finance and Fund your Project or Company? Are you an Investor, Real Estate developer, Construction Company, etc? or Do you need a loan to keep your investment or business going on in order to have a different look? Have you been trying to obtain a Loan from Banks or Loan Companies and got Ripped off and they have refused to grant you the Loan because of bad credit? do not close your company and stop your project because of bankruptcy, we are here for you for real, please be happy, rejoice and celebrate because your solution have come, we will end your financial worries now. Therefore come to us, we will grant you the loan you need without delay. We offer all types of non-recourse Loan and Funding at a low Interest Rate of The categories of Loan Financial Funding we offered include but not limited to: Business Loan, Personal Loan, Company Loan, Mortgage Loan, Debt Consolidation and Financial Funding for both Turnkey and mega projects etc from a minimum of Euro / US$1Million to Euro / US$5 Billion Max. Most importantly, Note that the loan company DO NOT charge any upfront fee or advance fee. This message is not scam for what so ever. This is 100% real, legal and legitimate loan company office in Turkey. Kindly get in touch for further details and procedures. Thanks for your cooperation with us. I will be waiting for your response. Further more details and directives contact me on my private email: bwilalessan...@gmail.com Thank you & best regards, Billy Wilfred. Contact me on my private email: bwilalessan...@gmail.com
Re: [PATCH 2/6] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
On Fri, Jul 13, 2018 at 07:36:32AM +0800, Huang, Ying wrote:
> @@ -1260,7 +1257,6 @@ static void swapcache_free(swp_entry_t entry)
> }
> }
>
> -#ifdef CONFIG_THP_SWAP
> static void swapcache_free_cluster(swp_entry_t entry)
> {
> unsigned long offset = swp_offset(entry);
> @@ -1271,6 +1267,9 @@ static void swapcache_free_cluster(swp_entry_t entry)
> unsigned int i, free_entries = 0;
> unsigned char val;
>
> + if (!IS_ENABLED(CONFIG_THP_SWAP))
> + return;
> +
VM_WARN_ON_ONCE(1) here too?
The rest of the patch looks good.
Re: [PATCH 2/6] mm/swapfile.c: Replace some #ifdef with IS_ENABLED()
On Fri, Jul 13, 2018 at 07:36:32AM +0800, Huang, Ying wrote:
> @@ -1260,7 +1257,6 @@ static void swapcache_free(swp_entry_t entry)
> }
> }
>
> -#ifdef CONFIG_THP_SWAP
> static void swapcache_free_cluster(swp_entry_t entry)
> {
> unsigned long offset = swp_offset(entry);
> @@ -1271,6 +1267,9 @@ static void swapcache_free_cluster(swp_entry_t entry)
> unsigned int i, free_entries = 0;
> unsigned char val;
>
> + if (!IS_ENABLED(CONFIG_THP_SWAP))
> + return;
> +
VM_WARN_ON_ONCE(1) here too?
The rest of the patch looks good.
Re: [PATCH] livepatch: Remove reliable stacktrace check in klp_try_switch_task()
> We bail out during patch registration for architectures, those don't > support reliable stack trace. Does anybody know if that change was intentional? I thought the plan was to allow non-consistency-model arches to still use livepatch, and that they'd just have to 'force' patches to completion instead. That seems a little more forgiving. -- Josh
Re: [PATCH] livepatch: Remove reliable stacktrace check in klp_try_switch_task()
> We bail out during patch registration for architectures, those don't > support reliable stack trace. Does anybody know if that change was intentional? I thought the plan was to allow non-consistency-model arches to still use livepatch, and that they'd just have to 'force' patches to completion instead. That seems a little more forgiving. -- Josh
Re: [PATCH] gpio: mxc: add power management support
Hi Anson,
On Tue, Jul 10, 2018 at 4:48 AM, Anson Huang wrote:
> GPIO registers could lose context on some i.MX SoCs,
> like on i.MX7D, when enter LPSR mode, the whole SoC
After further reviewing this patchI have a question: here you say that
i.MX7D needs to save some registers.
> will be powered off except LPSR domain, GPIO banks
> will lose context in this case, need to restore
> the context after resume from LPSR mode.
>
> This patch adds GPIO save/restore for those necessary
> registers, and put the save/restore operations in noirq
> suspend/resume phase, since GPIO is fundamental module
> which could be used by other peripherals' resume phase.
>
> Signed-off-by: Anson Huang
> ---
> drivers/gpio/gpio-mxc.c | 68
> +
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 2f28299..0fc52d8 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -45,6 +45,15 @@ struct mxc_gpio_hwdata {
> unsigned fall_edge;
> };
>
> +struct mxc_gpio_reg_saved {
> + u32 icr1;
> + u32 icr2;
> + u32 imr;
> + u32 gdir;
> + u32 edge_sel;
> + u32 dr;
> +};
> +
> struct mxc_gpio_port {
> struct list_head node;
> void __iomem *base;
> @@ -55,6 +64,7 @@ struct mxc_gpio_port {
> struct gpio_chip gc;
> struct device *dev;
> u32 both_edges;
> + struct mxc_gpio_reg_saved gpio_saved_reg;
> };
>
> static struct mxc_gpio_hwdata imx1_imx21_gpio_hwdata = {
> @@ -497,6 +507,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>
> list_add_tail(>node, _gpio_ports);
>
> + platform_set_drvdata(pdev, port);
> +
> return 0;
>
> out_irqdomain_remove:
> @@ -507,11 +519,67 @@ static int mxc_gpio_probe(struct platform_device *pdev)
> return err;
> }
>
> +static void mxc_gpio_save_regs(struct mxc_gpio_port *port)
> +{
> + if (mxc_gpio_hwtype == IMX21_GPIO)
> + return;
but here you only block IMX21_GPIO.
This means that mx31/mx35/mx51/mx53/mx6 will execute this code too
now. Is this always safe?
Shouldn't it this save/restore be executed only on mx7d?
Please clarify.
Re: [PATCH] gpio: mxc: add power management support
Hi Anson,
On Tue, Jul 10, 2018 at 4:48 AM, Anson Huang wrote:
> GPIO registers could lose context on some i.MX SoCs,
> like on i.MX7D, when enter LPSR mode, the whole SoC
After further reviewing this patchI have a question: here you say that
i.MX7D needs to save some registers.
> will be powered off except LPSR domain, GPIO banks
> will lose context in this case, need to restore
> the context after resume from LPSR mode.
>
> This patch adds GPIO save/restore for those necessary
> registers, and put the save/restore operations in noirq
> suspend/resume phase, since GPIO is fundamental module
> which could be used by other peripherals' resume phase.
>
> Signed-off-by: Anson Huang
> ---
> drivers/gpio/gpio-mxc.c | 68
> +
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
> index 2f28299..0fc52d8 100644
> --- a/drivers/gpio/gpio-mxc.c
> +++ b/drivers/gpio/gpio-mxc.c
> @@ -45,6 +45,15 @@ struct mxc_gpio_hwdata {
> unsigned fall_edge;
> };
>
> +struct mxc_gpio_reg_saved {
> + u32 icr1;
> + u32 icr2;
> + u32 imr;
> + u32 gdir;
> + u32 edge_sel;
> + u32 dr;
> +};
> +
> struct mxc_gpio_port {
> struct list_head node;
> void __iomem *base;
> @@ -55,6 +64,7 @@ struct mxc_gpio_port {
> struct gpio_chip gc;
> struct device *dev;
> u32 both_edges;
> + struct mxc_gpio_reg_saved gpio_saved_reg;
> };
>
> static struct mxc_gpio_hwdata imx1_imx21_gpio_hwdata = {
> @@ -497,6 +507,8 @@ static int mxc_gpio_probe(struct platform_device *pdev)
>
> list_add_tail(>node, _gpio_ports);
>
> + platform_set_drvdata(pdev, port);
> +
> return 0;
>
> out_irqdomain_remove:
> @@ -507,11 +519,67 @@ static int mxc_gpio_probe(struct platform_device *pdev)
> return err;
> }
>
> +static void mxc_gpio_save_regs(struct mxc_gpio_port *port)
> +{
> + if (mxc_gpio_hwtype == IMX21_GPIO)
> + return;
but here you only block IMX21_GPIO.
This means that mx31/mx35/mx51/mx53/mx6 will execute this code too
now. Is this always safe?
Shouldn't it this save/restore be executed only on mx7d?
Please clarify.
Re: [PATCH] locking/rwsem: Take read lock immediate if empty queue with no writer
On 07/13/2018 06:02 AM, Will Deacon wrote: > On Tue, Jul 10, 2018 at 02:31:30PM -0400, Waiman Long wrote: >> It was found that a constant stream of readers might cause the count to >> go negative most of the time after an initial trigger by a writer even >> if no writer was present afterward. As a result, most of the readers >> would have to go through the slowpath reducing their performance. >> >> To avoid that from happening, an additional check is added to detect >> the special case that the reader in the critical section is the only >> one in the wait queue and no writer is present. When that happens, it >> can just have the lock and return immediately without further action. >> Other incoming readers won't see a waiter is present and be forced >> into the slowpath. >> >> After the list_empty() calls, the CPU should have the lock cacheline >> anyway, so an additional semaphore count check shouldn't have any >> performance impact. >> >> Signed-off-by: Waiman Long >> --- >> kernel/locking/rwsem-xadd.c | 16 +++- >> 1 file changed, 15 insertions(+), 1 deletion(-) > This looks ok to me, but it would be nice to include some performance > figures in the commit log. Do you have any? Phrases such as "shouldn't have > any performance impact" and "probably generate better code" don't fill me > with good feelings ;) > > Will I just got the customer testing results back. I had posted a v2 patch with the customer data. I also remove wordings that may cause confusion. Cheers, Longman
Re: [PATCH] locking/rwsem: Take read lock immediate if empty queue with no writer
On 07/13/2018 06:02 AM, Will Deacon wrote: > On Tue, Jul 10, 2018 at 02:31:30PM -0400, Waiman Long wrote: >> It was found that a constant stream of readers might cause the count to >> go negative most of the time after an initial trigger by a writer even >> if no writer was present afterward. As a result, most of the readers >> would have to go through the slowpath reducing their performance. >> >> To avoid that from happening, an additional check is added to detect >> the special case that the reader in the critical section is the only >> one in the wait queue and no writer is present. When that happens, it >> can just have the lock and return immediately without further action. >> Other incoming readers won't see a waiter is present and be forced >> into the slowpath. >> >> After the list_empty() calls, the CPU should have the lock cacheline >> anyway, so an additional semaphore count check shouldn't have any >> performance impact. >> >> Signed-off-by: Waiman Long >> --- >> kernel/locking/rwsem-xadd.c | 16 +++- >> 1 file changed, 15 insertions(+), 1 deletion(-) > This looks ok to me, but it would be nice to include some performance > figures in the commit log. Do you have any? Phrases such as "shouldn't have > any performance impact" and "probably generate better code" don't fill me > with good feelings ;) > > Will I just got the customer testing results back. I had posted a v2 patch with the customer data. I also remove wordings that may cause confusion. Cheers, Longman
RE: [bug] kpti, perf_event, bts: sporadic truncated trace
On Fri, 13 Jul 2018, Metzger, Markus T wrote: > Hello Hugh, > > > A little "optimization" crept into alloc_bts_buffer() along the way, which > > now > > places bts_interrupt_threshold not on a record boundary. > > And Stephane has shown me the sentence in Vol 3B, 17.4.9, which says "This > > address must point to an offset from the BTS buffer base that is a multiple > > of the > > BTS record size." > > > > Please give the patch below a try, and let us know if it helps (if it does > > not, then I > > think we'll need perfier expertise than I can give). > > > > Hugh > > > > --- 4.18-rc4/arch/x86/events/intel/ds.c 2018-06-03 14:15:21.0 > > -0700 > > +++ linux/arch/x86/events/intel/ds.c2018-07-12 17:38:28.471378616 > > -0700 > > @@ -408,9 +408,11 @@ static int alloc_bts_buffer(int cpu) > > ds->bts_buffer_base = (unsigned long) cea; > > ds_update_cea(cea, buffer, BTS_BUFFER_SIZE, PAGE_KERNEL); > > ds->bts_index = ds->bts_buffer_base; > > - max = BTS_RECORD_SIZE * (BTS_BUFFER_SIZE / BTS_RECORD_SIZE); > > - ds->bts_absolute_maximum = ds->bts_buffer_base + max; > > - ds->bts_interrupt_threshold = ds->bts_absolute_maximum - (max / 16); > > + max = BTS_BUFFER_SIZE / BTS_RECORD_SIZE; > > + ds->bts_absolute_maximum = ds->bts_buffer_base + > > + max * BTS_RECORD_SIZE; > > + ds->bts_interrupt_threshold = ds->bts_absolute_maximum - > > + (max / 16) * BTS_RECORD_SIZE; > > return 0; > > } > > > > Your patch fixes it. Oh, very welcome news! Thanks for putting in all the effort you did to track down from your end: I'm relieved it was easier to fix than that. > > Will you send it to the list for inclusion? Sure, I'll send to x86 guys and lkml later today. I can't promise that it will make 4.18, since the regression it fixes pre-dates 4.18-rc; but they don't usually insist on that rule for something as small and benign as this, so I expect it'll make it. > I'd appreciate if it could also be backported > to 4.15, 4.16, and 4.17. I'll mark it Cc stable, so that it will soon percolate through to the 4.14 longterm and 4.17 stable trees. But 4.15 and 4.16 stable trees are now EOL, so you'll have to apply the patch where you need it yourself - it has no release-dependent subtlety, it should apply cleanly at offset. Hugh
RE: [bug] kpti, perf_event, bts: sporadic truncated trace
On Fri, 13 Jul 2018, Metzger, Markus T wrote: > Hello Hugh, > > > A little "optimization" crept into alloc_bts_buffer() along the way, which > > now > > places bts_interrupt_threshold not on a record boundary. > > And Stephane has shown me the sentence in Vol 3B, 17.4.9, which says "This > > address must point to an offset from the BTS buffer base that is a multiple > > of the > > BTS record size." > > > > Please give the patch below a try, and let us know if it helps (if it does > > not, then I > > think we'll need perfier expertise than I can give). > > > > Hugh > > > > --- 4.18-rc4/arch/x86/events/intel/ds.c 2018-06-03 14:15:21.0 > > -0700 > > +++ linux/arch/x86/events/intel/ds.c2018-07-12 17:38:28.471378616 > > -0700 > > @@ -408,9 +408,11 @@ static int alloc_bts_buffer(int cpu) > > ds->bts_buffer_base = (unsigned long) cea; > > ds_update_cea(cea, buffer, BTS_BUFFER_SIZE, PAGE_KERNEL); > > ds->bts_index = ds->bts_buffer_base; > > - max = BTS_RECORD_SIZE * (BTS_BUFFER_SIZE / BTS_RECORD_SIZE); > > - ds->bts_absolute_maximum = ds->bts_buffer_base + max; > > - ds->bts_interrupt_threshold = ds->bts_absolute_maximum - (max / 16); > > + max = BTS_BUFFER_SIZE / BTS_RECORD_SIZE; > > + ds->bts_absolute_maximum = ds->bts_buffer_base + > > + max * BTS_RECORD_SIZE; > > + ds->bts_interrupt_threshold = ds->bts_absolute_maximum - > > + (max / 16) * BTS_RECORD_SIZE; > > return 0; > > } > > > > Your patch fixes it. Oh, very welcome news! Thanks for putting in all the effort you did to track down from your end: I'm relieved it was easier to fix than that. > > Will you send it to the list for inclusion? Sure, I'll send to x86 guys and lkml later today. I can't promise that it will make 4.18, since the regression it fixes pre-dates 4.18-rc; but they don't usually insist on that rule for something as small and benign as this, so I expect it'll make it. > I'd appreciate if it could also be backported > to 4.15, 4.16, and 4.17. I'll mark it Cc stable, so that it will soon percolate through to the 4.14 longterm and 4.17 stable trees. But 4.15 and 4.16 stable trees are now EOL, so you'll have to apply the patch where you need it yourself - it has no release-dependent subtlety, it should apply cleanly at offset. Hugh
[PATCH v2] locking/rwsem: Take read lock immediate if queue empty with no writer
It was discovered that a constant stream of readers might cause the
count to go negative most of the time after an initial trigger by a
writer even if no writer was present afterward. As a result, most of the
readers would have to go through the slowpath reducing their performance.
To avoid that from happening, an additional check is added to detect
the special case that the reader in the critical section is the only
one in the wait queue and no writer is present. When that happens, it
can just have the lock and return immediately without further action.
Other incoming readers won't see a waiter is present and be forced
into the slowpath.
The additional code is in the slowpath and so should not have an impact
on rwsem performance. However, in the special case listed above, it may
greatly improve performance.
The issue was found in a customer site where they had an application
that pounded on the pread64 syscalls heavily on an XFS filesystem. The
application was run in a recent 4-socket boxes with a lot of CPUs. They
saw significant spinlock contention in the rwsem_down_read_failed() call.
With this patch applied, the system CPU usage went from 85% to 57%,
and the spinlock contention in the pread64 syscalls was gone.
v2: Add customer testing results and remove wording that may cause
confusion.
Signed-off-by: Waiman Long
---
kernel/locking/rwsem-xadd.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 3064c50..bf0570e 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -233,8 +233,19 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
waiter.type = RWSEM_WAITING_FOR_READ;
raw_spin_lock_irq(>wait_lock);
- if (list_empty(>wait_list))
+ if (list_empty(>wait_list)) {
+ /*
+* In the unlikely event that the task is the only one in
+* the wait queue and a writer isn't present, it can have
+* the lock and return immediately without going through
+* the remaining slowpath code.
+*/
+ if (unlikely(atomic_long_read(>count) >= 0)) {
+ raw_spin_unlock_irq(>wait_lock);
+ return sem;
+ }
adjustment += RWSEM_WAITING_BIAS;
+ }
list_add_tail(, >wait_list);
/* we're now waiting on the lock, but no longer actively locking */
--
1.8.3.1
[PATCH v2] locking/rwsem: Take read lock immediate if queue empty with no writer
It was discovered that a constant stream of readers might cause the
count to go negative most of the time after an initial trigger by a
writer even if no writer was present afterward. As a result, most of the
readers would have to go through the slowpath reducing their performance.
To avoid that from happening, an additional check is added to detect
the special case that the reader in the critical section is the only
one in the wait queue and no writer is present. When that happens, it
can just have the lock and return immediately without further action.
Other incoming readers won't see a waiter is present and be forced
into the slowpath.
The additional code is in the slowpath and so should not have an impact
on rwsem performance. However, in the special case listed above, it may
greatly improve performance.
The issue was found in a customer site where they had an application
that pounded on the pread64 syscalls heavily on an XFS filesystem. The
application was run in a recent 4-socket boxes with a lot of CPUs. They
saw significant spinlock contention in the rwsem_down_read_failed() call.
With this patch applied, the system CPU usage went from 85% to 57%,
and the spinlock contention in the pread64 syscalls was gone.
v2: Add customer testing results and remove wording that may cause
confusion.
Signed-off-by: Waiman Long
---
kernel/locking/rwsem-xadd.c | 13 -
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/kernel/locking/rwsem-xadd.c b/kernel/locking/rwsem-xadd.c
index 3064c50..bf0570e 100644
--- a/kernel/locking/rwsem-xadd.c
+++ b/kernel/locking/rwsem-xadd.c
@@ -233,8 +233,19 @@ static void __rwsem_mark_wake(struct rw_semaphore *sem,
waiter.type = RWSEM_WAITING_FOR_READ;
raw_spin_lock_irq(>wait_lock);
- if (list_empty(>wait_list))
+ if (list_empty(>wait_list)) {
+ /*
+* In the unlikely event that the task is the only one in
+* the wait queue and a writer isn't present, it can have
+* the lock and return immediately without going through
+* the remaining slowpath code.
+*/
+ if (unlikely(atomic_long_read(>count) >= 0)) {
+ raw_spin_unlock_irq(>wait_lock);
+ return sem;
+ }
adjustment += RWSEM_WAITING_BIAS;
+ }
list_add_tail(, >wait_list);
/* we're now waiting on the lock, but no longer actively locking */
--
1.8.3.1
[PATCH 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
This patch adds optional regulators, which can be used to power
up touchscreen. After enabling regulators, we need to wait 150msec.
This value is taken from official driver.
It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
Signed-off-by: Paweł Chmiel
---
drivers/input/touchscreen/atmel_mxt_ts.c | 45
1 file changed, 45 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
b/drivers/input/touchscreen/atmel_mxt_ts.c
index 54fe190fd4bc..a7625ec8fb9f 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -198,6 +199,7 @@ enum t100_type {
#define MXT_RESET_INVALID_CHG 100 /* msec */
#define MXT_RESET_TIME 200 /* msec */
#define MXT_RESET_TIMEOUT 3000/* msec */
+#define MXT_REGULATOR_DELAY150 /* msec */
#define MXT_CRC_TIMEOUT1000/* msec */
#define MXT_FW_RESET_TIME 3000/* msec */
#define MXT_FW_CHG_TIMEOUT 300 /* msec */
@@ -310,6 +312,8 @@ struct mxt_data {
struct t7_config t7_cfg;
struct mxt_dbg dbg;
struct gpio_desc *reset_gpio;
+ struct regulator *vdd_reg;
+ struct regulator *avdd_reg;
/* Cached parameters from object table */
u16 T5_address;
@@ -3076,6 +3080,40 @@ static int mxt_probe(struct i2c_client *client, const
struct i2c_device_id *id)
return error;
}
+ data->vdd_reg = devm_regulator_get_optional(>dev, "vdd");
+ if (IS_ERR(data->vdd_reg)) {
+ error = PTR_ERR(data->vdd_reg);
+ dev_err(>dev, "Failed to get vdd regulator: %d\n",
+ error);
+ return error;
+ }
+
+ if (data->vdd_reg) {
+ error = regulator_enable(data->vdd_reg);
+ if (error) {
+ dev_err(>dev, "Failed to enable vdd regulator:
%d\n",
+ error);
+ return error;
+ }
+ }
+
+ data->avdd_reg = devm_regulator_get_optional(>dev, "avdd");
+ if (IS_ERR(data->avdd_reg)) {
+ error = PTR_ERR(data->avdd_reg);
+ dev_err(>dev, "Failed to get avdd regulator: %d\n",
+ error);
+ return error;
+ }
+
+ if (data->avdd_reg) {
+ error = regulator_enable(data->avdd_reg);
+ if (error) {
+ dev_err(>dev, "Failed to enable avdd regulator:
%d\n",
+ error);
+ return error;
+ }
+ }
+
error = devm_request_threaded_irq(>dev, client->irq,
NULL, mxt_interrupt, IRQF_ONESHOT,
client->name, data);
@@ -3086,6 +3124,9 @@ static int mxt_probe(struct i2c_client *client, const
struct i2c_device_id *id)
disable_irq(client->irq);
+ if (!IS_ERR(data->vdd_reg) || !IS_ERR(data->avdd_reg))
+ msleep(MXT_REGULATOR_DELAY);
+
if (data->reset_gpio) {
msleep(MXT_RESET_GPIO_TIME);
gpiod_set_value(data->reset_gpio, 1);
@@ -3116,6 +3157,10 @@ static int mxt_remove(struct i2c_client *client)
struct mxt_data *data = i2c_get_clientdata(client);
disable_irq(data->irq);
+ if (!IS_ERR(data->avdd_reg))
+ regulator_disable(data->avdd_reg);
+ if (!IS_ERR(data->vdd_reg))
+ regulator_disable(data->vdd_reg);
sysfs_remove_group(>dev.kobj, _attr_group);
mxt_free_input_device(data);
mxt_free_object_table(data);
--
2.7.4
[PATCH 1/2] Input: atmel_mxt_ts: Add support for optional regulators.
This patch adds optional regulators, which can be used to power
up touchscreen. After enabling regulators, we need to wait 150msec.
This value is taken from official driver.
It was tested on Samsung Galaxy i9000 (based on Samsung S5PV210 SOC).
Signed-off-by: Paweł Chmiel
---
drivers/input/touchscreen/atmel_mxt_ts.c | 45
1 file changed, 45 insertions(+)
diff --git a/drivers/input/touchscreen/atmel_mxt_ts.c
b/drivers/input/touchscreen/atmel_mxt_ts.c
index 54fe190fd4bc..a7625ec8fb9f 100644
--- a/drivers/input/touchscreen/atmel_mxt_ts.c
+++ b/drivers/input/touchscreen/atmel_mxt_ts.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -198,6 +199,7 @@ enum t100_type {
#define MXT_RESET_INVALID_CHG 100 /* msec */
#define MXT_RESET_TIME 200 /* msec */
#define MXT_RESET_TIMEOUT 3000/* msec */
+#define MXT_REGULATOR_DELAY150 /* msec */
#define MXT_CRC_TIMEOUT1000/* msec */
#define MXT_FW_RESET_TIME 3000/* msec */
#define MXT_FW_CHG_TIMEOUT 300 /* msec */
@@ -310,6 +312,8 @@ struct mxt_data {
struct t7_config t7_cfg;
struct mxt_dbg dbg;
struct gpio_desc *reset_gpio;
+ struct regulator *vdd_reg;
+ struct regulator *avdd_reg;
/* Cached parameters from object table */
u16 T5_address;
@@ -3076,6 +3080,40 @@ static int mxt_probe(struct i2c_client *client, const
struct i2c_device_id *id)
return error;
}
+ data->vdd_reg = devm_regulator_get_optional(>dev, "vdd");
+ if (IS_ERR(data->vdd_reg)) {
+ error = PTR_ERR(data->vdd_reg);
+ dev_err(>dev, "Failed to get vdd regulator: %d\n",
+ error);
+ return error;
+ }
+
+ if (data->vdd_reg) {
+ error = regulator_enable(data->vdd_reg);
+ if (error) {
+ dev_err(>dev, "Failed to enable vdd regulator:
%d\n",
+ error);
+ return error;
+ }
+ }
+
+ data->avdd_reg = devm_regulator_get_optional(>dev, "avdd");
+ if (IS_ERR(data->avdd_reg)) {
+ error = PTR_ERR(data->avdd_reg);
+ dev_err(>dev, "Failed to get avdd regulator: %d\n",
+ error);
+ return error;
+ }
+
+ if (data->avdd_reg) {
+ error = regulator_enable(data->avdd_reg);
+ if (error) {
+ dev_err(>dev, "Failed to enable avdd regulator:
%d\n",
+ error);
+ return error;
+ }
+ }
+
error = devm_request_threaded_irq(>dev, client->irq,
NULL, mxt_interrupt, IRQF_ONESHOT,
client->name, data);
@@ -3086,6 +3124,9 @@ static int mxt_probe(struct i2c_client *client, const
struct i2c_device_id *id)
disable_irq(client->irq);
+ if (!IS_ERR(data->vdd_reg) || !IS_ERR(data->avdd_reg))
+ msleep(MXT_REGULATOR_DELAY);
+
if (data->reset_gpio) {
msleep(MXT_RESET_GPIO_TIME);
gpiod_set_value(data->reset_gpio, 1);
@@ -3116,6 +3157,10 @@ static int mxt_remove(struct i2c_client *client)
struct mxt_data *data = i2c_get_clientdata(client);
disable_irq(data->irq);
+ if (!IS_ERR(data->avdd_reg))
+ regulator_disable(data->avdd_reg);
+ if (!IS_ERR(data->vdd_reg))
+ regulator_disable(data->vdd_reg);
sysfs_remove_group(>dev.kobj, _attr_group);
mxt_free_input_device(data);
mxt_free_object_table(data);
--
2.7.4
[PATCH 2/2] Input: atmel_mxt_ts: Document optional voltage regulators
Document new optional voltage regulators, which can be used
to power down/up touchscreen.
Signed-off-by: Paweł Chmiel
---
Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index c88919480d37..17930ecadad3 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -31,6 +31,12 @@ Optional properties for main touchpad device:
- reset-gpios: GPIO specifier for the touchscreen's reset pin (active low)
+- avdd-supply: Analog power supply. It powers up the analog channel block
+of the controller to detect the touches.
+
+- vdd-supply: Digital power supply. It powers up the digital block
+of the controller to enable i2c communication.
+
Example:
touch@4b {
@@ -38,4 +44,6 @@ Example:
reg = <0x4b>;
interrupt-parent = <>;
interrupts = ;
+ avdd-supply = <_reg>;
+ vdd-supply = <_reg>;
};
--
2.7.4
[PATCH 2/2] Input: atmel_mxt_ts: Document optional voltage regulators
Document new optional voltage regulators, which can be used
to power down/up touchscreen.
Signed-off-by: Paweł Chmiel
---
Documentation/devicetree/bindings/input/atmel,maxtouch.txt | 8
1 file changed, 8 insertions(+)
diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
index c88919480d37..17930ecadad3 100644
--- a/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
+++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.txt
@@ -31,6 +31,12 @@ Optional properties for main touchpad device:
- reset-gpios: GPIO specifier for the touchscreen's reset pin (active low)
+- avdd-supply: Analog power supply. It powers up the analog channel block
+of the controller to detect the touches.
+
+- vdd-supply: Digital power supply. It powers up the digital block
+of the controller to enable i2c communication.
+
Example:
touch@4b {
@@ -38,4 +44,6 @@ Example:
reg = <0x4b>;
interrupt-parent = <>;
interrupts = ;
+ avdd-supply = <_reg>;
+ vdd-supply = <_reg>;
};
--
2.7.4
[PATCH 0/2] Input: atmel_mxt_ts: Add support for optional regulators
This two patches add optional regulator support to atmel_mxt_ts. First patch adds regulators to driver. Second patch updates documentation. Paweł Chmiel (2): Input: atmel_mxt_ts: Add support for optional regulators. Input: atmel_mxt_ts: Document optional voltage regulators .../devicetree/bindings/input/atmel,maxtouch.txt | 8 drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++ 2 files changed, 53 insertions(+) -- 2.7.4
[PATCH 0/2] Input: atmel_mxt_ts: Add support for optional regulators
This two patches add optional regulator support to atmel_mxt_ts. First patch adds regulators to driver. Second patch updates documentation. Paweł Chmiel (2): Input: atmel_mxt_ts: Add support for optional regulators. Input: atmel_mxt_ts: Document optional voltage regulators .../devicetree/bindings/input/atmel,maxtouch.txt | 8 drivers/input/touchscreen/atmel_mxt_ts.c | 45 ++ 2 files changed, 53 insertions(+) -- 2.7.4
[GIT PULL] rseq fixes
Linus,
Please pull the latest core-urgent-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
core-urgent-for-linus
# HEAD: 8a46580128a02bdc18d7dcc0cba19d3cea4fb9c4 rseq/selftests: cleanup:
Update comment above rseq_prepare_unload
Various rseq ABI fixes and cleanups: use get_user()/put_user(), validate
parameters and use proper uapi types, etc.
Thanks,
Ingo
-->
Mathieu Desnoyers (6):
rseq: Use __u64 for rseq_cs fields, validate user inputs
rseq: Use get_user/put_user rather than __get_user/__put_user
rseq: uapi: Update uapi comments
rseq: uapi: Declare rseq_cs field as union, update includes
rseq: Remove unused types_32_64.h uapi header
rseq/selftests: cleanup: Update comment above rseq_prepare_unload
include/uapi/linux/rseq.h | 102
include/uapi/linux/types_32_64.h| 50 --
kernel/rseq.c | 41 +--
tools/testing/selftests/rseq/rseq.h | 24 ++---
4 files changed, 100 insertions(+), 117 deletions(-)
delete mode 100644 include/uapi/linux/types_32_64.h
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index d620fa43756c..9a402fdb60e9 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -10,13 +10,8 @@
* Copyright (c) 2015-2018 Mathieu Desnoyers
*/
-#ifdef __KERNEL__
-# include
-#else
-# include
-#endif
-
-#include
+#include
+#include
enum rseq_cpu_id_state {
RSEQ_CPU_ID_UNINITIALIZED = -1,
@@ -52,10 +47,10 @@ struct rseq_cs {
__u32 version;
/* enum rseq_cs_flags */
__u32 flags;
- LINUX_FIELD_u32_u64(start_ip);
+ __u64 start_ip;
/* Offset from start_ip. */
- LINUX_FIELD_u32_u64(post_commit_offset);
- LINUX_FIELD_u32_u64(abort_ip);
+ __u64 post_commit_offset;
+ __u64 abort_ip;
} __attribute__((aligned(4 * sizeof(__u64;
/*
@@ -67,28 +62,30 @@ struct rseq_cs {
struct rseq {
/*
* Restartable sequences cpu_id_start field. Updated by the
-* kernel, and read by user-space with single-copy atomicity
-* semantics. Aligned on 32-bit. Always contains a value in the
-* range of possible CPUs, although the value may not be the
-* actual current CPU (e.g. if rseq is not initialized). This
-* CPU number value should always be compared against the value
-* of the cpu_id field before performing a rseq commit or
-* returning a value read from a data structure indexed using
-* the cpu_id_start value.
+* kernel. Read by user-space with single-copy atomicity
+* semantics. This field should only be read by the thread which
+* registered this data structure. Aligned on 32-bit. Always
+* contains a value in the range of possible CPUs, although the
+* value may not be the actual current CPU (e.g. if rseq is not
+* initialized). This CPU number value should always be compared
+* against the value of the cpu_id field before performing a rseq
+* commit or returning a value read from a data structure indexed
+* using the cpu_id_start value.
*/
__u32 cpu_id_start;
/*
-* Restartable sequences cpu_id field. Updated by the kernel,
-* and read by user-space with single-copy atomicity semantics.
-* Aligned on 32-bit. Values RSEQ_CPU_ID_UNINITIALIZED and
-* RSEQ_CPU_ID_REGISTRATION_FAILED have a special semantic: the
-* former means "rseq uninitialized", and latter means "rseq
-* initialization failed". This value is meant to be read within
-* rseq critical sections and compared with the cpu_id_start
-* value previously read, before performing the commit instruction,
-* or read and compared with the cpu_id_start value before returning
-* a value loaded from a data structure indexed using the
-* cpu_id_start value.
+* Restartable sequences cpu_id field. Updated by the kernel.
+* Read by user-space with single-copy atomicity semantics. This
+* field should only be read by the thread which registered this
+* data structure. Aligned on 32-bit. Values
+* RSEQ_CPU_ID_UNINITIALIZED and RSEQ_CPU_ID_REGISTRATION_FAILED
+* have a special semantic: the former means "rseq uninitialized",
+* and latter means "rseq initialization failed". This value is
+* meant to be read within rseq critical sections and compared
+* with the cpu_id_start value previously read, before performing
+* the commit instruction, or read and compared with the
+* cpu_id_start value before returning a value loaded from a data
+* structure indexed using the cpu_id_start value.
*/
__u32 cpu_id;
/*
@@ -105,27 +102,44 @@ struct rseq {
*
[GIT PULL] rseq fixes
Linus,
Please pull the latest core-urgent-for-linus git tree from:
git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git
core-urgent-for-linus
# HEAD: 8a46580128a02bdc18d7dcc0cba19d3cea4fb9c4 rseq/selftests: cleanup:
Update comment above rseq_prepare_unload
Various rseq ABI fixes and cleanups: use get_user()/put_user(), validate
parameters and use proper uapi types, etc.
Thanks,
Ingo
-->
Mathieu Desnoyers (6):
rseq: Use __u64 for rseq_cs fields, validate user inputs
rseq: Use get_user/put_user rather than __get_user/__put_user
rseq: uapi: Update uapi comments
rseq: uapi: Declare rseq_cs field as union, update includes
rseq: Remove unused types_32_64.h uapi header
rseq/selftests: cleanup: Update comment above rseq_prepare_unload
include/uapi/linux/rseq.h | 102
include/uapi/linux/types_32_64.h| 50 --
kernel/rseq.c | 41 +--
tools/testing/selftests/rseq/rseq.h | 24 ++---
4 files changed, 100 insertions(+), 117 deletions(-)
delete mode 100644 include/uapi/linux/types_32_64.h
diff --git a/include/uapi/linux/rseq.h b/include/uapi/linux/rseq.h
index d620fa43756c..9a402fdb60e9 100644
--- a/include/uapi/linux/rseq.h
+++ b/include/uapi/linux/rseq.h
@@ -10,13 +10,8 @@
* Copyright (c) 2015-2018 Mathieu Desnoyers
*/
-#ifdef __KERNEL__
-# include
-#else
-# include
-#endif
-
-#include
+#include
+#include
enum rseq_cpu_id_state {
RSEQ_CPU_ID_UNINITIALIZED = -1,
@@ -52,10 +47,10 @@ struct rseq_cs {
__u32 version;
/* enum rseq_cs_flags */
__u32 flags;
- LINUX_FIELD_u32_u64(start_ip);
+ __u64 start_ip;
/* Offset from start_ip. */
- LINUX_FIELD_u32_u64(post_commit_offset);
- LINUX_FIELD_u32_u64(abort_ip);
+ __u64 post_commit_offset;
+ __u64 abort_ip;
} __attribute__((aligned(4 * sizeof(__u64;
/*
@@ -67,28 +62,30 @@ struct rseq_cs {
struct rseq {
/*
* Restartable sequences cpu_id_start field. Updated by the
-* kernel, and read by user-space with single-copy atomicity
-* semantics. Aligned on 32-bit. Always contains a value in the
-* range of possible CPUs, although the value may not be the
-* actual current CPU (e.g. if rseq is not initialized). This
-* CPU number value should always be compared against the value
-* of the cpu_id field before performing a rseq commit or
-* returning a value read from a data structure indexed using
-* the cpu_id_start value.
+* kernel. Read by user-space with single-copy atomicity
+* semantics. This field should only be read by the thread which
+* registered this data structure. Aligned on 32-bit. Always
+* contains a value in the range of possible CPUs, although the
+* value may not be the actual current CPU (e.g. if rseq is not
+* initialized). This CPU number value should always be compared
+* against the value of the cpu_id field before performing a rseq
+* commit or returning a value read from a data structure indexed
+* using the cpu_id_start value.
*/
__u32 cpu_id_start;
/*
-* Restartable sequences cpu_id field. Updated by the kernel,
-* and read by user-space with single-copy atomicity semantics.
-* Aligned on 32-bit. Values RSEQ_CPU_ID_UNINITIALIZED and
-* RSEQ_CPU_ID_REGISTRATION_FAILED have a special semantic: the
-* former means "rseq uninitialized", and latter means "rseq
-* initialization failed". This value is meant to be read within
-* rseq critical sections and compared with the cpu_id_start
-* value previously read, before performing the commit instruction,
-* or read and compared with the cpu_id_start value before returning
-* a value loaded from a data structure indexed using the
-* cpu_id_start value.
+* Restartable sequences cpu_id field. Updated by the kernel.
+* Read by user-space with single-copy atomicity semantics. This
+* field should only be read by the thread which registered this
+* data structure. Aligned on 32-bit. Values
+* RSEQ_CPU_ID_UNINITIALIZED and RSEQ_CPU_ID_REGISTRATION_FAILED
+* have a special semantic: the former means "rseq uninitialized",
+* and latter means "rseq initialization failed". This value is
+* meant to be read within rseq critical sections and compared
+* with the cpu_id_start value previously read, before performing
+* the commit instruction, or read and compared with the
+* cpu_id_start value before returning a value loaded from a data
+* structure indexed using the cpu_id_start value.
*/
__u32 cpu_id;
/*
@@ -105,27 +102,44 @@ struct rseq {
*
Re: [PATCH 2/2] pinctrl: berlin: add the as370 SoC pinctrl driver
Hi Jisheng,
I love your patch! Yet something to improve:
[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v4.18-rc4 next-20180713]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Jisheng-Zhang/dt-binding-pinctrl-berlin-document-AS370-SoC-pinctrl/20180713-235943
base:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: sparse:
'MODULE_DEVICE_TABLE()' has implicit return type
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: data definition has
>> no type or storage class
MODULE_DEVICE_TABLE(of, as370_pinctrl_match);
^~~
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: error: type defaults to 'int'
>> in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: parameter names
>> (without types) in function declaration
cc1: some warnings being treated as errors
sparse warnings: (new ones prefixed by >>)
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: sparse:
>> 'MODULE_DEVICE_TABLE()' has implicit return type
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: data definition has
no type or storage class
MODULE_DEVICE_TABLE(of, as370_pinctrl_match);
^~~
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: error: type defaults to 'int'
in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: parameter names
(without types) in function declaration
cc1: some warnings being treated as errors
vim +330 drivers/pinctrl/berlin/pinctrl-as370.c
322
323 static const struct of_device_id as370_pinctrl_match[] = {
324 {
325 .compatible = "syna,as370-soc-pinctrl",
326 .data = _soc_pinctrl_data,
327 },
328 {}
329 };
> 330 MODULE_DEVICE_TABLE(of, as370_pinctrl_match);
331
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH 2/2] pinctrl: berlin: add the as370 SoC pinctrl driver
Hi Jisheng,
I love your patch! Yet something to improve:
[auto build test ERROR on pinctrl/devel]
[also build test ERROR on v4.18-rc4 next-20180713]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Jisheng-Zhang/dt-binding-pinctrl-berlin-document-AS370-SoC-pinctrl/20180713-235943
base:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git devel
config: x86_64-allmodconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-16) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: sparse:
'MODULE_DEVICE_TABLE()' has implicit return type
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: data definition has
>> no type or storage class
MODULE_DEVICE_TABLE(of, as370_pinctrl_match);
^~~
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: error: type defaults to 'int'
>> in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: parameter names
>> (without types) in function declaration
cc1: some warnings being treated as errors
sparse warnings: (new ones prefixed by >>)
>> drivers/pinctrl/berlin/pinctrl-as370.c:330:1: sparse:
>> 'MODULE_DEVICE_TABLE()' has implicit return type
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: data definition has
no type or storage class
MODULE_DEVICE_TABLE(of, as370_pinctrl_match);
^~~
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: error: type defaults to 'int'
in declaration of 'MODULE_DEVICE_TABLE' [-Werror=implicit-int]
drivers/pinctrl/berlin/pinctrl-as370.c:330:1: warning: parameter names
(without types) in function declaration
cc1: some warnings being treated as errors
vim +330 drivers/pinctrl/berlin/pinctrl-as370.c
322
323 static const struct of_device_id as370_pinctrl_match[] = {
324 {
325 .compatible = "syna,as370-soc-pinctrl",
326 .data = _soc_pinctrl_data,
327 },
328 {}
329 };
> 330 MODULE_DEVICE_TABLE(of, as370_pinctrl_match);
331
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: application/gzip
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On Fri, Jul 13, 2018 at 10:22 AM Jae Hyun Yoo
wrote:
>
> On 7/12/2018 11:21 AM, Jae Hyun Yoo wrote:
> > On 7/12/2018 2:33 AM, Brendan Higgins wrote:
> >> On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
> >> wrote:
> >>
> > + for (;;) {
> > + if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> > + (ASPEED_I2CD_BUS_BUSY_STS |
> > + ASPEED_I2CD_XFER_MODE_STS_MASK)))
>
> Is using the Transfer Mode State Machine bits necessary? The
> documentation marks it as "for debugging purpose only," so relying on
> it makes me nervous.
>
> >>>
> >>> As you said, the documentation marks it as "for debugging purpose only."
> >>> but ASPEED also uses this way in their SDK code because it's the best
> >>> way for checking bus busy status which can cover both single and
> >>> multi-master use cases.
> >>>
> >>
> >> Well, it would also be really nice to have access to this bit if
> >> someone wants
> >> to implement MCTP. Could we maybe check with Aspeed what them meant by
> >> "for
> >> debugging purposes only" and document it here? It makes me nervous to
> >> rely on
> >> debugging functionality for normal usage.
> >>
> >
> > Okay, I'll check it with Aspeed. Will let you know their response.
> >
>
> I've checked it with Gary Hsu and he confirmed
> that the bits reflect real information and good to be used in practical
> code.
Huh. For my own edification, could you ask them why they said "for debugging
purpose only" in the documentation? I am just really curious what they meant by
that. I would be satisfied if you just CC'ed me on your email thread with Gary,
and I can ask him myself.
>
> I'll add a comment like below:
>
> /*
> * This is marked as 'for debugging purpose only' in datasheet but
> * ASPEED confirmed that this reflects real information and good
> * to be used in practical code.
> */
>
> Is it acceptable then?
Yeah, that's fine.
Cheers
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On Fri, Jul 13, 2018 at 10:22 AM Jae Hyun Yoo
wrote:
>
> On 7/12/2018 11:21 AM, Jae Hyun Yoo wrote:
> > On 7/12/2018 2:33 AM, Brendan Higgins wrote:
> >> On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
> >> wrote:
> >>
> > + for (;;) {
> > + if (!(readl(bus->base + ASPEED_I2C_CMD_REG) &
> > + (ASPEED_I2CD_BUS_BUSY_STS |
> > + ASPEED_I2CD_XFER_MODE_STS_MASK)))
>
> Is using the Transfer Mode State Machine bits necessary? The
> documentation marks it as "for debugging purpose only," so relying on
> it makes me nervous.
>
> >>>
> >>> As you said, the documentation marks it as "for debugging purpose only."
> >>> but ASPEED also uses this way in their SDK code because it's the best
> >>> way for checking bus busy status which can cover both single and
> >>> multi-master use cases.
> >>>
> >>
> >> Well, it would also be really nice to have access to this bit if
> >> someone wants
> >> to implement MCTP. Could we maybe check with Aspeed what them meant by
> >> "for
> >> debugging purposes only" and document it here? It makes me nervous to
> >> rely on
> >> debugging functionality for normal usage.
> >>
> >
> > Okay, I'll check it with Aspeed. Will let you know their response.
> >
>
> I've checked it with Gary Hsu and he confirmed
> that the bits reflect real information and good to be used in practical
> code.
Huh. For my own edification, could you ask them why they said "for debugging
purpose only" in the documentation? I am just really curious what they meant by
that. I would be satisfied if you just CC'ed me on your email thread with Gary,
and I can ask him myself.
>
> I'll add a comment like below:
>
> /*
> * This is marked as 'for debugging purpose only' in datasheet but
> * ASPEED confirmed that this reflects real information and good
> * to be used in practical code.
> */
>
> Is it acceptable then?
Yeah, that's fine.
Cheers
[PATCH] staging: rtl8188eu: remove is_{multicast,broadcast}_mac_addr
Remove custom is_multicast_mac_addr() and is_broadcast_mac_addr().
Use is_multicast_ether_addr() instead.
By definition the broadcast address is also a multicast address.
is_multicast_ether_addr() returns true for broadcast addresses.
Hence checking for multicast in the conditional is sufficient.
Signed-off-by: Michael Straube
---
drivers/staging/rtl8188eu/core/rtw_recv.c | 3 +--
drivers/staging/rtl8188eu/include/ieee80211.h | 11 ---
2 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c
b/drivers/staging/rtl8188eu/core/rtw_recv.c
index 79567cf470de..53c65d5463c0 100644
--- a/drivers/staging/rtl8188eu/core/rtw_recv.c
+++ b/drivers/staging/rtl8188eu/core/rtw_recv.c
@@ -952,8 +952,7 @@ static int validate_recv_mgnt_frame(struct adapter
*padapter,
if (!memcmp(padapter->eeprompriv.mac_addr,
GetAddr1Ptr(precv_frame->pkt->data),
ETH_ALEN))
psta->sta_stats.rx_probersp_pkts++;
- else if
(is_broadcast_mac_addr(GetAddr1Ptr(precv_frame->pkt->data)) ||
-
is_multicast_mac_addr(GetAddr1Ptr(precv_frame->pkt->data)))
+ else if
(is_multicast_ether_addr(GetAddr1Ptr(precv_frame->pkt->data)))
psta->sta_stats.rx_probersp_bm_pkts++;
else
psta->sta_stats.rx_probersp_uo_pkts++;
diff --git a/drivers/staging/rtl8188eu/include/ieee80211.h
b/drivers/staging/rtl8188eu/include/ieee80211.h
index 1668bb531ead..3fbd92a95b17 100644
--- a/drivers/staging/rtl8188eu/include/ieee80211.h
+++ b/drivers/staging/rtl8188eu/include/ieee80211.h
@@ -515,17 +515,6 @@ enum ieee80211_state {
#define DEFAULT_MAX_SCAN_AGE (15 * HZ)
#define DEFAULT_FTS 2346
-static inline int is_multicast_mac_addr(const u8 *addr)
-{
- return ((addr[0] != 0xff) && (0x01 & addr[0]));
-}
-
-static inline int is_broadcast_mac_addr(const u8 *addr)
-{
- return (addr[0] == 0xff) && (addr[1] == 0xff) && (addr[2] == 0xff) &&
- (addr[3] == 0xff) && (addr[4] == 0xff) && (addr[5] == 0xff);
-}
-
#define CFG_IEEE80211_RESERVE_FCS BIT(0)
#define CFG_IEEE80211_COMPUTE_FCS BIT(1)
--
2.18.0
[PATCH] staging: rtl8188eu: remove is_{multicast,broadcast}_mac_addr
Remove custom is_multicast_mac_addr() and is_broadcast_mac_addr().
Use is_multicast_ether_addr() instead.
By definition the broadcast address is also a multicast address.
is_multicast_ether_addr() returns true for broadcast addresses.
Hence checking for multicast in the conditional is sufficient.
Signed-off-by: Michael Straube
---
drivers/staging/rtl8188eu/core/rtw_recv.c | 3 +--
drivers/staging/rtl8188eu/include/ieee80211.h | 11 ---
2 files changed, 1 insertion(+), 13 deletions(-)
diff --git a/drivers/staging/rtl8188eu/core/rtw_recv.c
b/drivers/staging/rtl8188eu/core/rtw_recv.c
index 79567cf470de..53c65d5463c0 100644
--- a/drivers/staging/rtl8188eu/core/rtw_recv.c
+++ b/drivers/staging/rtl8188eu/core/rtw_recv.c
@@ -952,8 +952,7 @@ static int validate_recv_mgnt_frame(struct adapter
*padapter,
if (!memcmp(padapter->eeprompriv.mac_addr,
GetAddr1Ptr(precv_frame->pkt->data),
ETH_ALEN))
psta->sta_stats.rx_probersp_pkts++;
- else if
(is_broadcast_mac_addr(GetAddr1Ptr(precv_frame->pkt->data)) ||
-
is_multicast_mac_addr(GetAddr1Ptr(precv_frame->pkt->data)))
+ else if
(is_multicast_ether_addr(GetAddr1Ptr(precv_frame->pkt->data)))
psta->sta_stats.rx_probersp_bm_pkts++;
else
psta->sta_stats.rx_probersp_uo_pkts++;
diff --git a/drivers/staging/rtl8188eu/include/ieee80211.h
b/drivers/staging/rtl8188eu/include/ieee80211.h
index 1668bb531ead..3fbd92a95b17 100644
--- a/drivers/staging/rtl8188eu/include/ieee80211.h
+++ b/drivers/staging/rtl8188eu/include/ieee80211.h
@@ -515,17 +515,6 @@ enum ieee80211_state {
#define DEFAULT_MAX_SCAN_AGE (15 * HZ)
#define DEFAULT_FTS 2346
-static inline int is_multicast_mac_addr(const u8 *addr)
-{
- return ((addr[0] != 0xff) && (0x01 & addr[0]));
-}
-
-static inline int is_broadcast_mac_addr(const u8 *addr)
-{
- return (addr[0] == 0xff) && (addr[1] == 0xff) && (addr[2] == 0xff) &&
- (addr[3] == 0xff) && (addr[4] == 0xff) && (addr[5] == 0xff);
-}
-
#define CFG_IEEE80211_RESERVE_FCS BIT(0)
#define CFG_IEEE80211_COMPUTE_FCS BIT(1)
--
2.18.0
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On Thu, Jul 12, 2018 at 11:21 AM Jae Hyun Yoo
wrote:
>
> On 7/12/2018 2:33 AM, Brendan Higgins wrote:
> > On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
> > wrote:
> >>
> >
> +/* Timeout for bus busy checking */
> +#define BUS_BUSY_CHECK_TIMEOUT 25 /* 250ms
> */
> +#define BUS_BUSY_CHECK_INTERVAL1
> /* 10ms */
> >>>
> >>> Could you add a comment on where you got these values from?
> >>>
> >>
> >> These are coming from ASPEED SDK code. Actually, they use 100ms for
> >> timeout and 10ms for interval but I increased the timeout value to
> >> 250ms so that it covers a various range of bus speed. I think, it
> >> should be computed at run time based on the current bus speed, or
> >> we could add these as device tree settings. How do you think about it?
> >>
> >
> > This should definitely be a device tree setting. If one of the busses is
> > being
> > used as a regular I2C bus, it could hold the bus for an unlimited amount of
> > time before sending a STOP. As for a default, 100ms is probably fine given
> > that, a) the limit will only apply to multi-master mode, and b) multi-master
> > mode will probably almost always be used with IPMB, or MCTP (MCTP actually
> > recommends a 100ms timeout for this purpose, see
> > https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
> > symbol PT2a). That being said, if you actually want to implement IPMB, or
> > MCTP
> > arbitration logic, it is much more complicated.
> >
>
> Okay then, I think, we can fix the timeout value to 100ms and enable the
> bus busy checking logic only when 'multi-master' is set in device tree.
> My thought is, no additional arbitration logic is needed because
> arbitration is performed in H/W level and H/W reports
> ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
> ARBIT_LOSS event is already being handled well by this driver code you
> implemented.
I still think it would be best to provide an option to specify the timeout value
it in the device tree regardless of master mode or not. Also, I am talking about
fairness arbitration not the physical level arbitration provided by the I2C
spec. The physical arbitration that Aspeed provides just allows multiple masters
to operate on the same bus according to the specification; this bus arbitration
does not guarantee forward progress or even guarentee that the actual message
will be sent, which is what you are trying to do here.
Since you are planning on implementing IPMB, you will probably need to implement
fairness arbitration. I am not familiar with your BMC-ME channel. It sounds like
it pre-dates MCTP, so it must implement its own fairness arbitration on top of
the IPMB layer (more like you bake in some assumptions about what the possible
state is at anytime that guarentee fairness).
Are you using the Aspeed BMC on both sides of the connection? If so, you might
be further ahead to implement MCTP fairness arbitration which can be used in
conjunction with IPMB. This will require a bit of work to do, but everyone will
be much happier in the long term (assuming MCTP does eventually become a thing).
>
> >> >
> >
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - return IRQ_HANDLED;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> >>>
> >>> Why do you check the slave if master fails (or vice versa)? I
> >>> understand that there are some status bits that have not been handled,
> >>> but it doesn't seem reasonable to assume that there is state that the
> >>> other should do something with; the only way this would happen is if
> >>> the state that you think you are in does not match the status bits you
> >>> have been given, but if this is the case, you are already hosed; I
> >>> don't think trying the other handler is likely to make things better,
> >>> unless there is something that I am missing.
> >>>
> >>
> >> In most of cases, interrupt bits are set one by one but there are also a
> >> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
> >> with combining master and slave events using a single interrupt call. It
> >> happens much in multi-master environment than single-master. For
> >> example, when master is waiting for a NORMAL_STOP interrupt in its
> >> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
> >> with the NORMAL_STOP in case of an another master immediately sends data
> >> just after acquiring the bus - it happens a lot in BMC-ME connection
> >> practically. In this case, the NORMAL_STOP interrupt should be handled
> >> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> >> handled by slave_irq so it's the reason why this code is added.
> >
> > That sucks. Well, it sounds like there are
Re: [PATCH] i2c: aspeed: Improve driver to support multi-master use cases stably
On Thu, Jul 12, 2018 at 11:21 AM Jae Hyun Yoo
wrote:
>
> On 7/12/2018 2:33 AM, Brendan Higgins wrote:
> > On Wed, Jun 27, 2018 at 10:55 AM Jae Hyun Yoo
> > wrote:
> >>
> >
> +/* Timeout for bus busy checking */
> +#define BUS_BUSY_CHECK_TIMEOUT 25 /* 250ms
> */
> +#define BUS_BUSY_CHECK_INTERVAL1
> /* 10ms */
> >>>
> >>> Could you add a comment on where you got these values from?
> >>>
> >>
> >> These are coming from ASPEED SDK code. Actually, they use 100ms for
> >> timeout and 10ms for interval but I increased the timeout value to
> >> 250ms so that it covers a various range of bus speed. I think, it
> >> should be computed at run time based on the current bus speed, or
> >> we could add these as device tree settings. How do you think about it?
> >>
> >
> > This should definitely be a device tree setting. If one of the busses is
> > being
> > used as a regular I2C bus, it could hold the bus for an unlimited amount of
> > time before sending a STOP. As for a default, 100ms is probably fine given
> > that, a) the limit will only apply to multi-master mode, and b) multi-master
> > mode will probably almost always be used with IPMB, or MCTP (MCTP actually
> > recommends a 100ms timeout for this purpose, see
> > https://www.dmtf.org/sites/default/files/standards/documents/DSP0237_1.1.0.pdf,
> > symbol PT2a). That being said, if you actually want to implement IPMB, or
> > MCTP
> > arbitration logic, it is much more complicated.
> >
>
> Okay then, I think, we can fix the timeout value to 100ms and enable the
> bus busy checking logic only when 'multi-master' is set in device tree.
> My thought is, no additional arbitration logic is needed because
> arbitration is performed in H/W level and H/W reports
> ASPEED_I2CD_INTR_ARBIT_LOSS when it fails acquiring a bus. The
> ARBIT_LOSS event is already being handled well by this driver code you
> implemented.
I still think it would be best to provide an option to specify the timeout value
it in the device tree regardless of master mode or not. Also, I am talking about
fairness arbitration not the physical level arbitration provided by the I2C
spec. The physical arbitration that Aspeed provides just allows multiple masters
to operate on the same bus according to the specification; this bus arbitration
does not guarantee forward progress or even guarentee that the actual message
will be sent, which is what you are trying to do here.
Since you are planning on implementing IPMB, you will probably need to implement
fairness arbitration. I am not familiar with your BMC-ME channel. It sounds like
it pre-dates MCTP, so it must implement its own fairness arbitration on top of
the IPMB layer (more like you bake in some assumptions about what the possible
state is at anytime that guarentee fairness).
Are you using the Aspeed BMC on both sides of the connection? If so, you might
be further ahead to implement MCTP fairness arbitration which can be used in
conjunction with IPMB. This will require a bit of work to do, but everyone will
be much happier in the long term (assuming MCTP does eventually become a thing).
>
> >> >
> >
> #if IS_ENABLED(CONFIG_I2C_SLAVE)
> - if (aspeed_i2c_slave_irq(bus)) {
> - dev_dbg(bus->dev, "irq handled by slave.\n");
> - return IRQ_HANDLED;
> + if (bus->master_state != ASPEED_I2C_MASTER_INACTIVE) {
> + if (!aspeed_i2c_master_irq(bus))
> >>>
> >>> Why do you check the slave if master fails (or vice versa)? I
> >>> understand that there are some status bits that have not been handled,
> >>> but it doesn't seem reasonable to assume that there is state that the
> >>> other should do something with; the only way this would happen is if
> >>> the state that you think you are in does not match the status bits you
> >>> have been given, but if this is the case, you are already hosed; I
> >>> don't think trying the other handler is likely to make things better,
> >>> unless there is something that I am missing.
> >>>
> >>
> >> In most of cases, interrupt bits are set one by one but there are also a
> >> lot of other cases that ASPEED I2C H/W sends multiple interrupt bits
> >> with combining master and slave events using a single interrupt call. It
> >> happens much in multi-master environment than single-master. For
> >> example, when master is waiting for a NORMAL_STOP interrupt in its
> >> MASTER_STOP state, SLAVE_MATCH and RX_DONE interrupts could come along
> >> with the NORMAL_STOP in case of an another master immediately sends data
> >> just after acquiring the bus - it happens a lot in BMC-ME connection
> >> practically. In this case, the NORMAL_STOP interrupt should be handled
> >> by master_irq and the SLAVE_MATCH and RX_DONE interrupts should be
> >> handled by slave_irq so it's the reason why this code is added.
> >
> > That sucks. Well, it sounds like there are
Re: [PATCH 3/3] ARM: dts: imx6sll-evk: enable SEIKO 43WVF1G lcdif panel
On Fri, Jul 13, 2018 at 4:58 AM, Anson Huang wrote: > Enable SEIKO 43WVF1G lcdif panel for DRM driver, > add necessary properties according to SEIKO 43WVF1G > driver's requirement, such as "dvdd-supply", "avdd-supply" > and "backlight" etc.. > > Signed-off-by: Anson Huang Reviewed-by: Fabio Estevam
Re: [PATCH 3/3] ARM: dts: imx6sll-evk: enable SEIKO 43WVF1G lcdif panel
On Fri, Jul 13, 2018 at 4:58 AM, Anson Huang wrote: > Enable SEIKO 43WVF1G lcdif panel for DRM driver, > add necessary properties according to SEIKO 43WVF1G > driver's requirement, such as "dvdd-supply", "avdd-supply" > and "backlight" etc.. > > Signed-off-by: Anson Huang Reviewed-by: Fabio Estevam
Re: [PATCH 2/3] ARM: dts: imx6sll-evk: correct lcd regulator GPIO pin
On Fri, Jul 13, 2018 at 4:58 AM, Anson Huang wrote: > On i.MX6SLL EVK board, lcd regulator is controlled > by GPIO4 IO03 using MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 pin, > NOT MX6SLL_PAD_ECSPI1_SCLK__GPIO4_IO08, correct it. > > Signed-off-by: Anson Huang Reviewed-by: Fabio Estevam
Re: [PATCH 2/3] ARM: dts: imx6sll-evk: correct lcd regulator GPIO pin
On Fri, Jul 13, 2018 at 4:58 AM, Anson Huang wrote: > On i.MX6SLL EVK board, lcd regulator is controlled > by GPIO4 IO03 using MX6SLL_PAD_KEY_ROW5__GPIO4_IO03 pin, > NOT MX6SLL_PAD_ECSPI1_SCLK__GPIO4_IO08, correct it. > > Signed-off-by: Anson Huang Reviewed-by: Fabio Estevam
Re: [PATCH] ARM: dts: imx6sl-evk: add missing GPIO iomux setting
Hi Anson,
On Fri, Jul 13, 2018 at 4:55 AM, Anson Huang wrote:
> On i.MX6SL EVK board, the MX6SL_PAD_KEY_ROW5 pin is
> used as lcd 3v3 regulator control pin, need to make
> sure MX6SL_PAD_KEY_ROW5 is muxed as GPIO function
> for controlling lcd 3v3 regulator.
>
> Signed-off-by: Anson Huang
> ---
> arch/arm/boot/dts/imx6sl-evk.dts | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/imx6sl-evk.dts
> b/arch/arm/boot/dts/imx6sl-evk.dts
> index 50bdc65..1294fba 100644
> --- a/arch/arm/boot/dts/imx6sl-evk.dts
> +++ b/arch/arm/boot/dts/imx6sl-evk.dts
> @@ -283,6 +283,7 @@
> imx6sl-evk {
> pinctrl_hog: hoggrp {
> fsl,pins = <
> + MX6SL_PAD_KEY_ROW5__GPIO4_IO030x17059
You could add a specific group for it instead of putting it inside the
hog group.
Re: [PATCH] ARM: dts: imx6sl-evk: add missing GPIO iomux setting
Hi Anson,
On Fri, Jul 13, 2018 at 4:55 AM, Anson Huang wrote:
> On i.MX6SL EVK board, the MX6SL_PAD_KEY_ROW5 pin is
> used as lcd 3v3 regulator control pin, need to make
> sure MX6SL_PAD_KEY_ROW5 is muxed as GPIO function
> for controlling lcd 3v3 regulator.
>
> Signed-off-by: Anson Huang
> ---
> arch/arm/boot/dts/imx6sl-evk.dts | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/imx6sl-evk.dts
> b/arch/arm/boot/dts/imx6sl-evk.dts
> index 50bdc65..1294fba 100644
> --- a/arch/arm/boot/dts/imx6sl-evk.dts
> +++ b/arch/arm/boot/dts/imx6sl-evk.dts
> @@ -283,6 +283,7 @@
> imx6sl-evk {
> pinctrl_hog: hoggrp {
> fsl,pins = <
> + MX6SL_PAD_KEY_ROW5__GPIO4_IO030x17059
You could add a specific group for it instead of putting it inside the
hog group.
Re: [PATCH 1/3] ARM: dts: imx6sll-evk: enable PWM1 for backlight driver
On Fri, Jul 13, 2018 at 4:58 AM, Anson Huang wrote: > Enable pwm1 module on i.MX6SLL EVK board to make > backlight driver really work with LCD panel connected. > > Signed-off-by: Anson Huang Reviewed-by: Fabio Estevam
Re: [PATCH 1/3] ARM: dts: imx6sll-evk: enable PWM1 for backlight driver
On Fri, Jul 13, 2018 at 4:58 AM, Anson Huang wrote: > Enable pwm1 module on i.MX6SLL EVK board to make > backlight driver really work with LCD panel connected. > > Signed-off-by: Anson Huang Reviewed-by: Fabio Estevam
Re: [PATCH] gpio: mxc: add power management support
On Fri, Jul 13, 2018 at 4:11 AM, Linus Walleij wrote: > On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > >> GPIO registers could lose context on some i.MX SoCs, >> like on i.MX7D, when enter LPSR mode, the whole SoC >> will be powered off except LPSR domain, GPIO banks >> will lose context in this case, need to restore >> the context after resume from LPSR mode. >> >> This patch adds GPIO save/restore for those necessary >> registers, and put the save/restore operations in noirq >> suspend/resume phase, since GPIO is fundamental module >> which could be used by other peripherals' resume phase. >> >> Signed-off-by: Anson Huang > > Hoping for some review on this before applying... > Fabio? Bartosz? Now I could find the patch on my Gmail Inbox :-) It looks good to me: Reviewed-by: Fabio Estevam
Re: [PATCH] gpio: mxc: add power management support
On Fri, Jul 13, 2018 at 4:11 AM, Linus Walleij wrote: > On Tue, Jul 10, 2018 at 9:53 AM Anson Huang wrote: > >> GPIO registers could lose context on some i.MX SoCs, >> like on i.MX7D, when enter LPSR mode, the whole SoC >> will be powered off except LPSR domain, GPIO banks >> will lose context in this case, need to restore >> the context after resume from LPSR mode. >> >> This patch adds GPIO save/restore for those necessary >> registers, and put the save/restore operations in noirq >> suspend/resume phase, since GPIO is fundamental module >> which could be used by other peripherals' resume phase. >> >> Signed-off-by: Anson Huang > > Hoping for some review on this before applying... > Fabio? Bartosz? Now I could find the patch on my Gmail Inbox :-) It looks good to me: Reviewed-by: Fabio Estevam
Re: [PATCH v2 2/2] ARM: dts: imx51-zii-scu3-esb: Fix RAVE SP watchdog compatible string
On Fri, Jul 13, 2018 at 2:30 PM, Andrey Smirnov wrote: > It looks like I made a nasty typo in the original patch which resulted > in missing watchdog device. Fix it. > > Cc: Fabio Estevam > Cc: Nikita Yushchenko > Cc: Lucas Stach > Cc: cphe...@gmail.com > Cc: Shawn Guo > Cc: Rob Herring > Cc: Mark Rutland > Cc: linux-arm-ker...@lists.infradead.org > Cc: devicet...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Andrew Lunn > Signed-off-by: Andrey Smirnov Reviewed-by: Fabio Estevam
Re: [PATCH v2 2/2] ARM: dts: imx51-zii-scu3-esb: Fix RAVE SP watchdog compatible string
On Fri, Jul 13, 2018 at 2:30 PM, Andrey Smirnov wrote: > It looks like I made a nasty typo in the original patch which resulted > in missing watchdog device. Fix it. > > Cc: Fabio Estevam > Cc: Nikita Yushchenko > Cc: Lucas Stach > Cc: cphe...@gmail.com > Cc: Shawn Guo > Cc: Rob Herring > Cc: Mark Rutland > Cc: linux-arm-ker...@lists.infradead.org > Cc: devicet...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Andrew Lunn > Signed-off-by: Andrey Smirnov Reviewed-by: Fabio Estevam
Re: [PATCH v2 1/2] ARM: dts: imx51-zii-scu3-esb: Add switch IRQ line pinumx config
On Fri, Jul 13, 2018 at 2:30 PM, Andrey Smirnov wrote: > Instead of relying on default values, configure PAD_AUD3_BB_CK to be a > GPIO explicitly. While at, it change the pad configuration to enable > a 100K pull-down (the pin is used as IRQ_TYPE_LEVEL_HIGH). > > Cc: Fabio Estevam > Cc: Nikita Yushchenko > Cc: Lucas Stach > Cc: cphe...@gmail.com > Cc: Shawn Guo > Cc: Rob Herring > Cc: Mark Rutland > Cc: linux-arm-ker...@lists.infradead.org > Cc: devicet...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Andrew Lunn > Reviewed-by: Andrew Lunn > Signed-off-by: Andrey Smirnov Reviewed-by: Fabio Estevam
[PATCH RT] locallock: add local_lock_bh()
For the ARM64 simd locking it would be easier to have local_lock_bh()
which grabs a local_lock with BH disabled and turns into a
local_bh_disable() on !RT.
Signed-off-by: Sebastian Andrzej Siewior
---
obviously required by the previous one…
include/linux/locallock.h | 33 +
1 file changed, 33 insertions(+)
diff --git a/include/linux/locallock.h b/include/linux/locallock.h
index 921eab83cd34..15aa0dea2bfb 100644
--- a/include/linux/locallock.h
+++ b/include/linux/locallock.h
@@ -47,9 +47,23 @@ static inline void __local_lock(struct local_irq_lock *lv)
lv->nestcnt++;
}
+static inline void __local_lock_bh(struct local_irq_lock *lv)
+{
+ if (lv->owner != current) {
+ spin_lock_bh(>lock);
+ LL_WARN(lv->owner);
+ LL_WARN(lv->nestcnt);
+ lv->owner = current;
+ }
+ lv->nestcnt++;
+}
+
#define local_lock(lvar) \
do { __local_lock(_local_var(lvar)); } while (0)
+#define local_lock_bh(lvar)\
+ do { __local_lock_bh(_local_var(lvar)); } while (0)
+
#define local_lock_on(lvar, cpu) \
do { __local_lock(_cpu(lvar, cpu)); } while (0)
@@ -88,12 +102,29 @@ static inline void __local_unlock(struct local_irq_lock
*lv)
spin_unlock(>lock);
}
+static inline void __local_unlock_bh(struct local_irq_lock *lv)
+{
+ LL_WARN(lv->nestcnt == 0);
+ LL_WARN(lv->owner != current);
+ if (--lv->nestcnt)
+ return;
+
+ lv->owner = NULL;
+ spin_unlock_bh(>lock);
+}
+
#define local_unlock(lvar) \
do {\
__local_unlock(this_cpu_ptr());\
put_local_var(lvar);\
} while (0)
+#define local_unlock_bh(lvar) \
+ do {\
+ __local_unlock_bh(this_cpu_ptr()); \
+ put_local_var(lvar);\
+ } while (0)
+
#define local_unlock_on(lvar, cpu) \
do { __local_unlock(_cpu(lvar, cpu)); } while (0)
@@ -253,6 +284,8 @@ static inline void local_irq_lock_init(int lvar) { }
#define local_lock(lvar) preempt_disable()
#define local_unlock(lvar) preempt_enable()
+#define local_lock_bh(lvar)local_bh_disable()
+#define local_unlock_bh(lvar) local_bh_enable()
#define local_lock_irq(lvar) local_irq_disable()
#define local_lock_irq_on(lvar, cpu) local_irq_disable()
#define local_unlock_irq(lvar) local_irq_enable()
--
2.18.0
Re: [PATCH v2 1/2] ARM: dts: imx51-zii-scu3-esb: Add switch IRQ line pinumx config
On Fri, Jul 13, 2018 at 2:30 PM, Andrey Smirnov wrote: > Instead of relying on default values, configure PAD_AUD3_BB_CK to be a > GPIO explicitly. While at, it change the pad configuration to enable > a 100K pull-down (the pin is used as IRQ_TYPE_LEVEL_HIGH). > > Cc: Fabio Estevam > Cc: Nikita Yushchenko > Cc: Lucas Stach > Cc: cphe...@gmail.com > Cc: Shawn Guo > Cc: Rob Herring > Cc: Mark Rutland > Cc: linux-arm-ker...@lists.infradead.org > Cc: devicet...@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: Andrew Lunn > Reviewed-by: Andrew Lunn > Signed-off-by: Andrey Smirnov Reviewed-by: Fabio Estevam
[PATCH RT] locallock: add local_lock_bh()
For the ARM64 simd locking it would be easier to have local_lock_bh()
which grabs a local_lock with BH disabled and turns into a
local_bh_disable() on !RT.
Signed-off-by: Sebastian Andrzej Siewior
---
obviously required by the previous one…
include/linux/locallock.h | 33 +
1 file changed, 33 insertions(+)
diff --git a/include/linux/locallock.h b/include/linux/locallock.h
index 921eab83cd34..15aa0dea2bfb 100644
--- a/include/linux/locallock.h
+++ b/include/linux/locallock.h
@@ -47,9 +47,23 @@ static inline void __local_lock(struct local_irq_lock *lv)
lv->nestcnt++;
}
+static inline void __local_lock_bh(struct local_irq_lock *lv)
+{
+ if (lv->owner != current) {
+ spin_lock_bh(>lock);
+ LL_WARN(lv->owner);
+ LL_WARN(lv->nestcnt);
+ lv->owner = current;
+ }
+ lv->nestcnt++;
+}
+
#define local_lock(lvar) \
do { __local_lock(_local_var(lvar)); } while (0)
+#define local_lock_bh(lvar)\
+ do { __local_lock_bh(_local_var(lvar)); } while (0)
+
#define local_lock_on(lvar, cpu) \
do { __local_lock(_cpu(lvar, cpu)); } while (0)
@@ -88,12 +102,29 @@ static inline void __local_unlock(struct local_irq_lock
*lv)
spin_unlock(>lock);
}
+static inline void __local_unlock_bh(struct local_irq_lock *lv)
+{
+ LL_WARN(lv->nestcnt == 0);
+ LL_WARN(lv->owner != current);
+ if (--lv->nestcnt)
+ return;
+
+ lv->owner = NULL;
+ spin_unlock_bh(>lock);
+}
+
#define local_unlock(lvar) \
do {\
__local_unlock(this_cpu_ptr());\
put_local_var(lvar);\
} while (0)
+#define local_unlock_bh(lvar) \
+ do {\
+ __local_unlock_bh(this_cpu_ptr()); \
+ put_local_var(lvar);\
+ } while (0)
+
#define local_unlock_on(lvar, cpu) \
do { __local_unlock(_cpu(lvar, cpu)); } while (0)
@@ -253,6 +284,8 @@ static inline void local_irq_lock_init(int lvar) { }
#define local_lock(lvar) preempt_disable()
#define local_unlock(lvar) preempt_enable()
+#define local_lock_bh(lvar)local_bh_disable()
+#define local_unlock_bh(lvar) local_bh_enable()
#define local_lock_irq(lvar) local_irq_disable()
#define local_lock_irq_on(lvar, cpu) local_irq_disable()
#define local_unlock_irq(lvar) local_irq_enable()
--
2.18.0
[PATCH RT v2] arm64: fpsimd: use a local_lock() in addition to local_bh_disable()
In v4.16-RT I noticed a number of warnings from task_fpsimd_load(). The
code disables BH and expects that it is not preemptible. On -RT the
task remains preemptible but remains the same CPU. This may corrupt the
content of the SIMD registers if the task is preempted during
saving/restoring those registers.
Add a locallock around this process. This avoids that the any function
within the locallock block is invoked more than once on the same CPU.
The kernel_neon_begin() can't be kept preemptible. If the task-switch notices
TIF_FOREIGN_FPSTATE then it would restore task's SIMD state and we lose the
state of registers used for in-kernel-work. We would require additional storage
for the in-kernel copy of the registers. But then the NEON-crypto checks for
the need-resched flag so it shouldn't that bad.
The preempt_disable() avoids the context switch while the kernel uses the SIMD
registers. Unfortunately we have to balance out the migrate_disable() counter
because local_lock_bh() is invoked in different context compared to its unlock
counterpart.
__efi_fpsimd_begin() should not use kernel_fpu_begin() due to its
preempt_disable() context and instead save the registers always in its
extra spot on RT.
Signed-off-by: Sebastian Andrzej Siewior
---
This seems to make work (crypto chacha20-neon + cyclictest). I have no
EFI so I have no clue if saving SIMD while calling to EFI works.
arch/arm64/kernel/fpsimd.c | 47 ++---
1 file changed, 28 insertions(+), 19 deletions(-)
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include
#include
@@ -235,7 +236,7 @@ static void sve_user_enable(void)
*whether TIF_SVE is clear or set, since these are not vector length
*dependent.
*/
-
+static DEFINE_LOCAL_IRQ_LOCK(fpsimd_lock);
/*
* Update current's FPSIMD/SVE registers from thread_struct.
*
@@ -594,7 +595,7 @@ int sve_set_vector_length(struct task_st
* non-SVE thread.
*/
if (task == current) {
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
task_fpsimd_save();
set_thread_flag(TIF_FOREIGN_FPSTATE);
@@ -605,7 +606,7 @@ int sve_set_vector_length(struct task_st
sve_to_fpsimd(task);
if (task == current)
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
/*
* Force reallocation of task SVE state to the correct size
@@ -837,7 +838,7 @@ asmlinkage void do_sve_acc(unsigned int
sve_alloc(current);
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
task_fpsimd_save();
fpsimd_to_sve(current);
@@ -849,7 +850,7 @@ asmlinkage void do_sve_acc(unsigned int
if (test_and_set_thread_flag(TIF_SVE))
WARN_ON(1); /* SVE access shouldn't have trapped */
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -925,7 +926,7 @@ void fpsimd_flush_thread(void)
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
memset(>thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
fpsimd_flush_task_state(current);
@@ -967,7 +968,7 @@ void fpsimd_flush_thread(void)
set_thread_flag(TIF_FOREIGN_FPSTATE);
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -979,9 +980,9 @@ void fpsimd_preserve_current_state(void)
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
task_fpsimd_save();
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -1021,14 +1022,14 @@ void fpsimd_restore_current_state(void)
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
task_fpsimd_load();
fpsimd_bind_to_cpu();
}
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -1041,7 +1042,7 @@ void fpsimd_update_current_state(struct
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
current->thread.fpsimd_state.user_fpsimd = *state;
if (system_supports_sve() && test_thread_flag(TIF_SVE))
@@ -1052,7 +1053,7 @@ void fpsimd_update_current_state(struct
if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE))
fpsimd_bind_to_cpu();
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -1115,7 +1116,7 @@ void kernel_neon_begin(void)
BUG_ON(!may_use_simd());
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
__this_cpu_write(kernel_neon_busy, true);
@@ -1129,8 +1130,14 @@ void
[PATCH RT v2] arm64: fpsimd: use a local_lock() in addition to local_bh_disable()
In v4.16-RT I noticed a number of warnings from task_fpsimd_load(). The
code disables BH and expects that it is not preemptible. On -RT the
task remains preemptible but remains the same CPU. This may corrupt the
content of the SIMD registers if the task is preempted during
saving/restoring those registers.
Add a locallock around this process. This avoids that the any function
within the locallock block is invoked more than once on the same CPU.
The kernel_neon_begin() can't be kept preemptible. If the task-switch notices
TIF_FOREIGN_FPSTATE then it would restore task's SIMD state and we lose the
state of registers used for in-kernel-work. We would require additional storage
for the in-kernel copy of the registers. But then the NEON-crypto checks for
the need-resched flag so it shouldn't that bad.
The preempt_disable() avoids the context switch while the kernel uses the SIMD
registers. Unfortunately we have to balance out the migrate_disable() counter
because local_lock_bh() is invoked in different context compared to its unlock
counterpart.
__efi_fpsimd_begin() should not use kernel_fpu_begin() due to its
preempt_disable() context and instead save the registers always in its
extra spot on RT.
Signed-off-by: Sebastian Andrzej Siewior
---
This seems to make work (crypto chacha20-neon + cyclictest). I have no
EFI so I have no clue if saving SIMD while calling to EFI works.
arch/arm64/kernel/fpsimd.c | 47 ++---
1 file changed, 28 insertions(+), 19 deletions(-)
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include
#include
@@ -235,7 +236,7 @@ static void sve_user_enable(void)
*whether TIF_SVE is clear or set, since these are not vector length
*dependent.
*/
-
+static DEFINE_LOCAL_IRQ_LOCK(fpsimd_lock);
/*
* Update current's FPSIMD/SVE registers from thread_struct.
*
@@ -594,7 +595,7 @@ int sve_set_vector_length(struct task_st
* non-SVE thread.
*/
if (task == current) {
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
task_fpsimd_save();
set_thread_flag(TIF_FOREIGN_FPSTATE);
@@ -605,7 +606,7 @@ int sve_set_vector_length(struct task_st
sve_to_fpsimd(task);
if (task == current)
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
/*
* Force reallocation of task SVE state to the correct size
@@ -837,7 +838,7 @@ asmlinkage void do_sve_acc(unsigned int
sve_alloc(current);
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
task_fpsimd_save();
fpsimd_to_sve(current);
@@ -849,7 +850,7 @@ asmlinkage void do_sve_acc(unsigned int
if (test_and_set_thread_flag(TIF_SVE))
WARN_ON(1); /* SVE access shouldn't have trapped */
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -925,7 +926,7 @@ void fpsimd_flush_thread(void)
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
memset(>thread.fpsimd_state, 0, sizeof(struct fpsimd_state));
fpsimd_flush_task_state(current);
@@ -967,7 +968,7 @@ void fpsimd_flush_thread(void)
set_thread_flag(TIF_FOREIGN_FPSTATE);
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -979,9 +980,9 @@ void fpsimd_preserve_current_state(void)
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
task_fpsimd_save();
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -1021,14 +1022,14 @@ void fpsimd_restore_current_state(void)
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
task_fpsimd_load();
fpsimd_bind_to_cpu();
}
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -1041,7 +1042,7 @@ void fpsimd_update_current_state(struct
if (!system_supports_fpsimd())
return;
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
current->thread.fpsimd_state.user_fpsimd = *state;
if (system_supports_sve() && test_thread_flag(TIF_SVE))
@@ -1052,7 +1053,7 @@ void fpsimd_update_current_state(struct
if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE))
fpsimd_bind_to_cpu();
- local_bh_enable();
+ local_unlock_bh(fpsimd_lock);
}
/*
@@ -1115,7 +1116,7 @@ void kernel_neon_begin(void)
BUG_ON(!may_use_simd());
- local_bh_disable();
+ local_lock_bh(fpsimd_lock);
__this_cpu_write(kernel_neon_busy, true);
@@ -1129,8 +1130,14 @@ void
[PATCH] .gitignore: add ZSTD-compressed files
For now, that's arch/x86/boot/compressed/vmlinux.bin.zst but probably more will come, thus let's be consistent with all other compressors. Signed-off-by: Adam Borowski --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 97ba6b79834c..0d09cf1c053c 100644 --- a/.gitignore +++ b/.gitignore @@ -42,6 +42,7 @@ *.tab.[ch] *.tar *.xz +*.zst Module.symvers modules.builtin -- 2.18.0
[PATCH] .gitignore: add ZSTD-compressed files
For now, that's arch/x86/boot/compressed/vmlinux.bin.zst but probably more will come, thus let's be consistent with all other compressors. Signed-off-by: Adam Borowski --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index 97ba6b79834c..0d09cf1c053c 100644 --- a/.gitignore +++ b/.gitignore @@ -42,6 +42,7 @@ *.tab.[ch] *.tar *.xz +*.zst Module.symvers modules.builtin -- 2.18.0
[PATCH v2 0/2] Follow up fixes for SCU3 ESB
Shawn: Here's a couple of fixes for things I missed in the [original-submission]. Sorry about the inconvenience. Changes since [v1]: - Reword commit message for patch 2/2 Thanks, Andrey Smirnov [v1] lkml.kernel.org/r/20180712023337.30112-1-andrew.smir...@gmail.com [original-submission] lkml.kernel.org/r/20180711050704.11492-1-andrew.smir...@gmail.com Andrey Smirnov (2): ARM: dts: imx51-zii-scu3-esb: Add switch IRQ line pinumx config ARM: dts: imx51-zii-scu3-esb: Fix RAVE SP watchdog compatible string arch/arm/boot/dts/imx51-zii-scu3-esb.dts | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) -- 2.17.1
[PATCH v2 0/2] Follow up fixes for SCU3 ESB
Shawn: Here's a couple of fixes for things I missed in the [original-submission]. Sorry about the inconvenience. Changes since [v1]: - Reword commit message for patch 2/2 Thanks, Andrey Smirnov [v1] lkml.kernel.org/r/20180712023337.30112-1-andrew.smir...@gmail.com [original-submission] lkml.kernel.org/r/20180711050704.11492-1-andrew.smir...@gmail.com Andrey Smirnov (2): ARM: dts: imx51-zii-scu3-esb: Add switch IRQ line pinumx config ARM: dts: imx51-zii-scu3-esb: Fix RAVE SP watchdog compatible string arch/arm/boot/dts/imx51-zii-scu3-esb.dts | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) -- 2.17.1
