8000 {cpu/sam0_common/sam0_sdhc,drivers/mtd}: do not allocate `work_area` twice by fabian18 · Pull Request #21169 · RIOT-OS/RIOT · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

{cpu/sam0_common/sam0_sdhc,drivers/mtd}: do not allocate work_area twice #21169

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Feb 19, 2025
Merged

{cpu/sam0_common/sam0_sdhc,drivers/mtd}: do not allocate work_area twice #21169

merged 2 commits into from
Feb 19, 2025

Conversation

fabian18
Copy link
Contributor
@fabian18 fabian18 commented Jan 28, 2025 •
edited
Loading

Contribution description

The MTD work_area could be allocated multiple times when MTD is reinitialized.
A check for not NULL is added.

Testing procedure

Custom board with an eMMC. I can craete a fatfs filesystem on it with:

ifneq (,$(filter mtd,$(USEMODULE)))
  USEMODULE += mtd_write_page
  USEMODULE += mtd_sdmmc_default
  USEMODULE += sdmmc_mmc
endif

ifneq (,$(filter vfs_default,$(USEMODULE)))
  USEMODULE += fatfs_vfs
  USEMODULE += mtd
endif

Issues/PRs references

@github-actions github-actions bot added the Area: drivers Area: Device drivers label Jan 28, 2025
@benpicco
Copy link
Contributor

MTD_DRIVER_FLAG_DIRECT_WRITE is still correct - you don't need to erase a sector to write it.
I do wonder how fatfs does unaligned writes though, mtd_diskio.c always operates on whole pages.

@fabian18
Copy link
Contributor Author

You are right, simply removing the flag does not work actually. Don´t know how it worked before.

It fails here:

RIOT/drivers/sdmmc/sdmmc.c

Lines 738 to 758 in 7e33118

