Re: [PATCH HID v2 03/16] HID: bpf: implement HID-BPF through bpf_struct_ops

[Alexei Starovoitov]

On Fri, Jun 7, 2024 at 8:28 AM Benjamin Tissoires <bentiss@xxxxxxxxxx> wrote:
> +struct hid_bpf_ops {
> +       /* hid_id needs to stay first so we can easily change it
> +        * from userspace.
> +        */
> +       int                     hid_id;
> +       u32                     flags;
> +
> +       /* private: internal use only */
> +       struct list_head        list;
> +
> +       /* public: rest is public */

Didn't notice it before, but the above comments are misleading.
The whole struct is private to the kernel and bpf prog, while
registering, can only touch a handful.
I'd drop "internal use" and "is public". It's not an uapi.

> +
> +/* fast path fields are put first to fill one cache line */

Also misleading. The whole struct fits one cache line.

> +
> +       /**
> +        * @hid_device_event: called whenever an event is coming in from the device
> +        *
> +        * It has the following arguments:
> +        *
> +        * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> +        *
> +        * Return: %0 on success and keep processing; a positive
> +        * value to change the incoming size buffer; a negative
> +        * error code to interrupt the processing of this event
> +        *
> +        * Context: Interrupt context.
> +        */
> +       int (*hid_device_event)(struct hid_bpf_ctx *ctx, enum hid_report_type report_type);
> +
> +/* control/slow paths put last */
> +
> +       /**
> +        * @hid_rdesc_fixup: called when the probe function parses the report descriptor
> +        * of the HID device
> +        *
> +        * It has the following arguments:
> +        *
> +        * ``ctx``: The HID-BPF context as &struct hid_bpf_ctx
> +        *
> +        * Return: %0 on success and keep processing; a positive
> +        * value to change the incoming size buffer; a negative
> +        * error code to interrupt the processing of this device
> +        */
> +       int (*hid_rdesc_fixup)(struct hid_bpf_ctx *ctx);
> +
> +       /* private: internal use only */
> +       struct hid_device *hdev;
> +} ____cacheline_aligned_in_smp;

Such alignment is an overkill.
I don't think you can measure the difference.

> +
>  struct hid_bpf_prog_list {
>         u16 prog_idx[HID_BPF_MAX_PROGS_PER_DEV];
>         u8 prog_cnt;
> @@ -129,6 +188,10 @@ struct hid_bpf {
>         bool destroyed;                 /* prevents the assignment of any progs */
>
>         spinlock_t progs_lock;          /* protects RCU update of progs */
> +
> +       struct hid_bpf_ops *rdesc_ops;
> +       struct list_head prog_list;
> +       struct mutex prog_list_lock;    /* protects RCU update of prog_list */

mutex protects rcu update... sounds very odd.
Just say that mutex protects prog_list update, because "RCU update"
has a different meaning. RCU logic itself is what protects Update part of rcU.

The rest looks good.