Page MenuHomeFreeBSD

Improve loading of multipage aligned buffers.
Needs ReviewPublic

Authored by mmel on Oct 11 2020, 9:00 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Dec 17, 6:36 AM
Unknown Object (File)
Oct 18 2024, 7:14 AM
Unknown Object (File)
Oct 8 2024, 8:29 AM
Unknown Object (File)
Oct 5 2024, 6:38 AM
Unknown Object (File)
Oct 2 2024, 4:10 PM
Unknown Object (File)
Oct 2 2024, 3:45 AM
Unknown Object (File)
Oct 1 2024, 6:06 AM
Unknown Object (File)
Sep 30 2024, 2:17 PM

Details

Summary

The multipage alignment requirements is incompatible with many aspects
of actual busdma code. Multi-page alignment requests are incompatible
with many aspects of current busdma code. Mainly with partially bounced
buffer segments and per-page loop in bus_dmamap_load_buffer(). Because
proper implementation would be a major restructuring of the code, add
the fix only for already known uses and do KASSERT for all other cases.

For this reason, bus_dmamap_load_buffer() should take the memory allocated
by bus_dmamap_alloc() as one segment bypassing per page segmentation. We can
do this because it is guaranteed that the memory is physically continuous.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

mmel requested review of this revision.Oct 11 2020, 9:00 AM
bz added inline comments.
sys/arm64/arm64/busdma_bounce.c
831 ↗(On Diff #78095)

alignment

835 ↗(On Diff #78095)

than

I tend to print the questionable value usually in KASSERTs as to see when it panics, what we got so we might be able to track down why we got there.

890 ↗(On Diff #78095)

than instead of that again

946 ↗(On Diff #78095)

and than again

I booted this on my emag:
CPU 1: APM eMAG 8180 r3p2 affinity: 0 1
and it seems to fix the issues that I had with cd sometimes detecting, other times not, as well as some weird reliability stuff relating to /etc/rc running at boot reliably.
I also had a weird issue where I'd type 'sudo reboot' and it would echo back 'sudo rebeoot' sometimes.
Not super extensive, but it's definitely not worse and preliminary indications are that it's better (though I'll admit to a smaller sample size with the patch than testing to produce the litany of horrors I've recounted).

This seems to fix issues with my USB connected optical drives not showing up as cd (only pass device). I've done 10 reboots, replugging it 10 times and they show up correctly each time.

Tested on:
FreeBSD 13.0-CURRENT r366770M
Hardware:
Pine64 RockPro64
USB adapter(s):
2x ASMedia Technology 1153e-based (external PSU connected) - Firmware: 141126_A1_EE_82, USB 3.0 compatible - Used for reboot/replug testing
JMicron-based PATA/SATA-adapter (PATA, external PSU connected), USB 2.0 compatible (don't know exact chipset)

This MR solved problem with eMMC cards on RK3399. Previously card slot was not initialized correctly due to some DMA mapping issues.

Tested on Armada 8040 DB - this patch fixes SDHCI DMA allocation issue.

This revision was not accepted when it landed; it landed in state Needs Review.Nov 2 2020, 8:26 AM
This revision was automatically updated to reflect the committed changes.
bz added inline comments.
head/sys/arm64/arm64/busdma_bounce.c
864

In the first if() case above we change sgsize which can become larger then buflen; in theory even the MIN() call at the start of the while loop can do that already.

I noticed this whit a call which started as "XXX-BZ buflen 0xe curaddr 0x1f382000 buf 0x1f382000 sgsize 0xe"

I am not entirely sure yet what is the proper way to deal with this "underflow" in `buflen -= sgsize;`; I am tempted to say

if (sgsize > buflen)
   buflen = 0;
else
   buflen -= sgsize;

but I am not (yet) sure what other implications that might have--as in: is there a valid case where we can end up here with buflen going negative?

In my case the return below would return EFBIG depsite it not being an error case otherwise.

The other concern I have and haven't checked yet, is if sgsize > buflen might lead to all other kinds of len problems with the segment downstream (and that would probably lead to a data inconsistency problem rather than an error/aborted transaction)?

head/sys/arm64/arm64/busdma_bounce.c
864

Can you, please, try this?

the three lines with sgsize should be replaced with

if (sgsize > (PAGE_SIZE - (curaddr & PAGE_MASK)) {
      sgsize = PAGE_SIZE - (curaddr & PAGE_MASK);
      sgsize =roundup2(sgsize, dmat->common.alignment));
      sgsize = MIN(sgsize, dmat->common.maxsegsz);
}

Untested, uncompiled.

head/sys/arm64/arm64/busdma_bounce.c
864

Fixing the () to make it compile this seems to be passing at quick glance and from reading compared to the old code seems to make sense.

Thanks a lot for the very quick reply.