| Message ID | 20201005051125.GA3245495@dtor-ws |
|---|---|
| State | Accepted |
| Commit | 21988a8e51479ceffe7b0568b170effabb708dfe |
| Headers | show |
| Series | ACPI / button: fix handling lid state changes when input device closed | expand |
Hi, On 10/5/20 7:11 AM, dmitry.torokhov@gmail.com wrote: > The original intent of 84d3f6b76447 was to delay evaluating lid state until > all drivers have been loaded, with input device being opened from userspace > serving as a signal for this condition. Let's ensure that state updates > happen even if userspace closed (or in the future inhibited) input device. > > Note that if we go through suspend/resume cycle we assume the system has > been fully initialized even if LID input device has not been opened yet. > > This has a side-effect of fixing access to input->users outside of > input->mutex protections by the way of eliminating said accesses and using > driver private flag. > > Fixes: 84d3f6b76447 ("ACPI / button: Delay acpi_lid_initialize_state() until first user space open") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Patch looks good to me: Reviewed-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > --- > drivers/acpi/button.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 78cfc70cb320..b8dd51d8f96d 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -154,6 +154,7 @@ struct acpi_button { > int last_state; > ktime_t last_time; > bool suspended; > + bool lid_state_initialized; > }; > > static struct acpi_device *lid_device; > @@ -384,6 +385,8 @@ static int acpi_lid_update_state(struct acpi_device *device, > > static void acpi_lid_initialize_state(struct acpi_device *device) > { > + struct acpi_button *button = acpi_driver_data(device); > + > switch (lid_init_state) { > case ACPI_BUTTON_LID_INIT_OPEN: > (void)acpi_lid_notify_state(device, 1); > @@ -395,13 +398,14 @@ static void acpi_lid_initialize_state(struct acpi_device *device) > default: > break; > } > + > + button->lid_state_initialized = true; > } > > static void acpi_button_notify(struct acpi_device *device, u32 event) > { > struct acpi_button *button = acpi_driver_data(device); > struct input_dev *input; > - int users; > > switch (event) { > case ACPI_FIXED_HARDWARE_EVENT: > @@ -410,10 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > case ACPI_BUTTON_NOTIFY_STATUS: > input = button->input; > if (button->type == ACPI_BUTTON_TYPE_LID) { > - mutex_lock(&button->input->mutex); > - users = button->input->users; > - mutex_unlock(&button->input->mutex); > - if (users) > + if (button->lid_state_initialized) > acpi_lid_update_state(device, true); > } else { > int keycode; > @@ -458,7 +459,7 @@ static int acpi_button_resume(struct device *dev) > struct acpi_button *button = acpi_driver_data(device); > > button->suspended = false; > - if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) { > + if (button->type == ACPI_BUTTON_TYPE_LID) { > button->last_state = !!acpi_lid_evaluate_state(device); > button->last_time = ktime_get(); > acpi_lid_initialize_state(device); >
On Mon, Oct 5, 2020 at 7:11 AM <dmitry.torokhov@gmail.com> wrote: > > The original intent of 84d3f6b76447 was to delay evaluating lid state until > all drivers have been loaded, with input device being opened from userspace > serving as a signal for this condition. Let's ensure that state updates > happen even if userspace closed (or in the future inhibited) input device. > > Note that if we go through suspend/resume cycle we assume the system has > been fully initialized even if LID input device has not been opened yet. > > This has a side-effect of fixing access to input->users outside of > input->mutex protections by the way of eliminating said accesses and using > driver private flag. > > Fixes: 84d3f6b76447 ("ACPI / button: Delay acpi_lid_initialize_state() until first user space open") > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> > --- > drivers/acpi/button.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c > index 78cfc70cb320..b8dd51d8f96d 100644 > --- a/drivers/acpi/button.c > +++ b/drivers/acpi/button.c > @@ -154,6 +154,7 @@ struct acpi_button { > int last_state; > ktime_t last_time; > bool suspended; > + bool lid_state_initialized; > }; > > static struct acpi_device *lid_device; > @@ -384,6 +385,8 @@ static int acpi_lid_update_state(struct acpi_device *device, > > static void acpi_lid_initialize_state(struct acpi_device *device) > { > + struct acpi_button *button = acpi_driver_data(device); > + > switch (lid_init_state) { > case ACPI_BUTTON_LID_INIT_OPEN: > (void)acpi_lid_notify_state(device, 1); > @@ -395,13 +398,14 @@ static void acpi_lid_initialize_state(struct acpi_device *device) > default: > break; > } > + > + button->lid_state_initialized = true; > } > > static void acpi_button_notify(struct acpi_device *device, u32 event) > { > struct acpi_button *button = acpi_driver_data(device); > struct input_dev *input; > - int users; > > switch (event) { > case ACPI_FIXED_HARDWARE_EVENT: > @@ -410,10 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) > case ACPI_BUTTON_NOTIFY_STATUS: > input = button->input; > if (button->type == ACPI_BUTTON_TYPE_LID) { > - mutex_lock(&button->input->mutex); > - users = button->input->users; > - mutex_unlock(&button->input->mutex); > - if (users) > + if (button->lid_state_initialized) > acpi_lid_update_state(device, true); > } else { > int keycode; > @@ -458,7 +459,7 @@ static int acpi_button_resume(struct device *dev) > struct acpi_button *button = acpi_driver_data(device); > > button->suspended = false; > - if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) { > + if (button->type == ACPI_BUTTON_TYPE_LID) { > button->last_state = !!acpi_lid_evaluate_state(device); > button->last_time = ktime_get(); > acpi_lid_initialize_state(device); > -- Applied as 5.10 material with the R-by from Hans, thanks!
diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c index 78cfc70cb320..b8dd51d8f96d 100644 --- a/drivers/acpi/button.c +++ b/drivers/acpi/button.c @@ -154,6 +154,7 @@ struct acpi_button { int last_state; ktime_t last_time; bool suspended; + bool lid_state_initialized; }; static struct acpi_device *lid_device; @@ -384,6 +385,8 @@ static int acpi_lid_update_state(struct acpi_device *device, static void acpi_lid_initialize_state(struct acpi_device *device) { + struct acpi_button *button = acpi_driver_data(device); + switch (lid_init_state) { case ACPI_BUTTON_LID_INIT_OPEN: (void)acpi_lid_notify_state(device, 1); @@ -395,13 +398,14 @@ static void acpi_lid_initialize_state(struct acpi_device *device) default: break; } + + button->lid_state_initialized = true; } static void acpi_button_notify(struct acpi_device *device, u32 event) { struct acpi_button *button = acpi_driver_data(device); struct input_dev *input; - int users; switch (event) { case ACPI_FIXED_HARDWARE_EVENT: @@ -410,10 +414,7 @@ static void acpi_button_notify(struct acpi_device *device, u32 event) case ACPI_BUTTON_NOTIFY_STATUS: input = button->input; if (button->type == ACPI_BUTTON_TYPE_LID) { - mutex_lock(&button->input->mutex); - users = button->input->users; - mutex_unlock(&button->input->mutex); - if (users) + if (button->lid_state_initialized) acpi_lid_update_state(device, true); } else { int keycode; @@ -458,7 +459,7 @@ static int acpi_button_resume(struct device *dev) struct acpi_button *button = acpi_driver_data(device); button->suspended = false; - if (button->type == ACPI_BUTTON_TYPE_LID && button->input->users) { + if (button->type == ACPI_BUTTON_TYPE_LID) { button->last_state = !!acpi_lid_evaluate_state(device); button->last_time = ktime_get(); acpi_lid_initialize_state(device);
The original intent of 84d3f6b76447 was to delay evaluating lid state until all drivers have been loaded, with input device being opened from userspace serving as a signal for this condition. Let's ensure that state updates happen even if userspace closed (or in the future inhibited) input device. Note that if we go through suspend/resume cycle we assume the system has been fully initialized even if LID input device has not been opened yet. This has a side-effect of fixing access to input->users outside of input->mutex protections by the way of eliminating said accesses and using driver private flag. Fixes: 84d3f6b76447 ("ACPI / button: Delay acpi_lid_initialize_state() until first user space open") Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/acpi/button.c | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-)