watchdog: add driver for StreamLabs USB watchdog device

drivers/watchdog/streamlabs_wdt.c line 63

Quoted 5 lines · Alexey Klimov

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

+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+

Meta-comment about watchdog drivers, this is per-driver, not per-device, which does not make sense for when these devices are on removable busses where you can have multiple ones.

I don't suggest changing this (as it follows the current style of other watchdog drivers), but perhaps a sysfs attribute for watchdog devices can do the same thing in the future so you can do this on a per-device basis.

Anyway, driver looks good to me, nice work!

Reviewed-by Greg Kroah-Hartman <[email protected]>

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:

Anyway, driver looks good to me, nice work!

Reviewed-by Greg Kroah-Hartman <[email protected]>

Quoted 2 lines · Greg KH

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:

[..]

Quoted 2 lines

Anyway, driver looks good to me, nice work!

Quoted 1 line

Reviewed-by Greg Kroah-Hartman <[email protected]>

Thanks, Greg. If you don't mind I'll use your tag in next version after making changes suggested by Guenter since there will be no significant functional changes. If code will change a lot, then the process (Documentation/process/submitting-patches.rst) will require me to drop the tag.

Quoted 8 lines · Alexey Klimov

Quoted 2 lines · Greg KH

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:

[..]

Quoted 2 lines

Anyway, driver looks good to me, nice work!

Quoted 7 lines

Quoted 1 line

Reviewed-by Greg Kroah-Hartman <[email protected]>

Thanks, Greg. If you don't mind I'll use your tag in next version after making changes suggested by Guenter since there will be no significant functional changes. If code will change a lot, then the process (Documentation/process/submitting-patches.rst) will require me to drop the tag.

Hi,

while thinking about this a question arose. How does this device react to a USB reset? A watchdog that can be disabled by a simple reset does not like very reliable to me. Do you need to implement pre/post_reset() ?

Quoted 18 lines · Oliver Neukum

Quoted 8 lines · Alexey Klimov

Quoted 2 lines · Greg KH

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:

[..]

Quoted 2 lines

Anyway, driver looks good to me, nice work!

Quoted 7 lines

Quoted 1 line

Reviewed-by Greg Kroah-Hartman <[email protected]>

Thanks, Greg. If you don't mind I'll use your tag in next version after making changes suggested by Guenter since there will be no significant functional changes. If code will change a lot, then the process (Documentation/process/submitting-patches.rst) will require me to drop the tag.

Quoted 6 lines

Hi,

while thinking about this a question arose. How does this device react to a USB reset? A watchdog that can be disabled by a simple reset does not like very reliable to me. Do you need to implement pre/post_reset() ?

You're right. Upon reset the watchdog is disabled even if it was active before. Adding empty ->pre_reset() and ->post_reset() helps to avoid that, but looking at Documentation and other drivers it seems that I need to do:
in pre_reset():
mutex_lock() to block any other I/O to the usb device; __usb_streamlabs_wdt_cmd(STOP) to stop the watchdog;
and do not unlock the mutex;

in post_reset():
if (watchdog_active())

        __usb_streamlabs_wdt_cmd(START);

mutex_unlock() to allow other's I/O to the usb deivce.

Seems right?

Best regards,
Alexey

Quoted 39 lines · Alexey Klimov

Quoted 18 lines · Oliver Neukum

Quoted 8 lines · Alexey Klimov

Quoted 2 lines · Greg KH

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:

[..]

Quoted 2 lines

Anyway, driver looks good to me, nice work!

Quoted 7 lines

Quoted 1 line

Reviewed-by Greg Kroah-Hartman <[email protected]>

Thanks, Greg. If you don't mind I'll use your tag in next version after making changes suggested by Guenter since there will be no significant functional changes. If code will change a lot, then the process (Documentation/process/submitting-patches.rst) will require me to drop the tag.

Quoted 6 lines

Hi,

while thinking about this a question arose. How does this device react to a USB reset? A watchdog that can be disabled by a simple reset does not like very reliable to me. Do you need to implement pre/post_reset() ?

You're right. Upon reset the watchdog is disabled even if it was active before. Adding empty ->pre_reset() and ->post_reset() helps to avoid that, but looking at Documentation and other drivers it seems that I need to do:
in pre_reset():
mutex_lock() to block any other I/O to the usb device; __usb_streamlabs_wdt_cmd(STOP) to stop the watchdog;
and do not unlock the mutex;

in post_reset():
if (watchdog_active())

        __usb_streamlabs_wdt_cmd(START);

mutex_unlock() to allow other's I/O to the usb deivce.

Quoted 2 lines

Seems right?

Not necessarily. Is other code doing something similar ? Using a mutex like this creates the risk for hung tasks.

Quoted 40 lines · Guenter Roeck

Quoted 39 lines · Alexey Klimov

