| Message ID | 1523243410-65424-2-git-send-email-xiaolei.li@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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.
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.
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.
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.
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
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
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;
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(+)