Skip to content

drivers/mtd: Remove bad block management in FTL when it is not needed #19018

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

Merged
jerpelea merged 1 commit into from
Jun 8, 2026
Merged

drivers/mtd: Remove bad block management in FTL when it is not needed #19018

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions drivers/mtd/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,15 @@ config FTL_READAHEAD
default n
depends on DRVR_READAHEAD

config FTL_BBM
bool "Enable bad block management in the FTL layer"
default y if MTD_NAND

@xiaoxiang781216 xiaoxiang781216 Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
default y if MTD_NAND
default !DEFAULT_SMALL

@PetervdPerk-NXP PetervdPerk-NXP Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@xiaoxiang781216 I don’t agree with enabling this by default for non‑DEFAULT_SMALL configurations. I have several chips that use FTL without bad block management, so it’s unclear why this should be applied universally to those devices. Bad block management is typically only required for NAND-based technologies.

@jlaitine jlaitine Jun 3, 2026 •
edited
Loading

Copy link
Copy Markdown
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 DEFAULT_SMALL is the correct one here. All of the boards which I am working on are !DEFAULT_SMALL and bad block management is not needed. Only reasonable default that I can think of is really MTD_NAND, because BBM on the host controller side is commonly needed only for raw nand devices. If the motivation behind the suggestion is that there are other than MTD_NAND devices, where default y is wanted, please give an example of that... perhaps we can add some additiona "default y if ..."?

In addition, we could have a DEFAULT_SMALL board using raw nand, in which case the BBM would be needed.

So I'd rather find a reasonable default based on whether the functionality is required or not, and not couple it to DEFAULT_SMALL.

@xiaoxiang781216 xiaoxiang781216 Jun 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

if so, please add assert if mtd support isbad or markbad, but FTL_BBM is false.

@jlaitine jlaitine Jun 3, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

if so, please add assert if mtd support isbad or markbad, but FTL_BBM is false.

Sounds reasonable but where? In ftl_open perhaps? DEBUGASSERT?

What if user wants to intentionally disable the bad block management? Perhaps there is no reason to do so...

@michallenc michallenc Jun 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No, I agree most of the drivers don't need it. But after this merge request you have to enable CONFIG_FTL_BBM if you use mtd_partition to create multiple partitions on your flash device. And that's not a theoretical bug, it's fairly common use case. If you don't enable CONFIG_FTL_BBM, you program runs into the debug assertion, even if your driver doesn't need bad block management. This should be solved by the diff above and we won't have to enable CONFIG_FTL_BBM for most of the NOR flash devices (perhaps for all of them...). I can create a merge request for the change if you want.

@jlaitine jlaitine Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ah sorry. Now I understand. The debugassert is definitely wrong! @xiaoxiang781216 I think we should remove that debugassert, and solve the potential misconfiguration you were worried about by other means.

Thanks @michallenc for you patience, sorry for me being so slow to understand what you were saying :)

@jlaitine jlaitine Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Simplest fix would be to just remove the DEBUGASSERT. I can also check if something could be done to make proper compile time check for original purpose why it was suggested to be added.

@jlaitine jlaitine Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Perhaps something in this direction: #19145 , do you think this would solve the issue @michallenc, @xiaoxiang781216 ?

@jlaitine jlaitine Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Or then we can just pick the original patch that @michallenc provided and keep the debugassert?

---help---
Enable logical to physical erase-block mapping in the FTL layer for
MTD devices that expose bad blocks via the MTD isbad/markbad methods.

This is needed for devices that require software bad block management.

config MTD_SECT512
bool "512B sector conversion"
default n
Expand Down Expand Up @@ -1442,6 +1451,7 @@ config MTD_GD5F
bool "SPI-based GD5F nand FLASH"
default n
select SPI
select FTL_BBM

if MTD_GD5F

Expand Down
48 changes: 45 additions & 3 deletions drivers/mtd/ftl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,12 @@ struct ftl_struct_s
FAR uint8_t *eblock; /* One, in-memory erase block */
int oflags;

/* The nand block map between logic block and physical block */
#ifdef CONFIG_FTL_BBM
/* The block map between logic block and physical block */

FAR off_t *lptable;
off_t lpcount;
#endif
};

/****************************************************************************
Expand Down Expand Up @@ -133,12 +135,14 @@ static const struct block_operations g_bops =
* Private Functions
****************************************************************************/

#ifdef CONFIG_FTL_BBM

/****************************************************************************
* Name: ftl_init_map
*
* Description: Allocate logical block and physical block mapping table
* space, and scan the entire nand flash device to establish
* the mapping relationship between logical block and physical
* space, and scan the entire MTD device to establish the
* mapping relationship between logical block and physical
* good block.
*
****************************************************************************/
Expand Down Expand Up @@ -207,6 +211,7 @@ static size_t ftl_get_cblock(FAR struct ftl_struct_s *dev, off_t start,

return count;
}
#endif

