Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()

From: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
To: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Cc: jikos@kernel.org, benjamin.tissoires@redhat.com,
	linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
	kernel-janitors@vger.kernel.org
Subject: Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
Date: Sun, 17 Sep 2023 22:37:23 +0200	[thread overview]
Message-ID: <5111a913-efc9-28c6-64ad-df2586bd0895@wanadoo.fr> (raw)
In-Reply-To: <87msxnupmc.fsf@nvidia.com>

Le 15/09/2023 à 22:51, Rahul Rameshbabu a écrit :
> On Fri, 15 Sep, 2023 22:14:18 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
>> Le 15/09/2023 à 20:16, Rahul Rameshbabu a écrit :
>>> Hi Christophe,
>>> On Sat, 26 Aug, 2023 19:42:16 +0200 Christophe JAILLET
>>> <christophe.jaillet@wanadoo.fr> wrote:
>>>> This serie fixes some missing clean-up function calls in the error handling of
>>>> the probe.
>>>>
>>>> Patch 1 and 2 fix some similar issues introduced in 2 different commits (hence 2
>>>> patches)
>>>>
>>>> Patch 3 is a proposal to be more future proof.
>>>>
>>>>
>>>> *Note*: I'm not 100% sure that the order of the functions is the best one in
>>>> thunderstrike_destroy(), but it is the way it was.
>>>>
>>>> My personal preference would be to undo things in reverse order they are
>>>> allocated, such as:
>>>> 	led_classdev_unregister(&ts->led_dev);
>>>> 	power_supply_unregister(ts->base.battery_dev.psy);
>>>> 	if (ts->haptics_dev)
>>>> 		input_unregister_device(ts->haptics_dev);
>>>> 	ida_free(&thunderstrike_ida, ts->id);
>>>> This order was explicitly chnaged by 3ab196f88237, so, as I can't test the
>>>> changes on a real harware, I've left it as-is.
>>>>
>>>> Christophe JAILLET (3):
>>>>     HID: nvidia-shield: Fix a missing led_classdev_unregister() in the
>>>>       probe error handling path
>>>>     HID: nvidia-shield: Fix some missing function calls() in the probe
>>>>       error handling path
>>>>     HID: nvidia-shield: Introduce thunderstrike_destroy()
>>>>
>>>>    drivers/hid/hid-nvidia-shield.c | 23 ++++++++++++++++-------
>>>>    1 file changed, 16 insertions(+), 7 deletions(-)
>>> I was wondering if you have time to address the comments in this
>>> submission. If not, I can re-spin the patches with the needed changes in
>>> upcoming days.
>>
>> I can send an update tomorrow, but I'm only working with -next, so should using
>> for-6.6/nvidia (as said in your comment in #1/3) be a must have, then it would
>> be more convenient for me if you make the changes by yourself.
> 
> Luckily, it does not have to be on top of for-6.6/nvidia to add the fix
> I mentioned with regards to the led_classdev flag for not trying to
> power off the led when unregistering the led_classdev. That should still
> merge nicely on top of for-6.6/nvidia. The main reason I mentioned it
> was due to the commit living there with regards to the issue involving
> unregistering the led_classdev without the mentioned flag.

Well, because of your comment on patch #1/3, I would prefer you to make 
the relevant changes.

Understanding this code if more time consuming than I first expected.

CJ

> 
> --
> Thanks for the patches,
> 
> Rahul Rameshbabu
> 
>>
>> CJ
>>
>>> --
>>> Thanks,
>>> Rahul Rameshbabu
>>>
> 