* [PATCH] HID: force setting drvdata to NULL when removing the driver
@ 2019-03-28 15:51 Benjamin Tissoires
2019-03-29 22:26 ` Dmitry Torokhov
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2019-03-28 15:51 UTC (permalink / raw)
To: Jiri Kosina, Bruno Prémont, Jonathan Cameron,
Srinivas Pandruvada, Hans de Goede, Jason Gerecke, Ping Cheng
Cc: linux-input, linux-kernel, Benjamin Tissoires
Or when the probe failed.
This is a common pattern in the HID drivers to reset the drvdata. Some
do it properly, some do it only in case of failure.
Anyway, it's never a good thing to have breadcrumbs, so force a clean
state when removing or when probe is failing.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-core.c | 2 ++
drivers/hid/hid-cougar.c | 6 ++----
drivers/hid/hid-gfrm.c | 7 -------
drivers/hid/hid-lenovo.c | 2 --
drivers/hid/hid-logitech-dj.c | 2 --
drivers/hid/hid-logitech-hidpp.c | 8 +++-----
drivers/hid/hid-picolcd_core.c | 7 +------
drivers/hid/hid-sensor-hub.c | 1 -
drivers/hid/wacom_sys.c | 18 +++++-------------
9 files changed, 13 insertions(+), 40 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b5a8af8daa74..011e49da4d63 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2203,6 +2203,7 @@ static int hid_device_probe(struct device *dev)
if (ret) {
hid_close_report(hdev);
hdev->driver = NULL;
+ hid_set_drvdata(hdev, NULL);
}
}
unlock:
@@ -2231,6 +2232,7 @@ static int hid_device_remove(struct device *dev)
else /* default remove */
hid_hw_stop(hdev);
hid_close_report(hdev);
+ hid_set_drvdata(hdev, NULL);
hdev->driver = NULL;
}
diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
index e0bb7b34f3a4..4ff3bc1d25e2 100644
--- a/drivers/hid/hid-cougar.c
+++ b/drivers/hid/hid-cougar.c
@@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
error = hid_parse(hdev);
if (error) {
hid_err(hdev, "parse failed\n");
- goto fail;
+ return error;
}
if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
@@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
error = hid_hw_start(hdev, connect_mask);
if (error) {
hid_err(hdev, "hw start failed\n");
- goto fail;
+ return error;
}
error = cougar_bind_shared_data(hdev, cougar);
@@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
fail_stop_and_cleanup:
hid_hw_stop(hdev);
-fail:
- hid_set_drvdata(hdev, NULL);
return error;
}
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index cf477f8c8f4c..1a20997e7750 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -127,12 +127,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
-static void gfrm_remove(struct hid_device *hdev)
-{
- hid_hw_stop(hdev);
- hid_set_drvdata(hdev, NULL);
-}
-
static const struct hid_device_id gfrm_devices[] = {
{ HID_BLUETOOTH_DEVICE(0x58, 0x2000),
.driver_data = GFRM100 },
@@ -146,7 +140,6 @@ static struct hid_driver gfrm_driver = {
.name = "gfrm",
.id_table = gfrm_devices,
.probe = gfrm_probe,
- .remove = gfrm_remove,
.input_mapping = gfrm_input_mapping,
.raw_event = gfrm_raw_event,
.input_configured = gfrm_input_configured,
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index eacc76d2ab96..cde9d22bb3ec 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -869,8 +869,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
led_classdev_unregister(&data_pointer->led_micmute);
led_classdev_unregister(&data_pointer->led_mute);
-
- hid_set_drvdata(hdev, NULL);
}
static void lenovo_remove_cptkbd(struct hid_device *hdev)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 826fa1e1c8d9..a75101293755 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1094,7 +1094,6 @@ static int logi_dj_probe(struct hid_device *hdev,
hid_parse_fail:
kfifo_free(&djrcv_dev->notif_fifo);
kfree(djrcv_dev);
- hid_set_drvdata(hdev, NULL);
return retval;
}
@@ -1145,7 +1144,6 @@ static void logi_dj_remove(struct hid_device *hdev)
kfifo_free(&djrcv_dev->notif_fifo);
kfree(djrcv_dev);
- hid_set_drvdata(hdev, NULL);
}
static const struct hid_device_id logi_dj_receivers[] = {
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 199cc256e9d9..b950a44157a2 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3237,15 +3237,15 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
ret = wtp_allocate(hdev, id);
if (ret)
- goto allocate_fail;
+ return ret;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
ret = m560_allocate(hdev);
if (ret)
- goto allocate_fail;
+ return ret;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
ret = k400_allocate(hdev);
if (ret)
- goto allocate_fail;
+ return ret;
}
INIT_WORK(&hidpp->work, delayed_work_cb);
@@ -3343,8 +3343,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
cancel_work_sync(&hidpp->work);
mutex_destroy(&hidpp->send_mutex);
-allocate_fail:
- hid_set_drvdata(hdev, NULL);
return ret;
}
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index c1b29a9eb41a..a5d30f267a7b 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -550,8 +550,7 @@ static int picolcd_probe(struct hid_device *hdev,
data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
if (data == NULL) {
hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
- error = -ENOMEM;
- goto err_no_cleanup;
+ return -ENOMEM;
}
spin_lock_init(&data->lock);
@@ -613,9 +612,6 @@ static int picolcd_probe(struct hid_device *hdev,
hid_hw_stop(hdev);
err_cleanup_data:
kfree(data);
-err_no_cleanup:
- hid_set_drvdata(hdev, NULL);
-
return error;
}
@@ -651,7 +647,6 @@ static void picolcd_remove(struct hid_device *hdev)
picolcd_exit_cir(data);
picolcd_exit_keys(data);
- hid_set_drvdata(hdev, NULL);
mutex_destroy(&data->mutex);
/* Finally, clean up the picolcd data itself */
kfree(data);
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 4256fdc5cd6d..a3db5d8b382d 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -755,7 +755,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
}
spin_unlock_irqrestore(&data->lock, flags);
mfd_remove_devices(&hdev->dev);
- hid_set_drvdata(hdev, NULL);
mutex_destroy(&data->mutex);
}
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index a8633b1437b2..c2fb614bff44 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2716,14 +2716,12 @@ static int wacom_probe(struct hid_device *hdev,
wacom_wac->features = *((struct wacom_features *)id->driver_data);
features = &wacom_wac->features;
- if (features->check_for_hid_type && features->hid_type != hdev->type) {
- error = -ENODEV;
- goto fail;
- }
+ if (features->check_for_hid_type && features->hid_type != hdev->type)
+ return -ENODEV;
error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
if (error)
- goto fail;
+ return error;
wacom_wac->hid_data.inputmode = -1;
wacom_wac->mode_report = -1;
@@ -2741,12 +2739,12 @@ static int wacom_probe(struct hid_device *hdev,
error = hid_parse(hdev);
if (error) {
hid_err(hdev, "parse failed\n");
- goto fail;
+ return error;
}
error = wacom_parse_and_register(wacom, false);
if (error)
- goto fail;
+ return error;
if (hdev->bus == BUS_BLUETOOTH) {
error = device_create_file(&hdev->dev, &dev_attr_speed);
@@ -2757,10 +2755,6 @@ static int wacom_probe(struct hid_device *hdev,
}
return 0;
-
-fail:
- hid_set_drvdata(hdev, NULL);
- return error;
}
static void wacom_remove(struct hid_device *hdev)
@@ -2789,8 +2783,6 @@ static void wacom_remove(struct hid_device *hdev)
wacom_release_resources(wacom);
kfifo_free(&wacom_wac->pen_fifo);
-
- hid_set_drvdata(hdev, NULL);
}
#ifdef CONFIG_PM
--
2.19.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: force setting drvdata to NULL when removing the driver
2019-03-28 15:51 [PATCH] HID: force setting drvdata to NULL when removing the driver Benjamin Tissoires
@ 2019-03-29 22:26 ` Dmitry Torokhov
2019-04-02 13:44 ` Benjamin Tissoires
0 siblings, 1 reply; 6+ messages in thread
From: Dmitry Torokhov @ 2019-03-29 22:26 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Bruno Prémont, Jonathan Cameron,
Srinivas Pandruvada, Hans de Goede, Jason Gerecke, Ping Cheng,
linux-input, lkml
Hi Benjamin,
On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Or when the probe failed.
>
> This is a common pattern in the HID drivers to reset the drvdata. Some
> do it properly, some do it only in case of failure.
> Anyway, it's never a good thing to have breadcrumbs, so force a clean
> state when removing or when probe is failing.
Just a data point: driver core already clears drvdata, so as long as
dev_get/set_drvdata() is the same as hid_get/set_drvdata() no special
handling is needed in HID core.
Thanks.
--
Dmitry
On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Or when the probe failed.
>
> This is a common pattern in the HID drivers to reset the drvdata. Some
> do it properly, some do it only in case of failure.
> Anyway, it's never a good thing to have breadcrumbs, so force a clean
> state when removing or when probe is failing.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-core.c | 2 ++
> drivers/hid/hid-cougar.c | 6 ++----
> drivers/hid/hid-gfrm.c | 7 -------
> drivers/hid/hid-lenovo.c | 2 --
> drivers/hid/hid-logitech-dj.c | 2 --
> drivers/hid/hid-logitech-hidpp.c | 8 +++-----
> drivers/hid/hid-picolcd_core.c | 7 +------
> drivers/hid/hid-sensor-hub.c | 1 -
> drivers/hid/wacom_sys.c | 18 +++++-------------
> 9 files changed, 13 insertions(+), 40 deletions(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index b5a8af8daa74..011e49da4d63 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2203,6 +2203,7 @@ static int hid_device_probe(struct device *dev)
> if (ret) {
> hid_close_report(hdev);
> hdev->driver = NULL;
> + hid_set_drvdata(hdev, NULL);
> }
> }
> unlock:
> @@ -2231,6 +2232,7 @@ static int hid_device_remove(struct device *dev)
> else /* default remove */
> hid_hw_stop(hdev);
> hid_close_report(hdev);
> + hid_set_drvdata(hdev, NULL);
> hdev->driver = NULL;
> }
>
> diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
> index e0bb7b34f3a4..4ff3bc1d25e2 100644
> --- a/drivers/hid/hid-cougar.c
> +++ b/drivers/hid/hid-cougar.c
> @@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
> error = hid_parse(hdev);
> if (error) {
> hid_err(hdev, "parse failed\n");
> - goto fail;
> + return error;
> }
>
> if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
> @@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
> error = hid_hw_start(hdev, connect_mask);
> if (error) {
> hid_err(hdev, "hw start failed\n");
> - goto fail;
> + return error;
> }
>
> error = cougar_bind_shared_data(hdev, cougar);
> @@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
>
> fail_stop_and_cleanup:
> hid_hw_stop(hdev);
> -fail:
> - hid_set_drvdata(hdev, NULL);
> return error;
> }
>
> diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
> index cf477f8c8f4c..1a20997e7750 100644
> --- a/drivers/hid/hid-gfrm.c
> +++ b/drivers/hid/hid-gfrm.c
> @@ -127,12 +127,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> -static void gfrm_remove(struct hid_device *hdev)
> -{
> - hid_hw_stop(hdev);
> - hid_set_drvdata(hdev, NULL);
> -}
> -
> static const struct hid_device_id gfrm_devices[] = {
> { HID_BLUETOOTH_DEVICE(0x58, 0x2000),
> .driver_data = GFRM100 },
> @@ -146,7 +140,6 @@ static struct hid_driver gfrm_driver = {
> .name = "gfrm",
> .id_table = gfrm_devices,
> .probe = gfrm_probe,
> - .remove = gfrm_remove,
> .input_mapping = gfrm_input_mapping,
> .raw_event = gfrm_raw_event,
> .input_configured = gfrm_input_configured,
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index eacc76d2ab96..cde9d22bb3ec 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -869,8 +869,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
>
> led_classdev_unregister(&data_pointer->led_micmute);
> led_classdev_unregister(&data_pointer->led_mute);
> -
> - hid_set_drvdata(hdev, NULL);
> }
>
> static void lenovo_remove_cptkbd(struct hid_device *hdev)
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index 826fa1e1c8d9..a75101293755 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -1094,7 +1094,6 @@ static int logi_dj_probe(struct hid_device *hdev,
> hid_parse_fail:
> kfifo_free(&djrcv_dev->notif_fifo);
> kfree(djrcv_dev);
> - hid_set_drvdata(hdev, NULL);
> return retval;
>
> }
> @@ -1145,7 +1144,6 @@ static void logi_dj_remove(struct hid_device *hdev)
>
> kfifo_free(&djrcv_dev->notif_fifo);
> kfree(djrcv_dev);
> - hid_set_drvdata(hdev, NULL);
> }
>
> static const struct hid_device_id logi_dj_receivers[] = {
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 199cc256e9d9..b950a44157a2 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -3237,15 +3237,15 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> ret = wtp_allocate(hdev, id);
> if (ret)
> - goto allocate_fail;
> + return ret;
> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
> ret = m560_allocate(hdev);
> if (ret)
> - goto allocate_fail;
> + return ret;
> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
> ret = k400_allocate(hdev);
> if (ret)
> - goto allocate_fail;
> + return ret;
> }
>
> INIT_WORK(&hidpp->work, delayed_work_cb);
> @@ -3343,8 +3343,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
> cancel_work_sync(&hidpp->work);
> mutex_destroy(&hidpp->send_mutex);
> -allocate_fail:
> - hid_set_drvdata(hdev, NULL);
> return ret;
> }
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index c1b29a9eb41a..a5d30f267a7b 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -550,8 +550,7 @@ static int picolcd_probe(struct hid_device *hdev,
> data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
> if (data == NULL) {
> hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
> - error = -ENOMEM;
> - goto err_no_cleanup;
> + return -ENOMEM;
> }
>
> spin_lock_init(&data->lock);
> @@ -613,9 +612,6 @@ static int picolcd_probe(struct hid_device *hdev,
> hid_hw_stop(hdev);
> err_cleanup_data:
> kfree(data);
> -err_no_cleanup:
> - hid_set_drvdata(hdev, NULL);
> -
> return error;
> }
>
> @@ -651,7 +647,6 @@ static void picolcd_remove(struct hid_device *hdev)
> picolcd_exit_cir(data);
> picolcd_exit_keys(data);
>
> - hid_set_drvdata(hdev, NULL);
> mutex_destroy(&data->mutex);
> /* Finally, clean up the picolcd data itself */
> kfree(data);
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index 4256fdc5cd6d..a3db5d8b382d 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -755,7 +755,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
> }
> spin_unlock_irqrestore(&data->lock, flags);
> mfd_remove_devices(&hdev->dev);
> - hid_set_drvdata(hdev, NULL);
> mutex_destroy(&data->mutex);
> }
>
> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index a8633b1437b2..c2fb614bff44 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2716,14 +2716,12 @@ static int wacom_probe(struct hid_device *hdev,
> wacom_wac->features = *((struct wacom_features *)id->driver_data);
> features = &wacom_wac->features;
>
> - if (features->check_for_hid_type && features->hid_type != hdev->type) {
> - error = -ENODEV;
> - goto fail;
> - }
> + if (features->check_for_hid_type && features->hid_type != hdev->type)
> + return -ENODEV;
>
> error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
> if (error)
> - goto fail;
> + return error;
>
> wacom_wac->hid_data.inputmode = -1;
> wacom_wac->mode_report = -1;
> @@ -2741,12 +2739,12 @@ static int wacom_probe(struct hid_device *hdev,
> error = hid_parse(hdev);
> if (error) {
> hid_err(hdev, "parse failed\n");
> - goto fail;
> + return error;
> }
>
> error = wacom_parse_and_register(wacom, false);
> if (error)
> - goto fail;
> + return error;
>
> if (hdev->bus == BUS_BLUETOOTH) {
> error = device_create_file(&hdev->dev, &dev_attr_speed);
> @@ -2757,10 +2755,6 @@ static int wacom_probe(struct hid_device *hdev,
> }
>
> return 0;
> -
> -fail:
> - hid_set_drvdata(hdev, NULL);
> - return error;
> }
>
> static void wacom_remove(struct hid_device *hdev)
> @@ -2789,8 +2783,6 @@ static void wacom_remove(struct hid_device *hdev)
> wacom_release_resources(wacom);
>
> kfifo_free(&wacom_wac->pen_fifo);
> -
> - hid_set_drvdata(hdev, NULL);
> }
>
> #ifdef CONFIG_PM
> --
> 2.19.2
>
--
Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: force setting drvdata to NULL when removing the driver
2019-03-29 22:26 ` Dmitry Torokhov
@ 2019-04-02 13:44 ` Benjamin Tissoires
2019-04-02 13:52 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2019-04-02 13:44 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Jiri Kosina, Bruno Prémont, Jonathan Cameron,
Srinivas Pandruvada, Hans de Goede, Jason Gerecke, Ping Cheng,
linux-input, lkml
Hi Dmitry,
On Fri, Mar 29, 2019 at 11:26 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Hi Benjamin,
>
> On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Or when the probe failed.
> >
> > This is a common pattern in the HID drivers to reset the drvdata. Some
> > do it properly, some do it only in case of failure.
> > Anyway, it's never a good thing to have breadcrumbs, so force a clean
> > state when removing or when probe is failing.
>
> Just a data point: driver core already clears drvdata, so as long as
> dev_get/set_drvdata() is the same as hid_get/set_drvdata() no special
> handling is needed in HID core.
You are correct (as always ;-P). I'll drop the hid-core changes and
send a v2 ASAP.
Cheers,
Benjamin
>
> Thanks.
>
> --
> Dmitry
>
> On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Or when the probe failed.
> >
> > This is a common pattern in the HID drivers to reset the drvdata. Some
> > do it properly, some do it only in case of failure.
> > Anyway, it's never a good thing to have breadcrumbs, so force a clean
> > state when removing or when probe is failing.
> >
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > drivers/hid/hid-core.c | 2 ++
> > drivers/hid/hid-cougar.c | 6 ++----
> > drivers/hid/hid-gfrm.c | 7 -------
> > drivers/hid/hid-lenovo.c | 2 --
> > drivers/hid/hid-logitech-dj.c | 2 --
> > drivers/hid/hid-logitech-hidpp.c | 8 +++-----
> > drivers/hid/hid-picolcd_core.c | 7 +------
> > drivers/hid/hid-sensor-hub.c | 1 -
> > drivers/hid/wacom_sys.c | 18 +++++-------------
> > 9 files changed, 13 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> > index b5a8af8daa74..011e49da4d63 100644
> > --- a/drivers/hid/hid-core.c
> > +++ b/drivers/hid/hid-core.c
> > @@ -2203,6 +2203,7 @@ static int hid_device_probe(struct device *dev)
> > if (ret) {
> > hid_close_report(hdev);
> > hdev->driver = NULL;
> > + hid_set_drvdata(hdev, NULL);
> > }
> > }
> > unlock:
> > @@ -2231,6 +2232,7 @@ static int hid_device_remove(struct device *dev)
> > else /* default remove */
> > hid_hw_stop(hdev);
> > hid_close_report(hdev);
> > + hid_set_drvdata(hdev, NULL);
> > hdev->driver = NULL;
> > }
> >
> > diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
> > index e0bb7b34f3a4..4ff3bc1d25e2 100644
> > --- a/drivers/hid/hid-cougar.c
> > +++ b/drivers/hid/hid-cougar.c
> > @@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
> > error = hid_parse(hdev);
> > if (error) {
> > hid_err(hdev, "parse failed\n");
> > - goto fail;
> > + return error;
> > }
> >
> > if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
> > @@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
> > error = hid_hw_start(hdev, connect_mask);
> > if (error) {
> > hid_err(hdev, "hw start failed\n");
> > - goto fail;
> > + return error;
> > }
> >
> > error = cougar_bind_shared_data(hdev, cougar);
> > @@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
> >
> > fail_stop_and_cleanup:
> > hid_hw_stop(hdev);
> > -fail:
> > - hid_set_drvdata(hdev, NULL);
> > return error;
> > }
> >
> > diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
> > index cf477f8c8f4c..1a20997e7750 100644
> > --- a/drivers/hid/hid-gfrm.c
> > +++ b/drivers/hid/hid-gfrm.c
> > @@ -127,12 +127,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > return ret;
> > }
> >
> > -static void gfrm_remove(struct hid_device *hdev)
> > -{
> > - hid_hw_stop(hdev);
> > - hid_set_drvdata(hdev, NULL);
> > -}
> > -
> > static const struct hid_device_id gfrm_devices[] = {
> > { HID_BLUETOOTH_DEVICE(0x58, 0x2000),
> > .driver_data = GFRM100 },
> > @@ -146,7 +140,6 @@ static struct hid_driver gfrm_driver = {
> > .name = "gfrm",
> > .id_table = gfrm_devices,
> > .probe = gfrm_probe,
> > - .remove = gfrm_remove,
> > .input_mapping = gfrm_input_mapping,
> > .raw_event = gfrm_raw_event,
> > .input_configured = gfrm_input_configured,
> > diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> > index eacc76d2ab96..cde9d22bb3ec 100644
> > --- a/drivers/hid/hid-lenovo.c
> > +++ b/drivers/hid/hid-lenovo.c
> > @@ -869,8 +869,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
> >
> > led_classdev_unregister(&data_pointer->led_micmute);
> > led_classdev_unregister(&data_pointer->led_mute);
> > -
> > - hid_set_drvdata(hdev, NULL);
> > }
> >
> > static void lenovo_remove_cptkbd(struct hid_device *hdev)
> > diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> > index 826fa1e1c8d9..a75101293755 100644
> > --- a/drivers/hid/hid-logitech-dj.c
> > +++ b/drivers/hid/hid-logitech-dj.c
> > @@ -1094,7 +1094,6 @@ static int logi_dj_probe(struct hid_device *hdev,
> > hid_parse_fail:
> > kfifo_free(&djrcv_dev->notif_fifo);
> > kfree(djrcv_dev);
> > - hid_set_drvdata(hdev, NULL);
> > return retval;
> >
> > }
> > @@ -1145,7 +1144,6 @@ static void logi_dj_remove(struct hid_device *hdev)
> >
> > kfifo_free(&djrcv_dev->notif_fifo);
> > kfree(djrcv_dev);
> > - hid_set_drvdata(hdev, NULL);
> > }
> >
> > static const struct hid_device_id logi_dj_receivers[] = {
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 199cc256e9d9..b950a44157a2 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -3237,15 +3237,15 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> > ret = wtp_allocate(hdev, id);
> > if (ret)
> > - goto allocate_fail;
> > + return ret;
> > } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
> > ret = m560_allocate(hdev);
> > if (ret)
> > - goto allocate_fail;
> > + return ret;
> > } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
> > ret = k400_allocate(hdev);
> > if (ret)
> > - goto allocate_fail;
> > + return ret;
> > }
> >
> > INIT_WORK(&hidpp->work, delayed_work_cb);
> > @@ -3343,8 +3343,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
> > cancel_work_sync(&hidpp->work);
> > mutex_destroy(&hidpp->send_mutex);
> > -allocate_fail:
> > - hid_set_drvdata(hdev, NULL);
> > return ret;
> > }
> >
> > diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> > index c1b29a9eb41a..a5d30f267a7b 100644
> > --- a/drivers/hid/hid-picolcd_core.c
> > +++ b/drivers/hid/hid-picolcd_core.c
> > @@ -550,8 +550,7 @@ static int picolcd_probe(struct hid_device *hdev,
> > data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
> > if (data == NULL) {
> > hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
> > - error = -ENOMEM;
> > - goto err_no_cleanup;
> > + return -ENOMEM;
> > }
> >
> > spin_lock_init(&data->lock);
> > @@ -613,9 +612,6 @@ static int picolcd_probe(struct hid_device *hdev,
> > hid_hw_stop(hdev);
> > err_cleanup_data:
> > kfree(data);
> > -err_no_cleanup:
> > - hid_set_drvdata(hdev, NULL);
> > -
> > return error;
> > }
> >
> > @@ -651,7 +647,6 @@ static void picolcd_remove(struct hid_device *hdev)
> > picolcd_exit_cir(data);
> > picolcd_exit_keys(data);
> >
> > - hid_set_drvdata(hdev, NULL);
> > mutex_destroy(&data->mutex);
> > /* Finally, clean up the picolcd data itself */
> > kfree(data);
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index 4256fdc5cd6d..a3db5d8b382d 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -755,7 +755,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
> > }
> > spin_unlock_irqrestore(&data->lock, flags);
> > mfd_remove_devices(&hdev->dev);
> > - hid_set_drvdata(hdev, NULL);
> > mutex_destroy(&data->mutex);
> > }
> >
> > diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> > index a8633b1437b2..c2fb614bff44 100644
> > --- a/drivers/hid/wacom_sys.c
> > +++ b/drivers/hid/wacom_sys.c
> > @@ -2716,14 +2716,12 @@ static int wacom_probe(struct hid_device *hdev,
> > wacom_wac->features = *((struct wacom_features *)id->driver_data);
> > features = &wacom_wac->features;
> >
> > - if (features->check_for_hid_type && features->hid_type != hdev->type) {
> > - error = -ENODEV;
> > - goto fail;
> > - }
> > + if (features->check_for_hid_type && features->hid_type != hdev->type)
> > + return -ENODEV;
> >
> > error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
> > if (error)
> > - goto fail;
> > + return error;
> >
> > wacom_wac->hid_data.inputmode = -1;
> > wacom_wac->mode_report = -1;
> > @@ -2741,12 +2739,12 @@ static int wacom_probe(struct hid_device *hdev,
> > error = hid_parse(hdev);
> > if (error) {
> > hid_err(hdev, "parse failed\n");
> > - goto fail;
> > + return error;
> > }
> >
> > error = wacom_parse_and_register(wacom, false);
> > if (error)
> > - goto fail;
> > + return error;
> >
> > if (hdev->bus == BUS_BLUETOOTH) {
> > error = device_create_file(&hdev->dev, &dev_attr_speed);
> > @@ -2757,10 +2755,6 @@ static int wacom_probe(struct hid_device *hdev,
> > }
> >
> > return 0;
> > -
> > -fail:
> > - hid_set_drvdata(hdev, NULL);
> > - return error;
> > }
> >
> > static void wacom_remove(struct hid_device *hdev)
> > @@ -2789,8 +2783,6 @@ static void wacom_remove(struct hid_device *hdev)
> > wacom_release_resources(wacom);
> >
> > kfifo_free(&wacom_wac->pen_fifo);
> > -
> > - hid_set_drvdata(hdev, NULL);
> > }
> >
> > #ifdef CONFIG_PM
> > --
> > 2.19.2
> >
>
>
> --
> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: force setting drvdata to NULL when removing the driver
2019-04-02 13:44 ` Benjamin Tissoires
@ 2019-04-02 13:52 ` Hans de Goede
2019-04-02 14:00 ` Benjamin Tissoires
0 siblings, 1 reply; 6+ messages in thread
From: Hans de Goede @ 2019-04-02 13:52 UTC (permalink / raw)
To: Benjamin Tissoires, Dmitry Torokhov
Cc: Jiri Kosina, Bruno Prémont, Jonathan Cameron,
Srinivas Pandruvada, Jason Gerecke, Ping Cheng, linux-input,
lkml
Hi,
On 02-04-19 15:44, Benjamin Tissoires wrote:
> Hi Dmitry,
>
> On Fri, Mar 29, 2019 at 11:26 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>>
>> Hi Benjamin,
>>
>> On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>>>
>>> Or when the probe failed.
>>>
>>> This is a common pattern in the HID drivers to reset the drvdata. Some
>>> do it properly, some do it only in case of failure.
>>> Anyway, it's never a good thing to have breadcrumbs, so force a clean
>>> state when removing or when probe is failing.
>>
>> Just a data point: driver core already clears drvdata, so as long as
>> dev_get/set_drvdata() is the same as hid_get/set_drvdata() no special
>> handling is needed in HID core.
>
> You are correct (as always ;-P). I'll drop the hid-core changes and
> send a v2 ASAP.
I was just looking at the same thing. I should have known about this
since I wrote the patch to make the core clear drvdata. I should have
mentioned that, but I was assuming that the hid code was special somehow,
however now I see it just uses a regular device_add, so indeed the core
changes are not necessary.
Given the large hid-logitech-*.c patch-set it might be easier for
Jiri if you split out the hid-logitech-*.c changes into a separate
patch, then he can keep that on his logitech branch (just a thought).
Regards,
Hans
>
> Cheers,
> Benjamin
>
>>
>> Thanks.
>>
>> --
>> Dmitry
>>
>> On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
>> <benjamin.tissoires@redhat.com> wrote:
>>>
>>> Or when the probe failed.
>>>
>>> This is a common pattern in the HID drivers to reset the drvdata. Some
>>> do it properly, some do it only in case of failure.
>>> Anyway, it's never a good thing to have breadcrumbs, so force a clean
>>> state when removing or when probe is failing.
>>>
>>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>>> ---
>>> drivers/hid/hid-core.c | 2 ++
>>> drivers/hid/hid-cougar.c | 6 ++----
>>> drivers/hid/hid-gfrm.c | 7 -------
>>> drivers/hid/hid-lenovo.c | 2 --
>>> drivers/hid/hid-logitech-dj.c | 2 --
>>> drivers/hid/hid-logitech-hidpp.c | 8 +++-----
>>> drivers/hid/hid-picolcd_core.c | 7 +------
>>> drivers/hid/hid-sensor-hub.c | 1 -
>>> drivers/hid/wacom_sys.c | 18 +++++-------------
>>> 9 files changed, 13 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index b5a8af8daa74..011e49da4d63 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -2203,6 +2203,7 @@ static int hid_device_probe(struct device *dev)
>>> if (ret) {
>>> hid_close_report(hdev);
>>> hdev->driver = NULL;
>>> + hid_set_drvdata(hdev, NULL);
>>> }
>>> }
>>> unlock:
>>> @@ -2231,6 +2232,7 @@ static int hid_device_remove(struct device *dev)
>>> else /* default remove */
>>> hid_hw_stop(hdev);
>>> hid_close_report(hdev);
>>> + hid_set_drvdata(hdev, NULL);
>>> hdev->driver = NULL;
>>> }
>>>
>>> diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
>>> index e0bb7b34f3a4..4ff3bc1d25e2 100644
>>> --- a/drivers/hid/hid-cougar.c
>>> +++ b/drivers/hid/hid-cougar.c
>>> @@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
>>> error = hid_parse(hdev);
>>> if (error) {
>>> hid_err(hdev, "parse failed\n");
>>> - goto fail;
>>> + return error;
>>> }
>>>
>>> if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
>>> @@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
>>> error = hid_hw_start(hdev, connect_mask);
>>> if (error) {
>>> hid_err(hdev, "hw start failed\n");
>>> - goto fail;
>>> + return error;
>>> }
>>>
>>> error = cougar_bind_shared_data(hdev, cougar);
>>> @@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
>>>
>>> fail_stop_and_cleanup:
>>> hid_hw_stop(hdev);
>>> -fail:
>>> - hid_set_drvdata(hdev, NULL);
>>> return error;
>>> }
>>>
>>> diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
>>> index cf477f8c8f4c..1a20997e7750 100644
>>> --- a/drivers/hid/hid-gfrm.c
>>> +++ b/drivers/hid/hid-gfrm.c
>>> @@ -127,12 +127,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> return ret;
>>> }
>>>
>>> -static void gfrm_remove(struct hid_device *hdev)
>>> -{
>>> - hid_hw_stop(hdev);
>>> - hid_set_drvdata(hdev, NULL);
>>> -}
>>> -
>>> static const struct hid_device_id gfrm_devices[] = {
>>> { HID_BLUETOOTH_DEVICE(0x58, 0x2000),
>>> .driver_data = GFRM100 },
>>> @@ -146,7 +140,6 @@ static struct hid_driver gfrm_driver = {
>>> .name = "gfrm",
>>> .id_table = gfrm_devices,
>>> .probe = gfrm_probe,
>>> - .remove = gfrm_remove,
>>> .input_mapping = gfrm_input_mapping,
>>> .raw_event = gfrm_raw_event,
>>> .input_configured = gfrm_input_configured,
>>> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
>>> index eacc76d2ab96..cde9d22bb3ec 100644
>>> --- a/drivers/hid/hid-lenovo.c
>>> +++ b/drivers/hid/hid-lenovo.c
>>> @@ -869,8 +869,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
>>>
>>> led_classdev_unregister(&data_pointer->led_micmute);
>>> led_classdev_unregister(&data_pointer->led_mute);
>>> -
>>> - hid_set_drvdata(hdev, NULL);
>>> }
>>>
>>> static void lenovo_remove_cptkbd(struct hid_device *hdev)
>>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
>>> index 826fa1e1c8d9..a75101293755 100644
>>> --- a/drivers/hid/hid-logitech-dj.c
>>> +++ b/drivers/hid/hid-logitech-dj.c
>>> @@ -1094,7 +1094,6 @@ static int logi_dj_probe(struct hid_device *hdev,
>>> hid_parse_fail:
>>> kfifo_free(&djrcv_dev->notif_fifo);
>>> kfree(djrcv_dev);
>>> - hid_set_drvdata(hdev, NULL);
>>> return retval;
>>>
>>> }
>>> @@ -1145,7 +1144,6 @@ static void logi_dj_remove(struct hid_device *hdev)
>>>
>>> kfifo_free(&djrcv_dev->notif_fifo);
>>> kfree(djrcv_dev);
>>> - hid_set_drvdata(hdev, NULL);
>>> }
>>>
>>> static const struct hid_device_id logi_dj_receivers[] = {
>>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>>> index 199cc256e9d9..b950a44157a2 100644
>>> --- a/drivers/hid/hid-logitech-hidpp.c
>>> +++ b/drivers/hid/hid-logitech-hidpp.c
>>> @@ -3237,15 +3237,15 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
>>> ret = wtp_allocate(hdev, id);
>>> if (ret)
>>> - goto allocate_fail;
>>> + return ret;
>>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
>>> ret = m560_allocate(hdev);
>>> if (ret)
>>> - goto allocate_fail;
>>> + return ret;
>>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
>>> ret = k400_allocate(hdev);
>>> if (ret)
>>> - goto allocate_fail;
>>> + return ret;
>>> }
>>>
>>> INIT_WORK(&hidpp->work, delayed_work_cb);
>>> @@ -3343,8 +3343,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>>> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
>>> cancel_work_sync(&hidpp->work);
>>> mutex_destroy(&hidpp->send_mutex);
>>> -allocate_fail:
>>> - hid_set_drvdata(hdev, NULL);
>>> return ret;
>>> }
>>>
>>> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
>>> index c1b29a9eb41a..a5d30f267a7b 100644
>>> --- a/drivers/hid/hid-picolcd_core.c
>>> +++ b/drivers/hid/hid-picolcd_core.c
>>> @@ -550,8 +550,7 @@ static int picolcd_probe(struct hid_device *hdev,
>>> data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
>>> if (data == NULL) {
>>> hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
>>> - error = -ENOMEM;
>>> - goto err_no_cleanup;
>>> + return -ENOMEM;
>>> }
>>>
>>> spin_lock_init(&data->lock);
>>> @@ -613,9 +612,6 @@ static int picolcd_probe(struct hid_device *hdev,
>>> hid_hw_stop(hdev);
>>> err_cleanup_data:
>>> kfree(data);
>>> -err_no_cleanup:
>>> - hid_set_drvdata(hdev, NULL);
>>> -
>>> return error;
>>> }
>>>
>>> @@ -651,7 +647,6 @@ static void picolcd_remove(struct hid_device *hdev)
>>> picolcd_exit_cir(data);
>>> picolcd_exit_keys(data);
>>>
>>> - hid_set_drvdata(hdev, NULL);
>>> mutex_destroy(&data->mutex);
>>> /* Finally, clean up the picolcd data itself */
>>> kfree(data);
>>> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
>>> index 4256fdc5cd6d..a3db5d8b382d 100644
>>> --- a/drivers/hid/hid-sensor-hub.c
>>> +++ b/drivers/hid/hid-sensor-hub.c
>>> @@ -755,7 +755,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
>>> }
>>> spin_unlock_irqrestore(&data->lock, flags);
>>> mfd_remove_devices(&hdev->dev);
>>> - hid_set_drvdata(hdev, NULL);
>>> mutex_destroy(&data->mutex);
>>> }
>>>
>>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
>>> index a8633b1437b2..c2fb614bff44 100644
>>> --- a/drivers/hid/wacom_sys.c
>>> +++ b/drivers/hid/wacom_sys.c
>>> @@ -2716,14 +2716,12 @@ static int wacom_probe(struct hid_device *hdev,
>>> wacom_wac->features = *((struct wacom_features *)id->driver_data);
>>> features = &wacom_wac->features;
>>>
>>> - if (features->check_for_hid_type && features->hid_type != hdev->type) {
>>> - error = -ENODEV;
>>> - goto fail;
>>> - }
>>> + if (features->check_for_hid_type && features->hid_type != hdev->type)
>>> + return -ENODEV;
>>>
>>> error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
>>> if (error)
>>> - goto fail;
>>> + return error;
>>>
>>> wacom_wac->hid_data.inputmode = -1;
>>> wacom_wac->mode_report = -1;
>>> @@ -2741,12 +2739,12 @@ static int wacom_probe(struct hid_device *hdev,
>>> error = hid_parse(hdev);
>>> if (error) {
>>> hid_err(hdev, "parse failed\n");
>>> - goto fail;
>>> + return error;
>>> }
>>>
>>> error = wacom_parse_and_register(wacom, false);
>>> if (error)
>>> - goto fail;
>>> + return error;
>>>
>>> if (hdev->bus == BUS_BLUETOOTH) {
>>> error = device_create_file(&hdev->dev, &dev_attr_speed);
>>> @@ -2757,10 +2755,6 @@ static int wacom_probe(struct hid_device *hdev,
>>> }
>>>
>>> return 0;
>>> -
>>> -fail:
>>> - hid_set_drvdata(hdev, NULL);
>>> - return error;
>>> }
>>>
>>> static void wacom_remove(struct hid_device *hdev)
>>> @@ -2789,8 +2783,6 @@ static void wacom_remove(struct hid_device *hdev)
>>> wacom_release_resources(wacom);
>>>
>>> kfifo_free(&wacom_wac->pen_fifo);
>>> -
>>> - hid_set_drvdata(hdev, NULL);
>>> }
>>>
>>> #ifdef CONFIG_PM
>>> --
>>> 2.19.2
>>>
>>
>>
>> --
>> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: force setting drvdata to NULL when removing the driver
2019-04-02 13:52 ` Hans de Goede
@ 2019-04-02 14:00 ` Benjamin Tissoires
2019-04-02 14:02 ` Hans de Goede
0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2019-04-02 14:00 UTC (permalink / raw)
To: Hans de Goede
Cc: Dmitry Torokhov, Jiri Kosina, Bruno Prémont,
Jonathan Cameron, Srinivas Pandruvada, Jason Gerecke, Ping Cheng,
linux-input, lkml
On Tue, Apr 2, 2019 at 3:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
> On 02-04-19 15:44, Benjamin Tissoires wrote:
> > Hi Dmitry,
> >
> > On Fri, Mar 29, 2019 at 11:26 PM Dmitry Torokhov
> > <dmitry.torokhov@gmail.com> wrote:
> >>
> >> Hi Benjamin,
> >>
> >> On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
> >> <benjamin.tissoires@redhat.com> wrote:
> >>>
> >>> Or when the probe failed.
> >>>
> >>> This is a common pattern in the HID drivers to reset the drvdata. Some
> >>> do it properly, some do it only in case of failure.
> >>> Anyway, it's never a good thing to have breadcrumbs, so force a clean
> >>> state when removing or when probe is failing.
> >>
> >> Just a data point: driver core already clears drvdata, so as long as
> >> dev_get/set_drvdata() is the same as hid_get/set_drvdata() no special
> >> handling is needed in HID core.
> >
> > You are correct (as always ;-P). I'll drop the hid-core changes and
> > send a v2 ASAP.
>
> I was just looking at the same thing. I should have known about this
> since I wrote the patch to make the core clear drvdata. I should have
> mentioned that, but I was assuming that the hid code was special somehow,
> however now I see it just uses a regular device_add, so indeed the core
> changes are not necessary.
>
> Given the large hid-logitech-*.c patch-set it might be easier for
> Jiri if you split out the hid-logitech-*.c changes into a separate
> patch, then he can keep that on his logitech branch (just a thought).
>
I do not expect the logitech changes to now create some big conflict.
But looking at the current for-5.2* branches, I should probably split
the series per driver given that there is no more dependency on
hid-core. This way, we can take the logitech changes in the
for-5.2/logitech branch, and you can rebase your work on top of it.
Cheers,
Benjamin
> Regards,
>
> Hans
>
>
> >
> > Cheers,
> > Benjamin
> >
> >>
> >> Thanks.
> >>
> >> --
> >> Dmitry
> >>
> >> On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
> >> <benjamin.tissoires@redhat.com> wrote:
> >>>
> >>> Or when the probe failed.
> >>>
> >>> This is a common pattern in the HID drivers to reset the drvdata. Some
> >>> do it properly, some do it only in case of failure.
> >>> Anyway, it's never a good thing to have breadcrumbs, so force a clean
> >>> state when removing or when probe is failing.
> >>>
> >>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >>> ---
> >>> drivers/hid/hid-core.c | 2 ++
> >>> drivers/hid/hid-cougar.c | 6 ++----
> >>> drivers/hid/hid-gfrm.c | 7 -------
> >>> drivers/hid/hid-lenovo.c | 2 --
> >>> drivers/hid/hid-logitech-dj.c | 2 --
> >>> drivers/hid/hid-logitech-hidpp.c | 8 +++-----
> >>> drivers/hid/hid-picolcd_core.c | 7 +------
> >>> drivers/hid/hid-sensor-hub.c | 1 -
> >>> drivers/hid/wacom_sys.c | 18 +++++-------------
> >>> 9 files changed, 13 insertions(+), 40 deletions(-)
> >>>
> >>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> >>> index b5a8af8daa74..011e49da4d63 100644
> >>> --- a/drivers/hid/hid-core.c
> >>> +++ b/drivers/hid/hid-core.c
> >>> @@ -2203,6 +2203,7 @@ static int hid_device_probe(struct device *dev)
> >>> if (ret) {
> >>> hid_close_report(hdev);
> >>> hdev->driver = NULL;
> >>> + hid_set_drvdata(hdev, NULL);
> >>> }
> >>> }
> >>> unlock:
> >>> @@ -2231,6 +2232,7 @@ static int hid_device_remove(struct device *dev)
> >>> else /* default remove */
> >>> hid_hw_stop(hdev);
> >>> hid_close_report(hdev);
> >>> + hid_set_drvdata(hdev, NULL);
> >>> hdev->driver = NULL;
> >>> }
> >>>
> >>> diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
> >>> index e0bb7b34f3a4..4ff3bc1d25e2 100644
> >>> --- a/drivers/hid/hid-cougar.c
> >>> +++ b/drivers/hid/hid-cougar.c
> >>> @@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
> >>> error = hid_parse(hdev);
> >>> if (error) {
> >>> hid_err(hdev, "parse failed\n");
> >>> - goto fail;
> >>> + return error;
> >>> }
> >>>
> >>> if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
> >>> @@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
> >>> error = hid_hw_start(hdev, connect_mask);
> >>> if (error) {
> >>> hid_err(hdev, "hw start failed\n");
> >>> - goto fail;
> >>> + return error;
> >>> }
> >>>
> >>> error = cougar_bind_shared_data(hdev, cougar);
> >>> @@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
> >>>
> >>> fail_stop_and_cleanup:
> >>> hid_hw_stop(hdev);
> >>> -fail:
> >>> - hid_set_drvdata(hdev, NULL);
> >>> return error;
> >>> }
> >>>
> >>> diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
> >>> index cf477f8c8f4c..1a20997e7750 100644
> >>> --- a/drivers/hid/hid-gfrm.c
> >>> +++ b/drivers/hid/hid-gfrm.c
> >>> @@ -127,12 +127,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>> return ret;
> >>> }
> >>>
> >>> -static void gfrm_remove(struct hid_device *hdev)
> >>> -{
> >>> - hid_hw_stop(hdev);
> >>> - hid_set_drvdata(hdev, NULL);
> >>> -}
> >>> -
> >>> static const struct hid_device_id gfrm_devices[] = {
> >>> { HID_BLUETOOTH_DEVICE(0x58, 0x2000),
> >>> .driver_data = GFRM100 },
> >>> @@ -146,7 +140,6 @@ static struct hid_driver gfrm_driver = {
> >>> .name = "gfrm",
> >>> .id_table = gfrm_devices,
> >>> .probe = gfrm_probe,
> >>> - .remove = gfrm_remove,
> >>> .input_mapping = gfrm_input_mapping,
> >>> .raw_event = gfrm_raw_event,
> >>> .input_configured = gfrm_input_configured,
> >>> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> >>> index eacc76d2ab96..cde9d22bb3ec 100644
> >>> --- a/drivers/hid/hid-lenovo.c
> >>> +++ b/drivers/hid/hid-lenovo.c
> >>> @@ -869,8 +869,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
> >>>
> >>> led_classdev_unregister(&data_pointer->led_micmute);
> >>> led_classdev_unregister(&data_pointer->led_mute);
> >>> -
> >>> - hid_set_drvdata(hdev, NULL);
> >>> }
> >>>
> >>> static void lenovo_remove_cptkbd(struct hid_device *hdev)
> >>> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> >>> index 826fa1e1c8d9..a75101293755 100644
> >>> --- a/drivers/hid/hid-logitech-dj.c
> >>> +++ b/drivers/hid/hid-logitech-dj.c
> >>> @@ -1094,7 +1094,6 @@ static int logi_dj_probe(struct hid_device *hdev,
> >>> hid_parse_fail:
> >>> kfifo_free(&djrcv_dev->notif_fifo);
> >>> kfree(djrcv_dev);
> >>> - hid_set_drvdata(hdev, NULL);
> >>> return retval;
> >>>
> >>> }
> >>> @@ -1145,7 +1144,6 @@ static void logi_dj_remove(struct hid_device *hdev)
> >>>
> >>> kfifo_free(&djrcv_dev->notif_fifo);
> >>> kfree(djrcv_dev);
> >>> - hid_set_drvdata(hdev, NULL);
> >>> }
> >>>
> >>> static const struct hid_device_id logi_dj_receivers[] = {
> >>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> >>> index 199cc256e9d9..b950a44157a2 100644
> >>> --- a/drivers/hid/hid-logitech-hidpp.c
> >>> +++ b/drivers/hid/hid-logitech-hidpp.c
> >>> @@ -3237,15 +3237,15 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>> if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
> >>> ret = wtp_allocate(hdev, id);
> >>> if (ret)
> >>> - goto allocate_fail;
> >>> + return ret;
> >>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
> >>> ret = m560_allocate(hdev);
> >>> if (ret)
> >>> - goto allocate_fail;
> >>> + return ret;
> >>> } else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
> >>> ret = k400_allocate(hdev);
> >>> if (ret)
> >>> - goto allocate_fail;
> >>> + return ret;
> >>> }
> >>>
> >>> INIT_WORK(&hidpp->work, delayed_work_cb);
> >>> @@ -3343,8 +3343,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >>> sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
> >>> cancel_work_sync(&hidpp->work);
> >>> mutex_destroy(&hidpp->send_mutex);
> >>> -allocate_fail:
> >>> - hid_set_drvdata(hdev, NULL);
> >>> return ret;
> >>> }
> >>>
> >>> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> >>> index c1b29a9eb41a..a5d30f267a7b 100644
> >>> --- a/drivers/hid/hid-picolcd_core.c
> >>> +++ b/drivers/hid/hid-picolcd_core.c
> >>> @@ -550,8 +550,7 @@ static int picolcd_probe(struct hid_device *hdev,
> >>> data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
> >>> if (data == NULL) {
> >>> hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
> >>> - error = -ENOMEM;
> >>> - goto err_no_cleanup;
> >>> + return -ENOMEM;
> >>> }
> >>>
> >>> spin_lock_init(&data->lock);
> >>> @@ -613,9 +612,6 @@ static int picolcd_probe(struct hid_device *hdev,
> >>> hid_hw_stop(hdev);
> >>> err_cleanup_data:
> >>> kfree(data);
> >>> -err_no_cleanup:
> >>> - hid_set_drvdata(hdev, NULL);
> >>> -
> >>> return error;
> >>> }
> >>>
> >>> @@ -651,7 +647,6 @@ static void picolcd_remove(struct hid_device *hdev)
> >>> picolcd_exit_cir(data);
> >>> picolcd_exit_keys(data);
> >>>
> >>> - hid_set_drvdata(hdev, NULL);
> >>> mutex_destroy(&data->mutex);
> >>> /* Finally, clean up the picolcd data itself */
> >>> kfree(data);
> >>> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> >>> index 4256fdc5cd6d..a3db5d8b382d 100644
> >>> --- a/drivers/hid/hid-sensor-hub.c
> >>> +++ b/drivers/hid/hid-sensor-hub.c
> >>> @@ -755,7 +755,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
> >>> }
> >>> spin_unlock_irqrestore(&data->lock, flags);
> >>> mfd_remove_devices(&hdev->dev);
> >>> - hid_set_drvdata(hdev, NULL);
> >>> mutex_destroy(&data->mutex);
> >>> }
> >>>
> >>> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> >>> index a8633b1437b2..c2fb614bff44 100644
> >>> --- a/drivers/hid/wacom_sys.c
> >>> +++ b/drivers/hid/wacom_sys.c
> >>> @@ -2716,14 +2716,12 @@ static int wacom_probe(struct hid_device *hdev,
> >>> wacom_wac->features = *((struct wacom_features *)id->driver_data);
> >>> features = &wacom_wac->features;
> >>>
> >>> - if (features->check_for_hid_type && features->hid_type != hdev->type) {
> >>> - error = -ENODEV;
> >>> - goto fail;
> >>> - }
> >>> + if (features->check_for_hid_type && features->hid_type != hdev->type)
> >>> + return -ENODEV;
> >>>
> >>> error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
> >>> if (error)
> >>> - goto fail;
> >>> + return error;
> >>>
> >>> wacom_wac->hid_data.inputmode = -1;
> >>> wacom_wac->mode_report = -1;
> >>> @@ -2741,12 +2739,12 @@ static int wacom_probe(struct hid_device *hdev,
> >>> error = hid_parse(hdev);
> >>> if (error) {
> >>> hid_err(hdev, "parse failed\n");
> >>> - goto fail;
> >>> + return error;
> >>> }
> >>>
> >>> error = wacom_parse_and_register(wacom, false);
> >>> if (error)
> >>> - goto fail;
> >>> + return error;
> >>>
> >>> if (hdev->bus == BUS_BLUETOOTH) {
> >>> error = device_create_file(&hdev->dev, &dev_attr_speed);
> >>> @@ -2757,10 +2755,6 @@ static int wacom_probe(struct hid_device *hdev,
> >>> }
> >>>
> >>> return 0;
> >>> -
> >>> -fail:
> >>> - hid_set_drvdata(hdev, NULL);
> >>> - return error;
> >>> }
> >>>
> >>> static void wacom_remove(struct hid_device *hdev)
> >>> @@ -2789,8 +2783,6 @@ static void wacom_remove(struct hid_device *hdev)
> >>> wacom_release_resources(wacom);
> >>>
> >>> kfifo_free(&wacom_wac->pen_fifo);
> >>> -
> >>> - hid_set_drvdata(hdev, NULL);
> >>> }
> >>>
> >>> #ifdef CONFIG_PM
> >>> --
> >>> 2.19.2
> >>>
> >>
> >>
> >> --
> >> Dmitry
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] HID: force setting drvdata to NULL when removing the driver
2019-04-02 14:00 ` Benjamin Tissoires
@ 2019-04-02 14:02 ` Hans de Goede
0 siblings, 0 replies; 6+ messages in thread
From: Hans de Goede @ 2019-04-02 14:02 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Dmitry Torokhov, Jiri Kosina, Bruno Prémont,
Jonathan Cameron, Srinivas Pandruvada, Jason Gerecke, Ping Cheng,
linux-input, lkml
Hi,
On 02-04-19 16:00, Benjamin Tissoires wrote:
> On Tue, Apr 2, 2019 at 3:52 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> Hi,
>>
>> On 02-04-19 15:44, Benjamin Tissoires wrote:
>>> Hi Dmitry,
>>>
>>> On Fri, Mar 29, 2019 at 11:26 PM Dmitry Torokhov
>>> <dmitry.torokhov@gmail.com> wrote:
>>>>
>>>> Hi Benjamin,
>>>>
>>>> On Thu, Mar 28, 2019 at 8:52 AM Benjamin Tissoires
>>>> <benjamin.tissoires@redhat.com> wrote:
>>>>>
>>>>> Or when the probe failed.
>>>>>
>>>>> This is a common pattern in the HID drivers to reset the drvdata. Some
>>>>> do it properly, some do it only in case of failure.
>>>>> Anyway, it's never a good thing to have breadcrumbs, so force a clean
>>>>> state when removing or when probe is failing.
>>>>
>>>> Just a data point: driver core already clears drvdata, so as long as
>>>> dev_get/set_drvdata() is the same as hid_get/set_drvdata() no special
>>>> handling is needed in HID core.
>>>
>>> You are correct (as always ;-P). I'll drop the hid-core changes and
>>> send a v2 ASAP.
>>
>> I was just looking at the same thing. I should have known about this
>> since I wrote the patch to make the core clear drvdata. I should have
>> mentioned that, but I was assuming that the hid code was special somehow,
>> however now I see it just uses a regular device_add, so indeed the core
>> changes are not necessary.
>>
>> Given the large hid-logitech-*.c patch-set it might be easier for
>> Jiri if you split out the hid-logitech-*.c changes into a separate
>> patch, then he can keep that on his logitech branch (just a thought).
>>
>
> I do not expect the logitech changes to now create some big conflict.
> But looking at the current for-5.2* branches, I should probably split
> the series per driver given that there is no more dependency on
> hid-core. This way, we can take the logitech changes in the
> for-5.2/logitech branch, and you can rebase your work on top of it.
Great, thank you.
Regards,
Hans
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-02 14:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-28 15:51 [PATCH] HID: force setting drvdata to NULL when removing the driver Benjamin Tissoires
2019-03-29 22:26 ` Dmitry Torokhov
2019-04-02 13:44 ` Benjamin Tissoires
2019-04-02 13:52 ` Hans de Goede
2019-04-02 14:00 ` Benjamin Tissoires
2019-04-02 14:02 ` Hans de Goede
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).