Message-ID: <CAN8TOE8uAi3=4MOG76Y7atAR1YjmssmXmOgu7Y=T8EjQtNx2Cw@mail.gmail.com>
Date:	Tue, 23 Apr 2013 00:06:59 -0700
From:	Brian Norris <computersforpeace@...il.com>
To:	Huang Shijie <b32955@...escale.com>
Cc:	dwmw2@...radead.org, dedekind1@...il.com,
	linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/8] mtd: get the ECC info from the Extended Parameter Page

On Mon, Apr 22, 2013 at 7:43 PM, Huang Shijie <b32955@...escale.com> wrote:
> 于 2013年04月23日 05:22, Brian Norris 写道:
>
>> On Mon, Apr 22, 2013 at 12:47 AM, Huang Shijie<b32955@...escale.com>
>> wrote:
>>>
>>> Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page
>>> to store the ECC info.
>>>
>>> The onfi spec tells us that if the nand chip's recommended ECC codeword
>>> size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then
>>> read the Extended ECC information that is part of the extended parameter
>>> page to retrieve the ECC requirements for this device.
>>>
>>> This patch implement the reading of the Extended Parameter Page, and
>>> parses
>>> the sections for ECC type, and get the ECC info from the ECC section.
>>>
>>> Tested this patch with Micron MT29F64G08CBABAWP.
>>>
>>> Signed-off-by: Huang Shijie<b32955@...escale.com>
>>> ---
>>>   drivers/mtd/nand/nand_base.c |   54
>>> ++++++++++++++++++++++++++++++++++++++++++
>>>   1 files changed, 54 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>>> index beff911..48ff097 100644
>>> --- a/drivers/mtd/nand/nand_base.c
>>> +++ b/drivers/mtd/nand/nand_base.c
>>> @@ -2846,6 +2846,56 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t
>>> len)
>>>          return crc;
>>>   }
>>>
>>> +/* Parse the Extended Parameter Page. */
>>> +static void nand_flash_detect_ext_param_page(struct mtd_info *mtd,
>>> +               struct nand_chip *chip, struct nand_onfi_params *p, int
>>> last)
>>> +{
>>
>> I think we want a return code (int) for this function. It obviously
>> can fail, and the caller needs to know this.
>
> I not sure who will uses ecc_strength/ecc_size, except the gpmi.
> So i ignore the return value.

Well, that's the dangerous part of this patch series: that it is
written entirely from the point of view of a specific use case (gpmi).
It can be improved a little to make it more generically useful.

> If you think we should add it, i will add it.

Well, the code doesn't clearly show what happens to
ecc_strength/ecc_size if (1) we are out of memory (but we are hosed in
this case anyway...) or (2) the extended parameter page is corrupted
and/or not present. In either case, the caller may want to zero out
the the parameters, set them to -1, or whatever else we decide for the
null case.

>> The "last" parameter is not very obvious until you read the whole
>> function, where you see that this function assumes a lot about the
>> caller. Please address the comments below and/or fully document the
>> parameters and calling context for this function.
>>
> ok. I can add more comments.
>
>
>>> +       struct onfi_ext_param_page *ep;
>>> +       struct onfi_ext_section *s;
>>> +       struct onfi_ext_ecc_info *ecc;
>>> +       uint8_t *cursor;
>>> +       int len;
>>> +       int i;
>>> +
>>> +       len = le16_to_cpu(p->ext_param_page_length) * 16;
>>> +       ep = kcalloc(1, max_t(int, len, sizeof(*p)), GFP_KERNEL);

Does this need to be zeroed out? We will overwrite it before using it
anyway. I'd just recommend kmalloc.

>>> +       if (!ep)
>>> +               goto ext_out;
>>> +
>>> +       /*
>>> +        * Skip the ONFI Parameter Pages.
>>> +        * The Change Read Columm command may does not works here.
>>
>> Why not?
>>
> You can give me a fix patch which bases on my patch set.
> I can test it. :)
>
> I tried to use the Change-read-columm command, but failed.

I believe the behavior here will really depend on the driver used. My
driver, for instance, intercepts these commands and sends them to the
controller in a different form. So my patches won't necessarily help
you if your driver is broken :)

>>> +        */
>>> +       for (i = last + 1; i<  p->num_of_param_pages; i++)
>>> +               chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p));
>>
>> You never sent a command to the chip. How can you expect to read from it?
>
> we have sent a command in the nand_flash_detect_onfi().

Right, I understand. But it's not very clean code if it makes this
assumption w/o documenting it.

>> It seems that you are writing this function with the assumption of a
>> particular calling context (a context in which the last command was
>> CMD_PARAM). IMO, it would make a lot more sense that this function
>> actually send its own CMD_PARAM followed by either X bytes of skipped
>> read_buf() or a change read column command. Then it doesn't need the
>> "last" argument, and its usage makes much more sense.
>>
> I added the "last" argument just because the Change-read-column command did
> not works.

Brian
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists