[PATCH 0/2] Fix Jffs2 type flash erase problem

[PATCH 0/2] Fix Jffs2 type flash erase problem

[Xiaolei Li]

Jffs2 clean marker is not written actually, because OOB write length is
set to 0 when do mtd_write(). So, "-j" option of flash_erase is usless now.

This patch adds support to access OOB available size by
/sys/class/mtd/mtdX/oobavail. Then, user can write clean marker to OOB
free area with the minimum size between OOB available size and 8. This
is the same with Jffs2 itself. Please refer the function
jffs2_write_nand_cleanmarker() in the kernel file fs/jffs2/wbuf.c.

And this patch depends on the reviewing patch "mtd: Add sysfs attribute for
mtd OOB available size"[1].

Changes relative to:
--------------------
tree	: https://github.com/sigma-star/mtd-utils
branch	: master
commit	:
	'commit 80de29a464c7 ("mkfs.ubifs: Allow root entry in device
	 table")'

Tests:
------
* do "flash_erase -j" operation on SLC NAND MT29F16G08ADBCA, which page
  size is 4096, oob size is 224.
* mount jffs2 file system, do "dd" test, and there is no problem.

[1] https://patchwork.kernel.org/patch/10319475/

Xiaolei Li (2):
  libmtd: Add support to access OOB available size
  misc-utils: flash_erase: Fix Jffs2 type flash erase problem

 include/libmtd.h         | 2 ++
 lib/libmtd.c             | 7 +++++++
 lib/libmtd_int.h         | 3 +++
 misc-utils/flash_erase.c | 9 +++++----
 4 files changed, 17 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH 0/2] Fix Jffs2 type flash erase problem

[Xiaolei Li]

Jffs2 clean marker is not written actually, because OOB write length is
set to 0 when do mtd_write(). So, "-j" option of flash_erase is usless now.

This patch adds support to access OOB available size by
/sys/class/mtd/mtdX/oobavail. Then, user can write clean marker to OOB
free area with the minimum size between OOB available size and 8. This
is the same with Jffs2 itself. Please refer the function
jffs2_write_nand_cleanmarker() in the kernel file fs/jffs2/wbuf.c.

And this patch depends on the reviewing patch "mtd: Add sysfs attribute for
mtd OOB available size"[1].

Changes relative to:
--------------------
tree	: https://github.com/sigma-star/mtd-utils
branch	: master
commit	:
	'commit 80de29a464c7 ("mkfs.ubifs: Allow root entry in device
	 table")'

Tests:
------
* do "flash_erase -j" operation on SLC NAND MT29F16G08ADBCA, which page
  size is 4096, oob size is 224.
* mount jffs2 file system, do "dd" test, and there is no problem.

[1] https://patchwork.kernel.org/patch/10319475/

Xiaolei Li (2):
  libmtd: Add support to access OOB available size
  misc-utils: flash_erase: Fix Jffs2 type flash erase problem

 include/libmtd.h         | 2 ++
 lib/libmtd.c             | 7 +++++++
 lib/libmtd_int.h         | 3 +++
 misc-utils/flash_erase.c | 9 +++++----
 4 files changed, 17 insertions(+), 4 deletions(-)

-- 
1.9.1

[PATCH 1/2] libmtd: Add support to access OOB available size

[Xiaolei Li]

Fetch OOB available size by accessing /sys/class/mtd/mtdX/oobavail.

Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
---
This patch depends on the reviewing patch "mtd: Add sysfs attribute for
mtd OOB available size"[1].

[1] https://patchwork.kernel.org/patch/10319475/
---
 include/libmtd.h | 2 ++
 lib/libmtd.c     | 7 +++++++
 lib/libmtd_int.h | 3 +++
 3 files changed, 12 insertions(+)

diff --git a/include/libmtd.h b/include/libmtd.h
index db85fb4..cc24af8 100644
--- a/include/libmtd.h
+++ b/include/libmtd.h
@@ -66,6 +66,7 @@ struct mtd_info
  * @min_io_size: minimum input/output unit size
  * @subpage_size: sub-page size
  * @oob_size: OOB size (zero if the device does not have OOB area)
+ * @oobavail: free OOB size
  * @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
@@ -84,6 +85,7 @@ struct mtd_dev_info
 	int min_io_size;
 	int subpage_size;
 	int oob_size;
+	int oobavail;
 	int region_cnt;
 	unsigned int writable:1;
 	unsigned int bb_allowed:1;
diff --git a/lib/libmtd.c b/lib/libmtd.c
index 86c89ae..76a9439 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -614,6 +614,10 @@ libmtd_t libmtd_open(void)
 	if (!lib->mtd_oob_size)
 		goto out_error;
 
+	lib->mtd_oobavail = mkpath(lib->mtd, MTD_OOBAVAIL);
+	if (!lib->mtd_oobavail)
+		goto out_error;
+
 	lib->mtd_region_cnt = mkpath(lib->mtd, MTD_REGION_CNT);
 	if (!lib->mtd_region_cnt)
 		goto out_error;
