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

Lint

Lint Skipped

Unit

Tests Skipped

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.