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

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
[off-list ref] wrote:

quoted

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 <redacted>
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]

quoted

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.

quoted

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

quoted

 }

 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)

[snip]

quoted

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