Skip to content

storage: disk_access: add disk_access_erase #75802

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.

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
35 changes: 35 additions & 0 deletions drivers/disk/flashdisk.c
Original file line number Diff line number Diff line change
Expand Up @@ -427,6 +427,40 @@ static int disk_flash_access_write(struct disk_info *disk, const uint8_t *buff,
return 0;
}

static int disk_flash_access_erase(struct disk_info *disk, uint32_t start_sector,
uint32_t sector_count)
{
struct flashdisk_data *ctx;
off_t fl_start, fl_end;
uint32_t size;
int rc = 0;

ctx = CONTAINER_OF(disk, struct flashdisk_data, info);

if (!sectors_in_range(ctx, start_sector, sector_count)) {
return -EINVAL;
}

fl_start = ctx->offset + start_sector * ctx->sector_size;
size = (sector_count * ctx->sector_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

can drop parentheses

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Style was copied from every other usage in the driver

fl_end = fl_start + size;

k_mutex_lock(&ctx->lock, K_FOREVER);

/* Erase the provided sectors */
if (flash_erase(ctx->info.dev, fl_start, size) < 0) {
rc = -EIO;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not reporting the errno returned by flash_erase()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as all other calls in the driver

}
/* Invalidate cache if it was pointing in this address range */
if (ctx->cache_valid && ((fl_start <= ctx->cached_addr) && (ctx->cached_addr < fl_end))) {
ctx->cache_valid = false;
ctx->cache_dirty = false;
}
k_mutex_unlock(&ctx->lock);

return rc;
}

static int disk_flash_access_ioctl(struct disk_info *disk, uint8_t cmd, void *buff)
{
int rc;
Expand Down Expand Up @@ -466,6 +500,7 @@ static const struct disk_operations flash_disk_ops = {
.status = disk_flash_access_status,
.read = disk_flash_access_read,
.write = disk_flash_access_write,
.erase = disk_flash_access_erase,
.ioctl = disk_flash_access_ioctl,
};

Expand Down
29 changes: 29 additions & 0 deletions drivers/disk/loopback_disk.c
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,34 @@ static int loopback_disk_access_write(struct disk_info *disk, const uint8_t *dat

return 0;
}
static int loopback_disk_access_erase(struct disk_info *disk, uint32_t start_sector,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add an empty line above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every other function in this driver has the same style

uint32_t num_sector)
{
const uint8_t erase_bytes[CONFIG_LOOPBACK_DISK_SECTOR_SIZE] = { 0x00 };
struct loopback_disk_access *ctx = get_ctx(disk);

if (start_sector + num_sector > ctx->num_sectors) {
LOG_WRN("Tried to erase past end of backing file");
return -EINVAL;
}

int ret = fs_seek(&ctx->file, start_sector * LOOPBACK_SECTOR_SIZE, FS_SEEK_SET);

if (ret != 0) {
LOG_ERR("Failed to seek backing file: %d", ret);
return ret;
}

for (int i = 0; i < num_sector; i++) {
ret = fs_write(&ctx->file, erase_bytes, LOOPBACK_SECTOR_SIZE);
if (ret < 0) {
LOG_ERR("Failed to erase backing file: %d", ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

Worth reporting the sector number? Just asking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Write and read functions don't

return ret;
}
}

return 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add an empty line below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto, style copied from driver

static int loopback_disk_access_ioctl(struct disk_info *disk, uint8_t cmd, void *buff)
{
struct loopback_disk_access *ctx = get_ctx(disk);
Expand Down Expand Up @@ -126,6 +154,7 @@ static const struct disk_operations loopback_disk_operations = {
.status = loopback_disk_access_status,
.read = loopback_disk_access_read,
.write = loopback_disk_access_write,
.erase = loopback_disk_access_erase,
.ioctl = loopback_disk_access_ioctl,
};

Expand Down
19 changes: 19 additions & 0 deletions drivers/disk/ramdisk.c
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,24 @@ static int disk_ram_access_write(struct disk_info *disk, const uint8_t *buff,
return 0;
}

static int disk_ram_access_erase(struct disk_info *disk, uint32_t sector,
uint32_t count)
{
const struct device *dev = disk->dev;
const struct ram_disk_config *config = dev->config;
uint32_t last_sector = sector + count;

if (last_sector < sector || last_sector > config->sector_count) {
LOG_ERR("Sector %" PRIu32 " is outside the range %zu",
last_sector, config->sector_count);
return -EINVAL;
}

memset(lba_to_address(dev, sector), 0, count * config->sector_size);

return 0;
}

static int disk_ram_access_ioctl(struct disk_info *disk, uint8_t cmd, void *buff)
{
const struct ram_disk_config *config = disk->dev->config;
Expand Down Expand Up @@ -122,6 +140,7 @@ static const struct disk_operations ram_disk_ops = {
.status = disk_ram_access_status,
.read = disk_ram_access_read,
.write = disk_ram_access_write,
.erase = disk_ram_access_erase,
.ioctl = disk_ram_access_ioctl,
};

Expand Down
24 changes: 24 additions & 0 deletions drivers/disk/sdmmc_stm32.c
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,29 @@ static int stm32_sdmmc_access_write(struct disk_info *disk,
return err;
}

static int stm32_sdmmc_access_erase(struct disk_info *disk, uint32_t sector, uint32_t count)
{
const struct device *dev = disk->dev;
struct stm32_sdmmc_priv *priv = dev->data;
int err;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
int err;
HAL_StatusTypeDef hal_err;
int err = 0;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Style was copied from the other functions in the driver


k_sem_take(&priv->thread_lock, K_FOREVER);

err = HAL_SD_Erase(&priv->hsd, sector, sector + count);
if (err != HAL_OK) {
LOG_ERR("sd erase block failed %d", err);
err = -EIO;
goto end;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

would prefer an else instruction instead of a goto instruction.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

style copied from read and write functions


while (!stm32_sdmmc_is_card_in_transfer(&priv->hsd)) {
}

end:
k_sem_give(&priv->thread_lock);
return err;
}

static int stm32_sdmmc_get_card_info(HandleTypeDef *hsd, CardInfoTypeDef *info)
{
#ifdef CONFIG_SDMMC_STM32_EMMC
Expand Down Expand Up @@ -606,6 +629,7 @@ static const struct disk_operations stm32_sdmmc_ops = {
.status = stm32_sdmmc_access_status,
.read = stm32_sdmmc_access_read,
.write = stm32_sdmmc_access_write,
.erase = stm32_sdmmc_access_erase,
.ioctl = stm32_sdmmc_access_ioctl,
};

Expand Down
9 changes: 9 additions & 0 deletions drivers/disk/sdmmc_subsys.c
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,14 @@ static int disk_sdmmc_access_write(struct disk_info *disk, const uint8_t *buf,
return sdmmc_write_blocks(&data->card, buf, sector, count);
}

static int disk_sdmmc_access_erase(struct disk_info *disk, uint32_t sector, uint32_t count)
{
const struct device *dev = disk->dev;
struct sdmmc_data *data = dev->data;
Comment on lines +90 to +91
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking: struct sdmmc_data *data = disk->dev->data shouod be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

style copied from other functions


return sdmmc_erase_blocks(&data->card, sector, count);
}

static int disk_sdmmc_access_ioctl(struct disk_info *disk, uint8_t cmd, void *buf)
{
const struct device *dev = disk->dev;
Expand All @@ -109,6 +117,7 @@ static const struct disk_operations sdmmc_disk_ops = {
.status = disk_sdmmc_access_status,
.read = disk_sdmmc_access_read,
.write = disk_sdmmc_access_write,
.erase = disk_sdmmc_access_erase,
.ioctl = disk_sdmmc_access_ioctl,
};

Expand Down
1 change: 1 addition & 0 deletions include/zephyr/drivers/disk.h
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ struct disk_operations {
uint32_t start_sector, uint32_t num_sector);
int (*write)(struct disk_info *disk, const uint8_t *data_buf,
uint32_t start_sector, uint32_t num_sector);
int (*erase)(struct disk_info *disk, uint32_t start_sector, uint32_t num_sector);
int (*ioctl)(struct disk_info *disk, uint8_t cmd, void *buff);
};

Expand Down
14 changes: 14 additions & 0 deletions include/zephyr/sd/sdmmc.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,20 @@
extern "C" {
#endif

/**
* @brief Erase blocks from SD card
*
* @param card SD card to write from
* @param start_block first block to erase
* @param num_blocks number of blocks to erase
* @retval 0 erase succeeded
* @retval -EBUSY: card is busy with another request
* @retval -EINVAL: requested erase outside of card bounds
Copy link
Contributor

Choose a reason for hiding this comment

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

"... or operation is not supported"
I still think -ENOTSUP would be better in this case.

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 same error condition (card->type == CARD_SDIO) for the read and write functions is not documented, I assumed it was for a reason.

* @retval -ETIMEDOUT: card write timed out
* @retval -EIO: I/O error
*/
int sdmmc_erase_blocks(struct sd_card *card, uint32_t start_block, uint32_t num_blocks);

/**
* @brief Write blocks to SD card from buffer
*
Expand Down
20 changes: 20 additions & 0 deletions include/zephyr/storage/disk_access.h
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,26 @@ int disk_access_read(const char *pdrv, uint8_t *data_buf,
int disk_access_write(const char *pdrv, const uint8_t *data_buf,
uint32_t start_sector, uint32_t num_sector);

enum disk_access_erase_type {
/** Erase the physical bytes on the disk to their natural erase value (0x00 or 0xFF) */
DISK_ACCESS_ERASE_PHYSICAL = 0,
};

/**
* @brief erase data from disk
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should probably note here that the value of the erased bytes is driver specific (or perhaps limit the acceptable values to 0xff and 0x0, but I'd prefer to leave it open ended)

Copy link
Contributor

Choose a reason for hiding this comment

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

One request here, otherwise I'm good with this change- we should document here that the erased data must read back as 0x0 or 0xFF. This way we align with what the test checks, and specifically exclude a disk operation like DISCARD from being implemented to satisfy this API.

Copy link
Contributor

Choose a reason for hiding this comment

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

There is a command to get the info, so with implementation of erase we also should provide the IOCTL or other means to get info on what erase value there is on the device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Finally a chance to close the very first issue I submitted to Zephyr #24417 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Heh. Yeah, still assigned to me, somehow. You can have it.

*
* The result of this operation depends on the type of erase, as specified by @a erase_type.
*
* @param[in] pdrv Disk name
* @param[in] start_sector Start disk sector to erase
* @param[in] num_sector Number of disk sectors to erase
* @param[in] erase_type Type of erase to perform
*
* @return 0 on success, negative errno code on fail
*/
int disk_access_erase(const char *pdrv, uint32_t start_sector, uint32_t num_sector,
enum disk_access_erase_type erase_type);

/**
* @brief Get/Configure disk parameters
*
Expand Down
30 changes: 30 additions & 0 deletions subsys/disk/disk_access.c
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,36 @@ int disk_access_write(const char *pdrv, const uint8_t *data_buf,
return rc;
}

int disk_access_erase(const char *pdrv, uint32_t start_sector, uint32_t num_sector,
enum disk_access_erase_type erase_type)
{
struct disk_info *disk = disk_access_get_di(pdrv);
uint32_t erase_sector_size;
int rc = -EINVAL;

/* Only support physical erase for now.
* This parameter is not passed through to the underlying disk to leave the design
* space open for future erase types (Other erase types may be dedicated functions).
*/
if (erase_type != DISK_ACCESS_ERASE_PHYSICAL) {
return -EINVAL;
}

/* Validate sector sizes, if underlying driver exposes a way to query it */
if (disk_access_ioctl(pdrv, DISK_IOCTL_GET_ERASE_BLOCK_SZ, &erase_sector_size) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Failure needs not to be reported to the caller, at least if not -ENOTSUP?
I see the other API functions do not, so maybe not really expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

if underlying driver exposes a way to query it

Is the key point here, this is a pre-check if we have the capability to check, otherwise just try the operation and see what happens.

/* Alignment check on both start and range of erase request */
if ((start_sector % erase_sector_size) || (num_sector % erase_sector_size)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if ((start_sector % erase_sector_size) || (num_sector % erase_sector_size)) {
if ((start_sector % erase_sector_size) != 0 ||
(num_sector % erase_sector_size) != 0) {

Copy link
Contributor Author

@JordanYates JordanYates Aug 21, 2025

Choose a reason for hiding this comment

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

I don't really think thats any clearer, especially when considering the extra parentheses required to make it unambiguous that you left out.

return -EINVAL;
}
}

if ((disk != NULL) && (disk->ops != NULL) && (disk->ops->erase != NULL)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

disk->ops->erase != NULL should maybe be reported as -ENOTSUP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not done that way in disk_access_read or disk_access_write

rc = disk->ops->erase(disk, start_sector, num_sector);
}

return rc;
}

int disk_access_ioctl(const char *pdrv, uint8_t cmd, void *buf)
{
struct disk_info *disk = disk_access_get_di(pdrv);
Expand Down
79 changes: 79 additions & 0 deletions subsys/sd/sd_ops.c
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,85 @@ int card_write_blocks(struct sd_card *card, const uint8_t *wbuf, uint32_t start_
return 0;
}

static int card_erase(struct sd_card *card, uint32_t start_block, uint32_t num_blocks)
{
int ret;
struct sdhc_command cmd;

LOG_DBG("ERASE: Sector = %u, Count = %u", start_block, num_blocks);
cmd.retries = CONFIG_SD_DATA_RETRIES;
cmd.timeout_ms = CONFIG_SD_CMD_TIMEOUT;

cmd.opcode = SD_ERASE_BLOCK_START;
cmd.response_type = (SD_RSP_TYPE_R1 | SD_SPI_RSP_TYPE_R1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could drop parentheses.
Ditto for a few occurrences below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

style copied from other functions

cmd.arg = start_block;
if (!(card->flags & SD_HIGH_CAPACITY_FLAG)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Coding style:

Suggested change
if (!(card->flags & SD_HIGH_CAPACITY_FLAG)) {
if ((card->flags & SD_HIGH_CAPACITY_FLAG) != 0) {

Ditto for a few occurrences below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think that's any clearer, especially since you got the condition around the wrong way and didn't notice.

/* Standard capacity cards use byte unit address */
cmd.arg *= card->block_size;
}
ret = sdhc_request(card->sdhc, &cmd, NULL);
if (ret) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (ret) {
if (ret != 0) {

Ditto for a few occurrences below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

every other usage in this driver is if (ret)

LOG_DBG("SD_ERASE_BLOCK_START failed (%d)", ret);
Copy link
Contributor

Choose a reason for hiding this comment

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

LOG_ERR()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Errors everywhere else are LOG_DBG

return ret;
}

cmd.opcode = SD_ERASE_BLOCK_END;
cmd.response_type = (SD_RSP_TYPE_R1 | SD_SPI_RSP_TYPE_R1);
cmd.arg = start_block + num_blocks - 1;
if (!(card->flags & SD_HIGH_CAPACITY_FLAG)) {
/* Standard capacity cards use byte unit address */
cmd.arg *= card->block_size;
}
ret = sdhc_request(card->sdhc, &cmd, NULL);
if (ret) {
LOG_DBG("SD_ERASE_BLOCK_END failed (%d)", ret);
return ret;
}

cmd.opcode = SD_ERASE_BLOCK_OPERATION;
cmd.response_type = (SD_RSP_TYPE_R1b | SD_SPI_RSP_TYPE_R1b);
cmd.arg = 0x00000000;
ret = sdhc_request(card->sdhc, &cmd, NULL);
if (ret) {
LOG_DBG("SD_ERASE_BLOCK_OPERATION failed (%d)", ret);
return ret;
}

/* Verify card is back in transfer state after write */
ret = sdmmc_wait_ready(card);
if (ret) {
LOG_ERR("Card did not return to ready state");
return -ETIMEDOUT;
}
return 0;
}

/* Erase blocks from SD card memory card */
int card_erase_blocks(struct sd_card *card, uint32_t start_block, uint32_t num_blocks)
{
int ret;

/* Overflow aware ((start_block + num_blocks) > card->block_count) */
if (num_blocks > card->block_count || (card->block_count - num_blocks) < start_block) {
return -EINVAL;
}
if (card->type == CARD_SDIO) {
LOG_WRN("SDIO does not support MMC commands");
return -ENOTSUP;
}
ret = k_mutex_lock(&card->lock, K_MSEC(CONFIG_SD_DATA_TIMEOUT));
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer with an empty line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Getting tired of writing "every other function in this file has the same style"

if (ret) {
LOG_WRN("Could not get SD card mutex");
return -EBUSY;
}
ret = card_erase(card, start_block, num_blocks);
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto

k_mutex_unlock(&card->lock);
if (ret) {
LOG_ERR("Erase failed");
}
return ret;
}

/* IO Control handler for SD MMC */
int card_ioctl(struct sd_card *card, uint8_t cmd, void *buf)
{
Expand Down
3 changes: 3 additions & 0 deletions subsys/sd/sd_ops.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ int card_read_blocks(struct sd_card *card, uint8_t *rbuf,
int card_write_blocks(struct sd_card *card, const uint8_t *wbuf,
uint32_t start_block, uint32_t num_blocks);

int card_erase_blocks(struct sd_card *card, uint32_t start_block,
uint32_t num_blocks);

int card_app_command(struct sd_card *card, int relative_card_address);

int sdmmc_read_status(struct sd_card *card);
Expand Down
6 changes: 6 additions & 0 deletions subsys/sd/sdmmc.c
Original file line number Diff line number Diff line change
Expand Up @@ -793,3 +793,9 @@ int sdmmc_write_blocks(struct sd_card *card, const uint8_t *wbuf, uint32_t start
{
return card_write_blocks(card, wbuf, start_block, num_blocks);
}

int sdmmc_erase_blocks(struct sd_card *card, uint32_t start_block,
uint32_t num_blocks)
{
return card_erase_blocks(card, start_block, num_blocks);
}
7 changes: 7 additions & 0 deletions tests/drivers/disk/disk_access/README.txt
Original file line number Diff line number Diff line change
Expand Up @@ -25,3 +25,10 @@ disk devices as well. The test has the following phases:
of various length to various sectors (once again, the driver must reject
writes that would be outside the bounds of the disk), then performs multiple
writes to the same location.

* Erase test: Verifies that the driver can consistently erase sectors. This test
follows the same flow as the write test, but at each step erases the data
written to the disk and reads it back to ensure all data is 0x00 or 0xFF. The
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't really imagine a reason that a disk would erase to anything besides 0x0 or 0xff, but if we are testing disks like this we need to document this as an API restriction in disk_access_erase. IE something like the following in the docstring:

Sectors may read as 0x00 or 0xFF after erasing, depending on the underlying device implementation

test first performs writes of various length to various sectors (once again,
the driver must reject erases that would be outside the bounds of the disk),
then performs multiple erases to the same location.
Loading