Date:   Mon, 7 Mar 2022 10:11:51 -0800
From:   Song Liu <song@...nel.org>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     Greg KH <gregkh@...uxfoundation.org>,
        Jiri Kosina <jikos@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <kafai@...com>,
        Song Liu <songliubraving@...com>, Yonghong Song <yhs@...com>,
        John Fastabend <john.fastabend@...il.com>,
        KP Singh <kpsingh@...nel.org>, Shuah Khan <shuah@...nel.org>,
        Dave Marchevsky <davemarchevsky@...com>,
        Joe Stringer <joe@...ium.io>,
        Tero Kristo <tero.kristo@...ux.intel.com>,
        open list <linux-kernel@...r.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>, bpf <bpf@...r.kernel.org>,
        linux-kselftest@...r.kernel.org
Subject: Re: [PATCH bpf-next v2 00/28] Introduce eBPF support for HID devices

On Sat, Mar 5, 2022 at 2:23 AM Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
> > >
> >
> > 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.

>
> >
> > >   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.
> >
> > >   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




>
> >
> > >   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
> > >   libbpf: add handling for BPF_F_INSERT_HEAD in HID programs
>
> Yeah, the libbpf changes are small enough to not really justify
> separate patches.
>
> >
> > >   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.
>
> >
> > >   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.
>
> >
> > >   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.
>
> >
> > 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
>

Powered by blists - more mailing lists