On Wed, Sep 14, 2016 at 07:54:34AM -0700, Guenter Roeck wrote:
> On 09/14/2016 01:06 AM, Mika Westerberg wrote:
> > On Tue, Sep 13, 2016 at 02:00:25PM -0700, Guenter Roeck wrote:
> > > On 09/13/2016 08:23 AM, Mika Westerberg wrote:
> > > > Starting from Intel Skylake the iTCO watchdog timer registers were
> > > > moved to
> > > > reside in the same register space with SMBus host controller. Not all
> > > > needed registers are available though and we need to unhide P2SB
> > > > (Primary
> > > > to Sideband) device briefly to be able to read status of required
> > > > NO_REBOOT
> > > > bit. The i2c-i801.c SMBus driver used to handle this and creation of the
> > > > iTCO watchdog platform device.
> > > >
> > > > Windows, on the other hand, does not use the iTCO watchdog hardware
> > > > directly even if it is available. Instead it relies on ACPI Watchdog
> > > > Action
> > > > Table (WDAT) table to describe the watchdog hardware to the OS. This
> > > > table
> > > > contains necessary information about the the hardware and also set of
> > > > actions which are executed by a driver as needed.
> > > >
> > > > This patch implements a new watchdog driver that takes advantage of the
> > > > ACPI WDAT table. We split the functionality into two parts: first part
> > > > enumerates the WDAT table and if found, populates resources and creates
> > > > platform device for the actual driver. The second part is the driver
> > > > itself.
> > > >
> > > > The reason for the split is that this way we can make the driver itself
> > > > to
> > > > be a module and loaded automatically if the WDAT table is found.
> > > > Otherwise
> > > > the module is not loaded.
> > > >
> > > What I don't really like is that the driver won't be in the watchdog
> > > directory,
> > > and will thus easily be overlooked in the future by watchdog maintainers.
> >
> > It is in ACPI directory because we expose the functionality to users as
> > "ACPI Watchdog Action Table (WDAT)" which works with other similar table
> > options in drivers/acpi/Kconfig.
> >
> > I'm fine moving the driver itself (wdat_wdt.c) under drivers/watchdog
> > but at least the enumeration part should be part of drivers/acpi and I
> > would still like to have only one user selectable option.
> >
>
> SGTM, but up to you and Wim to decide, really.
>
> > > > + wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000;
> > > > + wdat->wdd.min_timeout = wdat->period * tbl->min_count / 1000;
> > >
> > > Are those guaranteed to be correct, ie max_timeout >= min_timeout
> > > and both valid ?
> >
> > The WDAT spec says nothing about those. I'll add sanity check here and
> > return -EINVAL if the values cannot be used. While there I think I can
> > do the same for tbl->timer_period, just in case.
> >
> Using max_hw_heartbeat_ms would help a bit here. Then the actual timeout
> is not limited by the hardware maximum, and the watchdog core will ping
> the watchdog if the selected timeout is larger than the maximum hardware
> timeout.
>
> Not sure how well the core would handle a maximum timeout of 1ms, though,
> so some basic sanity checking might still make sense.
OK
> > > Reason for asking is that the core will accept any timeouts if, for
> > > example, max_timeout is 0.
> > >
> > > > + wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED;
> > > > + wdat->wdd.info = &wdat_wdt_info;
> > > > + wdat->wdd.ops = &wdat_wdt_ops;
> > > > + wdat->pdev = pdev;
> > > > +
> > > > + /* Request and map all resources */
> > > > + for (i = 0; i < pdev->num_resources; i++) {
> > > > + void __iomem *reg;
> > > > +
> > > > + res = &pdev->resource[i];
> > > > + if (resource_type(res) == IORESOURCE_MEM) {
> > > > + reg = devm_ioremap_resource(&pdev->dev, res);
> > > > + } else if (resource_type(res) == IORESOURCE_IO) {
> > > > + reg = devm_ioport_map(&pdev->dev, res->start,
> > > > 1);
> > > > + } else {
> > > > + dev_err(&pdev->dev, "Unsupported resource\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + if (!reg)
> > > > + return -ENOMEM;
> > > > +
> > > > + regs[i] = reg;
> > > > + }
> > > > +
> > > > + entries = (struct acpi_wdat_entry *)(tbl + 1);
> > > > + for (i = 0; i < tbl->entries; i++) {
> > > > + const struct acpi_generic_address *gas;
> > > > + struct wdat_instruction *instr;
> > > > + struct list_head *instructions;
> > > > + unsigned int action;
> > > > + struct resource r;
> > > > + int j;
> > > > +
> > > > + action = entries[i].action;
> > > > + if (action >= MAX_WDAT_ACTIONS) {
> > > > + dev_dbg(&pdev->dev, "Skipping unknown action:
> > > > %u\n",
> > > > + action);
> > > > + continue;
> > > > + }
> > > > +
> > > > + instr = devm_kzalloc(&pdev->dev, sizeof(*instr),
> > > > GFP_KERNEL);
> > > > + if (!instr)
> > > > + return -ENOMEM;
> > > > +
> > > > + INIT_LIST_HEAD(&instr->node);
> > > > + instr->entry = entries[i];
> > > > +
> > > > + gas = &entries[i].register_region;
> > > > +
> > > > + memset(&r, 0, sizeof(r));
> > > > + r.start = gas->address;
> > > > + r.end = r.start + gas->access_width;
> > > > + if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_MEMORY) {
> > > > + r.flags = IORESOURCE_MEM;
> > > > + } else if (gas->space_id == ACPI_ADR_SPACE_SYSTEM_IO) {
> > > > + r.flags = IORESOURCE_IO;
> > > > + } else {
> > > > + dev_dbg(&pdev->dev, "Unsupported address space:
> > > > %d\n",
> > > > + gas->space_id);
> > > > + continue;
> > > > + }
> > > > +
> > > > + /* Find the matching resource */
> > > > + for (j = 0; j < pdev->num_resources; j++) {
> > > > + res = &pdev->resource[j];
> > > > + if (resource_contains(res, &r)) {
> > > > + instr->reg = regs[j] + r.start -
> > > > res->start;
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + if (!instr->reg) {
> > > > + dev_err(&pdev->dev, "I/O resource not found\n");
> > > > + return -EINVAL;
> > > > + }
> > > > +
> > > > + instructions = wdat->instructions[action];
> > > > + if (!instructions) {
> > > > + instructions = devm_kzalloc(&pdev->dev,
> > > > + sizeof(*instructions),
> > > > GFP_KERNEL);
> > > > + if (!instructions)
> > > > + return -ENOMEM;
> > > > +
> > > > + INIT_LIST_HEAD(instructions);
> > > > + wdat->instructions[action] = instructions;
> > > > + }
> > > > +
> > > > + list_add_tail(&instr->node, instructions);
> > > > + }
> > > > +
> > > > + /* Make sure it is stopped now */
> > > > + ret = wdat_wdt_stop(&wdat->wdd);
> > >
> > > Why ? You could set max_hw_heartbeat_ms instead of max_timeout and
> > > inform the watchdog core that the watchdog is running (by setting
> > > the WDOG_HW_RUNNING status flag).
> >
> > Hmm is the watchdog core then kicking it?
> >
> > It is stopped here to make sure system does not reboot until userspace
> > explicitly opens the device and starts kicking the watchdog.
> >
>
> Yes, that functionality was recently added to the watchdog core.
Cool. So then I'll just set WDOG_HW_RUNNING and drop stopping the
watchdog here.
> > > > + */
> > > > + ret = wdat_wdt_stop(&wdat->wdd);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = wdat_wdt_set_timeout(&wdat->wdd,
> > > > wdat->wdd.timeout);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = wdat_wdt_enable_reboot(wdat);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + ret = wdat_wdt_ping(&wdat->wdd);
> > > > + if (ret)
> > > > + return ret;
> > >
> > > The watchdog is already running here. Why start it again ?
> >
> > No it's not. We stopped it few lines above before we reprogram it.
> >
> But you start it below, don't you ? And it is stopped here, so why ping it ?
>
> If it is necessary to ping the watchdog before starting it,
> maybe the start code should do it ?
Now that you mentioned, I don't immediately remember why we ping it
here. It should not be necessary at this point. I'll remove that call in
next revision.
