watchdog: add driver for StreamLabs USB watchdog device
PATCH 1/1 watchdog: add driver for StreamLabs USB watchdog device
This patch creates new driver that supports StreamLabs usb watchdog device. This device plugs into 9-pin usb header and connects to reset pin and reset button on common PC.
USB commands used to communicate with device were reverse engineered using usbmon.
--- drivers/watchdog/Kconfig | 15 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/streamlabs_wdt.c | 370 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 386 insertions(+) create mode 100644 drivers/watchdog/streamlabs_wdt.c
drivers/watchdog/Kconfig +15 -0
@@ -1705,4 +1705,19 @@ config USBPCWATCHDOG Most people will say N. +config USB_STREAMLABS_WATCHDOG + tristate "StreamLabs USB watchdog driver" + depends on USB + ---help--- + This is the driver for the USB Watchdog dongle from StreamLabs. + If you correctly connect reset pins to motherboard Reset pin and + to Reset button then this device will simply watch your kernel to make + sure it doesn't freeze, and if it does, it reboots your computer + after a certain amount of time. + + + To compile this driver as a module, choose M here: the + module will be called streamlabs_wdt. + + Most people will say N. Say yes or M if you want to use such usb device. endif # WATCHDOG
drivers/watchdog/Makefile +1 -0
@@ -25,6 +25,7 @@ obj-$(CONFIG_WDTPCI) += wdt_pci.o # USB-based Watchdog Cards obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_USB_STREAMLABS_WATCHDOG) += streamlabs_wdt.o # ALPHA Architecture
drivers/watchdog/streamlabs_wdt.c new file +370 -0
370 lines not on this page · raw message
Signature · 1 line
2.7.0.rc3
Guenter Roeck
maintainerQuoted 91 lines · Alexey Klimov
This patch creates new driver that supports StreamLabs usb watchdog device. This device plugs into 9-pin usb header and connects to reset pin and reset button on common PC.
USB commands used to communicate with device were reverse engineered using usbmon.
Signed-off-by Alexey Klimov <[email protected]>--- drivers/watchdog/Kconfig | 15 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/streamlabs_wdt.c | 370 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 386 insertions(+)create mode 100644
drivers/watchdog/streamlabs_wdt.cdrivers/watchdog/Kconfig +15
-0@@ -1705,4 +1705,19 @@ config USBPCWATCHDOG Most people will say N. +config USB_STREAMLABS_WATCHDOG + tristate "StreamLabs USB watchdog driver" + depends on USB + ---help--- + This is the driver for the USB Watchdog dongle from StreamLabs. + If you correctly connect reset pins to motherboard Reset pin and + to Reset button then this device will simply watch your kernel to make + sure it doesn't freeze, and if it does, it reboots your computer + after a certain amount of time. + + + To compile this driver as a module, choose M here: the + module will be called streamlabs_wdt. + + Most people will say N. Say yes or M if you want to use such usb device. endif # WATCHDOGdrivers/watchdog/Makefile +1
-0@@ -25,6 +25,7 @@ obj-$(CONFIG_WDTPCI) += wdt_pci.o # USB-based Watchdog Cards obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_USB_STREAMLABS_WATCHDOG) += streamlabs_wdt.o # ALPHA Architecturedrivers/watchdog/streamlabs_wdt.c new file +34
-0@@ -0,0 +1,370 @@ +/* +* StreamLabs USB Watchdog driver +* +* Copyright (c) 2016 Alexey Klimov <[email protected]> +* +* This program is free software; you may redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/usb.h> +#include <linux/watchdog.h> + +/* +* USB Watchdog device from Streamlabs +* http://www.stream-labs.com/products/devices/watchdog/ +* +* USB commands have been reverse engineered using usbmon. +*/ + +#define DRIVER_AUTHOR "Alexey Klimov <[email protected]>" +#define DRIVER_DESC "StreamLabs USB watchdog driver" +#define DRIVER_NAME "usb_streamlabs_wdt"
drivers/watchdog/streamlabs_wdt.c line 44
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ +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 */
/*
* Please use proper multi-line comments throughout. */
Quoted 38 lines
drivers/watchdog/streamlabs_wdt.c new file +38
-0+#define BUFFER_TRANSFER_LENGTH 32 +#define BUFFER_LENGTH 64 +#define USB_TIMEOUT 350 + +#define STREAMLABS_CMD_START 0 +#define STREAMLABS_CMD_STOP 1 + +#define STREAMLABS_WDT_MIN_TIMEOUT 1 +#define STREAMLABS_WDT_MAX_TIMEOUT 46 + +struct streamlabs_wdt { +struct watchdog_device wdt_dev; +struct usb_device *usbdev; +struct usb_interface *intf; + +struct kref kref; +struct mutex lock; +u8 *buffer; +}; + +static bool nowayout =WATCHDOG_NOWAYOUT ; + +static int usb_streamlabs_wdt_validate_response (u8 *buf) +{ +/* If watchdog device understood the command it will acknowledge +* with values 1,2,3,4 at indexes 10, 11, 12, 13 in response message. +*/ +if (buf[10 ] !=1 || buf[11 ] !=2 || buf[12 ] !=3 || buf[13 ] !=4 ) +return -EINVAL ; + +return 0 ; +} + +static int usb_streamlabs_wdt_command (struct watchdog_device *wdt_dev,int cmd) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); +int retval; +int size;
drivers/watchdog/streamlabs_wdt.c line 92
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+unsigned long timeout_msec; +int retry_counter =10 ;/* how many times to re-send stop cmd */ + +mutex_lock (&streamlabs_wdt->lock); + + timeout_msec = wdt_dev->timeout *MSEC_PER_SEC ; + +/* Prepare message that will be sent to device. +* This buffer is allocated by kzalloc(). Only initialize required +* fields.
But only once, and overwritten by the response. So the comment is quite pointless and misleading.
Quoted 18 lines
drivers/watchdog/streamlabs_wdt.c new file +18
-0+*/ +if (cmd ==STREAMLABS_CMD_START ) { + streamlabs_wdt->buffer[0 ] =0xcc ; + streamlabs_wdt->buffer[1 ] =0xaa ; + }else {/* assume stop command if it's not start */ + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + } + + streamlabs_wdt->buffer[3 ] =0x80 ; + + streamlabs_wdt->buffer[6 ] = (timeout_msec &0xff ) <<8 ; + streamlabs_wdt->buffer[7 ] = (timeout_msec &0xff00 ) >>8 ; +retry: + streamlabs_wdt->buffer[10 ] =0x00 ; + streamlabs_wdt->buffer[11 ] =0x00 ; + streamlabs_wdt->buffer[12 ] =0x00 ; + streamlabs_wdt->buffer[13 ] =0x00 ;
drivers/watchdog/streamlabs_wdt.c line 120
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ +/* send command to watchdog */ + retval =usb_interrupt_msg (streamlabs_wdt->usbdev, +usb_sndintpipe (streamlabs_wdt->usbdev,0x02 ), + streamlabs_wdt->buffer,BUFFER_TRANSFER_LENGTH , + &size,USB_TIMEOUT ); + +if (retval || size !=BUFFER_TRANSFER_LENGTH ) { +dev_err (&streamlabs_wdt->intf->dev, +"error %i when submitting interrupt msg\n" , retval);
Please no error messages if something goes wrong. We don't want to fill the kernel log with those messages.
Quoted 38 lines
drivers/watchdog/streamlabs_wdt.c new file +38
-0+ retval = -EIO ; + goto out; + } + +/* and read response from watchdog */ + retval =usb_interrupt_msg (streamlabs_wdt->usbdev, +usb_rcvintpipe (streamlabs_wdt->usbdev,0x81 ), + streamlabs_wdt->buffer,BUFFER_LENGTH , + &size,USB_TIMEOUT ); + +if (retval || size !=BUFFER_LENGTH ) { +dev_err (&streamlabs_wdt->intf->dev, +"error %i when receiving interrupt msg\n" , retval); + retval = -EIO ; + goto out; + } + +/* check if watchdog actually acked/recognized command */ + retval =usb_streamlabs_wdt_validate_response (streamlabs_wdt->buffer); +if (retval) { +dev_err (&streamlabs_wdt->intf->dev, +"watchdog didn't ACK command!\n" ); + goto out; + } + +/* Transition from enabled to disabled state in this device +* doesn't happen immediately. Usually, 2 or 3 (sometimes even 4) stop +* commands should be sent until watchdog answers 'I'm stopped!'. +* Retry stop command if watchdog fails to answer correctly +* about its state. After 10 attempts, report error and return -EIO. +*/ +if (cmd ==STREAMLABS_CMD_STOP ) { +if (--retry_counter <=0 ) { +dev_err (&streamlabs_wdt->intf->dev, +"failed to stop watchdog after 9 attempts!\n" ); + retval = -EIO ; + goto out; + }
drivers/watchdog/streamlabs_wdt.c line 168
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+/* +* Check if watchdog actually changed state to disabled. +* If watchdog is still enabled then reset message and retry +* stop command. +*/ +if (streamlabs_wdt->buffer[0 ] !=0xff || + streamlabs_wdt->buffer[1 ] !=0xbb ) { + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + goto retry;
Can you use a loop instead ? goto's are ok when needed, but here it just makes the code more difficult to read.
Quoted 16 lines
drivers/watchdog/streamlabs_wdt.c new file +16
-0+ } + } + +out: +mutex_unlock (&streamlabs_wdt->lock); +return retval; +} + +static int usb_streamlabs_wdt_start (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_START ); +} + +static int usb_streamlabs_wdt_stop (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_STOP );
drivers/watchdog/streamlabs_wdt.c line 194
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+} + +static int usb_streamlabs_wdt_settimeout (struct watchdog_device *wdt_dev, +unsigned int timeout) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +mutex_lock (&streamlabs_wdt->lock); + wdt_dev->timeout = timeout; +mutex_unlock (&streamlabs_wdt->lock);
I don't think this mutex protection is needed.
Note that there is a patch pending (and will make it into 4.6) which will make the set_timeout function optional if all it does is to set the timeout variable.
Quoted 20 lines
drivers/watchdog/streamlabs_wdt.c new file +20
-0+ +return 0 ; +} + +static void usb_streamlabs_wdt_release_resources (struct kref *kref) +{ +struct streamlabs_wdt *streamlabs_wdt = + container_of(kref, struct streamlabs_wdt, kref); + +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); +kfree (streamlabs_wdt); +} + +static void usb_streamlabs_wdt_ref (struct watchdog_device *wdt_dev) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +kref_get (&streamlabs_wdt->kref); +}
drivers/watchdog/streamlabs_wdt.c line 224
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ +static void usb_streamlabs_wdt_unref (struct watchdog_device *wdt_dev) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +kref_put (&streamlabs_wdt->kref, usb_streamlabs_wdt_release_resources); +} + +static const struct watchdog_info streamlabs_wdt_ident = { + .options = (WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING ),
Unnecessary ( )
drivers/watchdog/streamlabs_wdt.c line 234
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ .identity = DRIVER_NAME, +}; + +static struct watchdog_ops usb_streamlabs_wdt_ops = { + .owner =THIS_MODULE , + .start = usb_streamlabs_wdt_start, + .stop = usb_streamlabs_wdt_stop, + .set_timeout = usb_streamlabs_wdt_settimeout, + .ref = usb_streamlabs_wdt_ref, + .unref = usb_streamlabs_wdt_unref,
Those functions no longer exist (and are no longer needed) in the latest kernel.
Quoted 22 lines
drivers/watchdog/streamlabs_wdt.c new file +22
-0+}; + +static int usb_streamlabs_wdt_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{ +struct usb_device *dev =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 (dev->product && dev->manufacturer && + (strncmp (dev->product,"USBkit" ,6 ) !=0 + ||strncmp (dev->manufacturer,"STREAM LABS" ,11 ) !=0 )) +return -ENODEV ; + + streamlabs_wdt =kmalloc (sizeof (struct streamlabs_wdt ),GFP_KERNEL ); +if (!streamlabs_wdt) { +dev_err (&intf->dev,"kmalloc failed\n" );
drivers/watchdog/streamlabs_wdt.c line 266
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+return -ENOMEM ; + } + + streamlabs_wdt->buffer =kzalloc (BUFFER_LENGTH ,GFP_KERNEL ); +if (!streamlabs_wdt->buffer) { +dev_err (&intf->dev,"kzalloc for watchdog->buffer failed\n" ); + retval = -ENOMEM ; + goto err_nobuf; + } +
Please consider using devm_kzalloc().
Also, is it necessary to allocate the buffer separately instead of using a (possibly cache line aligned) array in struct streamlabs_wdt ?
Quoted 6 lines
drivers/watchdog/streamlabs_wdt.c new file +6
-0+mutex_init (&streamlabs_wdt->lock); + + streamlabs_wdt->wdt_dev.info = &streamlabs_wdt_ident; + streamlabs_wdt->wdt_dev.ops = &usb_streamlabs_wdt_ops; + streamlabs_wdt->wdt_dev.timeout =STREAMLABS_WDT_MAX_TIMEOUT ; + streamlabs_wdt->wdt_dev.max_timeout =STREAMLABS_WDT_MAX_TIMEOUT ;
drivers/watchdog/streamlabs_wdt.c line 282
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ streamlabs_wdt->wdt_dev.min_timeout =STREAMLABS_WDT_MIN_TIMEOUT ; + streamlabs_wdt->wdt_dev.parent = &intf->dev; + + streamlabs_wdt->usbdev =interface_to_usbdev (intf); + streamlabs_wdt->intf = intf; +usb_set_intfdata (intf, &streamlabs_wdt->wdt_dev); +watchdog_set_drvdata (&streamlabs_wdt->wdt_dev, streamlabs_wdt); + +watchdog_init_timeout (&streamlabs_wdt->wdt_dev, + streamlabs_wdt->wdt_dev.timeout, &intf->dev);
This is quite pointless, since timeout is already set, and you don't have a module parameter to overwrite it.
Quoted 27 lines
drivers/watchdog/streamlabs_wdt.c new file +27
-0+watchdog_set_nowayout (&streamlabs_wdt->wdt_dev, nowayout); + +kref_init (&streamlabs_wdt->kref); + + retval =usb_streamlabs_wdt_stop (&streamlabs_wdt->wdt_dev); +if (retval) + goto err_wdt_buf; + + retval =watchdog_register_device (&streamlabs_wdt->wdt_dev); +if (retval) { +dev_err (&intf->dev,"failed to register watchdog device\n" ); + goto err_wdt_buf; + } + +dev_info (&intf->dev,"StreamLabs USB watchdog loaded.\n" ); + +return 0 ; + +err_wdt_buf: +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); + +err_nobuf: +kfree (streamlabs_wdt); +return retval; +} +
drivers/watchdog/streamlabs_wdt.c line 319
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-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)) +usb_streamlabs_wdt_command (&streamlabs_wdt->wdt_dev, +STREAMLABS_CMD_STOP );
Please call usb_streamlabs_wdt_stop().
drivers/watchdog/streamlabs_wdt.c line 330
Quoted 11 lines
drivers/watchdog/streamlabs_wdt.c new file +11
-0+ +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_command (&streamlabs_wdt->wdt_dev, +STREAMLABS_CMD_START );
Please call usb_streamlabs_wdt_start().
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ +return 0 ; +} +
drivers/watchdog/streamlabs_wdt.c line 344
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+static void usb_streamlabs_wdt_disconnect (struct usb_interface *intf) +{ +struct streamlabs_wdt *streamlabs_wdt =usb_get_intfdata (intf); + +/* First, stop sending USB messages to device. */ +mutex_lock (&streamlabs_wdt->lock); +usb_set_intfdata (intf,NULL ); + streamlabs_wdt->usbdev =NULL ; +mutex_unlock (&streamlabs_wdt->lock); +
If user space has a keepalive pending (waiting for the mutex being released in usb_streamlabs_wdt_command()), this may result in a call to usb_interrupt_msg() with the device set to NULL. The mutex protection doesn't add any value since you don't check if ->usbdev is NULL in usb_streamlabs_wdt_command().
Quoted 27 lines
drivers/watchdog/streamlabs_wdt.c new file +26
-0+/* after commincation with device has stopped we can +* unregister watchdog device. unref callback will clear the rest on +* release of device if it was opened. +*/ +watchdog_unregister_device (&streamlabs_wdt->wdt_dev); +kref_put (&streamlabs_wdt->kref, usb_streamlabs_wdt_release_resources); +} + +static struct usb_device_id usb_streamlabs_wdt_device_table[] = { + {USB_DEVICE (USB_STREAMLABS_WATCHDOG_VENDOR ,USB_STREAMLABS_WATCHDOG_PRODUCT ) }, + { }/* Terminating entry */ +}; + +MODULE_DEVICE_TABLE (usb, usb_streamlabs_wdt_device_table); + +static struct usb_driver usb_streamlabs_wdt_driver = { + .name =DRIVER_NAME , + .probe = usb_streamlabs_wdt_probe, + .disconnect = usb_streamlabs_wdt_disconnect, + .suspend = usb_streamlabs_wdt_suspend, + .resume = usb_streamlabs_wdt_resume, + .reset_resume = usb_streamlabs_wdt_resume, + .id_table = usb_streamlabs_wdt_device_table, +}; + +module_usb_driver (usb_streamlabs_wdt_driver);
Hi Guenter,
Quoted 113 lines · Guenter Roeck
Quoted 105 lines · Alexey Klimov
This patch creates new driver that supports StreamLabs usb watchdog device. This device plugs into 9-pin usb header and connects to reset pin and reset button on common PC.
USB commands used to communicate with device were reverse engineered using usbmon.
Signed-off-by Alexey Klimov <[email protected]>--- drivers/watchdog/Kconfig | 15 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/streamlabs_wdt.c | 370 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 386 insertions(+)create mode 100644
drivers/watchdog/streamlabs_wdt.cdrivers/watchdog/Kconfig +7
-0@@ -1705,4 +1705,19 @@ config USBPCWATCHDOG Most people will say N. +config USB_STREAMLABS_WATCHDOG + tristate "StreamLabs USB watchdog driver" + depends on USB + ---help--- + This is the driver for the USB Watchdog dongle from StreamLabs. + If you correctly connect reset pins to motherboard Reset pin and + to Reset button then this device will simply watch your kernelto make
drivers/watchdog/Kconfig +8
-0+ 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 usbdevice.
endif # WATCHDOGdrivers/watchdog/Makefile +1
-0@@ -25,6 +25,7 @@ obj-$(CONFIG_WDTPCI) += wdt_pci.o # USB-based Watchdog Cards obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_USB_STREAMLABS_WATCHDOG) += streamlabs_wdt.o # ALPHA Architecturedrivers/watchdog/streamlabs_wdt.c new file +43
-0@@ -0,0 +1,370 @@ +/* +* StreamLabs USB Watchdog driver +* +* Copyright (c) 2016 Alexey Klimov <[email protected]> +* +* This program is free software; you may redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/usb.h> +#include <linux/watchdog.h> + +/* +* USB Watchdog device from Streamlabs +* http://www.stream-labs.com/products/devices/watchdog/ +* +* USB commands have been reverse engineered using usbmon. +*/ + +#define DRIVER_AUTHOR "Alexey Klimov <[email protected]>" +#define DRIVER_DESC "StreamLabs USB watchdog driver" +#define DRIVER_NAME "usb_streamlabs_wdt" + +MODULE_AUTHOR (DRIVER_AUTHOR ); +MODULE_DESCRIPTION (DRIVER_DESC ); +MODULE_LICENSE ("GPL" ); + +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0 +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011 + +/* one buffer is used for communication, however transmitted message is Quoted 2 lines
only
+ * 32 bytes long *//*
* Please use proper multi-line comments throughout.
Quoted 1 line
*/
Ok, will fix them all.
Quoted 49 lines
Quoted 49 lines
drivers/watchdog/streamlabs_wdt.c new file +26
-0+#define BUFFER_TRANSFER_LENGTH 32 +#define BUFFER_LENGTH 64 +#define USB_TIMEOUT 350 + +#define STREAMLABS_CMD_START 0 +#define STREAMLABS_CMD_STOP 1 + +#define STREAMLABS_WDT_MIN_TIMEOUT 1 +#define STREAMLABS_WDT_MAX_TIMEOUT 46 + +struct streamlabs_wdt { +struct watchdog_device wdt_dev; +struct usb_device *usbdev; +struct usb_interface *intf; + +struct kref kref; +struct mutex lock; +u8 *buffer; +}; + +static bool nowayout =WATCHDOG_NOWAYOUT ; + +static int usb_streamlabs_wdt_validate_response (u8 *buf) +{ +/* If watchdog device understood the command it will acknowledge +* with values 1,2,3,4 at indexes 10, 11, 12, 13 in response message.
drivers/watchdog/streamlabs_wdt.c new file +8
-0+*/ +if (buf[10 ] !=1 || buf[11 ] !=2 || buf[12 ] !=3 || buf[13 ] !=4 ) +return -EINVAL ; + +return 0 ; +} + +static int usb_streamlabs_wdt_command(struct watchdog_device *wdt_dev,int cmd)
drivers/watchdog/streamlabs_wdt.c new file +2
-0+{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c new file +4
-0+int retval; +int size; +unsigned long timeout_msec; +int retry_counter =10 ;/* how many times to re-send stop cmd */
drivers/watchdog/streamlabs_wdt.c new file +5
-0+ +mutex_lock (&streamlabs_wdt->lock); + + timeout_msec = wdt_dev->timeout *MSEC_PER_SEC ; +
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 92
Quoted 3 lines
drivers/watchdog/streamlabs_wdt.c new file +3
-0+/* Prepare message that will be sent to device. +* This buffer is allocated by kzalloc(). Only initialize required +* fields. But only once, and overwritten by the response. So the comment is quite pointless
and misleading.
Ok, I will do something with this comment during re-work and rebase.
Quoted 27 lines
Quoted 27 lines
drivers/watchdog/streamlabs_wdt.c new file +22
-0+*/ +if (cmd ==STREAMLABS_CMD_START ) { + streamlabs_wdt->buffer[0 ] =0xcc ; + streamlabs_wdt->buffer[1 ] =0xaa ; + }else {/* assume stop command if it's not start */ + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + } + + streamlabs_wdt->buffer[3 ] =0x80 ; + + streamlabs_wdt->buffer[6 ] = (timeout_msec &0xff ) <<8 ; + streamlabs_wdt->buffer[7 ] = (timeout_msec &0xff00 ) >>8 ; +retry: + streamlabs_wdt->buffer[10 ] =0x00 ; + streamlabs_wdt->buffer[11 ] =0x00 ; + streamlabs_wdt->buffer[12 ] =0x00 ; + streamlabs_wdt->buffer[13 ] =0x00 ; + +/* send command to watchdog */ + retval = usb_interrupt_msg(streamlabs_wdt->usbdev, + usb_sndintpipe(streamlabs_wdt->usbdev,
0x02),
+streamlabs_wdt->buffer,BUFFER_TRANSFER_LENGTH,drivers/watchdog/streamlabs_wdt.c new file +2
-0+ &size, USB_TIMEOUT); +
Quoted 8 lines
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +3
-0+if (retval || size !=BUFFER_TRANSFER_LENGTH ) { + dev_err(&streamlabs_wdt->intf->dev, + "error %i when submitting interrupt msg\n",retval);
Please no error messages if something goes wrong. We don't want to fill the kernel log with those messages.
Ok, will remove them. Or is it fine to convert them to dev_dbg?
Quoted 52 lines
Quoted 52 lines
drivers/watchdog/streamlabs_wdt.c new file +7
-0+ retval = -EIO ; + goto out; + } + +/* and read response from watchdog */ + retval = usb_interrupt_msg(streamlabs_wdt->usbdev, + usb_rcvintpipe(streamlabs_wdt->usbdev,
0x81),drivers/watchdog/streamlabs_wdt.c new file +6
-0+ streamlabs_wdt->buffer,BUFFER_LENGTH , + &size, USB_TIMEOUT); + +if (retval || size !=BUFFER_LENGTH ) { + dev_err(&streamlabs_wdt->intf->dev, + "error %i when receiving interrupt msg\n",retval);
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ retval = -EIO ; + goto out; + } + +/* check if watchdog actually acked/recognized command */ + retval =
usb_streamlabs_wdt_validate_response(streamlabs_wdt->buffer);drivers/watchdog/streamlabs_wdt.c new file +8
-0+if (retval) { +dev_err (&streamlabs_wdt->intf->dev, +"watchdog didn't ACK command!\n" ); + goto out; + } + +/* Transition from enabled to disabled state in this device +* doesn't happen immediately. Usually, 2 or 3 (sometimes even 4) stop
drivers/watchdog/streamlabs_wdt.c new file +8
-1+* commands should be sent until watchdog answers 'I'm stopped!'. +* Retry stop command if watchdog fails to answer correctly +* about its state. After 10 attempts, report error and return -EIO. +*/ +if (cmd ==STREAMLABS_CMD_STOP ) { +if (--retry_counter <=0 ) { + dev_err(&streamlabs_wdt->intf->dev, + "failed to stop watchdog after 9attempts!\n");
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ retval = -EIO ; + goto out; + } +/* +* Check if watchdog actually changed state to disabled. +* If watchdog is still enabled then reset message and retry
drivers/watchdog/streamlabs_wdt.c new file +4
-0+* stop command. +*/ +if (streamlabs_wdt->buffer[0 ] !=0xff || + streamlabs_wdt->buffer[1 ] !=0xbb )
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 168
Quoted 4 lines
{
drivers/watchdog/streamlabs_wdt.c new file +3
-0+ streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + goto retry;Can you use a loop instead ? goto's are ok when needed, but here it just makes the code more difficult to read.
Sure.
Quoted 31 lines
Quoted 23 lines
drivers/watchdog/streamlabs_wdt.c new file +22
-0+ } + } + +out: +mutex_unlock (&streamlabs_wdt->lock); +return retval; +} + +static int usb_streamlabs_wdt_start (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_START ); +} + +static int usb_streamlabs_wdt_stop (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_STOP ); +} + +static int usb_streamlabs_wdt_settimeout (struct watchdog_device *wdt_dev, +unsigned int timeout) +{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c line 194
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ +mutex_lock (&streamlabs_wdt->lock); + wdt_dev->timeout = timeout; +mutex_unlock (&streamlabs_wdt->lock);I don't think this mutex protection is needed.
Quoted 3 lines
Note that there is a patch pending (and will make it into 4.6) which will make the
set_timeoutfunction optional if all it does is to set the timeout variable.
Here comes some pain. For last week I tried to clonegit://www.linux-watchdog.org/linux-watchdog-next.git (I hope this is correct address) to rebase on top of 4.6 and test but almost always I got this after counting:
fatal: read error: Connection timed out
fatal: early EOF
fatal: index-pack failed
and counting takes 3-4 hours.
After all I cloned it (and www.linux-watchdog.org looks more healthy in last couple of days) but merge window should be opened soon so I will get new things in two weeks anyway. I think I will be able to rebase on 4.6-rc1 or near and re-send.
Looks like I'm not the first one who have troubles withgit://www.linux-watchdog.org
http://www.spinics.net/lists/linux-watchdog/msg07384.html
Quoted 28 lines
Quoted 28 lines
drivers/watchdog/streamlabs_wdt.c new file +17
-0+ +return 0 ; +} + +static void usb_streamlabs_wdt_release_resources (struct kref *kref) +{ +struct streamlabs_wdt *streamlabs_wdt = + container_of(kref, struct streamlabs_wdt, kref); + +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); +kfree (streamlabs_wdt); +} + +static void usb_streamlabs_wdt_ref (struct watchdog_device *wdt_dev) +{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c new file +7
-0+ +kref_get (&streamlabs_wdt->kref); +} + +static void usb_streamlabs_wdt_unref (struct watchdog_device *wdt_dev) +{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c new file +2
-0+ + kref_put(&streamlabs_wdt->kref,
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 224
Quoted 5 lines
usb_streamlabs_wdt_release_resources);drivers/watchdog/streamlabs_wdt.c new file +4
-0+} + +static const struct watchdog_info streamlabs_wdt_ident = { + .options = (WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING ),Unnecessary ( )
Ok.
Quoted 6 lines
Quoted 6 lines
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ .identity = DRIVER_NAME, +}; + +static struct watchdog_ops usb_streamlabs_wdt_ops = { + .owner =THIS_MODULE , + .start = usb_streamlabs_wdt_start,
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 234
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ .stop = usb_streamlabs_wdt_stop, + .set_timeout = usb_streamlabs_wdt_settimeout, + .ref = usb_streamlabs_wdt_ref, + .unref = usb_streamlabs_wdt_unref,Those functions no longer exist (and are no longer needed) in the latest kernel.
Got it.
Quoted 40 lines
Quoted 32 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+}; + +static int usb_streamlabs_wdt_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{ +struct usb_device *dev =interface_to_usbdev (intf); +struct streamlabs_wdt *streamlabs_wdt; +int retval; + +/* USB IDs of this device appear to be weird/unregistered. Hence, do
drivers/watchdog/streamlabs_wdt.c new file +3
-0+* 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
drivers/watchdog/streamlabs_wdt.c new file +7
-0+* behaves as a watchdog. */ +if (dev->product && dev->manufacturer && + (strncmp (dev->product,"USBkit" ,6 ) !=0 + ||strncmp (dev->manufacturer,"STREAM LABS" ,11 ) !=0 )) +return -ENODEV ; + + streamlabs_wdt = kmalloc(sizeof(struct streamlabs_wdt),
GFP_KERNEL);drivers/watchdog/streamlabs_wdt.c new file +8
-0+if (!streamlabs_wdt) { +dev_err (&intf->dev,"kmalloc failed\n" ); +return -ENOMEM ; + } + + streamlabs_wdt->buffer =kzalloc (BUFFER_LENGTH ,GFP_KERNEL ); +if (!streamlabs_wdt->buffer) { + dev_err(&intf->dev, "kzalloc for watchdog->bufferfailed\n");
drivers/watchdog/streamlabs_wdt.c line 266
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ retval = -ENOMEM ; + goto err_nobuf; + } +Please consider using
devm_kzalloc().
Quoted 2 lines
Also, is it necessary to allocate the buffer separately instead of using a (possibly cache line aligned) array in struct
streamlabs_wdt?
Thanks. Already moved to devm_kzalloc() and moved array inside structure instead of allocating buffer separately.
Quoted 13 lines
Quoted 13 lines
drivers/watchdog/streamlabs_wdt.c new file +13
-0+mutex_init (&streamlabs_wdt->lock); + + streamlabs_wdt->wdt_dev.info = &streamlabs_wdt_ident; + streamlabs_wdt->wdt_dev.ops = &usb_streamlabs_wdt_ops; + streamlabs_wdt->wdt_dev.timeout =STREAMLABS_WDT_MAX_TIMEOUT ; + streamlabs_wdt->wdt_dev.max_timeout =STREAMLABS_WDT_MAX_TIMEOUT ; + streamlabs_wdt->wdt_dev.min_timeout =STREAMLABS_WDT_MIN_TIMEOUT ; + streamlabs_wdt->wdt_dev.parent = &intf->dev; + + streamlabs_wdt->usbdev =interface_to_usbdev (intf); + streamlabs_wdt->intf = intf; +usb_set_intfdata (intf, &streamlabs_wdt->wdt_dev); +watchdog_set_drvdata (&streamlabs_wdt->wdt_dev, streamlabs_wdt);
Quoted 8 lines
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +3
-0+ + watchdog_init_timeout(&streamlabs_wdt->wdt_dev, + streamlabs_wdt->wdt_dev.timeout,
&intf->dev);This is quite pointless, since timeout is already set, and you don't have a module parameter to overwrite it.
Gonna remove it.
Quoted 43 lines
Quoted 38 lines
drivers/watchdog/streamlabs_wdt.c new file +11
-0+watchdog_set_nowayout (&streamlabs_wdt->wdt_dev, nowayout); + +kref_init (&streamlabs_wdt->kref); + + retval =usb_streamlabs_wdt_stop (&streamlabs_wdt->wdt_dev); +if (retval) + goto err_wdt_buf; + + retval =watchdog_register_device (&streamlabs_wdt->wdt_dev); +if (retval) { + dev_err(&intf->dev, "failed to register watchdogdevice\n");
drivers/watchdog/streamlabs_wdt.c new file +26
-0+ goto err_wdt_buf; + } + +dev_info (&intf->dev,"StreamLabs USB watchdog loaded.\n" ); + +return 0 ; + +err_wdt_buf: +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); + +err_nobuf: +kfree (streamlabs_wdt); +return retval; +} + + + +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)) + usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev, +Quoted 1 line
STREAMLABS_CMD_STOP);Please call
usb_streamlabs_wdt_stop().
Quoted 16 lines
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ +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 Quoted 3 lines
usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev, +
STREAMLABS_CMD_START);Please call
usb_streamlabs_wdt_start().
Thanks, I will add required calls. Out of curiosity, what about driver(s) that checks watchdog status in suspend/resume and calls stop/start only if watchdog active?
Quoted 12 lines
Quoted 12 lines
drivers/watchdog/streamlabs_wdt.c new file +12
-0+ +return 0 ; +} + +static void usb_streamlabs_wdt_disconnect (struct usb_interface *intf) +{ +struct streamlabs_wdt *streamlabs_wdt =usb_get_intfdata (intf); + +/* First, stop sending USB messages to device. */ +mutex_lock (&streamlabs_wdt->lock); +usb_set_intfdata (intf,NULL ); + streamlabs_wdt->usbdev =NULL ;
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c
Quoted 2 lines
drivers/watchdog/streamlabs_wdt.c new file +2
-0+mutex_unlock (&streamlabs_wdt->lock); +If user space has a keepalive pending (waiting for the mutex being released in
usb_streamlabs_wdt_command()), this may result in a call tousb_interrupt_msg()with the device set to NULL. The mutex protection doesn't add any value since you don't check if ->usbdev is NULL inusb_streamlabs_wdt_command().
In another email Oliver adviced to remove ->usbdev. So, I use ->intf and check it for NULL in usb_streamlabs_wdt_command(). I also do usb_set_intfdata(intf, NULL);streamlabs_wdt->intf = NULL;
here with mutex locked. Hope it's fine.
Thanks for review.
Signature · 1 line
Best regards, Klimov Alexey
Guenter Roeck
maintainerHi Alexey,
Quoted 217 lines · Alexey Klimov
Hi Guenter,
Quoted 113 lines · Guenter Roeck
Quoted 105 lines · Alexey Klimov
This patch creates new driver that supports StreamLabs usb watchdog device. This device plugs into 9-pin usb header and connects to reset pin and reset button on common PC.
USB commands used to communicate with device were reverse engineered using usbmon.
Signed-off-by Alexey Klimov <[email protected]>--- drivers/watchdog/Kconfig | 15 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/streamlabs_wdt.c | 370 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 386 insertions(+)create mode 100644
drivers/watchdog/streamlabs_wdt.cdrivers/watchdog/Kconfig +7
-0@@ -1705,4 +1705,19 @@ config USBPCWATCHDOG Most people will say N. +config USB_STREAMLABS_WATCHDOG + tristate "StreamLabs USB watchdog driver" + depends on USB + ---help--- + This is the driver for the USB Watchdog dongle from StreamLabs. + If you correctly connect reset pins to motherboard Reset pin and + to Reset button then this device will simply watch your kernelto make
drivers/watchdog/Kconfig +8
-0+ 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 usbdevice.
endif # WATCHDOGdrivers/watchdog/Makefile +1
-0@@ -25,6 +25,7 @@ obj-$(CONFIG_WDTPCI) += wdt_pci.o # USB-based Watchdog Cards obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_USB_STREAMLABS_WATCHDOG) += streamlabs_wdt.o # ALPHA Architecturedrivers/watchdog/streamlabs_wdt.c new file +43
-0@@ -0,0 +1,370 @@ +/* +* StreamLabs USB Watchdog driver +* +* Copyright (c) 2016 Alexey Klimov <[email protected]> +* +* This program is free software; you may redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/usb.h> +#include <linux/watchdog.h> + +/* +* USB Watchdog device from Streamlabs +* http://www.stream-labs.com/products/devices/watchdog/ +* +* USB commands have been reverse engineered using usbmon. +*/ + +#define DRIVER_AUTHOR "Alexey Klimov <[email protected]>" +#define DRIVER_DESC "StreamLabs USB watchdog driver" +#define DRIVER_NAME "usb_streamlabs_wdt" + +MODULE_AUTHOR (DRIVER_AUTHOR ); +MODULE_DESCRIPTION (DRIVER_DESC ); +MODULE_LICENSE ("GPL" ); + +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0 +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011 + +/* one buffer is used for communication, however transmitted message is Quoted 2 lines
only
+ * 32 bytes long *//*
* Please use proper multi-line comments throughout.Quoted 1 line
*/
Ok, will fix them all.
Quoted 49 lines
Quoted 49 lines
drivers/watchdog/streamlabs_wdt.c new file +26
-0+#define BUFFER_TRANSFER_LENGTH 32 +#define BUFFER_LENGTH 64 +#define USB_TIMEOUT 350 + +#define STREAMLABS_CMD_START 0 +#define STREAMLABS_CMD_STOP 1 + +#define STREAMLABS_WDT_MIN_TIMEOUT 1 +#define STREAMLABS_WDT_MAX_TIMEOUT 46 + +struct streamlabs_wdt { +struct watchdog_device wdt_dev; +struct usb_device *usbdev; +struct usb_interface *intf; + +struct kref kref; +struct mutex lock; +u8 *buffer; +}; + +static bool nowayout =WATCHDOG_NOWAYOUT ; + +static int usb_streamlabs_wdt_validate_response (u8 *buf) +{ +/* If watchdog device understood the command it will acknowledge +* with values 1,2,3,4 at indexes 10, 11, 12, 13 in response message.
drivers/watchdog/streamlabs_wdt.c new file +8
-0+*/ +if (buf[10 ] !=1 || buf[11 ] !=2 || buf[12 ] !=3 || buf[13 ] !=4 ) +return -EINVAL ; + +return 0 ; +} + +static int usb_streamlabs_wdt_command(struct watchdog_device *wdt_dev,int cmd)
drivers/watchdog/streamlabs_wdt.c new file +2
-0+{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c new file +4
-0+int retval; +int size; +unsigned long timeout_msec; +int retry_counter =10 ;/* how many times to re-send stop cmd */
drivers/watchdog/streamlabs_wdt.c new file +5
-0+ +mutex_lock (&streamlabs_wdt->lock); + + timeout_msec = wdt_dev->timeout *MSEC_PER_SEC ; +Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 92
Quoted 3 lines
drivers/watchdog/streamlabs_wdt.c new file +3
-0+/* Prepare message that will be sent to device. +* This buffer is allocated by kzalloc(). Only initialize required +* fields. But only once, and overwritten by the response. So the comment is quite pointless
and misleading.Ok, I will do something with this comment during re-work and rebase.
Quoted 27 lines
Quoted 27 lines
drivers/watchdog/streamlabs_wdt.c new file +22
-0+*/ +if (cmd ==STREAMLABS_CMD_START ) { + streamlabs_wdt->buffer[0 ] =0xcc ; + streamlabs_wdt->buffer[1 ] =0xaa ; + }else {/* assume stop command if it's not start */ + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + } + + streamlabs_wdt->buffer[3 ] =0x80 ; + + streamlabs_wdt->buffer[6 ] = (timeout_msec &0xff ) <<8 ; + streamlabs_wdt->buffer[7 ] = (timeout_msec &0xff00 ) >>8 ; +retry: + streamlabs_wdt->buffer[10 ] =0x00 ; + streamlabs_wdt->buffer[11 ] =0x00 ; + streamlabs_wdt->buffer[12 ] =0x00 ; + streamlabs_wdt->buffer[13 ] =0x00 ; + +/* send command to watchdog */ + retval = usb_interrupt_msg(streamlabs_wdt->usbdev, + usb_sndintpipe(streamlabs_wdt->usbdev,
0x02),
+streamlabs_wdt->buffer,BUFFER_TRANSFER_LENGTH,drivers/watchdog/streamlabs_wdt.c new file +2
-0+ &size, USB_TIMEOUT); +Quoted 8 lines
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +3
-0+if (retval || size !=BUFFER_TRANSFER_LENGTH ) { + dev_err(&streamlabs_wdt->intf->dev, + "error %i when submitting interrupt msg\n",retval);
Please no error messages if something goes wrong. We don't want to fill the kernel log with those messages.
Quoted 2 lines
Ok, will remove them. Or is it fine to convert them to
dev_dbg?
If you think the messages might be useful for debugging, sure.
Quoted 109 lines
Quoted 52 lines
Quoted 52 lines
drivers/watchdog/streamlabs_wdt.c new file +7
-0+ retval = -EIO ; + goto out; + } + +/* and read response from watchdog */ + retval = usb_interrupt_msg(streamlabs_wdt->usbdev, + usb_rcvintpipe(streamlabs_wdt->usbdev,
0x81),drivers/watchdog/streamlabs_wdt.c new file +6
-0+ streamlabs_wdt->buffer,BUFFER_LENGTH , + &size, USB_TIMEOUT); + +if (retval || size !=BUFFER_LENGTH ) { + dev_err(&streamlabs_wdt->intf->dev, + "error %i when receiving interrupt msg\n",retval);
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ retval = -EIO ; + goto out; + } + +/* check if watchdog actually acked/recognized command */ + retval =
usb_streamlabs_wdt_validate_response(streamlabs_wdt->buffer);drivers/watchdog/streamlabs_wdt.c new file +8
-0+if (retval) { +dev_err (&streamlabs_wdt->intf->dev, +"watchdog didn't ACK command!\n" ); + goto out; + } + +/* Transition from enabled to disabled state in this device +* doesn't happen immediately. Usually, 2 or 3 (sometimes even 4) stop
drivers/watchdog/streamlabs_wdt.c new file +8
-1+* commands should be sent until watchdog answers 'I'm stopped!'. +* Retry stop command if watchdog fails to answer correctly +* about its state. After 10 attempts, report error and return -EIO. +*/ +if (cmd ==STREAMLABS_CMD_STOP ) { +if (--retry_counter <=0 ) { + dev_err(&streamlabs_wdt->intf->dev, + "failed to stop watchdog after 9attempts!\n");
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ retval = -EIO ; + goto out; + } +/* +* Check if watchdog actually changed state to disabled. +* If watchdog is still enabled then reset message and retry
drivers/watchdog/streamlabs_wdt.c new file +4
-0+* stop command. +*/ +if (streamlabs_wdt->buffer[0 ] !=0xff || + streamlabs_wdt->buffer[1 ] !=0xbb )Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 168
Quoted 4 lines
{
drivers/watchdog/streamlabs_wdt.c new file +3
-0+ streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + goto retry;Can you use a loop instead ? goto's are ok when needed, but here it just makes the code more difficult to read.
Sure.
Quoted 31 lines
Quoted 23 lines
drivers/watchdog/streamlabs_wdt.c new file +22
-0+ } + } + +out: +mutex_unlock (&streamlabs_wdt->lock); +return retval; +} + +static int usb_streamlabs_wdt_start (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_START ); +} + +static int usb_streamlabs_wdt_stop (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_STOP ); +} + +static int usb_streamlabs_wdt_settimeout (struct watchdog_device *wdt_dev, +unsigned int timeout) +{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c line 194
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ +mutex_lock (&streamlabs_wdt->lock); + wdt_dev->timeout = timeout; +mutex_unlock (&streamlabs_wdt->lock);I don't think this mutex protection is needed.
Quoted 3 lines
Note that there is a patch pending (and will make it into 4.6) which will make the
set_timeoutfunction optional if all it does is to set the timeout variable.Here comes some pain. For last week I tried to clone
git://www.linux-watchdog.org/linux-watchdog-next.git(I hope this is correct address) to rebase on top of 4.6 and test but almost always I got this after counting:fatal: read error: Connection timed out
fatal: early EOF
fatal: index-pack failedand counting takes 3-4 hours.
Quoted 9 lines
After all I cloned it (and www.linux-watchdog.org looks more healthy in last couple of days) but merge window should be opened soon so I will get new things in two weeks anyway. I think I will be able to rebase on 4.6-rc1 or near and re-send.
Looks like I'm not the first one who have troubles with
git://www.linux-watchdog.org
http://www.spinics.net/lists/linux-watchdog/msg07384.html
Yes, sometimes Wim's service provider seems to be less than reliable.
If that happens, you can clone
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
and check out the watchdog-next branch. It is not in sync with Wim's tree, though, and typically has a few commits on top of it (or, once in a while, may be missing some commits).
Quoted 186 lines
Quoted 28 lines
Quoted 28 lines
drivers/watchdog/streamlabs_wdt.c new file +17
-0+ +return 0 ; +} + +static void usb_streamlabs_wdt_release_resources (struct kref *kref) +{ +struct streamlabs_wdt *streamlabs_wdt = + container_of(kref, struct streamlabs_wdt, kref); + +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); +kfree (streamlabs_wdt); +} + +static void usb_streamlabs_wdt_ref (struct watchdog_device *wdt_dev) +{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c new file +7
-0+ +kref_get (&streamlabs_wdt->kref); +} + +static void usb_streamlabs_wdt_unref (struct watchdog_device *wdt_dev) +{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c new file +2
-0+ + kref_put(&streamlabs_wdt->kref,Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 224
Quoted 5 lines
usb_streamlabs_wdt_release_resources);drivers/watchdog/streamlabs_wdt.c new file +4
-0+} + +static const struct watchdog_info streamlabs_wdt_ident = { + .options = (WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING ),Unnecessary ( )
Ok.
Quoted 6 lines
Quoted 6 lines
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ .identity = DRIVER_NAME, +}; + +static struct watchdog_ops usb_streamlabs_wdt_ops = { + .owner =THIS_MODULE , + .start = usb_streamlabs_wdt_start,Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 234
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ .stop = usb_streamlabs_wdt_stop, + .set_timeout = usb_streamlabs_wdt_settimeout, + .ref = usb_streamlabs_wdt_ref, + .unref = usb_streamlabs_wdt_unref,Those functions no longer exist (and are no longer needed) in the latest kernel.
Got it.
Quoted 40 lines
Quoted 32 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+}; + +static int usb_streamlabs_wdt_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{ +struct usb_device *dev =interface_to_usbdev (intf); +struct streamlabs_wdt *streamlabs_wdt; +int retval; + +/* USB IDs of this device appear to be weird/unregistered. Hence, do
drivers/watchdog/streamlabs_wdt.c new file +3
-0+* 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
drivers/watchdog/streamlabs_wdt.c new file +7
-0+* behaves as a watchdog. */ +if (dev->product && dev->manufacturer && + (strncmp (dev->product,"USBkit" ,6 ) !=0 + ||strncmp (dev->manufacturer,"STREAM LABS" ,11 ) !=0 )) +return -ENODEV ; + + streamlabs_wdt = kmalloc(sizeof(struct streamlabs_wdt),
GFP_KERNEL);drivers/watchdog/streamlabs_wdt.c new file +8
-0+if (!streamlabs_wdt) { +dev_err (&intf->dev,"kmalloc failed\n" ); +return -ENOMEM ; + } + + streamlabs_wdt->buffer =kzalloc (BUFFER_LENGTH ,GFP_KERNEL ); +if (!streamlabs_wdt->buffer) { + dev_err(&intf->dev, "kzalloc for watchdog->bufferfailed\n");
drivers/watchdog/streamlabs_wdt.c line 266
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ retval = -ENOMEM ; + goto err_nobuf; + } +Please consider using
devm_kzalloc().Quoted 2 lines
Also, is it necessary to allocate the buffer separately instead of using a (possibly cache line aligned) array in struct
streamlabs_wdt?Thanks. Already moved to
devm_kzalloc()and moved array inside structure instead of allocating buffer separately.Quoted 13 lines
Quoted 13 lines
drivers/watchdog/streamlabs_wdt.c new file +13
-0+mutex_init (&streamlabs_wdt->lock); + + streamlabs_wdt->wdt_dev.info = &streamlabs_wdt_ident; + streamlabs_wdt->wdt_dev.ops = &usb_streamlabs_wdt_ops; + streamlabs_wdt->wdt_dev.timeout =STREAMLABS_WDT_MAX_TIMEOUT ; + streamlabs_wdt->wdt_dev.max_timeout =STREAMLABS_WDT_MAX_TIMEOUT ; + streamlabs_wdt->wdt_dev.min_timeout =STREAMLABS_WDT_MIN_TIMEOUT ; + streamlabs_wdt->wdt_dev.parent = &intf->dev; + + streamlabs_wdt->usbdev =interface_to_usbdev (intf); + streamlabs_wdt->intf = intf; +usb_set_intfdata (intf, &streamlabs_wdt->wdt_dev); +watchdog_set_drvdata (&streamlabs_wdt->wdt_dev, streamlabs_wdt);Quoted 8 lines
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +3
-0+ + watchdog_init_timeout(&streamlabs_wdt->wdt_dev, + streamlabs_wdt->wdt_dev.timeout,
&intf->dev);This is quite pointless, since timeout is already set, and you don't have a module parameter to overwrite it.
Gonna remove it.
Quoted 43 lines
Quoted 38 lines
drivers/watchdog/streamlabs_wdt.c new file +11
-0+watchdog_set_nowayout (&streamlabs_wdt->wdt_dev, nowayout); + +kref_init (&streamlabs_wdt->kref); + + retval =usb_streamlabs_wdt_stop (&streamlabs_wdt->wdt_dev); +if (retval) + goto err_wdt_buf; + + retval =watchdog_register_device (&streamlabs_wdt->wdt_dev); +if (retval) { + dev_err(&intf->dev, "failed to register watchdogdevice\n");
drivers/watchdog/streamlabs_wdt.c new file +26
-0+ goto err_wdt_buf; + } + +dev_info (&intf->dev,"StreamLabs USB watchdog loaded.\n" ); + +return 0 ; + +err_wdt_buf: +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); + +err_nobuf: +kfree (streamlabs_wdt); +return retval; +} + + + +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)) + usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev, +Quoted 1 line
STREAMLABS_CMD_STOP);Please call
usb_streamlabs_wdt_stop().Quoted 16 lines
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ +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 Quoted 3 lines
usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev, +
STREAMLABS_CMD_START);Please call
usb_streamlabs_wdt_start().
Quoted 4 lines
Thanks, I will add required calls. Out of curiosity, what about driver(s) that checks watchdog status in suspend/resume and calls stop/start only if watchdog active?
Not sure I understand. Isn't that what you are doing ?
Quoted 21 lines
Quoted 12 lines
Quoted 12 lines
drivers/watchdog/streamlabs_wdt.c new file +12
-0+ +return 0 ; +} + +static void usb_streamlabs_wdt_disconnect (struct usb_interface *intf) +{ +struct streamlabs_wdt *streamlabs_wdt =usb_get_intfdata (intf); + +/* First, stop sending USB messages to device. */ +mutex_lock (&streamlabs_wdt->lock); +usb_set_intfdata (intf,NULL ); + streamlabs_wdt->usbdev =NULL ;Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c
Quoted 2 lines
drivers/watchdog/streamlabs_wdt.c new file +2
-0+mutex_unlock (&streamlabs_wdt->lock); +If user space has a keepalive pending (waiting for the mutex being released in
usb_streamlabs_wdt_command()), this may result in a call tousb_interrupt_msg()with the device set to NULL. The mutex protection doesn't add any value since you don't check if ->usbdev is NULL inusb_streamlabs_wdt_command().
Quoted 6 lines
In another email Oliver adviced to remove ->usbdev. So, I use ->intf and check it for NULL in
usb_streamlabs_wdt_command(). I also dousb_set_intfdata(intf, NULL);streamlabs_wdt->intf= NULL;
here with mutex locked. Hope it's fine.
I'll have to see the code to determine if it is clean.
Quoted 2 lines
Thanks for review.
My pleasure.
Thanks,
Guenter
Hi Guenter,
Quoted 26 lines · Guenter Roeck
Hi Alexey,
Quoted 22 lines · Alexey Klimov
Hi Guenter,
Quoted 18 lines · Guenter Roeck
Quoted 16 lines · Alexey Klimov
This patch creates new driver that supports StreamLabs usb watchdog device. This device plugs into 9-pin usb header and connects to reset pin and reset button on common PC.
USB commands used to communicate with device were reverse engineered using usbmon.
Signed-off-by Alexey Klimov <[email protected]>--- drivers/watchdog/Kconfig | 15 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/streamlabs_wdt.c | 370 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 386 insertions(+)
[...]
Quoted 68 lines
Quoted 68 lines
Quoted 18 lines
Quoted 18 lines
+static int
usb_streamlabs_wdt_command(struct watchdog_device *wdt_dev, int cmd)(quoted diff) +2
-0+{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c new file +4
-0+int retval; +int size; +unsigned long timeout_msec; +int retry_counter =10 ;/* how many times to re-send stop
cmd */drivers/watchdog/streamlabs_wdt.c new file +7
-0+ +mutex_lock (&streamlabs_wdt->lock); + + timeout_msec = wdt_dev->timeout *MSEC_PER_SEC ; + +/* Prepare message that will be sent to device. +* This buffer is allocated by kzalloc(). Only initialize Quoted 8 lines
Quoted 2 lines
required
+ * fields.But only once, and overwritten by the response. So the comment is quite pointless
and misleading.Ok, I will do something with this comment during re-work and rebase.
Quoted 36 lines
Quoted 30 lines
drivers/watchdog/streamlabs_wdt.c new file +22
-0+*/ +if (cmd ==STREAMLABS_CMD_START ) { + streamlabs_wdt->buffer[0 ] =0xcc ; + streamlabs_wdt->buffer[1 ] =0xaa ; + }else {/* assume stop command if it's not start */ + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + } + + streamlabs_wdt->buffer[3 ] =0x80 ; + + streamlabs_wdt->buffer[6 ] = (timeout_msec &0xff ) <<8 ; + streamlabs_wdt->buffer[7 ] = (timeout_msec &0xff00 ) >>8 ; +retry: + streamlabs_wdt->buffer[10 ] =0x00 ; + streamlabs_wdt->buffer[11 ] =0x00 ; + streamlabs_wdt->buffer[12 ] =0x00 ; + streamlabs_wdt->buffer[13 ] =0x00 ; + +/* send command to watchdog */ + retval = usb_interrupt_msg(streamlabs_wdt->usbdev, + usb_sndintpipe(streamlabs_wdt->usbdev,
0x02),
+streamlabs_wdt->buffer,BUFFER_TRANSFER_LENGTH,drivers/watchdog/streamlabs_wdt.c new file +5
-0+ &size, USB_TIMEOUT); + +if (retval || size !=BUFFER_TRANSFER_LENGTH ) { + dev_err(&streamlabs_wdt->intf->dev, + "error %i when submitting interrupt msg\n",Quoted 1 line
retval);
Please no error messages if something goes wrong. We don't want to fill the kernel log with those messages.
Quoted 4 lines
Quoted 2 lines
Ok, will remove them. Or is it fine to convert them to
dev_dbg?If you think the messages might be useful for debugging, sure.
Well, definetely they help me now.
Quoted 131 lines
Quoted 119 lines
Quoted 57 lines
Quoted 57 lines
drivers/watchdog/streamlabs_wdt.c new file +7
-0+ retval = -EIO ; + goto out; + } + +/* and read response from watchdog */ + retval = usb_interrupt_msg(streamlabs_wdt->usbdev, + usb_rcvintpipe(streamlabs_wdt->usbdev,
0x81),drivers/watchdog/streamlabs_wdt.c new file +6
-0+ streamlabs_wdt->buffer,BUFFER_LENGTH , + &size, USB_TIMEOUT); + +if (retval || size !=BUFFER_LENGTH ) { + dev_err(&streamlabs_wdt->intf->dev, + "error %i when receiving interrupt msg\n",retval);
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ retval = -EIO ; + goto out; + } + +/* check if watchdog actually acked/recognized command */ + retval =
usb_streamlabs_wdt_validate_response(streamlabs_wdt->buffer);drivers/watchdog/streamlabs_wdt.c new file +3
-0+if (retval) { + dev_err(&streamlabs_wdt->intf->dev, + "watchdog didn't ACKcommand!\n");
drivers/watchdog/streamlabs_wdt.c new file +5
-0+ goto out; + } + +/* Transition from enabled to disabled state in this device +* doesn't happen immediately. Usually, 2 or 3 (sometimes even 4)
stop
+ * commands should be sent until watchdog answers 'I'm
stopped!'.drivers/watchdog/streamlabs_wdt.c new file +7
-1+* Retry stop command if watchdog fails to answer correctly +* about its state. After 10 attempts, report error and return -EIO. +*/ +if (cmd ==STREAMLABS_CMD_STOP ) { +if (--retry_counter <=0 ) { + dev_err(&streamlabs_wdt->intf->dev, + "failed to stop watchdog after 9attempts!\n");
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ retval = -EIO ; + goto out; + } +/* +* Check if watchdog actually changed state to disabled. +* If watchdog is still enabled then reset message and retry
drivers/watchdog/streamlabs_wdt.c new file +4
-0+* stop command. +*/ +if (streamlabs_wdt->buffer[0 ] !=0xff || + streamlabs_wdt->buffer[1] !=
0xbb)
{Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 168
Quoted 3 lines
drivers/watchdog/streamlabs_wdt.c new file +3
-0+ streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + goto retry;Can you use a loop instead ? goto's are ok when needed, but here it just makes the code more difficult to read.
Sure.
Quoted 34 lines
Quoted 25 lines
drivers/watchdog/streamlabs_wdt.c new file +11
-0+ } + } + +out: +mutex_unlock (&streamlabs_wdt->lock); +return retval; +} + +static int usb_streamlabs_wdt_start (struct watchdog_device *wdt_dev) +{ + return usb_streamlabs_wdt_command(wdt_dev,
STREAMLABS_CMD_START);drivers/watchdog/streamlabs_wdt.c new file +8
-0+} + +static int usb_streamlabs_wdt_stop (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_STOP ); +} + +static int usb_streamlabs_wdt_settimeout(struct watchdog_device
*wdt_dev,drivers/watchdog/streamlabs_wdt.c new file +3
-0+ unsigned int timeout) +{ + struct streamlabs_wdt *streamlabs_wdt =
watchdog_get_drvdata(wdt_dev);drivers/watchdog/streamlabs_wdt.c line 194
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ +mutex_lock (&streamlabs_wdt->lock); + wdt_dev->timeout = timeout; +mutex_unlock (&streamlabs_wdt->lock);I don't think this mutex protection is needed.
Quoted 3 lines
Note that there is a patch pending (and will make it into 4.6) which will make the
set_timeoutfunction optional if all it does is to set the timeout variable.Here comes some pain. For last week I tried to clone
git://www.linux-watchdog.org/linux-watchdog-next.git(I hope this is correct address) to rebase on top of 4.6 and test but almost always I got this after counting:fatal: read error: Connection timed out
fatal: early EOF
fatal: index-pack failedand counting takes 3-4 hours.
Quoted 9 lines
After all I cloned it (and www.linux-watchdog.org looks more healthy in last couple of days) but merge window should be opened soon so I will get new things in two weeks anyway. I think I will be able to rebase on 4.6-rc1 or near and re-send.
Looks like I'm not the first one who have troubles with
git://www.linux-watchdog.org
http://www.spinics.net/lists/linux-watchdog/msg07384.htmlYes, sometimes Wim's service provider seems to be less than reliable.
Quoted 8 lines
If that happens, you can clone
git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git
and check out the watchdog-next branch. It is not in sync with Wim's tree, though,
and typically has a few commits on top of it (or, once in a while, may be missing
some commits).
Thanks for sharing this link.
Give me few more days or probably on weekend I will try to send rebased and re-tested v 2.
[...]
Quoted 35 lines
Quoted 35 lines
Quoted 33 lines
Quoted 11 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-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)) + usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev, +
STREAMLABS_CMD_STOP);Please call
usb_streamlabs_wdt_stop().Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ +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 Quoted 3 lines
usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev, +
STREAMLABS_CMD_START);Please call
usb_streamlabs_wdt_start().
Quoted 6 lines
Quoted 4 lines
Thanks, I will add required calls. Out of curiosity, what about driver(s) that checks watchdog status in suspend/resume and calls stop/start only if watchdog active?
Not sure I understand. Isn't that what you are doing ?
Ouch. Nevermind. I must be dreaming during night-time when I wrote this. I will change this calls to usb_streamlabs_wdt_{stop,start} as you pointed.
Quoted 23 lines
Quoted 23 lines
Quoted 21 lines
Quoted 4 lines
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ +return 0 ; +} +drivers/watchdog/streamlabs_wdt.c line 344
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+static void usb_streamlabs_wdt_disconnect (struct usb_interface *intf) +{ +struct streamlabs_wdt *streamlabs_wdt =usb_get_intfdata (intf); + +/* First, stop sending USB messages to device. */ +mutex_lock (&streamlabs_wdt->lock); +usb_set_intfdata (intf,NULL ); + streamlabs_wdt->usbdev =NULL ; +mutex_unlock (&streamlabs_wdt->lock); +If user space has a keepalive pending (waiting for the mutex being released in
usb_streamlabs_wdt_command()), this may result in a call tousb_interrupt_msg()with the device set to NULL. The mutex protection doesn't add any value since you don't check if ->usbdev is NULL inusb_streamlabs_wdt_command().
Quoted 8 lines
Quoted 6 lines
In another email Oliver adviced to remove ->usbdev. So, I use ->intf and check it for NULL in
usb_streamlabs_wdt_command(). I also dousb_set_intfdata(intf, NULL);streamlabs_wdt->intf= NULL;
here with mutex locked. Hope it's fine.I'll have to see the code to determine if it is clean.
I'm working on it. Please let me know if migrating to u16 buffer array is not acceptable way.
Thanks!
Signature · 1 line
Best regards, Klimov Alexey
kbuild test robot
build botreproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree…
Hi Alexey,
[auto build test ERROR on hwmon/hwmon-next]
[also build test ERROR on v4.5-rc7 next-20160309]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]
url: https://github.com/0day-ci/linux/commits/Alexey-Klimov/watchdog-add-driver-for-StreamLabs-USB-watchdog-device/20160310-103523 base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next config: sparc64-allmodconfig (attached as .config)
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All error/warnings (new ones prefixed by >>):
>> drivers/watchdog/streamlabs_wdt.c:233:2: error: unknown field 'ref' specified in initializer
.ref = usb_streamlabs_wdt_ref,
^
>> drivers/watchdog/streamlabs_wdt.c:233:2: warning: initialization from incompatible pointer type
drivers/watchdog/streamlabs_wdt.c:233:2: warning: (near initialization for 'usb_streamlabs_wdt_ops.get_timeleft')
>> drivers/watchdog/streamlabs_wdt.c:234:2: error: unknown field 'unref' specified in initializer .unref = usb_streamlabs_wdt_unref,
^
drivers/watchdog/streamlabs_wdt.c:234:2: warning: initialization from incompatible pointer type drivers/watchdog/streamlabs_wdt.c:234:2: warning: (near initialization for 'usb_streamlabs_wdt_ops.restart')
vim +/ref +233 drivers/watchdog/streamlabs_wdt.c
227
228 static struct watchdog_ops usb_streamlabs_wdt_ops = {
229 .owner = THIS_MODULE,
230 .start = usb_streamlabs_wdt_start,
231 .stop = usb_streamlabs_wdt_stop,
232 .set_timeout = usb_streamlabs_wdt_settimeout,
> 233 .ref = usb_streamlabs_wdt_ref, > 234 .unref = usb_streamlabs_wdt_unref,
235 };
236
237 static int usb_streamlabs_wdt_probe(struct usb_interface *intf,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Quoted 3 lines · Alexey Klimov
This patch creates new driver that supports StreamLabs usb watchdog device. This device plugs into 9-pin usb header and connects to reset pin and reset button on common PC.
Hi,
a few remarks.
Regards
Oliver
Quoted 101 lines
USB commands used to communicate with device were reverse engineered using usbmon.
Signed-off-by Alexey Klimov <[email protected]>--- drivers/watchdog/Kconfig | 15 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/streamlabs_wdt.c | 370 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 386 insertions(+) create mode 100644 drivers/watchdog/streamlabs_wdt.cdrivers/watchdog/Kconfig +15
-0@@ -1705,4 +1705,19 @@ config USBPCWATCHDOG Most people will say N. +config USB_STREAMLABS_WATCHDOG + tristate "StreamLabs USB watchdog driver" + depends on USB + ---help--- + This is the driver for the USB Watchdog dongle from StreamLabs. + If you correctly connect reset pins to motherboard Reset pin and + to Reset button then this device will simply watch your kernel to make + sure it doesn't freeze, and if it does, it reboots your computer + after a certain amount of time. + + + To compile this driver as a module, choose M here: the + module will be called streamlabs_wdt. + + Most people will say N. Say yes or M if you want to use such usb device. endif # WATCHDOGdrivers/watchdog/Makefile +1
-0@@ -25,6 +25,7 @@ obj-$(CONFIG_WDTPCI) += wdt_pci.o # USB-based Watchdog Cards obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_USB_STREAMLABS_WATCHDOG) += streamlabs_wdt.o # ALPHA Architecturedrivers/watchdog/streamlabs_wdt.c new file +47
-0@@ -0,0 +1,370 @@ +/* +* StreamLabs USB Watchdog driver +* +* Copyright (c) 2016 Alexey Klimov <[email protected]> +* +* This program is free software; you may redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/usb.h> +#include <linux/watchdog.h> + +/* +* USB Watchdog device from Streamlabs +* http://www.stream-labs.com/products/devices/watchdog/ +* +* USB commands have been reverse engineered using usbmon. +*/ + +#define DRIVER_AUTHOR "Alexey Klimov <[email protected]>" +#define DRIVER_DESC "StreamLabs USB watchdog driver" +#define DRIVER_NAME "usb_streamlabs_wdt" + +MODULE_AUTHOR (DRIVER_AUTHOR ); +MODULE_DESCRIPTION (DRIVER_DESC ); +MODULE_LICENSE ("GPL" ); + +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0 +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011 + +/* one buffer is used for communication, however transmitted message is only +* 32 bytes long */ +#define BUFFER_TRANSFER_LENGTH 32 +#define BUFFER_LENGTH 64 +#define USB_TIMEOUT 350
drivers/watchdog/streamlabs_wdt.c line 57
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ +#define STREAMLABS_CMD_START 0 +#define STREAMLABS_CMD_STOP 1 + +#define STREAMLABS_WDT_MIN_TIMEOUT 1 +#define STREAMLABS_WDT_MAX_TIMEOUT 46 + +struct streamlabs_wdt { +struct watchdog_device wdt_dev; +struct usb_device *usbdev;
You can always deduce the device from the interface. There's no need to store it.
Quoted 38 lines
drivers/watchdog/streamlabs_wdt.c new file +38
-0+struct usb_interface *intf; + +struct kref kref; +struct mutex lock; +u8 *buffer; +}; + +static bool nowayout =WATCHDOG_NOWAYOUT ; + +static int usb_streamlabs_wdt_validate_response (u8 *buf) +{ +/* If watchdog device understood the command it will acknowledge +* with values 1,2,3,4 at indexes 10, 11, 12, 13 in response message. +*/ +if (buf[10 ] !=1 || buf[11 ] !=2 || buf[12 ] !=3 || buf[13 ] !=4 ) +return -EINVAL ; + +return 0 ; +} + +static int usb_streamlabs_wdt_command (struct watchdog_device *wdt_dev,int cmd) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); +int retval; +int size; +unsigned long timeout_msec; +int retry_counter =10 ;/* how many times to re-send stop cmd */ + +mutex_lock (&streamlabs_wdt->lock); + + timeout_msec = wdt_dev->timeout *MSEC_PER_SEC ; + +/* Prepare message that will be sent to device. +* This buffer is allocated by kzalloc(). Only initialize required +* fields. +*/ +if (cmd ==STREAMLABS_CMD_START ) { + streamlabs_wdt->buffer[0 ] =0xcc ;
drivers/watchdog/streamlabs_wdt.c line 105
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ streamlabs_wdt->buffer[1 ] =0xaa ; + }else {/* assume stop command if it's not start */ + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + } + + streamlabs_wdt->buffer[3 ] =0x80 ; + + streamlabs_wdt->buffer[6 ] = (timeout_msec &0xff ) <<8 ; + streamlabs_wdt->buffer[7 ] = (timeout_msec &0xff00 ) >>8 ;
We have macros for such conversions. Please use them.
drivers/watchdog/streamlabs_wdt.c line 106
Quoted 1 line
drivers/watchdog/streamlabs_wdt.c new file +1
-0+retry:
Why rewrite only a part of the buffer this way?
Quoted 206 lines
drivers/watchdog/streamlabs_wdt.c new file +206
-0+ streamlabs_wdt->buffer[10 ] =0x00 ; + streamlabs_wdt->buffer[11 ] =0x00 ; + streamlabs_wdt->buffer[12 ] =0x00 ; + streamlabs_wdt->buffer[13 ] =0x00 ; + +/* send command to watchdog */ + retval =usb_interrupt_msg (streamlabs_wdt->usbdev, +usb_sndintpipe (streamlabs_wdt->usbdev,0x02 ), + streamlabs_wdt->buffer,BUFFER_TRANSFER_LENGTH , + &size,USB_TIMEOUT ); + +if (retval || size !=BUFFER_TRANSFER_LENGTH ) { +dev_err (&streamlabs_wdt->intf->dev, +"error %i when submitting interrupt msg\n" , retval); + retval = -EIO ; + goto out; + } + +/* and read response from watchdog */ + retval =usb_interrupt_msg (streamlabs_wdt->usbdev, +usb_rcvintpipe (streamlabs_wdt->usbdev,0x81 ), + streamlabs_wdt->buffer,BUFFER_LENGTH , + &size,USB_TIMEOUT ); + +if (retval || size !=BUFFER_LENGTH ) { +dev_err (&streamlabs_wdt->intf->dev, +"error %i when receiving interrupt msg\n" , retval); + retval = -EIO ; + goto out; + } + +/* check if watchdog actually acked/recognized command */ + retval =usb_streamlabs_wdt_validate_response (streamlabs_wdt->buffer); +if (retval) { +dev_err (&streamlabs_wdt->intf->dev, +"watchdog didn't ACK command!\n" ); + goto out; + } + +/* Transition from enabled to disabled state in this device +* doesn't happen immediately. Usually, 2 or 3 (sometimes even 4) stop +* commands should be sent until watchdog answers 'I'm stopped!'. +* Retry stop command if watchdog fails to answer correctly +* about its state. After 10 attempts, report error and return -EIO. +*/ +if (cmd ==STREAMLABS_CMD_STOP ) { +if (--retry_counter <=0 ) { +dev_err (&streamlabs_wdt->intf->dev, +"failed to stop watchdog after 9 attempts!\n" ); + retval = -EIO ; + goto out; + } +/* +* Check if watchdog actually changed state to disabled. +* If watchdog is still enabled then reset message and retry +* stop command. +*/ +if (streamlabs_wdt->buffer[0 ] !=0xff || + streamlabs_wdt->buffer[1 ] !=0xbb ) { + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + goto retry; + } + } + +out: +mutex_unlock (&streamlabs_wdt->lock); +return retval; +} + +static int usb_streamlabs_wdt_start (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_START ); +} + +static int usb_streamlabs_wdt_stop (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_STOP ); +} + +static int usb_streamlabs_wdt_settimeout (struct watchdog_device *wdt_dev, +unsigned int timeout) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +mutex_lock (&streamlabs_wdt->lock); + wdt_dev->timeout = timeout; +mutex_unlock (&streamlabs_wdt->lock); + +return 0 ; +} + +static void usb_streamlabs_wdt_release_resources (struct kref *kref) +{ +struct streamlabs_wdt *streamlabs_wdt = + container_of(kref, struct streamlabs_wdt, kref); + +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); +kfree (streamlabs_wdt); +} + +static void usb_streamlabs_wdt_ref (struct watchdog_device *wdt_dev) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +kref_get (&streamlabs_wdt->kref); +} + +static void usb_streamlabs_wdt_unref (struct watchdog_device *wdt_dev) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +kref_put (&streamlabs_wdt->kref, usb_streamlabs_wdt_release_resources); +} + +static const struct watchdog_info streamlabs_wdt_ident = { + .options = (WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING ), + .identity =DRIVER_NAME , +}; + +static struct watchdog_ops usb_streamlabs_wdt_ops = { + .owner =THIS_MODULE , + .start = usb_streamlabs_wdt_start, + .stop = usb_streamlabs_wdt_stop, + .set_timeout = usb_streamlabs_wdt_settimeout, + .ref = usb_streamlabs_wdt_ref, + .unref = usb_streamlabs_wdt_unref, +}; + +static int usb_streamlabs_wdt_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{ +struct usb_device *dev =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 (dev->product && dev->manufacturer && + (strncmp (dev->product,"USBkit" ,6 ) !=0 + ||strncmp (dev->manufacturer,"STREAM LABS" ,11 ) !=0 )) +return -ENODEV ; + + streamlabs_wdt =kmalloc (sizeof (struct streamlabs_wdt ),GFP_KERNEL ); +if (!streamlabs_wdt) { +dev_err (&intf->dev,"kmalloc failed\n" ); +return -ENOMEM ; + } + + streamlabs_wdt->buffer =kzalloc (BUFFER_LENGTH ,GFP_KERNEL ); +if (!streamlabs_wdt->buffer) { +dev_err (&intf->dev,"kzalloc for watchdog->buffer failed\n" ); + retval = -ENOMEM ; + goto err_nobuf; + } + +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->usbdev =interface_to_usbdev (intf); + streamlabs_wdt->intf = intf; +usb_set_intfdata (intf, &streamlabs_wdt->wdt_dev); +watchdog_set_drvdata (&streamlabs_wdt->wdt_dev, streamlabs_wdt); + +watchdog_init_timeout (&streamlabs_wdt->wdt_dev, + streamlabs_wdt->wdt_dev.timeout, &intf->dev); +watchdog_set_nowayout (&streamlabs_wdt->wdt_dev, nowayout); + +kref_init (&streamlabs_wdt->kref); + + retval =usb_streamlabs_wdt_stop (&streamlabs_wdt->wdt_dev); +if (retval) + goto err_wdt_buf; + + retval =watchdog_register_device (&streamlabs_wdt->wdt_dev); +if (retval) { +dev_err (&intf->dev,"failed to register watchdog device\n" ); + goto err_wdt_buf; + } + +dev_info (&intf->dev,"StreamLabs USB watchdog loaded.\n" ); + +return 0 ; + +err_wdt_buf: +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); + +err_nobuf: +kfree (streamlabs_wdt); +return retval; +} + + + +static int usb_streamlabs_wdt_suspend(struct usb_interface *intf,
drivers/watchdog/streamlabs_wdt.c line 322
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ pm_message_t message) +{ +struct streamlabs_wdt *streamlabs_wdt =usb_get_intfdata (intf); + +if (watchdog_active (&streamlabs_wdt->wdt_dev)) +usb_streamlabs_wdt_command (&streamlabs_wdt->wdt_dev, +STREAMLABS_CMD_STOP ); + +return 0 ; +}
If the result of a failure would be a spurious reset, you cannot drop errors.
Quoted 7 lines
drivers/watchdog/streamlabs_wdt.c new file +7
-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_command(&streamlabs_wdt->wdt_dev,
drivers/watchdog/streamlabs_wdt.c line 339
Quoted 10 lines
drivers/watchdog/streamlabs_wdt.c new file +10
-0+ STREAMLABS_CMD_START); + +return 0 ; +} + +static void usb_streamlabs_wdt_disconnect (struct usb_interface *intf) +{ +struct streamlabs_wdt *streamlabs_wdt =usb_get_intfdata (intf); + +/* First, stop sending USB messages to device. */
That is problematic. A disconnect can be triggered by removal of the hardware or in software through sysfs. In the latter case a running device without a driver would trigger a reset, wouldn't it? So I think for watchdog devices you need to go to an inactive state in disconnect().
Quoted 31 lines
drivers/watchdog/streamlabs_wdt.c new file +31
-0+mutex_lock (&streamlabs_wdt->lock); +usb_set_intfdata (intf,NULL ); + streamlabs_wdt->usbdev =NULL ; +mutex_unlock (&streamlabs_wdt->lock); + +/* after commincation with device has stopped we can +* unregister watchdog device. unref callback will clear the rest on +* release of device if it was opened. +*/ +watchdog_unregister_device (&streamlabs_wdt->wdt_dev); +kref_put (&streamlabs_wdt->kref, usb_streamlabs_wdt_release_resources); +} + +static struct usb_device_id usb_streamlabs_wdt_device_table[] = { + {USB_DEVICE (USB_STREAMLABS_WATCHDOG_VENDOR ,USB_STREAMLABS_WATCHDOG_PRODUCT ) }, + { }/* Terminating entry */ +}; + +MODULE_DEVICE_TABLE (usb, usb_streamlabs_wdt_device_table); + +static struct usb_driver usb_streamlabs_wdt_driver = { + .name =DRIVER_NAME , + .probe = usb_streamlabs_wdt_probe, + .disconnect = usb_streamlabs_wdt_disconnect, + .suspend = usb_streamlabs_wdt_suspend, + .resume = usb_streamlabs_wdt_resume, + .reset_resume = usb_streamlabs_wdt_resume, + .id_table = usb_streamlabs_wdt_device_table, +}; + +module_usb_driver (usb_streamlabs_wdt_driver);
Hi Oliver,
Quoted 118 lines · Oliver Neukum
Quoted 3 lines · Alexey Klimov
This patch creates new driver that supports StreamLabs usb watchdog device. This device plugs into 9-pin usb header and connects to reset pin and reset button on common PC.
Hi,
a few remarks.
Regards
OliverQuoted 106 lines
USB commands used to communicate with device were reverse engineered using usbmon.
Signed-off-by Alexey Klimov <[email protected]>--- drivers/watchdog/Kconfig | 15 ++ drivers/watchdog/Makefile | 1 + drivers/watchdog/streamlabs_wdt.c | 370 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 386 insertions(+) create mode 100644 drivers/watchdog/streamlabs_wdt.cdrivers/watchdog/Kconfig +15
-0@@ -1705,4 +1705,19 @@ config USBPCWATCHDOG Most people will say N. +config USB_STREAMLABS_WATCHDOG + tristate "StreamLabs USB watchdog driver" + depends on USB + ---help--- + This is the driver for the USB Watchdog dongle from StreamLabs. + If you correctly connect reset pins to motherboard Reset pin and + to Reset button then this device will simply watch your kernel to make + sure it doesn't freeze, and if it does, it reboots your computer + after a certain amount of time. + + + To compile this driver as a module, choose M here: the + module will be called streamlabs_wdt. + + Most people will say N. Say yes or M if you want to use such usb device. endif # WATCHDOGdrivers/watchdog/Makefile +1
-0@@ -25,6 +25,7 @@ obj-$(CONFIG_WDTPCI) += wdt_pci.o # USB-based Watchdog Cards obj-$(CONFIG_USBPCWATCHDOG) += pcwd_usb.o +obj-$(CONFIG_USB_STREAMLABS_WATCHDOG) += streamlabs_wdt.o # ALPHA Architecturedrivers/watchdog/streamlabs_wdt.c new file +52
-0@@ -0,0 +1,370 @@ +/* +* StreamLabs USB Watchdog driver +* +* Copyright (c) 2016 Alexey Klimov <[email protected]> +* +* This program is free software; you may redistribute it and/or modify +* it under the terms of the GNU General Public License as published by +* the Free Software Foundation; either version 2 of the License, or +* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful, +* but WITHOUT ANY WARRANTY; without even the implied warranty of +* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/errno.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/slab.h> +#include <linux/usb.h> +#include <linux/watchdog.h> + +/* +* USB Watchdog device from Streamlabs +* http://www.stream-labs.com/products/devices/watchdog/ +* +* USB commands have been reverse engineered using usbmon. +*/ + +#define DRIVER_AUTHOR "Alexey Klimov <[email protected]>" +#define DRIVER_DESC "StreamLabs USB watchdog driver" +#define DRIVER_NAME "usb_streamlabs_wdt" + +MODULE_AUTHOR (DRIVER_AUTHOR ); +MODULE_DESCRIPTION (DRIVER_DESC ); +MODULE_LICENSE ("GPL" ); + +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0 +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011 + +/* one buffer is used for communication, however transmitted message is only +* 32 bytes long */ +#define BUFFER_TRANSFER_LENGTH 32 +#define BUFFER_LENGTH 64 +#define USB_TIMEOUT 350 + +#define STREAMLABS_CMD_START 0 +#define STREAMLABS_CMD_STOP 1 + +#define STREAMLABS_WDT_MIN_TIMEOUT 1
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 57
Quoted 5 lines
drivers/watchdog/streamlabs_wdt.c new file +5
-0+#define STREAMLABS_WDT_MAX_TIMEOUT 46 + +struct streamlabs_wdt { +struct watchdog_device wdt_dev; +struct usb_device *usbdev;You can always deduce the device from the interface. There's no need to store it.
Thanks, I removed it.
Quoted 42 lines
Quoted 42 lines
drivers/watchdog/streamlabs_wdt.c new file +42
-0+struct usb_interface *intf; + +struct kref kref; +struct mutex lock; +u8 *buffer; +}; + +static bool nowayout =WATCHDOG_NOWAYOUT ; + +static int usb_streamlabs_wdt_validate_response (u8 *buf) +{ +/* If watchdog device understood the command it will acknowledge +* with values 1,2,3,4 at indexes 10, 11, 12, 13 in response message. +*/ +if (buf[10 ] !=1 || buf[11 ] !=2 || buf[12 ] !=3 || buf[13 ] !=4 ) +return -EINVAL ; + +return 0 ; +} + +static int usb_streamlabs_wdt_command (struct watchdog_device *wdt_dev,int cmd) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); +int retval; +int size; +unsigned long timeout_msec; +int retry_counter =10 ;/* how many times to re-send stop cmd */ + +mutex_lock (&streamlabs_wdt->lock); + + timeout_msec = wdt_dev->timeout *MSEC_PER_SEC ; + +/* Prepare message that will be sent to device. +* This buffer is allocated by kzalloc(). Only initialize required +* fields. +*/ +if (cmd ==STREAMLABS_CMD_START ) { + streamlabs_wdt->buffer[0 ] =0xcc ; + streamlabs_wdt->buffer[1 ] =0xaa ; + }else {/* assume stop command if it's not start */ + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ;
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 105
Quoted 6 lines
drivers/watchdog/streamlabs_wdt.c new file +6
-0+ } + + streamlabs_wdt->buffer[3 ] =0x80 ; + + streamlabs_wdt->buffer[6 ] = (timeout_msec &0xff ) <<8 ; + streamlabs_wdt->buffer[7 ] = (timeout_msec &0xff00 ) >>8 ;We have macros for such conversions. Please use them.
I screwed here. It should be:
buffer[6] = timeout_msec & 0xff;
buffer[7] = (timeout_msec >> 8) & 0xff;However, are you talking about using swab16() function? Or migrating to cpu_to_le() and friends functions?
If it's acceptable way to make buffer with u16 type?
It slightly decreases lines of code and no conversion is needed here that way. I can do just without swapping bytes:
buffer[3] = timeout_msec;
and that's it.
Quoted 3 lines
drivers/watchdog/streamlabs_wdt.c line 106
Quoted 1 line
drivers/watchdog/streamlabs_wdt.c new file +1
-0+retry:Why rewrite only a part of the buffer this way?
It was the attempt to somehow optimize retry loop. During re-work to make do-while loop I started to rewrite buffer completely before initiating write-read operation.
Quoted 211 lines
Quoted 211 lines
drivers/watchdog/streamlabs_wdt.c new file +211
-0+ streamlabs_wdt->buffer[10 ] =0x00 ; + streamlabs_wdt->buffer[11 ] =0x00 ; + streamlabs_wdt->buffer[12 ] =0x00 ; + streamlabs_wdt->buffer[13 ] =0x00 ; + +/* send command to watchdog */ + retval =usb_interrupt_msg (streamlabs_wdt->usbdev, +usb_sndintpipe (streamlabs_wdt->usbdev,0x02 ), + streamlabs_wdt->buffer,BUFFER_TRANSFER_LENGTH , + &size,USB_TIMEOUT ); + +if (retval || size !=BUFFER_TRANSFER_LENGTH ) { +dev_err (&streamlabs_wdt->intf->dev, +"error %i when submitting interrupt msg\n" , retval); + retval = -EIO ; + goto out; + } + +/* and read response from watchdog */ + retval =usb_interrupt_msg (streamlabs_wdt->usbdev, +usb_rcvintpipe (streamlabs_wdt->usbdev,0x81 ), + streamlabs_wdt->buffer,BUFFER_LENGTH , + &size,USB_TIMEOUT ); + +if (retval || size !=BUFFER_LENGTH ) { +dev_err (&streamlabs_wdt->intf->dev, +"error %i when receiving interrupt msg\n" , retval); + retval = -EIO ; + goto out; + } + +/* check if watchdog actually acked/recognized command */ + retval =usb_streamlabs_wdt_validate_response (streamlabs_wdt->buffer); +if (retval) { +dev_err (&streamlabs_wdt->intf->dev, +"watchdog didn't ACK command!\n" ); + goto out; + } + +/* Transition from enabled to disabled state in this device +* doesn't happen immediately. Usually, 2 or 3 (sometimes even 4) stop +* commands should be sent until watchdog answers 'I'm stopped!'. +* Retry stop command if watchdog fails to answer correctly +* about its state. After 10 attempts, report error and return -EIO. +*/ +if (cmd ==STREAMLABS_CMD_STOP ) { +if (--retry_counter <=0 ) { +dev_err (&streamlabs_wdt->intf->dev, +"failed to stop watchdog after 9 attempts!\n" ); + retval = -EIO ; + goto out; + } +/* +* Check if watchdog actually changed state to disabled. +* If watchdog is still enabled then reset message and retry +* stop command. +*/ +if (streamlabs_wdt->buffer[0 ] !=0xff || + streamlabs_wdt->buffer[1 ] !=0xbb ) { + streamlabs_wdt->buffer[0 ] =0xff ; + streamlabs_wdt->buffer[1 ] =0xbb ; + goto retry; + } + } + +out: +mutex_unlock (&streamlabs_wdt->lock); +return retval; +} + +static int usb_streamlabs_wdt_start (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_START ); +} + +static int usb_streamlabs_wdt_stop (struct watchdog_device *wdt_dev) +{ +return usb_streamlabs_wdt_command (wdt_dev,STREAMLABS_CMD_STOP ); +} + +static int usb_streamlabs_wdt_settimeout (struct watchdog_device *wdt_dev, +unsigned int timeout) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +mutex_lock (&streamlabs_wdt->lock); + wdt_dev->timeout = timeout; +mutex_unlock (&streamlabs_wdt->lock); + +return 0 ; +} + +static void usb_streamlabs_wdt_release_resources (struct kref *kref) +{ +struct streamlabs_wdt *streamlabs_wdt = + container_of(kref, struct streamlabs_wdt, kref); + +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); +kfree (streamlabs_wdt); +} + +static void usb_streamlabs_wdt_ref (struct watchdog_device *wdt_dev) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +kref_get (&streamlabs_wdt->kref); +} + +static void usb_streamlabs_wdt_unref (struct watchdog_device *wdt_dev) +{ +struct streamlabs_wdt *streamlabs_wdt =watchdog_get_drvdata (wdt_dev); + +kref_put (&streamlabs_wdt->kref, usb_streamlabs_wdt_release_resources); +} + +static const struct watchdog_info streamlabs_wdt_ident = { + .options = (WDIOF_SETTIMEOUT |WDIOF_KEEPALIVEPING ), + .identity =DRIVER_NAME , +}; + +static struct watchdog_ops usb_streamlabs_wdt_ops = { + .owner =THIS_MODULE , + .start = usb_streamlabs_wdt_start, + .stop = usb_streamlabs_wdt_stop, + .set_timeout = usb_streamlabs_wdt_settimeout, + .ref = usb_streamlabs_wdt_ref, + .unref = usb_streamlabs_wdt_unref, +}; + +static int usb_streamlabs_wdt_probe (struct usb_interface *intf, +const struct usb_device_id *id) +{ +struct usb_device *dev =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 (dev->product && dev->manufacturer && + (strncmp (dev->product,"USBkit" ,6 ) !=0 + ||strncmp (dev->manufacturer,"STREAM LABS" ,11 ) !=0 )) +return -ENODEV ; + + streamlabs_wdt =kmalloc (sizeof (struct streamlabs_wdt ),GFP_KERNEL ); +if (!streamlabs_wdt) { +dev_err (&intf->dev,"kmalloc failed\n" ); +return -ENOMEM ; + } + + streamlabs_wdt->buffer =kzalloc (BUFFER_LENGTH ,GFP_KERNEL ); +if (!streamlabs_wdt->buffer) { +dev_err (&intf->dev,"kzalloc for watchdog->buffer failed\n" ); + retval = -ENOMEM ; + goto err_nobuf; + } + +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->usbdev =interface_to_usbdev (intf); + streamlabs_wdt->intf = intf; +usb_set_intfdata (intf, &streamlabs_wdt->wdt_dev); +watchdog_set_drvdata (&streamlabs_wdt->wdt_dev, streamlabs_wdt); + +watchdog_init_timeout (&streamlabs_wdt->wdt_dev, + streamlabs_wdt->wdt_dev.timeout, &intf->dev); +watchdog_set_nowayout (&streamlabs_wdt->wdt_dev, nowayout); + +kref_init (&streamlabs_wdt->kref); + + retval =usb_streamlabs_wdt_stop (&streamlabs_wdt->wdt_dev); +if (retval) + goto err_wdt_buf; + + retval =watchdog_register_device (&streamlabs_wdt->wdt_dev); +if (retval) { +dev_err (&intf->dev,"failed to register watchdog device\n" ); + goto err_wdt_buf; + } + +dev_info (&intf->dev,"StreamLabs USB watchdog loaded.\n" ); + +return 0 ; + +err_wdt_buf: +mutex_destroy (&streamlabs_wdt->lock); +kfree (streamlabs_wdt->buffer); + +err_nobuf: +kfree (streamlabs_wdt); +return retval; +} + + + +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))
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 322
Quoted 5 lines
drivers/watchdog/streamlabs_wdt.c new file +5
-0+usb_streamlabs_wdt_command (&streamlabs_wdt->wdt_dev, +STREAMLABS_CMD_STOP ); + +return 0 ; +}If the result of a failure would be a spurious reset, you cannot drop errors.
Got it.
Quoted 15 lines
Quoted 15 lines
drivers/watchdog/streamlabs_wdt.c new file +15
-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_command (&streamlabs_wdt->wdt_dev, +STREAMLABS_CMD_START ); + +return 0 ; +} + +static void usb_streamlabs_wdt_disconnect (struct usb_interface *intf) +{ +struct streamlabs_wdt *streamlabs_wdt =usb_get_intfdata (intf);
Quoted 8 lines
drivers/watchdog/streamlabs_wdt.c line 339
Quoted 2 lines
drivers/watchdog/streamlabs_wdt.c new file +2
-0+ +/* First, stop sending USB messages to device. */ That is problematic. A disconnect can be triggered by removal of the hardware or in software through sysfs. In the latter case a running device without a driver would trigger a reset, wouldn't it? So I think for watchdog devices you need to go to an inactive state in
disconnect().
It has never occurred to me. Thanks for pointing it out. I will add stop command here.
[...]
Thanks for remarks.
Signature · 1 line
Best regards, Klimov Alexey
Quoted 16 lines · Alexey Klimov
Hi Oliver,
Quoted 8 lines · Oliver Neukum
drivers/watchdog/streamlabs_wdt.c line 105
Quoted 4 lines · Alexey Klimov
drivers/watchdog/streamlabs_wdt.c new file +4
-0+ streamlabs_wdt->buffer[3 ] =0x80 ; + + streamlabs_wdt->buffer[6 ] = (timeout_msec &0xff ) <<8 ; + streamlabs_wdt->buffer[7 ] = (timeout_msec &0xff00 ) >>8 ;We have macros for such conversions. Please use them.
I screwed here. It should be:
buffer[6] = timeout_msec & 0xff; buffer[7] = (timeout_msec >> 8) & 0xff;
Quoted 8 lines
However, are you talking about using
swab16()function? Or migrating tocpu_to_le()and friends functions?If it's acceptable way to make buffer with u16 type?
It slightly decreases lines of code and no conversion is needed here that way. I can do just without swapping bytes:
buffer[3] =timeout_msec;
and that's it.
NO!
You cannot make assumptions about the byte order of the host. This goes over the wire. It must be in a defined order. You need to use the cpu_to_* family of macros which will do the right thing. You just open coded them.
Sorry to be very blunt here, but we absolutely must not make assumptions about which hosts USB drivers can run on.
HTH
Oliver