Re: [PATCH] HID: wacom: Make use of the helper function devm_add_action_or_reset()

Hi Ping,

On Sun, Oct 17, 2021 at 02:58:47PM -0700, Ping Cheng wrote:

I tested the set of two patches. I didn't see any issues with them
applied. But, while reviewing the patches, I noticed a minor logic
mismatch between the current patch and the original code. I'd hope at
least one of the maintainers (Jiri, Benjamin, or Dimitry) reviews this
patch, especially the part that I commented below, to make sure that
we don't trigger any race condition.

Thank you Huoqing, Jason, and the maintainer team!

quoted

quoted

From 7adc05783c7e3120028d0d089bea224903c24ccd Mon Sep 17 00:00:00 2001
From: Jason Gerecke <jason.gerecke@wacom.com>
Date: Thu, 14 Oct 2021 07:31:31 -0700
Subject: [PATCH] RFC: HID: wacom: Shrink critical section in
 `wacom_add_shared_data`

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

The mutex_unlock was called after devm_add_action is finished, whether
it is a failure or success. The new code calls mutex_unlock before
devm_add_action is executed. Is that ok?

I think this is OK, but I would prefer if assignments that alter the
shared data (i.e. assignment to wacom_wac->shared->pen, etc) would
continue stay under mutex protection, so they need to be pulled up.

With that you can add my

Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

to the both patches, provided that Jason's comes first.

Thanks.

-- 
Dmitry