[3/4] mtd: Introduce mtd_block_isreserved()

In addition to mtd_block_isbad(), which checks if a block is bad or reserved,
it's needed to check if a block is reserved only (but not bad). This commit
adds an MTD interface for it, in a similar fashion to mtd_block_isbad().

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
---
 drivers/mtd/mtdcore.c        | 10 ++++++++++
 drivers/mtd/mtdpart.c        |  9 +++++++++
 drivers/mtd/nand/nand_base.c |  1 +
 drivers/mtd/nand/nand_bbt.c  | 14 ++++++++++++++
 include/linux/mtd/mtd.h      |  2 ++
 include/linux/mtd/nand.h     |  1 +
 6 files changed, 37 insertions(+)

On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
> In addition to mtd_block_isbad(), which checks if a block is bad or reserved,
> it's needed to check if a block is reserved only (but not bad). This commit
> adds an MTD interface for it, in a similar fashion to mtd_block_isbad().
> 
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> ---
>  drivers/mtd/mtdcore.c        | 10 ++++++++++
>  drivers/mtd/mtdpart.c        |  9 +++++++++
>  drivers/mtd/nand/nand_base.c |  1 +
>  drivers/mtd/nand/nand_bbt.c  | 14 ++++++++++++++
>  include/linux/mtd/mtd.h      |  2 ++
>  include/linux/mtd/nand.h     |  1 +
>  6 files changed, 37 insertions(+)
> 
> diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c
> index c5e9943..d65c5dc 100644
> --- a/drivers/mtd/mtdcore.c
> +++ b/drivers/mtd/mtdcore.c
> @@ -1012,6 +1012,16 @@ int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len)
>  }
>  EXPORT_SYMBOL_GPL(mtd_is_locked);
>  
> +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> +{
> +	if (!mtd->_block_isreserved)
> +		return 0;
> +	if (ofs < 0 || ofs > mtd->size)
> +		return -EINVAL;

At first, I was going to recommend that the out-of-bounds check go
before the !mtd->_block_isreserved check, since it's best to warn users
for invalid input. But then, mtd_block_isbad() has the same ordering, so
it'd be nice to consistent...

Do we flip a coin to decide whether to change both or leave as-is? :)

