Hi Benjamin,
> In order to provide fine control for the creation of different
> input devices in probe function of third party drivers, this patch
> split the allocations, the registrations and the free of input
> devices.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxx>
> ---
> drivers/hid/hid-input.c | 28 ++++++++++++++++++----------
> 1 file changed, 18 insertions(+), 10 deletions(-)
I don't like this patch, nor its purpose. Drivers should not depend on
the hid core working in a particular way internally, that spells
disaster. There must be some other way in which the same effect can be
achieved?
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 47f98a3..eea02b0 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1206,6 +1206,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> struct hid_driver *drv = hid->driver;
> struct hid_report *report;
> struct hid_input *hidinput = NULL;
> + struct hid_input *next;
> int i, j, k;
>
> INIT_LIST_HEAD(&hid->inputs);
> @@ -1238,7 +1239,7 @@ int hidinput_connect(struct hid_device *hid, unsigned int force)
> if (!hidinput) {
> hidinput = hidinput_allocate(hid);
> if (!hidinput)
> - goto out_unwind;
> + goto out_cleanup;
> }
>
> for (i = 0; i < report->maxfield; i++)
> @@ -1253,29 +1254,36 @@ 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 (input_register_device(hidinput->input))
> - goto out_cleanup;
> hidinput = NULL;
> }
> }
> }
>
> - if (hidinput) {
> + list_for_each_entry(hidinput, &hid->inputs, list) {
> if (drv->input_configured)
> drv->input_configured(hid, hidinput);
> if (input_register_device(hidinput->input))
> - goto out_cleanup;
> + goto out_unwind;
> }
>
> return 0;
>
> out_cleanup:
> - list_del(&hidinput->list);
> - input_free_device(hidinput->input);
> - kfree(hidinput);
> + list_for_each_entry_safe(hidinput, next, &hid->inputs, list) {
> + list_del(&hidinput->list);
> + input_free_device(hidinput->input);
> + kfree(hidinput);
> + }
> + return -1;
Irregular return in the error path?
> +
> out_unwind:
> + /* free the non-registered hidinput, starting from the faulty one */
> + list_for_each_entry_safe_from(hidinput, next, &hid->inputs, list) {
> + list_del(&hidinput->list);
> + input_free_device(hidinput->input);
> + kfree(hidinput);
> + }
> +
> /* unwind the ones we already registered */
> hidinput_disconnect(hid);
>
> --
> 1.8.0
>
Thanks,
Henrik
--
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
