Skip to contentSkip to conversation

HID: wacom: Fix wacom_probe error handling

PATCH Superseded Axel Lin 2 participants 4 messages Sep 17, 2012 – Sep 18, 2012 HID

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Axel Lin

author
#0 lore raw

PATCH 1/1 HID: wacom: Fix wacom_probe error handling

In the case of goto destroy_leds, we also need to call hid_hw_stop(hdev) and remove sysfs attribute files we created for speed and oleds.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/hid/hid-wacom.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/hid/hid-wacom.c +10 -2
index a0fafac..fae7fd7 100644
@@ -902,10 +902,18 @@ OLED_INIT(7);
 err_ac:
 	power_supply_unregister(&wdata->battery);
 err_battery:
-	device_remove_file(&hdev->dev, &dev_attr_speed);
-	hid_hw_stop(hdev);
 destroy_leds:
 	wacom_destroy_leds(hdev);
+	device_remove_file(&hdev->dev, &dev_attr_oled0_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled1_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled2_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled3_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled4_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled5_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled6_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled7_img);
+	device_remove_file(&hdev->dev, &dev_attr_speed);
+	hid_hw_stop(hdev);
 err_free:
 	kfree(wdata);
 	return ret;
Signature · 1 line
1.7.9.5
#1 lore raw

Dnia 17 Września 2012, 3:29 am, Pn, Axel Lin napisał(a):

Quoted 35 lines

In the case of goto destroy_leds, we also need to call hid_hw_stop(hdev) and
remove sysfs attribute files we created for speed and oleds.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/hid/hid-wacom.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/hid/hid-wacom.c +10 -3
index a0fafac..fae7fd7 100644
@@ -902,10 +902,18 @@ OLED_INIT(7);
 err_ac:
 	power_supply_unregister(&wdata->battery);
 err_battery:
-	device_remove_file(&hdev->dev, &dev_attr_speed);
-	hid_hw_stop(hdev);
 destroy_leds:
 	wacom_destroy_leds(hdev);
+	device_remove_file(&hdev->dev, &dev_attr_oled0_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled1_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled2_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled3_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled4_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled5_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled6_img);
+	device_remove_file(&hdev->dev, &dev_attr_oled7_img);
+	device_remove_file(&hdev->dev, &dev_attr_speed);
+	hid_hw_stop(hdev);
 err_free:
 	kfree(wdata);
 	return ret;
--
Quoted 1 line

1.7.9.5

Hi Alex,
The initial idea was that the OLEDs/LED are not critical for tablet functioning and that's why there was no call of hid_stop_hw if the OLED/LED initialisation faild.
What's is the general kernel/HID policy: is it OK to have a device working with some non-essential features missing?

Signature · 7 lines
Regards,
Przemo Firszt

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#2 lore raw

2012/9/17 Przemo Firszt <[email protected]>:

Quoted 31 lines

Dnia 17 Września 2012, 3:29 am, Pn, Axel Lin napisał(a):

Quoted 29 lines

In the case of goto destroy_leds, we also need to call hid_hw_stop(hdev) and
remove sysfs attribute files we created for speed and oleds.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/hid/hid-wacom.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/hid/hid-wacom.c +8 -2
index a0fafac..fae7fd7 100644
@@ -902,10 +902,18 @@ OLED_INIT(7);
 err_ac:
      power_supply_unregister(&wdata->battery);
 err_battery:
-     device_remove_file(&hdev->dev, &dev_attr_speed);
-     hid_hw_stop(hdev);
 destroy_leds:
      wacom_destroy_leds(hdev);
+     device_remove_file(&hdev->dev, &dev_attr_oled0_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled1_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled2_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled3_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled4_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled5_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled6_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled7_img);
Quoted 8 lines
Quoted 7 lines
drivers/hid/hid-wacom.c +2 -1
+     device_remove_file(&hdev->dev, &dev_attr_speed);
+     hid_hw_stop(hdev);
 err_free:
      kfree(wdata);
      return ret;
--

1.7.9.5

Hi Alex,

It's Axel.

Quoted 3 lines

The initial idea was that the OLEDs/LED are not critical for tablet functioning and that's why there was no call of hid_stop_hw if the OLED/LED initialisation faild.

Since it is not critical, current code ignore checking return value when device_create_file for speed and oleds.

But current code does goto destroy_leds if wacom_initialize_leds fails. Which means it returns error code in wacom_probe() and thus it should clean up all the allocated resources in the error patch.

If you want to ignore wacom_initialize_leds() failure, you should not call goto destroy_leds.

Comment?

Axel
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#3 lore raw
Quoted 40 lines · Axel Lin

2012/9/17 Przemo Firszt <[email protected]>:

Quoted 31 lines

Dnia 17 Września 2012, 3:29 am, Pn, Axel Lin napisał(a):

Quoted 29 lines

In the case of goto destroy_leds, we also need to call hid_hw_stop(hdev) and
remove sysfs attribute files we created for speed and oleds.

Signed-off-by Axel Lin <[email protected]>
---
 drivers/hid/hid-wacom.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/hid/hid-wacom.c +8 -2
index a0fafac..fae7fd7 100644
@@ -902,10 +902,18 @@ OLED_INIT(7);
 err_ac:
      power_supply_unregister(&wdata->battery);
 err_battery:
-     device_remove_file(&hdev->dev, &dev_attr_speed);
-     hid_hw_stop(hdev);
 destroy_leds:
      wacom_destroy_leds(hdev);
+     device_remove_file(&hdev->dev, &dev_attr_oled0_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled1_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled2_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled3_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled4_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled5_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled6_img);
+     device_remove_file(&hdev->dev, &dev_attr_oled7_img);
Quoted 8 lines
Quoted 7 lines
drivers/hid/hid-wacom.c +2 -1
+     device_remove_file(&hdev->dev, &dev_attr_speed);
+     hid_hw_stop(hdev);
 err_free:
      kfree(wdata);
      return ret;
--

1.7.9.5

Hi Alex,

Quoted 1 line

It's Axel.

Hi Axel,
I'm really sorry for that mistake :-(

Quoted 7 lines
Quoted 3 lines

The initial idea was that the OLEDs/LED are not critical for tablet functioning and that's why there was no call of hid_stop_hw if the OLED/LED initialisation faild.

Since it is not critical, current code ignore checking return value when device_create_file for speed and oleds.

Quoted 9 lines

But current code does goto destroy_leds if wacom_initialize_leds fails. Which means it returns error code in wacom_probe() and thus it should clean up all the allocated resources in the error patch.

If you want to ignore wacom_initialize_leds() failure, you should not call goto destroy_leds.

Comment?

See my reply to the v2 patch. Thanks for catching that! --
Kind regards,
Przemo

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html