The brace should be on if() line.

Why 128 and not 1024? There were some changes to smbus recently to allow 1024, and smbus is just a fancy iic bus.

priv can't be NULL here. clang will warn about this someday.
Coverity warns about it today.

How do you know nothing can or will sleep on this lock?

usrbufs can't be NULL.

buf can't be NULL.

m->buf can't be NULL here.

why wakeup_one and not just wakeup?

Some more explanation of why this is necessary is needed. It would be good to enshrine this requirement in the IICBUS_CALLBACK wrapper somehow, but since that's generated by the newbus stuff, perhaps that would be hard since the only fallback is to testing this in each bus' callback routine.

Same question as above.

Why use up 1K in the softc when it isn't necessary to do so?
This size is no longer a hard upper limit on the transfer length. If more than 128 bytes need to be moved, iicuio_move will call the necessary I/O function iteratively. I2C is slow enough that the smaller chunk size shouldn't matter for performance.

Oops, I didn't realize M_WAITOK mallocs aren't *ever* allowed to fail.
But, that brings up another question: should we try to honor O_NONBLOCK by passing M_NOWAIT in that case?

What do you mean by this? This lock is part of cdevpriv, so it can only be used in cdev calls. It's being destroyed in the cdevpriv dtor, which isn't executed until any outstanding cdev calls are purged. That would include anything that might contend or sx_sleep() on it.

Why schedule every waiting thread when only (at most) one of them will be able to take ownership of the bus?

You can drop these casts while you are here (I realize they are in the original code) since C allows implicit conversion from void *.

Please format block comments as:

/*
 * This is a block comment.
 */

Should you loop on a short write or perhaps convert that to an explicit error? (That is check for num_bytes == transferred_bytes after this completes.)

Please put the 'else' on the line as the '}'

I don't know if other drivers interpret O_NONBLOCK to mean M_NOWAIT for malloc(). That might be fine, but it strikes me as unusual.

You could collapse:

error = ENOMEM;
return (error);

to:

return (ENOMEM);

The latter seems more common in the kernel. error variables are typically used either to use goto to jump to common cleanup handling, or to save the return value of a function before dropping a lock.

Please move this '{' up to the previous line. (You follow this convention already in most other places)

"release release" ? :)

For non-boolean checks, testing against zero is preferred. For example, priv->started is a boolean, so '!priv->started' is fine, but error can have a range of values, so please use 'if (error == 0)' instead. Note that 'if (error)' is a commonly (ab)used violation of this rule (should be 'if (error != 0)', but I believe that most code is better about using '== 0' to check for lack of error.

Hmm, good point. I already allow short reads by honoring transferred_bytes in uiomove. I think I'd like to go for maximum flexibility and loop on short writes. I am operating on the assumption that the underlying bus code won't be stupid enough to, for example, perpetually return 0 bytes read/written without also returning some error.

I think in most, if not all, cases a short read or write will correspond with an iic bus error of some kind.

I dug around the mailing lists a little, and the consensus seems to be that O_NONBLOCK shouldn't imply M_NOWAIT, because malloc isn't expected to block for any length of time, and if it does you probably have bigger problems. I'm fine with just deleting that code & assuming the mallocs won't fail.

Oops, that was leftover from a goto I realized I didn't need.

Hey, at least I'm only double-releasing in a comment :)