ioat
AbandonedPublic
Actions

Authored by cem on Aug 22 2015, 1:25 AM.

Details

Reviewers

markj
jimharris
imp

Summary

Handle errors; use common error numbers instead of -1.

Add PCIIDs for newer QD/IOAT hardware (Haswell, Ivybridge, Broadwell).

Don't panic if a copy cannot be satisfied in one request, just fail it.

Use sysctls with CTL_*TUN instead of TUNABLE_FETCH in various places.

Not tested, just compiled it.

ioat(4): Drop LOGGING ifdef entirely

ioat(4): Style-cleanup tests / test code

Conditionalize /dev/ioat_test presense on hw.ioat.enable_ioat_test tunable.

But, compile it into the module unconditionally.

Update README to match reality.

Fix ioatcontrol Makefile to work with OBJDIR.

Test Plan

This is just the cleanups from jimharris's ioat branch to https://reviews.freebsd.org/D3456 .

Diff Detail

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 191
Build 191: arc lint + arc unit

Event Timeline

cem updated this revision to Diff 8120.Aug 22 2015, 1:25 AM

cem retitled this revision from to ioat(4): Just the cleanups / changes to users/jimarris/ioat.

cem updated this object.

cem edited the test plan for this revision. (Show Details)

cem added a reviewer: jimharris.

jimharris added inline comments.Aug 22 2015, 3:35 PM

sys/dev/ioat/ioat.c
85	I don't think this works as a sysctl. This value is needed when the driver loads, to determine whether or not to force legacy interrupt usage. By time the sysctl is available to force legacy interrupt usage, it will be too late.
187	I agree with imp@ that this should go back to including both device and vendor ID in the table.
243	I missed an explicit pci_enable_busmaster() call here (with matching pci_disable_busmaster() in detach).
412	Suggest moving the ioat_read_4 and ioat_log_message be moved to your new ioat3_selftest function.
446	This function needs style updates too.
sys/dev/ioat/ioat_sysctl.h
32 ↗	(On Diff #8120)	Maybe ioat_logger.h and ioat_sysctl.h contents should just be transferred to ioat.h, since there's not much here.
sys/dev/ioat/ioat_test.c
60	The same comment applies here as the force_legacy_interrupts SYSCTL above. I think this needs to be a tunable. (But then I would argue if someone does not want the /dev/ioat_test device enabled, they should just not load the test driver.)

cem added inline comments.Aug 22 2015, 3:52 PM

sys/dev/ioat/ioat.c
85	Sure. I will change it to RDTUN.
412	Sure. Will do.
sys/dev/ioat/ioat_sysctl.h
32 ↗	(On Diff #8120)	You mean ioat_internal.h? Then ioat_logger.c would have to include all of the headers to define the types that ioat_internal.h depends on. We could also just merge ioat_logger.c into ioat.c — not much there either.
sys/dev/ioat/ioat_test.c
60	It is a tunable — it's just readable as a sysctl (RDTUN). We could also split the /dev/ioat_test device out into a separate kld, but I figure it wasn't worth bothering. I don't imagine it adds much in the way of bloat to the size of the .ko. (I don't like editing the Makefile to enable/disable the test device.)

One more note - sys/conf/files.amd64 and sys/amd64/conf/NOTES should also be updated for ioat, so that it gets integrated into normal kernel builds.

You could add it to i386 too - the driver should work fine there too although I've never tested it. I think amd64 only is fine for now though.

sys/dev/ioat/ioat_sysctl.h
32 ↗	(On Diff #8120)	I agree on merging ioat_logger.c as well.
sys/dev/ioat/ioat_test.c
60	Sorry - I forgot oat_test was not a separate kld on my original branch. I think what you have here is fine. One alternative would be to change enable_ioat_test to a SYSCTL_PROC, so that the cdev can be created/deleted dynamically (and the sysctl could even be invoked within the ioatcontrol utility).

Will adjust files.amd64 and NOTES, thanks!

sys/dev/ioat/ioat_test.c
60	Hah, sure.

cem added reviewers: markj, imp.Aug 22 2015, 5:34 PM

cem mentioned this in D3456: Add ioat(4) driver and test tool.Aug 22 2015, 6:45 PM

Address CR feedback from D3456 and D3457.

Add ioat to sys/modules/Makefile, cond'l on amd64

This will stay here for historical interest, but it gets harder and harder to regenerate this diff because I'm touching files that have changed since user/jimharris/ioat branched from CURRENT. New changes will only land in D3456.

Revision Contents
Changeset List

Path

Size

sys/

amd64/

conf/

NOTES

1 line

conf/

files.amd64

2 lines

dev/

ioat/

10 lines

509 lines

102 lines

190 lines

11 lines

89 lines

modules/

ioat/

Makefile

14 lines

README

34 lines

tools/

ioat/

Makefile

7 lines

ioatcontrol.c

16 lines

Diff 8132

View Options

sys/amd64/conf/NOTES

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines	case 0: /* item name */
break;		break;
}		}
remain = byte2;		remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) {		if (vpd_nextbyte(&vrs, &byte2)) {
state = -2;		state = -2;
break;		break;
}		}
remain \|= byte2 << 8;		remain \|= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted Not Done Inline Actions I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f4, then. kib:* I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted Not Done Inline Actions Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow. I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like: if (vrs.off + remain - vrs.bytesinval > 0x8000) { pci_printf(cfg, "VPD data overflow, remain %#x\n", remain); state = -1; break; } The missing break in the old code isn't great either as you could potentially malloc() a really large value. jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f;		name = byte & 0x7f;
} else {		} else {
remain = byte & 0x7;		remain = byte & 0x7;
name = (byte >> 3) & 0xf;		name = (byte >> 3) & 0xf;
		}
		if (vrs.off + remain - vrs.bytesinval > 0x8000) {
		pci_printf(cfg,
		"VPD data overflow, remain %#x\n", remain);
		state = -1;
		break;
}		}
switch (name) {		switch (name) {
case 0x2: /* String */		case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1,		cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK);		M_DEVBUF, M_WAITOK);
i = 0;		i = 0;
state = 1;		state = 1;
break;		break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines

View Options

sys/conf/files.amd64

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines	case 0: /* item name */
break;		break;
}		}
remain = byte2;		remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) {		if (vpd_nextbyte(&vrs, &byte2)) {
state = -2;		state = -2;
break;		break;
}		}
remain \|= byte2 << 8;		remain \|= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted Not Done Inline Actions I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f4, then. kib:* I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted Not Done Inline Actions Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow. I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like: if (vrs.off + remain - vrs.bytesinval > 0x8000) { pci_printf(cfg, "VPD data overflow, remain %#x\n", remain); state = -1; break; } The missing break in the old code isn't great either as you could potentially malloc() a really large value. jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f;		name = byte & 0x7f;
} else {		} else {
remain = byte & 0x7;		remain = byte & 0x7;
name = (byte >> 3) & 0xf;		name = (byte >> 3) & 0xf;
		}
		if (vrs.off + remain - vrs.bytesinval > 0x8000) {
		pci_printf(cfg,
		"VPD data overflow, remain %#x\n", remain);
		state = -1;
		break;
}		}
switch (name) {		switch (name) {
case 0x2: /* String */		case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1,		cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK);		M_DEVBUF, M_WAITOK);
i = 0;		i = 0;
state = 1;		state = 1;
break;		break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines

View Options

sys/dev/ioat/ioat.h

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines	case 0: /* item name */
break;		break;
}		}
remain = byte2;		remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) {		if (vpd_nextbyte(&vrs, &byte2)) {
state = -2;		state = -2;
break;		break;
}		}
remain \|= byte2 << 8;		remain \|= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted Not Done Inline Actions I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f4, then. kib:* I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted Not Done Inline Actions Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow. I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like: if (vrs.off + remain - vrs.bytesinval > 0x8000) { pci_printf(cfg, "VPD data overflow, remain %#x\n", remain); state = -1; break; } The missing break in the old code isn't great either as you could potentially malloc() a really large value. jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f;		name = byte & 0x7f;
} else {		} else {
remain = byte & 0x7;		remain = byte & 0x7;
name = (byte >> 3) & 0xf;		name = (byte >> 3) & 0xf;
		}
		if (vrs.off + remain - vrs.bytesinval > 0x8000) {
		pci_printf(cfg,
		"VPD data overflow, remain %#x\n", remain);
		state = -1;
		break;
}		}
switch (name) {		switch (name) {
case 0x2: /* String */		case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1,		cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK);		M_DEVBUF, M_WAITOK);
i = 0;		i = 0;
state = 1;		state = 1;
break;		break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines

View Options

sys/dev/ioat/ioat.c

Show First 20 Lines • Show All 1,096 Lines • ▼ Show 20 Lines	case 0: /* item name */
break;		break;
}		}
remain = byte2;		remain = byte2;
if (vpd_nextbyte(&vrs, &byte2)) {		if (vpd_nextbyte(&vrs, &byte2)) {
state = -2;		state = -2;
break;		break;
}		}
remain \|= byte2 << 8;		remain \|= byte2 << 8;
if (remain > (0x7f*4 - vrs.off)) {
kibUnsubmitted Not Done Inline Actions I suspect this was an attempt to handle the F bit from the vpd cap address register. It should be 0x7fff instead of 0x7f4, then. kib:* I suspect this was an attempt to handle the F bit from the vpd cap address register. It should…
jhbUnsubmitted Not Done Inline Actions Or to also avoid inaddressible locations. However, the "short" case would also need the same check since you could have a "short" tag at the end of the VPD area that could potentially overflow. I would perhaps move this down to the bottom of this case before the 'switch (name)' (line 1115 in the old code in this review) so it tests both cases and code it something like: if (vrs.off + remain - vrs.bytesinval > 0x8000) { pci_printf(cfg, "VPD data overflow, remain %#x\n", remain); state = -1; break; } The missing break in the old code isn't great either as you could potentially malloc() a really large value. jhb: Or to also avoid inaddressible locations. However, the "short" case would also need the same…
state = -1;
pci_printf(cfg,
"invalid VPD data, remain %#x\n",
remain);
}
name = byte & 0x7f;		name = byte & 0x7f;
} else {		} else {
remain = byte & 0x7;		remain = byte & 0x7;
name = (byte >> 3) & 0xf;		name = (byte >> 3) & 0xf;
		}
		if (vrs.off + remain - vrs.bytesinval > 0x8000) {
		pci_printf(cfg,
		"VPD data overflow, remain %#x\n", remain);
		state = -1;
		break;
}		}
switch (name) {		switch (name) {
case 0x2: /* String */		case 0x2: /* String */
cfg->vpd.vpd_ident = malloc(remain + 1,		cfg->vpd.vpd_ident = malloc(remain + 1,
M_DEVBUF, M_WAITOK);		M_DEVBUF, M_WAITOK);
i = 0;		i = 0;
state = 1;		state = 1;
break;		break;
▲ Show 20 Lines • Show All 5,502 Lines • Show Last 20 Lines