From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751882AbdCRSwl convert rfc822-to-8bit (ORCPT ); Sat, 18 Mar 2017 14:52:41 -0400 Received: from mout.gmx.net ([212.227.17.20]:52499 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751699AbdCRSwi (ORCPT ); Sat, 18 Mar 2017 14:52:38 -0400 Message-ID: <1489863146.3390.8.camel@gmx.de> Subject: Re: [PATCH 1/4] cdc-acm: reassemble fragmented notifications From: Tobias Herzog To: Oliver Neukum Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Date: Sat, 18 Mar 2017 19:52:26 +0100 In-Reply-To: <1489569961.30434.6.camel@suse.com> References: <1479118868.21146.4.camel@suse.com> <1489522489-6233-1-git-send-email-t-herzog@gmx.de> <1489569961.30434.6.camel@suse.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu3.1 Mime-Version: 1.0 Content-Transfer-Encoding: 8BIT X-Provags-ID: V03:K0:zzGzBhrjXviPGDIPuPKW3iZNauw/zeIVmED7MwkbpmvdWaJ2ylg CCeEGZwlfMvVD54n9POHCGtoByAbi8T8yfrg0nuewuIAWgWLNuYV4ZrczNtheiSU9hIxqIJ zmQPZ9ORa9YGhwuftqHwJBVYPXKu9QCLErDOT8KJv7G7swhGDccxRLsXF67WLdS9u0gUlvr lphh2FJ1uCjkdM1n8W6Jw== X-UI-Out-Filterresults: notjunk:1;V01:K0:a5q2THwNKWk=:DcskbPtGKIr5hWa1pFRNd8 NUn5YN/tfM9j4MfsiXUqWX7rDk6Dd+yOBfgY3xG5fHtY1HAGJCkISZ+KXYOGrSTY+OVihcl/k GJtvt8UroFdLYQ3Xbq5/DqVxbrkPz9tLDVqjJj8PMpRwPzJMbW59qYEXYZuv7bE1sFok9bAgZ hSznOQqtN+wQ8UtOyIX/ZVQZuPn+qNPAnMx7Vlt95DPtcACTnyjKrsOiennoQeZ02+rjnTbVD usMRJniSJ8rZS2LspdoWWh+VRUgZ2QYfyaZYjNvKWaw4hHBXWzOt04SJ/LlgGDCuGk4SeIzmh 6ym/MVAWB41obQpFzvfBE6rY1gvvfzHaZrhwsuZC48xfdUK+wDxcMw8Munci739AxOtJX4w2w CkveMMLDBecv73fRz12tRhU+0Zr+5WEYjvZETVuwDF6wHN5VdwCMiSlwjlFyXbgXmsxe8KxG/ MuuzW5M+rIYkipM9CVfSOnN1khP2ZuaF6wR77ejC8dKh8BQ3/ByJwe/MZY3nR4dyASFmw2Y+E FvOuoe+85WozW2UxZMxiKozH6BWJxzc/FnENZOTMuv9PnKpgnAUsTeWLs6rOtQGG6qKdV8XYO UKU/jo/VQeqh2n6aBs95bB7ShM5Otcpsmewi3uyCNDhdY5Fr2bqH6IYhFI/RPUem4A+UDIsiu ebEjcQuTI0TmH+woH7uHhvn+mWUJypaRpFWO1/4NVKWIjRH/FTnH/18fbGr1bJ5sANnUs95Tg QLAqX55rZy7++DViCTb9/BTEgGA8MM8/QtaXqAzz2Y8HPDwKqQtfK+kNRM0nU/yxkS9izp5mc YmMy6tM Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > --- > >  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;