From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932066AbdCTPFK (ORCPT ); Mon, 20 Mar 2017 11:05:10 -0400 Received: from mx2.suse.de ([195.135.220.15]:53633 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754256AbdCTPEw (ORCPT ); Mon, 20 Mar 2017 11:04:52 -0400 Message-ID: <1490022179.25734.5.camel@suse.com> Subject: Re: [PATCH v2 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: Mon, 20 Mar 2017 16:02:59 +0100 In-Reply-To: <1489863159-10972-2-git-send-email-t-herzog@gmx.de> References: <1479118868.21146.4.camel@suse.com> <1489863159-10972-1-git-send-email-t-herzog@gmx.de> <1489863159-10972-2-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 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