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" <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).
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 <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.
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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
