Add support for ACPI WDAT based watchdog timer.

[t_uemura_macome.co.jp]

Summary

Simply said, WDAT is an abstraction for the real WDT hardware. For
instance, to add a newer generation WDT to ichwd(4), one must know the
detailed hardware registers, etc..

With WDAT, the necessary IO accesses to operate the WDT are comprehensively
described in it and no hardware knowledge is required.

With this driver, the WDT on Advantech ARK-1124C, Dell R210 and Dell R240 are
detected and operated flawlessly.

• While R210 is also supported by ichwd(4), others are not supported yet.

The unfortunate thing is that not all systems have WDAT defined.

Test Plan

ATM, this works properly on Advantech ARK-1124C, Dell R210 and Dell R240.

[hrs]

sys/dev/wdatwd/wdatwd.c

233

Do not use ! for tests:

e = wdatwd_action(sc, ACPI_WDAT_SET_COUNTDOWN, timeout, NULL);
if (e == 0)
  sc->timeout = timeout * sc->period;

280

Simply write the following:

sc->running = wdatwd_action(sc, ACPI_WDAT_SET_STOPPED_STATE, 0, NULL) ? true : false;

return (sc->running);

316

Use &cnt[0] and &cur[0] because there is no boundary checking if you use cnt or cur.

321

Move wdatwd_set_stop(sc) to the top and simplify the sc->running case by grouping the statements when not running because wdatwd_reset_countdown(sc) runs unconditionally:

if ((cmd & WD_INTERVAL) == 0)
  wdatwd_set_stop(sc);
else {
  if (!sc->running) {
    wdatwd_set_countdown(sc, cmd);
    wdatwd_set_running(sc);
    /* comments here */
  }
  wdatwd_reset_countdown(sc);
}

347

Same here as L.316.

358

• addr should be in ACPI_WDAT_ENTRY *, not void *
• remaining should be in ssize_t, not int
• The return value should be in size_t, not int

363

With the change in L.358, you can drop (int).

364

-EINVAL might be better than the magic number.

383

Use ssize_t

388

With the change in L.358, cp should be typecast to ACPI_WDAT_ENTRY * here.

389

The logic here is hard to follow. How about the following?

if (consumed < 0) {
    device_printf(sc->dev, "inconsistent WDAT table.\n");
    break;
}
remaining -= consumed;

397

No return is needed for void functions.

407

With the change in L.445:

static int
wdat_compare_region(ACPI_GENERIC_ADDRESS *rr, int *type, uint64_t *start, uint64_t *end)

410

With the change in L.445:

int overlap;

412

How about the following? See also L.445.

if (rr->AccessWidth < 1 || rr->AccessWidth > 4)
  return (-EINVAL);
s = rr->Address;
e = s + (1 << (rr->AccessWidth - 1));

441

Three assignments in L.452 should be just before this conditional. And then simplify the non-overlap case with the change in L.445:

if (*type != rr->SpaceId || s > *end || e < *start)
    return (OVERLAP_NONE);

445

Define macros instead of using magic numbers and comments. How about the following?

#define OVERLAP_NONE    0x0 // no overlap
#define OVERLAP_SUBSET  0x1 // rr is fully covered
#define OVERLAP_START   0x2 // the start is overlaped 
#define OVERLAP_END     0x4 // the end is overlapped

And this function should return the value of overlap directly. Your implementation returns EINVAL when rr->AccessWidth is invalid, but the return value is not used actually. So returning -EINVAL or any of OVERLAP_* constants would be more consistent.

447

Use |= instead of =

449

Use |= instead of +=

452

These should be before L.441

456

return (overlap);

465

Use consistent variable names. If you want to use two pointers, the should be res1 and res2 or r1 and r2.

467

There is no need to use found. See L.483 and L.521.

477

WIth the change in L.445:

overlap = wdat_compare_region(&wdat->entry.RegisterRegion, &type, &s, &e);

480

if (overlap == OVERLAP_NONE || overlap == -EINVAL || type != res->type)
  continue;

483

No need to use found here because overlap > OVERLAP_NONE means the same thing

486

Use a macro.

490

Same here

504

Same here

521

if  (overlap > OVERLAP_NONE)

536

if (r2 == NULL)

553

Testing a pointer against NULL should be written as:

