Page MenuHomeFreeBSD

Add support for ACPI WDAT based watchdog timer.
Needs RevisionPublic

Authored by t_uemura_macome.co.jp on Thu, Nov 24, 4:23 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 30, 5:32 AM
Unknown Object (File)
Mon, Nov 28, 7:32 PM
Subscribers

Details

Reviewers
takawata
des
hrs
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.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 48484
Build 45370: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Add comment only man style for now. You may want to take a read style(9).

share/man/man4/wdatwd.4
55

mandoc -Tlint say new line, new sentence.
e.g. add new line after period.

67

same above.

80

sort .Xr item in word, not section, mandoc -Tlint says.

Add comment only man style for now. You may want to take a read style(9).

Thanks. I'm reading style(9) now and will address style issues shortly, such as // one-liner comment -> /* one-liner comment */ etc..

sys/dev/wdatwd/wdatwd.c
100

Milli-sec per watchdog tick

hrs requested changes to this revision.Wed, Nov 30, 6:38 PM

I put my comments inline. Many of them are style-related, but please fix the memory leak of the sc->res and sc->action queue, at least.

share/man/man4/wdatwd.4
3

Please drop this line if you can accept. See https://reviews.freebsd.org/rS333391 for more details.

51

s/described by/in/
s/found on found on some ACPI implementations.//

54

I do not think this paragraph is needed (until L.59) because it is an implementation detail that the user does not need to care about.

66

"Whether the timeout is configurable or not. It is 0 if configurable or any positive value if not."

67

Drop the sentence after "If 0, the watchdog..." because the described behavior is not related directly to this sysctl variable.

70

"The default value of the watchdog timeout in millisecond if any." Avoid to use "milli-sec".

73

"The status of the watchdog timer. 0 if not running, or 1 if running."

75

"The current value of the watchdog timeout in millisecond. This can be 0 on some systems, and the zero value means that the default timeout is used."

91

Drop this sentence because this says nothing. All of the drivers are always in development.

sys/dev/wdatwd/wdatwd.c
32

Please put URL references into the manual page, and use canonical ones found in "Links to ACPI-Related Documents" at uefi.org/acpi.

64

This is hard to understand. How about the following?

/*
 * Resource entry.  Every instruction has the corresponding ACPI GAS but
 * two or more instructions may access the same or adjacent register region(s).
 * So we need to merge all the specified resources.
 */
68

"ACPI resource type, SYS_RES_IOPORT or SYS_RES_MEMORY."

87

s/are operated/run/
s/sequencially/sequentially/

100

Write millisecond instead of "milli-sec".

118

stop_in_sleep and running should be boolean.

125

Use upper case for the macro name and make the backslash right-justified. See style(9) for more details.

129

Drop the semicolon at the end.

131

Drop this macro and use goto instead. See comments for L.206

147

Add the following:

const char *rw = NULL;

and see comments for L.163 and L.206.

163

Replace this with

if (ACPI_FAILURE(status)) {
    rw = "AcpiRead";
    goto fail;
}

even if it looks redundant. It is hard to understand that the macro wdatwd_acpi_status() can end up with return(ENXIO).

170

Ditto

181

Ditto

186

Ditto

194

Ditto

199

Ditto

206

Put the following for the error handling:

    return (0);
fail:
    device_printf(sc->dev,
        "action: 0x%02x, %s() returned: %d\n",
        action, rw, status);
    return (ENXIO);
233

Do not use ! for tests:

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

Simply write the following:

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

return (sc->running);
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.

713

Please write this conditional separately like this because a long expression is hard to follow:

if (wdatwd_set_stop(sc) != 0)
  goto fail;
if ((e = wdatwd_clear_status(sc)) && e != EOPNOTSUPP)
  goto fail;
if ((e = wdatwd_set_reboot(sc)) && e != EOPNOTSUPP)
  goto fail;
if ((e = wdatwd_get_countdown(sc, &sc->default_timeout) && e != EOPNOTSUPP)
  goto fail;
725

timeout might be a better name.

726

"The current watchdog timeout in millisecond. If 0, the default timeout is used."

729

"Whether the watchdog timer is running or not"

731

timeout_default might be a better name.

733

"The default watchdog timeout in millisecond"

735

timeout_configurable might be a better name.

737

"Whether the watchdog timeout is configurable or not"

779

Drop = 0 and do not write code relying on this whenever possible because it can easily cause a bug.

783

Use more simple conditionals:

if (sc->stop_in_sleep == false)
  return (0);

return (wdatwd_reset_countdown(sc) && wdatwd_set_running(sc))
This revision now requires changes to proceed.Wed, Nov 30, 6:38 PM
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

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