On Mon, Jun 08, 2015 at 02:51:36PM -0500, Peter E. Berger wrote:
> From: "Peter E. Berger" <pberger@xxxxxxxxxxx>
>
> 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 <pberger@xxxxxxxxxxx>
> ---
> 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
