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

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

[Christophe JAILLET]

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(-)

-- 
2.34.1

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

[Christophe JAILLET]

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.

Fixes: f88af60e74a5 ("HID: nvidia-shield: Support LED functionality for Thunderstrike")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 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;
 }
 
-- 
2.34.1

[PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() in the probe error handling path

[Christophe JAILLET]

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 calls.

Fixes: 3ab196f88237 ("HID: nvidia-shield: Add battery support for Thunderstrike")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/hid/hid-nvidia-shield.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index 66a7478e2c9d..849b3f8409a0 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -1074,9 +1074,11 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
 err_stop:
 	hid_hw_stop(hdev);
 err_haptics:
+	power_supply_unregister(ts->base.battery_dev.psy);
 	if (ts->haptics_dev)
 		input_unregister_device(ts->haptics_dev);
 	led_classdev_unregister(&ts->led_dev);
+	ida_free(&thunderstrike_ida, ts->id);
 	return ret;
 }
 
-- 
2.34.1

[PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()

[Christophe JAILLET]

In order to simplify some error handling paths, and avoid code duplication
introduce thunderstrike_destroy() which undoes thunderstrike_create().

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
 drivers/hid/hid-nvidia-shield.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
index 849b3f8409a0..4e39e7c1a2c3 100644
--- a/drivers/hid/hid-nvidia-shield.c
+++ b/drivers/hid/hid-nvidia-shield.c
@@ -915,6 +915,20 @@ static struct shield_device *thunderstrike_create(struct hid_device *hdev)
 	return ERR_PTR(ret);
 }
 
+static void thunderstrike_destroy(struct hid_device *hdev)
+{
+	struct shield_device *dev = hid_get_drvdata(hdev);
+	struct thunderstrike *ts;
+
+	ts = container_of(dev, struct thunderstrike, base);
+
+	power_supply_unregister(ts->base.battery_dev.psy);
+	if (ts->haptics_dev)
+		input_unregister_device(ts->haptics_dev);
+	led_classdev_unregister(&ts->led_dev);
+	ida_free(&thunderstrike_ida, ts->id);
+}
+
 static int android_input_mapping(struct hid_device *hdev, struct hid_input *hi,
 				 struct hid_field *field,
 				 struct hid_usage *usage, unsigned long **bit,
@@ -1074,11 +1088,7 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
 err_stop:
 	hid_hw_stop(hdev);
 err_haptics:
-	power_supply_unregister(ts->base.battery_dev.psy);
-	if (ts->haptics_dev)
-		input_unregister_device(ts->haptics_dev);
-	led_classdev_unregister(&ts->led_dev);
-	ida_free(&thunderstrike_ida, ts->id);
+	thunderstrike_destroy(hdev);
 	return ret;
 }
 
@@ -1090,11 +1100,7 @@ static void shield_remove(struct hid_device *hdev)
 	ts = container_of(dev, struct thunderstrike, base);
 
 	hid_hw_close(hdev);
-	power_supply_unregister(dev->battery_dev.psy);
-	if (ts->haptics_dev)
-		input_unregister_device(ts->haptics_dev);
-	led_classdev_unregister(&ts->led_dev);
-	ida_free(&thunderstrike_ida, ts->id);
+	thunderstrike_destroy(hdev);
 	del_timer_sync(&ts->psy_stats_timer);
 	cancel_work_sync(&ts->hostcmd_req_work);
 	hid_hw_stop(hdev);
-- 
2.34.1

Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()

[kernel test robot]

