On Thu, Jun 18, 2015 at 06:43:35AM -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). This patch drops the global variables and
> replaces the redundant calls to request_firmware()/release_firmware()
> in download_fw() with a single call pair in edge_startup(); the firmware
> image pointer is then passed to download_fw() and build_i2c_fw_hdr().
>
> Also, the firmware has a "build_number" field, though it apparently is
> unused (according to observations of the three firmware images I have
> seen and confirmed by Digi Tech Support). This comment describes its
> structure, in case it is populated in a future release.
>
> Signed-off-by: Peter E. Berger <pberger@xxxxxxxxxxx>
> ---
> drivers/usb/serial/io_ti.c | 100 +++++++++++++++++++++++----------------------
> 1 file changed, 51 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> index 69378a7..c76820b 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 */
> @@ -751,18 +748,18 @@ exit:
> }
>
> /* 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)
> {
> __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.
> @@ -785,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);
> -
> /* Copy version number into firmware record */
> firmware_rec = (struct ti_i2c_firmware_rec *)buffer;
>
> - firmware_rec->Ver_Major = OperationalMajorVersion;
> - firmware_rec->Ver_Minor = OperationalMinorVersion;
> + firmware_rec->Ver_Major = fw_major_version;
> + firmware_rec->Ver_Minor = fw_minor_version;
>
> /* Pointer to fw_down memory image */
> img_header = (struct ti_i2c_image_header *)&fw->data[4];
Note that sanity checks on the firmware size are missing here; you could
fix that as a follow up.
> @@ -811,8 +795,6 @@ static int build_i2c_fw_hdr(__u8 *header, struct device *dev)
> &fw->data[4 + sizeof(struct ti_i2c_image_header)],
> le16_to_cpu(img_header->Length));
>
> - release_firmware(fw);
> -
> for (i=0; i < buffer_size; i++) {
> cs = (__u8)(cs + buffer[i]);
> }
> @@ -826,8 +808,8 @@ static int build_i2c_fw_hdr(__u8 *header, struct device *dev)
> i2c_header->Type = I2C_DESC_TYPE_FIRMWARE_BLANK;
> i2c_header->Size = cpu_to_le16(buffer_size);
> i2c_header->CheckSum = cs;
> - firmware_rec->Ver_Major = OperationalMajorVersion;
> - firmware_rec->Ver_Minor = OperationalMinorVersion;
> + firmware_rec->Ver_Major = fw_major_version;
> + firmware_rec->Ver_Minor = fw_minor_version;
>
> return 0;
> }
> @@ -934,7 +916,8 @@ static int ti_cpu_rev(struct edge_ti_manuf_descriptor *desc)
> * This routine downloads the main operating code into the TI5052, using the
> * boot code already burned into E2PROM or ROM.
> */
> -static int download_fw(struct edgeport_serial *serial)
> +static int download_fw(struct edgeport_serial *serial,
> + const struct firmware *fw)
> {
> struct device *dev = &serial->serial->dev->dev;
> int status = 0;
> @@ -943,6 +926,8 @@ static int download_fw(struct edgeport_serial *serial)
> struct usb_interface_descriptor *interface;
> int download_cur_ver;
> int download_new_ver;
> + u8 fw_major_version = fw->data[0];
> + u8 fw_minor_version = fw->data[1];
>
> /* This routine is entered by both the BOOT mode and the Download mode
> * We can determine which code is running by the reading the config
If it wasn't for the horrible error handling in this function, you'd
really want to request the firmware here before checking the boot mode
(but releasing it would then currently involve changing just about every
error path).
You should at least add the missing sanity checks here, though. That is,
verify the firmware size before parsing the firmware header. Note that
fw->size >= 4 is assumed in one of the branches below.
You should also set the serial->fw_version here rather than in
edge_startup.
> @@ -1050,14 +1035,13 @@ static int download_fw(struct edgeport_serial *serial)
> version in I2c */
> download_cur_ver = (firmware_version->Ver_Major << 8) +
> (firmware_version->Ver_Minor);
> - download_new_ver = (OperationalMajorVersion << 8) +
> - (OperationalMinorVersion);
> + download_new_ver = (fw_major_version << 8) +
> + (fw_minor_version);
>
> dev_dbg(dev, "%s - >> FW Versions Device %d.%d Driver %d.%d\n",
> __func__, firmware_version->Ver_Major,
> firmware_version->Ver_Minor,
> - OperationalMajorVersion,
> - OperationalMinorVersion);
> + fw_major_version, fw_minor_version);
>
> /* Check if we have an old version in the I2C and
> update if necessary */
> @@ -1066,8 +1050,7 @@ static int download_fw(struct edgeport_serial *serial)
> __func__,
> firmware_version->Ver_Major,
> firmware_version->Ver_Minor,
> - OperationalMajorVersion,
> - OperationalMinorVersion);
> + fw_major_version, fw_minor_version);
>
> record = kmalloc(1, GFP_KERNEL);
> if (!record) {
> @@ -1143,6 +1126,9 @@ static int download_fw(struct edgeport_serial *serial)
> kfree(rom_desc);
> kfree(ti_manuf_desc);
> return -ENODEV;
> + } else {
> + /* Same or newer fw version is already loaded */
> + serial->fw_version = download_cur_ver;
> }
> kfree(firmware_version);
> }
> @@ -1181,7 +1167,7 @@ static int download_fw(struct edgeport_serial *serial)
> * UMP Ram to I2C and the firmware will update the
> * record type from 0xf2 to 0x02.
> */
> - status = build_i2c_fw_hdr(header, dev);
> + status = build_i2c_fw_hdr(header, dev, fw);
> if (status) {
> kfree(vheader);
> kfree(header);
> @@ -1284,9 +1270,6 @@ static int download_fw(struct edgeport_serial *serial)
> __u8 cs = 0;
> __u8 *buffer;
> int buffer_size;
> - int err;
> - const struct firmware *fw;
> - const char *fw_name = "edgeport/down3.bin";
>
> /* Validate Hardware version number
> * Read Manufacturing Descriptor from TI Based Edgeport
> @@ -1334,16 +1317,7 @@ static int download_fw(struct edgeport_serial *serial)
>
> /* Initialize the buffer to 0xff (pad the buffer) */
> 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;
> - }
> memcpy(buffer, &fw->data[4], fw->size - 4);
Sanity check on fw->size has always been missing...
> - release_firmware(fw);
>
> for (i = sizeof(struct ti_i2c_image_header);
> i < buffer_size; i++) {
> @@ -1358,7 +1332,8 @@ static int download_fw(struct edgeport_serial *serial)
> header->CheckSum = cs;
>
> /* Download the operational code */
> - dev_dbg(dev, "%s - Downloading operational code image (TI UMP)\n", __func__);
> + dev_dbg(dev, "%s - Downloading operational code image version %d.%d (TI UMP)\n",
> + __func__, fw_major_version, fw_minor_version);
> status = download_code(serial, buffer, buffer_size);
>
> kfree(buffer);
> @@ -2383,6 +2358,12 @@ static int edge_startup(struct usb_serial *serial)
> {
> struct edgeport_serial *edge_serial;
> int status;
> + int err;
You forgot to drop err (use status instead).
> + const struct firmware *fw;
> + const char *fw_name = "edgeport/down3.bin";
> + struct device *dev = &serial->dev->dev;
> + u8 fw_major_version;
> + u8 fw_minor_version;
>
> /* create our private serial structure */
> edge_serial = kzalloc(sizeof(struct edgeport_serial), GFP_KERNEL);
> @@ -2393,7 +2374,28 @@ 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_err(&serial->interface->dev,
> + "%s - Failed to load image \"%s\" err %d\n",
> + __func__, fw_name, err);
> + kfree(edge_serial);
> + return err;
> + }
> +
> + /*
> + * If the on-board firmware version is newer, download_fw()
> + * will retain it and revise edge_serial->fw_version. The
> + * firmware also includes an le16 "build_number" at data+2, but
> + * it has been set to 0 in all three of the images I have seen,
> + * and Digi Tech Support suggests that it is safe to ignore it.
> + */
> + fw_major_version = fw->data[0];
> + fw_minor_version = fw->data[1];
> + edge_serial->fw_version = (fw_major_version << 8) + fw_minor_version;
So just move this bit to download_fw.
> +
> + status = download_fw(edge_serial, fw);
> + release_firmware(fw);
> if (status) {
> kfree(edge_serial);
> return status;
Thanks,
Johan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
