Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20221205210354.11846-1-andrew.smirnov@gmail.com>
 <CADyDSO4uh6b+sSZTkZ2_DR923=bA=kXgK1LqUMkknCMzf_DSwQ@mail.gmail.com> <CAHQ1cqGaL5v4PARTZU6_0tfSCz3=9b1THz-D-Bg1G64hBV+_Wg@mail.gmail.com>
In-Reply-To: <CAHQ1cqGaL5v4PARTZU6_0tfSCz3=9b1THz-D-Bg1G64hBV+_Wg@mail.gmail.com>
From:   David Rheinsberg <david.rheinsberg@gmail.com>
Date:   Mon, 12 Dec 2022 16:24:48 +0100
Message-ID: <CADyDSO5wBWKoWCO668cgPrZrCo9SfsJ0XqfCEbgRU3WErReibQ@mail.gmail.com>
Subject: Re: [RFC PATCH 0/2] Handling of non-numbered feature reports by hidraw
To:     Andrey Smirnov <andrew.smirnov@gmail.com>
Cc:     linux-input@vger.kernel.org, Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi

On Thu, 8 Dec 2022 at 21:59, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Thu, Dec 8, 2022 at 7:46 AM David Rheinsberg
> <david.rheinsberg@gmail.com> wrote:
> >
> > Hi
> >
> > On Mon, 5 Dec 2022 at 22:04, Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
> > > I'm working on a firmware of a device that exposes a HID interface via
> > > USB and/or BLE and uses, among other things, non-numbered feature
> > > reports. Included in this series are two paches I had to create in
> > > order for hidraw devices created for aforementioned subsystems to
> > > behave in the same way when exerciesd by the same test tool.
> > >
> > > I don't know if the patches are acceptable as-is WRT to not breaking
> > > existing userspace, hence the RFC tag.
> >
> > Can you elaborate why you remove the special handling from USBHID but
> > add it to UHID? They both operate logically on the same level, so
> > shouldn't we simply adjust uhid to include the report-id in buf[0]?
> >
> > Also, you override buf[0] in UHID, so I wonder what UHID currently
> > returns there?
> >
> > IOW, can you elaborate a bit what the current behavior of each of the
> > involved modules is, and what behavior you would expect? This would
> > allow to better understand what you are trying to achieve. The more
> > context you can give, the easier it is to understand what happens
> > there.
> >
>
> Sorry it's not very clear, so the difference between the cases is that
> in the case of UHID the report ID ends up being included as a part of
> "SET_FEATURE", so BlueZ checks UHID_DEV_NUMBERED_FEATURE_REPORTS,
> which is not set (correctly) and tries to send the whole payload. This
> ends up as a maxlen + 1 (extra byte) write to a property that is
> maxlen long, which gets rejected by device's BLE stack.
>
> In the case of USBHID the problem happens in "GET_FEATURE" path. When
> userspace reads the expected data back it gets an extra 0 prepended to
> the payload, so all of the actual payload has an offset of 1. This
> doesn't happen with UHID, which I think is the correct behavior here.
>
> Hopefully that explains the difference, let me know if something is unclear

Yes, thanks, I completely missed that. Lets continue discussion on the patches.

Thanks!
David