Hi Johan: I think my v5 patchset to address your review comments (here and in the other messages) is just about ready to submit. I'll include some point-by-point description details below:
On Thu, 2015-06-11 at 12:03 +0200, Johan Hovold wrote: > 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). Good point. I expanded the description in the now (in v5) separate 03/10 "Move request_firmware() calls out of download_fw()" patch to include "...and replaces the redundant calls to request_firmware()/release_firmware()..." > > > 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. Done. It's now in patch 1/10. > > 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. Done. It's now in patch 2/10. > > > /* 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. Done. > > > { > > __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. Done. > > > @@ -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. Done. Thanks. --Peter > > > + 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
