Re: [RFC] [PATCH] [MTD] Update internal API to support 64-bit device size

Artem Bityutskiy
2008-11-14T11:54:33Z

There are 2 compilation warnings:

fs/jffs2/erase.c: In function ‘jffs2_erase_failed’:
fs/jffs2/erase.c:178: warning: comparison is always true due to limited range of data type
fs/jffs2/erase.c: In function ‘jffs2_erase_callback’:
fs/jffs2/erase.c:212: warning: format ‘%08x’ expects type ‘unsigned int’, but argument 2 has type ‘u_int64_t’

And the first one points to a real problem.

> diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h
> index eae26bb..139aeb7 100644
> --- a/include/linux/mtd/mtd.h
> +++ b/include/linux/mtd/mtd.h
> @@ -15,6 +15,8 @@
> #include <linux/mtd/compatmac.h>
> #include <mtd/mtd-abi.h>
>
> +#include <asm/div64.h>
> +
> #define MTD_CHAR_MAJOR 90
> #define MTD_BLOCK_MAJOR 31
> #define MAX_MTD_DEVICES 32
> @@ -25,16 +27,16 @@
> #define MTD_ERASE_DONE 0x08
> #define MTD_ERASE_FAILED 0x10
>
> -#define MTD_FAIL_ADDR_UNKNOWN 0xffffffff
> +#define MTD_FAIL_ADDR_UNKNOWN -1LL

OK, but look at the 'jffs2_erase_failed()' function:

--------------------------------------------------------------
static void jffs2_erase_failed(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t bad_offset)
{
/* For NAND, if the failure did not occur at the device level for a
specific physical page, don't bother updating the bad block table. */
if (jffs2_cleanmarker_oob(c) && (bad_offset != MTD_FAIL_ADDR_UNKNOWN)) {
/* We had a device-level failure to erase. Let's see if we've
failed too many times. */
if (!jffs2_write_nand_badblock(c, jeb, bad_offset)) {
/* We'd like to give this block another try. */
... etc ...
--------------------------------------------------------------

bad_offs is uint32_t, and bad_offset != -1LL is always true, which is
a problem.

> +static inline u_int32_t mtd_div_by_ws(u_int64_t sz, struct mtd_info
> *mtd)
> +{
> + if (mtd->writesize_shift)
> + return sz >> mtd->writesize_shift;
> + do_div(sz, mtd->writesize);
> + return sz;
> +}

I would introduce mtd->writesize_mask instead of calculating it, because
it is presumably more optimal.

> +
> +static inline u_int32_t mtd_mod_by_ws(u_int64_t sz, struct mtd_info
> *mtd)
> +{
> + if (mtd->writesize_shift)
> + return sz & ((1 << mtd->writesize_shift) - 1);
> + return do_div(sz, mtd->writesize);
> +}

Ditto.

--
Best regards,
Artem Bityutskiy (Битюцкий Артём)