lkml.org 
[lkml]   [2017]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 1/4] cdc-acm: reassemble fragmented notifications
    From
    Date
    Hi,

    thank you for your feedback. I tried to fix all issues with v2. I was
    just unsure with one point (see comment).


    /tobias


    > Am Dienstag, den 14.03.2017, 21:14 +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,
    >
    > thank you for the patch. Unfortunately it has some issue.
    > Please see the comments inside.
    >
    > Regards
    > Oliver
    >
    > >
    > >
    > > Signed-off-by: Tobias Herzog <t-herzog@gmx.de>
    > > ---
    > >  drivers/usb/class/cdc-acm.c | 102 +++++++++++++++++++++++++++++++-
    > > ------------
    > >  drivers/usb/class/cdc-acm.h |   2 +
    > >  2 files changed, 75 insertions(+), 29 deletions(-)
    > >
    > > diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-
    > > acm.c
    > > index e35b150..40714fe 100644
    > > --- a/drivers/usb/class/cdc-acm.c
    > > +++ b/drivers/usb/class/cdc-acm.c
    > > @@ -282,39 +282,13 @@ static DEVICE_ATTR(iCountryCodeRelDate,
    > > S_IRUGO, show_country_rel_date, NULL);
    > >   * Interrupt handlers for various ACM device responses
    > >   */
    > >  
    > > -/* control interface reports status changes with "interrupt"
    > > transfers */
    > > -static void acm_ctrl_irq(struct urb *urb)
    > > +static void acm_process_notification(struct acm *acm, unsigned
    > > char *buf)
    > >  {
    > > - struct acm *acm = urb->context;
    > > - struct usb_cdc_notification *dr = urb->transfer_buffer;
    > > - unsigned char *data;
    > >   int newctrl;
    > >   int difference;
    > > - int retval;
    > > - int status = urb->status;
    > > + struct usb_cdc_notification *dr = (struct
    > > usb_cdc_notification *)buf;
    > > + unsigned char *data = (unsigned char *)(dr + 1);
    > >  
    > > - switch (status) {
    > > - case 0:
    > > - /* success */
    > > - break;
    > > - case -ECONNRESET:
    > > - case -ENOENT:
    > > - case -ESHUTDOWN:
    > > - /* this urb is terminated, clean up */
    > > - 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);
    > > -
    > > - data = (unsigned char *)(dr + 1);
    > >   switch (dr->bNotificationType) {
    > >   case USB_CDC_NOTIFY_NETWORK_CONNECTION:
    > >   dev_dbg(&acm->control->dev,
    > > @@ -363,8 +337,74 @@ 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 */
    > > + kfree(acm->notification_buffer);
    > > + acm->notification_buffer = NULL;
    > Why? Disconnect() will free it anyway. It should be enough
    > to discard the content.
    >
    > >
    > > + 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->notification_buffer)
    > > + dr = (struct usb_cdc_notification *)acm-
    > > >notification_buffer;
    > > +
    > > + /* assume the first package contains at least two bytes */
    > > + expected_size = dr->wLength + 8;
    > You need the explain where you got the 8 from. In fact a define would
    > be best.
    I feel better to use 'sizeof(struct usb_cdc_notification)' here, than
    using an separate, hard-coded define. In fact this is really what we
    want here, as the buffer needs to cover the notification header (i.e.
    struct usb_cdc_notification) + the additional (optional) data.

    >
    > >
    > > +
    > > + if (current_size < expected_size) {
    > > + /* notification is transmitted framented,
    > > reassemble */
    > Please fix the typo.
    >
    > >
    > > + if (!acm->notification_buffer) {
    > > + acm->notification_buffer =
    > > + kmalloc(expected_size,
    > > GFP_ATOMIC);
    > This can fail. You _must_ check for that.
    >
    > >
    > > + acm->nb_index = 0;
    > > + }
    > > +
    > > + copy_size = min(current_size,
    > > + expected_size - acm->nb_index);
    > > +
    > > + memcpy(&acm->notification_buffer[acm->nb_index],
    > > +        urb->transfer_buffer, copy_size);
    > > + acm->nb_index += copy_size;
    > > + current_size = acm->nb_index;
    > > + }
    > > +
    > > + if (current_size < expected_size)
    > > + goto exit;
    > This is an unneeded goto.
    >
    > >
    > > + /* notification complete */
    > > + acm_process_notification(acm, (unsigned char *)dr);
    > > +
    > > + kfree(acm->notification_buffer);
    > Why? If one message was fragmented, the next one will also likely be
    > fragmented. Why release the buffer before you know whether it can be
    > reused?
    >
    > >
    > > + acm->notification_buffer = NULL;
    > > +
    > >  exit:
    > >   retval = usb_submit_urb(urb, GFP_ATOMIC);
    > >   if (retval && retval != -EPERM)
    > > @@ -1488,6 +1528,8 @@ static int acm_probe(struct usb_interface
    > > *intf,
    > >    epctrl->bInterval ? epctrl->bInterval :
    > > 16);
    > >   acm->ctrlurb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
    > >   acm->ctrlurb->transfer_dma = acm->ctrl_dma;
    > > + acm->notification_buffer = NULL;
    > > + acm->nb_index = 0;
    > >  
    > >   dev_info(&intf->dev, "ttyACM%d: USB ACM device\n", minor);
    > >  
    > > @@ -1580,6 +1622,8 @@ static void acm_disconnect(struct
    > > usb_interface *intf)
    > >   usb_free_coherent(acm->dev, acm->ctrlsize, acm-
    > > >ctrl_buffer, acm->ctrl_dma);
    > >   acm_read_buffers_free(acm);
    > >  
    > > + kfree(acm->notification_buffer);
    > > +
    > >   if (!acm->combined_interfaces)
    > >   usb_driver_release_interface(&acm_driver, intf ==
    > > acm->control ?
    > >   acm->data : acm->control);
    > > diff --git a/drivers/usb/class/cdc-acm.h b/drivers/usb/class/cdc-
    > > acm.h
    > > index c980f11..bc07fb2 100644
    > > --- a/drivers/usb/class/cdc-acm.h
    > > +++ b/drivers/usb/class/cdc-acm.h
    > > @@ -98,6 +98,8 @@ struct acm {
    > >   struct acm_wb *putbuffer; /* for
    > > acm_tty_put_char() */
    > >   int rx_buflimit;
    > >   spinlock_t read_lock;
    > > + u8 *notification_buffer; /* to
    > > reassemble fragmented notifications */
    > > + unsigned int nb_index;
    > >   int write_used; /*
    > > number of non-empty write buffers */
    > >   int transmitting;
    > >   spinlock_t write_lock;

    \
     
     \ /
      Last update: 2017-03-18 19:53    [W:19.438 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site