diff mbox

[v2,2/6] HID: generic: create one input report per application type

Message ID 20180424080437.21367-3-benjamin.tissoires@redhat.com (mailing list archive)
State New, archived
Headers show

Commit Message

Benjamin Tissoires April 24, 2018, 8:04 a.m. UTC
It is not a good idea to try to fit all types of applications in the
same input report. There are a lot of devices that are needing
the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
description as it is based on the report ID.

Given that most devices with MULTI_INPUT I can think of split nicely
the devices inputs into application, it is a good thing to split the
devices by default based on this assumption.

Also make hid-multitouch following this rule, to not have to deal
with too many input created.

While we are at it, fix some checkpatch complaints about converting
'unsigned' to 'unsigned int'.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
 drivers/hid/hid-core.c       | 19 +++++++++++++------
 drivers/hid/hid-generic.c    | 15 +++++++++++++++
 drivers/hid/hid-gfrm.c       |  2 +-
 drivers/hid/hid-input.c      | 17 +++++++++++++++++
 drivers/hid/hid-magicmouse.c |  6 +++---
 include/linux/hid.h          | 10 +++++++---
 6 files changed, 56 insertions(+), 13 deletions(-)

Comments

Dmitry Torokhov Aug. 24, 2018, 3:44 p.m. UTC | #1
Hi Benjamin, Jiri,

On Tue, Apr 24, 2018 at 1:04 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> It is not a good idea to try to fit all types of applications in the
> same input report. There are a lot of devices that are needing
> the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
> description as it is based on the report ID.
>
> Given that most devices with MULTI_INPUT I can think of split nicely
> the devices inputs into application, it is a good thing to split the
> devices by default based on this assumption.
>
> Also make hid-multitouch following this rule, to not have to deal
> with too many input created.
>
> While we are at it, fix some checkpatch complaints about converting
> 'unsigned' to 'unsigned int'.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---

I see a spike of user reports on Bugzilla with HID devices being
broken on 4.18 that seem to point to this commit. For example:
https://bugzilla.kernel.org/show_bug.cgi?id=200849

Thanks.
Benjamin Tissoires Aug. 27, 2018, 3:27 p.m. UTC | #2
On Fri, Aug 24, 2018 at 5:45 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Benjamin, Jiri,
>
> On Tue, Apr 24, 2018 at 1:04 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > It is not a good idea to try to fit all types of applications in the
> > same input report. There are a lot of devices that are needing
> > the quirk HID_MULTI_INPUT but this quirk doesn't match the actual HID
> > description as it is based on the report ID.
> >
> > Given that most devices with MULTI_INPUT I can think of split nicely
> > the devices inputs into application, it is a good thing to split the
> > devices by default based on this assumption.
> >
> > Also make hid-multitouch following this rule, to not have to deal
> > with too many input created.
> >
> > While we are at it, fix some checkpatch complaints about converting
> > 'unsigned' to 'unsigned int'.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
>
> I see a spike of user reports on Bugzilla with HID devices being
> broken on 4.18 that seem to point to this commit. For example:
> https://bugzilla.kernel.org/show_bug.cgi?id=200849

Thanks Dmitry. I have requested logs from the reporters. It is
possible that the device will get split in 2: one for the pointer and
one for the buttons, and systemd would have no clue on how to handle
those cases. Depending on the logs, I'll see how we can fix that in a
clean way.

Cheers,
Benjamin
diff mbox

