[PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alexey Klimov]

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 <klimov.linux@gmail.com>
---
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/20220715234442.3910204-1-klimov.linux@gmail.com/


 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

diff --git a/MAINTAINERS b/MAINTAINERS
index 64379c699903..fb31c1823043 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <klimov.linux@gmail.com>
+L:	linux-watchdog@vger.kernel.org
+S:	Maintained
+F:	drivers/watchdog/streamlabs_wdt.c
+
 SUN3/3X
 M:	Sam Creasey <sammy@sammy.net>
 S:	Maintained
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 32fd37698932..64b7f947b196 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -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)
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index c324e9d820e9..2d601da9b5bd 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -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
 
diff --git a/drivers/watchdog/streamlabs_wdt.c b/drivers/watchdog/streamlabs_wdt.c
new file mode 100644
index 000000000000..f1130fe5559c
--- /dev/null
+++ b/drivers/watchdog/streamlabs_wdt.c
@@ -0,0 +1,360 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * StreamLabs USB Watchdog driver
+ *
+ * Copyright (c) 2016-2017,2022 Alexey Klimov <klimov.linux@gmail.com>
+ */
+
+#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 <klimov.linux@gmail.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
+
+/* 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)
+{
+	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;
+
+	return retval;
+}
+
+static int usb_streamlabs_get_msg(struct usb_device *usbdev, __le16 *buf)
+{
+	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;
+
+	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;
+
+	retval = usb_streamlabs_wdt_check_command(buf, cmd);
+	if (retval)
+		return retval;
+
+	retval = usb_streamlabs_wdt_check_timeout(buf, timeout_msec);
+	return retval;
+}
+
+static void usb_streamlabs_wdt_prepare_buf(__le16 *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_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;
+
+	streamlabs_wdt->buffer = devm_kzalloc(&intf->dev, BUFFER_LENGTH,
+					      GFP_KERNEL);
+	if (!streamlabs_wdt->buffer)
+		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 -ENODEV;
+
+	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);
-- 
2.36.1

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Greg KH]

On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:
> +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 <gregkh@linuxfoundation.org>

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Guenter Roeck]

On 7/24/22 20:06, Alexey Klimov wrote:
> 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 <klimov.linux@gmail.com>
> ---
> 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/20220715234442.3910204-1-klimov.linux@gmail.com/
> 
> 
>   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
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 64379c699903..fb31c1823043 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -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 <klimov.linux@gmail.com>
> +L:	linux-watchdog@vger.kernel.org
> +S:	Maintained
> +F:	drivers/watchdog/streamlabs_wdt.c
> +
>   SUN3/3X
>   M:	Sam Creasey <sammy@sammy.net>
>   S:	Maintained
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 32fd37698932..64b7f947b196 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -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)
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index c324e9d820e9..2d601da9b5bd 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -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
>   
> diff --git a/drivers/watchdog/streamlabs_wdt.c b/drivers/watchdog/streamlabs_wdt.c
> new file mode 100644
> index 000000000000..f1130fe5559c
> --- /dev/null
> +++ b/drivers/watchdog/streamlabs_wdt.c
> @@ -0,0 +1,360 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * StreamLabs USB Watchdog driver
> + *
> + * Copyright (c) 2016-2017,2022 Alexey Klimov <klimov.linux@gmail.com>
> + */
> +
> +#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 <klimov.linux@gmail.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
> +
Comment about the unit (ms) might be useful.

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


> +	return retval;
> +}
> +
> +static int usb_streamlabs_get_msg(struct usb_device *usbdev, __le16 *buf)
> +{
> +	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.

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

> +}
> +
> +static void usb_streamlabs_wdt_prepare_buf(__le16 *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);

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.

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

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

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

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

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Guenter Roeck]

On 7/25/22 01:51, Greg KH wrote:
> On Mon, Jul 25, 2022 at 04:06:05AM +0100, Alexey Klimov wrote:
>> +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.
> 

