linux-input.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 1/2] RFC: HID: wacom: Shrink critical section in `wacom_add_shared_data`
@ 2021-10-15  2:28 Cai Huoqing
  2021-10-15  2:28 ` [PATCH v2 2/2] HID: wacom: Make use of the helper function devm_add_action_or_reset() Cai Huoqing
  0 siblings, 1 reply; 2+ messages in thread
From: Cai Huoqing @ 2021-10-15  2:28 UTC (permalink / raw)
  To: killertofu, Ping.Cheng
  Cc: jikos, caihuoqing, benjamin.tissoires, linux-input, linux-kernel,
	Jason.Gerecke, skomra, joshua.dickens, Jason Gerecke

From: Jason Gerecke <jason.gerecke@wacom.com>

The size of the critical section in this function appears to be larger
than necessary. The `wacom_udev_list_lock` exists to ensure that one
interface cannot begin checking if a shared object exists while a second
interface is doing the same (otherwise both could determine that no
object exists yet and create their own independent objects rather than
sharing just one). It should be safe for the critical section to end
once a fresly-allocated shared object would be found by other threads
(i.e., once it has been added to `wacom_udev_list`, which is looped
over by `wacom_get_hdev_data`).

This commit is a necessary pre-requisite for a later change to swap the
use of `devm_add_action` with `devm_add_action_or_reset`, which would
otherwise deadlock in its error case.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
v1->v2:
	*Fix the repeated word 'that' in changelog
	*Sort to patch series with [PATCH v2 2/2] from Cai

 drivers/hid/wacom_sys.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 93f49b766376..62f50e4b837d 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -881,8 +881,8 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 	if (!data) {
 		data = kzalloc(sizeof(struct wacom_hdev_data), GFP_KERNEL);
 		if (!data) {
-			retval = -ENOMEM;
-			goto out;
+			mutex_unlock(&wacom_udev_list_lock);
+			return -ENOMEM;
 		}
 
 		kref_init(&data->kref);
@@ -890,11 +890,12 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 		list_add_tail(&data->list, &wacom_udev_list);
 	}
 
+	mutex_unlock(&wacom_udev_list_lock);
+
 	wacom_wac->shared = &data->shared;
 
 	retval = devm_add_action(&hdev->dev, wacom_remove_shared_data, wacom);
 	if (retval) {
-		mutex_unlock(&wacom_udev_list_lock);
 		wacom_remove_shared_data(wacom);
 		return retval;
 	}
@@ -904,8 +905,6 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 	else if (wacom_wac->features.device_type & WACOM_DEVICETYPE_PEN)
 		wacom_wac->shared->pen = hdev;
 
-out:
-	mutex_unlock(&wacom_udev_list_lock);
 	return retval;
 }
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

* [PATCH v2 2/2] HID: wacom: Make use of the helper function devm_add_action_or_reset()
  2021-10-15  2:28 [PATCH v2 1/2] RFC: HID: wacom: Shrink critical section in `wacom_add_shared_data` Cai Huoqing
@ 2021-10-15  2:28 ` Cai Huoqing
  0 siblings, 0 replies; 2+ messages in thread
From: Cai Huoqing @ 2021-10-15  2:28 UTC (permalink / raw)
  To: killertofu, Ping.Cheng
  Cc: jikos, caihuoqing, benjamin.tissoires, linux-input, linux-kernel,
	Jason.Gerecke, skomra, joshua.dickens

The helper function devm_add_action_or_reset() will internally
call devm_add_action(), and if devm_add_action() fails then it will
execute the action mentioned and return the error code. So
use devm_add_action_or_reset() instead of devm_add_action()
to simplify the error handling, reduce the code.

Signed-off-by: Cai Huoqing <caihuoqing@baidu.com>
---
v1->v2:
	*Sort to patch series with [PATCH v2 1/2] from Jason

 drivers/hid/wacom_sys.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 62f50e4b837d..2717d39600b4 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -894,11 +894,9 @@ static int wacom_add_shared_data(struct hid_device *hdev)
 
 	wacom_wac->shared = &data->shared;
 
-	retval = devm_add_action(&hdev->dev, wacom_remove_shared_data, wacom);
-	if (retval) {
-		wacom_remove_shared_data(wacom);
+	retval = devm_add_action_or_reset(&hdev->dev, wacom_remove_shared_data, wacom);
+	if (retval)
 		return retval;
-	}
 
 	if (wacom_wac->features.device_type & WACOM_DEVICETYPE_TOUCH)
 		wacom_wac->shared->touch = hdev;
-- 
2.25.1


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-10-15  2:28 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-15  2:28 [PATCH v2 1/2] RFC: HID: wacom: Shrink critical section in `wacom_add_shared_data` Cai Huoqing
2021-10-15  2:28 ` [PATCH v2 2/2] HID: wacom: Make use of the helper function devm_add_action_or_reset() Cai Huoqing

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