fix(sdmmc): correct miscalculated sector size (IDFGH-12789)

[huming2207]

Previous implementation will restrict the SD card sector size to be no more than 512 bytes. However 512 bytes is the minimum size for some old generic SD cards, and some new ones may come with 1024 or 4096 bytes per sector. We need to support those as well.

This (partially) fixes: https://github.com/espressif/esp-idf/issues/13749#issuecomment-2101695716

[github-actions[bot]]

Warnings

:warning:

**Some issues found for the commit messages in this PR:** - the commit message `"fix(sdmmc): correct miscalculated sector size"`: - body's lines must not be longer than 100 characters *** **Please fix these commit messages** - here are some basic tips: - follow [Conventional Commits style](https://www.conventionalcommits.org/en/v1.0.0/) - correct format of commit message should be: `(): `, for example `fix(esp32): Fixed startup timeout issue` - allowed types are: `change,ci,docs,feat,fix,refactor,remove,revert,test` - sufficiently descriptive message summary should be between 20 to 72 characters and start with upper case letter - avoid Jira references in commit messages (unavailable/irrelevant for our customers) `TIP:` Install pre-commit hooks and run this check when committing (uses the [Conventional Precommit Linter](https://github.com/espressif/conventional-precommit-linter)).

👋 Hello huming2207, we appreciate your contribution to this project!

---

📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more.

---

🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project.

---

Click to see more instructions ...

This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.

DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.

Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Resolve all warnings (⚠️ ) before requesting a review from human reviewers - they will appreciate it.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.

Review and merge process you can expect ...

We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.

This GitHub project is public mirror of our internal git repository

1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved, we synchronize it into our internal git repository.
4. In the internal git repository we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
5. If the change is approved and passes the tests it is merged into the default branch.
5. On next sync from the internal git repository merged change will appear in this public GitHub repository.

Generated by :no_entry_sign: dangerJS against 9a5f583a05e9bbedbdad1813133f7117841eca1a

[igrr]

@huming2207 thanks for the PR! The fix looks correct to me, we'll merge it soon.

However 512 bytes is the minimum size for some old generic SD cards, and some new ones may come with 1024 or 4096 bytes per sector.

Could you please mention the source for this statement? It doesn't seem to match SD specification. In SD v1 standard, the card provided read_bl_len and write_bl_len fields in the CSD register. These indicated maximum supported read and write block sizes, respectively. The actual block size would default to 512 bytes and could be set using CMD16, up to the limit specified in the CSD.

In SD v2 standard, these fields in CSD are no longer variable. E.g.,

READ_BL_LEN This field is fixed to 9h, which indicates READ_BL_LEN=512 Byte.

The description of CMD16 says the same:

SET_BLOCKLEN In case of SDSC Card, block length is set by this command. In case of SDHC and SDXC Cards, block length of the memory access commands are fixed to 512 bytes.

[huming2207]

Hi @igrr

Could you please mention the source for this statement? It doesn't seem to match SD specification.

Sorry that was straight out from my memory, I've double checked and I think I have misunderstood the standard and I thought all SD card sector size are fixed to whatever it was provided in the READ_BL_LEN in CSD. I should have read the spec first! 😅

Meanwhile indeed on some SD cards, the READ_BL_LEN can be 9 or 10 which means 1<<9 or 1<<10 and it means 512 or 1024 bytes for the maximum read block size. For example this 8Gbit SD NAND chip's READ_BL_LEN is 1024 bytes:

Based on the spec, I guess the original implemetation (which fixed to 512) should still works, but it may affect the I/O performance?

Regards, JAckson

[igrr]

Based on the spec, I guess the original implemetation (which fixed to 512) should still works, but it may affect the I/O performance?

I think I have this chip somewhere, I'll give it a try. I guess only the total size of the transfer should matter for performance, not the the block size.

The current implementation effectively always sets card->sector_size to 512, regardless of read_blk_len. https://github.com/espressif/esp-idf/blob/d4cd437ede613fffacc06ac6d6c93a083829022f/components/sdmmc/sdmmc_sd.c#L387-L391

So we are not using the possibility to increase the read/write block lengths, always setting the effective block length to 512: https://github.com/espressif/esp-idf/blob/d4cd437ede613fffacc06ac6d6c93a083829022f/components/sdmmc/sdmmc_cmd.c#L273-L278

Basically, the existing code should work even on cards with read_bl_len > 9. But anyway, your fix is logically correct, so we'll change this, even though this is effectively dead code...

[igrr]

@huming2207 Somewhat unrelated, I find that XTSD01GLGEAG is marked as "discontinued" at LCSC. And XTX website mentions a different variant, XTSDG01GWSIGA, but it's not in stock.

[huming2207]

The current implementation effectively always sets card->sector_size to 512, regardless of read_blk_len.

Ah... this will definitely cause some undefined behaviours on one of our products, so it needs to be fixed. For that product we need to deal with some real-time data. We can't use any filesystems because they are often too slow, so we wrote our own block-level storage instead. We do rely on the sector_size to cache the data. But so far we are using 512-byte sector SD NAND chips only, like CSNP1GCR01-AOW, so we are still safe for now.

Somewhat unrelated, I find that XTSD01GLGEAG is marked as "discontinued" at LCSC. And XTX website mentions a different variant, XTSDG01GWSIGA, but it's not in stock.

We haven't reached out to XTX for now, and we never tested their chips in a larger scale. We only have a few demo devices with their 1Gbit chip assembled but it seems working anyway. But I can't remember the part number and I'm not sure which one it is...😅

We planned to use Create Storage World's chip for production later. The CSW's chip seems to always report 512 bytes on their read/write_bl_len no matter what capacity it is.

[igrr]

this will definitely cause some undefined behaviours on one of our products, so it needs to be fixed. For that product we need to deal with some real-time data. We can't use any filesystems because they are often too slow, so we wrote our own block-level storage instead. We do rely on the sector_size to cache the data.

Sorry, could you explain why this will cause undefined behavior? To be clear, it is totally supported by SD standard to set the read/write block length to 512 bytes for SDSC cards, regardless of the maxium supported read_blk_len/write_blk_len. SDSC cards allow for the read/write block length to be larger than 512 bytes, but the host can still set the actual block length to 512. Linux and Zephyr SDMMC stacks do the same thing. Neither of them even issue MMC_SET_BLOCKLEN command in the standard card initialization flow, as far as I can see.

If you use card->sector_size value, then everything should work correctly, as that's the same value that SDMMC driver will use when sending data transfer commands.

[huming2207]

Sorry, could you explain why this will cause undefined behavior?

Hmm nevermind, sorry I double checked our code against the SD spec and I think I might have misunderstood the spec again... It actually doesn't matter at all even with a "wrongly" reported sector_size. 😅

[igrr]

@huming2207 I found that I don't actually have any SDSC card which right now which I can use to verify this change. Given that we have concluded that your application should work fine even with 512 byte sector_size value, I'll close the PR. If I get my hands on an SDSC card with read_bl_len > 9, I'll revisit this and get it merged.

Thanks again for taking your time on this PR.

[igrr]

I got one of those SD NAND chips with csd_ver=1 (i.e. SDSC) and read_bl_len=10. On a quick look, the tests seem to pass, but I've reopened the internal issue to verify that everything works correctly.