Quoted 18 lines · Oliver Neukum

Quoted 8 lines · Alexey Klimov

Quoted 2 lines · Greg KH

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:

[..]

Quoted 2 lines

Anyway, driver looks good to me, nice work!

Quoted 7 lines

Quoted 1 line

Reviewed-by Greg Kroah-Hartman <[email protected]>

Thanks, Greg. If you don't mind I'll use your tag in next version after making changes suggested by Guenter since there will be no significant functional changes. If code will change a lot, then the process (Documentation/process/submitting-patches.rst) will require me to drop the tag.

Quoted 6 lines

Hi,

while thinking about this a question arose. How does this device react to a USB reset? A watchdog that can be disabled by a simple reset does not like very reliable to me. Do you need to implement pre/post_reset() ?

You're right. Upon reset the watchdog is disabled even if it was active before. Adding empty ->pre_reset() and ->post_reset() helps to avoid that, but looking at Documentation and other drivers it seems that I need to do:
in pre_reset():
mutex_lock() to block any other I/O to the usb device; __usb_streamlabs_wdt_cmd(STOP) to stop the watchdog;
and do not unlock the mutex;

in post_reset():
if (watchdog_active())

        __usb_streamlabs_wdt_cmd(START);

mutex_unlock() to allow other's I/O to the usb deivce.

Quoted 4 lines

Quoted 2 lines

Seems right?

Not necessarily. Is other code doing something similar ? Using a mutex like this creates the risk for hung tasks.

Are mutexes intended to be used in situations where one function acquires the lock, then returns, and then a different function releases the lock? I'm not sure about this.

Perhaps a good old semaphore would be more appropriate. But it's clear that I/O to the device does need to be mutually exclusive with resets, one way or another.

Quoted 39 lines · Alexey Klimov

Quoted 18 lines · Oliver Neukum

Quoted 8 lines · Alexey Klimov

Quoted 2 lines · Greg KH

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:

[..]

Quoted 2 lines

Anyway, driver looks good to me, nice work!

Quoted 7 lines

Quoted 1 line

Reviewed-by Greg Kroah-Hartman <[email protected]>

Thanks, Greg. If you don't mind I'll use your tag in next version after making changes suggested by Guenter since there will be no significant functional changes. If code will change a lot, then the process (Documentation/process/submitting-patches.rst) will require me to drop the tag.

Quoted 6 lines

Hi,

while thinking about this a question arose. How does this device react to a USB reset? A watchdog that can be disabled by a simple reset does not like very reliable to me. Do you need to implement pre/post_reset() ?

You're right. Upon reset the watchdog is disabled even if it was active before. Adding empty ->pre_reset() and ->post_reset() helps to avoid that, but looking at Documentation and other drivers it seems that I need to do:
in pre_reset():
mutex_lock() to block any other I/O to the usb device; __usb_streamlabs_wdt_cmd(STOP) to stop the watchdog;
and do not unlock the mutex;

in post_reset():
if (watchdog_active())

        __usb_streamlabs_wdt_cmd(START);

mutex_unlock() to allow other's I/O to the usb deivce.

Quoted 2 lines

Seems right?

Not necessarily. Is other code doing something similar ? Using a mutex like this creates the risk for hung tasks.

On Tue, Jul 26, 2022 at 8:48 AM Oliver Neukum <[email protected]> wrote:

You're right. Upon reset the watchdog is disabled even if it was active before. Adding empty ->pre_reset() and ->post_reset() helps to avoid that, but looking at Documentation and other drivers it seems that I need to do:
in pre_reset():
mutex_lock() to block any other I/O to the usb device; __usb_streamlabs_wdt_cmd(STOP) to stop the watchdog;
and do not unlock the mutex;

in post_reset():
if (watchdog_active())

        __usb_streamlabs_wdt_cmd(START);

mutex_unlock() to allow other's I/O to the usb deivce.

Seems right?

Driver supports StreamLabs usb watchdog device. The device plugs into 9-pin usb header and connects to reset pins and reset button pins on desktop PC.

USB commands used to communicate with device were reverse engineered using usbmon.

Signed-off-by Alexey Klimov <[email protected]>

---
Changes since v4:

	-- added more comments;
	-- added nowayout check in ->disconnect();
	-- completely rework usb_streamlabs_wdt_cmd() ->
                 __usb_streamlabs_wdt_cmd() and functions
		that handle validation of the response;
	-- usb sending and receiving msgs are moved to separate
		functions;
         -- added soft_unbind=1 flag in usb_driver struct
		to make it possible to send URBs in ->disconnect();
         -- buffer is declared as __le16 now;
         -- made checkpatch.pl --strict happy;