It is really intended to be a system configuration parameter
(CONFIG_WATCHDOG_NOWAYOUT) which can be overridden. Also, I think it still
makes sense to have it, even for removable devices. The ability to bypass
the flag by pulling the device would defeat its purpose.

Guenter

> 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 <gregkh@linuxfoundation.org>

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alexey Klimov]

On Mon, Jul 25, 2022 at 9:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> 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 <gregkh@linuxfoundation.org>

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.

Best regards,
Alexey

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alexey Klimov]

On Mon, Jul 25, 2022 at 3:02 PM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 7/24/22 20:06, Alexey Klimov wrote:

[...]

> > + * 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
> > +
> Comment about the unit (ms) might be useful.

Yes. I'll add it.

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

Good point. I'll change it.


> > +     return retval;
> > +}
> > +
> > +static int usb_streamlabs_get_msg(struct usb_device *usbdev, __le16 *buf)
> > +{
> > +     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.
>
> > +     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;
> > +
> > +     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.

> > +}
> > +
> > +static void usb_streamlabs_wdt_prepare_buf(__le16 *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);
>
> 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).

> > +     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;
> > +
> > +     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/5714E7D3.4030809@roeck-us.net/
https://lore.kernel.org/linux-watchdog/1460988518.25119.28.camel@suse.com/

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.

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

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Guenter Roeck]

On Tue, Jul 26, 2022 at 02:31:07AM +0100, Alexey Klimov wrote:
> On Mon, Jul 25, 2022 at 3:02 PM Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On 7/24/22 20:06, Alexey Klimov wrote:
> 
> [...]
> 
> > > + * 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
> > > +
> > Comment about the unit (ms) might be useful.
> 
> Yes. I'll add it.
> 
> > > +#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)
> > > +{
> > > +     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;
> 
> Good point. I'll change it.
> 
> 
> > > +     return retval;
> > > +}
> > > +
> > > +static int usb_streamlabs_get_msg(struct usb_device *usbdev, __le16 *buf)
> > > +{
> > > +     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.
> >
> > > +     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;
> > > +
> > > +     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.
> 
> > > +}
> > > +
> > > +static void usb_streamlabs_wdt_prepare_buf(__le16 *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);
> >
> > 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).
> 
That should not be necessary unless your code accesses data beyond the
amount of data received (which it should not do in the first place).

> > > +     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;
> > > +
> > > +     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/5714E7D3.4030809@roeck-us.net/
> https://lore.kernel.org/linux-watchdog/1460988518.25119.28.camel@suse.com/
> 
> 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.
> 

If you don't trust __cacheline_aligned, please add a respective comment
explaining that to avoid this from coming up again. 

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

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

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

Thanks,
Guenter

> Thank you for the review.
> 
> [...]
> 
> Best regards,
> Alexey

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Oliver Neukum]

On 26.07.22 02:21, Alexey Klimov wrote:
> On Mon, Jul 25, 2022 at 9:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> 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 <gregkh@linuxfoundation.org>
> 
> 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() ?

	Regards
		Oliver

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alexey Klimov]

On Tue, Jul 26, 2022 at 8:48 AM Oliver Neukum <oneukum@suse.com> wrote:
>
> On 26.07.22 02:21, Alexey Klimov wrote:
> > On Mon, Jul 25, 2022 at 9:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> >>
> >> 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 <gregkh@linuxfoundation.org>
> >
> > 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() ?

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

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Guenter Roeck]

On Sun, Jul 31, 2022 at 03:34:16AM +0100, Alexey Klimov wrote:
> On Tue, Jul 26, 2022 at 8:48 AM Oliver Neukum <oneukum@suse.com> wrote:
> >
> > On 26.07.22 02:21, Alexey Klimov wrote:
> > > On Mon, Jul 25, 2022 at 9:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >>
> > >> 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 <gregkh@linuxfoundation.org>
> > >
> > > 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() ?
> 
> 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?
> 
Not necessarily. Is other code doing something similar ?
Using a mutex like this creates the risk for hung tasks.

Guenter

> Best regards,
> Alexey

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alan Stern]

