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>
>
> First of all, thanks for the patch.
Thanks for the great comments! I have a v4 patchset almost ready to
submit, which I think addresses them all. I'll describe my changes in
the context of your comments below and in your separate comments on the
second patch, then plan to submit a v4 patchset for your review.
>
> You should always run your patches through checkpatch before submitting,
> it would have let you know that there are some white space issues below
> (I will not comment on them further).
Done (and my apologies for missing that step before.)
>
> 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?
>
> 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.
>
> > ---
> > 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
> > @@ -101,6 +101,7 @@ struct edgeport_serial {
> > struct mutex es_lock;
> > int num_ports_open;
> > struct usb_serial *serial;
> > + struct delayed_work heartbeat_task;
>
> Rename heartbeat_work
Done.
>
> > };
> >
> >
> > @@ -209,6 +210,8 @@ 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);
> >
> > +/* Newer Edgeport firmware disconnects ports after periods of inactivity */
>
> You should expand this comment a bit (e.g. explain what
> EP_HEARTBEAT_SECS is).
j
I renamed EP_HEARTBEAT_SECS to FW_HEARTBEAT_SECS and expanded
explanatory comment.
>
> > +#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?
>
> > static int ti_vread_sync(struct usb_device *dev, __u8 request,
> > __u16 value, __u16 index, u8 *data, int size)
> > @@ -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.)
>
> > +
> > + /* 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.)
>
> Add braces to to if-block for readability.
Done.
>
> > +
> > + schedule_delayed_work(&serial->heartbeat_task, HZ * EP_HEARTBEAT_SECS);
>
> Use EP_HEARTBEAT_SECS * HZ throughout.
Done. (Though I renamed EP_HEARTBEAT_SECS to FW_HEARTBEAT_SECS, which
better matches its use.)
>
> > +
> > +}
> > +
> > static int edge_startup(struct usb_serial *serial)
> > {
> > struct edgeport_serial *edge_serial;
> > int status;
> >
> > + dev_dbg(&serial->dev->dev, "%s: serial:%s\n", __func__,
> > + serial->dev->serial);
> > /* create our private serial structure */
> > edge_serial = kzalloc(sizeof(struct edgeport_serial), GFP_KERNEL);
> > if (!edge_serial)
> > @@ -2393,6 +2418,10 @@ static int edge_startup(struct usb_serial *serial)
> > return status;
> > }
> >
> > + INIT_DELAYED_WORK(&edge_serial->heartbeat_task, heartbeat);
> > + schedule_delayed_work(&edge_serial->heartbeat_task,
> > + HZ * EP_HEARTBEAT_SECS);
> > +
> > return 0;
> > }
> >
> > @@ -2458,8 +2487,11 @@ err:
> > static int edge_port_remove(struct usb_serial_port *port)
> > {
> > struct edgeport_port *edge_port;
> > + struct edgeport_serial *edge_serial;
> > edge_port = usb_get_serial_port_data(port);
> > + edge_serial = edge_port->edge_serial;
> > + cancel_delayed_work_sync(&edge_serial->heartbeat_task);
>
> This should be done in edge_remove (you have one heartbeat work per
> interface, not per port).
Done. (In edge_release.)
>
> > edge_remove_sysfs_attrs(port);
> > kfree(edge_port);
> >
> > @@ -2506,6 +2538,34 @@ 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);
> > +
> > + dev_dbg(&serial->dev->dev, "%s: serial:%s\n", __func__,
> > + serial->dev->serial);
>
> Remove.
Done.
>
> > + cancel_delayed_work_sync(&edge_serial->heartbeat_task);
>
> Add newline.
Done.
>
> > + return 0;
> > +}
> > +
> > +/*
> > + * On some motherboards everything is re-enumerated on resume, so the
> > + * devices are disconnected, re-probed and then re-initialized (including
> > + * scheduling the heartbeat_task) via edge_startup() rather than here.
> > + */
>
> Comment not needed, this is (or should be) common knowledge. :) Either
> way, resume should match suspend.
Done: Removed the comment.
>
> > +static int edge_resume(struct usb_serial *serial)
> > +{
> > + struct edgeport_serial *edge_serial = usb_get_serial_data(serial);
> > +
> > + dev_dbg(&serial->dev->dev, "%s: serial:%s\n", __func__,
> > + serial->dev->serial);
>
> Drop.
Done.
Next I'll respond similarly describing how I have responded to your
comments on my v3 2/2 patch. Then I'll plan to submit the v4 patch for
your review.
Thanks.
--Peter
>
> > + schedule_delayed_work(&edge_serial->heartbeat_task,
> > + HZ * EP_HEARTBEAT_SECS);
> > + 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
