On Mon, Jun 08, 2015 at 02:36:36PM -0500, Peter Berger wrote: > On Fri, 2015-05-22 at 18:22 +0200, Johan Hovold wrote: > > On Fri, May 15, 2015 at 12:09:54AM -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) and using an insufficient timeout in ti_vsend_sync() > > > when doing UMPC_COPY_DNLD_TO_I2C operations. With this patch, firmware > > > downloads work as expected: if the on disk firmware version is newer > > > than that on the device, it will be downloaded. > > > > Thanks for fixing this. > > > > We should probably consider backporting some of this to stable as well. > > I am willing to start this process. Shall I wait until the patches are > first accepted here, or start this right away? I think the individual fixes should apply to older kernels without much extra effort. So as long a you try to keep individual fixes minimal (and with one fix per patch) the stable maintainers will usually take care of the backporting. > > But if you're fixing several independent issues, these should go in > > separate patches if possible. > > I suggest that these two patches are closely enough related (especially > now that the heartbeat patch uses the firmware version number that is > decoded in the "firmware download" patch) that they should be considered > together in a patchset. Do you agree or should I separate them? That's not what I meant. My point was that you may want to break this patch up into several patches that can be reviewed and backported individually (they may still depend on earlier patches in the series if necessary). > > > Signed-off-by: Peter E. Berger <pberger@xxxxxxxxxxx> > > > --- > > > drivers/usb/serial/io_ti.c | 72 ++++++++++++++++++++++++++++++++++------------ > > > 1 file changed, 53 insertions(+), 19 deletions(-) > > > > > > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c > > > index 1db929b..7e40fbb 100644 > > > --- a/drivers/usb/serial/io_ti.c > > > +++ b/drivers/usb/serial/io_ti.c > > > + /* Save Download Version Number */ > > > + *MajorVersion = (*fw)->data[0]; > > > + *MinorVersion = (*fw)->data[1]; > > > + *BuildNumber = le16_to_cpup((__le16 *)&(*fw)->data[2]); > > > > Use get_unaligned_le16(). > > On investigation I noticed that BuildNumber and OperationalBuildNumber > are decoded but not actually used anywhere in the driver, so I dropped > all references to them, including this one. Also, I asked Digi if > BuildNumber has ever been set in their distributed firmware images (it > is set to 0 in the three firmware images -- 4.80, 5.32 and 5.38 -- that > I have seen). Ok, but perhaps you should at least keep a comment in the code about what those two bytes are supposed to contain. Or just add a debug statement that includes the supposed build number. 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
