lkml.org 
[lkml]   [2017]   [Mar]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications
From
Date
Am Samstag, den 18.03.2017, 19:52 +0100 schrieb Tobias Herzog:
> USB devices may have very limitited endpoint packet sizes, so that
> notifications can not be transferred within one single usb packet.
> Reassembling of multiple packages may be necessary.

Hi,

almost perfect.
A few new issue. Comments inline.

>
> + struct usb_cdc_notification *dr = (struct usb_cdc_notification *)buf;
> + unsigned char *data = (unsigned char *)(dr + 1);

This is border line incorrect. It depends on the compiler not
adding padding. Please make it

buf + sizeof(struct usb_cdc_notification)

> - usb_mark_last_busy(acm->dev);
> -
> - data = (unsigned char *)(dr + 1);
> switch (dr->bNotificationType) {
> case USB_CDC_NOTIFY_NETWORK_CONNECTION:
> dev_dbg(&acm->control->dev,
> @@ -363,8 +337,77 @@ static void acm_ctrl_irq(struct urb *urb)
> __func__,
> dr->bNotificationType, dr->wIndex,
> dr->wLength, data[0], data[1]);
> + }
> +}
> +
> +/* control interface reports status changes with "interrupt" transfers */
> +static void acm_ctrl_irq(struct urb *urb)
> +{
> + struct acm *acm = urb->context;
> + struct usb_cdc_notification *dr = urb->transfer_buffer;
> + unsigned int current_size = urb->actual_length;
> + unsigned int expected_size, copy_size;
> + int retval;
> + int status = urb->status;
> +
> + switch (status) {
> + case 0:
> + /* success */
> break;
> + case -ECONNRESET:
> + case -ENOENT:
> + case -ESHUTDOWN:
> + /* this urb is terminated, clean up */
> + acm->nb_index = 0;
> + dev_dbg(&acm->control->dev,
> + "%s - urb shutting down with status: %d\n",
> + __func__, status);
> + return;
> + default:
> + dev_dbg(&acm->control->dev,
> + "%s - nonzero urb status received: %d\n",
> + __func__, status);
> + goto exit;
> }
> +
> + usb_mark_last_busy(acm->dev);
> +
> + if (acm->nb_index)
> + dr = (struct usb_cdc_notification *)acm->notification_buffer;
> +
> + /* size = notification-header + (optional) data */
> + expected_size = sizeof(struct usb_cdc_notification) +
> + le16_to_cpu(dr->wLength);
> +
> + if (current_size < expected_size) {
> + /* notification is transmitted fragmented, reassemble */
> + if (acm->nb_size < expected_size) {
> + if (acm->nb_size) {
> + kfree(acm->notification_buffer);
> + acm->nb_size = 0;
> + }
> + acm->notification_buffer =
> + kmalloc(expected_size, GFP_ATOMIC);

Given how kmalloc works you'd better round this up to a power of two.

> + if (!acm->notification_buffer)
> + goto exit;

This is most subtle. Please add a comment that this prevents a double
free if we get a disconnect()

> + acm->nb_size = expected_size;
> + }

Regards
Oliver

\
 
 \ /
  Last update: 2017-03-20 16:06    [W:0.077 / U:0.180 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site