Patchwork abuse of nand_correct_data in tmio_nand driver

login
register
mail settings
Submitter Atsushi Nemoto
Date July 22, 2009, 3:13 p.m.
Message ID <20090723.001338.27942898.anemo@mba.ocn.ne.jp>
Download mbox | patch
Permalink /patch/30081/
State New
Headers show

Comments

Atsushi Nemoto - July 22, 2009, 3:13 p.m.
On Wed, 22 Jul 2009 14:13:30 +0530, vimal singh <vimal.newwork@gmail.com> wrote:
> >> This can be handled by overriding 'chip->ecc.correct' in driver. You
> >> may reuse 'nand_correct_data()' code in the driver of 256 byte sector
> >> ECC.
> >
> > Yes, the driver can reuse nand_ecc code to implement its own
> > nand_correct rountine.
> >
> > Or how about adding support for 6byte-ecc/512byte-data to nand_ecc.c
> > to avoid code duplication?
> >
> > I mean something like this.  If it looks acceptable, I will prepare a
> > patch including similer change to nand_calculate_ecc.
> 
> I personally do not like any HW specific implementation going into the
> generic part
> of the code. This particular issue is specific to your HW, so better
> handle it in the
> driver only.

OK, but I still feel duplicating nand_ecc code is not so good.  How
about splitting nand_correct_data into two parts?  A pure calculation
function and a wrapper for mtd interface.  Like this:



There is a little bonus: the STANDALONE macro in nand_ecc.c can be
more useful with this change ;)

---
Atsushi Nemoto
vimal singh - July 23, 2009, 7:04 a.m.
On Wed, Jul 22, 2009 at 8:43 PM, Atsushi Nemoto<anemo@mba.ocn.ne.jp> wrote:
> On Wed, 22 Jul 2009 14:13:30 +0530, vimal singh <vimal.newwork@gmail.com> wrote:
>> >> This can be handled by overriding 'chip->ecc.correct' in driver. You
>> >> may reuse 'nand_correct_data()' code in the driver of 256 byte sector
>> >> ECC.
>> >
>> > Yes, the driver can reuse nand_ecc code to implement its own
>> > nand_correct rountine.
>> >
>> > Or how about adding support for 6byte-ecc/512byte-data to nand_ecc.c
>> > to avoid code duplication?
>> >
>> > I mean something like this.  If it looks acceptable, I will prepare a
>> > patch including similer change to nand_calculate_ecc.
>>
>> I personally do not like any HW specific implementation going into the
>> generic part
>> of the code. This particular issue is specific to your HW, so better
>> handle it in the
>> driver only.
>
> OK, but I still feel duplicating nand_ecc code is not so good.  How
> about splitting nand_correct_data into two parts?  A pure calculation
> function and a wrapper for mtd interface.  Like this:

But I do not see any thing extra, which you achieve from this
wrapper...  Is this a prototype, and you want to handle above scenario
in this wrapper (calling 'nand_correct_data' multiple times based on
something, probably 'ecc.bytes')?

