[v6] watchdog: add driver for StreamLabs USB watchdog device

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

--
2.37.2

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

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
>