@@ -637,6 +641,7 @@ void libmtd_close(libmtd_t desc)
 	free(lib->mtd_flags);
 	free(lib->mtd_region_cnt);
 	free(lib->mtd_oob_size);
+	free(lib->mtd_oobavail);
 	free(lib->mtd_subpage_size);
 	free(lib->mtd_min_io_size);
 	free(lib->mtd_size);
@@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
 		return -1;
 	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
 		return -1;
+	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
+		return -1;
 	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
 		return -1;
 	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))
diff --git a/lib/libmtd_int.h b/lib/libmtd_int.h
index 03b0863..3cab32b 100644
--- a/lib/libmtd_int.h
+++ b/lib/libmtd_int.h
@@ -44,6 +44,7 @@ extern "C" {
 #define MTD_MIN_IO_SIZE  "writesize"
 #define MTD_SUBPAGE_SIZE "subpagesize"
 #define MTD_OOB_SIZE     "oobsize"
+#define MTD_OOBAVAIL     "oobavail"
 #define MTD_REGION_CNT   "numeraseregions"
 #define MTD_FLAGS        "flags"
 
@@ -63,6 +64,7 @@ extern "C" {
  * @mtd_min_io_size: minimum I/O unit size file pattern
  * @mtd_subpage_size: sub-page size file pattern
  * @mtd_oob_size: MTD device OOB size file pattern
+ * @mtd_oobavail: MTD device free OOB size file pattern
  * @mtd_region_cnt: count of additional erase regions file pattern
  * @mtd_flags: MTD device flags file pattern
  * @sysfs_supported: non-zero if sysfs is supported by MTD
@@ -92,6 +94,7 @@ struct libmtd
 	char *mtd_min_io_size;
 	char *mtd_subpage_size;
 	char *mtd_oob_size;
+	char *mtd_oobavail;
 	char *mtd_region_cnt;
 	char *mtd_flags;
 	unsigned int sysfs_supported:1;
-- 
1.9.1

[PATCH 1/2] libmtd: Add support to access OOB available size

[Xiaolei Li]

Fetch OOB available size by accessing /sys/class/mtd/mtdX/oobavail.

Signed-off-by: Xiaolei Li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
This patch depends on the reviewing patch "mtd: Add sysfs attribute for
mtd OOB available size"[1].

[1] https://patchwork.kernel.org/patch/10319475/
---
 include/libmtd.h | 2 ++
 lib/libmtd.c     | 7 +++++++
 lib/libmtd_int.h | 3 +++
 3 files changed, 12 insertions(+)

diff --git a/include/libmtd.h b/include/libmtd.h
index db85fb4..cc24af8 100644
--- a/include/libmtd.h
+++ b/include/libmtd.h
@@ -66,6 +66,7 @@ struct mtd_info
  * @min_io_size: minimum input/output unit size
  * @subpage_size: sub-page size
  * @oob_size: OOB size (zero if the device does not have OOB area)
+ * @oobavail: free OOB size
  * @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
@@ -84,6 +85,7 @@ struct mtd_dev_info
 	int min_io_size;
 	int subpage_size;
 	int oob_size;
+	int oobavail;
 	int region_cnt;
 	unsigned int writable:1;
 	unsigned int bb_allowed:1;
diff --git a/lib/libmtd.c b/lib/libmtd.c
index 86c89ae..76a9439 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -614,6 +614,10 @@ libmtd_t libmtd_open(void)
 	if (!lib->mtd_oob_size)
 		goto out_error;
 
+	lib->mtd_oobavail = mkpath(lib->mtd, MTD_OOBAVAIL);
+	if (!lib->mtd_oobavail)
+		goto out_error;
+
 	lib->mtd_region_cnt = mkpath(lib->mtd, MTD_REGION_CNT);
 	if (!lib->mtd_region_cnt)
 		goto out_error;
@@ -637,6 +641,7 @@ void libmtd_close(libmtd_t desc)
 	free(lib->mtd_flags);
 	free(lib->mtd_region_cnt);
 	free(lib->mtd_oob_size);
+	free(lib->mtd_oobavail);
 	free(lib->mtd_subpage_size);
 	free(lib->mtd_min_io_size);
 	free(lib->mtd_size);
@@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
 		return -1;
 	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
 		return -1;
+	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
+		return -1;
 	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
 		return -1;
 	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))
