[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240825201347.pdphq33cmng4ltds@pali>
Date: Sun, 25 Aug 2024 22:13:47 +0200
From: Pali Rohár <pali@...nel.org>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
Cc: linux-input@...r.kernel.org, Erick Archer <erick.archer@...look.com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Input: alps - use guard notation when acquiring mutex
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@...il.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.
Powered by blists - more mailing lists