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.
