* [PATCH v2 2/3] HID: logitech-hidpp: check WTP report length
From: Peter Wu @ 2014-12-16 15:55 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Nestor Lopez Casado, linux-input, linux-kernel
In-Reply-To: <1418745323-17133-1-git-send-email-peter@lekensteyn.nl>
Malicious USB devices can send bogus reports smaller than the expected
buffer size. Ensure that the length for WTP reports is valid to avoid
reading out of bounds.
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
v2: splitted original report length check patch
---
drivers/hid/hid-logitech-hidpp.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index b32f751..2f1b0ac 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -805,6 +805,11 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
switch (data[0]) {
case 0x02:
+ if (size < 2) {
+ hid_err(hdev, "Received HID report of bad size (%d)",
+ size);
+ return 1;
+ }
if (hidpp->quirks & HIDPP_QUIRK_WTP_PHYSICAL_BUTTONS) {
input_event(wd->input, EV_KEY, BTN_LEFT,
!!(data[1] & 0x01));
@@ -818,6 +823,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
return wtp_mouse_raw_xy_event(hidpp, &data[7]);
}
case REPORT_ID_HIDPP_LONG:
+ /* size is already checked in hidpp_raw_event. */
if ((report->fap.feature_index != wd->mt_feature_index) ||
(report->fap.funcindex_clientid != EVENT_TOUCHPAD_RAW_XY))
return 1;
--
2.1.3
^ permalink raw reply related
* [PATCH v2 3/3] HID: logitech-hidpp: separate HID++ from WTP processing
From: Peter Wu @ 2014-12-16 15:55 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Nestor Lopez Casado, linux-input, linux-kernel
In-Reply-To: <1418745323-17133-1-git-send-email-peter@lekensteyn.nl>
Previously wtp_raw_event would be called through
hidpp_raw_hidpp_event (for the touchpad report) and hidpp_raw_event
(for the mouse report).
This patch removes one calling surface, making a clearer distinction
between "generic HID++ processing" (matching internal reports) and
device-specific event processing.
Suggested-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
v2: splitted original report length check patch. Restructured code.
---
drivers/hid/hid-logitech-hidpp.c | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2f1b0ac..3dcd59c 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -942,7 +942,7 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
/*
* If the mutex is locked then we have a pending answer from a
- * previoulsly sent command
+ * previously sent command.
*/
if (unlikely(mutex_is_locked(&hidpp->send_mutex))) {
/*
@@ -973,9 +973,6 @@ static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
return 1;
}
- if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
- return wtp_raw_event(hidpp->hid_dev, data, size);
-
return 0;
}
@@ -983,7 +980,9 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
u8 *data, int size)
{
struct hidpp_device *hidpp = hid_get_drvdata(hdev);
+ int r = 0;
+ /* Generic HID++ processing. */
switch (data[0]) {
case REPORT_ID_HIDPP_LONG:
if (size != HIDPP_REPORT_LONG_LENGTH) {
@@ -991,16 +990,23 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
size);
return 1;
}
- return hidpp_raw_hidpp_event(hidpp, data, size);
+ r = hidpp_raw_hidpp_event(hidpp, data, size);
+ break;
case REPORT_ID_HIDPP_SHORT:
if (size != HIDPP_REPORT_SHORT_LENGTH) {
hid_err(hdev, "received hid++ report of bad size (%d)",
size);
return 1;
}
- return hidpp_raw_hidpp_event(hidpp, data, size);
+ r = hidpp_raw_hidpp_event(hidpp, data, size);
+ break;
}
+ /* If no report is available for further processing, skip calling
+ * raw_event of subclasses. */
+ if (r != 0)
+ return r;
+
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
return wtp_raw_event(hdev, data, size);
--
2.1.3
^ permalink raw reply related
* [PATCH v2 1/3] HID: logitech-dj: check report length
From: Peter Wu @ 2014-12-16 15:55 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Nestor Lopez Casado, linux-input, linux-kernel
In-Reply-To: <CAN+gG=EL0kYim=_uaS4yj7c_XC8rKa1eELxpVXaOuvnWgT_eQw@mail.gmail.com>
Malicious USB devices can send bogus reports smaller than the expected
buffer size. Ensure that the length is valid to avoid reading out of
bounds.
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
v1: patch 2/3 HID: logitech-{dj,hidpp}: check report length
v2: splitted original report length check patch
---
drivers/hid/hid-logitech-dj.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index c917ab6..5bc6d80 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
switch (data[0]) {
case REPORT_ID_DJ_SHORT:
+ if (size != DJREPORT_SHORT_LENGTH) {
+ dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
+ return false;
+ }
return logi_dj_dj_event(hdev, report, data, size);
case REPORT_ID_HIDPP_SHORT:
- /* intentional fallthrough */
+ if (size != HIDPP_REPORT_SHORT_LENGTH) {
+ dev_err(&hdev->dev,
+ "Short HID++ report of bad size (%d)", size);
+ return false;
+ }
+ return logi_dj_hidpp_event(hdev, report, data, size);
case REPORT_ID_HIDPP_LONG:
+ if (size != HIDPP_REPORT_LONG_LENGTH) {
+ dev_err(&hdev->dev,
+ "Long HID++ report of bad size (%d)", size);
+ return false;
+ }
return logi_dj_hidpp_event(hdev, report, data, size);
}
--
2.1.3
^ permalink raw reply related
* Re: [PATCH 2/3] HID: logitech-{dj,hidpp}: check report length
From: Benjamin Tissoires @ 2014-12-16 15:38 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <9001812.kk9brShn75@al>
On Tue, Dec 16, 2014 at 10:20 AM, Peter Wu <peter@lekensteyn.nl> wrote:
> On Tuesday 16 December 2014 09:53:07 Benjamin Tissoires wrote:
>> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
>> > Malicious USB devices can send bogus reports smaller than the expected
>> > buffer size. Ensure that the length is valid to avoid reading out of
>> > bounds.
>> >
>> > For the old WTP, I do not have a HID descriptor so just check for the
>> > minimum length in hidpp_raw_event (this can be changed to an inequality
>> > later).
>>
>> Actually you have it :)
>> All the DJ devices share the same report descriptors as they are
>> provided by hid-logitech-dj :)
>
> I see, I thought it was read from the hardware, but that probably
> applies to the other interfaces. Looks like the report should have a
> length of (16 + 12 * 2 + 8 + 8) / 8 = 7 bytes, correct?
Well, if you count the report ID, you get 8 :)
(it's easier to just plug a DJ mouse and look for the incoming report ;-P )
>
>> Anyway, the problem here would be with the bluetooth touchpad T651
>> which sends its raw events over teh mouse (0x02) collection (hint:
>> there is a "< 21" in wtp_raw_event :-P )
>
> Huh, how can that be allowed if the mouse descriptor accept less? Does
> the bluetooth layer pad the report somehow?
2 things actually:
- the bluetooth T651 connects through hidp directly (the bluetooth hid
stack), and it has its own report descriptor which contains the vendor
defined extra data in the mouse collection.
- look at the magic mouse report descriptor, apple does not even
announce the raw report ID, so from time to time, vendor do _really_
ugly things :)
>
>> >
>> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
>> > ---
>> > Hi,
>> >
>> > If you know that the WTP report (ID 2) has a length of 2, then you can change
>> > "<" to "!=" and remove the paragraph from the commit message.
>>
>> "<" should be kept for the reason above.
>>
>> >
>> > Kind regards,
>> > Peter
>> > ---
>> > drivers/hid/hid-logitech-dj.c | 16 +++++++++++++++-
>> > drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
>> > 2 files changed, 24 insertions(+), 4 deletions(-)
>> >
>> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>> > index c917ab6..5bc6d80 100644
>> > --- a/drivers/hid/hid-logitech-dj.c
>> > +++ b/drivers/hid/hid-logitech-dj.c
>> > @@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
>> >
>> > switch (data[0]) {
>> > case REPORT_ID_DJ_SHORT:
>> > + if (size != DJREPORT_SHORT_LENGTH) {
>> > + dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
>> > + return false;
>> > + }
>> > return logi_dj_dj_event(hdev, report, data, size);
>> > case REPORT_ID_HIDPP_SHORT:
>> > - /* intentional fallthrough */
>> > + if (size != HIDPP_REPORT_SHORT_LENGTH) {
>> > + dev_err(&hdev->dev,
>> > + "Short HID++ report of bad size (%d)", size);
>> > + return false;
>> > + }
>> > + return logi_dj_hidpp_event(hdev, report, data, size);
>> > case REPORT_ID_HIDPP_LONG:
>> > + if (size != HIDPP_REPORT_LONG_LENGTH) {
>> > + dev_err(&hdev->dev,
>> > + "Long HID++ report of bad size (%d)", size);
>> > + return false;
>> > + }
>>
>> This hunk is good to me.
>>
>> > return logi_dj_hidpp_event(hdev, report, data, size);
>> > }
>> >
>> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> > index ae23dec..2315358 100644
>> > --- a/drivers/hid/hid-logitech-hidpp.c
>> > +++ b/drivers/hid/hid-logitech-hidpp.c
>> > @@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
>> > return 1;
>> > }
>> > return hidpp_raw_hidpp_event(hidpp, data, size);
>> > + case 0x02:
>> > + if (size < 2) {
>> > + hid_err(hdev, "Received HID report of bad size (%d)",
>> > + size);
>> > + return 1;
>> > + }
>> > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>> > + return wtp_raw_event(hdev, data, size);
>> > + return 1;
>> > }
>> >
>> > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
>> > - return wtp_raw_event(hdev, data, size);
>>
>> This one is OK, but I don't like it.
>>
>> wtp_raw_event also expects long hid++ reports, and I'd prefer having
>> the raw_event() callback first checking on the generic hid++ reports,
>> and then addressing the various subclasses of devices.
>> After a better look at the code, it occurs that the actual code is
>> already pretty messed up.
>> wtp_raw_event() is called both in the generic hidpp_raw_event() and in
>> the specific hidpp_raw_hidpp_event().
>> This is IMO a design flaw and it should be fixed in a better way.
>>
>> I'd better have:
>>
>> - A check on the report size
>> - A call to the specific hidpp_raw_hidpp_event()
>> - if the previous does not return 1 (consumed event), then check on
>> all subclasses and call their specific raw_event.
>>
>> Does that make sense?
>>
>> If you agree, you can split the patch in 3, one for the -dj, one for
>> the -hidpp checks, and one for the redesign. I'd be happy to make the
>> redesign if you do not want to reshuffle it in a third patch.
>
> wtp_raw_event got called earlier through the long HID++ report handler
> (which returns, so it cannot be called twice?). It looked surprising at
Yeah, that's what I was referring to when I said I badly designed this.
> first, so it makes sense to split it up. I'll send a V2 for this patch
> (leaving the other ones in this bundle untouched).
OK, thanks for that. I'll check on the rest after your series.
>
> Kind regards,
> Peter
>
> PS. I saw a mail on LKML from a maintainer who was not so happy with the
> timing of patches. If my patch submissions are at the wrong moment,
> please let me know.
This is my personal opinion and Jiri can say something different. I
tend not to send big patches while there is a window opened. Sometimes
Jiri has the time to get through them, sometime he does not.
In this case, I think the patches you sent should be in the bugs fixes
categories, and, IMO should make into 3.19-rc1 or 3.19-rc2 (especially
the length check which could lead to CVEs if not tackled soon enough).
For these kind of things there is no timing, and the sooner the
better.
That being said, make sure that you keep track of those patches in
case they get lost for obvious reasons and be prepared to remind about
them if they do not make their way in Jiri's tree.
Jiri, comments?
Cheers,
Benjamin
>
>>
>> Cheers,
>> Benjamin
>>
>> > -
>> > return 0;
>> > }
>> >
>> > --
>> > 2.1.3
>
^ permalink raw reply
* [PATCH] hid: Add battery quirk for USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO keyboard
From: Karl Relton @ 2014-12-16 15:37 UTC (permalink / raw)
To: jkosina; +Cc: linux-input
Apple bluetooth wireless keyboard (sold in UK) has always reported zero
for battery strength no matter what condition the batteries are actually
in. With this patch applied (applying same quirk as other Apple
keyboards), the battery strength is now correctly reported.
Signed-off-by: Karl Relton <karllinuxtest.relton@ntlworld.com>
---
--- linux-3.16.0.orig/drivers/hid/hid-input.c 2014-12-10 10:59:37.864211493 +0000
+++ linux-3.16.0/drivers/hid/hid-input.c 2014-12-04 17:37:47.145113489 +0000
@@ -312,6 +312,9 @@ static const struct hid_device_id hid_ba
USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ANSI),
HID_BATTERY_QUIRK_PERCENT | HID_BATTERY_QUIRK_FEATURE },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
+ USB_DEVICE_ID_APPLE_ALU_WIRELESS_2011_ISO),
+ HID_BATTERY_QUIRK_PERCENT | HID_BATTERY_QUIRK_FEATURE },
+ { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_ALU_WIRELESS_ANSI),
HID_BATTERY_QUIRK_PERCENT | HID_BATTERY_QUIRK_FEATURE },
{}
^ permalink raw reply
* [RFC PATCH] fixp-arith: replace sin/cos table by a better precision one
From: Mauro Carvalho Chehab @ 2014-12-16 15:30 UTC (permalink / raw)
To: Linux Media Mailing List, Prashant Laddha (prladdha)
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Dmitry Torokhov,
Hans de Goede, linux-input
In-Reply-To: <20141216094005.4c0c354b@recife.lan>
The cos table used at fixp-arith.h has only 8 bits of precision.
That causes problems if it is reused on other drivers.
As some media drivers require a higher precision sin/cos
implementation, replace the current implementation by one that
will provide 32 bits precision.
The values generated by the new implementation matches the
32 bit precision of glibc's sin for an angle measured in
integer degrees.
It also provides support for fractional angles via linear
interpolation. On experimental calculus, when used a table
with a 0.001 degree angle, the maximum error for sin is
0.000038, with is likely good enough for practical purposes.
There are some logic there that seems to be specific to the
usage inside ff-memless.c. Move those logic to there, as they're
not needed elsewhere.
Signed-off-by: Mauro Carvalho Chehab <mchehab@osg.samsung.com>
---
Instead of adding yet-another implementation of integer sin()/cos(),
let's fix the one that already exists on a worldwide header for one
that would provide the needed precision.
While I tested the implementation on userspace, I didn't actually
check if ff-memless and ov534 drivers are working properly with
this change. Please test.
diff --git a/drivers/input/ff-memless.c b/drivers/input/ff-memless.c
index 74c0d8c6002a..fcc6c3368182 100644
--- a/drivers/input/ff-memless.c
+++ b/drivers/input/ff-memless.c
@@ -237,6 +237,18 @@ static u16 ml_calculate_direction(u16 direction, u16 force,
(force + new_force)) << 1;
}
+#define FRAC_N 8
+static inline s16 fixp_new16(s16 a)
+{
+ return ((s32)a) >> (16 - FRAC_N);
+}
+
+static inline s16 fixp_mult(s16 a, s16 b)
+{
+ a = ((s32)a * 0x100) / 0x7fff;
+ return ((s32)(a * b)) >> FRAC_N;
+}
+
/*
* Combine two effects and apply gain.
*/
@@ -247,7 +259,7 @@ static void ml_combine_effects(struct ff_effect *effect,
struct ff_effect *new = state->effect;
unsigned int strong, weak, i;
int x, y;
- fixp_t level;
+ s16 level;
switch (new->type) {
case FF_CONSTANT:
@@ -255,8 +267,8 @@ static void ml_combine_effects(struct ff_effect *effect,
level = fixp_new16(apply_envelope(state,
new->u.constant.level,
&new->u.constant.envelope));
- x = fixp_mult(fixp_sin(i), level) * gain / 0xffff;
- y = fixp_mult(-fixp_cos(i), level) * gain / 0xffff;
+ x = fixp_mult(fixp_sin16(i), level) * gain / 0xffff;
+ y = fixp_mult(-fixp_cos16(i), level) * gain / 0xffff;
/*
* here we abuse ff_ramp to hold x and y of constant force
* If in future any driver wants something else than x and y
diff --git a/drivers/media/usb/gspca/ov534.c b/drivers/media/usb/gspca/ov534.c
index 90f0d637cd9d..50b72f6dfdc6 100644
--- a/drivers/media/usb/gspca/ov534.c
+++ b/drivers/media/usb/gspca/ov534.c
@@ -810,21 +810,16 @@ static void sethue(struct gspca_dev *gspca_dev, s32 val)
s16 huesin;
s16 huecos;
- /* fixp_sin and fixp_cos accept only positive values, while
- * our val is between -90 and 90
- */
- val += 360;
-
/* According to the datasheet the registers expect HUESIN and
* HUECOS to be the result of the trigonometric functions,
* scaled by 0x80.
*
- * The 0x100 here represents the maximun absolute value
+ * The 0x7fff here represents the maximum absolute value
* returned byt fixp_sin and fixp_cos, so the scaling will
* consider the result like in the interval [-1.0, 1.0].
*/
- huesin = fixp_sin(val) * 0x80 / 0x100;
- huecos = fixp_cos(val) * 0x80 / 0x100;
+ huesin = fixp_sin16(val) * 0x80 / 0x7fff;
+ huecos = fixp_cos16(val) * 0x80 / 0x7fff;
if (huesin < 0) {
sccb_reg_write(gspca_dev, 0xab,
diff --git a/include/linux/fixp-arith.h b/include/linux/fixp-arith.h
index 3089d7382325..857e7f782140 100644
--- a/include/linux/fixp-arith.h
+++ b/include/linux/fixp-arith.h
@@ -29,59 +29,104 @@
#include <linux/types.h>
-/* The type representing fixed-point values */
-typedef s16 fixp_t;
-
-#define FRAC_N 8
-#define FRAC_MASK ((1<<FRAC_N)-1)
-
-/* Not to be used directly. Use fixp_{cos,sin} */
-static const fixp_t cos_table[46] = {
- 0x0100, 0x00FF, 0x00FF, 0x00FE, 0x00FD, 0x00FC, 0x00FA, 0x00F8,
- 0x00F6, 0x00F3, 0x00F0, 0x00ED, 0x00E9, 0x00E6, 0x00E2, 0x00DD,
- 0x00D9, 0x00D4, 0x00CF, 0x00C9, 0x00C4, 0x00BE, 0x00B8, 0x00B1,
- 0x00AB, 0x00A4, 0x009D, 0x0096, 0x008F, 0x0087, 0x0080, 0x0078,
- 0x0070, 0x0068, 0x005F, 0x0057, 0x004F, 0x0046, 0x003D, 0x0035,
- 0x002C, 0x0023, 0x001A, 0x0011, 0x0008, 0x0000
+static const s32 sin_table[] = {
+ 0x00000000, 0x023be165, 0x04779632, 0x06b2f1d2, 0x08edc7b6, 0x0b27eb5c,
+ 0x0d61304d, 0x0f996a26, 0x11d06c96, 0x14060b67, 0x163a1a7d, 0x186c6ddd,
+ 0x1a9cd9ac, 0x1ccb3236, 0x1ef74bf2, 0x2120fb82, 0x234815ba, 0x256c6f9e,
+ 0x278dde6e, 0x29ac379f, 0x2bc750e8, 0x2ddf003f, 0x2ff31bdd, 0x32037a44,
+ 0x340ff241, 0x36185aee, 0x381c8bb5, 0x3a1c5c56, 0x3c17a4e7, 0x3e0e3ddb,
+ 0x3fffffff, 0x41ecc483, 0x43d464fa, 0x45b6bb5d, 0x4793a20f, 0x496af3e1,
+ 0x4b3c8c11, 0x4d084650, 0x4ecdfec6, 0x508d9210, 0x5246dd48, 0x53f9be04,
+ 0x55a6125a, 0x574bb8e5, 0x58ea90c2, 0x5a827999, 0x5c135399, 0x5d9cff82,
+ 0x5f1f5ea0, 0x609a52d1, 0x620dbe8a, 0x637984d3, 0x64dd894f, 0x6639b039,
+ 0x678dde6d, 0x68d9f963, 0x6a1de735, 0x6b598ea1, 0x6c8cd70a, 0x6db7a879,
+ 0x6ed9eba0, 0x6ff389de, 0x71046d3c, 0x720c8074, 0x730baeec, 0x7401e4bf,
+ 0x74ef0ebb, 0x75d31a5f, 0x76adf5e5, 0x777f903b, 0x7847d908, 0x7906c0af,
+ 0x79bc384c, 0x7a6831b8, 0x7b0a9f8c, 0x7ba3751c, 0x7c32a67c, 0x7cb82884,
+ 0x7d33f0c8, 0x7da5f5a3, 0x7e0e2e31, 0x7e6c924f, 0x7ec11aa3, 0x7f0bc095,
+ 0x7f4c7e52, 0x7f834ecf, 0x7fb02dc4, 0x7fd317b3, 0x7fec09e1, 0x7ffb025e,
+ 0x7fffffff
};
+/**
+ * fixp_sin32() returns the sin of an angle in degrees
+ *
+ * @degrees: angle, in degrees. It can be positive or negative
+ *
+ * The returned value ranges from -0x7fffffff to +0x7fffffff.
+ */
-/* a: 123 -> 123.0 */
-static inline fixp_t fixp_new(s16 a)
+static inline s32 fixp_sin32(int degrees)
{
- return a<<FRAC_N;
-}
+ s32 ret;
+ bool negative = false;
-/* a: 0xFFFF -> -1.0
- 0x8000 -> 1.0
- 0x0000 -> 0.0
-*/
-static inline fixp_t fixp_new16(s16 a)
-{
- return ((s32)a)>>(16-FRAC_N);
+ degrees = (degrees % 360 + 360) % 360;
+ if (degrees > 180) {
+ negative = true;
+ degrees -= 180;
+ }
+ if (degrees > 90)
+ degrees = 180 - degrees;
+
+ ret = sin_table[degrees];
+
+ return negative ? -ret : ret;
}
-static inline fixp_t fixp_cos(unsigned int degrees)
+/* cos(x) = sin(x + 90 degrees) */
+#define fixp_cos32(v) fixp_sin32((v) + 90)
+
+/*
+ * 16 bits variants
+ *
+ * The returned value ranges from -0x7fff to 0x7fff
+ */
+
+#define fixp_sin16(v) (fixp_sin32(v) >> 16)
+#define fixp_cos16(v) (fixp_cos32(v) >> 16)
+
+/**
+ * fixp_sin32_rad() - calculates the sin of an angle in radians
+ *
+ * @radians: angle, in radians
+ * @twopi: value to be used for 2*pi
+ *
+ * Provides a variant for the cases where just 360
+ * values is not enough. This function uses linear
+ * interpolation to a wider range of values given by
+ * twopi var.
+ *
+ * Experimental tests gave a maximum difference of
+ * 0.000038 between the value calculated by sin() and
+ * the one produced by this function, when twopi is
+ * equal to 360000. That seems to be enough precision
+ * for practical purposes.
+ */
+static inline s32 fixp_sin32_rad(u32 radians, u32 twopi)
{
- int quadrant = (degrees / 90) & 3;
- unsigned int i = degrees % 90;
+ int degrees;
+ s32 v1, v2, dx, dy;
+ s64 tmp;
- if (quadrant == 1 || quadrant == 3)
- i = 90 - i;
+ degrees = (radians * 360) / twopi;
- i >>= 1;
+ v1 = fixp_sin32(degrees);
+ v2 = fixp_sin32(degrees + 1);
- return (quadrant == 1 || quadrant == 2)? -cos_table[i] : cos_table[i];
-}
+ dx = twopi / 360;
+ dy = v2 - v1;
-static inline fixp_t fixp_sin(unsigned int degrees)
-{
- return -fixp_cos(degrees + 90);
-}
+ tmp = radians - (degrees * twopi) / 360;
+ tmp *= dy;
+ do_div(tmp, dx);
-static inline fixp_t fixp_mult(fixp_t a, fixp_t b)
-{
- return ((s32)(a*b))>>FRAC_N;
+ return v1 + tmp;
}
+/* cos(x) = sin(x + pi radians) */
+
+#define fixp_cos32_rad(rad, twopi) \
+ fixp_sin32_rad(rad + twopi/2, twopi)
+
#endif
--
2.1.0
^ permalink raw reply related
* Re: [PATCH v4 0/6] Touchscreen performance related fixes
From: Richard Cochran @ 2014-12-16 15:30 UTC (permalink / raw)
To: Catalin Crenguta
Cc: Griffis, Brad, Sebastian Andrzej Siewior, Nicolae Rosia,
R, Vignesh, Rob Herring, Pawel Moll, Mark Rutland, Ian Campbell,
Kumar Gala, Benoit Cousson, Tony Lindgren, Russell King,
Jonathan Cameron, Hartmut Knaack, Dmitry Torokhov, Lee Jones,
Lars-Peter Clausen, Peter Meerwald, Samuel Ortiz, Balbi, Felipe,
Sanjeev Sharma, Paul Gortmaker, Jan Kardell
In-Reply-To: <CABH3fy8S1Hb9LK=ZceWgJsNvLbq8pwu00Tcj_0vE4j6QzcO+kg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Tue, Dec 16, 2014 at 10:31:47AM +0200, Catalin Crenguta wrote:
>
> It seems that because the ribbon cable has both the analog and digital
> signals, the analog signals are affected by the digital ones (hence
> the touchscreen was working OK when the display was disabled). Putting
> decoupling capacitors on X+, X-, Y+, Y- reduces the noise and the
> false events disappear.
IOW, the root cause is a design flaw in the BB-View 4.3 Cape?
Thanks,
Richard
^ permalink raw reply
* Re: [PATCHv3 5/5] ARM: sunxi: dts: Add note ps2 pins conflict with hdmi
From: Chen-Yu Tsai @ 2014-12-16 15:21 UTC (permalink / raw)
To: Vishnu Patekar
Cc: linux-input-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Maxime Ripard, Dmitry Torokhov, Hans De Goede, Mark Rutland,
devicetree, Russell King - ARM Linux, Pawel Moll, Ian Campbell,
benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org,
msalter-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, linux-kernel,
ralf-6z/3iImG2C8G8FEW9MqTrA@public.gmane.org, Rob Herring,
jdelvare-l3A5Bk7waGM@public.gmane.org, Kumar Gala, Grant Likely,
linux-arm-kernel
In-Reply-To: <CAEzqOZtOycU5LT8b3SHOymMpMOMExGC9PHiZ9wHtAjxR2R5+Ew-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi,
On Sat, Dec 13, 2014 at 11:18 AM, Vishnu Patekar
<vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello Chen-Yu,
> Thank you for pointing out styling error.
>
> On Sat, Dec 13, 2014 at 7:36 AM, Chen-Yu Tsai <wens-jdAy2FN1RRM@public.gmane.org> wrote:
>>
>> Hi,
>>
>> On Sat, Dec 13, 2014 at 2:25 AM, VishnuPatekar
>> <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > 1. Please note that ps20 pins conflict with HDMI on Lime2 Board
>> > so, by deault ps20 and ps21 are disabled for Lime2 Board.
>> > There is no on board ps2 connector and these pins can be used
>> > for different purpose.
>> >
>> > Signed-off-by: VishnuPatekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> > ---
>> > arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts | 6 +++++-
>> > 1 file changed, 5 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > index ed364d5..951b615 100644
>> > --- a/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > +++ b/arch/arm/boot/dts/sun7i-a20-olinuxino-lime2.dts
>> > @@ -112,7 +112,11 @@
>> > pinctrl-0 = <&uart0_pins_a>;
>> > status = "okay";
>> > };
>> > -
>> > +
>> > + /* PS2 0 and PS2 1 are disabled by default; Please note
>> > that
>> > + ps20 pins conflict with HDMI on Lime2 Board
>> > + */
>> > +
>>
>> Multi-line comments should be:
>>
>> /*
>> * line 1
>> * line 2
>> * ...
>> */
>
> Okie.
So, in fact this patch shouldnt be needed. The LIME2 uses the dedicated
pins for HDMI DDC. PS2 0 pins PI20 and PI21 are in fact routed to
the GPIO-2 header. They are _not_ used for HDMI. Nothing bad should
happen unless you also multiplex PI20/PI21 to HSCL/HDSA.
If you check the schematics Olimex published and trace the HDMI lines,
you will see that this is the case.
ChenYu
>>
>>
>> And why is there a delete and insert for an empty line?
>> Weird... though git seems to ignore it when applying.
>
> I used clean_patch to clear the styling error. that might have modified it.
>
>
>>
>>
>> ChenYu
>>
>> > i2c0: i2c@01c2ac00 {
>> > pinctrl-names = "default";
>> > pinctrl-0 = <&i2c0_pins_a>;
>> > --
>> > 1.7.9.5
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/3] HID: logitech-{dj,hidpp}: check report length
From: Peter Wu @ 2014-12-16 15:20 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nestor Lopez Casado, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=HWqpy6MUfWvwBavk+EkmwbBiCCN0L1_i8H61WYRU0yGw@mail.gmail.com>
On Tuesday 16 December 2014 09:53:07 Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> > Malicious USB devices can send bogus reports smaller than the expected
> > buffer size. Ensure that the length is valid to avoid reading out of
> > bounds.
> >
> > For the old WTP, I do not have a HID descriptor so just check for the
> > minimum length in hidpp_raw_event (this can be changed to an inequality
> > later).
>
> Actually you have it :)
> All the DJ devices share the same report descriptors as they are
> provided by hid-logitech-dj :)
I see, I thought it was read from the hardware, but that probably
applies to the other interfaces. Looks like the report should have a
length of (16 + 12 * 2 + 8 + 8) / 8 = 7 bytes, correct?
> Anyway, the problem here would be with the bluetooth touchpad T651
> which sends its raw events over teh mouse (0x02) collection (hint:
> there is a "< 21" in wtp_raw_event :-P )
Huh, how can that be allowed if the mouse descriptor accept less? Does
the bluetooth layer pad the report somehow?
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
> > Hi,
> >
> > If you know that the WTP report (ID 2) has a length of 2, then you can change
> > "<" to "!=" and remove the paragraph from the commit message.
>
> "<" should be kept for the reason above.
>
> >
> > Kind regards,
> > Peter
> > ---
> > drivers/hid/hid-logitech-dj.c | 16 +++++++++++++++-
> > drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
> > 2 files changed, 24 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > index c917ab6..5bc6d80 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
> >
> > switch (data[0]) {
> > case REPORT_ID_DJ_SHORT:
> > + if (size != DJREPORT_SHORT_LENGTH) {
> > + dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
> > + return false;
> > + }
> > return logi_dj_dj_event(hdev, report, data, size);
> > case REPORT_ID_HIDPP_SHORT:
> > - /* intentional fallthrough */
> > + if (size != HIDPP_REPORT_SHORT_LENGTH) {
> > + dev_err(&hdev->dev,
> > + "Short HID++ report of bad size (%d)", size);
> > + return false;
> > + }
> > + return logi_dj_hidpp_event(hdev, report, data, size);
> > case REPORT_ID_HIDPP_LONG:
> > + if (size != HIDPP_REPORT_LONG_LENGTH) {
> > + dev_err(&hdev->dev,
> > + "Long HID++ report of bad size (%d)", size);
> > + return false;
> > + }
>
> This hunk is good to me.
>
> > return logi_dj_hidpp_event(hdev, report, data, size);
> > }
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index ae23dec..2315358 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
> > return 1;
> > }
> > return hidpp_raw_hidpp_event(hidpp, data, size);
> > + case 0x02:
> > + if (size < 2) {
> > + hid_err(hdev, "Received HID report of bad size (%d)",
> > + size);
> > + return 1;
> > + }
> > + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > + return wtp_raw_event(hdev, data, size);
> > + return 1;
> > }
> >
> > - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> > - return wtp_raw_event(hdev, data, size);
>
> This one is OK, but I don't like it.
>
> wtp_raw_event also expects long hid++ reports, and I'd prefer having
> the raw_event() callback first checking on the generic hid++ reports,
> and then addressing the various subclasses of devices.
> After a better look at the code, it occurs that the actual code is
> already pretty messed up.
> wtp_raw_event() is called both in the generic hidpp_raw_event() and in
> the specific hidpp_raw_hidpp_event().
> This is IMO a design flaw and it should be fixed in a better way.
>
> I'd better have:
>
> - A check on the report size
> - A call to the specific hidpp_raw_hidpp_event()
> - if the previous does not return 1 (consumed event), then check on
> all subclasses and call their specific raw_event.
>
> Does that make sense?
>
> If you agree, you can split the patch in 3, one for the -dj, one for
> the -hidpp checks, and one for the redesign. I'd be happy to make the
> redesign if you do not want to reshuffle it in a third patch.
wtp_raw_event got called earlier through the long HID++ report handler
(which returns, so it cannot be called twice?). It looked surprising at
first, so it makes sense to split it up. I'll send a V2 for this patch
(leaving the other ones in this bundle untouched).
Kind regards,
Peter
PS. I saw a mail on LKML from a maintainer who was not so happy with the
timing of patches. If my patch submissions are at the wrong moment,
please let me know.
>
> Cheers,
> Benjamin
>
> > -
> > return 0;
> > }
> >
> > --
> > 2.1.3
^ permalink raw reply
* Re: [PATCH 3/3] HID: logitech-hidpp: avoid unintended fall-through
From: Benjamin Tissoires @ 2014-12-16 14:54 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Benjamin Tissoires, Nestor Lopez Casado, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <1418691016-30681-4-git-send-email-peter@lekensteyn.nl>
On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> Add a return to avoid a fall-through. Introduced in commit
> 57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID: logitech-hidpp: add
> support of the first Logitech Wireless Touchpad").
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
This one is reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2315358..09eee17 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -811,6 +811,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
> input_event(wd->input, EV_KEY, BTN_RIGHT,
> !!(data[1] & 0x02));
> input_sync(wd->input);
> + return 0;
> } else {
> if (size < 21)
> return 1;
> --
> 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 2/3] HID: logitech-{dj,hidpp}: check report length
From: Benjamin Tissoires @ 2014-12-16 14:53 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <1418691016-30681-3-git-send-email-peter@lekensteyn.nl>
Hi Peter,
On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> Malicious USB devices can send bogus reports smaller than the expected
> buffer size. Ensure that the length is valid to avoid reading out of
> bounds.
>
> For the old WTP, I do not have a HID descriptor so just check for the
> minimum length in hidpp_raw_event (this can be changed to an inequality
> later).
Actually you have it :)
All the DJ devices share the same report descriptors as they are
provided by hid-logitech-dj :)
Anyway, the problem here would be with the bluetooth touchpad T651
which sends its raw events over teh mouse (0x02) collection (hint:
there is a "< 21" in wtp_raw_event :-P )
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
> Hi,
>
> If you know that the WTP report (ID 2) has a length of 2, then you can change
> "<" to "!=" and remove the paragraph from the commit message.
"<" should be kept for the reason above.
>
> Kind regards,
> Peter
> ---
> drivers/hid/hid-logitech-dj.c | 16 +++++++++++++++-
> drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index c917ab6..5bc6d80 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
>
> switch (data[0]) {
> case REPORT_ID_DJ_SHORT:
> + if (size != DJREPORT_SHORT_LENGTH) {
> + dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
> + return false;
> + }
> return logi_dj_dj_event(hdev, report, data, size);
> case REPORT_ID_HIDPP_SHORT:
> - /* intentional fallthrough */
> + if (size != HIDPP_REPORT_SHORT_LENGTH) {
> + dev_err(&hdev->dev,
> + "Short HID++ report of bad size (%d)", size);
> + return false;
> + }
> + return logi_dj_hidpp_event(hdev, report, data, size);
> case REPORT_ID_HIDPP_LONG:
> + if (size != HIDPP_REPORT_LONG_LENGTH) {
> + dev_err(&hdev->dev,
> + "Long HID++ report of bad size (%d)", size);
> + return false;
> + }
This hunk is good to me.
> return logi_dj_hidpp_event(hdev, report, data, size);
> }
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index ae23dec..2315358 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
> return 1;
> }
> return hidpp_raw_hidpp_event(hidpp, data, size);
> + case 0x02:
> + if (size < 2) {
> + hid_err(hdev, "Received HID report of bad size (%d)",
> + size);
> + return 1;
> + }
> + if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> + return wtp_raw_event(hdev, data, size);
> + return 1;
> }
>
> - if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
> - return wtp_raw_event(hdev, data, size);
This one is OK, but I don't like it.
wtp_raw_event also expects long hid++ reports, and I'd prefer having
the raw_event() callback first checking on the generic hid++ reports,
and then addressing the various subclasses of devices.
After a better look at the code, it occurs that the actual code is
already pretty messed up.
wtp_raw_event() is called both in the generic hidpp_raw_event() and in
the specific hidpp_raw_hidpp_event().
This is IMO a design flaw and it should be fixed in a better way.
I'd better have:
- A check on the report size
- A call to the specific hidpp_raw_hidpp_event()
- if the previous does not return 1 (consumed event), then check on
all subclasses and call their specific raw_event.
Does that make sense?
If you agree, you can split the patch in 3, one for the -dj, one for
the -hidpp checks, and one for the redesign. I'd be happy to make the
redesign if you do not want to reshuffle it in a third patch.
Cheers,
Benjamin
> -
> return 0;
> }
>
> --
> 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 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too
From: Peter Wu @ 2014-12-16 14:52 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Nestor Lopez Casado, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <CAN+gG=FiL1SY84W5acowe8hkoHnp6ObkbCtoeFu0s2juone=+Q@mail.gmail.com>
On Tuesday 16 December 2014 09:33:44 Benjamin Tissoires wrote:
> On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> > Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> > these errors too to avoid 5 second delays when the device reports an
> > error. Caught by... well, a bug in the QEMU emulation of this receiver.
> >
> > Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> > it has no functional difference.
> >
> > Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> > ---
>
> I'd like to have Nestor's opinion on this. I did not manage to find on
> the documentation that HID++ 2.0 Long report error code is 0xff, so
> introducing this change without Logitech's blessing would be
> unfortunate.
> I understand this will fix your qemu problem, but I am not entirely
> sure if we do not have to check on 0xff and 0x8f in both short and
> long responses.
>
> Cheers,
> Benjamin
The error code was found by probing the hardware. The HID++ 2.0 spec
does define some error codes, for example an OutOfRange error when
GetFeatureID is called with a featureIndex greater than the available
features count. The documentation also defines the valid FeatureIndex
range as 1..254, so I thought it was reasonable to assume that 0xff is
the HID++ 2.0 error indicator.
Nestor, so far I have only seen the OutOfRange error when the arguments
are invalid. Are there other cases where HID++ 2.0 are reported instead
of HID++ 1.0?
QEMU was not the problem though, it was just a bug in my
usb-ltunify-receiver device emulation which exposed this missing check.
Kind regards,
Peter
> > drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 2f420c0..ae23dec 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -105,6 +105,7 @@ struct hidpp_device {
> > };
> >
> >
> > +/* HID++ 1.0 error codes */
> > #define HIDPP_ERROR 0x8f
> > #define HIDPP_ERROR_SUCCESS 0x00
> > #define HIDPP_ERROR_INVALID_SUBID 0x01
> > @@ -119,6 +120,8 @@ struct hidpp_device {
> > #define HIDPP_ERROR_REQUEST_UNAVAILABLE 0x0a
> > #define HIDPP_ERROR_INVALID_PARAM_VALUE 0x0b
> > #define HIDPP_ERROR_WRONG_PIN_CODE 0x0c
> > +/* HID++ 2.0 error codes */
> > +#define HIDPP20_ERROR 0xff
> >
> > static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
> >
> > @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > }
> >
> > if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > - response->fap.feature_index == HIDPP_ERROR) {
> > + response->rap.sub_id == HIDPP_ERROR) {
> > + ret = response->rap.params[1];
> > + dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> > + goto exit;
> > + }
> > +
> > + if (response->report_id == REPORT_ID_HIDPP_LONG &&
> > + response->fap.feature_index == HIDPP20_ERROR) {
> > ret = response->fap.params[1];
> > - dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
> > + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> > goto exit;
> > }
> >
> > @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
> > static inline bool hidpp_match_error(struct hidpp_report *question,
> > struct hidpp_report *answer)
> > {
> > - return (answer->fap.feature_index == HIDPP_ERROR) &&
> > + return ((answer->rap.sub_id == HIDPP_ERROR) ||
> > + (answer->fap.feature_index == HIDPP20_ERROR)) &&
> > (answer->fap.funcindex_clientid == question->fap.feature_index) &&
> > (answer->fap.params[0] == question->fap.funcindex_clientid);
> > }
> > --
> > 2.1.3
^ permalink raw reply
* Re: [PATCH 1/3] HID: logitech-hidpp: detect HID++ 2.0 errors too
From: Benjamin Tissoires @ 2014-12-16 14:33 UTC (permalink / raw)
To: Peter Wu
Cc: Jiri Kosina, Nestor Lopez Casado, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <1418691016-30681-2-git-send-email-peter@lekensteyn.nl>
Hi Peter,
On Mon, Dec 15, 2014 at 7:50 PM, Peter Wu <peter@lekensteyn.nl> wrote:
> Devices speaking HID++ 2.0 report a different error code (0xff). Detect
> these errors too to avoid 5 second delays when the device reports an
> error. Caught by... well, a bug in the QEMU emulation of this receiver.
>
> Renamed fap to rap for HID++ 1.0 errors because it is more logical,
> it has no functional difference.
>
> Signed-off-by: Peter Wu <peter@lekensteyn.nl>
> ---
I'd like to have Nestor's opinion on this. I did not manage to find on
the documentation that HID++ 2.0 Long report error code is 0xff, so
introducing this change without Logitech's blessing would be
unfortunate.
I understand this will fix your qemu problem, but I am not entirely
sure if we do not have to check on 0xff and 0x8f in both short and
long responses.
Cheers,
Benjamin
> drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2f420c0..ae23dec 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -105,6 +105,7 @@ struct hidpp_device {
> };
>
>
> +/* HID++ 1.0 error codes */
> #define HIDPP_ERROR 0x8f
> #define HIDPP_ERROR_SUCCESS 0x00
> #define HIDPP_ERROR_INVALID_SUBID 0x01
> @@ -119,6 +120,8 @@ struct hidpp_device {
> #define HIDPP_ERROR_REQUEST_UNAVAILABLE 0x0a
> #define HIDPP_ERROR_INVALID_PARAM_VALUE 0x0b
> #define HIDPP_ERROR_WRONG_PIN_CODE 0x0c
> +/* HID++ 2.0 error codes */
> +#define HIDPP20_ERROR 0xff
>
> static void hidpp_connect_event(struct hidpp_device *hidpp_dev);
>
> @@ -192,9 +195,16 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> }
>
> if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> - response->fap.feature_index == HIDPP_ERROR) {
> + response->rap.sub_id == HIDPP_ERROR) {
> + ret = response->rap.params[1];
> + dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> + goto exit;
> + }
> +
> + if (response->report_id == REPORT_ID_HIDPP_LONG &&
> + response->fap.feature_index == HIDPP20_ERROR) {
> ret = response->fap.params[1];
> - dbg_hid("__hidpp_send_report got hidpp error %02X\n", ret);
> + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> goto exit;
> }
>
> @@ -271,7 +281,8 @@ static inline bool hidpp_match_answer(struct hidpp_report *question,
> static inline bool hidpp_match_error(struct hidpp_report *question,
> struct hidpp_report *answer)
> {
> - return (answer->fap.feature_index == HIDPP_ERROR) &&
> + return ((answer->rap.sub_id == HIDPP_ERROR) ||
> + (answer->fap.feature_index == HIDPP20_ERROR)) &&
> (answer->fap.funcindex_clientid == question->fap.feature_index) &&
> (answer->fap.params[0] == question->fap.funcindex_clientid);
> }
> --
> 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 v15 09/12] input: cyapa: add gen5 trackpad device firmware update function support
From: Jeremiah Mahler @ 2014-12-16 13:56 UTC (permalink / raw)
To: Dudley Du; +Cc: dmitry.torokhov, rydberg, bleung, linux-input, linux-kernel
In-Reply-To: <1418624603-19054-10-git-send-email-dudley.dulixin@gmail.com>
Dudley,
On Mon, Dec 15, 2014 at 02:23:20PM +0800, Dudley Du wrote:
> Add firmware image update function supported for gen5 trackpad device,
> it can be used through sysfs update_fw interface.
> TEST=test on Chromebooks.
>
> Signed-off-by: Dudley Du <dudley.dulixin@gmail.com>
> ---
> drivers/input/mouse/Kconfig | 1 +
> drivers/input/mouse/cyapa_gen5.c | 292 ++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 292 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/mouse/Kconfig b/drivers/input/mouse/Kconfig
> index d8b46b0..728490e 100644
> --- a/drivers/input/mouse/Kconfig
> +++ b/drivers/input/mouse/Kconfig
> @@ -206,6 +206,7 @@ config MOUSE_BCM5974
> config MOUSE_CYAPA
> tristate "Cypress APA I2C Trackpad support"
> depends on I2C
> + select CRC_ITU_T
> help
Just found out that if I2C_DESIGNWARE_PCI isn't enabled the touchpad
won't work. Verify this on your machines. Then perhaps add a depends
for I2C_DESIGNWARE_PCI instead of I2C since it would include the former.
[...]
--
- Jeremiah Mahler
^ permalink raw reply
* Re: [PATCH 1/7] input: alps: Set correct name of psmouse device in alps_init()
From: Pali Rohár @ 2014-12-16 11:58 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Hans de Goede, Vadim Klishko, linux-input, linux-kernel
In-Reply-To: <20141216050234.GA35408@dtor-ws>
[-- Attachment #1: Type: Text/Plain, Size: 3154 bytes --]
On Tuesday 16 December 2014 06:02:34 Dmitry Torokhov wrote:
> Hi Pali,
>
> On Fri, Nov 14, 2014 at 08:38:20PM +0100, Pali Rohár wrote:
> > On some laptops after starting them from off state (not
> > after reboot), function alps_probe_trackstick_v3() (called
> > from function alps_identify()) does not detect trackstick.
> > To fix this problem we need to reset device. But function
> > alps_identify() is called also from alps_detect() and we do
> > not want to reset device in detect function because it will
> > slow down initialization of all other non alps devices.
> >
> > This patch moves code for setting correct device name &
> > protocol from function alps_detect() to alps_init() which
> > already doing full device reset.
> >
> > So this patch removes need to do trackstick detection in
> > alps_detect() function.
> >
> > Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> > ---
> >
> > drivers/input/mouse/alps.c | 17 ++++++++++++++---
> > 1 file changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/input/mouse/alps.c
> > b/drivers/input/mouse/alps.c index 8d85c79..9ffa98d 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2392,6 +2392,10 @@ int alps_init(struct psmouse
> > *psmouse)
> >
> > if (input_register_device(priv->dev2))
> >
> > goto init_fail;
> >
> > + if (!(priv->flags & ALPS_DUALPOINT))
> > + psmouse->name = "GlidePoint TouchPad";
> > + psmouse->model = priv->proto_version;
> > +
> >
> > psmouse->protocol_handler = alps_process_byte;
> > psmouse->poll = alps_poll;
> > psmouse->disconnect = alps_disconnect;
> >
> > @@ -2422,11 +2426,18 @@ int alps_detect(struct psmouse
> > *psmouse, bool set_properties)
> >
> > return -1;
> >
> > if (set_properties) {
> >
> > + /*
> > + * NOTE: To detect model and trackstick presence we
need
> > to do + * full device reset. To speed up
detection
> > and prevent + * calling duplicate initialization
> > sequence (both in + * alps_detect() and
> > alps_init()) we set model/protocol + * version and
> > correct name in alps_init() (which will + * do
full
> > device reset). For now set name to DualPoint. + */
> >
> > psmouse->vendor = "ALPS";
> >
> > - psmouse->name = dummy.flags & ALPS_DUALPOINT ?
> > - "DualPoint TouchPad" : "GlidePoint";
> > - psmouse->model = dummy.proto_version << 8;
> > + psmouse->name = "DualPoint TouchPad";
> >
> > }
> >
> > +
>
> I do not quite like the way we change the device description
> back and forth. Do you think we could allocate the "real"
> priv structure in alps_detect() and have alps_init() expect
> to find it (and free it if set_properties is false). This way
> we'd go through initialization once in detect, it will be
> authoritative, and we would set the name of the device
> properly from the beginning.
>
> Thanks.
No without introducing another psmouse_reset call. I want to
reduce time of loading driver, so I think this is better.
--
Pali Rohár
pali.rohar@gmail.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCHv3 3/5] ARM: sunxi: dts: Add PS2 nodes to dtsi for A10 and A20
From: Maxime Ripard @ 2014-12-16 9:32 UTC (permalink / raw)
To: VishnuPatekar
Cc: linux-input, dmitry.torokhov, hdegoede, devicetree,
linux-arm-kernel, linux-kernel, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, galak, linux, grant.likely, benh, msalter, ralf,
jdelvare
In-Reply-To: <1418408748-9797-4-git-send-email-vishnupatekar0510@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]
Hi Vishnu,
On Fri, Dec 12, 2014 at 11:55:46PM +0530, VishnuPatekar wrote:
> 1) Fixup the sun4i ps/2 nodes interrupt property, sun4i interrupts take
> only 1 specifier
>
> 2) dt bindings should use the compat string for the earliest version of the
> hardware which has the relevant hardware block, unless there are differences,
> the A10 and A20 ps2 controllers are identical, so for both sun4i-a10-ps2
> should be used as compat string, update the sun7i.dtsi ps2 entries to
> use the sun4i-a10-ps2 compat string.
This shouldn't be a changelog, but what this patch actually does.
> Signed-off-by: VishnuPatekar <vishnupatekar0510@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> arch/arm/boot/dts/sun4i-a10.dtsi | 17 +++++++++++++++++
> arch/arm/boot/dts/sun7i-a20.dtsi | 18 ++++++++++++++++++
> 2 files changed, 35 insertions(+)
>
> diff --git a/arch/arm/boot/dts/sun4i-a10.dtsi b/arch/arm/boot/dts/sun4i-a10.dtsi
> index 7b4099f..ef9a01c 100644
> --- a/arch/arm/boot/dts/sun4i-a10.dtsi
> +++ b/arch/arm/boot/dts/sun4i-a10.dtsi
> @@ -629,6 +629,7 @@
> allwinner,drive = <0>;
> allwinner,pull = <0>;
> };
> +
This is an uneeded change
> };
>
> timer@01c20c00 {
> @@ -795,5 +796,21 @@
> #address-cells = <1>;
> #size-cells = <0>;
> };
> +
> + ps20: ps2@01c2a000 {
> + compatible = "allwinner,sun4i-a10-ps2";
> + reg = <0x01c2a000 0x400>;
> + interrupts = <62>;
> + clocks = <&apb1_gates 6>;
> + status = "disabled";
> + };
> +
> + ps21: ps2@01c2a400 {
> + compatible = "allwinner,sun4i-a10-ps2";
> + reg = <0x01c2a400 0x400>;
> + interrupts = <63>;
> + clocks = <&apb1_gates 7>;
> + status = "disabled";
> + };
> };
> };
> diff --git a/arch/arm/boot/dts/sun7i-a20.dtsi b/arch/arm/boot/dts/sun7i-a20.dtsi
> index e21ce59..6ab7714 100644
> --- a/arch/arm/boot/dts/sun7i-a20.dtsi
> +++ b/arch/arm/boot/dts/sun7i-a20.dtsi
> @@ -866,6 +866,7 @@
> allwinner,drive = <0>;
> allwinner,pull = <0>;
> };
> +
Ditto.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: The return value of hid_input_report and raw_event
From: Jiri Kosina @ 2014-12-16 9:27 UTC (permalink / raw)
To: Peter Wu; +Cc: linux-input
In-Reply-To: <7774789.jK4HDCWEI4@al>
On Mon, 15 Dec 2014, Peter Wu wrote:
> Hi Jiri and list,
>
> The HID core has a hid_input_report function which returns an integer,
> but all its callers are not really changing their behavior based on the
> return value. The few^Wonly exception that does not completely ignore
> the return value is the hid-logitech-dj driver which prints a debugging
> message.
>
> Should this method be changed to return void? The kerneldoc does not
> document its return value anyway.
Hi Peter,
yes, that definitely would be an acceptable cleanup.
> Confusion 2: the raw_event callback of the hid_driver structure returns
> an int, but over time non-negative values are ignored by
> hid_input_report. This happened in the following change:
> --------------------------------------------------
> commit b1a1442a23776756b254b69786848a94d92445ba
> Author: Jiri Kosina <jkosina@suse.cz>
> Date: Mon Jun 3 11:27:48 2013 +0200
>
> HID: core: fix reporting of raw events
>
> hdrw->raw event can return three different return value types:
>
> - ret < 0 indicates that the hdrv driver found an error while parsing
> - ret == 0 indicates no error has been encountered, and the driver has
> processed the report
> - ret > 0 indicates that there was no parsing error, and the driver hasn't
> processed the event.
>
> Calling hid_report_raw_event() has to be called appropriately so that it
> reflects what has been done by ->raw_event() callback, otherwise we might
> updates of the in-kernel structure are lost upon arrival of the report, which
> is wrong.
>
> Reported-and-tested-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> Reported-and-tested-by: Daniel Leung <daniel.leung@linux.intel.com>
> Signed-off-by: Jiri Kosina <jkosina@suse.cz>
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index c272078..8f616bd 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1293,7 +1293,7 @@ int hid_input_report(struct hid_device *hid, int type, u8 *data, int size, int i
>
> if (hdrv && hdrv->raw_event && hid_match_report(hid, report)) {
> ret = hdrv->raw_event(hid, report, data, size);
> - if (ret != 0) {
> + if (ret < 0) {
> ret = ret < 0 ? ret : 0;
> goto unlock;
> }
> --------------------------------------------------
>
> So now it does not matter whether the raw_event method returns 0 or 1.
> The documentation says:
>
> raw_event and event should return 0 on no action performed, 1 when
> no further processing should be done and negative on error
>
> What does "no further processing" mean? I guess that the intention was
> to signal from a HID driver to ignore the packet, but that the above
> change was done to update the state and allow hidraw to pick up the
> report.
This change was done so that we don't call hid_report_raw_event() when
->raw_event() callback encountered error during parsing (and therefore
returned negative value).
See this thread
https://lkml.org/lkml/2013/4/8/540
for an example.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v4 0/6] Touchscreen performance related fixes
From: Catalin Crenguta @ 2014-12-16 8:31 UTC (permalink / raw)
To: Griffis, Brad
Cc: Sebastian Andrzej Siewior, Nicolae Rosia, R, Vignesh, Rob Herring,
Pawel Moll, Mark Rutland, Ian Campbell, Kumar Gala,
Benoit Cousson, Tony Lindgren, Russell King, Jonathan Cameron,
Hartmut Knaack,
richardcochran-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org,
Dmitry Torokhov, Lee Jones, Lars-Peter Clausen, Peter Meerwald,
Samuel Ortiz, Balbi, Felipe, Sanjeev Sharma, Paul Gortmaker
In-Reply-To: <912A29987EAE174BA6CF187D7CDFA9CE26FA135A-YmePFLaaepqIQmiDNMet8wC/G2K4zDHf@public.gmane.org>
On Fri, Dec 12, 2014 at 4:16 PM, Griffis, Brad <bgriffis-l0cyMroinI0@public.gmane.org> wrote:
> How are you configuring ti,charge-delay in your dts? I've seen this behavior on some custom boards where we were using a smaller charge delay (0x400) to begin with, and by upping it to 0xb000 we resolved the issue. These patches however already specified ti,charge-delay = 0xb000 by default so this would surprise me that it's still seeing that issue. Was the touchscreen working as expected before these new patches, or does it have issues both ways?
It seems that because the ribbon cable has both the analog and digital
signals, the analog signals are affected by the digital ones (hence
the touchscreen was working OK when the display was disabled). Putting
decoupling capacitors on X+, X-, Y+, Y- reduces the noise and the
false events disappear.
I'm sorry for all the noise I have generated.
Best regards,
^ permalink raw reply
* [PATCH] Input: Add support for CLOCK_BOOTTIME
From: Aniroop, Mathur @ 2014-12-16 8:23 UTC (permalink / raw)
To: dmitry.torokhov, dtor, linux-input; +Cc: aniroop.mathur, a.mathur
From: Aniroop Mathur <a.mathur@samsung.com>
This patch adds support for CLOCK_BOOTTIME for input event timestamp.
CLOCK_BOOTTIME includes suspend time, so it would allow applications
to get correct time difference between two events even when system
resumes from suspend state.
Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
drivers/input/evdev.c | 33 +++++++++++++++++++++------------
1 file changed, 21 insertions(+), 12 deletions(-)
diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index de05545..7825794 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -28,6 +28,13 @@
#include <linux/cdev.h>
#include "input-compat.h"
+enum clock_type {
+ REAL = 0,
+ MONO,
+ BOOT,
+ CLK_MAX
+};
+
struct evdev {
int open;
struct input_handle handle;
@@ -108,8 +115,9 @@ static void evdev_queue_syn_dropped(struct evdev_client *client)
struct input_event ev;
ktime_t time;
- time = (client->clkid == CLOCK_MONOTONIC) ?
- ktime_get() : ktime_get_real();
+ time = (client->clkid == CLOCK_REALTIME) ?
+ ktime_get_real() : (client->clkid == CLOCK_MONOTONIC) ?
+ ktime_get() : ktime_get_boottime();
ev.time = ktime_to_timeval(time);
ev.type = EV_SYN;
@@ -159,7 +167,7 @@ static void __pass_event(struct evdev_client *client,
static void evdev_pass_values(struct evdev_client *client,
const struct input_value *vals, unsigned int count,
- ktime_t mono, ktime_t real)
+ ktime_t *ev_time)
{
struct evdev *evdev = client->evdev;
const struct input_value *v;
@@ -169,8 +177,9 @@ static void evdev_pass_values(struct evdev_client *client,
if (client->revoked)
return;
- event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
- mono : real);
+ event.time = ktime_to_timeval(client->clkid == CLOCK_REALTIME ?
+ ev_time[REAL] : client->clkid == CLOCK_MONOTONIC ?
+ ev_time[MONO] : ev_time[BOOT]);
/* Interrupts are disabled, just acquire the lock. */
spin_lock(&client->buffer_lock);
@@ -198,21 +207,21 @@ static void evdev_events(struct input_handle *handle,
{
struct evdev *evdev = handle->private;
struct evdev_client *client;
- ktime_t time_mono, time_real;
+ ktime_t ev_time[CLK_MAX];
- time_mono = ktime_get();
- time_real = ktime_mono_to_real(time_mono);
+ ev_time[MONO] = ktime_get();
+ ev_time[REAL] = ktime_mono_to_real(ev_time[MONO]);
+ ev_time[BOOT] = ktime_get_boottime();
rcu_read_lock();
client = rcu_dereference(evdev->grab);
if (client)
- evdev_pass_values(client, vals, count, time_mono, time_real);
+ evdev_pass_values(client, vals, count, ev_time);
else
list_for_each_entry_rcu(client, &evdev->client_list, node)
- evdev_pass_values(client, vals, count,
- time_mono, time_real);
+ evdev_pass_values(client, vals, count, ev_time);
rcu_read_unlock();
}
@@ -874,7 +883,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
case EVIOCSCLOCKID:
if (copy_from_user(&i, p, sizeof(unsigned int)))
return -EFAULT;
- if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME)
+ if (i != CLOCK_MONOTONIC && i != CLOCK_REALTIME && i != CLOCK_BOOTTIME)
return -EINVAL;
client->clkid = i;
return 0;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH 1/7] input: alps: Set correct name of psmouse device in alps_init()
From: Dmitry Torokhov @ 2014-12-16 5:02 UTC (permalink / raw)
To: Pali Rohár
Cc: Hans de Goede, Yunkang Tang, Vadim Klishko, linux-input,
linux-kernel
In-Reply-To: <1415993906-13307-2-git-send-email-pali.rohar@gmail.com>
Hi Pali,
On Fri, Nov 14, 2014 at 08:38:20PM +0100, Pali Rohár wrote:
> On some laptops after starting them from off state (not after reboot), function
> alps_probe_trackstick_v3() (called from function alps_identify()) does not
> detect trackstick. To fix this problem we need to reset device. But function
> alps_identify() is called also from alps_detect() and we do not want to reset
> device in detect function because it will slow down initialization of all other
> non alps devices.
>
> This patch moves code for setting correct device name & protocol from function
> alps_detect() to alps_init() which already doing full device reset.
>
> So this patch removes need to do trackstick detection in alps_detect() function.
>
> Signed-off-by: Pali Rohár <pali.rohar@gmail.com>
> ---
> drivers/input/mouse/alps.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 8d85c79..9ffa98d 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -2392,6 +2392,10 @@ int alps_init(struct psmouse *psmouse)
> if (input_register_device(priv->dev2))
> goto init_fail;
>
> + if (!(priv->flags & ALPS_DUALPOINT))
> + psmouse->name = "GlidePoint TouchPad";
> + psmouse->model = priv->proto_version;
> +
> psmouse->protocol_handler = alps_process_byte;
> psmouse->poll = alps_poll;
> psmouse->disconnect = alps_disconnect;
> @@ -2422,11 +2426,18 @@ int alps_detect(struct psmouse *psmouse, bool set_properties)
> return -1;
>
> if (set_properties) {
> + /*
> + * NOTE: To detect model and trackstick presence we need to do
> + * full device reset. To speed up detection and prevent
> + * calling duplicate initialization sequence (both in
> + * alps_detect() and alps_init()) we set model/protocol
> + * version and correct name in alps_init() (which will
> + * do full device reset). For now set name to DualPoint.
> + */
> psmouse->vendor = "ALPS";
> - psmouse->name = dummy.flags & ALPS_DUALPOINT ?
> - "DualPoint TouchPad" : "GlidePoint";
> - psmouse->model = dummy.proto_version << 8;
> + psmouse->name = "DualPoint TouchPad";
> }
> +
I do not quite like the way we change the device description back and
forth. Do you think we could allocate the "real" priv structure in
alps_detect() and have alps_init() expect to find it (and free it if
set_properties is false). This way we'd go through initialization once
in detect, it will be authoritative, and we would set the name of the
device properly from the beginning.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v6 1/2] Input: add regulator haptic driver
From: Dmitry Torokhov @ 2014-12-16 1:42 UTC (permalink / raw)
To: Jaewon Kim
Cc: Kukjin Kim, linux-kernel, linux-samsung-soc, linux-input,
Chanwoo Choi, Hyunhee Kim
In-Reply-To: <548F8645.3070002@samsung.com>
On Tue, Dec 16, 2014 at 10:09:25AM +0900, Jaewon Kim wrote:
> Hi Dmitry,
>
> 2014년 12월 14일 04:56에 Dmitry Torokhov 이(가) 쓴 글:
> >Hi Jaewon,
> >
> >On Fri, Dec 12, 2014 at 07:32:28PM +0900, Jaewon Kim wrote:
...
> >>+static int __maybe_unused regulator_haptic_suspend(struct device *dev)
> >>+{
> >>+ struct platform_device *pdev = to_platform_device(dev);
> >>+ struct regulator_haptic *haptic = platform_get_drvdata(pdev);
> >>+
> >>+ mutex_lock(&haptic->mutex);
> >>+ if (haptic->enabled) {
> >>+ regulator_haptic_enable(haptic, false);
> >>+ haptic->suspend_state = true;
> >Why do we only set suspend_state if an effect was playing? I think we
> >should always indicate that the device is suspended so that we do not
> >try to start playing another effect - while it is true that normally
> >effects are played by request from userspace which should be frozen by
> >now, it is theoretically possible to trigger an effect from kernel as
> >well.
>
> This variable name seems to make you confuse.
> I used this variable to restore the old state.
>
> When kernel is entering suspend state while the motor is vibrating,
> I store vibrating state for vibrate again after escape suspend state.
>
>
> I will change variable name to "suspend_restore".
> And prevent to start playing effect when kernel entering suspend state.
You do not need to save if haptic was playing or not - on resume, if
haptic->magnitude != 0 you need to restart playing, otherwise leave it
off.
Thanks.
--
Dmitry
^ permalink raw reply
* RE: [PATCH v15 00/12] input: cyapa: instruction of cyapa patches
From: Dudley Du @ 2014-12-16 1:28 UTC (permalink / raw)
To: Jeremiah Mahler
Cc: dmitry.torokhov@gmail.com, rydberg@euromail.se, bleung@google.com,
David Solda, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20141215141023.GB918@newt.localdomain>
Jeremiah,
No problem. :-)
Thank you very much for your help and so much time on review these patches.
I will fix this issue as soon as possible.
Thanks,
Dudley
> -----Original Message-----
> From: linux-input-owner@vger.kernel.org
> [mailto:linux-input-owner@vger.kernel.org] On Behalf Of Jeremiah Mahler
> Sent: 2014?12?15? 22:10
> To: Dudley Du
> Cc: dmitry.torokhov@gmail.com; rydberg@euromail.se; bleung@google.com;
> David Solda; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v15 00/12] input: cyapa: instruction of cyapa patches
>
> Dudley,
>
> On Mon, Dec 15, 2014 at 02:23:11PM +0800, Dudley Du wrote:
> > V15 patches have below updates, details of other updates see history list:
> > 1) Fix all warning errors of sparse tool when running with "make C=1".
> > 2) Change variable name "unique_str" to "product_id" for clearer meanings.
> >
> >
> > This patch series is aimed to re-design the cyapa driver to support
> > old gen3 trackpad devices and new gen5 trackpad devices in one
> [...]
>
> The patch set still tests fine on my Acer C720.
>
> I went through all the patches and found lots of minor things to fix
> as you would expect from this large amount of code.
>
> I hope you don't get discouraged :-)
>
> --
> - Jeremiah Mahler
> --
> 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
This message and any attachments may contain Cypress (or its subsidiaries) confidential information. If it has been received in error, please advise the sender and immediately delete this message.
^ permalink raw reply
* Re: [PATCH v6 1/2] Input: add regulator haptic driver
From: Jaewon Kim @ 2014-12-16 1:09 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Kukjin Kim, linux-kernel, linux-samsung-soc, linux-input,
Chanwoo Choi, Hyunhee Kim
In-Reply-To: <20141213195611.GE22702@dtor-ws>
Hi Dmitry,
2014년 12월 14일 04:56에 Dmitry Torokhov 이(가) 쓴 글:
> Hi Jaewon,
>
> On Fri, Dec 12, 2014 at 07:32:28PM +0900, Jaewon Kim wrote:
>> This patch adds support for haptic driver controlled by
>> voltage of regulator. And this driver support for
>> Force Feedback interface from input framework
>>
>> Signed-off-by: Jaewon Kim <jaewon02.kim@samsung.com>
>> Signed-off-by: Hyunhee Kim <hyunhee.kim@samsung.com>
>> Acked-by: Kyungmin Park <kyungmin.park@samsung.com>
>> Tested-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>
>> Reviewed-by: Pankaj Dubey <pankaj.dubey@samsung.com>
>> ---
>> .../devicetree/bindings/input/regulator-haptic.txt | 21 ++
>> drivers/input/misc/Kconfig | 11 +
>> drivers/input/misc/Makefile | 1 +
>> drivers/input/misc/regulator-haptic.c | 259 ++++++++++++++++++++
>> include/linux/input/regulator-haptic.h | 31 +++
>> 5 files changed, 323 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/input/regulator-haptic.txt
>> create mode 100644 drivers/input/misc/regulator-haptic.c
>> create mode 100644 include/linux/input/regulator-haptic.h
>>
>> diff --git a/Documentation/devicetree/bindings/input/regulator-haptic.txt b/Documentation/devicetree/bindings/input/regulator-haptic.txt
>> new file mode 100644
>> index 0000000..3ed1c7e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/input/regulator-haptic.txt
>> @@ -0,0 +1,21 @@
>> +* Regulator Haptic Device Tree Bindings
>> +
>> +Required Properties:
>> + - compatible : Should be "regulator-haptic"
>> + - haptic-supply : Power supply to the haptic motor.
>> + [*] refer Documentation/devicetree/bindings/regulator/regulator.txt
>> +
>> + - max-microvolt : The maximum voltage value supplied to the haptic motor.
>> + [The unit of the voltage is a micro]
>> +
>> + - min-microvolt : The minimum voltage value supplied to the haptic motor.
>> + [The unit of the voltage is a micro]
>> +
>> +Example:
>> +
>> + haptics {
>> + compatible = "regulator-haptic";
>> + haptic-supply = <&motor_regulator>;
>> + max-microvolt = <2700000>;
>> + min-microvolt = <1100000>;
>> + };
>> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
>> index 23297ab..e5e556d 100644
>> --- a/drivers/input/misc/Kconfig
>> +++ b/drivers/input/misc/Kconfig
>> @@ -394,6 +394,17 @@ config INPUT_CM109
>> To compile this driver as a module, choose M here: the module will be
>> called cm109.
>>
>> +config INPUT_REGULATOR_HAPTIC
>> + tristate "regulator haptics support"
>> + select INPUT_FF_MEMLESS
>> + help
>> + This option enables device driver support for the haptic controlled
>> + by regulator. This driver supports ff-memless interface
>> + from input framework.
>> +
>> + To compile this driver as a module, choose M here: the
>> + module will be called regulator-haptic.
>> +
>> config INPUT_RETU_PWRBUTTON
>> tristate "Retu Power button Driver"
>> depends on MFD_RETU
>> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
>> index 19c7603..1f135af 100644
>> --- a/drivers/input/misc/Makefile
>> +++ b/drivers/input/misc/Makefile
>> @@ -53,6 +53,7 @@ obj-$(CONFIG_INPUT_PMIC8XXX_PWRKEY) += pmic8xxx-pwrkey.o
>> obj-$(CONFIG_INPUT_POWERMATE) += powermate.o
>> obj-$(CONFIG_INPUT_PWM_BEEPER) += pwm-beeper.o
>> obj-$(CONFIG_INPUT_RB532_BUTTON) += rb532_button.o
>> +obj-$(CONFIG_INPUT_REGULATOR_HAPTIC) += regulator-haptic.o
>> obj-$(CONFIG_INPUT_RETU_PWRBUTTON) += retu-pwrbutton.o
>> obj-$(CONFIG_INPUT_GPIO_ROTARY_ENCODER) += rotary_encoder.o
>> obj-$(CONFIG_INPUT_SGI_BTNS) += sgi_btns.o
>> diff --git a/drivers/input/misc/regulator-haptic.c b/drivers/input/misc/regulator-haptic.c
>> new file mode 100644
>> index 0000000..2fa94bc
>> --- /dev/null
>> +++ b/drivers/input/misc/regulator-haptic.c
>> @@ -0,0 +1,259 @@
>> +/*
>> + * Regulator haptic driver
>> + *
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * Author: Jaewon Kim <jaewon02.kim@samsung.com>
>> + * Author: Hyunhee Kim <hyunhee.kim@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/input.h>
>> +#include <linux/input/regulator-haptic.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +#include <linux/slab.h>
>> +
>> +#define MAX_MAGNITUDE_SHIFT 16
>> +
>> +struct regulator_haptic {
>> + struct device *dev;
>> + struct input_dev *input_dev;
>> + struct regulator *regulator;
>> +
>> + struct work_struct work;
>> + struct mutex mutex;
>> +
>> + bool enabled;
>> + bool suspend_state;
>> + unsigned int max_volt;
>> + unsigned int min_volt;
>> + unsigned int intensity;
>> + unsigned int magnitude;
>> +};
>> +
>> +static void regulator_haptic_enable(struct regulator_haptic *haptic, bool state)
>> +{
>> + int error;
>> +
>> + if (haptic->enabled == state)
>> + return;
>> +
>> + if (state)
>> + error = regulator_enable(haptic->regulator);
>> + else
>> + error = regulator_disable(haptic->regulator);
>> + if (error) {
> Hmm, maybe:
>
> error = state ? regulator_enable(haptic->regulator) :
> regulator_disable(haptic->regulator);
> if (error)
> ...
>
Okay, i will change it.
>> + dev_err(haptic->dev, "cannot enable regulator\n");
>> + return;
>> + }
>> +
>> + haptic->enabled = state;
>> +}
>> +
>> +static void regulator_haptic_work(struct work_struct *work)
>> +{
>> + struct regulator_haptic *haptic = container_of(work,
>> + struct regulator_haptic, work);
>> + int error;
>> +
>> + if (haptic->suspend_state)
>> + return;
>> +
> Why is this check outside of mutex?
I will include this in next version.
>
>> + mutex_lock(&haptic->mutex);
>> +
>> + error = regulator_set_voltage(haptic->regulator,
>> + haptic->intensity + haptic->min_volt, haptic->max_volt);
>> + if (error) {
>> + dev_err(haptic->dev, "cannot set regulator voltage\n");
>> + goto err;
>> + }
>> +
>> + if (haptic->magnitude)
>> + regulator_haptic_enable(haptic, true);
>> + else
>> + regulator_haptic_enable(haptic, false);
>> +
>> +err:
>> + mutex_unlock(&haptic->mutex);
>> +}
>> +
>> +static int regulator_haptic_play_effect(struct input_dev *input, void *data,
>> + struct ff_effect *effect)
>> +{
>> + struct regulator_haptic *haptic = input_get_drvdata(input);
>> + u64 volt_mag_multi;
>> +
>> + haptic->magnitude = effect->u.rumble.strong_magnitude;
>> + if (!haptic->magnitude)
>> + haptic->magnitude = effect->u.rumble.weak_magnitude;
>> +
>> +
>> + volt_mag_multi = (u64)(haptic->max_volt - haptic->min_volt) *
>> + haptic->magnitude;
>> + haptic->intensity = (unsigned int)(volt_mag_multi >>
>> + MAX_MAGNITUDE_SHIFT);
>> +
>> + schedule_work(&haptic->work);
>> +
>> + return 0;
>> +}
>> +
>> +static void regulator_haptic_close(struct input_dev *input)
>> +{
>> + struct regulator_haptic *haptic = input_get_drvdata(input);
>> +
>> + cancel_work_sync(&haptic->work);
>> + regulator_haptic_enable(haptic, false);
>> +}
>> +
>> +#ifdef CONFIG_OF
>> +static int regulator_haptic_parse_dt(struct regulator_haptic *haptic)
>> +{
>> + struct device_node *node = haptic->dev->of_node;
>> + int error;
>> +
>> + error = of_property_read_u32(node, "max-microvolt", &haptic->max_volt);
>> + if (error) {
>> + dev_err(haptic->dev, "cannot parse max-microvolt\n");
>> + return error;
>> + }
>> +
>> + error = of_property_read_u32(node, "min-microvolt", &haptic->min_volt);
>> + if (error) {
>> + dev_err(haptic->dev, "cannot parse min-microvolt\n");
>> + return error;
>> + }
>> +
>> + return 0;
>> +}
>> +#else
>> +static int regulator_haptic_parse_dt(struct regulator_haptic *haptic)
>> +{
>> + return 0;
> This does not seem right. If you do not have platform data and CONFOG_OF
> is disabled you can't continue initialization.
>
>> +}
>> +#endif /* CONFIG_OF */
>> +
>> +static int regulator_haptic_probe(struct platform_device *pdev)
>> +{
>> + struct regulator_haptic *haptic;
>> + struct regulator_haptic_data *data = dev_get_platdata(&pdev->dev);
>> + struct input_dev *input_dev;
>> + int error;
>> +
>> + haptic = devm_kzalloc(&pdev->dev, sizeof(*haptic), GFP_KERNEL);
>> + if (!haptic)
>> + return -ENOMEM;
>> +
>> + haptic->dev = &pdev->dev;
>> + haptic->enabled = false;
>> + haptic->suspend_state = false;
>> + mutex_init(&haptic->mutex);
>> + INIT_WORK(&haptic->work, regulator_haptic_work);
>> +
>> + if (!data) {
>> + if (pdev->dev.of_node) {
> I'd rather we moved check for presence of of_node into
> regulator_haptic_parse_dt().
Okay. I will move it to haptic_parse_dt() and remove CONFIG_OF.
>
>
>> + error = regulator_haptic_parse_dt(haptic);
>> + if (error) {
>> + dev_err(&pdev->dev, "failed to parse device tree\n");
>> + return error;
>> + }
>> + } else {
>> + dev_err(&pdev->dev, "failed to get platdata\n");
>> + return -EINVAL;
>> + }
>> + } else {
>> + haptic->regulator = data->regulator;
> What is the point of having regulator in platform data and doing
> assignment here if you are going to clobber it a few lines down?
You are right, this is unnecessary process.
I will remove it in next version.
>
>> + haptic->max_volt = data->max_volt;
>> + haptic->min_volt = data->min_volt;
>> + }
>> +
>> + haptic->regulator = devm_regulator_get_exclusive(&pdev->dev, "haptic");
>> + if (IS_ERR(haptic->regulator)) {
>> + dev_err(&pdev->dev, "failed to get regulator\n");
>> + return PTR_ERR(haptic->regulator);
>> + }
>> +
>> + input_dev = devm_input_allocate_device(&pdev->dev);
>> + if (!input_dev)
>> + return -ENOMEM;
> Nit: extra space between return and error value.
>
>> +
>> + haptic->input_dev = input_dev;
>> + haptic->input_dev->name = "regulator-haptic";
>> + haptic->input_dev->dev.parent = &pdev->dev;
>> + haptic->input_dev->close = regulator_haptic_close;
>> + input_set_drvdata(haptic->input_dev, haptic);
>> + input_set_capability(haptic->input_dev, EV_FF, FF_RUMBLE);
>> +
>> + error = input_ff_create_memless(input_dev, NULL,
>> + regulator_haptic_play_effect);
>> + if (error) {
>> + dev_err(&pdev->dev, "failed to create force-feedback\n");
>> + return error;
>> + }
>> +
>> + error = input_register_device(haptic->input_dev);
>> + if (error) {
>> + dev_err(&pdev->dev, "failed to register input device\n");
>> + return error;
>> + }
>> +
>> + platform_set_drvdata(pdev, haptic);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused regulator_haptic_suspend(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
>> +
>> + mutex_lock(&haptic->mutex);
>> + if (haptic->enabled) {
>> + regulator_haptic_enable(haptic, false);
>> + haptic->suspend_state = true;
> Why do we only set suspend_state if an effect was playing? I think we
> should always indicate that the device is suspended so that we do not
> try to start playing another effect - while it is true that normally
> effects are played by request from userspace which should be frozen by
> now, it is theoretically possible to trigger an effect from kernel as
> well.
This variable name seems to make you confuse.
I used this variable to restore the old state.
When kernel is entering suspend state while the motor is vibrating,
I store vibrating state for vibrate again after escape suspend state.
I will change variable name to "suspend_restore".
And prevent to start playing effect when kernel entering suspend state.
>
>> + }
>> + mutex_unlock(&haptic->mutex);
>> +
>> + return 0;
>> +}
>> +
>> +static int __maybe_unused regulator_haptic_resume(struct device *dev)
>> +{
>> + struct platform_device *pdev = to_platform_device(dev);
>> + struct regulator_haptic *haptic = platform_get_drvdata(pdev);
>> +
>> + if (haptic->suspend_state) {
> I think you should be gating enabling regulator not on suspend_state but
> rather non-zero magnitude. And also lock the mutex to make absolutely
> sure you are not racing with work item.
>
>> + regulator_haptic_enable(haptic, true);
>> + haptic->suspend_state = false;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static SIMPLE_DEV_PM_OPS(regulator_haptic_pm_ops,
>> + regulator_haptic_suspend, regulator_haptic_resume);
>> +
>> +static struct of_device_id regulator_haptic_dt_match[] = {
>> + { .compatible = "regulator-haptic" },
>> + { /* sentinel */ },
>> +};
>> +
>> +static struct platform_driver regulator_haptic_driver = {
>> + .probe = regulator_haptic_probe,
>> + .driver = {
>> + .name = "regulator-haptic",
>> + .of_match_table = regulator_haptic_dt_match,
>> + .pm = ®ulator_haptic_pm_ops,
>> + },
>> +};
>> +module_platform_driver(regulator_haptic_driver);
>> +
>> +MODULE_AUTHOR("Jaewon Kim <jaewon02.kim@samsung.com>");
>> +MODULE_AUTHOR("Hyunhee Kim <hyunhee.kim@samsung.com>");
>> +MODULE_DESCRIPTION("Regulator haptic driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/input/regulator-haptic.h b/include/linux/input/regulator-haptic.h
>> new file mode 100644
>> index 0000000..05ae038
>> --- /dev/null
>> +++ b/include/linux/input/regulator-haptic.h
> Hmm, move it to include/linux/platform-data/ maybe?
Okay, i will move it.
>
>> @@ -0,0 +1,31 @@
>> +/*
>> + * Regulator Haptic Platform Data
>> + *
>> + * Copyright (c) 2014 Samsung Electronics Co., Ltd.
>> + * Author: Jaewon Kim <jaewon02.kim@samsung.com>
>> + * Author: Hyunhee Kim <hyunhee.kim@samsung.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#ifndef _REGULATOR_HAPTIC_H
>> +#define _REGULATOR_HAPTIC_H
>> +
>> +/*
>> + * struct regulator_haptic_data - Platform device data
>> + *
>> + * @regulator: Power supply to the haptic motor
>> + * @max_volt: maximum voltage value supplied to the haptic motor.
>> + * <The unit of the voltage is a micro>
>> + * @min_volt: minimum voltage value supplied to the haptic motor.
>> + * <The unit of the voltage is a micro>
>> + */
>> +struct regulator_haptic_data {
>> + struct regulator *regulator;
>> + unsigned int max_volt;
>> + unsigned int min_volt;
>> +};
>> +
>> +#endif /* _REGULATOR_HAPTIC_H */
>> --
>> 1.7.9.5
>>
> Thanks.
>
Thanks
Jaewon Kim
^ permalink raw reply
* [PATCH 3/3] HID: logitech-hidpp: avoid unintended fall-through
From: Peter Wu @ 2014-12-16 0:50 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Nestor Lopez Casado
Cc: linux-input, linux-kernel
In-Reply-To: <1418691016-30681-1-git-send-email-peter@lekensteyn.nl>
Add a return to avoid a fall-through. Introduced in commit
57ac86cf52e903d9e3e0f12b34c814cce6b65550 ("HID: logitech-hidpp: add
support of the first Logitech Wireless Touchpad").
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
drivers/hid/hid-logitech-hidpp.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2315358..09eee17 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -811,6 +811,7 @@ static int wtp_raw_event(struct hid_device *hdev, u8 *data, int size)
input_event(wd->input, EV_KEY, BTN_RIGHT,
!!(data[1] & 0x02));
input_sync(wd->input);
+ return 0;
} else {
if (size < 21)
return 1;
--
2.1.3
^ permalink raw reply related
* [PATCH 2/3] HID: logitech-{dj,hidpp}: check report length
From: Peter Wu @ 2014-12-16 0:50 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Nestor Lopez Casado
Cc: linux-input, linux-kernel
In-Reply-To: <1418691016-30681-1-git-send-email-peter@lekensteyn.nl>
Malicious USB devices can send bogus reports smaller than the expected
buffer size. Ensure that the length is valid to avoid reading out of
bounds.
For the old WTP, I do not have a HID descriptor so just check for the
minimum length in hidpp_raw_event (this can be changed to an inequality
later).
Signed-off-by: Peter Wu <peter@lekensteyn.nl>
---
Hi,
If you know that the WTP report (ID 2) has a length of 2, then you can change
"<" to "!=" and remove the paragraph from the commit message.
Kind regards,
Peter
---
drivers/hid/hid-logitech-dj.c | 16 +++++++++++++++-
drivers/hid/hid-logitech-hidpp.c | 12 +++++++++---
2 files changed, 24 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index c917ab6..5bc6d80 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -962,10 +962,24 @@ static int logi_dj_raw_event(struct hid_device *hdev,
switch (data[0]) {
case REPORT_ID_DJ_SHORT:
+ if (size != DJREPORT_SHORT_LENGTH) {
+ dev_err(&hdev->dev, "DJ report of bad size (%d)", size);
+ return false;
+ }
return logi_dj_dj_event(hdev, report, data, size);
case REPORT_ID_HIDPP_SHORT:
- /* intentional fallthrough */
+ if (size != HIDPP_REPORT_SHORT_LENGTH) {
+ dev_err(&hdev->dev,
+ "Short HID++ report of bad size (%d)", size);
+ return false;
+ }
+ return logi_dj_hidpp_event(hdev, report, data, size);
case REPORT_ID_HIDPP_LONG:
+ if (size != HIDPP_REPORT_LONG_LENGTH) {
+ dev_err(&hdev->dev,
+ "Long HID++ report of bad size (%d)", size);
+ return false;
+ }
return logi_dj_hidpp_event(hdev, report, data, size);
}
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ae23dec..2315358 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -992,11 +992,17 @@ static int hidpp_raw_event(struct hid_device *hdev, struct hid_report *report,
return 1;
}
return hidpp_raw_hidpp_event(hidpp, data, size);
+ case 0x02:
+ if (size < 2) {
+ hid_err(hdev, "Received HID report of bad size (%d)",
+ size);
+ return 1;
+ }
+ if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
+ return wtp_raw_event(hdev, data, size);
+ return 1;
}
- if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP)
- return wtp_raw_event(hdev, data, size);
-
return 0;
}
--
2.1.3
^ permalink raw reply related
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