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

On 14 10月 21 10:31:03, Jason Gerecke wrote:

I've attached an RFC patch which shrinks the critical section as I
previously described. This would be applied prior to Cai's patch.

Hi Jason,


I haved sorted the patches to a series, and fixed the repeated "that"
in changelog.

like this:
https://patchwork.kernel.org/project/linux-input/patch/20211015022803.3827-1-caihuoqing@baidu.com/

If there are any issue to resend v3 or later, feel free to sort my patch
as series. You also can attach your patch link directly here.

BTW, a minor issue, for 'RFC' prefix, you can use
git format-patch --rfc. It should be showed in subject prefix, like
"[RFC PATCH ..]" (in the link, I missed fixing it).

I would appreciate a few more sets of eyes reviewing / testing the change.

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....



On Thu, Oct 7, 2021 at 3:34 PM Cheng, Ping [off-list ref] wrote:

quoted

I didn’t add mutex_unlock nor work on wacom_remove_shared_data myself.
Benjamin probably sync’d unlock and Dmitry added shared_data for Wacom
driver many years ago. Thank you both!



With that said, I am willing to look into the code and test the patch to
make sure it doesn’t break anything, which may take a few more days…



*From:* Jason Gerecke [mailto:killertofu@gmail.com]
*Sent:* Thursday, October 7, 2021 2:48 PM



I have not tested this, but it seems like the failure case could trigger a
deadlock:



1. (wacom_sys.c:878): The `wacom_udev_list_lock` mutex is locked

2. (wacom_sys.c:888): The `data->kref` refcount is initialized to 1

67 more quoted lines

26 more quoted lines

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

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

11 more quoted lines

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? If there is no concern from
the maintainers, the patch has been:

Reviewed-by: Ping Cheng <ping.cheng@wacom.com>
Tested-by: Ping Cheng <ping.cheng@wacom.com>

Cheers,
Ping

quoted

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

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

24 more quoted lines

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

On Sun, 17 Oct 2021, 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.

I don't see any issue with that ordering, but I'd also prefer for clarity 
to keep updating the shared data structure under the mutex protection.

With that, please send me the series with both patches and the Acks / 
Review-by accumulated, and I'll apply it.

Thanks,

-- 
Jiri Kosina
SUSE Labs