Page MenuHomeFreeBSD

nvdimm: only enumerate present nvdimm devices
ClosedPublic

Authored by scott.d.phillips_intel.com on Dec 5 2018, 5:33 PM.

Details

Summary

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

Submitted by: D Scott Phillips <d.scott.phillips@intel.com>
Sponsored by: Intel Corporation
MFC after: 1 week

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

FYI, this diff is on top of D18346

scott.d.phillips_intel.com edited the summary of this revision. (Show Details)

rebase diff so that it is no longer against D18346

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

scott.d.phillips_intel.com retitled this revision from nvdimm: factor out NFIT table walking to nvdimm: only enumerate present nvdimm devices.
scott.d.phillips_intel.com edited the summary of this revision. (Show Details)
scott.d.phillips_intel.com edited the test plan for this revision. (Show Details)
kib added inline comments.Jan 27 2019, 7:25 PM
sys/dev/nvdimm/nvdimm.c
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 ?

sys/dev/nvdimm/nvdimm_nfit.c
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.

scott.d.phillips_intel.com 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
scott.d.phillips_intel.com marked 7 inline comments as done.Jan 29 2019, 8:00 PM
scott.d.phillips_intel.com added inline comments.
sys/dev/nvdimm/nvdimm.c
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.

sys/dev/nvdimm/nvdimm_nfit.c
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.

scott.d.phillips_intel.com marked 3 inline comments as done.
  • fix one more style(9) problem
scott.d.phillips_intel.com marked an inline comment as done.Jan 29 2019, 9:18 PM
kib accepted this revision.Jan 31 2019, 10:32 PM
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.