* [PATCH] HID: wacom: Fix wacom_probe error handling
@ 2012-09-17 2:29 Axel Lin
2012-09-17 8:28 ` Przemo Firszt
0 siblings, 1 reply; 4+ messages in thread
From: Axel Lin @ 2012-09-17 2:29 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Przemo Firszt, Ping Cheng, linux-input
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 <axel.lin@gmail.com>
---
drivers/hid/hid-wacom.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
index a0fafac..fae7fd7 100644
--- a/drivers/hid/hid-wacom.c
+++ b/drivers/hid/hid-wacom.c
@@ -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;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] HID: wacom: Fix wacom_probe error handling
2012-09-17 2:29 [PATCH] HID: wacom: Fix wacom_probe error handling Axel Lin
@ 2012-09-17 8:28 ` Przemo Firszt
2012-09-17 8:39 ` Axel Lin
0 siblings, 1 reply; 4+ messages in thread
From: Przemo Firszt @ 2012-09-17 8:28 UTC (permalink / raw)
To: Axel Lin; +Cc: Jiri Kosina, Przemo Firszt, Ping Cheng, linux-input
Dnia 17 Września 2012, 3:29 am, Pn, Axel Lin napisał(a):
> 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 <axel.lin@gmail.com>
> ---
> drivers/hid/hid-wacom.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
> index a0fafac..fae7fd7 100644
> --- a/drivers/hid/hid-wacom.c
> +++ b/drivers/hid/hid-wacom.c
> @@ -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;
> --
> 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?
--
Regards,
Przemo Firszt
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] HID: wacom: Fix wacom_probe error handling
2012-09-17 8:28 ` Przemo Firszt
@ 2012-09-17 8:39 ` Axel Lin
2012-09-18 22:12 ` Przemo Firszt
0 siblings, 1 reply; 4+ messages in thread
From: Axel Lin @ 2012-09-17 8:39 UTC (permalink / raw)
To: przemo; +Cc: Jiri Kosina, Ping Cheng, linux-input
2012/9/17 Przemo Firszt <przemo@firszt.eu>:
>
> Dnia 17 Września 2012, 3:29 am, Pn, Axel Lin napisał(a):
>> 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 <axel.lin@gmail.com>
>> ---
>> drivers/hid/hid-wacom.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
>> index a0fafac..fae7fd7 100644
>> --- a/drivers/hid/hid-wacom.c
>> +++ b/drivers/hid/hid-wacom.c
>> @@ -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;
>> --
>> 1.7.9.5
> Hi Alex,
It's Axel.
> 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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] HID: wacom: Fix wacom_probe error handling
2012-09-17 8:39 ` Axel Lin
@ 2012-09-18 22:12 ` Przemo Firszt
0 siblings, 0 replies; 4+ messages in thread
From: Przemo Firszt @ 2012-09-18 22:12 UTC (permalink / raw)
To: axel.lin; +Cc: Jiri Kosina, Ping Cheng, linux-input
On Mon, 2012-09-17 at 16:39 +0800, Axel Lin wrote:
> 2012/9/17 Przemo Firszt <przemo@firszt.eu>:
> >
> > Dnia 17 Września 2012, 3:29 am, Pn, Axel Lin napisał(a):
> >> 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 <axel.lin@gmail.com>
> >> ---
> >> drivers/hid/hid-wacom.c | 12 ++++++++++--
> >> 1 file changed, 10 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/hid/hid-wacom.c b/drivers/hid/hid-wacom.c
> >> index a0fafac..fae7fd7 100644
> >> --- a/drivers/hid/hid-wacom.c
> >> +++ b/drivers/hid/hid-wacom.c
> >> @@ -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;
> >> --
> >> 1.7.9.5
> > Hi Alex,
> It's Axel.
Hi Axel,
I'm really sorry for that mistake :-(
> > 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?
>
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 majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-09-18 22:12 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-17 2:29 [PATCH] HID: wacom: Fix wacom_probe error handling Axel Lin
2012-09-17 8:28 ` Przemo Firszt
2012-09-17 8:39 ` Axel Lin
2012-09-18 22:12 ` Przemo Firszt
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.