diff --git a/lib/libmtd_int.h b/lib/libmtd_int.h
index 03b0863..3cab32b 100644
--- a/lib/libmtd_int.h
+++ b/lib/libmtd_int.h
@@ -44,6 +44,7 @@ extern "C" {
 #define MTD_MIN_IO_SIZE  "writesize"
 #define MTD_SUBPAGE_SIZE "subpagesize"
 #define MTD_OOB_SIZE     "oobsize"
+#define MTD_OOBAVAIL     "oobavail"
 #define MTD_REGION_CNT   "numeraseregions"
 #define MTD_FLAGS        "flags"
 
@@ -63,6 +64,7 @@ extern "C" {
  * @mtd_min_io_size: minimum I/O unit size file pattern
  * @mtd_subpage_size: sub-page size file pattern
  * @mtd_oob_size: MTD device OOB size file pattern
+ * @mtd_oobavail: MTD device free OOB size file pattern
  * @mtd_region_cnt: count of additional erase regions file pattern
  * @mtd_flags: MTD device flags file pattern
  * @sysfs_supported: non-zero if sysfs is supported by MTD
@@ -92,6 +94,7 @@ struct libmtd
 	char *mtd_min_io_size;
 	char *mtd_subpage_size;
 	char *mtd_oob_size;
+	char *mtd_oobavail;
 	char *mtd_region_cnt;
 	char *mtd_flags;
 	unsigned int sysfs_supported:1;
-- 
1.9.1

[PATCH 2/2] misc-utils: flash_erase: Fix Jffs2 type flash erase problem

[Xiaolei Li]

Currently, Jffs2 clean marker is not written actually, because the oob
write length is set to 0 when do mtd_write().

So, get OOB available size at first, and set the correct clean marker
length, then program clean marker to free OOB area.

Fixes: d7e86124d55b ("mtd-utils: Support jffs2 flash-erase for large OOB (>32b)")
Signed-off-by: Xiaolei Li <xiaolei.li@mediatek.com>
---
 misc-utils/flash_erase.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/misc-utils/flash_erase.c b/misc-utils/flash_erase.c
index 0c9449f..e7a00ae 100644
--- a/misc-utils/flash_erase.c
+++ b/misc-utils/flash_erase.c
@@ -92,7 +92,7 @@ int main(int argc, char *argv[])
 {
 	libmtd_t mtd_desc;
 	struct mtd_dev_info mtd;
-	int fd;
+	int fd, cmlen = 8;
 	unsigned long long start;
 	unsigned int eb, eb_start, eb_cnt;
 	bool isNAND;
@@ -190,10 +190,11 @@ int main(int argc, char *argv[])
 	if (jffs2) {
 		cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK);
 		cleanmarker.nodetype = cpu_to_je16 (JFFS2_NODETYPE_CLEANMARKER);
-		if (!isNAND)
+		if (!isNAND) {
 			cleanmarker.totlen = cpu_to_je32(sizeof(cleanmarker));
-		else {
+		} else {
 			cleanmarker.totlen = cpu_to_je32(8);
+			cmlen = min(mtd.oobavail, 8);
 		}
 		cleanmarker.hdr_crc = cpu_to_je32(mtd_crc32(0, &cleanmarker, sizeof(cleanmarker) - 4));
 	}
@@ -242,7 +243,7 @@ int main(int argc, char *argv[])
 
 		/* write cleanmarker */
 		if (isNAND) {
-			if (mtd_write(mtd_desc, &mtd, fd, eb, 0, NULL, 0, &cleanmarker, 0,
+			if (mtd_write(mtd_desc, &mtd, fd, eb, 0, NULL, 0, &cleanmarker, cmlen,
 					MTD_OPS_AUTO_OOB) != 0) {
 				sys_errmsg("%s: MTD writeoob failure", mtd_device);
 				continue;
-- 
1.9.1

[PATCH 2/2] misc-utils: flash_erase: Fix Jffs2 type flash erase problem

[Xiaolei Li]

Currently, Jffs2 clean marker is not written actually, because the oob
write length is set to 0 when do mtd_write().

So, get OOB available size at first, and set the correct clean marker
length, then program clean marker to free OOB area.

Fixes: d7e86124d55b ("mtd-utils: Support jffs2 flash-erase for large OOB (>32b)")
Signed-off-by: Xiaolei Li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
---
 misc-utils/flash_erase.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/misc-utils/flash_erase.c b/misc-utils/flash_erase.c
index 0c9449f..e7a00ae 100644
--- a/misc-utils/flash_erase.c
+++ b/misc-utils/flash_erase.c
@@ -92,7 +92,7 @@ int main(int argc, char *argv[])
 {
 	libmtd_t mtd_desc;
 	struct mtd_dev_info mtd;
-	int fd;
+	int fd, cmlen = 8;
 	unsigned long long start;
 	unsigned int eb, eb_start, eb_cnt;
 	bool isNAND;
@@ -190,10 +190,11 @@ int main(int argc, char *argv[])
 	if (jffs2) {
 		cleanmarker.magic = cpu_to_je16 (JFFS2_MAGIC_BITMASK);
 		cleanmarker.nodetype = cpu_to_je16 (JFFS2_NODETYPE_CLEANMARKER);
-		if (!isNAND)
+		if (!isNAND) {
 			cleanmarker.totlen = cpu_to_je32(sizeof(cleanmarker));
-		else {
+		} else {
 			cleanmarker.totlen = cpu_to_je32(8);
+			cmlen = min(mtd.oobavail, 8);
 		}
 		cleanmarker.hdr_crc = cpu_to_je32(mtd_crc32(0, &cleanmarker, sizeof(cleanmarker) - 4));
 	}
@@ -242,7 +243,7 @@ int main(int argc, char *argv[])
 
 		/* write cleanmarker */
 		if (isNAND) {
-			if (mtd_write(mtd_desc, &mtd, fd, eb, 0, NULL, 0, &cleanmarker, 0,
+			if (mtd_write(mtd_desc, &mtd, fd, eb, 0, NULL, 0, &cleanmarker, cmlen,
 					MTD_OPS_AUTO_OOB) != 0) {
 				sys_errmsg("%s: MTD writeoob failure", mtd_device);
 				continue;
-- 
1.9.1

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[David Oberhollenzer]

Hi,

On 04/09/2018 05:10 AM, Xiaolei Li wrote:
> @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
>  		return -1;
>  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
>  		return -1;
> +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> +		return -1;
>  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
>  		return -1;
>  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))

I'm not sure if it is a good idea to do a hard fail here, since this
depends on a recent change to the kernel.

It might be preferable to catch and handle ENOENT, otherwise the next
release of mtd-utils will only work on the next kernel release onward.

Maybe mtd_oobavail could to be set to some reasonable default that
retains the current behaviour on "older" kernels?


Thanks,

David

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[David Oberhollenzer]

Hi,

On 04/09/2018 05:10 AM, Xiaolei Li wrote:
> @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
>  		return -1;
>  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
>  		return -1;
> +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> +		return -1;
>  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
>  		return -1;
>  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))

I'm not sure if it is a good idea to do a hard fail here, since this
depends on a recent change to the kernel.

It might be preferable to catch and handle ENOENT, otherwise the next
release of mtd-utils will only work on the next kernel release onward.

Maybe mtd_oobavail could to be set to some reasonable default that
retains the current behaviour on "older" kernels?


Thanks,

David

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

Hi David,

On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:
> Hi,
> 
> On 04/09/2018 05:10 AM, Xiaolei Li wrote:
> > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> >  		return -1;
> >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> >  		return -1;
> > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > +		return -1;
> >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> >  		return -1;
> >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))
> 
> I'm not sure if it is a good idea to do a hard fail here, since this
> depends on a recent change to the kernel.
> 
> It might be preferable to catch and handle ENOENT, otherwise the next
> release of mtd-utils will only work on the next kernel release onward.
> 
Yes, it is. The hard fail return here seems not good.

> Maybe mtd_oobavail could to be set to some reasonable default that
> retains the current behaviour on "older" kernels?
> 
What about setting 0 as default?

Thanks,
Xiaolei
> 
> Thanks,
> 
> David

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

Hi David,

On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:
> Hi,
> 
> On 04/09/2018 05:10 AM, Xiaolei Li wrote:
> > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> >  		return -1;
> >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> >  		return -1;
> > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > +		return -1;
> >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> >  		return -1;
> >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))
> 
> I'm not sure if it is a good idea to do a hard fail here, since this
> depends on a recent change to the kernel.
> 
> It might be preferable to catch and handle ENOENT, otherwise the next
> release of mtd-utils will only work on the next kernel release onward.
> 
Yes, it is. The hard fail return here seems not good.

> Maybe mtd_oobavail could to be set to some reasonable default that
> retains the current behaviour on "older" kernels?
> 
What about setting 0 as default?

Thanks,
Xiaolei
> 
> Thanks,
> 
> David

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[Boris Brezillon]

On Mon, 9 Apr 2018 15:25:39 +0800
xiaolei li <xiaolei.li@mediatek.com> wrote:

> Hi David,
> 
> On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:
> > Hi,
> > 
> > On 04/09/2018 05:10 AM, Xiaolei Li wrote:  
> > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > >  		return -1;
> > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > >  		return -1;
> > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > +		return -1;
> > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > >  		return -1;
> > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))  
> > 
> > I'm not sure if it is a good idea to do a hard fail here, since this
> > depends on a recent change to the kernel.
> > 
> > It might be preferable to catch and handle ENOENT, otherwise the next
> > release of mtd-utils will only work on the next kernel release onward.
> >   
> Yes, it is. The hard fail return here seems not good.
> 
> > Maybe mtd_oobavail could to be set to some reasonable default that
> > retains the current behaviour on "older" kernels?
> >   
> What about setting 0 as default?

I didn't look closely at the code yet, but shouldn't we do something
like:

1/ search for oobavail file in sysfs
2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
   this information
3/ if none of #1 and #2 are working, set oobavail to 0

Of course, all of this only makes sense if oobsize > 0. When that's not
the case, you can directly set oobavail to 0.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[Boris Brezillon]

On Mon, 9 Apr 2018 15:25:39 +0800
xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:

> Hi David,
> 
> On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:
> > Hi,
> > 
> > On 04/09/2018 05:10 AM, Xiaolei Li wrote:  
> > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > >  		return -1;
> > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > >  		return -1;
> > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > +		return -1;
> > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > >  		return -1;
> > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))  
> > 
> > I'm not sure if it is a good idea to do a hard fail here, since this
> > depends on a recent change to the kernel.
> > 
> > It might be preferable to catch and handle ENOENT, otherwise the next
> > release of mtd-utils will only work on the next kernel release onward.
> >   
> Yes, it is. The hard fail return here seems not good.
> 
> > Maybe mtd_oobavail could to be set to some reasonable default that
> > retains the current behaviour on "older" kernels?
> >   
> What about setting 0 as default?

I didn't look closely at the code yet, but shouldn't we do something
like:

1/ search for oobavail file in sysfs
2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
   this information
3/ if none of #1 and #2 are working, set oobavail to 0

Of course, all of this only makes sense if oobsize > 0. When that's not
the case, you can directly set oobavail to 0.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

Hi Boris,

On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> On Mon, 9 Apr 2018 15:25:39 +0800
> xiaolei li <xiaolei.li@mediatek.com> wrote:
> 
> > Hi David,
> > 
> > On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:
> > > Hi,
> > > 
> > > On 04/09/2018 05:10 AM, Xiaolei Li wrote:  
> > > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > > >  		return -1;
> > > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > > >  		return -1;
> > > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > > +		return -1;
> > > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > > >  		return -1;
> > > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))  
> > > 
> > > I'm not sure if it is a good idea to do a hard fail here, since this
> > > depends on a recent change to the kernel.
> > > 
> > > It might be preferable to catch and handle ENOENT, otherwise the next
> > > release of mtd-utils will only work on the next kernel release onward.
> > >   
> > Yes, it is. The hard fail return here seems not good.
> > 
> > > Maybe mtd_oobavail could to be set to some reasonable default that
> > > retains the current behaviour on "older" kernels?
> > >   
> > What about setting 0 as default?
> 
> I didn't look closely at the code yet, but shouldn't we do something
> like:
> 
> 1/ search for oobavail file in sysfs
> 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
>    this information
MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
here?

