On Mon, 2015-06-22 at 11:43 +0200, Johan Hovold wrote:
> 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.
I did some digging and it looks like we can actually do several sanity
checks on the firmware image. It seems that the Edgeport firmware
images have a 7-byte header:
u8 major_version;
u8 minor_version;
le16 build_number;
le16 length;
u8 checksum;
"length" appears to be the number of bytes of actual data following the
header.
"checksum" is apparently (from the images I have seen) the low order
byte resulting from adding the values of all the data bytes.
So, I'm testing a new ck_fw_sanity() function that checks for these
error conditions:
1. NULL (missing) firmware image
2. Incomplete firmware header:
#define FW_HEADER_SIZE 7
fw->size < FW_HEADER_SIZE
3. Bad firmware size:
fw->size != FW_HEADER_SIZE + length_data
4. Bad firmware checksum:
compute checksum and compare to the stored version
I call ck_fw_sanity() at the top of download_fw(), so, by the time we
call build_i2c_fw_hdr(), the firmware image has already been sanity
checked. Initial testing looks good to me. Do you agree that I should
include my new ck_fw_sanity() function and its invocations (as a new,
separate 4th patch) in the upcoming v7 patchset?
>
> > @@ -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).
Agreed! My colleague, Al Borchers, and I submitted some minor patches
for this driver in the early days of usb-serial (after we wrote
digi_acceleport.c), and I recall this function being frightful even
then. Given its current state, I think our best approach is to leave
the request_firmware() and release_firmware() calls in edge_startup(),
which I think agrees with your opinion.
>
> 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.
Agreed. In my current test implementation I call the new
check_fw_sanity() function (described above) here, before parsing the
firmware header. Sound OK?
>
> You should also set the serial->fw_version here rather than in
> edge_startup.
Done.
>
> > @@ -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...
Done (via the check_fw_sanity() call above.
>
> > - 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).
Done.
>
> > + 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.
Done.
I think I'm close to being able to submit a v7 patchset.
Thanks.
--Peter
>
> > +
> > + 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
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