Hi Christophe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linux-next/master]
[cannot apply to linus/master v6.5-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/HID-nvidia-shield-Fix-a-missing-led_classdev_unregister-in-the-probe-error-handling-path/20230827-014602
base:   linux-next/master
patch link:    https://lore.kernel.org/r/4c9a8c7f6b4eb879dd7ef4d44bb6a80b3f126d25.1693070958.git.christophe.jaillet%40wanadoo.fr
patch subject: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20230827/202308270307.EDe7t62T-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20230827/202308270307.EDe7t62T-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308270307.EDe7t62T-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/hid/hid-nvidia-shield.c: In function 'shield_probe':
>> drivers/hid/hid-nvidia-shield.c:1046:31: warning: variable 'ts' set but not used [-Wunused-but-set-variable]
    1046 |         struct thunderstrike *ts;
         |                               ^~


vim +/ts +1046 drivers/hid/hid-nvidia-shield.c

09308562d4afb1a Rahul Rameshbabu   2023-06-08  1042  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1043  static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1044  {
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1045  	struct shield_device *shield_dev = NULL;
09308562d4afb1a Rahul Rameshbabu   2023-06-08 @1046  	struct thunderstrike *ts;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1047  	int ret;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1048  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1049  	ret = hid_parse(hdev);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1050  	if (ret) {
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1051  		hid_err(hdev, "Parse failed\n");
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1052  		return ret;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1053  	}
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1054  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1055  	switch (id->product) {
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1056  	case USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER:
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1057  		shield_dev = thunderstrike_create(hdev);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1058  		break;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1059  	}
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1060  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1061  	if (unlikely(!shield_dev)) {
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1062  		hid_err(hdev, "Failed to identify SHIELD device\n");
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1063  		return -ENODEV;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1064  	}
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1065  	if (IS_ERR(shield_dev)) {
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1066  		hid_err(hdev, "Failed to create SHIELD device\n");
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1067  		return PTR_ERR(shield_dev);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1068  	}
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1069  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1070  	ts = container_of(shield_dev, struct thunderstrike, base);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1071  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1072  	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1073  	if (ret) {
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1074  		hid_err(hdev, "Failed to start HID device\n");
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1075  		goto err_haptics;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1076  	}
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1077  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1078  	ret = hid_hw_open(hdev);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1079  	if (ret) {
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1080  		hid_err(hdev, "Failed to open HID device\n");
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1081  		goto err_stop;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1082  	}
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1083  
3ab196f882377ed Rahul Rameshbabu   2023-08-07  1084  	thunderstrike_device_init_info(shield_dev);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1085  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1086  	return ret;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1087  
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1088  err_stop:
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1089  	hid_hw_stop(hdev);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1090  err_haptics:
2cc4637842495c6 Christophe JAILLET 2023-08-26  1091  	thunderstrike_destroy(hdev);
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1092  	return ret;
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1093  }
09308562d4afb1a Rahul Rameshbabu   2023-06-08  1094  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()

[Christophe JAILLET]

Le 26/08/2023 à 22:00, kernel test robot a écrit :
> Hi Christophe,
> 
> kernel test robot noticed the following build warnings:
> 
> [auto build test WARNING on linux-next/master]
> [cannot apply to linus/master v6.5-rc7]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/HID-nvidia-shield-Fix-a-missing-led_classdev_unregister-in-the-probe-error-handling-path/20230827-014602
> base:   linux-next/master
> patch link:    https://lore.kernel.org/r/4c9a8c7f6b4eb879dd7ef4d44bb6a80b3f126d25.1693070958.git.christophe.jaillet%40wanadoo.fr
> patch subject: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
> config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20230827/202308270307.EDe7t62T-lkp@intel.com/config)
> compiler: hppa-linux-gcc (GCC) 13.2.0