if (res->res != NULL) {

559

The memory regions for res itself must be released. And entries of the sc->action queue must also be released.

600

Return BUS_PROBE_DEFAULT instead of 0.

630

Is there any specific reason to use "!!"? If you want to convert a positive value to true, use the following instead:

sc->stop_in_sleep = (sc->wdat->Flags & ACPI_WDAT_STOPPED) ? true : false;

634

Do not put whitespace between the type and the variable name.

637

This for-statement block should be under if (bootverbose) because this can slow down the normal execution flow.

670

Using wdat_compare_region() to fill res->{type, start, end} is difficult to undestand. Splitting wdat_compare_region() into two functions, one is to fill res->{type, start, end} and another is to detect the overlap would be better. And EINVAL must be checked before inserting the entry into the sc->res queue.

689

Do not return without freeing the memory regions for the sc->res and sc->action queue. And ENXIO should be used as the return value.

[hrs]

sys/dev/wdatwd/wdatwd.c

617

Use nitems(sc->action) instead of ACPI_WDAT_ACTION_RESERVED

637

Same here as L.617

665

Same here as L.617

[hrs]

share/man/man4/wdatwd.4

65

The sysctl node name should be dev.wdatwd.%d, not dev.wdat.%d

69

Same as L.65

72

Same as L.65

74

Same as L.65

[t_uemura_macome.co.jp]

I will update the new revision shortly.

I don't know whether this is the appropriate place to express my thanks but I'm deeply grateful for thorough review, Watanabe-san and Sato-san.

share/man/man4/wdatwd.4

54

Agreed. But I noticed I should note that either the real hardware specific driver or this driver can be used at a time.

sys/dev/wdatwd/wdatwd.c

407

This function, along with its caller wdat_merge_resource() were extensively reworked as you recommended at L.670, split it into two functions to make everything clearer.

Now wdat_alloc_region() decodes a GAS, malloc() a new structure and set it up, then wdat_merge_region() tries to merge structures by calling wdat_compare_region(). I think the latest approach is much leaner than the previous.

452

These assignments ain't necessary anymore.

670

As commented at L.407, now it's wdat_alloc_region().

[t_uemura_macome.co.jp]

• Many changes pointed to by hrs.
• Style(9) changes, some pointed to by takawata.

[t_uemura_macome.co.jp]

sys/dev/wdatwd/wdatwd.c

279

Above wdatwd_action() may fail returning EOPNOTSUPP. If that is the case, sc->running should be kept unmodified.

329

wdatwd_get_countdown() and wdatwd_get_current_countdown() may fail returning EOPNOTSUPP. If so, passed cnt and cur values are kept uninitialized in those functions.

[hrs]

In D37493#856006, @t_uemura_macome.co.jp wrote:

• Many changes pointed to by hrs.
• Style(9) changes, some pointed to by takawata.

Thanks for the update. I added comments about two minor style changes and one bug. The bug is removal of elements inside the TAILQ_FOREACH() loops and iterations using TAILQ_PREV() and TAILQ_NEXT(). Read the comments and let me know if I am missing something. Other than them, the quality looks pretty good now.

sys/dev/wdatwd/wdatwd.c

130

Consitify wherever possible:

wdatwd_action(const struct wdatwd_softc *sc, const u_int action, const uint64_t val, uint64_t *ret)

223

Consitify wherever possible:

wdatwd_reset_countdown(const struct wdatwd_softc *sc)

232

/unsigned int/u_int/

255

Consitify wherever possible:

wdatwd_get_current_countdown(const struct wdatwd_softc *sc, uint64_t *timeout)

264

Consitify wherever possible:

wdatwd_get_countdown(const struct wdatwd_softc *sc, uint64_t *timeout)

302

Consitify wherever possible:

wdatwd_clear_status(const struct wdatwd_softc *sc)

311

Consitify wherever possible:

wdatwd_set_reboot(const struct wdatwd_softc *sc)

374

The return type should be ssize_t instead of int.

448

Constify wherever possible:

wdat_compare_region(const struct wdat_res *res1, const struct wdat_res *res2)

481

The conditional at L.695 should be handled in the head of this function like this:

if (TAILQ_EMPTY(&sc->res)) {
        TAILQ_INSERT_HEAD(&sc->res, newres, link);
        return;
}

488

You need to use TAILQ_FOREACH_SAFE because one or more elements may be removed inside the iteration. The necessary changes will look like this:

-       struct wdat_res         *res1, *res2;
+       struct wdat_res         *res1, *res2, *res0;

-       TAILQ_FOREACH(res1, &sc->res, link) {
+       TAILQ_FOREACH_SAFE(res1, &sc->res, link, res0) {
...

502

This causes an infinite loop because res1 is not updated when the conditional is true. Was the following what you wanted to mean?

if (res1->type != res2->type) {
        res1 = res2;
         continue;
}

515

Same as L.501

696

This condition should be handled by wdat_merge_region().

766

Use EVENTHANDLER_PRI_ANY instead of 0.

[takawata]

sys/dev/wdatwd/wdatwd.c

622

To select specific driver for the specific device, the return value may be BUS_PROBE_GENERIC or BUS_PROBE_LOW_PRIORITY.

[t_uemura_macome.co.jp]

Much appreciated for finding out bugs and missing bits. I will upload a new revision shortly.

sys/dev/wdatwd/wdatwd.c

502

This is a bug. Types here may be either ACPI_ADR_SPACE_SYSTEM_IO or ACPI_ADR_SPACE_SYSTEM_MEMORY. In sc->res, there may be both types of regions co-existing. If that is the case, those different-type regions can't be merged.

I initially meant to skip res2 if res1 and res2 are in different types, and continue to the one next to res2, try to merge it with res1.

I need to keep res1 as this is to be merged, I first copy to res_itr and give it to TAILQ_PREV() and TAILQ_NEXT(). On type mismatch, copy res2 to res_itr and subsequent TAILQ_*() return the one next to res2.

622

This driver is most appropriate driver for WDAT so I think the return value should be BUS_PROBE_DEFAULT (or may be more prioritized).

While the WDAT backing device may be supported by the hardware-specific driver, WDAT is supported only by this driver, ATM. Since there's no way to determine the WDAT backing device is, thus it's impossible to decide automatically which hardware driver can/may be used without using this driver.

[t_uemura_macome.co.jp]

Fixed a bug in wdat_merge_region(), inifinite loop might occur if res1 and res2
had different types.

Marked all the comments as Done.

[t_uemura_macome.co.jp]

sys/dev/wdatwd/wdatwd.c

506

Perhaps I found a bug here, something like the following is necessary.
res1->start = newres->start;

521

And here.
res1->end = newres->end;

[t_uemura_macome.co.jp]

Will upload the new revision shortly, fixing missing assignments bugs.

[t_uemura_macome.co.jp]

• Fixed a bug in wdat_merge_region(), that there were a couple of missing assignments. Also added some more comments.