Re: [PATCH v3 2/2] USB: io_ti: Fix Edgeport firmware download code

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Jun 15, 2015 at 09:56:06AM -0500, Peter Berger wrote:
> 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).

That's a bit excessive. In this case it seems that a series of three or
four patches would be appropriate. No need to do incremental changes to
your own changes (e.g. after patch review). The guideline is one logical
change per patch (e.g. fix firmware download timeout, fix firmware
revision handling, etc).

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



[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux