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

[Tobias Herzog]

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(>control->dev,
> > -   "%s - urb shutting down with status:
> > %d\n",
> > -   __func__, status);
> > -   return;
> > -   default:
> > -   dev_dbg(>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(>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(>control->dev,
> > +   "%s - urb shutting down with status:
> > %d\n",
> > +   __func__, status);
> > +   return;
> > +   default:
> > +   dev_dbg(>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 =
> > +   

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

[Oliver Neukum]

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(>control->dev,
> - "%s - urb shutting down with status: %d\n",
> - __func__, status);
> - return;
> - default:
> - dev_dbg(>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(>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(>control->dev,
> + "%s - urb shutting down with status: %d\n",
> + __func__, status);
> + return;
> + default:
> + dev_dbg(>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(>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.

>