Previous version:
https://lore.kernel.org/linux-watchdog/[email protected]/

  MAINTAINERS                       |   6 +
  drivers/watchdog/Kconfig          |  15 ++
  drivers/watchdog/Makefile         |   1 +
  drivers/watchdog/streamlabs_wdt.c | 360 ++++++++++++++++++++++++++++++
  4 files changed, 382 insertions(+)

create mode 100644 drivers/watchdog/streamlabs_wdt.c

MAINTAINERS +6 -0

index 64379c699903..fb31c1823043 100644
@@ -19190,6 +19190,12 @@ W:	http://www.stlinux.com
  F:	Documentation/networking/device_drivers/ethernet/stmicro/
  F:	drivers/net/ethernet/stmicro/stmmac/
 
+STREAMLABS USB WATCHDOG DRIVER
+M:	Alexey Klimov <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/watchdog/streamlabs_wdt.c
+
  SUN3/3X
  M:	Sam Creasey <[email protected]>
  S:	Maintained

drivers/watchdog/Kconfig +15 -0

index 32fd37698932..64b7f947b196 100644
@@ -2171,6 +2171,21 @@ 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.
+
  config KEEMBAY_WATCHDOG
  	tristate "Intel Keem Bay SoC non-secure watchdog"
  	depends on ARCH_KEEMBAY || (ARM64 && COMPILE_TEST)

drivers/watchdog/Makefile +1 -0

index c324e9d820e9..2d601da9b5bd 100644
@@ -33,6 +33,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 +33 -0

new file mode 100644
index 000000000000..f1130fe5559c
@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * StreamLabs USB Watchdog driver
+ *
+ * Copyright (c) 2016-2017,2022 Alexey Klimov <[email protected]>
+ */
+
+#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:
+ * https://www.stream-labs.com/products/devices/watch-dog/
+ *
+ * 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

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

+#define STREAMLABS_CMD_START	0xaacc
+#define STREAMLABS_CMD_STOP	0xbbff
+
+/* timeout values are taken from windows program */
+#define STREAMLABS_WDT_MIN_TIMEOUT	1
+#define STREAMLABS_WDT_MAX_TIMEOUT	46
+
+struct streamlabs_wdt {
+	struct watchdog_device wdt_dev;
+	struct usb_interface *intf;
+	/* Serialises usb communication with a device */
+	struct mutex lock;
+	__le16 *buffer;
+};
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+			__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+/* USB call wrappers to send and receive messages to/from the device. */
+static int usb_streamlabs_send_msg(struct usb_device *usbdev, __le16 *buf)
+{

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

+	return retval;
+}
+
+static int usb_streamlabs_get_msg(struct usb_device *usbdev, __le16 *buf)
+{

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

+	return retval;
+}
+
+/*
+ * This function is used to check if watchdog timeout in the received buffer
+ * matches the timeout passed from watchdog subsystem.
+ */
+static int usb_streamlabs_wdt_check_timeout(__le16 *buf, unsigned long timeout)
+{
+	if (buf[3] != cpu_to_le16(timeout))
+		return -EPROTO;
+
+	return 0;
+}
+
+static int usb_streamlabs_wdt_check_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 -EPROTO;
+
+	return 0;
+}
+
+/*
+ * This function is used to check if watchdog command in the received buffer
+ * matches the command passed to the device.
+ */
+static int usb_streamlabs_wdt_check_command(__le16 *buf, u16 cmd)
+{
+	if (buf[0] != cpu_to_le16(cmd))
+		return -EPROTO;
+
+	return 0;
+}
+
+static int usb_streamlabs_wdt_validate_response(__le16 *buf, u16 cmd,
+						unsigned long timeout_msec)
+{
+	int retval;
+

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

+	buf[5] = 0x0;
+	buf[6] = 0x0;
+}
+
+static int __usb_streamlabs_wdt_cmd(struct streamlabs_wdt *wdt, u16 cmd)
+{
+	struct usb_device *usbdev;
+	unsigned long timeout_msec;
+	/* how many times to re-try getting the state of the device */
+	unsigned int retry_counter = 10;
+	int retval;
+
+	if (unlikely(!wdt->intf))
+		return -ENODEV;
+
+	usbdev = interface_to_usbdev(wdt->intf);
+	timeout_msec = wdt->wdt_dev.timeout * MSEC_PER_SEC;
+
+	usb_streamlabs_wdt_prepare_buf(wdt->buffer, cmd, timeout_msec);
+
+	/* send command to watchdog */
+	retval = usb_streamlabs_send_msg(usbdev, wdt->buffer);
+	if (retval)
+		return retval;
+
+	/*
+	 * Transition from one state to another in this device
+	 * doesn't happen immediately, especially stopping the device
+	 * is not observed on the first reading of the response.
+	 * Plus to avoid potentially stale response message in the device
+	 * we keep reading the state of the device until we see:
+	 * -- that device recognised the sent command;
+	 * -- the received state (started or stopped) matches the state
+	 * that was requested;
+	 * -- the timeout passed matches the timeout value read from
+	 * the device.
+	 * Keep retrying 10 times and if watchdog device doesn't respond
+	 * correctly as expected we bail out and return an error.
+	 */
+	do {
+		retval = usb_streamlabs_get_msg(usbdev, wdt->buffer);
+		if (retval)
+			break;
+
+		retval = usb_streamlabs_wdt_validate_response(wdt->buffer, cmd,
+							      timeout_msec);
+	} while (retval && retry_counter--);
+
+	return retry_counter ? retval : -EIO;
+}
+
+static int usb_streamlabs_wdt_cmd(struct streamlabs_wdt *streamlabs_wdt, u16 cmd)
+{
+	int retval;
+
+	mutex_lock(&streamlabs_wdt->lock);
+	retval = __usb_streamlabs_wdt_cmd(streamlabs_wdt, cmd);
+	mutex_unlock(&streamlabs_wdt->lock);
+
+	return retval;
+}
+
+static int usb_streamlabs_wdt_start(struct watchdog_device *wdt_dev)
+{
+	struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
+
+	return usb_streamlabs_wdt_cmd(streamlabs_wdt, STREAMLABS_CMD_START);
+}
+
+static int usb_streamlabs_wdt_stop(struct watchdog_device *wdt_dev)
+{
+	struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
+
+	return usb_streamlabs_wdt_cmd(streamlabs_wdt, STREAMLABS_CMD_STOP);
+}
+
+static const struct watchdog_info streamlabs_wdt_ident = {
+	.options	= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+	.identity	= DRIVER_NAME,
+};
+
+static const 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)
+{
+	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 ||
+	    strncmp(usbdev->product, "USBkit", 6) ||
+	    strncmp(usbdev->manufacturer, "STREAM LABS", 11))
+		return -ENODEV;
+

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

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

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

