Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1600871 > unrolled thread

[PATCH 1/4] cdc-acm: reassemble fragmented notifications

Started byTobias Herzog <t-herzog@gmx.de>
First post2017-03-14 21:20 +0100
Last post2017-03-18 20:00 +0100
Articles 3 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/4] cdc-acm: reassemble fragmented notifications Tobias Herzog <t-herzog@gmx.de> - 2017-03-14 21:20 +0100
    Re: [PATCH 1/4] cdc-acm: reassemble fragmented notifications Oliver Neukum <oneukum@suse.com> - 2017-03-15 10:30 +0100
      Re: [PATCH 1/4] cdc-acm: reassemble fragmented notifications Tobias Herzog <t-herzog@gmx.de> - 2017-03-18 20:00 +0100

#1600871 — [PATCH 1/4] cdc-acm: reassemble fragmented notifications

FromTobias Herzog <t-herzog@gmx.de>
Date2017-03-14 21:20 +0100
Subject[PATCH 1/4] cdc-acm: reassemble fragmented notifications
Message-ID<tl15f-5mY-11@gated-at.bofh.it>
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.

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;
+		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;
+
+	if (current_size < expected_size) {
+		/* notification is transmitted framented, reassemble */
+		if (!acm->notification_buffer) {
+			acm->notification_buffer =
+					kmalloc(expected_size, GFP_ATOMIC);
+			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;
+
+	/* notification complete */
+	acm_process_notification(acm, (unsigned char *)dr);
+
+	kfree(acm->notification_buffer);
+	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;
-- 
2.1.4

[toc] | [next] | [standalone]

#1601170

FromOliver Neukum <oneukum@suse.com>
Date2017-03-15 10:30 +0100
Message-ID<tldpL-5Cc-11@gated-at.bofh.it>
In reply to#1600871
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.

> +
> +	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;

[toc] | [prev] | [next] | [standalone]

#1603842

FromTobias Herzog <t-herzog@gmx.de>
Date2017-03-18 20:00 +0100
Message-ID<tmrK2-27E-15@gated-at.bofh.it>
In reply to#1601170
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;

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web