Re: [PATCH bpf-next v2 00/28] Introduce eBPF support for HID devices

On Sat, Mar 5, 2022 at 2:23 AM Benjamin Tissoires
[off-list ref] wrote:

quoted

quoted

The set looks good so far. I will review the rest later.

[...]

A quick note about how we organize these patches. Maybe we can
merge some of these patches like:

Just to be sure we are talking about the same thing: you mean squash
the patch together?

Right, squash some patches together.

quoted

quoted

  bpf: introduce hid program type
  bpf/hid: add a new attach type to change the report descriptor
  bpf/hid: add new BPF type to trigger commands from userspace

I guess the three can merge into one.

quoted

  HID: hook up with bpf
  HID: allow to change the report descriptor from an eBPF program
  HID: bpf: compute only the required buffer size for the device
  HID: bpf: only call hid_bpf_raw_event() if a ctx is available

I haven't read through all of them, but I guess they can probably merge
as well.

There are certainly patches that we could squash together (3 and 4
from this list into the previous ones), but I'd like to keep some sort
of granularity here to not have a patch bomb that gets harder to come
back later.

Totally agreed with the granularity of patches. I am not a big fan of patch
bombs either. :)

I guess the problem I have with the current version is that I don't have a
big picture of the design while reading through relatively big patches. A
overview with the following information in the cover letter would be really
help here:
  1. How different types of programs are triggered (IRQ, user input, etc.);
  2. What are the operations and/or outcomes of these programs;
  3. How would programs of different types (or attach types) interact
   with each other (via bpf maps? chaining?)
  4. What's the new uapi;
  5. New helpers and other logistics

Sometimes, I find the changes to uapi are the key for me to understand the
patches, and I would like to see one or two patches with all the UAPI
changes (i.e. bpf_hid_attach_type). However, that may or may not apply to
this set due to granularity concerns.

Does this make sense?

Thanks,
Song

quoted

quoted

  libbpf: add HID program type and API
  libbpf: add new attach type BPF_HID_RDESC_FIXUP
  libbpf: add new attach type BPF_HID_USER_EVENT

There 3 can merge, and maybe also the one below

quoted

  libbpf: add handling for BPF_F_INSERT_HEAD in HID programs

Yeah, the libbpf changes are small enough to not really justify
separate patches.

quoted

quoted

  samples/bpf: add new hid_mouse example
  samples/bpf: add a report descriptor fixup
  samples/bpf: fix bpf_program__attach_hid() api change

Maybe it makes sense to merge these 3?

Sure, why not.

quoted

quoted

  bpf/hid: add hid_{get|set}_data helpers
  HID: bpf: implement hid_bpf_get|set_data
  bpf/hid: add bpf_hid_raw_request helper function
  HID: add implementation of bpf_hid_raw_request

We can have 1 or 2 patches for these helpers

OK, the patches should be self-contained enough.

quoted

quoted

  selftests/bpf: add tests for the HID-bpf initial implementation
  selftests/bpf: add report descriptor fixup tests
  selftests/bpf: add tests for hid_{get|set}_data helpers
  selftests/bpf: add test for user call of HID bpf programs
  selftests/bpf: hid: rely on uhid event to know if a test device is
    ready
  selftests/bpf: add tests for bpf_hid_hw_request
  selftests/bpf: Add a test for BPF_F_INSERT_HEAD

These selftests could also merge into 1 or 2 patches I guess.

I'd still like to link them to the granularity of the bpf changes, so
I can refer a selftest change to a specific commit/functionality
added. But that's just my personal taste, and I can be convinced
otherwise. This should give us maybe 4 patches instead of 7.

quoted

I understand rearranging these patches may take quite some effort.
But I do feel that's a cleaner approach (from someone doesn't know
much about HID). If you really hate it that way, we can discuss...

No worries. I don't mind iterating on the series. IIRC I already
rewrote it twice from scratch, and that's when the selftests I
introduced in the second rewrite were tremendously helpful :) And
honestly I don't think it'll be too much effort to reorder/squash the
patches given that the v2 is *very* granular.

Anyway, I prefer having the reviewers happy so we can have a solid
rock API from day 1 than keeping it obscure for everyone and having to
deal with design issues forever. So if it takes 10 or 20 revisions to
have everybody on the same page, that's fine with me (not that I want
to have that many revisions, just that I won't be afraid of the
bikeshedding we might have at some point).

Cheers,
Benjamin