jedec_dimm(4): report asset info and temperatures for DDR3 and DDR4 DIMMs

[rpokala]

Summary

A super-set of the functionality of jedec_ts(4). jedec_dimm(4) reports asset information (Part Number, Serial Number) encoded in the "Serial Presence Detect" (SPD) data on JEDEC DDR3 and DDR4 DIMMs. It also calculates and reports the memory capacity of the DIMM, in megabytes. If the DIMM includes a "Thermal Sensor On DIMM" (TSOD), the temperature is also reported.

Test Plan

On a system already using jedec_ts(4):

1. Update /boot/device.hints as described in jedec_dimm.4
2. Update /boot/loader.conf to load jedec_dimm(4) rather than jedec_ts(4)
3. Reboot
4. Using sysctl dev.jedec_dimm.X, confirm that proper info is reported.

[avg]

Tested on a system with DDR3 memory, looks good!
A code review is on my to do list.

jedec_dimm0: failed to read dram_type
jedec_dimm1: <DDR3 DIMM> at addr 0xa2 on smbus0
jedec_dimm1: TSOD: ST Microelectronics TSOD
jedec_dimm2: failed to read dram_type
jedec_dimm3: <DDR3 DIMM> at addr 0xa6 on smbus0
jedec_dimm3: TSOD: ST Microelectronics TSOD

I have hints for four slots, but only two are populated.

dev.jedec_dimm.3.temp: 26.7C
dev.jedec_dimm.3.serial: df193164
dev.jedec_dimm.3.part: 9965432-082.A00LF
dev.jedec_dimm.3.capacity: 4096
dev.jedec_dimm.3.type: DDR3
dev.jedec_dimm.1.temp: 27.2C
dev.jedec_dimm.1.serial: de193164
dev.jedec_dimm.1.part: 9965432-082.A00LF
dev.jedec_dimm.1.capacity: 4096
dev.jedec_dimm.1.type: DDR3

Thank you!

[cem]

Did a quick skim. I think with a driver like this, the proof is kind of in the pudding. Plus, you've done your research. Pretty much all of my comments are style nits, except the observation that it seems like no cleanup is done on failed attach. And detach cleanup may be incomplete? But maybe there are really no allocations and a lot of those strings just end up in the softc.

share/man/man4/jedec_dimm.4

103–105 ↗

(On Diff #39362)

I might condense this to just: "Per JEDEC, the high nibble of SPD addresses will be 0xa." Or something like that.

sys/dev/jedec_dimm/jedec_dimm.c

144–145 ↗

(On Diff #39362)

These are all not style(9) declarations

196–197 ↗

(On Diff #39362)

style(9) nit: this should be one line

224 ↗

(On Diff #39362)

This is not needed — by API, attach gets a zeroed softc. Drivers can/do rely on this.

303 ↗

(On Diff #39362)

Suggest using a named constant for the numeric value (even though you document it directly above).

360–363 ↗

(On Diff #39362)

Does the value need to be copied or could we just refer directly to the resource string?

384 ↗

(On Diff #39362)

M_ZERO seems redundant given snprintf

397 ↗

(On Diff #39362)

Nothing is cleaned up or freed on attach error.

421–424 ↗

(On Diff #39362)

This isn't the usual style(9) way of defining function parameters

602–603 ↗

(On Diff #39362)

free(NULL) is valid, so might as well remove the conditional.

[cem]

That's: approval modulo style nits and an adequate explanation of cleanup.

[rpokala]

Address some of @cem's review comments.

[rpokala]

share/man/man4/jedec_dimm.4

103–105 ↗

(On Diff #39362)

I want to be explicit about the DTI concept, because I refer to it later when explaining how to convert from jedec_ts(4) to jedec_dimm(4).

sys/dev/jedec_dimm/jedec_dimm.c

144–145 ↗

(On Diff #39362)

style(9) rightly prohibits K&R declarations except when needed for compatibility, which is not the case here. But these aren't K&R declarations; they're ANSI, just split up one arg/line. I could have sworn I've seen examples of that in the tree.

But whatever; I'm not wedded to this format, so I'll change it.

224 ↗

(On Diff #39362)

It seems to be a common (or at least, not-uncommon) idiom:

[threepio:clean/base/head] rpokala% egrep -rl 'bzero\(sc,' sys/dev | wc
      24      24     706

And actually, DEVICE_PROBE(9) says:

If a success code of zero is returned, the driver can assume that it will
be the one attached, but must not hold any resources when the probe
routine returns.  A driver may assume that the softc is preserved when it
returns a success code of zero.

That means the softc is not necessarily pre-zeroed when attach() is called.

303 ↗

(On Diff #39362)

Yeah, I meant to do that.

(On the other hand, I don't use named constants for all the masks and shifts involved with parsing the SPD info either, so...)

360–363 ↗

(On Diff #39362)

I found out about resource_string_value() via a recent change from @imp; I confirmed with him (and @jhb) that it does in fact need to be copied.

384 ↗

(On Diff #39362)

Perhaps. But my general rule is to pre-zero unless there's a good reason not to. This is a small, rare allocation, so overhead is negligible.

397 ↗

(On Diff #39362)

That's by design: errors are only possible before the first malloc(), and neither it nor anything that follows can error out.

[cem]

sys/dev/jedec_dimm/jedec_dimm.c

144–145 ↗

(On Diff #39362)

I'm not saying they aren't ANSI, just that style(9) doesn't split them up line by line. There is definitely non-style(9)-conforming code in the tree.

224 ↗

(On Diff #39362)

At least as I understand it, that isn't what that paragraph means. John can correct me if I'm mistaken here.

The softc is always zeroed by the generic code. If your driver's probe code happens to modify the softc *and* your driver's probe routine returns zero, then your softc will not be entirely zero in attach.

Neither of these conditions holds for your driver: BUS_PROBE_DEFAULT is not zero, and your probe routine does not modify the allocated softc in any way.

303 ↗

(On Diff #39362)

For some reason I was more ok with the masks and shifts, vs this flag bit. I can't argue it's a principled position.

[cem]

sys/dev/jedec_dimm/jedec_dimm.c

224 ↗

(On Diff #39362)

See device_probe_child() and device_set_driver(). The former iterates all eligible drivers, invoking the latter each time. Each time, the latter frees any prior softc and allocates a new one with M_ZERO. If a DEVICE_PROBE() returns zero, the loop is short-circuited, which means device_set_driver() will not be called again, preserving the contents of dev->softc.

[cem]

sys/dev/jedec_dimm/jedec_dimm.c

224 ↗

(On Diff #39362)

Minor correction: device_set_driver() will get called again, but with the same driver value, and there is a short-circuit check for that in device_set_driver() that results in no-op (softc is still preserved).

[cem]

LGTM. Earlier version worked great on my board / DIMMs.