[ndm] Ensure fatal errors are not suppressed
The FTL already has provisions for handling fatal errors in these cases,
but they were being remapped in the NDM driver itself.
Update NDM driver tests to use a fixture.
Bug: 93942
Test: fx test ftl-library-test
Change-Id: Ib0afd8027a700ce2ba7b460f4c20fe13287b6dd4
Reviewed-on: https://fuchsia-review.googlesource.com/c/fuchsia/+/646121
Reviewed-by: Martin Lindsay <mlindsay@google.com>
Commit-Queue: Brandon Castellano <bcastell@google.com>
(cherry picked from commit d2c6b6f95dc8fa99ab8f67b1801b758af8143de2)
diff --git a/zircon/system/ulib/ftl/ftln/ndm-driver.cc b/zircon/system/ulib/ftl/ftln/ndm-driver.cc
index 2f42106..5741c31 100644
--- a/zircon/system/ulib/ftl/ftln/ndm-driver.cc
+++ b/zircon/system/ulib/ftl/ftln/ndm-driver.cc
@@ -18,6 +18,10 @@
namespace {
+// Many of the NDM driver functions and tests require that these constants match. If they ever
+// are updated to be unique constants, the various API contacts need to be updated accordingly.
+static_assert(kNdmUncorrectableEcc == kNdmError, "kNdmUncorrectableEcc should map to kNdmError!");
+
bool g_init_performed = false;
// Extra configuration data saved to the partition info.
@@ -73,21 +77,12 @@
return ReadPagesImpl(page, 1, data, nullptr, dev);
}
-// Returns kNdmOk or kNdmError on ECC decode failure.
+// Returns kNdmOk, kNdmFatalError, or kNdmUncorrectableEcc on ECC decode failure.
int ReadSpare(uint32_t page, uint8_t* spare, void* dev) {
int result = ReadPagesImpl(page, 1, nullptr, spare, dev);
- if (result == kNdmFatalError || result == kNdmUncorrectableEcc) {
- return kNdmError;
- }
-
- // kNdmUnsafeEcc is also OK as the data is still correct.
- return kNdmOk;
-}
-
-// Returns kNdmOk or kNdmError.
-int ReadSpareNoEcc(uint32_t page, uint8_t* spare, void* dev) {
- int result = ReadPagesImpl(page, 1, nullptr, spare, dev);
- return result == kNdmFatalError ? kNdmError : kNdmOk;
+ // Ensure we translate errors to the correct values above, since
+ // kNdmUnsafeEcc is OK as the data is still correct.
+ return result == kNdmUnsafeEcc ? kNdmOk : result;
}
// Returns kNdmOk, kNdmError or kNdmFatalError. kNdmError triggers marking the block as bad.
@@ -131,20 +126,24 @@
return device->IsEmptyPage(page, data, spare) ? kTrue : kFalse;
}
-// Returns kNdmOk or kNdmError, but kNdmError implies aborting initialization.
+// Returns kNdmOk or kNdmFatalError, where kNdmFatalError implies aborting initialization.
int CheckPage(uint32_t page, uint8_t* data, uint8_t* spare, int* status, void* dev) {
int result = ReadPagesImpl(page, 1, data, spare, dev);
- NdmDriver* device = reinterpret_cast<NdmDriver*>(dev);
- if (result == kNdmUncorrectableEcc || result == kNdmFatalError ||
- device->IncompletePageWrite(spare, data)) {
+ if (result == kNdmFatalError) {
+ *status = NDM_PAGE_INVALID;
+ return kNdmFatalError;
+ }
+
+ // Mark page as invalid if the data fails ECC or if we detected a partial page write, since both
+ // the page and spare data should not be used in that case.
+ NdmDriver* device = reinterpret_cast<NdmDriver*>(dev);
+ if (result == kNdmUncorrectableEcc || device->IncompletePageWrite(spare, data)) {
*status = NDM_PAGE_INVALID;
return kNdmOk;
}
- bool empty = device->IsEmptyPage(page, data, spare) ? kTrue : kFalse;
-
- *status = empty ? NDM_PAGE_ERASED : NDM_PAGE_VALID;
+ *status = device->IsEmptyPage(page, data, spare) ? NDM_PAGE_ERASED : NDM_PAGE_VALID;
return kNdmOk;
}
@@ -366,7 +365,10 @@
driver->write_data_and_spare = WritePage;
driver->read_decode_data = ReadPage;
driver->read_decode_spare = ReadSpare;
- driver->read_spare = ReadSpareNoEcc;
+ // Since we have "free" decoding, we use the same ReadSpare function for this callback (which
+ // should never be invoked anyways if FSF_FREE_SPARE_ECC is set). If this changes in the future,
+ // this callback should be updated and the the FSF_FREE_SPARE_ECC flag cleared.
+ driver->read_spare = ReadSpare;
driver->data_and_spare_erased = IsEmpty;
driver->data_and_spare_check = CheckPage;
driver->erase_block = EraseBlock;
diff --git a/zircon/system/ulib/ftl/test/ndm_driver_test.cc b/zircon/system/ulib/ftl/test/ndm_driver_test.cc
index cd7e095..46a2b7e 100644
--- a/zircon/system/ulib/ftl/test/ndm_driver_test.cc
+++ b/zircon/system/ulib/ftl/test/ndm_driver_test.cc
@@ -56,74 +56,79 @@
return empty_;
}
-TEST(NdmDriverTest, CheckPageEccError) {
+class NdmDriverTest : public ::testing::Test {
+ public:
+ void SetUp() override {
+ driver.GetNdmDriver(&ndm);
+ ASSERT_NE(nullptr, ndm.data_and_spare_check);
+ ASSERT_NE(nullptr, ndm.read_decode_spare);
+ ASSERT_NE(nullptr, ndm.read_spare);
+
+ if (!(ndm.flags & FSF_FREE_SPARE_ECC)) {
+ // The current NDM driver has "free" decoding of the spare area, so we use the same callback
+ // for read_spare and read_decode_spare and set FSF_FREE_SPARE_ECC. If this flag is ever
+ // unset, these callbacks should be updated to prevent any potential read amplification.
+ ASSERT_NE(ndm.read_spare, ndm.read_decode_spare)
+ << "read_spare and read_decode_spare should have different callbacks if "
+ "FSF_FREE_SPARE_ECC is unset (see NdmBaseDriver::FillNdmDriver for details).";
+ }
+ }
+
MockDriver driver;
-
NDMDrvr ndm;
- driver.GetNdmDriver(&ndm);
- ASSERT_NE(nullptr, ndm.data_and_spare_check);
+};
+TEST_F(NdmDriverTest, CheckPageEccError) {
driver.set_result(ftl::kNdmUncorrectableEcc);
-
- int status;
+ int status = -1;
EXPECT_EQ(ftl::kNdmOk, ndm.data_and_spare_check(0, nullptr, nullptr, &status, &driver));
EXPECT_EQ(NDM_PAGE_INVALID, status);
}
-TEST(NdmDriverTest, CheckPageFatalError) {
- MockDriver driver;
-
- NDMDrvr ndm;
- driver.GetNdmDriver(&ndm);
- ASSERT_NE(nullptr, ndm.data_and_spare_check);
-
+TEST_F(NdmDriverTest, CheckPageFatalError) {
driver.set_result(ftl::kNdmFatalError);
-
- int status;
- EXPECT_EQ(ftl::kNdmOk, ndm.data_and_spare_check(0, nullptr, nullptr, &status, &driver));
+ int status = -1;
+ EXPECT_EQ(ftl::kNdmFatalError, ndm.data_and_spare_check(0, nullptr, nullptr, &status, &driver));
+ // Status should not be used in this case, but we check that it was populated regardless to avoid
+ // any erroneous misinterpretation of the original value.
EXPECT_EQ(NDM_PAGE_INVALID, status);
}
-TEST(NdmDriverTest, CheckPageEmpty) {
- MockDriver driver;
-
- NDMDrvr ndm;
- driver.GetNdmDriver(&ndm);
- ASSERT_NE(nullptr, ndm.data_and_spare_check);
-
- int status;
+TEST_F(NdmDriverTest, CheckPageEmpty) {
+ int status = -1;
EXPECT_EQ(ftl::kNdmOk, ndm.data_and_spare_check(0, nullptr, nullptr, &status, &driver));
EXPECT_EQ(NDM_PAGE_ERASED, status);
}
-TEST(NdmDriverTest, CheckPageValid) {
- MockDriver driver;
-
- NDMDrvr ndm;
- driver.GetNdmDriver(&ndm);
- ASSERT_NE(nullptr, ndm.data_and_spare_check);
-
+TEST_F(NdmDriverTest, CheckPageValid) {
driver.set_result(ftl::kNdmUnsafeEcc);
driver.set_empty(false);
-
- int status;
+ int status = -1;
EXPECT_EQ(ftl::kNdmOk, ndm.data_and_spare_check(0, nullptr, nullptr, &status, &driver));
EXPECT_EQ(NDM_PAGE_VALID, status);
}
-TEST(NdmDriverTest, CheckPageValidIncompleteWrite) {
- MockDriver driver;
-
- NDMDrvr ndm;
- driver.GetNdmDriver(&ndm);
- ASSERT_NE(nullptr, ndm.data_and_spare_check);
-
+TEST_F(NdmDriverTest, CheckPageValidIncompleteWrite) {
driver.set_result(ftl::kNdmUnsafeEcc);
driver.set_incomplete(true);
-
- int status;
+ int status = -1;
EXPECT_EQ(ftl::kNdmOk, ndm.data_and_spare_check(0, nullptr, nullptr, &status, &driver));
EXPECT_EQ(NDM_PAGE_INVALID, status);
}
+TEST_F(NdmDriverTest, ReadSpareFatalError) {
+ driver.set_result(ftl::kNdmFatalError);
+ EXPECT_EQ(ftl::kNdmFatalError, ndm.read_decode_spare(0, nullptr, &driver));
+}
+
+TEST_F(NdmDriverTest, ReadSpareEccError) {
+ driver.set_result(ftl::kNdmUncorrectableEcc);
+ EXPECT_EQ(ftl::kNdmError, ndm.read_decode_spare(0, nullptr, &driver));
+}
+
+TEST_F(NdmDriverTest, ReadSpareUnsafeEcc) {
+ driver.set_result(ftl::kNdmUnsafeEcc);
+ EXPECT_EQ(ftl::kNdmOk, ndm.read_decode_spare(0, nullptr, &driver));
+}
+
} // namespace
