Message-ID: <20160418170120.GF29343@roeck-us.net>
Date:	Mon, 18 Apr 2016 10:01:20 -0700
From:	Guenter Roeck <linux@...ck-us.net>
To:	Alexey Klimov <klimov.linux@...il.com>
Cc:	linux-watchdog@...r.kernel.org, wim@...ana.be, oneukum@...e.com,
	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
	yury.norov@...il.com
Subject: Re: [PATCH v2] watchdog: add driver for StreamLabs USB watchdog
 device

On Mon, Apr 18, 2016 at 03:53:36AM +0100, Alexey Klimov wrote:
> This patch creates new driver that supports StreamLabs usb watchdog
> device. This device plugs into 9-pin usb header and connects to
> reset pin and reset button on common PC.
> 
> USB commands used to communicate with device were reverse
> engineered using usbmon.
> 
> Signed-off-by: Alexey Klimov <klimov.linux@...il.com>
> ---
> Changes in v2:
>  -- coding style cleanups
>  -- turn some dev_err messages to dev_dbg
>  -- reimplemented usb_streamlabs_wdt_command() to use loop
>  -- re-worked disconnect routine
>  -- rebased to 4.6-rc2, removed set_timeout method
>  -- removed braces in .options field in streamlabs_wdt_indent
>  -- mem allocation migrated to devm_kzalloc
>  -- buffer for device struct moved inside main struct
>     to avoid additional memory allocation
>  -- removed watchdog_init_timeout()
>  -- re-worked usb_streamlabs_wdt_{resume,suspend}
>  -- removed struct usb_device pointer from main driver struct
>  -- buffer preparation for communication migrated to cpu_to_le16()
>     functions, also buffer is filled in as u16 elements to
>     make this byteorder usable
>  -- added stop command in usb_streamlabs_wdt_disconnect()
>  
>  drivers/watchdog/Kconfig          |  15 ++
>  drivers/watchdog/Makefile         |   1 +
>  drivers/watchdog/streamlabs_wdt.c | 313 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 329 insertions(+)
>  create mode 100644 drivers/watchdog/streamlabs_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index fb94765..130cf54 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1766,4 +1766,19 @@ config USBPCWATCHDOG
>  
>  	  Most people will say N.
>  
> +config USB_STREAMLABS_WATCHDOG
> +	tristate "StreamLabs USB watchdog driver"
> +	depends on USB
> +	---help---
> +	  This is the driver for the USB Watchdog dongle from StreamLabs.
> +	  If you correctly connect reset pins to motherboard Reset pin and
> +	  to Reset button then this device will simply watch your kernel to make
> +	  sure it doesn't freeze, and if it does, it reboots your computer
> +	  after a certain amount of time.
> +
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called streamlabs_wdt.
> +
> +	  Most people will say N. Say yes or M if you want to use such usb device.
>  endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index feb6270..9d36929 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_WDTPCI) += wdt_pci.o
>  
>  # USB-based Watchdog Cards
>  obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o
> +obj-$(CONFIG_USB_STREAMLABS_WATCHDOG) += streamlabs_wdt.o
>  
>  # ALPHA Architecture
>  
> diff --git a/drivers/watchdog/streamlabs_wdt.c b/drivers/watchdog/streamlabs_wdt.c
> new file mode 100644
> index 0000000..3e34cd8
> --- /dev/null
> +++ b/drivers/watchdog/streamlabs_wdt.c
> @@ -0,0 +1,313 @@
> +/*
> + * StreamLabs USB Watchdog driver
> + *
> + * Copyright (c) 2016 Alexey Klimov <klimov.linux@...il.com>
> + *
> + * This program is free software; you may redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +#include <linux/watchdog.h>
> +#include <asm/byteorder.h>
> +
> +/*
> + * USB Watchdog device from Streamlabs
> + * http://www.stream-labs.com/products/devices/watchdog/
> + *
> + * USB commands have been reverse engineered using usbmon.
> + */
> +
> +#define DRIVER_AUTHOR "Alexey Klimov <klimov.linux@...il.com>"
> +#define DRIVER_DESC "StreamLabs USB watchdog driver"
> +#define DRIVER_NAME "usb_streamlabs_wdt"
> +
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
> +
> +#define USB_STREAMLABS_WATCHDOG_VENDOR	0x13c0
> +#define USB_STREAMLABS_WATCHDOG_PRODUCT	0x0011
> +
> +/*
> + * one buffer is used for communication, however transmitted message is only
> + * 32 bytes long
> + */
> +#define BUFFER_TRANSFER_LENGTH	32
> +#define BUFFER_LENGTH		64
> +#define USB_TIMEOUT		350
> +
> +#define STREAMLABS_CMD_START	0xaacc
> +#define STREAMLABS_CMD_STOP	0xbbff
> +
> +#define STREAMLABS_WDT_MIN_TIMEOUT	1
> +#define STREAMLABS_WDT_MAX_TIMEOUT	46
> +
An explanation for those min/max limits would be nice.

