linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v6] watchdog: add driver for StreamLabs USB watchdog device
@ 2022-09-17  3:05 Alexey Klimov
  2022-09-17  3:15 ` Alexey Klimov
  2022-09-22 14:41 ` Guenter Roeck
  0 siblings, 2 replies; 4+ messages in thread
From: Alexey Klimov @ 2022-09-17  3:05 UTC (permalink / raw)
  To: linux-watchdog
  Cc: wim, linux, gregkh, oneukum, linux-usb, linux-kernel, atishp,
	atishp, yury.norov, aklimov, atomlin, klimov.linux

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 v5:
	-- moved to semaphore lock: down_killable()/up();
	If I have to go with down_timeout() option, then
	I am not sure which timeout value to use, apart
	from maximum or current timeout from device;
	-- implemented {pre,post}_reset methods (thanks, Oliver!);
	-- back to the version where buffer is a part of
	struct streamlabs_wdt and now also ____cacheline_aligned;
	-- watchdog_start() in probe instead of watchdog_stop();
	-- added watchdog_stop_on_reboot() in probe;
	-- other comments from Guenter.

Previous version:
https://lore.kernel.org/linux-usb/CALW4P+LmMJAm=3PWQfiEgYS7d_q0M0rdSJVhpZw9x9w3fQQMRA@mail.gmail.com/T/


 MAINTAINERS                       |   6 +
 drivers/watchdog/Kconfig          |  15 ++
 drivers/watchdog/Makefile         |   1 +
 drivers/watchdog/streamlabs_wdt.c | 412 ++++++++++++++++++++++++++++++
 4 files changed, 434 insertions(+)
 create mode 100644 drivers/watchdog/streamlabs_wdt.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 936490dcc97b..e56fd4d636d1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19480,6 +19480,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 9295492d24f7..8baf53fef3cc 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -2180,6 +2180,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 cdeb119e6e61..afc6fdc1b4d9 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..cc4f48f1fd36
--- /dev/null
+++ b/drivers/watchdog/streamlabs_wdt.c
@@ -0,0 +1,412 @@
+// 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	/* 350 ms */
+
+#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 semaphore lock;
+	__le16 buffer[BUFFER_LENGTH] ____cacheline_aligned;
+};
+
+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 (retval)
+		return retval;
+
+	if (size != BUFFER_TRANSFER_LENGTH)
+		return -EIO;
+
+	return 0;
+}
+
+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 (retval)
+		return retval;
+
+	if (size != BUFFER_LENGTH)
+		return -EIO;
+
+	return 0;
+}
+
+/*
+ * 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;
+
+	return usb_streamlabs_wdt_check_timeout(buf, timeout_msec);
+}
+
+static void usb_streamlabs_wdt_prepare_buf(__le16 *buf, u16 cmd,
+					   unsigned long timeout_msec)
+{
+	memset(buf, 0, BUFFER_LENGTH);
+
+	buf[0] = cpu_to_le16(cmd);
+	buf[1] = cpu_to_le16(0x8000);
+	buf[3] = cpu_to_le16(timeout_msec);
+}
+
+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;
+
+	retval = down_killable(&streamlabs_wdt->lock);
+	if (retval)
+		return retval;
+	retval = __usb_streamlabs_wdt_cmd(streamlabs_wdt, cmd);
+	up(&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;
+
+	sema_init(&streamlabs_wdt->lock, 1);
+
+	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);
+	watchdog_stop_on_reboot(&streamlabs_wdt->wdt_dev);
+
+	retval = usb_streamlabs_wdt_start(&streamlabs_wdt->wdt_dev);
+	if (retval)
+		return -ENODEV;
+	set_bit(WDOG_HW_RUNNING, &streamlabs_wdt->wdt_dev.status);
+
+	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);
+	int retval;
+
+	retval = down_killable(&streamlabs_wdt->lock);
+	if (retval) {
+		dev_warn(&intf->dev,
+			 "failed to acquire semaphore in disconnect\n");
+		return;
+	}
+
+	/*
+	 * 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;
+	up(&streamlabs_wdt->lock);
+}
+
+static int usb_streamlabs_wdt_pre_reset(struct usb_interface *intf)
+{
+	struct streamlabs_wdt *streamlabs_wdt = usb_get_intfdata(intf);
+	int retval;
+
+	retval = down_killable(&streamlabs_wdt->lock);
+	if (retval) {
+		dev_warn(&intf->dev,
+			 "failed to acquire semaphore in pre_reset\n");
+		return retval;
+	}
+
+	if (watchdog_active(&streamlabs_wdt->wdt_dev)) {
+		retval =  __usb_streamlabs_wdt_cmd(streamlabs_wdt,
+						   STREAMLABS_CMD_STOP);
+		/*
+		 * If ->pre_reset() callback fails, the ->disconnect()
+		 * callback will be called. To let it work we should
+		 * unlock semaphore here.
+		 */
+		if (retval)
+			up(&streamlabs_wdt->lock);
+	}
+
+	return retval;
+}
+
+static int usb_streamlabs_wdt_post_reset(struct usb_interface *intf)
+{
+	struct streamlabs_wdt *streamlabs_wdt = usb_get_intfdata(intf);
+	int retval = 0;
+
+	/* FIXME: potential race here, watchdog_active() check is unreliable? */
+	if (watchdog_active(&streamlabs_wdt->wdt_dev))
+		retval = __usb_streamlabs_wdt_cmd(streamlabs_wdt,
+						  STREAMLABS_CMD_START);
+
+	up(&streamlabs_wdt->lock);
+
+	return retval;
+}
+
+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,
+	.pre_reset	= usb_streamlabs_wdt_pre_reset,
+	.post_reset	= usb_streamlabs_wdt_post_reset,
+	.id_table	= usb_streamlabs_wdt_device_table,
+	.soft_unbind	= 1,
+};
+
+module_usb_driver(usb_streamlabs_wdt_driver);
--
2.37.2


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH v6] watchdog: add driver for StreamLabs USB watchdog device
  2022-09-17  3:05 [PATCH v6] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
