* [PATCH] watchdog: add driver for StreamLabs USB watchdog device
@ 2016-03-10 2:29 Alexey Klimov
2016-03-10 3:54 ` Guenter Roeck
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Alexey Klimov @ 2016-03-10 2:29 UTC (permalink / raw)
To: linux-watchdog
Cc: linux-usb, wim, linux, linux-kernel, yury.norov, klimov.linux
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 <klimov.linux@gmail.com>
---
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
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 80825a7..95d8f72 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -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
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f6a6a38..d54fd31 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -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
diff --git a/drivers/watchdog/streamlabs_wdt.c b/drivers/watchdog/streamlabs_wdt.c
new file mode 100644
index 0000000..031dbc35
--- /dev/null
+++ b/drivers/watchdog/streamlabs_wdt.c
@@ -0,0 +1,370 @@
+/*
+ * StreamLabs USB Watchdog driver
+ *
+ * Copyright (c) 2016 Alexey Klimov <klimov.linux@gmail.com>
+ *
+ * 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 <klimov.linux@gmail.com>"
+#define DRIVER_DESC "StreamLabs USB watchdog driver"
+#define DRIVER_NAME "usb_streamlabs_wdt"
+
+MODULE_AUTHOR(DRIVER_AUTHOR);
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_LICENSE("GPL");
+
+#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0
+#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011
+
+/* one buffer is used for communication, however transmitted message is only
+ * 32 bytes long */
+#define BUFFER_TRANSFER_LENGTH 32
+#define BUFFER_LENGTH 64
+#define USB_TIMEOUT 350
+
+#define STREAMLABS_CMD_START 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;
+ 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;
+ }
+
+ 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,
+ &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))
+ usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev,
+ STREAMLABS_CMD_STOP);
+
+ 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);
+
+ 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;
+ 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);
--
2.7.0.rc3
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-10 2:29 [PATCH] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
@ 2016-03-10 3:54 ` Guenter Roeck
2016-03-15 1:02 ` Alexey Klimov
2016-03-10 4:08 ` kbuild test robot
2016-03-10 9:23 ` Oliver Neukum
2 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2016-03-10 3:54 UTC (permalink / raw)
To: Alexey Klimov, linux-watchdog; +Cc: linux-usb, wim, linux-kernel, yury.norov
On 03/09/2016 06:29 PM, Alexey Klimov wrote:
> 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 <klimov.linux@gmail.com>
> ---
> 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
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 80825a7..95d8f72 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -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
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f6a6a38..d54fd31 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -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
>
> diff --git a/drivers/watchdog/streamlabs_wdt.c b/drivers/watchdog/streamlabs_wdt.c
> new file mode 100644
> index 0000000..031dbc35
> --- /dev/null
> +++ b/drivers/watchdog/streamlabs_wdt.c
> @@ -0,0 +1,370 @@
> +/*
> + * StreamLabs USB Watchdog driver
> + *
> + * Copyright (c) 2016 Alexey Klimov <klimov.linux@gmail.com>
> + *
> + * 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 <klimov.linux@gmail.com>"
> +#define DRIVER_DESC "StreamLabs USB watchdog driver"
> +#define DRIVER_NAME "usb_streamlabs_wdt"
> +
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
> +
> +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0
> +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011
> +
> +/* one buffer is used for communication, however transmitted message is only
> + * 32 bytes long */
/*
* Please use proper multi-line comments throughout.
*/
> +#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;
> + 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.
> + */
> + 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,
> + &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.
> + 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;
Can you use a loop instead ? goto's are ok when needed,
but here it just makes the code more difficult to read.
> + }
> + }
> +
> +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);
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.
> +
> + 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),
Unnecessary ( )
> + .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.
> +};
> +
> +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;
> + }
> +
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 ?
> + 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);
This is quite pointless, since timeout is already set, and you don't have
a module parameter to overwrite it.
> + 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))
> + usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev,
> + STREAMLABS_CMD_STOP);
Please call usb_streamlabs_wdt_stop().
> +
> + 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().
> +
> + 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;
> + 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().
> + /* 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);
>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-10 2:29 [PATCH] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
2016-03-10 3:54 ` Guenter Roeck
@ 2016-03-10 4:08 ` kbuild test robot
2016-03-10 9:23 ` Oliver Neukum
2 siblings, 0 replies; 9+ messages in thread
From: kbuild test robot @ 2016-03-10 4:08 UTC (permalink / raw)
To: Alexey Klimov
Cc: kbuild-all, linux-watchdog, linux-usb, wim, linux, linux-kernel,
yury.norov, klimov.linux
[-- Attachment #1: Type: text/plain, Size: 2116 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 44827 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-10 2:29 [PATCH] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
2016-03-10 3:54 ` Guenter Roeck
2016-03-10 4:08 ` kbuild test robot
@ 2016-03-10 9:23 ` Oliver Neukum
2016-03-16 0:57 ` Alexey Klimov
2 siblings, 1 reply; 9+ messages in thread
From: Oliver Neukum @ 2016-03-10 9:23 UTC (permalink / raw)
To: Alexey Klimov
Cc: linux-watchdog, yury.norov, wim, linux, linux-kernel, linux-usb
On Thu, 2016-03-10 at 02:29 +0000, Alexey Klimov wrote:
> 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
>
> USB commands used to communicate with device were reverse
> engineered using usbmon.
>
> Signed-off-by: Alexey Klimov <klimov.linux@gmail.com>
> ---
> 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
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 80825a7..95d8f72 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -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
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f6a6a38..d54fd31 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -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
>
> diff --git a/drivers/watchdog/streamlabs_wdt.c b/drivers/watchdog/streamlabs_wdt.c
> new file mode 100644
> index 0000000..031dbc35
> --- /dev/null
> +++ b/drivers/watchdog/streamlabs_wdt.c
> @@ -0,0 +1,370 @@
> +/*
> + * StreamLabs USB Watchdog driver
> + *
> + * Copyright (c) 2016 Alexey Klimov <klimov.linux@gmail.com>
> + *
> + * 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 <klimov.linux@gmail.com>"
> +#define DRIVER_DESC "StreamLabs USB watchdog driver"
> +#define DRIVER_NAME "usb_streamlabs_wdt"
> +
> +MODULE_AUTHOR(DRIVER_AUTHOR);
> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_LICENSE("GPL");
> +
> +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0
> +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011
> +
> +/* one buffer is used for communication, however transmitted message is only
> + * 32 bytes long */
> +#define BUFFER_TRANSFER_LENGTH 32
> +#define BUFFER_LENGTH 64
> +#define USB_TIMEOUT 350
> +
> +#define STREAMLABS_CMD_START 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.
> + 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;
> + }
> +
> + 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.
> +retry:
Why rewrite only a part of the buffer this way?
> + 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))
> + 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.
> +
> +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);
> +
> + /* 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().
> + 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);
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-10 3:54 ` Guenter Roeck
@ 2016-03-15 1:02 ` Alexey Klimov
2016-03-15 2:24 ` Guenter Roeck
0 siblings, 1 reply; 9+ messages in thread
From: Alexey Klimov @ 2016-03-15 1:02 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog, USB list, wim, Linux Kernel Mailing List, Yury Norov
Hi Guenter,
On Thu, Mar 10, 2016 at 3:54 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> On 03/09/2016 06:29 PM, Alexey Klimov wrote:
>>
>> 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 <klimov.linux@gmail.com>
>> ---
>> 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
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 80825a7..95d8f72 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -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
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index f6a6a38..d54fd31 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -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
>>
>> diff --git a/drivers/watchdog/streamlabs_wdt.c
>> b/drivers/watchdog/streamlabs_wdt.c
>> new file mode 100644
>> index 0000000..031dbc35
>> --- /dev/null
>> +++ b/drivers/watchdog/streamlabs_wdt.c
>> @@ -0,0 +1,370 @@
>> +/*
>> + * StreamLabs USB Watchdog driver
>> + *
>> + * Copyright (c) 2016 Alexey Klimov <klimov.linux@gmail.com>
>> + *
>> + * 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 <klimov.linux@gmail.com>"
>> +#define DRIVER_DESC "StreamLabs USB watchdog driver"
>> +#define DRIVER_NAME "usb_streamlabs_wdt"
>> +
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_LICENSE("GPL");
>> +
>> +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0
>> +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011
>> +
>> +/* one buffer is used for communication, however transmitted message is
>> only
>> + * 32 bytes long */
>
>
> /*
> * Please use proper multi-line comments throughout.
>
> */
Ok, will fix them all.
>> +#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;
>> + 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.
Ok, I will do something with this comment during re-work and rebase.
>> + */
>> + 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,
>> + &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.
Ok, will remove them. Or is it fine to convert them to dev_dbg?
>> + 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;
>
>
> Can you use a loop instead ? goto's are ok when needed,
> but here it just makes the code more difficult to read.
Sure.
>> + }
>> + }
>> +
>> +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);
>
>
> 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.
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 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 with
git://www.linux-watchdog.org
http://www.spinics.net/lists/linux-watchdog/msg07384.html
>> +
>> + 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),
>
>
> Unnecessary ( )
Ok.
>> + .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.
Got it.
>> +};
>> +
>> +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;
>> + }
>> +
>
>
> 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 ?
Thanks. Already moved to devm_kzalloc() and moved array inside
structure instead of allocating buffer separately.
>> + 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);
>
>
> This is quite pointless, since timeout is already set, and you don't have
> a module parameter to overwrite it.
Gonna remove it.
>> + 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))
>> + usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev,
>> +
>> STREAMLABS_CMD_STOP);
>
>
> Please call usb_streamlabs_wdt_stop().
>
>> +
>> + 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().
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?
>> +
>> + 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;
>> + 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().
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.
--
Best regards, Klimov Alexey
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-15 1:02 ` Alexey Klimov
@ 2016-03-15 2:24 ` Guenter Roeck
2016-03-16 1:11 ` Alexey Klimov
0 siblings, 1 reply; 9+ messages in thread
From: Guenter Roeck @ 2016-03-15 2:24 UTC (permalink / raw)
To: Alexey Klimov
Cc: linux-watchdog, USB list, wim, Linux Kernel Mailing List, Yury Norov
Hi Alexey,
On 03/14/2016 06:02 PM, Alexey Klimov wrote:
> Hi Guenter,
>
> On Thu, Mar 10, 2016 at 3:54 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>> On 03/09/2016 06:29 PM, Alexey Klimov wrote:
>>>
>>> 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 <klimov.linux@gmail.com>
>>> ---
>>> 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
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 80825a7..95d8f72 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -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
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index f6a6a38..d54fd31 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -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
>>>
>>> diff --git a/drivers/watchdog/streamlabs_wdt.c
>>> b/drivers/watchdog/streamlabs_wdt.c
>>> new file mode 100644
>>> index 0000000..031dbc35
>>> --- /dev/null
>>> +++ b/drivers/watchdog/streamlabs_wdt.c
>>> @@ -0,0 +1,370 @@
>>> +/*
>>> + * StreamLabs USB Watchdog driver
>>> + *
>>> + * Copyright (c) 2016 Alexey Klimov <klimov.linux@gmail.com>
>>> + *
>>> + * 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 <klimov.linux@gmail.com>"
>>> +#define DRIVER_DESC "StreamLabs USB watchdog driver"
>>> +#define DRIVER_NAME "usb_streamlabs_wdt"
>>> +
>>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>>> +MODULE_DESCRIPTION(DRIVER_DESC);
>>> +MODULE_LICENSE("GPL");
>>> +
>>> +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0
>>> +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011
>>> +
>>> +/* one buffer is used for communication, however transmitted message is
>>> only
>>> + * 32 bytes long */
>>
>>
>> /*
>> * Please use proper multi-line comments throughout.
>>
>> */
>
> Ok, will fix them all.
>
>
>>> +#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;
>>> + 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.
>
> Ok, I will do something with this comment during re-work and rebase.
>
>>> + */
>>> + 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,
>>> + &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.
>
> 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.
>>> + 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;
>>
>>
>> Can you use a loop instead ? goto's are ok when needed,
>> but here it just makes the code more difficult to read.
>
> Sure.
>
>>> + }
>>> + }
>>> +
>>> +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);
>>
>>
>> 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.
>
> 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 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 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).
>>> +
>>> + 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),
>>
>>
>> Unnecessary ( )
>
> Ok.
>
>>> + .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.
>
> Got it.
>
>>> +};
>>> +
>>> +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;
>>> + }
>>> +
>>
>>
>> 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 ?
>
> Thanks. Already moved to devm_kzalloc() and moved array inside
> structure instead of allocating buffer separately.
>
>>> + 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);
>>
>>
>> This is quite pointless, since timeout is already set, and you don't have
>> a module parameter to overwrite it.
>
> Gonna remove it.
>
>>> + 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))
>>> + usb_streamlabs_wdt_command(&streamlabs_wdt->wdt_dev,
>>> +
>>> STREAMLABS_CMD_STOP);
>>
>>
>> Please call usb_streamlabs_wdt_stop().
>>
>>> +
>>> + 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().
>
> 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 ?
>>> +
>>> + 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;
>>> + 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().
>
> 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.
>
I'll have to see the code to determine if it is clean.
> Thanks for review.
>
My pleasure.
Thanks,
Guenter
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-10 9:23 ` Oliver Neukum
@ 2016-03-16 0:57 ` Alexey Klimov
2016-03-16 9:07 ` Oliver Neukum
0 siblings, 1 reply; 9+ messages in thread
From: Alexey Klimov @ 2016-03-16 0:57 UTC (permalink / raw)
To: Oliver Neukum
Cc: Guenter Roeck, linux-watchdog, Yury Norov, wim,
Linux Kernel Mailing List, USB list
Hi Oliver,
On Thu, Mar 10, 2016 at 9:23 AM, Oliver Neukum <oneukum@suse.com> wrote:
> On Thu, 2016-03-10 at 02:29 +0000, Alexey Klimov wrote:
>> 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
>
>>
>> USB commands used to communicate with device were reverse
>> engineered using usbmon.
>>
>> Signed-off-by: Alexey Klimov <klimov.linux@gmail.com>
>> ---
>> 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
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 80825a7..95d8f72 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -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
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index f6a6a38..d54fd31 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -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
>>
>> diff --git a/drivers/watchdog/streamlabs_wdt.c b/drivers/watchdog/streamlabs_wdt.c
>> new file mode 100644
>> index 0000000..031dbc35
>> --- /dev/null
>> +++ b/drivers/watchdog/streamlabs_wdt.c
>> @@ -0,0 +1,370 @@
>> +/*
>> + * StreamLabs USB Watchdog driver
>> + *
>> + * Copyright (c) 2016 Alexey Klimov <klimov.linux@gmail.com>
>> + *
>> + * 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 <klimov.linux@gmail.com>"
>> +#define DRIVER_DESC "StreamLabs USB watchdog driver"
>> +#define DRIVER_NAME "usb_streamlabs_wdt"
>> +
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +MODULE_LICENSE("GPL");
>> +
>> +#define USB_STREAMLABS_WATCHDOG_VENDOR 0x13c0
>> +#define USB_STREAMLABS_WATCHDOG_PRODUCT 0x0011
>> +
>> +/* one buffer is used for communication, however transmitted message is only
>> + * 32 bytes long */
>> +#define BUFFER_TRANSFER_LENGTH 32
>> +#define BUFFER_LENGTH 64
>> +#define USB_TIMEOUT 350
>> +
>> +#define STREAMLABS_CMD_START 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.
Thanks, I removed it.
>> + 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;
>> + }
>> +
>> + 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.
>> +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.
>> + 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))
>> + 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.
>> +
>> +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);
>> +
>> + /* 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.
--
Best regards, Klimov Alexey
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-15 2:24 ` Guenter Roeck
@ 2016-03-16 1:11 ` Alexey Klimov
0 siblings, 0 replies; 9+ messages in thread
From: Alexey Klimov @ 2016-03-16 1:11 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-watchdog, USB list, wim, Linux Kernel Mailing List, Yury Norov
Hi Guenter,
On Tue, Mar 15, 2016 at 2:24 AM, Guenter Roeck <linux@roeck-us.net> wrote:
> Hi Alexey,
>
>
> On 03/14/2016 06:02 PM, Alexey Klimov wrote:
>>
>> Hi Guenter,
>>
>> On Thu, Mar 10, 2016 at 3:54 AM, Guenter Roeck <linux@roeck-us.net> wrote:
>>>
>>> On 03/09/2016 06:29 PM, Alexey Klimov wrote:
>>>>
>>>>
>>>> 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 <klimov.linux@gmail.com>
>>>> ---
>>>> drivers/watchdog/Kconfig | 15 ++
>>>> drivers/watchdog/Makefile | 1 +
>>>> drivers/watchdog/streamlabs_wdt.c | 370
>>>> ++++++++++++++++++++++++++++++++++++++
>>>> 3 files changed, 386 insertions(+)
[...]
>>>> +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.
>>>
>>>
>>>
>>> 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.
>>
>>>> + */
>>>> + 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,
>>>> + &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.
>>
>>
>> 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.
>>>> + 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;
>>>
>>>
>>>
>>> Can you use a loop instead ? goto's are ok when needed,
>>> but here it just makes the code more difficult to read.
>>
>>
>> Sure.
>>
>>>> + }
>>>> + }
>>>> +
>>>> +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);
>>>
>>>
>>>
>>> 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.
>>
>>
>> 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 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 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).
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.
[...]
>>>> +
>>>> +
>>>> +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().
>>>
>>>> +
>>>> + 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().
>>
>>
>> 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.
>>>> +
>>>> + 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;
>>>> + 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().
>>
>>
>> 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.
>>
>
> 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!
--
Best regards, Klimov Alexey
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device
2016-03-16 0:57 ` Alexey Klimov
@ 2016-03-16 9:07 ` Oliver Neukum
0 siblings, 0 replies; 9+ messages in thread
From: Oliver Neukum @ 2016-03-16 9:07 UTC (permalink / raw)
To: Alexey Klimov
Cc: Yury Norov, wim, Guenter Roeck, Linux Kernel Mailing List,
USB list, linux-watchdog
On Wed, 2016-03-16 at 00:57 +0000, Alexey Klimov wrote:
> Hi Oliver,
>
> On Thu, Mar 10, 2016 at 9:23 AM, Oliver Neukum <oneukum@suse.com> wrote:
> > On Thu, 2016-03-10 at 02:29 +0000, Alexey Klimov wrote:
> >> + 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.
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2016-03-16 9:10 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-10 2:29 [PATCH] watchdog: add driver for StreamLabs USB watchdog device Alexey Klimov
2016-03-10 3:54 ` Guenter Roeck
2016-03-15 1:02 ` Alexey Klimov
2016-03-15 2:24 ` Guenter Roeck
2016-03-16 1:11 ` Alexey Klimov
2016-03-10 4:08 ` kbuild test robot
2016-03-10 9:23 ` Oliver Neukum
2016-03-16 0:57 ` Alexey Klimov
2016-03-16 9:07 ` Oliver Neukum
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).