Hi Guenter, On Thu, Mar 10, 2016 at 3:54 AM, Guenter Roeck <linux@xxxxxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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@xxxxxxxxx> >> + * >> + * 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@xxxxxxxxx>" >> +#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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