>
> diff --git a/drivers/mtd/nand/nand_ecc.c b/drivers/mtd/nand/nand_ecc.c
> index c0cb87d..3920896 100644
> --- a/drivers/mtd/nand/nand_ecc.c
> +++ b/drivers/mtd/nand/nand_ecc.c
> @@ -425,14 +425,14 @@ EXPORT_SYMBOL(nand_calculate_ecc);
>  *
>  * Detect and correct a 1 bit error for 256/512 byte block
>  */
> -int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
> -                     unsigned char *read_ecc, unsigned char *calc_ecc)
> +int __nand_correct_data(unsigned char *buf,
> +                       unsigned char *read_ecc, unsigned char *calc_ecc,
> +                       unsigned int eccsize)
>  {
>        unsigned char b0, b1, b2, bit_addr;
>        unsigned int byte_addr;
>        /* 256 or 512 bytes/ecc  */
> -       const uint32_t eccsize_mult =
> -                       (((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
> +       const uint32_t eccsize_mult = eccsize >> 8;
>
>        /*
>         * b0 to b2 indicate which bit is faulty (if any)
> @@ -495,6 +495,16 @@ int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
>        printk(KERN_ERR "uncorrectable error : ");
>        return -1;
>  }
> +EXPORT_SYMBOL(__nand_correct_data);
> +
> +int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
> +                     unsigned char *read_ecc, unsigned char *calc_ecc)
> +{
> +       const uint32_t eccsize_mult =
> +                       (((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
> +       return __nand_correct_data(buf, read_ecc, calc_ecc,
> +                                  ((struct nand_chip *)mtd->priv)->ecc.size);
> +}
>  EXPORT_SYMBOL(nand_correct_data);
>
>  MODULE_LICENSE("GPL");
>
>
> There is a little bonus: the STANDALONE macro in nand_ecc.c can be
> more useful with this change ;)
>
> ---
> Atsushi Nemoto
>
Atsushi Nemoto - July 23, 2009, 3:11 p.m.
On Thu, 23 Jul 2009 12:34:43 +0530, vimal singh <vimal.newwork@gmail.com> wrote:
> > OK, but I still feel duplicating nand_ecc code is not so good.  How
> > about splitting nand_correct_data into two parts?  A pure calculation
> > function and a wrapper for mtd interface.  Like this:
> 
> But I do not see any thing extra, which you achieve from this
> wrapper...  Is this a prototype, and you want to handle above scenario
> in this wrapper (calling 'nand_correct_data' multiple times based on
> something, probably 'ecc.bytes')?

Yes, if we have __nand_correct_data(buf, read_ecc, calc_ecc, eccsize)
which do job based on its eccsize argument, the ecc.correct function
of the tmio_nand driver can be implemented like this:

int tmio_nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
		unsigned char *read_ecc, unsigned char *calc_ecc)
{
	int r0, r1;

	/* assume ecc.size = 512 and ecc.bytes = 6 */
	r0 = __nand_correct_data(buf, read_ecc, calc_ecc, 256);
	if (r0 < 0)
		return r0;
	r1 = __nand_correct_data(buf + 256, read_ecc + 3, calc_ecc + 3, 256);
	if (r1 < 0)
		return r1;
	return r0 | r1;
}

Note that this is not tested at all since I do not have tmio device.

Of course if Dmitry had another idea, I will not intrude this
approach.

---
Atsushi Nemoto
Dmitry Eremin-Solenikov - July 24, 2009, 5:24 p.m.
2009/7/23 Atsushi Nemoto <anemo@mba.ocn.ne.jp>:
> On Thu, 23 Jul 2009 12:34:43 +0530, vimal singh <vimal.newwork@gmail.com> wrote:
>> > OK, but I still feel duplicating nand_ecc code is not so good.  How
>> > about splitting nand_correct_data into two parts?  A pure calculation
>> > function and a wrapper for mtd interface.  Like this:
>>
>> But I do not see any thing extra, which you achieve from this
>> wrapper...  Is this a prototype, and you want to handle above scenario
>> in this wrapper (calling 'nand_correct_data' multiple times based on
>> something, probably 'ecc.bytes')?
>
> Yes, if we have __nand_correct_data(buf, read_ecc, calc_ecc, eccsize)
> which do job based on its eccsize argument, the ecc.correct function
> of the tmio_nand driver can be implemented like this:
>
> int tmio_nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
>                unsigned char *read_ecc, unsigned char *calc_ecc)
> {
>        int r0, r1;
>
>        /* assume ecc.size = 512 and ecc.bytes = 6 */
>        r0 = __nand_correct_data(buf, read_ecc, calc_ecc, 256);
>        if (r0 < 0)
>                return r0;
>        r1 = __nand_correct_data(buf + 256, read_ecc + 3, calc_ecc + 3, 256);
>        if (r1 < 0)
>                return r1;
>        return r0 | r1;
> }
>
> Note that this is not tested at all since I do not have tmio device.

This seems to be the thing that was initially ment by Zaurus kernels, so
this if Ian doesn't object,

Acked-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
vimal singh - July 26, 2009, 1:03 p.m.
On Thu, Jul 23, 2009 at 8:41 PM, Atsushi Nemoto<anemo@mba.ocn.ne.jp> wrote:
> On Thu, 23 Jul 2009 12:34:43 +0530, vimal singh <vimal.newwork@gmail.com> wrote:
>> > OK, but I still feel duplicating nand_ecc code is not so good.  How
>> > about splitting nand_correct_data into two parts?  A pure calculation
>> > function and a wrapper for mtd interface.  Like this:
>>
>> But I do not see any thing extra, which you achieve from this
>> wrapper...  Is this a prototype, and you want to handle above scenario
>> in this wrapper (calling 'nand_correct_data' multiple times based on
>> something, probably 'ecc.bytes')?
>
> Yes, if we have __nand_correct_data(buf, read_ecc, calc_ecc, eccsize)
> which do job based on its eccsize argument, the ecc.correct function
> of the tmio_nand driver can be implemented like this:
>
> int tmio_nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
>                unsigned char *read_ecc, unsigned char *calc_ecc)
> {
>        int r0, r1;
>
>        /* assume ecc.size = 512 and ecc.bytes = 6 */
>        r0 = __nand_correct_data(buf, read_ecc, calc_ecc, 256);
>        if (r0 < 0)
>                return r0;
>        r1 = __nand_correct_data(buf + 256, read_ecc + 3, calc_ecc + 3, 256);
>        if (r1 < 0)
>                return r1;
>        return r0 | r1;

Better 'r0 + r1', returned value should represent number of error bits
corrected.


After fixing above comment:

Acked-by: Vimal Singh <vimalsingh@ti.com>


---
Regards,
\/ | |\/| /-\ |_

____      __o
------   -\<,
-----  ( )/ ( )

Patch

diff --git a/drivers/mtd/nand/nand_ecc.c b/drivers/mtd/nand/nand_ecc.c
index c0cb87d..3920896 100644
--- a/drivers/mtd/nand/nand_ecc.c
+++ b/drivers/mtd/nand/nand_ecc.c
@@ -425,14 +425,14 @@  EXPORT_SYMBOL(nand_calculate_ecc);
  *
  * Detect and correct a 1 bit error for 256/512 byte block
  */
-int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
-		      unsigned char *read_ecc, unsigned char *calc_ecc)
+int __nand_correct_data(unsigned char *buf,
+			unsigned char *read_ecc, unsigned char *calc_ecc,
+			unsigned int eccsize)
 {
 	unsigned char b0, b1, b2, bit_addr;
 	unsigned int byte_addr;
 	/* 256 or 512 bytes/ecc  */
-	const uint32_t eccsize_mult =
-			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
+	const uint32_t eccsize_mult = eccsize >> 8;
 
 	/*
 	 * b0 to b2 indicate which bit is faulty (if any)
@@ -495,6 +495,16 @@  int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
 	printk(KERN_ERR "uncorrectable error : ");
 	return -1;
 }
+EXPORT_SYMBOL(__nand_correct_data);
+
+int nand_correct_data(struct mtd_info *mtd, unsigned char *buf,
+		      unsigned char *read_ecc, unsigned char *calc_ecc)
+{
+	const uint32_t eccsize_mult =
+			(((struct nand_chip *)mtd->priv)->ecc.size) >> 8;
+	return __nand_correct_data(buf, read_ecc, calc_ecc,
+				   ((struct nand_chip *)mtd->priv)->ecc.size);
+}
 EXPORT_SYMBOL(nand_correct_data);
 
 MODULE_LICENSE("GPL");