Re: [PATCH] HID: hid-input: allow input_configured callback return errors
From: David Herrmann
Date: Sat Sep 26 2015 - 11:18:53 EST
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;
>
> - 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.
> }
>
> 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().
> }
>
> static int rmi_input_mapping(struct hid_device *hdev,
[snip]
Looks good to me (regardless of the nit-picks):
Reviewed-by: David Herrmann <dh.herrmann@xxxxxxxxx>
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/