Message-ID: <CALW4P+Km7csSRgS5gBQz+B1mrpGy=aUF-yGZbd2EPNPcVabNow@mail.gmail.com>
Date:	Tue, 15 Mar 2016 01:02:44 +0000
From:	Alexey Klimov <klimov.linux@...il.com>
To:	Guenter Roeck <linux@...ck-us.net>
Cc:	linux-watchdog@...r.kernel.org,
	USB list <linux-usb@...r.kernel.org>, wim@...ana.be,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Yury Norov <yury.norov@...il.com>
Subject: Re: [PATCH] watchdog: add driver for StreamLabs USB watchdog device

Hi Guenter,

On Thu, Mar 10, 2016 at 3:54 AM, Guenter Roeck <linux@...ck-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@...il.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@...il.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@...il.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

Powered by blists - more mailing lists