Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1234554

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

Show all headers | View raw

On 09/28/2015 03:10 AM, David Herrmann wrote:
> Hi
>
> On Mon, Sep 28, 2015 at 2:23 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> [...]
>>>>   }
>>>>
>>>>   static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> [snip]
>>>
>>>> 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.

>>>>          /* 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH] HID: hid-input: allow input_configured callback return errors Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-09-26 01:20 +0200
  Re: [PATCH] HID: hid-input: allow input_configured callback return errors David Herrmann <dh.herrmann@gmail.com> - 2015-09-26 17:20 +0200
    Re: [PATCH] HID: hid-input: allow input_configured callback return  errors Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-09-28 02:30 +0200
      Re: [PATCH] HID: hid-input: allow input_configured callback return errors David Herrmann <dh.herrmann@gmail.com> - 2015-09-28 12:20 +0200
        Re: [PATCH] HID: hid-input: allow input_configured callback return  errors Andrew Duggan <aduggan@synaptics.com> - 2015-09-29 00:40 +0200
  Re: [PATCH] HID: hid-input: allow input_configured callback return  errors Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com> - 2015-09-26 19:50 +0200
  [PATCH v2] HID: hid-input: allow input_configured callback return  errors Dmitry Torokhov <dmitry.torokhov@gmail.com> - 2015-09-30 01:00 +0200
    Re: [PATCH v2] HID: hid-input: allow input_configured callback return  errors Jiri Kosina <jikos@kernel.org> - 2015-09-30 10:20 +0200

csiph-web