Re: [V3, PATCH] Add additional hidraw input/output report ioctls.

From: "Filipe Laíns" <lains@archlinux.org>
To: Dean Camera <dean@fourwalledcubicle.com>,
	linux-input@vger.kernel.org, jikos@kernel.org,
	benjamin.tissoires@redhat.com
Subject: Re: [V3, PATCH] Add additional hidraw input/output report ioctls.
Date: Thu, 26 Nov 2020 21:42:37 +0000	[thread overview]
Message-ID: <c8d4d17798e2c9fca417223d4edf5f4b1aebf75d.camel@archlinux.org> (raw)
In-Reply-To: <2c2bfd55-3a03-9839-22f5-16058ac24e35@fourwalledcubicle.com>

[-- Attachment #1: Type: text/plain, Size: 3312 bytes --]

On Fri, 2020-11-27 at 08:30 +1100, Dean Camera wrote:
> Hi Filipe,
> 
> Comments inline.
> 
> - Dean
> 
> On 27/11/2020 6:05 am, Filipe Laíns wrote:
> > Hi,
> > 
> > What is the difference between V1, V2 and V3? I think generally you would
> > add a
> > small summary.
> > 
> 
> Sorry, that's my fault -- the contents are identical. I am more used to 
> modern tooling with code review platforms, pull-requests or even emailed 
> attached patches, so the old tooling took me a few goes to get right.
> 
> V1 was mangled by Thunderbird, while V2 was missing the cover letter 
> when I submitted it via git send-email from my test machine. V3 is where 
> I (think) I beat the tooling into submission.

No worries, I was just wondering what the differences were, given that the
patches were identical.

*snip*

> > I am not sure using the same approach as HIDIOCGFEATURE is a good design
> > choice.
> > The first byte of the supplied buffer is the report ID, but you can set is
> > to 0
> > if you don't want to use numbered reports. From my understanding, this makes
> > it
> > impossible to use the ioctl with report ID 0, which valid per the HID spec.
> > 
> 
> Report ID 0 is reserved by the HID specification and may not be used in 
> a device with multiple reports (see "Device Class Definition for HID 
> 1.11", section "6.2.2.7 Global Items" where it states "Report ID zero is 
> reserved and should not be used.").

Yup, you are right! I missed that.

> I think the designers of HID forsaw a sane future where in userspace 
> everyone just assumed the report ID was present at all times, and the 
> HID driver would just omit it on the wire if it was zero. Unfortunatly 
> every platform seems to handle that differently now, with some always 
> requring it, and others selectively omitting it in their APIs.

Yes, it's unfortunate. Perhaps if the HID designers made the spec easier to read
we wouldn't have this issue.

> > My suggestion would be to automatically use numbered reports or not
> > depending if
> > the device uses them. A HID endpoint either uses numbered reports or not, it
> > doesn't make much sense to me to let users try to use numbered reports on
> > devices that do not use them or the other way round.
> > 
> > But I guess this is a question for Benjamin.
> 
> I'm *strongly* in favour of always having them at least in the 
> `ioctl()`, with a (reserved) zero value indicating it is unused - like 
> it is now. That makes userspace easier to deal with, and covers the 
> quirk case where a device does not list report IDs in its HID report 
> descriptor properly, but requires them anyway.
> 
> It also makes the new requests consistent with the existing request, so 
> there's no extra cognitive load from working with one then switching to 
> the others.

Yeah, it makes sense given that report 0 is reserved :)

> > 
> > I tried to track down the discussion about the addition of the
> > HIDIOCGFEATURE
> > ioctl but from what I saw there was no mention of this design flaw.
> > 
> > Am I missing something here?

I was :D

You can have my
Signed-off-by: Filipe Laíns <lains@archlinux.org>

Perhaps just split the hid-example.c change to another patch.

*snip*

Cheers,
Filipe Laíns

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]