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

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

[Dmitry Torokhov]

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-appleir.c b/drivers/hid/hid-appleir.c
index 0e6a42d..07cbc70 100644
--- a/drivers/hid/hid-appleir.c
+++ b/drivers/hid/hid-appleir.c
@@ -256,7 +256,7 @@ out:
 	return 0;
 }
 
-static void appleir_input_configured(struct hid_device *hid,
+static int appleir_input_configured(struct hid_device *hid,
 		struct hid_input *hidinput)
 {
 	struct input_dev *input_dev = hidinput->input;
@@ -275,6 +275,8 @@ static void appleir_input_configured(struct hid_device *hid,
 	for (i = 0; i < ARRAY_SIZE(appleir_key_table); i++)
 		set_bit(appleir->keymap[i], input_dev->keybit);
 	clear_bit(KEY_RESERVED, input_dev->keybit);
+
+	return 0;
 }
 
 static int appleir_input_mapping(struct hid_device *hid,
diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c
index 4e49462..aad8c16 100644
--- a/drivers/hid/hid-elo.c
+++ b/drivers/hid/hid-elo.c
@@ -37,7 +37,7 @@ static bool use_fw_quirk = true;
 module_param(use_fw_quirk, bool, S_IRUGO);
 MODULE_PARM_DESC(use_fw_quirk, "Do periodic pokes for broken M firmwares (default = true)");
 
-static void elo_input_configured(struct hid_device *hdev,
+static int elo_input_configured(struct hid_device *hdev,
 		struct hid_input *hidinput)
 {
 	struct input_dev *input = hidinput->input;
@@ -45,6 +45,8 @@ static void elo_input_configured(struct hid_device *hdev,
 	set_bit(BTN_TOUCH, input->keybit);
 	set_bit(ABS_PRESSURE, input->absbit);
 	input_set_abs_params(input, ABS_PRESSURE, 0, 256, 0, 0);
+
+	return 0;
 }
 
 static void elo_process_data(struct input_dev *input, const u8 *data, int size)
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;
 	}
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index e4bc6cb..8979f1f 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -848,7 +848,7 @@ static void lenovo_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 }
 
-static void lenovo_input_configured(struct hid_device *hdev,
+static int lenovo_input_configured(struct hid_device *hdev,
 		struct hid_input *hi)
 {
 	switch (hdev->product) {
@@ -863,6 +863,8 @@ static void lenovo_input_configured(struct hid_device *hdev,
 			}
 			break;
 	}
+
+	return 0;
 }
 
 
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 452e5d5..5fd9786 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1285,13 +1285,15 @@ static void hidpp_populate_input(struct hidpp_device *hidpp,
 		m560_populate_input(hidpp, input, origin_is_hid_core);
 }
 
-static void hidpp_input_configured(struct hid_device *hdev,
+static int hidpp_input_configured(struct hid_device *hdev,
 				struct hid_input *hidinput)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	struct input_dev *input = hidinput->input;
 
 	hidpp_populate_input(hidpp, input, true);
+
+	return 0;
 }
 
 static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 29a74c1..d6fa496 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -471,18 +471,22 @@ static int magicmouse_input_mapping(struct hid_device *hdev,
 	return 0;
 }
 
-static void magicmouse_input_configured(struct hid_device *hdev,
+static int magicmouse_input_configured(struct hid_device *hdev,
 		struct hid_input *hi)
 
 {
 	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
+	int ret;
 
-	int ret = magicmouse_setup_input(msc->input, hdev);
+	ret = magicmouse_setup_input(msc->input, hdev);
 	if (ret) {
 		hid_err(hdev, "magicmouse setup input failed (%d)\n", ret);
 		/* clean msc->input to notify probe() of the failure */
 		msc->input = NULL;
+		return ret;
 	}
+
+	return 0;
 }
 
 
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;
 
-	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;
 }
 
 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 600f207..756d1ef 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -859,14 +859,14 @@ not_claimed_input:
 	return 1;
 }
 
-static void ntrig_input_configured(struct hid_device *hid,
+static int ntrig_input_configured(struct hid_device *hid,
 		struct hid_input *hidinput)
 
 {
 	struct input_dev *input = hidinput->input;
 
 	if (hidinput->report->maxfield < 1)
-		return;
+		return 0;
 
 	switch (hidinput->report->field[0]->application) {
 	case HID_DG_PEN:
@@ -890,6 +890,8 @@ static void ntrig_input_configured(struct hid_device *hid,
 							"N-Trig MultiTouch";
 		break;
 	}
+
+	return 0;
 }
 
 static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
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;
 }
 
 static int rmi_input_mapping(struct hid_device *hdev,
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 661f94f..774cd22 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1360,20 +1360,27 @@ static int sony_register_touchpad(struct hid_input *hi, int touch_count,
 	return 0;
 }
 
-static void sony_input_configured(struct hid_device *hdev,
+static int sony_input_configured(struct hid_device *hdev,
 					struct hid_input *hidinput)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
+	int ret;
 
 	/*
 	 * The Dualshock 4 touchpad supports 2 touches and has a
 	 * resolution of 1920x942 (44.86 dots/mm).
 	 */
 	if (sc->quirks & DUALSHOCK4_CONTROLLER) {
-		if (sony_register_touchpad(hidinput, 2, 1920, 942) != 0)
+		ret = sony_register_touchpad(hidinput, 2, 1920, 942);
+		if (ret) {
 			hid_err(sc->hdev,
-				"Unable to initialize multi-touch slots\n");
+				"Unable to initialize multi-touch slots: %d\n",
+				ret);
+			return ret;
+		}
 	}
+
+	return 0;
 }
 
 /*
diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index b905d50..85ac435 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -731,7 +731,7 @@ static int uclogic_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	return 0;
 }
 
-static void uclogic_input_configured(struct hid_device *hdev,
+static int uclogic_input_configured(struct hid_device *hdev,
 		struct hid_input *hi)
 {
 	char *name;
@@ -741,7 +741,7 @@ static void uclogic_input_configured(struct hid_device *hdev,
 
 	/* no report associated (HID_QUIRK_MULTI_INPUT not set) */
 	if (!hi->report)
-		return;
+		return 0;
 
 	field = hi->report->field[0];
 
@@ -774,6 +774,8 @@ static void uclogic_input_configured(struct hid_device *hdev,
 			hi->input->name = name;
 		}
 	}
+
+	return 0;
 }
 
 /**
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f17980d..251a1d3 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -698,8 +698,8 @@ struct hid_driver {
 	int (*input_mapped)(struct hid_device *hdev,
 			struct hid_input *hidinput, struct hid_field *field,
 			struct hid_usage *usage, unsigned long **bit, int *max);
-	void (*input_configured)(struct hid_device *hdev,
-				 struct hid_input *hidinput);
+	int (*input_configured)(struct hid_device *hdev,
+				struct hid_input *hidinput);
 	void (*feature_mapping)(struct hid_device *hdev,
 			struct hid_field *field,
 			struct hid_usage *usage);
-- 
2.6.0.rc2.230.g3dd15c0


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

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

[David Herrmann]

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;

>
> -       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@gmail.com>

Thanks
David

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

[Dmitry Torokhov]

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

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

[David Herrmann]

Hi

On Mon, Sep 28, 2015 at 2:23 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
[...]
>>
>> >  }
>> >
>> >  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.

The comment might indeed be correct. If rmi_input_configured() failed,
probing is continued, but the device-internal state might be
different. rmi_probe() checks that, and explicitly continues device
probing in that case (if it didn't, the device would indeed be
rejected).

Sorry for the confusion. Your changes to rmi do look correct.

Thanks
David

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

[Andrew Duggan]

On 09/28/2015 03:10 AM, David Herrmann wrote:
> Hi
>
> On Mon, Sep 28, 2015 at 2:23 AM, Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> [...]
>>>>   }
>>>>
>>>>   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;

We should return 0 here. Otherwise, this will break certain keyboards on 
composite USB devices which share a VID and PID with our touchpad. If 
the RMI_DEVICE flag is not set then hid-rmi will pass those reports onto 
hid-input for processing.

>>>>          /* 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.
> The comment might indeed be correct. If rmi_input_configured() failed,
> probing is continued, but the device-internal state might be
> different. rmi_probe() checks that, and explicitly continues device
> probing in that case (if it didn't, the device would indeed be
> rejected).
>
> Sorry for the confusion. Your changes to rmi do look correct.

I will fix the comment, it does incorrectly imply that returning an 
error from rmi_probe() would disconnect hidraw. It is also hard to 
understand without the context of the previous version. If you look at 
the change (daebdd7) it was the call to hid_hw_stop() which disconnected 
hidraw and not the return code, if rmi_populate() can't find F11 on the 
device.

Otherwise, the changes to rmi look correct.

Andrew

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

[Nikolai Kondrashov]

On 09/26/2015 02:14 AM, Dmitry Torokhov 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.
>
> diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
> index b905d50..85ac435 100644
> --- a/drivers/hid/hid-uclogic.c
> +++ b/drivers/hid/hid-uclogic.c
> @@ -731,7 +731,7 @@ static int uclogic_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>   	return 0;
>   }
>
> -static void uclogic_input_configured(struct hid_device *hdev,
> +static int uclogic_input_configured(struct hid_device *hdev,
>   		struct hid_input *hi)
>   {
>   	char *name;
> @@ -741,7 +741,7 @@ static void uclogic_input_configured(struct hid_device *hdev,
>
>   	/* no report associated (HID_QUIRK_MULTI_INPUT not set) */
>   	if (!hi->report)
> -		return;
> +		return 0;
>
>   	field = hi->report->field[0];
>
> @@ -774,6 +774,8 @@ static void uclogic_input_configured(struct hid_device *hdev,
>   			hi->input->name = name;
>   		}
>   	}
> +
> +	return 0;
>   }
>
>   /**

The hid-uclogic.c change looks perfectly fine to me.

Thank you, Dmitri.

The next step would be to report devm_kzalloc failure instead of ignoring it.

Nick

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

[Dmitry Torokhov]

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>
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Reviewed-by: David Herrmann <dh.herrmann@gmail.com>
Acked-by: Nikolai Kondrashov <Nikolai.Kondrashov@redhat.com>
Acked-by: Andrew Duggan <aduggan@synaptics.com>
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---

v1->v2:
- addressed David's comments re: hid-multitouch.c
- changed hid-rmi.c to not return error if device is not RMI one as
  mentioned by Andrew Duggan.

 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     | 20 +++++++++++++++-----
 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, 66 insertions(+), 28 deletions(-)

diff --git a/drivers/hid/hid-appleir.c b/drivers/hid/hid-appleir.c
index 0e6a42d..07cbc70 100644
--- a/drivers/hid/hid-appleir.c
+++ b/drivers/hid/hid-appleir.c
@@ -256,7 +256,7 @@ out:
 	return 0;
 }
 
-static void appleir_input_configured(struct hid_device *hid,
+static int appleir_input_configured(struct hid_device *hid,
 		struct hid_input *hidinput)
 {
 	struct input_dev *input_dev = hidinput->input;
@@ -275,6 +275,8 @@ static void appleir_input_configured(struct hid_device *hid,
 	for (i = 0; i < ARRAY_SIZE(appleir_key_table); i++)
 		set_bit(appleir->keymap[i], input_dev->keybit);
 	clear_bit(KEY_RESERVED, input_dev->keybit);
+
+	return 0;
 }
 
 static int appleir_input_mapping(struct hid_device *hid,
diff --git a/drivers/hid/hid-elo.c b/drivers/hid/hid-elo.c
index 4e49462..aad8c16 100644
--- a/drivers/hid/hid-elo.c
+++ b/drivers/hid/hid-elo.c
@@ -37,7 +37,7 @@ static bool use_fw_quirk = true;
 module_param(use_fw_quirk, bool, S_IRUGO);
 MODULE_PARM_DESC(use_fw_quirk, "Do periodic pokes for broken M firmwares (default = true)");
 
-static void elo_input_configured(struct hid_device *hdev,
+static int elo_input_configured(struct hid_device *hdev,
 		struct hid_input *hidinput)
 {
 	struct input_dev *input = hidinput->input;
@@ -45,6 +45,8 @@ static void elo_input_configured(struct hid_device *hdev,
 	set_bit(BTN_TOUCH, input->keybit);
 	set_bit(ABS_PRESSURE, input->absbit);
 	input_set_abs_params(input, ABS_PRESSURE, 0, 256, 0, 0);
+
+	return 0;
 }
 
 static void elo_process_data(struct input_dev *input, const u8 *data, int size)
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;
 	}
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index e4bc6cb..8979f1f 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -848,7 +848,7 @@ static void lenovo_remove(struct hid_device *hdev)
 	hid_hw_stop(hdev);
 }
 
-static void lenovo_input_configured(struct hid_device *hdev,
+static int lenovo_input_configured(struct hid_device *hdev,
 		struct hid_input *hi)
 {
 	switch (hdev->product) {
@@ -863,6 +863,8 @@ static void lenovo_input_configured(struct hid_device *hdev,
 			}
 			break;
 	}
+
+	return 0;
 }
 
 
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 452e5d5..5fd9786 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -1285,13 +1285,15 @@ static void hidpp_populate_input(struct hidpp_device *hidpp,
 		m560_populate_input(hidpp, input, origin_is_hid_core);
 }
 
-static void hidpp_input_configured(struct hid_device *hdev,
+static int hidpp_input_configured(struct hid_device *hdev,
 				struct hid_input *hidinput)
 {
 	struct hidpp_device *hidpp = hid_get_drvdata(hdev);
 	struct input_dev *input = hidinput->input;
 
 	hidpp_populate_input(hidpp, input, true);
+
+	return 0;
 }
 
 static int hidpp_raw_hidpp_event(struct hidpp_device *hidpp, u8 *data,
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index 29a74c1..d6fa496 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -471,18 +471,22 @@ static int magicmouse_input_mapping(struct hid_device *hdev,
 	return 0;
 }
 
-static void magicmouse_input_configured(struct hid_device *hdev,
+static int magicmouse_input_configured(struct hid_device *hdev,
 		struct hid_input *hi)
 
 {
 	struct magicmouse_sc *msc = hid_get_drvdata(hdev);
+	int ret;
 
-	int ret = magicmouse_setup_input(msc->input, hdev);
+	ret = magicmouse_setup_input(msc->input, hdev);
 	if (ret) {
 		hid_err(hdev, "magicmouse setup input failed (%d)\n", ret);
 		/* clean msc->input to notify probe() of the failure */
 		msc->input = NULL;
