| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <56383F29.4040701@roeck-us.net>
Date: Mon, 2 Nov 2015 20:59:21 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Robin Gong <b38343@...escale.com>
Cc: wim@...ana.be, linux-watchdog@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v1 1/2] watchdog: add WDIOC_SETPRETIMEOUT and
WDIOC_GETPRETIMEOUT
On 11/02/2015 08:47 PM, Robin Gong wrote:
> On Mon, Nov 02, 2015 at 08:04:20PM -0800, Guenter Roeck wrote:
>> On 11/02/2015 07:29 PM, Robin Gong wrote:
>>> Since the watchdog common framework centrialize the IOCTL interfaces of
>>> device driver now, the SETPRETIMEOUT and GETPRETIMEOUT need to be added
>>> in the common code.
>>>
>>> Signed-off-by: Robin Gong <b38343@...escale.com>
>>> ---
>>> drivers/watchdog/watchdog_dev.c | 38 ++++++++++++++++++++++++++++++++++++++
>>> include/linux/watchdog.h | 9 +++++++++
>>> 2 files changed, 47 insertions(+)
>>>
>>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>>> index 6aaefba..02632fe 100644
>>> --- a/drivers/watchdog/watchdog_dev.c
>>> +++ b/drivers/watchdog/watchdog_dev.c
>>> @@ -218,6 +218,37 @@ out_timeout:
>>> }
>>>
>>> /*
>>> + * watchdog_set_pretimeout: set the watchdog timer pretimeout
>>> + * @wddev: the watchdog device to set the timeout for
>>> + * @timeout: pretimeout to set in seconds
>>> + */
>>> +
>>> +static int watchdog_set_pretimeout(struct watchdog_device *wddev,
>>> + unsigned int timeout)
>>
>> Please align with "(".
> Will fix in v2.
>>
>>> +{
>>> + int err;
>>> +
>>> + if ((wddev->ops->set_pretimeout == NULL) ||
>>
>> Unnecessary ( ), and "== NULL" is unnecessary as well.
>>
> why? It's useful if other device driver didn't implement set_pretimeout.
if (!wddev->ops->set_pretimeout ||
>>> + !(wddev->info->options & WDIOF_PRETIMEOUT))
>>> + return -EOPNOTSUPP;
>>> + if (watchdog_pretimeout_invalid(wddev, timeout))
>>> + return -EINVAL;
>>> +
>>> + mutex_lock(&wddev->lock);
>>> +
>>> + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
>>> + err = -ENODEV;
>>> + goto out_timeout;
>>> + }
>>> +
>>> + err = wddev->ops->set_pretimeout(wddev, timeout);
>>> +
>>> +out_timeout:
>>> + mutex_unlock(&wddev->lock);
>>> + return err;
>>> +}
>>> +
>>> +/*
>>> * watchdog_get_timeleft: wrapper to get the time left before a reboot
>>> * @wddev: the watchdog device to get the remaining time from
>>> * @timeleft: the time that's left
>>> @@ -393,6 +424,13 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>>> if (err)
>>> return err;
>>> return put_user(val, p);
>>> + case WDIOC_SETPRETIMEOUT:
>>> + if (get_user(val, p))
>>> + return -EFAULT;
>>> + err = watchdog_set_pretimeout(wdd, val);
>>> + return err;
>>
>> return watchdog_set_pretimeout(wdd, val);
>>
>>> + case WDIOC_GETPRETIMEOUT:
>>> + return put_user(wdd->pretimeout, p);
>>> default:
>>> return -ENOTTY;
>>> }
>>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>>> index d74a0e9..6ddb2d6 100644
>>> --- a/include/linux/watchdog.h
>>> +++ b/include/linux/watchdog.h
>>> @@ -25,6 +25,7 @@ struct watchdog_device;
>>> * @ping: The routine that sends a keepalive ping to the watchdog device.
>>> * @status: The routine that shows the status of the watchdog device.
>>> * @set_timeout:The routine for setting the watchdog devices timeout value.
>>> + * @set_pretimeout:The routine for setting the watchdog devices pretimeout.
>>> * @get_timeleft:The routine that get's the time that's left before a reset.
>>> * @ref: The ref operation for dyn. allocated watchdog_device structs
>>> * @unref: The unref operation for dyn. allocated watchdog_device structs
>>> @@ -44,6 +45,7 @@ struct watchdog_ops {
>>> int (*ping)(struct watchdog_device *);
>>> unsigned int (*status)(struct watchdog_device *);
>>> int (*set_timeout)(struct watchdog_device *, unsigned int);
>>> + int (*set_pretimeout)(struct watchdog_device *, unsigned int);
>>> unsigned int (*get_timeleft)(struct watchdog_device *);
>>> void (*ref)(struct watchdog_device *);
>>> void (*unref)(struct watchdog_device *);
>>> @@ -86,6 +88,7 @@ struct watchdog_device {
>>> const struct watchdog_ops *ops;
>>> unsigned int bootstatus;
>>> unsigned int timeout;
>>> + unsigned int pretimeout;
>>
>> Description (further below) missing.
>>
> I describe it in the ahead of this structure as the above.
>>> unsigned int min_timeout;
>>> unsigned int max_timeout;
>>> void *driver_data;
>>> @@ -123,6 +126,12 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
>>> (t < wdd->min_timeout || t > wdd->max_timeout));
>>> }
>>>
>>> +/* Use the following function to check if a pretimeout value is invalid */
>>> +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd, unsigned int t)
>>
>> Please fix checkpatch warnings.
>>
> You mean over 80 characters? Will fix in v2.
>>> +{
>>> + return ((wdd->timeout != 0) && (t >= wdd->timeout));
>>
>> Unnecessary ( ), and "!= 0" is unnecessary.
>>
> I think t >= wdd->timeout is need, since it's a pre-timeout.
return wdd->timeout && t >= wdd->timeout;
>>> +}
>>> +
>>> /* Use the following functions to manipulate watchdog driver specific data */
>>> static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
>>> {
>>>
>>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists