On Thu, 2015-07-30 at 15:44 +0200, Johan Hovold wrote:
> 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.
I'm on the road today, but I'll plan to make this and the other changes
that you and Sergei suggest when I get back tonight, so I hope to have a
v9 patchset to you by tomorrow. At first glance all look easy for me to
implement, but I have a question about one below.
>
> > + 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.
Hmmm... The fw->size refers to the "size_t size" element of struct
firmware (not to an element of edgeport_fw_hdr). Are you saying that I
should change this to "if (le16_to_cpu(fw->size) < sizeof..." or did I
mistake your meaning?
I'll work on this and the other changes when I get back tonight and hope
to have a new patchset for you by tomorrow.
Thanks.
--Peter
>
> > + 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
