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.
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).
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.
Just to clarify, is this hearbeat needed also when no port is 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
> };
>
>
> @@ -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).
> +#define EP_HEARTBEAT_SECS 15
Could this interval be increased from somewhat (you mention 60 seconds
above)?
> 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.
> +{
> + 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.
> +
> + 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.
> +
> + /* 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).
Add braces to to if-block for readability.
> +
> + schedule_delayed_work(&serial->heartbeat_task, HZ * EP_HEARTBEAT_SECS);
Use EP_HEARTBEAT_SECS * HZ throughout.
> +
> +}
> +
> 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).
> 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.
> + cancel_delayed_work_sync(&edge_serial->heartbeat_task);
Add newline.
> + 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.
> +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.
> + 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
