Expand the SMBUS API to add a smbus_trans() function.

[grembo]

Summary

This is a port from DragonFlyBSD, adapted to fit FreeBSD's
infrastructure, especially device detection and numbering.

Motivation was to be able to use i2c connected devices through
by the ig4 driver (Intel 4th generation i2c driver), like the
cyapa touchpad driver and isl light sensor driver (all three drivers,
ig4, cyapa and isl are already prepared and waiting for committing
this change, see also http://blog.grem.de/pages/c720.html).

Test Plan

I tested those changes successfully on an Acer C720 laptop, I don't
own any significant smbus enabled devices, so maybe some more testing
would be interesting. I assume that the risk for current is quite limited
though.

@jhb I'm not certain if you're the correct person to review this change,
feel free to remove yourself and/or also add more reviewers.

[jhb]

sys/dev/smbus/smb.h

35 ↗

(On Diff #4055)

Even if it goes to HEAD it will break older binaries compiled against 9.x, etc. if they are run on an 11+ system. That said, I'm not sure this is a widely used API either. I would perhaps post this specific question on arch@ to ask if it's ok to break the ABI without providing a wrapper in this case. At least since you have changed the structure size it is relatively easy to add wrappers in the future if we find that we need them.

sys/dev/smbus/smbus.c

112

Very nice. I think the if addr check is fine for location_str.

204

I would probably go ahead and use a switch here to match other bus drivers (even though there is only 1 ivar currently). Perhaps something like:

addr = device_get_ivars(child);
if (addr == NULL)
    return (ENOENT);
switch (which) {
...

OTOH, you might also consider returning -1 for the addr for devices without a valid addr. (and make the ACCESSOR return an int.) Drivers could then do this in probe methods:

if (smbus_get_addr(dev) < 0)
    /* Not an enumerated device. */
    return (ENXIO);

Similarly, smbus.c's probe should probably be changed to do something like:

if (smbus_get_addr(dev) >= 0)
   /* Ignore enumerated devices. */
   return (ENXIO);

(Of course, I'd argue that /dev/smbX should be something that the smbus driver provides directly instead of having a fake device_t child to do it. /dev/pci isn't provided by a fake PCI device, it's handled by the bus driver directly. Then you would always have a valid smbus_get_addr()).

sys/dev/smbus/smbus.h

41 ↗

(On Diff #4135)

I think SMBUS_IVAR_ADDR is probably more consistent with other drivers?

Also, you should possibly add a __BUS_ACCESSOR() so drivers can use 'smbus_get_addr()'.

[grembo]

I sent an email to arch, let's see if there is any additional feedback. I'll submit a new version with the changes from the inline comments soon.

Here is the output of devinfo -v, I'm not sure what to make of if (isl_probe and cyapa_probe return ENXIO if no device is found):

ig4iic0 pnpinfo vendor=0x8086 device=0x9c61 subvendor=0x1025 subdevice=0x0a11 class=0x0c8000 at pci0:0:21:1 handle=\_SB_.PCI0.I2C0
  smbus0
    unknown
    cyapa1 at addr=0x67
    isl0
    cyapa0
ig4iic1 pnpinfo vendor=0x8086 device=0x9c62 subvendor=0x1025 subdevice=0x0a11 class=0x0c8000 at pci0:0:21:2 handle=\_SB_.PCI0.I2C1
  smbus1
    unknown
    isl2 at addr=0x44
    isl1
    cyapa2
hdac1 pnpinfo vendor=0x8086 device=0x9c20 subvendor=0x1025 subdevice=0x0a11 class=0x040300 at pci0:0:27:0 handle=\_SB_.PCI0.HDEF

sys/dev/smbus/smbus.c

204

I did what you suggested and made smbus_get_addr return -1 (using a slightly different code structure). While I agree that it would be nicer if smbus could provide smb itself, I think that it's a bit too much for this patch.

sys/dev/smbus/smbus.h

41 ↗

(On Diff #4135)

SMBUS_IVAR_ADDR was already defined in smbconf.h, so I removed the enum from smbus.h and turned the define in smbconf.h into an enum. I also added __BUS_ACCESSOR for addr.

[grembo]

Changes like discussed:

• Remove enum smbus_child_ivars, turn SMBUS_IVAR_ADDR into enum
• Add __BUS_ACCESSOR to address which returns int (can return -1)
• Change code structure of smbus_read_ivar and make it set set response to -1 in case no addr is set

[imp]

My biggest worry is about compat with the past. Otherwise there's stuff that can be improved, but it is minor.

share/man/man4/smb.4

29 ↗

(On Diff #4135)

Be sure to update this just before you commit. At least try to remember. People notice when this isn't within a couple of days and may send you email :)

156–165 ↗

(On Diff #4135)

So is the limit 32 or 1024? Or is 32 the limit before you need an external buffer?

sys/dev/smbus/smb.c

109 ↗

(On Diff #4135)

Not sure this comment adds much value.

189–190 ↗

(On Diff #4135)

mixing hex and decimal like this makes my brain hurt. Please use hex for both.

233 ↗

(On Diff #4135)

Don't you need an error != 0 here too?

242 ↗

(On Diff #4135)

ditto?

253 ↗

(On Diff #4135)

ditto?

268 ↗

(On Diff #4135)

why not error = EINVAL; ?

276–278 ↗

(On Diff #4135)

If you are ditching compat you can kill this. If not, then you need a lot more stuff like this...

sys/dev/smbus/smb.h

50 ↗

(On Diff #4135)

Why int for a count? Also, I'd use the SMB_MAXBLOCKSIZE. However, here it says 255 is the max, while below it tests for 1024. Which is it? ditto below for rcount.

53 ↗

(On Diff #4135)

Note: All the IOCTLs change number when you change the size of smbcmd like you have. That's likely OK, but I wanted to make sure this isn't an interface we need to have a compat shim for...

[grembo]

@imp Did a couple of more changes, please check the next iteration that I'll submit shortly.

share/man/man4/smb.4

29 ↗

(On Diff #4135)

I'll try :)

156–165 ↗

(On Diff #4135)

It's 32 byte for SMB 2.0, but 1024 for I2C transactions.

sys/dev/smbus/smb.c

109 ↗

(On Diff #4135)

I removed it.

189–190 ↗

(On Diff #4135)

It does.

233 ↗

(On Diff #4135)

That's been ported as is. I have to agree though, so I'll add:

if (error)
    break;

to match the style used in smb.c (instead of "if (!error && ...))

242 ↗

(On Diff #4135)

ditto.

253 ↗

(On Diff #4135)

ditto.

268 ↗

(On Diff #4135)

We already discussed that in this code review.
https://reviews.freebsd.org/D1955?id=4055#inline-11733

276–278 ↗

(On Diff #4135)

good point.

sys/dev/smbus/smb.h

50 ↗

(On Diff #4135)

See above for 32 vs 1024 byte.
We could also make it unsigned, but I wouldn't do that without a compelling reason.

Not sure about this comment, I blindly ported that one. I'll remove the comment.

[grembo]

Changes as suggested by @imp

[grembo]

@jhb and @imp:

Maybe you can check the latest changes:

https://reviews.freebsd.org/D1955?vs=4135&id=4320&whitespace=ignore-all#toc

AFAIK there are three issues left, suggestions are welcome:

1. Backwards compatibility - @imp suggested that there could be uses of SMB in xorg's video drivers, but I couldn't confirm that these actually depend on this interface. Maybe you can point me to the correct ports, as I can't find anything.
2. Clarifying that 32 bytes is the normal SMB message size and 1024 is allowed to allow I2C pass-through (maybe just documentation).
3. Figuring out if the custom device smbus_add_child algorithm can be avoided.

I really would like to commit this to HEAD, as there is plenty of time until 11-RELEASE.

1. Deal with it when somebody reports issues (as the actual use of this interface in the field outside of the kernel is unclear).
2. Based on your input.
3. Using the current code this works ok, I would like to figure out why it's required though as the interaction with the generic smb driver is suboptimal in general.

My gut feeling is that there will be some fall-out whatsoever, so maybe it's better to commit this sooner rather than later, so problems can be worked out as they come up.

[jhb]

The changes I suggested should let you use the generic add_child method.

sys/dev/smbus/smb.c

103 ↗

(On Diff #4320)

This should change to not blindly attach (this was one of the things I mentioned earlier to remove the need for the custom smbus_add_child()). There are a few options you can use.

1. You can fail attach if smbus_get_addr(dev) is not -1.

2. You can return BUS_PROBE_NOWILDCARD so that it will not attach to devices unless they have hardcoded the "smb" name.

I would probably do both, so:

smb_probe(device_t dev)
{

    if (smbus_get_addr(dev) != -1)
        return (ENXIO);

    device_set_desc(dev, "SMBus generic I/O");
    return (BUS_PROBE_NOWILDCARD);
}

The other part of this change is that any other drivers like isl or cyapa should _not_ have an identify routine (just like PCI drivers don't create devices, they probe to devices created by the bus). They should also require a valid address by failing to probe a device if smbus_get_addr() is -1, so:

cyapa_probe(dev)
{
    ...
    if (smbus_get_addr(dev) == -1)
        return (ENXIO);
    ...
    /* Current probe routine */
}

[grembo]

Improvements as suggested by @jhb. This works now, thanks a lot for the
insight.

This also allowed me to replace device_get_ivars with smbus_get_addr in isl
and cyapa.

As devlist always showed "unknown" when smb wasn't loaded beforehand (and
then an additional smb device afterwards, without unknown going away)
I did the following change:

• device_add_child(dev, NULL, -1);

+ device_add_child(dev, "smb", -1);

Hope this is acceptable (the results are what I would like to see).

[jhb]

sys/dev/smbus/smbus.c

110

Yes, this is definitely correct. I had thought this was done in an identify routine in the 'smb' driver instead.

Odd, smb has an identify routine as well. This just gets weirder the more I look at this. :)

Can you try replacing this explicit 'device_add_child()' with a call to 'bus_generic_probe()'? That will actually call smb_identify() which is currently unused.

Hmm, thinking some more, it may be that the identify routine gets used when you kldload smb after smbus. Using bus_generic_probe() is more correct then as it makes things more consistent so that the device is always added by the same bit of code. If that works, please use that instead of the explicit device_add_child().

[grembo]

Use bus_generic_probe instead of device_add_child like suggested
by @jhb, works, thank you.

[grembo]

Same as before, but pass all changed files to arcanist.

[jhb]

In general I think this looks good. Just a few minor suggestions, and perhaps ask Warren to look over the manpage changes.

share/man/man4/smb.4

165 ↗

(On Diff #4659)

Please start new sentences on a new line in manpages. This makes it easier for translators. You might also want to add Warren Block on this to review the manpage changes for you.

sys/dev/smbus/smbus.c

108

You can probably drop this blank line, but that's a very minor nit.

151

Perhaps move this under bootverbose?

If you want to list the found devices that don't have an attached driver during boot, then you might consider having a no_match method to do this. You can look at the PCI bus driver in sys/dev/pci/pci.c for an example implementation.

[grembo]

Put output of successful probes under bootverbose (nomatch isn't
not worth the effort and could be added at a later point in time).

Minor formatting changes to man page and smbus as suggested.

[grembo]

@wblock Added you as a reviewer for man page changes. The code should be good enough at this point.

@jhb If you agree, could you please accept this revision? I will wait for Warren's feedback on the man page before committing it.

[wblock]

Just some comments on the man page. I have not checked it with textproc/igor or "mandoc -tlist", that should be done before the next pass.

Thanks!

share/man/man4/smb.4

40 ↗

(On Diff #4809)

Replace "i/o" with "I/O".

43 ↗

(On Diff #4809)

"In order to" is usually better replaced with just "To".

77 ↗

(On Diff #4809)

"to talk to" is ugly. Is it needed?

89 ↗

(On Diff #4809)

This can be rearranged:

command does not transfer any data.
It just issues the device address with write intent to the bus.

94 ↗

(On Diff #4809)

As above:

command does not transfer any data.
It just issues the device address with read intent to the bus.

98 ↗

(On Diff #4809)

"The SendByte command" is redundant and can be just

.Em SendByte
sends the byte provided in the

These can be done with the other commands also. The only time to point out whether it is a command or field or something is when that is not clear from the context.

100 ↗

(On Diff #4809)

"the cmd field" is somewhat redundant, could be just

...provided in
.Fa cmd
to the device.

107 ↗

(On Diff #4809)

As above:

be returned in
.Fa cmd .

130 ↗

(On Diff #4809)

"The" is not needed. Also, use active rather than passive: will be -> is.

Returned data is stored in

137 ↗

(On Diff #4809)

"and" is not needed.

139 ↗

(On Diff #4809)

As above:

Returned data is stored in

148 ↗

(On Diff #4809)

No comma after "device".

166 ↗

(On Diff #4809)

Does "available" mean it can be read from that constant, or that is the constant to modify if needed?

173 ↗

(On Diff #4809)

No "and".

183 ↗

(On Diff #4809)

"roll-up" should probably be written as two words. Also, break the line for a new sentence.

190 ↗

(On Diff #4809)

The comma goes at the end of the macro on the previous line, or the formatting might not be handled correctly. Note that there is a space before it. Also, "and" is not needed:

.Fa wbuf ,
then reads

[grembo]

Changes to man page like discussed with @wblock, for the remaining changes
to the man page I'll open a new code review request once this one has been
committed.

[wblock]

Looks good.

[grembo]

@adrian & @jhb: Could you both accept the revision (only changes since last accept were to the man page), so I can commit it. Thanks.

[jhb]

It was already accepted. If you just need it for the workflow so that the auto-close works you can accept it as yourself (for future reference)

[grembo]

Set all ending man page changes to closed for new review request.

[avg]

I realize that it has been quite a while since the code was reviewed and committed, but still.
Does smb_trans functionality really belong to smbus? I do not see anything like that in the SMBus specification.
Perhaps what was done here would have been better done in the i2c code?
I think that it should be possible to have a driver that provides both smbus and i2c interfaces if its hardware allows for that.
(Or just i2c plus iicsmb).
Most of pure SMBus HBAs can not support smbus_trans.

[avg]

head/sys/dev/smbus/smbus.h

33 ↗

(On Diff #4998)

Shouldn't SMBUS_ADDR_MAX be 0xf0 if we are using FreeBSD's conventional 8-bit slave addresses?

[avg]

head/sys/dev/smbus/smbus.c

148 ↗

(On Diff #4998)

This looks like it does an equivalent of smbus_readb(dev, slave, 0x01, ...), but only for drivers that implement smbus_trans.
I wonder why readb and not, say, recvb.
Also, why 0x01? 0x00 is even more magic...

[avg]

head/sys/dev/smbus/smbus.c

109 ↗

(On Diff #4998)

The slave address is incremented by 1 in this loop.
So, are we using 7-bit formatted slave addresses here?
Is smbus_trans expected to take such addresses?
Because I thought that the FreeBSD convention is to use 8-bit formatted addresses (with the lsb always being zero).
Yes, smb(4) says this:

The slave address is specified in the seven most
significant bits (i.e., “left-justified”). The least significant bit of
the slave address must be zero.

[grembo]

@avg Unfortunately I don't have much time at my hands right now and - like you pointed out - this has been committed quite a while ago. So I think it's best if you open a PR and create a patch/new code review. If time permits, I'm more than happy to help you testing the improved code.