lkml.org 
[lkml]   [2022]   [Mar]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH bpf-next v2 14/28] selftests/bpf: add tests for hid_{get|set}_data helpers
    On Tue, Mar 15, 2022 at 5:51 PM Tero Kristo <tero.kristo@linux.intel.com> 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@redhat.com>
    > >
    > > ---
    > >
    > > 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@mail.gmail.com/

    \
     
     \ /
      Last update: 2022-09-17 16:17    [W:2.692 / U:0.016 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site