Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220712145850.599666-1-benjamin.tissoires@redhat.com>
 <20220712145850.599666-13-benjamin.tissoires@redhat.com> <YtD09KwkxvJAbgCy@kroah.com>
In-Reply-To: <YtD09KwkxvJAbgCy@kroah.com>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Fri, 15 Jul 2022 11:56:46 +0200
Message-ID: <CAO-hwJ+d6mNO2L5kZtOC6QVrDy+LZ6ECoY2f83C93GFPKbSx7g@mail.gmail.com>
Subject: Re: [PATCH bpf-next v6 12/23] HID: initial BPF implementation
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     Jiri Kosina <jikos@kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Andrii Nakryiko <andrii@kernel.org>,
        Martin KaFai Lau <kafai@fb.com>,
        Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>,
        John Fastabend <john.fastabend@gmail.com>,
        KP Singh <kpsingh@kernel.org>, Shuah Khan <shuah@kernel.org>,
        Dave Marchevsky <davemarchevsky@fb.com>,
        Joe Stringer <joe@cilium.io>, Jonathan Corbet <corbet@lwn.net>,
        Tero Kristo <tero.kristo@linux.intel.com>,
        lkml <linux-kernel@vger.kernel.org>,
        "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@vger.kernel.org>,
        Linux Doc Mailing List <linux-doc@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Jul 15, 2022 at 7:02 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Tue, Jul 12, 2022 at 04:58:39PM +0200, Benjamin Tissoires wrote:
> > --- /dev/null
> > +++ b/drivers/hid/bpf/Kconfig
> > @@ -0,0 +1,19 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
> > +menu "HID-BPF support"
> > +     #depends on x86_64
> > +
> > +config HID_BPF
> > +     bool "HID-BPF support"
> > +     default y
>
> Things are only default y if you can't boot your machine without it.
> Perhaps just mirror what HID is to start with and do not select HID?
>
> > +     depends on BPF && BPF_SYSCALL
> > +     select HID
>
> select is rough, why not depend?

Let me try to explain this mess, maybe you can give me the piece that
I am missing:

The requirements I have (or want) are:
- HID-BPF should be "part" of HID-core (or something similar of "part"):
  I intend to have device fixes as part of the regular HID flow, so
allowing distros to opt out seems a little bit dangerous
- the HID tree is not as clean as some other trees:
  drivers/hid/ sees both core elements and leaf drivers
  transport layers are slightly better, they are in their own
subdirectories, but some transport layers are everywhere in the kernel
code or directly in drivers/hid (uhid and hid-logitech-dj for
instance)
- HID can be loaded as a module (only ubuntu is using that), and this
is less and less relevant because of all of the various transport
layers we have basically prevent a clean unloading of the module

These made me think that I should have a separate bpf subdir for
HID-BPF, to keep things separated, which means I can not include
HID-BPF in hid.ko directly, it goes into a separate driver. And then I
have a chicken and egg problem:
- HID-core needs to call functions from HID-BPF (to hook into it)
- but HID-BPF needs to also call functions from HID-core (for
accessing HID internals)

I have solved that situation with struct hid_bpf_ops but it is not the
cleanest possible way.

And that's also why I did "select HID", because HID-BPF without HID is
pointless.

One last bit I should add. hid-bpf.ko should be allowed to be compiled
in as a module, but I had issues at boot because kfuncs were not
getting registered properly (though it works for the net test driver).
So I decided to make hid-bpf a boolean instead of a tristate.

As I type all of this, I am starting to wonder if I should not tackle
the very first point and separate hid-core in its own subdir. This way
I can have a directory with only the core part, and having hid-bpf in
here wouldn't be too much of an issue.

Thoughts?

Cheers,
Benjamin