| Message ID | 20250508183733.514124-3-csokas.bence@prolan.hu |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mtd: Verify written data in paranoid mode | expand |
Hi Bence, kernel test robot noticed the following build warnings: [auto build test WARNING on d76bb1ebb5587f66b0f8b8099bfbb44722bc08b3] url: https://github.com/intel-lab-lkp/linux/commits/Bence-Cs-k-s/mtd-Verify-written-data-in-paranoid-mode/20250509-024044 base: d76bb1ebb5587f66b0f8b8099bfbb44722bc08b3 patch link: https://lore.kernel.org/r/20250508183733.514124-3-csokas.bence%40prolan.hu patch subject: [PATCH v2] mtd: Verify written data in paranoid mode config: i386-buildonly-randconfig-002-20250509 (https://download.01.org/0day-ci/archive/20250509/202505091318.0QvCicb9-lkp@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250509/202505091318.0QvCicb9-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202505091318.0QvCicb9-lkp@intel.com/ All warnings (new ones prefixed by >>): >> drivers/mtd/mtdcore.c:1775:12: warning: '_mtd_verify' defined but not used [-Wunused-function] 1775 | static int _mtd_verify(struct mtd_info *mtd, loff_t to, size_t len, const u8 *buf) | ^~~~~~~~~~~ vim +/_mtd_verify +1775 drivers/mtd/mtdcore.c 1774 > 1775 static int _mtd_verify(struct mtd_info *mtd, loff_t to, size_t len, const u8 *buf) 1776 { 1777 struct device *dev = &mtd->dev; 1778 u_char *verify_buf; 1779 size_t r_retlen; 1780 int ret; 1781 1782 verify_buf = devm_kmalloc(dev, len, GFP_KERNEL); 1783 if (!verify_buf) 1784 return -ENOMEM; 1785 1786 ret = mtd_read(mtd, to, len, &r_retlen, verify_buf); 1787 if (ret < 0) 1788 goto err; 1789 1790 if (len != r_retlen) { 1791 /* We shouldn't see short reads */ 1792 dev_err(dev, "Verify failed, written %zd but only read %zd", 1793 len, r_retlen); 1794 ret = -EIO; 1795 goto err; 1796 } 1797 1798 if (memcmp(verify_buf, buf, len)) { 1799 dev_err(dev, "Verify failed, compare mismatch!"); 1800 ret = -EIO; 1801 } 1802 1803 err: 1804 devm_kfree(dev, verify_buf); 1805 return ret; 1806 } 1807
Hello, On 08/05/2025 at 20:37:34 +02, Bence Csókás <csokas.bence@prolan.hu> wrote: > From: Csókás, Bence <csokas.bence@prolan.hu> > > Add MTD_PARANOID config option for verifying all written data to prevent > silent bit errors being undetected, at the cost of some bandwidth overhead. > > Signed-off-by: Csókás, Bence <csokas.bence@prolan.hu> > --- ... > +int mtd_write_oob(struct mtd_info *mtd, loff_t to, > + struct mtd_oob_ops *ops) > +{ > + int ret = _mtd_write_oob(mtd, to, ops); > + > +#if IS_ENABLED(CONFIG_MTD_PARANOID) > + if (ret < 0) > + return ret; > + > + ret = _mtd_verify(mtd, to, ops->retlen, ops->datbuf); > +#endif // CONFIG_MTD_PARANOID > + return ret; > +} I'd prefer to extend mtd_write_oob() with a simple if (IS_ENABLED(CONFIG_MTD_PARANOID)) mtd_verify(); It will also likely solve the kernel test robot report. Thanks, Miquèl
diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig index 796a2eccbef0..e75f4a57df6a 100644 --- a/drivers/mtd/Kconfig +++ b/drivers/mtd/Kconfig @@ -206,6 +206,20 @@ config MTD_PARTITIONED_MASTER the parent of the partition device be the master device, rather than what lies behind the master. +config MTD_PARANOID + bool "Read back written data (paranoid mode)" + help + This option makes the MTD core read back all data on a write and + report an error if it doesn't match the written data. This can + safeguard against silent bit errors resulting from a faulty Flash, + controller oddities, bus noise etc. + + It is up to the layer above MTD (e.g. the filesystem) to handle + this condition, for example by going read-only to prevent further + data corruption, or to mark a certain region of Flash as bad. + + If you are unsure, select 'n'. + source "drivers/mtd/chips/Kconfig" source "drivers/mtd/maps/Kconfig" diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c index 5ba9a741f5ac..139cbac51132 100644 --- a/drivers/mtd/mtdcore.c +++ b/drivers/mtd/mtdcore.c @@ -1745,8 +1745,8 @@ int mtd_read_oob(struct mtd_info *mtd, loff_t from, struct mtd_oob_ops *ops) } EXPORT_SYMBOL_GPL(mtd_read_oob); -int mtd_write_oob(struct mtd_info *mtd, loff_t to, - struct mtd_oob_ops *ops) +static int _mtd_write_oob(struct mtd_info *mtd, loff_t to, + struct mtd_oob_ops *ops) { struct mtd_info *master = mtd_get_master(mtd); int ret; @@ -1771,6 +1771,53 @@ int mtd_write_oob(struct mtd_info *mtd, loff_t to, return mtd_write_oob_std(mtd, to, ops); } + +static int _mtd_verify(struct mtd_info *mtd, loff_t to, size_t len, const u8 *buf) +{ + struct device *dev = &mtd->dev; + u_char *verify_buf; + size_t r_retlen; + int ret; + + verify_buf = devm_kmalloc(dev, len, GFP_KERNEL); + if (!verify_buf) + return -ENOMEM; + + ret = mtd_read(mtd, to, len, &r_retlen, verify_buf); + if (ret < 0) + goto err; + + if (len != r_retlen) { + /* We shouldn't see short reads */ + dev_err(dev, "Verify failed, written %zd but only read %zd", + len, r_retlen); + ret = -EIO; + goto err; + } + + if (memcmp(verify_buf, buf, len)) { + dev_err(dev, "Verify failed, compare mismatch!"); + ret = -EIO; + } + +err: + devm_kfree(dev, verify_buf); + return ret; +} + +int mtd_write_oob(struct mtd_info *mtd, loff_t to, + struct mtd_oob_ops *ops) +{ + int ret = _mtd_write_oob(mtd, to, ops); + +#if IS_ENABLED(CONFIG_MTD_PARANOID) + if (ret < 0) + return ret; + + ret = _mtd_verify(mtd, to, ops->retlen, ops->datbuf); +#endif // CONFIG_MTD_PARANOID + return ret; +} EXPORT_SYMBOL_GPL(mtd_write_oob); /**