Page MenuHomeFreeBSD

nvdimm(4): Export NVDIMM health flags via sysctl
ClosedPublic

Authored by rpokala on Feb 16 2021, 9:02 AM.

Details

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

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 37148
Build 34037: arc lint + arc unit

Event Timeline

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
409

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

Address review comments from @mav

  • strdup(9) is M_WAITOK
  • Log if any fault bit is set
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
409

Ah, right you are. Good catch.

This revision is now accepted and ready to land.Feb 17 2021, 3:08 AM
sys/dev/nvdimm/nvdimm.c
45

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

372–375

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;
}
379–396

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

397–401

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.

409

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);
sys/dev/nvdimm/nvdimm.c
45

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

372–375

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.

379–396

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

397–401

Noted.

Address review comments from @cem

  • <sys/types.h> is unnecessary
  • Log attach failures
  • sbuf error-checking can be deferred to after sbuf_finish()
This revision now requires review to proceed.Feb 17 2021, 11:55 PM
cem added inline comments.
sys/dev/nvdimm/nvdimm.c
384–385

fwiw, sbuf_new_auto is M_WAITOK and can't fail

This revision is now accepted and ready to land.Feb 18 2021, 1:57 AM
sys/dev/nvdimm/nvdimm.c
384–385

Ooo, you're right about that one too!

Address review comments from @cem

  • sbuf_new_auto() is M_WAITOK
This revision now requires review to proceed.Feb 18 2021, 2:15 AM
This revision is now accepted and ready to land.Feb 18 2021, 9:44 PM

Still accepted from earlier