int sdmmc_erase_blocks(sdmmc_dev_t *dev,
uint32_t block_addr, uint16_t block_num)
{
DEBUG("[sdmmc] %s dev=%p addr=%"PRIu32" num=%u\n",
__func__, dev, block_addr, block_num);
assert(dev);
assert(dev->driver);
assert(block_num);
int res = _assert_card(dev);
if (res) {
return res;
}
if (IS_USED(MODULE_SDMMC_MMC) && (dev->type == SDMMC_CARD_TYPE_MMC)) {
/* MMCs don't support the erase of a single block but only the erase of
* a group of blocks. Blocks are erased implicitly on write. */
DEBUG("[sdmmc] MMCs don't support the erase of single blocks\n");
return -ENOTSUP;
}

Is then simply the if condition wrong here?

RIOT/drivers/mtd/mtd.c

Lines 77 to 84 in 7e33118

#ifdef MODULE_MTD_WRITE_PAGE
if ((mtd->driver->flags & MTD_DRIVER_FLAG_DIRECT_WRITE) == 0) {
mtd->work_area = malloc(mtd->pages_per_sector * mtd->page_size);
if (mtd->work_area == NULL) {
res = -ENOMEM;
}
}
#endif

Compared to:

RIOT/cpu/sam0_common/sam0_sdhc/mtd_sdhc.c

Lines 47 to 54 in 7e33118

#if IS_USED(MODULE_MTD_WRITE_PAGE)
/* TODO: move to MTD layer */
dev->work_area = malloc(SD_MMC_BLOCK_SIZE);
if (dev->work_area == NULL) {
return -ENOMEM;
}
dev->write_size = 1;
#endif

@fabian18
Copy link
Contributor Author

I am missing the check for unaligned memory access based on write_size in the MTD layer.

Should
int mtd_write_page_raw(mtd_dev_t *mtd, const void *src, uint32_t page, uint32_t offset, uint32_t count)

check for unaligned access and then if MODULE_MTD_WRITE_PAGE is used take care of calling that instead?

@fabian18
Copy link
Contributor Author

For mtd_flashpage there is this kind of "work_area":

        uint8_t tmp[FLASHPAGE_WRITE_BLOCK_SIZE]
                __attribute__ ((aligned (FLASHPAGE_WRITE_BLOCK_ALIGNMENT)));

The allocation of work_area could be moved to the storage driver.
If in mtd_init(), the driver->init() did not allocate a work area, then the MTD layer can call malloc.

@benpicco
Copy link
Contributor

It fails here

Do you mean the

     if (IS_USED(MODULE_SDMMC_MMC) && (dev->type == SDMMC_CARD_TYPE_MMC)) { 
         /* MMCs don't support the erase of a single block but only the erase of 
          * a group of blocks. Blocks are erased implicitly on write. */ 
         DEBUG("[sdmmc] MMCs don't support the erase of single blocks\n"); 
         return -ENOTSUP; 
     }

again, with fatfs this shouldn't be an issue as we always use mtd_write_sector() which with MTD_DRIVER_FLAG_DIRECT_WRITE should never call mtd_erase_sector().

What filesystem are you trying to use? Or are you using a raw MTD device without a filesystem?

@fabian18
Copy link
Contributor Author

That's where it fails when I remove the MTD_DRIVER_FLAG_DIRECT_WRITE flag, yes.
I am using fatfs. I also tried lwext4, but it did not work. I did not spend much time to investigate why.

with fatfs this shouldn't be an issue as we always use mtd_write_sector()

For whatever reason I land here:

static int mtd_sdmmc_write_page(mtd_dev_t *dev, const void *buff, uint32_t page,
                                 uint32_t offset, uint32_t size)
{
    mtd_sdmmc_t *mtd_sd = (mtd_sdmmc_t*)dev;

    DEBUG("mtd_sdmmc_write_page: page:%" PRIu32 " offset:%" PRIu32 " size:%" PRIu32 "\n",
          page, offset, size);

    if (offset || size % SDMMC_SDHC_BLOCK_SIZE) {
#if IS_USED(MODULE_MTD_WRITE_PAGE)
        if (dev->work_area == NULL) {
            DEBUG("mtd_sdmmc_write_page: no work area\n");
            return -ENOTSUP;
        }

@benpicco
Copy link
Contributor

Can you enable debug? What are offset and size?

@fabian18 fabian18 force-pushed the pr/mtd_sdmmc_remove_MTD_DRIVER_FLAG_DIRECT_WRITE branch from 921939a to 9e7a269 Compare February 12, 2025 15:26
@github-actions github-actions bot added Platform: ARM Platform: This PR/issue effects ARM-based platforms Area: cpu Area: CPU/MCU ports labels Feb 12, 2025
@fabian18
Copy link
Contributor Author

I wanted to debug. But suddenly it works with two different boards. I do not 100% trust it still. Its very strange.
But I think a check if work_area already exists is preventing an accidental double allocation when mtd_init is called multiple times.
I will change title and description of this PR.

8000 @fabian18 fabian18 changed the title drivers/mtd_sdmmc: remove MTD_DRIVER_FLAG_DIRECT_WRITE {cpu/sam0_common/sam0_sdhc,drivers/mtd}: do not allocate work_area twice Feb 12, 2025
@fabian18
Copy link
Contributor Author

There is still something else required. The fatfs filesystem does only do aligned reads and writes, but when I want to call mtd_read_page in a test, it fails for unaligned size, because the work are is not there.

Proposal: allocate work_area when write_size is not 1?

@benpicco
Copy link
Contributor

Proposal: allocate work_area when write_size is not 1?

still should be depended on a module (and then on init) - you don't want to pull on all that malloc and copy-on-write code if you just have a fatfs and don't want to interact with the raw MTD

benpicco
benpicco reviewed Feb 14, 2025
View reviewed changes
cpu/sam0_common/sam0_sdhc/mtd_sdhc.c
@@ -46,9 +46,11 @@ static int _init(mtd_dev_t *dev)

#if IS_USED(MODULE_MTD_WRITE_PAGE)
/* TODO: move to MTD layer */
Copy link
Contributor
@benpicco benpicco Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that TODO still valid?
Almost looks like it has been moved to mtd and can be removed here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, mtd_sam0_sdhc_driver has the MTD_DRIVER_FLAG_DIRECT_WRITE flag set which means the MTD layer would not allocate the work area, but the driver needs it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The difference is that the mtd_sam0_sdhc_driver.init allocates a work_area, but mtd_sdmmc_driver.init does not.
Is this actually the work around for the original motivation of the PR?

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 17, 2025
@riot-ci
Copy link
riot-ci commented Feb 17, 2025

Murdock results

✔️ PASSED

9e7a269 drivers/mtd: prevent work_area double allocation

Success Failures Total Runtime
10271 0 10271 09m:54s

Artifacts

@fabian18 fabian18 added this pull request to the merge queue Feb 19, 2025
Merged via the queue into RIOT-OS:master with commit 8bbeba1 Feb 19, 2025
30 checks passed
@mguetschow mguetschow added this to the Release 2025.04 milestone Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benpicco benpicco benpicco approved these changes

@dylad dylad Awaiting requested review from dylad dylad is a code owner

@keestux keestux Awaiting requested review from keestux keestux is a code owner

Assignees
No one assigned
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ARM Platform: This PR/issue effects ARM-based platforms
Projects
None yet
Milestone
Release 2025.04
Development

Successfully merging this pull request may close these issues.
4 participants
@fabian18 @benpicco @riot-ci @mguetschow
0