答复: [PATCH] mtd: cmdlinepart: allow fill-up partition at any point

* 答复: [PATCH] mtd: cmdlinepart: allow fill-up partition at any point
       [not found] <C3050A4DBA34F345975765E43127F10F62D61353@SZXEMA512-MBX.china.huawei.com>
@ 2015-10-13  8:53 ` Yanjiantao
  2015-10-13 17:51   ` Brian Norris
  2015-10-13 11:33 ` questions about [PATCH]mtd: nand: fix SCAN2NDPAGE check for BBM Yanjiantao
  1 sibling, 1 reply; 6+ messages in thread
From: Yanjiantao @ 2015-10-13  8:53 UTC (permalink / raw)
  To: computersforpeace@gmail.com
  Cc: dmw2@infradead.org, linux-mtd@lists.infradead.org,
	linux-kernel@vger.kernel.org, Caizhiyong, pengjiancheng

Hi,
In linux-3.10 and later kernel versions, block 'markbad' or 'checkbad' method is to mark or check 1st, 2nd/last page, which depends on chip->bbt_options:
markbad flows is as follows:
                /* Write to first/last page(s) if necessary */
                if (chip->bbt_options & NAND_BBT_SCANLASTPAGE)
                        wr_ofs += mtd->erasesize - mtd->writesize;
                do {
                        res = nand_do_write_oob(mtd, wr_ofs, &ops);
                        if (!ret)
                                ret = res;

                        i++;
                        wr_ofs += mtd->writesize;
                } while ((chip->bbt_options & NAND_BBT_SCAN2NDPAGE) && i < 2);
Check bad flows is the same.

1. when NAND_BBT_SCANLASTPAGE is set, markbad and checkbad on the last page only.
2. when NAND_BBT_SCAN2NDPAGE is set, markbad and checkbad on the 1st and 2nd page.

Questions:
1. for some NAND Flash, badblock marker is on 1st or 2nd or last page(spansion), in this case , check badblock my fail.
2. for some NAND Flash, bad block marker is on 1st or last page(hynix H27UBG8T2CTR) , in this case, check badblock may fail too.
3. set NAND_BBT_SCANLASTPAGE means only mark or check the last page, which is not compliant with SAMSUNG/ TOSHIBA/ HYNIX/MICRON/spansion. they claim bad block marker is on 1st/2nd, or 1st/last, or 1st/2nd/last, or the 1st only. 

-----邮件原件-----
发件人: Caizhiyong 
发送时间: 2015年10月13日 15:35
收件人: Yanjiantao
主题: FW: [PATCH] mtd: cmdlinepart: allow fill-up partition at any point

Looking forward to your feedback.
Best regards.
Cai Zhiyong.
http://www.huawei.com

> -----Original Message-----
> From: Brian Norris [mailto:computersforpeace@gmail.com]
> Sent: Wednesday, September 30, 2015 3:56 AM
> To: Ben Shelton
> Cc: dmw2@infradead.org; linux-mtd@lists.infradead.org;
> linux-kernel@vger.kernel.org; Caizhiyong
> Subject: Re: [PATCH] mtd: cmdlinepart: allow fill-up partition at any point
> 
> Sorry for delay... a lot of things came on my plate, so things got
> buried.
> 
> I fixed up a few conflicts locally with some of my patches, but I have a
> few problems with your patch, so I'll have to request another revision.
> 
> On Thu, May 07, 2015 at 09:57:41PM -0500, Ben Shelton wrote:
> > Currently, a fill-up partition (indicated by '-') must be the last
> > partition, and no other partitions can go after it.  Change the
> > cmdlinepart parsing code to allow a fill-up partition at any point.
> > This is useful, for example, if you want to reserve a partition at the
> > end of the flash where the bad block table will go.
> >
> > Signed-off-by: Ben Shelton <ben.shelton@ni.com>
> 
> For one, I think we need a little update to the parser documentation
> (i.e., code comments at the top) to show at least an example of this new
> allowed syntax.
> 
> > ---
> >  drivers/mtd/cmdlinepart.c | 33 ++++++++++++++++++++++++++-------
> >  1 file changed, 26 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c
> > index c850300..2d0eda2 100644
> > --- a/drivers/mtd/cmdlinepart.c
> > +++ b/drivers/mtd/cmdlinepart.c
> > @@ -97,6 +97,7 @@ static struct mtd_partition * newpart(char *s,
> >  				      char **retptr,
> >  				      int *num_parts,
> >  				      int this_part,
> > +				      int size_remaining_found,
> >  				      unsigned char **extra_mem_ptr,
> >  				      int extra_mem_size)
> >  {
> > @@ -110,9 +111,16 @@ static struct mtd_partition * newpart(char *s,
> >
> >  	/* fetch the partition size */
> >  	if (*s == '-') {
> > +		if (size_remaining_found) {
> > +			printk(KERN_ERR ERRP
> 
> Trivial: we can use pr_err() now (see l2-mtd.git).
> 
> > +			       "more than one '-' partition specified\n");
> > +			return ERR_PTR(-EINVAL);
> > +		}
> > +
> >  		/* assign all remaining space to this partition */
> >  		size = SIZE_REMAINING;
> >  		s++;
> > +		size_remaining_found = 1;
> >  	} else {
> >  		size = memparse(s, &s);
> >  		if (size < PAGE_SIZE) {
> > @@ -169,13 +177,10 @@ static struct mtd_partition * newpart(char *s,
> >
> >  	/* test if more partitions are following */
> >  	if (*s == ',') {
> > -		if (size == SIZE_REMAINING) {
> > -			printk(KERN_ERR ERRP "no partitions allowed after a fill-up
> partition\n");
> > -			return ERR_PTR(-EINVAL);
> > -		}
> >  		/* more partitions follow, parse them */
> >  		parts = newpart(s + 1, &s, num_parts, this_part + 1,
> > -				&extra_mem, extra_mem_size);
> > +				size_remaining_found, &extra_mem,
> > +				extra_mem_size);
> >  		if (IS_ERR(parts))
> >  			return parts;
> >  	} else {
> > @@ -252,6 +257,7 @@ static int mtdpart_setup_real(char *s)
> >  				&s,		/* out: updated cmdline ptr */
> >  				&num_parts,	/* out: number of parts */
> >  				0,		/* first partition */
> > +				0,		/* size_remaining not found */
> >  				(unsigned char**)&this_mtd, /* out: extra mem */
> >  				mtd_id_len + 1 + sizeof(*this_mtd) +
> >  				sizeof(void*)-1 /*alignment*/);
> > @@ -313,6 +319,7 @@ static int parse_cmdline_partitions(struct mtd_info
> *master,
> >  	int i, err;
> >  	struct cmdline_mtd_partition *part;
> >  	const char *mtd_id = master->name;
> > +	int sr_part_num = -1;
> >
> >  	/* parse command line */
> >  	if (!cmdline_parsed) {
> > @@ -339,8 +346,10 @@ static int parse_cmdline_partitions(struct mtd_info
> *master,
> >  		else
> >  			offset = part->parts[i].offset;
> >
> > -		if (part->parts[i].size == SIZE_REMAINING)
> > -			part->parts[i].size = master->size - offset;
> > +		if (part->parts[i].size == SIZE_REMAINING) {
> > +			sr_part_num = i;
> > +			continue;
> > +		}
> 
> Here, you are dropping this property for fill partitions:
> 
>   "if specified or truncated size is 0 the part is skipped"
> 
> Since the size is no longer computed in this loop, it will never match
> the 'size == 0' check.
> 
> >
> >  		if (offset + part->parts[i].size > master->size) {
> >  			printk(KERN_WARNING ERRP
> > @@ -361,6 +370,16 @@ static int parse_cmdline_partitions(struct mtd_info
> *master,
> >  		}
> >  	}
> >
> > +	/* if a partition was marked as SIZE_REMAINING */
> > +	if (sr_part_num != -1) {
> > +		/* fix up the size of the SIZE_REMAINING partition */
> > +		part->parts[sr_part_num].size = master->size - offset;
> 
> The use of 'offset' doesn't look correct. At this point, 'offset' is the
> address of the end of the last partition. That looks like you're still
> sizing the fill partition to be only at the end of the flash, which is
> not the point of this patch. Am I missing something?
> 
> > +
> > +		/* fix up the offsets of the subsequent partitions */
> > +		for (i = (sr_part_num + 1); i < part->num_parts; i++)
> > +			part->parts[i].offset += part->parts[sr_part_num].size;
> 
> Is this a legal adjustment? Is it possible to have fixed address (not
> OFFSET_CONTINUOUS) partitions after the fill partition? I see that would
> actually make this kind of ambiguous. So maybe you need to specify this
> in the documentation and check for it in the parsing -- see that once
> size_remaining_found > 0, we don't allow any more partitions
> '@<address'> (i.e., not OFFSET_CONTINUOUS).
> 
> > +	}
> > +
> >  	*pparts = kmemdup(part->parts, sizeof(*part->parts) * part->num_parts,
> >  			  GFP_KERNEL);
> >  	if (!*pparts)
> 
> I think this patch speaks to our need for unit tests... that way we can
> clearly and definitively show pass/fail for any regressions.
> 
> This reminds me of Cai Zhiyong's attempts to unify this parser with the
> new block/cmdline-parser, without getting any test results for the
> variety of existing MTD use cases...
> 
> Brian

^ permalink raw reply	[flat|nested] 6+ messages in thread