[PATCH v2] uhid: Remove local copy of uhid header

[PATCH v2] uhid: Remove local copy of uhid header

[Luiz Augusto von Dentz]

From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>

uhid.h is part of kernel uapi nowadays so it can be included
directly from linux/uhid.h so this removes the local copy to use it
instead.
---
 Makefile.plugins           |   2 +-
 profiles/input/uhid_copy.h | 199 -------------------------------------
 src/shared/uhid.h          |   3 +-
 3 files changed, 2 insertions(+), 202 deletions(-)
 delete mode 100644 profiles/input/uhid_copy.h

diff --git a/Makefile.plugins b/Makefile.plugins
index 69fb01001..7693c767f 100644
--- a/Makefile.plugins
+++ b/Makefile.plugins
@@ -68,7 +68,7 @@ endif
 
 if HOG
 builtin_modules += hog
-builtin_sources += profiles/input/hog.c profiles/input/uhid_copy.h \
+builtin_sources += profiles/input/hog.c \
 			profiles/input/hog-lib.c profiles/input/hog-lib.h \
 			profiles/deviceinfo/dis.c profiles/deviceinfo/dis.h \
 			profiles/battery/bas.c profiles/battery/bas.h \
diff --git a/profiles/input/uhid_copy.h b/profiles/input/uhid_copy.h
deleted file mode 100644
index 0ef73d4cc..000000000
--- a/profiles/input/uhid_copy.h
+++ /dev/null
@@ -1,199 +0,0 @@
-#ifndef __UHID_H_
-#define __UHID_H_
-
-/*
- * User-space I/O driver support for HID subsystem
- * Copyright (c) 2012 David Herrmann
- */
-
-/*
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of the GNU General Public License as published by the Free
- * Software Foundation; either version 2 of the License, or (at your option)
- * any later version.
- */
-
-/*
- * Public header for user-space communication. We try to keep every structure
- * aligned but to be safe we also use __attribute__((__packed__)). Therefore,
- * the communication should be ABI compatible even between architectures.
- */
-
-#include <linux/input.h>
-#include <linux/types.h>
-#include <linux/hid.h>
-
-enum uhid_event_type {
-	__UHID_LEGACY_CREATE,
-	UHID_DESTROY,
-	UHID_START,
-	UHID_STOP,
-	UHID_OPEN,
-	UHID_CLOSE,
-	UHID_OUTPUT,
-	__UHID_LEGACY_OUTPUT_EV,
-	__UHID_LEGACY_INPUT,
-	UHID_GET_REPORT,
-	UHID_GET_REPORT_REPLY,
-	UHID_CREATE2,
-	UHID_INPUT2,
-	UHID_SET_REPORT,
-	UHID_SET_REPORT_REPLY,
-};
-
-struct uhid_create2_req {
-	__u8 name[128];
-	__u8 phys[64];
-	__u8 uniq[64];
-	__u16 rd_size;
-	__u16 bus;
-	__u32 vendor;
-	__u32 product;
-	__u32 version;
-	__u32 country;
-	__u8 rd_data[HID_MAX_DESCRIPTOR_SIZE];
-} __attribute__((__packed__));
-
-enum uhid_dev_flag {
-	UHID_DEV_NUMBERED_FEATURE_REPORTS			= (1ULL << 0),
-	UHID_DEV_NUMBERED_OUTPUT_REPORTS			= (1ULL << 1),
-	UHID_DEV_NUMBERED_INPUT_REPORTS				= (1ULL << 2),
-};
-
-struct uhid_start_req {
-	__u64 dev_flags;
-};
-
-#define UHID_DATA_MAX 4096
-
-enum uhid_report_type {
-	UHID_FEATURE_REPORT,
-	UHID_OUTPUT_REPORT,
-	UHID_INPUT_REPORT,
-};
-
-struct uhid_input2_req {
-	__u16 size;
-	__u8 data[UHID_DATA_MAX];
-} __attribute__((__packed__));
-
-struct uhid_output_req {
-	__u8 data[UHID_DATA_MAX];
-	__u16 size;
-	__u8 rtype;
-} __attribute__((__packed__));
-
-struct uhid_get_report_req {
-	__u32 id;
-	__u8 rnum;
-	__u8 rtype;
-} __attribute__((__packed__));
-
-struct uhid_get_report_reply_req {
-	__u32 id;
-	__u16 err;
-	__u16 size;
-	__u8 data[UHID_DATA_MAX];
-} __attribute__((__packed__));
-
-struct uhid_set_report_req {
-	__u32 id;
-	__u8 rnum;
-	__u8 rtype;
-	__u16 size;
-	__u8 data[UHID_DATA_MAX];
-} __attribute__((__packed__));
-
-struct uhid_set_report_reply_req {
-	__u32 id;
-	__u16 err;
-} __attribute__((__packed__));
-
-/*
- * Compat Layer
- * All these commands and requests are obsolete. You should avoid using them in
- * new code. We support them for backwards-compatibility, but you might not get
- * access to new feature in case you use them.
- */
-
-enum uhid_legacy_event_type {
-	UHID_CREATE			= __UHID_LEGACY_CREATE,
-	UHID_OUTPUT_EV			= __UHID_LEGACY_OUTPUT_EV,
-	UHID_INPUT			= __UHID_LEGACY_INPUT,
-	UHID_FEATURE			= UHID_GET_REPORT,
-	UHID_FEATURE_ANSWER		= UHID_GET_REPORT_REPLY,
-};
-
-/* Obsolete! Use UHID_CREATE2. */
-struct uhid_create_req {
-	__u8 name[128];
-	__u8 phys[64];
-	__u8 uniq[64];
-	__u8 *rd_data;
-	__u16 rd_size;
-
-	__u16 bus;
-	__u32 vendor;
-	__u32 product;
-	__u32 version;
-	__u32 country;
-} __attribute__((__packed__));
-
-/* Obsolete! Use UHID_INPUT2. */
-struct uhid_input_req {
-	__u8 data[UHID_DATA_MAX];
-	__u16 size;
-} __attribute__((__packed__));
-
-/* Obsolete! Kernel uses UHID_OUTPUT exclusively now. */
-struct uhid_output_ev_req {
-	__u16 type;
-	__u16 code;
-	__s32 value;
-} __attribute__((__packed__));
-
-/* Obsolete! Kernel uses ABI compatible UHID_GET_REPORT. */
-struct uhid_feature_req {
-	__u32 id;
-	__u8 rnum;
-	__u8 rtype;
-} __attribute__((__packed__));
-
-/* Obsolete! Use ABI compatible UHID_GET_REPORT_REPLY. */
-struct uhid_feature_answer_req {
-	__u32 id;
-	__u16 err;
-	__u16 size;
-	__u8 data[UHID_DATA_MAX];
-} __attribute__((__packed__));
-
-/*
- * UHID Events
- * All UHID events from and to the kernel are encoded as "struct uhid_event".
- * The "type" field contains a UHID_* type identifier. All payload depends on
- * that type and can be accessed via ev->u.XYZ accordingly.
- * If user-space writes short events, they're extended with 0s by the kernel. If
- * the kernel writes short events, user-space shall extend them with 0s.
- */
-
-struct uhid_event {
-	__u32 type;
-
-	union {
-		struct uhid_create_req create;
-		struct uhid_input_req input;
-		struct uhid_output_req output;
-		struct uhid_output_ev_req output_ev;
-		struct uhid_feature_req feature;
-		struct uhid_get_report_req get_report;
-		struct uhid_feature_answer_req feature_answer;
-		struct uhid_get_report_reply_req get_report_reply;
-		struct uhid_create2_req create2;
-		struct uhid_input2_req input2;
-		struct uhid_set_report_req set_report;
-		struct uhid_set_report_reply_req set_report_reply;
-		struct uhid_start_req start;
-	} u;
-} __attribute__((__packed__));
-
-#endif /* __UHID_H_ */
diff --git a/src/shared/uhid.h b/src/shared/uhid.h
index 839809d9a..55ae839f3 100644
--- a/src/shared/uhid.h
+++ b/src/shared/uhid.h
@@ -10,8 +10,7 @@
 
 #include <stdbool.h>
 #include <stdint.h>
