Re: [PATCH] ACPI / button: fix handling lid state changes when input device closed
On Mon, Oct 5, 2020 at 7:11 AM 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
> ---
> 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!
Re: [PATCH] ACPI / button: fix handling lid state changes when input device closed
Hi,
On 10/5/20 7:11 AM, dmitry.torok...@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
Patch looks good to me:
Reviewed-by: Hans de Goede
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);
[PATCH] ACPI / button: fix handling lid state changes when input device closed
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
---
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);
--
2.28.0.806.g8561365e88-goog
--
Dmitry
