aacraid: log DMA failures

[luporl]

Summary

Log an error message when bus_dmamap_load() fails to load the
buffer to DMA area and invokes the callback function with an
error.

This makes it much easier to figure out what is going wrong and why a read/write command failed.

It is hard to abort the I/O operation at aacraid DMA callback and propagate the error to upper layers, but at least we can log the error and help whoever is trying to debug an issue.

Test Plan

Tested on a Talos II machine with an aacraid controller.

[imp]

sys/dev/aacraid/aacraid.c

1302

Does this need a rate limiter?

[luporl]

Address review's comments

[luporl]

sys/dev/aacraid/aacraid.c

1302

Yes, it could generate a lot of messages.
Limiting it to 1 message in each 10 seconds (mostly because I prefer it to be displayed more often than not and this error end up compromising the system in a short time).

[luporl]

@imp I've added the rate limiter, do you think this revision is ok now?

[scottl]

I've put rate limiters into other drivers and have been unhappy with the results. Whether it's once a second, 10 seconds, 100 seconds, or whatever, an unattended system will wind up with thousands of messages in its logs that obfuscate more important messages. I think a better tactic is to print a message once, and maintain a sysctl with a counter.

[luporl]

Address reviewer's comments:

• log DMA load buffer error message only once for each device
• add a per device error counter, exported via sysctl (inspired by mpr code)

[luporl]

@scottl do you think this is ok now?

[emaste]

sys/dev/aacraid/aacraid.c

227–229

it looks like this was never used before

254

does it make sense to put this under dev.aacraid instead? I think the sysctl setup is simplified by doing so

[scottl]

sys/dev/aacraid/aacraid.c

254

Correct, it should be dev.aacraid.

[luporl]

Simplify sysctl setup

[luporl]

sys/dev/aacraid/aacraid.c

254

Ok, it's now always added to dev.aacraid and setup is skipped if for some reason sysctl_ctx or sysctl_tree is NULL.

[luporl]

sys/dev/aacraid/aacraid.c

227–229

It's really not used. Do you think it's better to remove it, or just ignore it?

[emaste]

sys/dev/aacraid/aacraid.c

227–229

I would delete it in a separate commit, either before or after this change.

[luporl]

Is this change good to go now?

sys/dev/aacraid/aacraid.c

227–229

Ok, I'll delete it after this change.

[markj]

sys/dev/aacraid/aacraid.c

1310

Isn't it possible that a bogus command will trigger controller firmware bugs and command timeouts? IMHO it is better to abort the command at this stage, though I'm not sure yet how to do that in this driver.

[luporl]

sys/dev/aacraid/aacraid.c

1310

Yes, I think it's possible. I agree that it would be better to abort the command here. That was my plan, until I realized this wasn't a simple task. I guess we would need to replicate part of what is being done in the interrupt handler, to clean up and wake up who is waiting on this command to complete. But I'm afraid I won't have cycles available to implement proper command abort.

In any case, we already allow bogus commands with current code and this change at least warns about DMA errors.