The main thing is that could you improve the error handling in
hv_kbd_on_channel_callback() explained inline.
On Sun, Sep 15, 2013 at 10:28:54PM -0700, K. Y. Srinivasan wrote:
> Add a new driver to support synthetic keyboard. On the next generation
> Hyper-V guest firmware, many legacy devices will not be emulated and this
> driver will be required.
>
> I would like to thank Vojtech Pavlik <vojtech@xxxxxxx> for helping me with the
> details of the AT keyboard driver.
>
> Signed-off-by: K. Y. Srinivasan <kys@xxxxxxxxxxxxx>
> ---
> drivers/input/serio/Kconfig | 7 +
> drivers/input/serio/Makefile | 1 +
> drivers/input/serio/hyperv-keyboard.c | 379 +++++++++++++++++++++++++++++++++
> 3 files changed, 387 insertions(+), 0 deletions(-)
> create mode 100644 drivers/input/serio/hyperv-keyboard.c
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index 1e691a3..f3996e7 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -267,4 +267,11 @@ config SERIO_OLPC_APSP
> To compile this driver as a module, choose M here: the module will
> be called olpc_apsp.
>
> +config HYPERV_KEYBOARD
> + tristate "Microsoft Synthetic Keyboard driver"
> + depends on HYPERV
> + default HYPERV
> + help
> + Select this option to enable the Hyper-V Keyboard driver.
> +
> endif
> diff --git a/drivers/input/serio/Makefile b/drivers/input/serio/Makefile
> index 12298b1..815d874 100644
> --- a/drivers/input/serio/Makefile
> +++ b/drivers/input/serio/Makefile
> @@ -28,3 +28,4 @@ obj-$(CONFIG_SERIO_ALTERA_PS2) += altera_ps2.o
> obj-$(CONFIG_SERIO_ARC_PS2) += arc_ps2.o
> obj-$(CONFIG_SERIO_APBPS2) += apbps2.o
> obj-$(CONFIG_SERIO_OLPC_APSP) += olpc_apsp.o
> +obj-$(CONFIG_HYPERV_KEYBOARD) += hyperv-keyboard.o
> diff --git a/drivers/input/serio/hyperv-keyboard.c b/drivers/input/serio/hyperv-keyboard.c
> new file mode 100644
> index 0000000..0d4625f
> --- /dev/null
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -0,0 +1,379 @@
> +/*
> + * Copyright (c) 2013, Microsoft Corporation.
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + */
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/completion.h>
> +#include <linux/hyperv.h>
> +#include <linux/serio.h>
> +#include <linux/slab.h>
> +
> +
Extra blank line.
> +/*
> + * Current version 1.0
> + *
> + */
> +#define SYNTH_KBD_VERSION_MAJOR 1
> +#define SYNTH_KBD_VERSION_MINOR 0
> +#define SYNTH_KBD_VERSION (SYNTH_KBD_VERSION_MINOR | \
> + (SYNTH_KBD_VERSION_MAJOR << 16))
> +
> +
> +/*
> + * Message types in the synthetic input protocol
> + */
> +enum synth_kbd_msg_type {
> + SYNTH_KBD_PROTOCOL_REQUEST = 1,
> + SYNTH_KBD_PROTOCOL_RESPONSE = 2,
> + SYNTH_KBD_EVENT = 3,
> + SYNTH_KBD_LED_INDICATORS = 4,
> +};
> +
> +/*
> + * Basic message structures.
> + */
> +struct synth_kbd_msg_hdr {
> + enum synth_kbd_msg_type type;
> +};
Enum type is wrong here because sizeof(enum) is up to the compiler to
decide.
> +
> +struct synth_kbd_msg {
> + struct synth_kbd_msg_hdr header;
> + char data[1]; /* Enclosed message */
> +};
You could use a zero size array instead.
> +
> +union synth_kbd_version {
> + struct {
> + u16 minor_version;
> + u16 major_version;
> + };
> + u32 version;
> +};
> +
> +/*
> + * Protocol messages
> + */
> +struct synth_kbd_protocol_request {
> + struct synth_kbd_msg_hdr header;
> + union synth_kbd_version version_requested;
> +};
> +
> +struct synth_kbd_protocol_response {
> + struct synth_kbd_msg_hdr header;
> + u32 accepted:1;
> + u32 reserved:31;
> +};
> +
> +struct synth_kbd_keystroke {
> + struct synth_kbd_msg_hdr header;
> + u16 make_code;
> + u16 reserved0;
> + u32 is_unicode:1;
> + u32 is_break:1;
> + u32 is_e0:1;
> + u32 is_e1:1;
> + u32 reserved:28;
> +};
> +
> +
Extra blank line.
> +#define HK_MAXIMUM_MESSAGE_SIZE 256
> +
> +#define KBD_VSC_SEND_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +#define KBD_VSC_RECV_RING_BUFFER_SIZE (10 * PAGE_SIZE)
> +
> +#define XTKBD_EMUL0 0xe0
> +#define XTKBD_EMUL1 0xe1
> +#define XTKBD_RELEASE 0x80
> +
> +
Extra blank.
> +/*
> + * Represents a keyboard device
> + */
> +struct hv_kbd_dev {
> + unsigned char keycode[256];
> + struct hv_device *device;
> + struct synth_kbd_protocol_request protocol_req;
> + struct synth_kbd_protocol_response protocol_resp;
> + /* Synchronize the request/response if needed */
> + struct completion wait_event;
> + struct serio *hv_serio;
> +};
> +
> +
Extra blank.
> +static struct hv_kbd_dev *hv_kbd_alloc_device(struct hv_device *device)
> +{
> + struct hv_kbd_dev *kbd_dev;
> + struct serio *hv_serio;
> +
> + kbd_dev = kzalloc(sizeof(struct hv_kbd_dev), GFP_KERNEL);
> +
Spurious blank line.
> + if (!kbd_dev)
> + return NULL;
> +
> + hv_serio = kzalloc(sizeof(struct serio), GFP_KERNEL);
> +
Spurious blank.
> + if (hv_serio == NULL) {
> + kfree(kbd_dev);
> + return NULL;
> + }
> +
> + hv_serio->id.type = SERIO_8042_XL;
Pointless tab before the '='.
> + strlcpy(hv_serio->name, dev_name(&device->device),
> + sizeof(hv_serio->name));
> + strlcpy(hv_serio->phys, dev_name(&device->device),
> + sizeof(hv_serio->phys));
> + hv_serio->dev.parent = &device->device;
Why are there two spaces before the '='?
> +
> +
Extra blank line.
> + kbd_dev->device = device;
> + kbd_dev->hv_serio = hv_serio;
> + hv_set_drvdata(device, kbd_dev);
> + init_completion(&kbd_dev->wait_event);
> +
> + return kbd_dev;
> +}
> +
> +static void hv_kbd_free_device(struct hv_kbd_dev *device)
> +{
> + serio_unregister_port(device->hv_serio);
> + kfree(device->hv_serio);
> + hv_set_drvdata(device->device, NULL);
> + kfree(device);
> +}
> +
> +static void hv_kbd_on_receive(struct hv_device *device,
> + struct vmpacket_descriptor *packet)
> +{
> + struct synth_kbd_msg *msg;
> + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> + struct synth_kbd_keystroke *ks_msg;
> + u16 scan_code;
> +
> + msg = (struct synth_kbd_msg *)((unsigned long)packet +
> + (packet->offset8 << 3));
> +
> + switch (msg->header.type) {
> + case SYNTH_KBD_PROTOCOL_RESPONSE:
> + memcpy(&kbd_dev->protocol_resp, msg,
> + sizeof(struct synth_kbd_protocol_response));
> + complete(&kbd_dev->wait_event);
> + break;
> + case SYNTH_KBD_EVENT:
> + ks_msg = (struct synth_kbd_keystroke *)msg;
> + scan_code = ks_msg->make_code;
> +
> + /*
> + * Inject the information through the serio interrupt.
> + */
> + if (ks_msg->is_e0)
> + serio_interrupt(kbd_dev->hv_serio, XTKBD_EMUL0, 0);
> + serio_interrupt(kbd_dev->hv_serio,
> + scan_code | (ks_msg->is_break ?
> + XTKBD_RELEASE : 0),
> + 0);
> +
It would be more readable to do:
if (ks_msg->is_break)
scan_code |= XTKBD_RELEASE;
serio_interrupt(kbd_dev->hv_serio, scan_code, 0);
> + break;
> +
> + default:
> + pr_err("unhandled message type %d\n", msg->header.type);
Just a question. This can only be triggered by the hyperviser, right?
> + }
> +}
> +
> +static void hv_kbd_on_channel_callback(void *context)
> +{
> + const int packet_size = 0x100;
> + int ret;
> + struct hv_device *device = context;
> + u32 bytes_recvd;
> + u64 req_id;
> + struct vmpacket_descriptor *desc;
> + unsigned char *buffer;
> + int bufferlen = packet_size;
> +
> + buffer = kmalloc(bufferlen, GFP_ATOMIC);
> + if (!buffer)
> + return;
> +
> + do {
Forever loops should be while (1) { instead of do { } while (1).
> + ret = vmbus_recvpacket_raw(device->channel, buffer,
> + bufferlen, &bytes_recvd, &req_id);
> +
> + switch (ret) {
> + case 0:
> + if (bytes_recvd <= 0) {
There can never be a negative number of bytes_recvd.
> + kfree(buffer);
> + return;
> + }
> + desc = (struct vmpacket_descriptor *)buffer;
> +
> + switch (desc->type) {
> + case VM_PKT_COMP:
> + break;
> +
> + case VM_PKT_DATA_INBAND:
> + hv_kbd_on_receive(device, desc);
This is the error handling I mentioned at the top. hv_kbd_on_receive()
doesn't take into consideration the amount of data we recieved, it
trusts the offset we recieved from the user. There is an out of bounds
read.
> + break;
> +
> + default:
> + pr_err("unhandled packet type %d, tid %llx len %d\n",
> + desc->type, req_id, bytes_recvd);
> + break;
> + }
> +
> + break;
> +
> + case -ENOBUFS:
> + kfree(buffer);
> + /* Handle large packet */
> + bufferlen = bytes_recvd;
> + buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> +
> + if (!buffer)
> + return;
> +
> + break;
> + }
> + } while (1);
> +
> +}
> +
> +static int hv_kbd_connect_to_vsp(struct hv_device *device)
> +{
> + int ret = 0;
Don't initialize this.
> + int t;
> + struct hv_kbd_dev *kbd_dev = hv_get_drvdata(device);
> + struct synth_kbd_protocol_request *request;
> + struct synth_kbd_protocol_response *response;
> +
> + request = &kbd_dev->protocol_req;
> + memset(request, 0, sizeof(struct synth_kbd_protocol_request));
> +
> + request->header.type = SYNTH_KBD_PROTOCOL_REQUEST;
> + request->version_requested.version = SYNTH_KBD_VERSION;
> +
> + ret = vmbus_sendpacket(device->channel, request,
> + sizeof(struct synth_kbd_protocol_request),
> + (unsigned long)request,
> + VM_PKT_DATA_INBAND,
> + VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED);
> + if (ret)
> + goto cleanup;
There is no cleanup. Just return directly.
> +
> + t = wait_for_completion_timeout(&kbd_dev->wait_event, 10 * HZ);
> + if (!t) {
> + ret = -ETIMEDOUT;
> + goto cleanup;
return -ETIMEOUT;
> + }
> +
> + response = &kbd_dev->protocol_resp;
> +
> + if (!response->accepted) {
> + pr_err("synth_kbd protocol request failed (version %d)\n",
> + SYNTH_KBD_VERSION);
> + ret = -ENODEV;
> + goto cleanup;
Just return -ENODEV;
> + }
> +
return 0;
> +
> +cleanup:
> + return ret;
> +}
> +
> +static int hv_kbd_probe(struct hv_device *device,
> + const struct hv_vmbus_device_id *dev_id)
> +{
> + int ret;
> + struct hv_kbd_dev *kbd_dev;
> +
> + kbd_dev = hv_kbd_alloc_device(device);
> +
Delete the blank line.
> + if (!kbd_dev)
> + return -ENOMEM;
> +
> + ret = vmbus_open(device->channel,
> + KBD_VSC_SEND_RING_BUFFER_SIZE,
> + KBD_VSC_RECV_RING_BUFFER_SIZE,
> + NULL,
> + 0,
> + hv_kbd_on_channel_callback,
> + device
> + );
> +
Delete the blank line.
> + if (ret)
> + goto probe_err0;
> +
> + ret = hv_kbd_connect_to_vsp(device);
> +
Delete the blank line.
> + if (ret)
> + goto probe_err1;
> +
> + serio_register_port(kbd_dev->hv_serio);
> +
> + return ret;
return 0;
> +
> +probe_err1:
> + vmbus_close(device->channel);
The label here should be "err_close:". The number is more GW-Basic
style than C.
> +
> +probe_err0:
The label should be "err_free_dev:".
> + hv_kbd_free_device(kbd_dev);
> +
> + return ret;
> +}
> +
> +
Extra blank line.
> +static int hv_kbd_remove(struct hv_device *dev)
regards,
dan carpenter
--
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
