linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] hid: Enable report fixup on rebind
@ 2012-04-22 12:21 Henrik Rydberg
  2012-04-22 12:21 ` [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly Henrik Rydberg
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Henrik Rydberg @ 2012-04-22 12:21 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: Dmitry Torokhov, linux-input, linux-kernel, Henrik Rydberg

Hi Jiri,

This patchset contains a resolution to the problem with
driver-specific modification of the report descriptor. The core of the
problem lies with the dual semantics of hid_parse_report(), which is
therefore split into two functions. As a consequence, the hid core can
handle the rebind case internally, with no changes to the
drivers. Out-of-tree drivers will work fine as long as they operate in
the same way as the in-kernel drivers.

The first two patches are bug fixes found in the process. I am hoping
for some test feedback on those. The third patch is the main one.

Henrik Rydberg (3):
  hid-hyperv: Do not use hid_parse_report() directly
  hid-logitech: Collect report descriptors before sending
  hid: Handle driver-specific device descriptor in core

 drivers/hid/hid-core.c        |  112 +++++++++++++++++++++++++++++++++--------
 drivers/hid/hid-hyperv.c      |   14 +++++-
 drivers/hid/hid-logitech-dj.c |   71 +++++++++++---------------
 include/linux/hid.h           |   14 ++----
 4 files changed, 135 insertions(+), 76 deletions(-)

-- 
1.7.10


^ permalink raw reply	[flat|nested] 16+ messages in thread
* [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly
  2012-04-22 12:21 [PATCH 0/3] hid: Enable report fixup on rebind Henrik Rydberg
@ 2012-04-22 12:21 ` Henrik Rydberg
  2012-04-27  8:53   ` Jiri Kosina
  2012-04-22 12:21 ` [PATCH 2/3] hid-logitech: Collect report descriptors before sending Henrik Rydberg
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 16+ messages in thread
From: Henrik Rydberg @ 2012-04-22 12:21 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Dmitry Torokhov, linux-input, linux-kernel, Henrik Rydberg,
	K. Y. Srinivasan

Upcoming changes will split the semantics of hid_parse_report()
and hid_parse(), so make sure drivers use hid_parse() in probe().

Compiled, not tested.

Cc: "K. Y. Srinivasan" <kys@microsoft.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-hyperv.c |   14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index 4066324..032e6c0 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -430,6 +430,15 @@ cleanup:
 	return ret;
 }
 
