| 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: <20101220182934.GA23715@kroah.com>
Date: Mon, 20 Dec 2010 10:29:34 -0800
From: Greg KH <greg@...ah.com>
To: Melchior FRANZ <melchior.franz@...il.com>
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] USB: add support for Dream Cheeky DL100B Webmail
Notifier (1d34:0004)
On Sun, Dec 19, 2010 at 11:10:12PM +0100, Melchior FRANZ wrote:
> From: Melchior FRANZ <mfranz@....at>
>
> Add support for Dream Cheeky DL100B Webmail Notifier (1d34:0004) as
> usbled device. Lets hid-core ignore the device's HID-ness.
Nice idea, but a few comments below as to how to make this a bit
"cleaner"
>
> Signed-off-by: Melchior FRANZ <mfranz@....at>
> ---
>
> So far the USBLED driver only supports Delcom's "USB Visual Signal
> Indicator" (http://www.delcomproducts.com/products_USBLMP.asp). The
> driver generates virtual files "red", "blue", and "green" under the
> device's /sys/ directory, where color values can be read and written to.
>
> This patch adds support for Dream Cheeky's "DL100B Webmail Notifier"
> (http://www.dreamcheeky.com/webmail-notifier -- available from several
> shops, such as http://www.conrad.at/ce/de/product/777048/USB-WEBMAIL).
> This device isn't as pretty as Delcom's, but it's *far* cheaper, and
> its 3 LEDs can be set in 32 brightness steps each. The grey envelope
> contour can easily be removed, leaving a rather neutral white box (with
> a few small holes), which is useful for generic signalling purposes.
> Of course, the small circuit board can easily be put into a prettier
> case. The price difference to Delcom's thingy lets you a lot of room. :-)
>
> The DL100B device pretends to be a HID, but the HID descriptor shows
> that it's not overly useful as such (see below). The patch therefore
> removes the "HID-ness" (hid-core.c, hid-ids.h), and adds the necessary
> commands to usbled.c. The protocol comes from the developer's manual
> that Dream Cheeky kindly provided (815DeveloperManual.pdf).
>
> Please review and consider for inclusion. (The patch is diffed against
> 2.6.36.2.)
>
> m.
>
>
> HID descriptor:
>
> 0: 05 01 Usage Page 'Generic Desktop Controls'
> 2: 09 10 Usage 'Reserved'
> 4: a1 01 Collection 'Application (mouse, keyboard)'
> 6: 05 00 Usage Page 'Undefined'
> 8: 19 10 Usage Minimum = 16
> 10: 29 11 Usage Maximum = 17
> 12: 15 00 Logical Minimum = 0
> 14: 25 0f Logical Maximum = 15
> 16: 75 08 Report Size = 8
> 18: 95 08 Report Count = 8
> 20: 91 02 Output data *var abs lin pref-state null-pos non-vol bit-field
> 22: 19 10 Usage Minimum = 16
> 24: 29 11 Usage Maximum = 17
> 26: 15 00 Logical Minimum = 0
> 28: 25 0f Logical Maximum = 15
> 30: 75 08 Report Size = 8
> 32: 95 08 Report Count = 8
> 34: 81 00 Input data array abs lin pref-state null-pos non-vol bit-field
> 36: c0 End Collection
>
All of this information should be above the --- line so it would be
included in the changelog. Please do that next time.
> @@ -20,12 +20,13 @@
> #define DRIVER_AUTHOR "Greg Kroah-Hartman, greg@...ah.com"
> #define DRIVER_DESC "USB LED Driver"
>
> -#define VENDOR_ID 0x0fc5
> -#define PRODUCT_ID 0x1223
> +#define VENDOR_ID_DELCOM 0x0fc5
> +#define VENDOR_ID_DREAM_CHEEKY 0x1d34
>
> /* table of devices that work with this driver */
> static const struct usb_device_id id_table[] = {
> - { USB_DEVICE(VENDOR_ID, PRODUCT_ID) },
> + { USB_DEVICE(VENDOR_ID_DELCOM, 0x1223) },
> + { USB_DEVICE(VENDOR_ID_DREAM_CHEEKY, 0x0004) }, /* DL100B */
Why not put the "device type" in the id table? Then you don't have to
check the device vendor/product in the probe function and you would
automatically know the type for later on.
Also, make the type an enumerated type, not just a simple flag like you
did here:
> @@ -35,6 +36,7 @@ struct usb_led {
> unsigned char blue;
> unsigned char red;
> unsigned char green;
> + unsigned char dream_cheeky:1;
> };
>
> #define BLUE 0x04
Add a enum led_type to this structure that you copy out of the id table
when the device is probed. Make 0 be the old type so that dynamice
device ids can at least have a chance to work that way.
> @@ -43,7 +45,6 @@ struct usb_led {
> static void change_color(struct usb_led *led)
> {
> int retval;
> - unsigned char color = 0x07;
> unsigned char *buffer;
>
> buffer = kmalloc(8, GFP_KERNEL);
> @@ -52,25 +53,51 @@ static void change_color(struct usb_led *led)
> return;
> }
>
> - if (led->blue)
> - color &= ~(BLUE);
> - if (led->red)
> - color &= ~(RED);
> - if (led->green)
> - color &= ~(GREEN);
> - dev_dbg(&led->udev->dev,
> - "blue = %d, red = %d, green = %d, color = %.2x\n",
> - led->blue, led->red, led->green, color);
> -
> - retval = usb_control_msg(led->udev,
> - usb_sndctrlpipe(led->udev, 0),
> - 0x12,
> - 0xc8,
> - (0x02 * 0x100) + 0x0a,
> - (0x00 * 0x100) + color,
> - buffer,
> - 8,
> - 2000);
> + if (led->dream_cheeky) {
Then switch on the enumerated type here, instead of a if, making it
easier to add different types here in the future.
Care to make those changes up and resend?
thanks,
greg k-h
--
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