Re: [PATCH v5 0/3] mtd: use the full-id as the keyword for some nand chips

[Brian Norris <computersforpeace@gmail.com>]

On 03/14/2013 12:07 AM, Artem Bityutskiy wrote:
> On Wed, 2013-03-13 at 21:48 -0700, Brian Norris wrote:
>> On Wed, Mar 13, 2013 at 7:59 PM, Huang Shijie <b32955@freescale.com> wrote:
>>> As time goes on, we begin to meet the situation that we can not
>>> get enough information from some nand chips's id data.
>>> Take some Toshiba's nand chips for example.
>>> I have 4 Toshiba's nand chips in my hand:
>>>          TC58NVG2S0F, TC58NVG3S0F, TC58NVG5D2, TC58NVG6D2
>>>
>>> When we read these chips' datasheets, we will get the geometry of these chips:
>>>          TC58NVG2S0F : 4096 + 224
>>>          TC58NVG3S0F : 4096 + 232
>>>          TC58NVG5D2  : 8192 + 640
>>>          TC58NVG6D2  : 8192 + 640
>>>
>>> But we can not parse out the correct oob size for these chips from the id data.
>>> So it is time to add some new fields to the nand_flash_dev{},
>>> and update the detection mechanisms.
>>>
>>> v4 --> v4:
>>>          [1] remove the id_len field.
>>>          [2] based on Artem "mtd: nand: use more reasonable integer types"
>>>          [3] add more comments.
>>
>> Sorry for not posting this earlier, but why don't we want an id_len
>> field? NAND chips often only have a 5-byte or 6-byte ID "defined" in
>> their datasheet, and so the next few bytes aren't guaranteed to be any
>> particular value. Wouldn't we want to accommodate for any potential
>> variation in the "undefined" bytes? (These bytes do typically have a
>> pattern, and in fact, we currently rely on that fact.) Also, I can
>> easily foresee a situation in which someone might want to support NAND
>> based solely on the datasheet, not waiting till they get a sample of
>> that chip in their hands. In that case, they cannot specify a full 8
>> bytes in the table; they can (and should) only specify the few
>> substring found in their datasheet.
>>
>> Really, the only argument I see for dropping id_len is to save space.
>> I think this is a bad choice.
>
> Let's take a flash which has 5 bytes ID length, suppose it is
> {1, 2, 3, 4, 5}. The 8-byte table value for this flash would be:
> {1, 2, 3, 4, 5, 0, 0, 0}. When we read the ID sequence, we need to start
> with allocating an 8-byte array and initializing it to 0. Then we read
> the 5-byte ID and end up with: {1, 2, 3, 4, 5, 0, 0, 0}. And we

When we "read the 5-byte ID", are you referring to reading the ID from 
the NAND flash? Unfortunately, things are *never* this nice. Those last 
3 bytes can be anything. Often they wrap around; see my nand_id_len() 
function. But they rarely are 0.

So, we have no way to identify (100% guaranteed) a 5-byte or 6-byte ID 
that comes from the flash. But we *can* compare a substring of it 
against 5 or 6 byte ID in our table, if they are marked with lengths.

> successfully match the table entry.
>
> That was my thinking.
>
> There is a potential problem: there may be a flash with 6 bytes ID with
> value {1, 2, 3, 4, 5, 0}, in which case the algorithm would not work.
>
> However, I consider this to be very unlikely. And even if this unlikely
> situation happens, it will probably be noticed and we could fix this
> with adding the ID length field.
>
> My rationale is avoiding over-designing and trying to cover low
> probability theoretical cases.

I agree about avoiding over-designing. I only would add that my 
scenario's are not so theoretical. It just happens that I (and others) 
have successfully managed to devise heuristics for most of the 5-byte 
and 6-byte IDs I've seen.

> But yes, I did not really strongly opposed the field, it was more that I
> asked questions from Huang about why is this needed, and expected to
> hear some justification. Huang preferred to answer that he does not
> really need this, and I just thought that if this is all he can say,
> then he should not add it.
>
> This is often, generally, the role of maintainer who cannot get into all
> the details - ask questions and validate the answers. Generally, good
> answers have correlation with quality code.

That is entirely reasonable.

> You did provide good arguments thanks! If my rationally is not
> convincing enough and you think this is not over-engineering, let's have
> the ID length field.

I do not think that it is over-engineering, but with the immediate needs 
(Huang's patch set), it is not necessary. I am OK with either result. 
It's not too difficult to add the field as needed.

> BTW, Huang, I think we should introduce a Macro instead of the '8'
> constant. And if ATM we do not have 8-byte ID's in our table, we should
> make the macro to contain a smaller number. This number can be increased
> when needed.

Sounds like a good idea. (But I don't expect that this will need increased.)

Brian