On Tue, Mar 15, 2022 at 5:51 PM Tero Kristo <tero.kristo@xxxxxxxxxxxxxxx> wrote:
>
> Hi Benjamin,
>
> On 04/03/2022 19:28, Benjamin Tissoires wrote:
> > Simple test added here, with one use of each helper.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> >
> > ---
> >
> > changes in v2:
> > - split the patch with libbpf left outside.
> > ---
> > tools/testing/selftests/bpf/prog_tests/hid.c | 65 ++++++++++++++++++++
> > tools/testing/selftests/bpf/progs/hid.c | 45 ++++++++++++++
> > 2 files changed, 110 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/prog_tests/hid.c b/tools/testing/selftests/bpf/prog_tests/hid.c
> > index 91543b8078ca..74426523dd6f 100644
> > --- a/tools/testing/selftests/bpf/prog_tests/hid.c
> > +++ b/tools/testing/selftests/bpf/prog_tests/hid.c
> > @@ -297,6 +297,68 @@ static int test_hid_raw_event(struct hid *hid_skel, int uhid_fd, int sysfs_fd)
> > return ret;
> > }
> >
> > +/*
> > + * Attach hid_set_get_data to the given uhid device,
> > + * retrieve and open the matching hidraw node,
> > + * inject one event in the uhid device,
> > + * check that the program makes correct use of bpf_hid_{set|get}_data.
> > + */
> > +static int test_hid_set_get_data(struct hid *hid_skel, int uhid_fd, int sysfs_fd)
> > +{
> > + int err, hidraw_ino, hidraw_fd = -1;
> > + char hidraw_path[64] = {0};
> > + u8 buf[10] = {0};
> > + int ret = -1;
> > +
> > + /* attach hid_set_get_data program */
> > + hid_skel->links.hid_set_get_data =
> > + bpf_program__attach_hid(hid_skel->progs.hid_set_get_data, sysfs_fd);
> > + if (!ASSERT_OK_PTR(hid_skel->links.hid_set_get_data,
> > + "attach_hid(hid_set_get_data)"))
> > + return PTR_ERR(hid_skel->links.hid_set_get_data);
> > +
> > + hidraw_ino = get_hidraw(hid_skel->links.hid_set_get_data);
> > + if (!ASSERT_GE(hidraw_ino, 0, "get_hidraw"))
> > + goto cleanup;
> > +
> > + /* open hidraw node to check the other side of the pipe */
> > + sprintf(hidraw_path, "/dev/hidraw%d", hidraw_ino);
> > + hidraw_fd = open(hidraw_path, O_RDWR | O_NONBLOCK);
> > +
> > + if (!ASSERT_GE(hidraw_fd, 0, "open_hidraw"))
> > + goto cleanup;
> > +
> > + /* inject one event */
> > + buf[0] = 1;
> > + buf[1] = 42;
> > + send_event(uhid_fd, buf, 6);
> > +
> > + /* read the data from hidraw */
> > + memset(buf, 0, sizeof(buf));
> > + err = read(hidraw_fd, buf, sizeof(buf));
> > + if (!ASSERT_EQ(err, 6, "read_hidraw"))
> > + goto cleanup;
> > +
> > + if (!ASSERT_EQ(buf[2], (42 >> 2), "hid_set_get_data"))
> > + goto cleanup;
> > +
> > + if (!ASSERT_EQ(buf[3], 1, "hid_set_get_data"))
> > + goto cleanup;
> > +
> > + if (!ASSERT_EQ(buf[4], 42, "hid_set_get_data"))
> > + goto cleanup;
> > +
> > + ret = 0;
> > +
> > +cleanup:
> > + if (hidraw_fd >= 0)
> > + close(hidraw_fd);
> > +
> > + hid__detach(hid_skel);
> > +
> > + return ret;
> > +}
> > +
> > /*
> > * Attach hid_rdesc_fixup to the given uhid device,
> > * retrieve and open the matching hidraw node,
> > @@ -395,6 +457,9 @@ void serial_test_hid_bpf(void)
> > err = test_hid_raw_event(hid_skel, uhid_fd, sysfs_fd);
> > ASSERT_OK(err, "hid");
> >
> > + err = test_hid_set_get_data(hid_skel, uhid_fd, sysfs_fd);
> > + ASSERT_OK(err, "hid_set_get_data");
> > +
> > err = test_rdesc_fixup(hid_skel, uhid_fd, sysfs_fd);
> > ASSERT_OK(err, "hid_rdesc_fixup");
> >
> > diff --git a/tools/testing/selftests/bpf/progs/hid.c b/tools/testing/selftests/bpf/progs/hid.c
> > index 2270448d0d3f..de6668471940 100644
> > --- a/tools/testing/selftests/bpf/progs/hid.c
> > +++ b/tools/testing/selftests/bpf/progs/hid.c
> > @@ -66,3 +66,48 @@ int hid_rdesc_fixup(struct hid_bpf_ctx *ctx)
> >
> > return 0;
> > }
> > +
> > +SEC("hid/device_event")
> > +int hid_set_get_data(struct hid_bpf_ctx *ctx)
> > +{
> > + int ret;
> > + __u8 *buf;
> > +
> > + buf = bpf_ringbuf_reserve(&ringbuf, 8, 0);
>
> Ordering of patches is probably wrong, it seems the ringbuf is defined
> in patch #21 but used here.
>
> Also, this usage of ringbuf leads into running out of available memory
> in the buffer if used for long time, it is not evident from the test
> case written here but I spent a couple of hours debugging my own BPF
> program that used ringbuf in similar way as what is done here. Basically
> the producer idx is increased with the bpf_ringbuf_reserve / discard,
> but the consumer index is not if you don't have a consumer in place.
Oh, that's good to know.
FWIW, on v3, we won't have to use a ringbuf at all. This is a new API
break (sorry for that), but I think this will be better in the long
run.
I have chosen to use the hid_bpf_get_data() from [0]. The net
advantage is that we get a pointer to the internal buffer that can be
of any size, and we can even re-inject that same pointer into
bpf_hid_raw_request() without having to use another buffer.
I'm currently working on the documentation part and small fixes now
that the big rewrite with that implementation is now done.
Cheers,
Benjamin
>
> I ended up using a global statically allocated buffer for the purpose
> for now.
>
> -Tero
>
[0] https://lore.kernel.org/linux-input/CAO-hwJJqP5iivQZOu0LTYa1D5OuM_aVi=LH27Udc_VYkbFsrww@xxxxxxxxxxxxxx/
