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

From: Jason Gerecke <killertofu@gmail.com>
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>
Subject: Re: [PATCH] HID: wacom: Make use of the helper function devm_add_action_or_reset()
Date: Tue, 26 Oct 2021 09:56:41 -0700	[thread overview]
Message-ID: <CANRwn3TGkin=4aEKibUicmH-UtRz_SFz7+S6dAsTwXVxRzzi9g@mail.gmail.com> (raw)
In-Reply-To: <CANRwn3Q_LksYwX5x+dKw9OzPcYBQr_N5=5bLpZgNPtd88Zqpfg@mail.gmail.com>

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.com> 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 clarity
>> 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 necessary, but we can take a look... I believe all of its members are either flags (so already atomic) or initialized during probe and then just used as a handle with appropriate NULL checks (but maybe two threads could be simultaneously issuing events to the same device?).
>
> If a patch to add mutex protection to the shared struct is necessary, that'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 = 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’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....