Hi Daniel, On Mon, Jul 9, 2018 at 8:05 PM Daniel M. Lambea <dmlambea@xxxxxxxxx> 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@xxxxxxxxx> > --- > > 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@xxxxxxxxx> > + */ > + > +/* > + * 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@xxxxxxxxx>"); > +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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
