aacraid: log DMA failures
Needs ReviewPublic
Actions

Authored by luporl on Jun 17 2021, 12:50 PM.

Details

Reviewers

jhibbits
scottl
emaste
imp

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.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 41866
Build 38754: arc lint + arc unit

Event Timeline

luporl created this revision.Jun 17 2021, 12:50 PM

Herald added subscribers: Contributor Reviews (src), imp. · View Herald TranscriptJun 17 2021, 12:50 PM

luporl requested review of this revision.Jun 17 2021, 12:50 PM

Harbormaster completed remote builds in B39947: Diff 91003.Jun 17 2021, 12:50 PM

luporl mentioned this in D30798: aacraid: reduce max I/O size to avoid DMA issues.Jun 17 2021, 1:13 PM

luporl added reviewers: jhibbits, scottl, emaste.

imp added inline comments.Jun 17 2021, 4:15 PM

sys/dev/aacraid/aacraid.c
1320	Does this need a rate limiter?

Address review's comments

Harbormaster completed remote builds in B40021: Diff 91175.Jun 21 2021, 5:48 PM

luporl marked an inline comment as done.Jun 21 2021, 5:57 PM

luporl added inline comments.

sys/dev/aacraid/aacraid.c
1320	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 marked an inline comment as done.Jun 21 2021, 5:58 PM

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

luporl added a reviewer: imp.Jul 23 2021, 11:47 AM

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.

This revision now requires changes to proceed.Jul 23 2021, 2:42 PM

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)

Harbormaster completed remote builds in B41291: Diff 94432.Aug 31 2021, 12:08 PM

@scottl do you think this is ok now?

emaste added inline comments.Sep 30 2021, 12:47 PM

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 added inline comments.Sep 30 2021, 5:35 PM

sys/dev/aacraid/aacraid.c
254	Correct, it should be dev.aacraid.

Simplify sysctl setup

Harbormaster completed remote builds in B41866: Diff 96021.Sep 30 2021, 5:55 PM

luporl marked 2 inline comments as done.Sep 30 2021, 5:58 PM

luporl added inline comments.

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 marked an inline comment as done.Sep 30 2021, 6:06 PM

luporl added inline comments.

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 added inline comments.Sep 30 2021, 6:20 PM

sys/dev/aacraid/aacraid.c
227–229	I would delete it in a separate commit, either before or after this change.

Is this change good to go now?

sys/dev/aacraid/aacraid.c
227–229	Ok, I'll delete it after this change.

luporl marked an inline comment as done.Dec 1 2021, 4:36 PM

markj added a subscriber: markj.Aug 4 2022, 6:42 PM

markj added inline comments.

sys/dev/aacraid/aacraid.c
1328	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 added inline comments.Aug 5 2022, 12:08 PM

sys/dev/aacraid/aacraid.c
1328	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.