Re: [PATCH v3 3/4] HID: wiimote: use scoped cleanup in wiimote and led probes

["David Rheinsberg" <david@readahead.eu>]

Hi

On Wed, Jul 29, 2026, at 6:49 PM, Rafael Passos wrote:
> Cleanup code in wiimote/led probe function, using the scoped cleanup.
> This prevents mistakes in future changes to this function.
>
> In wiimote_probe_clenaup, a few functions are safe to call without
> checking. For the hid_hw calls, a new bit mask was introduced to track
> probing state.

Is this patch worth it? the led-probe looks ok, but the wiimote_probe() change looks convoluted. If you really want to go that route I would prefer if you reuse wiimote_destroy() and ensure it checks for the right conditions, rather than adding __wiimote_probe_cleanup().

Thanks
David

> Signed-off-by: Rafael Passos <rafael@rcpassos.me>
> ---
>  drivers/hid/hid-wiimote-core.c    | 68 ++++++++++++++++++-------------
>  drivers/hid/hid-wiimote-modules.c | 17 ++++----
>  drivers/hid/hid-wiimote.h         |  1 +
>  3 files changed, 48 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 762b3c383194e..31ee86affc553 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1772,16 +1772,40 @@ static void wiimote_destroy(struct wiimote_data *wdata)
>  /* Global id allocator for wii remotes */
>  static DEFINE_IDA(wiimote_ida);
> 
> +#define WIIMOTE_PROBE_HW_STARTED  BIT(0)  // hid_hw_start succeeded
> +#define WIIMOTE_PROBE_HW_OPENED   BIT(1)  // hid_hw_open succeeded
> +
> +static void __wiimote_probe_cleanup(struct wiimote_data *wdata)
> +{
> +	if (!wdata)
> +		return;
> +
> +	if (wdata->player_id)
> +		ida_free(&wiimote_ida, wdata->player_id);
> +
> +	// safe, debugfs checks IS_ERR_OR_NULL
> +	wiidebug_deinit(wdata);
> +	// safe, checks dev for NULL
> +	device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
> +	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> +	if (wdata->probe_state & WIIMOTE_PROBE_HW_OPENED)
> +		hid_hw_close(wdata->hdev);
> +	if (wdata->probe_state & WIIMOTE_PROBE_HW_STARTED)
> +		hid_hw_stop(wdata->hdev);
> +	kfree(wdata);
> +}
> +
> +DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
> +	__wiimote_probe_cleanup(_T))
> +
>  static int wiimote_hid_probe(struct hid_device *hdev,
>  				const struct hid_device_id *id)
>  {
> -	struct wiimote_data *wdata;
>  	int ret;
> -	int player_id;
> 
>  	hdev->quirks |= HID_QUIRK_NO_INIT_REPORTS;
> 
> -	wdata = wiimote_create(hdev);
> +	struct wiimote_data *wdata __free(wiimote_probe_cleanup) = 
> wiimote_create(hdev);
>  	if (!wdata) {
>  		hid_err(hdev, "Can't alloc device\n");
>  		return -ENOMEM;
> @@ -1790,68 +1814,54 @@ static int wiimote_hid_probe(struct hid_device 
> *hdev,
>  	ret = hid_parse(hdev);
>  	if (ret) {
>  		hid_err(hdev, "HID parse failed\n");
> -		goto err;
> +		return ret;
>  	}
> 
>  	ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>  	if (ret) {
>  		hid_err(hdev, "HW start failed\n");
> -		goto err;
> +		return ret;
>  	}
> +	wdata->probe_state |= WIIMOTE_PROBE_HW_STARTED;
> 
>  	ret = hid_hw_open(hdev);
>  	if (ret) {
>  		hid_err(hdev, "cannot start hardware I/O\n");
> -		goto err_stop;
> +		return ret;
>  	}
> +	wdata->probe_state |= WIIMOTE_PROBE_HW_OPENED;
> 
>  	ret = device_create_file(&hdev->dev, &dev_attr_extension);
>  	if (ret) {
>  		hid_err(hdev, "cannot create sysfs attribute\n");
> -		goto err_close;
> +		return ret;
>  	}
> 
>  	ret = device_create_file(&hdev->dev, &dev_attr_devtype);
>  	if (ret) {
>  		hid_err(hdev, "cannot create sysfs attribute\n");
> -		goto err_ext;
> +		return ret;
>  	}
> 
>  	ret = wiidebug_init(wdata);
>  	if (ret)
> -		goto err_free;
> +		return ret;
> 
> -	player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
> +	int player_id = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
>  	if (player_id < 1) {
>  		hid_err(hdev, "cannot allocate controller id\n");
>  		ret = player_id;
> -		goto err_free;
> +		return ret;
>  	}
> -
>  	wdata->player_id = player_id;
> 
> +
>  	hid_info(hdev, "New device registered (Wiimote %d)\n", player_id);
> 
>  	/* schedule device detection */
>  	wiimote_schedule(wdata);
> -
> +	retain_and_null_ptr(wdata);
>  	return 0;
> -
> -err_free:
> -	wiimote_destroy(wdata);
> -	return ret;
> -
> -err_ext:
> -	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> -err_close:
> -	hid_hw_close(hdev);
> -err_stop:
> -	hid_hw_stop(hdev);
> -err:
> -	input_free_device(wdata->ir);
> -	input_free_device(wdata->accel);
> -	kfree(wdata);
> -	return ret;
>  }
> 
>  static void wiimote_hid_remove(struct hid_device *hdev)
> diff --git a/drivers/hid/hid-wiimote-modules.c 
> b/drivers/hid/hid-wiimote-modules.c
> index 3cd6144667404..47fa6a8ecdaef 100644
> --- a/drivers/hid/hid-wiimote-modules.c
> +++ b/drivers/hid/hid-wiimote-modules.c
> @@ -341,11 +341,11 @@ static int wiimod_led_probe(const struct 
> wiimod_ops *ops,
>  {
>  	struct device *dev = &wdata->hdev->dev;
>  	size_t namesz = strlen(dev_name(dev)) + 9;
> -	struct led_classdev *led;
>  	char *name;
>  	int ret;
> 
> -	led = kzalloc(sizeof(struct led_classdev) + namesz, GFP_KERNEL);
> +	struct led_classdev *led __free(kfree) =
> +		kzalloc(sizeof(struct led_classdev) + namesz, GFP_KERNEL);
>  	if (!led)
>  		return -ENOMEM;
> 
> @@ -359,8 +359,12 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
> 
>  	wdata->leds[ops->arg] = led;
>  	ret = led_classdev_register(dev, led);
> -	if (ret)
> -		goto err_free;
> +	if (ret) {
> +		wdata->leds[ops->arg] = NULL;
> +		return ret;
> +	}
> +
> +	retain_and_null_ptr(led);
> 
>  	/* enable LED1 to stop initial LED-blinking */
>  	if (ops->arg == 0) {
> @@ -369,11 +373,6 @@ static int wiimod_led_probe(const struct wiimod_ops *ops,
>  	}
> 
>  	return 0;
> -
> -err_free:
> -	wdata->leds[ops->arg] = NULL;
> -	kfree(led);
> -	return ret;
>  }
> 
>  static void wiimod_led_remove(const struct wiimod_ops *ops,
> diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
> index a53f72d5077ef..6812efa589c93 100644
> --- a/drivers/hid/hid-wiimote.h
> +++ b/drivers/hid/hid-wiimote.h
> @@ -154,6 +154,7 @@ struct wiimote_data {
>  	struct timer_list timer;
>  	struct wiimote_debug *debug;
>  	__u8 player_id;
> +	__u8 probe_state;
> 
>  	union {
>  		struct input_dev *input;
> -- 
> 2.53.0