* [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
@ 2023-08-26 17:42 Christophe JAILLET
2023-08-26 17:42 ` [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path Christophe JAILLET
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Christophe JAILLET @ 2023-08-26 17:42 UTC (permalink / raw)
To: rrameshbabu, jikos, benjamin.tissoires
Cc: linux-input, linux-kernel, kernel-janitors, 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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path
2023-08-26 17:42 [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Christophe JAILLET
@ 2023-08-26 17:42 ` Christophe JAILLET
2023-08-27 19:41 ` Rahul Rameshbabu
2023-08-26 17:42 ` [PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() " Christophe JAILLET
` (3 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Christophe JAILLET @ 2023-08-26 17:42 UTC (permalink / raw)
To: rrameshbabu, jikos, benjamin.tissoires
Cc: linux-input, linux-kernel, kernel-janitors, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() in the probe error handling path
2023-08-26 17:42 [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Christophe JAILLET
2023-08-26 17:42 ` [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path Christophe JAILLET
@ 2023-08-26 17:42 ` Christophe JAILLET
2023-08-27 19:42 ` Rahul Rameshbabu
2023-08-26 17:42 ` [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy() Christophe JAILLET
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Christophe JAILLET @ 2023-08-26 17:42 UTC (permalink / raw)
To: rrameshbabu, jikos, benjamin.tissoires
Cc: linux-input, linux-kernel, kernel-janitors, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
2023-08-26 17:42 [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Christophe JAILLET
2023-08-26 17:42 ` [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path Christophe JAILLET
2023-08-26 17:42 ` [PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() " Christophe JAILLET
@ 2023-08-26 17:42 ` Christophe JAILLET
2023-08-26 20:00 ` kernel test robot
2023-08-26 21:43 ` kernel test robot
2023-08-27 19:41 ` [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Rahul Rameshbabu
2023-09-15 18:16 ` Rahul Rameshbabu
4 siblings, 2 replies; 15+ messages in thread
From: Christophe JAILLET @ 2023-08-26 17:42 UTC (permalink / raw)
To: rrameshbabu, jikos, benjamin.tissoires
Cc: linux-input, linux-kernel, kernel-janitors, 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
2023-08-26 17:42 ` [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy() Christophe JAILLET
@ 2023-08-26 20:00 ` kernel test robot
2023-08-26 21:13 ` Christophe JAILLET
2023-08-26 21:43 ` kernel test robot
1 sibling, 1 reply; 15+ messages in thread
From: kernel test robot @ 2023-08-26 20:00 UTC (permalink / raw)
To: Christophe JAILLET, rrameshbabu, jikos, benjamin.tissoires
Cc: oe-kbuild-all, linux-input, linux-kernel, kernel-janitors,
Christophe JAILLET
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
2023-08-26 20:00 ` kernel test robot
@ 2023-08-26 21:13 ` Christophe JAILLET
2023-08-27 19:41 ` Rahul Rameshbabu
0 siblings, 1 reply; 15+ messages in thread
From: Christophe JAILLET @ 2023-08-26 21:13 UTC (permalink / raw)
To: kernel test robot, rrameshbabu, jikos, benjamin.tissoires
Cc: oe-kbuild-all, linux-input, linux-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
2023-08-26 17:42 ` [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy() Christophe JAILLET
2023-08-26 20:00 ` kernel test robot
@ 2023-08-26 21:43 ` kernel test robot
1 sibling, 0 replies; 15+ messages in thread
From: kernel test robot @ 2023-08-26 21:43 UTC (permalink / raw)
To: Christophe JAILLET, rrameshbabu, jikos, benjamin.tissoires
Cc: llvm, oe-kbuild-all, linux-input, linux-kernel, kernel-janitors,
Christophe JAILLET
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy()
2023-08-26 21:13 ` Christophe JAILLET
@ 2023-08-27 19:41 ` Rahul Rameshbabu
0 siblings, 0 replies; 15+ messages in thread
From: Rahul Rameshbabu @ 2023-08-27 19:41 UTC (permalink / raw)
To: Christophe JAILLET
Cc: kernel test robot, jikos, benjamin.tissoires, oe-kbuild-all,
linux-input, linux-kernel, kernel-janitors
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
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
2023-08-26 17:42 [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Christophe JAILLET
` (2 preceding siblings ...)
2023-08-26 17:42 ` [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy() Christophe JAILLET
@ 2023-08-27 19:41 ` Rahul Rameshbabu
2023-09-15 18:16 ` Rahul Rameshbabu
4 siblings, 0 replies; 15+ messages in thread
From: Rahul Rameshbabu @ 2023-08-27 19:41 UTC (permalink / raw)
To: Christophe JAILLET
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path
2023-08-26 17:42 ` [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path Christophe JAILLET
@ 2023-08-27 19:41 ` Rahul Rameshbabu
0 siblings, 0 replies; 15+ messages in thread
From: Rahul Rameshbabu @ 2023-08-27 19:41 UTC (permalink / raw)
To: Christophe JAILLET
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() in the probe error handling path
2023-08-26 17:42 ` [PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() " Christophe JAILLET
@ 2023-08-27 19:42 ` Rahul Rameshbabu
0 siblings, 0 replies; 15+ messages in thread
From: Rahul Rameshbabu @ 2023-08-27 19:42 UTC (permalink / raw)
To: Christophe JAILLET
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
2023-08-26 17:42 [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Christophe JAILLET
` (3 preceding siblings ...)
2023-08-27 19:41 ` [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Rahul Rameshbabu
@ 2023-09-15 18:16 ` Rahul Rameshbabu
2023-09-15 20:14 ` Christophe JAILLET
4 siblings, 1 reply; 15+ messages in thread
From: Rahul Rameshbabu @ 2023-09-15 18:16 UTC (permalink / raw)
To: Christophe JAILLET
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, kernel-janitors
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
2023-09-15 18:16 ` Rahul Rameshbabu
@ 2023-09-15 20:14 ` Christophe JAILLET
2023-09-15 20:51 ` Rahul Rameshbabu
0 siblings, 1 reply; 15+ messages in thread
From: Christophe JAILLET @ 2023-09-15 20:14 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, kernel-janitors
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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
2023-09-15 20:14 ` Christophe JAILLET
@ 2023-09-15 20:51 ` Rahul Rameshbabu
2023-09-17 20:37 ` Christophe JAILLET
0 siblings, 1 reply; 15+ messages in thread
From: Rahul Rameshbabu @ 2023-09-15 20:51 UTC (permalink / raw)
To: Christophe JAILLET
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, kernel-janitors
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
>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe()
2023-09-15 20:51 ` Rahul Rameshbabu
@ 2023-09-17 20:37 ` Christophe JAILLET
0 siblings, 0 replies; 15+ messages in thread
From: Christophe JAILLET @ 2023-09-17 20:37 UTC (permalink / raw)
To: Rahul Rameshbabu
Cc: jikos, benjamin.tissoires, linux-input, linux-kernel, kernel-janitors
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
>>>
>
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2023-09-17 20:38 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-26 17:42 [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Christophe JAILLET
2023-08-26 17:42 ` [PATCH 1/3] HID: nvidia-shield: Fix a missing led_classdev_unregister() in the probe error handling path Christophe JAILLET
2023-08-27 19:41 ` Rahul Rameshbabu
2023-08-26 17:42 ` [PATCH 2/3] HID: nvidia-shield: Fix some missing function calls() " Christophe JAILLET
2023-08-27 19:42 ` Rahul Rameshbabu
2023-08-26 17:42 ` [PATCH 3/3] HID: nvidia-shield: Introduce thunderstrike_destroy() Christophe JAILLET
2023-08-26 20:00 ` kernel test robot
2023-08-26 21:13 ` Christophe JAILLET
2023-08-27 19:41 ` Rahul Rameshbabu
2023-08-26 21:43 ` kernel test robot
2023-08-27 19:41 ` [PATCH 0/3] HID: nvidia-shield: Fix the error handling path of shield_probe() Rahul Rameshbabu
2023-09-15 18:16 ` Rahul Rameshbabu
2023-09-15 20:14 ` Christophe JAILLET
2023-09-15 20:51 ` Rahul Rameshbabu
2023-09-17 20:37 ` Christophe JAILLET
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).