On Sun, Jul 31, 2022 at 01:20:55AM -0700, Guenter Roeck wrote:
> On Sun, Jul 31, 2022 at 03:34:16AM +0100, Alexey Klimov wrote:
> > On Tue, Jul 26, 2022 at 8:48 AM Oliver Neukum <oneukum@suse.com> wrote:
> > >
> > > On 26.07.22 02:21, Alexey Klimov wrote:
> > > > On Mon, Jul 25, 2022 at 9:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >>
> > > >> 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 <gregkh@linuxfoundation.org>
> > > >
> > > > 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() ?
> > 
> > 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?
> > 
> 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.

Alan Stern

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alexey Klimov]

On Tue, Jul 26, 2022 at 4:24 AM Guenter Roeck <linux@roeck-us.net> wrote:
>

[...]

> > > > +
> > > > +     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 -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".
>
> Should I have used the term "active" ? "started" ?
>
> > 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.

Thanks for the explanation. I was confused initially by the phrase
"explicitly stopped when running" since in reality it is "explicitly
stopped when stopped" (or not active, not running).
On the second thought, I can issue a start command and indicate to the
watchdog core via set_bit(WDOG_HW_RUNNING, &wdd->status) that it is
running or return -ENODEV if the start fails instead of stopping the
watchdog. I just would like to have a command sent to the device in
->probe() that checks that the device behaves like expected. If there
are no objects I'll change it like this.

Best regards,
Alexey

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alexey Klimov]

On Sun, Jul 31, 2022 at 9:20 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Sun, Jul 31, 2022 at 03:34:16AM +0100, Alexey Klimov wrote:
> > On Tue, Jul 26, 2022 at 8:48 AM Oliver Neukum <oneukum@suse.com> wrote:
> > >
> > > On 26.07.22 02:21, Alexey Klimov wrote:
> > > > On Mon, Jul 25, 2022 at 9:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > >>
> > > >> 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 <gregkh@linuxfoundation.org>
> > > >
> > > > 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() ?
> >
> > 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?
> >
> Not necessarily. Is other code doing something similar ?
> Using a mutex like this creates the risk for hung tasks.

If other code means drivers in tree, then yes.
Examples are drivers/usb/storage/usb.c, usbtmc.c, synaptics_usb.c,
vub300.c, zd_usb.c, usb-skeleton.c, etc.

I'll check the Alan's suggestion and will try to implement that.

Best regards,
Alexey

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Alexey Klimov]

Hi Alan,

Thank you for your responses here and in another email thread.

On Sun, Jul 31, 2022 at 3:17 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Sun, Jul 31, 2022 at 01:20:55AM -0700, Guenter Roeck wrote:
> > On Sun, Jul 31, 2022 at 03:34:16AM +0100, Alexey Klimov wrote:
> > > On Tue, Jul 26, 2022 at 8:48 AM Oliver Neukum <oneukum@suse.com> wrote:
> > > >
> > > > On 26.07.22 02:21, Alexey Klimov wrote:
> > > > > On Mon, Jul 25, 2022 at 9:51 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > > >>
> > > > >> 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 <gregkh@linuxfoundation.org>
> > > > >
> > > > > 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() ?
> > >
> > > 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?
> > >
> > 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.

Thanks for the idea, I'll look into implementing this.
Also, just to let you know there are a lot of drivers who do mutex
lock in pre_reset and mutex release in post_reset.
And there is 16-years old commit 47104b0dd32cec467574822b0dc3517b3de3f0ad
Maybe usb skeleton driver could be updated as well.

Best regards,
Alexey

Re: [PATCH v5] watchdog: add driver for StreamLabs USB watchdog device

[Oliver Neukum]

On 31.07.22 04:34, Alexey Klimov wrote:
> On Tue, Jul 26, 2022 at 8:48 AM Oliver Neukum <oneukum@suse.com> 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?

Hi,

I suppose if you make reenabling the watchdog conditional,
you may just as well do the same for disabling it.

	Regards
		Oliver