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?
>
> 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?
>
> > 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
> > @@ -188,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 */
> > @@ -213,6 +209,25 @@ static int edge_remove_sysfs_attrs(struct usb_serial_port *port);
> > /* Newer Edgeport firmware disconnects ports after periods of inactivity */
> > #define EP_HEARTBEAT_SECS 15
> >
> > +int wrap_request_firmware(const struct firmware **fw, struct device *dev,
> > + __u8 *MajorVersion, __u8 *MinorVersion, __u16 *BuildNumber)
>
> Is it really necessary to wrap the request call? Looks like what you
> really want is a edge_fw_get_version() helper. And I'm not sure even
> that is really needed to fix the bug at hand (we want to keep fixes
> minimal).
Excellent point. I was trying to make minimal changes to the
pre-exisiting code flow, but your suggestion here and below led me to
what I think is a much better overall approach: I have removed
wrap_request_firmware() and its three invocations, and reorganized the
code to instead call request_firmware() in edge_startup(); pass the
firmware image to download_fw() and build_i2c_fw_hdr(); and release it
on return from download_fw().
>
> I know this driver wasn't a good example, but don't use CamelCase.
I have dropped the use of CamelCase in all the patched code (except in
references to struct ti_i2c_firmware_rec from io_usbvend.h).
>
> Also drop __ from the types here and elsewhere.
I have dropped __ from types in all the patched code.
>
> > +{
> > + const char *fw_name = "edgeport/down3.bin";
> > + int err;
> > +
> > + err = request_firmware(fw, fw_name, dev);
> > + if (err) {
> > + dev_dbg(dev, "%s - Failed to load image \"%s\" err %d\n",
> > + __func__, fw_name, err);
> > + } else {
>
> Return err here and skip the else clause.
Done.
>
> > + /* 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).
>
> > + }
> > + return err;
>
> return 0
Done. (Though this code has moved to edge_startup().)
>
> > +}
> > +
> > static int ti_vread_sync(struct usb_device *dev, __u8 request,
> > __u16 value, __u16 index, u8 *data, int size)
> > {
> > @@ -235,10 +250,13 @@ static int ti_vsend_sync(struct usb_device *dev, __u8 request,
> > __u16 value, __u16 index, u8 *data, int size)
> > {
> > int status;
> > + int timeout = 1000; /* Timeout in msecs */
> >
> > + if (request == UMPC_COPY_DNLD_TO_I2C) /* Downloads take longer */
> > + timeout = 10000;
> > status = usb_control_msg(dev, usb_sndctrlpipe(dev, 0), request,
> > (USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT),
> > - value, index, data, size, 1000);
> > + value, index, data, size, timeout);
> > if (status < 0)
> > return status;
> > if (status != size) {
> > @@ -762,7 +780,9 @@ static int build_i2c_fw_hdr(__u8 *header, struct device *dev)
> > 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";
> > + __u8 OperationalMajorVersion = 0;
> > + __u8 OperationalMinorVersion = 0;
> > + __u16 OperationalBuildNumber = 0;
>
> CamelCase could be ok here in order to minimise diff. Could be fixed in
> a follow up patch.
As part of the reworked code that now passes a firmware image pointer
into download_fw(), I think CamelCase variables now seem out of place
here, so I have dropped them. Let me know if you disagree.
>
> [...]
>
> > /* This routine is entered by both the BOOT mode and the Download mode
> > * We can determine which code is running by the reading the config
> > @@ -1046,6 +1065,19 @@ static int download_fw(struct edgeport_serial *serial)
> > return status;
> > }
> >
> > + /* Get version numbers for on-disk fw */
> > + if (wrap_request_firmware(&fw, dev,
> > + &OperationalMajorVersion,
> > + &OperationalMinorVersion,
> > + &OperationalBuildNumber)) {
> > + dev_dbg(dev, "%s - FW version query failed. Setting version to 0.",
> > + __func__);
> > + OperationalMajorVersion = 0;
> > + OperationalMinorVersion = 0;
> > + OperationalBuildNumber = 0;
>
> No need to reset.
Done. (This code block is no longer needed, so I removed it.)
>
> > + }
> > + release_firmware(fw);
> > +
>
> This looks odd, loading the firmware only to check the version. Why not
> use this image later if needed?
>
> I know this code is extremely messy, but it should be doable.
Agreed on both points! Your comments pointed me in a much better
direction: I dropped wrap_request_firmware() and its three invocations
altogether, and have rearranged things to instead call
request_firmware() from edge_startup(); pass the image to download_fw()
and build_i2c_fw_hdr(); and release it on return from download_fw().
Next I'll prepare, check and send a v4 patchset for your consideration.
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