+	retval = devm_watchdog_register_device(&intf->dev,
+					       &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 driver 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);
+
+	mutex_lock(&streamlabs_wdt->lock);
+	/*
+	 * If disconnect happens via sysfs or on rmmod, then try to stop
+	 * the watchdog. In case of physical detachment of the device this call
+	 * will fail but we continue.
+	 */
+	if (!nowayout)
+		__usb_streamlabs_wdt_cmd(streamlabs_wdt, STREAMLABS_CMD_STOP);
+	/* Stop sending (new) messages to the device */
+	streamlabs_wdt->intf = NULL;
+	mutex_unlock(&streamlabs_wdt->lock);
+}
+
+static const 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,
+	.soft_unbind	= 1,
+};
+
+module_usb_driver(usb_streamlabs_wdt_driver);

On 7/24/22 20:06, Alexey Klimov wrote:

drivers/watchdog/streamlabs_wdt.c line 43

Quoted 7 lines

+ * one buffer is used for communication, however transmitted message is only
+ * 32 bytes long
+ */

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

+#define BUFFER_TRANSFER_LENGTH       32
+#define BUFFER_LENGTH                64
+#define USB_TIMEOUT          350
+

Comment about the unit (ms) might be useful.

Quoted 23 lines

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

+#define STREAMLABS_CMD_START 0xaacc
+#define STREAMLABS_CMD_STOP  0xbbff
+
+/* timeout values are taken from windows program */
+#define STREAMLABS_WDT_MIN_TIMEOUT   1
+#define STREAMLABS_WDT_MAX_TIMEOUT   46
+
+struct streamlabs_wdt {
+     struct watchdog_device wdt_dev;
+     struct usb_interface *intf;
+     /* Serialises usb communication with a device */
+     struct mutex lock;
+     __le16 *buffer;
+};
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+                     __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+/* USB call wrappers to send and receive messages to/from the device. */
+static int usb_streamlabs_send_msg(struct usb_device *usbdev, __le16 *buf)
+{

drivers/watchdog/streamlabs_wdt.c line 76

Quoted 10 lines

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

+     int retval;
+     int size;
+
+     retval = usb_interrupt_msg(usbdev, usb_sndintpipe(usbdev, 0x02),
+                                buf, BUFFER_TRANSFER_LENGTH,
+                                &size, USB_TIMEOUT);
+
+     if (size != BUFFER_TRANSFER_LENGTH)
+             return -EIO;
+

If usb_interrupt_msg() returns an error, it will likely not set size, which may result in a random -EIO. I think this should be something like

        if (retval)
                return retval;
        if (size != BUFFER_TRANSFER_LENGTH)
                return -EIO;

        return 0;

Quoted 5 lines

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

+     return retval;
+}
+
+static int usb_streamlabs_get_msg(struct usb_device *usbdev, __le16 *buf)
+{

drivers/watchdog/streamlabs_wdt.c line 91

Quoted 10 lines

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

+     int retval;
+     int size;
+
+     retval = usb_interrupt_msg(usbdev, usb_rcvintpipe(usbdev, 0x81),
+                                buf, BUFFER_LENGTH,
+                                &size, USB_TIMEOUT);
+
+     if (size != BUFFER_LENGTH)
+             return -EIO;
+

Same here.

Quoted 49 lines

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

+     return retval;
+}
+
+/*
+ * This function is used to check if watchdog timeout in the received buffer
+ * matches the timeout passed from watchdog subsystem.
+ */
+static int usb_streamlabs_wdt_check_timeout(__le16 *buf, unsigned long timeout)
+{
+     if (buf[3] != cpu_to_le16(timeout))
+             return -EPROTO;
+
+     return 0;
+}
+
+static int usb_streamlabs_wdt_check_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 -EPROTO;
+
+     return 0;
+}
+
+/*
+ * This function is used to check if watchdog command in the received buffer
+ * matches the command passed to the device.
+ */
+static int usb_streamlabs_wdt_check_command(__le16 *buf, u16 cmd)
+{
+     if (buf[0] != cpu_to_le16(cmd))
+             return -EPROTO;
+
+     return 0;
+}
+
+static int usb_streamlabs_wdt_validate_response(__le16 *buf, u16 cmd,
+                                             unsigned long timeout_msec)
+{
+     int retval;
+
+     retval = usb_streamlabs_wdt_check_response((u8 *)buf);
+     if (retval)
+             return retval;
+

drivers/watchdog/streamlabs_wdt.c line 146

Quoted 6 lines

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

+     retval = usb_streamlabs_wdt_check_command(buf, cmd);
+     if (retval)
+             return retval;
+
+     retval = usb_streamlabs_wdt_check_timeout(buf, timeout_msec);
+     return retval;

assignment to retval is unnecessary.

Quoted 9 lines

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

+}
+
+static void usb_streamlabs_wdt_prepare_buf(__le16 *buf, u16 cmd,
+                                        unsigned long timeout_msec)
+{
+     /*
+      * remaining elements expected to be zero everytime during
+      * communication
+      */

drivers/watchdog/streamlabs_wdt.c line 158

Quoted 3 lines

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

+     buf[0] = cpu_to_le16(cmd);
+     buf[1] = cpu_to_le16(0x8000);
+     buf[3] = cpu_to_le16(timeout_msec);

Not setting buf[2] and buf[4] contradicts the comment above. Maybe those offsets are not _expected_ to be set by the device, but that is not guaranteed. It may be safer to just use memset() at the beginning of the function to clear the buffer.

Quoted 111 lines

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

+     buf[5] = 0x0;
+     buf[6] = 0x0;
+}
+
+static int __usb_streamlabs_wdt_cmd(struct streamlabs_wdt *wdt, u16 cmd)
+{
+     struct usb_device *usbdev;
+     unsigned long timeout_msec;
+     /* how many times to re-try getting the state of the device */
+     unsigned int retry_counter = 10;
+     int retval;
+
+     if (unlikely(!wdt->intf))
+             return -ENODEV;
+
+     usbdev = interface_to_usbdev(wdt->intf);
+     timeout_msec = wdt->wdt_dev.timeout * MSEC_PER_SEC;
+
+     usb_streamlabs_wdt_prepare_buf(wdt->buffer, cmd, timeout_msec);
+
+     /* send command to watchdog */
+     retval = usb_streamlabs_send_msg(usbdev, wdt->buffer);
+     if (retval)
+             return retval;
+
+     /*
+      * Transition from one state to another in this device
+      * doesn't happen immediately, especially stopping the device
+      * is not observed on the first reading of the response.
+      * Plus to avoid potentially stale response message in the device
+      * we keep reading the state of the device until we see:
+      * -- that device recognised the sent command;
+      * -- the received state (started or stopped) matches the state
+      * that was requested;
+      * -- the timeout passed matches the timeout value read from
+      * the device.
+      * Keep retrying 10 times and if watchdog device doesn't respond
+      * correctly as expected we bail out and return an error.
+      */
+     do {
+             retval = usb_streamlabs_get_msg(usbdev, wdt->buffer);
+             if (retval)
+                     break;
+
+             retval = usb_streamlabs_wdt_validate_response(wdt->buffer, cmd,
+                                                           timeout_msec);
+     } while (retval && retry_counter--);
+
+     return retry_counter ? retval : -EIO;
+}
+
+static int usb_streamlabs_wdt_cmd(struct streamlabs_wdt *streamlabs_wdt, u16 cmd)
+{
+     int retval;
+
+     mutex_lock(&streamlabs_wdt->lock);
+     retval = __usb_streamlabs_wdt_cmd(streamlabs_wdt, cmd);
+     mutex_unlock(&streamlabs_wdt->lock);
+
+     return retval;
+}
+
+static int usb_streamlabs_wdt_start(struct watchdog_device *wdt_dev)
+{
+     struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
+
+     return usb_streamlabs_wdt_cmd(streamlabs_wdt, STREAMLABS_CMD_START);
+}
+
+static int usb_streamlabs_wdt_stop(struct watchdog_device *wdt_dev)
+{
+     struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
+
+     return usb_streamlabs_wdt_cmd(streamlabs_wdt, STREAMLABS_CMD_STOP);
+}
+
+static const struct watchdog_info streamlabs_wdt_ident = {
+     .options        = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+     .identity       = DRIVER_NAME,
+};
+
+static const 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)
+{
+     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 ||
+         strncmp(usbdev->product, "USBkit", 6) ||
+         strncmp(usbdev->manufacturer, "STREAM LABS", 11))
+             return -ENODEV;
+
+     streamlabs_wdt = devm_kzalloc(&intf->dev, sizeof(struct streamlabs_wdt),
+                                   GFP_KERNEL);
+     if (!streamlabs_wdt)
+             return -ENOMEM;
+

drivers/watchdog/streamlabs_wdt.c line 274

Quoted 5 lines

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

+     streamlabs_wdt->buffer = devm_kzalloc(&intf->dev, BUFFER_LENGTH,
+                                           GFP_KERNEL);
+     if (!streamlabs_wdt->buffer)
+             return -ENOMEM;
+

Nit: buffer could be made part of struct streamlabs_wdt and be tagged with ____cacheline_aligned to avoid the double allocation.

Quoted 13 lines

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

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

drivers/watchdog/streamlabs_wdt.c line 292

Quoted 5 lines

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

+
+     retval = usb_streamlabs_wdt_stop(&streamlabs_wdt->wdt_dev);
+     if (retval)
+             return -ENODEV;
+

A comment explaining why the watchdog is explicitly stopped when running might be useful.

Quoted 2 lines · Guenter Roeck

On 7/24/22 20:06, Alexey Klimov wrote:

[...]

Quoted 8 lines

drivers/watchdog/streamlabs_wdt.c line 43

Quoted 7 lines

+ * one buffer is used for communication, however transmitted message is only
+ * 32 bytes long
+ */

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

+#define BUFFER_TRANSFER_LENGTH       32
+#define BUFFER_LENGTH                64
+#define USB_TIMEOUT          350
+

Comment about the unit (ms) might be useful.