Thanks,
Xiaolei

> 3/ if none of #1 and #2 are working, set oobavail to 0
> 
> Of course, all of this only makes sense if oobsize > 0. When that's not
> the case, you can directly set oobavail to 0.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

Hi Boris,

On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> On Mon, 9 Apr 2018 15:25:39 +0800
> xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> 
> > Hi David,
> > 
> > On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:
> > > Hi,
> > > 
> > > On 04/09/2018 05:10 AM, Xiaolei Li wrote:  
> > > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > > >  		return -1;
> > > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > > >  		return -1;
> > > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > > +		return -1;
> > > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > > >  		return -1;
> > > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))  
> > > 
> > > I'm not sure if it is a good idea to do a hard fail here, since this
> > > depends on a recent change to the kernel.
> > > 
> > > It might be preferable to catch and handle ENOENT, otherwise the next
> > > release of mtd-utils will only work on the next kernel release onward.
> > >   
> > Yes, it is. The hard fail return here seems not good.
> > 
> > > Maybe mtd_oobavail could to be set to some reasonable default that
> > > retains the current behaviour on "older" kernels?
> > >   
> > What about setting 0 as default?
> 
> I didn't look closely at the code yet, but shouldn't we do something
> like:
> 
> 1/ search for oobavail file in sysfs
> 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
>    this information
MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
here?

