Re: [PATCH] HID: hid-input: allow input_configured callback return errors
From: Dmitry Torokhov
Date: Sun Sep 27 2015 - 20:23:43 EST
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@xxxxxxxxx> 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@xxxxxxxxxxx>
> > Signed-off-by: Arve Hjønnevåg <arve@xxxxxxxxxxx>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@xxxxxxxxx>
> > ---
> > 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-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/