1. Patchew
  2. linux
  3. [PATCH 0/6] Convert joystick drivers to use new cleanup facilities
  4. View patch

[PATCH 1/6] Input: db9 - use guard notation when acquiring mutex

Dmitry Torokhov posted 6 patches 1 year, 3 months ago
[PATCH 1/6] Input: db9 - use guard notation when acquiring mutex
Posted by Dmitry Torokhov 1 year, 3 months ago 
Using guard notation makes the code more compact and error handling
more robust by ensuring that mutexes are released in all code paths
when control leaves critical section.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/joystick/db9.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c
index 682a29c27832..7ac0cfc3e786 100644
--- a/drivers/input/joystick/db9.c
+++ b/drivers/input/joystick/db9.c
@@ -505,24 +505,22 @@ static int db9_open(struct input_dev *dev)
 {
 	struct db9 *db9 = input_get_drvdata(dev);
 	struct parport *port = db9->pd->port;
-	int err;
 
-	err = mutex_lock_interruptible(&db9->mutex);
-	if (err)
-		return err;
-
-	if (!db9->used++) {
-		parport_claim(db9->pd);
-		parport_write_data(port, 0xff);
-		if (db9_modes[db9->mode].reverse) {
-			parport_data_reverse(port);
-			parport_write_control(port, DB9_NORMAL);
+	scoped_guard(mutex_intr, &db9->mutex) {
+		if (!db9->used++) {
+			parport_claim(db9->pd);
+			parport_write_data(port, 0xff);
+			if (db9_modes[db9->mode].reverse) {
+				parport_data_reverse(port);
+				parport_write_control(port, DB9_NORMAL);
+			}
+			mod_timer(&db9->timer, jiffies + DB9_REFRESH_TIME);
 		}
-		mod_timer(&db9->timer, jiffies + DB9_REFRESH_TIME);
+
+		return 0;
 	}
 
-	mutex_unlock(&db9->mutex);
-	return 0;
+	return -EINTR;
 }
 
 static void db9_close(struct input_dev *dev)
@@ -530,14 +528,14 @@ static void db9_close(struct input_dev *dev)
 	struct db9 *db9 = input_get_drvdata(dev);
 	struct parport *port = db9->pd->port;
 
-	mutex_lock(&db9->mutex);
+	guard(mutex)(&db9->mutex);
+
 	if (!--db9->used) {
 		del_timer_sync(&db9->timer);
 		parport_write_control(port, 0x00);
 		parport_data_forward(port);
 		parport_release(db9->pd);
 	}
-	mutex_unlock(&db9->mutex);
 }
 
 static void db9_attach(struct parport *pp)
-- 
2.46.0.469.g59c65b2a67-goog
Re: [PATCH 1/6] Input: db9 - use guard notation when acquiring mutex
Posted by David Lechner 1 year ago 
On 9/3/24 11:30 PM, Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that mutexes are released in all code paths
> when control leaves critical section.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  drivers/input/joystick/db9.c | 30 ++++++++++++++----------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c
> index 682a29c27832..7ac0cfc3e786 100644
> --- a/drivers/input/joystick/db9.c
> +++ b/drivers/input/joystick/db9.c
> @@ -505,24 +505,22 @@ static int db9_open(struct input_dev *dev)
>  {
>  	struct db9 *db9 = input_get_drvdata(dev);
>  	struct parport *port = db9->pd->port;
> -	int err;
>  
> -	err = mutex_lock_interruptible(&db9->mutex);
> -	if (err)
> -		return err;
> -
> -	if (!db9->used++) {
> -		parport_claim(db9->pd);
> -		parport_write_data(port, 0xff);
> -		if (db9_modes[db9->mode].reverse) {
> -			parport_data_reverse(port);
> -			parport_write_control(port, DB9_NORMAL);
> +	scoped_guard(mutex_intr, &db9->mutex) {
> +		if (!db9->used++) {
> +			parport_claim(db9->pd);
> +			parport_write_data(port, 0xff);
> +			if (db9_modes[db9->mode].reverse) {
> +				parport_data_reverse(port);
> +				parport_write_control(port, DB9_NORMAL);
> +			}
> +			mod_timer(&db9->timer, jiffies + DB9_REFRESH_TIME);
>  		}
> -		mod_timer(&db9->timer, jiffies + DB9_REFRESH_TIME);
> +
> +		return 0;
>  	}
>  
> -	mutex_unlock(&db9->mutex);
> -	return 0;
> +	return -EINTR;

This patch and any others like it are potentially introducing a bug.

From inspecting the source code, it looks like
mutex_lock_interruptible() can return -EINTR, -EALREADY, or -EDEADLK.

Before this patch, the return value of mutex_lock_interruptible() was
passed to the caller. Now, the return value is reduced to pass/fail
and only -EINTR is returned on failure when the reason could have
been something else.
Re: [PATCH 1/6] Input: db9 - use guard notation when acquiring mutex
Posted by Dmitry Torokhov 1 year ago 
On Wed, Nov 20, 2024 at 12:33:36PM -0600, David Lechner wrote:
> On 9/3/24 11:30 PM, Dmitry Torokhov wrote:
> > Using guard notation makes the code more compact and error handling
> > more robust by ensuring that mutexes are released in all code paths
> > when control leaves critical section.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  drivers/input/joystick/db9.c | 30 ++++++++++++++----------------
> >  1 file changed, 14 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/input/joystick/db9.c b/drivers/input/joystick/db9.c
> > index 682a29c27832..7ac0cfc3e786 100644
> > --- a/drivers/input/joystick/db9.c
> > +++ b/drivers/input/joystick/db9.c
> > @@ -505,24 +505,22 @@ static int db9_open(struct input_dev *dev)
> >  {
> >  	struct db9 *db9 = input_get_drvdata(dev);
> >  	struct parport *port = db9->pd->port;
> > -	int err;
> >  
> > -	err = mutex_lock_interruptible(&db9->mutex);
> > -	if (err)
> > -		return err;
> > -
> > -	if (!db9->used++) {
> > -		parport_claim(db9->pd);
> > -		parport_write_data(port, 0xff);
> > -		if (db9_modes[db9->mode].reverse) {
> > -			parport_data_reverse(port);
> > -			parport_write_control(port, DB9_NORMAL);
> > +	scoped_guard(mutex_intr, &db9->mutex) {
> > +		if (!db9->used++) {
> > +			parport_claim(db9->pd);
> > +			parport_write_data(port, 0xff);
> > +			if (db9_modes[db9->mode].reverse) {
> > +				parport_data_reverse(port);
> > +				parport_write_control(port, DB9_NORMAL);
> > +			}
> > +			mod_timer(&db9->timer, jiffies + DB9_REFRESH_TIME);
> >  		}
> > -		mod_timer(&db9->timer, jiffies + DB9_REFRESH_TIME);
> > +
> > +		return 0;
> >  	}
> >  
> > -	mutex_unlock(&db9->mutex);
> > -	return 0;
> > +	return -EINTR;
> 
> This patch and any others like it are potentially introducing a bug.
> 
> From inspecting the source code, it looks like
> mutex_lock_interruptible() can return -EINTR, -EALREADY, or -EDEADLK.
> 
> Before this patch, the return value of mutex_lock_interruptible() was
> passed to the caller. Now, the return value is reduced to pass/fail
> and only -EINTR is returned on failure when the reason could have
> been something else.

It is documented that mutex_lock_interruptible() only returns 0 or
-EINTR. These additional errors only returned from __mutex_lock_common()
for WW mutexes.

If there is another form of scoped_cond_guard() that would make
available error code returned by the constructor of the locking
primitive we can switch to it later.

Thanks.

-- 
Dmitry

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.