Hi Johan: Again, I think I'm about ready to post a v5 patchset that addresses your comments here and elsewhere. I'll include point-by-point response notes to your comments below. Thanks! --Peter On Thu, 2015-06-11 at 12:26 +0200, Johan Hovold wrote: > On Mon, Jun 08, 2015 at 02:51:37PM -0500, Peter E. Berger wrote: > > From: "Peter E. Berger" <pberger@xxxxxxxxxxx> > > > > When using newer Edgeport devices such as the EP/416, idle ports are > > automatically bounced (disconnected and then reconnected) approximately > > every 60 seconds. This breaks programs (e.g: minicom) where idle periods > > are common, normal and expected. > > > > I confirmed with the manufacturer (Digi International) that some Edgeports > > now ship from the factory with firmware that expects periodic "heartbeat" > > queries from the driver to keep idle ports alive. This patch implements > > heartbeat support using the mechanism Digi suggested (periodically > > requesting an I2C descriptor address) that appears effective on Edgeports > > running the newer firmware (that require it) and benign on Edgeport > > devices running older firmware. Since we know that Edgeport firmware > > version 4.80 (the version distributed in /lib/firmware/down3.bin and used > > for Edgeports that are either running still older versions or have no > > onboard non-volatile firmware image) does not require heartbeat support, > > this patch only schedules heartbeats on devices running firmware versions > > newer than 4.80. > > > > Signed-off-by: Peter E. Berger <pberger@xxxxxxxxxxx> > > --- > > drivers/usb/serial/io_ti.c | 81 +++++++++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 80 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c > > index 00e1034..4dd3648 100644 > > --- a/drivers/usb/serial/io_ti.c > > +++ b/drivers/usb/serial/io_ti.c > > @@ -101,6 +101,7 @@ struct edgeport_serial { > > struct mutex es_lock; > > int num_ports_open; > > struct usb_serial *serial; > > + struct delayed_work heartbeat_work; > > int fw_version; > > }; > > > > @@ -206,6 +207,18 @@ static void edge_send(struct usb_serial_port *port, struct tty_struct *tty); > > static int edge_create_sysfs_attrs(struct usb_serial_port *port); > > static int edge_remove_sysfs_attrs(struct usb_serial_port *port); > > > > +/* > > + * Some release of Edgeport firmware "down3.bin" after version 4.80 > > + * introduced code to automatically disconnect idle or closed devices > > + * after periods of inactivity, typically ~60 seconds. Digi International > > + * Tech Support suggests adding driver "heartbeat" code to reset the > > + * firmware timer by requesting a descriptor record every 15 seconds, > > + * which should be effective with newer firmware versions that require > > + * it, and benign with older versions that do not. In practice 40 seconds > > + * seems often enough. > > + */ > > +#define FW_HEARTBEAT_VERSION_CUTOFF ((4<<8) + 80) > > Add spaces around << Done. > > > +#define FW_HEARTBEAT_SECS 40 > > > > static int ti_vread_sync(struct usb_device *dev, __u8 request, > > __u16 value, __u16 index, u8 *data, int size) > > @@ -2351,6 +2364,28 @@ static void edge_break(struct tty_struct *tty, int break_state) > > __func__, status); > > } > > > > +static void edge_heartbeat_work(struct work_struct *taskp) > > Call the argument "work" as well. Done. > > > +{ > > + struct edgeport_serial *serial; > > + struct ti_i2c_desc *rom_desc; > > + > > + serial = container_of(taskp, struct edgeport_serial, > > + heartbeat_work.work); > > + > > + rom_desc = kmalloc(sizeof(*rom_desc), GFP_KERNEL); > > + > > + /* Descriptor address request is enough to reset the firmware timer */ > > + if (!rom_desc || !get_descriptor_addr(serial, I2C_DESC_TYPE_ION, > > + rom_desc)) { > > + dev_warn(&serial->serial->interface->dev, > > + "%s - Incomplete heartbeat.", __func__); > > Missing '\n' on error message (and skip the period). Done. > > Use dev_err. Done. > > > + } > > + kfree(rom_desc); > > + > > + schedule_delayed_work(&serial->heartbeat_work, FW_HEARTBEAT_SECS * HZ); > > + > > Stray newline. Done. > > > +} > > + > > static int edge_startup(struct usb_serial *serial) > > { > > struct edgeport_serial *edge_serial; > > @@ -2362,6 +2397,8 @@ static int edge_startup(struct usb_serial *serial) > > u8 fw_major_version; > > u8 fw_minor_version; > > > > + dev_dbg(&serial->interface->dev, "%s: serial:%s\n", __func__, > > + serial->dev->serial); > > Drop this one. Done. > > > /* create our private serial structure */ > > edge_serial = kzalloc(sizeof(struct edgeport_serial), GFP_KERNEL); > > if (!edge_serial) > > @@ -2392,6 +2429,13 @@ static int edge_startup(struct usb_serial *serial) > > return status; > > } > > > > + if (edge_serial->fw_version > FW_HEARTBEAT_VERSION_CUTOFF) { > > + INIT_DELAYED_WORK(&edge_serial->heartbeat_work, > > + edge_heartbeat_work); > > Init unconditionally. Done. > > > + schedule_delayed_work(&edge_serial->heartbeat_work, > > + FW_HEARTBEAT_SECS * HZ); > > But only schedule if > cutoff-version, possibly refactor that to an > inline helper function edge_heartbeat_reschedule(). Done. > > > + } > > + > > return 0; > > } > > > > @@ -2401,7 +2445,11 @@ static void edge_disconnect(struct usb_serial *serial) > > > > static void edge_release(struct usb_serial *serial) > > { > > - kfree(usb_get_serial_data(serial)); > > + struct edgeport_serial *edge_serial = usb_get_serial_data(serial); > > + > > + if (edge_serial->fw_version > FW_HEARTBEAT_VERSION_CUTOFF) > > + cancel_delayed_work_sync(&edge_serial->heartbeat_work); > > Cancel unconditionally. Done. > > > + kfree(edge_serial); > > } > > > > static int edge_port_probe(struct usb_serial_port *port) > > @@ -2505,6 +2553,29 @@ static int edge_remove_sysfs_attrs(struct usb_serial_port *port) > > return 0; > > } > > > > +#ifdef CONFIG_PM > > +static int edge_suspend(struct usb_serial *serial, pm_message_t message) > > +{ > > + struct edgeport_serial *edge_serial = usb_get_serial_data(serial); > > + > > + if (edge_serial->fw_version > FW_HEARTBEAT_VERSION_CUTOFF) > > + cancel_delayed_work_sync(&edge_serial->heartbeat_work); > > Same here. Done. > > + > > + return 0; > > +} > > + > > +static int edge_resume(struct usb_serial *serial) > > +{ > > + struct edgeport_serial *edge_serial = usb_get_serial_data(serial); > > + > > + if (edge_serial->fw_version > FW_HEARTBEAT_VERSION_CUTOFF) { > > + schedule_delayed_work(&edge_serial->heartbeat_work, > > + FW_HEARTBEAT_SECS * HZ); > > + } > > Use helper here as well. Done. > > > + > > + return 0; > > +} > > +#endif > > 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