Re: [RFC][PATCH] Add NAND lock/unlock routines

[Artem Bityutskiy <dedekind1@gmail.com>]

On Thu, 2009-12-17 at 15:11 +0530, Vimal Singh wrote:
> On Mon, Dec 7, 2009 at 4:58 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > On Mon, 2009-12-07 at 16:36 +0530, Vimal Singh wrote:
> >> On Mon, Dec 7, 2009 at 2:59 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> > On Mon, 2009-12-07 at 12:18 +0530, Vimal Singh wrote:
> >> >> On Fri, Dec 4, 2009 at 2:08 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> >> >> > Hi, some cosmetic comments:
> >> >> >
> >> >> > On Wed, 2009-12-02 at 19:54 +0530, Vimal Singh wrote:
> >> >> >> I am not sure how useful it will be, but still here is a patch for review.
> >> >> >> -vimal
> >> >> >>
> >> >> >> From: Vimal Singh <vimalsingh@ti.com>
> >> >> >> Date: Tue, 24 Nov 2009 18:26:43 +0530
> >> >> >> Subject: [PATCH] Add NAND lock/unlock routines
> >> >> >>
> >> >> >> At least 'Micron' NAND parts have lock/unlock feature.
> >> >> >> Adding routines for this.
> >> >> >>
> >> >> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com>
> >> >> >> ---
> >> >> >>  drivers/mtd/nand/nand_base.c |  217 +++++++++++++++++++++++++++++++++++++++++-
> >> >> >>  include/linux/mtd/nand.h     |    6 +
> >> >> >>  2 files changed, 221 insertions(+), 2 deletions(-)
> >> >> >>
> >> >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> >> >> index 2957cc7..e447c24 100644
> >> >> >> --- a/drivers/mtd/nand/nand_base.c
> >> >> >> +++ b/drivers/mtd/nand/nand_base.c
> >> >> >> @@ -757,6 +757,218 @@ static int nand_wait(struct mtd_info *mtd,
> >> >> >> struct nand_chip *chip)
> >> >> >>  }
> >> >> >>
> >> >> >>  /**
> >> >> >> + * __nand_unlock - [REPLACABLE] unlocks specified locked blockes
> >> >> >> + *
> >> >> >> + * @param mtd - mtd info
> >> >> >> + * @param ofs - offset to start unlock from
> >> >> >> + * @param len - length to unlock
> >> >> >> + * @invert -  when = 0, unlock the range of blocks within the lower and
> >> >> >> + *                      upper boundary address
> >> >> >> + *            whne = 1, unlock the range of blocks outside the boundaries
> >> >> >> + *                      of the lower and upper boundary address
> >> >> >> + *
> >> >> >> + * @return - unlock status
> >> >> >> + */
> >> >> >> +static int __nand_unlock(struct mtd_info *mtd, loff_t ofs,
> >> >> >> +                                     uint64_t len, int invert)
> >> >> >> +{
> >> >> >> +     int ret = 0;
> >> >> >> +     int status, page;
> >> >> >> +     struct nand_chip *chip = mtd->priv;
> >> >> >> +
> >> >> >> +     DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n",
> >> >> >> +                     __func__, (unsigned long long)ofs, len);
> >> >> >> +
> >> >> >> +     /* Submit address of first page to unlock */
> >> >> >> +     page = (int)(ofs >> chip->page_shift);
> >> >> >
> >> >> > The compiler will automatically cast the result to int I believe.
> >> >>
> >> >> I just copied this line from erase functions.
> >> >> I believe its better to cast here as otherwise we may see compiler warnings.
> >> >
> >> > Good point. Could you please create a validation checking helper instead
> >> > of duplicating code?
> >>
> >> IMHO that should be done in a separate patch.
> >
> > Right, you can first send this as a separate patch, and then the rest as
> > a follow up one.
> 
> when I went back on validation checking's I notice:
> 
> -nand_read: does validation for access to past end of the device
> -nand_do_read_oob: does it for access to past oob and device.
> -nand_read_oob: does for access to past end of the device
> 
> -nand_write: does it for access to past end of the device
> -nand_do_write_ops: does it for page alignment
> -nand_do_write_oob: does it for access to past oob and device and page
> alignment
> -panic_nand_write: does it for access to past end of the device
> 
> -nand_erase_nand: does it for access to past end of the device and
> block alignment 'lock/unlock' routines are doing same validations as
> 'nand_erase_nand' does.
> 
> There is no consistancy in validation checks other than between
> 'erase' and 'lock/unlock'.
> Now since currently only 'erase' function does those validations. We
> can have patch for separate validation functions only after
> 'lock/unlock' patch.
> 
> Any comment or suggestions?

Well, of course my suggestion is to make that as consistent as possible,
and send a series of patches.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)