watchdog: add driver for StreamLabs USB watchdog device

PATCH Superseded Alexey Klimov 3 participants 7 messages Apr 18, 2016 – Apr 19, 2016 Watchdog device drivers

Share & tools

Alexey Klimov

author

2016-04-18 02:53 UTC #0 lore raw

PATCH v2 1/1 watchdog: add driver for StreamLabs USB watchdog device

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 <[email protected]>

---
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

drivers/watchdog/Kconfig +15 -0

index fb94765..130cf54 100644
@@ -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

drivers/watchdog/Makefile +1 -0

index feb6270..9d36929 100644
@@ -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

drivers/watchdog/streamlabs_wdt.c new file +313 -0

313 lines not on this page · raw message

Signature · 1 line

2.8.0.rc3

Oliver Neukum

2016-04-18 08:32 UTC #1 lore raw

Quoted 6 lines · Alexey Klimov

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.

Almost. I see only one issue.

drivers/watchdog/streamlabs_wdt.c line 64

Quoted 6 lines

drivers/watchdog/streamlabs_wdt.c new file +6 -0

+struct streamlabs_wdt {
+	struct watchdog_device wdt_dev;
+	struct usb_interface *intf;
+
+	struct mutex lock;
+	u8 buffer[BUFFER_LENGTH];

That is wrong.

Quoted 2 lines

(quoted diff) +2 -0
+};
+

[..]

Quoted 16 lines

drivers/watchdog/streamlabs_wdt.c new file +16 -0

+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;
+
+	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;
+	}

drivers/watchdog/streamlabs_wdt.c line 135

Quoted 10 lines

drivers/watchdog/streamlabs_wdt.c new file +10 -0

+
+	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,

Because of this line.

The problem is subtle. Your buffer and your lock share a cacheline. On some architecture the cache is not consistent with respect to DMA. On them cachelines holding a buffer for DMA need to be flushed to RAM and invalidated and you may read from them only after DMA has finished.

Thus you may have prepared a cacheline for DMA but somebody tries taking the lock. Then the cacheline with the lock is read from RAM. If that happens before you finish the DMA the data resulting from DMA is lost.

The fix is to allocate the buffer with its own allocation. The VM subsystem makes sure separate allocation don't share cachelines.

That is the long explanation for what I mean when I say that you violate the DMA rules.

Regards
Oliver

Guenter Roeck

maintainer

2016-04-18 13:57 UTC #2 lore raw

Quoted 58 lines · Oliver Neukum

Quoted 6 lines · Alexey Klimov
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.
Almost. I see only one issue.
drivers/watchdog/streamlabs_wdt.c line 64
Quoted 6 lines
drivers/watchdog/streamlabs_wdt.c new file +6 -0
+struct streamlabs_wdt {
+	struct watchdog_device wdt_dev;
+	struct usb_interface *intf;
+
+	struct mutex lock;
+	u8 buffer[BUFFER_LENGTH];
That is wrong.
Quoted 2 lines
(quoted diff) +2 -0
+};
+
[..]
Quoted 16 lines
drivers/watchdog/streamlabs_wdt.c new file +16 -0
+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;
+
+	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;
+	}
drivers/watchdog/streamlabs_wdt.c line 135
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10 -0
+
+	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,
Because of this line.
The problem is subtle. Your buffer and your lock share a cacheline. On some architecture the cache is not consistent with respect to DMA. On them cachelines holding a buffer for DMA need to be flushed to RAM and invalidated and you may read from them only after DMA has finished.

Quoted 7 lines

Thus you may have prepared a cacheline for DMA but somebody tries taking the lock. Then the cacheline with the lock is read from RAM. If that happens before you finish the DMA the data resulting from DMA is lost.
The fix is to allocate the buffer with its own allocation. The VM subsystem makes sure separate allocation don't share cachelines.

Hi Oliver,

For my own education, would adding ____cacheline_aligned to the buffer variable declaration solve the problem as well ?

Thanks,
Guenter

Quoted 8 lines

That is the long explanation for what I mean when I say that you violate the DMA rules.
Regards
Oliver

