Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Sun, 17 Oct 2021 21:53:25 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Ping Cheng <pinglinux@gmail.com>
Cc:     Jiri Kosina <jkosina@suse.cz>,
        Benjamin Tissoires <benjamin.tissoires@gmail.com>,
        Jason Gerecke <killertofu@gmail.com>,
        Jiri Kosina <jikos@kernel.org>,
        Benjamin Tissoires <benjamin.tissoires@redhat.com>,
        Linux Input <linux-input@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Aaron Skomra <skomra@gmail.com>,
        "Dickens, Joshua" <joshua.dickens@wacom.com>, caihuoqing@baidu.com
Subject: Re: [PATCH] HID: wacom: Make use of the helper function
 devm_add_action_or_reset()
Message-ID: <YWz9xQizZB04DAqe@google.com>
References: <20210922125939.427-1-caihuoqing@baidu.com>
 <nycvar.YFH.7.76.2110071338010.29107@cbobk.fhfr.pm>
 <CANRwn3SZagP7uCSHVDGMPMqQiKyUQJSjq143_DA1y0UPvsmkAA@mail.gmail.com>
 <DB6PR07MB4278FF50AB23B9B69411CA3B9BB19@DB6PR07MB4278.eurprd07.prod.outlook.com>
 <CANRwn3TTgZ9+T7h81tNShvEB8QWkrbKLPrQSnviFKMHa8Zga_Q@mail.gmail.com>
 <20211015025815.GA3874@LAPTOP-UKSR4ENP.internal.baidu.com>
 <CAF8JNhLF8_f1x1K52ay_cmkKqpNiY7P4kMwt=ia6ws9Yd9uoNQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAF8JNhLF8_f1x1K52ay_cmkKqpNiY7P4kMwt=ia6ws9Yd9uoNQ@mail.gmail.com>
Precedence: bulk

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