Re: [PATCH bpf-next v6 12/23] HID: initial BPF implementation

[Greg KH]

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@xxxxxxxxxxxxxxxxxxx> 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