Yes. I'll add it.

Quoted 37 lines

Quoted 23 lines

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

+#define STREAMLABS_CMD_START 0xaacc
+#define STREAMLABS_CMD_STOP  0xbbff
+
+/* timeout values are taken from windows program */
+#define STREAMLABS_WDT_MIN_TIMEOUT   1
+#define STREAMLABS_WDT_MAX_TIMEOUT   46
+
+struct streamlabs_wdt {
+     struct watchdog_device wdt_dev;
+     struct usb_interface *intf;
+     /* Serialises usb communication with a device */
+     struct mutex lock;
+     __le16 *buffer;
+};
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+                     __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+/* USB call wrappers to send and receive messages to/from the device. */
+static int usb_streamlabs_send_msg(struct usb_device *usbdev, __le16 *buf)
+{

drivers/watchdog/streamlabs_wdt.c line 76

Quoted 10 lines

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

+     int retval;
+     int size;
+
+     retval = usb_interrupt_msg(usbdev, usb_sndintpipe(usbdev, 0x02),
+                                buf, BUFFER_TRANSFER_LENGTH,
+                                &size, USB_TIMEOUT);
+
+     if (size != BUFFER_TRANSFER_LENGTH)
+             return -EIO;
+

If usb_interrupt_msg() returns an error, it will likely not set size, which may result in a random -EIO. I think this should be something like

Quoted 6 lines

        if (retval)
                return retval;
        if (size != BUFFER_TRANSFER_LENGTH)
                return -EIO;

        return 0;

Good point. I'll change it.

Quoted 66 lines

Quoted 5 lines

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

+     return retval;
+}
+
+static int usb_streamlabs_get_msg(struct usb_device *usbdev, __le16 *buf)
+{

drivers/watchdog/streamlabs_wdt.c line 91

Quoted 10 lines

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

+     int retval;
+     int size;
+
+     retval = usb_interrupt_msg(usbdev, usb_rcvintpipe(usbdev, 0x81),
+                                buf, BUFFER_LENGTH,
+                                &size, USB_TIMEOUT);
+
+     if (size != BUFFER_LENGTH)
+             return -EIO;
+

Same here.

Quoted 49 lines

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

+     return retval;
+}
+
+/*
+ * This function is used to check if watchdog timeout in the received buffer
+ * matches the timeout passed from watchdog subsystem.
+ */
+static int usb_streamlabs_wdt_check_timeout(__le16 *buf, unsigned long timeout)
+{
+     if (buf[3] != cpu_to_le16(timeout))
+             return -EPROTO;
+
+     return 0;
+}
+
+static int usb_streamlabs_wdt_check_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 -EPROTO;
+
+     return 0;
+}
+
+/*
+ * This function is used to check if watchdog command in the received buffer
+ * matches the command passed to the device.
+ */
+static int usb_streamlabs_wdt_check_command(__le16 *buf, u16 cmd)
+{
+     if (buf[0] != cpu_to_le16(cmd))
+             return -EPROTO;
+
+     return 0;
+}
+
+static int usb_streamlabs_wdt_validate_response(__le16 *buf, u16 cmd,
+                                             unsigned long timeout_msec)
+{
+     int retval;
+
+     retval = usb_streamlabs_wdt_check_response((u8 *)buf);
+     if (retval)
+             return retval;
+

Quoted 8 lines

drivers/watchdog/streamlabs_wdt.c line 146

Quoted 6 lines

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

+     retval = usb_streamlabs_wdt_check_command(buf, cmd);
+     if (retval)
+             return retval;
+
+     retval = usb_streamlabs_wdt_check_timeout(buf, timeout_msec);
+     return retval;

assignment to retval is unnecessary.

Ok.

Quoted 9 lines

Quoted 9 lines

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

+}
+
+static void usb_streamlabs_wdt_prepare_buf(__le16 *buf, u16 cmd,
+                                        unsigned long timeout_msec)
+{
+     /*
+      * remaining elements expected to be zero everytime during
+      * communication
+      */

Quoted 8 lines

drivers/watchdog/streamlabs_wdt.c line 158

Quoted 3 lines

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

+     buf[0] = cpu_to_le16(cmd);
+     buf[1] = cpu_to_le16(0x8000);
+     buf[3] = cpu_to_le16(timeout_msec);

Not setting buf[2] and buf[4] contradicts the comment above. Maybe those offsets are not _expected_ to be set by the device, but that is not guaranteed. It may be safer to just use memset() at the beginning of the function to clear the buffer.

Sure. I guess it makes sense to zero the buffer before reading the message from the device too?
Before usb_streamlabs_get_msg(usbdev, wdt->buffer).

Quoted 111 lines

Quoted 111 lines

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

+     buf[5] = 0x0;
+     buf[6] = 0x0;
+}
+
+static int __usb_streamlabs_wdt_cmd(struct streamlabs_wdt *wdt, u16 cmd)
+{
+     struct usb_device *usbdev;
+     unsigned long timeout_msec;
+     /* how many times to re-try getting the state of the device */
+     unsigned int retry_counter = 10;
+     int retval;
+
+     if (unlikely(!wdt->intf))
+             return -ENODEV;
+
+     usbdev = interface_to_usbdev(wdt->intf);
+     timeout_msec = wdt->wdt_dev.timeout * MSEC_PER_SEC;
+
+     usb_streamlabs_wdt_prepare_buf(wdt->buffer, cmd, timeout_msec);
+
+     /* send command to watchdog */
+     retval = usb_streamlabs_send_msg(usbdev, wdt->buffer);
+     if (retval)
+             return retval;
+
+     /*
+      * Transition from one state to another in this device
+      * doesn't happen immediately, especially stopping the device
+      * is not observed on the first reading of the response.
+      * Plus to avoid potentially stale response message in the device
+      * we keep reading the state of the device until we see:
+      * -- that device recognised the sent command;
+      * -- the received state (started or stopped) matches the state
+      * that was requested;
+      * -- the timeout passed matches the timeout value read from
+      * the device.
+      * Keep retrying 10 times and if watchdog device doesn't respond
+      * correctly as expected we bail out and return an error.
+      */
+     do {
+             retval = usb_streamlabs_get_msg(usbdev, wdt->buffer);
+             if (retval)
+                     break;
+
+             retval = usb_streamlabs_wdt_validate_response(wdt->buffer, cmd,
+                                                           timeout_msec);
+     } while (retval && retry_counter--);
+
+     return retry_counter ? retval : -EIO;
+}
+
+static int usb_streamlabs_wdt_cmd(struct streamlabs_wdt *streamlabs_wdt, u16 cmd)
+{
+     int retval;
+
+     mutex_lock(&streamlabs_wdt->lock);
+     retval = __usb_streamlabs_wdt_cmd(streamlabs_wdt, cmd);
+     mutex_unlock(&streamlabs_wdt->lock);
+
+     return retval;
+}
+
+static int usb_streamlabs_wdt_start(struct watchdog_device *wdt_dev)
+{
+     struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
+
+     return usb_streamlabs_wdt_cmd(streamlabs_wdt, STREAMLABS_CMD_START);
+}
+
+static int usb_streamlabs_wdt_stop(struct watchdog_device *wdt_dev)
+{
+     struct streamlabs_wdt *streamlabs_wdt = watchdog_get_drvdata(wdt_dev);
+
+     return usb_streamlabs_wdt_cmd(streamlabs_wdt, STREAMLABS_CMD_STOP);
+}
+
+static const struct watchdog_info streamlabs_wdt_ident = {
+     .options        = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING,
+     .identity       = DRIVER_NAME,
+};
+
+static const 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)
+{
+     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 ||
+         strncmp(usbdev->product, "USBkit", 6) ||
+         strncmp(usbdev->manufacturer, "STREAM LABS", 11))
+             return -ENODEV;
+
+     streamlabs_wdt = devm_kzalloc(&intf->dev, sizeof(struct streamlabs_wdt),
+                                   GFP_KERNEL);
+     if (!streamlabs_wdt)
+             return -ENOMEM;
+