+static int mousevsc_hid_parse(struct hid_device *hid)
+{
+	struct hv_device *dev = hid_get_drvdata(hid);
+	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
+
+	return hid_parse_report(hid, input_dev->report_desc,
+				input_dev->report_desc_size);
+}
+
 static int mousevsc_hid_open(struct hid_device *hid)
 {
 	return 0;
@@ -449,6 +458,7 @@ static void mousevsc_hid_stop(struct hid_device *hid)
 }
 
 static struct hid_ll_driver mousevsc_ll_driver = {
+	.parse = mousevsc_hid_parse,
 	.open = mousevsc_hid_open,
 	.close = mousevsc_hid_close,
 	.start = mousevsc_hid_start,
@@ -510,9 +520,9 @@ static int mousevsc_probe(struct hv_device *device,
 	if (ret)
 		goto probe_err1;
 
-	ret = hid_parse_report(hid_dev, input_dev->report_desc,
-				input_dev->report_desc_size);
+	hid_set_drvdata(hid_dev, device);
 
+	ret = hid_parse(hid_dev);
 	if (ret) {
 		hid_err(hid_dev, "parse failed\n");
 		goto probe_err2;
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 2/3] hid-logitech: Collect report descriptors before sending
  2012-04-22 12:21 [PATCH 0/3] hid: Enable report fixup on rebind Henrik Rydberg
  2012-04-22 12:21 ` [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly Henrik Rydberg
@ 2012-04-22 12:21 ` Henrik Rydberg
       [not found]   ` <CAE7qMrqEJmBWo7JDHuB2DnWJ4vkvv9jya7RDJguiXsrs2xgadQ@mail.gmail.com>
  2012-04-22 12:21 ` [PATCH 3/3] hid: Handle driver-specific device descriptor in core Henrik Rydberg
  2012-04-22 21:27 ` [PATCH 0/3] hid: Enable report fixup on rebind Jiri Kosina
  3 siblings, 1 reply; 16+ messages in thread
From: Henrik Rydberg @ 2012-04-22 12:21 UTC (permalink / raw)
  To: Jiri Kosina
  Cc: Dmitry Torokhov, linux-input, linux-kernel, Henrik Rydberg,
	Nestor Lopez Casado, Benjamin Tissoires

The current code allows several consecutive calls to hid_parse_report(),
which may have happened to work before, but would cause a memory leak
and generally be incorrect. This patch collects all the reports
before sending them once.

Compiled, not tested.

Cc: Nestor Lopez Casado <nlopezcasad@logitech.com>
Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-logitech-dj.c |   71 +++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 42 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 2b56efc..e1c38bb 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -155,6 +155,14 @@ static const char media_descriptor[] = {
 /* Maximum size of all defined hid reports in bytes (including report id) */
 #define MAX_REPORT_SIZE 8
 
+/* Make sure all descriptors are present here */
+#define MAX_RDESC_SIZE				\
+	(sizeof(kbd_descriptor) +		\
+	 sizeof(mse_descriptor) +		\
+	 sizeof(consumer_descriptor) +		\
+	 sizeof(syscontrol_descriptor) +	\
+	 sizeof(media_descriptor))
+
 /* Number of possible hid report types that can be created by this driver.
  *
  * Right now, RF report types have the same report types (or report id's)
@@ -473,9 +481,17 @@ static int logi_dj_output_hidraw_report(struct hid_device *hid, u8 * buf,
 	return 0;
 }
 
+static void rdcat(char **rdesc, unsigned int *rsize, const char *data, unsigned int size)
+{
+	memcpy(*rdesc + *rsize, data, size);
+	*rsize += size;
+}
+
 static int logi_dj_ll_parse(struct hid_device *hid)
 {
 	struct dj_device *djdev = hid->driver_data;
+	unsigned int rsize = 0;
+	char *rdesc;
 	int retval;
 
 	dbg_hid("%s\n", __func__);
@@ -483,70 +499,38 @@ static int logi_dj_ll_parse(struct hid_device *hid)
 	djdev->hdev->version = 0x0111;
 	djdev->hdev->country = 0x00;
 
+	rdesc = kmalloc(MAX_RDESC_SIZE, GFP_KERNEL);
+	if (!rdesc)
+		return -ENOMEM;
+
 	if (djdev->reports_supported & STD_KEYBOARD) {
 		dbg_hid("%s: sending a kbd descriptor, reports_supported: %x\n",
 			__func__, djdev->reports_supported);
-		retval = hid_parse_report(hid,
-					  (u8 *) kbd_descriptor,
-					  sizeof(kbd_descriptor));
-		if (retval) {
-			dbg_hid("%s: sending a kbd descriptor, hid_parse failed"
-				" error: %d\n", __func__, retval);
-			return retval;
-		}
+		rdcat(&rdesc, &rsize, kbd_descriptor, sizeof(kbd_descriptor));
 	}
 
 	if (djdev->reports_supported & STD_MOUSE) {
 		dbg_hid("%s: sending a mouse descriptor, reports_supported: "
 			"%x\n", __func__, djdev->reports_supported);
-		retval = hid_parse_report(hid,
-					  (u8 *) mse_descriptor,
-					  sizeof(mse_descriptor));
-		if (retval) {
-			dbg_hid("%s: sending a mouse descriptor, hid_parse "
-				"failed error: %d\n", __func__, retval);
-			return retval;
-		}
+		rdcat(&rdesc, &rsize, mse_descriptor, sizeof(mse_descriptor));
 	}
 
 	if (djdev->reports_supported & MULTIMEDIA) {
 		dbg_hid("%s: sending a multimedia report descriptor: %x\n",
 			__func__, djdev->reports_supported);
-		retval = hid_parse_report(hid,
-					  (u8 *) consumer_descriptor,
-					  sizeof(consumer_descriptor));
-		if (retval) {
-			dbg_hid("%s: sending a consumer_descriptor, hid_parse "
-				"failed error: %d\n", __func__, retval);
-			return retval;
-		}
+		rdcat(&rdesc, &rsize, consumer_descriptor, sizeof(consumer_descriptor));
 	}
 
 	if (djdev->reports_supported & POWER_KEYS) {
 		dbg_hid("%s: sending a power keys report descriptor: %x\n",
 			__func__, djdev->reports_supported);
-		retval = hid_parse_report(hid,
-					  (u8 *) syscontrol_descriptor,
-					  sizeof(syscontrol_descriptor));
-		if (retval) {
-			dbg_hid("%s: sending a syscontrol_descriptor, "
-				"hid_parse failed error: %d\n",
-				__func__, retval);
-			return retval;
-		}
+		rdcat(&rdesc, &rsize, syscontrol_descriptor, sizeof(syscontrol_descriptor));
 	}
 
 	if (djdev->reports_supported & MEDIA_CENTER) {
 		dbg_hid("%s: sending a media center report descriptor: %x\n",
 			__func__, djdev->reports_supported);
-		retval = hid_parse_report(hid,
-					  (u8 *) media_descriptor,
-					  sizeof(media_descriptor));
-		if (retval) {
-			dbg_hid("%s: sending a media_descriptor, hid_parse "
-				"failed error: %d\n", __func__, retval);
-			return retval;
-		}
+		rdcat(&rdesc, &rsize, media_descriptor, sizeof(media_descriptor));
 	}
 
 	if (djdev->reports_supported & KBD_LEDS) {
@@ -554,7 +538,10 @@ static int logi_dj_ll_parse(struct hid_device *hid)
 			__func__, djdev->reports_supported);
 	}
 
-	return 0;
+	retval = hid_parse_report(hid, rdesc, rsize);
+	kfree(rdesc);
+
+	return retval;
 }
 
 static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int type,
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH 3/3] hid: Handle driver-specific device descriptor in core
  2012-04-22 12:21 [PATCH 0/3] hid: Enable report fixup on rebind Henrik Rydberg
  2012-04-22 12:21 ` [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly Henrik Rydberg
  2012-04-22 12:21 ` [PATCH 2/3] hid-logitech: Collect report descriptors before sending Henrik Rydberg
@ 2012-04-22 12:21 ` Henrik Rydberg
  2012-04-22 21:27 ` [PATCH 0/3] hid: Enable report fixup on rebind Jiri Kosina
  3 siblings, 0 replies; 16+ messages in thread
From: Henrik Rydberg @ 2012-04-22 12:21 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: Dmitry Torokhov, linux-input, linux-kernel, Henrik Rydberg

The low-level driver can read the report descriptor, but it cannot
determine driver-specific changes to it. The hid core can fixup
and parse the report descriptor during driver attach, but does
not have direct access to the descriptor when doing so.

To be able to handle attach/detach of hid drivers properly,
a semantic change to hid_parse_report() is needed. This function has
been used in two ways, both as descriptor reader in the ll drivers and
as a parsor in the probe of the drivers. This patch splits the usage
by introducing hid_open_report(), and modifies the hid_parse() macro
to call hid_open_report() instead. The only usage of hid_parse_report()
is then to read and store the device descriptor. As a consequence, we
can handle the report fixups automatically inside the hid core.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
---
 drivers/hid/hid-core.c |  112 ++++++++++++++++++++++++++++++++++++++----------
 include/linux/hid.h    |   14 ++----
 2 files changed, 94 insertions(+), 32 deletions(-)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 4da66b4..9ed4ff3 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -546,12 +546,11 @@ static void hid_free_report(struct hid_report *report)
 }
 
 /*
- * Free a device structure, all reports, and all fields.
+ * Close report. This function returns the device
+ * state to the point prior to hid_open_report().
  */
-
-static void hid_device_release(struct device *dev)
+static void hid_close_report(struct hid_device *device)
 {
-	struct hid_device *device = container_of(dev, struct hid_device, dev);
 	unsigned i, j;
 
 	for (i = 0; i < HID_REPORT_TYPES; i++) {
@@ -562,11 +561,34 @@ static void hid_device_release(struct device *dev)
 			if (report)
 				hid_free_report(report);
 		}
+		memset(report_enum, 0, sizeof(*report_enum));
+		INIT_LIST_HEAD(&report_enum->report_list);
 	}
 
 	kfree(device->rdesc);
+	device->rdesc = NULL;
+	device->rsize = 0;
+
 	kfree(device->collection);
-	kfree(device);
+	device->collection = NULL;
+	device->collection_size = 0;
+	device->maxcollection = 0;
+	device->maxapplication = 0;
+
+	device->status &= ~HID_STAT_PARSED;
+}
+
+/*
+ * Free a device structure, all reports, and all fields.
+ */
+
+static void hid_device_release(struct device *dev)
+{
+	struct hid_device *hid = container_of(dev, struct hid_device, dev);
+
+	hid_close_report(hid);
+	kfree(hid->dev_rdesc);
+	kfree(hid);
 }
 
 /*
@@ -643,15 +665,37 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
  * @start: report start
  * @size: report size
  *
+ * Allocate the device report as read by the bus driver. This function should
+ * only be called from parse() in ll drivers.
+ */
+int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size)
+{
+	hid->dev_rdesc = kmemdup(start, size, GFP_KERNEL);
+	if (!hid->dev_rdesc)
+		return -ENOMEM;
+	hid->dev_rsize = size;
+	return 0;
+}
+EXPORT_SYMBOL_GPL(hid_parse_report);
+
+/**
+ * hid_open_report - open a driver-specific device report
+ *
+ * @device: hid device
+ *
  * Parse a report description into a hid_device structure. Reports are
  * enumerated, fields are attached to these reports.
  * 0 returned on success, otherwise nonzero error value.
+ *
+ * This function (or the equivalent hid_parse() macro) should only be
+ * called from probe() in drivers, before starting the device.
  */
-int hid_parse_report(struct hid_device *device, __u8 *start,
-		unsigned size)
+int hid_open_report(struct hid_device *device)
 {
 	struct hid_parser *parser;
 	struct hid_item item;
+	unsigned int size;
+	__u8 *start;
 	__u8 *end;
 	int ret;
 	static int (*dispatch_type[])(struct hid_parser *parser,
@@ -662,6 +706,14 @@ int hid_parse_report(struct hid_device *device, __u8 *start,
 		hid_parser_reserved
 	};
 
+	if (WARN_ON(device->status & HID_STAT_PARSED))
+		return -EBUSY;
+
+	start = device->dev_rdesc;
+	if (WARN_ON(!start))
+		return -ENODEV;
+	size = device->dev_rsize;
+
 	if (device->driver->report_fixup)
 		start = device->driver->report_fixup(device, start, &size);
 
@@ -679,6 +731,15 @@ int hid_parse_report(struct hid_device *device, __u8 *start,
 	parser->device = device;
 
 	end = start + size;
+
+	device->collection = kcalloc(HID_DEFAULT_NUM_COLLECTIONS,
+				     sizeof(struct hid_collection), GFP_KERNEL);
+	if (!device->collection) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	device->collection_size = HID_DEFAULT_NUM_COLLECTIONS;
+
 	ret = -EINVAL;
 	while ((start = fetch_item(start, end, &item)) != NULL) {
 
@@ -704,6 +765,7 @@ int hid_parse_report(struct hid_device *device, __u8 *start,
 				goto err;
 			}
 			vfree(parser);
+			device->status |= HID_STAT_PARSED;
 			return 0;
 		}
 	}
@@ -711,9 +773,10 @@ int hid_parse_report(struct hid_device *device, __u8 *start,
 	hid_err(device, "item fetching failed at offset %d\n", (int)(end - start));
 err:
 	vfree(parser);
+	hid_close_report(device);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(hid_parse_report);
+EXPORT_SYMBOL_GPL(hid_open_report);
 
 /*
  * Convert a signed n-bit integer to signed 32-bit integer. Common
@@ -1718,12 +1781,14 @@ static int hid_device_probe(struct device *dev)
 		if (hdrv->probe) {
 			ret = hdrv->probe(hdev, id);
 		} else { /* default probe */
-			ret = hid_parse(hdev);
+			ret = hid_open_report(hdev);
 			if (!ret)
 				ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 		}
-		if (ret)
+		if (ret) {
+			hid_close_report(hdev);
 			hdev->driver = NULL;
+		}
 	}
 unlock:
 	up(&hdev->driver_lock);
@@ -1744,6 +1809,7 @@ static int hid_device_remove(struct device *dev)
 			hdrv->remove(hdev);
 		else /* default remove */
 			hid_hw_stop(hdev);
+		hid_close_report(hdev);
 		hdev->driver = NULL;
 	}
 
@@ -2075,6 +2141,16 @@ int hid_add_device(struct hid_device *hdev)
             && (hid_ignore(hdev) || (hdev->quirks & HID_QUIRK_IGNORE)))
 		return -ENODEV;
 
+	/*
+	 * Read the device report descriptor once and use as template
+	 * for the driver-specific modifications.
+	 */
+	ret = hdev->ll_driver->parse(hdev);
+	if (ret)
+		return ret;
+	if (!hdev->dev_rdesc)
+		return -ENODEV;
+
 	/* XXX hack, any other cleaner solution after the driver core
 	 * is converted to allow more than 20 bytes as the device name? */
 	dev_set_name(&hdev->dev, "%04X:%04X:%04X.%04X", hdev->bus,
@@ -2103,7 +2179,6 @@ EXPORT_SYMBOL_GPL(hid_add_device);
 struct hid_device *hid_allocate_device(void)
 {
 	struct hid_device *hdev;
-	unsigned int i;
 	int ret = -ENOMEM;
 
 	hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
@@ -2114,23 +2189,13 @@ struct hid_device *hid_allocate_device(void)
 	hdev->dev.release = hid_device_release;
 	hdev->dev.bus = &hid_bus_type;
 
-	hdev->collection = kcalloc(HID_DEFAULT_NUM_COLLECTIONS,
-			sizeof(struct hid_collection), GFP_KERNEL);
-	if (hdev->collection == NULL)
-		goto err;
-	hdev->collection_size = HID_DEFAULT_NUM_COLLECTIONS;
-
-	for (i = 0; i < HID_REPORT_TYPES; i++)
-		INIT_LIST_HEAD(&hdev->report_enum[i].report_list);
+	hid_close_report(hdev);
 
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
 	sema_init(&hdev->driver_lock, 1);
 
 	return hdev;
-err:
-	put_device(&hdev->dev);
-	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(hid_allocate_device);
 
@@ -2141,6 +2206,9 @@ static void hid_remove_device(struct hid_device *hdev)
 		hid_debug_unregister(hdev);
 		hdev->status &= ~HID_STAT_ADDED;
 	}
+	kfree(hdev->dev_rdesc);
+	hdev->dev_rdesc = NULL;
+	hdev->dev_rsize = 0;
 }
 
 /**
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 3a95da6..d8e7cc7 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -467,6 +467,8 @@ struct hid_driver;
 struct hid_ll_driver;
 
 struct hid_device {							/* device report descriptor */
+	__u8 *dev_rdesc;
+	unsigned dev_rsize;
 	__u8 *rdesc;
 	unsigned rsize;
 	struct hid_collection *collection;				/* List of HID collections */
@@ -735,6 +737,7 @@ void hid_output_report(struct hid_report *report, __u8 *data);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
 int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
+int hid_open_report(struct hid_device *device);
 int hid_check_keys_pressed(struct hid_device *hid);
 int hid_connect(struct hid_device *hid, unsigned int connect_mask);
 void hid_disconnect(struct hid_device *hid);
@@ -805,16 +808,7 @@ static inline void hid_map_usage_clear(struct hid_input *hidinput,
  */
 static inline int __must_check hid_parse(struct hid_device *hdev)
 {
-	int ret;
-
-	if (hdev->status & HID_STAT_PARSED)
-		return 0;
-
-	ret = hdev->ll_driver->parse(hdev);
-	if (!ret)
-		hdev->status |= HID_STAT_PARSED;
-
-	return ret;
+	return hid_open_report(hdev);
 }
 
 /**
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-04-22 12:21 [PATCH 0/3] hid: Enable report fixup on rebind Henrik Rydberg
                   ` (2 preceding siblings ...)
  2012-04-22 12:21 ` [PATCH 3/3] hid: Handle driver-specific device descriptor in core Henrik Rydberg
@ 2012-04-22 21:27 ` Jiri Kosina
  2012-04-23  7:52   ` Nikolai Kondrashov
                     ` (2 more replies)
  3 siblings, 3 replies; 16+ messages in thread
From: Jiri Kosina @ 2012-04-22 21:27 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Dmitry Torokhov, linux-input, linux-kernel, Nikolai Kondrashov

On Sun, 22 Apr 2012, Henrik Rydberg wrote:

> This patchset contains a resolution to the problem with
> driver-specific modification of the report descriptor. The core of the
> problem lies with the dual semantics of hid_parse_report(), which is
> therefore split into two functions. As a consequence, the hid core can
> handle the rebind case internally, with no changes to the
> drivers. Out-of-tree drivers will work fine as long as they operate in
> the same way as the in-kernel drivers.
> 
> The first two patches are bug fixes found in the process. I am hoping
> for some test feedback on those. The third patch is the main one.

Hi Henrik,

thanks a lot for looking into this. I will look into the patches in more 
detail tomorrow. I am now just adding Nikolai to CC, as he was the first 
one to bring this up and actually has a driver that exposed the fact that 
just freeing/reinstatiating rdesc is not enough.

Thanks!

> 
> Henrik Rydberg (3):
>   hid-hyperv: Do not use hid_parse_report() directly
>   hid-logitech: Collect report descriptors before sending
>   hid: Handle driver-specific device descriptor in core
> 
>  drivers/hid/hid-core.c        |  112 +++++++++++++++++++++++++++++++++--------
>  drivers/hid/hid-hyperv.c      |   14 +++++-
>  drivers/hid/hid-logitech-dj.c |   71 +++++++++++---------------
>  include/linux/hid.h           |   14 ++----
>  4 files changed, 135 insertions(+), 76 deletions(-)
> 
> -- 
> 1.7.10
> 

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-04-22 21:27 ` [PATCH 0/3] hid: Enable report fixup on rebind Jiri Kosina
@ 2012-04-23  7:52   ` Nikolai Kondrashov
  2012-04-29 15:33   ` Nikolai Kondrashov
  2012-05-06 16:52   ` Nikolai Kondrashov
  2 siblings, 0 replies; 16+ messages in thread
From: Nikolai Kondrashov @ 2012-04-23  7:52 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: Henrik Rydberg, Dmitry Torokhov, linux-input, linux-kernel

On 04/23/2012 12:27 AM, Jiri Kosina wrote:
> On Sun, 22 Apr 2012, Henrik Rydberg wrote:
>> This patchset contains a resolution to the problem with
>> driver-specific modification of the report descriptor. The core of the
>> problem lies with the dual semantics of hid_parse_report(), which is
>> therefore split into two functions. As a consequence, the hid core can
>> handle the rebind case internally, with no changes to the
>> drivers. Out-of-tree drivers will work fine as long as they operate in
>> the same way as the in-kernel drivers.
>>
>> The first two patches are bug fixes found in the process. I am hoping
>> for some test feedback on those. The third patch is the main one.
>
> Hi Henrik,
>
> thanks a lot for looking into this. I will look into the patches in more
> detail tomorrow. I am now just adding Nikolai to CC, as he was the first
> one to bring this up and actually has a driver that exposed the fact that
> just freeing/reinstatiating rdesc is not enough.

Thanks Henrik, Jiri!

I'll be testing this as soon as I can.

Sincerely,
Nick

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly
  2012-04-22 12:21 ` [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly Henrik Rydberg
@ 2012-04-27  8:53   ` Jiri Kosina
  2012-04-30 20:15     ` KY Srinivasan
  0 siblings, 1 reply; 16+ messages in thread
From: Jiri Kosina @ 2012-04-27  8:53 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Dmitry Torokhov, linux-input, linux-kernel, K. Y. Srinivasan

On Sun, 22 Apr 2012, Henrik Rydberg wrote:

> Upcoming changes will split the semantics of hid_parse_report()
> and hid_parse(), so make sure drivers use hid_parse() in probe().
> 
> Compiled, not tested.
> 
> Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

KY, could I please get your Ack/Tested-by for this patch?

Thanks,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-04-22 21:27 ` [PATCH 0/3] hid: Enable report fixup on rebind Jiri Kosina
  2012-04-23  7:52   ` Nikolai Kondrashov
@ 2012-04-29 15:33   ` Nikolai Kondrashov
  2012-04-30 12:06     ` Henrik Rydberg
  2012-05-06 16:52   ` Nikolai Kondrashov
  2 siblings, 1 reply; 16+ messages in thread
From: Nikolai Kondrashov @ 2012-04-29 15:33 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: Henrik Rydberg, Dmitry Torokhov, linux-input, linux-kernel

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

On 04/23/2012 12:27 AM, Jiri Kosina wrote:
> On Sun, 22 Apr 2012, Henrik Rydberg wrote:
>
>> This patchset contains a resolution to the problem with
>> driver-specific modification of the report descriptor. The core of the
>> problem lies with the dual semantics of hid_parse_report(), which is
>> therefore split into two functions. As a consequence, the hid core can
>> handle the rebind case internally, with no changes to the
>> drivers. Out-of-tree drivers will work fine as long as they operate in
>> the same way as the in-kernel drivers.
>>
>> The first two patches are bug fixes found in the process. I am hoping
>> for some test feedback on those. The third patch is the main one.

I've tested the last patch and it seems to work as required. I did a series
of generic<->specific driver rebindings both ways in a loop and it works.

So,
Tested-by: Nikolai Kondrashov <spbnick@gmail.com>

Thanks Henrik!

I'm not sure, though, if one thing works as it should: the quirks are not
reset when changing the driver. So, quirks set by one driver may affect
behavior of another. I don't understand full quirks usage and ownership yet,
so can't argue if this is right or wrong.

Regarding the code, could it be the time to fix naming of report
descriptor-handling functions to improve distinction from report-handling
ones? I'm attaching a patch applying on top of Henrik's changes which should
illustrate the proposal.

Thanks!

Sincerely,
Nick

[-- Attachment #2: 0001-hid-Differentiate-report-descriptors-and-reports.patch --]
[-- Type: text/x-patch, Size: 26161 bytes --]

>From 2423f42177f13d1b27eac8e4168c39afbd937114 Mon Sep 17 00:00:00 2001
From: Nikolai Kondrashov <spbnick@gmail.com>
Date: Sun, 29 Apr 2012 17:51:21 +0300
Subject: [PATCH 1/1] hid: Differentiate report descriptors and reports

Improve distinction between report descriptors and reports:
  * rename hid_(open|parse|close)_report to hid_(open|parse|close)_rdesc,
  * rename "report_fixup" hid_driver callback to "rdesc_fixup",
  * refine relevant comments.

Signed-off-by: Nikolai Kondrashov <spbnick@gmail.com>
---
 drivers/hid/hid-apple.c       |    4 ++--
 drivers/hid/hid-cherry.c      |    4 ++--
 drivers/hid/hid-core.c        |   44 ++++++++++++++++++++---------------------
 drivers/hid/hid-cypress.c     |    4 ++--
 drivers/hid/hid-dr.c          |    4 ++--
 drivers/hid/hid-elecom.c      |    4 ++--
 drivers/hid/hid-hyperv.c      |    2 +-
 drivers/hid/hid-keytouch.c    |    4 ++--
 drivers/hid/hid-kye.c         |    4 ++--
 drivers/hid/hid-lg.c          |    4 ++--
 drivers/hid/hid-logitech-dj.c |    2 +-
 drivers/hid/hid-microsoft.c   |    4 ++--
 drivers/hid/hid-monterey.c    |    4 ++--
 drivers/hid/hid-ortek.c       |    4 ++--
 drivers/hid/hid-petalynx.c    |    4 ++--
 drivers/hid/hid-prodikeys.c   |    4 ++--
 drivers/hid/hid-saitek.c      |    4 ++--
 drivers/hid/hid-samsung.c     |    8 ++++----
 drivers/hid/hid-sony.c        |    4 ++--
 drivers/hid/hid-sunplus.c     |    4 ++--
 drivers/hid/hid-uclogic.c     |    4 ++--
 drivers/hid/hid-waltop.c      |    4 ++--
 drivers/hid/hid-zydacron.c    |    4 ++--
 drivers/hid/usbhid/hid-core.c |    2 +-
 include/linux/hid.h           |   18 ++++++++---------
 net/bluetooth/hidp/core.c     |    2 +-
 26 files changed, 77 insertions(+), 77 deletions(-)

diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
index 299d238..c2a1641 100644
--- a/drivers/hid/hid-apple.c
+++ b/drivers/hid/hid-apple.c
@@ -275,7 +275,7 @@ static int apple_event(struct hid_device *hdev, struct hid_field *field,
 /*
  * MacBook JIS keyboard has wrong logical maximum
  */
-static __u8 *apple_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *apple_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	struct apple_sc *asc = hid_get_drvdata(hdev);
@@ -533,7 +533,7 @@ MODULE_DEVICE_TABLE(hid, apple_devices);
 static struct hid_driver apple_driver = {
 	.name = "apple",
 	.id_table = apple_devices,
-	.report_fixup = apple_report_fixup,
+	.rdesc_fixup = apple_rdesc_fixup,
 	.probe = apple_probe,
 	.remove = apple_remove,
 	.event = apple_event,
diff --git a/drivers/hid/hid-cherry.c b/drivers/hid/hid-cherry.c
index 888ece6..3c42b66 100644
--- a/drivers/hid/hid-cherry.c
+++ b/drivers/hid/hid-cherry.c
@@ -26,7 +26,7 @@
  * Cherry Cymotion keyboard have an invalid HID report descriptor,
  * that needs fixing before we can parse it.
  */
-static __u8 *ch_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *ch_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize >= 17 && rdesc[11] == 0x3c && rdesc[12] == 0x02) {
@@ -67,7 +67,7 @@ MODULE_DEVICE_TABLE(hid, ch_devices);
 static struct hid_driver ch_driver = {
 	.name = "cherry",
 	.id_table = ch_devices,
-	.report_fixup = ch_report_fixup,
+	.rdesc_fixup = ch_rdesc_fixup,
 	.input_mapping = ch_input_mapping,
 };
 
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 9ed4ff3..e44d588 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -546,10 +546,10 @@ static void hid_free_report(struct hid_report *report)
 }
 
 /*
- * Close report. This function returns the device
- * state to the point prior to hid_open_report().
+ * Close report descriptor. This function returns the device
+ * state to the point prior to hid_open_rdesc().
  */
-static void hid_close_report(struct hid_device *device)
+static void hid_close_rdesc(struct hid_device *device)
 {
 	unsigned i, j;
 
@@ -586,7 +586,7 @@ static void hid_device_release(struct device *dev)
 {
 	struct hid_device *hid = container_of(dev, struct hid_device, dev);
 
-	hid_close_report(hid);
+	hid_close_rdesc(hid);
 	kfree(hid->dev_rdesc);
 	kfree(hid);
 }
@@ -659,16 +659,16 @@ static u8 *fetch_item(__u8 *start, __u8 *end, struct hid_item *item)
 }
 
 /**
- * hid_parse_report - parse device report
+ * hid_parse_rdesc - parse device report descriptor
  *
  * @device: hid device
- * @start: report start
- * @size: report size
+ * @start: report descriptor start
+ * @size: report descriptor size
  *
- * Allocate the device report as read by the bus driver. This function should
- * only be called from parse() in ll drivers.
+ * Allocate the device report descriptor as read by the bus driver.
+ * This function should only be called from parse() in ll drivers.
  */
-int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size)
+int hid_parse_rdesc(struct hid_device *hid, __u8 *start, unsigned size)
 {
 	hid->dev_rdesc = kmemdup(start, size, GFP_KERNEL);
 	if (!hid->dev_rdesc)
@@ -676,21 +676,21 @@ int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size)
 	hid->dev_rsize = size;
 	return 0;
 }
-EXPORT_SYMBOL_GPL(hid_parse_report);
+EXPORT_SYMBOL_GPL(hid_parse_rdesc);
 
 /**
- * hid_open_report - open a driver-specific device report
+ * hid_open_rdesc - open a driver-specific device report descriptor
  *
  * @device: hid device
  *
- * Parse a report description into a hid_device structure. Reports are
+ * Parse a report descriptor into a hid_device structure. Reports are
  * enumerated, fields are attached to these reports.
  * 0 returned on success, otherwise nonzero error value.
  *
  * This function (or the equivalent hid_parse() macro) should only be
  * called from probe() in drivers, before starting the device.
  */
-int hid_open_report(struct hid_device *device)
+int hid_open_rdesc(struct hid_device *device)
 {
 	struct hid_parser *parser;
 	struct hid_item item;
@@ -714,8 +714,8 @@ int hid_open_report(struct hid_device *device)
 		return -ENODEV;
 	size = device->dev_rsize;
 
-	if (device->driver->report_fixup)
-		start = device->driver->report_fixup(device, start, &size);
+	if (device->driver->rdesc_fixup)
+		start = device->driver->rdesc_fixup(device, start, &size);
 
 	device->rdesc = kmemdup(start, size, GFP_KERNEL);
 	if (device->rdesc == NULL)
@@ -773,10 +773,10 @@ int hid_open_report(struct hid_device *device)
 	hid_err(device, "item fetching failed at offset %d\n", (int)(end - start));
 err:
 	vfree(parser);
-	hid_close_report(device);
+	hid_close_rdesc(device);
 	return ret;
 }
-EXPORT_SYMBOL_GPL(hid_open_report);
+EXPORT_SYMBOL_GPL(hid_open_rdesc);
 
 /*
  * Convert a signed n-bit integer to signed 32-bit integer. Common
@@ -1781,12 +1781,12 @@ static int hid_device_probe(struct device *dev)
 		if (hdrv->probe) {
 			ret = hdrv->probe(hdev, id);
 		} else { /* default probe */
-			ret = hid_open_report(hdev);
+			ret = hid_open_rdesc(hdev);
 			if (!ret)
 				ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 		}
 		if (ret) {
-			hid_close_report(hdev);
+			hid_close_rdesc(hdev);
 			hdev->driver = NULL;
 		}
 	}
@@ -1809,7 +1809,7 @@ static int hid_device_remove(struct device *dev)
 			hdrv->remove(hdev);
 		else /* default remove */
 			hid_hw_stop(hdev);
-		hid_close_report(hdev);
+		hid_close_rdesc(hdev);
 		hdev->driver = NULL;
 	}
 
