From: Oliver Neukum <oneukum@suse.com>
To: Tobias Herzog <t-herzog@gmx.de>
Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org,
linux-usb@vger.kernel.org
Subject: Re: [PATCH 1/4] cdc-acm: reassemble fragmented notifications
Date: Wed, 15 Mar 2017 10:26:01 +0100 [thread overview]
Message-ID: <1489569961.30434.6.camel@suse.com> (raw)
In-Reply-To: <1489522489-6233-1-git-send-email-t-herzog@gmx.de>
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;
next prev parent reply other threads:[~2017-03-15 9:26 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <1479118868.21146.4.camel@suse.com>
2017-03-14 20:14 ` [PATCH 1/4] cdc-acm: reassemble fragmented notifications Tobias Herzog
2017-03-14 20:14 ` [PATCH 2/4] cdc-acm: fix possible invalid access when processing notification Tobias Herzog
2017-03-15 9:26 ` Oliver Neukum
2017-03-14 20:14 ` [PATCH 3/4] cdc-acm: log message for serial state notification Tobias Herzog
2017-03-14 20:14 ` [PATCH 4/4] cdc-acm: remove unused element of struct acm Tobias Herzog
2017-03-15 9:26 ` Oliver Neukum [this message]
2017-03-18 18:52 ` [PATCH 1/4] cdc-acm: reassemble fragmented notifications Tobias Herzog
2017-03-18 18:52 ` [PATCH v2 0/4] " Tobias Herzog
2017-03-18 18:52 ` [PATCH v2 1/4] " Tobias Herzog
2017-03-20 15:02 ` Oliver Neukum
2017-03-24 21:50 ` Tobias Herzog
2017-03-28 8:04 ` Oliver Neukum
2017-03-18 18:52 ` [PATCH v2 2/4] cdc-acm: fix possible invalid access when processing notification Tobias Herzog
2017-03-19 9:50 ` Sergei Shtylyov
2017-03-20 15:04 ` Oliver Neukum
2017-03-18 18:52 ` [PATCH v2 3/4] cdc-acm: log message for serial state notification Tobias Herzog
2017-03-18 18:52 ` [PATCH v2 4/4] cdc-acm: remove unused element of struct acm Tobias Herzog
2017-03-30 20:15 ` [PATCH v3 0/4] cdc-acm: reassemble fragmented notifications Tobias Herzog
2017-03-30 20:15 ` [PATCH v3 1/4] cdc-acm: fix possible invalid access when processing notification Tobias Herzog
2017-03-31 9:31 ` Oliver Neukum
2017-03-30 20:15 ` [PATCH v3 2/4] cdc-acm: reassemble fragmented notifications Tobias Herzog
2017-03-31 9:33 ` Oliver Neukum
2017-03-30 20:15 ` [PATCH v3 3/4] cdc-acm: log message for serial state notification Tobias Herzog
2017-03-31 9:34 ` Oliver Neukum
2017-03-30 20:15 ` [PATCH v3 4/4] cdc-acm: remove unused element of struct acm Tobias Herzog
2017-03-31 9:35 ` Oliver Neukum
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1489569961.30434.6.camel@suse.com \
--to=oneukum@suse.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=t-herzog@gmx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).