Thanks,
Xiaolei

> 3/ if none of #1 and #2 are working, set oobavail to 0
> 
> Of course, all of this only makes sense if oobsize > 0. When that's not
> the case, you can directly set oobavail to 0.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[Boris Brezillon]

On Mon, 9 Apr 2018 16:33:11 +0800
xiaolei li <xiaolei.li@mediatek.com> wrote:

> Hi Boris,
> 
> On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> > On Mon, 9 Apr 2018 15:25:39 +0800
> > xiaolei li <xiaolei.li@mediatek.com> wrote:
> >   
> > > Hi David,
> > > 
> > > On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
> > > > Hi,
> > > > 
> > > > On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
> > > > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > > > >  		return -1;
> > > > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > > > >  		return -1;
> > > > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > > > +		return -1;
> > > > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > > > >  		return -1;
> > > > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
> > > > 
> > > > I'm not sure if it is a good idea to do a hard fail here, since this
> > > > depends on a recent change to the kernel.
> > > > 
> > > > It might be preferable to catch and handle ENOENT, otherwise the next
> > > > release of mtd-utils will only work on the next kernel release onward.
> > > >     
> > > Yes, it is. The hard fail return here seems not good.
> > >   
> > > > Maybe mtd_oobavail could to be set to some reasonable default that
> > > > retains the current behaviour on "older" kernels?
> > > >     
> > > What about setting 0 as default?  
> > 
> > I didn't look closely at the code yet, but shouldn't we do something
> > like:
> > 
> > 1/ search for oobavail file in sysfs
> > 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
> >    this information  
> MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
> here?

Yes we should, otherwise old kernels won't work with new versions of
mtd-utils.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[Boris Brezillon]

On Mon, 9 Apr 2018 16:33:11 +0800
xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:

> Hi Boris,
> 
> On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> > On Mon, 9 Apr 2018 15:25:39 +0800
> > xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >   
> > > Hi David,
> > > 
> > > On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
> > > > Hi,
> > > > 
> > > > On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
> > > > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > > > >  		return -1;
> > > > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > > > >  		return -1;
> > > > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > > > +		return -1;
> > > > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > > > >  		return -1;
> > > > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
> > > > 
> > > > I'm not sure if it is a good idea to do a hard fail here, since this
> > > > depends on a recent change to the kernel.
> > > > 
> > > > It might be preferable to catch and handle ENOENT, otherwise the next
> > > > release of mtd-utils will only work on the next kernel release onward.
> > > >     
> > > Yes, it is. The hard fail return here seems not good.
> > >   
> > > > Maybe mtd_oobavail could to be set to some reasonable default that
> > > > retains the current behaviour on "older" kernels?
> > > >     
> > > What about setting 0 as default?  
> > 
> > I didn't look closely at the code yet, but shouldn't we do something
> > like:
> > 
> > 1/ search for oobavail file in sysfs
> > 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
> >    this information  
> MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
> here?

Yes we should, otherwise old kernels won't work with new versions of
mtd-utils.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

On Mon, 2018-04-09 at 10:37 +0200, Boris Brezillon wrote:
> On Mon, 9 Apr 2018 16:33:11 +0800
> xiaolei li <xiaolei.li@mediatek.com> wrote:
> 
> > Hi Boris,
> > 
> > On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> > > On Mon, 9 Apr 2018 15:25:39 +0800
> > > xiaolei li <xiaolei.li@mediatek.com> wrote:
> > >   
> > > > Hi David,
> > > > 
> > > > On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
> > > > > Hi,
> > > > > 
> > > > > On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
> > > > > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > > > > >  		return -1;
> > > > > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > > > > >  		return -1;
> > > > > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > > > > +		return -1;
> > > > > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > > > > >  		return -1;
> > > > > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
> > > > > 
> > > > > I'm not sure if it is a good idea to do a hard fail here, since this
> > > > > depends on a recent change to the kernel.
> > > > > 
> > > > > It might be preferable to catch and handle ENOENT, otherwise the next
> > > > > release of mtd-utils will only work on the next kernel release onward.
> > > > >     
> > > > Yes, it is. The hard fail return here seems not good.
> > > >   
> > > > > Maybe mtd_oobavail could to be set to some reasonable default that
> > > > > retains the current behaviour on "older" kernels?
> > > > >     
> > > > What about setting 0 as default?  
> > > 
> > > I didn't look closely at the code yet, but shouldn't we do something
> > > like:
> > > 
> > > 1/ search for oobavail file in sysfs
> > > 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
> > >    this information  
> > MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
> > here?
> 
> Yes we should, otherwise old kernels won't work with new versions of
> mtd-utils.
OK.

@David, do you have other comments? If no, I will work for next patch to
add GETOOBSEL/GETECCLAYOUT ioctl support as Boris's suggestion.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

On Mon, 2018-04-09 at 10:37 +0200, Boris Brezillon wrote:
> On Mon, 9 Apr 2018 16:33:11 +0800
> xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> 
> > Hi Boris,
> > 
> > On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> > > On Mon, 9 Apr 2018 15:25:39 +0800
> > > xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> > >   
> > > > Hi David,
> > > > 
> > > > On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
> > > > > Hi,
> > > > > 
> > > > > On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
> > > > > > @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> > > > > >  		return -1;
> > > > > >  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> > > > > >  		return -1;
> > > > > > +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> > > > > > +		return -1;
> > > > > >  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> > > > > >  		return -1;
> > > > > >  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
> > > > > 
> > > > > I'm not sure if it is a good idea to do a hard fail here, since this
> > > > > depends on a recent change to the kernel.
> > > > > 
> > > > > It might be preferable to catch and handle ENOENT, otherwise the next
> > > > > release of mtd-utils will only work on the next kernel release onward.
> > > > >     
> > > > Yes, it is. The hard fail return here seems not good.
> > > >   
> > > > > Maybe mtd_oobavail could to be set to some reasonable default that
> > > > > retains the current behaviour on "older" kernels?
> > > > >     
> > > > What about setting 0 as default?  
> > > 
> > > I didn't look closely at the code yet, but shouldn't we do something
> > > like:
> > > 
> > > 1/ search for oobavail file in sysfs
> > > 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
> > >    this information  
> > MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
> > here?
> 
> Yes we should, otherwise old kernels won't work with new versions of
> mtd-utils.
OK.

@David, do you have other comments? If no, I will work for next patch to
add GETOOBSEL/GETECCLAYOUT ioctl support as Boris's suggestion.

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[David Oberhollenzer]

On 04/09/2018 10:45 AM, xiaolei li wrote:
> On Mon, 2018-04-09 at 10:37 +0200, Boris Brezillon wrote:
>> On Mon, 9 Apr 2018 16:33:11 +0800
>> xiaolei li <xiaolei.li@mediatek.com> wrote:
>>
>>> Hi Boris,
>>>
>>> On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
>>>> On Mon, 9 Apr 2018 15:25:39 +0800
>>>> xiaolei li <xiaolei.li@mediatek.com> wrote:
>>>>   
>>>>> Hi David,
>>>>>
>>>>> On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
>>>>>> Hi,
>>>>>>
>>>>>> On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
>>>>>>> @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
>>>>>>>  		return -1;
>>>>>>>  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
>>>>>>>  		return -1;
>>>>>>> +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
>>>>>>> +		return -1;
>>>>>>>  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
>>>>>>>  		return -1;
>>>>>>>  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
>>>>>>
>>>>>> I'm not sure if it is a good idea to do a hard fail here, since this
>>>>>> depends on a recent change to the kernel.
>>>>>>
>>>>>> It might be preferable to catch and handle ENOENT, otherwise the next
>>>>>> release of mtd-utils will only work on the next kernel release onward.
>>>>>>     
>>>>> Yes, it is. The hard fail return here seems not good.
>>>>>   
>>>>>> Maybe mtd_oobavail could to be set to some reasonable default that
>>>>>> retains the current behaviour on "older" kernels?
>>>>>>     
>>>>> What about setting 0 as default?  
>>>>
>>>> I didn't look closely at the code yet, but shouldn't we do something
>>>> like:
>>>>
>>>> 1/ search for oobavail file in sysfs
>>>> 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
>>>>    this information  
>>> MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
>>> here?
>>
>> Yes we should, otherwise old kernels won't work with new versions of
>> mtd-utils.
> OK.
> 
> @David, do you have other comments? If no, I will work for next patch to
> add GETOOBSEL/GETECCLAYOUT ioctl support as Boris's suggestion.
> 
> 

If a fallback to GETOOBSEL/GETECCLAYOUT works, I'm fine with that.

Thanks,

David

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[David Oberhollenzer]

On 04/09/2018 10:45 AM, xiaolei li wrote:
> On Mon, 2018-04-09 at 10:37 +0200, Boris Brezillon wrote:
>> On Mon, 9 Apr 2018 16:33:11 +0800
>> xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>>
>>> Hi Boris,
>>>
>>> On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
>>>> On Mon, 9 Apr 2018 15:25:39 +0800
>>>> xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
>>>>   
>>>>> Hi David,
>>>>>
>>>>> On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
>>>>>> Hi,
>>>>>>
>>>>>> On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
>>>>>>> @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
>>>>>>>  		return -1;
>>>>>>>  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
>>>>>>>  		return -1;
>>>>>>> +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
>>>>>>> +		return -1;
>>>>>>>  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
>>>>>>>  		return -1;
>>>>>>>  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
>>>>>>
>>>>>> I'm not sure if it is a good idea to do a hard fail here, since this
>>>>>> depends on a recent change to the kernel.
>>>>>>
>>>>>> It might be preferable to catch and handle ENOENT, otherwise the next
>>>>>> release of mtd-utils will only work on the next kernel release onward.
>>>>>>     
>>>>> Yes, it is. The hard fail return here seems not good.
>>>>>   
>>>>>> Maybe mtd_oobavail could to be set to some reasonable default that
>>>>>> retains the current behaviour on "older" kernels?
>>>>>>     
>>>>> What about setting 0 as default?  
>>>>
>>>> I didn't look closely at the code yet, but shouldn't we do something
>>>> like:
>>>>
>>>> 1/ search for oobavail file in sysfs
>>>> 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
>>>>    this information  
>>> MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
>>> here?
>>
>> Yes we should, otherwise old kernels won't work with new versions of
>> mtd-utils.
> OK.
> 
> @David, do you have other comments? If no, I will work for next patch to
> add GETOOBSEL/GETECCLAYOUT ioctl support as Boris's suggestion.
> 
> 

If a fallback to GETOOBSEL/GETECCLAYOUT works, I'm fine with that.

Thanks,

David

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

