I think CTLTYPE_STRING should be CTLTYPE_INT here.

Yeah, that seems right to me. (Also A -> I as well.) I'll write some manual pages and adjust the patch, thanks!

share/man/man4/Makefile needs to be updated to include this man page. Suggest only installing it on amd64.

I would not add ioatcontrol.8 to share/man/man8/Makefile, since ioatcontrol is not part of the standard build.

Since you added ioat to sys/modules/Makefile, ioat.ko will be built unless MODULES_OVERRIDE is set. If a user is setting MODULES_OVERRIDE, then they need to modify MODULES_OVERRIDE for _any_ kernel module that they want to build, which implies that every single driver man page should mention MODULES_OVERRIDE (and none of them do). For consistency, I would just remove the reference to it.

If you disagree and want to keep it, it should also be noted that MODULES_OVERRIDE can be set in the kernel configuration file with "makeoptions", as a (very handy) alternative to make.conf.

"on a some"

Could you qualify "newer"?

Trigger an interrupt under what condition?

What does "grab" mean? Does it give the caller exclusive access to the channel?

What's a submission lock?

Right. Will do.

Yes, I noticed other tools/tools/*/*.8 exist but aren't hooked up to the build (e.g. cxgbetool). I'll leave that be.

Oh, ok. Will drop that part then.

Sandybridge+. I will add a blurb to the page.

Successful completion of the null operation with DMA_INT_EN flagged. I'll note the flag.

No; I was trying to avoid 'acquire' because that has some baggage too.

Really the caller just uses ioat_get_dmaengine() to look up the bus_dmaengine_t for a given channel index, which is just a number. There is no refcount or exclusive access semantics here. I'll reword.

Exclusive access to submit operations on the given channel / DMA engine.

Tylersburg and Jasper Forest are pre-Sandy Bridge. (On Tylersburg, it was actually in the chipset, not the CPU.) Also, these are only on Xeon-based CPUs..

Maybe just simplify this to "on some Intel server platforms" and avoid the extra complexity?

Can do.

Why not create a separate driver for this?

Why? A link to an Intel Programmer's Guide would be nice

This can fail

This can fail

retry == FALSE

Bad style (not explicitly checking types). Should this be checking 0 or NULL or false or FALSE or ...?

This can fail

This can also fail

This can fail

Why? A link to an Intel programmer's guide would be helpful

Why 8? At the very least this deserves a well-named #define.

I don't like this API because it defeats the purpose of the logging infrastructure in FreeBSD.

• %d for times :(..? why not %ju with appropriate (uintmax_t) casting?

tv_sec and tv_usec are suseconds_t, which is typedef'ed to long, so at the very least these shouldn't be truncated by casting them to (int).

• Also, why do you need to get the time in the logs? syslog does a pretty darn good job at grabbing the time right now... IIRC grabbing the time (even from the kernel) was expensive, and this defeats the "X seen N times" message squelching from syslogd, which means that it will spam /var/log/messages, etc.

Do any of these structures need to be __packed?

"miscompare" sounds awkward

Will tx == NULL always be the result of an ENOMEM condition?

Can't these be omitted?

What if make_dev fails? It would be helpful to log the failure, instead of silently failing...

This does not seem MPSAFE (what if 2 things come in at the same time and try to create the cdev)?

Usually stuff bitrots in tools/ . It would be a good idea to put it in usr.sbin, then conditionally build it on amd64.

SRCS= ${PROG}.c is implied

What if I pass in -1?

Again, what if I pass in -1?

It would be nice to err(3) on failure... especially because invalid data might cause t.status to blow up at the end if it's bogus.

It was discussed earlier. The summary is it seems like an unreasonable amount of extra work for limited benefit.

No it can't?

void
mtx_lock(struct mtx *mutex);

Again, nope.

void
mtx_unlock(struct mtx *mutex);

Again, nope.

retry is a boolean_t. Using boolean NOT on a boolean is perfectly reasonable.

Good catch.

No, it can't.

The callout_reset() and callout_schedule() function families return non-
zero if the callout was pending before the new function invocation was
scheduled.

I would be totally okay with a patch reworking the logging handling. This revision has sailed, though.

tv_sec and tv_usec are suseconds_t, which is typedef'ed to long, so at the very least these shouldn't be truncated by casting them to (int).

Fortunately, there are only 1 million microseconds in a second, which is fully representable in int, so it's perfectly fine.

It doesn't look like they will have any padding bytes without the keyword. So, I guess not.

Yes. The (test) routine is defined about 50 lines above this comment and it's pretty clear that NULL only results from memory allocation failures.

No. The devfs code invokes d_open() unconditionally. I looked.

It cannot fail. The kernel panics if it fails.

That's why it's not marked MPSAFE. sysctl will take Giant before invoking this function.

Bitrot is acceptable. This does not belong in usr.sbin.

channel_index is unsigned.

Also unsigned.

As written, the ioctl cannot fail.

The compiler cannot "blow up" t.status because it cannot know that it is not modified by ioctl(2).

Annoyingly, the C11 thread lib defines a mtx_lock(3) which can indeed fail.