@ 2022-09-17  3:15 ` Alexey Klimov
  2022-09-22 14:37   ` Guenter Roeck
  2022-09-22 14:41 ` Guenter Roeck
  1 sibling, 1 reply; 4+ messages in thread
From: Alexey Klimov @ 2022-09-17  3:15 UTC (permalink / raw)
  To: linux, wim
  Cc: linux-watchdog, gregkh, oneukum, linux-usb, linux-kernel, atishp,
	atishp, yury.norov, aklimov, atomlin, stern, klimov.linux

Hi Wim/Guenter,

For me it seems that there could be a potential race condition. I have to rely
on watchdog_active(&streamlabs_wdt->wdt_dev) function which tests the WDOG_ACTIVE
bit in struct watchdog_device->status member.
The watchdog_dev changes the state of the device with ->start() or ->ping() and
->stop() methods and updates the WDOG_ACTIVE accordingly.
In {pre,post}_reset methods here I have to change the state of the device from
running to stopped and back to running conditionally, however WDOG_ACTIVE bit
could be updated in between these callbacks execution or starting/stopping
the device can race.
For instance, I see the potential dangerous race like this:

	CPUX					CPUY

	..				watchdog_stop() {
	..					if (wdd->ops->stop) {
							...
							err = wdd->ops->stop(wdd)
						}
usb_streamlabs_wdt_pre_reset() {
	if (watchdog_active())
		stop_command();			/* WDOG_ACTIVE bit is still set
	...					 here indicating that watchdog is
}						 started, but ->stop() has already
						 finished */
	...
usb_streamlabs_wdt_post_reset() {
	if (watchdog_active())
		start_command();
}
	...					/* WDOG_ACTIVE is updated here */
						clear_bit(WDOG_ACTIVE, &wdd->status);
					}

As a result, the watchdog subsystem "thinks" that watchdog is not active and should
not be pinged. However, the driver observed using watchdog_active() that watchdog
was active during {pre,post}_reset and restarted the device which will lead to
unexpected reset. It is very unlikely race to happen but consequence is fatal.
In other words, there are two independent paths leading to driver changing
the state of the watchdog device and one path relies on status that can be changed
by another path.

Thinking about that I see the following approaches:

1. Introduce a variable in struct streamlabs_wdt that tracks the state of the
watchdog device itself and checking/updating the state of a device happens under
semaphore lock.
Obviously, this "internal" to the driver state variable should be used in
{pre,post}_reset. In case there will be other drivers (say, USB ones) they also
need to implement this.

or

2. The updates to wdd->status should happen under wd_data->lock.
Currently, it is mutex-based. The acquiring and releasing the lock could be
exported for the drivers to use. The mutex lock probably should be switched
to a binary semaphore for that.

In such case, in pre_reset() for example, I would need to do:
static int pre_reset()
{
	lock_wdd();
	acquire_internal_driver_lock();
	
	if (watchdog_active())
		stop_command();
}

static int post_reset()
{

	if (watchdog_active())
		start_command();

	release_internal_driver_lock();
	unlock_wdd();
}

There should be an order that we have to acquire subsystem wdd lock first, then
internal driver lock. Otherwise there could be deadlocks.

This could be done if you think it's more wiser move.

or

3. The {pre,post}_reset callbacks should execute watchdog_dev.c subsystem functions
(not sure which functions exactly). Eventually, it will look similar to what is
described in the previous point with respect to locks order.
I meant something like this:

static int pre_reset()
{
	watchdog_dev_pre_reset_prepare();
}

static int post_reset()
{
	watchdog_dev_post_reset_done();
}

In watchdog_dev.c:
void watchdog_dev_pre_reset_prepare()
{
	mutex_lock(&wd_data->lock);	<-- should be changed to semaphore too?

	watchdog_stop(wdd);		<-- without updating WDOG_ACTIVE bit?
					 or there should be a way to indicate
					 to watchdog_dev_post_reset_done() if
					 watchdog should be started or not
}

void watchdog_dev_post_reset_done()
{
	if (watchdog_active())
		watchdog_start(wdd);

        mutex_unlock(&wd_data->lock);
}

I didn't really thought about point 3 yet. For me personally the point 2 seems
the like right way to go but you have more experience with that. The exported
locks could be re-used by other drivers if needed in future.
In case of point 1 each USB driver should deal with {pre,post}_reset by themselves.

Any thoughts?

Thanks,
Alexey

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v6] watchdog: add driver for StreamLabs USB watchdog device
  2022-09-17  3:15 ` Alexey Klimov
