On Mon, Jun 08, 2015 at 02:51:36PM -0500, Peter E. Berger wrote: > From: "Peter E. Berger" <[email protected]> > > The io_ti driver fails to download firmware to Edgeport devices such > as the EP/416, even when the on-disk firmware image > (/lib/firmware/edgeport/down3.bin) is more current than the version > on the EP/416. The current download code is broken in a few ways. > Notably it mis-uses global variables OperationalMajorVersion and > OperationalMinorVersion (reading their values before they've been properly > initialized and subsequently initializing them multiple times without > synchronization) and using an insufficient timeout in ti_vsend_sync() > when doing UMPC_COPY_DNLD_TO_I2C operations. With this patch, firmware > downloads work as expected: if the on disk firmware version is newer > than that on the device, it will be downloaded.
This looks like a great improvement to the firmware handling in general too by avoiding the redundant image loads (you should mention that as well). > Signed-off-by: Peter E. Berger <[email protected]> > --- > drivers/usb/serial/io_ti.c | 99 > +++++++++++++++++++++++----------------------- > 1 file changed, 49 insertions(+), 50 deletions(-) > > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c > index ddbb8fe..00e1034 100644 > --- a/drivers/usb/serial/io_ti.c > +++ b/drivers/usb/serial/io_ti.c > @@ -101,6 +101,7 @@ struct edgeport_serial { > struct mutex es_lock; > int num_ports_open; > struct usb_serial *serial; > + int fw_version; > }; > > > @@ -187,10 +188,6 @@ static const struct usb_device_id id_table_combined[] = { > > MODULE_DEVICE_TABLE(usb, id_table_combined); > > -static unsigned char OperationalMajorVersion; > -static unsigned char OperationalMinorVersion; > -static unsigned short OperationalBuildNumber; > - > static int closing_wait = EDGE_CLOSING_WAIT; > static bool ignore_cpu_rev; > static int default_uart_mode; /* RS232 */ > @@ -232,10 +229,13 @@ static int ti_vsend_sync(struct usb_device *dev, __u8 > request, > __u16 value, __u16 index, u8 *data, int size) > { > int status; > + int timeout = 1000; /* Timeout in msecs */ > > + if (request == UMPC_COPY_DNLD_TO_I2C) /* Downloads take longer */ > + timeout = 10000; > status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request, > (USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT), > - value, index, data, size, 1000); > + value, index, data, size, timeout); > if (status < 0) > return status; > if (status != size) { > @@ -748,18 +748,18 @@ exit: > } The firmware-download timeout fix is independent from the rest, so should go in its own patch. I'd also prefer if you add a timeout argument to the function (rather than check for UMPC_COPY_DNLD_TO_I2C) and update the call sites. Add two new defines for default and firmware-download timeouts. > /* Build firmware header used for firmware update */ > -static int build_i2c_fw_hdr(__u8 *header, struct device *dev) > +static int build_i2c_fw_hdr(u8 *header, struct device *dev, > + const struct firmware *fw) Continuation lines should be indented at least two tabs. > { > __u8 *buffer; > int buffer_size; > int i; > - int err; > __u8 cs = 0; > struct ti_i2c_desc *i2c_header; > struct ti_i2c_image_header *img_header; > struct ti_i2c_firmware_rec *firmware_rec; > - const struct firmware *fw; > - const char *fw_name = "edgeport/down3.bin"; > + u8 fw_major_version = fw->data[0]; > + u8 fw_minor_version = fw->data[1]; > > /* In order to update the I2C firmware we must change the type 2 record > * to type 0xF2. This will force the UMP to come up in Boot Mode. > @@ -782,24 +782,11 @@ static int build_i2c_fw_hdr(__u8 *header, struct device > *dev) > // Set entire image of 0xffs > memset(buffer, 0xff, buffer_size); > > - err = request_firmware(&fw, fw_name, dev); > - if (err) { > - dev_err(dev, "Failed to load image \"%s\" err %d\n", > - fw_name, err); > - kfree(buffer); > - return err; > - } > - > - /* Save Download Version Number */ > - OperationalMajorVersion = fw->data[0]; > - OperationalMinorVersion = fw->data[1]; > - OperationalBuildNumber = fw->data[2] | (fw->data[3] << 8); You're right, just drop the unused build number. > @@ -2387,7 +2371,22 @@ static int edge_startup(struct usb_serial *serial) > edge_serial->serial = serial; > usb_set_serial_data(serial, edge_serial); > > - status = download_fw(edge_serial); > + err = request_firmware(&fw, fw_name, dev); > + if (err) { > + dev_dbg(&serial->interface->dev, > + "%s - Failed to load image \"%s\" err %d\n", > + __func__, fw_name, err); Use dev_err here. > + kfree(edge_serial); > + return err; > + } > + > + fw_major_version = fw->data[0]; > + fw_minor_version = fw->data[1]; > + /* If on-board fw is newer, download_fw() will revise "fw_version" */ > + edge_serial->fw_version = (fw_major_version << 8) + fw_minor_version; > + > + status = download_fw(edge_serial, fw); > + release_firmware(fw); > if (status) { > kfree(edge_serial); > return status; Great job! Thanks, Johan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
