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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi David,

On Sat, Sep 26, 2015 at 05:16:12PM +0200, David Herrmann wrote:
> Hi
> 
> On Sat, Sep 26, 2015 at 1:14 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> > When configuring input device via input_configured callback we may
> > encounter errors (for example input_mt_init_slots() may fail). Instead
> > of continuing with half-initialized input device let's allow driver
> > indicate failures.
> >
> > Signed-off-by: Jaikumar Ganesh <jaikumarg@android.com>
> > Signed-off-by: Arve Hjønnevåg <arve@android.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/hid/hid-appleir.c        |  4 +++-
> >  drivers/hid/hid-elo.c            |  4 +++-
> >  drivers/hid/hid-input.c          | 10 ++++++----
> >  drivers/hid/hid-lenovo.c         |  4 +++-
> >  drivers/hid/hid-logitech-hidpp.c |  4 +++-
> >  drivers/hid/hid-magicmouse.c     |  8 ++++++--
> >  drivers/hid/hid-multitouch.c     | 19 ++++++++++++++-----
> >  drivers/hid/hid-ntrig.c          |  6 ++++--
> >  drivers/hid/hid-rmi.c            | 11 +++++++----
> >  drivers/hid/hid-sony.c           | 13 ++++++++++---
> >  drivers/hid/hid-uclogic.c        |  6 ++++--
> >  include/linux/hid.h              |  4 ++--
> >  12 files changed, 65 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index 53aeaf6..2ba6bf6 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -1510,8 +1510,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> >                                  * UGCI) cram a lot of unrelated inputs into the
> >                                  * same interface. */
> >                                 hidinput->report = report;
> > -                               if (drv->input_configured)
> > -                                       drv->input_configured(hid, hidinput);
> > +                               if (drv->input_configured &&
> > +                                   drv->input_configured(hid, hidinput))
> > +                                       goto out_cleanup;
> >                                 if (input_register_device(hidinput->input))
> >                                         goto out_cleanup;
> >                                 hidinput = NULL;
> > @@ -1532,8 +1533,9 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> >         }
> >
> >         if (hidinput) {
> > -               if (drv->input_configured)
> > -                       drv->input_configured(hid, hidinput);
> > +               if (drv->input_configured &&
> > +                   drv->input_configured(hid, hidinput))
> > +                       goto out_cleanup;
> >                 if (input_register_device(hidinput->input))
> >                         goto out_cleanup;
> >         }
> 
> [snip]
> 
> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > index 426b2f1..73d7d59 100644
> > --- a/drivers/hid/hid-multitouch.c
> > +++ b/drivers/hid/hid-multitouch.c
> > @@ -725,12 +725,13 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
> >                 mt_sync_frame(td, report->field[0]->hidinput->input);
> >  }
> >
> > -static void mt_touch_input_configured(struct hid_device *hdev,
> > +static int mt_touch_input_configured(struct hid_device *hdev,
> >                                         struct hid_input *hi)
> >  {
> >         struct mt_device *td = hid_get_drvdata(hdev);
> >         struct mt_class *cls = &td->mtclass;
> >         struct input_dev *input = hi->input;
> > +       int ret;
> >
> >         if (!td->maxcontacts)
> >                 td->maxcontacts = MT_DEFAULT_MAXCONTACT;
> > @@ -752,9 +753,12 @@ static void mt_touch_input_configured(struct hid_device *hdev,
> >         if (td->is_buttonpad)
> >                 __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
> >
> > -       input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
> > +       ret = input_mt_init_slots(input, td->maxcontacts, td->mt_flags);
> > +       if (ret)
> > +               return ret;
> >
> >         td->mt_flags = 0;
> > +       return 0;
> >  }
> >
> >  static int mt_input_mapping(struct hid_device *hdev, struct hid_input *hi,
> > @@ -930,15 +934,19 @@ static void mt_post_parse(struct mt_device *td)
> >                 cls->quirks &= ~MT_QUIRK_CONTACT_CNT_ACCURATE;
> >  }
> >
> > -static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
> > +static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
> >  {
> >         struct mt_device *td = hid_get_drvdata(hdev);
> >         char *name;
> >         const char *suffix = NULL;
> >         struct hid_field *field = hi->report->field[0];
> > +       int ret = 0;
> 
> int ret;

Right.

> 
> >
> > -       if (hi->report->id == td->mt_report_id)
> > -               mt_touch_input_configured(hdev, hi);
> > +       if (hi->report->id == td->mt_report_id) {
> > +               ret = mt_touch_input_configured(hdev, hi);
> > +               if (ret)
> > +                       return ret;
> > +       }
> >
> >         /*
> >          * some egalax touchscreens have "application == HID_DG_TOUCHSCREEN"
> > @@ -989,6 +997,7 @@ static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
> >                         hi->input->name = name;
> >                 }
> >         }
> > +       return ret;
> 
> return 0;
> 
> And maybe add an empty line before the final return, just like you did
> with all the other callbacks.

OK.

> 
> >  }
> >
> >  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.

Andrew, can you double check?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html