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

[Pali Rohár]

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