From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S936463AbdCXVu2 convert rfc822-to-8bit (ORCPT ); Fri, 24 Mar 2017 17:50:28 -0400 Received: from mout.gmx.net ([212.227.17.21]:61472 "EHLO mout.gmx.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933123AbdCXVuT (ORCPT ); Fri, 24 Mar 2017 17:50:19 -0400 Message-ID: <1490392212.2779.33.camel@gmx.de> Subject: Re: [PATCH v2 1/4] cdc-acm: reassemble fragmented notifications From: Tobias Herzog To: Oliver Neukum Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Date: Fri, 24 Mar 2017 22:50:12 +0100 In-Reply-To: <1490022179.25734.5.camel@suse.com> 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> <1490022179.25734.5.camel@suse.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.5.2-0ubuntu3.1 Mime-Version: 1.0 Content-Transfer-Encoding: 8BIT X-Provags-ID: V03:K0:PVpOG+4I0sL/WV1/4dC0snlo+GwNHlrovF8o7iz5rCCeefP4ZqQ bdIMdB8IeQfxrNe40y3R2jRslQtLrAGy7Q1VqcoHxuldS8riI2yPrH716yLUjogHxXYOYQI eMx+wxSwS4Z+G8wcbkrS6jqE2t7KVH1sJUOPxg+yCOPa9eihJXO4z2N15PiEwGpJsIqnWDo VNM5Xr3+0ZlZxwoRu8DYQ== X-UI-Out-Filterresults: notjunk:1;V01:K0:ZGYpBGS6JWM=:VrZRySGcTWPNvvsC8ywmtg jZfmuUHxRIAAtaug/uo9nC63GTtKRUeBIp326xleTXZYp45dwf01h0EVKSB8aE90IXVjanyWe T6UuRc1/LtXTEAdiOl9b8kMxQO3XNG1ji2XwLEy+WweZoY3SckpARD95VMh+4Z/s/HulDmIsm sp9be+VMwv+nRrke0LsUrSh84SvXCvTajIArzRp+MKt75ouY5jCwLPOyS6CjOkXf8UNTUEkuV gEuZiw2sHZYeDR47gkY0O7LPKa6UP3pwb5bAMFoa6VNPydFPG/+Vj3GCQkDF4xQw31QULzRM5 5lNWrjbDNjz0J+sXOerHifgnixlYr80xquQXkAyOVWXo4Ast1EKTRMgidJO5AVk8B9AGYVplN s9BaXR9jrka5BEcw+fEu1V6pbXwzzM3qAGHhzpEUkL16YHZLX4tHThkkVRvb/0QaITySpvjot HKIJFi1Zdg/5fuEIygnKoWs1+gJ8U+mdeTjsWTYn+53bbfQrUI1nXqRs6qih10Eymx+GvyR2W OUVQM1x9N/DaC00HPBCtsaUcf+aJFPeWbba7SJyzHJ+r3Pg8Jw3h2QL83Em3lsTp0c0xp0dDn BowRLDrA14AXjBkDBeM8r9OWAoU/IyWUx9kO7NXAseFwl1zNOd7WR61pJtDfBfGwF2BbZ4GDB pGTRxnC0SCBET9iZpXKdpvTad4reslvKIVnCMJCHWarfVs/uQT5brAJL5zbezTZ0Nks8Th46B 06I+LKyX8YwbAej6SQ/HbnD9o0KN/9RL16YOQOeaFtRIxla1BrE5jERr1UnYIHqmPDH1noXl/ xbMIUL7 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >