nvdimm(4): Export NVDIMM health flags via sysctl

[rpokala]

Summary

The ACPI NFIT specification defines a set of "NVDIMM State Flags". These
flags are already reported by `acpidump -t', but this change makes them
available on a per-device basis, in a format that is more easily parsed.

To simplify this, introduce acpi_nfit_get_memory_maps_by_dimm(), which
locates the (ACPI_NFIT_MEMORY_MAP)s associated with a given
(nfit_handle_t).

Test Plan

Compare sysctl dev.nvdimm.X.flags to acpidump -t

[mav]

In my driver in TrueNAS I am printing this in dmesg if anything other than ACPI_NFIT_MEM_HEALTH_ENABLED is set. Everything read from NFIT applies more to boot state, rather than run-time.

sys/dev/nvdimm/nvdimm.c

410

strdup() uses M_WAITOK inside. It can not fail.

[rpokala]

Address review comments from @mav

• strdup(9) is M_WAITOK
• Log if any fault bit is set

[rpokala]

In D28700#642544, @mav wrote:

In my driver in TrueNAS I am printing this in dmesg if anything other than ACPI_NFIT_MEM_HEALTH_ENABLED is set. Everything read from NFIT applies more to boot state, rather than run-time.

Good idea.

sys/dev/nvdimm/nvdimm.c

410

Ah, right you are. Good catch.

[cem]

sys/dev/nvdimm/nvdimm.c

46

nit: sys/types is redundant here with sys/param at the top

373–376

I think we should probably log a reason when we fail attach (and there isn't concern we're the wrong driver for the hardware).

Also, stylistically, I'd suggest a single exit path that frees these allocations.

// much earlier:
maps = NULL;
sb = NULL;

if (num_maps == 0) {
  error = ENXIO;
  log();
  goto out;
}

380–397

Should we do this per-map instead of OR'ing all maps' flags together?

398–402

For sbuf it is canonical to just do error detection at finish(), as long as there isn't anything super expensive between the first possible failure and finish. It would be a good fit here, IMO.

410

We really should have a sbuf_detach() function for this but we don't appear to. strdup works.

418–425

e.g.,

  if (error == 0) {
    ...; // as-is
  }
  error = 0; // missing label is ok
out:
  if (error != 0) {
    free(nv->nv_flush_addr, ...);
    free(maps, ...);
  }
  return (error);

[rpokala]

sys/dev/nvdimm/nvdimm.c

46

So it is! And in fact, style(9) mentions that. I'll remove it.

(<rant-let>I much prefer the way we do it at ${JOB}, where everything includes what it uses, without magic undeclared dependencies.</rant-let>)

373–376

Also, stylistically, I'd suggest a single exit path that frees these allocations.

That's also my preference, when left to my own devices. (And my own device drivers; see jedec_dimm(4) and imcsmb(4). 😜)

But that's not the style used in this driver, and I generally try to match the rest of the file.

380–397

As explained in the comment, all these maps are associated with the same NVDIMM, so a failure on any one of the maps needs to be reported for the whole device.

(FWIW, neither I nor @mav have seen more than a single map on a given device, though a single map might span multiple devices.)

398–402

Noted.

[rpokala]

Address review comments from @cem

• <sys/types.h> is unnecessary
• Log attach failures
• sbuf error-checking can be deferred to after sbuf_finish()

[cem]

sys/dev/nvdimm/nvdimm.c

385–386

fwiw, sbuf_new_auto is M_WAITOK and can't fail

[rpokala]

sys/dev/nvdimm/nvdimm.c

385–386

Ooo, you're right about that one too!

[rpokala]

Address review comments from @cem

• sbuf_new_auto() is M_WAITOK

[cem]

Still accepted from earlier