Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications
Am Freitag, den 24.03.2017, 22:50 +0100 schrieb Tobias Herzog:
> Hi Oliver,
>
> thank you for your patience... :) I have a question to one of your
> comments (see below).
>
> Best regards,
> Tobias
>
> >
> > Am Samstag, den 18.03.2017, 19:52 +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,
> >
> > almost perfect.
> > A few new issue. Comments inline.
> >
> > >
> > >
> > >
> > > + struct usb_cdc_notification *dr = (struct
> > > usb_cdc_notification *)buf;
> > > + unsigned char *data = (unsigned char *)(dr + 1);
> > This is border line incorrect. It depends on the compiler not
> > adding padding. Please make it
> >
> > buf + sizeof(struct usb_cdc_notification)
> >
> > >
> > >
> > > - 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,77 @@ 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 */
> > > + acm->nb_index = 0;
> > > + 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->nb_index)
> > > + dr = (struct usb_cdc_notification *)acm-
> > > >
> > > > notification_buffer;
> > > +
> > > + /* size = notification-header + (optional) data */
> > > + expected_size = sizeof(struct usb_cdc_notification) +
> > > + le16_to_cpu(dr->wLength);
> > > +
> > > + if (current_size < expected_size) {
> > > + /* notification is transmitted fragmented,
> > > reassemble */
> > > + if (acm->nb_size < expected_size) {
> > > + if (acm->nb_size) {
> > > + kfree(acm->notification_buffer);
> > > + acm->nb_size = 0;
> > > + }
> > > + acm->notification_buffer =
> > > + kmalloc(expected_size,
> > > GFP_ATOMIC);
> > Given how kmalloc works you'd better round this up to a power of two.
> >
> > >
> > >
> > > + if (!acm->notification_buffer)
> > > + goto exit;
> > This is most subtle. Please add a comment that this prevents a double
> > free if we get a disconnect()
> I'm unsure if I got this right: Are you talking about the fact, that
> the use of 'kmalloc' will make 'acm->notification_buffer' valid again
> (i.e. NULL or pointing to a correctly allocated block), after it was
yes
> "invalidated" by 'kfree' (in case the previously allocated buffer was
> too small)?
> The 'goto exit'-thing for me is just not to use the buffer if
> allocation fails. Or am I missing anything here?
You need to consider the hot unplug case.
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications
Hi Oliver,
thank you for your patience... :) I have a question to one of your
comments (see below).
Best regards,
Tobias
> Am Samstag, den 18.03.2017, 19:52 +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,
>
> almost perfect.
> A few new issue. Comments inline.
>
> >
> >
> > + struct usb_cdc_notification *dr = (struct
> > usb_cdc_notification *)buf;
> > + unsigned char *data = (unsigned char *)(dr + 1);
> This is border line incorrect. It depends on the compiler not
> adding padding. Please make it
>
> buf + sizeof(struct usb_cdc_notification)
>
> >
> > - 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,77 @@ 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 */
> > + acm->nb_index = 0;
> > + 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->nb_index)
> > + dr = (struct usb_cdc_notification *)acm-
> > >notification_buffer;
> > +
> > + /* size = notification-header + (optional) data */
> > + expected_size = sizeof(struct usb_cdc_notification) +
> > + le16_to_cpu(dr->wLength);
> > +
> > + if (current_size < expected_size) {
> > + /* notification is transmitted fragmented,
> > reassemble */
> > + if (acm->nb_size < expected_size) {
> > + if (acm->nb_size) {
> > + kfree(acm->notification_buffer);
> > + acm->nb_size = 0;
> > + }
> > + acm->notification_buffer =
> > + kmalloc(expected_size,
> > GFP_ATOMIC);
> Given how kmalloc works you'd better round this up to a power of two.
>
> >
> > + if (!acm->notification_buffer)
> > + goto exit;
> This is most subtle. Please add a comment that this prevents a double
> free if we get a disconnect()
I'm unsure if I got this right: Are you talking about the fact, that
the use of 'kmalloc' will make 'acm->notification_buffer' valid again
(i.e. NULL or pointing to a correctly allocated block), after it was
"invalidated" by 'kfree' (in case the previously allocated buffer was
too small)?
The 'goto exit'-thing for me is just not to use the buffer if
allocation fails. Or am I missing anything here?
>
> >
> > + acm->nb_size = expected_size;
> > + }
> Regards
> Oliver
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications
Am Samstag, den 18.03.2017, 19:52 +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,
almost perfect.
A few new issue. Comments inline.
>
> + struct usb_cdc_notification *dr = (struct usb_cdc_notification *)buf;
> + unsigned char *data = (unsigned char *)(dr + 1);
This is border line incorrect. It depends on the compiler not
adding padding. Please make it
buf + sizeof(struct usb_cdc_notification)
> - 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,77 @@ 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 */
> + acm->nb_index = 0;
> + 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->nb_index)
> + dr = (struct usb_cdc_notification *)acm->notification_buffer;
> +
> + /* size = notification-header + (optional) data */
> + expected_size = sizeof(struct usb_cdc_notification) +
> + le16_to_cpu(dr->wLength);
> +
> + if (current_size < expected_size) {
> + /* notification is transmitted fragmented, reassemble */
> + if (acm->nb_size < expected_size) {
> + if (acm->nb_size) {
> + kfree(acm->notification_buffer);
> + acm->nb_size = 0;
> + }
> + acm->notification_buffer =
> + kmalloc(expected_size, GFP_ATOMIC);
Given how kmalloc works you'd better round this up to a power of two.
> + if (!acm->notification_buffer)
> + goto exit;
This is most subtle. Please add a comment that this prevents a double
free if we get a disconnect()
> + acm->nb_size = expected_size;
> + }
Regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/4] cdc-acm: reassemble fragmented notifications
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
---
drivers/usb/class/cdc-acm.c | 106
drivers/usb/class/cdc-acm.h | 3 ++
2 files changed, 80 insertions(+), 29 deletions(-)
diff --git a/drivers/usb/class/cdc-acm.c b/drivers/usb/class/cdc-acm.c
index e35b150..74acca1 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;
-
- 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;
- }
+ struct usb_cdc_notification *dr = (struct usb_cdc_notification *)buf;
+ unsigned char *data = (unsigned char *)(dr + 1);
- 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,77 @@ 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 */
+ acm->nb_index = 0;
+ 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->nb_index)
+ dr = (struct usb_cdc_notification *)acm->notification_buffer;
+
+ /* size = notification-header + (optional) data */
+ expected_size = sizeof(struct usb_cdc_notification) +
+ le16_to_cpu(dr->wLength);
+
+ if (current_size < expected_size) {
+ /* notification is transmitted fragmented, reassemble */
+ if (acm->nb_size < expected_size) {
+ if (acm->nb_size) {
+ kfree(acm->notification_buffer);
+ acm->nb_size = 0;
+ }
+ acm->notification_buffer =
+ kmalloc(expected_size, GFP_ATOMIC);
+ if (!acm->notification_buffer)
+ goto exit;
+ acm->nb_size = expected_size;
+ }
+
+ 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) {
+ /* notification complete */
+ acm_process_notification(acm, (unsigned char *)dr);
+ acm->nb_index = 0;
+ }
+
exit:
retval = usb_submit_urb(urb, GFP_ATOMIC);
if (retval && retval != -EPERM)
