* [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown
@ 2014-08-22 20:16 Benjamin Tissoires
2014-08-22 20:16 ` [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device Benjamin Tissoires
2014-08-25 7:50 ` [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown Jiri Kosina
0 siblings, 2 replies; 7+ messages in thread
From: Benjamin Tissoires @ 2014-08-22 20:16 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel
Commit "HID: logitech: perform bounds checking on device_id early
enough" unfortunately leaks some errors to dmesg which are not real
ones:
- if the report is not a DJ one, then there is not point in checking
the device_id
- the receiver (index 0) can also receive some notifications which
can be safely ignored given the current implementation
Move out the test regarding the report_id and also discards
printing errors when the receiver got notified.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-dj.c | 43 +++++++++++++++++++++++++------------------
drivers/hid/hid-logitech-dj.h | 1 +
2 files changed, 26 insertions(+), 18 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index b7ba829..9bf8637 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -656,7 +656,6 @@ static int logi_dj_raw_event(struct hid_device *hdev,
struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
struct dj_report *dj_report = (struct dj_report *) data;
unsigned long flags;
- bool report_processed = false;
dbg_hid("%s, size:%d\n", __func__, size);
@@ -683,34 +682,42 @@ static int logi_dj_raw_event(struct hid_device *hdev,
* device (via hid_input_report() ) and return 1 so hid-core does not do
* anything else with it.
*/
+
+ /* case 1) */
+ if (data[0] != REPORT_ID_DJ_SHORT)
+ return false;
+
if ((dj_report->device_index < DJ_DEVICE_INDEX_MIN) ||
(dj_report->device_index > DJ_DEVICE_INDEX_MAX)) {
- dev_err(&hdev->dev, "%s: invalid device index:%d\n",
+ /*
+ * Device index is wrong, bail out.
+ * This driver can ignore safely the receiver notifications,
+ * so ignore those reports too.
+ */
+ if (dj_report->device_index != DJ_RECEIVER_INDEX)
+ dev_err(&hdev->dev, "%s: invalid device index:%d\n",
__func__, dj_report->device_index);
return false;
}
spin_lock_irqsave(&djrcv_dev->lock, flags);
- if (dj_report->report_id == REPORT_ID_DJ_SHORT) {
- switch (dj_report->report_type) {
- case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
- case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
- logi_dj_recv_queue_notification(djrcv_dev, dj_report);
- break;
- case REPORT_TYPE_NOTIF_CONNECTION_STATUS:
- if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] ==
- STATUS_LINKLOSS) {
- logi_dj_recv_forward_null_report(djrcv_dev, dj_report);
- }
- break;
- default:
- logi_dj_recv_forward_report(djrcv_dev, dj_report);
+ switch (dj_report->report_type) {
+ case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
+ case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
+ logi_dj_recv_queue_notification(djrcv_dev, dj_report);
+ break;
+ case REPORT_TYPE_NOTIF_CONNECTION_STATUS:
+ if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] ==
+ STATUS_LINKLOSS) {
+ logi_dj_recv_forward_null_report(djrcv_dev, dj_report);
}
- report_processed = true;
+ break;
+ default:
+ logi_dj_recv_forward_report(djrcv_dev, dj_report);
}
spin_unlock_irqrestore(&djrcv_dev->lock, flags);
- return report_processed;
+ return true;
}
static int logi_dj_probe(struct hid_device *hdev,
diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
index 4a40003..daeb0aa 100644
--- a/drivers/hid/hid-logitech-dj.h
+++ b/drivers/hid/hid-logitech-dj.h
@@ -27,6 +27,7 @@
#define DJ_MAX_PAIRED_DEVICES 6
#define DJ_MAX_NUMBER_NOTIFICATIONS 8
+#define DJ_RECEIVER_INDEX 0
#define DJ_DEVICE_INDEX_MIN 1
#define DJ_DEVICE_INDEX_MAX 6
--
2.1.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device
2014-08-22 20:16 [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown Benjamin Tissoires
@ 2014-08-22 20:16 ` Benjamin Tissoires
2014-08-25 7:51 ` Jiri Kosina
2014-08-27 21:06 ` Jiri Kosina
2014-08-25 7:50 ` [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown Jiri Kosina
1 sibling, 2 replies; 7+ messages in thread
From: Benjamin Tissoires @ 2014-08-22 20:16 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, linux-kernel
We can do once the test of the validity of the dj_device, which removes
some duplicated code in various functions.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-dj.c | 35 +++++++++++------------------------
1 file changed, 11 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 9bf8637..71f5692 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -385,18 +385,6 @@ static void logi_dj_recv_forward_null_report(struct dj_receiver_dev *djrcv_dev,
djdev = djrcv_dev->paired_dj_devices[dj_report->device_index];
- if (!djdev) {
- dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
- " is NULL, index %d\n", dj_report->device_index);
- kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
-
- if (schedule_work(&djrcv_dev->work) == 0) {
- dbg_hid("%s: did not schedule the work item, was already "
- "queued\n", __func__);
- }
- return;
- }
-
memset(reportbuffer, 0, sizeof(reportbuffer));
for (i = 0; i < NUMBER_OF_HID_REPORTS; i++) {
@@ -421,18 +409,6 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
dj_device = djrcv_dev->paired_dj_devices[dj_report->device_index];
- if (dj_device == NULL) {
- dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
- " is NULL, index %d\n", dj_report->device_index);
- kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
-
- if (schedule_work(&djrcv_dev->work) == 0) {
- dbg_hid("%s: did not schedule the work item, was already "
- "queued\n", __func__);
- }
- return;
- }
-
if ((dj_report->report_type > ARRAY_SIZE(hid_reportid_size_map) - 1) ||
(hid_reportid_size_map[dj_report->report_type] == 0)) {
dbg_hid("invalid report type:%x\n", dj_report->report_type);
@@ -701,8 +677,17 @@ static int logi_dj_raw_event(struct hid_device *hdev,
}
spin_lock_irqsave(&djrcv_dev->lock, flags);
+
+ if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
+ /* received an event for an unknown device, bail out */
+ logi_dj_recv_queue_notification(djrcv_dev, dj_report);
+ goto out;
+ }
+
switch (dj_report->report_type) {
case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
+ /* pairing notifications are handled above the switch */
+ break;
case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
logi_dj_recv_queue_notification(djrcv_dev, dj_report);
break;
@@ -715,6 +700,8 @@ static int logi_dj_raw_event(struct hid_device *hdev,
default:
logi_dj_recv_forward_report(djrcv_dev, dj_report);
}
+
+out:
spin_unlock_irqrestore(&djrcv_dev->lock, flags);
return true;
--
2.1.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown
2014-08-22 20:16 [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown Benjamin Tissoires
2014-08-22 20:16 ` [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device Benjamin Tissoires
@ 2014-08-25 7:50 ` Jiri Kosina
2014-08-25 7:55 ` Markus Trippelsdorf
1 sibling, 1 reply; 7+ messages in thread
From: Jiri Kosina @ 2014-08-25 7:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, linux-kernel, Markus Trippelsdorf, Ben Hawkes
On Fri, 22 Aug 2014, Benjamin Tissoires wrote:
> Commit "HID: logitech: perform bounds checking on device_id early
> enough" unfortunately leaks some errors to dmesg which are not real
> ones:
> - if the report is not a DJ one, then there is not point in checking
> the device_id
> - the receiver (index 0) can also receive some notifications which
> can be safely ignored given the current implementation
>
> Move out the test regarding the report_id and also discards
> printing errors when the receiver got notified.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I have now queued this one for 3.17. Adding Markus to CC -- Markus, this
should make the spurious error messages you have reported go away.
Thanks.
> ---
> drivers/hid/hid-logitech-dj.c | 43 +++++++++++++++++++++++++------------------
> drivers/hid/hid-logitech-dj.h | 1 +
> 2 files changed, 26 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index b7ba829..9bf8637 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -656,7 +656,6 @@ static int logi_dj_raw_event(struct hid_device *hdev,
> struct dj_receiver_dev *djrcv_dev = hid_get_drvdata(hdev);
> struct dj_report *dj_report = (struct dj_report *) data;
> unsigned long flags;
> - bool report_processed = false;
>
> dbg_hid("%s, size:%d\n", __func__, size);
>
> @@ -683,34 +682,42 @@ static int logi_dj_raw_event(struct hid_device *hdev,
> * device (via hid_input_report() ) and return 1 so hid-core does not do
> * anything else with it.
> */
> +
> + /* case 1) */
> + if (data[0] != REPORT_ID_DJ_SHORT)
> + return false;
> +
> if ((dj_report->device_index < DJ_DEVICE_INDEX_MIN) ||
> (dj_report->device_index > DJ_DEVICE_INDEX_MAX)) {
> - dev_err(&hdev->dev, "%s: invalid device index:%d\n",
> + /*
> + * Device index is wrong, bail out.
> + * This driver can ignore safely the receiver notifications,
> + * so ignore those reports too.
> + */
> + if (dj_report->device_index != DJ_RECEIVER_INDEX)
> + dev_err(&hdev->dev, "%s: invalid device index:%d\n",
> __func__, dj_report->device_index);
> return false;
> }
>
> spin_lock_irqsave(&djrcv_dev->lock, flags);
> - if (dj_report->report_id == REPORT_ID_DJ_SHORT) {
> - switch (dj_report->report_type) {
> - case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
> - case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
> - logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> - break;
> - case REPORT_TYPE_NOTIF_CONNECTION_STATUS:
> - if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> - STATUS_LINKLOSS) {
> - logi_dj_recv_forward_null_report(djrcv_dev, dj_report);
> - }
> - break;
> - default:
> - logi_dj_recv_forward_report(djrcv_dev, dj_report);
> + switch (dj_report->report_type) {
> + case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
> + case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
> + logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> + break;
> + case REPORT_TYPE_NOTIF_CONNECTION_STATUS:
> + if (dj_report->report_params[CONNECTION_STATUS_PARAM_STATUS] ==
> + STATUS_LINKLOSS) {
> + logi_dj_recv_forward_null_report(djrcv_dev, dj_report);
> }
> - report_processed = true;
> + break;
> + default:
> + logi_dj_recv_forward_report(djrcv_dev, dj_report);
> }
> spin_unlock_irqrestore(&djrcv_dev->lock, flags);
>
> - return report_processed;
> + return true;
> }
>
> static int logi_dj_probe(struct hid_device *hdev,
> diff --git a/drivers/hid/hid-logitech-dj.h b/drivers/hid/hid-logitech-dj.h
> index 4a40003..daeb0aa 100644
> --- a/drivers/hid/hid-logitech-dj.h
> +++ b/drivers/hid/hid-logitech-dj.h
> @@ -27,6 +27,7 @@
>
> #define DJ_MAX_PAIRED_DEVICES 6
> #define DJ_MAX_NUMBER_NOTIFICATIONS 8
> +#define DJ_RECEIVER_INDEX 0
> #define DJ_DEVICE_INDEX_MIN 1
> #define DJ_DEVICE_INDEX_MAX 6
>
> --
> 2.1.0
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device
2014-08-22 20:16 ` [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device Benjamin Tissoires
@ 2014-08-25 7:51 ` Jiri Kosina
2014-08-27 21:06 ` Jiri Kosina
1 sibling, 0 replies; 7+ messages in thread
From: Jiri Kosina @ 2014-08-25 7:51 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input, linux-kernel
On Fri, 22 Aug 2014, Benjamin Tissoires wrote:
> We can do once the test of the validity of the dj_device, which removes
> some duplicated code in various functions.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
I will queue this cleanup for next merge window. Thanks.
> ---
> drivers/hid/hid-logitech-dj.c | 35 +++++++++++------------------------
> 1 file changed, 11 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 9bf8637..71f5692 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -385,18 +385,6 @@ static void logi_dj_recv_forward_null_report(struct dj_receiver_dev *djrcv_dev,
>
> djdev = djrcv_dev->paired_dj_devices[dj_report->device_index];
>
> - if (!djdev) {
> - dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
> - " is NULL, index %d\n", dj_report->device_index);
> - kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
> -
> - if (schedule_work(&djrcv_dev->work) == 0) {
> - dbg_hid("%s: did not schedule the work item, was already "
> - "queued\n", __func__);
> - }
> - return;
> - }
> -
> memset(reportbuffer, 0, sizeof(reportbuffer));
>
> for (i = 0; i < NUMBER_OF_HID_REPORTS; i++) {
> @@ -421,18 +409,6 @@ static void logi_dj_recv_forward_report(struct dj_receiver_dev *djrcv_dev,
>
> dj_device = djrcv_dev->paired_dj_devices[dj_report->device_index];
>
> - if (dj_device == NULL) {
> - dbg_hid("djrcv_dev->paired_dj_devices[dj_report->device_index]"
> - " is NULL, index %d\n", dj_report->device_index);
> - kfifo_in(&djrcv_dev->notif_fifo, dj_report, sizeof(struct dj_report));
> -
> - if (schedule_work(&djrcv_dev->work) == 0) {
> - dbg_hid("%s: did not schedule the work item, was already "
> - "queued\n", __func__);
> - }
> - return;
> - }
> -
> if ((dj_report->report_type > ARRAY_SIZE(hid_reportid_size_map) - 1) ||
> (hid_reportid_size_map[dj_report->report_type] == 0)) {
> dbg_hid("invalid report type:%x\n", dj_report->report_type);
> @@ -701,8 +677,17 @@ static int logi_dj_raw_event(struct hid_device *hdev,
> }
>
> spin_lock_irqsave(&djrcv_dev->lock, flags);
> +
> + if (!djrcv_dev->paired_dj_devices[dj_report->device_index]) {
> + /* received an event for an unknown device, bail out */
> + logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> + goto out;
> + }
> +
> switch (dj_report->report_type) {
> case REPORT_TYPE_NOTIF_DEVICE_PAIRED:
> + /* pairing notifications are handled above the switch */
> + break;
> case REPORT_TYPE_NOTIF_DEVICE_UNPAIRED:
> logi_dj_recv_queue_notification(djrcv_dev, dj_report);
> break;
> @@ -715,6 +700,8 @@ static int logi_dj_raw_event(struct hid_device *hdev,
> default:
> logi_dj_recv_forward_report(djrcv_dev, dj_report);
> }
> +
> +out:
> spin_unlock_irqrestore(&djrcv_dev->lock, flags);
>
> return true;
> --
> 2.1.0
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown
2014-08-25 7:50 ` [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown Jiri Kosina
@ 2014-08-25 7:55 ` Markus Trippelsdorf
2014-08-25 8:00 ` Jiri Kosina
0 siblings, 1 reply; 7+ messages in thread
From: Markus Trippelsdorf @ 2014-08-25 7:55 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Benjamin Tissoires, linux-input, linux-kernel, Ben Hawkes
On 2014.08.25 at 02:50 -0500, Jiri Kosina wrote:
> On Fri, 22 Aug 2014, Benjamin Tissoires wrote:
>
> > Commit "HID: logitech: perform bounds checking on device_id early
> > enough" unfortunately leaks some errors to dmesg which are not real
> > ones:
> > - if the report is not a DJ one, then there is not point in checking
> > the device_id
> > - the receiver (index 0) can also receive some notifications which
> > can be safely ignored given the current implementation
> >
> > Move out the test regarding the report_id and also discards
> > printing errors when the receiver got notified.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>
> I have now queued this one for 3.17. Adding Markus to CC -- Markus, this
> should make the spurious error messages you have reported go away.
Indeed it does. Feel free to add:
Reported-and-tested-by: Markus Trippelsdorf <markus@trippelsdorf.de>
--
Markus
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown
2014-08-25 7:55 ` Markus Trippelsdorf
@ 2014-08-25 8:00 ` Jiri Kosina
0 siblings, 0 replies; 7+ messages in thread
From: Jiri Kosina @ 2014-08-25 8:00 UTC (permalink / raw)
To: Markus Trippelsdorf
Cc: Benjamin Tissoires, linux-input, linux-kernel, Ben Hawkes
On Mon, 25 Aug 2014, Markus Trippelsdorf wrote:
> > > Commit "HID: logitech: perform bounds checking on device_id early
> > > enough" unfortunately leaks some errors to dmesg which are not real
> > > ones:
> > > - if the report is not a DJ one, then there is not point in checking
> > > the device_id
> > > - the receiver (index 0) can also receive some notifications which
> > > can be safely ignored given the current implementation
> > >
> > > Move out the test regarding the report_id and also discards
> > > printing errors when the receiver got notified.
> > >
> > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > I have now queued this one for 3.17. Adding Markus to CC -- Markus, this
> > should make the spurious error messages you have reported go away.
>
> Indeed it does. Feel free to add:
>
> Reported-and-tested-by: Markus Trippelsdorf <markus@trippelsdorf.de>
Excellent, thanks. Will be pushing it to Linus this week.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device
2014-08-22 20:16 ` [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device Benjamin Tissoires
2014-08-25 7:51 ` Jiri Kosina
@ 2014-08-27 21:06 ` Jiri Kosina
1 sibling, 0 replies; 7+ messages in thread
From: Jiri Kosina @ 2014-08-27 21:06 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input, linux-kernel
On Fri, 22 Aug 2014, Benjamin Tissoires wrote:
> We can do once the test of the validity of the dj_device, which removes
> some duplicated code in various functions.
Queued for 3.18, thanks Benjamin.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2014-08-27 21:06 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-22 20:16 [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown Benjamin Tissoires
2014-08-22 20:16 ` [PATCH 2/2] HID: logitech-dj: break out testing of validity of dj_device Benjamin Tissoires
2014-08-25 7:51 ` Jiri Kosina
2014-08-27 21:06 ` Jiri Kosina
2014-08-25 7:50 ` [PATCH 1/2] HID: logitech-dj: prevent false errors to be shown Jiri Kosina
2014-08-25 7:55 ` Markus Trippelsdorf
2014-08-25 8:00 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).