Re: [PATCH 02/11] HID: hid-input: simplify hid_input allocation and registration

["Henrik Rydberg" <rydberg@euromail.se>]

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