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.