imcsmb(4): Intel integrated Memory Controller (iMC) SMBus controller driver

[rpokala]

Summary

imcsmb(4) provides smbus(4) support for the SMBus controller functionality
in the integrated Memory Controllers (iMCs) embedded in Intel Sandybridge-
Xeon, Ivybridge-Xeon, Haswell-Xeon, and Broadwell-Xeon CPUs. Each CPU
implements one or more iMCs, depending on the number of cores; each iMC
implements two SMBus controllers (iMC-SMBs).

Test Plan

Tested with jedec_ts(4) slave devices on SuperMicro dual-Haswell (@allanjude) and SuperMicro Broadwell-DE (@rpokala). Tested with jedec_ts(4) and an out-of-tree NVDIMM driver by Panasas, using proprietary modifications as described in imcsmb.4 and imcsmb_pci.c

[rpokala]

style(9) changes: Correct formatting of function prototypes and declarations, and re-order variables.

[rpokala]

Remove some stray comments from the manpage.

[avg]

Generally looks good, but I have many code style comments and a couple of functional comments.
Gonna do another round after the issues are addressed.
Thanks!

sys/dev/imcsmb/imcsmb.c

56 ↗

(On Diff #39549)

Looks like these declarations of static functions are not really required.

104 ↗

(On Diff #39549)

What might those things be?..

It appears that imcsmb explicitly adds a single child and does not provide for any other child devices.
So, you can just do device_probe_and_attach(sc->smbus).
Alternatively, you can keep bus_generic_attach call below and remove this call.

133 ↗

(On Diff #39549)

• redundant braces
• better to use rc != 0
• no need for goto in so simple code

158 ↗

(On Diff #39549)

let's save a byte and a nit of vertical space here

190 ↗

(On Diff #39549)

Why this requirement?
It does not appear to be justified.

460 ↗

(On Diff #39549)

You either need 35000 as a starting value or to decrement by one to get 35ms.
Right now it's 3.5 ms.

493 ↗

(On Diff #39549)

style calls for (x & y) != 0

505 ↗

(On Diff #39549)

no space after ! operator

sys/dev/imcsmb/imcsmb_pci.c

149 ↗

(On Diff #39549)

I usually do not provide declarations for static functions unless they are really needed (and try to arrange function definitions such that the declarations are not needed).
E.g., in this case you declare these functions just to define them a few lines below.

174 ↗

(On Diff #39549)

bzero is not needed here.

190 ↗

(On Diff #39549)

Cast to void * is not needed, any pointer is compatible with that type.

194 ↗

(On Diff #39549)

imcsmb_pci and imcsmb are so intimately related, can we really expect any other drivers trying to place their devices on this bus?
E.g., imcsmb_pci, as it is now, won't provide ivars to those "cuckoo children".

222 ↗

(On Diff #39549)

• redundant braces
• style suggests the explicit comparison rc != 0

223 ↗

(On Diff #39549)

no need for goto as well

252 ↗

(On Diff #39549)

• redundant braces
• since there is no cleanup, the label and goto-s are not need as well as rc variable

261 ↗

(On Diff #39549)

A return is sufficient here.

360 ↗

(On Diff #39549)

It might make sense to add the new MODULE_PNP_INFOstuff here as I see a lot of ongoing work on adding that data.
That may require changing the format of imcsmb_pci_devices table and the device matching code.

sys/dev/imcsmb/imcsmb_var.h

84 ↗

(On Diff #39549)

This works, but the conventional way of providing ivars is to hide this kind of a structure (in imcsmb_pci.c) and to provide accessors for individual values. But that might an overkill in this case.

[rpokala]

Thanks for finding time for this! I'll address many of your comments and post an updated diff later today.

sys/dev/imcsmb/imcsmb.c

56 ↗

(On Diff #39549)

I was taught to always declare functions, even static ones, near the top of the file. It might not strictly be required, but it's not prohibited either, and I find it useful.

104 ↗

(On Diff #39549)

I admit that the bus_generic_probe() / bus_generic_attach() calls are cargo-culted from somewhere. I'll change that to device_probe_and_attach().

133 ↗

(On Diff #39549)

The braces are again a matter of personal style, and are allowed by style(9). I find it much safer to include them, because I've seen so many cases where their lack has lead to mistakes.

Good point about (rc != 0); that's my usual idiom, so I'll shamelessly blame my (departed) co-author for this one. ;-)

As far as the goto, it's another one of my rules-of-thumb: functions have one entry-point and one exit-point, and do any necessary cleanup on the way out. But you're right, it's a bit overkill in this case. I'll reverse the overall logic to test for (rc == 0) and only call device_delete_children() in that case.

190 ↗

(On Diff #39549)

This part definitely comes from my co-author. The issue is that we sometimes need to use this driver while panicking, and mutexes aren't allowed in that state. Since waiting requires a sleepable lock, he felt it better to just restrict this driver to not allowing waiting.

460 ↗

(On Diff #39549)

Whoops! Good catch!

493 ↗

(On Diff #39549)

Indeed it does.

505 ↗

(On Diff #39549)

Yeah, I think I did that a few places.

sys/dev/imcsmb/imcsmb_pci.c

149 ↗

(On Diff #39549)

I talked about this in ichsmb.c; my habit is to include them.

174 ↗

(On Diff #39549)

@cem made the same comment about the same thing in jedec_dimm(4). As I noted in that review, it's not strictly necessary, but there are quite a few drivers which do it anyway. But it's not a big deal either way; I'll remove it.

194 ↗

(On Diff #39549)

Again, it was cargo-culting. The original version of this driver had a single driver for the PCI device and the two controllers, and had directly-attached children rather than going through smbus(4).

I'll clean this up.

360 ↗

(On Diff #39549)

I've talked to @imp about this; I'm explicitly leaving it out for the initial commit, to make it easier to MFC. I plan on adding the MODULE_PNP_INFO in a follow-up, after I get guidance on how to make it work with the existing table. @imp said it's definitely possible, but the manpage for MODULE_PNP_INFO is still WIP so I'll need his help.

[avg]

sys/dev/imcsmb/imcsmb.c

190 ↗

(On Diff #39549)

Well, you can make a special case for panicstr != NULL && (how & SMB_WAIT) != 0.
BTW, all locks are just NOPs after a panic.

[rpokala]

sys/dev/imcsmb/imcsmb.c

104 ↗

(On Diff #39549)

Now that I think about it, imcsmb can only directly have a single smbus child, which is what slave devices like jedec_dimm(4) actually connect to. Similarly, imcsmb_pci can only directly have a pair of imcsmb children. So I'm getting rid of all the generic probe and attach stuff for both.

190 ↗

(On Diff #39549)

The more I think about this, the less I want to change it at this juncture; it would require a whole bunch of re-testing with our (proprietary <sigh>) NVDIMM driver.

Would you be okay with me leaving this as-is for now, with a promise to take a closer look at this idea in the near future?

sys/dev/imcsmb/imcsmb_pci.c

194 ↗

(On Diff #39549)

As mentioned above, since imcsmb_pci has exactly two imcsmb children, and imcsmb has exactly one smbus child, I have removed the generic probe/attach from both.

[rpokala]

Address most of @avg's review comments.

[rpokala]

sys/dev/imcsmb/imcsmb.c

505 ↗

(On Diff #39549)

Changed to (write_op == 0).

[avg]

sys/dev/imcsmb/imcsmb.c

56 ↗

(On Diff #39549)

I am certainly not asking for any change here.
Just sharing my line of reasoning.
These declarations do not provide any benefit at all. So, why have them?

104 ↗

(On Diff #39549)

Have you tested this?
I think that device_probe_and_attach is still required.
I think that the added device will not be automatically probed.

133 ↗

(On Diff #39549)

Regarding style(9), well, I know. It is a very recent change. I am certainly not insisting on any change, just sharing my reasoning.

In this case the function was very simple, the condition was trivial and the conditional code was trivial as well.
So, the code that could be just two lines actually used five lines because of the closing brace and surrounding empty lines.

In general, wherever the prescribed style leaves room for different flavours I always ask myself which variant would have more benefit rather than following a single dogmatic rule.
But that's just me.

190 ↗

(On Diff #39549)

Sure, no problem.

sys/dev/imcsmb/imcsmb_pci.c

174 ↗

(On Diff #39549)

I would say it differently.
It's not necessary at all unless the driver modifies the softc in its probe method, but wants to undo the modifications in its attach method. But the only reason to modify softc in probe is to pass some information from probe to attach, so practically it should never be necessary to zero softc in attach.

194 ↗

(On Diff #39549)

I think that you still need something to trigger probe-and-attach for the child devices.

[rpokala]

At this point, it looks like the only outstanding issue is either adding device_probe_and_attach(), or confirming that it's not necessary. I'll try to do that tomorrow.

Thanks again!

sys/dev/imcsmb/imcsmb.c

56 ↗

(On Diff #39549)

When skimming through files, it's handy to see up front what's in the file.

104 ↗

(On Diff #39549)

Hmmm... I did test, and it did work for me, but that might have been because smbus(4) was already loaded and had already parsed the hints. I'll double-check.

133 ↗

(On Diff #39549)

Sure. In my experience, having the braces is more beneficial than being a few lines shorter. :-)

sys/dev/imcsmb/imcsmb_pci.c

174 ↗

(On Diff #39549)

Yeah. @cem said essentially the same thing, and you're both right. Which is why I removed it here and in imcsmb.c. The next time I hack on jedec_dimm(4), I'll do the same there. :-)

194 ↗

(On Diff #39549)

I think it worked for me because smbus(4) was already loaded, the hints were already parsed, and smbus(4)'s probe and attach were able to use them as soon as they were instantiated by imcsmb_attach().

I'll do some more testing, potentially doing the explicit device_probe_and_attach(sc->smbus) in imcsmb_attach(). I don't think we need anything like that here though.

sys/dev/imcsmb/imcsmb_var.h

84 ↗

(On Diff #39549)

By that do you mean creating an imcsmb_pci_if.m, defining imcsmb_pci_get_reg_smb_stat, etc? Yeah, that's way overkill here. :-)

[avg]

sys/dev/imcsmb/imcsmb_var.h

84 ↗

(On Diff #39549)

It's not as hardcore but still some of boilerplate code.
For example, see IICBUS_ACCESSOR and its use in sys/dev/iicbus/iicbus.h.
And also iicbus_read_ivar in the corresponding C file.

[jhb]

share/man/man4/imcsmb.4

54 ↗

(On Diff #39817)

I would treat semi-colons like a line break and start the new sentence on a new line (in this case just move "each" to the next line) to match the rule of starting new sentences on a new line.

sys/dev/imcsmb/imcsmb.c

124 ↗

(On Diff #39817)

Given you use bus_generic_detach() here (which is fine), I would just use 'bus_generic_attach()' at the end of attach above.

143 ↗

(On Diff #39817)

Note that in most drivers, 'probe' is listed in the source before attach and detach and (also in the DEVMETHODS list) so that the code order matches the device life cycle.

218 ↗

(On Diff #39817)

style(9) would put ()'s around the call to icmsmb_transfer() and would also put a blank line before the return (same in the other one liners below).

332 ↗

(On Diff #39817)

You don't need casts when casting a void * pointer in C btw.

525 ↗

(On Diff #39817)

I would put probe first here as mentioned earlier.

528 ↗

(On Diff #39817)

You don't need this if you aren't going to call bus_generic_probe() (which you don't need for this driver)

529 ↗

(On Diff #39817)

You don't need the bus_driver_added and bus_new_pass lines here. The default handlers for these methods are the 'bus_generic_*' ones already from sys/kern/bus_if.m.

sys/dev/imcsmb/imcsmb_pci.c

168 ↗

(On Diff #39817)

Similar idea of doing 'probe', 'attach', and 'detach' for the order of functions, DEVMETHODS, etc.

252 ↗

(On Diff #39817)

Most other drivers would probably just do 'return (BUS_PROBE_DEFAULT);' here rather than using a goto.

283 ↗

(On Diff #39817)

I would probably use 'atomic_store_rel_int(&sc->semaphore, 0)' here.

308 ↗

(On Diff #39817)

I would perhaps write this as:

if (!atomic_cmpset_acq_int(&sc->semaphore, 0, 1)
   return (EWOULDBLOCK);

...

return (0);

and ditch the goto.

336 ↗

(On Diff #39817)

None of the bus methods here are needed.

194 ↗

(On Diff #39549)

You need bus_generic_attach() here. Nothing will call it automagically during boot for example.

sys/dev/imcsmb/imcsmb_var.h

84 ↗

(On Diff #39549)

I think in this case it's fine to just let the child drivers use device_get_ivars() directly rather than doing fullblown accessors.

[rpokala]

Call only bus_generic_attach(), but not bus_generic_probe(), from the attach
methods. This behaves properly for both pre-loading via loader.conf and loading
via kldload.

Also address some comments from @jhb.

[rpokala]

sys/dev/imcsmb/imcsmb.c

143 ↗

(On Diff #39817)

I'd prefer to keep it alphabetical, again for ease of skimming and searching.

529 ↗

(On Diff #39817)

I could have sworn that someone told me they were required. I must have misunderstood.

104 ↗

(On Diff #39549)

Discussed with @avg and @jhb in email; bus_generic_attach() is needed for the child's probe() and attach() to be called when the module is preloaded via loader.conf. When the driver is loaded as a module, the module event handler (or the generic one, if one isn't provided to DRIVER_MODULE) ends up causing the children to be probed and attached.

[rpokala]

sys/dev/imcsmb/imcsmb_pci.c

194 ↗

(On Diff #39549)

As mentioned elsewhere, added back bus_generic_attach(), which causes proper behavior for both pre-loaded and kldload-loaded cases.

[rpokala]

Address a few more things @jhb mentioned.

[rpokala]

sys/dev/imcsmb/imcsmb_pci.c

252 ↗

(On Diff #39817)

I've come across many places where an inline return didn't clean up properly. That lead to my personal habit of having a single return, preceded by whatever cleanup is necessary, with a label to jump there if needed.

283 ↗

(On Diff #39817)

Yeah, now that I'm looking more closely at it, I'm not sure why the original author did it that way; yours is clearer.

308 ↗

(On Diff #39817)

I'll switch to cmpset, but I'm going to keep the return at the end, because some of the motherboard-specific code in our internal version of the driver can change the return code.

[rpokala]

@avg / @jhb : I think I've addressed everything you were concerned about -- modulo some personal style, which is still conformant with style(9).

Please take another look. Thanks!