Input: alps - use guard notation when acquiring mutex

[PATCH] Input: alps - use guard notation when acquiring mutex

Posted by Dmitry Torokhov 1 year, 3 months ago

This 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/mouse/alps.c | 48 +++++++++++++++++++++-----------------
 1 file changed, 27 insertions(+), 21 deletions(-)

diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index d5ef5a112d6f..4e37fc3f1a9e 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -1396,24 +1396,16 @@ static bool alps_is_valid_package_ss4_v2(struct psmouse *psmouse)
 
 static DEFINE_MUTEX(alps_mutex);
 
-static void alps_register_bare_ps2_mouse(struct work_struct *work)
+static int alps_do_register_bare_ps2_mouse(struct alps_data *priv)
 {
-	struct alps_data *priv =
-		container_of(work, struct alps_data, dev3_register_work.work);
 	struct psmouse *psmouse = priv->psmouse;
 	struct input_dev *dev3;
-	int error = 0;
-
-	mutex_lock(&alps_mutex);
-
-	if (priv->dev3)
-		goto out;
+	int error;
 
 	dev3 = input_allocate_device();
 	if (!dev3) {
 		psmouse_err(psmouse, "failed to allocate secondary device\n");
-		error = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 
 	snprintf(priv->phys3, sizeof(priv->phys3), "%s/%s",
@@ -1446,21 +1438,35 @@ static void alps_register_bare_ps2_mouse(struct work_struct *work)
 		psmouse_err(psmouse,
 			    "failed to register secondary device: %d\n",
 			    error);
-		input_free_device(dev3);
-		goto out;
+		goto err_free_input;
 	}
 
 	priv->dev3 = dev3;
+	return 0;
 
-out:
-	/*
-	 * Save the error code so that we can detect that we
-	 * already tried to create the device.
-	 */
-	if (error)
-		priv->dev3 = ERR_PTR(error);
+err_free_input:
+	input_free_device(dev3);
+	return error;
+}
 
-	mutex_unlock(&alps_mutex);
+static void alps_register_bare_ps2_mouse(struct work_struct *work)
+{
+	struct alps_data *priv = container_of(work, struct alps_data,
+					      dev3_register_work.work);
+	int error;
+
+	guard(mutex)(&alps_mutex);
+
+	if (!priv->dev3) {
+		error = alps_do_register_bare_ps2_mouse(priv);
+		if (error) {
+			/*
+			 * Save the error code so that we can detect that we
+			 * already tried to create the device.
+			 */
+			priv->dev3 = ERR_PTR(error);
+		}
+	}
 }
 
 static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
-- 
2.46.0.295.g3b9ea8a38a-goog


-- 
Dmitry

Re: [PATCH] Input: alps - use guard notation when acquiring mutex

Posted by Markus Elfring 1 year, 3 months ago

> This makes the code …

Would you ever like to improve such a change description with imperative wordings?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.11-rc5#n94

Regards,
Markus

Re: [PATCH] Input: alps - use guard notation when acquiring mutex

Posted by Pali Rohár 1 year, 3 months ago

On Saturday 24 August 2024 22:30:57 Dmitry Torokhov wrote:
> This 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/mouse/alps.c | 48 +++++++++++++++++++++-----------------
>  1 file changed, 27 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index d5ef5a112d6f..4e37fc3f1a9e 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1396,24 +1396,16 @@ static bool alps_is_valid_package_ss4_v2(struct psmouse *psmouse)
>  
>  static DEFINE_MUTEX(alps_mutex);
>  
> -static void alps_register_bare_ps2_mouse(struct work_struct *work)
> +static int alps_do_register_bare_ps2_mouse(struct alps_data *priv)
>  {
> -	struct alps_data *priv =
> -		container_of(work, struct alps_data, dev3_register_work.work);
>  	struct psmouse *psmouse = priv->psmouse;
>  	struct input_dev *dev3;
> -	int error = 0;
> -
> -	mutex_lock(&alps_mutex);
> -
> -	if (priv->dev3)
> -		goto out;
> +	int error;
>  
>  	dev3 = input_allocate_device();
>  	if (!dev3) {
>  		psmouse_err(psmouse, "failed to allocate secondary device\n");
> -		error = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
>  
>  	snprintf(priv->phys3, sizeof(priv->phys3), "%s/%s",
> @@ -1446,21 +1438,35 @@ static void alps_register_bare_ps2_mouse(struct work_struct *work)
>  		psmouse_err(psmouse,
>  			    "failed to register secondary device: %d\n",
>  			    error);
> -		input_free_device(dev3);
> -		goto out;
> +		goto err_free_input;
>  	}
>  
>  	priv->dev3 = dev3;
> +	return 0;
>  
> -out:
> -	/*
> -	 * Save the error code so that we can detect that we
> -	 * already tried to create the device.
> -	 */
> -	if (error)
> -		priv->dev3 = ERR_PTR(error);
> +err_free_input:
> +	input_free_device(dev3);
> +	return error;
> +}
>  
> -	mutex_unlock(&alps_mutex);
> +static void alps_register_bare_ps2_mouse(struct work_struct *work)
> +{
> +	struct alps_data *priv = container_of(work, struct alps_data,
> +					      dev3_register_work.work);
> +	int error;
> +
> +	guard(mutex)(&alps_mutex);
> +
> +	if (!priv->dev3) {
> +		error = alps_do_register_bare_ps2_mouse(priv);
> +		if (error) {
> +			/*
> +			 * Save the error code so that we can detect that we
> +			 * already tried to create the device.
> +			 */
> +			priv->dev3 = ERR_PTR(error);
> +		}
> +	}
>  }
>  
>  static void alps_report_bare_ps2_packet(struct psmouse *psmouse,
> -- 
> 2.46.0.295.g3b9ea8a38a-goog
> 
> 
> -- 
> Dmitry

Hello, I'm not familiar with new guards and their usage. But if this is
a preferred way for handling mutexes then go ahead.

I just looked at the code and I do not see any obvious error neither in
old nor in new version.

Re: [PATCH] Input: alps - use guard notation when acquiring mutex

Posted by Dmitry Torokhov 1 year, 3 months ago

Hi Pali,

On Sun, Aug 25, 2024 at 10:13:47PM +0200, Pali Rohár wrote:
> 
> Hello, I'm not familiar with new guards and their usage. But if this is
> a preferred way for handling mutexes then go ahead.
> 
> I just looked at the code and I do not see any obvious error neither in
> old nor in new version.

Is this a Reviewed-by or Acked-by? If neither that is fine too, just
want to make sure.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: alps - use guard notation when acquiring mutex

Posted by Pali Rohár 1 year, 3 months ago

On Thursday 29 August 2024 10:57:41 Dmitry Torokhov wrote:
> Hi Pali,
> 
> On Sun, Aug 25, 2024 at 10:13:47PM +0200, Pali Rohár wrote:
> > 
> > Hello, I'm not familiar with new guards and their usage. But if this is
> > a preferred way for handling mutexes then go ahead.
> > 
> > I just looked at the code and I do not see any obvious error neither in
> > old nor in new version.
> 
> Is this a Reviewed-by or Acked-by? If neither that is fine too, just
> want to make sure.
> 
> Thanks.
> 
> -- 
> Dmitry

Hello, I have looked at it again, and you can add my:
Acked-by: Pali Rohár <pali@kernel.org>

Re: [PATCH] Input: alps - use guard notation when acquiring mutex

Posted by Dmitry Torokhov 1 year, 3 months ago

On Thu, Sep 05, 2024 at 05:44:14PM +0200, Pali Rohár wrote:
> On Thursday 29 August 2024 10:57:41 Dmitry Torokhov wrote:
> > Hi Pali,
> > 
> > On Sun, Aug 25, 2024 at 10:13:47PM +0200, Pali Rohár wrote:
> > > 
> > > Hello, I'm not familiar with new guards and their usage. But if this is
> > > a preferred way for handling mutexes then go ahead.
> > > 
> > > I just looked at the code and I do not see any obvious error neither in
> > > old nor in new version.
> > 
> > Is this a Reviewed-by or Acked-by? If neither that is fine too, just
> > want to make sure.
> > 
> > Thanks.
> > 
> > -- 
> > Dmitry
> 
> Hello, I have looked at it again, and you can add my:
> Acked-by: Pali Rohár <pali@kernel.org>

Thanks Pali.

-- 
Dmitry