On Friday 01 May 2015 16:22:08 Fu Wei wrote:
switch (cmd) {
case WDIOC_SETPRETIMEOUT:
if (get_user(new_value, p))
return -EFAULT;
ret = sbsa_gwdt_set_pretimeout(wdd, new_value);
break;
case WDIOC_GETPRETIMEOUT:
ret = put_user(gwdt->pretimeout, (unsigned int __user
*)arg);
break;
case SBSA_GWDIOC_SETPREACTION:
if (get_user(new_preaction, preaction))
return -EFAULT;
ret = sbsa_gwdt_set_preaction(gwdt, new_preaction);
break;
case SBSA_GWDIOC_GETPREACTION:
ret = put_user(gwdt->preaction, (int __user *)arg);
break;
}
return ret;
+}
Don't introduce driver specific commands here. Instead, make a separate patch to add new generic callbacks so the user API is shared with all drivers.
yes, that makes sense, I will make a separate patch for that, how about sysfs interface?
I think an ioctl interface makes most sense here if we want something to be configured, for consistency with the existing interfaces.
However, we should try to avoid adding driver specific interfaces for anything that can be part of the core.
It would be helpful to add support for the WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT calls in the watchdog core as well.
yes, there is a define in watchdog.h, but haven't been implemented in watchdog_dev.c only the kempld_wdt.c (which has two stages timeout also)use this , so I do the same way.
So for this driver, I this we can keep WDIOC_*PRETIMEOUT, and move SBSA_GWDIOC_*PREACTION to somewhere else(maybe remove?)
any suggestion ? thought?
I see two drivers that implement WDIOC_SETPRETIMEOUT. My guess is that the previous authors did not see it worthwhile to implement it in the core if there are only one or two users. However, now that there are three drivers including this one, it's clear that the support belongs into the core with a separate callback instead of a custom ioctl implementation.
+static irqreturn_t sbsa_gwdt_interrupt(int irq, void *dev_id) +{
struct sbsa_gwdt *gwdt = (struct sbsa_gwdt *)dev_id;
struct watchdog_device *wdd = &gwdt->wdd;
u32 status;
unsigned int left;
pr_debug("SBSA Watchdog: in %s\n", __func__);
status = sbsa_gwdt_reg_read(SBSA_GWDT_CF, SBSA_GWDT_WCS, wdd);
if (status & SBSA_GWDT_WCS_WS0) {
if (printk_ratelimit()) {
pr_crit("SBSA Watchdog WS0 is triggered\n");
left = sbsa_gwdt_get_timeleft(wdd);
pr_crit("please try to feed the dog in %ds!\n", left);
}
/* FIXME:anything we can do here?
* send_signal(sig, info, p, group); ??
*/
if (gwdt->preaction == SBSA_GWDT_PREACT_FEEDDOG) {
sbsa_gwdt_keepalive(wdd);
pr_warn("kernel keep system running by feeding
dog!\n");
What is the point of this? Is there any realistic use case for setting SBSA_GWDT_PREACT_FEEDDOG?
} else if (gwdt->preaction == SBSA_GWDT_PREACT_PANIC) {
panic("SBSA Watchdog pre-timeout");
}
Same question here: what is this good for?
I am not sure what is that best operation to do in this interrupt routine, so I offer some options to user. but we can discuss this and just keep one operation here(so we can delete SBSA_GWDIOC_*PREACTION as well)
now , we have tree option: (1) just a warning to tell user space : you need to feed the dog ( but what is the best way to sent this message? by signal ?)
This is very unlikely to ever be seen by a real user, and the message will otherwise just be lost.
(2) feed the dog in the driver, this make sure kernel is alive, if not, WS1 will be triggered
This seems counterproductive, because it prevents the watchdog hardware to do its job of rebooting the system when user space is unresponsive.
(3)panic() (if we have kdump, we can tell what is wrong with kernel or app), backup and reboot. by this way , we have time to backup important date and debug instead of reset directly like normal WDT.
I can see value of this one, but if the other two are not needed, we can avoid adding an interface to configure the behavior and always do it this way if the pretimeout is set, or default to doing nothing when it is not set.
Use platform_get_irq() here to allow deferred probing of the irqchip.
I tried to used that API before, but I found out I need not only a irq number, but also flags, so I turn to use resource. any thing we can do to improve this ? any thought?
I don't see where in your driver you actually use the flags, but I also don't fully understand the ACPI portion. It seems that the probe function just sets a variable in the watchdog specific structure, but that one is never used.
+static const struct of_device_id sbsa_gwdt_of_match[] = {
{ .compatible = "arm,sbsa_gwdt", },
{},
+}; +MODULE_DEVICE_TABLE(of, sbsa_gwdt_of_match);
Please add a binding document for this that describes the mandatory properties and resources.
OK , will do in this patchset, right?
Yes, but as a separate patch.
Great thanks for your help, Arnd, and some of your suggestion will go into my next patch(V2). but we still need to decide : (1) do we need "PREACTION" code to give user option for the first timeout
My impression is that we should leave it out for now. Adding user space interfaces is complicated, and if someone absolutely needs it later, we can add it then.
Arnd