-
-#include "profiles/input/uhid_copy.h"
+#include <linux/uhid.h>
 
 struct bt_uhid;
 
-- 
2.33.1

RE: [v2] uhid: Remove local copy of uhid header

[bluez.test.bot]

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

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=584157

---Test result---

Test Summary:
CheckPatch                    PASS      1.37 seconds
GitLint                       PASS      0.97 seconds
Prep - Setup ELL              PASS      42.74 seconds
Build - Prep                  PASS      0.50 seconds
Build - Configure             PASS      8.03 seconds
Build - Make                  PASS      182.26 seconds
Make Check                    PASS      9.75 seconds
Make Distcheck                PASS      218.98 seconds
Build w/ext ELL - Configure   PASS      8.10 seconds
Build w/ext ELL - Make        PASS      173.15 seconds



---
Regards,
Linux Bluetooth

Re: [PATCH v2] uhid: Remove local copy of uhid header

[Bastien Nocera]

On Mon, 2021-11-22 at 13:17 -0800, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> 
> uhid.h is part of kernel uapi nowadays so it can be included
> directly from linux/uhid.h so this removes the local copy to use it
> instead.

This will cause the same problems that importing those headers into the
repository was supposed to solve. If you remove those headers, then
older distributions will be stuck on old kernel headers, and bluez
compilations will regularly fail when it uses new structs, struct
members, functions, or constants that are in the upstream uapi headers
but not yet propagated to the user's distribution.

