| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <55E9B628.6030404@gmail.com>
Date: Fri, 4 Sep 2015 17:18:00 +0200
From: Clément Vuchener <clement.vuchener@...il.com>
To: Jiri Kosina <jikos@...nel.org>
Cc: linux-api@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-input@...r.kernel.org
Subject: Re: [PATCH 1/1] Corsair Vengeance K90 driver
On 09/04/2015 03:27 PM, Jiri Kosina wrote:
> On Sat, 29 Aug 2015, Clément Vuchener wrote:
>
>
> This is missing changelog and Signed-off-by: line. I know that you have
> provided that in your cover letter, but cover letter never make it to the
> actual GIT commits.
>
> So please fix that and resend the patch, thanks.
>
>> ---
>> Documentation/ABI/testing/sysfs-class-k90_profile | 55 ++
>> .../ABI/testing/sysfs-driver-hid-corsair-k90 | 15 +
>> drivers/hid/Kconfig | 10 +
>> drivers/hid/Makefile | 1 +
>> drivers/hid/hid-core.c | 1 +
>> drivers/hid/hid-corsair-k90.c | 690
>> +++++++++++++++++++++
>> drivers/hid/hid-ids.h | 3 +
>> 7 files changed, 775 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
>> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
>> create mode 100644 drivers/hid/hid-corsair-k90.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile
>> b/Documentation/ABI/testing/sysfs-class-k90_profile
>> new file mode 100644
>> index 0000000..3275c16
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-k90_profile
>> @@ -0,0 +1,55 @@
>> +What: /sys/class/k90_profile/<profile>/profile_number
>> +Date: August 2015
>> +KernelVersion: 4.2
>> +Contact: Clement Vuchener <clement.vuchener@...il.com>
>> +Description: Get the number of the profile.
>> +
>> +What: /sys/class/k90_profile/<profile>/bindings
>> +Date: August 2015
>> +KernelVersion: 4.2
>> +Contact: Clement Vuchener <clement.vuchener@...il.com>
>> +Description: Write bindings to the keyboard on-board profile.
>> + The data structure is:
>> + - number of bindings in this structure (1 byte)
>> + - size of this data structure (2 bytes big endian)
>> + - size of the macro data written to
>> + /sys/class/k90_profile/<profile>/data (2 bytes big endian)
>> + - array of bindings referencing the data from
>> + /sys/class/k90_profile/<profile>/data, each containing:
>> + * 0x10 for a key usage, 0x20 for a macro (1 byte)
>> + * offset of the key usage/macro data (2 bytes big endian)
>> + * length of the key usage/macro data (2 bytes big endian)
>> +
> This looks like violation of one-value-per-attribute rule for sysfs ABI.
> Could you please think about it once again with this aspect on mind?
Per key attributes would be nice but I don't think I can do that. The profile must be written all at once and I don't know any way to read it from the hardware (the windows driver I studied does not do it). So writing one value only would erase all the others.
I think I will remove this part from the driver. The same thing can easily be done in user space through libusb and writing profile should be exceptional enough that it is not a problem to detach the driver while doing it. That part of the driver is not really useful with the current ABI.
>
> [ ... snip ... ]
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index e6fce23..f0d9125 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1807,6 +1807,7 @@ static const struct hid_device_id
>> hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS2) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS,
>> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>> USB_DEVICE_ID_CYPRESS_BARCODE_1) },
> Your mail client is corrupting long lines, making tha patch application
> impossible. Please fix that for your following submissions.
>
>> diff --git a/drivers/hid/hid-corsair-k90.c b/drivers/hid/hid-corsair-k90.c
>> new file mode 100644
>> index 0000000..67c1095
>> --- /dev/null
>> +++ b/drivers/hid/hid-corsair-k90.c
>> @@ -0,0 +1,690 @@
>> +/*
>> + * HID driver for Corsair Vengeance K90 Keyboard
> Usually we try to be a little bit more generic, and name the driver
> according to the vendor (and fold all the vedor-specific stuff into the
> one driver).
>
> So my suggestion would be to name the driver hid-corsair, keeping the
> possibility for adding more devices later open.
>
> [ ... snip ... ]
>> +static int k90_init_special_functions(struct hid_device *dev)
>> +{
>> + int ret, i;
>> + struct usb_interface *usbif = to_usb_interface(dev->dev.parent);
>> + struct usb_device *usbdev = interface_to_usbdev(usbif);
>> + char data[8];
>> + struct k90_drvdata *drvdata =
>> + kzalloc(sizeof(struct k90_drvdata), GFP_KERNEL);
>> + size_t name_sz;
>> + char *name;
>> + struct k90_led *led;
>> +
>> + if (!drvdata) {
>> + ret = -ENOMEM;
>> + goto fail_drvdata;
>> + }
>> + hid_set_drvdata(dev, drvdata);
>> +
>> + /* Get current status */
>> + ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
>> + K90_REQUEST_STATUS,
>> + USB_DIR_IN | USB_TYPE_VENDOR |
>> + USB_RECIP_DEVICE, 0, 0, data, 8,
>> + USB_CTRL_SET_TIMEOUT);
> So you apparently also depend on USB ...
>
>> + if (ret < 0) {
>> + hid_warn(dev, "Failed to get K90 initial state (error %d).\n",
>> + ret);
>> + drvdata->backlight.brightness = 0;
>> + drvdata->current_profile = 1;
>> + } else {
>> + drvdata->backlight.brightness = data[4];
>> + drvdata->current_profile = data[7];
>> + }
>> + /* Get current mode */
>> + ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
>> + K90_REQUEST_GET_MODE,
>> + USB_DIR_IN | USB_TYPE_VENDOR |
>> + USB_RECIP_DEVICE, 0, 0, data, 2,
>> + USB_CTRL_SET_TIMEOUT);
>> + if (ret < 0)
>> + hid_warn(dev, "Failed to get K90 initial mode (error %d).\n",
>> + ret);
>> + else {
>> + switch (data[0]) {
>> + case K90_MACRO_MODE_HW:
>> + drvdata->macro_mode = 1;
>> + break;
>> + case K90_MACRO_MODE_SW:
>> + drvdata->macro_mode = 0;
>> + break;
>> + default:
>> + hid_warn(dev, "K90 in unknown mode: %02x.\n",
>> + data[0]);
>> + }
>> + }
>> +
>> + /* Init LED device for backlight */
>> + name_sz =
>> + strlen(dev_name(&dev->dev)) + sizeof(K90_BACKLIGHT_LED_SUFFIX);
>> + name = devm_kzalloc(&dev->dev, name_sz, GFP_KERNEL);
>> + if (!name) {
>> + ret = -ENOMEM;
>> + goto fail_backlight;
>> + }
>> + snprintf(name, name_sz, "%s" K90_BACKLIGHT_LED_SUFFIX,
>> + dev_name(&dev->dev));
>> + led = &drvdata->backlight;
>> + led->cdev.name = name;
>> + led->cdev.max_brightness = 3;
>> + led->cdev.brightness_set = k90_brightness_set;
>> + led->cdev.brightness_get = k90_brightness_get;
>> + INIT_WORK(&led->work, k90_backlight_work);
>> + ret = led_classdev_register(&dev->dev, &led->cdev);
> ... and also on LED subsystem, but this dependency is not expressed in
> Kconfig.
>
> Thanks,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists