From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752773AbdCOJ0x (ORCPT ); Wed, 15 Mar 2017 05:26:53 -0400 Received: from mx2.suse.de ([195.135.220.15]:58072 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751935AbdCOJ0v (ORCPT ); Wed, 15 Mar 2017 05:26:51 -0400 Message-ID: <1489569961.30434.6.camel@suse.com> Subject: Re: [PATCH 1/4] cdc-acm: reassemble fragmented notifications From: Oliver Neukum To: Tobias Herzog Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Date: Wed, 15 Mar 2017 10:26:01 +0100 In-Reply-To: <1489522489-6233-1-git-send-email-t-herzog@gmx.de> References: <1479118868.21146.4.camel@suse.com> <1489522489-6233-1-git-send-email-t-herzog@gmx.de> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > + > + 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;