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!
> > From 7adc05783c7e3120028d0d089bea224903c24ccd Mon Sep 17 00:00:00 2001
> > From: Jason Gerecke <jason.gerecke@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > ---
> > 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? If there is no concern from
the maintainers, the patch has been:
Reviewed-by: Ping Cheng <ping.cheng@xxxxxxxxx>
Tested-by: Ping Cheng <ping.cheng@xxxxxxxxx>
Cheers,
Ping
> > 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
> >
>
