linux-kernel - Re: [PATCH] Add support for keyboard on SEGA Dreamcast

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <200709042304.51510.dtor@insightbb.com>
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/