On Sun, Jun 28, 2015 at 01:28:20PM -0500, Peter E. Berger wrote:
> From: "Peter E. Berger" <pber...@brimson.com>
>
> 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 <pber...@brimson.com>
> ---
> 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?
Why not use a struct edgeport_fw_hdr instead? That could be reused when
parsing the header in download_fw as well.
> + 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.
> + return 1;
return -EINVAL on errors
> + }
> +
> + 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.
Space after ':'?
> + return 1;
> + }
> +
> + for (pos = FW_HEADER_SIZE; pos < length_total; ++pos)
Use pos < fw->size
> + 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.
> 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.
> 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
