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

Andrew Duggan
2015-09-28T22:32:20Z

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