Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
on all CPUs"), the ACPI core allows multiple notify calls to be active
at the same time. This means that two instances of quickstart_notify()
running at the same time can mess which each others input sequences.
Fix this by protecting the input sequence with a mutex.
Compile-tested only.
Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
Signed-off-by: Armin Wolf <[email protected]>
---
This applies on the branch "review-hans". Maybe we could somehow
document the concurrency rules for ACPI notify handlers?
---
drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
index ba3a7a25dda7..e40f852d42c1 100644
--- a/drivers/platform/x86/quickstart.c
+++ b/drivers/platform/x86/quickstart.c
@@ -18,6 +18,7 @@
#include <linux/input/sparse-keymap.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/mutex.h>
#include <linux/platform_device.h>
#include <linux/sysfs.h>
#include <linux/types.h>
@@ -35,6 +36,7 @@
struct quickstart_data {
struct device *dev;
+ struct mutex input_lock; /* Protects input sequence during notify */
struct input_dev *input_device;
char input_name[32];
char phys[32];
@@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
switch (event) {
case QUICKSTART_EVENT_RUNTIME:
+ mutex_lock(&data->input_lock);
sparse_keymap_report_event(data->input_device, 0x1, 1, true);
+ mutex_unlock(&data->input_lock);
+
acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
break;
default:
@@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
}
+static void quickstart_mutex_destroy(void *data)
+{
+ struct mutex *lock = data;
+
+ mutex_destroy(lock);
+}
+
static int quickstart_probe(struct platform_device *pdev)
{
struct quickstart_data *data;
@@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
data->dev = &pdev->dev;
dev_set_drvdata(&pdev->dev, data);
+ mutex_init(&data->input_lock);
+ ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
+ if (ret < 0)
+ return ret;
+
/* We have to initialize the device wakeup before evaluating GHID because
* doing so will notify the device if the button was used to wake the machine
* from S5.
--
2.39.2
Am 27.03.24 um 22:45 schrieb Armin Wolf:
> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
> on all CPUs"), the ACPI core allows multiple notify calls to be active
> at the same time. This means that two instances of quickstart_notify()
> running at the same time can mess which each others input sequences.
>
> Fix this by protecting the input sequence with a mutex.
>
> Compile-tested only.
Any thoughts on this?
Armin Wolf
> Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
> Signed-off-by: Armin Wolf <[email protected]>
> ---
> This applies on the branch "review-hans". Maybe we could somehow
> document the concurrency rules for ACPI notify handlers?
> ---
> drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
> index ba3a7a25dda7..e40f852d42c1 100644
> --- a/drivers/platform/x86/quickstart.c
> +++ b/drivers/platform/x86/quickstart.c
> @@ -18,6 +18,7 @@
> #include <linux/input/sparse-keymap.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/platform_device.h>
> #include <linux/sysfs.h>
> #include <linux/types.h>
> @@ -35,6 +36,7 @@
>
> struct quickstart_data {
> struct device *dev;
> + struct mutex input_lock; /* Protects input sequence during notify */
> struct input_dev *input_device;
> char input_name[32];
> char phys[32];
> @@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
>
> switch (event) {
> case QUICKSTART_EVENT_RUNTIME:
> + mutex_lock(&data->input_lock);
> sparse_keymap_report_event(data->input_device, 0x1, 1, true);
> + mutex_unlock(&data->input_lock);
> +
> acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
> break;
> default:
> @@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
> acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
> }
>
> +static void quickstart_mutex_destroy(void *data)
> +{
> + struct mutex *lock = data;
> +
> + mutex_destroy(lock);
> +}
> +
> static int quickstart_probe(struct platform_device *pdev)
> {
> struct quickstart_data *data;
> @@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
> data->dev = &pdev->dev;
> dev_set_drvdata(&pdev->dev, data);
>
> + mutex_init(&data->input_lock);
> + ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
> + if (ret < 0)
> + return ret;
> +
> /* We have to initialize the device wakeup before evaluating GHID because
> * doing so will notify the device if the button was used to wake the machine
> * from S5.
> --
> 2.39.2
>
>
Hi,
On 4/6/24 8:57 PM, Armin Wolf wrote:
> Am 27.03.24 um 22:45 schrieb Armin Wolf:
>
>> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
>> on all CPUs"), the ACPI core allows multiple notify calls to be active
>> at the same time. This means that two instances of quickstart_notify()
>> running at the same time can mess which each others input sequences.
>>
>> Fix this by protecting the input sequence with a mutex.
>>
>> Compile-tested only.
>
> Any thoughts on this?
I wonder if we need this at all ?
The input_event() / input_report_key() / input_sync() functions
which underpin sparse_keymap_report_event() all are safe to be called
from multiple threads at the same time AFAIK.
The only thing which can then still go "wrong" if we have
2 sparse_keymap_report_event() functions racing for the same
quickstart button and thus for the same keycode is that we may
end up with:
input_report_key(dev, keycode, 1);
input_report_key(dev, keycode, 1); /* This is a no-op */
input_sync(); /* + another input_sync() somewhere which is a no-op */
input_report_key(dev, keycode, 0);
input_report_key(dev, keycode, 0); /* This is a no-op */
input_sync(); /* + another input_sync() somewhere which is a no-op */
IOW if 2 racing notifiers hit the perfect race conditions then
only 1 key press is reported, instead of 2 which seems like
it is not a problem since arguably if the same event gets
reported twice at the exact same time it probably really
is only a single button press.
Also I think it is highly unlikely we will actually see
2 notifiers for this racing in practice.
So I don't think we need this at all. But if others feel strongly
about adding this I can still merge it... ?
Regards,
Hans
>> Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
>> Signed-off-by: Armin Wolf <[email protected]>
>> ---
>> This applies on the branch "review-hans". Maybe we could somehow
>> document the concurrency rules for ACPI notify handlers?
>> ---
>> drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
>> index ba3a7a25dda7..e40f852d42c1 100644
>> --- a/drivers/platform/x86/quickstart.c
>> +++ b/drivers/platform/x86/quickstart.c
>> @@ -18,6 +18,7 @@
>> #include <linux/input/sparse-keymap.h>
>> #include <linux/kernel.h>
>> #include <linux/module.h>
>> +#include <linux/mutex.h>
>> #include <linux/platform_device.h>
>> #include <linux/sysfs.h>
>> #include <linux/types.h>
>> @@ -35,6 +36,7 @@
>>
>> struct quickstart_data {
>> struct device *dev;
>> + struct mutex input_lock; /* Protects input sequence during notify */
>> struct input_dev *input_device;
>> char input_name[32];
>> char phys[32];
>> @@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
>>
>> switch (event) {
>> case QUICKSTART_EVENT_RUNTIME:
>> + mutex_lock(&data->input_lock);
>> sparse_keymap_report_event(data->input_device, 0x1, 1, true);
>> + mutex_unlock(&data->input_lock);
>> +
>> acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
>> break;
>> default:
>> @@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
>> acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
>> }
>>
>> +static void quickstart_mutex_destroy(void *data)
>> +{
>> + struct mutex *lock = data;
>> +
>> + mutex_destroy(lock);
>> +}
>> +
>> static int quickstart_probe(struct platform_device *pdev)
>> {
>> struct quickstart_data *data;
>> @@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
>> data->dev = &pdev->dev;
>> dev_set_drvdata(&pdev->dev, data);
>>
>> + mutex_init(&data->input_lock);
>> + ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
>> + if (ret < 0)
>> + return ret;
>> +
>> /* We have to initialize the device wakeup before evaluating GHID because
>> * doing so will notify the device if the button was used to wake the machine
>> * from S5.
>> --
>> 2.39.2
>>
>>
>
Am 07.04.24 um 17:32 schrieb Hans de Goede:
> Hi,
>
> On 4/6/24 8:57 PM, Armin Wolf wrote:
>> Am 27.03.24 um 22:45 schrieb Armin Wolf:
>>
>>> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
>>> on all CPUs"), the ACPI core allows multiple notify calls to be active
>>> at the same time. This means that two instances of quickstart_notify()
>>> running at the same time can mess which each others input sequences.
>>>
>>> Fix this by protecting the input sequence with a mutex.
>>>
>>> Compile-tested only.
>> Any thoughts on this?
> I wonder if we need this at all ?
>
> The input_event() / input_report_key() / input_sync() functions
> which underpin sparse_keymap_report_event() all are safe to be called
> from multiple threads at the same time AFAIK.
>
> The only thing which can then still go "wrong" if we have
> 2 sparse_keymap_report_event() functions racing for the same
> quickstart button and thus for the same keycode is that we may
> end up with:
>
> input_report_key(dev, keycode, 1);
> input_report_key(dev, keycode, 1); /* This is a no-op */
> input_sync(); /* + another input_sync() somewhere which is a no-op */
> input_report_key(dev, keycode, 0);
> input_report_key(dev, keycode, 0); /* This is a no-op */
> input_sync(); /* + another input_sync() somewhere which is a no-op */
>
> IOW if 2 racing notifiers hit the perfect race conditions then
> only 1 key press is reported, instead of 2 which seems like
> it is not a problem since arguably if the same event gets
> reported twice at the exact same time it probably really
> is only a single button press.
>
> Also I think it is highly unlikely we will actually see
> 2 notifiers for this racing in practice.
>
> So I don't think we need this at all. But if others feel strongly
> about adding this I can still merge it... ?
>
> Regards,
>
> Hans
Hi,
the locking issue was originally brought up by Ilpo Jarvinen during the review of the lenovo-wmi-camera driver.
Also the race condition can cause an invalid input sequence to be emitted:
input_report_key(dev, keycode, 1);
input_sync();
input_report_key(dev, keycode, 0); // Possible invalid sequence?
input_report_key(dev, keycode, 1);
input_sync();
input_sync();
input_report_key(dev, keycode, 0);
input_sync();
I think this input sequence would be invalid, so we need the locking.
Thanks,
Armin Wolf
>>> Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
>>> Signed-off-by: Armin Wolf <[email protected]>
>>> ---
>>> This applies on the branch "review-hans". Maybe we could somehow
>>> document the concurrency rules for ACPI notify handlers?
>>> ---
>>> drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
>>> 1 file changed, 17 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
>>> index ba3a7a25dda7..e40f852d42c1 100644
>>> --- a/drivers/platform/x86/quickstart.c
>>> +++ b/drivers/platform/x86/quickstart.c
>>> @@ -18,6 +18,7 @@
>>> #include <linux/input/sparse-keymap.h>
>>> #include <linux/kernel.h>
>>> #include <linux/module.h>
>>> +#include <linux/mutex.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/sysfs.h>
>>> #include <linux/types.h>
>>> @@ -35,6 +36,7 @@
>>>
>>> struct quickstart_data {
>>> struct device *dev;
>>> + struct mutex input_lock; /* Protects input sequence during notify */
>>> struct input_dev *input_device;
>>> char input_name[32];
>>> char phys[32];
>>> @@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
>>>
>>> switch (event) {
>>> case QUICKSTART_EVENT_RUNTIME:
>>> + mutex_lock(&data->input_lock);
>>> sparse_keymap_report_event(data->input_device, 0x1, 1, true);
>>> + mutex_unlock(&data->input_lock);
>>> +
>>> acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
>>> break;
>>> default:
>>> @@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
>>> acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
>>> }
>>>
>>> +static void quickstart_mutex_destroy(void *data)
>>> +{
>>> + struct mutex *lock = data;
>>> +
>>> + mutex_destroy(lock);
>>> +}
>>> +
>>> static int quickstart_probe(struct platform_device *pdev)
>>> {
>>> struct quickstart_data *data;
>>> @@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
>>> data->dev = &pdev->dev;
>>> dev_set_drvdata(&pdev->dev, data);
>>>
>>> + mutex_init(&data->input_lock);
>>> + ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
>>> + if (ret < 0)
>>> + return ret;
>>> +
>>> /* We have to initialize the device wakeup before evaluating GHID because
>>> * doing so will notify the device if the button was used to wake the machine
>>> * from S5.
>>> --
>>> 2.39.2
>>>
>>>
>
Hi,
On 4/8/24 8:01 AM, Armin Wolf wrote:
> Am 07.04.24 um 17:32 schrieb Hans de Goede:
>
>> Hi,
>>
>> On 4/6/24 8:57 PM, Armin Wolf wrote:
>>> Am 27.03.24 um 22:45 schrieb Armin Wolf:
>>>
>>>> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
>>>> on all CPUs"), the ACPI core allows multiple notify calls to be active
>>>> at the same time. This means that two instances of quickstart_notify()
>>>> running at the same time can mess which each others input sequences.
>>>>
>>>> Fix this by protecting the input sequence with a mutex.
>>>>
>>>> Compile-tested only.
>>> Any thoughts on this?
>> I wonder if we need this at all ?
>>
>> The input_event() / input_report_key() / input_sync() functions
>> which underpin sparse_keymap_report_event() all are safe to be called
>> from multiple threads at the same time AFAIK.
>>
>> The only thing which can then still go "wrong" if we have
>> 2 sparse_keymap_report_event() functions racing for the same
>> quickstart button and thus for the same keycode is that we may
>> end up with:
>>
>> input_report_key(dev, keycode, 1);
>> input_report_key(dev, keycode, 1); /* This is a no-op */
>> input_sync(); /* + another input_sync() somewhere which is a no-op */
>> input_report_key(dev, keycode, 0);
>> input_report_key(dev, keycode, 0); /* This is a no-op */
>> input_sync(); /* + another input_sync() somewhere which is a no-op */
>>
>> IOW if 2 racing notifiers hit the perfect race conditions then
>> only 1 key press is reported, instead of 2 which seems like
>> it is not a problem since arguably if the same event gets
>> reported twice at the exact same time it probably really
>> is only a single button press.
>>
>> Also I think it is highly unlikely we will actually see
>> 2 notifiers for this racing in practice.
>>
>> So I don't think we need this at all. But if others feel strongly
>> about adding this I can still merge it... ?
>>
>> Regards,
>>
>> Hans
>
> Hi,
>
> the locking issue was originally brought up by Ilpo Jarvinen during the review of the lenovo-wmi-camera driver.
> Also the race condition can cause an invalid input sequence to be emitted:
>
> input_report_key(dev, keycode, 1);
> input_sync();
> input_report_key(dev, keycode, 0); // Possible invalid sequence?
> input_report_key(dev, keycode, 1);
> input_sync();
> input_sync();
> input_report_key(dev, keycode, 0);
> input_sync();
>
>
> I think this input sequence would be invalid, so we need the locking.
The:
input_report_key(dev, keycode, 0); // Possible invalid sequence?
input_report_key(dev, keycode, 1);
input_sync();
Part is equivalent of:
input_report_key(dev, keycode, 1);
input_sync();
Since there is no sync() after the release event it will
never reach userspace.
With that said, I'm still happy to merge this if you
prefer to have the locking in place.
Either way please let me know how you want to proceed.
Regards,
Hans
>
> Thanks,
> Armin Wolf
>
>>>> Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
>>>> Signed-off-by: Armin Wolf <[email protected]>
>>>> ---
>>>> This applies on the branch "review-hans". Maybe we could somehow
>>>> document the concurrency rules for ACPI notify handlers?
>>>> ---
>>>> drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
>>>> 1 file changed, 17 insertions(+)
>>>>
>>>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
>>>> index ba3a7a25dda7..e40f852d42c1 100644
>>>> --- a/drivers/platform/x86/quickstart.c
>>>> +++ b/drivers/platform/x86/quickstart.c
>>>> @@ -18,6 +18,7 @@
>>>> #include <linux/input/sparse-keymap.h>
>>>> #include <linux/kernel.h>
>>>> #include <linux/module.h>
>>>> +#include <linux/mutex.h>
>>>> #include <linux/platform_device.h>
>>>> #include <linux/sysfs.h>
>>>> #include <linux/types.h>
>>>> @@ -35,6 +36,7 @@
>>>>
>>>> struct quickstart_data {
>>>> struct device *dev;
>>>> + struct mutex input_lock; /* Protects input sequence during notify */
>>>> struct input_dev *input_device;
>>>> char input_name[32];
>>>> char phys[32];
>>>> @@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
>>>>
>>>> switch (event) {
>>>> case QUICKSTART_EVENT_RUNTIME:
>>>> + mutex_lock(&data->input_lock);
>>>> sparse_keymap_report_event(data->input_device, 0x1, 1, true);
>>>> + mutex_unlock(&data->input_lock);
>>>> +
>>>> acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
>>>> break;
>>>> default:
>>>> @@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
>>>> acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
>>>> }
>>>>
>>>> +static void quickstart_mutex_destroy(void *data)
>>>> +{
>>>> + struct mutex *lock = data;
>>>> +
>>>> + mutex_destroy(lock);
>>>> +}
>>>> +
>>>> static int quickstart_probe(struct platform_device *pdev)
>>>> {
>>>> struct quickstart_data *data;
>>>> @@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
>>>> data->dev = &pdev->dev;
>>>> dev_set_drvdata(&pdev->dev, data);
>>>>
>>>> + mutex_init(&data->input_lock);
>>>> + ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> /* We have to initialize the device wakeup before evaluating GHID because
>>>> * doing so will notify the device if the button was used to wake the machine
>>>> * from S5.
>>>> --
>>>> 2.39.2
>>>>
>>>>
>>
>
Am 08.04.24 um 09:49 schrieb Hans de Goede:
> Hi,
>
> On 4/8/24 8:01 AM, Armin Wolf wrote:
>> Am 07.04.24 um 17:32 schrieb Hans de Goede:
>>
>>> Hi,
>>>
>>> On 4/6/24 8:57 PM, Armin Wolf wrote:
>>>> Am 27.03.24 um 22:45 schrieb Armin Wolf:
>>>>
>>>>> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
>>>>> on all CPUs"), the ACPI core allows multiple notify calls to be active
>>>>> at the same time. This means that two instances of quickstart_notify()
>>>>> running at the same time can mess which each others input sequences.
>>>>>
>>>>> Fix this by protecting the input sequence with a mutex.
>>>>>
>>>>> Compile-tested only.
>>>> Any thoughts on this?
>>> I wonder if we need this at all ?
>>>
>>> The input_event() / input_report_key() / input_sync() functions
>>> which underpin sparse_keymap_report_event() all are safe to be called
>>> from multiple threads at the same time AFAIK.
>>>
>>> The only thing which can then still go "wrong" if we have
>>> 2 sparse_keymap_report_event() functions racing for the same
>>> quickstart button and thus for the same keycode is that we may
>>> end up with:
>>>
>>> input_report_key(dev, keycode, 1);
>>> input_report_key(dev, keycode, 1); /* This is a no-op */
>>> input_sync(); /* + another input_sync() somewhere which is a no-op */
>>> input_report_key(dev, keycode, 0);
>>> input_report_key(dev, keycode, 0); /* This is a no-op */
>>> input_sync(); /* + another input_sync() somewhere which is a no-op */
>>>
>>> IOW if 2 racing notifiers hit the perfect race conditions then
>>> only 1 key press is reported, instead of 2 which seems like
>>> it is not a problem since arguably if the same event gets
>>> reported twice at the exact same time it probably really
>>> is only a single button press.
>>>
>>> Also I think it is highly unlikely we will actually see
>>> 2 notifiers for this racing in practice.
>>>
>>> So I don't think we need this at all. But if others feel strongly
>>> about adding this I can still merge it... ?
>>>
>>> Regards,
>>>
>>> Hans
>> Hi,
>>
>> the locking issue was originally brought up by Ilpo Jarvinen during the review of the lenovo-wmi-camera driver.
>> Also the race condition can cause an invalid input sequence to be emitted:
>>
>> input_report_key(dev, keycode, 1);
>> input_sync();
>> input_report_key(dev, keycode, 0); // Possible invalid sequence?
>> input_report_key(dev, keycode, 1);
>> input_sync();
>> input_sync();
>> input_report_key(dev, keycode, 0);
>> input_sync();
>>
>>
>> I think this input sequence would be invalid, so we need the locking.
> The:
>
> input_report_key(dev, keycode, 0); // Possible invalid sequence?
> input_report_key(dev, keycode, 1);
> input_sync();
>
> Part is equivalent of:
>
> input_report_key(dev, keycode, 1);
> input_sync();
>
> Since there is no sync() after the release event it will
> never reach userspace.
>
> With that said, I'm still happy to merge this if you
> prefer to have the locking in place.
>
> Either way please let me know how you want to proceed.
>
> Regards,
>
> Hans
I would prefer to have the locking in place, just in case.
Thanks,
Armin Wolf
>> Thanks,
>> Armin Wolf
>>
>>>>> Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
>>>>> Signed-off-by: Armin Wolf <[email protected]>
>>>>> ---
>>>>> This applies on the branch "review-hans". Maybe we could somehow
>>>>> document the concurrency rules for ACPI notify handlers?
>>>>> ---
>>>>> drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
>>>>> 1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
>>>>> index ba3a7a25dda7..e40f852d42c1 100644
>>>>> --- a/drivers/platform/x86/quickstart.c
>>>>> +++ b/drivers/platform/x86/quickstart.c
>>>>> @@ -18,6 +18,7 @@
>>>>> #include <linux/input/sparse-keymap.h>
>>>>> #include <linux/kernel.h>
>>>>> #include <linux/module.h>
>>>>> +#include <linux/mutex.h>
>>>>> #include <linux/platform_device.h>
>>>>> #include <linux/sysfs.h>
>>>>> #include <linux/types.h>
>>>>> @@ -35,6 +36,7 @@
>>>>>
>>>>> struct quickstart_data {
>>>>> struct device *dev;
>>>>> + struct mutex input_lock; /* Protects input sequence during notify */
>>>>> struct input_dev *input_device;
>>>>> char input_name[32];
>>>>> char phys[32];
>>>>> @@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
>>>>>
>>>>> switch (event) {
>>>>> case QUICKSTART_EVENT_RUNTIME:
>>>>> + mutex_lock(&data->input_lock);
>>>>> sparse_keymap_report_event(data->input_device, 0x1, 1, true);
>>>>> + mutex_unlock(&data->input_lock);
>>>>> +
>>>>> acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
>>>>> break;
>>>>> default:
>>>>> @@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
>>>>> acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
>>>>> }
>>>>>
>>>>> +static void quickstart_mutex_destroy(void *data)
>>>>> +{
>>>>> + struct mutex *lock = data;
>>>>> +
>>>>> + mutex_destroy(lock);
>>>>> +}
>>>>> +
>>>>> static int quickstart_probe(struct platform_device *pdev)
>>>>> {
>>>>> struct quickstart_data *data;
>>>>> @@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
>>>>> data->dev = &pdev->dev;
>>>>> dev_set_drvdata(&pdev->dev, data);
>>>>>
>>>>> + mutex_init(&data->input_lock);
>>>>> + ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
>>>>> + if (ret < 0)
>>>>> + return ret;
>>>>> +
>>>>> /* We have to initialize the device wakeup before evaluating GHID because
>>>>> * doing so will notify the device if the button was used to wake the machine
>>>>> * from S5.
>>>>> --
>>>>> 2.39.2
>>>>>
>>>>>
>
Hi,
On 3/27/24 10:45 PM, Armin Wolf wrote:
> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
> on all CPUs"), the ACPI core allows multiple notify calls to be active
> at the same time. This means that two instances of quickstart_notify()
> running at the same time can mess which each others input sequences.
>
> Fix this by protecting the input sequence with a mutex.
>
> Compile-tested only.
>
> Fixes: afd66f2a739e ("platform/x86: Add ACPI quickstart button (PNP0C32) driver")
> Signed-off-by: Armin Wolf <[email protected]>
Thank you for your patch, I've applied this patch to my review-hans
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.
Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.
Regards,
Hans
> ---
> This applies on the branch "review-hans". Maybe we could somehow
> document the concurrency rules for ACPI notify handlers?
> ---
> drivers/platform/x86/quickstart.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/drivers/platform/x86/quickstart.c b/drivers/platform/x86/quickstart.c
> index ba3a7a25dda7..e40f852d42c1 100644
> --- a/drivers/platform/x86/quickstart.c
> +++ b/drivers/platform/x86/quickstart.c
> @@ -18,6 +18,7 @@
> #include <linux/input/sparse-keymap.h>
> #include <linux/kernel.h>
> #include <linux/module.h>
> +#include <linux/mutex.h>
> #include <linux/platform_device.h>
> #include <linux/sysfs.h>
> #include <linux/types.h>
> @@ -35,6 +36,7 @@
>
> struct quickstart_data {
> struct device *dev;
> + struct mutex input_lock; /* Protects input sequence during notify */
> struct input_dev *input_device;
> char input_name[32];
> char phys[32];
> @@ -73,7 +75,10 @@ static void quickstart_notify(acpi_handle handle, u32 event, void *context)
>
> switch (event) {
> case QUICKSTART_EVENT_RUNTIME:
> + mutex_lock(&data->input_lock);
> sparse_keymap_report_event(data->input_device, 0x1, 1, true);
> + mutex_unlock(&data->input_lock);
> +
> acpi_bus_generate_netlink_event(DRIVER_NAME, dev_name(data->dev), event, 0);
> break;
> default:
> @@ -147,6 +152,13 @@ static void quickstart_notify_remove(void *context)
> acpi_remove_notify_handler(handle, ACPI_DEVICE_NOTIFY, quickstart_notify);
> }
>
> +static void quickstart_mutex_destroy(void *data)
> +{
> + struct mutex *lock = data;
> +
> + mutex_destroy(lock);
> +}
> +
> static int quickstart_probe(struct platform_device *pdev)
> {
> struct quickstart_data *data;
> @@ -165,6 +177,11 @@ static int quickstart_probe(struct platform_device *pdev)
> data->dev = &pdev->dev;
> dev_set_drvdata(&pdev->dev, data);
>
> + mutex_init(&data->input_lock);
> + ret = devm_add_action_or_reset(&pdev->dev, quickstart_mutex_destroy, &data->input_lock);
> + if (ret < 0)
> + return ret;
> +
> /* We have to initialize the device wakeup before evaluating GHID because
> * doing so will notify the device if the button was used to wake the machine
> * from S5.
> --
> 2.39.2
>
On Sun, 7 Apr 2024, Hans de Goede wrote:
> On 4/6/24 8:57 PM, Armin Wolf wrote:
> > Am 27.03.24 um 22:45 schrieb Armin Wolf:
> >
> >> Since commit e2ffcda16290 ("ACPI: OSL: Allow Notify () handlers to run
> >> on all CPUs"), the ACPI core allows multiple notify calls to be active
> >> at the same time. This means that two instances of quickstart_notify()
> >> running at the same time can mess which each others input sequences.
> >>
> >> Fix this by protecting the input sequence with a mutex.
> >>
> >> Compile-tested only.
> >
> > Any thoughts on this?
>
> I wonder if we need this at all ?
>
> The input_event() / input_report_key() / input_sync() functions
> which underpin sparse_keymap_report_event() all are safe to be called
> from multiple threads at the same time AFAIK.
>
> The only thing which can then still go "wrong" if we have
> 2 sparse_keymap_report_event() functions racing for the same
> quickstart button and thus for the same keycode is that we may
> end up with:
>
> input_report_key(dev, keycode, 1);
> input_report_key(dev, keycode, 1); /* This is a no-op */
> input_sync(); /* + another input_sync() somewhere which is a no-op */
> input_report_key(dev, keycode, 0);
> input_report_key(dev, keycode, 0); /* This is a no-op */
> input_sync(); /* + another input_sync() somewhere which is a no-op */
>
> IOW if 2 racing notifiers hit the perfect race conditions then
> only 1 key press is reported, instead of 2 which seems like
> it is not a problem since arguably if the same event gets
> reported twice at the exact same time it probably really
> is only a single button press.
>
> Also I think it is highly unlikely we will actually see
> 2 notifiers for this racing in practice.
>
> So I don't think we need this at all. But if others feel strongly
> about adding this I can still merge it... ?
Hi,
I know you already merged this and I agree it's not very likely race but
still it can turn two presses into one which seems unwanted side-effect,
even if it's unlikely to occur in practice.
--
i.