* Re: NULL pointer dereference in i2c-hid
From: Mika Westerberg @ 2014-12-11 8:58 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: linux-input, linux-kernel, benjamin.tissoires, aduggan, jkosina
In-Reply-To: <31518562.V5Oyo0POsI@xps13>
On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> pointer dereference.
I'll look into this.
I can reproduce this easily with i2c-hid + hid-multitouch following your
directions.
^ permalink raw reply
* About "HID: logitech-hidpp: add support of the first Logitech Wireless Touchpad"
From: Peter Wu @ 2014-12-11 10:02 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, linux-kernel, Andrew de los Reyes, Jiri Kosina
Hi Benjamin,
In commit 57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID:
logitech-hidpp: add support of the first Logitech Wireless Touchpad")
which is in jikos/hid, you made this change to wtp_raw_event:
switch (data[0]) {
case 0x02:
- if (size < 21)
- return 1;
- return wtp_mouse_raw_xy_event(hidpp, &data[7]);
+ if (hidpp->quirks & HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS) {
+ input_event(wd->input, EV_KEY, BTN_LEFT,
+ !!(data[1] & 0x01));
+ input_event(wd->input, EV_KEY, BTN_RIGHT,
+ !!(data[1] & 0x02));
+ input_sync(wd->input);
+ } else {
+ if (size < 21)
+ return 1;
+ return wtp_mouse_raw_xy_event(hidpp, &data[7]);
+ }
case REPORT_ID_HIDPP_LONG:
if ((report->fap.feature_index != wd->mt_feature_index) ||
(report->fap.funcindex_clientid != EVENT_TOUCHPAD_RAW_XY))
return 1;
hidpp_touchpad_raw_xy_event(hidpp, data + 4, &raw);
wtp_send_raw_xy_event(hidpp, &raw);
return 0;
}
Report ID 2 is the mouse descriptor, so it seems correct in that it
falls-through to the next case, but I wanted to check with you that this
is indeed your intention. If so, could you explicitly mark it with a
comment, /* fallthrough */ ?
--
Kind regards,
Peter
https://lekensteyn.nl
^ permalink raw reply
* [PATCH 0/4] HID: logitech-hidpp: fixes for error conditions
From: Peter Wu @ 2014-12-11 12:51 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado
Cc: Peter Hutterer, linux-input, linux-kernel
Hi Jiri,
Here are four patches intended for the 3.19 stream and are based on
jikos/hid (for-next, v3.18-rc4-144-gd9372ee).
* The first is actually from Benjamin Tissoires, but modified to remove
a now unneeded goto.
* The second one depends on the first (it could work without, but there
will be a context mismatch).
* The third one can be applied independently of the others and is needed
to avoid a possible buffer overread.
* The fourth and final patch fixes an unbalanced hid_device_io_start().
Tested by booting with three paired USB devices (QEMU + USB
passthrough), two of them are powered off and one M525 is active. evbug
registers mouse events.
Kind regards,
Peter
Peter Wu (4):
HID: logitech-hidpp: do not return the name length
HID: logitech-hidpp: check name retrieval return code
HID: logitech-hidpp: add boundary check for name retrieval
HID: logitech-hidpp: disable io in probe error path
drivers/hid/hid-logitech-hidpp.c | 36 ++++++++++++++++++++----------------
1 file changed, 20 insertions(+), 16 deletions(-)
--
2.1.3
^ permalink raw reply
* [PATCH 1/4] HID: logitech-hidpp: do not return the name length
From: Peter Wu @ 2014-12-11 12:51 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado
Cc: Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <1418302280-14794-1-git-send-email-peter@lekensteyn.nl>
We do not make any use of the actual name length get through
hidpp_get_device_name(). Original patch by Benjamin Tissoires, this
patch also replaces a (now) unnecessary goto by return NULL.
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
drivers/hid/hid-logitech-hidpp.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1a6395d..5066df8 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
return count;
}
-static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
+static char *hidpp_get_device_name(struct hidpp_device *hidpp)
{
u8 feature_type;
u8 feature_index;
@@ -473,28 +473,23 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_GET_DEVICE_NAME_TYPE,
&feature_index, &feature_type);
if (ret)
- goto out_err;
+ return NULL;
ret = hidpp_devicenametype_get_count(hidpp, feature_index,
&__name_length);
if (ret)
- goto out_err;
+ return NULL;
name = kzalloc(__name_length + 1, GFP_KERNEL);
if (!name)
- goto out_err;
+ return NULL;
- *name_length = __name_length + 1;
while (index < __name_length)
index += hidpp_devicenametype_get_device_name(hidpp,
feature_index, index, name + index,
__name_length - index);
return name;
-
-out_err:
- *name_length = 0;
- return NULL;
}
/* -------------------------------------------------------------------------- */
@@ -989,7 +984,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
char *name;
- u8 name_length;
if (use_unifying)
/*
@@ -999,7 +993,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
*/
name = hidpp_get_unifying_name(hidpp);
else
- name = hidpp_get_device_name(hidpp, &name_length);
+ name = hidpp_get_device_name(hidpp);
if (!name)
hid_err(hdev, "unable to retrieve the name of the device");
@@ -1053,7 +1047,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
bool connected = atomic_read(&hidpp->connected);
struct input_dev *input;
char *name, *devm_name;
- u8 name_length;
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
wtp_connect(hdev, connected);
@@ -1080,7 +1073,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
return;
}
- name = hidpp_get_device_name(hidpp, &name_length);
+ name = hidpp_get_device_name(hidpp);
if (!name) {
hid_err(hdev, "unable to retrieve the name of the device");
} else {
--
2.1.3
^ permalink raw reply related
* [PATCH 2/4] HID: logitech-hidpp: check name retrieval return code
From: Peter Wu @ 2014-12-11 12:51 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado
Cc: Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <1418302280-14794-1-git-send-email-peter@lekensteyn.nl>
hidpp_devicenametype_get_device_name() may return a negative value on
protocol errors (for example, when the device is powered off).
Explicitly check this condition to avoid a long-running loop.
(0 cannot be returned as __name_length - index > 0, but check for it
anyway as it would otherwise result in an infinite loop.)
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
drivers/hid/hid-logitech-hidpp.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5066df8..4d72c20 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -484,10 +484,16 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
if (!name)
return NULL;
- while (index < __name_length)
- index += hidpp_devicenametype_get_device_name(hidpp,
+ while (index < __name_length) {
+ ret = hidpp_devicenametype_get_device_name(hidpp,
feature_index, index, name + index,
__name_length - index);
+ if (ret <= 0) {
+ kfree(name);
+ return NULL;
+ }
+ index += ret;
+ }
return name;
}
--
2.1.3
^ permalink raw reply related
* [PATCH 3/4] HID: logitech-hidpp: add boundary check for name retrieval
From: Peter Wu @ 2014-12-11 12:51 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado
Cc: Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <1418302280-14794-1-git-send-email-peter@lekensteyn.nl>
The HID response has a limited size. Do not trust the value returned by
hardware, check that it really fits in the message.
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
drivers/hid/hid-logitech-hidpp.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4d72c20..4292cc3 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -313,6 +313,9 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
len = response.rap.params[1];
+ if (2 + len > sizeof(response.rap.params))
+ return NULL;
+
name = kzalloc(len + 1, GFP_KERNEL);
if (!name)
return NULL;
--
2.1.3
^ permalink raw reply related
* [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path
From: Peter Wu @ 2014-12-11 12:51 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes
Cc: Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <1418302280-14794-1-git-send-email-peter@lekensteyn.nl>
Balance a hid_device_io_start() call with hid_device_io_stop() in the
error path. This avoids processing of HID reports when the probe fails
which possibly leads to invalid memory access in hid_device_probe() as
report_enum->report_id_hash might already be freed via
hid_close_report().
hid_set_drvdata() is called before wtp_allocate, be consistent and clear
drvdata too on the error path of wtp_allocate.
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
Hi Andrew,
There are few users of hid_device_io_start/stop, this is the first one
to get start/stop out of sync. Should the comment of
hid_device_io_start() be updated to ensure that hid_device_io_stop()
gets called before probe() returns? Or should the hid-core be changed to
handle this out-of-sync issue:
if (ret) {
if (hdev->io_started))
down(&hdev->driver_input_lock);
hid_close_report(hdev);
hdev->driver = NULL;
}
Is my observation correct or not that HID reports can arrive during
hid_close_report() when io_started == true?
Kind regards,
Peter
---
drivers/hid/hid-logitech-hidpp.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 4292cc3..2f420c0 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
ret = wtp_allocate(hdev, id);
if (ret)
- return ret;
+ goto wtp_allocate_fail;
}
INIT_WORK(&hidpp->work, delayed_work_cb);
@@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
if (!connected) {
hid_err(hdev, "Device not connected");
+ hid_device_io_stop(hdev);
goto hid_parse_fail;
}
@@ -1186,6 +1187,7 @@ hid_hw_start_fail:
hid_parse_fail:
cancel_work_sync(&hidpp->work);
mutex_destroy(&hidpp->send_mutex);
+wtp_allocate_fail:
hid_set_drvdata(hdev, NULL);
return ret;
}
--
2.1.3
^ permalink raw reply related
* Re: [PATCH 2/2] HID: logitech-hidpp: prefix the name with Logitech
From: Peter Wu @ 2014-12-11 13:24 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <20141210231739.GD4861@mail.corp.redhat.com>
On Wednesday 10 December 2014 18:17:40 Benjamin Tissoires wrote:
> On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > On Wednesday 10 December 2014 17:21:10 Benjamin Tissoires wrote:
> > > Current names are reported as "K750", "M705", and it can be misleading
> > > for the users when they look at their input device list.
> > >
> > > Prefixing the names with "Logitech " makes things better.
> >
> > Doesn't this apply to all input devices? Like, every USB device can be
> > queried for the manufacturer which could then by included by the input
> > subsystem.
>
> Yes and no. Yes, it's good to have the manufacturer name in the input
> subsystem, and no, because usbhid already prepend the name with the
> manufacturer.
I see, previously (and now, if the device name cannot be retrieved
because one unplugs the receiver during probe which I have actually
tested) a string such as "Logitech Unifying Device. Wireless PID:XXX"
shows up. Now the only string visible is "M512".
Speaking of errors retrieving the name, if a device is turned off then
the name cannot be queried using HID++ 2.0 (think of "T650" instead of
"Wireless Rechargeable Touchpad T650"). Wouldn't this confuse userspace
applications which rely on a constant name?
> >
> > What about duplicate names? Can this patch cause a conflict with devices
> > having the same name?
>
> I am not sure what you mean here. I am adding a prefix to the name, so I
> do not see how a conflict can be possible. If there were a "M705" and a
> "Logitech M705", with different wireless PID, that would be worrisome to
> some extend.
> But I am pretty sure this will not happen.
I thought that this name would be used for /dev/bus/hid/devices/XXX and
therefore had to be unique, but this is not the case (the name is
available under the input device). No worries then!
> >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > ---
> > >
> > > Hi Jiri,
> > >
> > > I'd love to see this one in 3.19 (after a strong review, of course).
> > > The idea came with the mouse DPI database that Peter is currently working on
> > > (see http://who-t.blogspot.com.au/2014/12/building-a-dpi-database-for-mice.html).
> > >
> > > I think, if you do not qualify the series for 3.19, we should drop it entirely.
> > > 3.19 introduces the hidpp module and changes the labels. The DPI hwdb will check
> > > on the label to match the actual mouse, so if this one does not get into 3.19,
> > > we will end up in 3 entries for each Logitech device.
> > >
> > > Cheers,
> > > Benjamin
> > >
> > >
> > > drivers/hid/hid-logitech-hidpp.c | 34 ++++++++++++++++++++++++++++++++++
> > > 1 file changed, 34 insertions(+)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 3846305..8cf4352 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -282,6 +282,33 @@ static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
> > > (report->rap.sub_id == 0x41);
> > > }
> > >
> > > +/**
> > > + * hidpp_prefix_name() prefixes the current given name with "Logitech ".
> > > + */
> > > +static void hidpp_prefix_name(char **name, int name_length)
> > > +{
> > > +#define PREFIX_SIZE 9 /* "Logitech " */
> > > +
> > > + int new_length;
> > > + char *new_name;
> > > +
> > > + if (name_length > PREFIX_SIZE &&
> > > + strncmp(*name, "Logitech ", PREFIX_SIZE) == 0)
> >
> > I think you meant || here, not &&.
>
> No, I meant &&. The idea is to not add a prefix if the provided name
> already contains the prefix. Read that as "if the size of the name may
> contain the prefix and that the prefix is here, then we bail out".
Ah, I somehow read it as "if the name is longer than what can be
stored".
> >
> > > + /* The prefix has is already in the name */
> > > + return;
> > > +
> > > + new_length = name_length + PREFIX_SIZE;
> >
> > Stylistic note, but 'PREFIX_SIZE + name_length' would match the order of
> > the string that gets prepended.
>
> Works for me.
>
> I will send a v2 with your comments addressed.
Thanks!
Kind regards,
Peter
> Cheers,
> Benjamin
>
> >
> > Kind regards,
> > Peter
> >
> > > + new_name = kzalloc(new_length, GFP_KERNEL);
> > > + if (!new_name)
> > > + return;
> > > +
> > > + snprintf(new_name, new_length, "Logitech %s", *name);
> > > +
> > > + kfree(*name);
> > > +
> > > + *name = new_name;
> > > +}
> > > +
> > > /* -------------------------------------------------------------------------- */
> > > /* HIDP++ 1.0 commands */
> > > /* -------------------------------------------------------------------------- */
> > > @@ -318,6 +345,10 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
> > > return NULL;
> > >
> > > memcpy(name, &response.rap.params[2], len);
> > > +
> > > + /* include the terminating '\0' */
> > > + hidpp_prefix_name(&name, len + 1);
> > > +
> > > return name;
> > > }
> > >
> > > @@ -489,6 +520,9 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> > > feature_index, index, name + index,
> > > __name_length - index);
> > >
> > > + /* include the terminating '\0' */
> > > + hidpp_prefix_name(&name, __name_length + 1);
> > > +
> > > return name;
> > >
> > > out_err:
> > >
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Mika Westerberg @ 2014-12-11 14:03 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: linux-input, linux-kernel, benjamin.tissoires, aduggan, jkosina
In-Reply-To: <20141211085801.GR1300@lahna.fi.intel.com>
On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
> On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> > my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> > i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> > other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> > if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> > pointer dereference.
>
> I'll look into this.
>
> I can reproduce this easily with i2c-hid + hid-multitouch following your
> directions.
Can you try the below patch?
I think we shouldn't free buffers yet in ->stop() because we need the
command buffer sending power commands to the device. Also it seems that
->start() re-allocates buffers anyway if maximum size increases.
It shouldn't even leak memory as we release buffers at ->remove()
anyway.
diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 62cec01937ea..68a8c938feea 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
static void i2c_hid_stop(struct hid_device *hid)
{
- struct i2c_client *client = hid->driver_data;
- struct i2c_hid *ihid = i2c_get_clientdata(client);
-
hid->claimed = 0;
-
- i2c_hid_free_buffers(ihid);
}
static int i2c_hid_open(struct hid_device *hid)
^ permalink raw reply related
* Re: About "HID: logitech-hidpp: add support of the first Logitech Wireless Touchpad"
From: Benjamin Tissoires @ 2014-12-11 15:19 UTC (permalink / raw)
To: Peter Wu; +Cc: linux-input, linux-kernel, Andrew de los Reyes, Jiri Kosina
In-Reply-To: <1659058.m7A8Z3l7lH@al>
Hi Peter,
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> Hi Benjamin,
>
> In commit 57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID:
> logitech-hidpp: add support of the first Logitech Wireless Touchpad")
> which is in jikos/hid, you made this change to wtp_raw_event:
>
> switch (data[0]) {
> case 0x02:
> - if (size < 21)
> - return 1;
> - return wtp_mouse_raw_xy_event(hidpp, &data[7]);
> + if (hidpp->quirks & HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS) {
> + input_event(wd->input, EV_KEY, BTN_LEFT,
> + !!(data[1] & 0x01));
> + input_event(wd->input, EV_KEY, BTN_RIGHT,
> + !!(data[1] & 0x02));
> + input_sync(wd->input);
> + } else {
> + if (size < 21)
> + return 1;
> + return wtp_mouse_raw_xy_event(hidpp, &data[7]);
> + }
> case REPORT_ID_HIDPP_LONG:
> if ((report->fap.feature_index != wd->mt_feature_index) ||
> (report->fap.funcindex_clientid != EVENT_TOUCHPAD_RAW_XY))
> return 1;
> hidpp_touchpad_raw_xy_event(hidpp, data + 4, &raw);
>
> wtp_send_raw_xy_event(hidpp, &raw);
> return 0;
> }
>
> Report ID 2 is the mouse descriptor, so it seems correct in that it
> falls-through to the next case, but I wanted to check with you that this
> is indeed your intention. If so, could you explicitly mark it with a
> comment, /* fallthrough */ ?
Yep, it is indeed a bug. I see what happened:
The old code contained a return at the end of the case, and I completely
forget to prevent the fall through with a break. Gosh, I should have
been more careful. Thanks for spotting that!
Cheers,
Benjamin
> --
> Kind regards,
> Peter
> https://lekensteyn.nl
>
^ permalink raw reply
* Re: [PATCH 1/4] HID: logitech-hidpp: do not return the name length
From: Benjamin Tissoires @ 2014-12-11 15:22 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <1418302280-14794-2-git-send-email-peter@lekensteyn.nl>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> We do not make any use of the actual name length get through
> hidpp_get_device_name(). Original patch by Benjamin Tissoires, this
> patch also replaces a (now) unnecessary goto by return NULL.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Thanks for the v2 Peter.
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 19 ++++++-------------
> 1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1a6395d..5066df8 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -461,7 +461,7 @@ static int hidpp_devicenametype_get_device_name(struct hidpp_device *hidpp,
> return count;
> }
>
> -static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> +static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> {
> u8 feature_type;
> u8 feature_index;
> @@ -473,28 +473,23 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp, u8 *name_length)
> ret = hidpp_root_get_feature(hidpp, HIDPP_PAGE_GET_DEVICE_NAME_TYPE,
> &feature_index, &feature_type);
> if (ret)
> - goto out_err;
> + return NULL;
>
> ret = hidpp_devicenametype_get_count(hidpp, feature_index,
> &__name_length);
> if (ret)
> - goto out_err;
> + return NULL;
>
> name = kzalloc(__name_length + 1, GFP_KERNEL);
> if (!name)
> - goto out_err;
> + return NULL;
>
> - *name_length = __name_length + 1;
> while (index < __name_length)
> index += hidpp_devicenametype_get_device_name(hidpp,
> feature_index, index, name + index,
> __name_length - index);
>
> return name;
> -
> -out_err:
> - *name_length = 0;
> - return NULL;
> }
>
> /* -------------------------------------------------------------------------- */
> @@ -989,7 +984,6 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> {
> struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> char *name;
> - u8 name_length;
>
> if (use_unifying)
> /*
> @@ -999,7 +993,7 @@ static void hidpp_overwrite_name(struct hid_device *hdev, bool use_unifying)
> */
> name = hidpp_get_unifying_name(hidpp);
> else
> - name = hidpp_get_device_name(hidpp, &name_length);
> + name = hidpp_get_device_name(hidpp);
>
> if (!name)
> hid_err(hdev, "unable to retrieve the name of the device");
> @@ -1053,7 +1047,6 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> bool connected = atomic_read(&hidpp->connected);
> struct input_dev *input;
> char *name, *devm_name;
> - u8 name_length;
>
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> wtp_connect(hdev, connected);
> @@ -1080,7 +1073,7 @@ static void hidpp_connect_event(struct hidpp_device *hidpp)
> return;
> }
>
> - name = hidpp_get_device_name(hidpp, &name_length);
> + name = hidpp_get_device_name(hidpp);
> if (!name) {
> hid_err(hdev, "unable to retrieve the name of the device");
> } else {
> --
> 2.1.3
>
^ permalink raw reply
* Re: [PATCH 2/4] HID: logitech-hidpp: check name retrieval return code
From: Benjamin Tissoires @ 2014-12-11 15:23 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <1418302280-14794-3-git-send-email-peter@lekensteyn.nl>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> hidpp_devicenametype_get_device_name() may return a negative value on
> protocol errors (for example, when the device is powered off).
> Explicitly check this condition to avoid a long-running loop.
>
> (0 cannot be returned as __name_length - index > 0, but check for it
> anyway as it would otherwise result in an infinite loop.)
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 5066df8..4d72c20 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -484,10 +484,16 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> if (!name)
> return NULL;
>
> - while (index < __name_length)
> - index += hidpp_devicenametype_get_device_name(hidpp,
> + while (index < __name_length) {
> + ret = hidpp_devicenametype_get_device_name(hidpp,
> feature_index, index, name + index,
> __name_length - index);
> + if (ret <= 0) {
> + kfree(name);
> + return NULL;
> + }
> + index += ret;
> + }
>
> return name;
> }
> --
> 2.1.3
>
^ permalink raw reply
* Re: [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path
From: Benjamin Tissoires @ 2014-12-11 15:31 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Andrew de los Reyes,
Peter Hutterer, linux-input, linux-kernel
In-Reply-To: <1418302280-14794-5-git-send-email-peter@lekensteyn.nl>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> Balance a hid_device_io_start() call with hid_device_io_stop() in the
> error path. This avoids processing of HID reports when the probe fails
> which possibly leads to invalid memory access in hid_device_probe() as
> report_enum->report_id_hash might already be freed via
> hid_close_report().
Well spotted too!
>
> hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> drvdata too on the error path of wtp_allocate.
This is not strictly speaking required. We will have a dangling value in
hdev->private_data, but this should be overwritten before the next use.
Anyway, it makes sense to clean up after a failure, so the patch is
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
> Hi Andrew,
>
> There are few users of hid_device_io_start/stop, this is the first one
> to get start/stop out of sync. Should the comment of
> hid_device_io_start() be updated to ensure that hid_device_io_stop()
> gets called before probe() returns? Or should the hid-core be changed to
> handle this out-of-sync issue:
>
> if (ret) {
> if (hdev->io_started))
> down(&hdev->driver_input_lock);
> hid_close_report(hdev);
> hdev->driver = NULL;
> }
>
> Is my observation correct or not that HID reports can arrive during
> hid_close_report() when io_started == true?
>
> Kind regards,
> Peter
> ---
> drivers/hid/hid-logitech-hidpp.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 4292cc3..2f420c0 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> ret = wtp_allocate(hdev, id);
> if (ret)
> - return ret;
> + goto wtp_allocate_fail;
> }
>
> INIT_WORK(&hidpp->work, delayed_work_cb);
> @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> if (!connected) {
> hid_err(hdev, "Device not connected");
> + hid_device_io_stop(hdev);
> goto hid_parse_fail;
> }
>
> @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> hid_parse_fail:
> cancel_work_sync(&hidpp->work);
> mutex_destroy(&hidpp->send_mutex);
> +wtp_allocate_fail:
> hid_set_drvdata(hdev, NULL);
> return ret;
> }
> --
> 2.1.3
>
^ permalink raw reply
* Re: [PATCH 3/4] HID: logitech-hidpp: add boundary check for name retrieval
From: Benjamin Tissoires @ 2014-12-11 15:25 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <1418302280-14794-4-git-send-email-peter@lekensteyn.nl>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> The HID response has a limited size. Do not trust the value returned by
> hardware, check that it really fits in the message.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
Well spotted!
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 4d72c20..4292cc3 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -313,6 +313,9 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
>
> len = response.rap.params[1];
>
> + if (2 + len > sizeof(response.rap.params))
> + return NULL;
> +
> name = kzalloc(len + 1, GFP_KERNEL);
> if (!name)
> return NULL;
> --
> 2.1.3
>
^ permalink raw reply
* Re: [PATCH 2/2] HID: logitech-hidpp: prefix the name with Logitech
From: Benjamin Tissoires @ 2014-12-11 15:50 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Peter Hutterer, linux-input,
linux-kernel
In-Reply-To: <2407359.LsiQoHNGbj@al>
On Dec 11 2014 or thereabouts, Peter Wu wrote:
> On Wednesday 10 December 2014 18:17:40 Benjamin Tissoires wrote:
> > On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > > On Wednesday 10 December 2014 17:21:10 Benjamin Tissoires wrote:
> > > > Current names are reported as "K750", "M705", and it can be misleading
> > > > for the users when they look at their input device list.
> > > >
> > > > Prefixing the names with "Logitech " makes things better.
> > >
> > > Doesn't this apply to all input devices? Like, every USB device can be
> > > queried for the manufacturer which could then by included by the input
> > > subsystem.
> >
> > Yes and no. Yes, it's good to have the manufacturer name in the input
> > subsystem, and no, because usbhid already prepend the name with the
> > manufacturer.
>
> I see, previously (and now, if the device name cannot be retrieved
> because one unplugs the receiver during probe which I have actually
> tested) a string such as "Logitech Unifying Device. Wireless PID:XXX"
> shows up. Now the only string visible is "M512".
Well, if you unplug the receiver during the probe(), the input nodes
should be cleared, so whatever the name you get, it does not really
matter.
>
> Speaking of errors retrieving the name, if a device is turned off then
> the name cannot be queried using HID++ 2.0 (think of "T650" instead of
> "Wireless Rechargeable Touchpad T650"). Wouldn't this confuse userspace
> applications which rely on a constant name?
It will. However, this only matters for the devices which have a
HIDPP_QUIRK_DELAYED_INIT. We do not overwrite the name of the input node
for the others.
Making hidpp_connect_event() and wtp_connect() returning an error code
could help us not to create the input node until we have all the
information (setup parameters, and name).
This will require a little bit of tuning in the code. Again, I am not
sure I will have the time to do this today, so feel free to start a
first version on this if you want.
Many thanks for the deep analysis you are conducting on
hid-logitech-hidpp. This is really appreciated.
Cheers,
Benjamin
>
> > >
> > > What about duplicate names? Can this patch cause a conflict with devices
> > > having the same name?
> >
> > I am not sure what you mean here. I am adding a prefix to the name, so I
> > do not see how a conflict can be possible. If there were a "M705" and a
> > "Logitech M705", with different wireless PID, that would be worrisome to
> > some extend.
> > But I am pretty sure this will not happen.
>
> I thought that this name would be used for /dev/bus/hid/devices/XXX and
> therefore had to be unique, but this is not the case (the name is
> available under the input device). No worries then!
>
> > >
> > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > > ---
> > > >
> > > > Hi Jiri,
> > > >
> > > > I'd love to see this one in 3.19 (after a strong review, of course).
> > > > The idea came with the mouse DPI database that Peter is currently working on
> > > > (see http://who-t.blogspot.com.au/2014/12/building-a-dpi-database-for-mice.html).
> > > >
> > > > I think, if you do not qualify the series for 3.19, we should drop it entirely.
> > > > 3.19 introduces the hidpp module and changes the labels. The DPI hwdb will check
> > > > on the label to match the actual mouse, so if this one does not get into 3.19,
> > > > we will end up in 3 entries for each Logitech device.
> > > >
> > > > Cheers,
> > > > Benjamin
> > > >
> > > >
> > > > drivers/hid/hid-logitech-hidpp.c | 34 ++++++++++++++++++++++++++++++++++
> > > > 1 file changed, 34 insertions(+)
> > > >
> > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > index 3846305..8cf4352 100644
> > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > @@ -282,6 +282,33 @@ static inline bool hidpp_report_is_connect_event(struct hidpp_report *report)
> > > > (report->rap.sub_id == 0x41);
> > > > }
> > > >
> > > > +/**
> > > > + * hidpp_prefix_name() prefixes the current given name with "Logitech ".
> > > > + */
> > > > +static void hidpp_prefix_name(char **name, int name_length)
> > > > +{
> > > > +#define PREFIX_SIZE 9 /* "Logitech " */
> > > > +
> > > > + int new_length;
> > > > + char *new_name;
> > > > +
> > > > + if (name_length > PREFIX_SIZE &&
> > > > + strncmp(*name, "Logitech ", PREFIX_SIZE) == 0)
> > >
> > > I think you meant || here, not &&.
> >
> > No, I meant &&. The idea is to not add a prefix if the provided name
> > already contains the prefix. Read that as "if the size of the name may
> > contain the prefix and that the prefix is here, then we bail out".
>
> Ah, I somehow read it as "if the name is longer than what can be
> stored".
>
> > >
> > > > + /* The prefix has is already in the name */
> > > > + return;
> > > > +
> > > > + new_length = name_length + PREFIX_SIZE;
> > >
> > > Stylistic note, but 'PREFIX_SIZE + name_length' would match the order of
> > > the string that gets prepended.
> >
> > Works for me.
> >
> > I will send a v2 with your comments addressed.
>
> Thanks!
>
> Kind regards,
> Peter
>
> > Cheers,
> > Benjamin
> >
> > >
> > > Kind regards,
> > > Peter
> > >
> > > > + new_name = kzalloc(new_length, GFP_KERNEL);
> > > > + if (!new_name)
> > > > + return;
> > > > +
> > > > + snprintf(new_name, new_length, "Logitech %s", *name);
> > > > +
> > > > + kfree(*name);
> > > > +
> > > > + *name = new_name;
> > > > +}
> > > > +
> > > > /* -------------------------------------------------------------------------- */
> > > > /* HIDP++ 1.0 commands */
> > > > /* -------------------------------------------------------------------------- */
> > > > @@ -318,6 +345,10 @@ static char *hidpp_get_unifying_name(struct hidpp_device *hidpp_dev)
> > > > return NULL;
> > > >
> > > > memcpy(name, &response.rap.params[2], len);
> > > > +
> > > > + /* include the terminating '\0' */
> > > > + hidpp_prefix_name(&name, len + 1);
> > > > +
> > > > return name;
> > > > }
> > > >
> > > > @@ -489,6 +520,9 @@ static char *hidpp_get_device_name(struct hidpp_device *hidpp)
> > > > feature_index, index, name + index,
> > > > __name_length - index);
> > > >
> > > > + /* include the terminating '\0' */
> > > > + hidpp_prefix_name(&name, __name_length + 1);
> > > > +
> > > > return name;
> > > >
> > > > out_err:
> > > >
>
^ permalink raw reply
* Re: [systemd-devel] Supporting U2F over HID on Linux?
From: Andy Lutomirski @ 2014-12-11 17:36 UTC (permalink / raw)
To: David Herrmann; +Cc: Jiri Kosina, open list:HID CORE LAYER, Benjamin Tissoires
In-Reply-To: <CANq1E4Q8gTgMj+twRE3xjs1OfKUmbOmOatk_sDPPAW2BB4jscw@mail.gmail.com>
On Thu, Dec 11, 2014 at 12:49 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi
>
> On Wed, Dec 10, 2014 at 6:07 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> On Tue, Dec 9, 2014 at 12:46 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>> On Mon, Nov 3, 2014 at 12:41 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>>>> On Mon, Nov 3, 2014 at 12:21 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>>>>> On Mon, 3 Nov 2014, David Herrmann wrote:
>>>>>
>>>>>> > Agreed, mostly. My only real concern is that this could be annoying
>>>>>> > for the userspace developers who will need to target Linux and HIDAPI
>>>>>> > separately. Admittedly the Linux support will be trivial.
>>>>>>
>>>>>> I see. I'll not stop you from using hidraw, I'd just not recommend it.
>>>>>> Especially for security stuff I dislike exposing the whole HID device
>>>>>> writable. But yeah, I guess you got my point.
>>>>>
>>>>> Alright, I am basically thinking loudly now, but how about we allow HID
>>>>> drivers that would override default hidraw interface?
>>>>>
>>>>> I am very well aware of the fact that this could be opening a can of
>>>>> worms, so we'll have to make it very restrictive. How about, let's say, we
>>>>> allow HID drivers to provide custom hidraw interface (completely
>>>>> overriding the one that HID core would normally create) only for cases
>>>>> such as:
>>>>>
>>>>> - the intent is to expose only certain parts of a combined device
>>>>> - the intent is to introduce some level of access control
>>>>>
>>>>> I.e. still no interference of kernel with data parsing allowed.
>>>>
>>>> Hmm. Would this be like a filter on hidraw actions?
>>>>
>>>> How would udev distinguish these special hidraw devices from normal
>>>> hidraw devices?
>>>>
>>>> Also, for U2F, this could be a little awkward. There's some crazy
>>>> fragmentation stuff to allow a U2F request to be split across HID
>>>> requests, and I think a kernel driver would much rather get the
>>>> original unfragmented application request.
>>>>
>>>
>>> I started writing a driver for this. I got enumeration working. I
>>> assume I should create a "u2f" device class, and then... ?
>>>
>>> Where am I supposed to get my character device from? Do I register my
>>> own chrdev major? Do I use misc? Is there some input thing I'm
>>> supposed to use?
>>
>> Another question:
>>
>> I'm hitting this in hid_input_report:
>>
>> if (down_trylock(&hid->driver_input_lock)) {
>> pr_err("HID: trylock failed\n"); // I added this
>> return -EBUSY;
>> }
>>
>> This is a problem for u2f: u2f reports are actual protocol messages,
>> and there isn't a retransmit mechanism. Losing messages randomly
>> causes the handshake to fail, and then nothing works.
>>
>> I *think* that the only way I can hit that failure is during probe (or
>> if the USB stack somehow completes two transfers at once). This still
>> makes it quite awkward to start IO from the probe routine.
>>
>> I could start a work item to take driver_input_lock, release it again,
>> and then start the handshake, but that sucks.
>
> HID reports are ordered. It's the responsibility of the
> transport-layer to not provide multiple reports in parallel. The
> reason we have this down_trylock() is to drop packages if no driver is
> loaded, yet. I wonder how you can trigger this? Does this happen
> during runtime or only driver load? Do you send GET_REPORT requests to
> the device?
It happens during driver probe. I don't set GET_REPORT to my device
-- the protocol consists solely of SET_REPORT from the driver followed
by an asynchronous (but generally reasonably quick) report back from
the device.
During probe, I set INIT, and the device responds by saying "I'm a
device. I speak this version of the protocol.
Shouldn't that code be more like:
mutex_lock(whatever);
if (driver loaded)
deliver the report;
mutex_unlock(whatever);
and then the core could hold the lock briefly as well when probing or
in hid_hw_open/hid_hw_close.
(Also, shouldn't hid_hw_open have some kind of reference counting to
avoid interference between hidraw and real drivers? Or does that
already work correctly somehow?)
--Andy
>
> Thanks
> David
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path
From: Andrew de los Reyes @ 2014-12-11 17:37 UTC (permalink / raw)
To: Benjamin Tissoires, Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, Andrew de los Reyes,
Peter Hutterer, Linux Input, linux-kernel@vger.kernel.org
In-Reply-To: <20141211153118.GF29747@mail.corp.redhat.com>
On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > Balance a hid_device_io_start() call with hid_device_io_stop() in the
> > error path. This avoids processing of HID reports when the probe fails
> > which possibly leads to invalid memory access in hid_device_probe() as
> > report_enum->report_id_hash might already be freed via
> > hid_close_report().
>
> Well spotted too!
>
> >
> > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> > drvdata too on the error path of wtp_allocate.
>
> This is not strictly speaking required. We will have a dangling value in
> hdev->private_data, but this should be overwritten before the next use.
>
> Anyway, it makes sense to clean up after a failure, so the patch is
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> Cheers,
> Benjamin
>
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> > Hi Andrew,
> >
> > There are few users of hid_device_io_start/stop, this is the first one
> > to get start/stop out of sync. Should the comment of
> > hid_device_io_start() be updated to ensure that hid_device_io_stop()
> > gets called before probe() returns? Or should the hid-core be changed to
> > handle this out-of-sync issue:
I do not have a strong opinion on this, and will defer to others. The
reason I needed to communicate during probe() was to have the driver
probe the actual device for details. In this use case, I would be okay
to disable IO at the end of probe() and have it become reenabled via
the normal (default) methods.
-andrew
> >
> > if (ret) {
> > if (hdev->io_started))
> > down(&hdev->driver_input_lock);
> > hid_close_report(hdev);
> > hdev->driver = NULL;
> > }
> >
> > Is my observation correct or not that HID reports can arrive during
> > hid_close_report() when io_started == true?
> >
> > Kind regards,
> > Peter
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 4292cc3..2f420c0 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> > ret = wtp_allocate(hdev, id);
> > if (ret)
> > - return ret;
> > + goto wtp_allocate_fail;
> > }
> >
> > INIT_WORK(&hidpp->work, delayed_work_cb);
> > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> > if (!connected) {
> > hid_err(hdev, "Device not connected");
> > + hid_device_io_stop(hdev);
> > goto hid_parse_fail;
> > }
> >
> > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> > hid_parse_fail:
> > cancel_work_sync(&hidpp->work);
> > mutex_destroy(&hidpp->send_mutex);
> > +wtp_allocate_fail:
> > hid_set_drvdata(hdev, NULL);
> > return ret;
> > }
> > --
> > 2.1.3
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/4] HID: logitech-hidpp: disable io in probe error path
From: Peter Wu @ 2014-12-11 17:53 UTC (permalink / raw)
To: Andrew de los Reyes
Cc: Benjamin Tissoires, Jiri Kosina, Nestor Lopez Casado,
Andrew de los Reyes, Peter Hutterer, Linux Input,
linux-kernel@vger.kernel.org
In-Reply-To: <CAG_cf+fx8Xk4t7fsGkb6bkFEPCXQg6rcAOk7arUD34B8QRShgQ@mail.gmail.com>
On Thursday 11 December 2014 09:37:06 Andrew de los Reyes wrote:
> On Thu Dec 11 2014 at 7:31:43 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Dec 11 2014 or thereabouts, Peter Wu wrote:
> > > Balance a hid_device_io_start() call with hid_device_io_stop() in the
> > > error path. This avoids processing of HID reports when the probe fails
> > > which possibly leads to invalid memory access in hid_device_probe() as
> > > report_enum->report_id_hash might already be freed via
> > > hid_close_report().
> >
> > Well spotted too!
> >
> > >
> > > hid_set_drvdata() is called before wtp_allocate, be consistent and clear
> > > drvdata too on the error path of wtp_allocate.
> >
> > This is not strictly speaking required. We will have a dangling value in
> > hdev->private_data, but this should be overwritten before the next use.
> >
> > Anyway, it makes sense to clean up after a failure, so the patch is
> > Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > Cheers,
> > Benjamin
> >
> > >
> > > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > > ---
> > > Hi Andrew,
> > >
> > > There are few users of hid_device_io_start/stop, this is the first one
> > > to get start/stop out of sync. Should the comment of
> > > hid_device_io_start() be updated to ensure that hid_device_io_stop()
> > > gets called before probe() returns? Or should the hid-core be changed to
> > > handle this out-of-sync issue:
>
> I do not have a strong opinion on this, and will defer to others. The
> reason I needed to communicate during probe() was to have the driver
> probe the actual device for details. In this use case, I would be okay
> to disable IO at the end of probe() and have it become reenabled via
> the normal (default) methods.
>
> -andrew
Keeping the reports enabled when the probe succeeds is fine, I am
referring to the error path. If the probe fails, then reports should
never be accepted or a corruption may occur (if I see it correctly).
Is this analysis correct?
hid_device_probe()
hid_device_io_start()
return FAILURE
hid_close_report(device)
report_enum = device
->report_enum[i]
hid_free_report(report_enum
->report_id_hash[j]) <-- NOTE: freed
... interrupt occurs ...
hid_input_report()
hid_get_report()
report = report_enum->report_id_hash[n];
^ NOTE: use-after-free
return report if not NULL
hdrv->raw_event(report) <--- BOOM?
kfree(device->rdesc)
device->driver = NULL
Kind regards,
Peter
> > >
> > > if (ret) {
> > > if (hdev->io_started))
> > > down(&hdev->driver_input_lock);
> > > hid_close_report(hdev);
> > > hdev->driver = NULL;
> > > }
> > >
> > > Is my observation correct or not that HID reports can arrive during
> > > hid_close_report() when io_started == true?
> > >
> > > Kind regards,
> > > Peter
> > > ---
> > > drivers/hid/hid-logitech-hidpp.c | 4 +++-
> > > 1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > index 4292cc3..2f420c0 100644
> > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > @@ -1121,7 +1121,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> > > ret = wtp_allocate(hdev, id);
> > > if (ret)
> > > - return ret;
> > > + goto wtp_allocate_fail;
> > > }
> > >
> > > INIT_WORK(&hidpp->work, delayed_work_cb);
> > > @@ -1141,6 +1141,7 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > > if (id->group != HID_GROUP_LOGITECH_DJ_DEVICE) {
> > > if (!connected) {
> > > hid_err(hdev, "Device not connected");
> > > + hid_device_io_stop(hdev);
> > > goto hid_parse_fail;
> > > }
> > >
> > > @@ -1186,6 +1187,7 @@ hid_hw_start_fail:
> > > hid_parse_fail:
> > > cancel_work_sync(&hidpp->work);
> > > mutex_destroy(&hidpp->send_mutex);
> > > +wtp_allocate_fail:
> > > hid_set_drvdata(hdev, NULL);
> > > return ret;
> > > }
> > > --
> > > 2.1.3
> > >
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Gabriele Mazzotta @ 2014-12-11 18:16 UTC (permalink / raw)
To: Mika Westerberg
Cc: linux-input, linux-kernel, benjamin.tissoires, aduggan, jkosina
In-Reply-To: <20141211140307.GE1300@lahna.fi.intel.com>
On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
> > On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> > > my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> > > i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> > > other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> > > if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> > > pointer dereference.
> >
> > I'll look into this.
> >
> > I can reproduce this easily with i2c-hid + hid-multitouch following your
> > directions.
>
> Can you try the below patch?
>
> I think we shouldn't free buffers yet in ->stop() because we need the
> command buffer sending power commands to the device. Also it seems that
> ->start() re-allocates buffers anyway if maximum size increases.
>
> It shouldn't even leak memory as we release buffers at ->remove()
> anyway.
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 62cec01937ea..68a8c938feea 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
>
> static void i2c_hid_stop(struct hid_device *hid)
> {
> - struct i2c_client *client = hid->driver_data;
> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> -
> hid->claimed = 0;
> -
> - i2c_hid_free_buffers(ihid);
> }
>
> static int i2c_hid_open(struct hid_device *hid)
Yes, it works, thanks.
This change seems to also prevent kernel ooops when I unload either
i2c-hid or i2c-designware-platform while the touchpad is in use,
thing that is likely to happen because of the other bug I reported.
Speaking of it, does any of you have any suggestion on how to debug it?
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Andrew Duggan @ 2014-12-11 18:40 UTC (permalink / raw)
To: Gabriele Mazzotta, Mika Westerberg
Cc: linux-input, linux-kernel, benjamin.tissoires, jkosina
In-Reply-To: <2128690.c90ERDd9ZP@xps13>
On 12/11/2014 10:16 AM, Gabriele Mazzotta wrote:
> On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
>> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
>>> On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
>>>> my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
>>>> i2c-hid and hid-rmi can be loaded and unloaded independelty from each
>>>> other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
>>>> if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
>>>> pointer dereference.
>>> I'll look into this.
>>>
>>> I can reproduce this easily with i2c-hid + hid-multitouch following your
>>> directions.
>> Can you try the below patch?
>>
>> I think we shouldn't free buffers yet in ->stop() because we need the
>> command buffer sending power commands to the device. Also it seems that
>> ->start() re-allocates buffers anyway if maximum size increases.
>>
>> It shouldn't even leak memory as we release buffers at ->remove()
>> anyway.
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 62cec01937ea..68a8c938feea 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
>>
>> static void i2c_hid_stop(struct hid_device *hid)
>> {
>> - struct i2c_client *client = hid->driver_data;
>> - struct i2c_hid *ihid = i2c_get_clientdata(client);
>> -
>> hid->claimed = 0;
>> -
>> - i2c_hid_free_buffers(ihid);
>> }
>>
>> static int i2c_hid_open(struct hid_device *hid)
> Yes, it works, thanks.
>
> This change seems to also prevent kernel ooops when I unload either
> i2c-hid or i2c-designware-platform while the touchpad is in use,
> thing that is likely to happen because of the other bug I reported.
>
> Speaking of it, does any of you have any suggestion on how to debug it?
I was able to reproduce the initial issue by unloading hid-rmi and
i2c-hid while holding my fingers on the touchpad. Mika's patch fixes it
for me.
For the original bug, you can modprobe i2c-hid debug=1 and we can see
what data the touchpad is reporting. That might help narrowing down if
it's noise which the touchpad thinks are fingers or if there is a
problem with the I2C lines causing spurious interrupts.
Andrew
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Benjamin Tissoires @ 2014-12-11 18:41 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: Mika Westerberg, linux-input, linux-kernel@vger.kernel.org,
Benjamin Tissoires, Andrew Duggan, Jiri Kosina
In-Reply-To: <2128690.c90ERDd9ZP@xps13>
On Thu, Dec 11, 2014 at 1:16 PM, Gabriele Mazzotta
<gabriele.mzt@gmail.com> wrote:
> On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
>> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
>> > On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
>> > > my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
>> > > i2c-hid and hid-rmi can be loaded and unloaded independelty from each
>> > > other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
>> > > if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
>> > > pointer dereference.
>> >
>> > I'll look into this.
>> >
>> > I can reproduce this easily with i2c-hid + hid-multitouch following your
>> > directions.
>>
>> Can you try the below patch?
>>
>> I think we shouldn't free buffers yet in ->stop() because we need the
>> command buffer sending power commands to the device. Also it seems that
>> ->start() re-allocates buffers anyway if maximum size increases.
>>
>> It shouldn't even leak memory as we release buffers at ->remove()
>> anyway.
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 62cec01937ea..68a8c938feea 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
>>
>> static void i2c_hid_stop(struct hid_device *hid)
>> {
>> - struct i2c_client *client = hid->driver_data;
>> - struct i2c_hid *ihid = i2c_get_clientdata(client);
>> -
>> hid->claimed = 0;
>> -
>> - i2c_hid_free_buffers(ihid);
>> }
>>
>> static int i2c_hid_open(struct hid_device *hid)
Mika,
you can add my Rev-by when submitting this patch to the mailing list.
>
> Yes, it works, thanks.
>
> This change seems to also prevent kernel ooops when I unload either
> i2c-hid or i2c-designware-platform while the touchpad is in use,
> thing that is likely to happen because of the other bug I reported.
>
> Speaking of it, does any of you have any suggestion on how to debug it?
Hehe, I coincidentally just replied to your bug with one patch to try
(that was proposed by the intel folks back in May[1]).
The powertop problem is IMO really worrying because the purpose of
i2c_hid was to reduce power consumption :)
Anyway, thanks for the quick test.
Cheers,
Benjamin
[1] https://patchwork.kernel.org/patch/4133771/
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Gabriele Mazzotta @ 2014-12-11 19:11 UTC (permalink / raw)
To: Andrew Duggan
Cc: Mika Westerberg, linux-input, linux-kernel, benjamin.tissoires,
jkosina
In-Reply-To: <5489E505.3050903@synaptics.com>
On Thursday 11 December 2014 10:40:05 Andrew Duggan wrote:
> On 12/11/2014 10:16 AM, Gabriele Mazzotta wrote:
> > On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
> >> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
> >>> On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> >>>> my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> >>>> i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> >>>> other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> >>>> if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> >>>> pointer dereference.
> >>> I'll look into this.
> >>>
> >>> I can reproduce this easily with i2c-hid + hid-multitouch following your
> >>> directions.
> >> Can you try the below patch?
> >>
> >> I think we shouldn't free buffers yet in ->stop() because we need the
> >> command buffer sending power commands to the device. Also it seems that
> >> ->start() re-allocates buffers anyway if maximum size increases.
> >>
> >> It shouldn't even leak memory as we release buffers at ->remove()
> >> anyway.
> >>
> >> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> >> index 62cec01937ea..68a8c938feea 100644
> >> --- a/drivers/hid/i2c-hid/i2c-hid.c
> >> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> >> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
> >>
> >> static void i2c_hid_stop(struct hid_device *hid)
> >> {
> >> - struct i2c_client *client = hid->driver_data;
> >> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> >> -
> >> hid->claimed = 0;
> >> -
> >> - i2c_hid_free_buffers(ihid);
> >> }
> >>
> >> static int i2c_hid_open(struct hid_device *hid)
> > Yes, it works, thanks.
> >
> > This change seems to also prevent kernel ooops when I unload either
> > i2c-hid or i2c-designware-platform while the touchpad is in use,
> > thing that is likely to happen because of the other bug I reported.
> >
> > Speaking of it, does any of you have any suggestion on how to debug it?
> I was able to reproduce the initial issue by unloading hid-rmi and
> i2c-hid while holding my fingers on the touchpad. Mika's patch fixes it
> for me.
>
> For the original bug, you can modprobe i2c-hid debug=1 and we can see
> what data the touchpad is reporting. That might help narrowing down if
> it's noise which the touchpad thinks are fingers or if there is a
> problem with the I2C lines causing spurious interrupts.
>
> Andrew
I've already tried to do that and here what I got:
When I release the finger, the last message is repeated 81 times.
If the byte containing informations about the width of the finger
becomes equal to either c0 or 0c at least once, the last message is
repeated indefinitely and changes as soon as I start using the touchpad.
The only way to stop it is to unload and reload i2c-hid.
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Andrew Duggan @ 2014-12-11 19:21 UTC (permalink / raw)
To: Gabriele Mazzotta
Cc: Mika Westerberg, linux-input, linux-kernel, benjamin.tissoires,
jkosina
In-Reply-To: <2171175.gv8IA3dxxN@xps13>
On 12/11/2014 11:11 AM, Gabriele Mazzotta wrote:
> On Thursday 11 December 2014 10:40:05 Andrew Duggan wrote:
>> On 12/11/2014 10:16 AM, Gabriele Mazzotta wrote:
>>> On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
>>>> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
>>>>> On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
>>>>>> my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
>>>>>> i2c-hid and hid-rmi can be loaded and unloaded independelty from each
>>>>>> other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
>>>>>> if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
>>>>>> pointer dereference.
>>>>> I'll look into this.
>>>>>
>>>>> I can reproduce this easily with i2c-hid + hid-multitouch following your
>>>>> directions.
>>>> Can you try the below patch?
>>>>
>>>> I think we shouldn't free buffers yet in ->stop() because we need the
>>>> command buffer sending power commands to the device. Also it seems that
>>>> ->start() re-allocates buffers anyway if maximum size increases.
>>>>
>>>> It shouldn't even leak memory as we release buffers at ->remove()
>>>> anyway.
>>>>
>>>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>>>> index 62cec01937ea..68a8c938feea 100644
>>>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>>>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>>>> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
>>>>
>>>> static void i2c_hid_stop(struct hid_device *hid)
>>>> {
>>>> - struct i2c_client *client = hid->driver_data;
>>>> - struct i2c_hid *ihid = i2c_get_clientdata(client);
>>>> -
>>>> hid->claimed = 0;
>>>> -
>>>> - i2c_hid_free_buffers(ihid);
>>>> }
>>>>
>>>> static int i2c_hid_open(struct hid_device *hid)
>>> Yes, it works, thanks.
>>>
>>> This change seems to also prevent kernel ooops when I unload either
>>> i2c-hid or i2c-designware-platform while the touchpad is in use,
>>> thing that is likely to happen because of the other bug I reported.
>>>
>>> Speaking of it, does any of you have any suggestion on how to debug it?
>> I was able to reproduce the initial issue by unloading hid-rmi and
>> i2c-hid while holding my fingers on the touchpad. Mika's patch fixes it
>> for me.
>>
>> For the original bug, you can modprobe i2c-hid debug=1 and we can see
>> what data the touchpad is reporting. That might help narrowing down if
>> it's noise which the touchpad thinks are fingers or if there is a
>> problem with the I2C lines causing spurious interrupts.
>>
>> Andrew
> I've already tried to do that and here what I got:
>
> When I release the finger, the last message is repeated 81 times.
> If the byte containing informations about the width of the finger
> becomes equal to either c0 or 0c at least once, the last message is
> repeated indefinitely and changes as soon as I start using the touchpad.
> The only way to stop it is to unload and reload i2c-hid.
The reports before log throttling kicks in would still be useful. For
instance c0 is outside of the range of finger width which we report so
something is wrong there. But, the touchpad should stop interrupting
once the finger is lifted. The fact that subsequent reads are reporting
the same data does sound like a problem with I2C getting confused and
continuously interrupting and reading the old finger data. I am also
curious about the value of the byte after the report id.
Andrew
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Gabriele Mazzotta @ 2014-12-11 19:25 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Mika Westerberg, linux-input, linux-kernel@vger.kernel.org,
Benjamin Tissoires, Andrew Duggan, Jiri Kosina
In-Reply-To: <CAN+gG=Fa5dPtfMSsU2MpqQJ6PWMTjN2izOfx2-ee-Shq21Hutw@mail.gmail.com>
On Thursday 11 December 2014 13:41:57 Benjamin Tissoires wrote:
> On Thu, Dec 11, 2014 at 1:16 PM, Gabriele Mazzotta
> <gabriele.mzt@gmail.com> wrote:
> > On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
> >> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
> >> > On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> >> > > my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> >> > > i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> >> > > other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> >> > > if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> >> > > pointer dereference.
> >> >
> >> > I'll look into this.
> >> >
> >> > I can reproduce this easily with i2c-hid + hid-multitouch following your
> >> > directions.
> >>
> >> Can you try the below patch?
> >>
> >> I think we shouldn't free buffers yet in ->stop() because we need the
> >> command buffer sending power commands to the device. Also it seems that
> >> ->start() re-allocates buffers anyway if maximum size increases.
> >>
> >> It shouldn't even leak memory as we release buffers at ->remove()
> >> anyway.
> >>
> >> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> >> index 62cec01937ea..68a8c938feea 100644
> >> --- a/drivers/hid/i2c-hid/i2c-hid.c
> >> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> >> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
> >>
> >> static void i2c_hid_stop(struct hid_device *hid)
> >> {
> >> - struct i2c_client *client = hid->driver_data;
> >> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> >> -
> >> hid->claimed = 0;
> >> -
> >> - i2c_hid_free_buffers(ihid);
> >> }
> >>
> >> static int i2c_hid_open(struct hid_device *hid)
>
> Mika,
>
> you can add my Rev-by when submitting this patch to the mailing list.
>
>
> >
> > Yes, it works, thanks.
> >
> > This change seems to also prevent kernel ooops when I unload either
> > i2c-hid or i2c-designware-platform while the touchpad is in use,
> > thing that is likely to happen because of the other bug I reported.
> >
> > Speaking of it, does any of you have any suggestion on how to debug it?
>
> Hehe, I coincidentally just replied to your bug with one patch to try
> (that was proposed by the intel folks back in May[1]).
> The powertop problem is IMO really worrying because the purpose of
> i2c_hid was to reduce power consumption :)
>
> Anyway, thanks for the quick test.
>
> Cheers,
> Benjamin
>
> [1] https://patchwork.kernel.org/patch/4133771/
Thanks for the reply.
It is sad that using the touchpad as PS/2 device gives me a better
battery life. What makes me use hid-rmi is that the touchpad works
definitely better with it (touchpad more reactive, correct min/max range
and palm detection that takes into account the width of the fingers).
The only other advantage of using the touchpad as PS/2 device (maybe
disadvantage for someone else) is that the keyboard illumination is
automatically turned on when the touchpad is in use. For some reason
it doesn't happen with hid-rmi, but that's really not a problem.
Anyway, I tried the patch and unfortunately it makes no difference.
Gabriele
^ permalink raw reply
* Re: NULL pointer dereference in i2c-hid
From: Gabriele Mazzotta @ 2014-12-11 19:40 UTC (permalink / raw)
To: Andrew Duggan
Cc: Mika Westerberg, linux-input, linux-kernel, benjamin.tissoires,
jkosina
In-Reply-To: <5489EEC7.1010709@synaptics.com>
On Thursday 11 December 2014 11:21:43 Andrew Duggan wrote:
> On 12/11/2014 11:11 AM, Gabriele Mazzotta wrote:
> > On Thursday 11 December 2014 10:40:05 Andrew Duggan wrote:
> >> On 12/11/2014 10:16 AM, Gabriele Mazzotta wrote:
> >>> On Thursday 11 December 2014 16:03:07 Mika Westerberg wrote:
> >>>> On Thu, Dec 11, 2014 at 10:58:01AM +0200, Mika Westerberg wrote:
> >>>>> On Wed, Dec 10, 2014 at 06:04:51PM +0100, Gabriele Mazzotta wrote:
> >>>>>> my laptop uses a touchpad that needs hid-rmi along with i2c-hid to work.
> >>>>>> i2c-hid and hid-rmi can be loaded and unloaded independelty from each
> >>>>>> other, however since 34f439e4afcd ("HID: i2c-hid: add runtime PM support")
> >>>>>> if I unload hid-rmi and after it I also unload i2c-hid, I get a NULL
> >>>>>> pointer dereference.
> >>>>> I'll look into this.
> >>>>>
> >>>>> I can reproduce this easily with i2c-hid + hid-multitouch following your
> >>>>> directions.
> >>>> Can you try the below patch?
> >>>>
> >>>> I think we shouldn't free buffers yet in ->stop() because we need the
> >>>> command buffer sending power commands to the device. Also it seems that
> >>>> ->start() re-allocates buffers anyway if maximum size increases.
> >>>>
> >>>> It shouldn't even leak memory as we release buffers at ->remove()
> >>>> anyway.
> >>>>
> >>>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> >>>> index 62cec01937ea..68a8c938feea 100644
> >>>> --- a/drivers/hid/i2c-hid/i2c-hid.c
> >>>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> >>>> @@ -705,12 +705,7 @@ static int i2c_hid_start(struct hid_device *hid)
> >>>>
> >>>> static void i2c_hid_stop(struct hid_device *hid)
> >>>> {
> >>>> - struct i2c_client *client = hid->driver_data;
> >>>> - struct i2c_hid *ihid = i2c_get_clientdata(client);
> >>>> -
> >>>> hid->claimed = 0;
> >>>> -
> >>>> - i2c_hid_free_buffers(ihid);
> >>>> }
> >>>>
> >>>> static int i2c_hid_open(struct hid_device *hid)
> >>> Yes, it works, thanks.
> >>>
> >>> This change seems to also prevent kernel ooops when I unload either
> >>> i2c-hid or i2c-designware-platform while the touchpad is in use,
> >>> thing that is likely to happen because of the other bug I reported.
> >>>
> >>> Speaking of it, does any of you have any suggestion on how to debug it?
> >> I was able to reproduce the initial issue by unloading hid-rmi and
> >> i2c-hid while holding my fingers on the touchpad. Mika's patch fixes it
> >> for me.
> >>
> >> For the original bug, you can modprobe i2c-hid debug=1 and we can see
> >> what data the touchpad is reporting. That might help narrowing down if
> >> it's noise which the touchpad thinks are fingers or if there is a
> >> problem with the I2C lines causing spurious interrupts.
> >>
> >> Andrew
> > I've already tried to do that and here what I got:
> >
> > When I release the finger, the last message is repeated 81 times.
> > If the byte containing informations about the width of the finger
> > becomes equal to either c0 or 0c at least once, the last message is
> > repeated indefinitely and changes as soon as I start using the touchpad.
> > The only way to stop it is to unload and reload i2c-hid.
> The reports before log throttling kicks in would still be useful. For
> instance c0 is outside of the range of finger width which we report so
> something is wrong there. But, the touchpad should stop interrupting
> once the finger is lifted. The fact that subsequent reads are reporting
> the same data does sound like a problem with I2C getting confused and
> continuously interrupting and reading the old finger data. I am also
> curious about the value of the byte after the report id.
>
> Andrew
If I'm not wrong c0 means that the width is 12 on y axis, while 0c means
that the width is 12 on the x axis.
I have to correct myself. The important thing is that the byte is either
cx or xc, where x is anything below c.
Another correction. Sometimes unloading i2c-hid is not enough, I have to
first disable the touchpad with xinput and then unload i2c-hid. If I
don't do it, the messages starts reappearing as soon as I reload i2c-hid.
I did several tests in the past months and I'm quite sure that the bug
happens only past xc/cy.
Here few lines right before the bug. The last line is repeated indefinitely:
[ 1983.527097] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 61 0a 5f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.537211] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 63 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.547329] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 64 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.557486] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 66 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.567663] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 67 5b 68 0a 60 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.577719] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 6a 0a 61 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.587852] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 67 5b 6b 0a 61 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.598001] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 67 5b 6e 0a 62 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.608215] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 61 0a 62 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.618288] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 64 0b 63 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.628493] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 67 0b 63 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.638552] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 69 0b 64 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.648663] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 68 5b 6c 0b 64 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.658789] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 68 5b 6f 0b 64 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.668923] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 61 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.678819] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 64 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.689230] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 66 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.699435] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 69 5b 68 0b 65 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.709502] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 6a 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.719574] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 6c 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.729713] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 69 5b 6e 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.739863] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 60 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.750001] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 62 0b 66 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.760150] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6a 5b 64 0b 67 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.770291] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 66 0b 67 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.780445] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 68 0b 67 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.790490] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6a 5b 5a 0b 68 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.800667] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6a 5b 4e 0c 69 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.810691] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 31 0c 69 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.820963] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 24 0c 6a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.831071] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 17 0c 6a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.841178] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 09 0c 6a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.851325] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 0b 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.861435] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6b 5b 0d 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.871566] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6b 5b 0f 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.881735] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 01 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.891975] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 03 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.902073] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 05 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.912155] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 07 0c 6b 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.922224] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 09 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.932364] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 0b 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.942480] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6c 5b 0d 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.952612] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6c 5b 0f 0c 6c 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.962774] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a f1 0d 6d 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.972932] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a f3 0d 6e 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.982872] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a f6 0d 6f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1983.993194] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6d 5a f9 0d 6f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.003295] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a fc 0d 6f 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.013511] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6d 5a ff 0d 70 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.023590] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a f2 0d 70 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.033747] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6e 5a f5 0e 71 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.043850] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a f8 0e 71 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.053873] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a fb 0e 71 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.064077] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6e 5a fe 0e 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.074207] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a f1 0e 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.084425] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 6f 5a f3 0e 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.094533] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a f6 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.104629] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a f8 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.114742] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a fa 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.124890] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a fc 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.135006] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 6f 5a fe 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.145149] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a f0 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.155317] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 70 5a e2 0e 73 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.165380] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a d4 0d 74 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.175532] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a d6 0d 74 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.185409] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a d8 0d 75 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.195761] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 70 5a db 0d 75 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.205909] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 70 5a dd 0d 75 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.216034] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 71 5a d0 0e 76 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.226198] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 71 5a d3 0e 77 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.236301] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 71 5a d6 0e 77 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.246520] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 71 5a cb 0f 79 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.256573] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 01 00 72 5a b1 0e 78 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.266697] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 72 5a a7 0e 77 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.276823] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 72 5a 6d 0d 72 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.286921] i2c_hid i2c-DLL060A:00: input: 20 00 0c 0c 01 00 73 59 96 0a 5a 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.296888] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 00 00 73 59 96 00 00 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.306825] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 00 00 73 59 96 00 00 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 1984.316980] i2c_hid i2c-DLL060A:00: input: 20 00 0c 04 00 00 73 59 96 00 00 03 09 40 00 00 90 86 1d 00 00 00 00 00 00 00 00 00 00 00 00 00
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox