lkml.org 
[lkml]   [2012]   [Nov]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH 02/11] HID: hid-input: simplify hid_input allocation and registration
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@gmail.com>
> ---
> 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


\
 
 \ /
  Last update: 2012-11-27 21:21    [W:0.184 / U:5.484 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site