* [PATCH 2/2] HID: logitech-hidpp: remove support for the G700 over USB
From: Benjamin Tissoires @ 2019-08-13 13:38 UTC (permalink / raw)
To: Filipe Laíns, Jiri Kosina
Cc: linux-input, linux-kernel, Benjamin Tissoires, stable
In-Reply-To: <20190813133807.12384-1-benjamin.tissoires@redhat.com>
The G700 suffers from the same issue than the G502:
when plugging it in, the driver tries to contact it but it fails.
This timeout is problematic as it introduce a delay in the boot,
and having only the mouse event node means that the hardware
macros keys can not be relayed to the userspace.
Link: https://github.com/libratbag/libratbag/issues/797
Fixes: 91cf9a98ae41 ("HID: logitech-hidpp: make .probe usbhid capable")
Cc: stable@vger.kernel.org # v5.2
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 343052b117a9..0179f7ed77e5 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3751,8 +3751,6 @@ static const struct hid_device_id hidpp_devices[] = {
{ /* Logitech G403 Wireless Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
- { /* Logitech G700 Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC06B) },
{ /* Logitech G703 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
{ /* Logitech G703 Hero Gaming Mouse over USB */
--
2.19.2
^ permalink raw reply related
* [PATCH 1/2] Revert "HID: logitech-hidpp: add USB PID for a few more supported mice"
From: Benjamin Tissoires @ 2019-08-13 13:38 UTC (permalink / raw)
To: Filipe Laíns, Jiri Kosina
Cc: linux-input, linux-kernel, Benjamin Tissoires
In-Reply-To: <20190813133807.12384-1-benjamin.tissoires@redhat.com>
This partially reverts commit 27fc32fd9417968a459d43d9a7c50fd423d53eb9.
It turns out that the G502 has some issues with hid-logitech-hidpp:
when plugging it in, the driver tries to contact it but it fails.
So the driver bails out leaving only the mouse event node available.
This timeout is problematic as it introduce a delay in the boot,
and having only the mouse event node means that the hardware
macros keys can not be relayed to the userspace.
Filipe and I just gave a shot at the following devices:
G403 Wireless (0xC082)
G703 (0xC087)
G703 Hero (0xC090)
G903 (0xC086)
G903 Hero (0xC091)
G Pro (0xC088)
Reverting the device we are not sure that works flawlessly.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 20 --------------------
1 file changed, 20 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 21268c9fa71a..343052b117a9 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3749,30 +3749,10 @@ static const struct hid_device_id hidpp_devices[] = {
{ L27MHZ_DEVICE(HID_ANY_ID) },
- { /* Logitech G203/Prodigy Gaming Mouse */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC084) },
- { /* Logitech G302 Gaming Mouse */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC07F) },
- { /* Logitech G303 Gaming Mouse */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC080) },
- { /* Logitech G400 Gaming Mouse */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC07E) },
{ /* Logitech G403 Wireless Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC082) },
- { /* Logitech G403 Gaming Mouse */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC083) },
- { /* Logitech G403 Hero Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC08F) },
- { /* Logitech G502 Proteus Core Gaming Mouse */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC07D) },
- { /* Logitech G502 Proteus Spectrum Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC332) },
- { /* Logitech G502 Hero Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC08B) },
{ /* Logitech G700 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC06B) },
- { /* Logitech G700s Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC07C) },
{ /* Logitech G703 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC087) },
{ /* Logitech G703 Hero Gaming Mouse over USB */
--
2.19.2
^ permalink raw reply related
* [PATCH 0/2] Fix support of a few Logitech devices broken in 5.3
From: Benjamin Tissoires @ 2019-08-13 13:38 UTC (permalink / raw)
To: Filipe Laíns, Jiri Kosina
Cc: linux-input, linux-kernel, Benjamin Tissoires
Hi Jiri,
another set of patches to send to Linus ASAP.
It turns out that we have been breaking devices, so this should
be sent before 5.3 final.
Cheers,
Benjamin
Benjamin Tissoires (2):
Revert "HID: logitech-hidpp: add USB PID for a few more supported
mice"
HID: logitech-hidpp: remove support for the G700 over USB
drivers/hid/hid-logitech-hidpp.c | 22 ----------------------
1 file changed, 22 deletions(-)
--
2.19.2
^ permalink raw reply
* Re: WARNING in usbtouch_reset_resume
From: Andrey Konovalov @ 2019-08-13 13:12 UTC (permalink / raw)
To: syzbot
Cc: allison, Dmitry Torokhov, Greg Kroah-Hartman, Kate Stewart,
linux-input, LKML, USB list, Michael Ellerman, rfontana,
Henrik Rydberg, syzkaller-bugs, Thomas Gleixner
In-Reply-To: <000000000000c8bea1058f5d6bf6@google.com>
On Mon, Aug 5, 2019 at 1:58 PM syzbot
<syzbot+91f7bbcce580376d784e@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: e96407b4 usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=1104baf8600000
> kernel config: https://syzkaller.appspot.com/x/.config?x=cfa2c18fb6a8068e
> dashboard link: https://syzkaller.appspot.com/bug?extid=91f7bbcce580376d784e
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14199a62600000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=17d94aaa600000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+91f7bbcce580376d784e@syzkaller.appspotmail.com
>
> input: USB Touchscreen 0eef:0002 as
> /devices/platform/dummy_hcd.0/usb1/1-1/1-1:0.180/input/input5
> usb 1-1: reset high-speed USB device number 2 using dummy_hcd
> usb 1-1: Using ep0 maxpacket: 32
> ------------[ cut here ]------------
> DEBUG_LOCKS_WARN_ON(lock->magic != lock)
> WARNING: CPU: 0 PID: 12 at kernel/locking/mutex.c:912 __mutex_lock_common
> kernel/locking/mutex.c:912 [inline]
> WARNING: CPU: 0 PID: 12 at kernel/locking/mutex.c:912
> __mutex_lock+0xd31/0x1360 kernel/locking/mutex.c:1077
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.3.0-rc2+ #25
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> panic+0x2a3/0x6da kernel/panic.c:219
> __warn.cold+0x20/0x4a kernel/panic.c:576
> report_bug+0x262/0x2a0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:179 [inline]
> fixup_bug arch/x86/kernel/traps.c:174 [inline]
> do_error_trap+0x12b/0x1e0 arch/x86/kernel/traps.c:272
> do_invalid_op+0x32/0x40 arch/x86/kernel/traps.c:291
> invalid_op+0x23/0x30 arch/x86/entry/entry_64.S:1026
> RIP: 0010:__mutex_lock_common kernel/locking/mutex.c:912 [inline]
> RIP: 0010:__mutex_lock+0xd31/0x1360 kernel/locking/mutex.c:1077
> Code: d2 0f 85 f6 05 00 00 44 8b 05 7b de 1c 02 45 85 c0 0f 85 0a f4 ff ff
> 48 c7 c6 c0 92 c6 85 48 c7 c7 60 90 c6 85 e8 f4 c4 a6 fb <0f> 0b e9 f0 f3
> ff ff 65 48 8b 1c 25 00 ef 01 00 be 08 00 00 00 48
> RSP: 0018:ffff8881da20f6b0 EFLAGS: 00010286
> RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> RDX: 0000000000000000 RSI: ffffffff8128a0fd RDI: ffffed103b441ec8
> RBP: ffff8881da20f820 R08: ffff8881da1f9800 R09: fffffbfff0d9ee35
> R10: fffffbfff0d9ee34 R11: ffffffff86cf71a3 R12: 0000000000000000
> R13: dffffc0000000000 R14: ffff8881d2e291c8 R15: ffff8881d2fba228
> usbtouch_reset_resume+0xb1/0x170
> drivers/input/touchscreen/usbtouchscreen.c:1611
> usb_resume_interface drivers/usb/core/driver.c:1242 [inline]
> usb_resume_interface.isra.0+0x184/0x390 drivers/usb/core/driver.c:1210
> usb_resume_both+0x26a/0x7b0 drivers/usb/core/driver.c:1412
> __rpm_callback+0x27e/0x3c0 drivers/base/power/runtime.c:355
> rpm_callback+0x18f/0x230 drivers/base/power/runtime.c:485
> rpm_resume+0x10f7/0x1870 drivers/base/power/runtime.c:849
> __pm_runtime_resume+0x103/0x180 drivers/base/power/runtime.c:1076
> pm_runtime_get_sync include/linux/pm_runtime.h:226 [inline]
> usb_autoresume_device+0x1e/0x60 drivers/usb/core/driver.c:1599
> usb_remote_wakeup+0x7b/0xb0 drivers/usb/core/hub.c:3603
> hub_port_connect_change drivers/usb/core/hub.c:5199 [inline]
> port_event drivers/usb/core/hub.c:5359 [inline]
> hub_event+0x246c/0x3640 drivers/usb/core/hub.c:5441
> process_one_work+0x92b/0x1530 kernel/workqueue.c:2269
> worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> kthread+0x318/0x420 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
#syz dup: WARNING in usbtouch_open
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: INFO: trying to register non-static key in usbtouch_reset_resume
From: Andrey Konovalov @ 2019-08-13 13:11 UTC (permalink / raw)
To: syzbot
Cc: allison, Dmitry Torokhov, Greg Kroah-Hartman, linux-input, LKML,
USB list, Michael Ellerman, rfontana, Henrik Rydberg,
syzkaller-bugs, Thomas Gleixner
In-Reply-To: <000000000000773344058bbe0858@google.com>
On Thu, Jun 20, 2019 at 11:33 AM syzbot
<syzbot+933daad9be4e67ba91a9@syzkaller.appspotmail.com> wrote:
>
> syzbot has found a reproducer for the following crash on:
>
> HEAD commit: 9939f56e usb-fuzzer: main usb gadget fuzzer driver
> git tree: https://github.com/google/kasan.git usb-fuzzer
> console output: https://syzkaller.appspot.com/x/log.txt?x=100c063aa00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=df134eda130bb43a
> dashboard link: https://syzkaller.appspot.com/bug?extid=933daad9be4e67ba91a9
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=144fa5e6a00000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13d12de6a00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+933daad9be4e67ba91a9@syzkaller.appspotmail.com
>
> usb 1-1: reset low-speed USB device number 2 using dummy_hcd
> INFO: trying to register non-static key.
> the code is fine but needs lockdep annotation.
> turning off the locking correctness validator.
> CPU: 0 PID: 12 Comm: kworker/0:1 Not tainted 5.2.0-rc5+ #11
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> Google 01/01/2011
> Workqueue: usb_hub_wq hub_event
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xca/0x13e lib/dump_stack.c:113
> assign_lock_key kernel/locking/lockdep.c:775 [inline]
> register_lock_class+0x11ae/0x1240 kernel/locking/lockdep.c:1084
> __lock_acquire+0x11d/0x5340 kernel/locking/lockdep.c:3674
> lock_acquire+0x100/0x2b0 kernel/locking/lockdep.c:4303
> __mutex_lock_common kernel/locking/mutex.c:926 [inline]
> __mutex_lock+0xf9/0x12b0 kernel/locking/mutex.c:1073
> usbtouch_reset_resume+0xb1/0x170
> drivers/input/touchscreen/usbtouchscreen.c:1611
> usb_resume_interface drivers/usb/core/driver.c:1242 [inline]
> usb_resume_interface.isra.0+0x184/0x390 drivers/usb/core/driver.c:1210
> usb_resume_both+0x23d/0x780 drivers/usb/core/driver.c:1412
> __rpm_callback+0x27e/0x3c0 drivers/base/power/runtime.c:355
> rpm_callback+0x18f/0x230 drivers/base/power/runtime.c:485
> rpm_resume+0x10c5/0x1840 drivers/base/power/runtime.c:849
> __pm_runtime_resume+0x103/0x180 drivers/base/power/runtime.c:1076
> pm_runtime_get_sync include/linux/pm_runtime.h:226 [inline]
> usb_autoresume_device+0x1e/0x60 drivers/usb/core/driver.c:1599
> usb_remote_wakeup+0x7b/0xb0 drivers/usb/core/hub.c:3601
> hub_port_connect_change drivers/usb/core/hub.c:5190 [inline]
> port_event drivers/usb/core/hub.c:5350 [inline]
> hub_event+0x23d7/0x3590 drivers/usb/core/hub.c:5432
> process_one_work+0x905/0x1570 kernel/workqueue.c:2269
> worker_thread+0x96/0xe20 kernel/workqueue.c:2415
> kthread+0x30b/0x410 kernel/kthread.c:255
> ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
> dummy_hcd dummy_hcd.0: port status 0x00010100 has changes
> dummy_hcd dummy_hcd.0: port status 0x00010101 has changes
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb 1-1: reset low-speed USB device number 2 using dummy_hcd
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb usb1: dummy_bus_suspend
> usb usb1: dummy_bus_resume
> dummy_hcd dummy_hcd.0: port status 0x00010100 has changes
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb 1-1: reset low-speed USB device number 2 using dummy_hcd
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb usb1: dummy_bus_suspend
> usb usb1: dummy_bus_resume
> dummy_hcd dummy_hcd.0: port status 0x00010100 has changes
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb 1-1: reset low-speed USB device number 2 using dummy_hcd
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb usb1: dummy_bus_suspend
> usb usb1: dummy_bus_resume
> dummy_hcd dummy_hcd.0: port status 0x00010100 has changes
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb 1-1: reset low-speed USB device number 2 using dummy_hcd
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb usb1: dummy_bus_suspend
> usb usb1: dummy_bus_resume
> dummy_hcd dummy_hcd.0: port status 0x00010100 has changes
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
> usb 1-1: reset low-speed USB device number 2 using dummy_hcd
> dummy_hcd dummy_hcd.0: port status 0x00100303 has changes
>
#syz dup: WARNING in usbtouch_open
^ permalink raw reply
* Re: [PATCH] HID: uhid: actually use the err number from userspace
From: David Rheinsberg @ 2019-08-13 10:49 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, linux-kernel
In-Reply-To: <20190812162105.13428-1-benjamin.tissoires@redhat.com>
Hey
On Mon, Aug 12, 2019 at 6:21 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> This can help debugging the situation
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
>
> Hi,
>
> not entirely sure if we can use this in a such simple way.
>
> However, this is useful to mimic device behaviour from userspace.
>
> Cheers,
> Benjamin
>
> drivers/hid/uhid.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
> index fa0cc0899827..2fa32e7fc733 100644
> --- a/drivers/hid/uhid.c
> +++ b/drivers/hid/uhid.c
> @@ -284,7 +284,7 @@ static int uhid_hid_set_report(struct hid_device *hid, unsigned char rnum,
> goto unlock;
>
> if (uhid->report_buf.u.set_report_reply.err)
> - ret = -EIO;
> + ret = -uhid->report_buf.u.set_report_reply.err;
I am generally in favor of this. But:
1) can you do this for both set_report *and* get_report?
2) I think you have to filter some of the error codes. For instance,
if you return one of the -ERESTARTSYS codes, this might cause the
syscall to restart (if auto-restart is enabled on this context). At
the same time, this is not *that* bad. It might even be useful for the
userspace driver to trigger an EINTR. At least we should be aware of
this. So maybe filters are not necessary.. Mhhh. Comments?
Thanks
David
> else
> ret = count;
>
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH] Input: cros_ec_keyb: Add back missing mask for event_type
From: Nicolas Boichat @ 2019-08-13 9:47 UTC (permalink / raw)
To: Fei Shao
Cc: linux-arm Mailing List, Dmitry Torokhov, Benson Leung,
Enric Balletbo i Serra, Guenter Roeck, Ting Shen, Brian Norris,
open list:HID CORE LAYER, lkml
In-Reply-To: <20190813093821.74158-1-fshao@chromium.org>
On Tue, Aug 13, 2019 at 5:38 PM Fei Shao <fshao@chromium.org> wrote:
>
> In the previous patch we didn't mask out event_type in case statement,
> so switches are always picked instead of buttons, which results in
> ChromeOS devices misbehaving when power button is pressed.
> This patch adds back the missing mask.
>
> Fixes: d096aa3eb604 ("Input: cros_ec_keyb: mask out extra flags in event_type")
> Signed-off-by: Fei Shao <fshao@chromium.org>
Reviewed-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> drivers/input/keyboard/cros_ec_keyb.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
> index 38cb6d82d8fe..bef7bee6f05e 100644
> --- a/drivers/input/keyboard/cros_ec_keyb.c
> +++ b/drivers/input/keyboard/cros_ec_keyb.c
> @@ -226,6 +226,8 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> {
> struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
> notifier);
> + uint8_t mkbp_event_type = ckdev->ec->event_data.event_type &
> + EC_MKBP_EVENT_TYPE_MASK;
> u32 val;
> unsigned int ev_type;
>
> @@ -237,7 +239,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> if (queued_during_suspend && !device_may_wakeup(ckdev->dev))
> return NOTIFY_OK;
>
> - switch (ckdev->ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK) {
> + switch (mkbp_event_type) {
> case EC_MKBP_EVENT_KEY_MATRIX:
> pm_wakeup_event(ckdev->dev, 0);
>
> @@ -264,7 +266,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
> case EC_MKBP_EVENT_SWITCH:
> pm_wakeup_event(ckdev->dev, 0);
>
> - if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
> + if (mkbp_event_type == EC_MKBP_EVENT_BUTTON) {
> val = get_unaligned_le32(
> &ckdev->ec->event_data.data.buttons);
> ev_type = EV_KEY;
> --
> 2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply
* Re: [PATCH v4 3/9] nvmem: core: add nvmem_device_find
From: Srinivas Kandagatla @ 2019-08-13 9:40 UTC (permalink / raw)
To: Thomas Bogendoerfer, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Alessandro Zummo,
Alexandre Belloni, Greg Kroah-Hartman, Jiri Slaby,
Evgeniy Polyakov, linux-mips, linux-kernel, linux-input, netdev,
linux-rtc, linux-serial
In-Reply-To: <20190809103235.16338-4-tbogendoerfer@suse.de>
On 09/08/2019 11:32, Thomas Bogendoerfer wrote:
> nvmem_device_find provides a way to search for nvmem devices with
> the help of a match function simlair to bus_find_device.
>
> Signed-off-by: Thomas Bogendoerfer <tbogendoerfer@suse.de>
> ---
> drivers/nvmem/core.c | 62 ++++++++++++++++++++++--------------------
> include/linux/nvmem-consumer.h | 9 ++++++
> 2 files changed, 41 insertions(+), 30 deletions(-)
Have you considered using nvmem_register_notifier() ?
--srini
^ permalink raw reply
* [PATCH] Input: cros_ec_keyb: Add back missing mask for event_type
From: Fei Shao @ 2019-08-13 9:38 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Dmitry Torokhov, Benson Leung, Enric Balletbo i Serra,
Guenter Roeck, Ting Shen, Brian Norris, Fei Shao, linux-input,
linux-kernel
In the previous patch we didn't mask out event_type in case statement,
so switches are always picked instead of buttons, which results in
ChromeOS devices misbehaving when power button is pressed.
This patch adds back the missing mask.
Fixes: d096aa3eb604 ("Input: cros_ec_keyb: mask out extra flags in event_type")
Signed-off-by: Fei Shao <fshao@chromium.org>
---
drivers/input/keyboard/cros_ec_keyb.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/input/keyboard/cros_ec_keyb.c b/drivers/input/keyboard/cros_ec_keyb.c
index 38cb6d82d8fe..bef7bee6f05e 100644
--- a/drivers/input/keyboard/cros_ec_keyb.c
+++ b/drivers/input/keyboard/cros_ec_keyb.c
@@ -226,6 +226,8 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
{
struct cros_ec_keyb *ckdev = container_of(nb, struct cros_ec_keyb,
notifier);
+ uint8_t mkbp_event_type = ckdev->ec->event_data.event_type &
+ EC_MKBP_EVENT_TYPE_MASK;
u32 val;
unsigned int ev_type;
@@ -237,7 +239,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
if (queued_during_suspend && !device_may_wakeup(ckdev->dev))
return NOTIFY_OK;
- switch (ckdev->ec->event_data.event_type & EC_MKBP_EVENT_TYPE_MASK) {
+ switch (mkbp_event_type) {
case EC_MKBP_EVENT_KEY_MATRIX:
pm_wakeup_event(ckdev->dev, 0);
@@ -264,7 +266,7 @@ static int cros_ec_keyb_work(struct notifier_block *nb,
case EC_MKBP_EVENT_SWITCH:
pm_wakeup_event(ckdev->dev, 0);
- if (ckdev->ec->event_data.event_type == EC_MKBP_EVENT_BUTTON) {
+ if (mkbp_event_type == EC_MKBP_EVENT_BUTTON) {
val = get_unaligned_le32(
&ckdev->ec->event_data.data.buttons);
ev_type = EV_KEY;
--
2.23.0.rc1.153.gdeed80330f-goog
^ permalink raw reply related
* Re: [PATCH v4 8/9] MIPS: SGI-IP27: fix readb/writeb addressing
From: Philippe Mathieu-Daudé @ 2019-08-13 8:47 UTC (permalink / raw)
To: Greg Kroah-Hartman, Andy Shevchenko
Cc: Thomas Bogendoerfer, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, Lee Jones, David S. Miller, Srinivas Kandagatla,
Alessandro Zummo, Alexandre Belloni, Jiri Slaby, Evgeniy Polyakov,
linux-mips, Linux Kernel Mailing List, linux-input, netdev,
open list:REAL TIME CLOCK (RTC) SUBSYSTEM,
open list:SERIAL DRIVERS
In-Reply-To: <20190811072907.GA1416@kroah.com>
Hi Thomas,
On 8/11/19 9:29 AM, Greg Kroah-Hartman wrote:
> On Sat, Aug 10, 2019 at 04:22:23PM +0300, Andy Shevchenko wrote:
>> On Fri, Aug 9, 2019 at 1:34 PM Thomas Bogendoerfer
>> <tbogendoerfer@suse.de> wrote:
>>>
>>> Our chosen byte swapping, which is what firmware already uses, is to
>>> do readl/writel by normal lw/sw intructions (data invariance). This
>>> also means we need to mangle addresses for u8 and u16 accesses. The
>>> mangling for 16bit has been done aready, but 8bit one was missing.
>>> Correcting this causes different addresses for accesses to the
>>> SuperIO and local bus of the IOC3 chip. This is fixed by changing
>>> byte order in ioc3 and m48rtc_rtc structs.
>>
>>> /* serial port register map */
>>> struct ioc3_serialregs {
>>> - uint32_t sscr;
>>> - uint32_t stpir;
>>> - uint32_t stcir;
>>> - uint32_t srpir;
>>> - uint32_t srcir;
>>> - uint32_t srtr;
>>> - uint32_t shadow;
>>> + u32 sscr;
>>> + u32 stpir;
>>> + u32 stcir;
>>> + u32 srpir;
>>> + u32 srcir;
>>> + u32 srtr;
>>> + u32 shadow;
>>> };
>>
>> Isn't it a churn? AFAIU kernel documentation the uint32_t is okay to
>> use, just be consistent inside one module / driver.
>> Am I mistaken?
>
> No, but really it uint* shouldn't be used anywhere in the kernel source
> as it does not make sense.
If you respin your series, please send this cleanup as a separate patch.
Thanks,
Phil.
^ permalink raw reply
* Re: WARNING in usbhid_raw_request/usb_submit_urb
From: Oliver Neukum @ 2019-08-13 8:14 UTC (permalink / raw)
To: Hillf Danton, syzbot
Cc: gustavo, Jiri Slaby, andreyknvl, syzkaller-bugs, gregkh, stern,
Jiri Kosina, linux-input, linux-kernel, linux-usb
In-Reply-To: <20190813042649.888-1-hdanton@sina.com>
Am Dienstag, den 13.08.2019, 12:26 +0800 schrieb Hillf Danton:
> [respin with the mess in Cc list cleaned up]
> Followup of commit e3e14de50dff ("HID: fix start/stop cycle in usbhid driver")
>
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1214,6 +1214,8 @@ static void usbhid_stop(struct hid_devic
>
> hid->claimed = 0;
>
> + if (!usbhid->urbin) /* freeing buffers only once */
> + return;
> usb_free_urb(usbhid->urbin);
> usb_free_urb(usbhid->urbctrl);
> usb_free_urb(usbhid->urbout);
This looks rather suspicious. Why is stop() called multiple times?
Do we have a refcounting issue? If not, what controls locking?
Regards
Oliver
^ permalink raw reply
* Re: [PATCH 1/2] HID: do not call hid_set_drvdata(hdev, NULL) in drivers
From: Bruno Prémont @ 2019-08-13 5:53 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jonathan Cameron, Srinivas Pandruvada, Ping Cheng, Jason Gerecke,
Jiri Kosina, linux-input, linux-kernel
In-Reply-To: <20190812162740.15898-2-benjamin.tissoires@redhat.com>
On Mon, 12 Aug 2019 18:27:39 +0200 Benjamin Tissoires wrote:
> This is a common pattern in the HID drivers to reset the drvdata. Some
> do it properly, some do it only in case of failure.
>
> But, this is actually already handled by driver core, so there is no need
> to do it manually.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
For hid-picolcd_core.c:
Acked-by: Bruno Prémont <bonbons@linux-vserver.org>
> ---
> drivers/hid/hid-cougar.c | 6 ++----
> drivers/hid/hid-gfrm.c | 7 -------
> drivers/hid/hid-lenovo.c | 2 --
> drivers/hid/hid-picolcd_core.c | 7 +------
> drivers/hid/hid-sensor-hub.c | 1 -
> 5 files changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
> index e0bb7b34f3a4..4ff3bc1d25e2 100644
> --- a/drivers/hid/hid-cougar.c
> +++ b/drivers/hid/hid-cougar.c
> @@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
> error = hid_parse(hdev);
> if (error) {
> hid_err(hdev, "parse failed\n");
> - goto fail;
> + return error;
> }
>
> if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
> @@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
> error = hid_hw_start(hdev, connect_mask);
> if (error) {
> hid_err(hdev, "hw start failed\n");
> - goto fail;
> + return error;
> }
>
> error = cougar_bind_shared_data(hdev, cougar);
> @@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
>
> fail_stop_and_cleanup:
> hid_hw_stop(hdev);
> -fail:
> - hid_set_drvdata(hdev, NULL);
> return error;
> }
>
> diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
> index 86c317320bf2..699186ff2349 100644
> --- a/drivers/hid/hid-gfrm.c
> +++ b/drivers/hid/hid-gfrm.c
> @@ -123,12 +123,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
> return ret;
> }
>
> -static void gfrm_remove(struct hid_device *hdev)
> -{
> - hid_hw_stop(hdev);
> - hid_set_drvdata(hdev, NULL);
> -}
> -
> static const struct hid_device_id gfrm_devices[] = {
> { HID_BLUETOOTH_DEVICE(0x58, 0x2000),
> .driver_data = GFRM100 },
> @@ -142,7 +136,6 @@ static struct hid_driver gfrm_driver = {
> .name = "gfrm",
> .id_table = gfrm_devices,
> .probe = gfrm_probe,
> - .remove = gfrm_remove,
> .input_mapping = gfrm_input_mapping,
> .raw_event = gfrm_raw_event,
> .input_configured = gfrm_input_configured,
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 364bc7f11d9d..96fa2a2c2cd3 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -866,8 +866,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
>
> led_classdev_unregister(&data_pointer->led_micmute);
> led_classdev_unregister(&data_pointer->led_mute);
> -
> - hid_set_drvdata(hdev, NULL);
> }
>
> static void lenovo_remove_cptkbd(struct hid_device *hdev)
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 5f7a39a5d4af..1b5c63241af0 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -534,8 +534,7 @@ static int picolcd_probe(struct hid_device *hdev,
> data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
> if (data == NULL) {
> hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
> - error = -ENOMEM;
> - goto err_no_cleanup;
> + return -ENOMEM;
> }
>
> spin_lock_init(&data->lock);
> @@ -597,9 +596,6 @@ static int picolcd_probe(struct hid_device *hdev,
> hid_hw_stop(hdev);
> err_cleanup_data:
> kfree(data);
> -err_no_cleanup:
> - hid_set_drvdata(hdev, NULL);
> -
> return error;
> }
>
> @@ -635,7 +631,6 @@ static void picolcd_remove(struct hid_device *hdev)
> picolcd_exit_cir(data);
> picolcd_exit_keys(data);
>
> - hid_set_drvdata(hdev, NULL);
> mutex_destroy(&data->mutex);
> /* Finally, clean up the picolcd data itself */
> kfree(data);
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index be92a6f79687..94c7398b5c27 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -742,7 +742,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
> }
> spin_unlock_irqrestore(&data->lock, flags);
> mfd_remove_devices(&hdev->dev);
> - hid_set_drvdata(hdev, NULL);
> mutex_destroy(&data->mutex);
> }
>
^ permalink raw reply
* Re: [PATCH v4 1/3] dt-bindings: Add pixart vendor
From: Rob Herring @ 2019-08-12 22:46 UTC (permalink / raw)
Cc: robh+dt, mark.rutland, jic23, linux-kernel, linux-iio,
baylibre-upstreaming, dmitry.torokhov, linux-input, devicetree,
Alexandre Mergnat
In-Reply-To: <20190713080455.17513-2-amergnat@baylibre.com>
On Sat, 13 Jul 2019 10:04:53 +0200, Alexandre Mergnat wrote:
> PixArt Imaging Inc. is expertized in CMOS image sensors (CIS),
> capacitive touch controllers and related imaging application development.
>
> Signed-off-by: Alexandre Mergnat <amergnat@baylibre.com>
> ---
> Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
> 1 file changed, 2 insertions(+)
>
Reviewed-by: Rob Herring <robh@kernel.org>
^ permalink raw reply
* Re: [PATCH v4 7/9] mfd: ioc3: Add driver for SGI IOC3 chip
From: Jakub Kicinski @ 2019-08-12 19:52 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Ralf Baechle, Paul Burton, James Hogan, Dmitry Torokhov,
Lee Jones, David S. Miller, Srinivas Kandagatla, Alessandro Zummo,
Alexandre Belloni, Greg Kroah-Hartman, Jiri Slaby,
Evgeniy Polyakov, linux-mips, linux-kernel, linux-input, netdev,
linux-rtc, linux-serial
In-Reply-To: <20190811093212.88635fb1a6c796a073ec71ff@suse.de>
On Sun, 11 Aug 2019 09:32:12 +0200, Thomas Bogendoerfer wrote:
> > Also please don't use stdint types in the kernel, please try checkpatch
> > to catch coding style issues.
>
> my patch already reduces them and checkpatch only warns about usage of printk
> for the network part. Changing that to dev_warn/dev_err in the mfd patch didn't
> seem the right thing to do. As I'm splitting the conversion patch into a few
> steps I could also replace the printks.
Thanks for looking into it. I was referring to the use of uint32_t
instead of u32. Perhaps checkpatch has to be motivated with the --strict
option to point those out?
^ permalink raw reply
* Re: KMSAN: uninit-value in gtco_probe
From: syzbot @ 2019-08-12 18:21 UTC (permalink / raw)
To: dmitry.torokhov, glider, granthernandez, linux-input,
linux-kernel, syzkaller-bugs
In-Reply-To: <0000000000007a3d3b058fea6016@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: 61ccdad1 Revert "drm/bochs: Use shadow buffer for bochs fr..
git tree: kmsan
console output: https://syzkaller.appspot.com/x/log.txt?x=106e8536600000
kernel config: https://syzkaller.appspot.com/x/.config?x=27abc558ecb16a3b
dashboard link: https://syzkaller.appspot.com/bug?extid=bb54195a43a54b1e5e5e
compiler: clang version 9.0.0 (/home/glider/llvm/clang
80fee25776c2fb61e74c1ecb1a523375c2500b69)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1766194a600000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=13d5879a600000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+bb54195a43a54b1e5e5e@syzkaller.appspotmail.com
usb 1-1: config 0 has no interface number 0
usb 1-1: New USB device found, idVendor=078c, idProduct=1002,
bcdDevice=e6.47
usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
usb 1-1: config 0 descriptor??
gtco 1-1:0.219: Collection level already at zero
==================================================================
BUG: KMSAN: uninit-value in parse_hid_report_descriptor
drivers/input/tablet/gtco.c:297 [inline]
BUG: KMSAN: uninit-value in gtco_probe+0x18c7/0x3520
drivers/input/tablet/gtco.c:938
CPU: 1 PID: 621 Comm: kworker/1:1 Not tainted 5.3.0-rc3+ #17
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x191/0x1f0 lib/dump_stack.c:113
kmsan_report+0x162/0x2d0 mm/kmsan/kmsan_report.c:109
__msan_warning+0x75/0xe0 mm/kmsan/kmsan_instr.c:294
parse_hid_report_descriptor drivers/input/tablet/gtco.c:297 [inline]
gtco_probe+0x18c7/0x3520 drivers/input/tablet/gtco.c:938
usb_probe_interface+0xd19/0x1310 drivers/usb/core/driver.c:361
really_probe+0x1373/0x1dc0 drivers/base/dd.c:552
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:709
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:816
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
__device_attach+0x489/0x750 drivers/base/dd.c:882
device_initial_probe+0x4a/0x60 drivers/base/dd.c:929
bus_probe_device+0x131/0x390 drivers/base/bus.c:514
device_add+0x25b5/0x2df0 drivers/base/core.c:2114
usb_set_configuration+0x309f/0x3710 drivers/usb/core/message.c:2027
generic_probe+0xe7/0x280 drivers/usb/core/generic.c:210
usb_probe_device+0x146/0x200 drivers/usb/core/driver.c:266
really_probe+0x1373/0x1dc0 drivers/base/dd.c:552
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:709
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:816
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
__device_attach+0x489/0x750 drivers/base/dd.c:882
device_initial_probe+0x4a/0x60 drivers/base/dd.c:929
bus_probe_device+0x131/0x390 drivers/base/bus.c:514
device_add+0x25b5/0x2df0 drivers/base/core.c:2114
usb_new_device+0x23e5/0x2fb0 drivers/usb/core/hub.c:2536
hub_port_connect drivers/usb/core/hub.c:5098 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
port_event drivers/usb/core/hub.c:5359 [inline]
hub_event+0x581d/0x72f0 drivers/usb/core/hub.c:5441
process_one_work+0x1572/0x1ef0 kernel/workqueue.c:2269
worker_thread+0x111b/0x2460 kernel/workqueue.c:2415
kthread+0x4b5/0x4f0 kernel/kthread.c:256
ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Uninit was stored to memory at:
kmsan_save_stack_with_flags mm/kmsan/kmsan.c:187 [inline]
kmsan_internal_chain_origin+0xcc/0x150 mm/kmsan/kmsan.c:345
__msan_chain_origin+0x6b/0xe0 mm/kmsan/kmsan_instr.c:190
parse_hid_report_descriptor drivers/input/tablet/gtco.c:298 [inline]
gtco_probe+0x1a7c/0x3520 drivers/input/tablet/gtco.c:938
usb_probe_interface+0xd19/0x1310 drivers/usb/core/driver.c:361
really_probe+0x1373/0x1dc0 drivers/base/dd.c:552
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:709
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:816
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
__device_attach+0x489/0x750 drivers/base/dd.c:882
device_initial_probe+0x4a/0x60 drivers/base/dd.c:929
bus_probe_device+0x131/0x390 drivers/base/bus.c:514
device_add+0x25b5/0x2df0 drivers/base/core.c:2114
usb_set_configuration+0x309f/0x3710 drivers/usb/core/message.c:2027
generic_probe+0xe7/0x280 drivers/usb/core/generic.c:210
usb_probe_device+0x146/0x200 drivers/usb/core/driver.c:266
really_probe+0x1373/0x1dc0 drivers/base/dd.c:552
driver_probe_device+0x1ba/0x510 drivers/base/dd.c:709
__device_attach_driver+0x5b8/0x790 drivers/base/dd.c:816
bus_for_each_drv+0x28e/0x3b0 drivers/base/bus.c:454
__device_attach+0x489/0x750 drivers/base/dd.c:882
device_initial_probe+0x4a/0x60 drivers/base/dd.c:929
bus_probe_device+0x131/0x390 drivers/base/bus.c:514
device_add+0x25b5/0x2df0 drivers/base/core.c:2114
usb_new_device+0x23e5/0x2fb0 drivers/usb/core/hub.c:2536
hub_port_connect drivers/usb/core/hub.c:5098 [inline]
hub_port_connect_change drivers/usb/core/hub.c:5213 [inline]
port_event drivers/usb/core/hub.c:5359 [inline]
hub_event+0x581d/0x72f0 drivers/usb/core/hub.c:5441
process_one_work+0x1572/0x1ef0 kernel/workqueue.c:2269
worker_thread+0x111b/0x2460 kernel/workqueue.c:2415
kthread+0x4b5/0x4f0 kernel/kthread.c:256
ret_from_fork+0x35/0x40 arch/x86/entry/entry_64.S:355
Local variable description: ----globalval.i@gtco_probe
Variable was created at:
parse_hid_report_descriptor drivers/input/tablet/gtco.c:221 [inline]
gtco_probe+0xcd6/0x3520 drivers/input/tablet/gtco.c:938
usb_probe_interface+0xd19/0x1310 drivers/usb/core/driver.c:361
==================================================================
^ permalink raw reply
* Re: [PATCH] Input: add support for polling to input devices
From: Dmitry Torokhov @ 2019-08-12 17:11 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: open list:HID CORE LAYER, lkml, Rob Herring
In-Reply-To: <CAO-hwJL1Jq5XjqV32fD7+_nMpi3PhUbrB5QQ+EEs3N6=mBy-1g@mail.gmail.com>
Hi Benjamin,
On Mon, Aug 12, 2019 at 06:50:38PM +0200, Benjamin Tissoires wrote:
> Hi Dmitry,
>
> On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > Separating "normal" and "polled" input devices was a mistake, as often we
> > want to allow the very same device work on both interrupt-driven and
> > polled mode, depending on the board on which the device is used.
> >
> > This introduces new APIs:
> >
> > - input_setup_polling
> > - input_set_poll_interval
> > - input_set_min_poll_interval
> > - input_set_max_poll_interval
> >
> > These new APIs allow switching an input device into polled mode with sysfs
> > attributes matching drivers using input_polled_dev APIs that will be
> > eventually removed.
>
> Are you sure that using sysfs is the correct way here?
> I would think using generic properties that can be overwritten by the
> DSDT or by the device tree would make things easier from a driver
> point of view.
Couple of points: I wanted it to be compatible with input-polldev.c so
the sysfs attributes must match (so that we can convert existing drivers
and zap input-polldev). I also am not sure if polling parameters are
property of board, or it is either fundamental hardware limitation or
simply desired system behavior. If Rob is OK with adding device
properties I'd be fine adding them as a followup, otherwise we can have
udev adjust the behavior as needed for given box shortly after boot.
>
> Also, checkpatch complains about a few octal permissions that are
> preferred over symbolic ones, and there is a possible 'out of memory'
> nessage at drivers/input/input-poller.c:75.
Yes, there is. It is there so we would know what device we were trying
to set up when OOM happened. You can probable decipher the driver from
the stack trace, but figuring particular device instance is harder.
Thanks.
>
> Cheers,
> Benjamin
>
> >
> > Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/Makefile | 2 +-
> > drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
> > drivers/input/input-poller.h | 18 +++
> > drivers/input/input.c | 36 ++++--
> > include/linux/input.h | 12 ++
> > 5 files changed, 268 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> > index 40de6a7be641..e35650930371 100644
> > --- a/drivers/input/Makefile
> > +++ b/drivers/input/Makefile
> > @@ -6,7 +6,7 @@
> > # Each configuration option enables a list of files.
> >
> > obj-$(CONFIG_INPUT) += input-core.o
> > -input-core-y := input.o input-compat.o input-mt.o ff-core.o
> > +input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
> >
> > obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> > obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
> > diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
> > new file mode 100644
> > index 000000000000..e041adb04f5a
> > --- /dev/null
> > +++ b/drivers/input/input-poller.c
> > @@ -0,0 +1,208 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Support for polling mode for input devices.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/input.h>
> > +#include <linux/jiffies.h>
> > +#include <linux/mutex.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/workqueue.h>
> > +#include "input-poller.h"
> > +
> > +struct input_dev_poller {
> > + void (*poll)(struct input_dev *dev);
> > +
> > + unsigned int poll_interval; /* msec */
> > + unsigned int poll_interval_max; /* msec */
> > + unsigned int poll_interval_min; /* msec */
> > +
> > + struct input_dev *input;
> > + struct delayed_work work;
> > +};
> > +
> > +static void input_dev_poller_queue_work(struct input_dev_poller *poller)
> > +{
> > + unsigned long delay;
> > +
> > + delay = msecs_to_jiffies(poller->poll_interval);
> > + if (delay >= HZ)
> > + delay = round_jiffies_relative(delay);
> > +
> > + queue_delayed_work(system_freezable_wq, &poller->work, delay);
> > +}
> > +
> > +static void input_dev_poller_work(struct work_struct *work)
> > +{
> > + struct input_dev_poller *poller =
> > + container_of(work, struct input_dev_poller, work.work);
> > +
> > + poller->poll(poller->input);
> > + input_dev_poller_queue_work(poller);
> > +}
> > +
> > +void input_dev_poller_finalize(struct input_dev_poller *poller)
> > +{
> > + if (!poller->poll_interval)
> > + poller->poll_interval = 500;
> > + if (!poller->poll_interval_max)
> > + poller->poll_interval_max = poller->poll_interval;
> > +}
> > +
> > +void input_dev_poller_start(struct input_dev_poller *poller)
> > +{
> > + /* Only start polling if polling is enabled */
> > + if (poller->poll_interval > 0) {
> > + poller->poll(poller->input);
> > + input_dev_poller_queue_work(poller);
> > + }
> > +}
> > +
> > +void input_dev_poller_stop(struct input_dev_poller *poller)
> > +{
> > + cancel_delayed_work_sync(&poller->work);
> > +}
> > +
> > +int input_setup_polling(struct input_dev *dev,
> > + void (*poll_fn)(struct input_dev *dev))
> > +{
> > + struct input_dev_poller *poller;
> > +
> > + poller = kzalloc(sizeof(*poller), GFP_KERNEL);
> > + if (!poller) {
> > + dev_err(dev->dev.parent ?: &dev->dev,
> > + "%s: unable to allocate poller structure\n", __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
> > + poller->input = dev;
> > + poller->poll = poll_fn;
> > +
> > + dev->poller = poller;
> > + return 0;
> > +}
> > +EXPORT_SYMBOL(input_setup_polling);
> > +
> > +static bool input_dev_ensure_poller(struct input_dev *dev)
> > +{
> > + if (!dev->poller) {
> > + dev_err(dev->dev.parent ?: &dev->dev,
> > + "poller structure has not been set up\n");
> > + return false;
> > + }
> > +
> > + return true;
> > +}
> > +
> > +void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
> > +{
> > + if (input_dev_ensure_poller(dev))
> > + dev->poller->poll_interval = interval;
> > +}
> > +EXPORT_SYMBOL(input_set_poll_interval);
> > +
> > +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
> > +{
> > + if (input_dev_ensure_poller(dev))
> > + dev->poller->poll_interval_min = interval;
> > +}
> > +EXPORT_SYMBOL(input_set_min_poll_interval);
> > +
> > +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
> > +{
> > + if (input_dev_ensure_poller(dev))
> > + dev->poller->poll_interval_max = interval;
> > +}
> > +EXPORT_SYMBOL(input_set_max_poll_interval);
> > +
> > +/* SYSFS interface */
> > +
> > +static ssize_t input_dev_get_poll_interval(struct device *dev,
> > + struct device_attribute *attr,
> > + char *buf)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return sprintf(buf, "%d\n", input->poller->poll_interval);
> > +}
> > +
> > +static ssize_t input_dev_set_poll_interval(struct device *dev,
> > + struct device_attribute *attr,
> > + const char *buf, size_t count)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > + struct input_dev_poller *poller = input->poller;
> > + unsigned int interval;
> > + int err;
> > +
> > + err = kstrtouint(buf, 0, &interval);
> > + if (err)
> > + return err;
> > +
> > + if (interval < poller->poll_interval_min)
> > + return -EINVAL;
> > +
> > + if (interval > poller->poll_interval_max)
> > + return -EINVAL;
> > +
> > + mutex_lock(&input->mutex);
> > +
> > + poller->poll_interval = interval;
> > +
> > + if (input->users) {
> > + cancel_delayed_work_sync(&poller->work);
> > + if (poller->poll_interval > 0)
> > + input_dev_poller_queue_work(poller);
> > + }
> > +
> > + mutex_unlock(&input->mutex);
> > +
> > + return count;
> > +}
> > +
> > +static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
> > + input_dev_get_poll_interval, input_dev_set_poll_interval);
> > +
> > +static ssize_t input_dev_get_poll_max(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return sprintf(buf, "%d\n", input->poller->poll_interval_max);
> > +}
> > +
> > +static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
> > +
> > +static ssize_t input_dev_get_poll_min(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return sprintf(buf, "%d\n", input->poller->poll_interval_min);
> > +}
> > +
> > +static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
> > +
> > +static umode_t input_poller_attrs_visible(struct kobject *kobj,
> > + struct attribute *attr, int n)
> > +{
> > + struct device *dev = kobj_to_dev(kobj);
> > + struct input_dev *input = to_input_dev(dev);
> > +
> > + return input->poller ? attr->mode : 0;
> > +}
> > +
> > +static struct attribute *input_poller_attrs[] = {
> > + &dev_attr_poll.attr,
> > + &dev_attr_max.attr,
> > + &dev_attr_min.attr,
> > + NULL
> > +};
> > +
> > +struct attribute_group input_poller_attribute_group = {
> > + .is_visible = input_poller_attrs_visible,
> > + .attrs = input_poller_attrs,
> > +};
> > diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
> > new file mode 100644
> > index 000000000000..e3fca0be1d32
> > --- /dev/null
> > +++ b/drivers/input/input-poller.h
> > @@ -0,0 +1,18 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +#ifndef _INPUT_POLLER_H
> > +#define _INPUT_POLLER_H
> > +
> > +/*
> > + * Support for polling mode for input devices.
> > + */
> > +#include <linux/sysfs.h>
> > +
> > +struct input_dev_poller;
> > +
> > +void input_dev_poller_finalize(struct input_dev_poller *poller);
> > +void input_dev_poller_start(struct input_dev_poller *poller);
> > +void input_dev_poller_stop(struct input_dev_poller *poller);
> > +
> > +extern struct attribute_group input_poller_attribute_group;
> > +
> > +#endif /* _INPUT_POLLER_H */
> > diff --git a/drivers/input/input.c b/drivers/input/input.c
> > index 7494a0dede79..c08aa3596144 100644
> > --- a/drivers/input/input.c
> > +++ b/drivers/input/input.c
> > @@ -24,6 +24,7 @@
> > #include <linux/mutex.h>
> > #include <linux/rcupdate.h>
> > #include "input-compat.h"
> > +#include "input-poller.h"
> >
> > MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> > MODULE_DESCRIPTION("Input core");
> > @@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)
> >
> > handle->open++;
> >
> > - if (!dev->users++ && dev->open)
> > - retval = dev->open(dev);
> > + if (dev->users++) {
> > + /*
> > + * Device is already opened, so we can exit immediately and
> > + * report success.
> > + */
> > + goto out;
> > + }
> >
> > - if (retval) {
> > - dev->users--;
> > - if (!--handle->open) {
> > + if (dev->open) {
> > + retval = dev->open(dev);
> > + if (retval) {
> > + dev->users--;
> > + handle->open--;
> > /*
> > * Make sure we are not delivering any more events
> > * through this handle
> > */
> > synchronize_rcu();
> > + goto out;
> > }
> > }
> >
> > + if (dev->poller)
> > + input_dev_poller_start(dev->poller);
> > +
> > out:
> > mutex_unlock(&dev->mutex);
> > return retval;
> > @@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)
> >
> > __input_release_device(handle);
> >
> > - if (!--dev->users && dev->close)
> > - dev->close(dev);
> > + if (!--dev->users) {
> > + if (dev->poller)
> > + input_dev_poller_stop(dev->poller);
> > +
> > + if (dev->close)
> > + dev->close(dev);
> > + }
> >
> > if (!--handle->open) {
> > /*
> > @@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
> > &input_dev_attr_group,
> > &input_dev_id_attr_group,
> > &input_dev_caps_attr_group,
> > + &input_poller_attribute_group,
> > NULL
> > };
> >
> > @@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)
> >
> > input_ff_destroy(dev);
> > input_mt_destroy_slots(dev);
> > + kfree(dev->poller);
> > kfree(dev->absinfo);
> > kfree(dev->vals);
> > kfree(dev);
> > @@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
> > if (!dev->setkeycode)
> > dev->setkeycode = input_default_setkeycode;
> >
> > + if (dev->poller)
> > + input_dev_poller_finalize(dev->poller);
> > +
> > error = device_add(&dev->dev);
> > if (error)
> > goto err_free_vals;
> > diff --git a/include/linux/input.h b/include/linux/input.h
> > index e95a439d8bd5..94f277cd806a 100644
> > --- a/include/linux/input.h
> > +++ b/include/linux/input.h
> > @@ -21,6 +21,8 @@
> > #include <linux/timer.h>
> > #include <linux/mod_devicetable.h>
> >
> > +struct input_dev_poller;
> > +
> > /**
> > * struct input_value - input value representation
> > * @type: type of value (EV_KEY, EV_ABS, etc)
> > @@ -71,6 +73,8 @@ enum input_clock_type {
> > * not sleep
> > * @ff: force feedback structure associated with the device if device
> > * supports force feedback effects
> > + * @poller: poller structure associated with the device if device is
> > + * set up to use polling mode
> > * @repeat_key: stores key code of the last key pressed; used to implement
> > * software autorepeat
> > * @timer: timer for software autorepeat
> > @@ -156,6 +160,8 @@ struct input_dev {
> >
> > struct ff_device *ff;
> >
> > + struct input_dev_poller *poller;
> > +
> > unsigned int repeat_key;
> > struct timer_list timer;
> >
> > @@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);
> >
> > void input_reset_device(struct input_dev *);
> >
> > +int input_setup_polling(struct input_dev *dev,
> > + void (*poll_fn)(struct input_dev *dev));
> > +void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
> > +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
> > +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
> > +
> > int __must_check input_register_handler(struct input_handler *);
> > void input_unregister_handler(struct input_handler *);
> >
> > --
> > 2.23.0.rc1.153.gdeed80330f-goog
> >
> >
> > --
> > Dmitry
--
Dmitry
^ permalink raw reply
* Re: [PATCH] HID: apple: Fix stuck function keys when using FN
From: Benjamin Tissoires @ 2019-08-12 16:57 UTC (permalink / raw)
To: João Moreno
Cc: Benjamin Tissoires, Jiri Kosina, open list:HID CORE LAYER, lkml
In-Reply-To: <CAHxFc3TjZu7_u0U3ZoB466WGNzbfYLe4ZB7C4LuKdBAwkRum5Q@mail.gmail.com>
Hi João,
On Thu, Aug 8, 2019 at 10:35 PM João Moreno <mail@joaomoreno.com> wrote:
>
> Hi Benjamin,
>
> On Mon, 8 Jul 2019 at 22:35, João Moreno <mail@joaomoreno.com> wrote:
> >
> > Hi Benjamin,
> >
> > No worries, also pretty busy over here. Didn't mean to press.
> >
> > On Mon, 1 Jul 2019 at 10:32, Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > Hi João,
> > >
> > > On Sun, Jun 30, 2019 at 10:15 PM João Moreno <mail@joaomoreno.com> wrote:
> > > >
> > > > Hi Jiri & Benjamin,
> > > >
> > > > Let me know if you need something else to get this patch moving forward. This
> > > > fixes an issue I hit daily, it would be great to get it fixed.
> > >
> > > Sorry for the delay, I am very busy with internal corporate stuff, and
> > > I tried setting up a new CI system at home, and instead of spending a
> > > couple of ours, I am down to 2 weeks of hard work, without possibility
> > > to switch to the new right now :(
> > > Anyway.
> > >
> > > >
> > > > Thanks.
> > > >
> > > > On Mon, 10 Jun 2019 at 23:31, Joao Moreno <mail@joaomoreno.com> wrote:
> > > > >
> > > > > This fixes an issue in which key down events for function keys would be
> > > > > repeatedly emitted even after the user has raised the physical key. For
> > > > > example, the driver fails to emit the F5 key up event when going through
> > > > > the following steps:
> > > > > - fnmode=1: hold FN, hold F5, release FN, release F5
> > > > > - fnmode=2: hold F5, hold FN, release F5, release FN
> > >
> > > Ouch :/
> > >
> >
> > Right?!
> >
> > > > >
> > > > > The repeated F5 key down events can be easily verified using xev.
> > > > >
> > > > > Signed-off-by: Joao Moreno <mail@joaomoreno.com>
> > > > > ---
> > > > > drivers/hid/hid-apple.c | 21 +++++++++++----------
> > > > > 1 file changed, 11 insertions(+), 10 deletions(-)
> > > > >
> > > > > diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> > > > > index 1cb41992aaa1..81867a6fa047 100644
> > > > > --- a/drivers/hid/hid-apple.c
> > > > > +++ b/drivers/hid/hid-apple.c
> > > > > @@ -205,20 +205,21 @@ static int hidinput_apple_event(struct hid_device *hid, struct input_dev *input,
> > > > > trans = apple_find_translation (table, usage->code);
> > > > >
> > > > > if (trans) {
> > > > > - if (test_bit(usage->code, asc->pressed_fn))
> > > > > - do_translate = 1;
> > > > > - else if (trans->flags & APPLE_FLAG_FKEY)
> > > > > - do_translate = (fnmode == 2 && asc->fn_on) ||
> > > > > - (fnmode == 1 && !asc->fn_on);
> > > > > + int fn_on = value ? asc->fn_on :
> > > > > + test_bit(usage->code, asc->pressed_fn);
> > > > > +
> > > > > + if (!value)
> > > > > + clear_bit(usage->code, asc->pressed_fn);
> > > > > + else if (asc->fn_on)
> > > > > + set_bit(usage->code, asc->pressed_fn);
> > >
> > > I have the feeling that this is not the correct fix here.
> > >
> > > I might be wrong, but the following sequence might also mess up the
> > > driver state, depending on how the reports are emitted:
> > > - hold FN, hold F4, hold F5, release F4, release FN, release F5
> > >
> >
> > I believe this should be fine. Following the code:
> >
> > - hold FN, sets asc->fn_on to true
> > - hold F4, in the trans block fn_on will be true and we'll set the F4
> > bit in the bitmap
> > - hold F5, in the trans block fn_on will be true and we'll set the F5 bit
> > - release F4, in the trans block fn_on will be true (because of the bitmap) and
> > we'll clear the F4 bit
> > - release FN, asc->fn_on will be false, but it doesn't matter since...
> > - release F5, in the trans block we'll look into the bitmap (instead
> > of asc->fn_on),
> > so fn_on will be true and we'll clear the F5 bit
> >
> > I tested it in practice using my changes:
> >
> > Interestingly the Apple keyboard doesn't seem to emit an even for F5 when F4 is
> > pressed, seems like a hardware limitation. But F6 does work. So, when I execute
> > these events in that order, everything works as it should: xev reports
> > the following:
> >
> > KeyPress F4
> > KeyPress F6
> > KeyRelease F4
> > KeyRelease F6
> >
> > > The reason is that the driver only considers you have one key pressed
> > > with the modifier, and as the code changed its state based on the last
> > > value.
> > >
> >
> > I believe the bitmap takes care of storing the FN state per key press. The
> > trick I did was to check on the global `asc->fn_on` state only when a key
> > is pressed, but check on the bitmap instead when it's released.
> >
> > Let me know what you think. Am I missing something here?
> >
> > Cheers,
> > João.
> >
> > > IMO a better fix would:
> > >
> > > - keep the existing `trans` mapping lookout
> > > - whenever a `trans` mapping gets found:
> > > * get both translated and non-translated currently reported values
> > > (`test_bit(keycode, input_dev->key)`)
> > > * if one of them is set to true, then consider the keycode to be the
> > > one of the key (no matter fn_on)
> > > -> deal with `value` with the corrected keycode
> > > * if the key was not pressed:
> > > -> chose the keycode based on `fn_on` and `fnmode` states
> > > and report the key press event
> > >
> > > This should remove the nasty pressed_fn state which depends on the
> > > other pressed keys.
> > >
> > > Cheers,
> > > Benjamin
> > >
> > > > > +
> > > > > + if (trans->flags & APPLE_FLAG_FKEY)
> > > > > + do_translate = (fnmode == 2 && fn_on) ||
> > > > > + (fnmode == 1 && !fn_on);
> > > > > else
> > > > > do_translate = asc->fn_on;
> > > > >
> > > > > if (do_translate) {
> > > > > - if (value)
> > > > > - set_bit(usage->code, asc->pressed_fn);
> > > > > - else
> > > > > - clear_bit(usage->code, asc->pressed_fn);
> > > > > -
> > > > > input_event(input, usage->type, trans->to,
> > > > > value);
> > > > >
> > > > > --
> > > > > 2.19.1
> > > > >
>
> Gave this another look and I still haven't found any issues, let me
> know if you still
> think I'm missing something. Thanks!
>
OK, I am tempted to take the patch, but I'll need to add this as a
regression test in my test-suite. This is too complex that it can
easily break next time someone looks at it.
Can you send me the report descriptors of the device using
hid-recorder from hid-tools[0].
Ideally, if you can put the faulty sequence in the logs, that would
help writing down the use case.
Cheers,
Benjamin
[0] https://gitlab.freedesktop.org/libevdev/hid-tools
^ permalink raw reply
* Re: [PATCH] Input: add support for polling to input devices
From: Benjamin Tissoires @ 2019-08-12 16:50 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER, lkml
In-Reply-To: <20190809173548.GA32693@dtor-ws>
Hi Dmitry,
On Fri, Aug 9, 2019 at 7:35 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> Separating "normal" and "polled" input devices was a mistake, as often we
> want to allow the very same device work on both interrupt-driven and
> polled mode, depending on the board on which the device is used.
>
> This introduces new APIs:
>
> - input_setup_polling
> - input_set_poll_interval
> - input_set_min_poll_interval
> - input_set_max_poll_interval
>
> These new APIs allow switching an input device into polled mode with sysfs
> attributes matching drivers using input_polled_dev APIs that will be
> eventually removed.
Are you sure that using sysfs is the correct way here?
I would think using generic properties that can be overwritten by the
DSDT or by the device tree would make things easier from a driver
point of view.
Also, checkpatch complains about a few octal permissions that are
preferred over symbolic ones, and there is a possible 'out of memory'
nessage at drivers/input/input-poller.c:75.
Cheers,
Benjamin
>
> Tested-by: Michal Vokáč <michal.vokac@ysoft.com>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> drivers/input/Makefile | 2 +-
> drivers/input/input-poller.c | 208 +++++++++++++++++++++++++++++++++++
> drivers/input/input-poller.h | 18 +++
> drivers/input/input.c | 36 ++++--
> include/linux/input.h | 12 ++
> 5 files changed, 268 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/input/Makefile b/drivers/input/Makefile
> index 40de6a7be641..e35650930371 100644
> --- a/drivers/input/Makefile
> +++ b/drivers/input/Makefile
> @@ -6,7 +6,7 @@
> # Each configuration option enables a list of files.
>
> obj-$(CONFIG_INPUT) += input-core.o
> -input-core-y := input.o input-compat.o input-mt.o ff-core.o
> +input-core-y := input.o input-compat.o input-mt.o input-poller.o ff-core.o
>
> obj-$(CONFIG_INPUT_FF_MEMLESS) += ff-memless.o
> obj-$(CONFIG_INPUT_POLLDEV) += input-polldev.o
> diff --git a/drivers/input/input-poller.c b/drivers/input/input-poller.c
> new file mode 100644
> index 000000000000..e041adb04f5a
> --- /dev/null
> +++ b/drivers/input/input-poller.c
> @@ -0,0 +1,208 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Support for polling mode for input devices.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/input.h>
> +#include <linux/jiffies.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/workqueue.h>
> +#include "input-poller.h"
> +
> +struct input_dev_poller {
> + void (*poll)(struct input_dev *dev);
> +
> + unsigned int poll_interval; /* msec */
> + unsigned int poll_interval_max; /* msec */
> + unsigned int poll_interval_min; /* msec */
> +
> + struct input_dev *input;
> + struct delayed_work work;
> +};
> +
> +static void input_dev_poller_queue_work(struct input_dev_poller *poller)
> +{
> + unsigned long delay;
> +
> + delay = msecs_to_jiffies(poller->poll_interval);
> + if (delay >= HZ)
> + delay = round_jiffies_relative(delay);
> +
> + queue_delayed_work(system_freezable_wq, &poller->work, delay);
> +}
> +
> +static void input_dev_poller_work(struct work_struct *work)
> +{
> + struct input_dev_poller *poller =
> + container_of(work, struct input_dev_poller, work.work);
> +
> + poller->poll(poller->input);
> + input_dev_poller_queue_work(poller);
> +}
> +
> +void input_dev_poller_finalize(struct input_dev_poller *poller)
> +{
> + if (!poller->poll_interval)
> + poller->poll_interval = 500;
> + if (!poller->poll_interval_max)
> + poller->poll_interval_max = poller->poll_interval;
> +}
> +
> +void input_dev_poller_start(struct input_dev_poller *poller)
> +{
> + /* Only start polling if polling is enabled */
> + if (poller->poll_interval > 0) {
> + poller->poll(poller->input);
> + input_dev_poller_queue_work(poller);
> + }
> +}
> +
> +void input_dev_poller_stop(struct input_dev_poller *poller)
> +{
> + cancel_delayed_work_sync(&poller->work);
> +}
> +
> +int input_setup_polling(struct input_dev *dev,
> + void (*poll_fn)(struct input_dev *dev))
> +{
> + struct input_dev_poller *poller;
> +
> + poller = kzalloc(sizeof(*poller), GFP_KERNEL);
> + if (!poller) {
> + dev_err(dev->dev.parent ?: &dev->dev,
> + "%s: unable to allocate poller structure\n", __func__);
> + return -ENOMEM;
> + }
> +
> + INIT_DELAYED_WORK(&poller->work, input_dev_poller_work);
> + poller->input = dev;
> + poller->poll = poll_fn;
> +
> + dev->poller = poller;
> + return 0;
> +}
> +EXPORT_SYMBOL(input_setup_polling);
> +
> +static bool input_dev_ensure_poller(struct input_dev *dev)
> +{
> + if (!dev->poller) {
> + dev_err(dev->dev.parent ?: &dev->dev,
> + "poller structure has not been set up\n");
> + return false;
> + }
> +
> + return true;
> +}
> +
> +void input_set_poll_interval(struct input_dev *dev, unsigned int interval)
> +{
> + if (input_dev_ensure_poller(dev))
> + dev->poller->poll_interval = interval;
> +}
> +EXPORT_SYMBOL(input_set_poll_interval);
> +
> +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval)
> +{
> + if (input_dev_ensure_poller(dev))
> + dev->poller->poll_interval_min = interval;
> +}
> +EXPORT_SYMBOL(input_set_min_poll_interval);
> +
> +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval)
> +{
> + if (input_dev_ensure_poller(dev))
> + dev->poller->poll_interval_max = interval;
> +}
> +EXPORT_SYMBOL(input_set_max_poll_interval);
> +
> +/* SYSFS interface */
> +
> +static ssize_t input_dev_get_poll_interval(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + struct input_dev *input = to_input_dev(dev);
> +
> + return sprintf(buf, "%d\n", input->poller->poll_interval);
> +}
> +
> +static ssize_t input_dev_set_poll_interval(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct input_dev *input = to_input_dev(dev);
> + struct input_dev_poller *poller = input->poller;
> + unsigned int interval;
> + int err;
> +
> + err = kstrtouint(buf, 0, &interval);
> + if (err)
> + return err;
> +
> + if (interval < poller->poll_interval_min)
> + return -EINVAL;
> +
> + if (interval > poller->poll_interval_max)
> + return -EINVAL;
> +
> + mutex_lock(&input->mutex);
> +
> + poller->poll_interval = interval;
> +
> + if (input->users) {
> + cancel_delayed_work_sync(&poller->work);
> + if (poller->poll_interval > 0)
> + input_dev_poller_queue_work(poller);
> + }
> +
> + mutex_unlock(&input->mutex);
> +
> + return count;
> +}
> +
> +static DEVICE_ATTR(poll, S_IRUGO | S_IWUSR,
> + input_dev_get_poll_interval, input_dev_set_poll_interval);
> +
> +static ssize_t input_dev_get_poll_max(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct input_dev *input = to_input_dev(dev);
> +
> + return sprintf(buf, "%d\n", input->poller->poll_interval_max);
> +}
> +
> +static DEVICE_ATTR(max, S_IRUGO, input_dev_get_poll_max, NULL);
> +
> +static ssize_t input_dev_get_poll_min(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct input_dev *input = to_input_dev(dev);
> +
> + return sprintf(buf, "%d\n", input->poller->poll_interval_min);
> +}
> +
> +static DEVICE_ATTR(min, S_IRUGO, input_dev_get_poll_min, NULL);
> +
> +static umode_t input_poller_attrs_visible(struct kobject *kobj,
> + struct attribute *attr, int n)
> +{
> + struct device *dev = kobj_to_dev(kobj);
> + struct input_dev *input = to_input_dev(dev);
> +
> + return input->poller ? attr->mode : 0;
> +}
> +
> +static struct attribute *input_poller_attrs[] = {
> + &dev_attr_poll.attr,
> + &dev_attr_max.attr,
> + &dev_attr_min.attr,
> + NULL
> +};
> +
> +struct attribute_group input_poller_attribute_group = {
> + .is_visible = input_poller_attrs_visible,
> + .attrs = input_poller_attrs,
> +};
> diff --git a/drivers/input/input-poller.h b/drivers/input/input-poller.h
> new file mode 100644
> index 000000000000..e3fca0be1d32
> --- /dev/null
> +++ b/drivers/input/input-poller.h
> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +#ifndef _INPUT_POLLER_H
> +#define _INPUT_POLLER_H
> +
> +/*
> + * Support for polling mode for input devices.
> + */
> +#include <linux/sysfs.h>
> +
> +struct input_dev_poller;
> +
> +void input_dev_poller_finalize(struct input_dev_poller *poller);
> +void input_dev_poller_start(struct input_dev_poller *poller);
> +void input_dev_poller_stop(struct input_dev_poller *poller);
> +
> +extern struct attribute_group input_poller_attribute_group;
> +
> +#endif /* _INPUT_POLLER_H */
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 7494a0dede79..c08aa3596144 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -24,6 +24,7 @@
> #include <linux/mutex.h>
> #include <linux/rcupdate.h>
> #include "input-compat.h"
> +#include "input-poller.h"
>
> MODULE_AUTHOR("Vojtech Pavlik <vojtech@suse.cz>");
> MODULE_DESCRIPTION("Input core");
> @@ -603,20 +604,31 @@ int input_open_device(struct input_handle *handle)
>
> handle->open++;
>
> - if (!dev->users++ && dev->open)
> - retval = dev->open(dev);
> + if (dev->users++) {
> + /*
> + * Device is already opened, so we can exit immediately and
> + * report success.
> + */
> + goto out;
> + }
>
> - if (retval) {
> - dev->users--;
> - if (!--handle->open) {
> + if (dev->open) {
> + retval = dev->open(dev);
> + if (retval) {
> + dev->users--;
> + handle->open--;
> /*
> * Make sure we are not delivering any more events
> * through this handle
> */
> synchronize_rcu();
> + goto out;
> }
> }
>
> + if (dev->poller)
> + input_dev_poller_start(dev->poller);
> +
> out:
> mutex_unlock(&dev->mutex);
> return retval;
> @@ -655,8 +667,13 @@ void input_close_device(struct input_handle *handle)
>
> __input_release_device(handle);
>
> - if (!--dev->users && dev->close)
> - dev->close(dev);
> + if (!--dev->users) {
> + if (dev->poller)
> + input_dev_poller_stop(dev->poller);
> +
> + if (dev->close)
> + dev->close(dev);
> + }
>
> if (!--handle->open) {
> /*
> @@ -1502,6 +1519,7 @@ static const struct attribute_group *input_dev_attr_groups[] = {
> &input_dev_attr_group,
> &input_dev_id_attr_group,
> &input_dev_caps_attr_group,
> + &input_poller_attribute_group,
> NULL
> };
>
> @@ -1511,6 +1529,7 @@ static void input_dev_release(struct device *device)
>
> input_ff_destroy(dev);
> input_mt_destroy_slots(dev);
> + kfree(dev->poller);
> kfree(dev->absinfo);
> kfree(dev->vals);
> kfree(dev);
> @@ -2175,6 +2194,9 @@ int input_register_device(struct input_dev *dev)
> if (!dev->setkeycode)
> dev->setkeycode = input_default_setkeycode;
>
> + if (dev->poller)
> + input_dev_poller_finalize(dev->poller);
> +
> error = device_add(&dev->dev);
> if (error)
> goto err_free_vals;
> diff --git a/include/linux/input.h b/include/linux/input.h
> index e95a439d8bd5..94f277cd806a 100644
> --- a/include/linux/input.h
> +++ b/include/linux/input.h
> @@ -21,6 +21,8 @@
> #include <linux/timer.h>
> #include <linux/mod_devicetable.h>
>
> +struct input_dev_poller;
> +
> /**
> * struct input_value - input value representation
> * @type: type of value (EV_KEY, EV_ABS, etc)
> @@ -71,6 +73,8 @@ enum input_clock_type {
> * not sleep
> * @ff: force feedback structure associated with the device if device
> * supports force feedback effects
> + * @poller: poller structure associated with the device if device is
> + * set up to use polling mode
> * @repeat_key: stores key code of the last key pressed; used to implement
> * software autorepeat
> * @timer: timer for software autorepeat
> @@ -156,6 +160,8 @@ struct input_dev {
>
> struct ff_device *ff;
>
> + struct input_dev_poller *poller;
> +
> unsigned int repeat_key;
> struct timer_list timer;
>
> @@ -372,6 +378,12 @@ void input_unregister_device(struct input_dev *);
>
> void input_reset_device(struct input_dev *);
>
> +int input_setup_polling(struct input_dev *dev,
> + void (*poll_fn)(struct input_dev *dev));
> +void input_set_poll_interval(struct input_dev *dev, unsigned int interval);
> +void input_set_min_poll_interval(struct input_dev *dev, unsigned int interval);
> +void input_set_max_poll_interval(struct input_dev *dev, unsigned int interval);
> +
> int __must_check input_register_handler(struct input_handler *);
> void input_unregister_handler(struct input_handler *);
>
> --
> 2.23.0.rc1.153.gdeed80330f-goog
>
>
> --
> Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] HID: do not call hid_set_drvdata(hdev, NULL) in drivers
From: Srinivas Pandruvada @ 2019-08-12 16:39 UTC (permalink / raw)
To: Benjamin Tissoires, Bruno Prémont, Jonathan Cameron,
Ping Cheng, Jason Gerecke, Jiri Kosina
Cc: linux-input, linux-kernel
In-Reply-To: <20190812162740.15898-2-benjamin.tissoires@redhat.com>
On Mon, 2019-08-12 at 18:27 +0200, Benjamin Tissoires wrote:
> This is a common pattern in the HID drivers to reset the drvdata.
> Some
> do it properly, some do it only in case of failure.
>
> But, this is actually already handled by driver core, so there is no
> need
> to do it manually.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
for hid-sensor-hub.c
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/hid/hid-cougar.c | 6 ++----
> drivers/hid/hid-gfrm.c | 7 -------
> drivers/hid/hid-lenovo.c | 2 --
> drivers/hid/hid-picolcd_core.c | 7 +------
> drivers/hid/hid-sensor-hub.c | 1 -
> 5 files changed, 3 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
> index e0bb7b34f3a4..4ff3bc1d25e2 100644
> --- a/drivers/hid/hid-cougar.c
> +++ b/drivers/hid/hid-cougar.c
> @@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
> error = hid_parse(hdev);
> if (error) {
> hid_err(hdev, "parse failed\n");
> - goto fail;
> + return error;
> }
>
> if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
> @@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
> error = hid_hw_start(hdev, connect_mask);
> if (error) {
> hid_err(hdev, "hw start failed\n");
> - goto fail;
> + return error;
> }
>
> error = cougar_bind_shared_data(hdev, cougar);
> @@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
>
> fail_stop_and_cleanup:
> hid_hw_stop(hdev);
> -fail:
> - hid_set_drvdata(hdev, NULL);
> return error;
> }
>
> diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
> index 86c317320bf2..699186ff2349 100644
> --- a/drivers/hid/hid-gfrm.c
> +++ b/drivers/hid/hid-gfrm.c
> @@ -123,12 +123,6 @@ static int gfrm_probe(struct hid_device *hdev,
> const struct hid_device_id *id)
> return ret;
> }
>
> -static void gfrm_remove(struct hid_device *hdev)
> -{
> - hid_hw_stop(hdev);
> - hid_set_drvdata(hdev, NULL);
> -}
> -
> static const struct hid_device_id gfrm_devices[] = {
> { HID_BLUETOOTH_DEVICE(0x58, 0x2000),
> .driver_data = GFRM100 },
> @@ -142,7 +136,6 @@ static struct hid_driver gfrm_driver = {
> .name = "gfrm",
> .id_table = gfrm_devices,
> .probe = gfrm_probe,
> - .remove = gfrm_remove,
> .input_mapping = gfrm_input_mapping,
> .raw_event = gfrm_raw_event,
> .input_configured = gfrm_input_configured,
> diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
> index 364bc7f11d9d..96fa2a2c2cd3 100644
> --- a/drivers/hid/hid-lenovo.c
> +++ b/drivers/hid/hid-lenovo.c
> @@ -866,8 +866,6 @@ static void lenovo_remove_tpkbd(struct hid_device
> *hdev)
>
> led_classdev_unregister(&data_pointer->led_micmute);
> led_classdev_unregister(&data_pointer->led_mute);
> -
> - hid_set_drvdata(hdev, NULL);
> }
>
> static void lenovo_remove_cptkbd(struct hid_device *hdev)
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-
> picolcd_core.c
> index 5f7a39a5d4af..1b5c63241af0 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -534,8 +534,7 @@ static int picolcd_probe(struct hid_device *hdev,
> data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
> if (data == NULL) {
> hid_err(hdev, "can't allocate space for Minibox PicoLCD
> device data\n");
> - error = -ENOMEM;
> - goto err_no_cleanup;
> + return -ENOMEM;
> }
>
> spin_lock_init(&data->lock);
> @@ -597,9 +596,6 @@ static int picolcd_probe(struct hid_device *hdev,
> hid_hw_stop(hdev);
> err_cleanup_data:
> kfree(data);
> -err_no_cleanup:
> - hid_set_drvdata(hdev, NULL);
> -
> return error;
> }
>
> @@ -635,7 +631,6 @@ static void picolcd_remove(struct hid_device
> *hdev)
> picolcd_exit_cir(data);
> picolcd_exit_keys(data);
>
> - hid_set_drvdata(hdev, NULL);
> mutex_destroy(&data->mutex);
> /* Finally, clean up the picolcd data itself */
> kfree(data);
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-
> hub.c
> index be92a6f79687..94c7398b5c27 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -742,7 +742,6 @@ static void sensor_hub_remove(struct hid_device
> *hdev)
> }
> spin_unlock_irqrestore(&data->lock, flags);
> mfd_remove_devices(&hdev->dev);
> - hid_set_drvdata(hdev, NULL);
> mutex_destroy(&data->mutex);
> }
>
^ permalink raw reply
* [PATCH 2/2] HID: wacom: do not call hid_set_drvdata(hdev, NULL)
From: Benjamin Tissoires @ 2019-08-12 16:27 UTC (permalink / raw)
To: Bruno Prémont, Jonathan Cameron, Srinivas Pandruvada,
Ping Cheng, Jason Gerecke, Jiri Kosina
Cc: linux-input, linux-kernel, Benjamin Tissoires
In-Reply-To: <20190812162740.15898-1-benjamin.tissoires@redhat.com>
This is a common pattern in the HID drivers to reset the drvdata.
However, this is actually already handled by driver core, so there
is no need to do it manually.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/wacom_sys.c | 18 +++++-------------
1 file changed, 5 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index 53bddb50aeba..69ccfdd51a6f 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2718,14 +2718,12 @@ static int wacom_probe(struct hid_device *hdev,
wacom_wac->features = *((struct wacom_features *)id->driver_data);
features = &wacom_wac->features;
- if (features->check_for_hid_type && features->hid_type != hdev->type) {
- error = -ENODEV;
- goto fail;
- }
+ if (features->check_for_hid_type && features->hid_type != hdev->type)
+ return -ENODEV;
error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
if (error)
- goto fail;
+ return error;
wacom_wac->hid_data.inputmode = -1;
wacom_wac->mode_report = -1;
@@ -2743,12 +2741,12 @@ static int wacom_probe(struct hid_device *hdev,
error = hid_parse(hdev);
if (error) {
hid_err(hdev, "parse failed\n");
- goto fail;
+ return error;
}
error = wacom_parse_and_register(wacom, false);
if (error)
- goto fail;
+ return error;
if (hdev->bus == BUS_BLUETOOTH) {
error = device_create_file(&hdev->dev, &dev_attr_speed);
@@ -2759,10 +2757,6 @@ static int wacom_probe(struct hid_device *hdev,
}
return 0;
-
-fail:
- hid_set_drvdata(hdev, NULL);
- return error;
}
static void wacom_remove(struct hid_device *hdev)
@@ -2791,8 +2785,6 @@ static void wacom_remove(struct hid_device *hdev)
wacom_release_resources(wacom);
kfifo_free(&wacom_wac->pen_fifo);
-
- hid_set_drvdata(hdev, NULL);
}
#ifdef CONFIG_PM
--
2.19.2
^ permalink raw reply related
* [PATCH 1/2] HID: do not call hid_set_drvdata(hdev, NULL) in drivers
From: Benjamin Tissoires @ 2019-08-12 16:27 UTC (permalink / raw)
To: Bruno Prémont, Jonathan Cameron, Srinivas Pandruvada,
Ping Cheng, Jason Gerecke, Jiri Kosina
Cc: linux-input, linux-kernel, Benjamin Tissoires
In-Reply-To: <20190812162740.15898-1-benjamin.tissoires@redhat.com>
This is a common pattern in the HID drivers to reset the drvdata. Some
do it properly, some do it only in case of failure.
But, this is actually already handled by driver core, so there is no need
to do it manually.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-cougar.c | 6 ++----
drivers/hid/hid-gfrm.c | 7 -------
drivers/hid/hid-lenovo.c | 2 --
drivers/hid/hid-picolcd_core.c | 7 +------
drivers/hid/hid-sensor-hub.c | 1 -
5 files changed, 3 insertions(+), 20 deletions(-)
diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
index e0bb7b34f3a4..4ff3bc1d25e2 100644
--- a/drivers/hid/hid-cougar.c
+++ b/drivers/hid/hid-cougar.c
@@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
error = hid_parse(hdev);
if (error) {
hid_err(hdev, "parse failed\n");
- goto fail;
+ return error;
}
if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
@@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
error = hid_hw_start(hdev, connect_mask);
if (error) {
hid_err(hdev, "hw start failed\n");
- goto fail;
+ return error;
}
error = cougar_bind_shared_data(hdev, cougar);
@@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
fail_stop_and_cleanup:
hid_hw_stop(hdev);
-fail:
- hid_set_drvdata(hdev, NULL);
return error;
}
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index 86c317320bf2..699186ff2349 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -123,12 +123,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
-static void gfrm_remove(struct hid_device *hdev)
-{
- hid_hw_stop(hdev);
- hid_set_drvdata(hdev, NULL);
-}
-
static const struct hid_device_id gfrm_devices[] = {
{ HID_BLUETOOTH_DEVICE(0x58, 0x2000),
.driver_data = GFRM100 },
@@ -142,7 +136,6 @@ static struct hid_driver gfrm_driver = {
.name = "gfrm",
.id_table = gfrm_devices,
.probe = gfrm_probe,
- .remove = gfrm_remove,
.input_mapping = gfrm_input_mapping,
.raw_event = gfrm_raw_event,
.input_configured = gfrm_input_configured,
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index 364bc7f11d9d..96fa2a2c2cd3 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -866,8 +866,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
led_classdev_unregister(&data_pointer->led_micmute);
led_classdev_unregister(&data_pointer->led_mute);
-
- hid_set_drvdata(hdev, NULL);
}
static void lenovo_remove_cptkbd(struct hid_device *hdev)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index 5f7a39a5d4af..1b5c63241af0 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -534,8 +534,7 @@ static int picolcd_probe(struct hid_device *hdev,
data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
if (data == NULL) {
hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
- error = -ENOMEM;
- goto err_no_cleanup;
+ return -ENOMEM;
}
spin_lock_init(&data->lock);
@@ -597,9 +596,6 @@ static int picolcd_probe(struct hid_device *hdev,
hid_hw_stop(hdev);
err_cleanup_data:
kfree(data);
-err_no_cleanup:
- hid_set_drvdata(hdev, NULL);
-
return error;
}
@@ -635,7 +631,6 @@ static void picolcd_remove(struct hid_device *hdev)
picolcd_exit_cir(data);
picolcd_exit_keys(data);
- hid_set_drvdata(hdev, NULL);
mutex_destroy(&data->mutex);
/* Finally, clean up the picolcd data itself */
kfree(data);
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index be92a6f79687..94c7398b5c27 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -742,7 +742,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
}
spin_unlock_irqrestore(&data->lock, flags);
mfd_remove_devices(&hdev->dev);
- hid_set_drvdata(hdev, NULL);
mutex_destroy(&data->mutex);
}
--
2.19.2
^ permalink raw reply related
* [PATCH 0/2] HID: do not call hid_set_drvdata(hdev, NULL) in drivers
From: Benjamin Tissoires @ 2019-08-12 16:27 UTC (permalink / raw)
To: Bruno Prémont, Jonathan Cameron, Srinivas Pandruvada,
Ping Cheng, Jason Gerecke, Jiri Kosina
Cc: linux-input, linux-kernel, Benjamin Tissoires
Hi,
the wacom one is split dfrom the other drivers as the change is slightly
bigger.
Cheers,
Benjamin
Benjamin Tissoires (2):
HID: do not call hid_set_drvdata(hdev, NULL) in drivers
HID: wacom: do not call hid_set_drvdata(hdev, NULL)
drivers/hid/hid-cougar.c | 6 ++----
drivers/hid/hid-gfrm.c | 7 -------
drivers/hid/hid-lenovo.c | 2 --
drivers/hid/hid-picolcd_core.c | 7 +------
drivers/hid/hid-sensor-hub.c | 1 -
drivers/hid/wacom_sys.c | 18 +++++-------------
6 files changed, 8 insertions(+), 33 deletions(-)
--
2.19.2
^ permalink raw reply
* Re: [PATCH] HID: logitech-dj: add support of the G700(s) receiver
From: Hans de Goede @ 2019-08-12 16:26 UTC (permalink / raw)
To: Benjamin Tissoires, Jiri Kosina; +Cc: linux-input, linux-kernel
In-Reply-To: <20190812160804.11803-1-benjamin.tissoires@redhat.com>
Hi,
On 12-08-19 18:08, Benjamin Tissoires wrote:
> Both the G700 and the G700s are sharing the same receiver.
> Include support for this receiver in hid-logitech-dj so that userspace
> can differentiate both.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Nice, looks good to me:
Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Regards,
Hans
> ---
> drivers/hid/hid-logitech-dj.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index c547cba05fbb..d6250b0cb9f8 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -959,6 +959,7 @@ static void logi_hidpp_recv_queue_notif(struct hid_device *hdev,
> break;
> case 0x07:
> device_type = "eQUAD step 4 Gaming";
> + logi_hidpp_dev_conn_notif_equad(hdev, hidpp_report, &workitem);
> break;
> case 0x08:
> device_type = "eQUAD step 4 for gamepads";
> @@ -1832,6 +1833,10 @@ static const struct hid_device_id logi_dj_receivers[] = {
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_2),
> .driver_data = recvr_type_hidpp},
> + { /* Logitech G700(s) receiver (0xc531) */
> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> + 0xc531),
> + .driver_data = recvr_type_gaming_hidpp},
> { /* Logitech lightspeed receiver (0xc539) */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH,
> USB_DEVICE_ID_LOGITECH_NANO_RECEIVER_LIGHTSPEED),
>
^ permalink raw reply
* [PATCH 2/2] HID: multitouch: add support for the Smart Tech panel
From: Benjamin Tissoires @ 2019-08-12 16:23 UTC (permalink / raw)
To: Matthias Fend, Jiri Kosina; +Cc: linux-input, linux-kernel, Benjamin Tissoires
In-Reply-To: <20190812162326.14253-1-benjamin.tissoires@redhat.com>
This panel is not very friendly to us:
it exposes multiple multitouch collections, some of them being of
logical application stylus.
Usually, a device has only one report per application, and that is
what I assumed in commit 8dfe14b3b47f ("HID: multitouch: ditch mt_report_id")
To avoid breaking all working device, add a new class and a new quirk
for that situation.
Reported-and-tested-by: Matthias Fend <Matthias.Fend@wolfvision.net>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-multitouch.c | 34 ++++++++++++++++++++++++++++------
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 0d190d93ca7c..3cfeb1629f79 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -68,6 +68,7 @@ MODULE_LICENSE("GPL");
#define MT_QUIRK_STICKY_FINGERS BIT(16)
#define MT_QUIRK_ASUS_CUSTOM_UP BIT(17)
#define MT_QUIRK_WIN8_PTP_BUTTONS BIT(18)
+#define MT_QUIRK_SEPARATE_APP_REPORT BIT(19)
#define MT_INPUTMODE_TOUCHSCREEN 0x02
#define MT_INPUTMODE_TOUCHPAD 0x03
@@ -103,6 +104,7 @@ struct mt_usages {
struct mt_application {
struct list_head list;
unsigned int application;
+ unsigned int report_id;
struct list_head mt_usages; /* mt usages list */
__s32 quirks;
@@ -203,6 +205,7 @@ static void mt_post_parse(struct mt_device *td, struct mt_application *app);
#define MT_CLS_VTL 0x0110
#define MT_CLS_GOOGLE 0x0111
#define MT_CLS_RAZER_BLADE_STEALTH 0x0112
+#define MT_CLS_SMART_TECH 0x0113
#define MT_DEFAULT_MAXCONTACT 10
#define MT_MAX_MAXCONTACT 250
@@ -354,6 +357,12 @@ static const struct mt_class mt_classes[] = {
MT_QUIRK_CONTACT_CNT_ACCURATE |
MT_QUIRK_WIN8_PTP_BUTTONS,
},
+ { .name = MT_CLS_SMART_TECH,
+ .quirks = MT_QUIRK_ALWAYS_VALID |
+ MT_QUIRK_IGNORE_DUPLICATES |
+ MT_QUIRK_CONTACT_CNT_ACCURATE |
+ MT_QUIRK_SEPARATE_APP_REPORT,
+ },
{ }
};
@@ -510,8 +519,9 @@ static struct mt_usages *mt_allocate_usage(struct hid_device *hdev,
}
static struct mt_application *mt_allocate_application(struct mt_device *td,
- unsigned int application)
+ struct hid_report *report)
{
+ unsigned int application = report->application;
struct mt_application *mt_application;
mt_application = devm_kzalloc(&td->hdev->dev, sizeof(*mt_application),
@@ -536,6 +546,7 @@ static struct mt_application *mt_allocate_application(struct mt_device *td,
mt_application->scantime = DEFAULT_ZERO;
mt_application->raw_cc = DEFAULT_ZERO;
mt_application->quirks = td->mtclass.quirks;
+ mt_application->report_id = report->id;
list_add_tail(&mt_application->list, &td->applications);
@@ -543,19 +554,23 @@ static struct mt_application *mt_allocate_application(struct mt_device *td,
}
static struct mt_application *mt_find_application(struct mt_device *td,
- unsigned int application)
+ struct hid_report *report)
{
+ unsigned int application = report->application;
struct mt_application *tmp, *mt_application = NULL;
list_for_each_entry(tmp, &td->applications, list) {
if (application == tmp->application) {
- mt_application = tmp;
- break;
+ if (!(td->mtclass.quirks & MT_QUIRK_SEPARATE_APP_REPORT) ||
+ tmp->report_id == report->id) {
+ mt_application = tmp;
+ break;
+ }
}
}
if (!mt_application)
- mt_application = mt_allocate_application(td, application);
+ mt_application = mt_allocate_application(td, report);
return mt_application;
}
@@ -572,7 +587,7 @@ static struct mt_report_data *mt_allocate_report_data(struct mt_device *td,
return NULL;
rdata->report = report;
- rdata->application = mt_find_application(td, report->application);
+ rdata->application = mt_find_application(td, report);
if (!rdata->application) {
devm_kfree(&td->hdev->dev, rdata);
@@ -1562,6 +1577,9 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
case HID_VD_ASUS_CUSTOM_MEDIA_KEYS:
suffix = "Custom Media Keys";
break;
+ case HID_DG_PEN:
+ suffix = "Stylus";
+ break;
default:
suffix = "UNKNOWN";
break;
@@ -2023,6 +2041,10 @@ static const struct hid_device_id mt_devices[] = {
HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
USB_VENDOR_ID_SYNAPTICS, 0x8323) },
+ /* Smart Tech panels */
+ { .driver_data = MT_CLS_SMART_TECH,
+ MT_USB_DEVICE(0x0b8c, 0x0092)},
+
/* Stantum panels */
{ .driver_data = MT_CLS_CONFIDENCE,
MT_USB_DEVICE(USB_VENDOR_ID_STANTUM_STM,
--
2.19.2
^ permalink raw reply related
* [PATCH 1/2] HID: multitouch: do not filter mice nodes
From: Benjamin Tissoires @ 2019-08-12 16:23 UTC (permalink / raw)
To: Matthias Fend, Jiri Kosina; +Cc: linux-input, linux-kernel, Benjamin Tissoires
In-Reply-To: <20190812162326.14253-1-benjamin.tissoires@redhat.com>
It was a good idea at the time to not create a mouse node for the
multitouch touchscreens, but:
- touchscreens following the Win 8 protocol should not have this
disturbing mouse node anymore, or if they have, it should be
used for something else (like a joystick attached to the screen)
- touchpads have it, and they should not use it unless there is a bug,
but when the laptop has a trackstick, the data are reported through this
mouse node.
So instead of whitelisting all of the devices that have a need for the
mouse node, just export it.
hid-input.c will append a suffix to it ('Mouse'), so users will eventually
see if something goes wrong.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-multitouch.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index b603c14d043b..0d190d93ca7c 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -263,7 +263,8 @@ static const struct mt_class mt_classes[] = {
MT_QUIRK_HOVERING |
MT_QUIRK_CONTACT_CNT_ACCURATE |
MT_QUIRK_STICKY_FINGERS |
- MT_QUIRK_WIN8_PTP_BUTTONS },
+ MT_QUIRK_WIN8_PTP_BUTTONS,
+ .export_all_inputs = true },
{ .name = MT_CLS_EXPORT_ALL_INPUTS,
.quirks = MT_QUIRK_ALWAYS_VALID |
MT_QUIRK_CONTACT_CNT_ACCURATE,
--
2.19.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox