Patchwork [PATCHv2,4/5] libmtd: add support for 64-bit offsets, OOB

login
register
mail settings
Submitter Kevin Cernekee
Date July 8, 2010, 12:30 a.m.
Message ID <c65631c48fb5d2de8d25f65bffa5e22d@localhost>
Download mbox | patch
Permalink /patch/58212/
State New
Headers show

Comments

Kevin Cernekee - July 8, 2010, 12:30 a.m.
Change mtd_erase() so that it attempts to use MEMERASE64 first, then falls
back to the old <2.6.31 MEMERASE if MEMERASE64 is unsupported.

Add mtd_read_oob(), mtd_write_oob() functions to wrap the OOB ioctls.
Similar ioctl fallback logic is used in these functions as well.

Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
---
 include/libmtd.h |   36 ++++++++++++++++++++++-
 lib/libmtd.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 111 insertions(+), 7 deletions(-)
Artem Bityutskiy - July 13, 2010, 10:49 a.m.
On Wed, 2010-07-07 at 17:30 -0700, Kevin Cernekee wrote:
> Change mtd_erase() so that it attempts to use MEMERASE64 first, then falls
> back to the old <2.6.31 MEMERASE if MEMERASE64 is unsupported.
> 
> Add mtd_read_oob(), mtd_write_oob() functions to wrap the OOB ioctls.
> Similar ioctl fallback logic is used in these functions as well.
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  include/libmtd.h |   36 ++++++++++++++++++++++-
>  lib/libmtd.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 111 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libmtd.h b/include/libmtd.h
> index 0aea966..292d8c3 100644
> --- a/include/libmtd.h
> +++ b/include/libmtd.h
> @@ -66,6 +66,7 @@ struct mtd_info
>   * @region_cnt: count of additional erase regions
>   * @writable: zero if the device is read-only
>   * @bb_allowed: non-zero if the MTD device may have bad eraseblocks
> + * @legacy_ioctls: non-zero if the kernel lacks MEMERASE64, MEM*OOB64
>   */
>  struct mtd_dev_info
>  {
> @@ -84,6 +85,7 @@ struct mtd_dev_info
>  	int region_cnt;
>  	unsigned int writable:1;
>  	unsigned int bb_allowed:1;
> +	unsigned int legacy_ioctls:1;
>  };

Kevin, I'm sorry, but thinking about this some more, this is not a
property of mtd device, this is a property of whole mtd subsystem. So
this flag should live in 'struct mtd_info', just like the 'unsigned int
sysfs_supported:1;' field.

You should find out whether the new ioctl's are supported in
'libmtd_open()'. This also means that all libmtd calls which need this
flag will need a 'libmtd_t desc' parameter.

Also, is it possible to have 2 separate patches - 1st adds support for
64-bit offest, second adds OOB calls.

Is this doable? I think this should not be too difficult to change.
Artem Bityutskiy - July 13, 2010, 10:50 a.m.
On Wed, 2010-07-07 at 17:30 -0700, Kevin Cernekee wrote:
> Change mtd_erase() so that it attempts to use MEMERASE64 first, then falls
> back to the old <2.6.31 MEMERASE if MEMERASE64 is unsupported.
> 
> Add mtd_read_oob(), mtd_write_oob() functions to wrap the OOB ioctls.
> Similar ioctl fallback logic is used in these functions as well.
> 
> Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> ---
>  include/libmtd.h |   36 ++++++++++++++++++++++-
>  lib/libmtd.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 111 insertions(+), 7 deletions(-)
> 
> diff --git a/include/libmtd.h b/include/libmtd.h
> index 0aea966..292d8c3 100644
> --- a/include/libmtd.h
> +++ b/include/libmtd.h
> @@ -66,6 +66,7 @@ struct mtd_info
>   * @region_cnt: count of additional erase regions
>   * @writable: zero if the device is read-only
>   * @bb_allowed: non-zero if the MTD device may have bad eraseblocks
> + * @legacy_ioctls: non-zero if the kernel lacks MEMERASE64, MEM*OOB64
>   */
>  struct mtd_dev_info
>  {
> @@ -84,6 +85,7 @@ struct mtd_dev_info
>  	int region_cnt;
>  	unsigned int writable:1;
>  	unsigned int bb_allowed:1;
> +	unsigned int legacy_ioctls:1;
>  };
>  
>  /**
> @@ -146,7 +148,37 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd);
>   * This function erases eraseblock @eb of MTD device described by @fd. Returns
>   * %0 in case of success and %-1 in case of failure.
>   */
> -int mtd_erase(const struct mtd_dev_info *mtd, int fd, int eb);
> +int mtd_erase(struct mtd_dev_info *mtd, int fd, int eb);
> +
> +/**
> + * mtd_read_oob - read OOB bytes
> + * @mtd: MTD device description object
> + * @fd: MTD device node file descriptor
> + * @start: page-aligned start address
> + * @length: number of OOB bytes to read
> + * @data: read buffer
> + *
> + * This function reads @length OOB bytes starting from address @start on
> + * MTD device described by @fd. Returns %0 in case of success and %-1 in
> + * case of failure.
> + */
> +int mtd_read_oob(struct mtd_dev_info *mtd, int fd, uint64_t start,
> +	uint64_t length, void *data);

Minor, but for consistency, please, use the same indentation style as
the rest of the code. Namely, aline the second line:

int mtd_read_oob(struct mtd_dev_info *mtd, int fd, uint64_t start,
		 uint64_t length, void *data);

Use tabs, but then adjust by adding spaces.
Kevin Cernekee - July 14, 2010, 12:09 a.m.
On Tue, Jul 13, 2010 at 3:49 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> You should find out whether the new ioctl's are supported in
> 'libmtd_open()'. This also means that all libmtd calls which need this
> flag will need a 'libmtd_t desc' parameter.

Typically I just try the new ioctl first, then fall back to the old
one if it returned -ENOTTY.  Could you recommend a way to detect the
presence of the new ioctls from libmtd_open()?

Thanks.
Artem Bityutskiy - July 14, 2010, 2:51 a.m.
On Tue, 2010-07-13 at 17:09 -0700, Kevin Cernekee wrote:
> On Tue, Jul 13, 2010 at 3:49 AM, Artem Bityutskiy <dedekind1@gmail.com> wrote:
> > You should find out whether the new ioctl's are supported in
> > 'libmtd_open()'. This also means that all libmtd calls which need this
> > flag will need a 'libmtd_t desc' parameter.
> 
> Typically I just try the new ioctl first, then fall back to the old
> one if it returned -ENOTTY.  Could you recommend a way to detect the
> presence of the new ioctls from libmtd_open()?

Hmm, how about MEMERASE64 with start = length = -1? You can distinguish
between -ENOTTY and other error codes.
Artem Bityutskiy - July 17, 2010, 7:35 a.m.
On Tue, 2010-07-13 at 13:49 +0300, Artem Bityutskiy wrote:
> On Wed, 2010-07-07 at 17:30 -0700, Kevin Cernekee wrote:
> > Change mtd_erase() so that it attempts to use MEMERASE64 first, then falls
> > back to the old <2.6.31 MEMERASE if MEMERASE64 is unsupported.
> > 
> > Add mtd_read_oob(), mtd_write_oob() functions to wrap the OOB ioctls.
> > Similar ioctl fallback logic is used in these functions as well.
> > 
> > Signed-off-by: Kevin Cernekee <cernekee@gmail.com>
> > ---
> >  include/libmtd.h |   36 ++++++++++++++++++++++-
> >  lib/libmtd.c     |   82 ++++++++++++++++++++++++++++++++++++++++++++++++++---
> >  2 files changed, 111 insertions(+), 7 deletions(-)
> > 
> > diff --git a/include/libmtd.h b/include/libmtd.h
> > index 0aea966..292d8c3 100644
> > --- a/include/libmtd.h
> > +++ b/include/libmtd.h
> > @@ -66,6 +66,7 @@ struct mtd_info
> >   * @region_cnt: count of additional erase regions
> >   * @writable: zero if the device is read-only
> >   * @bb_allowed: non-zero if the MTD device may have bad eraseblocks
> > + * @legacy_ioctls: non-zero if the kernel lacks MEMERASE64, MEM*OOB64
> >   */
> >  struct mtd_dev_info
> >  {
> > @@ -84,6 +85,7 @@ struct mtd_dev_info
> >  	int region_cnt;
> >  	unsigned int writable:1;
> >  	unsigned int bb_allowed:1;
> > +	unsigned int legacy_ioctls:1;
> >  };
> 
> Kevin, I'm sorry, but thinking about this some more, this is not a
> property of mtd device, this is a property of whole mtd subsystem. So
> this flag should live in 'struct mtd_info', just like the 'unsigned int
> sysfs_supported:1;' field.
> 
> You should find out whether the new ioctl's are supported in
> 'libmtd_open()'. This also means that all libmtd calls which need this
> flag will need a 'libmtd_t desc' parameter.
> 
> Also, is it possible to have 2 separate patches - 1st adds support for
> 64-bit offest, second adds OOB calls.
> 
> Is this doable? I think this should not be too difficult to change.

I thought some more, and no, this is not doable because you need a
device node, which you do not have when the library is being
initialized. I'll take a look at these patches once again.

Patch

diff --git a/include/libmtd.h b/include/libmtd.h
index 0aea966..292d8c3 100644
--- a/include/libmtd.h
+++ b/include/libmtd.h
@@ -66,6 +66,7 @@  struct mtd_info
  * @region_cnt: count of additional erase regions
  * @writable: zero if the device is read-only
  * @bb_allowed: non-zero if the MTD device may have bad eraseblocks
+ * @legacy_ioctls: non-zero if the kernel lacks MEMERASE64, MEM*OOB64
  */
 struct mtd_dev_info
 {
@@ -84,6 +85,7 @@  struct mtd_dev_info
 	int region_cnt;
 	unsigned int writable:1;
 	unsigned int bb_allowed:1;
+	unsigned int legacy_ioctls:1;
 };
 
 /**
@@ -146,7 +148,37 @@  int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd);
  * This function erases eraseblock @eb of MTD device described by @fd. Returns
  * %0 in case of success and %-1 in case of failure.
  */
-int mtd_erase(const struct mtd_dev_info *mtd, int fd, int eb);
+int mtd_erase(struct mtd_dev_info *mtd, int fd, int eb);
+
+/**
+ * mtd_read_oob - read OOB bytes
+ * @mtd: MTD device description object
+ * @fd: MTD device node file descriptor
+ * @start: page-aligned start address
+ * @length: number of OOB bytes to read
+ * @data: read buffer
+ *
+ * This function reads @length OOB bytes starting from address @start on
+ * MTD device described by @fd. Returns %0 in case of success and %-1 in
+ * case of failure.
+ */
+int mtd_read_oob(struct mtd_dev_info *mtd, int fd, uint64_t start,
+	uint64_t length, void *data);
+
+/**
+ * mtd_write_oob - write OOB bytes
+ * @mtd: MTD device description object
+ * @fd: MTD device node file descriptor
+ * @start: page-aligned start address
+ * @length: number of OOB bytes to write
+ * @data: write buffer
+ *
+ * This function writes @length OOB bytes starting from address @start on
+ * MTD device described by @fd. Returns %0 in case of success and %-1 in
+ * case of failure.
+ */
+int mtd_write_oob(struct mtd_dev_info *mtd, int fd, uint64_t start,
+	uint64_t length, void *data);
 
 /**
  * mtd_torture - torture an eraseblock.
@@ -157,7 +189,7 @@  int mtd_erase(const struct mtd_dev_info *mtd, int fd, int eb);
  * This function tortures eraseblock @eb. Returns %0 in case of success and %-1
  * in case of failure.
  */
-int mtd_torture(const struct mtd_dev_info *mtd, int fd, int eb);
+int mtd_torture(struct mtd_dev_info *mtd, int fd, int eb);
 
 /**
  * mtd_is_bad - check if eraseblock is bad.
diff --git a/lib/libmtd.c b/lib/libmtd.c
index 3ff031c..66e794a 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -32,6 +32,7 @@ 
 #include <sys/stat.h>
 #include <sys/ioctl.h>
 #include <mtd/mtd-user.h>
+#include <mtd/mtd-abi.h>
 
 #include <libmtd.h>
 #include "libmtd_int.h"
@@ -789,13 +790,84 @@  int mtd_get_dev_info(libmtd_t desc, const char *node, struct mtd_dev_info *mtd)
 	return mtd_get_dev_info1(desc, mtd_num, mtd);
 }
 
-int mtd_erase(const struct mtd_dev_info *mtd, int fd, int eb)
+int mtd_erase(struct mtd_dev_info *mtd, int fd, int eb)
 {
-	struct erase_info_user ei;
+	int ret;
+	struct erase_info_user64 ei;
 
-	ei.start = eb * mtd->eb_size;;
+	ei.start = (__u64)eb * mtd->eb_size;
 	ei.length = mtd->eb_size;
-	return ioctl(fd, MEMERASE, &ei);
+
+	if (!mtd->legacy_ioctls) {
+		ret = ioctl(fd, MEMERASE64, &ei);
+		if (ret < 0 && errno == ENOTTY)
+			mtd->legacy_ioctls = 1;
+	}
+
+	if (mtd->legacy_ioctls) {
+		struct erase_info_user ei_old;
+
+		ei_old.start = ei.start;
+		ei_old.length = ei.length;
+
+		if (ei_old.start != ei.start) {
+			errno = EOVERFLOW;
+			return -1;
+		}
+
+		ret = ioctl(fd, MEMERASE, &ei_old);
+	}
+
+	return ret;
+}
+
+static int mtd_oob_op(int cmd, int cmd_old, struct mtd_dev_info *mtd, int fd,
+	uint64_t start, uint64_t length, void *data)
+{
+	int ret;
+	struct mtd_oob_buf64 oob;
+
+	oob.start = start;
+	oob.length = length;
+	oob.usr_ptr = (__u64)(unsigned long)data;
+
+	if (!mtd->legacy_ioctls) {
+		ret = ioctl(fd, cmd, &oob);
+		if (ret < 0 && errno == ENOTTY)
+			mtd->legacy_ioctls = 1;
+	}
+
+	if (mtd->legacy_ioctls) {
+		struct mtd_oob_buf oob_old;
+
+		oob_old.start = oob.start;
+		oob_old.length = oob.length;
+		oob_old.ptr = data;
+
+		if (oob_old.start != oob.start ||
+				oob_old.length != oob.length) {
+			errno = EOVERFLOW;
+			return -1;
+		}
+
+		ret = ioctl(fd, cmd_old, &oob_old);
+	}
+
+	return ret;
+}
+
+int mtd_read_oob(struct mtd_dev_info *mtd, int fd, uint64_t start,
+	uint64_t length, void *data)
+{
+	return mtd_oob_op(MEMREADOOB64, MEMREADOOB, mtd, fd, start, length,
+		data);
+}
+
+int mtd_write_oob(struct mtd_dev_info *mtd, int fd, uint64_t start,
+	uint64_t length, void *data)
+{
+	return mtd_oob_op(MEMWRITEOOB64, MEMWRITEOOB, mtd, fd, start, length,
+		data);
 }
 
 /* Patterns to write to a physical eraseblock when torturing it */
@@ -820,7 +892,7 @@  static int check_pattern(const void *buf, uint8_t patt, int size)
 	return 1;
 }
 
-int mtd_torture(const struct mtd_dev_info *mtd, int fd, int eb)
+int mtd_torture(struct mtd_dev_info *mtd, int fd, int eb)
 {
 	int err, i, patt_count;
 	void *buf;