/****************************************************************************
* Name: ftl_open
Expand Down Expand Up @@ -273,11 +278,15 @@ static int ftl_close(FAR struct inode *inode)
static ssize_t ftl_mtd_bread(FAR struct ftl_struct_s *dev, off_t startblock,
size_t nblocks, FAR uint8_t *buffer)
{
#ifdef CONFIG_FTL_BBM
off_t mask = dev->blkper - 1;
size_t nread = nblocks;
#endif
ssize_t ret = OK;

#ifdef CONFIG_FTL_BBM
if (dev->lptable == NULL)
#endif
{
ret = MTD_BREAD(dev->mtd, startblock, nblocks, buffer);
if (ret != nblocks)
Expand All @@ -289,6 +298,7 @@ static ssize_t ftl_mtd_bread(FAR struct ftl_struct_s *dev, off_t startblock,
return ret;
}

#ifdef CONFIG_FTL_BBM
while (nblocks > 0)
{
off_t startphysicalblock;
Expand Down Expand Up @@ -324,6 +334,7 @@ static ssize_t ftl_mtd_bread(FAR struct ftl_struct_s *dev, off_t startblock,
}

return nblocks != nread ? nread - nblocks : ret;
#endif
}

/****************************************************************************
Expand All @@ -339,10 +350,14 @@ static ssize_t ftl_mtd_bread(FAR struct ftl_struct_s *dev, off_t startblock,
static ssize_t ftl_mtd_bwrite(FAR struct ftl_struct_s *dev, off_t startblock,
FAR const uint8_t *buffer)
{
#ifdef CONFIG_FTL_BBM
off_t starteraseblock;
#endif
ssize_t ret;

#ifdef CONFIG_FTL_BBM
if (dev->lptable == NULL)
#endif
{
ret = MTD_BWRITE(dev->mtd, startblock, dev->blkper, buffer);
if (ret != dev->blkper)
Expand All @@ -354,6 +369,7 @@ static ssize_t ftl_mtd_bwrite(FAR struct ftl_struct_s *dev, off_t startblock,
return ret;
}

#ifdef CONFIG_FTL_BBM
starteraseblock = startblock / dev->blkper;
while (1)
{
Expand All @@ -372,6 +388,7 @@ static ssize_t ftl_mtd_bwrite(FAR struct ftl_struct_s *dev, off_t startblock,
MTD_MARKBAD(dev->mtd, dev->lptable[starteraseblock]);
ftl_update_map(dev, starteraseblock);
}
#endif
}

/****************************************************************************
Expand All @@ -389,7 +406,9 @@ static ssize_t ftl_mtd_erase(FAR struct ftl_struct_s *dev, off_t startblock)
{
ssize_t ret;

#ifdef CONFIG_FTL_BBM
if (dev->lptable == NULL)
#endif
{
ret = MTD_ERASE(dev->mtd, startblock, 1);
if (ret < 0 && ret != -ENOSYS)
Expand All @@ -402,6 +421,7 @@ static ssize_t ftl_mtd_erase(FAR struct ftl_struct_s *dev, off_t startblock)
return OK;
}

#ifdef CONFIG_FTL_BBM
while (1)
{
if (startblock >= dev->lpcount)
Expand All @@ -418,6 +438,7 @@ static ssize_t ftl_mtd_erase(FAR struct ftl_struct_s *dev, off_t startblock)
MTD_MARKBAD(dev->mtd, dev->lptable[startblock]);
ftl_update_map(dev, startblock);
}
#endif
}

/****************************************************************************
Expand Down Expand Up @@ -517,7 +538,9 @@ static ssize_t ftl_flush_direct(FAR struct ftl_struct_s *dev,
}
}

#ifdef CONFIG_FTL_BBM
if (dev->lptable == NULL)
#endif
{
ret = MTD_BWRITE(dev->mtd, startblock, count, buffer);
if (ret != count)
Expand All @@ -527,6 +550,7 @@ static ssize_t ftl_flush_direct(FAR struct ftl_struct_s *dev,
return ret;
}
}
#ifdef CONFIG_FTL_BBM
else
{
if (starteraseblock >= dev->lpcount)
Expand All @@ -544,6 +568,7 @@ static ssize_t ftl_flush_direct(FAR struct ftl_struct_s *dev,
continue;
}
}
#endif

nblocks -= count;
startblock += count;
Expand All @@ -567,7 +592,11 @@ static ssize_t ftl_flush(FAR void *priv, FAR const uint8_t *buffer,
int nbytes;
int ret;

#ifdef CONFIG_FTL_BBM
if (dev->mtd->erase == NULL && dev->lptable == NULL)
#else
if (dev->mtd->erase == NULL)
#endif
{
ret = MTD_BWRITE(dev->mtd, startblock, nblocks, buffer);
if (ret != nblocks)
Expand Down Expand Up @@ -922,6 +951,15 @@ int ftl_initialize_by_path(FAR const char *path, FAR struct mtd_dev_s *mtd,

finfo("path=\"%s\"\n", path);

#ifndef CONFIG_FTL_BBM
/* It is likely a configuration error if the mtd driver implements
* the bad block management, but it is still disabled by the
* configuration.
*/

DEBUGASSERT(mtd->isbad == NULL && mtd->markbad == NULL);
#endif

/* Allocate a FTL device structure */

dev = kmm_zalloc(sizeof(struct ftl_struct_s));
Expand Down Expand Up @@ -978,6 +1016,7 @@ int ftl_initialize_by_path(FAR const char *path, FAR struct mtd_dev_s *mtd,
}
#endif

#ifdef CONFIG_FTL_BBM
if (MTD_ISBAD(dev->mtd, 0) != -ENOSYS)
{
ret = ftl_init_map(dev);
Expand All @@ -986,15 +1025,18 @@ int ftl_initialize_by_path(FAR const char *path, FAR struct mtd_dev_s *mtd,
goto out;
}
}
#endif

/* Inode private data is a reference to the FTL device structure */

ret = register_blockdriver(path, &g_bops, 0, dev);
if (ret < 0)
{
ferr("ERROR: register_blockdriver failed: %d\n", -ret);
#ifdef CONFIG_FTL_BBM
kmm_free(dev->lptable);
out:
#endif
#ifdef FTL_HAVE_RWBUFFER
rwb_uninitialize(&dev->rwb);
#endif
Expand Down
Loading