@ 2022-09-22 14:37   ` Guenter Roeck
  0 siblings, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2022-09-22 14:37 UTC (permalink / raw)
  To: Alexey Klimov, wim
  Cc: linux-watchdog, gregkh, oneukum, linux-usb, linux-kernel, atishp,
	atishp, yury.norov, aklimov, atomlin, stern

On 9/16/22 20:15, Alexey Klimov wrote:
> Hi Wim/Guenter,
> 
> For me it seems that there could be a potential race condition. I have to rely
> on watchdog_active(&streamlabs_wdt->wdt_dev) function which tests the WDOG_ACTIVE
> bit in struct watchdog_device->status member.
> The watchdog_dev changes the state of the device with ->start() or ->ping() and
> ->stop() methods and updates the WDOG_ACTIVE accordingly.
> In {pre,post}_reset methods here I have to change the state of the device from
> running to stopped and back to running conditionally, however WDOG_ACTIVE bit
> could be updated in between these callbacks execution or starting/stopping
> the device can race.
> For instance, I see the potential dangerous race like this:
> 
> 	CPUX					CPUY
> 
> 	..				watchdog_stop() {
> 	..					if (wdd->ops->stop) {
> 							...
> 							err = wdd->ops->stop(wdd)
> 						}
> usb_streamlabs_wdt_pre_reset() {
> 	if (watchdog_active())
> 		stop_command();			/* WDOG_ACTIVE bit is still set
> 	...					 here indicating that watchdog is
> }						 started, but ->stop() has already
> 						 finished */
> 	...
> usb_streamlabs_wdt_post_reset() {
> 	if (watchdog_active())
> 		start_command();
> }
> 	...					/* WDOG_ACTIVE is updated here */
> 						clear_bit(WDOG_ACTIVE, &wdd->status);
> 					}
> 
> As a result, the watchdog subsystem "thinks" that watchdog is not active and should
> not be pinged. However, the driver observed using watchdog_active() that watchdog
> was active during {pre,post}_reset and restarted the device which will lead to
> unexpected reset. It is very unlikely race to happen but consequence is fatal.
> In other words, there are two independent paths leading to driver changing
> the state of the watchdog device and one path relies on status that can be changed
> by another path.
> 
> Thinking about that I see the following approaches:
> 
> 1. Introduce a variable in struct streamlabs_wdt that tracks the state of the
> watchdog device itself and checking/updating the state of a device happens under
> semaphore lock.
> Obviously, this "internal" to the driver state variable should be used in
> {pre,post}_reset. In case there will be other drivers (say, USB ones) they also
> need to implement this.
> 
> or
> 
> 2. The updates to wdd->status should happen under wd_data->lock.
> Currently, it is mutex-based. The acquiring and releasing the lock could be
> exported for the drivers to use. The mutex lock probably should be switched
> to a binary semaphore for that.
> 
> In such case, in pre_reset() for example, I would need to do:
> static int pre_reset()
> {
> 	lock_wdd();
> 	acquire_internal_driver_lock();
> 	
> 	if (watchdog_active())
> 		stop_command();
> }
> 
> static int post_reset()
> {
> 
> 	if (watchdog_active())
> 		start_command();
> 
> 	release_internal_driver_lock();
> 	unlock_wdd();
> }
> 
> There should be an order that we have to acquire subsystem wdd lock first, then
> internal driver lock. Otherwise there could be deadlocks.
> 
> This could be done if you think it's more wiser move.
> 
> or
> 
> 3. The {pre,post}_reset callbacks should execute watchdog_dev.c subsystem functions
> (not sure which functions exactly). Eventually, it will look similar to what is
> described in the previous point with respect to locks order.
> I meant something like this:
> 
> static int pre_reset()
> {
> 	watchdog_dev_pre_reset_prepare();
> }
> 
> static int post_reset()
> {
> 	watchdog_dev_post_reset_done();
> }
> 
> In watchdog_dev.c:
> void watchdog_dev_pre_reset_prepare()
> {
> 	mutex_lock(&wd_data->lock);	<-- should be changed to semaphore too?
> 
> 	watchdog_stop(wdd);		<-- without updating WDOG_ACTIVE bit?
> 					 or there should be a way to indicate
> 					 to watchdog_dev_post_reset_done() if
> 					 watchdog should be started or not
> }
> 
> void watchdog_dev_post_reset_done()
> {
> 	if (watchdog_active())
> 		watchdog_start(wdd);
> 
>          mutex_unlock(&wd_data->lock);
> }
> 
> I didn't really thought about point 3 yet. For me personally the point 2 seems
> the like right way to go but you have more experience with that. The exported
> locks could be re-used by other drivers if needed in future.
> In case of point 1 each USB driver should deal with {pre,post}_reset by themselves.
> 
> Any thoughts?
> 

Please go with 1). pre_reset/post_reset functionality is a first in the watchdog
subsystem and the first to require locking outside the scope of a function or set
of functions. I'd rather avoid having to deal with the potential consequences
in the watchdog core. We can do that if/when it becomes more common and after
we have a good understanding of the potential consequences.

Thanks,
Guenter

> Thanks,
> Alexey


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v6] watchdog: add driver for StreamLabs USB watchdog device
  2022-09-17  3:05 [PATCH v6] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
  2022-09-17  3:15 ` Alexey Klimov
