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

234

Do not use ! for tests:

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

281

Simply write the following:

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

return (sc->running);

317

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

322

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);
}

348

Same here as L.316.

359

• 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

364

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

365

-EINVAL might be better than the magic number.

384

Use ssize_t

389

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

390

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;

398

No return is needed for void functions.

408

With the change in L.445:

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

411

With the change in L.445:

int overlap;

413

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));

442

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);

446

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.

448

Use |= instead of =

450

Use |= instead of +=

453

These should be before L.441

457

return (overlap);

466

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

468

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

478

WIth the change in L.445:

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

481

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

484

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

487

Use a macro.

491

Same here

505

Same here

522

if  (overlap > OVERLAP_NONE)

537

if (r2 == NULL)

554

Testing a pointer against NULL should be written as:

if (res->res != NULL) {

560

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

601

Return BUS_PROBE_DEFAULT instead of 0.

631

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;

635

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

638

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

671

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.

690

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

618

Use nitems(sc->action) instead of ACPI_WDAT_ACTION_RESERVED

638

Same here as L.617

666

Same here as L.617

[hrs]

share/man/man4/wdatwd.4

66

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

70

Same as L.65

73

Same as L.65

75

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

55

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

408

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.

453

These assignments ain't necessary anymore.

671

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

278

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

328

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

129

Consitify wherever possible:

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

222

Consitify wherever possible:

wdatwd_reset_countdown(const struct wdatwd_softc *sc)

231

/unsigned int/u_int/

254

Consitify wherever possible:

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

263

Consitify wherever possible:

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

301

Consitify wherever possible:

wdatwd_clear_status(const struct wdatwd_softc *sc)

310

Consitify wherever possible:

wdatwd_set_reboot(const struct wdatwd_softc *sc)

373

The return type should be ssize_t instead of int.

447

Constify wherever possible:

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

480

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;
}

487

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) {
...

501

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;
}

514

Same as L.501

695

This condition should be handled by wdat_merge_region().

765

Use EVENTHANDLER_PRI_ANY instead of 0.

[takawata]

sys/dev/wdatwd/wdatwd.c

621

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

501

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.

621

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.