> +	return mtd->_block_isreserved(mtd, ofs);
> +}
> +EXPORT_SYMBOL_GPL(mtd_block_isreserved);
> +
>  int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs)
>  {
>  	if (!mtd->_block_isbad)
> diff --git a/drivers/mtd/mtdpart.c b/drivers/mtd/mtdpart.c
> index 1ca9aec..921e8c6 100644
> --- a/drivers/mtd/mtdpart.c
> +++ b/drivers/mtd/mtdpart.c
> @@ -290,6 +290,13 @@ static void part_resume(struct mtd_info *mtd)
>  	part->master->_resume(part->master);
>  }
>  
> +static int part_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> +{
> +	struct mtd_part *part = PART(mtd);
> +	ofs += part->offset;
> +	return part->master->_block_isreserved(part->master, ofs);
> +}
> +
>  static int part_block_isbad(struct mtd_info *mtd, loff_t ofs)
>  {
>  	struct mtd_part *part = PART(mtd);
> @@ -422,6 +429,8 @@ static struct mtd_part *allocate_partition(struct mtd_info *master,
>  		slave->mtd._unlock = part_unlock;
>  	if (master->_is_locked)
>  		slave->mtd._is_locked = part_is_locked;
> +	if (master->_block_isreserved)
> +		slave->mtd._block_isreserved = part_block_isreserved;
>  	if (master->_block_isbad)
>  		slave->mtd._block_isbad = part_block_isbad;
>  	if (master->_block_markbad)
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 5826da3..58f5884 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4044,6 +4044,7 @@ int nand_scan_tail(struct mtd_info *mtd)
>  	mtd->_unlock = NULL;
>  	mtd->_suspend = nand_suspend;
>  	mtd->_resume = nand_resume;
> +	mtd->_block_isreserved = nand_isreserved_bbt;

I think you want a small wrapper function in case we aren't using a BBT
at all. See nand_block_checkbad(), which checks for !chip->bbt. So we'd
need:

	if (!chip->bbt)
		return 0;

>  	mtd->_block_isbad = nand_block_isbad;
>  	mtd->_block_markbad = nand_block_markbad;
>  	mtd->writebufsize = mtd->writesize;
> diff --git a/drivers/mtd/nand/nand_bbt.c b/drivers/mtd/nand/nand_bbt.c
> index ea9a266..fd21169 100644
> --- a/drivers/mtd/nand/nand_bbt.c
> +++ b/drivers/mtd/nand/nand_bbt.c
> @@ -1312,6 +1312,20 @@ int nand_default_bbt(struct mtd_info *mtd)
>  }
>  
>  /**
> + * nand_isreserved_bbt - [NAND Interface] Check if a block is reserved
> + * @mtd: MTD device structure
> + * @offs: offset in the device
> + */
> +int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs)
> +{
> +	struct nand_chip *this = mtd->priv;
> +	int block;
> +
> +	block = (int)(offs >> this->bbt_erase_shift);
> +	return bbt_get_entry(this, block) == BBT_BLOCK_RESERVED;
> +}
> +
> +/**
>   * nand_isbad_bbt - [NAND Interface] Check if a block is bad
>   * @mtd: MTD device structure
>   * @offs: offset in the device
> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index a1b0b4c..031ff3a 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -222,6 +222,7 @@ struct mtd_info {
>  	int (*_lock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  	int (*_unlock) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  	int (*_is_locked) (struct mtd_info *mtd, loff_t ofs, uint64_t len);
> +	int (*_block_isreserved) (struct mtd_info *mtd, loff_t ofs);
>  	int (*_block_isbad) (struct mtd_info *mtd, loff_t ofs);
>  	int (*_block_markbad) (struct mtd_info *mtd, loff_t ofs);
>  	int (*_suspend) (struct mtd_info *mtd);
> @@ -302,6 +303,7 @@ static inline void mtd_sync(struct mtd_info *mtd)
>  int mtd_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  int mtd_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>  int mtd_is_locked(struct mtd_info *mtd, loff_t ofs, uint64_t len);
> +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs);
>  int mtd_block_isbad(struct mtd_info *mtd, loff_t ofs);
>  int mtd_block_markbad(struct mtd_info *mtd, loff_t ofs);
>  
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 0747fef..3b78c6b 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -806,6 +806,7 @@ extern struct nand_manufacturers nand_manuf_ids[];
>  extern int nand_scan_bbt(struct mtd_info *mtd, struct nand_bbt_descr *bd);
>  extern int nand_default_bbt(struct mtd_info *mtd);
>  extern int nand_markbad_bbt(struct mtd_info *mtd, loff_t offs);
> +extern int nand_isreserved_bbt(struct mtd_info *mtd, loff_t offs);
>  extern int nand_isbad_bbt(struct mtd_info *mtd, loff_t offs, int allowbbt);
>  extern int nand_erase_nand(struct mtd_info *mtd, struct erase_info *instr,
>  			   int allowbbt);

Otherwise, looks good.

Brian

On 12 May 06:31 PM, Brian Norris wrote:
> On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
[..]
> >  
> > +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > +{
> > +	if (!mtd->_block_isreserved)
> > +		return 0;
> > +	if (ofs < 0 || ofs > mtd->size)
> > +		return -EINVAL;
> 
> At first, I was going to recommend that the out-of-bounds check go
> before the !mtd->_block_isreserved check, since it's best to warn users
> for invalid input. But then, mtd_block_isbad() has the same ordering, so
> it'd be nice to consistent...
> 
> Do we flip a coin to decide whether to change both or leave as-is? :)
> 

Actually, I just followed the same convention as all the other functions,
not just mtd_block_isbad().

I'll add a patch changing them all so the parameters checking is done first.

On Wed, May 14, 2014 at 08:37:21PM -0300, Ezequiel Garcia wrote:
> On 12 May 06:31 PM, Brian Norris wrote:
> > On Fri, Mar 21, 2014 at 08:57:43AM -0300, Ezequiel Garcia wrote:
> [..]
> > >  
> > > +int mtd_block_isreserved(struct mtd_info *mtd, loff_t ofs)
> > > +{
> > > +	if (!mtd->_block_isreserved)
> > > +		return 0;
> > > +	if (ofs < 0 || ofs > mtd->size)
> > > +		return -EINVAL;
> > 
> > At first, I was going to recommend that the out-of-bounds check go
> > before the !mtd->_block_isreserved check, since it's best to warn users
> > for invalid input. But then, mtd_block_isbad() has the same ordering, so
> > it'd be nice to consistent...
> > 
> > Do we flip a coin to decide whether to change both or leave as-is? :)
> > 
> 
> Actually, I just followed the same convention as all the other functions,
> not just mtd_block_isbad().

All? Like what? mtd_read_oob()? mtd_get_fact_prot_info()? These return
-EOPNOTSUPP, which is an informative error code. But that's different
than returning 0 for mtd_block_isbad() or mtd_block_isreserved(), even
if the block doesn't exist.

> I'll add a patch changing them all so the parameters checking is done first.

Can you mention which ones you think are problematic before adding
another patch? I'd be careful on playing with error codes for APIs that
are already have reasonable enough error codes. AFAICT,
mtd_block_isbad() (and your mtd_block_isreserved()) are the only
problems.

(Also, is it just me, or is mtd_writev() missing bounds checking?)

Brian