Hi Oliver,
thank you for your patience... :) I have a question to one of your
comments (see below).
Best regards,
Tobias
> 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()
I'm unsure if I got this right: Are you talking about the fact, that
the use of 'kmalloc' will make 'acm->notification_buffer' valid again
(i.e. NULL or pointing to a correctly allocated block), after it was
"invalidated" by 'kfree' (in case the previously allocated buffer was
too small)?
The 'goto exit'-thing for me is just not to use the buffer if
allocation fails. Or am I missing anything here?
>
> >
> > + acm->nb_size = expected_size;
> > + }
> Regards
> Oliver
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
