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" <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(). > > 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 <pberger@xxxxxxxxxxx> > > --- > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
