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

On Tue, Oct 19, 2021 at 8:36 AM Jason Gerecke [off-list ref] wrote:

On Sun, Oct 17, 2021 at 9:53 PM Dmitry Torokhov [off-list ref] wrote:

quoted

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 [off-list ref] wrote:

quoted

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