Oliver Neukum

2016-04-18 14:08 UTC #3 lore raw

Quoted 63 lines · Guenter Roeck

Quoted 62 lines · Oliver Neukum
Quoted 6 lines · Alexey Klimov
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.
Almost. I see only one issue.
drivers/watchdog/streamlabs_wdt.c line 64
Quoted 6 lines
drivers/watchdog/streamlabs_wdt.c new file +6 -0
+struct streamlabs_wdt {
+	struct watchdog_device wdt_dev;
+	struct usb_interface *intf;
+
+	struct mutex lock;
+	u8 buffer[BUFFER_LENGTH];
That is wrong.
Quoted 2 lines
(quoted diff) +2 -0
+};
+
[..]
Quoted 16 lines
drivers/watchdog/streamlabs_wdt.c new file +16 -0
+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;
+
+	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;
+	}
drivers/watchdog/streamlabs_wdt.c line 135
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10 -0
+
+	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,
Because of this line.
The problem is subtle. Your buffer and your lock share a cacheline. On some architecture the cache is not consistent with respect to DMA. On them cachelines holding a buffer for DMA need to be flushed to RAM and invalidated and you may read from them only after DMA has finished.
Thus you may have prepared a cacheline for DMA but somebody tries taking the lock. Then the cacheline with the lock is read from RAM. If that happens before you finish the DMA the data resulting from DMA is lost.

Quoted 8 lines

Quoted 3 lines
The fix is to allocate the buffer with its own allocation. The VM subsystem makes sure separate allocation don't share cachelines.
Hi Oliver,
For my own education, would adding ____cacheline_aligned to the buffer variable declaration solve the problem as well ?

Possibly. We have never gone that route. The obvious problems is that I am not sure our alignment is known before boot.

Regards
Oliver

Guenter Roeck

maintainer

2016-04-18 16:39 UTC #4 lore raw

Quoted 68 lines · Oliver Neukum

Quoted 67 lines · Guenter Roeck
Quoted 65 lines · Oliver Neukum
Quoted 6 lines · Alexey Klimov
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.
Almost. I see only one issue.
drivers/watchdog/streamlabs_wdt.c line 64
Quoted 6 lines
drivers/watchdog/streamlabs_wdt.c new file +6 -0
+struct streamlabs_wdt {
+	struct watchdog_device wdt_dev;
+	struct usb_interface *intf;
+
+	struct mutex lock;
+	u8 buffer[BUFFER_LENGTH];
That is wrong.
Quoted 2 lines
(quoted diff) +2 -0
+};
+
[..]
Quoted 16 lines
drivers/watchdog/streamlabs_wdt.c new file +16 -0
+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;
+
+	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;
+	}
drivers/watchdog/streamlabs_wdt.c line 135
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10 -0
+
+	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,
Because of this line.
The problem is subtle. Your buffer and your lock share a cacheline. On some architecture the cache is not consistent with respect to DMA. On them cachelines holding a buffer for DMA need to be flushed to RAM and invalidated and you may read from them only after DMA has finished.
Thus you may have prepared a cacheline for DMA but somebody tries taking the lock. Then the cacheline with the lock is read from RAM. If that happens before you finish the DMA the data resulting from DMA is lost.
The fix is to allocate the buffer with its own allocation. The VM subsystem makes sure separate allocation don't share cachelines.

Quoted 8 lines

Quoted 4 lines
Hi Oliver,
For my own education, would adding ____cacheline_aligned to the buffer variable declaration solve the problem as well ?
Possibly. We have never gone that route. The obvious problems is that I am not sure our alignment is known before boot.

Seems scary. I always thought that the alignment associated with ____cacheline_aligned would be the maximum possible for a given build/architecture. If not, what is the value of having ____cacheline_aligned in the first place ?

Thanks,
Guenter

Oliver Neukum

2016-04-19 07:49 UTC #5 lore raw

Quoted 8 lines · Guenter Roeck