Quoted 8 lines

drivers/watchdog/streamlabs_wdt.c line 274

Quoted 5 lines

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

+     streamlabs_wdt->buffer = devm_kzalloc(&intf->dev, BUFFER_LENGTH,
+                                           GFP_KERNEL);
+     if (!streamlabs_wdt->buffer)
+             return -ENOMEM;
+

Nit: buffer could be made part of struct streamlabs_wdt and be tagged with ____cacheline_aligned to avoid the double allocation.

It was discussed in the past.
https://lore.kernel.org/linux-watchdog/[email protected]/
https://lore.kernel.org/linux-watchdog/[email protected]/

The conclusion there was that with separate allocation it is guaranteed to not share a cacheline with mutex lock.
Do we know for sure that it is safe with ____cacheline_aligned attribute?

Oliver, thoughts?

I see that a lot of drivers use cacheline alignment for buffers, so I guess that should be okay nowadays and I can change it back to initial version with cacheline alignment.

Quoted 13 lines

Quoted 13 lines

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

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

Quoted 8 lines

drivers/watchdog/streamlabs_wdt.c line 292

Quoted 5 lines

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

+
+     retval = usb_streamlabs_wdt_stop(&streamlabs_wdt->wdt_dev);
+     if (retval)
+             return -ENODEV;
+

