On Mon, Jun 08, 2015 at 01:32:13PM -0500, Peter Berger wrote:
> On Fri, 2015-05-22 at 17:21 +0200, Johan Hovold wrote:
> > On Fri, May 15, 2015 at 12:09:53AM -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 Edgeports now
> > > ship from the factory with firmware that expects periodic "heartbeat"
> > > queries from the driver to keep ports alive. This patch implements
> > > heartbeat support using the mechanism Digi suggested (requesting an
> > > I2C descriptor address every 15 seconds) that appears effective on
> > > Edgeports running the newer firmware (that require it) and benign on
> > > Edgeport devices running older firmware.
> > >
> > > Signed-off-by: Peter E. Berger <pberger@xxxxxxxxxxx>
> > You say you've tested this with older firmware, but I'd still prefer if
> > we only enable this when actually needed. Just set a flag during probe
> > if the firmware requires this heartbeat and only start it if needed.
>
> Good suggestion. I've reorganized the code to schedule heartbeats only
> if the firmware version is newer than 4.80. We know that the heartbeat
> requirement was added after version 4.80 (which is the version
> distributed in /lib/firmware/edgeport/down3.bin), but not precisely
> when. The two other versions I have seen (5.32 and 5.38) both require
> heartbeats. I asked Digi which version first introduced this
> requirement but have not yet heard back. Meanwhile, since the heartbeat
> code uses Digi's suggested mechanism that is both innocuous on older
> firmware that do not require it (I tested this on 4.80), and effective
> on newer ones like 5.38 that do, I think using 4.80 as the cutoff
> version is the best we can do. Sound OK?
That sounds good.
> > Just to clarify, is this hearbeat needed also when no port is open?
>
> Yes, the heartbeat code is required to keep idle Edgeport devices from
> bouncing (disconnecting/reconnecting) even when no ports are open.
Ok. Perhaps this should be documented in the code unless it already is
in v4.
> > > ---
> > > drivers/usb/serial/io_ti.c | 68 ++++++++++++++++++++++++++++++++++++++++++++++
> > > 1 file changed, 68 insertions(+)
> > >
> > > diff --git a/drivers/usb/serial/io_ti.c b/drivers/usb/serial/io_ti.c
> > > index ddbb8fe..1db929b 100644
> > > --- a/drivers/usb/serial/io_ti.c
> > > +++ b/drivers/usb/serial/io_ti.c
> > > +#define EP_HEARTBEAT_SECS 15
> >
> > Could this interval be increased from somewhat (you mention 60 seconds
> > above)?
>
> Digi originally suggested 15 seconds, but in practice 40 seconds seems
> to work on the devices I have tested here. I have asked Digi if 40
> seconds will be safe on all Edgeport devices, but have not yet heard
> back. Meanwhile, I have set this to 40 seconds for the v4 patch. Sound
> OK?
Yes, we can always decrease it if anyone experiences disconnect issues,
or if Digi recommends a lower interval.
> > > @@ -2373,11 +2376,33 @@ static void edge_break(struct tty_struct *tty, int break_state)
> > > __func__, status);
> > > }
> > >
> > > +static void heartbeat(struct work_struct *taskp)
> >
> > Rename edge_heartbeat_work.
>
> Done.
>
> >
> > > +{
> > > + struct edgeport_serial *serial;
> > > + struct ti_i2c_desc rom_desc;
> >
> > This one cannot be allocated om the stack as it is eventually used for
> > DMA. Use kmalloc.
>
> Done.
>
> >
> > > +
> > > + serial = container_of(taskp, struct edgeport_serial,
> > > + heartbeat_task.work);
> > > +
> > > + dev_dbg(&serial->serial->dev->dev, "%s - serial:%s",
> > > + __func__, serial->serial->dev->serial);
> >
> > Just remove this one.
>
> Done. (Though, since devices like the 16-port EP/416 present as eight
> separate 2-port devices, and do not always probe in the same order, it
> is very useful during debugging to have the individual device serial
> numbers available via dev_dbg() to see, for example: if the heartbeats
> are being scheduled for the correct devices. But I can easily add this
> back in my development/testing copies as needed.)
You should still be able to rely on the USB interface name, which all
debug statement should include (e.g. "usb 1-1.3.1").
> > > +
> > > + /* Descriptor address request is enough to reset the firmware timer */
> > > + if (!get_descriptor_addr(serial, I2C_DESC_TYPE_ION, &rom_desc))
> > > + dev_warn(&serial->serial->dev->dev,
> > > + "%s - Incomplete heartbeat.", __func__);
> >
> > Use serial->serial->interface->dev for all messages throughout (won't
> > mention again).
>
> Done. (In all the patched code.)
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