Strong NAK on this one and the uinput header change too.

Re: [PATCH v2] uhid: Remove local copy of uhid header

[Luiz Augusto von Dentz]

Hi Bastien,

On Mon, Nov 22, 2021 at 3:06 PM Bastien Nocera <hadess@hadess.net> wrote:
>
> On Mon, 2021-11-22 at 13:17 -0800, Luiz Augusto von Dentz wrote:
> > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> >
> > uhid.h is part of kernel uapi nowadays so it can be included
> > directly from linux/uhid.h so this removes the local copy to use it
> > instead.
>
> This will cause the same problems that importing those headers into the
> repository was supposed to solve. If you remove those headers, then
> older distributions will be stuck on old kernel headers, and bluez
> compilations will regularly fail when it uses new structs, struct
> members, functions, or constants that are in the upstream uapi headers
> but not yet propagated to the user's distribution.

I'm not following you on this, the distros don't have uapi headers
that match their kernel release? Afaik being a kernel uapi means these
APIs are considered stable and I suspect they haven't been changed in
a while, you are right that this introduces a kernel dependency if we
were to use new members but I still think this is better than having
copies that at some point goes out of sync, specially when nothing
indicates that things like input_event was not accepted by the kernel.

> Strong NAK on this one and the uinput header change too.

I didn't introduce the usage of any new symbols, so the only new
requirement is that linux/uinput.h and linux/uhid.h headers exist, I
could however rollback if we have another way to check if those
headers exists use then otherwise we can keep using copies, perhaps
move then under linux/ directory, anyway it is not like we don't have
kernel dependencies already and we actually have been debating on
having the bluetooth socket definitions as part of the uapi instead of
libbluetooth, so we will need to sort out how we can update the
userspace parts with new kernel interface without breaking the build
on systems that don't actually ship with e.g. linux/bluetooth.h.

-- 
Luiz Augusto von Dentz

Re: [PATCH v2] uhid: Remove local copy of uhid header

