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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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!
> 
> > > 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