> +struct streamlabs_wdt {
> +	struct watchdog_device wdt_dev;
> +	struct usb_interface *intf;
> +
> +	struct mutex lock;
> +	u8 buffer[BUFFER_LENGTH];
> +};
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +
> +/*
> + * This function is used to check if watchdog actually changed
> + * its state to disabled that is reported in first two bytes of response
> + * message.
> + */
> +static int usb_streamlabs_wdt_check_stop(u16 *buf)
> +{
> +	if (buf[0] != cpu_to_le16(STREAMLABS_CMD_STOP))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int usb_streamlabs_wdt_validate_response(u8 *buf)
> +{
> +	/*
> +	 * If watchdog device understood the command it will acknowledge
> +	 * with values 1,2,3,4 at indexes 10, 11, 12, 13 in response message
> +	 * when response treated as 8bit message.
> +	 */
> +	if (buf[10] != 1 || buf[11] != 2 || buf[12] != 3 || buf[13] != 4)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static void usb_streamlabs_wdt_prepare_buf(u16 *buf, u16 cmd,
> +						unsigned long timeout_msec)
> +{
> +	/*
> +	 * remaining elements expected to be zero everytime during
> +	 * communication
> +	 */
> +	buf[0] = cpu_to_le16(cmd);
> +	buf[1] = cpu_to_le16(0x8000);
> +
> +	buf[3] = cpu_to_le16(timeout_msec);
> +	buf[5] = 0x0;
> +	buf[6] = 0x0;
> +}
> +
> +static int usb_streamlabs_wdt_command(struct watchdog_device *wdt_dev, u16 cmd)
> +{
> +	struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
> +	struct usb_device *usbdev;
> +	int retval;
> +	int size;
> +	unsigned long timeout_msec;
> +
Unnecessary empty line.

> +	int retry_counter = 10;		/* how many times to re-send stop cmd */
> +
> +	mutex_lock(&streamlabs_wdt->lock);
> +
> +	if (unlikely(!streamlabs_wdt->intf)) {
> +		mutex_unlock(&streamlabs_wdt->lock);
> +		return -ENODEV;

Here is where the goto out; would make sense for consistency.

> +	}
> +
> +	usbdev = interface_to_usbdev(streamlabs_wdt->intf);
> +	timeout_msec = wdt_dev->timeout * MSEC_PER_SEC;
> +
> +	do {
> +		usb_streamlabs_wdt_prepare_buf((u16 *) streamlabs_wdt->buffer,
> +							cmd, timeout_msec);
> +		/* send command to watchdog */
> +		retval = usb_interrupt_msg(usbdev, usb_sndintpipe(usbdev, 0x02),
> +				streamlabs_wdt->buffer, BUFFER_TRANSFER_LENGTH,
> +				&size, USB_TIMEOUT);
> +
> +		if (retval || size != BUFFER_TRANSFER_LENGTH) {
> +			dev_dbg(&streamlabs_wdt->intf->dev,
> +				"error %i when submitting interrupt msg\n",
> +									retval);
> +			retval = -EIO;

This is a bit inconsistent. The if() branch is entered even if there is
no error (resulting in a false error message), and any actual error is
overwritten with -EIO. It is customary to pass errors to the caller,
so this should be something like 
			if (retval >= 0)
				retval = -EIO;

or even better use two separate if statements and also fix the log message.

> +			goto out;

Those goto statements are really unnecessary; you can use break; instead.

> +		}
> +
> +		/* and read response from watchdog */
> +		retval = usb_interrupt_msg(usbdev, usb_rcvintpipe(usbdev, 0x81),
> +				streamlabs_wdt->buffer, BUFFER_LENGTH,
> +				&size, USB_TIMEOUT);
> +
> +		if (retval || size != BUFFER_LENGTH) {
> +			dev_dbg(&streamlabs_wdt->intf->dev,
> +				"error %i when receiving interrupt msg\n",
> +									retval);

Same here.

> +			retval = -EIO;
> +			goto out;
> +		}
> +
> +		/* check if watchdog actually acked/recognized command */
> +		retval = usb_streamlabs_wdt_validate_response(streamlabs_wdt->buffer);
> +		if (retval) {
> +			dev_dbg(&streamlabs_wdt->intf->dev,
> +					"watchdog didn't ACK command!\n");
> +			goto out;
> +		}
> +
> +		/*
> +		 * Transition from enabled to disabled state in this device
> +		 * for stop command doesn't happen immediately. Usually, 2 or 3
> +		 * (sometimes even 4) stop commands should be sent until
> +		 * watchdog answers 'I'm stopped!'.
> +		 * Retry only stop command if watchdog fails to answer correctly
> +		 * about its state. After 10 attempts go out and return error.
> +		 * We do not retry start command.
> +		 */
> +	} while (cmd != STREAMLABS_CMD_START
> +		&& usb_streamlabs_wdt_check_stop((u16 *) streamlabs_wdt->buffer)
> +		&& --retry_counter >= 0);

Looking into this closely, this means that the loop is only executed for the
stop command. This suggests that it would be better to implement the loop itself
in a separate function, and call that function from here. Effectively that would
move the mutex protection into the calling code, but you would not need the odd
conditions above and teh code would overall be much cleaner and easier to
understand.

> +
> +out:
> +	mutex_unlock(&streamlabs_wdt->lock);
> +	return retry_counter > 0 ? retval : -EIO;
> +}
> +
> +static int usb_streamlabs_wdt_start(struct watchdog_device *wdt_dev)
> +{
> +	return usb_streamlabs_wdt_command(wdt_dev, STREAMLABS_CMD_START);
> +}
> +
> +static int usb_streamlabs_wdt_stop(struct watchdog_device *wdt_dev)
> +{
> +	return usb_streamlabs_wdt_command(wdt_dev, STREAMLABS_CMD_STOP);
> +}
> +
> +static const struct watchdog_info streamlabs_wdt_ident = {
> +	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
> +	.identity	= DRIVER_NAME,
> +};
> +
> +static struct watchdog_ops usb_streamlabs_wdt_ops = {
> +	.owner	= THIS_MODULE,
> +	.start	= usb_streamlabs_wdt_start,
> +	.stop	= usb_streamlabs_wdt_stop,
> +};
> +
> +static int usb_streamlabs_wdt_probe(struct usb_interface *intf,
> +			const struct usb_device_id *id)

Please align continuation lines with '('.

> +{
> +	struct usb_device *usbdev = interface_to_usbdev(intf);
> +	struct streamlabs_wdt *streamlabs_wdt;
> +	int retval;
> +
> +	/*
> +	 * USB IDs of this device appear to be weird/unregistered. Hence, do
> +	 * an additional check on product and manufacturer.
> +	 * If there is similar device in the field with same values then
> +	 * there is stop command in probe() below that checks if the device
> +	 * behaves as a watchdog.
> +	 */
> +	if (usbdev->product && usbdev->manufacturer &&

So product == NULL || manufacturer == NULL is interpreted as acceptable,
and there is no need to check anything else in that case ?

Or should it be

	if (!usbdev->product || !usbdev->manufacturer || ...

instead ?

> +		(strncmp(usbdev->product, "USBkit", 6) != 0

!= 0 is unnecessary.

> +		|| strncmp(usbdev->manufacturer, "STREAM LABS", 11) != 0))

Same here.

> +		return -ENODEV;
> +
> +	streamlabs_wdt = devm_kzalloc(&intf->dev, sizeof(struct streamlabs_wdt),
> +								GFP_KERNEL);
> +	if (!streamlabs_wdt)
> +		return -ENOMEM;
> +
> +	mutex_init(&streamlabs_wdt->lock);
> +
> +	streamlabs_wdt->wdt_dev.info = &streamlabs_wdt_ident;
> +	streamlabs_wdt->wdt_dev.ops = &usb_streamlabs_wdt_ops;
> +	streamlabs_wdt->wdt_dev.timeout = STREAMLABS_WDT_MAX_TIMEOUT;
> +	streamlabs_wdt->wdt_dev.max_timeout = STREAMLABS_WDT_MAX_TIMEOUT;
> +	streamlabs_wdt->wdt_dev.min_timeout = STREAMLABS_WDT_MIN_TIMEOUT;
> +	streamlabs_wdt->wdt_dev.parent = &intf->dev;
> +
> +	streamlabs_wdt->intf = intf;
> +	usb_set_intfdata(intf, &streamlabs_wdt->wdt_dev);
> +	watchdog_set_drvdata(&streamlabs_wdt->wdt_dev, streamlabs_wdt);
> +	watchdog_set_nowayout(&streamlabs_wdt->wdt_dev, nowayout);
> +
> +	retval = usb_streamlabs_wdt_stop(&streamlabs_wdt->wdt_dev);
> +	if (retval)
> +		return retval;
> +
> +	retval = watchdog_register_device(&streamlabs_wdt->wdt_dev);
> +	if (retval) {
> +		dev_err(&intf->dev, "failed to register watchdog device\n");
> +		return retval;
> +	}
> +
> +	dev_info(&intf->dev, "StreamLabs USB watchdog loaded.\n");
> +
> +	return 0;
> +}
> +
> +
> +
> +static int usb_streamlabs_wdt_suspend(struct usb_interface *intf,
> +					pm_message_t message)
> +{
> +	struct streamlabs_wdt *streamlabs_wdt = usb_get_intfdata(intf);
> +
> +	if (watchdog_active(&streamlabs_wdt->wdt_dev))
> +		return usb_streamlabs_wdt_stop(&streamlabs_wdt->wdt_dev);
> +
> +	return 0;
> +}
> +
> +static int usb_streamlabs_wdt_resume(struct usb_interface *intf)
> +{
> +	struct streamlabs_wdt *streamlabs_wdt = usb_get_intfdata(intf);
> +
> +	if (watchdog_active(&streamlabs_wdt->wdt_dev))
> +		return usb_streamlabs_wdt_start(&streamlabs_wdt->wdt_dev);
> +
> +	return 0;
> +}
> +
> +static void usb_streamlabs_wdt_disconnect(struct usb_interface *intf)
> +{
> +	struct streamlabs_wdt *streamlabs_wdt = usb_get_intfdata(intf);
> +
> +	usb_streamlabs_wdt_stop(&streamlabs_wdt->wdt_dev);
> +
Strictly speaking the wdt _could_ be restarted right here.
It might be better to have a mutex-protected version of
usb_streamlabs_wdt_stop() and call it after the lock has been acquired.

> +	mutex_lock(&streamlabs_wdt->lock);
> +	usb_set_intfdata(intf, NULL);
> +	streamlabs_wdt->intf = NULL;
> +	mutex_unlock(&streamlabs_wdt->lock);
> +
> +	watchdog_unregister_device(&streamlabs_wdt->wdt_dev);
> +}
> +
> +static struct usb_device_id usb_streamlabs_wdt_device_table[] = {
> +	{ USB_DEVICE(USB_STREAMLABS_WATCHDOG_VENDOR, USB_STREAMLABS_WATCHDOG_PRODUCT) },
> +	{ }	/* Terminating entry */
> +};
> +
> +MODULE_DEVICE_TABLE(usb, usb_streamlabs_wdt_device_table);
> +
> +static struct usb_driver usb_streamlabs_wdt_driver = {
> +	.name		= DRIVER_NAME,
> +	.probe		= usb_streamlabs_wdt_probe,
> +	.disconnect	= usb_streamlabs_wdt_disconnect,
> +	.suspend	= usb_streamlabs_wdt_suspend,
> +	.resume		= usb_streamlabs_wdt_resume,
> +	.reset_resume	= usb_streamlabs_wdt_resume,
> +	.id_table	= usb_streamlabs_wdt_device_table,
> +};
> +
> +module_usb_driver(usb_streamlabs_wdt_driver);
> -- 
> 2.8.0.rc3
> 

Powered by blists - more mailing lists