[Bastien Nocera]

On Mon, 2021-11-22 at 15:46 -0800, Luiz Augusto von Dentz wrote:
> Hi Bastien,
> 
> On Mon, Nov 22, 2021 at 3:06 PM Bastien Nocera <hadess@hadess.net>
> wrote:
> > 
> > On Mon, 2021-11-22 at 13:17 -0800, Luiz Augusto von Dentz wrote:
> > > From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > > 
> > > uhid.h is part of kernel uapi nowadays so it can be included
> > > directly from linux/uhid.h so this removes the local copy to use
> > > it
> > > instead.
> > 
> > This will cause the same problems that importing those headers into
> > the
> > repository was supposed to solve. If you remove those headers, then
> > older distributions will be stuck on old kernel headers, and bluez
> > compilations will regularly fail when it uses new structs, struct
> > members, functions, or constants that are in the upstream uapi
> > headers
> > but not yet propagated to the user's distribution.
> 
> I'm not following you on this, the distros don't have uapi headers
> that match their kernel release? Afaik being a kernel uapi means
> these
> APIs are considered stable and I suspect they haven't been changed in
> a while, you are right that this introduces a kernel dependency if we
> were to use new members but I still think this is better than having
> copies that at some point goes out of sync, specially when nothing
> indicates that things like input_event was not accepted by the
> kernel.

Let's say you want to use a KEY_* definition that was recently added to
the kernel, what would you do?

> > Strong NAK on this one and the uinput header change too.
> 
> I didn't introduce the usage of any new symbols, so the only new
> requirement is that linux/uinput.h and linux/uhid.h headers exist, I
> could however rollback if we have another way to check if those
> headers exists use then otherwise we can keep using copies, perhaps
> move then under linux/ directory, anyway it is not like we don't have
> kernel dependencies already and we actually have been debating on
> having the bluetooth socket definitions as part of the uapi instead
> of
> libbluetooth, so we will need to sort out how we can update the
> userspace parts with new kernel interface without breaking the build
> on systems that don't actually ship with e.g. linux/bluetooth.h.

You're talking about the state of things now, I'm talking about the
compilation failures once you rely on a slightly newer header that
isn't shipped with a distribution.

But if you're willing to react to the compilation failure in the
future, I can't really stand in your way. It just seems weird to do
this now just to undo it the moment you'll need a slightly newer kernel
header.

Cheers

Re: [PATCH v2] uhid: Remove local copy of uhid header

[Marcel Holtmann]

Hi Batien,

>>>> uhid.h is part of kernel uapi nowadays so it can be included
>>>> directly from linux/uhid.h so this removes the local copy to use
>>>> it
>>>> instead.
>>> 
>>> This will cause the same problems that importing those headers into
>>> the
>>> repository was supposed to solve. If you remove those headers, then
>>> older distributions will be stuck on old kernel headers, and bluez
>>> compilations will regularly fail when it uses new structs, struct
>>> members, functions, or constants that are in the upstream uapi
>>> headers
>>> but not yet propagated to the user's distribution.
>> 
>> I'm not following you on this, the distros don't have uapi headers
>> that match their kernel release? Afaik being a kernel uapi means
>> these
>> APIs are considered stable and I suspect they haven't been changed in
>> a while, you are right that this introduces a kernel dependency if we
>> were to use new members but I still think this is better than having
>> copies that at some point goes out of sync, specially when nothing
>> indicates that things like input_event was not accepted by the
>> kernel.
> 
> Let's say you want to use a KEY_* definition that was recently added to
> the kernel, what would you do?
> 
>>> Strong NAK on this one and the uinput header change too.
>> 
>> I didn't introduce the usage of any new symbols, so the only new
>> requirement is that linux/uinput.h and linux/uhid.h headers exist, I
>> could however rollback if we have another way to check if those
>> headers exists use then otherwise we can keep using copies, perhaps
>> move then under linux/ directory, anyway it is not like we don't have
>> kernel dependencies already and we actually have been debating on
>> having the bluetooth socket definitions as part of the uapi instead
>> of
>> libbluetooth, so we will need to sort out how we can update the
>> userspace parts with new kernel interface without breaking the build
>> on systems that don't actually ship with e.g. linux/bluetooth.h.
> 
> You're talking about the state of things now, I'm talking about the
> compilation failures once you rely on a slightly newer header that
> isn't shipped with a distribution.
> 
> But if you're willing to react to the compilation failure in the
> future, I can't really stand in your way. It just seems weird to do
> this now just to undo it the moment you'll need a slightly newer kernel
> header.

