Re: [PATCH v4 4/4] HID: wiimote: wiimote_probe with scoped cleanup

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

Hi

On Mon, Aug 17, 2026, at 11:38 PM, Rafael Passos wrote:
> Use the safer scoped cleanup with a single destroy function.
> A new bitmask was introduced to track probing state.
> This is needed because the hid_hw calls cannot be made with null.
>
> A few other functions are safe to call without checking.
> These cases are annotated with comments above them.
>
> Also, a new debugfs entry was added tracking this new state (bitmask).
>
> Signed-off-by: Rafael Passos <rafael@rcpassos.me>

I am really not sold on this. This does not make the code any simpler, does it? IMO, the goto-paths are much easier to read than tracking the state at runtime.

Do you think this makes the code easier to understand? Am I off here?

Thanks
David

> ---
>  drivers/hid/hid-wiimote-core.c  | 78 +++++++++++++++++++--------------
>  drivers/hid/hid-wiimote-debug.c |  4 ++
>  drivers/hid/hid-wiimote.h       |  9 ++++
>  3 files changed, 58 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 05f8ddb7909b..044da4daa010 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -679,6 +679,8 @@ static void wiimote_modules_load(struct wiimote_data *wdata,
>  		wiiproto_req_leds(wdata, player_leds[(wdata->player_id - 1) % 4]);
>  	}
> 
> +
> +	wdata->init_state |= WIIMOTE_MODULES_LOADED;
>  	return;
> 
>  error:
> @@ -742,6 +744,8 @@ static void wiimote_ext_load(struct wiimote_data 
> *wdata, unsigned int ext)
> 
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock)
>  		wdata->state.exttype = ext;
> +
> +	wdata->init_state |= WIIMOTE_EXT_LOADED;
>  }
> 
>  static void wiimote_ext_unload(struct wiimote_data *wdata)
> @@ -774,6 +778,8 @@ static void wiimote_mp_load(struct wiimote_data *wdata)
> 
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock)
>  		wdata->state.mp = mode;
> +
> +	wdata->init_state |= WIIMOTE_MP_LOADED;
>  }
> 
>  static void wiimote_mp_unload(struct wiimote_data *wdata)
> @@ -1751,39 +1757,56 @@ static DEFINE_IDA(wiimote_ida);
> 
>  static void wiimote_destroy(struct wiimote_data *wdata)
>  {
> +	if (!wdata)
> +		return;
> +
> +	// safe, debugfs checks IS_ERR_OR_NULL
>  	wiidebug_deinit(wdata);
> 
> -	ida_free(&wiimote_ida, wdata->player_id);
> +	if (wdata->player_id)
> +		ida_free(&wiimote_ida, wdata->player_id);
> 
>  	/* prevent init_worker from being scheduled again */
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock)
>  		wdata->state.flags |= WIIPROTO_FLAG_EXITING;
> 
> -	cancel_work_sync(&wdata->init_worker);
> -	timer_shutdown_sync(&wdata->timer);
> +	if (wdata->init_state & WIIMOTE_PROBE_READY) {
> +		cancel_work_sync(&wdata->init_worker);
> +		timer_shutdown_sync(&wdata->timer);
> +	}
> 
> +	// safe, checks dev for NULL
>  	device_remove_file(&wdata->hdev->dev, &dev_attr_devtype);
>  	device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> 
> -	wiimote_mp_unload(wdata);
> -	wiimote_ext_unload(wdata);
> -	wiimote_modules_unload(wdata);
> +	if (wdata->init_state & WIIMOTE_MP_LOADED)
> +		wiimote_mp_unload(wdata);
> +	if (wdata->init_state & WIIMOTE_EXT_LOADED)
> +		wiimote_ext_unload(wdata);
> +	if (wdata->init_state & WIIMOTE_MODULES_LOADED)
> +		wiimote_modules_unload(wdata);
> +
>  	cancel_work_sync(&wdata->queue.worker);
> -	hid_hw_close(wdata->hdev);
> -	hid_hw_stop(wdata->hdev);
> +
> +	if (wdata->init_state & WIIMOTE_PROBE_HW_OPENED)
> +		hid_hw_close(wdata->hdev);
> +	if (wdata->init_state & WIIMOTE_PROBE_HW_STARTED)
> +		hid_hw_stop(wdata->hdev);
> 
>  	kfree(wdata);
>  }
> 
> +DEFINE_FREE(wiimote_probe_cleanup, struct wiimote_data *,
> +	wiimote_destroy(_T))
> +
>  static int wiimote_hid_probe(struct hid_device *hdev,
>  				const struct hid_device_id *id)
>  {
> -	struct wiimote_data *wdata;
>  	int ret;
> 
>  	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;
> @@ -1792,41 +1815,43 @@ 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->init_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->init_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;
> 
>  	ret = ida_alloc_min(&wiimote_ida, 1, GFP_KERNEL);
>  	if (ret < 1) {
>  		hid_err(hdev, "cannot allocate controller id\n");
> -		goto err_free;
> +		return ret;
>  	}
> 
>  	wdata->player_id = ret;
> @@ -1834,24 +1859,10 @@ static int wiimote_hid_probe(struct hid_device *hdev,
> 
>  	/* schedule device detection */
>  	wiimote_schedule(wdata);
> +	wdata->init_state |= WIIMOTE_PROBE_READY;
> 
> +	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)
> @@ -1902,3 +1913,4 @@ module_exit(wiimote_exit);
>  MODULE_LICENSE("GPL");
>  MODULE_AUTHOR("David Herrmann <dh.herrmann@gmail.com>");
>  MODULE_DESCRIPTION("Driver for Nintendo Wii / Wii U peripherals");
> +
> diff --git a/drivers/hid/hid-wiimote-debug.c b/drivers/hid/hid-wiimote-debug.c
> index b8027bb23608..1353ab022acb 100644
> --- a/drivers/hid/hid-wiimote-debug.c
> +++ b/drivers/hid/hid-wiimote-debug.c
> @@ -184,6 +184,9 @@ int wiidebug_init(struct wiimote_data *wdata)
>  	debugfs_create_u8("player_id", S_IRUSR,
>  		   dbg->wdata->hdev->debug_dir, &wdata->player_id);
> 
> +	debugfs_create_u8("init_state", S_IRUSR,
> +		   dbg->wdata->hdev->debug_dir, &wdata->init_state);
> +
>  	scoped_guard(spinlock_irqsave, &wdata->state.lock)
>  		wdata->debug = dbg;
> 
> @@ -203,5 +206,6 @@ void wiidebug_deinit(struct wiimote_data *wdata)
>  	debugfs_remove(dbg->drm);
>  	debugfs_remove(dbg->eeprom);
>  	debugfs_lookup_and_remove("player_id", dbg->wdata->hdev->debug_dir);
> +	debugfs_lookup_and_remove("init_state", dbg->wdata->hdev->debug_dir);
>  	kfree(dbg);
>  }
> diff --git a/drivers/hid/hid-wiimote.h b/drivers/hid/hid-wiimote.h
> index 8e5002f515e2..147751973702 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 init_state;
> 
>  	union {
>  		struct input_dev *input;
> @@ -376,4 +377,12 @@ static inline int wiimote_cmd_wait_noint(struct 
> wiimote_data *wdata)
>  		return 0;
>  }
> 
> +/* controller initialization tracker bits */
> +#define WIIMOTE_PROBE_HW_STARTED  BIT(0)  // hid_hw_start succeeded
> +#define WIIMOTE_PROBE_HW_OPENED   BIT(1)  // hid_hw_open succeeded
> +#define WIIMOTE_PROBE_READY       BIT(2)  // wiimote_schedule succeeded
> +#define WIIMOTE_MP_LOADED         BIT(3)  // wiimote_mp_load succeeded
> +#define WIIMOTE_EXT_LOADED        BIT(4)  // wiimote_ext_load succeeded
> +#define WIIMOTE_MODULES_LOADED    BIT(5)  // wiimote_modules_load succeeded
> +
>  #endif
> -- 
> 2.55.0