| Message ID | ce9ab5791001130529l6f2ef9d5k79b6329a98baed54@mail.gmail.com |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, 2010-01-13 at 18:59 +0530, Vimal Singh wrote: > From 310f7faa8f319bd9384512f7d5a7f13dcfbeebc8 Mon Sep 17 00:00:00 2001 > From: Vimal Singh <vimalsingh@ti.com> > Date: Wed, 13 Jan 2010 18:11:47 +0530 > Subject: [PATCH] Creating helper func for block alignment verfication > > These checks are fairly common in 'nand_erase_nand', 'nand_lock' > and 'nand_unlock' functions. > > Signed-off-by: Vimal Singh <vimalsingh@ti.com> > --- > drivers/mtd/nand/nand_base.c | 97 +++++++++++++++--------------------------- > 1 files changed, 34 insertions(+), 63 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index 4e27426..c80cec5 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -108,6 +108,37 @@ static int nand_do_write_oob(struct > */ > DEFINE_LED_TRIGGER(nand_led_trigger); > > +static int block_alignment_verification(struct mtd_info *mtd, > + loff_t ofs, uint64_t len) > +{ This function checks not only alignment, so the name is bad. I suggest check_offs_len() - it at least does not lie about what it does :-) > + struct nand_chip *chip = mtd->priv; > + > + DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n", > + __func__, (unsigned long long)ofs, len); No, you should keep the DEBUG part in the caller. Because of __func__. Also please, introduce the helper in the _first_ patch, and then use it in your functions in the second patch. This is more logical.
On Thu, Jan 28, 2010 at 8:16 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > Hi, > > On Wed, 2010-01-13 at 18:59 +0530, Vimal Singh wrote: >> From 310f7faa8f319bd9384512f7d5a7f13dcfbeebc8 Mon Sep 17 00:00:00 2001 >> From: Vimal Singh <vimalsingh@ti.com> >> Date: Wed, 13 Jan 2010 18:11:47 +0530 >> Subject: [PATCH] Creating helper func for block alignment verfication >> >> These checks are fairly common in 'nand_erase_nand', 'nand_lock' >> and 'nand_unlock' functions. >> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com> >> --- >> drivers/mtd/nand/nand_base.c | 97 +++++++++++++++--------------------------- >> 1 files changed, 34 insertions(+), 63 deletions(-) >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index 4e27426..c80cec5 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -108,6 +108,37 @@ static int nand_do_write_oob(struct >> */ >> DEFINE_LED_TRIGGER(nand_led_trigger); >> >> +static int block_alignment_verification(struct mtd_info *mtd, >> + loff_t ofs, uint64_t len) >> +{ > > This function checks not only alignment, so the name is bad. I suggest > check_offs_len() - it at least does not lie about what it does :-) OK, no problem. > >> + struct nand_chip *chip = mtd->priv; >> + >> + DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n", >> + __func__, (unsigned long long)ofs, len); > > No, you should keep the DEBUG part in the caller. Because of __func__. Agree. > > Also please, introduce the helper in the _first_ patch, and then use it > in your functions in the second patch. This is more logical. Before 1st patch this helper will be called by just one function "nand_erase_nand". And then in that creating helper function does not makes sense to me. To me doing this in 2nd patch looks more logical. Either way we will achieve same goal only number of lines in patches will defer. So, if you still insist I can make it 1st patch.
On Fri, 2010-01-29 at 09:49 +0530, Vimal Singh wrote: > On Thu, Jan 28, 2010 at 8:16 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > > Hi, > > > > On Wed, 2010-01-13 at 18:59 +0530, Vimal Singh wrote: > >> From 310f7faa8f319bd9384512f7d5a7f13dcfbeebc8 Mon Sep 17 00:00:00 2001 > >> From: Vimal Singh <vimalsingh@ti.com> > >> Date: Wed, 13 Jan 2010 18:11:47 +0530 > >> Subject: [PATCH] Creating helper func for block alignment verfication > >> > >> These checks are fairly common in 'nand_erase_nand', 'nand_lock' > >> and 'nand_unlock' functions. > >> > >> Signed-off-by: Vimal Singh <vimalsingh@ti.com> > >> --- > >> drivers/mtd/nand/nand_base.c | 97 +++++++++++++++--------------------------- > >> 1 files changed, 34 insertions(+), 63 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > >> index 4e27426..c80cec5 100644 > >> --- a/drivers/mtd/nand/nand_base.c > >> +++ b/drivers/mtd/nand/nand_base.c > >> @@ -108,6 +108,37 @@ static int nand_do_write_oob(struct > >> */ > >> DEFINE_LED_TRIGGER(nand_led_trigger); > >> > >> +static int block_alignment_verification(struct mtd_info *mtd, > >> + loff_t ofs, uint64_t len) > >> +{ > > > > This function checks not only alignment, so the name is bad. I suggest > > check_offs_len() - it at least does not lie about what it does :-) > > OK, no problem. > > > > >> + struct nand_chip *chip = mtd->priv; > >> + > >> + DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n", > >> + __func__, (unsigned long long)ofs, len); > > > > No, you should keep the DEBUG part in the caller. Because of __func__. > > Agree. > > > > > Also please, introduce the helper in the _first_ patch, and then use it > > in your functions in the second patch. This is more logical. > > Before 1st patch this helper will be called by just one function > "nand_erase_nand". And then in that creating helper function does not > makes sense to me. It does. It will be a preparation to the next patch. > To me doing this in 2nd patch looks more logical. > > Either way we will achieve same goal only number of lines in patches will defer. > So, if you still insist I can make it 1st patch. OK, it is not a big deal.
On Fri, Jan 29, 2010 at 10:15 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Fri, 2010-01-29 at 09:49 +0530, Vimal Singh wrote: >> On Thu, Jan 28, 2010 at 8:16 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: >> > Hi, >> > >> > On Wed, 2010-01-13 at 18:59 +0530, Vimal Singh wrote: >> >> From 310f7faa8f319bd9384512f7d5a7f13dcfbeebc8 Mon Sep 17 00:00:00 2001 >> >> From: Vimal Singh <vimalsingh@ti.com> >> >> Date: Wed, 13 Jan 2010 18:11:47 +0530 >> >> Subject: [PATCH] Creating helper func for block alignment verfication >> >> >> >> These checks are fairly common in 'nand_erase_nand', 'nand_lock' >> >> and 'nand_unlock' functions. >> >> >> >> Signed-off-by: Vimal Singh <vimalsingh@ti.com> >> >> --- >> >> drivers/mtd/nand/nand_base.c | 97 +++++++++++++++--------------------------- >> >> 1 files changed, 34 insertions(+), 63 deletions(-) >> >> >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> >> index 4e27426..c80cec5 100644 >> >> --- a/drivers/mtd/nand/nand_base.c >> >> +++ b/drivers/mtd/nand/nand_base.c >> >> @@ -108,6 +108,37 @@ static int nand_do_write_oob(struct >> >> */ >> >> DEFINE_LED_TRIGGER(nand_led_trigger); >> >> >> >> +static int block_alignment_verification(struct mtd_info *mtd, >> >> + loff_t ofs, uint64_t len) >> >> +{ >> > >> > This function checks not only alignment, so the name is bad. I suggest >> > check_offs_len() - it at least does not lie about what it does :-) >> >> OK, no problem. >> >> > >> >> + struct nand_chip *chip = mtd->priv; >> >> + >> >> + DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n", >> >> + __func__, (unsigned long long)ofs, len); >> > >> > No, you should keep the DEBUG part in the caller. Because of __func__. >> >> Agree. >> >> > >> > Also please, introduce the helper in the _first_ patch, and then use it >> > in your functions in the second patch. This is more logical. >> >> Before 1st patch this helper will be called by just one function >> "nand_erase_nand". And then in that creating helper function does not >> makes sense to me. > > It does. It will be a preparation to the next patch. > >> To me doing this in 2nd patch looks more logical. >> >> Either way we will achieve same goal only number of lines in patches will defer. >> So, if you still insist I can make it 1st patch. > > OK, it is not a big deal. Well in that case I'll send this patch set by changing the patch order sometime today.
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index 4e27426..c80cec5 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -108,6 +108,37 @@ static int nand_do_write_oob(struct */ DEFINE_LED_TRIGGER(nand_led_trigger); +static int block_alignment_verification(struct mtd_info *mtd, + loff_t ofs, uint64_t len) +{ + struct nand_chip *chip = mtd->priv; + + DEBUG(MTD_DEBUG_LEVEL3, "%s: start = 0x%012llx, len = %llu\n", + __func__, (unsigned long long)ofs, len); + + /* Start address must align on block boundary */ + if (ofs & ((1 << chip->phys_erase_shift) - 1)) { + DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__); + ret = -EINVAL; + } + + /* Length must align on block boundary */ + if (len & ((1 << chip->phys_erase_shift) - 1)) { + DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n", + __func__); + ret = -EINVAL; + } + + /* Do not allow past end of device */ + if (ofs + len > mtd->size) { + DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n", + __func__); + ret = -EINVAL; + } + + return 0; +} + /** * nand_release_device - [GENERIC] release chip * @mtd: MTD device structure @@ -894,28 +925,8 @@ int nand_unlock(struct mtd_info int chipnr; struct nand_chip *chip = mtd->priv; - /* Start address must align on block boundary */ - if (ofs & ((1 << chip->phys_erase_shift) - 1)) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__); - ret = -EINVAL; - goto out; - } - - /* Length must align on block boundary */ - if (len & ((1 << chip->phys_erase_shift) - 1)) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n", - __func__); - ret = -EINVAL; + if (block_alignment_verification(mtd, ofs, len)) goto out; - } - - /* Do not allow past end of device */ - if (ofs + len > mtd->size) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n", - __func__); - ret = -EINVAL; - goto out; - } /* Align to last block address if size addresses end of the device */ if (ofs + len == mtd->size) @@ -968,31 +979,8 @@ int nand_lock(struct mtd_info int chipnr, 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); - - /* Start address must align on block boundary */ - if (ofs & ((1 << chip->phys_erase_shift) - 1)) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__); - ret = -EINVAL; - goto out; - } - - /* Length must align on block boundary */ - if (len & ((1 << chip->phys_erase_shift) - 1)) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n", - __func__); - ret = -EINVAL; + if (block_alignment_verification(mtd, ofs, len)) goto out; - } - - /* Do not allow past end of device */ - if (ofs + len > mtd->size) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Past end of device\n", - __func__); - ret = -EINVAL; - goto out; - } nand_get_device(chip, mtd, FL_LOCKING); @@ -2495,25 +2483,8 @@ int nand_erase_nand(struct __func__, (unsigned long long)instr->addr, (unsigned long long)instr->len); - /* Start address must align on block boundary */ - if (instr->addr & ((1 << chip->phys_erase_shift) - 1)) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Unaligned address\n", __func__); + if (block_alignment_verification(mtd, instr->addr, instr->len)) return -EINVAL; - } - - /* Length must align on block boundary */ - if (instr->len & ((1 << chip->phys_erase_shift) - 1)) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Length not block aligned\n", - __func__); - return -EINVAL; - } - - /* Do not allow erase past end of device */ - if ((instr->len + instr->addr) > mtd->size) { - DEBUG(MTD_DEBUG_LEVEL0, "%s: Erase past end of device\n", - __func__); - return -EINVAL; - } instr->fail_addr = MTD_FAIL_ADDR_UNKNOWN;