diff mbox series

[v4,3/7] ACPI: button: Access input device's users under appropriate mutex

Message ID 20200608112211.12125-4-andrzej.p@collabora.com (mailing list archive)
State Under Review
Headers show
Series [v4,1/7] Input: add input_device_enabled() | expand

Commit Message

Andrzej Pietrasiewicz June 8, 2020, 11:22 a.m. UTC
Inspecting input device's 'users' member should be done under device's
mutex, so add appropriate invocations.

Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
---
 drivers/acpi/button.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki June 24, 2020, 3 p.m. UTC | #1
On Mon, Jun 8, 2020 at 1:22 PM Andrzej Pietrasiewicz
<andrzej.p@collabora.com> wrote:
>
> Inspecting input device's 'users' member should be done under device's
> mutex, so add appropriate invocations.
>
> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>

This looks like a fix that might be applied independently of the other
patches in the series.

Do you want me to pick it up?

> ---
>  drivers/acpi/button.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 78cfc70cb320..ff7ab291f678 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -456,13 +456,16 @@ static int acpi_button_resume(struct device *dev)
>  {
>         struct acpi_device *device = to_acpi_device(dev);
>         struct acpi_button *button = acpi_driver_data(device);
> +       struct input_dev *input = button->input;
>
>         button->suspended = false;
> -       if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) {
> +       mutex_lock(&input->mutex);
> +       if (button->type == ACPI_BUTTON_TYPE_LID && input->users) {
>                 button->last_state = !!acpi_lid_evaluate_state(device);
>                 button->last_time = ktime_get();
>                 acpi_lid_initialize_state(device);
>         }
> +       mutex_unlock(&input->mutex);
>         return 0;
>  }
>  #endif
> --
> 2.17.1
>
Dmitry Torokhov June 25, 2020, 5:23 a.m. UTC | #2
On Wed, Jun 24, 2020 at 05:00:09PM +0200, Rafael J. Wysocki wrote:
> On Mon, Jun 8, 2020 at 1:22 PM Andrzej Pietrasiewicz
> <andrzej.p@collabora.com> wrote:
> >
> > Inspecting input device's 'users' member should be done under device's
> > mutex, so add appropriate invocations.
> >
> > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> 
> This looks like a fix that might be applied independently of the other
> patches in the series.
> 
> Do you want me to pick it up?

If you pick it we'll have to have a dance with this series. Can I apply
instead?

I do not think this change has any practical effect as nobody
attaches/detached input handlers or opening/closing input devices when
system goes through device resume phase.

> 
> > ---
> >  drivers/acpi/button.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> > index 78cfc70cb320..ff7ab291f678 100644
> > --- a/drivers/acpi/button.c
> > +++ b/drivers/acpi/button.c
> > @@ -456,13 +456,16 @@ static int acpi_button_resume(struct device *dev)
> >  {
> >         struct acpi_device *device = to_acpi_device(dev);
> >         struct acpi_button *button = acpi_driver_data(device);
> > +       struct input_dev *input = button->input;
> >
> >         button->suspended = false;
> > -       if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) {
> > +       mutex_lock(&input->mutex);
> > +       if (button->type == ACPI_BUTTON_TYPE_LID && input->users) {
> >                 button->last_state = !!acpi_lid_evaluate_state(device);
> >                 button->last_time = ktime_get();
> >                 acpi_lid_initialize_state(device);
> >         }
> > +       mutex_unlock(&input->mutex);
> >         return 0;
> >  }
> >  #endif
> > --
> > 2.17.1
> >

Thanks.
Rafael J. Wysocki June 25, 2020, 10:55 a.m. UTC | #3
On Thu, Jun 25, 2020 at 7:23 AM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Jun 24, 2020 at 05:00:09PM +0200, Rafael J. Wysocki wrote:
> > On Mon, Jun 8, 2020 at 1:22 PM Andrzej Pietrasiewicz
> > <andrzej.p@collabora.com> wrote:
> > >
> > > Inspecting input device's 'users' member should be done under device's
> > > mutex, so add appropriate invocations.
> > >
> > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> >
> > This looks like a fix that might be applied independently of the other
> > patches in the series.
> >
> > Do you want me to pick it up?
>
> If you pick it we'll have to have a dance with this series. Can I apply
> instead?

Yes, please.

Also feel free to add

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

to it.

> I do not think this change has any practical effect as nobody
> attaches/detached input handlers or opening/closing input devices when
> system goes through device resume phase.

Indeed.

Thanks!
Dmitry Torokhov Oct. 5, 2020, 5:08 a.m. UTC | #4
On Thu, Jun 25, 2020 at 12:55:29PM +0200, Rafael J. Wysocki wrote:
> On Thu, Jun 25, 2020 at 7:23 AM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > On Wed, Jun 24, 2020 at 05:00:09PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Jun 8, 2020 at 1:22 PM Andrzej Pietrasiewicz
> > > <andrzej.p@collabora.com> wrote:
> > > >
> > > > Inspecting input device's 'users' member should be done under device's
> > > > mutex, so add appropriate invocations.
> > > >
> > > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
> > >
> > > This looks like a fix that might be applied independently of the other
> > > patches in the series.
> > >
> > > Do you want me to pick it up?
> >
> > If you pick it we'll have to have a dance with this series. Can I apply
> > instead?
> 
> Yes, please.
> 
> Also feel free to add
> 
> Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> to it.

Looking at the driver I think the patch and the original use of
input->users is not proper. I'll post another patch addressing this
shortly.

Thanks.
diff mbox series

Patch

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 78cfc70cb320..ff7ab291f678 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -456,13 +456,16 @@  static int acpi_button_resume(struct device *dev)
 {
 	struct acpi_device *device = to_acpi_device(dev);
 	struct acpi_button *button = acpi_driver_data(device);
+	struct input_dev *input = button->input;
 
 	button->suspended = false;
-	if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) {
+	mutex_lock(&input->mutex);
+	if (button->type == ACPI_BUTTON_TYPE_LID && input->users) {
 		button->last_state = !!acpi_lid_evaluate_state(device);
 		button->last_time = ktime_get();
 		acpi_lid_initialize_state(device);
 	}
+	mutex_unlock(&input->mutex);
 	return 0;
 }
 #endif