Created attachment 244746 [details] patch files for different architectures While debugging jh7110 board's MMC driver I found out that the problem had been a faulty algorithm in DMA code. _bus_dmamap_count_pages() in sys/riscv/riscv/busdma_bounce.c is called to count a number of new bounce pages needed in order to complete the DMA transfer (this happens at least when physical addresses used for data segments go out of DMA address range). However, unlike a function bounce_bus_dmapmap_load_buffer() which calls it, _bus_dmamap_count_pages() does not take into account that segment size may be limited by dmat->common.maxsegsz which is set when creating bus_dma_tag_t in the driver code. This may lead to a wrong number of pages counted and thus some needed bounce pages will not be created. In my case the consequence was that some addresses outside of device's 32-bit DMA range were assigned to descriptors (IO data structures read by DMA controller) which nevertheless can only contain first 32 bits of those addresses. This causes the whole booting process to halt. After a failed transfer MMC driver code stays waiting for a next interrupt that never arrives. I found that the _bus_dmamap_count_pages() is similarly deficient in other architectures though it's possible this bug hasn't surfaced previously. My actual case is related to charcteristics of Designware's MMC devices (see comments on file dwmmc.c, line 140) which imply maximum size for data segments. I don't see any reason why this issue couldn't affect other architectures as well if circumstances are right, so I've supplied patches for each architecture in an attachment file. ARM and powerpc have the _bus_dmamap_count_pages() function in file busdma_machdep.c while other archs (including ARM64) have it in busdma_bounce.c
Mitchell, is this something you can have a look at?
Hi Jari, Thanks for your efforts in identifying this. I worked on this problem a while ago when I was hacking on the JH7100. Back then I created a revision that fixed the problem on riscv, see https://reviews.freebsd.org/D34118. It contains my own description of the problem and why it has been unlikely to manifest in most other cases. Our changes appear to be the same, differing only stylistically. I would be happy to help land either version of the fix. Do you have an account on reviews.freebsd.org? That would be the best place to submit these changes for wider review and eventual inclusion in the tree. Bugzilla makes code contributions difficult.
(In reply to Mitchell Horne from comment #2) Okay, I must have seen that reviews site previously but it hasn't entered into my consciousness for a long time. It's not mentioned on "Contributing to FreeBSD" page or "FreeBSD Search Services" page. It seems to be not mentioned even on FreeBSD Developers' Handbook. So I may have got a false impression that these sources of information have all the relevant sites listed. So yes, I will keep reviews site on my mind now for the future.
^Triage: promote version number to a currently supported value.
A commit in branch main references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=b134c10d658c3b350e04aa8dbd2628e955ddcce0 commit b134c10d658c3b350e04aa8dbd2628e955ddcce0 Author: Mitchell Horne <mhorne@FreeBSD.org> AuthorDate: 2021-05-25 21:04:56 +0000 Commit: Mitchell Horne <mhorne@FreeBSD.org> CommitDate: 2024-02-16 18:38:48 +0000 busdma: fix page miscount for small segment sizes For small segments (< PAGE_SIZE) there is a mismatch between how required bounce pages are counted in _bus_dmamap_count_pages() and bounce_bus_dmamap_load_buffer(). This problem has been observed on the RISC-V VisionFive v2 SoC (and earlier revisions of the hardware) which has memory physically addressed above 4GB. This requires some bouncing for the dwmmc driver, which has has a maximum segment size of 2048 bytes. When attempting to load a page-aligned 4-page buffer that requires bouncing, we can end up counting 4 bounce pages for an 8-segment transfer. These pages will be incorrectly configured to cover only the first half of the transfer (4 x 2048 bytes). Fix the immediate issue by adding the maxsegsz check to _bus_dmamap_count_pages(); this is what _bus_dmamap_count_phys() does already. The result is that we will inefficiently allocate a separate bounce page for each segment (8 pages for the example above), but the transfer will proceed in its entirety. The more complete fix is to address the shortcomings in how small segments are assigned to bounce pages, so that we opportunistically batch multiple segments to a page whenever they fit (e.g. two 2048 bytes segments per 4096 page). This will be addressed more holistically in the future. For now this change will prevent the (silent) incomplete transfers that have been observed. PR: 273694 Reported by: Jari Sihvola <jsihv@gmx.com> Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34118 sys/arm/arm/busdma_machdep.c | 11 ++++++----- sys/arm64/arm64/busdma_bounce.c | 1 + sys/powerpc/powerpc/busdma_machdep.c | 1 + sys/riscv/riscv/busdma_bounce.c | 1 + sys/x86/x86/busdma_bounce.c | 1 + 5 files changed, 10 insertions(+), 5 deletions(-)
A commit in branch stable/14 references this bug: URL: https://cgit.FreeBSD.org/src/commit/?id=0a40193a6e0fdf73b618aec3bccfb05bd1d0ddf0 commit 0a40193a6e0fdf73b618aec3bccfb05bd1d0ddf0 Author: Mitchell Horne <mhorne@FreeBSD.org> AuthorDate: 2021-05-25 21:04:56 +0000 Commit: Mitchell Horne <mhorne@FreeBSD.org> CommitDate: 2024-04-18 18:09:02 +0000 busdma: fix page miscount for small segment sizes For small segments (< PAGE_SIZE) there is a mismatch between how required bounce pages are counted in _bus_dmamap_count_pages() and bounce_bus_dmamap_load_buffer(). This problem has been observed on the RISC-V VisionFive v2 SoC (and earlier revisions of the hardware) which has memory physically addressed above 4GB. This requires some bouncing for the dwmmc driver, which has has a maximum segment size of 2048 bytes. When attempting to load a page-aligned 4-page buffer that requires bouncing, we can end up counting 4 bounce pages for an 8-segment transfer. These pages will be incorrectly configured to cover only the first half of the transfer (4 x 2048 bytes). Fix the immediate issue by adding the maxsegsz check to _bus_dmamap_count_pages(); this is what _bus_dmamap_count_phys() does already. The result is that we will inefficiently allocate a separate bounce page for each segment (8 pages for the example above), but the transfer will proceed in its entirety. The more complete fix is to address the shortcomings in how small segments are assigned to bounce pages, so that we opportunistically batch multiple segments to a page whenever they fit (e.g. two 2048 bytes segments per 4096 page). This will be addressed more holistically in the future. For now this change will prevent the (silent) incomplete transfers that have been observed. PR: 273694 Reported by: Jari Sihvola <jsihv@gmx.com> Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D34118 (cherry picked from commit b134c10d658c3b350e04aa8dbd2628e955ddcce0) sys/arm/arm/busdma_machdep.c | 11 ++++++----- sys/arm64/arm64/busdma_bounce.c | 1 + sys/powerpc/powerpc/busdma_machdep.c | 1 + sys/riscv/riscv/busdma_bounce.c | 1 + sys/x86/x86/busdma_bounce.c | 1 + 5 files changed, 10 insertions(+), 5 deletions(-)