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@xxxxxxxxxx> wrote: > 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