@ 2022-09-22 14:41 ` Guenter Roeck
  1 sibling, 0 replies; 4+ messages in thread
From: Guenter Roeck @ 2022-09-22 14:41 UTC (permalink / raw)
  To: Alexey Klimov, linux-watchdog
  Cc: wim, gregkh, oneukum, linux-usb, linux-kernel, atishp, atishp,
	yury.norov, aklimov, atomlin

On 9/16/22 20:05, 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 v5:
> 	-- moved to semaphore lock: down_killable()/up();
> 	If I have to go with down_timeout() option, then
> 	I am not sure which timeout value to use, apart
> 	from maximum or current timeout from device;
> 	-- implemented {pre,post}_reset methods (thanks, Oliver!);
> 	-- back to the version where buffer is a part of
> 	struct streamlabs_wdt and now also ____cacheline_aligned;
> 	-- watchdog_start() in probe instead of watchdog_stop();
> 	-- added watchdog_stop_on_reboot() in probe;
> 	-- other comments from Guenter.
> 
> Previous version:
> https://lore.kernel.org/linux-usb/CALW4P+LmMJAm=3PWQfiEgYS7d_q0M0rdSJVhpZw9x9w3fQQMRA@mail.gmail.com/T/
> 
> 
>   MAINTAINERS                       |   6 +
>   drivers/watchdog/Kconfig          |  15 ++
>   drivers/watchdog/Makefile         |   1 +
>   drivers/watchdog/streamlabs_wdt.c | 412 ++++++++++++++++++++++++++++++
>   4 files changed, 434 insertions(+)
>   create mode 100644 drivers/watchdog/streamlabs_wdt.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 936490dcc97b..e56fd4d636d1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19480,6 +19480,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 9295492d24f7..8baf53fef3cc 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -2180,6 +2180,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 cdeb119e6e61..afc6fdc1b4d9 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..cc4f48f1fd36
> --- /dev/null
> +++ b/drivers/watchdog/streamlabs_wdt.c
> @@ -0,0 +1,412 @@
> +// 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	/* 350 ms */
> +
> +#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 semaphore lock;
> +	__le16 buffer[BUFFER_LENGTH] ____cacheline_aligned;
> +};
> +
> +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 (retval)
> +		return retval;
> +
> +	if (size != BUFFER_TRANSFER_LENGTH)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +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 (retval)
> +		return retval;
> +
> +	if (size != BUFFER_LENGTH)
> +		return -EIO;
> +
> +	return 0;
> +}
> +
> +/*
> + * 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;
> +
> +	return usb_streamlabs_wdt_check_timeout(buf, timeout_msec);
> +}
> +
> +static void usb_streamlabs_wdt_prepare_buf(__le16 *buf, u16 cmd,
> +					   unsigned long timeout_msec)
> +{
> +	memset(buf, 0, BUFFER_LENGTH);
> +
> +	buf[0] = cpu_to_le16(cmd);
> +	buf[1] = cpu_to_le16(0x8000);
> +	buf[3] = cpu_to_le16(timeout_msec);
> +}
> +
> +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;
> +
> +	retval = down_killable(&streamlabs_wdt->lock);
> +	if (retval)
> +		return retval;
> +	retval = __usb_streamlabs_wdt_cmd(streamlabs_wdt, cmd);
> +	up(&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;
> +
> +	sema_init(&streamlabs_wdt->lock, 1);
> +
> +	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);
> +	watchdog_stop_on_reboot(&streamlabs_wdt->wdt_dev);
> +
> +	retval = usb_streamlabs_wdt_start(&streamlabs_wdt->wdt_dev);
> +	if (retval)
> +		return -ENODEV;
> +	set_bit(WDOG_HW_RUNNING, &streamlabs_wdt->wdt_dev.status);
> +
> +	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);
> +	int retval;
> +
> +	retval = down_killable(&streamlabs_wdt->lock);
> +	if (retval) {
> +		dev_warn(&intf->dev,
> +			 "failed to acquire semaphore in disconnect\n");
> +		return;
> +	}
> +
> +	/*
> +	 * 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;
> +	up(&streamlabs_wdt->lock);
> +}
> +
> +static int usb_streamlabs_wdt_pre_reset(struct usb_interface *intf)
> +{
> +	struct streamlabs_wdt *streamlabs_wdt = usb_get_intfdata(intf);
> +	int retval;
> +
> +	retval = down_killable(&streamlabs_wdt->lock);
> +	if (retval) {
> +		dev_warn(&intf->dev,
> +			 "failed to acquire semaphore in pre_reset\n");
> +		return retval;
> +	}
> +
> +	if (watchdog_active(&streamlabs_wdt->wdt_dev)) {
> +		retval =  __usb_streamlabs_wdt_cmd(streamlabs_wdt,
> +						   STREAMLABS_CMD_STOP);
> +		/*
> +		 * If ->pre_reset() callback fails, the ->disconnect()
> +		 * callback will be called. To let it work we should

should -> need to ?

It isn't really optional, or am I missing something ?

Also, is that the only condition where post_reset will never be called ?
How about some other disconnect condition experienced after pre_reset(),
even if pre_reset() itself was successful ? Or a shutdown after the call
to pre_reset() ?

> +		 * unlock semaphore here.
> +		 */
> +		if (retval)
> +			up(&streamlabs_wdt->lock);
> +	}
> +
> +	return retval;
> +}
> +
> +static int usb_streamlabs_wdt_post_reset(struct usb_interface *intf)
> +{
> +	struct streamlabs_wdt *streamlabs_wdt = usb_get_intfdata(intf);
> +	int retval = 0;
> +
> +	/* FIXME: potential race here, watchdog_active() check is unreliable? */

You'll need to handle that here.

> +	if (watchdog_active(&streamlabs_wdt->wdt_dev))
> +		retval = __usb_streamlabs_wdt_cmd(streamlabs_wdt,
> +						  STREAMLABS_CMD_START);
> +
> +	up(&streamlabs_wdt->lock);
> +
> +	return retval;
> +}
> +
> +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,
> +	.pre_reset	= usb_streamlabs_wdt_pre_reset,
> +	.post_reset	= usb_streamlabs_wdt_post_reset,
> +	.id_table	= usb_streamlabs_wdt_device_table,
> +	.soft_unbind	= 1,
> +};
> +
> +module_usb_driver(usb_streamlabs_wdt_driver);
> --
> 2.37.2
> 


^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-09-22 14:42 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-17  3:05 [PATCH v6] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
2022-09-17  3:15 ` Alexey Klimov
2022-09-22 14:37   ` Guenter Roeck
2022-09-22 14:41 ` Guenter Roeck

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).