+		return ret;
 	}
+
+	return 0;
 }
 
 
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 426b2f1..2ed42d8 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;
 
-	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,8 @@ static void mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
 			hi->input->name = name;
 		}
 	}
+
+	return 0;
 }
 
 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index 600f207..756d1ef 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -859,14 +859,14 @@ not_claimed_input:
 	return 1;
 }
 
-static void ntrig_input_configured(struct hid_device *hid,
+static int ntrig_input_configured(struct hid_device *hid,
 		struct hid_input *hidinput)
 
 {
 	struct input_dev *input = hidinput->input;
 
 	if (hidinput->report->maxfield < 1)
-		return;
+		return 0;
 
 	switch (hidinput->report->field[0]->application) {
 	case HID_DG_PEN:
@@ -890,6 +890,8 @@ static void ntrig_input_configured(struct hid_device *hid,
 							"N-Trig MultiTouch";
 		break;
 	}
+
+	return 0;
 }
 
 static int ntrig_probe(struct hid_device *hdev, const struct hid_device_id *id)
diff --git a/drivers/hid/hid-rmi.c b/drivers/hid/hid-rmi.c
index 2c14812..67cd059 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 0;
 
 	/* 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;
 }
 
 static int rmi_input_mapping(struct hid_device *hdev,
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 661f94f..774cd22 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -1360,20 +1360,27 @@ static int sony_register_touchpad(struct hid_input *hi, int touch_count,
 	return 0;
 }
 
-static void sony_input_configured(struct hid_device *hdev,
+static int sony_input_configured(struct hid_device *hdev,
 					struct hid_input *hidinput)
 {
 	struct sony_sc *sc = hid_get_drvdata(hdev);
+	int ret;
 
 	/*
 	 * The Dualshock 4 touchpad supports 2 touches and has a
 	 * resolution of 1920x942 (44.86 dots/mm).
 	 */
 	if (sc->quirks & DUALSHOCK4_CONTROLLER) {
-		if (sony_register_touchpad(hidinput, 2, 1920, 942) != 0)
+		ret = sony_register_touchpad(hidinput, 2, 1920, 942);
+		if (ret) {
 			hid_err(sc->hdev,
-				"Unable to initialize multi-touch slots\n");
+				"Unable to initialize multi-touch slots: %d\n",
+				ret);
+			return ret;
+		}
 	}
+
+	return 0;
 }
 
 /*
diff --git a/drivers/hid/hid-uclogic.c b/drivers/hid/hid-uclogic.c
index b905d50..85ac435 100644
--- a/drivers/hid/hid-uclogic.c
+++ b/drivers/hid/hid-uclogic.c
@@ -731,7 +731,7 @@ static int uclogic_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 	return 0;
 }
 
-static void uclogic_input_configured(struct hid_device *hdev,
+static int uclogic_input_configured(struct hid_device *hdev,
 		struct hid_input *hi)
 {
 	char *name;
@@ -741,7 +741,7 @@ static void uclogic_input_configured(struct hid_device *hdev,
 
 	/* no report associated (HID_QUIRK_MULTI_INPUT not set) */
 	if (!hi->report)
-		return;
+		return 0;
 
 	field = hi->report->field[0];
 
@@ -774,6 +774,8 @@ static void uclogic_input_configured(struct hid_device *hdev,
 			hi->input->name = name;
 		}
 	}
+
+	return 0;
 }
 
 /**
diff --git a/include/linux/hid.h b/include/linux/hid.h
index f17980d..251a1d3 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -698,8 +698,8 @@ struct hid_driver {
 	int (*input_mapped)(struct hid_device *hdev,
 			struct hid_input *hidinput, struct hid_field *field,
 			struct hid_usage *usage, unsigned long **bit, int *max);
-	void (*input_configured)(struct hid_device *hdev,
-				 struct hid_input *hidinput);
+	int (*input_configured)(struct hid_device *hdev,
+				struct hid_input *hidinput);
 	void (*feature_mapping)(struct hid_device *hdev,
 			struct hid_field *field,
 			struct hid_usage *usage);
-- 
2.6.0.rc2.230.g3dd15c0

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

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

[Jiri Kosina]

On Tue, 29 Sep 2015, Dmitry Torokhov 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.

Applied to hid.git#for-4.4/upstream, thanks Dmitry.

-- 
Jiri Kosina
SUSE Labs