On Mon, 2018-04-09 at 10:53 +0200, David Oberhollenzer wrote:
> On 04/09/2018 10:45 AM, xiaolei li wrote:
> > On Mon, 2018-04-09 at 10:37 +0200, Boris Brezillon wrote:
> >> On Mon, 9 Apr 2018 16:33:11 +0800
> >> xiaolei li <xiaolei.li@mediatek.com> wrote:
> >>
> >>> Hi Boris,
> >>>
> >>> On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> >>>> On Mon, 9 Apr 2018 15:25:39 +0800
> >>>> xiaolei li <xiaolei.li@mediatek.com> wrote:
> >>>>   
> >>>>> Hi David,
> >>>>>
> >>>>> On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
> >>>>>>> @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> >>>>>>>  		return -1;
> >>>>>>>  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> >>>>>>>  		return -1;
> >>>>>>> +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> >>>>>>> +		return -1;
> >>>>>>>  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> >>>>>>>  		return -1;
> >>>>>>>  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
> >>>>>>
> >>>>>> I'm not sure if it is a good idea to do a hard fail here, since this
> >>>>>> depends on a recent change to the kernel.
> >>>>>>
> >>>>>> It might be preferable to catch and handle ENOENT, otherwise the next
> >>>>>> release of mtd-utils will only work on the next kernel release onward.
> >>>>>>     
> >>>>> Yes, it is. The hard fail return here seems not good.
> >>>>>   
> >>>>>> Maybe mtd_oobavail could to be set to some reasonable default that
> >>>>>> retains the current behaviour on "older" kernels?
> >>>>>>     
> >>>>> What about setting 0 as default?  
> >>>>
> >>>> I didn't look closely at the code yet, but shouldn't we do something
> >>>> like:
> >>>>
> >>>> 1/ search for oobavail file in sysfs
> >>>> 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
> >>>>    this information  
> >>> MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
> >>> here?
> >>
> >> Yes we should, otherwise old kernels won't work with new versions of
> >> mtd-utils.
> > OK.
> > 
> > @David, do you have other comments? If no, I will work for next patch to
> > add GETOOBSEL/GETECCLAYOUT ioctl support as Boris's suggestion.
> > 
> > 
> 
> If a fallback to GETOOBSEL/GETECCLAYOUT works, I'm fine with that.

OK. Thanks.

> 
> Thanks,
> 
> David

Re: [PATCH 1/2] libmtd: Add support to access OOB available size

[xiaolei li]

On Mon, 2018-04-09 at 10:53 +0200, David Oberhollenzer wrote:
> On 04/09/2018 10:45 AM, xiaolei li wrote:
> > On Mon, 2018-04-09 at 10:37 +0200, Boris Brezillon wrote:
> >> On Mon, 9 Apr 2018 16:33:11 +0800
> >> xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >>
> >>> Hi Boris,
> >>>
> >>> On Mon, 2018-04-09 at 09:35 +0200, Boris Brezillon wrote:
> >>>> On Mon, 9 Apr 2018 15:25:39 +0800
> >>>> xiaolei li <xiaolei.li-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> wrote:
> >>>>   
> >>>>> Hi David,
> >>>>>
> >>>>> On Mon, 2018-04-09 at 08:58 +0200, David Oberhollenzer wrote:  
> >>>>>> Hi,
> >>>>>>
> >>>>>> On 04/09/2018 05:10 AM, Xiaolei Li wrote:    
> >>>>>>> @@ -769,6 +774,8 @@ int mtd_get_dev_info1(libmtd_t desc, int mtd_num, struct mtd_dev_info *mtd)
> >>>>>>>  		return -1;
> >>>>>>>  	if (dev_read_pos_int(lib->mtd_oob_size, mtd_num, &mtd->oob_size))
> >>>>>>>  		return -1;
> >>>>>>> +	if (dev_read_pos_int(lib->mtd_oobavail, mtd_num, &mtd->oobavail))
> >>>>>>> +		return -1;
> >>>>>>>  	if (dev_read_pos_int(lib->mtd_region_cnt, mtd_num, &mtd->region_cnt))
> >>>>>>>  		return -1;
> >>>>>>>  	if (dev_read_hex_int(lib->mtd_flags, mtd_num, &ret))    
> >>>>>>
> >>>>>> I'm not sure if it is a good idea to do a hard fail here, since this
> >>>>>> depends on a recent change to the kernel.
> >>>>>>
> >>>>>> It might be preferable to catch and handle ENOENT, otherwise the next
> >>>>>> release of mtd-utils will only work on the next kernel release onward.
> >>>>>>     
> >>>>> Yes, it is. The hard fail return here seems not good.
> >>>>>   
> >>>>>> Maybe mtd_oobavail could to be set to some reasonable default that
> >>>>>> retains the current behaviour on "older" kernels?
> >>>>>>     
> >>>>> What about setting 0 as default?  
> >>>>
> >>>> I didn't look closely at the code yet, but shouldn't we do something
> >>>> like:
> >>>>
> >>>> 1/ search for oobavail file in sysfs
> >>>> 2/ if it's not there use the GETOOBSEL or GETECCLAYOUT ioctl to get
> >>>>    this information  
> >>> MEMGETOOBSEL and GETECCLAYOUT are obsoleted. Should we keep using them
> >>> here?
> >>
> >> Yes we should, otherwise old kernels won't work with new versions of
> >> mtd-utils.
> > OK.
> > 
> > @David, do you have other comments? If no, I will work for next patch to
> > add GETOOBSEL/GETECCLAYOUT ioctl support as Boris's suggestion.
> > 
> > 
> 
> If a fallback to GETOOBSEL/GETECCLAYOUT works, I'm fine with that.

OK. Thanks.

> 
> Thanks,
> 
> David