[PATCH RESEND] HID: multitouch: Add memory barriers

[PATCH RESEND] HID: multitouch: Add memory barriers

[Andri Yngvason]

This fixes a race with the release-timer by adding acquire/release
barrier semantics.

I noticed that contacts were sometimes sticking, even with the "sticky
fingers" quirk enabled. This fixes that problem.

Cc: stable@vger.kernel.org
Signed-off-by: Andri Yngvason <andri@yngvason.is>
---
 drivers/hid/hid-multitouch.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 2e72922e36f5..91a4d3fc30e0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1186,7 +1186,7 @@ static void mt_touch_report(struct hid_device *hid,
 	int contact_count = -1;
 
 	/* sticky fingers release in progress, abort */
-	if (test_and_set_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
+	if (test_and_set_bit_lock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
 		return;
 
 	scantime = *app->scantime;
@@ -1267,7 +1267,7 @@ static void mt_touch_report(struct hid_device *hid,
 			del_timer(&td->release_timer);
 	}
 
-	clear_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags);
+	clear_bit_unlock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags);
 }
 
 static int mt_touch_input_configured(struct hid_device *hdev,
@@ -1699,11 +1699,11 @@ static void mt_expired_timeout(struct timer_list *t)
 	 * An input report came in just before we release the sticky fingers,
 	 * it will take care of the sticky fingers.
 	 */
-	if (test_and_set_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
+	if (test_and_set_bit_lock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
 		return;
 	if (test_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags))
 		mt_release_contacts(hdev);
-	clear_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags);
+	clear_bit_unlock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags);
 }
 
 static int mt_probe(struct hid_device *hdev, const struct hid_device_id *id)
-- 
2.37.1

Re: [PATCH RESEND] HID: multitouch: Add memory barriers

[Greg KH]

On Wed, Aug 17, 2022 at 11:32:48AM +0000, Andri Yngvason wrote:
> This fixes a race with the release-timer by adding acquire/release
> barrier semantics.

What race?

> 
> I noticed that contacts were sometimes sticking, even with the "sticky
> fingers" quirk enabled. This fixes that problem.
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Andri Yngvason <andri@yngvason.is>
> ---
>  drivers/hid/hid-multitouch.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 2e72922e36f5..91a4d3fc30e0 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -1186,7 +1186,7 @@ static void mt_touch_report(struct hid_device *hid,
>  	int contact_count = -1;
>  
>  	/* sticky fingers release in progress, abort */
> -	if (test_and_set_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
> +	if (test_and_set_bit_lock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))

So this is now a lock?

Why not just use a real lock instead?

thanks,

greg k-h

Re: [PATCH RESEND] HID: multitouch: Add memory barriers

[Andri Yngvason]

Hi Greg,

mið., 17. ágú. 2022 kl. 11:59 skrifaði Greg KH <gregkh@linuxfoundation.org>:
>
> On Wed, Aug 17, 2022 at 11:32:48AM +0000, Andri Yngvason wrote:
> > This fixes a race with the release-timer by adding acquire/release
> > barrier semantics.
>
> What race?
>

The race is between the release timer and processing of hid input. It
is quite certain that these atomic checks are broken as is because
they're lacking memory barriers and this patch does fix an actual
problem for me.

I must admit that I don't know exactly where the execution reordering
occurs and I haven't checked the generated assembly code, but if you
look at e.g. mt_expired_timeout(), you'll see that there's nothing to
keep the compiler or CPU from hoisting the test_bit() call above the
test_and_set_bit() call.

> >
> > I noticed that contacts were sometimes sticking, even with the "sticky
> > fingers" quirk enabled. This fixes that problem.
> >
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Andri Yngvason <andri@yngvason.is>
> > ---
> >  drivers/hid/hid-multitouch.c | 8 ++++----
> >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > index 2e72922e36f5..91a4d3fc30e0 100644
> > --- a/drivers/hid/hid-multitouch.c
> > +++ b/drivers/hid/hid-multitouch.c
> > @@ -1186,7 +1186,7 @@ static void mt_touch_report(struct hid_device *hid,
> >       int contact_count = -1;
> >
> >       /* sticky fingers release in progress, abort */
> > -     if (test_and_set_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
> > +     if (test_and_set_bit_lock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
>
> So this is now a lock?
>
> Why not just use a real lock instead?
>

I don't know. This stuff was introduced in
9609827458c37d7b2c37f2a9255631c603a5004c by Benjamin Tissoires.

Cheers,
Andri

Re: [PATCH RESEND] HID: multitouch: Add memory barriers

[Greg KH]

On Wed, Aug 17, 2022 at 04:14:20PM +0000, Andri Yngvason wrote:
> Hi Greg,
> 
> mið., 17. ágú. 2022 kl. 11:59 skrifaði Greg KH <gregkh@linuxfoundation.org>:
> >
> > On Wed, Aug 17, 2022 at 11:32:48AM +0000, Andri Yngvason wrote:
> > > This fixes a race with the release-timer by adding acquire/release
> > > barrier semantics.
> >
> > What race?
> >
> 
> The race is between the release timer and processing of hid input. It
> is quite certain that these atomic checks are broken as is because
> they're lacking memory barriers and this patch does fix an actual
> problem for me.

Perhaps you should say that in this changelog then.

> 
> I must admit that I don't know exactly where the execution reordering
> occurs and I haven't checked the generated assembly code, but if you
> look at e.g. mt_expired_timeout(), you'll see that there's nothing to
> keep the compiler or CPU from hoisting the test_bit() call above the
> test_and_set_bit() call.
> 
> > >
> > > I noticed that contacts were sometimes sticking, even with the "sticky
> > > fingers" quirk enabled. This fixes that problem.
> > >
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Andri Yngvason <andri@yngvason.is>
> > > ---
> > >  drivers/hid/hid-multitouch.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> > > index 2e72922e36f5..91a4d3fc30e0 100644
> > > --- a/drivers/hid/hid-multitouch.c
> > > +++ b/drivers/hid/hid-multitouch.c
> > > @@ -1186,7 +1186,7 @@ static void mt_touch_report(struct hid_device *hid,
> > >       int contact_count = -1;
> > >
> > >       /* sticky fingers release in progress, abort */
> > > -     if (test_and_set_bit(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
> > > +     if (test_and_set_bit_lock(MT_IO_FLAGS_RUNNING, &td->mt_io_flags))
> >
> > So this is now a lock?
> >
> > Why not just use a real lock instead?
> >
> 
> I don't know. This stuff was introduced in
> 9609827458c37d7b2c37f2a9255631c603a5004c by Benjamin Tissoires.

Then this needs a "Fixes:" tag?

thanks,

greg k-h