Patch

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5d7cc6bbbac6..68819106f4fc 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -57,7 +57,9 @@  MODULE_PARM_DESC(ignore_special_drivers, "Ignore any special drivers and handle
  * Register a new report for a device.
  */
 
-struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id)
+struct hid_report *hid_register_report(struct hid_device *device,
+				       unsigned int type, unsigned int id,
+				       unsigned int application)
 {
 	struct hid_report_enum *report_enum = device->report_enum + type;
 	struct hid_report *report;
@@ -78,6 +80,7 @@  struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
 	report->type = type;
 	report->size = 0;
 	report->device = device;
+	report->application = application;
 	report_enum->report_id_hash[id] = report;
 
 	list_add_tail(&report->list, &report_enum->report_list);
@@ -221,11 +224,15 @@  static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 {
 	struct hid_report *report;
 	struct hid_field *field;
-	unsigned usages;
-	unsigned offset;
-	unsigned i;
+	unsigned int usages;
+	unsigned int offset;
+	unsigned int i;
+	unsigned int application;
+
+	application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
 
-	report = hid_register_report(parser->device, report_type, parser->global.report_id);
+	report = hid_register_report(parser->device, report_type,
+				     parser->global.report_id, application);
 	if (!report) {
 		hid_err(parser->device, "hid_register_report failed\n");
 		return -1;
@@ -259,7 +266,7 @@  static int hid_add_field(struct hid_parser *parser, unsigned report_type, unsign
 
 	field->physical = hid_lookup_collection(parser, HID_COLLECTION_PHYSICAL);
 	field->logical = hid_lookup_collection(parser, HID_COLLECTION_LOGICAL);
-	field->application = hid_lookup_collection(parser, HID_COLLECTION_APPLICATION);
+	field->application = application;
 
 	for (i = 0; i < usages; i++) {
 		unsigned j = i;
diff --git a/drivers/hid/hid-generic.c b/drivers/hid/hid-generic.c
index c25b4718de44..3b6eccbc2519 100644
--- a/drivers/hid/hid-generic.c
+++ b/drivers/hid/hid-generic.c
@@ -56,6 +56,20 @@  static bool hid_generic_match(struct hid_device *hdev,
 	return true;
 }
 
+static int hid_generic_probe(struct hid_device *hdev,
+			     const struct hid_device_id *id)
+{
+	int ret;
+
+	hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
+
+	ret = hid_parse(hdev);
+	if (ret)
+		return ret;
+
+	return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
+}
+
 static const struct hid_device_id hid_table[] = {
 	{ HID_DEVICE(HID_BUS_ANY, HID_GROUP_ANY, HID_ANY_ID, HID_ANY_ID) },
 	{ }
@@ -66,6 +80,7 @@  static struct hid_driver hid_generic = {
 	.name = "hid-generic",
 	.id_table = hid_table,
 	.match = hid_generic_match,
+	.probe = hid_generic_probe,
 };
 module_hid_driver(hid_generic);
 
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index 075b1c020846..cf477f8c8f4c 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -116,7 +116,7 @@  static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		 * those reports reach gfrm_raw_event() from hid_input_report().
 		 */
 		if (!hid_register_report(hdev, HID_INPUT_REPORT,
-					 GFRM100_SEARCH_KEY_REPORT_ID)) {
+					 GFRM100_SEARCH_KEY_REPORT_ID, 0)) {
 			ret = -ENOMEM;
 			goto done;
 		}
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 62e42664955e..361643683c08 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1620,6 +1620,20 @@  static struct hid_input *hidinput_match(struct hid_report *report)
 	return NULL;
 }
 
+static struct hid_input *hidinput_match_application(struct hid_report *report)
+{
+	struct hid_device *hid = report->device;
+	struct hid_input *hidinput;
+
+	list_for_each_entry(hidinput, &hid->inputs, list) {
+		if (hidinput->report &&
+		    hidinput->report->application == report->application)
+			return hidinput;
+	}
+
+	return NULL;
+}
+
 static inline void hidinput_configure_usages(struct hid_input *hidinput,
 					     struct hid_report *report)
 {
@@ -1680,6 +1694,9 @@  int hidinput_connect(struct hid_device *hid, unsigned int force)
 			 */
 			if (hid->quirks & HID_QUIRK_MULTI_INPUT)
 				hidinput = hidinput_match(report);
+			else if (hid->maxapplication > 1 &&
+				 (hid->quirks & HID_QUIRK_INPUT_PER_APP))
+				hidinput = hidinput_match_application(report);
 
 			if (!hidinput) {
 				hidinput = hidinput_allocate(hid);
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 42ed887ba0be..b454c4386157 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -531,12 +531,12 @@  static int magicmouse_probe(struct hid_device *hdev,
 
 	if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			MOUSE_REPORT_ID);
+			MOUSE_REPORT_ID, 0);
 	else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			TRACKPAD_REPORT_ID);
+			TRACKPAD_REPORT_ID, 0);
 		report = hid_register_report(hdev, HID_INPUT_REPORT,
-			DOUBLE_REPORT_ID);
+			DOUBLE_REPORT_ID, 0);
 	}
 
 	if (!report) {
diff --git a/include/linux/hid.h b/include/linux/hid.h
index e1ec7f91f926..d5ebeacc3b57 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -341,6 +341,7 @@  struct hid_item {
 /* BIT(8) reserved for backward compatibility, was HID_QUIRK_NO_EMPTY_INPUT */
 /* BIT(9) reserved for backward compatibility, was NO_INIT_INPUT_REPORTS */
 #define HID_QUIRK_ALWAYS_POLL			BIT(10)
+#define HID_QUIRK_INPUT_PER_APP			BIT(11)
 #define HID_QUIRK_SKIP_OUTPUT_REPORTS		BIT(16)
 #define HID_QUIRK_SKIP_OUTPUT_REPORT_ID		BIT(17)
 #define HID_QUIRK_NO_OUTPUT_REPORTS_ON_INTR_EP	BIT(18)
@@ -465,8 +466,9 @@  struct hid_field {
 struct hid_report {
 	struct list_head list;
 	struct list_head hidinput_list;
-	unsigned id;					/* id of this report */
-	unsigned type;					/* report type */
+	unsigned int id;				/* id of this report */
+	unsigned int type;				/* report type */
+	unsigned int application;			/* application usage for this report */
 	struct hid_field *field[HID_MAX_FIELDS];	/* fields of the report */
 	unsigned maxfield;				/* maximum valid field index */
 	unsigned size;					/* size of the report (bits) */
@@ -868,7 +870,9 @@  void hid_output_report(struct hid_report *report, __u8 *data);
 void __hid_request(struct hid_device *hid, struct hid_report *rep, int reqtype);
 u8 *hid_alloc_report_buf(struct hid_report *report, gfp_t flags);
 struct hid_device *hid_allocate_device(void);
-struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
+struct hid_report *hid_register_report(struct hid_device *device,
+				       unsigned int type, unsigned int id,
+				       unsigned int application);
 int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
 struct hid_report *hid_validate_values(struct hid_device *hid,
 				       unsigned int type, unsigned int id,