On Wed, 2015-07-15 at 11:36 +0200, Johan Hovold wrote:
> On Sun, Jun 28, 2015 at 01:28:20PM -0500, Peter E. Berger wrote:
> > From: "Peter E. Berger" <pberger@xxxxxxxxxxx>
> >
> > Do what we can to verify that the driver's firmware image is valid
> > (before attempting to download it to the Edgeport) by adding a new
> > function, check_fw_sanity(), and a call to it in in download_fw().
> > Also add an fw == NULL check in edge_startup().
> >
> > Note: It looks like some Edgeports (models like the EP/416 with on-board
> > E2PROM) may be able to function even if the on-disk firmware image is
> > bad or missing, iff their local E2PROM versions are valid. But most
> > Edgeport models (I've tried EP/1 and EP/8) do not appear to have this
> > capability and they always rely on the on-disk firmware image.
> >
> > I tested an implementation that calls the new check_fw_sanity()
> > function at the top of download_fw() and, rather than simply returning
> > an error if the firmware image is bad or missing, it saves the result
> > and defers the decision until later when it may find that it is running
> > on a E2PROM-equipped device with a valid image. But I think this is
> > messier than it is worth (adding still more messiness to the already
> > very messy download_fw()) for such a marginal possible benefit. So, at
> > least for now, I have chosen the much simpler approach of returning an
> > error whenever edge_startup() fails to load an on-disk firmware image, or
> > check_fw_sanity() indicates that it is unusable.
> >
> > Signed-off-by: Peter E. Berger <pberger@xxxxxxxxxxx>
> > ---
> > drivers/usb/serial/io_ti.c | 63 +++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 62 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> > index 4492c17..7c5f6fd 100644
> > --- a/drivers/usb/serial/io_ti.c
> > +++ b/drivers/usb/serial/io_ti.c
> > @@ -35,6 +35,7 @@
> > #include <linux/uaccess.h>
> > #include <linux/usb.h>
> > #include <linux/usb/serial.h>
> > +#include <asm/unaligned.h>
> >
> > #include "io_16654.h"
> > #include "io_usbvend.h"
> > @@ -909,6 +910,64 @@ static int ti_cpu_rev(struct edge_ti_manuf_descriptor *desc)
> > return TI_GET_CPU_REVISION(desc->CpuRev_BoardRev);
> > }
> >
> > +/*
> > + * Edgeport firmware images start with a 7-byte header:
> > + *
> > + * u8 major_version;
> > + * u8 minor_version;
> > + * le16 build_number;
> > + * le16 length;
> > + * u8 checksum;
> > + *
> > + * "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.
> > + *
> > + */
> > +static int check_fw_sanity(struct edgeport_serial *serial,
> > + const struct firmware *fw)
> > +{
> > +#define FW_HEADER_SIZE 7
> > +#define FW_LENGTH_OFFSET 4
> > +#define FW_CHECKSUM_OFFSET 6
>
> Could you move this to the top of the file with the other defines?
I used your next suggestion to create a new struct edgeport_fw_hdr, so I
was able to drop these #defines altogether.
>
> Why not use a struct edgeport_fw_hdr instead? That could be reused when
> parsing the header in download_fw as well.
Great suggestion. Done.
> > + u16 length_data;
> > + u16 length_total;
> > + u8 checksum;
> > + int checksum_new = 0;
> > + int pos;
> > + struct device *dev = &serial->serial->interface->dev;
> > +
> > + if (fw->size < FW_HEADER_SIZE) {
> > + dev_err(dev, "%s - Incomplete fw header\n", __func__);
>
> You can drop the function name from these error messages that are
> already descriptive enough.
Done.
>
> > + return 1;
>
> return -EINVAL on errors
Done.
>
> > + }
> > +
> > + length_data = get_unaligned_le16(&fw->data[FW_LENGTH_OFFSET]);
> > + checksum = fw->data[FW_CHECKSUM_OFFSET];
> > + length_total = length_data + FW_HEADER_SIZE;
> > +
> > + if (fw->size != length_total) {
> > + dev_err(dev, "%s - Bad fw size (Expected:%d, Got:%d)\n",
> > + __func__, length_total, (int)fw->size);
>
> Use %u and %zu and drop the cast.
Done.
> Space after ':'?
Done.
>
> > + return 1;
> > + }
> > +
> > + for (pos = FW_HEADER_SIZE; pos < length_total; ++pos)
>
> Use pos < fw->size
Done.
>
> > + checksum_new = (checksum_new + fw->data[pos]) & 0xFF;
> > +
> > + if (checksum_new != checksum) {
> > + dev_err(dev, "%s - Bad fw checksum (Expected:0x%x, Got:0x%x)\n",
> > + __func__, checksum, checksum_new);
> > + return 1;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * DownloadTIFirmware - Download run-time operating firmware to the TI5052
> > *
> > @@ -928,6 +987,8 @@ static int download_fw(struct edgeport_serial *serial,
> > u8 fw_major_version;
> > u8 fw_minor_version;
> >
> > + if (check_fw_sanity(serial, fw))
> > + return -EINVAL;
>
> I'd add a newline here.
Done.
>
> > fw_major_version = fw->data[0];
> > fw_minor_version = fw->data[1];
> > /* If on-board version is newer, "fw_version" will be updated below. */
> > @@ -2377,7 +2438,7 @@ static int edge_startup(struct usb_serial *serial)
> > usb_set_serial_data(serial, edge_serial);
> >
> > status = request_firmware(&fw, fw_name, dev);
> > - if (status) {
> > + if (status || fw == NULL) {
>
> This isn't needed. I had the interface wrong; fw will never be updated
> on errors. Just checking status is enough.
Done.
Thanks,
--Peter
>
> > dev_err(&serial->interface->dev,
> > "%s - Failed to load image \"%s\" err %d\n",
> > __func__, fw_name, 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
