Re: [PATCH] HID: wacom: Make use of the helper function devm_add_action_or_reset()
From: Cai Huoqing <hidden>
Date: 2021-10-15 02:58:31
Also in:
lkml
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 3. (wacom_sys.c:893): The `wacom_wac->shared` pointer is set 4. (wacom_sys.c:895): We call `devm_add_action_or_reset` 5. Adding the action fails, causing the `devm_add_action_or_reset` to immediately call `wacom_remove_shared_data` 6. (wacom_sys.c:866): The reference count of `data->kref` is decremented, triggering a call to `wacom_release_shared_data` 7. (wacom_sys.c:844): The `wacom_release_shared_data` function blocks on the previously-locked `wacom_udev_list_lock` mutex I *think* it would be safe to shrink the critical section in `wacom_add_shared_data` to end before the call to `devm_add_action_or_reset`. It might be possible to push the unlock as far back as line 892. That should be sufficient to protect `wacom_udev_list` and ensure that we don't accidentally create two "shared" objects when only one is desired. I'll defer to Ping since it looks like she added the mutex though :) Jason On Thu, Oct 7, 2021 at 4:39 AM Jiri Kosina [off-list ref] wrote: On Wed, 22 Sep 2021, Cai Huoqing wrote:quoted
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 <redacted>CCing Jason and Ping to Ack this for the Wacom driver.quoted
--- drivers/hid/wacom_sys.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c index 93f49b766376..3aed7ba249f7 100644 --- a/drivers/hid/wacom_sys.c +++ b/drivers/hid/wacom_sys.c@@ -892,10 +892,9 @@ static int wacom_add_shared_data(struct hid_device*hdev)quoted
wacom_wac->shared = &data->shared; - retval = devm_add_action(&hdev->dev, wacom_remove_shared_data,wacom);quoted
+ retval = devm_add_action_or_reset(&hdev->dev,wacom_remove_shared_data, wacom);quoted
if (retval) { mutex_unlock(&wacom_udev_list_lock); - wacom_remove_shared_data(wacom); return retval; } -- 2.25.1
quoted hunk
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); 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