Re: [PATCH 4/6] Watchdog: introdouce "pretimeout" into framework

[Fu Wei]

Hi Guenter,

Great thanks for your review,
feedback inline below :-)

On 15 May 2015 at 21:33, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> On 05/15/2015 04:24 AM, fu.wei@xxxxxxxxxx wrote:
>>
>> From: Fu Wei <fu.wei@xxxxxxxxxx>
>>
>> Reasons:
>> (1)kernel already has two watchdog drivers are using "pretimeout":
>>         drivers/char/ipmi/ipmi_watchdog.c
>>         drivers/watchdog/kempld_wdt.c(but the definition is different)
>> (2)some other dirvers are going to use this: ARM SBSA Generic Watchdog
>>
>> Signed-off-by: Fu Wei <fu.wei@xxxxxxxxxx>
>> ---
>>   drivers/watchdog/watchdog_core.c | 66
>> ++++++++++++++++++++++++++++++++++++++++
>>   drivers/watchdog/watchdog_dev.c  | 48 +++++++++++++++++++++++++++++
>>   include/linux/watchdog.h         | 19 ++++++++++++
>>   3 files changed, 133 insertions(+)
>>
>> diff --git a/drivers/watchdog/watchdog_core.c
>> b/drivers/watchdog/watchdog_core.c
>> index cec9b55..6ca9578 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -54,6 +54,14 @@ static void watchdog_check_min_max_timeout(struct
>> watchdog_device *wdd)
>>                 wdd->min_timeout = 0;
>>                 wdd->max_timeout = 0;
>>         }
>> +       if (wdd->min_pretimeout && wdd->min_timeout < wdd->min_pretimeout)
>> {
>> +               pr_info("Invalid min timeout, resetting to min
>> pretimeout!\n");
>> +               wdd->min_timeout = wdd->min_pretimeout;
>> +       }
>> +       if (wdd->max_pretimeout && wdd->max_timeout < wdd->max_pretimeout)
>> {
>> +               pr_info("Invalid max timeout, resetting to max
>> pretimeout!\n");
>> +               wdd->max_timeout = wdd->max_pretimeout;
>> +       }
>
>
> I am a bit concerned about the context dependency introduced here. If
> someone calls
> _init_pretimeout after calling init_timeout, this may result in still
> invalid timeout
> values.

yes, that logic is not very clean, so my thought is :
maybe we can integrate watchdog_init_timeout and watchdog_init_pretimeout,
if maintainer agree to add pretimeout into framework.

>
>
>>   }
>>
>>   /**
>> @@ -98,6 +106,63 @@ int watchdog_init_timeout(struct watchdog_device *wdd,
>>   }
>>   EXPORT_SYMBOL_GPL(watchdog_init_timeout);
>>
>> +static void watchdog_check_min_max_pretimeout(struct watchdog_device
>> *wdd)
>> +{
>> +       /*
>> +        * Check that we have valid min and max pretimeout values, if
>> +        * not reset them both to 0 (=not used or unknown)
>> +        */
>> +       if (wdd->min_pretimeout > wdd->max_pretimeout) {
>> +               pr_info("Invalid min and max pretimeout, resetting to
>> 0!\n");
>> +               wdd->min_pretimeout = 0;
>> +               wdd->max_pretimeout = 0;
>> +       }
>> +}
>> +
>> +/**
>> + * watchdog_init_pretimeout() - initialize the pretimeout field
>> + * @pretimeout_parm: pretimeout module parameter
>> + * @dev: Device that stores the timeout-sec property
>> + *
>> + * Initialize the pretimeout field of the watchdog_device struct with
>> either
>> + * the pretimeout module parameter (if it is valid value) or the
>> timeout-sec
>> + * property (only if it is a valid value and the timeout_parm is out of
>> bounds).
>> + * If none of them are valid then we keep the old value (which should
>> normally
>> + * be the default pretimeout value.
>> + *
>> + * A zero is returned on success and -EINVAL for failure.
>> + */
>
>
> The new API function also needs to be documented in
> Documentation/watchdog/watchdog-kernel-api.txt.

good point, thanks will do

>
>> +int watchdog_init_pretimeout(struct watchdog_device *wdd,
>> +                            unsigned int pretimeout_parm, struct device
>> *dev)
>> +{
>> +       int ret = 0;
>> +       u32 timeouts[2];
>> +
>> +       watchdog_check_min_max_pretimeout(wdd);
>> +
>> +       /* try to get the timeout module parameter first */
>> +       if (!watchdog_pretimeout_invalid(wdd, pretimeout_parm) &&
>> +           pretimeout_parm) {
>> +               wdd->pretimeout = pretimeout_parm;
>> +               return ret;
>> +       }
>> +       if (pretimeout_parm)
>> +               ret = -EINVAL;
>> +
>> +       /* try to get the timeout_sec property */
>> +       if (!dev || !dev->of_node)
>> +               return ret;
>> +       ret = of_property_read_u32_array(dev->of_node,
>> +                                        "timeout-sec", timeouts, 2);
>> +       if (!watchdog_pretimeout_invalid(wdd, timeouts[1]) && timeouts[1])
>
>
> Guess we are still waiting for feedback from the devicetree maintainers on
> this.

yes!

>
> Both the above synchronization concern and this makes me wonder if we should
> introduce
> watchdog_init_timeouts() instead, which would take pretimeout as additional
> parameter.
> What do you think about that ?

yes, that is what I am thinking about

>
>
>> +               wdd->pretimeout = timeouts[1];
>> +       else
>> +               ret = -EINVAL;
>> +
>> +       return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(watchdog_init_pretimeout);
>> +
>>   /**
>>    * watchdog_register_device() - register a watchdog device
>>    * @wdd: watchdog device
>> @@ -119,6 +184,7 @@ int watchdog_register_device(struct watchdog_device
>> *wdd)
>>         if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
>>                 return -EINVAL;
>>
>> +       watchdog_check_min_max_pretimeout(wdd);
>>         watchdog_check_min_max_timeout(wdd);
>>
>>         /*
>> diff --git a/drivers/watchdog/watchdog_dev.c
>> b/drivers/watchdog/watchdog_dev.c
>> index 6aaefba..b519257 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -218,6 +218,38 @@ out_timeout:
>>   }
>>
>>   /*
>> + *     watchdog_set_pretimeout: set the watchdog timer pretimeout
>> + *     @wddev: the watchdog device to set the timeout for
>> + *     @pretimeout: pretimeout to set in seconds
>> + */
>> +
>> +static int watchdog_set_pretimeout(struct watchdog_device *wddev,
>> +                                  unsigned int pretimeout)
>> +{
>> +       int err;
>> +
>> +       if (!wddev->ops->set_pretimeout ||
>> +           !(wddev->info->options & WDIOF_PRETIMEOUT))
>> +               return -EOPNOTSUPP;
>> +
>> +       if (watchdog_pretimeout_invalid(wddev, pretimeout))
>> +               return -EINVAL;
>> +
>> +       mutex_lock(&wddev->lock);
>> +
>> +       if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
>> +               err = -ENODEV;
>> +               goto out_pretimeout;
>> +       }
>> +
>> +       err = wddev->ops->set_pretimeout(wddev, pretimeout);
>> +
>> +out_pretimeout:
>> +       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
>> @@ -388,6 +420,22 @@ static long watchdog_ioctl(struct file *file,
>> unsigned int cmd,
>>                 if (wdd->timeout == 0)
>>                         return -EOPNOTSUPP;
>>                 return put_user(wdd->timeout, p);
>> +       case WDIOC_SETPRETIMEOUT:
>> +               if (get_user(val, p))
>> +                       return -EFAULT;
>> +               err = watchdog_set_pretimeout(wdd, val);
>> +               if (err < 0)
>> +                       return err;
>> +               /* If the watchdog is active then we send a keepalive ping
>> +                * to make sure that the watchdog keep's running (and if
>
>
> s/keep's/keeps/

Great thanks, typo

>
>> +                * possible that it takes the new timeout) */
>
>
> Please use the common multi-line comment style (that it isn't used above is
> not
> an argument).

yes, my bad, will fixed .

>
>
>> +               watchdog_ping(wdd);
>> +               /* Fall */
>> +       case WDIOC_GETPRETIMEOUT:
>> +               /* timeout == 0 means that we don't use the pretimeout */
>> +               if (wdd->pretimeout == 0)
>> +                       return -EOPNOTSUPP;
>> +               return put_user(wdd->pretimeout, p);
>>         case WDIOC_GETTIMELEFT:
>>                 err = watchdog_get_timeleft(wdd, &val);
>>                 if (err)
>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>> index a746bf5..f0a3c5b 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 value
>>    * @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 *);
>> @@ -62,6 +64,9 @@ struct watchdog_ops {
>>    * @timeout:  The watchdog devices timeout value.
>>    * @min_timeout:The watchdog devices minimum timeout value.
>>    * @max_timeout:The watchdog devices maximum timeout value.
>> + * @pretimeout:        The watchdog devices pretimeout value.
>> + * @min_pretimeout:The watchdog devices minimum pretimeout value.
>> + * @max_pretimeout:The watchdog devices maximum pretimeout value.
>>    * @driver-data:Pointer to the drivers private data.
>>    * @lock:     Lock for watchdog core internal use only.
>>    * @status:   Field that contains the devices internal status bits.
>> @@ -86,6 +91,9 @@ struct watchdog_device {
>>         unsigned int timeout;
>>         unsigned int min_timeout;
>>         unsigned int max_timeout;
>> +       unsigned int pretimeout;
>> +       unsigned int min_pretimeout;
>> +       unsigned int max_pretimeout;
>>         void *driver_data;
>>         struct mutex lock;
>>         unsigned long status;
>> @@ -120,6 +128,14 @@ 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)
>> +{
>> +       return ((wdd->max_pretimeout != 0) &&
>> +               (t < wdd->min_pretimeout || t > wdd->max_pretimeout));
>> +}
>> +
>>   /* Use the following functions to manipulate watchdog driver specific
>> data */
>>   static inline void watchdog_set_drvdata(struct watchdog_device *wdd,
>> void *data)
>>   {
>> @@ -134,6 +150,9 @@ static inline void *watchdog_get_drvdata(struct
>> watchdog_device *wdd)
>>   /* drivers/watchdog/watchdog_core.c */
>>   extern int watchdog_init_timeout(struct watchdog_device *wdd,
>>                                   unsigned int timeout_parm, struct device
>> *dev);
>> +extern int watchdog_init_pretimeout(struct watchdog_device *wdd,
>> +                                   unsigned int pretimeout_parm,
>> +                                   struct device *dev);
>
>
> Please drop the 'extern'. Guess it may be time to clean up the watchdog core
> code ;-).

yes, you are right, but in different patchset ??

>
> Thanks,
> Guenter
>



-- 
Best regards,

Fu Wei
Software Engineer
Red Hat Software (Beijing) Co.,Ltd.Shanghai Branch
Ph: +86 21 61221326(direct)
Ph: +86 186 2020 4684 (mobile)
Room 1512, Regus One Corporate Avenue,Level 15,
One Corporate Avenue,222 Hubin Road,Huangpu District,
Shanghai,China 200021
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/