> -----Original Message----- > From: Dan Carpenter [mailto:dan.carpenter@xxxxxxxxxx] > Sent: Monday, September 16, 2013 1:21 AM > To: KY Srinivasan > Cc: gregkh@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; linux-input@xxxxxxxxxxxxxxx; > dmitry.torokhov@xxxxxxxxx; vojtech@xxxxxxx; olaf@xxxxxxxxx; > apw@xxxxxxxxxxxxx; jasowang@xxxxxxxxxx > Subject: Re: [PATCH 1/1] Drivers: input: serio: New driver to support Hyper-V > synthetic keyboard > > The main thing is that could you improve the error handling in > hv_kbd_on_channel_callback() explained inline. Thanks Dan. I will address all the relevant issues you have pointed out. I have answered/responded to your comments in-line. > > 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? Yes. > > > + } > > +} > > + > > +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. What user are you referring to. The message is sent by the host - the user keystroke is normalized into a fixed size packet by the host and sent to the guest. We will parse this packet, based on the host specified layout here. > > > + 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 Regards, K. Y -- 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
