Re: [PATCH v7 2/4] USB: io_ti: Move request_firmware() calls out of download_fw()
On Wed, 2015-07-15 at 11:21 +0200, Johan Hovold wrote: Hi Johan: Thanks for another thoughtful and helpful review. I think I have addressed all the issues you noted and have a v8 patchset just about ready to submit. First I'll respond to each of the specific points here and in your other v7 review emails to make sure I have not missed anything. On Sun, Jun 28, 2015 at 01:28:19PM -0500, Peter E. Berger wrote: From: Peter E. Berger pber...@brimson.com 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(). This looks good now apart from one small issue I mention below. Done (see below). Could you also please rename the patch summary (Subject) to something more descriptive such as USB: io_ti: fix firmware version handling or similar? Done. Signed-off-by: Peter E. Berger pber...@brimson.com --- drivers/usb/serial/io_ti.c | 91 +- 1 file changed, 42 insertions(+), 49 deletions(-) diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c index 69378a7..4492c17 100644 --- a/drivers/usb/serial/io_ti.c +++ b/drivers/usb/serial/io_ti.c @@ -943,6 +925,13 @@ static int download_fw(struct edgeport_serial *serial) struct usb_interface_descriptor *interface; int download_cur_ver; int download_new_ver; + u8 fw_major_version; + u8 fw_minor_version; + I think you should add minimal sanity checks on the firmware length here as part of this patch. That is, make sure that fw-size = 4, or if you prefer, fw-size = FW_HEADER_SIZE (i.e. 8). Done (using sizeof(struct edgeport_fw_hdr)). + fw_major_version = fw-data[0]; + fw_minor_version = fw-data[1]; 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
Re: [PATCH v7 2/4] USB: io_ti: Move request_firmware() calls out of download_fw()
On Sun, Jun 28, 2015 at 01:28:19PM -0500, Peter E. Berger wrote: From: Peter E. Berger pber...@brimson.com 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(). This looks good now apart from one small issue I mention below. Could you also please rename the patch summary (Subject) to something more descriptive such as USB: io_ti: fix firmware version handling or similar? Signed-off-by: Peter E. Berger pber...@brimson.com --- drivers/usb/serial/io_ti.c | 91 +- 1 file changed, 42 insertions(+), 49 deletions(-) diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c index 69378a7..4492c17 100644 --- a/drivers/usb/serial/io_ti.c +++ b/drivers/usb/serial/io_ti.c @@ -943,6 +925,13 @@ static int download_fw(struct edgeport_serial *serial) struct usb_interface_descriptor *interface; int download_cur_ver; int download_new_ver; + u8 fw_major_version; + u8 fw_minor_version; + I think you should add minimal sanity checks on the firmware length here as part of this patch. That is, make sure that fw-size = 4, or if you prefer, fw-size = FW_HEADER_SIZE (i.e. 8). + fw_major_version = fw-data[0]; + fw_minor_version = fw-data[1]; 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
[PATCH v7 2/4] USB: io_ti: Move request_firmware() calls out of download_fw()
From: Peter E. Berger pber...@brimson.com
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 pber...@brimson.com
---
drivers/usb/serial/io_ti.c | 91 +-
1 file changed, 42 insertions(+), 49 deletions(-)
diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
index 69378a7..4492c17 100644
--- a/drivers/usb/serial/io_ti.c
+++ b/drivers/usb/serial/io_ti.c
@@ -101,6 +101,7 @@ struct edgeport_serial {
struct mutex es_lock;
int num_ports_open;
struct usb_serial *serial;
+ int fw_version;
};
@@ -187,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 */
@@ -751,18 +748,17 @@ exit:
}
/* Build firmware header used for firmware update */
-static int build_i2c_fw_hdr(__u8 *header, struct device *dev)
+static int build_i2c_fw_hdr(u8 *header, struct device *dev,
+ const struct firmware *fw,
+ u8 fw_major_version, u8 fw_minor_version)
{
__u8 *buffer;
int buffer_size;
int i;
- int err;
__u8 cs = 0;
struct ti_i2c_desc *i2c_header;
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;
/* In order to update the I2C firmware we must change the type 2 record
* to type 0xF2. This will force the UMP to come up in Boot Mode.
@@ -785,24 +781,11 @@ static int build_i2c_fw_hdr(__u8 *header, struct device
*dev)
// Set entire image of 0xffs
memset(buffer, 0xff, buffer_size);
- err = request_firmware(fw, fw_name, dev);
- if (err) {
- dev_err(dev, Failed to load image \%s\ err %d\n,
- fw_name, err);
- kfree(buffer);
- return err;
- }
-
- /* Save Download Version Number */
- OperationalMajorVersion = fw-data[0];
- OperationalMinorVersion = fw-data[1];
- OperationalBuildNumber = fw-data[2] | (fw-data[3] 8);
-
/* Copy version number into firmware record */
firmware_rec = (struct ti_i2c_firmware_rec *)buffer;
- firmware_rec-Ver_Major = OperationalMajorVersion;
- firmware_rec-Ver_Minor = OperationalMinorVersion;
+ firmware_rec-Ver_Major = fw_major_version;
+ firmware_rec-Ver_Minor = fw_minor_version;
/* Pointer to fw_down memory image */
img_header = (struct ti_i2c_image_header *)fw-data[4];
@@ -811,8 +794,6 @@ static int build_i2c_fw_hdr(__u8 *header, struct device
*dev)
fw-data[4 + sizeof(struct ti_i2c_image_header)],
le16_to_cpu(img_header-Length));
- release_firmware(fw);
-
for (i=0; i buffer_size; i++) {
cs = (__u8)(cs + buffer[i]);
}
@@ -826,8 +807,8 @@ static int build_i2c_fw_hdr(__u8 *header, struct device
*dev)
i2c_header-Type= I2C_DESC_TYPE_FIRMWARE_BLANK;
i2c_header-Size= cpu_to_le16(buffer_size);
i2c_header-CheckSum= cs;
- firmware_rec-Ver_Major = OperationalMajorVersion;
- firmware_rec-Ver_Minor = OperationalMinorVersion;
+ firmware_rec-Ver_Major = fw_major_version;
+ firmware_rec-Ver_Minor = fw_minor_version;
return 0;
}
@@ -934,7 +915,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 +925,13 @@ static int download_fw(struct edgeport_serial *serial)
struct usb_interface_descriptor *interface;
int download_cur_ver;
int
