| Message ID | 20240328012336.145612-2-W_Armin@gmx.de (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [1/4] platform/x86: wmi: Mark simple WMI drivers as legacy-free | expand |
On 3/27/24 6:23 PM, Armin Wolf wrote: > Multiple WMI events can be received concurrently, so multiple instances > of xiaomi_wmi_notify() can be active at the same time. Since the input > device is shared between those handlers, the key input sequence can be > disturbed. Since locking is needed for all notify related calls in all WMI drivers, is there a generic way to add this support in WMI core driver? Like defining some common function which will hold the lock and call driver specific notify handler? I am just thinking aloud.. If it is not feasible, then it is fine. > Fix this by protecting the key input sequence with a mutex. > > Compile-tested only. > > Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") > Signed-off-by: Armin Wolf <W_Armin@gmx.de> > --- > drivers/platform/x86/xiaomi-wmi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c > index 1f5f108d87c0..7efbdc111803 100644 > --- a/drivers/platform/x86/xiaomi-wmi.c > +++ b/drivers/platform/x86/xiaomi-wmi.c > @@ -2,8 +2,10 @@ > /* WMI driver for Xiaomi Laptops */ > > #include <linux/acpi.h> > +#include <linux/device.h> > #include <linux/input.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/wmi.h> > > #include <uapi/linux/input-event-codes.h> > @@ -20,12 +22,21 @@ > > struct xiaomi_wmi { > struct input_dev *input_dev; > + struct mutex key_lock; /* Protects the key event sequence */ > unsigned int key_code; > }; > > +static void xiaomi_mutex_destroy(void *data) > +{ > + struct mutex *lock = data; > + > + mutex_destroy(lock); > +} > + > static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > { > struct xiaomi_wmi *data; > + int ret; > > if (wdev == NULL || context == NULL) > return -EINVAL; > @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) > return -ENOMEM; > dev_set_drvdata(&wdev->dev, data); > > + mutex_init(&data->key_lock); > + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); > + if (ret < 0) > + return ret; > + > data->input_dev = devm_input_allocate_device(&wdev->dev); > if (data->input_dev == NULL) > return -ENOMEM; > @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) > if (data == NULL) > return; > > + mutex_lock(&data->key_lock); > input_report_key(data->input_dev, data->key_code, 1); > input_sync(data->input_dev); > input_report_key(data->input_dev, data->key_code, 0); > input_sync(data->input_dev); > + mutex_unlock(&data->key_lock); > } > > static const struct wmi_device_id xiaomi_wmi_id_table[] = { > -- > 2.39.2 > >
Am 28.03.24 um 03:58 schrieb Kuppuswamy Sathyanarayanan: > On 3/27/24 6:23 PM, Armin Wolf wrote: >> Multiple WMI events can be received concurrently, so multiple instances >> of xiaomi_wmi_notify() can be active at the same time. Since the input >> device is shared between those handlers, the key input sequence can be >> disturbed. > Since locking is needed for all notify related calls in all WMI drivers, > is there a generic way to add this support in WMI core driver? Like > defining some common function which will hold the lock and call > driver specific notify handler? I am just thinking aloud.. If it is not > feasible, then it is fine. Hi, actually, not all notify-related calls need locking. It just so happens that input drivers must protect their key sequence, other WMI drivers might not need to use any locking at all. I would prefer if the WMI driver does the locking, so it can be optimized for each WMI driver. Thanks, Armin Wolf >> Fix this by protecting the key input sequence with a mutex. >> >> Compile-tested only. >> >> Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") >> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >> --- >> drivers/platform/x86/xiaomi-wmi.c | 18 ++++++++++++++++++ >> 1 file changed, 18 insertions(+) >> >> diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c >> index 1f5f108d87c0..7efbdc111803 100644 >> --- a/drivers/platform/x86/xiaomi-wmi.c >> +++ b/drivers/platform/x86/xiaomi-wmi.c >> @@ -2,8 +2,10 @@ >> /* WMI driver for Xiaomi Laptops */ >> >> #include <linux/acpi.h> >> +#include <linux/device.h> >> #include <linux/input.h> >> #include <linux/module.h> >> +#include <linux/mutex.h> >> #include <linux/wmi.h> >> >> #include <uapi/linux/input-event-codes.h> >> @@ -20,12 +22,21 @@ >> >> struct xiaomi_wmi { >> struct input_dev *input_dev; >> + struct mutex key_lock; /* Protects the key event sequence */ >> unsigned int key_code; >> }; >> >> +static void xiaomi_mutex_destroy(void *data) >> +{ >> + struct mutex *lock = data; >> + >> + mutex_destroy(lock); >> +} >> + >> static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) >> { >> struct xiaomi_wmi *data; >> + int ret; >> >> if (wdev == NULL || context == NULL) >> return -EINVAL; >> @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) >> return -ENOMEM; >> dev_set_drvdata(&wdev->dev, data); >> >> + mutex_init(&data->key_lock); >> + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); >> + if (ret < 0) >> + return ret; >> + >> data->input_dev = devm_input_allocate_device(&wdev->dev); >> if (data->input_dev == NULL) >> return -ENOMEM; >> @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) >> if (data == NULL) >> return; >> >> + mutex_lock(&data->key_lock); >> input_report_key(data->input_dev, data->key_code, 1); >> input_sync(data->input_dev); >> input_report_key(data->input_dev, data->key_code, 0); >> input_sync(data->input_dev); >> + mutex_unlock(&data->key_lock); >> } >> >> static const struct wmi_device_id xiaomi_wmi_id_table[] = { >> -- >> 2.39.2 >> >>
On 3/28/24 5:26 PM, Armin Wolf wrote: > Am 28.03.24 um 03:58 schrieb Kuppuswamy Sathyanarayanan: > >> On 3/27/24 6:23 PM, Armin Wolf wrote: >>> Multiple WMI events can be received concurrently, so multiple instances >>> of xiaomi_wmi_notify() can be active at the same time. Since the input >>> device is shared between those handlers, the key input sequence can be >>> disturbed. >> Since locking is needed for all notify related calls in all WMI drivers, >> is there a generic way to add this support in WMI core driver? Like >> defining some common function which will hold the lock and call >> driver specific notify handler? I am just thinking aloud.. If it is not >> feasible, then it is fine. > > Hi, > > actually, not all notify-related calls need locking. It just so happens that > input drivers must protect their key sequence, other WMI drivers might not need > to use any locking at all. Got it. > > I would prefer if the WMI driver does the locking, so it can be optimized for > each WMI driver. Why not implement this support? > > Thanks, > Armin Wolf > >>> Fix this by protecting the key input sequence with a mutex. >>> >>> Compile-tested only. >>> >>> Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") >>> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >>> --- >>> drivers/platform/x86/xiaomi-wmi.c | 18 ++++++++++++++++++ >>> 1 file changed, 18 insertions(+) >>> >>> diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c >>> index 1f5f108d87c0..7efbdc111803 100644 >>> --- a/drivers/platform/x86/xiaomi-wmi.c >>> +++ b/drivers/platform/x86/xiaomi-wmi.c >>> @@ -2,8 +2,10 @@ >>> /* WMI driver for Xiaomi Laptops */ >>> >>> #include <linux/acpi.h> >>> +#include <linux/device.h> >>> #include <linux/input.h> >>> #include <linux/module.h> >>> +#include <linux/mutex.h> >>> #include <linux/wmi.h> >>> >>> #include <uapi/linux/input-event-codes.h> >>> @@ -20,12 +22,21 @@ >>> >>> struct xiaomi_wmi { >>> struct input_dev *input_dev; >>> + struct mutex key_lock; /* Protects the key event sequence */ >>> unsigned int key_code; >>> }; >>> >>> +static void xiaomi_mutex_destroy(void *data) >>> +{ >>> + struct mutex *lock = data; >>> + >>> + mutex_destroy(lock); >>> +} >>> + >>> static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) >>> { >>> struct xiaomi_wmi *data; >>> + int ret; >>> >>> if (wdev == NULL || context == NULL) >>> return -EINVAL; >>> @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) >>> return -ENOMEM; >>> dev_set_drvdata(&wdev->dev, data); >>> >>> + mutex_init(&data->key_lock); >>> + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); >>> + if (ret < 0) >>> + return ret; >>> + >>> data->input_dev = devm_input_allocate_device(&wdev->dev); >>> if (data->input_dev == NULL) >>> return -ENOMEM; >>> @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) >>> if (data == NULL) >>> return; >>> >>> + mutex_lock(&data->key_lock); >>> input_report_key(data->input_dev, data->key_code, 1); >>> input_sync(data->input_dev); >>> input_report_key(data->input_dev, data->key_code, 0); >>> input_sync(data->input_dev); >>> + mutex_unlock(&data->key_lock); >>> } >>> >>> static const struct wmi_device_id xiaomi_wmi_id_table[] = { >>> -- >>> 2.39.2 >>> >>>
Am 29.03.24 um 02:37 schrieb Kuppuswamy Sathyanarayanan: > On 3/28/24 5:26 PM, Armin Wolf wrote: >> Am 28.03.24 um 03:58 schrieb Kuppuswamy Sathyanarayanan: >> >>> On 3/27/24 6:23 PM, Armin Wolf wrote: >>>> Multiple WMI events can be received concurrently, so multiple instances >>>> of xiaomi_wmi_notify() can be active at the same time. Since the input >>>> device is shared between those handlers, the key input sequence can be >>>> disturbed. >>> Since locking is needed for all notify related calls in all WMI drivers, >>> is there a generic way to add this support in WMI core driver? Like >>> defining some common function which will hold the lock and call >>> driver specific notify handler? I am just thinking aloud.. If it is not >>> feasible, then it is fine. >> Hi, >> >> actually, not all notify-related calls need locking. It just so happens that >> input drivers must protect their key sequence, other WMI drivers might not need >> to use any locking at all. > Got it. > >> I would prefer if the WMI driver does the locking, so it can be optimized for >> each WMI driver. > Why not implement this support? Because different WMI drivers will most certainly have different need when it comes to locking. Some might want to use a simple mutex, some might want to use a RW-lock, and others might need something totally different. Implementing all of this inside the WMI driver core will be difficult. Thanks, Armin Wolf >> Thanks, >> Armin Wolf >> >>>> Fix this by protecting the key input sequence with a mutex. >>>> >>>> Compile-tested only. >>>> >>>> Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") >>>> Signed-off-by: Armin Wolf <W_Armin@gmx.de> >>>> --- >>>> drivers/platform/x86/xiaomi-wmi.c | 18 ++++++++++++++++++ >>>> 1 file changed, 18 insertions(+) >>>> >>>> diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c >>>> index 1f5f108d87c0..7efbdc111803 100644 >>>> --- a/drivers/platform/x86/xiaomi-wmi.c >>>> +++ b/drivers/platform/x86/xiaomi-wmi.c >>>> @@ -2,8 +2,10 @@ >>>> /* WMI driver for Xiaomi Laptops */ >>>> >>>> #include <linux/acpi.h> >>>> +#include <linux/device.h> >>>> #include <linux/input.h> >>>> #include <linux/module.h> >>>> +#include <linux/mutex.h> >>>> #include <linux/wmi.h> >>>> >>>> #include <uapi/linux/input-event-codes.h> >>>> @@ -20,12 +22,21 @@ >>>> >>>> struct xiaomi_wmi { >>>> struct input_dev *input_dev; >>>> + struct mutex key_lock; /* Protects the key event sequence */ >>>> unsigned int key_code; >>>> }; >>>> >>>> +static void xiaomi_mutex_destroy(void *data) >>>> +{ >>>> + struct mutex *lock = data; >>>> + >>>> + mutex_destroy(lock); >>>> +} >>>> + >>>> static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) >>>> { >>>> struct xiaomi_wmi *data; >>>> + int ret; >>>> >>>> if (wdev == NULL || context == NULL) >>>> return -EINVAL; >>>> @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) >>>> return -ENOMEM; >>>> dev_set_drvdata(&wdev->dev, data); >>>> >>>> + mutex_init(&data->key_lock); >>>> + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); >>>> + if (ret < 0) >>>> + return ret; >>>> + >>>> data->input_dev = devm_input_allocate_device(&wdev->dev); >>>> if (data->input_dev == NULL) >>>> return -ENOMEM; >>>> @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) >>>> if (data == NULL) >>>> return; >>>> >>>> + mutex_lock(&data->key_lock); >>>> input_report_key(data->input_dev, data->key_code, 1); >>>> input_sync(data->input_dev); >>>> input_report_key(data->input_dev, data->key_code, 0); >>>> input_sync(data->input_dev); >>>> + mutex_unlock(&data->key_lock); >>>> } >>>> >>>> static const struct wmi_device_id xiaomi_wmi_id_table[] = { >>>> -- >>>> 2.39.2 >>>> >>>>
diff --git a/drivers/platform/x86/xiaomi-wmi.c b/drivers/platform/x86/xiaomi-wmi.c index 1f5f108d87c0..7efbdc111803 100644 --- a/drivers/platform/x86/xiaomi-wmi.c +++ b/drivers/platform/x86/xiaomi-wmi.c @@ -2,8 +2,10 @@ /* WMI driver for Xiaomi Laptops */ #include <linux/acpi.h> +#include <linux/device.h> #include <linux/input.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/wmi.h> #include <uapi/linux/input-event-codes.h> @@ -20,12 +22,21 @@ struct xiaomi_wmi { struct input_dev *input_dev; + struct mutex key_lock; /* Protects the key event sequence */ unsigned int key_code; }; +static void xiaomi_mutex_destroy(void *data) +{ + struct mutex *lock = data; + + mutex_destroy(lock); +} + static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) { struct xiaomi_wmi *data; + int ret; if (wdev == NULL || context == NULL) return -EINVAL; @@ -35,6 +46,11 @@ static int xiaomi_wmi_probe(struct wmi_device *wdev, const void *context) return -ENOMEM; dev_set_drvdata(&wdev->dev, data); + mutex_init(&data->key_lock); + ret = devm_add_action_or_reset(&wdev->dev, xiaomi_mutex_destroy, &data->key_lock); + if (ret < 0) + return ret; + data->input_dev = devm_input_allocate_device(&wdev->dev); if (data->input_dev == NULL) return -ENOMEM; @@ -59,10 +75,12 @@ static void xiaomi_wmi_notify(struct wmi_device *wdev, union acpi_object *dummy) if (data == NULL) return; + mutex_lock(&data->key_lock); input_report_key(data->input_dev, data->key_code, 1); input_sync(data->input_dev); input_report_key(data->input_dev, data->key_code, 0); input_sync(data->input_dev); + mutex_unlock(&data->key_lock); } static const struct wmi_device_id xiaomi_wmi_id_table[] = {
Multiple WMI events can be received concurrently, so multiple instances of xiaomi_wmi_notify() can be active at the same time. Since the input device is shared between those handlers, the key input sequence can be disturbed. Fix this by protecting the key input sequence with a mutex. Compile-tested only. Fixes: edb73f4f0247 ("platform/x86: wmi: add Xiaomi WMI key driver") Signed-off-by: Armin Wolf <W_Armin@gmx.de> --- drivers/platform/x86/xiaomi-wmi.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) -- 2.39.2