if I can’t build the tarballs on 5.10.0-7-powerpc, then they don’t get released.

Regards

Marcel

Re: [PATCH v2] uhid: Remove local copy of uhid header

[Luiz Augusto von Dentz]

Hi Marcel, Bastien,

On Tue, Nov 23, 2021 at 5:24 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Batien,
>
> >>>> uhid.h is part of kernel uapi nowadays so it can be included
> >>>> directly from linux/uhid.h so this removes the local copy to use
> >>>> it
> >>>> instead.
> >>>
> >>> This will cause the same problems that importing those headers into
> >>> the
> >>> repository was supposed to solve. If you remove those headers, then
> >>> older distributions will be stuck on old kernel headers, and bluez
> >>> compilations will regularly fail when it uses new structs, struct
> >>> members, functions, or constants that are in the upstream uapi
> >>> headers
> >>> but not yet propagated to the user's distribution.
> >>
> >> I'm not following you on this, the distros don't have uapi headers
> >> that match their kernel release? Afaik being a kernel uapi means
> >> these
> >> APIs are considered stable and I suspect they haven't been changed in
> >> a while, you are right that this introduces a kernel dependency if we
> >> were to use new members but I still think this is better than having
> >> copies that at some point goes out of sync, specially when nothing
> >> indicates that things like input_event was not accepted by the
> >> kernel.
> >
> > Let's say you want to use a KEY_* definition that was recently added to
> > the kernel, what would you do?
> >
> >>> Strong NAK on this one and the uinput header change too.
> >>
> >> I didn't introduce the usage of any new symbols, so the only new
> >> requirement is that linux/uinput.h and linux/uhid.h headers exist, I
> >> could however rollback if we have another way to check if those
> >> headers exists use then otherwise we can keep using copies, perhaps
> >> move then under linux/ directory, anyway it is not like we don't have
> >> kernel dependencies already and we actually have been debating on
> >> having the bluetooth socket definitions as part of the uapi instead
> >> of
> >> libbluetooth, so we will need to sort out how we can update the
> >> userspace parts with new kernel interface without breaking the build
> >> on systems that don't actually ship with e.g. linux/bluetooth.h.
> >
> > You're talking about the state of things now, I'm talking about the
> > compilation failures once you rely on a slightly newer header that
> > isn't shipped with a distribution.
> >
> > But if you're willing to react to the compilation failure in the
> > future, I can't really stand in your way. It just seems weird to do
> > this now just to undo it the moment you'll need a slightly newer kernel
> > header.
>
> if I can’t build the tarballs on 5.10.0-7-powerpc, then they don’t get released.

The way I see this is only really a problem for unstable uapi headers,
so in case we are early adopters it is probably a good practice to
have a copy to make sure the packages can be compiled on systems
without these headers, once these headers becomes stable and most
distros already ship with them, imo, there is no reason to keep the
copies as they are not subject to major redesign and normally just
receive minor updates for bugs, etc,  which is the kind of change we
would likely miss given we were no longer paying attention to changes
on those headers as they are considered stable.

Also we might as well include these headers to be check like we do in
the following check:

https://git.kernel.org/pub/scm/bluetooth/bluez.git/tree/configure.ac#n66

-- 
Luiz Augusto von Dentz