Quoted 1 line
Possibly. We have never gone that route. The obvious problems is
that
Quoted 2 lines
I am not sure our alignment is known before boot.
Seems scary. I always thought that the alignment associated with ____cacheline_aligned would be the maximum possible for a given build/architecture. If not, what is the value of having ____cacheline_aligned in the first place ?

I thought it was for performance to prevent cache bouncing. I honestly don't know. You'd have to ask a specific question on LKML.

Sorry
Oliver

Guenter Roeck

maintainer

2016-04-18 17:01 UTC #6 lore raw

In reply to Alexey Klimov

Quoted 123 lines · Alexey Klimov

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 <[email protected]>

 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

drivers/watchdog/Kconfig +15 -0

index fb94765..130cf54 100644
@@ -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

drivers/watchdog/Makefile +1 -0

index feb6270..9d36929 100644
@@ -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

drivers/watchdog/streamlabs_wdt.c new file +48 -0

new file mode 100644
index 0000000..3e34cd8
@@ -0,0 +1,313 @@
+/*
+ * StreamLabs USB Watchdog driver
+ *
+ * Copyright (c) 2016 Alexey Klimov <[email protected]>
+ *
+ * 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 <[email protected]>"
+#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
+ */

drivers/watchdog/streamlabs_wdt.c line 58

Quoted 10 lines

drivers/watchdog/streamlabs_wdt.c new file +10 -0

+#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.

Quoted 49 lines

drivers/watchdog/streamlabs_wdt.c new file +49 -0

+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;

drivers/watchdog/streamlabs_wdt.c line 117

Quoted 10 lines

drivers/watchdog/streamlabs_wdt.c new file +10 -0

+}
+
+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.

drivers/watchdog/streamlabs_wdt.c line 124

Quoted 7 lines

drivers/watchdog/streamlabs_wdt.c new file +7 -0

+	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.

Quoted 8 lines

drivers/watchdog/streamlabs_wdt.c new file +8 -0

+	}
+
+	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);

drivers/watchdog/streamlabs_wdt.c line 142

Quoted 10 lines

drivers/watchdog/streamlabs_wdt.c new file +10 -0

+		/* 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.

drivers/watchdog/streamlabs_wdt.c line 143

Quoted 1 line

drivers/watchdog/streamlabs_wdt.c new file +1 -0
+			goto out;

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

drivers/watchdog/streamlabs_wdt.c line 154

Quoted 11 lines

drivers/watchdog/streamlabs_wdt.c new file +11 -0

+		}
+
+		/* 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.

Quoted 14 lines

drivers/watchdog/streamlabs_wdt.c new file +14 -0

+			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

drivers/watchdog/streamlabs_wdt.c line 178

Quoted 10 lines

drivers/watchdog/streamlabs_wdt.c new file +10 -0

+		 * 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.

Quoted 19 lines

drivers/watchdog/streamlabs_wdt.c new file +19 -0

+
+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,

drivers/watchdog/streamlabs_wdt.c line 207

Quoted 10 lines

drivers/watchdog/streamlabs_wdt.c new file +10 -0

+};
+
+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 '('.

drivers/watchdog/streamlabs_wdt.c line 220

Quoted 13 lines

drivers/watchdog/streamlabs_wdt.c new file +13 -0

+{
+	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 ?

drivers/watchdog/streamlabs_wdt.c line 221

Quoted 1 line

drivers/watchdog/streamlabs_wdt.c new file +1 -0
+		(strncmp(usbdev->product, "USBkit", 6) != 0

!= 0 is unnecessary.

drivers/watchdog/streamlabs_wdt.c line 222

Quoted 1 line

drivers/watchdog/streamlabs_wdt.c new file +1 -0
+		|| strncmp(usbdev->manufacturer, "STREAM LABS", 11) != 0))

Same here.

Quoted 55 lines

drivers/watchdog/streamlabs_wdt.c new file +55 -0

+		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);

drivers/watchdog/streamlabs_wdt.c line 287

Quoted 10 lines

drivers/watchdog/streamlabs_wdt.c new file +10 -0

+
+	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.

Quoted 29 lines

drivers/watchdog/streamlabs_wdt.c new file +26 -0

+	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);

Signature · 1 line

2.8.0.rc3