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
| ||
|
Date: Tue, 4 Sep 2007 23:04:50 -0400 From: Dmitry Torokhov <dtor@...ightbb.com> To: Adrian McMenamin <lkmladrian@...il.com> Cc: Paul Mundt <lethal@...ux-sh.org>, linuxsh-dev@...ts.sourceforge.net, linux-kernel@...r.kernel.org Subject: Re: [PATCH] Add support for keyboard on SEGA Dreamcast Hi Adrian, On Tuesday 04 September 2007 19:34, Adrian McMenamin wrote: > This patch will add support for the Dreamcast keyboard when used > alongside the maple bus patch (http://lkml.org/lkml/2007/9/4/165) and > the pvr2 patch. > > Signed off by: Adrian McMenamin <adrian@...en.demon.co.uk> > Thnank you very much for your patch. Couple of comments: > + > +static unsigned char dc_kbd_keycode[256] = { Const? > + > +static int dc_kbd_connect(struct maple_device *dev) > +{ > + int i, retval; > + unsigned long data = be32_to_cpu(dev->devinfo.function_data[0]); > + struct dc_kbd *kbd; > + if (dev->function != MAPLE_FUNC_KEYBOARD) > + return -EINVAL; > + > + kbd = kzalloc(sizeof(struct dc_kbd), GFP_KERNEL); > + if (unlikely(!kbd)) > + return -ENOMEM; > + > + dev->private_data = kbd; > + kbd->dev = input_allocate_device(); > + if (unlikely(!kbd->dev)) > + goto cleanup; > + > + kbd->dev->evbit[0] = BIT(EV_KEY) | BIT(EV_REP); > + > + for (i = 0; i < 255; i++) > + set_bit(dc_kbd_keycode[i], kbd->dev->keybit); > + > + clear_bit(0, kbd->dev->keybit); > + > + kbd->dev->private = kbd; > + > + kbd->dev->name = dev->product_name; > + kbd->dev->id.bustype = BUS_MAPLE; Do we really need a new bus type? Would not BUS_HOST suffice? > + > + retval = input_register_device(kbd->dev); > + if (unlikely(retval)) > + goto cleanup; > + > + maple_getcond_callback(dev, dc_kbd_callback, (25 * HZ) / 1000, > + MAPLE_FUNC_KEYBOARD); > + > + printk(KERN_INFO "input: keyboard(0x%lx): %s\n", data, > + kbd->dev->name); Input core already prints a line when a new input device is registered, do we need another one? > + > + return 0; > + > + cleanup: > + kfree(kbd); > + return -EINVAL; It loks like call to input_free_device() is missing here. You would leak memory if input_register_device would fail. I would also get rid of all likelys/unlikelys here - driver registration is not a hot path... > +} > + > +static void dc_kbd_disconnect(struct maple_device *dev) > +{ > + struct dc_kbd *kbd = dev->private_data; > + > + input_unregister_device(kbd->dev); > + kfree(kbd); Are we guaranteed that the dc_kbd_callback is not running in a separate thread? Please also consider implementing support for changing keyma. Since the keymap is pretty full I think the best way is to copy the vanilla keymap into a per-device memory and set up keymap, keycodesize and keycodemax in input device structure. Thank you. -- Dmitry - 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