[PATCH 1/4] mtd: nand: add accessors, macros for in-memory BBT
Brian Norris
computersforpeace at gmail.com
Tue Jul 30 20:40:13 EDT 2013
Hi Ezequiel,
On 07/30/2013 03:02 PM, Ezequiel Garcia wrote:
> Hi Brian,
>
> Here's my attempt at reviewing this patchset, given the recent
> discussion about needing more MTD reviewing.
>
> I don't have enough experience with the NAND core to say anything about this,
> but I noticed you're doing a bunch of related but not necessarily tied changes.
>
> IMHO, splitting this patch further might make reviewing a lot easier, see below.
At first I disagreed, but after re-reading my work, I agree in part. The
original code was quite ugly, but that doesn't excuse me for not making
a little effort to make it easier to review.
> On Tue, Jul 23, 2013 at 11:27:56PM -0700, Brian Norris wrote:
>> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
>> index 2672643..3f18776 100644
>> --- a/drivers/mtd/nand/nand_bbt.c
>> +++ b/drivers/mtd/nand/nand_bbt.c
>> @@ -71,6 +71,28 @@
>> #include <linux/export.h>
>> #include <linux/string.h>
>>
>> +#define BBT_BLOCK_GOOD 0x00
>> +#define BBT_BLOCK_WORN 0x01
>> +#define BBT_BLOCK_RESERVED 0x02
>> +#define BBT_BLOCK_FACTORY_BAD 0x03
>> +
>> +#define BBT_ENTRY_MASK 0x03
>> +#define BBT_ENTRY_SHIFT 2
>> +
>
> You can have one patch to change all the magic numbers into this nice
> macros.
>
> [...]
>
> And then another patch to use the new accesors.
I plan to still lump the macros + accessors together, since most of the
shifting and masking ugliness is very intertwined. It's easier (IMO) to
just reason about a hunk like this:
[combined hunk]
- this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
+ bbt_mark_entry(this, (offs << 2) + (act >> 1), BBT_BLOCK_RESERVED);
Than the next two (as if they are split to 2 patches):
[hunk for patch 1]
- this->bbt[offs + (act >> 3)] |= 0x2 << (act & 0x06);
+ this->bbt[offs + ((act >> 1) >> BBT_ENTRY_SHIFT)] |=
+ BBT_BLOCK_RESERVED << ((act >> 1) & BBT_ENTRY_MASK);
[hunk for patch 2]
- this->bbt[offs + ((act >> 1) >> BBT_ENTRY_SHIFT)] |=
- BBT_BLOCK_RESERVED << ((act >> 1) & BBT_ENTRY_MASK);
+ bbt_mark_entry(this, (offs << 2) + (act >> 1), BBT_BLOCK_RESERVED);
But I can split again, if the next series is still hard to review.
> [...]
>> int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt)
>> {
>> struct nand_chip *this = mtd->priv;
>> - int block;
>> - uint8_t res;
>> + int block, res;
>>
>> - /* Get block number * 2 */
>
> And then a third patch to make the block * 2 -> block change.
Yes, this one can be kept pretty separate, and it makes the intermediate
changes easier to read.
>> - block = (int)(offs >> (this->bbt_erase_shift - 1));
>> - res = (this->bbt[block >> 3] >> (block & 0x06)) & 0x03;
>> + block = (int)(offs >> this->bbt_erase_shift);
>> + res = bbt_get_entry(this, block);
>>
>> pr_debug("nand_isbad_bbt(): bbt info for offs 0x%08x: "
>> "(block %d) 0x%02x\n",
>> - (unsigned int)offs, block >> 1, res);
>> + (unsigned int)offs, block, res);
>>
>> - switch ((int)res) {
>> - case 0x00:
>> + switch (res) {
>
> Mmm.. and then if you are really paranoid (like me) you can
> make the uint8_t -> int type change in another patch.
I'll consider it, but I don't consider myself quite that paranoid ;)
>> + case BBT_BLOCK_GOOD:
>> return 0;
>> - case 0x01:
>> + case BBT_BLOCK_WORN:
>> return 1;
>> - case 0x02:
>> + case BBT_BLOCK_RESERVED:
>> return allowbbt ? 0 : 1;
>> }
>> return 1;
>
> Hope this is of any help!
Yes, thanks for the review! I'll send a split v2 series, and I hope it
will be more reviewable.
Brian
More information about the linux-mtd
mailing list