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

[Andrew Duggan]

On 09/28/2015 03:10 AM, David Herrmann wrote:
> 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;

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/