| Message ID | CAABJi+M-_hWdc6L-W3k8kA06Zd3tjAEb06LhsHOcwtzyjRRYbA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Daniel, On Mon, Jul 9, 2018 at 8:05 PM Daniel M. Lambea <dmlambea@gmail.com> wrote: > > Cougar 500k Gaming Keyboard have some special function keys that > make the keyboard stop responding when pressed. Implement the custom > vendor interface that deals with the extended keypresses to fix. > > The bug can be reproduced by plugging in the keyboard, then pressing the > rightmost part of the spacebar. > > Signed-off-by: Daniel M. Lambea <dmlambea@gmail.com> > --- > > One of those special function keys is just the right-half part of the > spacebar, so the chance of hitting it is very high. This is very annoying to the > user, since the only way of recovering the device back is to unplug it > and to plug > it back. > > The code, built as a DKMS module, has been released and tested by > others. For more > information, please refer to the bug: > https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1511511 Thanks for your contribution. I have a few comments I'll inline below. > > diff -uprN -X hid-vanilla/Documentation/dontdiff > hid-vanilla/drivers/hid/hid-cougar.c hid/drivers/hid/hid-cougar.c > --- hid-vanilla/drivers/hid/hid-cougar.c 1970-01-01 00:00:00.000000000 +0000 > +++ hid/drivers/hid/hid-cougar.c 2018-07-09 18:42:42.187292906 +0100 > @@ -0,0 +1,313 @@ > +/* > + * HID driver for Cougar 500k Gaming Keyboard > + * > + * Copyright (c) 2018 Daniel M. Lambea <dmlambea@gmail.com> > + */ > + > +/* > + * This program is free software; you can redistribute it and/or modify it > + * under the terms of the GNU General Public License as published by the Free > + * Software Foundation; either version 2 of the License, or (at your option) > + * any later version. > + */ The new and preferred way of adding the GPLv2+ license is by using SPDX: // SPDX-License-Identifier: GPL-2.0+ > + > +#include <linux/hid.h> > +#include <linux/module.h> > +#include <linux/usb.h> Generally, I really dislike when a HID driver needs to access usb.h. This basically kills replaying the devices through uhid, and is usually not future-proof. I usually recommend to detect which interface we are based on the report descriptors. > + > +#include "hid-ids.h" > + > +MODULE_AUTHOR("Daniel M. Lambea <dmlambea@gmail.com>"); > +MODULE_DESCRIPTION("Cougar 500k Gaming Keyboard"); > +MODULE_LICENSE("GPL"); > +MODULE_INFO(key_mappings, "G1-G6 are mapped to F13-F18"); > + > +static int cougar_g6_is_space = 1; > +module_param_named(g6_is_space, cougar_g6_is_space, int, 0600); > +MODULE_PARM_DESC(g6_is_space, > + "If set, G6 programmable key sends SPACE instead of F18 (0=off, > 1=on) (default=1)"); > + > + > +#define COUGAR_KEYB_INTFNO 0 > +#define COUGAR_MOUSE_INTFNO 1 > +#define COUGAR_RESERVED_INTFNO 2 > + > +#define COUGAR_RESERVED_FLD_CODE 1 > +#define COUGAR_RESERVED_FLD_ACTION 2 > + > +#define COUGAR_KEY_G1 0x83 > +#define COUGAR_KEY_G2 0x84 > +#define COUGAR_KEY_G3 0x85 > +#define COUGAR_KEY_G4 0x86 > +#define COUGAR_KEY_G5 0x87 > +#define COUGAR_KEY_G6 0x78 > +#define COUGAR_KEY_FN 0x0d > +#define COUGAR_KEY_MR 0x6f > +#define COUGAR_KEY_M1 0x80 > +#define COUGAR_KEY_M2 0x81 > +#define COUGAR_KEY_M3 0x82 > +#define COUGAR_KEY_LEDS 0x67 > +#define COUGAR_KEY_LOCK 0x6e > + > + > +/* Default key mappings */ > +static unsigned char cougar_mapping[][2] = { > + { COUGAR_KEY_G1, KEY_F13 }, > + { COUGAR_KEY_G2, KEY_F14 }, > + { COUGAR_KEY_G3, KEY_F15 }, > + { COUGAR_KEY_G4, KEY_F16 }, > + { COUGAR_KEY_G5, KEY_F17 }, > + { COUGAR_KEY_G6, KEY_F18 }, // This is handled individually > + { COUGAR_KEY_LOCK, KEY_SCREENLOCK }, > + { 0, 0 }, > +}; > + > +struct cougar_kbd_data { > + struct hid_device *owner; > + struct input_dev *input; > +}; > + > +/* > + * Constant-friendly rdesc fixup for mouse interface > + */ > +static __u8 *cougar_report_fixup(struct hid_device *hdev, __u8 *rdesc, > + unsigned int *rsize) > +{ > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); As mentioned, I'd rather see the fixup decided with the actual content of the rdesc, not the USB interface. > + > + if (intf->cur_altsetting->desc.bInterfaceNumber == COUGAR_MOUSE_INTFNO && > + (rdesc[0x73] | rdesc[0x74] << 8) >= HID_MAX_USAGES) { > + hid_info(hdev, "usage count exceeds max: fixing up report descriptor"); > + rdesc[0x73] = ((HID_MAX_USAGES-1) & 0xff); > + rdesc[0x74] = ((HID_MAX_USAGES-1) >> 8); > + } > + return rdesc; > +} > + > +/* > + * Returns a sibling hid_device with the given intf number > + */ > +static struct hid_device *cougar_get_sibling_hid_device(struct > hid_device *hdev, > + const __u8 intfno) I am not a big fan of the siblings here. I think you want it for rerouting the events from the proprietary HID node to the keyboard one. Also, the data you are sharing is not 'correct'. You should not share a data bound to a specific hid device, but a shared one that is refcounted and that will have its own life span. For an example of that, see wacom_sys.c in drivers/hid/. > +{ > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + struct usb_device *device; > + struct usb_host_config *hostcfg; > + struct hid_device *siblingHdev; > + int i; > + > + if (intf->cur_altsetting->desc.bInterfaceNumber == intfno) > + hid_err(hdev, "returning hid device's data from myself?"); > + > + device = interface_to_usbdev(intf); > + hostcfg = device->config; > + siblingHdev = NULL; > + for (i = 0; i < hostcfg->desc.bNumInterfaces; i++) { > + if (i == intfno) > + return usb_get_intfdata(hostcfg->interface[i]); > + } > + return NULL; > +} > + > +static int cougar_set_drvdata_from_keyb_interface(struct hid_device *hdev) > +{ > + struct hid_device *sibling; > + struct cougar_kbd_data *kbd; > + > + /* Search for the keyboard */ > + sibling = cougar_get_sibling_hid_device(hdev, COUGAR_KEYB_INTFNO); > + if (sibling == NULL) { > + hid_err(hdev, > + "no sibling hid device found for intf %d", COUGAR_KEYB_INTFNO); > + return -ENODEV; > + } > + > + kbd = hid_get_drvdata(sibling); > + if (kbd == NULL || kbd->input == NULL) { > + hid_err(hdev, > + "keyboard descriptor not found in intf %d", COUGAR_KEYB_INTFNO); > + return -ENODATA; > + } > + > + /* And save its data on reserved, custom vendor intf. device */ > + hid_set_drvdata(hdev, kbd); > + return 0; > +} > + > +/* > + * The probe will save the keyb's input device, so that the > + * vendor intf will be able to send keys as if it were the > + * keyboard itself. > + */ > +static int cougar_probe(struct hid_device *hdev, > + const struct hid_device_id *id) > +{ > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + struct cougar_kbd_data *kbd = NULL; > + unsigned int mask; > + int ret; > + __u8 intfno; > + > + intfno = intf->cur_altsetting->desc.bInterfaceNumber; > + hid_dbg(hdev, "about to probe intf %d", intfno); > + > + if (intfno == COUGAR_KEYB_INTFNO) { > + kbd = devm_kzalloc(&hdev->dev, sizeof(*kbd), GFP_KERNEL); > + if (kbd == NULL) > + return -ENOMEM; > + > + hid_dbg(hdev, "attaching kbd data to intf %d", intfno); Not sure we need these debug messages. > + hid_set_drvdata(hdev, kbd); > + } > + > + /* Parse and start hw */ > + ret = hid_parse(hdev); > + if (ret) > + goto err_cleanup; > + > + /* Reserved, custom vendor interface connects hidraw only */ > + mask = intfno == COUGAR_RESERVED_INTFNO ? > + HID_CONNECT_HIDRAW : HID_CONNECT_DEFAULT; > + ret = hid_hw_start(hdev, mask); > + if (ret) > + goto err_cleanup; if you are using the devm API, using 'goto' is usually a hint that something went wrong... > + > + if (intfno == COUGAR_RESERVED_INTFNO) { > + ret = cougar_set_drvdata_from_keyb_interface(hdev); This should probably happen before hid_hw_start(), as you might have a race between an incoming report and the fetching of the sibling device input node. > + if (ret) > + goto err_stop_and_cleanup; > + > + hid_dbg(hdev, "keyboard descriptor attached to intf %d", intfno); > + > + ret = hid_hw_open(hdev); > + if (ret) > + goto err_stop_and_cleanup; > + } > + hid_dbg(hdev, "intf %d probed successfully", intfno); > + > + return 0; > + > +err_stop_and_cleanup: > + hid_hw_stop(hdev); this one is fine though (but unusual) > +err_cleanup: > + hid_set_drvdata(hdev, NULL); This is nice to do, but not really important. A driver that has its .probe() called should reset it if it needs it. > + if (kbd != NULL && kbd->owner == hdev) > + devm_kfree(&hdev->dev, kbd); The point of using the devm (dev managed) API is to not have to call kfree. This can be removed without a blink. > + return ret; > +} > + > +/* > + * Keeps track of the keyboard's hid_input > + */ > +static int cougar_input_configured(struct hid_device *hdev, struct > hid_input *hidinput) > +{ > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + __u8 intfno = intf->cur_altsetting->desc.bInterfaceNumber; > + struct cougar_kbd_data *kbd; > + > + if (intfno != COUGAR_KEYB_INTFNO) { > + /* Skip interfaces other than COUGAR_KEYB_INTFNO, > + * which is the one containing the configured hidinput > + */ > + hid_dbg(hdev, "input_configured: skipping intf %d", intfno); > + return 0; > + } > + kbd = hid_get_drvdata(hdev); > + kbd->owner = hdev; This should probably be set in .probe() > + kbd->input = hidinput->input; > + hid_dbg(hdev, "input_configured: intf %d configured", intfno); > + return 0; > +} > + > +/* > + * Converts events from vendor intf to input key events > + */ > +static int cougar_raw_event(struct hid_device *hdev, struct hid_report *report, > + u8 *data, int size) > +{ > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + struct cougar_kbd_data *kbd; > + unsigned char action, code, transcode; > + int i; > + > + if (intf->cur_altsetting->desc.bInterfaceNumber != COUGAR_RESERVED_INTFNO) > + return 0; I'd rather have you store a flag in kbd to know which interface you are in instead of relying on USB each time. > + > + // Enable this to see a dump of the data received from reserved interface > + //print_hex_dump(KERN_ERR, DRIVER_NAME " data : ", No C++ style comments, please. > DUMP_PREFIX_OFFSET, 8, 1, data, size, 0); Your email client seems to have introduce line breaks here, you need to fix it so you don't have to do manual processing of the patches. > + > + code = data[COUGAR_RESERVED_FLD_CODE]; > + switch (code) { > + case COUGAR_KEY_FN: > + hid_dbg(hdev, "FN (special function) key is handled by the > hardware itself"); > + break; > + case COUGAR_KEY_MR: > + hid_dbg(hdev, "MR (macro recording) key is handled by the > hardware itself"); > + break; > + case COUGAR_KEY_M1: > + hid_dbg(hdev, "M1 (macro set 1) key is handled by the > hardware itself"); > + break; > + case COUGAR_KEY_M2: > + hid_dbg(hdev, "M2 (macro set 2) key is handled by the > hardware itself"); > + break; > + case COUGAR_KEY_M3: > + hid_dbg(hdev, "M3 (macro set 3) key is handled by the > hardware itself"); > + break; > + case COUGAR_KEY_LEDS: > + hid_dbg(hdev, "LED (led set) key is handled by the hardware itself"); What is the point of all of these hid_dbg messages? > + break; > + default: > + /* Try normal key mappings */ > + for (i = 0; cougar_mapping[i][0]; i++) { Here you have a mapping table, so I wonder if you better not have a IGNORE flag in your mapping table and include the previous checks in the mapping table so you only have one loop instead of a switch + loop. > + if (cougar_mapping[i][0] == code) { > + action = data[COUGAR_RESERVED_FLD_ACTION]; > + hid_dbg(hdev, "found mapping for code %x", code); > + if (code == COUGAR_KEY_G6 && cougar_g6_is_space) If you have an explicit test for it that will be run for all of the inpus, I wonder if you should not put the test at the beginning. > + transcode = KEY_SPACE; > + else > + transcode = cougar_mapping[i][1]; > + > + kbd = hid_get_drvdata(hdev); > + input_event(kbd->input, EV_KEY, transcode, action); > + input_sync(kbd->input); > + hid_dbg(hdev, "translated to %x", transcode); > + return 0; > + } > + } > + hid_warn(hdev, "unmapped key code %d: ignoring", code); > + } > + return 0; > +} > + > +static void cougar_remove(struct hid_device *hdev) > +{ > + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > + struct cougar_kbd_data *kbd = hid_get_drvdata(hdev); > + > + hid_dbg(hdev, "removing %d", intf->cur_altsetting->desc.bInterfaceNumber); > + if (intf->cur_altsetting->desc.bInterfaceNumber == COUGAR_RESERVED_INTFNO) > + hid_hw_close(hdev); Again, I'd rather have this info stored in the driverdata instead of relying on the usb stack. > + > + hid_hw_stop(hdev); > + hid_set_drvdata(hdev, NULL); > + if (kbd != NULL && kbd->owner == hdev) > + devm_kfree(&hdev->dev, kbd); Same comments than in probe(). One more applies here: you now have a race between the removal of the keyboard device and the incoming events from the proprietary collection: - you call remove of the HID devices, only the keyboard one is being processed - an IRQ comes in, .raw_event is called for the proprietary interface - .raw_events tries to access the drvdata from the keyboard interface that has been removed -> kernel oops I am not entirely sure if USB will allow that or not, but I *think* this can be triggered by rmmod-ing your driver. > +} > + > +static struct hid_device_id cougar_id_table[] = { > + { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, > USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD) }, > + {} > +}; > +MODULE_DEVICE_TABLE(hid, cougar_id_table); > + > +static struct hid_driver cougar_driver = { > + .name = "cougar", > + .id_table = cougar_id_table, > + .report_fixup = cougar_report_fixup, > + .probe = cougar_probe, > + .input_configured = cougar_input_configured, > + .remove = cougar_remove, > + .raw_event = cougar_raw_event, > +}; > + > +module_hid_driver(cougar_driver); > diff -uprN -X hid-vanilla/Documentation/dontdiff > hid-vanilla/drivers/hid/hid-ids.h hid/drivers/hid/hid-ids.h > --- hid-vanilla/drivers/hid/hid-ids.h 2018-07-09 17:48:30.189316299 +0100 > +++ hid/drivers/hid/hid-ids.h 2018-07-09 17:54:29.332832182 +0100 > @@ -1001,6 +1001,9 @@ > #define USB_VENDOR_ID_SINO_LITE 0x1345 > #define USB_DEVICE_ID_SINO_LITE_CONTROLLER 0x3008 > > +#define USB_VENDOR_ID_SOLID_YEAR 0x060b > +#define USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD 0x500a > + > #define USB_VENDOR_ID_SOUNDGRAPH 0x15c2 > #define USB_DEVICE_ID_SOUNDGRAPH_IMON_FIRST 0x0034 > #define USB_DEVICE_ID_SOUNDGRAPH_IMON_LAST 0x0046 > diff -uprN -X hid-vanilla/Documentation/dontdiff > hid-vanilla/drivers/hid/hid-quirks.c hid/drivers/hid/hid-quirks.c > --- hid-vanilla/drivers/hid/hid-quirks.c 2018-07-09 17:48:30.193316294 +0100 > +++ hid/drivers/hid/hid-quirks.c 2018-07-09 17:54:42.708814231 +0100 > @@ -610,6 +610,9 @@ static const struct hid_device_id hid_ha > { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP_LTD, > USB_DEVICE_ID_SUPER_DUAL_BOX_PRO) }, > { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP_LTD, > USB_DEVICE_ID_SUPER_JOY_BOX_5_PRO) }, > #endif > +#if IS_ENABLED(CONFIG_HID_COUGAR) > + { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, > USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD) }, > +#endif This can be dropped as of v4.16, and I strongly encourage you to drop it. This way, hid-generic can bind to your keyboard during the initramfs phase, and when teh full system will be set up, hid-cougar will take over. This is useful for LUKS passwords. > #if IS_ENABLED(CONFIG_HID_SONY) > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, > USB_DEVICE_ID_LOGITECH_HARMONY_PS3) }, > { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SMK, > USB_DEVICE_ID_SMK_PS3_BDREMOTE) }, > diff -uprN -X hid-vanilla/Documentation/dontdiff > hid-vanilla/drivers/hid/Kconfig hid/drivers/hid/Kconfig > --- hid-vanilla/drivers/hid/Kconfig 2018-07-09 17:48:30.185316305 +0100 > +++ hid/drivers/hid/Kconfig 2018-07-09 18:46:10.075657150 +0100 > @@ -207,6 +207,16 @@ config HID_CORSAIR > - Vengeance K90 > - Scimitar PRO RGB > > +config HID_COUGAR > + tristate "Cougar devices" > + depends on HID > + help > + Support for Cougar devices that are not fully compliant with the > + HID standard. > + > + Supported devices: > + - Cougar 500k Gaming Keyboard > + > config HID_PRODIKEYS > tristate "Prodikeys PC-MIDI Keyboard support" > depends on HID && SND > diff -uprN -X hid-vanilla/Documentation/dontdiff > hid-vanilla/drivers/hid/Makefile hid/drivers/hid/Makefile > --- hid-vanilla/drivers/hid/Makefile 2018-07-09 17:48:30.185316305 +0100 > +++ hid/drivers/hid/Makefile 2018-07-09 17:54:15.140851231 +0100 > @@ -35,6 +35,7 @@ obj-$(CONFIG_HID_CHERRY) += hid-cherry.o > obj-$(CONFIG_HID_CHICONY) += hid-chicony.o > obj-$(CONFIG_HID_CMEDIA) += hid-cmedia.o > obj-$(CONFIG_HID_CORSAIR) += hid-corsair.o > +obj-$(CONFIG_HID_COUGAR) += hid-cougar.o > obj-$(CONFIG_HID_CP2112) += hid-cp2112.o > obj-$(CONFIG_HID_CYPRESS) += hid-cypress.o > obj-$(CONFIG_HID_DRAGONRISE) += hid-dr.o Cheers, Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Benjamin Thank you very much for your detailed review. I appreciate it very much since this is my very first attempt of writing something related to the Linux kernel and your comments and guidelines are very valuable to me. I will address all the issues you've spotted and send you back a new patch. Regarding the sibling interface, I find the examples at wacom_sys.c very illustrative. Regards, Daniel M. Lambea On Tue, Jul 10, 2018 at 5:16 PM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > Hi Daniel, > > On Mon, Jul 9, 2018 at 8:05 PM Daniel M. Lambea <dmlambea@gmail.com> wrote: >> >> Cougar 500k Gaming Keyboard have some special function keys that >> make the keyboard stop responding when pressed. Implement the custom >> vendor interface that deals with the extended keypresses to fix. >> >> The bug can be reproduced by plugging in the keyboard, then pressing the >> rightmost part of the spacebar. >> >> Signed-off-by: Daniel M. Lambea <dmlambea@gmail.com> >> --- >> >> One of those special function keys is just the right-half part of the >> spacebar, so the chance of hitting it is very high. This is very annoying to the >> user, since the only way of recovering the device back is to unplug it >> and to plug >> it back. >> >> The code, built as a DKMS module, has been released and tested by >> others. For more >> information, please refer to the bug: >> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1511511 > > Thanks for your contribution. I have a few comments I'll inline below. > >> >> diff -uprN -X hid-vanilla/Documentation/dontdiff >> hid-vanilla/drivers/hid/hid-cougar.c hid/drivers/hid/hid-cougar.c >> --- hid-vanilla/drivers/hid/hid-cougar.c 1970-01-01 00:00:00.000000000 +0000 >> +++ hid/drivers/hid/hid-cougar.c 2018-07-09 18:42:42.187292906 +0100 >> @@ -0,0 +1,313 @@ >> +/* >> + * HID driver for Cougar 500k Gaming Keyboard >> + * >> + * Copyright (c) 2018 Daniel M. Lambea <dmlambea@gmail.com> >> + */ >> + >> +/* >> + * This program is free software; you can redistribute it and/or modify it >> + * under the terms of the GNU General Public License as published by the Free >> + * Software Foundation; either version 2 of the License, or (at your option) >> + * any later version. >> + */ > > The new and preferred way of adding the GPLv2+ license is by using SPDX: > // SPDX-License-Identifier: GPL-2.0+ > >> + >> +#include <linux/hid.h> >> +#include <linux/module.h> >> +#include <linux/usb.h> > > Generally, I really dislike when a HID driver needs to access usb.h. > This basically kills replaying the devices through uhid, and is > usually not future-proof. I usually recommend to detect which > interface we are based on the report descriptors. >> + >> +#include "hid-ids.h" >> + >> +MODULE_AUTHOR("Daniel M. Lambea <dmlambea@gmail.com>"); >> +MODULE_DESCRIPTION("Cougar 500k Gaming Keyboard"); >> +MODULE_LICENSE("GPL"); >> +MODULE_INFO(key_mappings, "G1-G6 are mapped to F13-F18"); >> + >> +static int cougar_g6_is_space = 1; >> +module_param_named(g6_is_space, cougar_g6_is_space, int, 0600); >> +MODULE_PARM_DESC(g6_is_space, >> + "If set, G6 programmable key sends SPACE instead of F18 (0=off, >> 1=on) (default=1)"); >> + >> + >> +#define COUGAR_KEYB_INTFNO 0 >> +#define COUGAR_MOUSE_INTFNO 1 >> +#define COUGAR_RESERVED_INTFNO 2 >> + >> +#define COUGAR_RESERVED_FLD_CODE 1 >> +#define COUGAR_RESERVED_FLD_ACTION 2 >> + >> +#define COUGAR_KEY_G1 0x83 >> +#define COUGAR_KEY_G2 0x84 >> +#define COUGAR_KEY_G3 0x85 >> +#define COUGAR_KEY_G4 0x86 >> +#define COUGAR_KEY_G5 0x87 >> +#define COUGAR_KEY_G6 0x78 >> +#define COUGAR_KEY_FN 0x0d >> +#define COUGAR_KEY_MR 0x6f >> +#define COUGAR_KEY_M1 0x80 >> +#define COUGAR_KEY_M2 0x81 >> +#define COUGAR_KEY_M3 0x82 >> +#define COUGAR_KEY_LEDS 0x67 >> +#define COUGAR_KEY_LOCK 0x6e >> + >> + >> +/* Default key mappings */ >> +static unsigned char cougar_mapping[][2] = { >> + { COUGAR_KEY_G1, KEY_F13 }, >> + { COUGAR_KEY_G2, KEY_F14 }, >> + { COUGAR_KEY_G3, KEY_F15 }, >> + { COUGAR_KEY_G4, KEY_F16 }, >> + { COUGAR_KEY_G5, KEY_F17 }, >> + { COUGAR_KEY_G6, KEY_F18 }, // This is handled individually >> + { COUGAR_KEY_LOCK, KEY_SCREENLOCK }, >> + { 0, 0 }, >> +}; >> + >> +struct cougar_kbd_data { >> + struct hid_device *owner; >> + struct input_dev *input; >> +}; >> + >> +/* >> + * Constant-friendly rdesc fixup for mouse interface >> + */ >> +static __u8 *cougar_report_fixup(struct hid_device *hdev, __u8 *rdesc, >> + unsigned int *rsize) >> +{ >> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); > > As mentioned, I'd rather see the fixup decided with the actual content > of the rdesc, not the USB interface. > >> + >> + if (intf->cur_altsetting->desc.bInterfaceNumber == COUGAR_MOUSE_INTFNO && >> + (rdesc[0x73] | rdesc[0x74] << 8) >= HID_MAX_USAGES) { >> + hid_info(hdev, "usage count exceeds max: fixing up report descriptor"); >> + rdesc[0x73] = ((HID_MAX_USAGES-1) & 0xff); >> + rdesc[0x74] = ((HID_MAX_USAGES-1) >> 8); >> + } >> + return rdesc; >> +} >> + >> +/* >> + * Returns a sibling hid_device with the given intf number >> + */ >> +static struct hid_device *cougar_get_sibling_hid_device(struct >> hid_device *hdev, >> + const __u8 intfno) > > I am not a big fan of the siblings here. I think you want it for > rerouting the events from the proprietary HID node to the keyboard > one. > > Also, the data you are sharing is not 'correct'. You should not share > a data bound to a specific hid device, but a shared one that is > refcounted and that will have its own life span. > > For an example of that, see wacom_sys.c in drivers/hid/. > >> +{ >> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); >> + struct usb_device *device; >> + struct usb_host_config *hostcfg; >> + struct hid_device *siblingHdev; >> + int i; >> + >> + if (intf->cur_altsetting->desc.bInterfaceNumber == intfno) >> + hid_err(hdev, "returning hid device's data from myself?"); >> + >> + device = interface_to_usbdev(intf); >> + hostcfg = device->config; >> + siblingHdev = NULL; >> + for (i = 0; i < hostcfg->desc.bNumInterfaces; i++) { >> + if (i == intfno) >> + return usb_get_intfdata(hostcfg->interface[i]); >> + } >> + return NULL; >> +} >> + >> +static int cougar_set_drvdata_from_keyb_interface(struct hid_device *hdev) >> +{ >> + struct hid_device *sibling; >> + struct cougar_kbd_data *kbd; >> + >> + /* Search for the keyboard */ >> + sibling = cougar_get_sibling_hid_device(hdev, COUGAR_KEYB_INTFNO); >> + if (sibling == NULL) { >> + hid_err(hdev, >> + "no sibling hid device found for intf %d", COUGAR_KEYB_INTFNO); >> + return -ENODEV; >> + } >> + >> + kbd = hid_get_drvdata(sibling); >> + if (kbd == NULL || kbd->input == NULL) { >> + hid_err(hdev, >> + "keyboard descriptor not found in intf %d", COUGAR_KEYB_INTFNO); >> + return -ENODATA; >> + } >> + >> + /* And save its data on reserved, custom vendor intf. device */ >> + hid_set_drvdata(hdev, kbd); >> + return 0; >> +} >> + >> +/* >> + * The probe will save the keyb's input device, so that the >> + * vendor intf will be able to send keys as if it were the >> + * keyboard itself. >> + */ >> +static int cougar_probe(struct hid_device *hdev, >> + const struct hid_device_id *id) >> +{ >> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); >> + struct cougar_kbd_data *kbd = NULL; >> + unsigned int mask; >> + int ret; >> + __u8 intfno; >> + >> + intfno = intf->cur_altsetting->desc.bInterfaceNumber; >> + hid_dbg(hdev, "about to probe intf %d", intfno); >> + >> + if (intfno == COUGAR_KEYB_INTFNO) { >> + kbd = devm_kzalloc(&hdev->dev, sizeof(*kbd), GFP_KERNEL); >> + if (kbd == NULL) >> + return -ENOMEM; >> + >> + hid_dbg(hdev, "attaching kbd data to intf %d", intfno); > > Not sure we need these debug messages. > >> + hid_set_drvdata(hdev, kbd); >> + } >> + >> + /* Parse and start hw */ >> + ret = hid_parse(hdev); >> + if (ret) >> + goto err_cleanup; >> + >> + /* Reserved, custom vendor interface connects hidraw only */ >> + mask = intfno == COUGAR_RESERVED_INTFNO ? >> + HID_CONNECT_HIDRAW : HID_CONNECT_DEFAULT; >> + ret = hid_hw_start(hdev, mask); >> + if (ret) >> + goto err_cleanup; > > if you are using the devm API, using 'goto' is usually a hint that > something went wrong... > >> + >> + if (intfno == COUGAR_RESERVED_INTFNO) { >> + ret = cougar_set_drvdata_from_keyb_interface(hdev); > > This should probably happen before hid_hw_start(), as you might have a > race between an incoming report and the fetching of the sibling device > input node. > >> + if (ret) >> + goto err_stop_and_cleanup; >> + >> + hid_dbg(hdev, "keyboard descriptor attached to intf %d", intfno); >> + >> + ret = hid_hw_open(hdev); >> + if (ret) >> + goto err_stop_and_cleanup; >> + } >> + hid_dbg(hdev, "intf %d probed successfully", intfno); >> + >> + return 0; >> + >> +err_stop_and_cleanup: >> + hid_hw_stop(hdev); > > this one is fine though (but unusual) > >> +err_cleanup: >> + hid_set_drvdata(hdev, NULL); > > This is nice to do, but not really important. A driver that has its > .probe() called should reset it if it needs it. > >> + if (kbd != NULL && kbd->owner == hdev) >> + devm_kfree(&hdev->dev, kbd); > > The point of using the devm (dev managed) API is to not have to call > kfree. This can be removed without a blink. > >> + return ret; >> +} >> + >> +/* >> + * Keeps track of the keyboard's hid_input >> + */ >> +static int cougar_input_configured(struct hid_device *hdev, struct >> hid_input *hidinput) >> +{ >> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); >> + __u8 intfno = intf->cur_altsetting->desc.bInterfaceNumber; >> + struct cougar_kbd_data *kbd; >> + >> + if (intfno != COUGAR_KEYB_INTFNO) { >> + /* Skip interfaces other than COUGAR_KEYB_INTFNO, >> + * which is the one containing the configured hidinput >> + */ >> + hid_dbg(hdev, "input_configured: skipping intf %d", intfno); >> + return 0; >> + } >> + kbd = hid_get_drvdata(hdev); >> + kbd->owner = hdev; > > This should probably be set in .probe() > >> + kbd->input = hidinput->input; >> + hid_dbg(hdev, "input_configured: intf %d configured", intfno); >> + return 0; >> +} >> + >> +/* >> + * Converts events from vendor intf to input key events >> + */ >> +static int cougar_raw_event(struct hid_device *hdev, struct hid_report *report, >> + u8 *data, int size) >> +{ >> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); >> + struct cougar_kbd_data *kbd; >> + unsigned char action, code, transcode; >> + int i; >> + >> + if (intf->cur_altsetting->desc.bInterfaceNumber != COUGAR_RESERVED_INTFNO) >> + return 0; > > I'd rather have you store a flag in kbd to know which interface you > are in instead of relying on USB each time. > >> + >> + // Enable this to see a dump of the data received from reserved interface >> + //print_hex_dump(KERN_ERR, DRIVER_NAME " data : ", > > No C++ style comments, please. > >> DUMP_PREFIX_OFFSET, 8, 1, data, size, 0); > > Your email client seems to have introduce line breaks here, you need > to fix it so you don't have to do manual processing of the patches. > >> + >> + code = data[COUGAR_RESERVED_FLD_CODE]; >> + switch (code) { >> + case COUGAR_KEY_FN: >> + hid_dbg(hdev, "FN (special function) key is handled by the >> hardware itself"); >> + break; >> + case COUGAR_KEY_MR: >> + hid_dbg(hdev, "MR (macro recording) key is handled by the >> hardware itself"); >> + break; >> + case COUGAR_KEY_M1: >> + hid_dbg(hdev, "M1 (macro set 1) key is handled by the >> hardware itself"); >> + break; >> + case COUGAR_KEY_M2: >> + hid_dbg(hdev, "M2 (macro set 2) key is handled by the >> hardware itself"); >> + break; >> + case COUGAR_KEY_M3: >> + hid_dbg(hdev, "M3 (macro set 3) key is handled by the >> hardware itself"); >> + break; >> + case COUGAR_KEY_LEDS: >> + hid_dbg(hdev, "LED (led set) key is handled by the hardware itself"); > > What is the point of all of these hid_dbg messages? > >> + break; >> + default: >> + /* Try normal key mappings */ >> + for (i = 0; cougar_mapping[i][0]; i++) { > > Here you have a mapping table, so I wonder if you better not have a > IGNORE flag in your mapping table and include the previous checks in > the mapping table so you only have one loop instead of a switch + > loop. > >> + if (cougar_mapping[i][0] == code) { >> + action = data[COUGAR_RESERVED_FLD_ACTION]; >> + hid_dbg(hdev, "found mapping for code %x", code); >> + if (code == COUGAR_KEY_G6 && cougar_g6_is_space) > > If you have an explicit test for it that will be run for all of the > inpus, I wonder if you should not put the test at the beginning. > >> + transcode = KEY_SPACE; >> + else >> + transcode = cougar_mapping[i][1]; >> + >> + kbd = hid_get_drvdata(hdev); >> + input_event(kbd->input, EV_KEY, transcode, action); >> + input_sync(kbd->input); >> + hid_dbg(hdev, "translated to %x", transcode); >> + return 0; >> + } >> + } >> + hid_warn(hdev, "unmapped key code %d: ignoring", code); >> + } >> + return 0; >> +} >> + >> +static void cougar_remove(struct hid_device *hdev) >> +{ >> + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); >> + struct cougar_kbd_data *kbd = hid_get_drvdata(hdev); >> + >> + hid_dbg(hdev, "removing %d", intf->cur_altsetting->desc.bInterfaceNumber); >> + if (intf->cur_altsetting->desc.bInterfaceNumber == COUGAR_RESERVED_INTFNO) >> + hid_hw_close(hdev); > > Again, I'd rather have this info stored in the driverdata instead of > relying on the usb stack. > >> + >> + hid_hw_stop(hdev); >> + hid_set_drvdata(hdev, NULL); >> + if (kbd != NULL && kbd->owner == hdev) >> + devm_kfree(&hdev->dev, kbd); > > Same comments than in probe(). > > One more applies here: > you now have a race between the removal of the keyboard device and the > incoming events from the proprietary collection: > - you call remove of the HID devices, only the keyboard one is being processed > - an IRQ comes in, .raw_event is called for the proprietary interface > - .raw_events tries to access the drvdata from the keyboard interface > that has been removed -> kernel oops > > I am not entirely sure if USB will allow that or not, but I *think* > this can be triggered by rmmod-ing your driver. > >> +} >> + >> +static struct hid_device_id cougar_id_table[] = { >> + { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, >> USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD) }, >> + {} >> +}; >> +MODULE_DEVICE_TABLE(hid, cougar_id_table); >> + >> +static struct hid_driver cougar_driver = { >> + .name = "cougar", >> + .id_table = cougar_id_table, >> + .report_fixup = cougar_report_fixup, >> + .probe = cougar_probe, >> + .input_configured = cougar_input_configured, >> + .remove = cougar_remove, >> + .raw_event = cougar_raw_event, >> +}; >> + >> +module_hid_driver(cougar_driver); >> diff -uprN -X hid-vanilla/Documentation/dontdiff >> hid-vanilla/drivers/hid/hid-ids.h hid/drivers/hid/hid-ids.h >> --- hid-vanilla/drivers/hid/hid-ids.h 2018-07-09 17:48:30.189316299 +0100 >> +++ hid/drivers/hid/hid-ids.h 2018-07-09 17:54:29.332832182 +0100 >> @@ -1001,6 +1001,9 @@ >> #define USB_VENDOR_ID_SINO_LITE 0x1345 >> #define USB_DEVICE_ID_SINO_LITE_CONTROLLER 0x3008 >> >> +#define USB_VENDOR_ID_SOLID_YEAR 0x060b >> +#define USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD 0x500a >> + >> #define USB_VENDOR_ID_SOUNDGRAPH 0x15c2 >> #define USB_DEVICE_ID_SOUNDGRAPH_IMON_FIRST 0x0034 >> #define USB_DEVICE_ID_SOUNDGRAPH_IMON_LAST 0x0046 >> diff -uprN -X hid-vanilla/Documentation/dontdiff >> hid-vanilla/drivers/hid/hid-quirks.c hid/drivers/hid/hid-quirks.c >> --- hid-vanilla/drivers/hid/hid-quirks.c 2018-07-09 17:48:30.193316294 +0100 >> +++ hid/drivers/hid/hid-quirks.c 2018-07-09 17:54:42.708814231 +0100 >> @@ -610,6 +610,9 @@ static const struct hid_device_id hid_ha >> { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP_LTD, >> USB_DEVICE_ID_SUPER_DUAL_BOX_PRO) }, >> { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP_LTD, >> USB_DEVICE_ID_SUPER_JOY_BOX_5_PRO) }, >> #endif >> +#if IS_ENABLED(CONFIG_HID_COUGAR) >> + { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, >> USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD) }, >> +#endif > > This can be dropped as of v4.16, and I strongly encourage you to drop > it. This way, hid-generic can bind to your keyboard during the > initramfs phase, and when teh full system will be set up, hid-cougar > will take over. This is useful for LUKS passwords. > >> #if IS_ENABLED(CONFIG_HID_SONY) >> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, >> USB_DEVICE_ID_LOGITECH_HARMONY_PS3) }, >> { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SMK, >> USB_DEVICE_ID_SMK_PS3_BDREMOTE) }, >> diff -uprN -X hid-vanilla/Documentation/dontdiff >> hid-vanilla/drivers/hid/Kconfig hid/drivers/hid/Kconfig >> --- hid-vanilla/drivers/hid/Kconfig 2018-07-09 17:48:30.185316305 +0100 >> +++ hid/drivers/hid/Kconfig 2018-07-09 18:46:10.075657150 +0100 >> @@ -207,6 +207,16 @@ config HID_CORSAIR >> - Vengeance K90 >> - Scimitar PRO RGB >> >> +config HID_COUGAR >> + tristate "Cougar devices" >> + depends on HID >> + help >> + Support for Cougar devices that are not fully compliant with the >> + HID standard. >> + >> + Supported devices: >> + - Cougar 500k Gaming Keyboard >> + >> config HID_PRODIKEYS >> tristate "Prodikeys PC-MIDI Keyboard support" >> depends on HID && SND >> diff -uprN -X hid-vanilla/Documentation/dontdiff >> hid-vanilla/drivers/hid/Makefile hid/drivers/hid/Makefile >> --- hid-vanilla/drivers/hid/Makefile 2018-07-09 17:48:30.185316305 +0100 >> +++ hid/drivers/hid/Makefile 2018-07-09 17:54:15.140851231 +0100 >> @@ -35,6 +35,7 @@ obj-$(CONFIG_HID_CHERRY) += hid-cherry.o >> obj-$(CONFIG_HID_CHICONY) += hid-chicony.o >> obj-$(CONFIG_HID_CMEDIA) += hid-cmedia.o >> obj-$(CONFIG_HID_CORSAIR) += hid-corsair.o >> +obj-$(CONFIG_HID_COUGAR) += hid-cougar.o >> obj-$(CONFIG_HID_CP2112) += hid-cp2112.o >> obj-$(CONFIG_HID_CYPRESS) += hid-cypress.o >> obj-$(CONFIG_HID_DRAGONRISE) += hid-dr.o > > Cheers, > Benjamin -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff -uprN -X hid-vanilla/Documentation/dontdiff hid-vanilla/drivers/hid/hid-cougar.c hid/drivers/hid/hid-cougar.c --- hid-vanilla/drivers/hid/hid-cougar.c 1970-01-01 00:00:00.000000000 +0000 +++ hid/drivers/hid/hid-cougar.c 2018-07-09 18:42:42.187292906 +0100 @@ -0,0 +1,313 @@ +/* + * HID driver for Cougar 500k Gaming Keyboard + * + * Copyright (c) 2018 Daniel M. Lambea <dmlambea@gmail.com> + */ + +/* + * This program is free software; you can redistribute it and/or modify it + * under the terms of the GNU General Public License as published by the Free + * Software Foundation; either version 2 of the License, or (at your option) + * any later version. + */ + +#include <linux/hid.h> +#include <linux/module.h> +#include <linux/usb.h> + +#include "hid-ids.h" + +MODULE_AUTHOR("Daniel M. Lambea <dmlambea@gmail.com>"); +MODULE_DESCRIPTION("Cougar 500k Gaming Keyboard"); +MODULE_LICENSE("GPL"); +MODULE_INFO(key_mappings, "G1-G6 are mapped to F13-F18"); + +static int cougar_g6_is_space = 1; +module_param_named(g6_is_space, cougar_g6_is_space, int, 0600); +MODULE_PARM_DESC(g6_is_space, + "If set, G6 programmable key sends SPACE instead of F18 (0=off, 1=on) (default=1)"); + + +#define COUGAR_KEYB_INTFNO 0 +#define COUGAR_MOUSE_INTFNO 1 +#define COUGAR_RESERVED_INTFNO 2 + +#define COUGAR_RESERVED_FLD_CODE 1 +#define COUGAR_RESERVED_FLD_ACTION 2 + +#define COUGAR_KEY_G1 0x83 +#define COUGAR_KEY_G2 0x84 +#define COUGAR_KEY_G3 0x85 +#define COUGAR_KEY_G4 0x86 +#define COUGAR_KEY_G5 0x87 +#define COUGAR_KEY_G6 0x78 +#define COUGAR_KEY_FN 0x0d +#define COUGAR_KEY_MR 0x6f +#define COUGAR_KEY_M1 0x80 +#define COUGAR_KEY_M2 0x81 +#define COUGAR_KEY_M3 0x82 +#define COUGAR_KEY_LEDS 0x67 +#define COUGAR_KEY_LOCK 0x6e + + +/* Default key mappings */ +static unsigned char cougar_mapping[][2] = { + { COUGAR_KEY_G1, KEY_F13 }, + { COUGAR_KEY_G2, KEY_F14 }, + { COUGAR_KEY_G3, KEY_F15 }, + { COUGAR_KEY_G4, KEY_F16 }, + { COUGAR_KEY_G5, KEY_F17 }, + { COUGAR_KEY_G6, KEY_F18 }, // This is handled individually + { COUGAR_KEY_LOCK, KEY_SCREENLOCK }, + { 0, 0 }, +}; + +struct cougar_kbd_data { + struct hid_device *owner; + struct input_dev *input; +}; + +/* + * Constant-friendly rdesc fixup for mouse interface + */ +static __u8 *cougar_report_fixup(struct hid_device *hdev, __u8 *rdesc, + unsigned int *rsize) +{ + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + + if (intf->cur_altsetting->desc.bInterfaceNumber == COUGAR_MOUSE_INTFNO && + (rdesc[0x73] | rdesc[0x74] << 8) >= HID_MAX_USAGES) { + hid_info(hdev, "usage count exceeds max: fixing up report descriptor"); + rdesc[0x73] = ((HID_MAX_USAGES-1) & 0xff); + rdesc[0x74] = ((HID_MAX_USAGES-1) >> 8); + } + return rdesc; +} + +/* + * Returns a sibling hid_device with the given intf number + */ +static struct hid_device *cougar_get_sibling_hid_device(struct hid_device *hdev, + const __u8 intfno) +{ + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + struct usb_device *device; + struct usb_host_config *hostcfg; + struct hid_device *siblingHdev; + int i; + + if (intf->cur_altsetting->desc.bInterfaceNumber == intfno) + hid_err(hdev, "returning hid device's data from myself?"); + + device = interface_to_usbdev(intf); + hostcfg = device->config; + siblingHdev = NULL; + for (i = 0; i < hostcfg->desc.bNumInterfaces; i++) { + if (i == intfno) + return usb_get_intfdata(hostcfg->interface[i]); + } + return NULL; +} + +static int cougar_set_drvdata_from_keyb_interface(struct hid_device *hdev) +{ + struct hid_device *sibling; + struct cougar_kbd_data *kbd; + + /* Search for the keyboard */ + sibling = cougar_get_sibling_hid_device(hdev, COUGAR_KEYB_INTFNO); + if (sibling == NULL) { + hid_err(hdev, + "no sibling hid device found for intf %d", COUGAR_KEYB_INTFNO); + return -ENODEV; + } + + kbd = hid_get_drvdata(sibling); + if (kbd == NULL || kbd->input == NULL) { + hid_err(hdev, + "keyboard descriptor not found in intf %d", COUGAR_KEYB_INTFNO); + return -ENODATA; + } + + /* And save its data on reserved, custom vendor intf. device */ + hid_set_drvdata(hdev, kbd); + return 0; +} + +/* + * The probe will save the keyb's input device, so that the + * vendor intf will be able to send keys as if it were the + * keyboard itself. + */ +static int cougar_probe(struct hid_device *hdev, + const struct hid_device_id *id) +{ + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + struct cougar_kbd_data *kbd = NULL; + unsigned int mask; + int ret; + __u8 intfno; + + intfno = intf->cur_altsetting->desc.bInterfaceNumber; + hid_dbg(hdev, "about to probe intf %d", intfno); + + if (intfno == COUGAR_KEYB_INTFNO) { + kbd = devm_kzalloc(&hdev->dev, sizeof(*kbd), GFP_KERNEL); + if (kbd == NULL) + return -ENOMEM; + + hid_dbg(hdev, "attaching kbd data to intf %d", intfno); + hid_set_drvdata(hdev, kbd); + } + + /* Parse and start hw */ + ret = hid_parse(hdev); + if (ret) + goto err_cleanup; + + /* Reserved, custom vendor interface connects hidraw only */ + mask = intfno == COUGAR_RESERVED_INTFNO ? + HID_CONNECT_HIDRAW : HID_CONNECT_DEFAULT; + ret = hid_hw_start(hdev, mask); + if (ret) + goto err_cleanup; + + if (intfno == COUGAR_RESERVED_INTFNO) { + ret = cougar_set_drvdata_from_keyb_interface(hdev); + if (ret) + goto err_stop_and_cleanup; + + hid_dbg(hdev, "keyboard descriptor attached to intf %d", intfno); + + ret = hid_hw_open(hdev); + if (ret) + goto err_stop_and_cleanup; + } + hid_dbg(hdev, "intf %d probed successfully", intfno); + + return 0; + +err_stop_and_cleanup: + hid_hw_stop(hdev); +err_cleanup: + hid_set_drvdata(hdev, NULL); + if (kbd != NULL && kbd->owner == hdev) + devm_kfree(&hdev->dev, kbd); + return ret; +} + +/* + * Keeps track of the keyboard's hid_input + */ +static int cougar_input_configured(struct hid_device *hdev, struct hid_input *hidinput) +{ + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + __u8 intfno = intf->cur_altsetting->desc.bInterfaceNumber; + struct cougar_kbd_data *kbd; + + if (intfno != COUGAR_KEYB_INTFNO) { + /* Skip interfaces other than COUGAR_KEYB_INTFNO, + * which is the one containing the configured hidinput + */ + hid_dbg(hdev, "input_configured: skipping intf %d", intfno); + return 0; + } + kbd = hid_get_drvdata(hdev); + kbd->owner = hdev; + kbd->input = hidinput->input; + hid_dbg(hdev, "input_configured: intf %d configured", intfno); + return 0; +} + +/* + * Converts events from vendor intf to input key events + */ +static int cougar_raw_event(struct hid_device *hdev, struct hid_report *report, + u8 *data, int size) +{ + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + struct cougar_kbd_data *kbd; + unsigned char action, code, transcode; + int i; + + if (intf->cur_altsetting->desc.bInterfaceNumber != COUGAR_RESERVED_INTFNO) + return 0; + + // Enable this to see a dump of the data received from reserved interface + //print_hex_dump(KERN_ERR, DRIVER_NAME " data : ", DUMP_PREFIX_OFFSET, 8, 1, data, size, 0); + + code = data[COUGAR_RESERVED_FLD_CODE]; + switch (code) { + case COUGAR_KEY_FN: + hid_dbg(hdev, "FN (special function) key is handled by the hardware itself"); + break; + case COUGAR_KEY_MR: + hid_dbg(hdev, "MR (macro recording) key is handled by the hardware itself"); + break; + case COUGAR_KEY_M1: + hid_dbg(hdev, "M1 (macro set 1) key is handled by the hardware itself"); + break; + case COUGAR_KEY_M2: + hid_dbg(hdev, "M2 (macro set 2) key is handled by the hardware itself"); + break; + case COUGAR_KEY_M3: + hid_dbg(hdev, "M3 (macro set 3) key is handled by the hardware itself"); + break; + case COUGAR_KEY_LEDS: + hid_dbg(hdev, "LED (led set) key is handled by the hardware itself"); + break; + default: + /* Try normal key mappings */ + for (i = 0; cougar_mapping[i][0]; i++) { + if (cougar_mapping[i][0] == code) { + action = data[COUGAR_RESERVED_FLD_ACTION]; + hid_dbg(hdev, "found mapping for code %x", code); + if (code == COUGAR_KEY_G6 && cougar_g6_is_space) + transcode = KEY_SPACE; + else + transcode = cougar_mapping[i][1]; + + kbd = hid_get_drvdata(hdev); + input_event(kbd->input, EV_KEY, transcode, action); + input_sync(kbd->input); + hid_dbg(hdev, "translated to %x", transcode); + return 0; + } + } + hid_warn(hdev, "unmapped key code %d: ignoring", code); + } + return 0; +} + +static void cougar_remove(struct hid_device *hdev) +{ + struct usb_interface *intf = to_usb_interface(hdev->dev.parent); + struct cougar_kbd_data *kbd = hid_get_drvdata(hdev); + + hid_dbg(hdev, "removing %d", intf->cur_altsetting->desc.bInterfaceNumber); + if (intf->cur_altsetting->desc.bInterfaceNumber == COUGAR_RESERVED_INTFNO) + hid_hw_close(hdev); + + hid_hw_stop(hdev); + hid_set_drvdata(hdev, NULL); + if (kbd != NULL && kbd->owner == hdev) + devm_kfree(&hdev->dev, kbd); +} + +static struct hid_device_id cougar_id_table[] = { + { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD) }, + {} +}; +MODULE_DEVICE_TABLE(hid, cougar_id_table); + +static struct hid_driver cougar_driver = { + .name = "cougar", + .id_table = cougar_id_table, + .report_fixup = cougar_report_fixup, + .probe = cougar_probe, + .input_configured = cougar_input_configured, + .remove = cougar_remove, + .raw_event = cougar_raw_event, +}; + +module_hid_driver(cougar_driver); diff -uprN -X hid-vanilla/Documentation/dontdiff hid-vanilla/drivers/hid/hid-ids.h hid/drivers/hid/hid-ids.h --- hid-vanilla/drivers/hid/hid-ids.h 2018-07-09 17:48:30.189316299 +0100 +++ hid/drivers/hid/hid-ids.h 2018-07-09 17:54:29.332832182 +0100 @@ -1001,6 +1001,9 @@ #define USB_VENDOR_ID_SINO_LITE 0x1345 #define USB_DEVICE_ID_SINO_LITE_CONTROLLER 0x3008 +#define USB_VENDOR_ID_SOLID_YEAR 0x060b +#define USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD 0x500a + #define USB_VENDOR_ID_SOUNDGRAPH 0x15c2 #define USB_DEVICE_ID_SOUNDGRAPH_IMON_FIRST 0x0034 #define USB_DEVICE_ID_SOUNDGRAPH_IMON_LAST 0x0046 diff -uprN -X hid-vanilla/Documentation/dontdiff hid-vanilla/drivers/hid/hid-quirks.c hid/drivers/hid/hid-quirks.c --- hid-vanilla/drivers/hid/hid-quirks.c 2018-07-09 17:48:30.193316294 +0100 +++ hid/drivers/hid/hid-quirks.c 2018-07-09 17:54:42.708814231 +0100 @@ -610,6 +610,9 @@ static const struct hid_device_id hid_ha { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP_LTD, USB_DEVICE_ID_SUPER_DUAL_BOX_PRO) }, { HID_USB_DEVICE(USB_VENDOR_ID_WISEGROUP_LTD, USB_DEVICE_ID_SUPER_JOY_BOX_5_PRO) }, #endif +#if IS_ENABLED(CONFIG_HID_COUGAR) + { HID_USB_DEVICE(USB_VENDOR_ID_SOLID_YEAR, USB_DEVICE_ID_COUGAR_500K_GAMING_KEYBOARD) }, +#endif #if IS_ENABLED(CONFIG_HID_SONY) { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_HARMONY_PS3) }, { HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_SMK, USB_DEVICE_ID_SMK_PS3_BDREMOTE) }, diff -uprN -X hid-vanilla/Documentation/dontdiff hid-vanilla/drivers/hid/Kconfig hid/drivers/hid/Kconfig --- hid-vanilla/drivers/hid/Kconfig 2018-07-09 17:48:30.185316305 +0100 +++ hid/drivers/hid/Kconfig 2018-07-09 18:46:10.075657150 +0100 @@ -207,6 +207,16 @@ config HID_CORSAIR - Vengeance K90 - Scimitar PRO RGB +config HID_COUGAR + tristate "Cougar devices" + depends on HID + help + Support for Cougar devices that are not fully compliant with the + HID standard. + + Supported devices: + - Cougar 500k Gaming Keyboard + config HID_PRODIKEYS tristate "Prodikeys PC-MIDI Keyboard support" depends on HID && SND diff -uprN -X hid-vanilla/Documentation/dontdiff hid-vanilla/drivers/hid/Makefile hid/drivers/hid/Makefile --- hid-vanilla/drivers/hid/Makefile 2018-07-09 17:48:30.185316305 +0100 +++ hid/drivers/hid/Makefile 2018-07-09 17:54:15.140851231 +0100 @@ -35,6 +35,7 @@ obj-$(CONFIG_HID_CHERRY) += hid-cherry.o obj-$(CONFIG_HID_CHICONY) += hid-chicony.o obj-$(CONFIG_HID_CMEDIA) += hid-cmedia.o obj-$(CONFIG_HID_CORSAIR) += hid-corsair.o +obj-$(CONFIG_HID_COUGAR) += hid-cougar.o obj-$(CONFIG_HID_CP2112) += hid-cp2112.o obj-$(CONFIG_HID_CYPRESS) += hid-cypress.o obj-$(CONFIG_HID_DRAGONRISE) += hid-dr.o
Cougar 500k Gaming Keyboard have some special function keys that make the keyboard stop responding when pressed. Implement the custom vendor interface that deals with the extended keypresses to fix. The bug can be reproduced by plugging in the keyboard, then pressing the rightmost part of the spacebar. Signed-off-by: Daniel M. Lambea <dmlambea@gmail.com> --- One of those special function keys is just the right-half part of the spacebar, so the chance of hitting it is very high. This is very annoying to the user, since the only way of recovering the device back is to unplug it and to plug it back. The code, built as a DKMS module, has been released and tested by others. For more information, please refer to the bug: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1511511 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html