A comment explaining why the watchdog is explicitly stopped when running might be useful.

What do you mean by saying "when running"?
Everytime during my testing the initial state is "stopped" on boot/power on/after reset, so not sure what you mean by saying "when running".

There is a comment above that explains the stop command but I will add/change comments that explain things better.
The point of executing "stop" command here is to check that device being probed behaves like we expect it to. This is a bit paranoid check since I am a not 100% sure that all devices with such USB ids are watchdogs -- that's why additional checks for usbdev->product and usbdev->manufacturer and this stop command that doesn't change the initial state. In theory, I can remove this stop command at all.

Thank you for the review.

[...]

Best regards,
Alexey

Quoted 14 lines

Quoted 5 lines

Quoted 5 lines

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

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

Quoted 8 lines

drivers/watchdog/streamlabs_wdt.c line 292

Quoted 5 lines

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

+
+     retval = usb_streamlabs_wdt_stop(&streamlabs_wdt->wdt_dev);
+     if (retval)
+             return -ENODEV;
+

A comment explaining why the watchdog is explicitly stopped when running might be useful.

Quoted 4 lines

What do you mean by saying "when running"?
Everytime during my testing the initial state is "stopped" on boot/power on/after reset, so not sure what you mean by saying "when running".

Should I have used the term "active" ? "started" ?

Quoted 9 lines

There is a comment above that explains the stop command but I will add/change comments that explain things better.
The point of executing "stop" command here is to check that device being probed behaves like we expect it to. This is a bit paranoid check since I am a not 100% sure that all devices with such USB ids are watchdogs -- that's why additional checks for usbdev->product and usbdev->manufacturer and this stop command that doesn't change the initial state. In theory, I can remove this stop command at all.

Normally one does not want a watchdog to stop if it is running (started ? active ? pick your term) when the device is instantiated to avoid gaps in watchdog coverage. The watchdog core provides a flag, WDOG_HW_RUNNING, for this purpose (sorry, there is the 'running' term again). It is used by many drivers, and ensures that the time from loading the Linux kernel to opening the watchdog device is protected by the watchdog.

Calling the stop function during probe suggests that you at least considered the possibility that the watchdog may be running/started/active. Per your explanation, you still want it to stop explicitly if/when that happens. All I am asking you is to add a comment explaining why this is not needed/wanted/relevant/supported for this driver. One explanation might, for example, be that the state can not be detected reliably.