Make the code consistent with the comments
ClosedPublic
Actions

Authored by kjopek_gmail.com on May 21 2021, 3:52 PM.

Details

Reviewers

mmel
manu
avg

Commits

rGa5e76683b2de: dwmmc: Calculate the maximum transaction length correctly.
rGdfb736022285: dwmmc: Calculate the maximum transaction length correctly.

Summary

We need to reserve two segments. Three sectors is just 3/4 of the segment.

This fixes panic with fsck on dirty filesystem.
Actually I also tested version with single segment reserved and it has
shown to be stable. However, I do believe there may be the cases
when the second segment is also required.

Test Plan

Tested on RockPro64.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kjopek_gmail.com requested review of this revision.May 21 2021, 3:52 PM

kjopek_gmail.com created this revision.

Remove redundant space.

kjopek_gmail.com mentioned this in D28274: Solve problems with rk3399 dwmmc failing to update clock..May 26 2021, 5:19 AM

emaste added a subscriber: emaste.May 26 2021, 2:04 PM

Sorry for long delay. Can you, please, also test "(IDMAC_MAX_SIZE * (IDMAC_DESC_SEGS - 1)) / MMC_SECTOR_SIZE"? Minus one should be sufficient and it passed all my tests.

I performed again test with

(IDMAC_MAX_SIZE * (IDMAC_DESC_SEGS - 1)) / MMC_SECTOR_SIZE

and it worked perfectly:

Starting file system checks:
** SU+J Recovering /dev/mmcsd0s5
** Reading 4194304 byte journal from inode 4.
** Building recovery table.
** Resolving unreferenced inode list.
** Processing journal entries.
** 2 journal records in 512 bytes for 12.50% utilization
** Freed 0 inodes (0 dirs) 0 blocks, and 0 frags.

***** FILE SYSTEM MARKED CLEAN *****
/dev/mmcsd0s6: 8058 files, 18530 used, 3374021 free (37 frags, 421748 blocks, 0.0% fragmentation)

decke added a subscriber: decke.Jul 23 2021, 7:29 PM

@mmel I think we need to do something with this review. We have at least one bug report: https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=256822 (similar to what I observe).

Friendly ping

Please commit this.
If not too much trouble please also update dwmmc_get_tran_settings (used with MMCCAM) to use the same formula.
Thank you very much!

This revision is now accepted and ready to land.Sep 22 2021, 7:30 AM

I'm not against this being commited but I'd like to understand why it's needed.
IIRC @mmel said this has to do with bounce buffer (and the comment seems to agree, is there somewhere I can read about this ?

@manu: The handling of bouncing buffers itself is correct in this case. I did not observe any issues in sys/arm64/arm64/busdma_bounce.c. However this is needed, because currently it returns invalid value: instead of making reservation for 2 segments it makes reservation for 3/4 segment which in turns leads to crashes in bouncing buffers code. Comment above the line is valid - for sure we need to reserve two segments (it works for one reserved segment too, but I am not sure about all edge cases), but we need to correctly reserve 2 segments, not 3 sectors.

@avg: Surely, I can provide small patch for it this weekend.