MIME-Version: 1.0
References: <20211126130141.1811848-1-tero.kristo@linux.intel.com>
In-Reply-To: <20211126130141.1811848-1-tero.kristo@linux.intel.com>
From:   Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date:   Tue, 30 Nov 2021 15:44:56 +0100
Message-ID: <CAO-hwJLq6Jnvos=CR_-D6FD-7W56q2eYRVyRMbmE5NFaXLHrng@mail.gmail.com>
Subject: Re: [RFCv2 0/8] USI stylus support series
To:     Tero Kristo <tero.kristo@linux.intel.com>
Cc:     "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
        Jiri Kosina <jikos@kernel.org>,
        Mika Westerberg <mika.westerberg@linux.intel.com>,
        lkml <linux-kernel@vger.kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>,
        Peter Hutterer <peter.hutterer@who-t.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Tero,

On Fri, Nov 26, 2021 at 2:02 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi,
>
> This series is an update based on comments from Benjamin. What is done
> is this series is to ditch the separate hid-driver for USI, and add the
> generic support to core layers. This part basically brings the support
> for providing USI events, without programmability (patches 1-6).

That part seems to be almost good for now. I have a few things to check:
- patch2: "HID: hid-input: Add suffix also for HID_DG_PEN" I need to
ensure there are no touchscreens affected by this (there used to be a
mess with some vendors where they would not declare things properly)
- patch5: "HID: core: map USI pen style reports directly" this one
feels plain wrong. I would need to have a look at the report
descriptor but this is too specific in a very generic code

>
> Additionally, a HID-BPF based sample is provided which can be used to
> program / query pen parameters in comparison to the old driver level
> implementation (patches 7-8, patch #8 is an incremental change on top of
> patch #7 which just converts the fifo to socket so that the client can
> also get results back from the server.)

After a few more thoughts, I wondered what your input is on this. We
should be able to do the very same with plain hidraw... However, you
added a `hid/raw_event` processing that will still be kept in the
kernel, so maybe bpf would be useful for that at least.

>
> The whole series is based on top of Benjamin's hid-bpf support work, and
> I've pushed a branch at [1] with a series that works and brings in
> the dependency. There are also a few separate patches in this series to
> fix the problems I found from Benjamin's initial work for hid-bpf; I
> wasn't able to get things working without those. The branch is also
> based on top of 5.16-rc2 which required some extra changes to the
> patches from Benjamin.

Yeah, I also rebased on top of 5.16 shortly after sharing that branch
and got roughly the same last fix (HID: bpf: compile fix for
bpf_hid_foreach_rdesc_item). I am *very* interested in your "HID: bpf:
execute BPF programs in proper context" because that is something that
was bothering me a lot :)

"HID: bpf: add expected_attach_type to bpf prog during detach" is
something I'll need to bring in too

but "HID: bpf: fix file mapping" is actually wrong. I initially wanted
to attach BPF programs to hidraw, but shortly realized that this is
not working because the `hid/rdesc_fixup` kills the hidraw node and so
releases the BPF programs. The way I am now attaching it is to use the
fd associated with the modalias in the sysfs file (for instance: `sudo
./hid_surface_dial /sys/bus/hid/devices/0005:045E:091B.*/modalias`).
This way, the reference to the struct hid_device is kept even if we
disconnect the device and reprobe it.

Thanks again for your work, and I'd be curious to have your thoughts
on hid-bpf and if you think it is better than hidraw/evdev write/new
ioctls for your use case.

Cheers,
Benjamin

>
> -Tero
>
> [1] https://github.com/t-kristo/linux/tree/usi-5.16-rfc-v2-bpf
>
>