Page MenuHomeFreeBSD

nvdimm: only enumerate present nvdimm devices
ClosedPublic

Authored by scottph 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

scottph created this revision.Dec 5 2018, 5:33 PM

FYI, this diff is on top of D18346

scottph edited the summary of this revision. (Show Details)Dec 10 2018, 10:11 PM
scottph updated this revision to Diff 51841.

rebase diff so that it is no longer against D18346

scottph added inline comments.Dec 11 2018, 11:31 PM
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()

scottph retitled this revision from nvdimm: factor out NFIT table walking to nvdimm: only enumerate present nvdimm devices.Jan 3 2019, 8:10 PM
scottph edited the summary of this revision. (Show Details)
scottph edited the test plan for this revision. (Show Details)
scottph updated this revision to Diff 52523.
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.

scottph edited the summary of this revision. (Show Details)Jan 29 2019, 7:57 PM
scottph updated this revision to Diff 53403.
  • 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 marked 7 inline comments as done.Jan 29 2019, 8:00 PM
scottph 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.

scottph marked 3 inline comments as done.Jan 29 2019, 9:17 PM
scottph updated this revision to Diff 53404.
  • fix one more style(9) problem
scottph 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.