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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux