* [PATCH] ACPI / button: fix handling lid state changes when input device closed
@ 2020-10-05 5:11 dmitry.torokhov
2020-10-05 8:05 ` Hans de Goede
2020-10-05 11:29 ` Rafael J. Wysocki
0 siblings, 2 replies; 3+ messages in thread
From: dmitry.torokhov @ 2020-10-05 5:11 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Hans de Goede, Andrzej Pietrasiewicz, linux-acpi, linux-kernel
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);
--
2.28.0.806.g8561365e88-goog
--
Dmitry
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ACPI / button: fix handling lid state changes when input device closed
2020-10-05 5:11 [PATCH] ACPI / button: fix handling lid state changes when input device closed dmitry.torokhov
@ 2020-10-05 8:05 ` Hans de Goede
2020-10-05 11:29 ` Rafael J. Wysocki
1 sibling, 0 replies; 3+ messages in thread
From: Hans de Goede @ 2020-10-05 8:05 UTC (permalink / raw)
To: dmitry.torokhov, Rafael J. Wysocki
Cc: Andrzej Pietrasiewicz, linux-acpi, linux-kernel
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);
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ACPI / button: fix handling lid state changes when input device closed
2020-10-05 5:11 [PATCH] ACPI / button: fix handling lid state changes when input device closed dmitry.torokhov
2020-10-05 8:05 ` Hans de Goede
@ 2020-10-05 11:29 ` Rafael J. Wysocki
1 sibling, 0 replies; 3+ messages in thread
From: Rafael J. Wysocki @ 2020-10-05 11:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Rafael J. Wysocki, Hans de Goede, Andrzej Pietrasiewicz,
ACPI Devel Maling List, Linux Kernel Mailing List
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!
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-10-05 11:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 5:11 [PATCH] ACPI / button: fix handling lid state changes when input device closed dmitry.torokhov
2020-10-05 8:05 ` Hans de Goede
2020-10-05 11:29 ` Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).