Page MenuHomeFreeBSD

WIP/RFC: another busdma fix for small bounce transfers
Needs ReviewPublic

Authored by mhorne on Wed, Nov 27, 5:23 PM.
Tags
None
Referenced Files
F106166731: D47807.diff
Thu, Dec 26, 11:55 AM
Unknown Object (File)
Fri, Dec 13, 2:11 PM
Unknown Object (File)
Sun, Dec 1, 11:24 AM

Details

Reviewers
jhb
markj
mmel
Summary

More fallout from a77e1f0f81df.

When the tag has an alignment requirement, we should clamp sgsize to be
no larger than the remaining buflen. Otherwise, we will transfer more
than is required, and this can manifest as data corruption.

The issue is observed on powerpc as noted in the pull request:
https://github.com/freebsd/freebsd-src/pull/1415

I also observe the issue locally on riscv hardware, with an 8-byte
transfer having 64-byte alignment.

Try as I might, I cannot articulate the reason for rounding sgsize up to
the tag's alignment, at least in the current code. In fact, it is not
done on arm or arm64 (the latter removed by mmel@ in f635cef2a420).

I speculate that the original purpose of this is now achieved by the line:

sgsize = MIN(buflen, PAGE_SIZE - (curaddr & PAGE_MASK));

I find this code very difficult to reason about, nor is it easy to
experiment with. The fix I have here should be the safest fix, but it
may not be the most correct. This code is fragile and I would benefit
from other knowledgable eyes on it.

Thoughts? What is the correct action to take here?

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 60841
Build 57725: arc lint + arc unit

Event Timeline

I'm sorry for the delayed follow-up. I'm trying to understand the purpose of the roundup. It appears to have been introduced by commit daf6545e6158f; the implication seems to be that some drivers depend on the property that "the relative alignment of two consecutive bytes in the I/O stream have a difference of 1 even if they are not physically contiguous."

I don't really understand why that property is important, but assuming so, I think your patch is probably the right thing to do.