[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <732d629c-d1b9-4483-95c3-9a80bd7b2511@huaweicloud.com>
Date: Tue, 2 Sep 2025 17:03:54 +0800
From: Wang Zhaolong <wangzhaolong@...weicloud.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
Cc: richard@....at, vigneshr@...com, linux-mtd@...ts.infradead.org,
linux-kernel@...r.kernel.org, chengzhihao1@...wei.com, yi.zhang@...wei.com,
yangerkun@...wei.com
Subject: Re: [PATCH] mtd: core: only increment ecc_stats.badblocks on
confirmed good->bad transition
Hi Miquèl,
> Hello Wang,
>
>> @@ -2349,17 +2351,35 @@ int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs)
>> return -EROFS;
>>
>> if (mtd->flags & MTD_SLC_ON_MLC_EMULATION)
>> ofs = (loff_t)mtd_div_by_eb(ofs, mtd) * master->erasesize;
>>
>> - ret = master->_block_markbad(master, mtd_get_master_ofs(mtd, ofs));
>> + moffs = mtd_get_master_ofs(mtd, ofs);
>> +
>> + /* Pre-check: remember current state if available. */
>> + if (master->_block_isbad)
>> + oldbad = master->_block_isbad(master, moffs);
>> +
>> + ret = master->_block_markbad(master, moffs);
>> if (ret)
>> return ret;
>>
>> - while (mtd->parent) {
>> - mtd->ecc_stats.badblocks++;
>> - mtd = mtd->parent;
>> + /*
>> + * Post-check and bump stats only on a confirmed good->bad transition.
>> + * If _block_isbad is not implemented, be conservative and do not bump.
>> + */
>> + if (master->_block_isbad) {
>> + /* If it was already bad, nothing to do. */
>> + if (oldbad > 0)
>> + return 0;
>> +
>> + if (master->_block_isbad(master, moffs) > 0) {
>> + while (mtd->parent) {
>> + mtd->ecc_stats.badblocks++;
>> + mtd = mtd->parent;
>> + }
>> + }
>
> I don't think you can assume the block was already bad and must not be
> accounted as a new bad block if you cannot verify that. In this case we
> must remain conservative and tell userspace a new block was marked bad,
> I believe.
Thanks for the review and the clear guidance.
My intent was to avoid inflating badblocks by only bumping the counter on a
confirmed good->bad transition. I tried to ground the decision on observable
state (pre/post isbad) rather than return codes, and to avoid assuming success
implies immediate visibility across drivers. That’s why the increment was
tied to verification and skipped when _block_isbad was unavailable.
Your point about being conservative towards userspace is well taken: if we
cannot verify, we should still account the bad block. Skipping the increment
only when we can prove the block was already bad makes the contract clearer
and avoids under-reporting. Keeping the increment outside the if (_block_isbad)
block also results in simpler, more readable code.
> Said otherwise, the { while () badblocks++ } block shall remain outside
> of the if (_block_isbad) condition and remain untouched. Just bail out
> early if you are sure this is not needed.
>
I’ll send a V2 shortly that:
- Checks old state when _block_isbad exists and bails out early if already bad
- Otherwise calls ->_block_markbad() and increments the counter on success, with
the increment left outside of the conditional as you suggested
Thanks again for the direction.
Best regards,
Wang Zhaolong
Powered by blists - more mailing lists