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