Re: [PATCH] HID: hid-input: allow input_configured callback return errors

On 09/28/2015 03:10 AM, David Herrmann wrote:

Hi

On Mon, Sep 28, 2015 at 2:23 AM, Dmitry Torokhov
[off-list ref] wrote:
[...]

quoted

quoted

quoted

  }

  static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)

[snip]

quoted

diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 2c14812..33280f3 100644
--- a/drivers/hid/hid-rmi.c
+++ b/drivers/hid/hid-rmi.c

@@ -1173,7 +1173,7 @@ static int rmi_populate(struct hid_device *hdev)
         return 0;
  }

-static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
+static int rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
  {
         struct rmi_data *data = hid_get_drvdata(hdev);
         struct input_dev *input = hi->input;

@@ -1185,10 +1185,10 @@ static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
         hid_dbg(hdev, "Opening low level driver\n");
         ret = hid_hw_open(hdev);
         if (ret)
-               return;
+               return ret;

         if (!(data->device_flags & RMI_DEVICE))
-               return;
+               return -ENXIO;

We should return 0 here. Otherwise, this will break certain keyboards on composite USB devices which share a VID and PID with our touchpad. If the RMI_DEVICE flag is not set then hid-rmi will pass those reports onto hid-input for processing.

quoted

quoted

quoted

         /* Allow incoming hid reports */
         hid_device_io_start(hdev);

@@ -1228,7 +1228,9 @@ static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
         input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 0x0f, 0, 0);
         input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 0x0f, 0, 0);

-       input_mt_init_slots(input, data->max_fingers, INPUT_MT_POINTER);
+       ret = input_mt_init_slots(input, data->max_fingers, INPUT_MT_POINTER);
+       if (ret < 0)
+               goto exit;

         if (data->button_count) {
                 __set_bit(EV_KEY, input->evbit);

@@ -1244,6 +1246,7 @@ static void rmi_input_configured(struct hid_device *hdev, struct hid_input *hi)
  exit:
         hid_device_io_stop(hdev);
         hid_hw_close(hdev);
+       return ret;

rmi_probe() has an explicit comment that it *wants* hid_probe() to
continue on failure, to make sure hidraw is loaded. Not sure what to
make with that, but please either remove the comment in rmi_probe() or
make sure to always return 0 from rmi_input_configured().

I think that comment is erroneous since the fact that we could not
attach hidinput interface should not affect hidraw in any shape or form.

The comment might indeed be correct. If rmi_input_configured() failed,
probing is continued, but the device-internal state might be
different. rmi_probe() checks that, and explicitly continues device
probing in that case (if it didn't, the device would indeed be
rejected).

Sorry for the confusion. Your changes to rmi do look correct.

I will fix the comment, it does incorrectly imply that returning an error from rmi_probe() would disconnect hidraw. It is also hard to understand without the context of the previous version. If you look at the change (daebdd7) it was the call to hid_hw_stop() which disconnected hidraw and not the return code, if rmi_populate() can't find F11 on the device.

Otherwise, the changes to rmi look correct.

Andrew