On Wed, Jul 22, 2015 at 01:56:14PM -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().
>
> Signed-off-by: Peter E. Berger <pberger@xxxxxxxxxxx>
> ---
> drivers/usb/serial/io_ti.c | 115 ++++++++++++++++++++++++++-------------------
> 1 file changed, 66 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> index 69378a7..6cff12c 100644
> --- a/drivers/usb/serial/io_ti.c
> +++ b/drivers/usb/serial/io_ti.c
> @@ -71,6 +71,25 @@ struct product_info {
> __u8 hardware_type; /* Type of hardware */
> } __attribute__((packed));
>
> +/*
> + * Edgeport firmware header
> + *
> + * "build_number" 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.
> + *
> + * "length" is the number of bytes of actual data following the header.
> + *
> + * "checksum" is the low order byte resulting from adding the values of
> + * all the data bytes.
> + */
> +struct edgeport_fw_hdr {
> + u8 major_version;
> + u8 minor_version;
> + u16 build_number;
> + u16 length;
These should be __le16.
> + u8 checksum;
> +} __packed;
> +
> struct edgeport_port {
> __u16 uart_base;
> __u16 dma_address;
> @@ -934,7 +934,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 +944,17 @@ static int download_fw(struct edgeport_serial *serial)
> struct usb_interface_descriptor *interface;
> int download_cur_ver;
> int download_new_ver;
> + struct edgeport_fw_hdr *fw_hdr = (struct edgeport_fw_hdr *)fw->data;
> +
> + if (fw->size < sizeof(struct edgeport_fw_hdr)) {
Missing le16_to_cpu on fw->size.
> + dev_err(&serial->serial->interface->dev,
> + "%s - Incomplete firmware header.\n", __func__);
No need to include the function name here.
> + return -EINVAL;
> + }
> +
> + /* If on-board version is newer, "fw_version" will be updated below. */
> + serial->fw_version = (fw_hdr->major_version << 8) +
> + fw_hdr->minor_version;
>
> /* 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
> @@ -2383,6 +2387,9 @@ static int edge_startup(struct usb_serial *serial)
> {
> struct edgeport_serial *edge_serial;
> int status;
> + const struct firmware *fw;
> + const char *fw_name = "edgeport/down3.bin";
> + struct device *dev = &serial->interface->dev;
>
> /* create our private serial structure */
> edge_serial = kzalloc(sizeof(struct edgeport_serial), GFP_KERNEL);
> @@ -2393,7 +2400,17 @@ static int edge_startup(struct usb_serial *serial)
> edge_serial->serial = serial;
> usb_set_serial_data(serial, edge_serial);
>
> - status = download_fw(edge_serial);
> + status = request_firmware(&fw, fw_name, dev);
> + if (status) {
> + dev_err(&serial->interface->dev,
Why not use dev here as well?
> + "%s - Failed to load image \"%s\" err %d\n",
> + __func__, fw_name, status);
You can drop the function name here as well.
> + kfree(edge_serial);
> + return status;
> + }
> +
> + 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
