Re: [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path

[Rahul Rameshbabu]

On Sat, 26 Aug, 2023 19:42:17 +0200 Christophe JAILLET <christophe.jaillet@xxxxxxxxxx> wrote:
> The commit in Fixes updated the error handling path of
> thunderstrike_create() and the remove function but not the error handling
> path of shield_probe(), should an error occur after a successful
> thunderstrike_create() call.
>
> Add the missing call.

You are right that the led instance needs to be cleaned up here.
However, there is another bug that is introduced by this patch since
this is in the error path where hid_hw_start failed. The problem is that
led_classdev_unregister makes sure to set the led state to off in the
cleanup actions. The problem is that it is unsafe to do so in this
context since we cannot send hid_hw_raw_request at this point.

I discuss this in the following patch submission.

  https://lore.kernel.org/linux-input/20230807163620.16855-1-rrameshbabu@xxxxxxxxxx/

I think we should take the alternative approach I mentioned in the
mentioned patch where we set the LED_RETAIN_AT_SHUTDOWN led_classdev
flag. Then in the context of shield_remove, we can explicitly call
led_set_brightness(&ts->led_dev, LED_OFF).

You will want to base this suggested change on top of the for-6.6/nvidia
branch in the hid subsystem tree.

  https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-6.6/nvidia

>
> Fixes: f88af60e74a5 ("HID: nvidia-shield: Support LED functionality for Thunderstrike")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@xxxxxxxxxx>
> ---
>  drivers/hid/hid-nvidia-shield.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
> index 9a3576dbf421..66a7478e2c9d 100644
> --- a/drivers/hid/hid-nvidia-shield.c
> +++ b/drivers/hid/hid-nvidia-shield.c
> @@ -1076,6 +1076,7 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  err_haptics:
>  	if (ts->haptics_dev)
>  		input_unregister_device(ts->haptics_dev);
> +	led_classdev_unregister(&ts->led_dev);
>  	return ret;
>  }

--
Thanks,

Rahul Rameshbabu