Page MenuHomeFreeBSD

nvdimm: only enumerate present nvdimm devices

Authored by scottph on Dec 5 2018, 5:33 PM.



Not all child devices of the NVDIMM root device represent DIMM
devices which are present in the system. The spec says (ACPI 6.2,
sec 9.20.2):

For each NVDIMM present or intended to be supported by platform,
platform firmware also exposes an NVDIMM device ... under the
NVDIMM root device.

Present NVDIMM devices are found by walking all of the NFIT
table's SPA ranges, then walking the NVDIMM regions mentioned by
those SPA ranges.

A set of NFIT walking helper functions are introduced to avoid the
need to splat the enumeration logic across several disparate

Submitted by: D Scott Phillips <>
Sponsored by: Intel Corporation
MFC after: 1 week

Diff Detail

rS FreeBSD src repository - subversion
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

scottph edited the summary of this revision. (Show Details)

rebase diff so that it is no longer against D18346

170 ↗(On Diff #51841)

Here we also need AcpiAttachData(handle, acpi_fake_objhandler, child); so that acpi_get_device() can work in nvdimm_find_by_handle()

scottph retitled this revision from nvdimm: factor out NFIT table walking to nvdimm: only enumerate present nvdimm devices.
scottph edited the summary of this revision. (Show Details)
scottph edited the test plan for this revision. (Show Details)
178 ↗(On Diff #52523)

see note below about the loop variable name.

181 ↗(On Diff #52523)

Do we need to flush already created children ?

185 ↗(On Diff #52523)

Why -1 ?

205 ↗(On Diff #52523)

Please do not name non-integer loop variable 'i'.

216 ↗(On Diff #52523)

Do we need to clear other already created spa's ?

41 ↗(On Diff #52523)

This '{' should be on the new line, alone.

_* are C-std reserved names, better not use them at all.

46 ↗(On Diff #52523)

return (*a - *b);

64 ↗(On Diff #52523)

You want to mask the value after dereferencing the pointer, right ? I would write it as

val &= mask;

on the next line.

More important, this code potentially accesses unaligned types, esp. when you do the trick with fetching uint16_t and passing UINT16_MAX for the mask. This only works on x86, and even there it works by chance.

73 ↗(On Diff #52523)

There is no need in {} around single-line block, usually.

114 ↗(On Diff #52523)

And there the body block is multi-line, so {} are warranted by style.

Several more {} vs. no-{} below, I did not marked them.

scottph edited the summary of this revision. (Show Details)
  • rebase patch
  • add commit message tags
  • Don't use i for non-int loop variables
  • Fix some style(9) issues with braces
  • Don't use variable names prefixed with underscore
  • Handle unaligned accesses in find_matches()
  • Fix an off-by-one error in the allocation of ivars
scottph added inline comments.
170 ↗(On Diff #51841)

This comment is obsoleted by the nvdimm root device's own ivars.

181 ↗(On Diff #52523)

the child devices and spas get destroyed on detach, which I think covers the case you mention.

185 ↗(On Diff #52523)

You're right, I think the -1 was wrong.

46 ↗(On Diff #52523)

oops, messed up a space here.

64 ↗(On Diff #52523)

changed this to bcopy to handle the unaligned access.

114 ↗(On Diff #52523)

Think I got them all, sorry for the simple mistakes.

scottph marked 3 inline comments as done.
  • fix one more style(9) problem
This revision is now accepted and ready to land.Jan 31 2019, 10:32 PM
This revision was automatically updated to reflect the committed changes.