On Thu, 2015-06-11 at 11:10 +0200, Johan Hovold wrote:
> 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).
Ah! Good idea. I've reworked the 2 patches into 10 separate
incremental patches, which also include fixes to address your other
review comments (below and from your other separate messages).
>
> > > > 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.
Understood. I expanded the comment block in edge_startup() (where the
firmware version information is decoded) to include information about
the currently unused "build_number".
Thanks.
--Peter
>
> 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
