Messages in this thread | | | Subject | Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications | From | Oliver Neukum <> | Date | Mon, 20 Mar 2017 16:02:59 +0100 |
| |
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
| |