* [PATCH RFC bpf-next v2 07/10] HID: bpf: allow to inject HID event from BPF
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Benjamin Tissoires
In-Reply-To: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
It can be interesting to inject events from BPF as if the event were
to come from the device.
For example, some multitouch devices do not all the time send a proximity
out event, and we might want to send it for the physical device.
Compared to uhid, we can now inject events on any physical device, not
just uhid virtual ones.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
Documentation/hid/hid-bpf.rst | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 29 +++++++++++++++++++++++++++++
drivers/hid/hid-core.c | 1 +
include/linux/hid_bpf.h | 2 ++
4 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/Documentation/hid/hid-bpf.rst b/Documentation/hid/hid-bpf.rst
index a575004d9025..0765b3298ecf 100644
--- a/Documentation/hid/hid-bpf.rst
+++ b/Documentation/hid/hid-bpf.rst
@@ -179,7 +179,7 @@ Available API that can be used in syscall HID-BPF programs:
-----------------------------------------------------------
.. kernel-doc:: drivers/hid/bpf/hid_bpf_dispatch.c
- :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_allocate_context hid_bpf_release_context
+ :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_input_report hid_bpf_allocate_context hid_bpf_release_context
General overview of a HID-BPF program
=====================================
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index a5b88b491b80..e1a650f4a626 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -508,6 +508,34 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
kfree(dma_data);
return ret;
}
+
+/**
+ * hid_bpf_input_report - Inject a HID report in the kernel from a HID device
+ *
+ * @ctx: the HID-BPF context previously allocated in hid_bpf_allocate_context()
+ * @type: the type of the report (%HID_INPUT_REPORT, %HID_FEATURE_REPORT, %HID_OUTPUT_REPORT)
+ * @buf: a %PTR_TO_MEM buffer
+ * @buf__sz: the size of the data to transfer
+ *
+ * @returns %0 on success, a negative error code otherwise.
+ */
+__bpf_kfunc int
+hid_bpf_input_report(struct hid_bpf_ctx *ctx, enum hid_report_type type, u8 *buf,
+ const size_t buf__sz)
+{
+ struct hid_device *hdev;
+ size_t size = buf__sz;
+ int ret;
+
+ /* check arguments */
+ ret = __hid_bpf_hw_check_params(ctx, buf, &size, type);
+ if (ret)
+ return ret;
+
+ hdev = (struct hid_device *)ctx->hid; /* discard const */
+
+ return hid_input_report(hdev, type, buf, size, 0);
+}
__bpf_kfunc_end_defs();
/*
@@ -542,6 +570,7 @@ BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE)
BTF_ID_FLAGS(func, hid_bpf_hw_request)
BTF_ID_FLAGS(func, hid_bpf_hw_output_report)
+BTF_ID_FLAGS(func, hid_bpf_input_report)
BTF_KFUNCS_END(hid_bpf_syscall_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 1243595890ba..b1fa0378e8f4 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2975,6 +2975,7 @@ static struct hid_bpf_ops hid_ops = {
.hid_get_report = hid_get_report,
.hid_hw_raw_request = hid_hw_raw_request,
.hid_hw_output_report = hid_hw_output_report,
+ .hid_input_report = hid_input_report,
.owner = THIS_MODULE,
.bus_type = &hid_bus_type,
};
diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
index 5c7ff93dc73e..17b08f500098 100644
--- a/include/linux/hid_bpf.h
+++ b/include/linux/hid_bpf.h
@@ -104,6 +104,8 @@ struct hid_bpf_ops {
size_t len, enum hid_report_type rtype,
enum hid_class_request reqtype);
int (*hid_hw_output_report)(struct hid_device *hdev, __u8 *buf, size_t len);
+ int (*hid_input_report)(struct hid_device *hid, enum hid_report_type type,
+ u8 *data, u32 size, int interrupt);
struct module *owner;
const struct bus_type *bus_type;
};
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 08/10] selftests/hid: add tests for hid_bpf_input_report
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Benjamin Tissoires
In-Reply-To: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
Usual way of testing, we call the function and ensures we receive
the event
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/hid_bpf.c | 49 +++++++++++++++++++++-
tools/testing/selftests/hid/progs/hid.c | 22 ++++++++++
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 4 ++
3 files changed, 73 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index 8332014838b0..f825623e3edc 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -749,6 +749,52 @@ TEST_F(hid_bpf, test_hid_change_report)
ASSERT_EQ(buf[2], 0) TH_LOG("leftovers_from_previous_test");
}
+/*
+ * Call hid_bpf_input_report against the given uhid device,
+ * check that the program is called and does the expected.
+ */
+TEST_F(hid_bpf, test_hid_user_input_report_call)
+{
+ struct hid_hw_request_syscall_args args = {
+ .retval = -1,
+ .size = 10,
+ };
+ DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
+ .ctx_in = &args,
+ .ctx_size_in = sizeof(args),
+ );
+ __u8 buf[10] = {0};
+ int err, prog_fd;
+
+ LOAD_BPF;
+
+ args.hid = self->hid_id;
+ args.data[0] = 1; /* report ID */
+ args.data[1] = 2; /* report ID */
+ args.data[2] = 42; /* report ID */
+
+ prog_fd = bpf_program__fd(self->skel->progs.hid_user_input_report);
+
+ /* check that there is no data to read from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, -1) TH_LOG("read_hidraw");
+
+ err = bpf_prog_test_run_opts(prog_fd, &tattrs);
+
+ ASSERT_OK(err) TH_LOG("error while calling bpf_prog_test_run_opts");
+
+ ASSERT_EQ(args.retval, 0);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+ ASSERT_EQ(buf[0], 1);
+ ASSERT_EQ(buf[1], 2);
+ ASSERT_EQ(buf[2], 42);
+}
+
/*
* Call hid_bpf_hw_output_report against the given uhid device,
* check that the program is called and does the expected.
@@ -797,8 +843,7 @@ TEST_F(hid_bpf, test_hid_user_output_report_call)
}
/*
- * Attach hid_user_raw_request to the given uhid device,
- * call the bpf program from userspace
+ * Call hid_hw_raw_request against the given uhid device,
* check that the program is called and does the expected.
*/
TEST_F(hid_bpf, test_hid_user_raw_request_call)
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 2c2b679a83b1..f67d35def142 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -125,6 +125,28 @@ int hid_user_output_report(struct hid_hw_request_syscall_args *args)
return 0;
}
+SEC("syscall")
+int hid_user_input_report(struct hid_hw_request_syscall_args *args)
+{
+ struct hid_bpf_ctx *ctx;
+ const size_t size = args->size;
+ int i, ret = 0;
+
+ if (size > sizeof(args->data))
+ return -7; /* -E2BIG */
+
+ ctx = hid_bpf_allocate_context(args->hid);
+ if (!ctx)
+ return -1; /* EPERM check */
+
+ ret = hid_bpf_input_report(ctx, HID_INPUT_REPORT, args->data, size);
+ args->retval = ret;
+
+ hid_bpf_release_context(ctx);
+
+ return 0;
+}
+
static const __u8 rdesc[] = {
0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
0x09, 0x32, /* USAGE (Z) */
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 50c6a0d5765e..9cd56821d0f1 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -96,5 +96,9 @@ extern int hid_bpf_hw_request(struct hid_bpf_ctx *ctx,
enum hid_class_request reqtype) __ksym;
extern int hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx,
__u8 *buf, size_t buf__sz) __ksym;
+extern int hid_bpf_input_report(struct hid_bpf_ctx *ctx,
+ enum hid_report_type type,
+ __u8 *data,
+ size_t buf__sz) __ksym;
#endif /* __HID_BPF_HELPERS_H */
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 09/10] HID: bpf: allow to use bpf_timer_set_sleepable_cb() in tracing callbacks.
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Benjamin Tissoires
In-Reply-To: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
Export the sleepable kfuncs we have on HID-BPF in tracing bpf programs,
but with the condition of being used in a sleepable context.
This allows to use the bpf_timer when used in a sleepable context
through bpf_timer_set_sleepable_cb() and initiate work from a device event.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
new in v2
---
drivers/hid/bpf/hid_bpf_dispatch.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index e1a650f4a626..275f2057c48d 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -544,6 +544,11 @@ __bpf_kfunc_end_defs();
*/
BTF_KFUNCS_START(hid_bpf_kfunc_ids)
BTF_ID_FLAGS(func, hid_bpf_get_data, KF_RET_NULL)
+BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_hw_request, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_hw_output_report, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_input_report, KF_SLEEPABLE)
BTF_KFUNCS_END(hid_bpf_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
@@ -566,11 +571,11 @@ static const struct btf_kfunc_id_set hid_bpf_fmodret_set = {
/* for syscall HID-BPF */
BTF_KFUNCS_START(hid_bpf_syscall_kfunc_ids)
BTF_ID_FLAGS(func, hid_bpf_attach_prog)
-BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL)
-BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE)
-BTF_ID_FLAGS(func, hid_bpf_hw_request)
-BTF_ID_FLAGS(func, hid_bpf_hw_output_report)
-BTF_ID_FLAGS(func, hid_bpf_input_report)
+BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE | KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_hw_request, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_hw_output_report, KF_SLEEPABLE)
+BTF_ID_FLAGS(func, hid_bpf_input_report, KF_SLEEPABLE)
BTF_KFUNCS_END(hid_bpf_syscall_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 10/10] selftests/hid: add test for bpf_timer
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Benjamin Tissoires
In-Reply-To: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
This test checks that we can actually delay a workload in a sleepable
context through bpf_timer.
When an event is injected, we push it on a map of type queue and schedule
a work.
When that work kicks in, it pulls the event from the queue, and wakes
up userspace through a ring buffer.
The use of the ring buffer is there to not have sleeps in userspace
because we have no guarantees of the timing of when those jobs will be
called.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
new in v2
---
tools/testing/selftests/hid/hid_bpf.c | 83 +++++++++++
tools/testing/selftests/hid/progs/hid.c | 152 +++++++++++++++++++++
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 2 +
3 files changed, 237 insertions(+)
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index f825623e3edc..c16efb43dd91 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -875,6 +875,89 @@ TEST_F(hid_bpf, test_hid_user_raw_request_call)
ASSERT_EQ(args.data[1], 2);
}
+static __u8 workload_data;
+
+static int handle_event(void *ctx, void *data, size_t data_sz)
+{
+ const __u8 *e = data;
+
+ workload_data = *e;
+
+ return 0;
+}
+
+TEST_F(hid_bpf, test_hid_schedule_work_defer_events_2)
+{
+ struct hid_hw_request_syscall_args args = {
+ .retval = -1,
+ .size = 10,
+ };
+ DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
+ .ctx_in = &args,
+ .ctx_size_in = sizeof(args),
+ );
+ const struct test_program progs[] = {
+ { .name = "hid_defer_bpf_timer" },
+ };
+ struct ring_buffer *rb = NULL;
+ __u8 buf[10] = {0};
+ int prog_fd, err;
+
+ LOAD_PROGRAMS(progs);
+
+ /* Set up ring buffer polling */
+ rb = ring_buffer__new(bpf_map__fd(self->skel->maps.rb), handle_event, NULL, NULL);
+ ASSERT_OK_PTR(rb) TH_LOG("Failed to create ring buffer");
+ ASSERT_EQ(workload_data, 0);
+
+ args.hid = self->hid_id;
+ prog_fd = bpf_program__fd(self->skel->progs.hid_setup_timer);
+
+ err = bpf_prog_test_run_opts(prog_fd, &tattrs);
+
+ ASSERT_OK(err) TH_LOG("error while calling bpf_prog_test_run_opts");
+
+ /* check that there is no data to read from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, -1) TH_LOG("read_hidraw");
+
+ /* inject one event */
+ buf[0] = 1;
+ buf[1] = 47;
+ buf[2] = 50;
+ uhid_send_event(_metadata, self->uhid_fd, buf, 6);
+
+ err = ring_buffer__poll(rb, 100 /* timeout, ms */);
+ ASSERT_EQ(err, 1) TH_LOG("error while calling ring_buffer__poll");
+ ASSERT_EQ(workload_data, 3);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+ ASSERT_EQ(buf[0], 2);
+ ASSERT_EQ(buf[1], 3);
+ ASSERT_EQ(buf[2], 4) TH_LOG("leftovers_from_previous_test");
+
+ err = ring_buffer__poll(rb, 100 /* timeout, ms */);
+ ASSERT_EQ(err, 1) TH_LOG("error while calling ring_buffer__poll");
+ ASSERT_EQ(workload_data, 4);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, 6) TH_LOG("read_hidraw");
+ ASSERT_EQ(buf[0], 2);
+ ASSERT_EQ(buf[1], 4);
+ ASSERT_EQ(buf[2], 6);
+
+ /* read the data from hidraw */
+ memset(buf, 0, sizeof(buf));
+ err = read(self->hidraw_fd, buf, sizeof(buf));
+ ASSERT_EQ(err, -1) TH_LOG("read_hidraw");
+}
+
/*
* Attach hid_insert{0,1,2} to the given uhid device,
* retrieve and open the matching hidraw node,
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index f67d35def142..05afa056167e 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -250,3 +250,155 @@ int BPF_PROG(hid_test_insert3, struct hid_bpf_ctx *hid_ctx)
return 0;
}
+
+struct test_report {
+ __u8 data[6];
+};
+
+struct {
+ __uint(type, BPF_MAP_TYPE_QUEUE);
+ __uint(max_entries, 8);
+ __type(value, struct test_report);
+} queue SEC(".maps");
+
+struct {
+ __uint(type, BPF_MAP_TYPE_RINGBUF);
+ __uint(max_entries, 8);
+} rb SEC(".maps");
+
+struct elem {
+ struct bpf_timer t;
+};
+
+struct {
+ __uint(type, BPF_MAP_TYPE_HASH);
+ __uint(max_entries, 1024);
+ __type(key, u32);
+ __type(value, struct elem);
+} timer_map SEC(".maps");
+
+/* callback for timer_map timers */
+
+static int timer_cb1(void *map, int *key, struct bpf_timer *timer)
+{
+ struct hid_bpf_ctx *hid_ctx;
+ struct test_report buf;
+ __u8 *rb_elem;
+ int err;
+ int i, ret = 0;
+
+ /* do not pop the event, it'll be done in hid_offload_test() when
+ * notifying user space, this also allows to retry sending it
+ * if hid_bpf_input_report fails
+ */
+ if (bpf_map_peek_elem(&queue, &buf))
+ return 0;
+
+ hid_ctx = hid_bpf_allocate_context(*key);
+ if (!hid_ctx)
+ return 0; /* EPERM check */
+
+ buf.data[0] = 2;
+
+ /* re-inject the modified event into the HID stack */
+ err = hid_bpf_input_report(hid_ctx, HID_INPUT_REPORT, buf.data, sizeof(buf.data));
+ if (err == -16 /* -EBUSY */) {
+ /*
+ * This happens when we schedule the work with a 0 delay:
+ * the thread immediately starts but the current input
+ * processing hasn't finished yet. So the semaphore is
+ * already taken, and hid_input_report returns -EBUSY
+ */
+ /* schedule another attempt */
+ bpf_timer_start(timer, 5 * 1000, 0);
+
+ goto out;
+ }
+
+ if (bpf_map_pop_elem(&queue, &buf))
+ goto out;
+
+ rb_elem = bpf_ringbuf_reserve(&rb, sizeof(*rb_elem), 0);
+ if (!rb_elem)
+ goto out;
+
+ *rb_elem = buf.data[1];
+
+ bpf_ringbuf_submit(rb_elem, 0);
+
+ /* call ourself once again until there is no more events in the queue */
+ bpf_timer_start(timer, 10 * 1000 * 1000, 0);
+
+ out:
+ hid_bpf_release_context(hid_ctx);
+ return 0;
+}
+
+#define CLOCK_MONOTONIC 1
+
+SEC("?fmod_ret/hid_bpf_device_event")
+int BPF_PROG(hid_defer_bpf_timer, struct hid_bpf_ctx *hctx)
+{
+ __u8 *data = hid_bpf_get_data(hctx, 0 /* offset */, 4 /* size */);
+ struct test_report buf = {
+ .data = {2, 3, 4, 5, 6, 7},
+ };
+ struct bpf_timer *timer;
+ int key = hctx->hid->id;
+ struct elem init = {};
+
+ if (!data)
+ return 0; /* EPERM check */
+
+ /* Only schedule a delayed work when reportID is 1, otherwise
+ * simply forward it to hidraw
+ */
+ if (data[0] != 1)
+ return 0;
+
+ bpf_map_push_elem(&queue, &buf, BPF_ANY);
+ buf.data[0] = 2;
+ buf.data[1] = 4;
+ buf.data[2] = 6;
+ bpf_map_push_elem(&queue, &buf, BPF_ANY);
+
+ timer = bpf_map_lookup_elem(&timer_map, &key);
+ if (!timer)
+ return 3;
+
+ bpf_timer_set_sleepable_cb(timer, timer_cb1);
+
+ if (bpf_timer_start(timer, 5 * 1000 * 1000, 0) != 0)
+ return 2;
+
+ return -1; /* discard the event */
+}
+
+SEC("syscall")
+int hid_setup_timer(struct hid_hw_request_syscall_args *args)
+{
+ struct hid_bpf_ctx *ctx;
+ struct bpf_timer *timer;
+ struct elem init = {};
+ int key = args->hid;
+ int i, ret = 0;
+
+ ctx = hid_bpf_allocate_context(args->hid);
+ if (!ctx)
+ return -1; /* EPERM check */
+
+ bpf_map_update_elem(&timer_map, &key, &init, 0);
+
+ timer = bpf_map_lookup_elem(&timer_map, &key);
+ if (!timer) {
+ hid_bpf_release_context(ctx);
+ return 1;
+ }
+
+ bpf_timer_init(timer, &timer_map, CLOCK_MONOTONIC | BPF_F_TIMER_SLEEPABLE);
+
+ hid_bpf_release_context(ctx);
+
+ return 0;
+}
+
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 9cd56821d0f1..8235a28e7dee 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -100,5 +100,7 @@ extern int hid_bpf_input_report(struct hid_bpf_ctx *ctx,
enum hid_report_type type,
__u8 *data,
size_t buf__sz) __ksym;
+extern int bpf_timer_set_sleepable_cb(struct bpf_timer *timer,
+ int (callback_fn)(void *map, int *key, struct bpf_timer *timer)) __ksym;
#endif /* __HID_BPF_HELPERS_H */
--
2.43.0
^ permalink raw reply related
* [PATCH] HID: nintendo: Fix N64 controller being identified as mouse
From: Nuno Pereira @ 2024-02-14 21:36 UTC (permalink / raw)
To: Daniel Ogorchock; +Cc: linux-input@vger.kernel.org
This patch is regarding the recent addition of support for the NSO controllers to hid-nintendo.
All controllers are working correctly with the exception of the N64 controller, which is being identified as a mouse by udev. This results in the joystick controlling the mouse cursor and the controller not being detected by games.
The reason for this is because the N64's C buttons have been attributed to BTN_FORWARD, BTN_BACK, BTN_LEFT, BTN_RIGHT, which are buttons typically attributed to mice.
This patch changes those buttons to controller buttons, making the controller be correctly identified as such.
Signed-off-by: Nuno Pereira <nf.pereira@outlook.pt>
---
drivers/hid/hid-nintendo.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index ccc4032fb2b0..4b2c81b49b80 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -481,10 +481,10 @@ static const struct joycon_ctlr_button_mapping n64con_button_mappings[] = {
{ BTN_TR, JC_BTN_R, },
{ BTN_TR2, JC_BTN_LSTICK, }, /* ZR */
{ BTN_START, JC_BTN_PLUS, },
- { BTN_FORWARD, JC_BTN_Y, }, /* C UP */
- { BTN_BACK, JC_BTN_ZR, }, /* C DOWN */
- { BTN_LEFT, JC_BTN_X, }, /* C LEFT */
- { BTN_RIGHT, JC_BTN_MINUS, }, /* C RIGHT */
+ { BTN_SELECT, JC_BTN_Y, }, /* C UP */
+ { BTN_X, JC_BTN_ZR, }, /* C DOWN */
+ { BTN_Y, JC_BTN_X, }, /* C LEFT */
+ { BTN_C, JC_BTN_MINUS, }, /* C RIGHT */
{ BTN_MODE, JC_BTN_HOME, },
{ BTN_Z, JC_BTN_CAP, },
{ /* sentinel */ },
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 01/10] backlight: Match backlight device against struct fb_info.bl_dev
From: Daniel Thompson @ 2024-02-15 12:02 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm
In-Reply-To: <20240212162645.5661-2-tzimmermann@suse.de>
On Mon, Feb 12, 2024 at 05:16:34PM +0100, Thomas Zimmermann wrote:
> Framebuffer drivers for devices with dedicated backlight are supposed
> to set struct fb_info.bl_dev to the backlight's respective device. Use
> the value to match backlight and framebuffer in the backlight core code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply
* Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
From: Daniel Thompson @ 2024-02-15 12:06 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm, Bruno Prémont
In-Reply-To: <20240212162645.5661-5-tzimmermann@suse.de>
On Mon, Feb 12, 2024 at 05:16:37PM +0100, Thomas Zimmermann wrote:
> The driver sets struct fb_info.bl_dev to the correct backlight
> device.
This looks like it was copied from a different patch since you
added code to do this as part of the patch!
> Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
> <snip>
> diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
> index d7dddd99d325e..4500f6e03d32f 100644
> --- a/drivers/hid/hid-picolcd_fb.c
> +++ b/drivers/hid/hid-picolcd_fb.c
> @@ -493,6 +493,10 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
> info->fix = picolcdfb_fix;
> info->fix.smem_len = PICOLCDFB_SIZE*8;
>
> +#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
> + info->bl_dev = data->backlight;
> +#endif
> +
> fbdata = info->par;
> spin_lock_init(&fbdata->lock);
> fbdata->picolcd = data;
Daniel.
^ permalink raw reply
* Re: [PATCH 05/10] backlight/aat2870-backlight: Remove struct backlight.check_fb
From: Daniel Thompson @ 2024-02-15 12:06 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm
In-Reply-To: <20240212162645.5661-6-tzimmermann@suse.de>
On Mon, Feb 12, 2024 at 05:16:38PM +0100, Thomas Zimmermann wrote:
> The driver's implementation of check_fb always returns true, which
> is the default if no implementation has been set. So remove the code
> from the driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply
* Re: [PATCH 06/10] backlight/pwm-backlight: Remove struct backlight_ops.check_fb
From: Daniel Thompson @ 2024-02-15 12:08 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm, Uwe Kleine-König
In-Reply-To: <20240212162645.5661-7-tzimmermann@suse.de>
On Mon, Feb 12, 2024 at 05:16:39PM +0100, Thomas Zimmermann wrote:
> The internal check_fb callback from struct pwm_bl_data is never
> implemented. thus the driver's implementation of check_fb always
> returns true, which is the backlight core's default if no
> implementation has been set. So remove the code from the driver.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: "Uwe Kleine-König" <u.kleine-koenig@pengutronix.de>
Yay! Cleaning up platform bus legacy at the same time ;-).
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply
* Re: [PATCH 10/10] backlight: Add controls_device callback to struct backlight_ops
From: Daniel Thompson @ 2024-02-15 12:09 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm
In-Reply-To: <20240212162645.5661-11-tzimmermann@suse.de>
On Mon, Feb 12, 2024 at 05:16:43PM +0100, Thomas Zimmermann wrote:
> Replace check_fb with controls_device in struct backlight_ops. The
> new callback interface takes a Linux device instead of a framebuffer.
> Resolves one of the dependencies of backlight.h on fb.h.
>
> The few drivers that had custom implementations of check_fb can easily
> use the framebuffer's Linux device instead. Update them accordingly.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply
* Re: [PATCH 00/10] backlight: Replace struct fb_info in interfaces
From: Daniel Thompson @ 2024-02-15 12:13 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm
In-Reply-To: <20240212162645.5661-1-tzimmermann@suse.de>
On Mon, Feb 12, 2024 at 05:16:33PM +0100, Thomas Zimmermann wrote:
> Backlight drivers implement struct backlight_ops.check_fb, which
> uses struct fb_info in its interface. Replace the callback with one
> the does not use fb_info.
>
> In DRM, we have several drivers that implement backlight support. By
> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
> At the same time, fbdev is deprecated for new drivers and likely to
> be replaced on many systems.
>
> This patchset is part of a larger effort to implement the backlight
> code without depending on fbdev.
>
> Patch 1 makes the backlight core match backlight and framebuffer
> devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
> drivers and remove unnecessary implementations of check_fb. Finally,
> patch 10 replaces the check_fb hook with controls_device, which
> uses the framebuffer's Linux device instead of the framebuffer.
I won't reply individually but I also took a look at the patches for
the combo devices and it all looked good to me from a backlight
point of view.
However I don't want to drop Reviewed-by: on them since it risks those
bit being mistaken for an ack and merged ahead of the patch 1...
Daniel.
^ permalink raw reply
* Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
From: Thomas Zimmermann @ 2024-02-15 12:19 UTC (permalink / raw)
To: Daniel Thompson
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm, Bruno Prémont
In-Reply-To: <20240215120629.GH9758@aspen.lan>
Hi
Am 15.02.24 um 13:06 schrieb Daniel Thompson:
> On Mon, Feb 12, 2024 at 05:16:37PM +0100, Thomas Zimmermann wrote:
>> The driver sets struct fb_info.bl_dev to the correct backlight
>> device.
> This looks like it was copied from a different patch since you
> added code to do this as part of the patch!
Yeah, I did. I'll reword this sentence to make I more precise.
Best regards
Thomas
>
>> Thus rely on the backlight core code to match backlight
>> and framebuffer devices, and remove the extra check_fb function
>> from struct backlight_ops.
>> <snip>
>> diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
>> index d7dddd99d325e..4500f6e03d32f 100644
>> --- a/drivers/hid/hid-picolcd_fb.c
>> +++ b/drivers/hid/hid-picolcd_fb.c
>> @@ -493,6 +493,10 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
>> info->fix = picolcdfb_fix;
>> info->fix.smem_len = PICOLCDFB_SIZE*8;
>>
>> +#ifdef CONFIG_HID_PICOLCD_BACKLIGHT
>> + info->bl_dev = data->backlight;
>> +#endif
>> +
>> fbdata = info->par;
>> spin_lock_init(&fbdata->lock);
>> fbdata->picolcd = data;
>
> Daniel.
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* Re: [PATCH 00/10] backlight: Replace struct fb_info in interfaces
From: Thomas Zimmermann @ 2024-02-15 12:23 UTC (permalink / raw)
To: Daniel Thompson
Cc: lee, jingoohan1, deller, javierm, dri-devel, linux-fbdev,
linux-input, linux-pwm
In-Reply-To: <20240215121326.GL9758@aspen.lan>
Hi
Am 15.02.24 um 13:13 schrieb Daniel Thompson:
> On Mon, Feb 12, 2024 at 05:16:33PM +0100, Thomas Zimmermann wrote:
>> Backlight drivers implement struct backlight_ops.check_fb, which
>> uses struct fb_info in its interface. Replace the callback with one
>> the does not use fb_info.
>>
>> In DRM, we have several drivers that implement backlight support. By
>> including <linux/backlight.h> these drivers depend on <linux/fb.h>.
>> At the same time, fbdev is deprecated for new drivers and likely to
>> be replaced on many systems.
>>
>> This patchset is part of a larger effort to implement the backlight
>> code without depending on fbdev.
>>
>> Patch 1 makes the backlight core match backlight and framebuffer
>> devices via struct fb_info.bl_dev. Patches 2 to 9 then go through
>> drivers and remove unnecessary implementations of check_fb. Finally,
>> patch 10 replaces the check_fb hook with controls_device, which
>> uses the framebuffer's Linux device instead of the framebuffer.
> I won't reply individually but I also took a look at the patches for
> the combo devices and it all looked good to me from a backlight
> point of view.
>
> However I don't want to drop Reviewed-by: on them since it risks those
> bit being mistaken for an ack and merged ahead of the patch 1...
Thanks for reviewing. Unless someone objects, my intention is to merge
everything via the drm-misc, so all patches should go in at once. I do
have a lot more patches that untangle backlight and fbdev almost
completely, but most of these changes are in the actual graphics drivers
rather than the backlight core code. So hopefully everything can go
through the DRM tree; or maybe the fbdev tree.
Best regards
Thomas
>
>
> Daniel.
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* Re: [PATCH 0/4] leds: trigger: Improve handling of led_trigger_event() and simplify mute audio trigger
From: Takashi Iwai @ 2024-02-15 12:29 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Pavel Machek, Lee Jones, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer, linux-leds@vger.kernel.org,
linux-sound, open list:HID CORE LAYER, linux-mips
In-Reply-To: <30d49088-283c-40f3-b97b-fd5f5174a467@gmail.com>
On Tue, 13 Feb 2024 08:30:30 +0100,
Heiner Kallweit wrote:
>
> If a simple trigger is assigned to a LED, then the LED may be off until
> the next led_trigger_event() call. This may be an issue for simple
> triggers with rare led_trigger_event() calls, e.g. power supply
> charging indicators (drivers/power/supply/power_supply_leds.c).
> Therefore persist the brightness value of the last led_trigger_event()
> call and use this value if the trigger is assigned to a LED.
> This change allows to use simple triggers in more cases.
> As a first use case simplify handling of the mute audio trigger.
>
> This series touches few subsystems. I'd propose to handle it via
> the LED subsystem.
>
> Heiner Kallweit (4):
> leds: trigger: Store brightness set by led_trigger_event()
> ALSA: control-led: Integrate mute led trigger
> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER
> leds: trigger: audio: Remove this trigger
LGTM.
Reviewed-by: Takashi Iwai <tiwai@suse.de>
One thing I'm not 100% sure is the movement from ledtrig:audio-mute
and ledtrig:audio-micmute alias into snd-ctl-led module. Who would
use/process those aliases? I don't think this would be a problem, but
it might change the loading order.
Thanks!
Takashi
^ permalink raw reply
* Re: [RFC PATCH v2 1/6] dt-bindings: mfd: add entry for Marvell 88PM886 PMIC
From: Rob Herring @ 2024-02-15 14:20 UTC (permalink / raw)
To: Karel Balej
Cc: Karel Balej, Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Lee Jones, Liam Girdwood, Mark Brown, linux-input, devicetree,
linux-kernel, Duje Mihanović, ~postmarketos/upstreaming,
phone-devel
In-Reply-To: <20240211094609.2223-2-karelb@gimli.ms.mff.cuni.cz>
On Sun, Feb 11, 2024 at 10:35:51AM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> Marvell 88PM886 is a PMIC with several subdevices such as onkey,
> regulators or battery and charger. It comes in at least two revisions,
> A0 and A1 -- only A1 is described here at the moment.
>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>
> Notes:
> RFC v2:
> - Address Rob's feedback:
> - Drop mention of 88PM880.
> - Make sure the file passes bindings check (add the necessary header
> and fix `interrupt-cells`).
> - Other small changes.
> - Add regulators. Changes with respect to the regulator RFC series:
> - Address Krzysztof's comments:
> - Drop unused compatible.
> - Fix sub-node pattern.
>
> .../bindings/mfd/marvell,88pm88x.yaml | 74 +++++++++++++++++++
Filename should match the compatible.
In general, drop the 'x' wildcard.
> .../regulator/marvell,88pm88x-regulator.yaml | 28 +++++++
> 2 files changed, 102 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> create mode 100644 Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
>
> diff --git a/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> new file mode 100644
> index 000000000000..29ab979862d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml
> @@ -0,0 +1,74 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/mfd/marvell,88pm88x.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell 88PM88X PMIC core
> +
> +maintainers:
> + - Karel Balej <balejk@matfyz.cz>
> +
> +description:
> + Marvell 88PM886 is a PMIC providing several functions such as onkey,
> + regulators or battery and charger.
> +
> +properties:
> + compatible:
> + const: marvell,88pm886-a1
> +
> + reg:
> + maxItems: 1
> +
> + interrupt-controller: true
What is the device providing interrupts to (in DT)?
> +
> + interrupts:
> + maxItems: 1
> +
> + "#interrupt-cells":
> + const: 1
> +
> + regulators:
> + $ref: /schemas/regulator/marvell,88pm88x-regulator.yaml#
That's simple enough, I'd just move the regulator nodes into this doc.
> +
> +required:
> + - compatible
> + - reg
> + - interrupt-controller
> + - interrupts
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + pmic@30 {
> + compatible = "marvell,88pm886-a1";
> + reg = <0x30>;
> + interrupts = <0 4 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-parent = <&gic>;
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + regulators {
> + ldo2: ldo2 {
> + regulator-min-microvolt = <3100000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + ldo15: ldo15 {
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + };
> +
> + buck2: buck2 {
> + regulator-min-microvolt = <1800000>;
> + regulator-max-microvolt = <1800000>;
> + };
> + };
> + };
> + };
> +...
> diff --git a/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
> new file mode 100644
> index 000000000000..1b4b5f1b4932
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/marvell,88pm88x-regulator.yaml
> @@ -0,0 +1,28 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/regulator/marvell,88pm88x-regulator.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Regulators of Marvell 88PM88X PMICs.
> +
> +maintainers:
> + - Karel Balej <balejk@matfyz.cz>
> +
> +description: |
> + This is a part of device tree bindings for Marvell 88PM88X MFD.
> +
> + The regulators node is represented as a sub-node of the PMIC node on the
> + device tree.
> +
> + See also Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml for
> + additional information and example.
> +
> +patternProperties:
> + "^(ldo(1[0-6]|[1-9])|buck[1-5])$":
> + type: object
> + $ref: /schemas/regulator/regulator.yaml#
> + description: LDO or buck regulator.
> + unevaluatedProperties: false
> +
> +additionalProperties: false
> --
> 2.43.0
>
^ permalink raw reply
* Re: [RFC PATCH v2 4/6] dt-bindings: input: add entry for Marvell 88PM88X PMICs onkey
From: Rob Herring @ 2024-02-15 14:22 UTC (permalink / raw)
To: Karel Balej
Cc: Karel Balej, Dmitry Torokhov, Krzysztof Kozlowski, Conor Dooley,
Lee Jones, Liam Girdwood, Mark Brown, linux-input, devicetree,
linux-kernel, Duje Mihanović, ~postmarketos/upstreaming,
phone-devel
In-Reply-To: <20240211094609.2223-5-karelb@gimli.ms.mff.cuni.cz>
On Sun, Feb 11, 2024 at 10:35:54AM +0100, Karel Balej wrote:
> From: Karel Balej <balejk@matfyz.cz>
>
> Marvell 88PM88X PMICs provide onkey functionality -- add the bindings.
>
> Signed-off-by: Karel Balej <balejk@matfyz.cz>
> ---
>
> Notes:
> RFC v2:
> - Add wakeup-source property and reference onkey schema from MFD.
> - Reword commit message.
>
> .../bindings/input/marvell,88pm88x-onkey.yaml | 32 +++++++++++++++++++
> .../bindings/mfd/marvell,88pm88x.yaml | 8 +++++
> 2 files changed, 40 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
>
> diff --git a/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml b/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
> new file mode 100644
> index 000000000000..5d3d451d0e1f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/marvell,88pm88x-onkey.yaml
> @@ -0,0 +1,32 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/marvell,88pm88x-onkey.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Onkey driver for Marvell 88PM88X PMICs.
> +
> +maintainers:
> + - Karel Balej <balejk@matfyz.cz>
> +
> +description: |
> + This module is part of the 88PM88X MFD device. For more details
> + see Documentation/devicetree/bindings/mfd/marvell,88pm88x.yaml.
> +
> + The onkey controller is represented as a sub-node of the PMIC node in
> + the device tree.
Why do you need a child node? You don't. Just add 'wakeup-source' to the
parent.
> +
> +allOf:
> + - $ref: input.yaml#
Doesn't look like you are using any properties from this?
> +
> +properties:
> + compatible:
> + const: marvell,88pm88x-onkey
> +
> + wakeup-source: true
> +
> +required:
> + - compatible
> +
> +additionalProperties: false
> +...
^ permalink raw reply
* Re: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
From: Benjamin Tissoires @ 2024-02-15 15:23 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest
In-Reply-To: <20240214-hid-bpf-sleepable-v2-2-5756b054724d@kernel.org>
On Feb 14 2024, Benjamin Tissoires wrote:
> They are implemented as a kfunc, which means a little bit of tweaks in
> the verifier.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> changes in v2 (compared to the one attaches to v1 0/9):
> - make use of a kfunc
> - add a (non-used) BPF_F_TIMER_SLEEPABLE
> - the callback is *not* called, it makes the kernel crashes
> ---
> include/linux/bpf_verifier.h | 2 +
> include/uapi/linux/bpf.h | 12 +++++
> kernel/bpf/helpers.c | 105 ++++++++++++++++++++++++++++++++++++++++---
> kernel/bpf/verifier.c | 72 ++++++++++++++++++++++++++---
> 4 files changed, 180 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> index 84365e6dd85d..789ef5fec547 100644
> --- a/include/linux/bpf_verifier.h
> +++ b/include/linux/bpf_verifier.h
> @@ -426,6 +426,7 @@ struct bpf_verifier_state {
> * while they are still in use.
> */
> bool used_as_loop_entry;
> + bool in_sleepable;
>
> /* first and last insn idx of this verifier state */
> u32 first_insn_idx;
> @@ -626,6 +627,7 @@ struct bpf_subprog_info {
> bool is_async_cb: 1;
> bool is_exception_cb: 1;
> bool args_cached: 1;
> + bool is_sleepable: 1;
>
> u8 arg_cnt;
> struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index d96708380e52..0838597028a9 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -7427,6 +7427,18 @@ enum {
> BPF_F_TIMER_CPU_PIN = (1ULL << 1),
> };
>
> +/* Extra flags to control bpf_timer_init() behaviour, in addition to CLOCK_*.
> + * - BPF_F_TIMER_SLEEPABLE: Timer will run in a workqueue in a sleepable
> + * context.
> + */
> +enum {
> + /* CLOCK_* are using bits 0 to 3 */
> + BPF_F_TIMER_SLEEPABLE = (1ULL << 4),
> + __MAX_BPF_F_TIMER_INIT,
> +};
> +
> +#define MAX_BPF_F_TIMER_INIT __MAX_BPF_F_TIMER_INIT
> +
> /* BPF numbers iterator state */
> struct bpf_iter_num {
> /* opaque iterator state; having __u64 here allows to preserve correct
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index 4db1c658254c..2dbc09ce841a 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1097,9 +1097,11 @@ const struct bpf_func_proto bpf_snprintf_proto = {
> */
> struct bpf_hrtimer {
> struct hrtimer timer;
> + struct work_struct work;
> struct bpf_map *map;
> struct bpf_prog *prog;
> void __rcu *callback_fn;
> + void __rcu *sleepable_cb_fn;
> void *value;
> };
>
> @@ -1113,18 +1115,59 @@ struct bpf_timer_kern {
> struct bpf_spin_lock lock;
> } __attribute__((aligned(8)));
>
> +static void bpf_timer_work_cb(struct work_struct *work)
> +{
> + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
> + struct bpf_map *map = t->map;
> + void *value = t->value;
> + bpf_callback_t callback_fn;
> + void *key;
> + u32 idx;
> +
> + BTF_TYPE_EMIT(struct bpf_timer);
> +
> + rcu_read_lock();
> + callback_fn = rcu_dereference(t->sleepable_cb_fn);
> + rcu_read_unlock();
> + if (!callback_fn)
> + return;
> +
> + /* FIXME: do we need any locking? */
> + if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> +
> + /* compute the key */
> + idx = ((char *)value - array->value) / array->elem_size;
> + key = &idx;
> + } else { /* hash or lru */
> + key = value - round_up(map->key_size, 8);
> + }
> +
> + /* FIXME: this crashes the system with
> + * BUG: kernel NULL pointer dereference, address: 000000000000000b
> + */
> + /* callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0); */
I've found an interesting side effect here.
If I uncomment the line above, I get the following dumpstack:
[ 5.375257] BUG: kernel NULL pointer dereference, address: 000000000000000b
[ 5.376587] #PF: supervisor instruction fetch in kernel mode
[ 5.376932] #PF: error_code(0x0010) - not-present page
[ 5.377249] PGD 1016e6067 P4D 1016e6067 PUD 1016e5067 PMD 0
[ 5.377602] Oops: 0010 [#1] PREEMPT SMP NOPTI
[ 5.377876] CPU: 0 PID: 10 Comm: kworker/0:1 Not tainted 6.8.0-rc2-gd2c1e1837606-dirty #285
[ 5.378378] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-1.fc39 04/01/2014
[ 5.378913] Workqueue: events bpf_timer_work_cb
[ 5.379197] RIP: 0010:0xb
[ 5.379369] Code: Unable to access opcode bytes at 0xffffffffffffffe1.
[ 5.379768] RSP: 0018:ffffa5300005be10 EFLAGS: 00010246
[ 5.380088] RAX: 0000000000000068 RBX: 000000000000000b RCX: 0000000000000000
[ 5.380523] RDX: ffffa530012a5ff0 RSI: ffffa530012a5fe8 RDI: ffff915ec2bad000
[ 5.380947] RBP: ffff915ec2bad000 R08: 0000000000000000 R09: 0000000000000001
[ 5.381368] R10: 0000000000000000 R11: ffffffff9166f820 R12: ffffa530012a5ff0
[ 5.381793] R13: ffffa530012a5fe8 R14: ffff915ec0073005 R15: ffffffff900aea8a
[ 5.382213] FS: 0000000000000000(0000) GS:ffff915efbc00000(0000) knlGS:0000000000000000
[ 5.382691] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 5.383036] CR2: ffffffffffffffe1 CR3: 000000010761e003 CR4: 0000000000770ef0
[ 5.383460] PKRU: 55555554
[ 5.383643] Call Trace:
[ 5.383831] <TASK>
[ 5.383994] ? __die+0x23/0x70
[ 5.384228] ? page_fault_oops+0x181/0x4a0
[ 5.384541] ? exc_page_fault+0x67/0x1a0
[ 5.384840] ? asm_exc_page_fault+0x26/0x30
[ 5.385154] ? process_one_work+0x16a/0x4b0
[ 5.385467] ? bpf_timer_work_cb+0xfc/0x240
[ 5.385789] ? process_one_work+0x1d4/0x4b0
[ 5.386101] ? worker_thread+0x1d5/0x3c0
[ 5.386393] ? __pfx_worker_thread+0x10/0x10
[ 5.386724] ? kthread+0xf5/0x130
[ 5.386974] ? __pfx_kthread+0x10/0x10
[ 5.387255] ? ret_from_fork+0x31/0x50
[ 5.387537] ? __pfx_kthread+0x10/0x10
[ 5.387820] ? ret_from_fork_asm+0x1b/0x30
[ 5.388127] </TASK>
And it looks like the callback is properly called, but the fault comes
after:
(gdb) list *(process_one_work+0x16a)
0xffffffff810aea8a is in process_one_work (kernel/workqueue.c:2606).
2601 * disabled.
2602 */
2603 set_work_pool_and_clear_pending(work, pool->id);
2604
2605 pwq->stats[PWQ_STAT_STARTED]++;
2606 raw_spin_unlock_irq(&pool->lock);
2607
2608 lock_map_acquire(&pwq->wq->lockdep_map);
2609 lock_map_acquire(&lockdep_map);
2610 /*
And there is no real reason for the spinlock to not be set unless there
is a memroy corruption.
And if in my program I call bpf_timer_set_callback() before bpf_timer_set_sleepable_cb()
like:
bpf_timer_set_callback(timer, timer_cb1);
bpf_timer_set_sleepable_cb(timer, timer_cb1);
I do not see the corruption in the memory.
So it seems that when BPF calls the helper, the callback_fn is set in a
mode that it is not polluting the memory, while when calling the function
pointer from a kfunc, something is not properly set and there is a
memory corruption.
Does that rings any bell?
> + /* The verifier checked that return value is zero. */
> +}
> +
> static DEFINE_PER_CPU(struct bpf_hrtimer *, hrtimer_running);
>
> static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
> {
> struct bpf_hrtimer *t = container_of(hrtimer, struct bpf_hrtimer, timer);
> + bpf_callback_t callback_fn, sleepable_cb_fn;
> struct bpf_map *map = t->map;
> void *value = t->value;
> - bpf_callback_t callback_fn;
> void *key;
> u32 idx;
>
> BTF_TYPE_EMIT(struct bpf_timer);
> + sleepable_cb_fn = rcu_dereference_check(t->sleepable_cb_fn, rcu_read_lock_bh_held());
> + if (sleepable_cb_fn) {
> + schedule_work(&t->work);
> + goto out;
> + }
> +
> callback_fn = rcu_dereference_check(t->callback_fn, rcu_read_lock_bh_held());
> if (!callback_fn)
> goto out;
> @@ -1154,10 +1197,14 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
> return HRTIMER_NORESTART;
> }
>
> +#define BPF_TIMER_CLOCK_MASK (MAX_CLOCKS - 1) /* 0xf */
> +#define BPF_TIMER_FLAGS_MASK GENMASK_ULL(63, 4)
> +
> BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map,
> u64, flags)
> {
> - clockid_t clockid = flags & (MAX_CLOCKS - 1);
> + clockid_t clockid = flags & BPF_TIMER_CLOCK_MASK;
> + u64 bpf_timer_flags = flags & BPF_TIMER_FLAGS_MASK;
> struct bpf_hrtimer *t;
> int ret = 0;
>
> @@ -1168,7 +1215,7 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
> if (in_nmi())
> return -EOPNOTSUPP;
>
> - if (flags >= MAX_CLOCKS ||
> + if (bpf_timer_flags & ~(BPF_F_TIMER_SLEEPABLE) ||
> /* similar to timerfd except _ALARM variants are not supported */
> (clockid != CLOCK_MONOTONIC &&
> clockid != CLOCK_REALTIME &&
> @@ -1190,7 +1237,10 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
> t->map = map;
> t->prog = NULL;
> rcu_assign_pointer(t->callback_fn, NULL);
> + rcu_assign_pointer(t->sleepable_cb_fn, NULL);
> + /* FIXME: probably do something about the SLEEPABLE flag */
> hrtimer_init(&t->timer, clockid, HRTIMER_MODE_REL_SOFT);
> + INIT_WORK(&t->work, bpf_timer_work_cb);
> t->timer.function = bpf_timer_cb;
> WRITE_ONCE(timer->timer, t);
> /* Guarantee the order between timer->timer and map->usercnt. So
> @@ -1221,8 +1271,8 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
> .arg3_type = ARG_ANYTHING,
> };
>
> -BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
> - struct bpf_prog_aux *, aux)
> +static int __bpf_timer_set_callback(struct bpf_timer_kern *timer, void *callback_fn,
> + struct bpf_prog_aux *aux, bool is_sleepable)
> {
> struct bpf_prog *prev, *prog = aux->prog;
> struct bpf_hrtimer *t;
> @@ -1260,12 +1310,24 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callb
> bpf_prog_put(prev);
> t->prog = prog;
> }
> - rcu_assign_pointer(t->callback_fn, callback_fn);
> + if (is_sleepable) {
> + rcu_assign_pointer(t->sleepable_cb_fn, callback_fn);
> + rcu_assign_pointer(t->callback_fn, NULL);
> + } else {
> + rcu_assign_pointer(t->callback_fn, callback_fn);
> + rcu_assign_pointer(t->sleepable_cb_fn, NULL);
> + }
> out:
> __bpf_spin_unlock_irqrestore(&timer->lock);
> return ret;
> }
>
> +BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
> + struct bpf_prog_aux *, aux)
> +{
> + return __bpf_timer_set_callback(timer, callback_fn, aux, false);
> +}
> +
> static const struct bpf_func_proto bpf_timer_set_callback_proto = {
> .func = bpf_timer_set_callback,
> .gpl_only = true,
> @@ -1353,6 +1415,7 @@ BPF_CALL_1(bpf_timer_cancel, struct bpf_timer_kern *, timer)
> * if it was running.
> */
> ret = ret ?: hrtimer_cancel(&t->timer);
> + ret = ret ?: cancel_work_sync(&t->work);
> return ret;
> }
>
> @@ -1407,6 +1470,8 @@ void bpf_timer_cancel_and_free(void *val)
> */
> if (this_cpu_read(hrtimer_running) != t)
> hrtimer_cancel(&t->timer);
> +
> + cancel_work_sync(&t->work);
> kfree(t);
> }
>
> @@ -2542,6 +2607,33 @@ __bpf_kfunc void bpf_throw(u64 cookie)
> WARN(1, "A call to BPF exception callback should never return\n");
> }
>
> +/* FIXME: use kernel doc style */
> +/* Description
> + * Configure the timer to call *callback_fn* static function in a
> + * sleepable context.
> + * Return
> + * 0 on success.
> + * **-EINVAL** if *timer* was not initialized with bpf_timer_init() earlier.
> + * **-EPERM** if *timer* is in a map that doesn't have any user references.
> + * The user space should either hold a file descriptor to a map with timers
> + * or pin such map in bpffs. When map is unpinned or file descriptor is
> + * closed all timers in the map will be cancelled and freed.
> + */
> +__bpf_kfunc int bpf_timer_set_sleepable_cb(struct bpf_timer_kern *timer,
> + int (callback_fn)(void *map, int *key, struct bpf_timer *timer))
> +{
> + struct bpf_throw_ctx ctx = {};
> +
> + /* FIXME: definietely not sure this is OK */
> + arch_bpf_stack_walk(bpf_stack_walker, &ctx);
> + WARN_ON_ONCE(!ctx.aux);
> +
> + if (!ctx.aux)
> + return -EINVAL;
> +
> + return __bpf_timer_set_callback(timer, (void *)callback_fn, ctx.aux, true);
> +}
> +
> __bpf_kfunc_end_defs();
>
> BTF_KFUNCS_START(generic_btf_ids)
> @@ -2573,6 +2665,7 @@ BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
> #endif
> BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
> BTF_ID_FLAGS(func, bpf_throw)
> +BTF_ID_FLAGS(func, bpf_timer_set_sleepable_cb)
I also just realized that this should go into common_btf_ids so that
anybody can use it, not just tracing.
Cheers,
Benjamin
> BTF_KFUNCS_END(generic_btf_ids)
>
> static const struct btf_kfunc_id_set generic_kfunc_set = {
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 7831adba9abf..67da3f7bddb5 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -503,6 +503,8 @@ static bool is_dynptr_ref_function(enum bpf_func_id func_id)
> static bool is_sync_callback_calling_kfunc(u32 btf_id);
> static bool is_bpf_throw_kfunc(struct bpf_insn *insn);
>
> +static bool is_bpf_timer_set_sleepable_cb_kfunc(u32 btf_id);
> +
> static bool is_sync_callback_calling_function(enum bpf_func_id func_id)
> {
> return func_id == BPF_FUNC_for_each_map_elem ||
> @@ -513,7 +515,8 @@ static bool is_sync_callback_calling_function(enum bpf_func_id func_id)
>
> static bool is_async_callback_calling_function(enum bpf_func_id func_id)
> {
> - return func_id == BPF_FUNC_timer_set_callback;
> + return func_id == BPF_FUNC_timer_set_callback ||
> + is_bpf_timer_set_sleepable_cb_kfunc(func_id);
> }
>
> static bool is_callback_calling_function(enum bpf_func_id func_id)
> @@ -1414,6 +1417,7 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state,
> }
> dst_state->speculative = src->speculative;
> dst_state->active_rcu_lock = src->active_rcu_lock;
> + dst_state->in_sleepable = src->in_sleepable;
> dst_state->curframe = src->curframe;
> dst_state->active_lock.ptr = src->active_lock.ptr;
> dst_state->active_lock.id = src->active_lock.id;
> @@ -2413,6 +2417,7 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
> * Initialize it similar to do_check_common().
> */
> elem->st.branches = 1;
> + elem->st.in_sleepable = env->subprog_info[subprog].is_sleepable;
> frame = kzalloc(sizeof(*frame), GFP_KERNEL);
> if (!frame)
> goto err;
> @@ -5257,7 +5262,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
>
> static bool in_sleepable(struct bpf_verifier_env *env)
> {
> - return env->prog->aux->sleepable;
> + return env->prog->aux->sleepable ||
> + (env->cur_state && env->cur_state->in_sleepable);
> }
>
> /* The non-sleepable programs and sleepable programs with explicit bpf_rcu_read_lock()
> @@ -5439,6 +5445,26 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
> return -EACCES;
> }
> break;
> + case BPF_TIMER:
> + /* FIXME: kptr does the above, should we use the same? */
> + if (src != ACCESS_DIRECT) {
> + verbose(env, "bpf_timer cannot be accessed indirectly by helper\n");
> + return -EACCES;
> + }
> + if (!tnum_is_const(reg->var_off)) {
> + verbose(env, "bpf_timer access cannot have variable offset\n");
> + return -EACCES;
> + }
> + if (p != off + reg->var_off.value) {
> + verbose(env, "bpf_timer access misaligned expected=%u off=%llu\n",
> + p, off + reg->var_off.value);
> + return -EACCES;
> + }
> + if (size != bpf_size_to_bytes(BPF_DW)) {
> + verbose(env, "bpf_timer access size must be BPF_DW\n");
> + return -EACCES;
> + }
> + break;
> default:
> verbose(env, "%s cannot be accessed directly by load/store\n",
> btf_field_type_name(field->type));
> @@ -9439,11 +9465,13 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
>
> if (insn->code == (BPF_JMP | BPF_CALL) &&
> insn->src_reg == 0 &&
> - insn->imm == BPF_FUNC_timer_set_callback) {
> + (insn->imm == BPF_FUNC_timer_set_callback ||
> + is_bpf_timer_set_sleepable_cb_kfunc(insn->imm))) {
> struct bpf_verifier_state *async_cb;
>
> /* there is no real recursion here. timer callbacks are async */
> env->subprog_info[subprog].is_async_cb = true;
> + env->subprog_info[subprog].is_sleepable = is_bpf_timer_set_sleepable_cb_kfunc(insn->imm);
> async_cb = push_async_cb(env, env->subprog_info[subprog].start,
> insn_idx, subprog);
> if (!async_cb)
> @@ -10412,6 +10440,10 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
> break;
> }
>
> + if (is_bpf_timer_set_sleepable_cb_kfunc(func_id))
> + err = push_callback_call(env, insn, insn_idx, meta.subprogno,
> + set_timer_callback_state);
> +
> if (err)
> return err;
>
> @@ -10789,6 +10821,7 @@ enum {
> KF_ARG_LIST_NODE_ID,
> KF_ARG_RB_ROOT_ID,
> KF_ARG_RB_NODE_ID,
> + KF_ARG_TIMER_ID,
> };
>
> BTF_ID_LIST(kf_arg_btf_ids)
> @@ -10797,6 +10830,7 @@ BTF_ID(struct, bpf_list_head)
> BTF_ID(struct, bpf_list_node)
> BTF_ID(struct, bpf_rb_root)
> BTF_ID(struct, bpf_rb_node)
> +BTF_ID(struct, bpf_timer_kern)
>
> static bool __is_kfunc_ptr_arg_type(const struct btf *btf,
> const struct btf_param *arg, int type)
> @@ -10840,6 +10874,12 @@ static bool is_kfunc_arg_rbtree_node(const struct btf *btf, const struct btf_par
> return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_RB_NODE_ID);
> }
>
> +static bool is_kfunc_arg_timer(const struct btf *btf, const struct btf_param *arg)
> +{
> + bool ret = __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_TIMER_ID);
> + return ret;
> +}
> +
> static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf,
> const struct btf_param *arg)
> {
> @@ -10908,6 +10948,7 @@ enum kfunc_ptr_arg_type {
> KF_ARG_PTR_TO_RB_NODE,
> KF_ARG_PTR_TO_NULL,
> KF_ARG_PTR_TO_CONST_STR,
> + KF_ARG_PTR_TO_TIMER,
> };
>
> enum special_kfunc_type {
> @@ -10934,6 +10975,7 @@ enum special_kfunc_type {
> KF_bpf_percpu_obj_drop_impl,
> KF_bpf_throw,
> KF_bpf_iter_css_task_new,
> + KF_bpf_timer_set_sleepable_cb,
> };
>
> BTF_SET_START(special_kfunc_set)
> @@ -10960,6 +11002,7 @@ BTF_ID(func, bpf_throw)
> #ifdef CONFIG_CGROUPS
> BTF_ID(func, bpf_iter_css_task_new)
> #endif
> +BTF_ID(func, bpf_timer_set_sleepable_cb)
> BTF_SET_END(special_kfunc_set)
>
> BTF_ID_LIST(special_kfunc_list)
> @@ -10990,6 +11033,7 @@ BTF_ID(func, bpf_iter_css_task_new)
> #else
> BTF_ID_UNUSED
> #endif
> +BTF_ID(func, bpf_timer_set_sleepable_cb)
>
> static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
> {
> @@ -11061,6 +11105,9 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
> if (is_kfunc_arg_const_str(meta->btf, &args[argno]))
> return KF_ARG_PTR_TO_CONST_STR;
>
> + if (is_kfunc_arg_timer(meta->btf, &args[argno]))
> + return KF_ARG_PTR_TO_TIMER;
> +
> if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) {
> if (!btf_type_is_struct(ref_t)) {
> verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n",
> @@ -11318,6 +11365,11 @@ static bool is_bpf_throw_kfunc(struct bpf_insn *insn)
> insn->imm == special_kfunc_list[KF_bpf_throw];
> }
>
> +static bool is_bpf_timer_set_sleepable_cb_kfunc(u32 btf_id)
> +{
> + return btf_id == special_kfunc_list[KF_bpf_timer_set_sleepable_cb];
> +}
> +
> static bool is_rbtree_lock_required_kfunc(u32 btf_id)
> {
> return is_bpf_rbtree_api_kfunc(btf_id);
> @@ -11693,6 +11745,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> case KF_ARG_PTR_TO_CALLBACK:
> case KF_ARG_PTR_TO_REFCOUNTED_KPTR:
> case KF_ARG_PTR_TO_CONST_STR:
> + case KF_ARG_PTR_TO_TIMER:
> /* Trusted by default */
> break;
> default:
> @@ -11973,6 +12026,9 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> if (ret)
> return ret;
> break;
> + case KF_ARG_PTR_TO_TIMER:
> + /* FIXME: should we do anything here? */
> + break;
> }
> }
>
> @@ -15591,7 +15647,9 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
> return DONE_EXPLORING;
>
> case BPF_CALL:
> - if (insn->src_reg == 0 && insn->imm == BPF_FUNC_timer_set_callback)
> + if (insn->src_reg == 0 &&
> + (insn->imm == BPF_FUNC_timer_set_callback ||
> + is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)))
> /* Mark this call insn as a prune point to trigger
> * is_state_visited() check before call itself is
> * processed by __check_func_call(). Otherwise new
> @@ -16767,6 +16825,9 @@ static bool states_equal(struct bpf_verifier_env *env,
> if (old->active_rcu_lock != cur->active_rcu_lock)
> return false;
>
> + if (old->in_sleepable != cur->in_sleepable)
> + return false;
> +
> /* for states to be equal callsites have to be the same
> * and all frame states need to be equivalent
> */
> @@ -19644,7 +19705,8 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
> continue;
> }
>
> - if (insn->imm == BPF_FUNC_timer_set_callback) {
> + if (insn->imm == BPF_FUNC_timer_set_callback ||
> + is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)) {
> /* The verifier will process callback_fn as many times as necessary
> * with different maps and the register states prepared by
> * set_timer_callback_state will be accurate.
>
> --
> 2.43.0
>
^ permalink raw reply
* Re: [PATCH v1 1/1] input: touchcreen: tsc2007: make interrupt optional
From: Dmitry Torokhov @ 2024-02-15 17:55 UTC (permalink / raw)
To: Svyatoslav Ryhel
Cc: Uwe Kleine-König, Benjamin Bara, Richard Leitner,
linux-input, linux-kernel
In-Reply-To: <20240210175530.137361-2-clamor95@gmail.com>
Hi Svyatoslav,
On Sat, Feb 10, 2024 at 07:55:30PM +0200, Svyatoslav Ryhel wrote:
> In case tsc2007 is used as an ADC sensor there will be no interrupt
> provided at all, so set up an interrupt only if one is present and
> remove associated warning.
If we want to do this, we should better handle the input device portion
of the driver. We have 2 options:
- switch the input device into polling mode when interrupt is absent
- do not create input device
Those do not need to be mutually exclusive (i.e. we could use absence of
both device tree interrupt property as well as lack of poll-interval
property to suppress creation of the input device and only leave iio.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v1 1/1] input: touchcreen: tsc2007: make interrupt optional
From: Svyatoslav Ryhel @ 2024-02-15 18:09 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Uwe Kleine-König, Benjamin Bara, Richard Leitner,
linux-input, linux-kernel
In-Reply-To: <Zc5QANGhS8EPvgEy@google.com>
чт, 15 лют. 2024 р. о 19:55 Dmitry Torokhov <dmitry.torokhov@gmail.com> пише:
>
> Hi Svyatoslav,
>
> On Sat, Feb 10, 2024 at 07:55:30PM +0200, Svyatoslav Ryhel wrote:
> > In case tsc2007 is used as an ADC sensor there will be no interrupt
> > provided at all, so set up an interrupt only if one is present and
> > remove associated warning.
>
> If we want to do this, we should better handle the input device portion
> of the driver. We have 2 options:
>
> - switch the input device into polling mode when interrupt is absent
> - do not create input device
>
> Those do not need to be mutually exclusive (i.e. we could use absence of
> both device tree interrupt property as well as lack of poll-interval
> property to suppress creation of the input device and only leave iio.
>
> Thanks.
>
I do not care about input part and suppressing it is perfectly fine for me.
Which implementation would be accepted?
I suppose I may isolate input device creation into separate function and
add check for interrupt or poll-interval. If both are not present then input
device creation will be skipped. Will this be sufficient?
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
From: Martin KaFai Lau @ 2024-02-16 6:36 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Eduard Zingerman, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Jiri Kosina,
Benjamin Tissoires, Jonathan Corbet, Shuah Khan
In-Reply-To: <20240214-hid-bpf-sleepable-v2-2-5756b054724d@kernel.org>
On 2/14/24 9:18 AM, Benjamin Tissoires wrote:
> +static void bpf_timer_work_cb(struct work_struct *work)
> +{
> + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
> + struct bpf_map *map = t->map;
> + void *value = t->value;
> + bpf_callback_t callback_fn;
> + void *key;
> + u32 idx;
> +
> + BTF_TYPE_EMIT(struct bpf_timer);
> +
> + rcu_read_lock();
> + callback_fn = rcu_dereference(t->sleepable_cb_fn);
> + rcu_read_unlock();
I took a very brief look at patch 2. One thing that may worth to ask here, the
rcu_read_unlock() seems to be done too early. It is protecting the
t->sleepable_cb_fn (?), so should it be done after finished using the callback_fn?
A high level design question. The intention of the new
bpf_timer_set_sleepable_cb() kfunc is actually to delay work to a workqueue. It
is useful to delay work from the bpf_timer_cb and it may also useful to delay
work from other bpf running context (e.g. the networking hooks like "tc"). The
bpf_timer_set_sleepable_cb() seems to be unnecessary forcing delay-work must be
done in a bpf_timer_cb.
Have you thought about if it is possible to create a more generic kfunc like
bpf_schedule_work() to delay work to a workqueue ?
> + if (!callback_fn)
> + return;
> +
> + /* FIXME: do we need any locking? */
> + if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> +
> + /* compute the key */
> + idx = ((char *)value - array->value) / array->elem_size;
> + key = &idx;
> + } else { /* hash or lru */
> + key = value - round_up(map->key_size, 8);
> + }
> +
> + /* FIXME: this crashes the system with
> + * BUG: kernel NULL pointer dereference, address: 000000000000000b
> + */
> + /* callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0); */
> + /* The verifier checked that return value is zero. */
> +}
> +
[ ... ]
> +/* FIXME: use kernel doc style */
> +/* Description
> + * Configure the timer to call *callback_fn* static function in a
> + * sleepable context.
> + * Return
> + * 0 on success.
> + * **-EINVAL** if *timer* was not initialized with bpf_timer_init() earlier.
> + * **-EPERM** if *timer* is in a map that doesn't have any user references.
> + * The user space should either hold a file descriptor to a map with timers
> + * or pin such map in bpffs. When map is unpinned or file descriptor is
> + * closed all timers in the map will be cancelled and freed.
> + */
> +__bpf_kfunc int bpf_timer_set_sleepable_cb(struct bpf_timer_kern *timer,
> + int (callback_fn)(void *map, int *key, struct bpf_timer *timer))
> +{
> + struct bpf_throw_ctx ctx = {};
> +
> + /* FIXME: definietely not sure this is OK */
> + arch_bpf_stack_walk(bpf_stack_walker, &ctx);
> + WARN_ON_ONCE(!ctx.aux);
> +
> + if (!ctx.aux)
> + return -EINVAL;
> +
> + return __bpf_timer_set_callback(timer, (void *)callback_fn, ctx.aux, true);
> +}
> +
^ permalink raw reply
* Re: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
From: Benjamin Tissoires @ 2024-02-16 8:13 UTC (permalink / raw)
To: Martin KaFai Lau
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Eduard Zingerman, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Jiri Kosina,
Benjamin Tissoires, Jonathan Corbet, Shuah Khan
In-Reply-To: <a72147f5-2b7d-4267-9881-6a645c575838@linux.dev>
On Feb 15 2024, Martin KaFai Lau wrote:
> On 2/14/24 9:18 AM, Benjamin Tissoires wrote:
> > +static void bpf_timer_work_cb(struct work_struct *work)
> > +{
> > + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
> > + struct bpf_map *map = t->map;
> > + void *value = t->value;
> > + bpf_callback_t callback_fn;
> > + void *key;
> > + u32 idx;
> > +
> > + BTF_TYPE_EMIT(struct bpf_timer);
> > +
> > + rcu_read_lock();
> > + callback_fn = rcu_dereference(t->sleepable_cb_fn);
> > + rcu_read_unlock();
>
> I took a very brief look at patch 2. One thing that may worth to ask here,
> the rcu_read_unlock() seems to be done too early. It is protecting the
> t->sleepable_cb_fn (?), so should it be done after finished using the
> callback_fn?
Probably :)
TBH, everytime I work with RCUs I spent countless hours trying to
re-understand everything, and in this case I'm currently in the "let's
make it work" process than fixing concurrency issues.
I still gave it a shot in case it solves my issue, but no, I still have
the crash.
But given that callback_fn might sleep, isn't it an issue to keep the
RCU_reader lock so long? (we don't seem to call synchronize_rcu() so it
might be fine, but I'd like the confirmation from someone else).
>
> A high level design question. The intention of the new
> bpf_timer_set_sleepable_cb() kfunc is actually to delay work to a workqueue.
> It is useful to delay work from the bpf_timer_cb and it may also useful to
> delay work from other bpf running context (e.g. the networking hooks like
> "tc"). The bpf_timer_set_sleepable_cb() seems to be unnecessary forcing
> delay-work must be done in a bpf_timer_cb.
Basically I'm just a monkey here. I've been told that I should use
bpf_timer[0]. But my implementation is not finished, as Alexei mentioned
that we should bypass hrtimer if I'm not wrong [1].
>
> Have you thought about if it is possible to create a more generic kfunc like
> bpf_schedule_work() to delay work to a workqueue ?
>
AFAIU if we were to have a separate bpf_schedule_work(), we still need
all of the infra of bpf_timer, because we need to keep the programs
around in the same way bpf_timer does. So basically, bpf_timer will not
only be about hrtimers, but anything that need to run an async callback.
I submitted this RFC v2 not for the "this is ready", but mostly because
there is a crash and I can't see where it comes from, and I suspect this
is from a piece I do not understand (translation from the BPF langage
into actual elf assembly).
Cheers,
Benjamin
[0] https://lore.kernel.org/bpf/ztou4yyrsdfmmhdwgu2f2noartpqklhvtbw7vj2ptk54eqohvb@qci7bcnbd56q/T/#mc9cab17138b13c83299f0836ca0b2dde0643ea4b
[1] https://lore.kernel.org/bpf/ztou4yyrsdfmmhdwgu2f2noartpqklhvtbw7vj2ptk54eqohvb@qci7bcnbd56q/T/#mf59824ad625992b980afbc4f27c83e76245815e7
>
>
> > + if (!callback_fn)
> > + return;
> > +
> > + /* FIXME: do we need any locking? */
> > + if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> > + struct bpf_array *array = container_of(map, struct bpf_array, map);
> > +
> > + /* compute the key */
> > + idx = ((char *)value - array->value) / array->elem_size;
> > + key = &idx;
> > + } else { /* hash or lru */
> > + key = value - round_up(map->key_size, 8);
> > + }
> > +
> > + /* FIXME: this crashes the system with
> > + * BUG: kernel NULL pointer dereference, address: 000000000000000b
> > + */
> > + /* callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0); */
> > + /* The verifier checked that return value is zero. */
> > +}
> > +
>
> [ ... ]
>
> > +/* FIXME: use kernel doc style */
> > +/* Description
> > + * Configure the timer to call *callback_fn* static function in a
> > + * sleepable context.
> > + * Return
> > + * 0 on success.
> > + * **-EINVAL** if *timer* was not initialized with bpf_timer_init() earlier.
> > + * **-EPERM** if *timer* is in a map that doesn't have any user references.
> > + * The user space should either hold a file descriptor to a map with timers
> > + * or pin such map in bpffs. When map is unpinned or file descriptor is
> > + * closed all timers in the map will be cancelled and freed.
> > + */
> > +__bpf_kfunc int bpf_timer_set_sleepable_cb(struct bpf_timer_kern *timer,
> > + int (callback_fn)(void *map, int *key, struct bpf_timer *timer))
> > +{
> > + struct bpf_throw_ctx ctx = {};
> > +
> > + /* FIXME: definietely not sure this is OK */
> > + arch_bpf_stack_walk(bpf_stack_walker, &ctx);
> > + WARN_ON_ONCE(!ctx.aux);
> > +
> > + if (!ctx.aux)
> > + return -EINVAL;
> > +
> > + return __bpf_timer_set_callback(timer, (void *)callback_fn, ctx.aux, true);
> > +}
> > +
>
^ permalink raw reply
* Re: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
From: Benjamin Tissoires @ 2024-02-16 9:50 UTC (permalink / raw)
To: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest
In-Reply-To: <mth43jfjhwtatwfo3unefrze62opht3yklleblslyz2adc6p5p@wm3miaqhhtkt>
On Feb 15 2024, Benjamin Tissoires wrote:
> On Feb 14 2024, Benjamin Tissoires wrote:
> > They are implemented as a kfunc, which means a little bit of tweaks in
> > the verifier.
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> >
> > ---
> >
> > changes in v2 (compared to the one attaches to v1 0/9):
> > - make use of a kfunc
> > - add a (non-used) BPF_F_TIMER_SLEEPABLE
> > - the callback is *not* called, it makes the kernel crashes
> > ---
> > include/linux/bpf_verifier.h | 2 +
> > include/uapi/linux/bpf.h | 12 +++++
> > kernel/bpf/helpers.c | 105 ++++++++++++++++++++++++++++++++++++++++---
> > kernel/bpf/verifier.c | 72 ++++++++++++++++++++++++++---
> > 4 files changed, 180 insertions(+), 11 deletions(-)
> >
> > diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
> > index 84365e6dd85d..789ef5fec547 100644
> > --- a/include/linux/bpf_verifier.h
> > +++ b/include/linux/bpf_verifier.h
> > @@ -426,6 +426,7 @@ struct bpf_verifier_state {
> > * while they are still in use.
> > */
> > bool used_as_loop_entry;
> > + bool in_sleepable;
> >
> > /* first and last insn idx of this verifier state */
> > u32 first_insn_idx;
> > @@ -626,6 +627,7 @@ struct bpf_subprog_info {
> > bool is_async_cb: 1;
> > bool is_exception_cb: 1;
> > bool args_cached: 1;
> > + bool is_sleepable: 1;
> >
> > u8 arg_cnt;
> > struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index d96708380e52..0838597028a9 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -7427,6 +7427,18 @@ enum {
> > BPF_F_TIMER_CPU_PIN = (1ULL << 1),
> > };
> >
> > +/* Extra flags to control bpf_timer_init() behaviour, in addition to CLOCK_*.
> > + * - BPF_F_TIMER_SLEEPABLE: Timer will run in a workqueue in a sleepable
> > + * context.
> > + */
> > +enum {
> > + /* CLOCK_* are using bits 0 to 3 */
> > + BPF_F_TIMER_SLEEPABLE = (1ULL << 4),
> > + __MAX_BPF_F_TIMER_INIT,
> > +};
> > +
> > +#define MAX_BPF_F_TIMER_INIT __MAX_BPF_F_TIMER_INIT
> > +
> > /* BPF numbers iterator state */
> > struct bpf_iter_num {
> > /* opaque iterator state; having __u64 here allows to preserve correct
> > diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> > index 4db1c658254c..2dbc09ce841a 100644
> > --- a/kernel/bpf/helpers.c
> > +++ b/kernel/bpf/helpers.c
> > @@ -1097,9 +1097,11 @@ const struct bpf_func_proto bpf_snprintf_proto = {
> > */
> > struct bpf_hrtimer {
> > struct hrtimer timer;
> > + struct work_struct work;
> > struct bpf_map *map;
> > struct bpf_prog *prog;
> > void __rcu *callback_fn;
> > + void __rcu *sleepable_cb_fn;
> > void *value;
> > };
> >
> > @@ -1113,18 +1115,59 @@ struct bpf_timer_kern {
> > struct bpf_spin_lock lock;
> > } __attribute__((aligned(8)));
> >
> > +static void bpf_timer_work_cb(struct work_struct *work)
> > +{
> > + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
> > + struct bpf_map *map = t->map;
> > + void *value = t->value;
> > + bpf_callback_t callback_fn;
> > + void *key;
> > + u32 idx;
> > +
> > + BTF_TYPE_EMIT(struct bpf_timer);
> > +
> > + rcu_read_lock();
> > + callback_fn = rcu_dereference(t->sleepable_cb_fn);
> > + rcu_read_unlock();
> > + if (!callback_fn)
> > + return;
> > +
> > + /* FIXME: do we need any locking? */
> > + if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> > + struct bpf_array *array = container_of(map, struct bpf_array, map);
> > +
> > + /* compute the key */
> > + idx = ((char *)value - array->value) / array->elem_size;
> > + key = &idx;
> > + } else { /* hash or lru */
> > + key = value - round_up(map->key_size, 8);
> > + }
> > +
> > + /* FIXME: this crashes the system with
> > + * BUG: kernel NULL pointer dereference, address: 000000000000000b
> > + */
> > + /* callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0); */
>
> I've found an interesting side effect here.
>
> If I uncomment the line above, I get the following dumpstack:
>
> [ 5.375257] BUG: kernel NULL pointer dereference, address: 000000000000000b
> [ 5.376587] #PF: supervisor instruction fetch in kernel mode
> [ 5.376932] #PF: error_code(0x0010) - not-present page
> [ 5.377249] PGD 1016e6067 P4D 1016e6067 PUD 1016e5067 PMD 0
> [ 5.377602] Oops: 0010 [#1] PREEMPT SMP NOPTI
> [ 5.377876] CPU: 0 PID: 10 Comm: kworker/0:1 Not tainted 6.8.0-rc2-gd2c1e1837606-dirty #285
> [ 5.378378] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.16.3-1.fc39 04/01/2014
> [ 5.378913] Workqueue: events bpf_timer_work_cb
> [ 5.379197] RIP: 0010:0xb
> [ 5.379369] Code: Unable to access opcode bytes at 0xffffffffffffffe1.
> [ 5.379768] RSP: 0018:ffffa5300005be10 EFLAGS: 00010246
> [ 5.380088] RAX: 0000000000000068 RBX: 000000000000000b RCX: 0000000000000000
> [ 5.380523] RDX: ffffa530012a5ff0 RSI: ffffa530012a5fe8 RDI: ffff915ec2bad000
> [ 5.380947] RBP: ffff915ec2bad000 R08: 0000000000000000 R09: 0000000000000001
> [ 5.381368] R10: 0000000000000000 R11: ffffffff9166f820 R12: ffffa530012a5ff0
> [ 5.381793] R13: ffffa530012a5fe8 R14: ffff915ec0073005 R15: ffffffff900aea8a
> [ 5.382213] FS: 0000000000000000(0000) GS:ffff915efbc00000(0000) knlGS:0000000000000000
> [ 5.382691] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 5.383036] CR2: ffffffffffffffe1 CR3: 000000010761e003 CR4: 0000000000770ef0
> [ 5.383460] PKRU: 55555554
> [ 5.383643] Call Trace:
> [ 5.383831] <TASK>
> [ 5.383994] ? __die+0x23/0x70
> [ 5.384228] ? page_fault_oops+0x181/0x4a0
> [ 5.384541] ? exc_page_fault+0x67/0x1a0
> [ 5.384840] ? asm_exc_page_fault+0x26/0x30
> [ 5.385154] ? process_one_work+0x16a/0x4b0
> [ 5.385467] ? bpf_timer_work_cb+0xfc/0x240
> [ 5.385789] ? process_one_work+0x1d4/0x4b0
> [ 5.386101] ? worker_thread+0x1d5/0x3c0
> [ 5.386393] ? __pfx_worker_thread+0x10/0x10
> [ 5.386724] ? kthread+0xf5/0x130
> [ 5.386974] ? __pfx_kthread+0x10/0x10
> [ 5.387255] ? ret_from_fork+0x31/0x50
> [ 5.387537] ? __pfx_kthread+0x10/0x10
> [ 5.387820] ? ret_from_fork_asm+0x1b/0x30
> [ 5.388127] </TASK>
>
> And it looks like the callback is properly called, but the fault comes
> after:
>
> (gdb) list *(process_one_work+0x16a)
> 0xffffffff810aea8a is in process_one_work (kernel/workqueue.c:2606).
> 2601 * disabled.
> 2602 */
> 2603 set_work_pool_and_clear_pending(work, pool->id);
> 2604
> 2605 pwq->stats[PWQ_STAT_STARTED]++;
> 2606 raw_spin_unlock_irq(&pool->lock);
> 2607
> 2608 lock_map_acquire(&pwq->wq->lockdep_map);
> 2609 lock_map_acquire(&lockdep_map);
> 2610 /*
>
> And there is no real reason for the spinlock to not be set unless there
> is a memroy corruption.
>
> And if in my program I call bpf_timer_set_callback() before bpf_timer_set_sleepable_cb()
> like:
> bpf_timer_set_callback(timer, timer_cb1);
> bpf_timer_set_sleepable_cb(timer, timer_cb1);
>
> I do not see the corruption in the memory.
>
> So it seems that when BPF calls the helper, the callback_fn is set in a
> mode that it is not polluting the memory, while when calling the function
> pointer from a kfunc, something is not properly set and there is a
> memory corruption.
>
> Does that rings any bell?
Sigh... I found the issue, it was in my code: basically push_callback_call()
was never called because I called it from the helper validation call,
not the kfunc one :(
---
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index cb1266566b69..58082df468e5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -501,6 +501,7 @@ static bool is_dynptr_ref_function(enum bpf_func_id func_id)
}
static bool is_sync_callback_calling_kfunc(u32 btf_id);
+static bool is_callback_calling_kfunc(u32 btf_id);
static bool is_bpf_throw_kfunc(struct bpf_insn *insn);
static bool is_bpf_timer_set_sleepable_cb_kfunc(u32 btf_id);
@@ -9452,7 +9453,7 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
*/
env->subprog_info[subprog].is_cb = true;
if (bpf_pseudo_kfunc_call(insn) &&
- !is_sync_callback_calling_kfunc(insn->imm)) {
+ !is_callback_calling_kfunc(insn->imm)) {
verbose(env, "verifier bug: kfunc %s#%d not marked as callback-calling\n",
func_id_name(insn->imm), insn->imm);
return -EFAULT;
@@ -10440,10 +10441,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
break;
}
- if (is_bpf_timer_set_sleepable_cb_kfunc(func_id))
- err = push_callback_call(env, insn, insn_idx, meta.subprogno,
- set_timer_callback_state);
-
if (err)
return err;
@@ -11370,6 +11367,12 @@ static bool is_bpf_timer_set_sleepable_cb_kfunc(u32 btf_id)
return btf_id == special_kfunc_list[KF_bpf_timer_set_sleepable_cb];
}
+static bool is_callback_calling_kfunc(u32 btf_id)
+{
+ return is_sync_callback_calling_kfunc(btf_id) ||
+ is_bpf_timer_set_sleepable_cb_kfunc(btf_id);
+}
+
static bool is_rbtree_lock_required_kfunc(u32 btf_id)
{
return is_bpf_rbtree_api_kfunc(btf_id);
@@ -12140,6 +12143,16 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
}
}
+ if (is_bpf_timer_set_sleepable_cb_kfunc(meta.func_id)) {
+ err = push_callback_call(env, insn, insn_idx, meta.subprogno,
+ set_timer_callback_state);
+ if (err) {
+ verbose(env, "kfunc %s#%d failed callback verification\n",
+ func_name, meta.func_id);
+ return err;
+ }
+ }
+
rcu_lock = is_kfunc_bpf_rcu_read_lock(&meta);
rcu_unlock = is_kfunc_bpf_rcu_read_unlock(&meta);
---
[...]
Cheers,
Benjamin
^ permalink raw reply related
* Regression with Lenovo ThinkPad Compact USB Keyboard
From: Raphaël Halimi @ 2024-02-16 11:51 UTC (permalink / raw)
To: Linux Stable Mailing List
Cc: Linux Regressions Mailing List, Linux Input Mailing List,
Dmitry Torokhov, Jiri Kosina, Benjamin Tissoires,
Mikhail Khvainitski
Dear developers,
(sorry for the long CC list, it looks quite long to me, but I tried to
follow the issue reporting guide as closely as possible)
Since patches [1], [2] and [3] were applied to the kernel, there is a
regression with Lenovo ThinkPad Compact USB Keyboard (old model, not II).
[1]
https://github.com/torvalds/linux/commit/46a0a2c96f0f47628190f122c2e3d879e590bcbe
[2]
https://github.com/torvalds/linux/commit/2f2bd7cbd1d1548137b351040dc4e037d18cdfdc
[3]
https://github.com/torvalds/linux/commit/43527a0094c10dfbf0d5a2e7979395a38de3ff65
The regression is that a middle click is performed when releasing middle
button after wheel emulation.
The bug appears randomly, it can be after 5 minutes or 1 hour of
keyboard usage, and can only be worked around by unplugging/re-plugging
the keyboard. (I ended up resorting to simulate an unplug/replug, with a
script which echoes 0 then 1 to /sys/bus/usb/devices/<id>/authorized,
since I was afraid to damage the Micro-USB outlet by physically
unplugging/re-plugging too much).
Those spurious clicks are very annoying, since they can open links in
new tabs when scrolling in Firefox, or pasting text when scrolling in
terminals, or other unwanted stuff.
I witnessed it with latest kernels (Debian unstable) as well as stable
kernels (Debian 12 Bookworm, stable).
On Debian Stable, the last working kernel was 5.10.127, the regression
appeared in 5.10.136 (i read all changelogs on kernel.org between those
two releases but couldn't find anything about hid-lenovo, so I can't
tell exactly in which release the regression appeared, Debian upgraded
directly from .127 to .136).
I reported it in Debian [4], and apparently I'm not the only person
suffering from it [5].
[4] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1058758#32
[5] https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1058758#42
I would understand that such bugs would end up in a development kernel
like the ones provided by Debian Unstable, but not with stable kernels
like the ones provided by Debian Stable.
Regards,
--
Raphaël Halimi
^ permalink raw reply
* Re: [PATCH 0/4] leds: trigger: Improve handling of led_trigger_event() and simplify mute audio trigger
From: Heiner Kallweit @ 2024-02-16 12:18 UTC (permalink / raw)
To: Takashi Iwai
Cc: Pavel Machek, Lee Jones, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer, linux-leds@vger.kernel.org,
linux-sound, open list:HID CORE LAYER, linux-mips
In-Reply-To: <87zfw1ewrd.wl-tiwai@suse.de>
On 15.02.2024 13:29, Takashi Iwai wrote:
> On Tue, 13 Feb 2024 08:30:30 +0100,
> Heiner Kallweit wrote:
>>
>> If a simple trigger is assigned to a LED, then the LED may be off until
>> the next led_trigger_event() call. This may be an issue for simple
>> triggers with rare led_trigger_event() calls, e.g. power supply
>> charging indicators (drivers/power/supply/power_supply_leds.c).
>> Therefore persist the brightness value of the last led_trigger_event()
>> call and use this value if the trigger is assigned to a LED.
>> This change allows to use simple triggers in more cases.
>> As a first use case simplify handling of the mute audio trigger.
>>
>> This series touches few subsystems. I'd propose to handle it via
>> the LED subsystem.
>>
>> Heiner Kallweit (4):
>> leds: trigger: Store brightness set by led_trigger_event()
>> ALSA: control-led: Integrate mute led trigger
>> Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER
>> leds: trigger: audio: Remove this trigger
>
> LGTM.
>
> Reviewed-by: Takashi Iwai <tiwai@suse.de>
>
> One thing I'm not 100% sure is the movement from ledtrig:audio-mute
> and ledtrig:audio-micmute alias into snd-ctl-led module. Who would
> use/process those aliases? I don't think this would be a problem, but
> it might change the loading order.
>
The ledtrig:% aliases are used when a LED device is registered that has
a default trigger. Like in the case here with the input leds (patch 3).
There might also be DT-defined LEDs with a audio mute default trigger.
snd-ctl-led has a dependency on snd, so at least wrt snd the load order
doesn't change.
>
> Thanks!
>
> Takashi
Heiner
^ permalink raw reply
* Re: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
From: Toke Høiland-Jørgensen @ 2024-02-16 14:18 UTC (permalink / raw)
To: Benjamin Tissoires, Martin KaFai Lau
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Eduard Zingerman, Song Liu, Yonghong Song,
KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa, Jiri Kosina,
Benjamin Tissoires, Jonathan Corbet, Shuah Khan
In-Reply-To: <r3yhu4h23tdg2dqj7eq3lhevsigvvb3qkge3icxmaqpgkayvoi@gxfxstkr2pxl>
Benjamin Tissoires <bentiss@kernel.org> writes:
> On Feb 15 2024, Martin KaFai Lau wrote:
>> On 2/14/24 9:18 AM, Benjamin Tissoires wrote:
>> > +static void bpf_timer_work_cb(struct work_struct *work)
>> > +{
>> > + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
>> > + struct bpf_map *map = t->map;
>> > + void *value = t->value;
>> > + bpf_callback_t callback_fn;
>> > + void *key;
>> > + u32 idx;
>> > +
>> > + BTF_TYPE_EMIT(struct bpf_timer);
>> > +
>> > + rcu_read_lock();
>> > + callback_fn = rcu_dereference(t->sleepable_cb_fn);
>> > + rcu_read_unlock();
>>
>> I took a very brief look at patch 2. One thing that may worth to ask here,
>> the rcu_read_unlock() seems to be done too early. It is protecting the
>> t->sleepable_cb_fn (?), so should it be done after finished using the
>> callback_fn?
>
> Probably :)
>
> TBH, everytime I work with RCUs I spent countless hours trying to
> re-understand everything, and in this case I'm currently in the "let's
> make it work" process than fixing concurrency issues.
> I still gave it a shot in case it solves my issue, but no, I still have
> the crash.
>
> But given that callback_fn might sleep, isn't it an issue to keep the
> RCU_reader lock so long? (we don't seem to call synchronize_rcu() so it
> might be fine, but I'd like the confirmation from someone else).
You're right, it isn't. From the RCU/checklist.rst doc:
13. Unlike most flavors of RCU, it *is* permissible to block in an
SRCU read-side critical section (demarked by srcu_read_lock()
and srcu_read_unlock()), hence the "SRCU": "sleepable RCU".
Please note that if you don't need to sleep in read-side critical
sections, you should be using RCU rather than SRCU, because RCU
is almost always faster and easier to use than is SRCU.
So we can't use the regular RCU protection for the callback in this
usage. We'll need to either convert it to SRCU, or add another
protection mechanism to make sure the callback function is not freed
from under us (like a refcnt). I suspect the latter may be simpler (from
reading the rest of that documentation around SRCU.
>> A high level design question. The intention of the new
>> bpf_timer_set_sleepable_cb() kfunc is actually to delay work to a workqueue.
>> It is useful to delay work from the bpf_timer_cb and it may also useful to
>> delay work from other bpf running context (e.g. the networking hooks like
>> "tc"). The bpf_timer_set_sleepable_cb() seems to be unnecessary forcing
>> delay-work must be done in a bpf_timer_cb.
>
> Basically I'm just a monkey here. I've been told that I should use
> bpf_timer[0]. But my implementation is not finished, as Alexei mentioned
> that we should bypass hrtimer if I'm not wrong [1].
I don't think getting rid of the hrtimer in favour of
schedule_delayed_work() makes any sense. schedule_delayed_work() does
exactly the same as you're doing in this version of the patch: it
schedules a timer callback, and calls queue_work() from inside that
timer callback. It just uses "regular" timers instead of hrtimers. So I
don't think there's any performance benefit from using that facility; on
the contrary, it would require extra logic to handle cancellation etc;
might as well just re-use the existing hrtimer-based callback logic we
already have, and do a schedule_work() from the hrtimer callback like
you're doing now.
-Toke
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox