> -----Original Message----- > From: Dmitry Torokhov [mailto:dmitry.torokhov@xxxxxxxxx] > Sent: Saturday, November 05, 2011 2:48 AM > To: KY Srinivasan > Cc: gregkh@xxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devel@xxxxxxxxxxxxxxxxxxxxxx; virtualization@xxxxxxxxxxxxxx; ohering@xxxxxxxx; > joe@xxxxxxxxxxx; jkosina@xxxxxxx > Subject: Re: [PATCH 1/1] Staging: hv: Move the mouse driver out of staging > > Hi KY, Dimitry, Let me begin by thanking you for taking the time to review. I have incorporated pretty much all your suggestions. These changes have cleaned up the code considerably. I will send the patch out that incorporates these changes shortly. I will also send out the next version of the patch to move the mouse driver out of staging. More details in-line. > Can we call it mousevsc_alloc_device? Done. > > > +{ > > + struct mousevsc_dev *input_dev; > > + > > + input_dev = kzalloc(sizeof(struct mousevsc_dev), GFP_KERNEL); > > + > > + if (!input_dev) > > + return NULL; > > + > > + input_dev->device = device; > > + hv_set_drvdata(device, input_dev); > > + init_completion(&input_dev->wait_event); > > + > > + return input_dev; > > +} > > + > > +static void free_input_device(struct mousevsc_dev *device) > > Can we call it mousevsc_free_device? Done. > This could probably be: > > static const mousevsc_prt_msg ack = { > .type = PIPE_MESSAGE_DATA, > .size = sizeof(struct synthhid_device_info_ack), > .ack = { > .header = { > .type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK, > .size = 1, > }, > .reserved = 0, > }, > }; > Done. > > > + > > + /* Assume success for now */ > > + input_device->dev_info_status = 0; > > + > > + memcpy(&input_device->hid_dev_info, &device_info->hid_dev_info, > > + sizeof(struct hv_input_dev_info)); > > + > > or simply: > > input_device->hid_dev_info = device_info->hid_dev_info; Done. > > > + desc = &device_info->hid_descriptor; > > + WARN_ON(desc->bLength == 0); > > Should also goto cleanup as such descriptor is not usable. Done. > > > + > > + input_device->hid_desc = kzalloc(desc->bLength, GFP_ATOMIC); > > + > > + if (!input_device->hid_desc) > > if you do > > input_device->dev_info_status = -ENOMEM; > > you could use it later instead of defaulting to -ENOMEM. > > > + goto cleanup; > > + > > + memcpy(input_device->hid_desc, desc, desc->bLength); > > + > > + input_device->report_desc_size = desc->desc[0].wDescriptorLength; > > + if (input_device->report_desc_size == 0) > > input_device->dev_info_status = -EINVAL; > > > + goto cleanup; > > + input_device->report_desc = kzalloc(input_device->report_desc_size, > > + GFP_ATOMIC); > > + > > + if (!input_device->report_desc) > > input_device->dev_info_status = -ENOMEM; > > > + goto cleanup; > > + > > + memcpy(input_device->report_desc, > > + ((unsigned char *)desc) + desc->bLength, > > + desc->desc[0].wDescriptorLength); > > + > > + /* Send the ack */ > > + memset(&ack, 0, sizeof(struct mousevsc_prt_msg)); > > + > > + ack.type = PIPE_MESSAGE_DATA; > > + ack.size = sizeof(struct synthhid_device_info_ack); > > + > > + ack.ack.header.type = SYNTH_HID_INITIAL_DEVICE_INFO_ACK; > > + ack.ack.header.size = 1; > > + ack.ack.reserved = 0; > > + > > + ret = vmbus_sendpacket(input_device->device->channel, > > + &ack, > > + sizeof(struct pipe_prt_msg) - sizeof(unsigned char) + > > + sizeof(struct synthhid_device_info_ack), > > + (unsigned long)&ack, > > + VM_PKT_DATA_INBAND, > > + > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > > + if (ret != 0) > > input_device->dev_info_status = ret; > > > + goto cleanup; > > + > > + complete(&input_device->wait_event); > > + > > + return; > > + > > +cleanup: > > + kfree(input_device->hid_desc); > > + input_device->hid_desc = NULL; > > + > > + kfree(input_device->report_desc); > > + input_device->report_desc = NULL; > > Do you have to clean it up here? You still need to clean it up in main > unwind path so consolidate both error and success path and just signal > completion and let main code figure it all out. Done. > > + > > +static void mousevsc_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 { > > + ret = vmbus_recvpacket_raw(device->channel, buffer, > > + bufferlen, &bytes_recvd, &req_id); > > + > > + switch (ret) { > > + case 0: > > + if (bytes_recvd <= 0) { > > + kfree(buffer); > > + return; > > + } > > + desc = (struct vmpacket_descriptor *)buffer; > > + > > + switch (desc->type) { > > + case VM_PKT_COMP: > > + break; > > + > > + case VM_PKT_DATA_INBAND: > > + mousevsc_on_receive(device, desc); > > + 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); > > Instead of potentially ever-increasing buffer that you also allocate > (and it looks like leaking on every callback invocation) can you just > repeat the read if you know that there are more data and use single > pre-allocated buffer? The ring-buffer protocol is such that we need to consume the full message. Also, why do you say we are leaking memory? > > > + > > + if (!buffer) > > + return; > > + > > + break; > > + } > > + } while (1); > > + > > +} > > + > > +static int mousevsc_connect_to_vsp(struct hv_device *device) > > +{ > > + int ret = 0; > > + int t; > > + struct mousevsc_dev *input_dev = hv_get_drvdata(device); > > + struct mousevsc_prt_msg *request; > > + struct mousevsc_prt_msg *response; > > + > > + request = &input_dev->protocol_req; > > + > > + memset(request, 0, sizeof(struct mousevsc_prt_msg)); > > + > > + request->type = PIPE_MESSAGE_DATA; > > + request->size = sizeof(struct synthhid_protocol_request); > > + > > + request->request.header.type = SYNTH_HID_PROTOCOL_REQUEST; > > + request->request.header.size = sizeof(unsigned int); > > + request->request.version_requested.version = > SYNTHHID_INPUT_VERSION; > > > > This is constant data; just do the same as with ack in > on_receive_device_info. It does not need to be a part of input_dev, does > it? Done. > > > + ret = vmbus_sendpacket(device->channel, request, > > + sizeof(struct pipe_prt_msg) - > > + sizeof(unsigned char) + > > + sizeof(struct synthhid_protocol_request), > > + (unsigned long)request, > > + VM_PKT_DATA_INBAND, > > + > VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED); > > + if (ret) > > + goto cleanup; > > + > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ); > > + if (!t) { > > + ret = -ETIMEDOUT; > > + goto cleanup; > > + } > > + > > + response = &input_dev->protocol_resp; > > + > > + if (!response->response.approved) { > > + pr_err("synthhid protocol request failed (version %d)\n", > > + SYNTHHID_INPUT_VERSION); > > + ret = -ENODEV; > > + goto cleanup; > > + } > > + > > + t = wait_for_completion_timeout(&input_dev->wait_event, 5*HZ); > > 5 seconds? That's a long time of HV to respond... Well, this is a safe number! We never wait this long. > > > + if (!t) { > > + ret = -ETIMEDOUT; > > + goto cleanup; > > + } > > + > > + /* > > + * We should have gotten the device attr, hid desc and report > > + * desc at this point > > + */ > > + if (input_dev->dev_info_status) > > + ret = -ENOMEM; > > Just do > > ret = input_dev->dev_info_status; Done. > > unconditionally. > > > + > > +cleanup: > > + > > + return ret; > > +} > > + > > +static int mousevsc_hid_open(struct hid_device *hid) > > +{ > > + return 0; > > +} > > + > > +static int mousevsc_hid_start(struct hid_device *hid) > > +{ > > + return 0; > > +} > > + > > +static void mousevsc_hid_close(struct hid_device *hid) > > +{ > > +} > > + > > +static void mousevsc_hid_stop(struct hid_device *hid) > > +{ > > +} > > + > > +static struct hid_ll_driver mousevsc_ll_driver = { > > + .open = mousevsc_hid_open, > > + .close = mousevsc_hid_close, > > + .start = mousevsc_hid_start, > > + .stop = mousevsc_hid_stop, > > +}; > > + > > +static struct hid_driver mousevsc_hid_driver; > > + > > +static int mousevsc_probe(struct hv_device *device, > > + const struct hv_vmbus_device_id *dev_id) > > +{ > > + int ret = 0; > > + struct mousevsc_dev *input_dev; > > + struct hid_device *hid_dev; > > + > > + input_dev = alloc_input_device(device); > > + > > + if (!input_dev) > > + return -ENOMEM; > > + > > + input_dev->init_complete = false; > > Should this also be in alloc_input_device()? Done. > > > + > > + ret = vmbus_open(device->channel, > > + INPUTVSC_SEND_RING_BUFFER_SIZE, > > + INPUTVSC_RECV_RING_BUFFER_SIZE, > > + NULL, > > + 0, > > + mousevsc_on_channel_callback, > > + device > > + ); > > + > > This blank line is extra IMO. > > > + if (ret != 0) { > > + free_input_device(input_dev); > > + return ret; > > Please do not mix error unwinding with gotos and unwinding in error > handling branches. Done. > > > + } > > + > > + ret = mousevsc_connect_to_vsp(device); > > + > > This blank line is extra IMO. > > > + if (ret != 0) > > + goto probe_err0; > > + > > + /* workaround SA-167 */ > > + if (input_dev->report_desc[14] == 0x25) > > + input_dev->report_desc[14] = 0x29; > > + > > + hid_dev = hid_allocate_device(); > > + if (IS_ERR(hid_dev)) { > > + ret = PTR_ERR(hid_dev); > > + goto probe_err0; > > + } > > + > > + hid_dev->ll_driver = &mousevsc_ll_driver; > > + hid_dev->driver = &mousevsc_hid_driver; > > You are not really hid driver; you are more of a "provider" so why do > you need to set hid_dev->driver in addition to hid_dev->ll_driver? > True, but hid_parse_report() expects that the driver field be set; so I need to fake this. > > + hid_dev->bus = BUS_VIRTUAL; > > + hid_dev->vendor = input_dev->hid_dev_info.vendor; > > + hid_dev->product = input_dev->hid_dev_info.product; > > + hid_dev->version = input_dev->hid_dev_info.version; > > + input_dev->hid_device = hid_dev; > > + > > + sprintf(hid_dev->name, "%s", "Microsoft Vmbus HID-compliant Mouse"); > > strlcpy? > > > + > > + ret = hid_parse_report(hid_dev, input_dev->report_desc, > > + input_dev->report_desc_size); > > + > > + if (ret) { > > + hid_err(hid_dev, "parse failed\n"); > > + goto probe_err1; > > + } > > + > > + ret = hid_hw_start(hid_dev, HID_CONNECT_HIDINPUT | > HID_CONNECT_HIDDEV); > > Why do you need to call hid_hw_start instead of letting HID core figure > it out for you? I am not a hid expert; but all hid low level drivers appear to do this. Initially, I was directly invoking hid_connect() directly and based on your Input, I chose to use hid_hw_start() which all other drivers are using. > > > + > > + if (ret) { > > + hid_err(hid_dev, "hw start failed\n"); > > + goto probe_err1; > > + } > > + > > + input_dev->connected = true; > > + input_dev->init_complete = true; > > + > > + return ret; > > + > > +probe_err1: > > + hid_destroy_device(hid_dev); > > + > > +probe_err0: > > + vmbus_close(device->channel); > > + free_input_device(input_dev); > > + return ret; > > +} > > + > > + > > +static int mousevsc_remove(struct hv_device *dev) > > +{ > > + struct mousevsc_dev *input_dev = hv_get_drvdata(dev); > > + > > + vmbus_close(dev->channel); > > + > > + if (input_dev->connected) { > > Can we even get here if input_dev->connected == false? > > > + hidinput_disconnect(input_dev->hid_device); > > You did not explicitly connect hidinput; leave disconnecting to the HID > core as well. > > It looks like all that is needed is: > > static int mousevsc_remove(struct hv_device *dev) > { > struct mousevsc_dev *mouse = hv_get_drvdata(dev); > > vmbus_close(dev->channel); > hid_destroy_device(mouse->hid_device); > mousevcs_free_device(mouse); > > return 0; > } Done. > > > + input_dev->connected = false; > > + hid_destroy_device(input_dev->hid_device); > > + } > > + > > + free_input_device(input_dev); > > + > > + return 0; > > +} > > + > > +static const struct hv_vmbus_device_id id_table[] = { > > + /* Mouse guid */ > > + { VMBUS_DEVICE(0x9E, 0xB6, 0xA8, 0xCF, 0x4A, 0x5B, 0xc0, 0x4c, > > + 0xB9, 0x8B, 0x8B, 0xA1, 0xA1, 0xF3, 0xF9, 0x5A) }, > > + { }, > > +}; > > + > > +MODULE_DEVICE_TABLE(vmbus, id_table); > > + > > +static struct hv_driver mousevsc_drv = { > > + .name = KBUILD_MODNAME, > > + .id_table = id_table, > > + .probe = mousevsc_probe, > > + .remove = mousevsc_remove, > > +}; > > + > > +static int __init mousevsc_init(void) > > +{ > > + return vmbus_driver_register(&mousevsc_drv); > > +} > > + > > +static void __exit mousevsc_exit(void) > > +{ > > + vmbus_driver_unregister(&mousevsc_drv); > > +} > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_VERSION(HV_DRV_VERSION); > > +module_init(mousevsc_init); > > +module_exit(mousevsc_exit); > > -- > > 1.7.4.1 > > > > Thanks. Thank you! K. Y _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
