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

[David Herrmann]

Hi

On Mon, Sep 28, 2015 at 2:23 AM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> 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;
>> >
>> >         /* 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.

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