On Fri, May 15, 2015 at 12:09:54AM -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.
Thanks for fixing this.
We should probably consider backporting some of this to stable as well.
But if you're fixing several independent issues, these should go in
separate patches if possible.
> Signed-off-by: Peter E. Berger <pberger@xxxxxxxxxxx>
> ---
> drivers/usb/serial/io_ti.c | 72 ++++++++++++++++++++++++++++++++++------------
> 1 file changed, 53 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> index 1db929b..7e40fbb 100644
> --- a/drivers/usb/serial/io_ti.c
> +++ b/drivers/usb/serial/io_ti.c
> @@ -188,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 */
> @@ -213,6 +209,25 @@ static int edge_remove_sysfs_attrs(struct usb_serial_port *port);
> /* Newer Edgeport firmware disconnects ports after periods of inactivity */
> #define EP_HEARTBEAT_SECS 15
>
> +int wrap_request_firmware(const struct firmware **fw, struct device *dev,
> + __u8 *MajorVersion, __u8 *MinorVersion, __u16 *BuildNumber)
Is it really necessary to wrap the request call? Looks like what you
really want is a edge_fw_get_version() helper. And I'm not sure even
that is really needed to fix the bug at hand (we want to keep fixes
minimal).
I know this driver wasn't a good example, but don't use CamelCase.
Also drop __ from the types here and elsewhere.
> +{
> + const char *fw_name = "edgeport/down3.bin";
> + int err;
> +
> + err = request_firmware(fw, fw_name, dev);
> + if (err) {
> + dev_dbg(dev, "%s - Failed to load image \"%s\" err %d\n",
> + __func__, fw_name, err);
> + } else {
Return err here and skip the else clause.
> + /* Save Download Version Number */
> + *MajorVersion = (*fw)->data[0];
> + *MinorVersion = (*fw)->data[1];
> + *BuildNumber = le16_to_cpup((__le16 *)&(*fw)->data[2]);
Use get_unaligned_le16().
> + }
> + return err;
return 0
> +}
> +
> static int ti_vread_sync(struct usb_device *dev, __u8 request,
> __u16 value, __u16 index, u8 *data, int size)
> {
> @@ -235,10 +250,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) {
> @@ -762,7 +780,9 @@ static int build_i2c_fw_hdr(__u8 *header, struct device *dev)
> 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 OperationalMajorVersion = 0;
> + __u8 OperationalMinorVersion = 0;
> + __u16 OperationalBuildNumber = 0;
CamelCase could be ok here in order to minimise diff. Could be fixed in
a follow up patch.
[...]
> /* 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
> @@ -1046,6 +1065,19 @@ static int download_fw(struct edgeport_serial *serial)
> return status;
> }
>
> + /* Get version numbers for on-disk fw */
> + if (wrap_request_firmware(&fw, dev,
> + &OperationalMajorVersion,
> + &OperationalMinorVersion,
> + &OperationalBuildNumber)) {
> + dev_dbg(dev, "%s - FW version query failed. Setting version to 0.",
> + __func__);
> + OperationalMajorVersion = 0;
> + OperationalMinorVersion = 0;
> + OperationalBuildNumber = 0;
No need to reset.
> + }
> + release_firmware(fw);
> +
This looks odd, loading the firmware only to check the version. Why not
use this image later if needed?
I know this code is extremely messy, but it should be doable.
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
