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;
Date:   Fri, 15 Jul 2022 13:43:49 +0200
From:   Greg KH <gregkh@linuxfoundation.org>
To:     Benjamin Tissoires <benjamin.tissoires@redhat.com>
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>
Subject: Re: [PATCH bpf-next v6 12/23] HID: initial BPF implementation
Message-ID: <YtFS9UeLF8ZefT/F@kroah.com>
References: <20220712145850.599666-1-benjamin.tissoires@redhat.com>
 <20220712145850.599666-13-benjamin.tissoires@redhat.com>
 <YtD09KwkxvJAbgCy@kroah.com>
 <CAO-hwJ+d6mNO2L5kZtOC6QVrDy+LZ6ECoY2f83C93GFPKbSx7g@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAO-hwJ+d6mNO2L5kZtOC6QVrDy+LZ6ECoY2f83C93GFPKbSx7g@mail.gmail.com>
Precedence: bulk

On Fri, Jul 15, 2022 at 11:56:46AM +0200, Benjamin Tissoires wrote:
> 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.

We've had this problem with the USB core in the past, and yes, that was
the simplest solution (see drivers/usb/core/)

Otherwise you could do:
	default HID
as the dependancy here, but that might get messy if hid can be a module.

Try splitting the hid core out first, you want to do that anyway and
that should make this simpler as you found out :)

thanks,

greg k-h