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.tissoi...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
