From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751819AbcACQp0 (ORCPT ); Sun, 3 Jan 2016 11:45:26 -0500 Received: from mail-lf0-f44.google.com ([209.85.215.44]:32966 "EHLO mail-lf0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750792AbcACQpV (ORCPT ); Sun, 3 Jan 2016 11:45:21 -0500 Date: Sun, 3 Jan 2016 17:45:56 +0100 From: Johan Hovold To: Mathieu OTHACEHE Cc: johan@kernel.org, gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org Subject: Re: [PATCH 3/3] USB: mxu11x0: move firmware download and endpoint testing to probe callback Message-ID: <20160103164556.GF25304@localhost> References: <1451831161-26792-1-git-send-email-m.othacehe@gmail.com> <1451831161-26792-4-git-send-email-m.othacehe@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1451831161-26792-4-git-send-email-m.othacehe@gmail.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jan 03, 2016 at 03:26:01PM +0100, Mathieu OTHACEHE wrote: > Move interrupt in endpoint test and firmware download to a new probe > callback. This avoids unnecessary memory allocations done by core > before port_probe callback is called. > > If the device has to be reseted (firmware downloaded) or if the > interface is incorrect (no interrupt in endpoint), the probe fails > by returning ENODEV. > > Signed-off-by: Mathieu OTHACEHE > --- > drivers/usb/serial/mxu11x0.c | 130 ++++++++++++++++++++++++++----------------- > 1 file changed, 78 insertions(+), 52 deletions(-) > > diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c > index 354fcb5..0392531 100644 > --- a/drivers/usb/serial/mxu11x0.c > +++ b/drivers/usb/serial/mxu11x0.c > @@ -272,7 +272,8 @@ static int mxu1_send_ctrl_urb(struct usb_serial *serial, > } > > static int mxu1_download_firmware(struct usb_serial *serial, > - const struct firmware *fw_p) > + const struct firmware *fw_p, > + struct usb_endpoint_descriptor *endpoint) > { > int status = 0; > int buffer_size; > @@ -285,7 +286,7 @@ static int mxu1_download_firmware(struct usb_serial *serial, > struct mxu1_firmware_header *header; > unsigned int pipe; > > - pipe = usb_sndbulkpipe(dev, serial->port[0]->bulk_out_endpointAddress); > + pipe = usb_sndbulkpipe(dev, endpoint->bEndpointAddress); > > buffer_size = fw_p->size + sizeof(*header); > buffer = kmalloc(buffer_size, GFP_KERNEL); > @@ -336,16 +337,85 @@ static void mxu1_release(struct usb_serial *serial) > kfree(mxdev); > } > > -static int mxu1_port_probe(struct usb_serial_port *port) > +static int mxu1_probe(struct usb_serial *serial, const struct usb_device_id *id) > { > - struct mxu1_port *mxport; > - struct mxu1_device *mxdev; > + struct usb_host_interface *cur_altsetting; > + char fw_name[32]; > + const struct firmware *fw_p = NULL; > + struct usb_device *dev = serial->dev; > + u16 model; > + int err; > + struct usb_endpoint_descriptor *endpoint, *interrupt_in, *bulk_out; > + int i; > + > + dev_dbg(&serial->interface->dev, "%s - product 0x%04X, num configurations %d, configuration value %d\n", > + __func__, le16_to_cpu(dev->descriptor.idProduct), > + dev->descriptor.bNumConfigurations, > + dev->actconfig->desc.bConfigurationValue); > + > + cur_altsetting = serial->interface->cur_altsetting; > + interrupt_in = NULL; > + bulk_out = NULL; > + > + for (i = 0; i < cur_altsetting->desc.bNumEndpoints; i++) { > + endpoint = &cur_altsetting->endpoint[i].desc; > + if (usb_endpoint_is_bulk_out(endpoint)) { > + dev_dbg(&serial->interface->dev, > + "found bulk out on endpoint %d\n", i); > + bulk_out = endpoint; > + } > + > + if (usb_endpoint_is_int_in(endpoint)) { > + dev_dbg(&serial->interface->dev, > + "found interrupt in on endpoint %d\n", i); > + interrupt_in = endpoint; > + } > + } > + > + /* if we have only 1 bulk out endpoint, download firmware */ > + if (bulk_out && (cur_altsetting->desc.bNumEndpoints == 1)) { No need for inner parentheses. > + > + model = le16_to_cpu(dev->descriptor.idProduct); > + snprintf(fw_name, > + sizeof(fw_name), > + "moxa/moxa-%04x.fw", > + model); > + > + err = request_firmware(&fw_p, fw_name, &serial->interface->dev); > + if (err) { > + dev_err(&serial->interface->dev, "failed to request firmware: %d\n", > + err); > + return -ENODEV; > + } > + > + err = mxu1_download_firmware(serial, fw_p, bulk_out); > + if (err) > + goto err_release_firmware; > > - if (!port->interrupt_in_urb) { > - dev_err(&port->dev, "no interrupt urb\n"); > + /* device is being reset */ > + err = -ENODEV; > + goto err_release_firmware; > + > + } else if (!interrupt_in) { > + /* firmware is already loaded but there is > + * no interrupt in endpoint available > + */ Multi-line comments should be on the following format /* * ... */ Looks good otherwise. Johan