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;
MIME-Version: 1.0
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>
 <nycvar.YFH.7.76.2110181725050.12554@cbobk.fhfr.pm> <CANRwn3Q_LksYwX5x+dKw9OzPcYBQr_N5=5bLpZgNPtd88Zqpfg@mail.gmail.com>
In-Reply-To: <CANRwn3Q_LksYwX5x+dKw9OzPcYBQr_N5=5bLpZgNPtd88Zqpfg@mail.gmail.com>
From:   Jason Gerecke <killertofu@gmail.com>
Date:   Tue, 26 Oct 2021 09:56:41 -0700
Message-ID: <CANRwn3TGkin=4aEKibUicmH-UtRz_SFz7+S6dAsTwXVxRzzi9g@mail.gmail.com>
Subject: Re: [PATCH] HID: wacom: Make use of the helper function devm_add_action_or_reset()
To:     Jiri Kosina <jikos@kernel.org>,
        Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc:     Ping Cheng <pinglinux@gmail.com>,
        Benjamin Tissoires <benjamin.tissoires@gmail.com>,
        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>,
        Cai Huoqing <caihuoqing@baidu.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Tue, Oct 19, 2021 at 8:36 AM Jason Gerecke <killertofu@gmail.com> wrote:
>
> On Sun, Oct 17, 2021 at 9:53 PM Dmitry Torokhov <dmitry.torokhov@gmail.co=
m> wrote:
>>
>> 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.
>
>
>
> On Mon, Oct 18, 2021 at 8:26 AM Jiri Kosina <jikos@kernel.org> wrote:
>>
>> I don't see any issue with that ordering, but I'd also prefer for clarit=
y
>> to keep updating the shared data structure under the mutex protection.
>>
>
> The data behind the "shared" struct (e.g. wacom_wac->shared->pen) is not =
currently under any mutex protection. I don't think mutex protection is nec=
essary, but we can take a look... I believe all of its members are either f=
lags (so already atomic) or initialized during probe and then just used as =
a handle with appropriate NULL checks (but maybe two threads could be simul=
taneously issuing events to the same device?).
>
> If a patch to add mutex protection to the shared struct is necessary, tha=
t's going to be a seperate patch that touches a lot more of the driver.

Following up on this. I took a second look at the shared struct, and
believe that things should work fine during initialization and
steady-state. There are, however, opportunities for e.g. one
device/thread to be removed and set e.g. `shared->touch =3D NULL` while
a second device/thread is attempting to send an event out of that
device. This is going to be very rare and only on disconnect, which is
probably why we've never received reports of real-world issues.

This shared issue is present with or without the changes by Cai and
myself. I would ask that these two patches be merged while we look at
introducing a new mutex to protect the contents of the shared pointer.

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