Patchwork [v3] mtd/docg3: fixes and cleanups

login
register
mail settings
Submitter Robert Jarzmik
Date Dec. 9, 2011, 4:13 p.m.
Message ID <877h2590zp.fsf@free.fr>
Download mbox | patch
Permalink /patch/130413/
State New
Headers show

Comments

Robert Jarzmik - Dec. 9, 2011, 4:13 p.m.
Artem Bityutskiy <dedekind1@gmail.com> writes:

> On Fri, 2011-12-02 at 20:00 +0100, Robert Jarzmik wrote:
>> This patch takes into account checkpatch, sparse and ECC
>> comments.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Pushed to l2-mtd-2.6.git, thanks. But I still see this warning when
> I compile on x86_64:
>
> drivers/mtd/devices/docg3.c:1050:12: warning: ‘doc_get_erase_count’ defined
> but not used [-Wunused-function]

Hi Artem,

I have found no clean and good way to handle the erase count.  As a fallback,
please accept the following patch which removes the unused function. It's kind
of a "looser" approach, but that's the best I can do by now.

The patch is based on your current l2-tree (commit
af64b37f63189cef7575fb3f8e670df767c3e15b "mtdoops: fix the oops_page_used array
size").

Cheers.

--
Robert

From c87ece988e535905702b40460c6685984f08db88 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Fri, 9 Dec 2011 17:02:49 +0100
Subject: [PATCH] mtd: docg3: remove unused function

As the MTD api has no use for the number of erase cycles
each block has endured, remove the function which calculated
that value.

If one day MTD api finds it usefull for wear levelling
algorithms to have this information, the function should be
put back in place.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/devices/docg3.c |   42 ------------------------------------------
 1 files changed, 0 insertions(+), 42 deletions(-)
Artem Bityutskiy - Dec. 12, 2011, 9:13 p.m.
On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
> Artem Bityutskiy <dedekind1@gmail.com> writes:
> 
> > On Fri, 2011-12-02 at 20:00 +0100, Robert Jarzmik wrote:
> >> This patch takes into account checkpatch, sparse and ECC
> >> comments.
> >> 
> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> >
> > Pushed to l2-mtd-2.6.git, thanks. But I still see this warning when
> > I compile on x86_64:
> >
> > drivers/mtd/devices/docg3.c:1050:12: warning: ‘doc_get_erase_count’ defined
> > but not used [-Wunused-function]
> 
> Hi Artem,
> 
> I have found no clean and good way to handle the erase count.  As a fallback,
> please accept the following patch which removes the unused function. It's kind
> of a "looser" approach, but that's the best I can do by now.

What do you mean by "handle" ? Is the problem that docg3 maintains
per-eraseblock erase-counters and you do not know how to use them? May
be it is better to just embrace this function with "#ifdef 0" and add a
comment that we can do this but do not have a good use for this?

Artem.
Robert Jarzmik - Dec. 12, 2011, 10:03 p.m.
Artem Bityutskiy <dedekind1@gmail.com> writes:

> On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
> What do you mean by "handle" ? Is the problem that docg3 maintains
> per-eraseblock erase-counters and you do not know how to use them?
Yes, exactly, docg3 maintains that counter for each block.

I thought that upper layers (UBI,...) could use both the erase counter and the
ECC number of flipped bits to select which block to erase next. After a quick
glance, I don't think they can do it.

> May be it is better to just embrace this function with "#ifdef 0" and add a
> comment that we can do this but do not have a good use for this?
Ah yes, I didn't think about that possibility. It's looks better, I'll submit
another patch with a comment and the #if 0.

Cheers.
Mike Dunn - Dec. 13, 2011, 4:16 a.m.
On 12/12/2011 02:03 PM, Robert Jarzmik wrote:
> Artem Bityutskiy <dedekind1@gmail.com> writes:
>
>> On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
>> What do you mean by "handle" ? Is the problem that docg3 maintains
>> per-eraseblock erase-counters and you do not know how to use them?
> Yes, exactly, docg3 maintains that counter for each block.
>
> I thought that upper layers (UBI,...) could use both the erase counter and the
> ECC number of flipped bits to select which block to erase next. After a quick
> glance, I don't think they can do it.


The erase counters are surely used by the TrueFFS library.  The diskonchip and
TrueFFS were both developed by M-Sys.  But since it's a unique feature of this
device, it would probably be impractical for ubi or mtd to use them.  I'm
impressed that you discovered them, though.  I had no clue that they existed on
the diskonchip P3.  They are probably on the G4 too.


>> May be it is better to just embrace this function with "#ifdef 0" and add a
>> comment that we can do this but do not have a good use for this?
> Ah yes, I didn't think about that possibility. It's looks better, I'll submit
> another patch with a comment and the #if 0.


Yeah, leave it in, commented out.  But don't submit a patch that produces a
compiler warning!

Thanks,
Mike

Patch

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 2a32072..f05c5bc 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1038,48 +1038,6 @@  static int doc_block_isbad(struct mtd_info *mtd, loff_t from)
 }
 
 /**
- * doc_get_erase_count - Get block erase count
- * @docg3: the device
- * @from: the offset in which the block is.
- *
- * Get the number of times a block was erased. The number is the maximum of
- * erase times between first and second plane (which should be equal normally).
- *
- * Returns The number of erases, or -EINVAL or -EIO on error.
- */
-static int doc_get_erase_count(struct docg3 *docg3, loff_t from)
-{
-	u8 buf[DOC_LAYOUT_WEAR_SIZE];
-	int ret, plane1_erase_count, plane2_erase_count;
-	int block0, block1, page, ofs;
-
-	doc_dbg("doc_get_erase_count(from=%lld, buf=%p)\n", from, buf);
-	if (from % DOC_LAYOUT_PAGE_SIZE)
-		return -EINVAL;
-	calc_block_sector(from, &block0, &block1, &page, &ofs, docg3->reliable);
-	if (block1 > docg3->max_block)
-		return -EINVAL;
-
-	ret = doc_reset_seq(docg3);
-	if (!ret)
-		ret = doc_read_page_prepare(docg3, block0, block1, page,
-					    ofs + DOC_LAYOUT_WEAR_OFFSET);
-	if (!ret)
-		ret = doc_read_page_getbytes(docg3, DOC_LAYOUT_WEAR_SIZE,
-					     buf, 1);
-	doc_read_page_finish(docg3);
-
-	if (ret || (buf[0] != DOC_ERASE_MARK) || (buf[2] != DOC_ERASE_MARK))
-		return -EIO;
-	plane1_erase_count = (u8)(~buf[1]) | ((u8)(~buf[4]) << 8)
-		| ((u8)(~buf[5]) << 16);
-	plane2_erase_count = (u8)(~buf[3]) | ((u8)(~buf[6]) << 8)
-		| ((u8)(~buf[7]) << 16);
-
-	return max(plane1_erase_count, plane2_erase_count);
-}
-
-/**
  * doc_get_op_status - get erase/write operation status
  * @docg3: the device
  *