Messages in this thread | | | Subject | Re: [PATCH] HID: hid-input: allow input_configured callback return errors | From | Andrew Duggan <> | Date | Mon, 28 Sep 2015 15:22:50 -0700 |
| |
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
| |