Received-SPF: pass (google.com: domain of linux-kernel+bounces-214630-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Fri, 14 Jun 2024 11:25:25 +0200
From: Benjamin Tissoires <bentiss@kernel.org>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Shuah Khan <shuah@kernel.org>, Jiri Kosina <jikos@kernel.org>, 
	Jonathan Corbet <corbet@lwn.net>, Alexei Starovoitov <ast@kernel.org>, 
	"open list:KERNEL SELFTEST FRAMEWORK" <linux-kselftest@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>, 
	"open list:HID CORE LAYER" <linux-input@vger.kernel.org>, "open list:DOCUMENTATION" <linux-doc@vger.kernel.org>
Subject: Re: [PATCH HID v3 03/16] HID: bpf: implement HID-BPF through
 bpf_struct_ops
Message-ID: <toq56n25r3jkxbnea6g5wxj3oc4zpgqbor5qjzg444uyh3bx3a@zh4sepdjrwob>
References: <20240608-hid_bpf_struct_ops-v3-0-6ac6ade58329@kernel.org>
 <20240608-hid_bpf_struct_ops-v3-3-6ac6ade58329@kernel.org>
 <CAADnVQJDZju=QMiAH=TpYHr6KQh3-TT-jNtJ-O2u7JstY5486Q@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAADnVQJDZju=QMiAH=TpYHr6KQh3-TT-jNtJ-O2u7JstY5486Q@mail.gmail.com>

On Jun 10 2024, Alexei Starovoitov wrote:
> On Sat, Jun 8, 2024 at 2:01 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> >
> > +
> > +static int hid_bpf_ops_init_member(const struct btf_type *t,
> > +                                const struct btf_member *member,
> > +                                void *kdata, const void *udata)
> > +{
> > +       u32 moff = __btf_member_bit_offset(t, member) / 8;
> > +       u32 flags;
> > +
> > +       switch (moff) {
> > +       case offsetof(struct hid_bpf_ops, hid_id):
> > +               /* For hid_id and flags fields, this function has to copy it
> > +                * and return 1 to indicate that the data has been handled by
> > +                * the struct_ops type, or the verifier will reject the map if
> > +                * the value of those fields is not zero.
> > +                */
> > +               ((struct hid_bpf_ops *)kdata)->hid_id = ((struct hid_bpf_ops *)udata)->hid_id;
> > +               return 1;
> > +       case offsetof(struct hid_bpf_ops, flags):
> > +               flags = ((struct hid_bpf_ops *)udata)->flags;
> > +               if (flags & ~BPF_F_BEFORE)
> > +                       return -EINVAL;
> > +               ((struct hid_bpf_ops *)kdata)->flags = flags;
> 
> minor nit: I'd cast kdata/udate in the beginning of
> the function to make the lines shorter and less verbose.
> Similar to how bpf_tcp_ca_init_member() does it.

The change looks like:

---
diff --git a/drivers/hid/bpf/hid_bpf_struct_ops.c b/drivers/hid/bpf/hid_bpf_struct_ops.c
index 9192c66cde20..dde547e734ed 100644
--- a/drivers/hid/bpf/hid_bpf_struct_ops.c
+++ b/drivers/hid/bpf/hid_bpf_struct_ops.c
@@ -97,8 +97,14 @@ static int hid_bpf_ops_init_member(const struct btf_type *t,
                                 const struct btf_member *member,
                                 void *kdata, const void *udata)
 {
-       u32 moff = __btf_member_bit_offset(t, member) / 8;
-       u32 flags;
+       const struct hid_bpf_ops *uhid_bpf_ops;
+       struct hid_bpf_ops *khid_bpf_ops;
+       u32 moff;
+
+       uhid_bpf_ops = (const struct hid_bpf_ops *)udata;
+       khid_bpf_ops = (struct hid_bpf_ops *)kdata;
+
+       moff = __btf_member_bit_offset(t, member) / 8;
 
        switch (moff) {
        case offsetof(struct hid_bpf_ops, hid_id):
@@ -107,13 +113,12 @@ static int hid_bpf_ops_init_member(const struct btf_type *t,
                 * the struct_ops type, or the verifier will reject the map if
                 * the value of those fields is not zero.
                 */
-               ((struct hid_bpf_ops *)kdata)->hid_id = ((struct hid_bpf_ops *)udata)->hid_id;
+               khid_bpf_ops->hid_id = uhid_bpf_ops->hid_id;
                return 1;
        case offsetof(struct hid_bpf_ops, flags):
-               flags = ((struct hid_bpf_ops *)udata)->flags;
-               if (flags & ~BPF_F_BEFORE)
+               if (uhid_bpf_ops->flags & ~BPF_F_BEFORE)
                        return -EINVAL;
-               ((struct hid_bpf_ops *)kdata)->flags = flags;
+               khid_bpf_ops->flags = uhid_bpf_ops->flags;
                return 1;
        }
        return 0;
---

> 
> Acked-by: Alexei Starovoitov <ast@kernel.org>

And I'm going to apply the series now

Cheers,
Benjamin