Re: [PATCH v4 2/2] USB: io_ti: Add heartbeat to keep idle Edgeport ports from disconnecting

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

 



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



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

  Powered by Linux