| Message ID | ZsrBkWIpyEqzClUG@google.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | d5322d537c2355fb2e7b1b0362a21e9c87c4b585 |
| Headers | show |
| Series | Input: alps - use guard notation when acquiring mutex | expand |
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.
> 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
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.
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>
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.
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,
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(-)