ig4 - Intel fourth gen integrated I2C SMBus driver.

[grembo]

Summary

This driver provides access to devices connected to the
integrated I2C devices in intel mobile CPUs over SMBus.
It has been ported and adapted from DragonFlyBSD.

It has been proven to run stable for about half a year
on at least two machines, but the code quality is a bit
suboptimal, which I hope to improve through this code review.

Specific questions:

• Are the file headers acceptable for ported code, or should there be also a mention of the FreeBSD project and/or my name?
• Timing in wait_status (ig4_iic.c) is dicey (see XXX and various comments).

Two new drivers (cyapa(4) and isl(4)) will take advantage
of ig4 (see http://blog.grem.de/pages/c720.html).

Test Plan

kldload ig4
kldload smb
(kldload isl/cyapa)
igor
mandoc -Tlint

[wblock]

Just comments, man page not tested with igor or mandoc -Tlint.

share/man/man4/ig4.4

35 ↗

(On Diff #5014)

Avoid using the informal "you" and "your" (https://www.freebsd.org/doc/en_US.ISO8859-1/books/fdp-primer/writing-style.html#writing-style-tips). This can be "the kernel configuration file:"

50 ↗

(On Diff #5014)

Shouldn't this be "...the SMBus"?

51 ↗

(On Diff #5014)

Do not capitalize "line" or "processors" unless those words are part of a specific brand name. The same for "intelligent systems".

Try to avoid "asides", sections that interrupt or stand outside the main flow of the sentence. "...including the i7-4650U..." could be used to avoid the parentheses, but the sentence is already too long and should be split into several. This can probably be done better than the example below. Generally, shorter sentences without pauses or asides are easier to read and understand.

driver provides access to peripherals attached to a I2C SMB controller.
.Nm
supports the SMBus controller found in fourth generation Intel(R) Core(TM) processors
based on the Mobile U-Processor line for Intelligent Systems.
This includes the i7-4650U, i5-4300U, i3-4010U, and 2980U.

54 ↗

(On Diff #5014)

s/The following/These/

59 ↗

(On Diff #5014)

This sentence is a little unclear. It can be misread as saying that setting the sysctl causes values to be dumped immediately, rather than just enabling debug output. It is also a bit passive rather than active. Maybe:

Set this to a non-zero value for controller register values to be dumped to the console and syslog.

[grembo]

Will send a new revision with man page changes shortly (is there a better workflow? It always seems odd to comment on changes here and then create another arc revision).

share/man/man4/ig4.4

35 ↗

(On Diff #5014)

I copied this from other manpages in man4, seems to be a standard text block, so I won't change it right now and leave a mass update to @manpages.

[root@ /usr/src/share/man/man4]# grep -il "the following lines in your" * | wc -l

207

51 ↗

(On Diff #5014)

I changed this to something similar, please check.

59 ↗

(On Diff #5014)

That's exactly what it does, it dumps them immediately and does not enable further debug output. It's similar to what snd_hda does.

sysctl debug.ig4_dump=1
debug.ig4_dump: 0 -> 1

Apr 27 10:39:05 flimsy kernel: ig4iic0: ig4iic register dump:
Apr 27 10:39:05 flimsy kernel: ig4iic0:   IG4_REG_CTL             00000063
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_TAR_ADD         00000067
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_SS_SCL_HCNT     00000064
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_SS_SCL_LCNT     0000007d
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_FS_SCL_HCNT     00000064
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_FS_SCL_LCNT     0000007d
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_INTR_STAT       00000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_INTR_MASK       00000204
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_RAW_INTR_STAT   00000010
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_RX_TL           00000001
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_TX_TL           00000010
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_I2C_EN          00000001
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_I2C_STA         00000006
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_TXFLR           00000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_RXFLR           00000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_SDA_HOLD        00000001
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_TX_ABRT_SOURCE  02000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_SLV_DATA_NACK   00000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_DMA_CTRL        00000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_DMA_TDLR        00000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_DMA_RDLR        00000000
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_SDA_SETUP       00000064
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_ENABLE_STATUS   00000001
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_COMP_PARAM1     001f1fee
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_COMP_VER        3131352a
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_COMP_TYPE       44570140
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_CLK_PARMS       00000001
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_RESETS          00000003
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_GENERAL         55000004
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_SW_LTR_VALUE    00000800
Apr 27 10:39:06 flimsy kernel: ig4iic0:   IG4_REG_AUTO_LTR_VALUE  00000800

[grembo]

Man page changes as suggested by @wblock.

[jhb]

How does this work in conjunction with the I2C bits in the KMS driver? (Are they completely independent and both can be loaded at the same time or do they conflict in some way?)

sys/dev/ichiic/ig4_iic.c

182 ↗

(On Diff #5019)

You shouldn't hardcode 10 ticks. hz is variable. Instead, you should use some time period that you care about, here it seems you perhaps mean 10 milliseconds?

Note that you can remove the imprecise ticks usage if you do the math based on the time passed to DELAY (25 microseconds) or mtx_sleep (10 milliseconds).

478 ↗

(On Diff #5019)

You don't really need the lock in attach, and it's probably not a good idea to hold it across new-bus calls like device_add_child.

481 ↗

(On Diff #5019)

Are these printfs only for debugging?

611 ↗

(On Diff #5019)

You can simplify this block by doing:

error = bus_generic_attach(sc->dev);
if (error) {
    device_printf(...);
    return;
}
mtx_lock(..);
sc->generic_attached;
mtx_unlock(...);

On the other hand, I think you don't even need generic_attached at all. bus_generic_detach() will skip any unattached children devices, so it is fine to just call always. That will let you remove the locking and 'generic_attached' member entirely.

635 ↗

(On Diff #5019)

I would detach child devices first and then quiesce the hardware. That will let any pending requests from child devices drain before you try to stop the hardware. In general it is better in detach routines to detach external references to the driver (like character devices, network interfaces, etc.) before shutting down the hardware itself. So I would do:

error = bus_generic_detach(sc->dev);
if (error)
    return (error);
device_delete_child(...);
bus_teardown_intr(...);
mtx_lock(...);
/* clear interrupt registers */
mtx_unlock(...);

Note, btw, that you shouldn't hold the mutex around several of these operations (new-bus, bus_teardown_intr).

sys/dev/ichiic/ig4_pci.c

88 ↗

(On Diff #5019)

This variable is always set to 1, perhaps it can be removed?

115 ↗

(On Diff #5019)

I prefer using 'bus_*(sc->regs_res, ...)' rather than 'bus_space_*(' with an explicit tag and handle as it is shorter and clearer. Old code still uses explicit tags and handles but newer drivers don't, and when I cleanup drivers to add locking I tend to convert them over to this as well.

sys/dev/ichiic/ig4_reg.h

19 ↗

(On Diff #5019)

The license is fine, though you could perhaps ask if they are open to switching to a 2-clause instead of 3-clause license.

[kib]

In D2372#43321, @jhb wrote:

How does this work in conjunction with the I2C bits in the KMS driver? (Are they completely independent and both can be loaded at the same time or do they conflict in some way?)

The i2c buses presented on the video outputs of Intel GFXes are dedicated to the communication with displays. They are controlled through the registers of GFX BAR and have no relation to the mobo infrastructure.

Can you point me to the relevant datasheet for this controllers ? Both 7-series spec update and 8-series PCH datasheets show completely different device ids.

sys/dev/ichiic/ig4_var.h

73 ↗

(On Diff #5019)

What is the purpose of the mutex ? Is it supposed to serialize access to the controller ?

If yes, it does not work at all, it seems, since you drop the mutex during the wait for controller actions.

[kib]

In D2372#43358, @kostikbel wrote:

Can you point me to the relevant datasheet for this controllers ? Both 7-series spec update and 8-series PCH datasheets show completely different device ids.

Ok, found the datasheets myself. This is the i2c controller which can do dma, right ?

[grembo]

@jhb This shouldn't interfere with I2C in KMS.
@jhb In terms of licensing, are there any best practices? Like if you port one feature from another BSD to FreeBSD (like a patch to the keyboard driver, maybe a few lines of code), will this affect the the license of the original file? When porting something completely new (like in this case), at which point should the porter be mentioned in the header - you clearly don't want to blame the original author for porting and/or system specific bugs. Something like:

* This code is derived from software contributed to The DragonFly Project
* by Matthew Dillon <[e-mail]> and was subsequently ported
* to FreeBSD by Firstname Lastname <[e-mail]>

(I don't think I can convince Matt to switch to a 2-clause license)

@kostikbel Yes, based on the Component Parameter Register at least the one in my laptop supports DMA.

[wblock]

share/man/man4/ig4.4

32 ↗

(On Diff #5019)

s/CPUs/CPU/

Is "device" needed?

.Nd Intel(R) fourth generation mobile CPU integrated I2C SMBus driver

53 ↗

(On Diff #5019)

Works for me, thanks!

35 ↗

(On Diff #5014)

Well, yes, it's just easier to fix a new page than an existing one.

59 ↗

(On Diff #5014)

Okay, but it's not clear that it is a one-shot. Does the sysctl reset to zero immediately?

[grembo]

Further man page improvements as suggested by @wblock

share/man/man4/ig4.4

32 ↗

(On Diff #5019)

Fine with me.

35 ↗

(On Diff #5014)

I changed this, if you agree with the new wording I would like to open a separate review request for all 228 man pages that use the exact same or very similar wording, so that future man page authors won't need to get corrected (maybe igor could check for that phrase as well):

[root@ /usr/src/share/man]# grep -R "following lines in your" *  | wc -l
     228

If I did such a review request, I assume that I won't change the man page date (as this is only a style fix), correct?

59 ↗

(On Diff #5014)

That's exactly how it works. I changed the wording, please check.

[wblock]

Man page looks good to me.

If you want to batch-modify the others to remove "your", that is a non-content change that would not require updating the .Dd in any of the man pages. However, that is not your fault and you are not obligated to fix it.

igor's -y option does some style checking. That option is normally off, because style is somewhat subjective:

igor -Ry /usr/src/share/man/man4/* | less -RS

Thanks!

share/man/man4/ig4.4

62 ↗

(On Diff #5043)

Nice!

[grembo]

Next iteration follows soon.

sys/dev/ichiic/ig4_iic.c

182 ↗

(On Diff #5014)

I changed this as suggested, so it should always be 1/100 second regardless of hz, changed the counter to be in us and add to it based on DELAY and mtx_sleep.

This of course ignores:

• Any time spent in other code within the loop (reg_read etc.)
• If mtx_sleep exits early.

Not sure if this is acceptable or not.

478 ↗

(On Diff #5014)

Ok, removed the lock. It's still held around calls to set_controller, as this requires the lock to be hold while calling mtx_sleep.

481 ↗

(On Diff #5014)

I removed those, as they're available through sysctl.

611 ↗

(On Diff #5014)

Ok, removed the member and the lock. I checked bus_generic_detach, it returns EBUSY in case the device is not attached, will that be a problem in ig4_detach?

635 ↗

(On Diff #5014)

I changed this based on what I understood. I'm not certain if locking is ok now. Note that bus_generic_detach returns EBUSY in case the device is not attached, could that be a problem?

int
bus_generic_detach(device_t dev)
{
        device_t child;
        int error;

        if (dev->state != DS_ATTACHED)
                return (EBUSY);

sys/dev/ichiic/ig4_pci.c

88 ↗

(On Diff #5014)

Removed it.

115 ↗

(On Diff #5014)

ok

sys/dev/ichiic/ig4_reg.h

19 ↗

(On Diff #5014)

I changed the license so it clear that I messed with this code. Please let me know if this is a bad idea.

[grembo]

Next iteration based on code review.

@jhb: Thank you for your detailed reviews.

[jhb]

In D2372#43414, @grembo wrote:

@jhb In terms of licensing, are there any best practices? Like if you port one feature from another BSD to FreeBSD (like a patch to the keyboard driver, maybe a few lines of code), will this affect the the license of the original file?

Generally small fixes do not warrant updating the license, but you should really ask core@ for firmer guidance on this.

When porting something completely new (like in this case), at which point should the porter be mentioned in the header - you clearly don't want to blame the original author for porting and/or system specific bugs. Something like:

* This code is derived from software contributed to The DragonFly Project
* by Matthew Dillon <[e-mail]> and was subsequently ported
* to FreeBSD by Firstname Lastname <[e-mail]>

It is fine to add something like this, but It should be clear that it is separate from the license/copyright block and not a condition of the license.

[jhb]

sys/dev/ichiic/ig4_iic.c

182 ↗

(On Diff #5165)

I think this is fine. Presumably if mtx_sleep() doesn't timeout but awakens early it means you are going to break out of the loop because there is something to do.

478 ↗

(On Diff #5165)

That's fine.

635 ↗

(On Diff #5165)

Ah, that's a good point about when detach is called to cleanup a failed attach. You can make the call to bus_generic_detach() conditional on 'device_is_attached(dev)'. This is similar to your previous generic_attached variable, but a bit simpler. I think the rest of detach looks fine. If you have multiple child devices you might consider deleting all of them on detach via 'device_delete_children()' rather than a single device_delete_child.

sys/dev/ichiic/ig4_reg.h

19 ↗

(On Diff #5165)

I think this is fine. It is in the license block, but I think it's clear it doesn't alter the license terms.

[grembo]

sys/dev/ichiic/ig4_iic.c

635 ↗

(On Diff #5165)

Ok.

There should be at most one smbus device per device, but it's still good to know.

sys/dev/ichiic/ig4_var.h

73 ↗

(On Diff #5165)

It's about serializing access to softc. I don't know if serializing access to the controller is actually necessary.

[grembo]

Make calling bus_generic_detach dependent on device_is_attached(dev)

[grembo]

sys/dev/ichiic/ig4_var.h

73 ↗

(On Diff #5165)

I mean in terms of mutexes.

@jhb I noticed that there are no man pages for bus_space_*, but not for bus_* (like bus_barrier, bus_copy_*). Do you know if anyone is working on these (a quick an lazy google search didn't give me any results)?

[grembo]

sys/dev/ichiic/ig4_var.h

73 ↗

(On Diff #5233)

Sorry for spamming you, this was supposed to read "there are man pages for bus_space_*". @wblock Maybe you know as well.

[grembo]

@jhb & @adrian: Do you think this is acceptable now?

[jhb]

sys/dev/ichiic/ig4_var.h

73 ↗

(On Diff #5233)

I believe @phk just didn't update the manpage when he added bus_*. It would be good to update the manpage though. :-/