On x86_64, gcc 12.3.0 does not complain. :(

Let see first if there is some comment on the serie, then I'll send a v2 
to fix the warning.

CJ

> reproduce: (https://download.01.org/0day-ci/archive/20230827/202308270307.EDe7t62T-lkp@intel.com/reproduce)
> 
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202308270307.EDe7t62T-lkp@intel.com/
> 
> All warnings (new ones prefixed by >>):
> 
>     drivers/hid/hid-nvidia-shield.c: In function 'shield_probe':
>>> drivers/hid/hid-nvidia-shield.c:1046:31: warning: variable 'ts' set but not used [-Wunused-but-set-variable]
>      1046 |         struct thunderstrike *ts;
>           |                               ^~
> 
> 
> vim +/ts +1046 drivers/hid/hid-nvidia-shield.c
> 
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1042
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1043  static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1044  {
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1045  	struct shield_device *shield_dev = NULL;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08 @1046  	struct thunderstrike *ts;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1047  	int ret;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1048
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1049  	ret = hid_parse(hdev);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1050  	if (ret) {
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1051  		hid_err(hdev, "Parse failed\n");
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1052  		return ret;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1053  	}
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1054
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1055  	switch (id->product) {
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1056  	case USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER:
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1057  		shield_dev = thunderstrike_create(hdev);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1058  		break;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1059  	}
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1060
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1061  	if (unlikely(!shield_dev)) {
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1062  		hid_err(hdev, "Failed to identify SHIELD device\n");
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1063  		return -ENODEV;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1064  	}
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1065  	if (IS_ERR(shield_dev)) {
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1066  		hid_err(hdev, "Failed to create SHIELD device\n");
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1067  		return PTR_ERR(shield_dev);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1068  	}
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1069
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1070  	ts = container_of(shield_dev, struct thunderstrike, base);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1071
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1072  	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1073  	if (ret) {
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1074  		hid_err(hdev, "Failed to start HID device\n");
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1075  		goto err_haptics;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1076  	}
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1077
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1078  	ret = hid_hw_open(hdev);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1079  	if (ret) {
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1080  		hid_err(hdev, "Failed to open HID device\n");
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1081  		goto err_stop;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1082  	}
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1083
> 3ab196f882377ed Rahul Rameshbabu   2023-08-07  1084  	thunderstrike_device_init_info(shield_dev);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1085
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1086  	return ret;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1087
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1088  err_stop:
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1089  	hid_hw_stop(hdev);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1090  err_haptics:
> 2cc4637842495c6 Christophe JAILLET 2023-08-26  1091  	thunderstrike_destroy(hdev);
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1092  	return ret;
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1093  }
> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1094
> 

Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()

[kernel test robot]

Hi Christophe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linux-next/master]
[cannot apply to linus/master v6.5-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/HID-nvidia-shield-Fix-a-missing-led_classdev_unregister-in-the-probe-error-handling-path/20230827-014602
base:   linux-next/master
patch link:    https://lore.kernel.org/r/4c9a8c7f6b4eb879dd7ef4d44bb6a80b3f126d25.1693070958.git.christophe.jaillet%40wanadoo.fr
patch subject: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
config: i386-buildonly-randconfig-003-20230827 (https://download.01.org/0day-ci/archive/20230827/202308270516.Ch4MucBs-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce: (https://download.01.org/0day-ci/archive/20230827/202308270516.Ch4MucBs-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202308270516.Ch4MucBs-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/hid/hid-nvidia-shield.c:1046:24: warning: variable 'ts' set but not used [-Wunused-but-set-variable]
           struct thunderstrike *ts;
                                 ^
   1 warning generated.


vim +/ts +1046 drivers/hid/hid-nvidia-shield.c

09308562d4afb1 Rahul Rameshbabu   2023-06-08  1042  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1043  static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1044  {
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1045  	struct shield_device *shield_dev = NULL;
09308562d4afb1 Rahul Rameshbabu   2023-06-08 @1046  	struct thunderstrike *ts;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1047  	int ret;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1048  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1049  	ret = hid_parse(hdev);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1050  	if (ret) {
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1051  		hid_err(hdev, "Parse failed\n");
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1052  		return ret;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1053  	}
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1054  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1055  	switch (id->product) {
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1056  	case USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER:
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1057  		shield_dev = thunderstrike_create(hdev);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1058  		break;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1059  	}
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1060  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1061  	if (unlikely(!shield_dev)) {
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1062  		hid_err(hdev, "Failed to identify SHIELD device\n");
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1063  		return -ENODEV;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1064  	}
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1065  	if (IS_ERR(shield_dev)) {
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1066  		hid_err(hdev, "Failed to create SHIELD device\n");
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1067  		return PTR_ERR(shield_dev);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1068  	}
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1069  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1070  	ts = container_of(shield_dev, struct thunderstrike, base);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1071  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1072  	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1073  	if (ret) {
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1074  		hid_err(hdev, "Failed to start HID device\n");
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1075  		goto err_haptics;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1076  	}
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1077  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1078  	ret = hid_hw_open(hdev);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1079  	if (ret) {
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1080  		hid_err(hdev, "Failed to open HID device\n");
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1081  		goto err_stop;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1082  	}
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1083  
3ab196f882377e Rahul Rameshbabu   2023-08-07  1084  	thunderstrike_device_init_info(shield_dev);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1085  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1086  	return ret;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1087  
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1088  err_stop:
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1089  	hid_hw_stop(hdev);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1090  err_haptics:
2cc4637842495c Christophe JAILLET 2023-08-26  1091  	thunderstrike_destroy(hdev);
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1092  	return ret;
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1093  }
09308562d4afb1 Rahul Rameshbabu   2023-06-08  1094  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()

[Rahul Rameshbabu]

On Sat, 26 Aug, 2023 23:13:01 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> wrote:
> Le 26/08/2023 à 22:00, kernel test robot a écrit :
>> Hi Christophe,
>> kernel test robot noticed the following build warnings:
>> [auto build test WARNING on linux-next/master]
>> [cannot apply to linus/master v6.5-rc7]
>> [If your patch is applied to the wrong git tree, kindly drop us a note.
>> And when submitting patch, we suggest to use '--base' as documented in
>> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>> url:
>> https://github.com/intel-lab-lkp/linux/commits/Christophe-JAILLET/HID-nvidia-shield-Fix-a-missing-led_classdev_unregister-in-the-probe-error-handling-path/20230827-014602
>> base:   linux-next/master
>> patch link:    https://lore.kernel.org/r/4c9a8c7f6b4eb879dd7ef4d44bb6a80b3f126d25.1693070958.git.christophe.jaillet%40wanadoo.fr
>> patch subject: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
>> config: parisc-allyesconfig (https://download.01.org/0day-ci/archive/20230827/202308270307.EDe7t62T-lkp@intel.com/config)
>> compiler: hppa-linux-gcc (GCC) 13.2.0
>
> On x86_64, gcc 12.3.0 does not complain. :(

The key to getting gcc to spew the warning is passing W=1 for the make
variable to get the compiler to invoke the unused warning.

  [nix-shell:~/Documents/linux]$ make help | grep -i W=
   make W=n   [targets] Enable extra build checks, n=1,2,3 where
 		Multiple levels can be combined with W=12 or W=123

  linux on  cjaillet/shield-fixup [?] via ❄️  impure (linux-6.1.31)
  ❯ make -C $dev/lib/modules/*/build M=$(pwd)/drivers/hid W=1 modules
    CC [M]  /home/binary-eater/Documents/linux/drivers/hid/hid-nvidia-shield.o
  /home/binary-eater/Documents/linux/drivers/hid/hid-nvidia-shield.c: In function ‘shield_probe’:
  /home/binary-eater/Documents/linux/drivers/hid/hid-nvidia-shield.c:1046:31: warning: variable ‘ts’ set but not used [-Wunused-but-set-variable]
   1046 |         struct thunderstrike *ts;
        |                               ^~
    MODPOST /home/binary-eater/Documents/linux/drivers/hid/Module.symvers
    CC [M]  /home/binary-eater/Documents/linux/drivers/hid/hid-nvidia-shield.mod.o
    LD [M]  /home/binary-eater/Documents/linux/drivers/hid/hid-nvidia-shield.ko
    BTF [M] /home/binary-eater/Documents/linux/drivers/hid/hid-nvidia-shield.ko

  linux on  cjaillet/shield-fixup [?] via ❄️  impure (linux-6.1.31) took 4s
  ❯ gcc --version
  gcc (GCC) 12.2.0
  Copyright (C) 2022 Free Software Foundation, Inc.
  This is free software; see the source for copying conditions.  There is NO
  warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Hope this is helpful.

-- Rahul Rameshbabu

>
> Let see first if there is some comment on the serie, then I'll send a v2 to fix
> the warning.
>
> CJ
>
>> reproduce: (https://download.01.org/0day-ci/archive/20230827/202308270307.EDe7t62T-lkp@intel.com/reproduce)
>> If you fix the issue in a separate patch/commit (i.e. not just a new version
>> of
>> the same patch/commit), kindly add following tags
>> | Reported-by: kernel test robot <lkp@intel.com>
>> | Closes: https://lore.kernel.org/oe-kbuild-all/202308270307.EDe7t62T-lkp@intel.com/
>> All warnings (new ones prefixed by >>):
>>     drivers/hid/hid-nvidia-shield.c: In function 'shield_probe':
>>>> drivers/hid/hid-nvidia-shield.c:1046:31: warning: variable 'ts' set but not used [-Wunused-but-set-variable]
>>      1046 |         struct thunderstrike *ts;
>>           |                               ^~
>> vim +/ts +1046 drivers/hid/hid-nvidia-shield.c
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1042
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1043  static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1044  {
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1045  	struct shield_device *shield_dev = NULL;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08 @1046  	struct thunderstrike *ts;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1047  	int ret;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1048
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1049  	ret = hid_parse(hdev);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1050  	if (ret) {
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1051  		hid_err(hdev, "Parse failed\n");
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1052  		return ret;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1053  	}
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1054
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1055  	switch (id->product) {
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1056  	case USB_DEVICE_ID_NVIDIA_THUNDERSTRIKE_CONTROLLER:
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1057  		shield_dev = thunderstrike_create(hdev);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1058  		break;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1059  	}
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1060
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1061  	if (unlikely(!shield_dev)) {
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1062  		hid_err(hdev, "Failed to identify SHIELD device\n");
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1063  		return -ENODEV;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1064  	}
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1065  	if (IS_ERR(shield_dev)) {
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1066  		hid_err(hdev, "Failed to create SHIELD device\n");
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1067  		return PTR_ERR(shield_dev);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1068  	}
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1069
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1070  	ts = container_of(shield_dev, struct thunderstrike, base);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1071
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1072  	ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1073  	if (ret) {
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1074  		hid_err(hdev, "Failed to start HID device\n");
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1075  		goto err_haptics;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1076  	}
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1077
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1078  	ret = hid_hw_open(hdev);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1079  	if (ret) {
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1080  		hid_err(hdev, "Failed to open HID device\n");
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1081  		goto err_stop;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1082  	}
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1083
>> 3ab196f882377ed Rahul Rameshbabu   2023-08-07  1084  	thunderstrike_device_init_info(shield_dev);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1085
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1086  	return ret;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1087
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1088  err_stop:
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1089  	hid_hw_stop(hdev);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1090  err_haptics:
>> 2cc4637842495c6 Christophe JAILLET 2023-08-26  1091  	thunderstrike_destroy(hdev);
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1092  	return ret;
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1093  }
>> 09308562d4afb1a Rahul Rameshbabu   2023-06-08  1094
>> 

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

[Rahul Rameshbabu]

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.
>

I really appreciate the contribution.

>
> *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.

I agree with this proposal. Let's clean this up in the patch that
implements thunderstrike_destroy. The order change in 3ab196f88237 is
more a matter of sloppiness rather than due to handling some functional
side effect that requires the order change.

>
> 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(-)

--
Thanks,

Rahul Rameshbabu

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@wanadoo.fr> 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@nvidia.com/

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@wanadoo.fr>
> ---
>  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

Re: [PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() in the probe error handling path

[Rahul Rameshbabu]

On Sat, 26 Aug, 2023 19:42:18 +0200 Christophe JAILLET <christophe.jaillet@wanadoo.fr> 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 calls.
>
> Fixes: 3ab196f88237 ("HID: nvidia-shield: Add battery support for Thunderstrike")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
>  drivers/hid/hid-nvidia-shield.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
> index 66a7478e2c9d..849b3f8409a0 100644
> --- a/drivers/hid/hid-nvidia-shield.c
> +++ b/drivers/hid/hid-nvidia-shield.c
> @@ -1074,9 +1074,11 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  err_stop:
>  	hid_hw_stop(hdev);
>  err_haptics:

Functionally, the change looks good to me. Can we update this label from
err_haptics to err_ts_create? Because of the label name, I accidentally
forgot to add other cleanup in this context originally... (Rust borrow
checker please rescue me)

> +	power_supply_unregister(ts->base.battery_dev.psy);
>  	if (ts->haptics_dev)
>  		input_unregister_device(ts->haptics_dev);
>  	led_classdev_unregister(&ts->led_dev);
> +	ida_free(&thunderstrike_ida, ts->id);
>  	return ret;
>  }

--
Thanks,

Rahul Rameshbabu

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

[Rahul Rameshbabu]

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.

--
Thanks,

Rahul Rameshbabu

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

[Christophe JAILLET]

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.

CJ

> 
> --
> Thanks,
> 
> Rahul Rameshbabu
> 

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

[Rahul Rameshbabu]

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.

--
Thanks for the patches,

Rahul Rameshbabu

>
> CJ
>
>> --
>> Thanks,
>> Rahul Rameshbabu
>> 

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

[Christophe JAILLET]

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
>>>
>