@@ -2189,7 +2189,7 @@ struct hid_device *hid_allocate_device(void)
 	hdev->dev.release = hid_device_release;
 	hdev->dev.bus = &hid_bus_type;
 
-	hid_close_report(hdev);
+	hid_close_rdesc(hdev);
 
 	init_waitqueue_head(&hdev->debug_wait);
 	INIT_LIST_HEAD(&hdev->debug_list);
diff --git a/drivers/hid/hid-cypress.c b/drivers/hid/hid-cypress.c
index 2f0be4c..7af1f12 100644
--- a/drivers/hid/hid-cypress.c
+++ b/drivers/hid/hid-cypress.c
@@ -31,7 +31,7 @@
  * Some USB barcode readers from cypress have usage min and usage max in
  * the wrong order
  */
-static __u8 *cp_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *cp_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
@@ -138,7 +138,7 @@ MODULE_DEVICE_TABLE(hid, cp_devices);
 static struct hid_driver cp_driver = {
 	.name = "cypress",
 	.id_table = cp_devices,
-	.report_fixup = cp_report_fixup,
+	.rdesc_fixup = cp_rdesc_fixup,
 	.input_mapped = cp_input_mapped,
 	.event = cp_event,
 	.probe = cp_probe,
diff --git a/drivers/hid/hid-dr.c b/drivers/hid/hid-dr.c
index e832f44..01b9043 100644
--- a/drivers/hid/hid-dr.c
+++ b/drivers/hid/hid-dr.c
@@ -236,7 +236,7 @@ static __u8 pid0011_rdesc_fixed[] = {
 	0xC0                /*  End Collection                  */
 };
 
-static __u8 *dr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *dr_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 				unsigned int *rsize)
 {
 	switch (hdev->product) {
@@ -294,7 +294,7 @@ MODULE_DEVICE_TABLE(hid, dr_devices);
 static struct hid_driver dr_driver = {
 	.name = "dragonrise",
 	.id_table = dr_devices,
-	.report_fixup = dr_report_fixup,
+	.rdesc_fixup = dr_rdesc_fixup,
 	.probe = dr_probe,
 };
 
diff --git a/drivers/hid/hid-elecom.c b/drivers/hid/hid-elecom.c
index 79d0c61..37ac28e 100644
--- a/drivers/hid/hid-elecom.c
+++ b/drivers/hid/hid-elecom.c
@@ -20,7 +20,7 @@
 
 #include "hid-ids.h"
 
-static __u8 *elecom_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *elecom_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize >= 48 && rdesc[46] == 0x05 && rdesc[47] == 0x0c) {
@@ -39,7 +39,7 @@ MODULE_DEVICE_TABLE(hid, elecom_devices);
 static struct hid_driver elecom_driver = {
 	.name = "elecom",
 	.id_table = elecom_devices,
-	.report_fixup = elecom_report_fixup
+	.rdesc_fixup = elecom_rdesc_fixup
 };
 
 static int __init elecom_init(void)
diff --git a/drivers/hid/hid-hyperv.c b/drivers/hid/hid-hyperv.c
index 032e6c0..406c02c 100644
--- a/drivers/hid/hid-hyperv.c
+++ b/drivers/hid/hid-hyperv.c
@@ -435,7 +435,7 @@ static int mousevsc_hid_parse(struct hid_device *hid)
 	struct hv_device *dev = hid_get_drvdata(hid);
 	struct mousevsc_dev *input_dev = hv_get_drvdata(dev);
 
-	return hid_parse_report(hid, input_dev->report_desc,
+	return hid_parse_rdesc(hid, input_dev->report_desc,
 				input_dev->report_desc_size);
 }
 
diff --git a/drivers/hid/hid-keytouch.c b/drivers/hid/hid-keytouch.c
index 07cd825..2a2a49c 100644
--- a/drivers/hid/hid-keytouch.c
+++ b/drivers/hid/hid-keytouch.c
@@ -27,7 +27,7 @@ static __u8 keytouch_fixed_rdesc[] = {
 0x26, 0xff, 0x00, 0x05, 0x07, 0x19, 0x00, 0x2a, 0xff, 0x00, 0x81, 0x00, 0xc0
 };
 
-static __u8 *keytouch_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *keytouch_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	hid_info(hdev, "fixing up Keytouch IEC report descriptor\n");
@@ -47,7 +47,7 @@ MODULE_DEVICE_TABLE(hid, keytouch_devices);
 static struct hid_driver keytouch_driver = {
 	.name = "keytouch",
 	.id_table = keytouch_devices,
-	.report_fixup = keytouch_report_fixup,
+	.rdesc_fixup = keytouch_rdesc_fixup,
 };
 
 static int __init keytouch_init(void)
diff --git a/drivers/hid/hid-kye.c b/drivers/hid/hid-kye.c
index b4f0d82..1eca937 100644
--- a/drivers/hid/hid-kye.c
+++ b/drivers/hid/hid-kye.c
@@ -270,7 +270,7 @@ static __u8 easypen_m610x_rdesc_fixed[] = {
 	0xC0                          /*  End Collection                  */
 };
 
-static __u8 *kye_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *kye_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	switch (hdev->product) {
@@ -417,7 +417,7 @@ static struct hid_driver kye_driver = {
 	.name = "kye",
 	.id_table = kye_devices,
 	.probe = kye_probe,
-	.report_fixup = kye_report_fixup,
+	.rdesc_fixup = kye_rdesc_fixup,
 };
 
 static int __init kye_init(void)
diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
index e7a7bd1..16e3a56 100644
--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -106,7 +106,7 @@ static __u8 dfp_rdesc_fixed[] = {
  * above the logical maximum described in descriptor. This extends
  * the original value of 0x28c of logical maximum to 0x104d
  */
-static __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *lg_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
@@ -485,7 +485,7 @@ MODULE_DEVICE_TABLE(hid, lg_devices);
 static struct hid_driver lg_driver = {
 	.name = "logitech",
 	.id_table = lg_devices,
-	.report_fixup = lg_report_fixup,
+	.rdesc_fixup = lg_rdesc_fixup,
 	.input_mapping = lg_input_mapping,
 	.input_mapped = lg_input_mapped,
 	.event = lg_event,
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index e1c38bb..523d38a 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -538,7 +538,7 @@ static int logi_dj_ll_parse(struct hid_device *hid)
 			__func__, djdev->reports_supported);
 	}
 
-	retval = hid_parse_report(hid, rdesc, rsize);
+	retval = hid_parse_rdesc(hid, rdesc, rsize);
 	kfree(rdesc);
 
 	return retval;
diff --git a/drivers/hid/hid-microsoft.c b/drivers/hid/hid-microsoft.c
index e5c699b..43b1ad4 100644
--- a/drivers/hid/hid-microsoft.c
+++ b/drivers/hid/hid-microsoft.c
@@ -34,7 +34,7 @@
  * Microsoft Wireless Desktop Receiver (Model 1028) has
  * 'Usage Min/Max' where it ought to have 'Physical Min/Max'
  */
-static __u8 *ms_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *ms_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	unsigned long quirks = (unsigned long)hid_get_drvdata(hdev);
@@ -208,7 +208,7 @@ MODULE_DEVICE_TABLE(hid, ms_devices);
 static struct hid_driver ms_driver = {
 	.name = "microsoft",
 	.id_table = ms_devices,
-	.report_fixup = ms_report_fixup,
+	.rdesc_fixup = ms_rdesc_fixup,
 	.input_mapping = ms_input_mapping,
 	.input_mapped = ms_input_mapped,
 	.event = ms_event,
diff --git a/drivers/hid/hid-monterey.c b/drivers/hid/hid-monterey.c
index dedf757..f0b739a 100644
--- a/drivers/hid/hid-monterey.c
+++ b/drivers/hid/hid-monterey.c
@@ -22,7 +22,7 @@
 
 #include "hid-ids.h"
 
-static __u8 *mr_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *mr_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize >= 30 && rdesc[29] == 0x05 && rdesc[30] == 0x09) {
@@ -61,7 +61,7 @@ MODULE_DEVICE_TABLE(hid, mr_devices);
 static struct hid_driver mr_driver = {
 	.name = "monterey",
 	.id_table = mr_devices,
-	.report_fixup = mr_report_fixup,
+	.rdesc_fixup = mr_rdesc_fixup,
 	.input_mapping = mr_input_mapping,
 };
 
diff --git a/drivers/hid/hid-ortek.c b/drivers/hid/hid-ortek.c
index 0ffa1d2..697d463 100644
--- a/drivers/hid/hid-ortek.c
+++ b/drivers/hid/hid-ortek.c
@@ -24,7 +24,7 @@
 
 #include "hid-ids.h"
 
-static __u8 *ortek_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *ortek_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize >= 56 && rdesc[54] == 0x25 && rdesc[55] == 0x01) {
@@ -48,7 +48,7 @@ MODULE_DEVICE_TABLE(hid, ortek_devices);
 static struct hid_driver ortek_driver = {
 	.name = "ortek",
 	.id_table = ortek_devices,
-	.report_fixup = ortek_report_fixup
+	.rdesc_fixup = ortek_rdesc_fixup
 };
 
 static int __init ortek_init(void)
diff --git a/drivers/hid/hid-petalynx.c b/drivers/hid/hid-petalynx.c
index f1ea3ff..1ca2821e 100644
--- a/drivers/hid/hid-petalynx.c
+++ b/drivers/hid/hid-petalynx.c
@@ -23,7 +23,7 @@
 #include "hid-ids.h"
 
 /* Petalynx Maxter Remote has maximum for consumer page set too low */
-static __u8 *pl_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *pl_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize >= 60 && rdesc[39] == 0x2a && rdesc[40] == 0xf5 &&
@@ -100,7 +100,7 @@ MODULE_DEVICE_TABLE(hid, pl_devices);
 static struct hid_driver pl_driver = {
 	.name = "petalynx",
 	.id_table = pl_devices,
-	.report_fixup = pl_report_fixup,
+	.rdesc_fixup = pl_rdesc_fixup,
 	.input_mapping = pl_input_mapping,
 	.probe = pl_probe,
 };
diff --git a/drivers/hid/hid-prodikeys.c b/drivers/hid/hid-prodikeys.c
index b71b77a..f9f6d8b 100644
--- a/drivers/hid/hid-prodikeys.c
+++ b/drivers/hid/hid-prodikeys.c
@@ -741,7 +741,7 @@ static int pcmidi_snd_terminate(struct pcmidi_snd *pm)
 /*
  * PC-MIDI report descriptor for report id is wrong.
  */
-static __u8 *pk_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *pk_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize == 178 &&
@@ -883,7 +883,7 @@ MODULE_DEVICE_TABLE(hid, pk_devices);
 static struct hid_driver pk_driver = {
 	.name = "prodikeys",
 	.id_table = pk_devices,
-	.report_fixup = pk_report_fixup,
+	.rdesc_fixup = pk_rdesc_fixup,
 	.input_mapping = pk_input_mapping,
 	.raw_event = pk_raw_event,
 	.probe = pk_probe,
diff --git a/drivers/hid/hid-saitek.c b/drivers/hid/hid-saitek.c
index 45aea77..30efbfe 100644
--- a/drivers/hid/hid-saitek.c
+++ b/drivers/hid/hid-saitek.c
@@ -21,7 +21,7 @@
 
 #include "hid-ids.h"
 
-static __u8 *saitek_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *saitek_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize == 137 && rdesc[20] == 0x09 && rdesc[21] == 0x33
@@ -52,7 +52,7 @@ MODULE_DEVICE_TABLE(hid, saitek_devices);
 static struct hid_driver saitek_driver = {
 	.name = "saitek",
 	.id_table = saitek_devices,
-	.report_fixup = saitek_report_fixup
+	.rdesc_fixup = saitek_rdesc_fixup
 };
 
 static int __init saitek_init(void)
diff --git a/drivers/hid/hid-samsung.c b/drivers/hid/hid-samsung.c
index 3c1fd8a..1b2ba50 100644
--- a/drivers/hid/hid-samsung.c
+++ b/drivers/hid/hid-samsung.c
@@ -61,7 +61,7 @@ static inline void samsung_irda_dev_trace(struct hid_device *hdev,
 		 rsize);
 }
 
-static __u8 *samsung_irda_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *samsung_irda_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize == 184 && rdesc[175] == 0x25 && rdesc[176] == 0x40 &&
@@ -131,11 +131,11 @@ static int samsung_kbd_mouse_input_mapping(struct hid_device *hdev,
 	return 1;
 }
 
-static __u8 *samsung_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *samsung_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 	unsigned int *rsize)
 {
 	if (USB_DEVICE_ID_SAMSUNG_IR_REMOTE == hdev->product)
-		rdesc = samsung_irda_report_fixup(hdev, rdesc, rsize);
+		rdesc = samsung_irda_rdesc_fixup(hdev, rdesc, rsize);
 	return rdesc;
 }
 
@@ -193,7 +193,7 @@ MODULE_DEVICE_TABLE(hid, samsung_devices);
 static struct hid_driver samsung_driver = {
 	.name = "samsung",
 	.id_table = samsung_devices,
-	.report_fixup = samsung_report_fixup,
+	.rdesc_fixup = samsung_rdesc_fixup,
 	.input_mapping = samsung_input_mapping,
 	.probe = samsung_probe,
 };
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 5cd25bd..0254b4e 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -39,7 +39,7 @@ struct sony_sc {
 };
 
 /* Sony Vaio VGX has wrongly mouse pointer declared as constant */
-static __u8 *sony_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *sony_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
@@ -227,7 +227,7 @@ static struct hid_driver sony_driver = {
 	.id_table = sony_devices,
 	.probe = sony_probe,
 	.remove = sony_remove,
-	.report_fixup = sony_report_fixup,
+	.rdesc_fixup = sony_rdesc_fixup,
 	.raw_event = sony_raw_event
 };
 
diff --git a/drivers/hid/hid-sunplus.c b/drivers/hid/hid-sunplus.c
index d484a00..28bdc26 100644
--- a/drivers/hid/hid-sunplus.c
+++ b/drivers/hid/hid-sunplus.c
@@ -22,7 +22,7 @@
 
 #include "hid-ids.h"
 
-static __u8 *sp_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *sp_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	if (*rsize >= 107 && rdesc[104] == 0x26 && rdesc[105] == 0x80 &&
@@ -61,7 +61,7 @@ MODULE_DEVICE_TABLE(hid, sp_devices);
 static struct hid_driver sp_driver = {
 	.name = "sunplus",
 	.id_table = sp_devices,
-	.report_fixup = sp_report_fixup,
+	.rdesc_fixup = sp_rdesc_fixup,
 	.input_mapping = sp_input_mapping,
 };
 
diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index 1f11289..fad6076 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -352,7 +352,7 @@ static __u8 pf1209_rdesc_fixed[] = {
 	0xC0                /*  End Collection                      */
 };
 
-static __u8 *uclogic_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *uclogic_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 					unsigned int *rsize)
 {
 	switch (hdev->product) {
@@ -409,7 +409,7 @@ MODULE_DEVICE_TABLE(hid, uclogic_devices);
 static struct hid_driver uclogic_driver = {
 	.name = "uclogic",
 	.id_table = uclogic_devices,
-	.report_fixup = uclogic_report_fixup,
+	.rdesc_fixup = uclogic_rdesc_fixup,
 };
 
 static int __init uclogic_init(void)
diff --git a/drivers/hid/hid-waltop.c b/drivers/hid/hid-waltop.c
index 2cfd95c..3314eae 100644
--- a/drivers/hid/hid-waltop.c
+++ b/drivers/hid/hid-waltop.c
@@ -543,7 +543,7 @@ err:
 	return ret;
 }
 
-static __u8 *waltop_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *waltop_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 		unsigned int *rsize)
 {
 	switch (hdev->product) {
@@ -646,7 +646,7 @@ static struct hid_driver waltop_driver = {
 	.name = "waltop",
 	.id_table = waltop_devices,
 	.probe = waltop_probe,
-	.report_fixup = waltop_report_fixup,
+	.rdesc_fixup = waltop_rdesc_fixup,
 	.raw_event = waltop_raw_event,
 	.remove = waltop_remove,
 };
diff --git a/drivers/hid/hid-zydacron.c b/drivers/hid/hid-zydacron.c
index 1ad85f2..7e6adc1 100644
--- a/drivers/hid/hid-zydacron.c
+++ b/drivers/hid/hid-zydacron.c
@@ -27,7 +27,7 @@ struct zc_device {
 * Zydacron remote control has an invalid HID report descriptor,
 * that needs fixing before we can parse it.
 */
-static __u8 *zc_report_fixup(struct hid_device *hdev, __u8 *rdesc,
+static __u8 *zc_rdesc_fixup(struct hid_device *hdev, __u8 *rdesc,
 	unsigned int *rsize)
 {
 	if (*rsize >= 253 &&
@@ -213,7 +213,7 @@ MODULE_DEVICE_TABLE(hid, zc_devices);
 static struct hid_driver zc_driver = {
 	.name = "zydacron",
 	.id_table = zc_devices,
-	.report_fixup = zc_report_fixup,
+	.rdesc_fixup = zc_rdesc_fixup,
 	.input_mapping = zc_input_mapping,
 	.raw_event = zc_raw_event,
 	.probe = zc_probe,
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 5bf91db..6f0f556 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1009,7 +1009,7 @@ static int usbhid_parse(struct hid_device *hid)
 		goto err;
 	}
 
-	ret = hid_parse_report(hid, rdesc, rsize);
+	ret = hid_parse_rdesc(hid, rdesc, rsize);
 	kfree(rdesc);
 	if (ret) {
 		dbg_hid("parsing report descriptor failed\n");
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d8e7cc7..e1c083a 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -466,7 +466,7 @@ enum hid_type {
 struct hid_driver;
 struct hid_ll_driver;
 
-struct hid_device {							/* device report descriptor */
+struct hid_device {
 	__u8 *dev_rdesc;
 	unsigned dev_rsize;
 	__u8 *rdesc;
@@ -616,7 +616,7 @@ struct hid_usage_id {
  * @raw_event: if report in report_table, this hook is called (NULL means nop)
  * @usage_table: on which events to call event (NULL means all)
  * @event: if usage in usage_table, this hook is called (NULL means nop)
- * @report_fixup: called before report descriptor parsing (NULL means nop)
+ * @rdesc_fixup: called before report descriptor parsing (NULL means nop)
  * @input_mapping: invoked on input registering before mapping an usage
  * @input_mapped: invoked on input registering after mapping an usage
  * @feature_mapping: invoked on feature registering
@@ -654,7 +654,7 @@ struct hid_driver {
 	int (*event)(struct hid_device *hdev, struct hid_field *field,
 			struct hid_usage *usage, __s32 value);
 
-	__u8 *(*report_fixup)(struct hid_device *hdev, __u8 *buf,
+	__u8 *(*rdesc_fixup)(struct hid_device *hdev, __u8 *buf,
 			unsigned int *size);
 
 	int (*input_mapping)(struct hid_device *hdev,
@@ -736,8 +736,8 @@ unsigned int hidinput_count_leds(struct hid_device *hid);
 void hid_output_report(struct hid_report *report, __u8 *data);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
-int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
-int hid_open_report(struct hid_device *device);
+int hid_parse_rdesc(struct hid_device *hid, __u8 *start, unsigned size);
+int hid_open_rdesc(struct hid_device *device);
 int hid_check_keys_pressed(struct hid_device *hid);
 int hid_connect(struct hid_device *hid, unsigned int connect_mask);
 void hid_disconnect(struct hid_device *hid);
@@ -798,17 +798,17 @@ static inline void hid_map_usage_clear(struct hid_input *hidinput,
 }
 
 /**
- * hid_parse - parse HW reports
+ * hid_parse - parse HW report descriptors
  *
  * @hdev: hid device
  *
  * Call this from probe after you set up the device (if needed). Your
- * report_fixup will be called (if non-NULL) after reading raw report from
- * device before passing it to hid layer for real parsing.
+ * rdesc_fixup will be called (if non-NULL) after reading raw report descriptor
+ * from device before passing it to hid layer for real parsing.
  */
 static inline int __must_check hid_parse(struct hid_device *hdev)
 {
-	return hid_open_report(hdev);
+	return hid_open_rdesc(hdev);
 }
 
 /**
diff --git a/net/bluetooth/hidp/core.c b/net/bluetooth/hidp/core.c
index d478be1..0e6ad45 100644
--- a/net/bluetooth/hidp/core.c
+++ b/net/bluetooth/hidp/core.c
@@ -873,7 +873,7 @@ static int hidp_parse(struct hid_device *hid)
 {
 	struct hidp_session *session = hid->driver_data;
 
-	return hid_parse_report(session->hid, session->rd_data,
+	return hid_parse_rdesc(session->hid, session->rd_data,
 			session->rd_size);
 }
 
-- 
1.7.10


^ permalink raw reply related	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-04-29 15:33   ` Nikolai Kondrashov
@ 2012-04-30 12:06     ` Henrik Rydberg
  2012-04-30 12:06       ` Jiri Kosina
  2012-04-30 12:25       ` Nikolai Kondrashov
  0 siblings, 2 replies; 16+ messages in thread
From: Henrik Rydberg @ 2012-04-30 12:06 UTC (permalink / raw)
  To: Nikolai Kondrashov
  Cc: Jiri Kosina, Dmitry Torokhov, linux-input, linux-kernel

Hi Nikolai,

> >>The first two patches are bug fixes found in the process. I am hoping
> >>for some test feedback on those. The third patch is the main one.
> 
> I've tested the last patch and it seems to work as required. I did a series
> of generic<->specific driver rebindings both ways in a loop and it works.
> 
> So,
> Tested-by: Nikolai Kondrashov <spbnick@gmail.com>

Great, thanks!

> I'm not sure, though, if one thing works as it should: the quirks are not
> reset when changing the driver. So, quirks set by one driver may affect
> behavior of another. I don't understand full quirks usage and ownership yet,
> so can't argue if this is right or wrong.

The common quirks are a device property, so this is fine. There are
some drivers with quirks too, but they handle those themselves.

> Regarding the code, could it be the time to fix naming of report
> descriptor-handling functions to improve distinction from report-handling
> ones? I'm attaching a patch applying on top of Henrik's changes which should
> illustrate the proposal.

The changes you propose force changes to out-of-tree-drivers, without
any obvious benefit. Also, there is already a large set of patches on
review based on this one. So although I agree that some renames and
movements could clarify things, the content and timing will need some
more thought.

Thanks,
Henrik

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-04-30 12:06     ` Henrik Rydberg
@ 2012-04-30 12:06       ` Jiri Kosina
  2012-04-30 12:25       ` Nikolai Kondrashov
  1 sibling, 0 replies; 16+ messages in thread
From: Jiri Kosina @ 2012-04-30 12:06 UTC (permalink / raw)
  To: Henrik Rydberg
  Cc: Nikolai Kondrashov, Dmitry Torokhov, linux-input, linux-kernel

On Mon, 30 Apr 2012, Henrik Rydberg wrote:

> > Regarding the code, could it be the time to fix naming of report
> > descriptor-handling functions to improve distinction from report-handling
> > ones? I'm attaching a patch applying on top of Henrik's changes which should
> > illustrate the proposal.
> 
> The changes you propose force changes to out-of-tree-drivers, without
> any obvious benefit. 

I agree that *_parse_rdesc() would be much better than *_parse_report() 
(we have been having this slight misnomer in the HID code since ever).

I wouldn't care too much about out-of-tree drivers, they have to adapt 
gradually to our random HID API changes anyway.

Thanks to both of you,

-- 
Jiri Kosina
SUSE Labs

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-04-30 12:06     ` Henrik Rydberg
  2012-04-30 12:06       ` Jiri Kosina
@ 2012-04-30 12:25       ` Nikolai Kondrashov
  1 sibling, 0 replies; 16+ messages in thread
From: Nikolai Kondrashov @ 2012-04-30 12:25 UTC (permalink / raw)
  To: Henrik Rydberg; +Cc: Jiri Kosina, Dmitry Torokhov, linux-input, linux-kernel

Hi Henrik,

On Mon, Apr 30, 2012 at 3:06 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
> The changes you propose force changes to out-of-tree-drivers, without
> any obvious benefit. Also, there is already a large set of patches on
> review based on this one. So although I agree that some renames and
> movements could clarify things, the content and timing will need some
> more thought.

OK, taking into account Jiri's comment also, I'll retry this change
after things settle down a bit.

Thanks!

Sincerely,
Nick

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] hid-logitech: Collect report descriptors before sending
       [not found]   ` <CAE7qMrqEJmBWo7JDHuB2DnWJ4vkvv9jya7RDJguiXsrs2xgadQ@mail.gmail.com>
@ 2012-04-30 14:44     ` Benjamin Tissoires
  2012-04-30 18:14       ` Henrik Rydberg
  0 siblings, 1 reply; 16+ messages in thread
From: Benjamin Tissoires @ 2012-04-30 14:44 UTC (permalink / raw)
  To: Nestor Lopez Casado
  Cc: Henrik Rydberg, Jiri Kosina, Dmitry Torokhov, linux-input, linux-kernel

Hello Henrik,

I've been able to test the changes with different Logitech mice and keyboards.

So Jiri,
Tested-By: Benjamin Tissoires <benjamin.tissoires@gmail.com
for this change and this patch and the 3rd one (Hid: Handle
driver-specific device descriptor in core).

Cheers,
Benjamin

On Mon, Apr 23, 2012 at 15:23, Nestor Lopez Casado
<nlopezcasad@logitech.com> wrote:
> This change looks good to me. I can't test right now, but I will check as
> soon as I have some time to work on our kernel drivers again.
>
> Nestor.
>
>
> On Sun, Apr 22, 2012 at 2:21 PM, Henrik Rydberg <rydberg@euromail.se> wrote:
>>
>> The current code allows several consecutive calls to hid_parse_report(),
>> which may have happened to work before, but would cause a memory leak
>> and generally be incorrect. This patch collects all the reports
>> before sending them once.
>>
>> Compiled, not tested.
>>
>> Cc: Nestor Lopez Casado <nlopezcasad@logitech.com>
>> Cc: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
>> ---
>>  drivers/hid/hid-logitech-dj.c |   71
>> +++++++++++++++++------------------------
>>  1 file changed, 29 insertions(+), 42 deletions(-)
>>
>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> index 2b56efc..e1c38bb 100644
>> --- a/drivers/hid/hid-logitech-dj.c
>> +++ b/drivers/hid/hid-logitech-dj.c
>> @@ -155,6 +155,14 @@ static const char media_descriptor[] = {
>>  /* Maximum size of all defined hid reports in bytes (including report id)
>> */
>>  #define MAX_REPORT_SIZE 8
>>
>> +/* Make sure all descriptors are present here */
>> +#define MAX_RDESC_SIZE                         \
>> +       (sizeof(kbd_descriptor) +               \
>> +        sizeof(mse_descriptor) +               \
>> +        sizeof(consumer_descriptor) +          \
>> +        sizeof(syscontrol_descriptor) +        \
>> +        sizeof(media_descriptor))
>> +
>>  /* Number of possible hid report types that can be created by this
>> driver.
>>  *
>>  * Right now, RF report types have the same report types (or report id's)
>> @@ -473,9 +481,17 @@ static int logi_dj_output_hidraw_report(struct
>> hid_device *hid, u8 * buf,
>>        return 0;
>>  }
>>
>> +static void rdcat(char **rdesc, unsigned int *rsize, const char *data,
>> unsigned int size)
>> +{
>> +       memcpy(*rdesc + *rsize, data, size);
>> +       *rsize += size;
>> +}
>> +
>>  static int logi_dj_ll_parse(struct hid_device *hid)
>>  {
>>        struct dj_device *djdev = hid->driver_data;
>> +       unsigned int rsize = 0;
>> +       char *rdesc;
>>        int retval;
>>
>>        dbg_hid("%s\n", __func__);
>> @@ -483,70 +499,38 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>>        djdev->hdev->version = 0x0111;
>>        djdev->hdev->country = 0x00;
>>
>> +       rdesc = kmalloc(MAX_RDESC_SIZE, GFP_KERNEL);
>> +       if (!rdesc)
>> +               return -ENOMEM;
>> +
>>        if (djdev->reports_supported & STD_KEYBOARD) {
>>                dbg_hid("%s: sending a kbd descriptor, reports_supported:
>> %x\n",
>>                        __func__, djdev->reports_supported);
>> -               retval = hid_parse_report(hid,
>> -                                         (u8 *) kbd_descriptor,
>> -                                         sizeof(kbd_descriptor));
>> -               if (retval) {
>> -                       dbg_hid("%s: sending a kbd descriptor, hid_parse
>> failed"
>> -                               " error: %d\n", __func__, retval);
>> -                       return retval;
>> -               }
>> +               rdcat(&rdesc, &rsize, kbd_descriptor,
>> sizeof(kbd_descriptor));
>>        }
>>
>>        if (djdev->reports_supported & STD_MOUSE) {
>>                dbg_hid("%s: sending a mouse descriptor, reports_supported:
>> "
>>                        "%x\n", __func__, djdev->reports_supported);
>> -               retval = hid_parse_report(hid,
>> -                                         (u8 *) mse_descriptor,
>> -                                         sizeof(mse_descriptor));
>> -               if (retval) {
>> -                       dbg_hid("%s: sending a mouse descriptor, hid_parse
>> "
>> -                               "failed error: %d\n", __func__, retval);
>> -                       return retval;
>> -               }
>> +               rdcat(&rdesc, &rsize, mse_descriptor,
>> sizeof(mse_descriptor));
>>        }
>>
>>        if (djdev->reports_supported & MULTIMEDIA) {
>>                dbg_hid("%s: sending a multimedia report descriptor: %x\n",
>>                        __func__, djdev->reports_supported);
>> -               retval = hid_parse_report(hid,
>> -                                         (u8 *) consumer_descriptor,
>> -                                         sizeof(consumer_descriptor));
>> -               if (retval) {
>> -                       dbg_hid("%s: sending a consumer_descriptor,
>> hid_parse "
>> -                               "failed error: %d\n", __func__, retval);
>> -                       return retval;
>> -               }
>> +               rdcat(&rdesc, &rsize, consumer_descriptor,
>> sizeof(consumer_descriptor));
>>        }
>>
>>        if (djdev->reports_supported & POWER_KEYS) {
>>                dbg_hid("%s: sending a power keys report descriptor: %x\n",
>>                        __func__, djdev->reports_supported);
>> -               retval = hid_parse_report(hid,
>> -                                         (u8 *) syscontrol_descriptor,
>> -                                         sizeof(syscontrol_descriptor));
>> -               if (retval) {
>> -                       dbg_hid("%s: sending a syscontrol_descriptor, "
>> -                               "hid_parse failed error: %d\n",
>> -                               __func__, retval);
>> -                       return retval;
>> -               }
>> +               rdcat(&rdesc, &rsize, syscontrol_descriptor,
>> sizeof(syscontrol_descriptor));
>>        }
>>
>>        if (djdev->reports_supported & MEDIA_CENTER) {
>>                dbg_hid("%s: sending a media center report descriptor:
>> %x\n",
>>                        __func__, djdev->reports_supported);
>> -               retval = hid_parse_report(hid,
>> -                                         (u8 *) media_descriptor,
>> -                                         sizeof(media_descriptor));
>> -               if (retval) {
>> -                       dbg_hid("%s: sending a media_descriptor, hid_parse
>> "
>> -                               "failed error: %d\n", __func__, retval);
>> -                       return retval;
>> -               }
>> +               rdcat(&rdesc, &rsize, media_descriptor,
>> sizeof(media_descriptor));
>>        }
>>
>>        if (djdev->reports_supported & KBD_LEDS) {
>> @@ -554,7 +538,10 @@ static int logi_dj_ll_parse(struct hid_device *hid)
>>                        __func__, djdev->reports_supported);
>>        }
>>
>> -       return 0;
>> +       retval = hid_parse_report(hid, rdesc, rsize);
>> +       kfree(rdesc);
>> +
>> +       return retval;
>>  }
>>
>>  static int logi_dj_ll_input_event(struct input_dev *dev, unsigned int
>> type,
>> --
>> 1.7.10
>>
>

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] hid-logitech: Collect report descriptors before sending
  2012-04-30 14:44     ` Benjamin Tissoires
@ 2012-04-30 18:14       ` Henrik Rydberg
  0 siblings, 0 replies; 16+ messages in thread
From: Henrik Rydberg @ 2012-04-30 18:14 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: Nestor Lopez Casado, Jiri Kosina, Dmitry Torokhov, linux-input,
	linux-kernel

> I've been able to test the changes with different Logitech mice and keyboards.
> 
> So Jiri,
> Tested-By: Benjamin Tissoires <benjamin.tissoires@gmail.com
> for this change and this patch and the 3rd one (Hid: Handle
> driver-specific device descriptor in core).

Thanks, Benjamin.

Henrik

^ permalink raw reply	[flat|nested] 16+ messages in thread
* RE: [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly
  2012-04-27  8:53   ` Jiri Kosina
@ 2012-04-30 20:15     ` KY Srinivasan
  0 siblings, 0 replies; 16+ messages in thread
From: KY Srinivasan @ 2012-04-30 20:15 UTC (permalink / raw)
  To: Jiri Kosina, Henrik Rydberg; +Cc: Dmitry Torokhov, linux-input, linux-kernel



> -----Original Message-----
> From: Jiri Kosina [mailto:jkosina@suse.cz]
> Sent: Friday, April 27, 2012 4:53 AM
> To: Henrik Rydberg
> Cc: Dmitry Torokhov; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org;
> KY Srinivasan
> Subject: Re: [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly
> 
> On Sun, 22 Apr 2012, Henrik Rydberg wrote:
> 
> > Upcoming changes will split the semantics of hid_parse_report()
> > and hid_parse(), so make sure drivers use hid_parse() in probe().
> >
> > Compiled, not tested.
> >
> > Cc: "K. Y. Srinivasan" <kys@microsoft.com>
> > Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> 
> KY, could I please get your Ack/Tested-by for this patch?

Done.

Tested-and-acked-by:  K. Y. Srinivasan <kys@microsoft.com>

K. Y
 




^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-04-22 21:27 ` [PATCH 0/3] hid: Enable report fixup on rebind Jiri Kosina
  2012-04-23  7:52   ` Nikolai Kondrashov
  2012-04-29 15:33   ` Nikolai Kondrashov
@ 2012-05-06 16:52   ` Nikolai Kondrashov
  2012-05-06 16:54     ` Nikolai Kondrashov
  2 siblings, 1 reply; 16+ messages in thread
From: Nikolai Kondrashov @ 2012-05-06 16:52 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: Henrik Rydberg, Dmitry Torokhov, linux-input, linux-kernel

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

Hi Jiri, Henrik,

On 04/23/2012 12:27 AM, Jiri Kosina wrote:
> On Sun, 22 Apr 2012, Henrik Rydberg wrote:
>
>> This patchset contains a resolution to the problem with
>> driver-specific modification of the report descriptor. The core of the
>> problem lies with the dual semantics of hid_parse_report(), which is
>> therefore split into two functions. As a consequence, the hid core can
>> handle the rebind case internally, with no changes to the
>> drivers. Out-of-tree drivers will work fine as long as they operate in
>> the same way as the in-kernel drivers.
>>
>> The first two patches are bug fixes found in the process. I am hoping
>> for some test feedback on those. The third patch is the main one.
>
> Hi Henrik,
>
> thanks a lot for looking into this. I will look into the patches in more
> detail tomorrow. I am now just adding Nikolai to CC, as he was the first
> one to bring this up and actually has a driver that exposed the fact that
> just freeing/reinstatiating rdesc is not enough.

I've managed to throw together a script which rebinds a device to a specific
out-of-tree HID driver automatically. Please find it attached. I'll be
bundling it with the modules, probably.

I'm planning to use it with udev rules looking like this one:

SUBSYSTEM=="hid", ACTION=="add", ENV{HID_ID}=="0003:0000172F:*", \
         RUN+="/usr/local/bin/digimend-rebind"

So, the out-of-tree HID module problem is finally solved, at least for my
current uses. Thanks a lot!

Sincerely,
Nick

[-- Attachment #2: digimend-rebind --]
[-- Type: text/plain, Size: 2255 bytes --]

#!/bin/sh
#
# DIGImend specific driver rebinding script. To be invoked by udev.
# Author: Nikolai Kondrashov <spbnick@gmail.com>
#
# 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.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.
#

# Write a string to a file, ignoring ENODEV.
write_ignore_enodev() {
    local str="$1"
    local path="$2"
    local output=""

    # Write the string with tee, capturing error output
    if ! output="`(echo \"$str\" | tee \"$path\" >/dev/null) 2>&1`"; then
        # Raise anything except ENODEV
        if [ "${output##*: }" != "No such device" ]; then
            echo "$output" >&2
            return 1
        fi
    fi
}

(
	set -e -u

    progname="`basename \"$0\"`"

	id="`basename $DEVPATH`"

    current_driver_path="`readlink -v -f /sys${DEVPATH}/driver`"
    if [ -e "$current_driver_path" ]; then
        current_driver="`basename \"$current_driver_path\"`"
    else
        current_driver=""
    fi

	specific_module="`modprobe -R $MODALIAS`"

	# Assume the driver would be called the same as module,
	# but without the "hid_" or "hid-" prefix
	specific_driver="${specific_module#hid[_-]}"
    specific_driver_path="/sys/bus/hid/drivers/$specific_driver"

	if [ "$current_driver" != "$specific_driver" ]; then
        logger -p daemon.notice -t "$progname" "rebinding $DEVPATH"

		# Ensure the specific driver module is loaded
		modprobe "$specific_module"

		# Unbind from the current driver, if any
        if [ -n "$current_driver" ]; then
            write_ignore_enodev "$id" "$current_driver_path/unbind"
        fi

		# Bind to the specific driver
        write_ignore_enodev "$id" "$specific_driver_path/bind"
	fi
) 2>&1 | logger -p daemon.warning -t "$progname"

^ permalink raw reply	[flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] hid: Enable report fixup on rebind
  2012-05-06 16:52   ` Nikolai Kondrashov
@ 2012-05-06 16:54     ` Nikolai Kondrashov
  0 siblings, 0 replies; 16+ messages in thread
From: Nikolai Kondrashov @ 2012-05-06 16:54 UTC (permalink / raw)
  To: Jiri Kosina; +Cc: Henrik Rydberg, Dmitry Torokhov, linux-input, linux-kernel

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

On 05/06/2012 07:52 PM, Nikolai Kondrashov wrote:
> I've managed to throw together a script which rebinds a device to a specific
> out-of-tree HID driver automatically. Please find it attached. I'll be
> bundling it with the modules, probably.

Now with proper indentation, sorry.

Sincerely,
Nick

[-- Attachment #2: digimend-rebind --]
[-- Type: text/plain, Size: 2303 bytes --]

#!/bin/sh
#
# DIGImend specific driver rebinding script. To be invoked by udev.
# Author: Nikolai Kondrashov <spbnick@gmail.com>
#
# 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.
#
# This program is distributed in the hope that it will be useful,
# but WITHOUT ANY WARRANTY; without even the implied warranty of
# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
# GNU General Public License for more details.
#
# You should have received a copy of the GNU General Public License
# along with this program.  If not, see <http://www.gnu.org/licenses/>.
#

# Write a string to a file, ignoring ENODEV.
write_ignore_enodev() {
    local str="$1"
    local path="$2"
    local output=""

    # Write the string with tee, capturing error output
    if ! output="`(echo \"$str\" | tee \"$path\" >/dev/null) 2>&1`"; then
        # Raise anything except ENODEV
        if [ "${output##*: }" != "No such device" ]; then
            echo "$output" >&2
            return 1
        fi
    fi
}

(
    set -e -u

    progname="`basename \"$0\"`"

    id="`basename $DEVPATH`"

    current_driver_path="`readlink -v -f /sys${DEVPATH}/driver`"
    if [ -e "$current_driver_path" ]; then
        current_driver="`basename \"$current_driver_path\"`"
    else
        current_driver=""
    fi

    specific_module="`modprobe -R $MODALIAS`"

    # Assume the driver would be called the same as module,
    # but without the "hid_" or "hid-" prefix
    specific_driver="${specific_module#hid[_-]}"
    specific_driver_path="/sys/bus/hid/drivers/$specific_driver"

    if [ "$current_driver" != "$specific_driver" ]; then
        logger -p daemon.notice -t "$progname" "rebinding $DEVPATH"

        # Ensure the specific driver module is loaded
        modprobe "$specific_module"

        # Unbind from the current driver, if any
        if [ -n "$current_driver" ]; then
            write_ignore_enodev "$id" "$current_driver_path/unbind"
        fi

        # Bind to the specific driver
        write_ignore_enodev "$id" "$specific_driver_path/bind"
    fi
) 2>&1 | logger -p daemon.warning -t "$progname"

^ permalink raw reply	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2012-05-06 16:54 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-22 12:21 [PATCH 0/3] hid: Enable report fixup on rebind Henrik Rydberg
2012-04-22 12:21 ` [PATCH 1/3] hid-hyperv: Do not use hid_parse_report() directly Henrik Rydberg
2012-04-27  8:53   ` Jiri Kosina
2012-04-30 20:15     ` KY Srinivasan
2012-04-22 12:21 ` [PATCH 2/3] hid-logitech: Collect report descriptors before sending Henrik Rydberg
     [not found]   ` <CAE7qMrqEJmBWo7JDHuB2DnWJ4vkvv9jya7RDJguiXsrs2xgadQ@mail.gmail.com>
2012-04-30 14:44     ` Benjamin Tissoires
2012-04-30 18:14       ` Henrik Rydberg
2012-04-22 12:21 ` [PATCH 3/3] hid: Handle driver-specific device descriptor in core Henrik Rydberg
2012-04-22 21:27 ` [PATCH 0/3] hid: Enable report fixup on rebind Jiri Kosina
2012-04-23  7:52   ` Nikolai Kondrashov
2012-04-29 15:33   ` Nikolai Kondrashov
2012-04-30 12:06     ` Henrik Rydberg
2012-04-30 12:06       ` Jiri Kosina
2012-04-30 12:25       ` Nikolai Kondrashov
2012-05-06 16:52   ` Nikolai Kondrashov
2012-05-06 16:54     ` Nikolai Kondrashov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).