[PATCH v2 0/3] fat: Added functions to determine the FAT variant (12/16/32bit)

[PATCH v2 0/3] fat: Added functions to determine the FAT variant (12/16/32bit)

[Carmeli Tamir]

Along the FAT FS code, the FAT variant (whether this is FAT12, FAT16 or FAT32) is
determined by checking the fat_bits field of struct msdos_sb_info. 
This is somewhat error prone as it forces the usage of magics (12, 16, 32)
multiple times in the code.

This series replaces the places in which the variant is checked with three
inline functions - IS_FAT12, IS_FAT16 and IS_FAT16.

The introduction of these simple inline functions makes a clearer API for determining the variant,
rather than searching the code for some field in a struct, and therefore
increases the code's maintainability and readability.

In addition, minor cleanups around code that checks for the FAT variant,
and fixed comments from v1.

Carmeli Tamir (3):
  Removed fat_first_ent
  Moved and inlined MAX_FAT
  IS_FAT functions

 fs/fat/cache.c                |  2 +-
 fs/fat/dir.c                  |  4 ++--
 fs/fat/fat.h                  | 30 +++++++++++++++++++++++++++++-
 fs/fat/fatent.c               | 16 +++++++---------
 fs/fat/inode.c                | 24 ++++++++++++++----------
 fs/fat/misc.c                 |  2 +-
 include/uapi/linux/msdos_fs.h |  5 -----
 7 files changed, 54 insertions(+), 29 deletions(-)

-- 
2.7.4

[PATCH v2 1/3] fat: Removed FAT_FIRST_ENT macro

[Carmeli Tamir]

The comment edited in this patch was the only reference to the 
FAT_FIRST_ENT macro, which is not used anymore. Moreover, the
commented line of code does not compile with the current code.

Since the FAT_FIRST_ENT macro checks the FAT variant in a way that the
patch series changes, I removed it, and instead wrote a clear explanation
of what was checked.

I verified that the changed comment is correct according to Microsoft FAT
spec, search for "BPB_Media" in the following references:

1. Microsoft FAT specification 2005
(http://read.pudn.com/downloads77/ebook/294884/FAT32%20Spec%20%28SDA%20Contribution%29.pdf).
Search for 'volume label'.
2. Microsoft Extensible Firmware Initiative, FAT32 File System Specification
(https://staff.washington.edu/dittrich/misc/fatgen103.pdf).
Search for 'volume label'.


Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
 fs/fat/inode.c                | 12 ++++++++----
 include/uapi/linux/msdos_fs.h |  3 ---
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index e981e9d..708de6d 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -1804,11 +1804,15 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	fat_ent_access_init(sb);
 
 	/*
-	 * The low byte of FAT's first entry must have same value with
-	 * media-field.  But in real world, too many devices is
-	 * writing wrong value.  So, removed that validity check.
+	 * The low byte of the first FAT entry must have the same value as
+	 * the media field of the boot sector. But in real world, too many
+	 * devices are writing wrong values. So, removed that validity check.
 	 *
-	 * if (FAT_FIRST_ENT(sb, media) != first)
+	 * The removed check compared the first FAT entry to a value dependent
+	 * on the media field like this:
+	 * == (0x0F00 | media), for FAT12
+	 * == (0XFF00 | media), for FAT16
+	 * == (0x0FFFFF | media), for FAT32
 	 */
 
 	error = -EINVAL;
diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
index 1216e6c..833c707 100644
--- a/include/uapi/linux/msdos_fs.h
+++ b/include/uapi/linux/msdos_fs.h
@@ -58,9 +58,6 @@
 #define MSDOS_DOT	".          "	/* ".", padded to MSDOS_NAME chars */
 #define MSDOS_DOTDOT	"..         "	/* "..", padded to MSDOS_NAME chars */
 
-#define FAT_FIRST_ENT(s, x)	((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
-	MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
-
 /* start of data cluster's entry (number of reserved clusters) */
 #define FAT_START_ENT	2
 
-- 
2.7.4

[PATCH v2 2/3] fat: Moved MAX_FAT to fat.h and changed it to inline function

[Carmeli Tamir]

MAX_FAT is useless in msdos_fs.h, since it uses the MSDOS_SB function
that is defined in fat.h. So really, this macro can be only called
from code that already includes fat.h.

Hence, this patch moves it to fat.h, right after MSDOS_SB is defined.
I also changed it to an inline function in order to save the double call
to MSDOS_SB. This was suggested by joe@perches.com in the previous
version.

This patch is required for the next in the series, in which the variant
(whether this is FAT12, FAT16 or FAT32) checks are replaced with new 
macros.


Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
 fs/fat/fat.h                  | 9 +++++++++
 include/uapi/linux/msdos_fs.h | 2 --
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 4e1b2f6..11bc4a2 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -142,6 +142,15 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
+/* Maximum number of clusters */
+static inline u32 MAX_FAT(struct super_block *sb)
+{
+	struct msdos_sb_info *sbi = MSDOS_SB(sb);
+
+	return sbi->fat_bits == 32 ? MAX_FAT32 :
+		sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
+}
+
 static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
 {
 	return container_of(inode, struct msdos_inode_info, vfs_inode);
diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
index 833c707..a577389 100644
--- a/include/uapi/linux/msdos_fs.h
+++ b/include/uapi/linux/msdos_fs.h
@@ -65,8 +65,6 @@
 #define MAX_FAT12	0xFF4
 #define MAX_FAT16	0xFFF4
 #define MAX_FAT32	0x0FFFFFF6
-#define MAX_FAT(s)	(MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
-	MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
 
 /* bad cluster mark */
 #define BAD_FAT12	0xFF7
-- 
2.7.4

[PATCH v2 3/3] fat: New inline functions to determine the FAT variant (32, 16 or 12)

[Carmeli Tamir]

This patch introduces 3 new inline functions - IS_FAT12, IS_FAT16 and
IS_FAT32, and replaces every occurrence in the code in which the FS 
variant (whether this is FAT12, FAT16 or FAT32) was previously checked 
using msdos_sb_info->fat_bits.


Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
---
 fs/fat/cache.c  |  2 +-
 fs/fat/dir.c    |  4 ++--
 fs/fat/fat.h    | 25 ++++++++++++++++++++++---
 fs/fat/fatent.c | 16 +++++++---------
 fs/fat/inode.c  | 12 ++++++------
 fs/fat/misc.c   |  2 +-
 6 files changed, 39 insertions(+), 22 deletions(-)

diff --git a/fs/fat/cache.c b/fs/fat/cache.c
index 78d501c..30c51b9 100644
--- a/fs/fat/cache.c
+++ b/fs/fat/cache.c
@@ -363,7 +363,7 @@ int fat_bmap(struct inode *inode, sector_t sector, sector_t *phys,
 
 	*phys = 0;
 	*mapped_blocks = 0;
-	if ((sbi->fat_bits != 32) && (inode->i_ino == MSDOS_ROOT_INO)) {
+	if (!IS_FAT32(sbi) && (inode->i_ino == MSDOS_ROOT_INO)) {
 		if (sector < (sbi->dir_entries >> sbi->dir_per_block_bits)) {
 			*phys = sector + sbi->dir_start;
 			*mapped_blocks = 1;
diff --git a/fs/fat/dir.c b/fs/fat/dir.c
index c8366cb..b0b8f44 100644
--- a/fs/fat/dir.c
+++ b/fs/fat/dir.c
@@ -57,7 +57,7 @@ static inline void fat_dir_readahead(struct inode *dir, sector_t iblock,
 	if ((iblock & (sbi->sec_per_clus - 1)) || sbi->sec_per_clus == 1)
 		return;
 	/* root dir of FAT12/FAT16 */
-	if ((sbi->fat_bits != 32) && (dir->i_ino == MSDOS_ROOT_INO))
+	if (!IS_FAT32(sbi) && (dir->i_ino == MSDOS_ROOT_INO))
 		return;
 
 	bh = sb_find_get_block(sb, phys);
@@ -1313,7 +1313,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
 		}
 	}
 	if (dir->i_ino == MSDOS_ROOT_INO) {
-		if (sbi->fat_bits != 32)
+		if (!IS_FAT32(sbi))
 			goto error;
 	} else if (MSDOS_I(dir)->i_start == 0) {
 		fat_msg(sb, KERN_ERR, "Corrupted directory (i_pos %lld)",
diff --git a/fs/fat/fat.h b/fs/fat/fat.h
index 11bc4a2..5b6f1c8 100644
--- a/fs/fat/fat.h
+++ b/fs/fat/fat.h
@@ -142,13 +142,32 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
 	return sb->s_fs_info;
 }
 
+/*
+ * Functions that determine the variant of the FAT file system (i.e.,
+ * whether this is FAT12, FAT16 or FAT32.
+ */
+static inline bool IS_FAT12(const struct msdos_sb_info *sbi)
+{
+	return sbi->fat_bits == 12;
+}
+
+static inline bool IS_FAT16(const struct msdos_sb_info *sbi)
+{
+	return sbi->fat_bits == 16;
+}
+
+static inline bool IS_FAT32(const struct msdos_sb_info *sbi)
+{
+	return sbi->fat_bits == 32;
+}
+
 /* Maximum number of clusters */
 static inline u32 MAX_FAT(struct super_block *sb)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
-	return sbi->fat_bits == 32 ? MAX_FAT32 :
-		sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
+	return IS_FAT32(sbi) ? MAX_FAT32 :
+		IS_FAT16(sbi) ? MAX_FAT16 : MAX_FAT12;
 }
 
 static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
@@ -266,7 +285,7 @@ static inline int fat_get_start(const struct msdos_sb_info *sbi,
 				const struct msdos_dir_entry *de)
 {
 	int cluster = le16_to_cpu(de->start);
-	if (sbi->fat_bits == 32)
+	if (IS_FAT32(sbi))
 		cluster |= (le16_to_cpu(de->starthi) << 16);
 	return cluster;
 }
diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
index f58c0ca..9166d96 100644
--- a/fs/fat/fatent.c
+++ b/fs/fat/fatent.c
@@ -290,19 +290,17 @@ void fat_ent_access_init(struct super_block *sb)
 
 	mutex_init(&sbi->fat_lock);
 
-	switch (sbi->fat_bits) {
-	case 32:
+	if (IS_FAT32(sbi)) {
 		sbi->fatent_shift = 2;
 		sbi->fatent_ops = &fat32_ops;
-		break;
-	case 16:
+	} else if (IS_FAT16(sbi)) {
 		sbi->fatent_shift = 1;
 		sbi->fatent_ops = &fat16_ops;
-		break;
-	case 12:
+	} else if (IS_FAT12(sbi)) {
 		sbi->fatent_shift = -1;
 		sbi->fatent_ops = &fat12_ops;
-		break;
+	} else {
+		fat_fs_error(sb, "invalid FAT variant, %u bits", sbi->fat_bits);
 	}
 }
 
@@ -310,7 +308,7 @@ static void mark_fsinfo_dirty(struct super_block *sb)
 {
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
-	if (sb_rdonly(sb) || sbi->fat_bits != 32)
+	if (sb_rdonly(sb) || !IS_FAT32(sbi))
 		return;
 
 	__mark_inode_dirty(sbi->fsinfo_inode, I_DIRTY_SYNC);
@@ -327,7 +325,7 @@ static inline int fat_ent_update_ptr(struct super_block *sb,
 	/* Is this fatent's blocks including this entry? */
 	if (!fatent->nr_bhs || bhs[0]->b_blocknr != blocknr)
 		return 0;
-	if (sbi->fat_bits == 12) {
+	if (IS_FAT12(sbi)) {
 		if ((offset + 1) < sb->s_blocksize) {
 			/* This entry is on bhs[0]. */
 			if (fatent->nr_bhs == 2) {
diff --git a/fs/fat/inode.c b/fs/fat/inode.c
index 708de6d..a84b61b 100644
--- a/fs/fat/inode.c
+++ b/fs/fat/inode.c
@@ -686,7 +686,7 @@ static void fat_set_state(struct super_block *sb,
 
 	b = (struct fat_boot_sector *) bh->b_data;
 
-	if (sbi->fat_bits == 32) {
+	if (IS_FAT32(sbi)) {
 		if (set)
 			b->fat32.state |= FAT_STATE_DIRTY;
 		else
@@ -1397,7 +1397,7 @@ static int fat_read_root(struct inode *inode)
 	inode->i_mode = fat_make_mode(sbi, ATTR_DIR, S_IRWXUGO);
 	inode->i_op = sbi->dir_ops;
 	inode->i_fop = &fat_dir_operations;
-	if (sbi->fat_bits == 32) {
+	if (IS_FAT32(sbi)) {
 		MSDOS_I(inode)->i_start = sbi->root_cluster;
 		error = fat_calc_dir_size(inode);
 		if (error < 0)
@@ -1424,7 +1424,7 @@ static unsigned long calc_fat_clusters(struct super_block *sb)
 	struct msdos_sb_info *sbi = MSDOS_SB(sb);
 
 	/* Divide first to avoid overflow */
-	if (sbi->fat_bits != 12) {
+	if (!IS_FAT12(sbi)) {
 		unsigned long ent_per_sec = sb->s_blocksize * 8 / sbi->fat_bits;
 		return ent_per_sec * sbi->fat_length;
 	}
@@ -1744,7 +1744,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 	}
 
 	/* interpret volume ID as a little endian 32 bit integer */
-	if (sbi->fat_bits == 32)
+	if (IS_FAT32(sbi))
 		sbi->vol_id = bpb.fat32_vol_id;
 	else /* fat 16 or 12 */
 		sbi->vol_id = bpb.fat16_vol_id;
@@ -1770,11 +1770,11 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
 
 	total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
 
-	if (sbi->fat_bits != 32)
+	if (!IS_FAT32(sbi))
 		sbi->fat_bits = (total_clusters > MAX_FAT12) ? 16 : 12;
 
 	/* some OSes set FAT_STATE_DIRTY and clean it on unmount. */
-	if (sbi->fat_bits == 32)
+	if (IS_FAT32(sbi))
 		sbi->dirty = bpb.fat32_state & FAT_STATE_DIRTY;
 	else /* fat 16 or 12 */
 		sbi->dirty = bpb.fat16_state & FAT_STATE_DIRTY;
diff --git a/fs/fat/misc.c b/fs/fat/misc.c
index fce0a76..5368c6a 100644
--- a/fs/fat/misc.c
+++ b/fs/fat/misc.c
@@ -64,7 +64,7 @@ int fat_clusters_flush(struct super_block *sb)
 	struct buffer_head *bh;
 	struct fat_boot_fsinfo *fsinfo;
 
-	if (sbi->fat_bits != 32)
+	if (!IS_FAT32(sbi))
 		return 0;
 
 	bh = sb_bread(sb, sbi->fsinfo_sector);
-- 
2.7.4

Re: [PATCH v2 2/3] fat: Moved MAX_FAT to fat.h and changed it to inline function

[OGAWA Hirofumi]

Carmeli Tamir <carmeli.tamir@gmail.com> writes:

> MAX_FAT is useless in msdos_fs.h, since it uses the MSDOS_SB function
> that is defined in fat.h. So really, this macro can be only called
> from code that already includes fat.h.
>
> Hence, this patch moves it to fat.h, right after MSDOS_SB is defined.
> I also changed it to an inline function in order to save the double call
> to MSDOS_SB. This was suggested by joe@perches.com in the previous
> version.
>
> This patch is required for the next in the series, in which the variant
> (whether this is FAT12, FAT16 or FAT32) checks are replaced with new 
> macros.

Could you use lower case chars for inline functions? Yeah, MSDOS_SB() is
upper case though, it is historical reason.

Thanks.

> Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
> ---
>  fs/fat/fat.h                  | 9 +++++++++
>  include/uapi/linux/msdos_fs.h | 2 --
>  2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 4e1b2f6..11bc4a2 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -142,6 +142,15 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
>  	return sb->s_fs_info;
>  }
>  
> +/* Maximum number of clusters */
> +static inline u32 MAX_FAT(struct super_block *sb)
> +{
> +	struct msdos_sb_info *sbi = MSDOS_SB(sb);
> +
> +	return sbi->fat_bits == 32 ? MAX_FAT32 :
> +		sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
> +}
> +
>  static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
>  {
>  	return container_of(inode, struct msdos_inode_info, vfs_inode);
> diff --git a/include/uapi/linux/msdos_fs.h b/include/uapi/linux/msdos_fs.h
> index 833c707..a577389 100644
> --- a/include/uapi/linux/msdos_fs.h
> +++ b/include/uapi/linux/msdos_fs.h
> @@ -65,8 +65,6 @@
>  #define MAX_FAT12	0xFF4
>  #define MAX_FAT16	0xFFF4
>  #define MAX_FAT32	0x0FFFFFF6
> -#define MAX_FAT(s)	(MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
> -	MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
>  
>  /* bad cluster mark */
>  #define BAD_FAT12	0xFF7

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 3/3] fat: New inline functions to determine the FAT variant (32, 16 or 12)

[OGAWA Hirofumi]

Carmeli Tamir <carmeli.tamir@gmail.com> writes:

> This patch introduces 3 new inline functions - IS_FAT12, IS_FAT16 and
> IS_FAT32, and replaces every occurrence in the code in which the FS 
> variant (whether this is FAT12, FAT16 or FAT32) was previously checked 
> using msdos_sb_info->fat_bits.

Could you use lower case chars for inline functions?

> Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
> ---
>  fs/fat/cache.c  |  2 +-
>  fs/fat/dir.c    |  4 ++--
>  fs/fat/fat.h    | 25 ++++++++++++++++++++++---
>  fs/fat/fatent.c | 16 +++++++---------
>  fs/fat/inode.c  | 12 ++++++------
>  fs/fat/misc.c   |  2 +-
>  6 files changed, 39 insertions(+), 22 deletions(-)
>
> diff --git a/fs/fat/cache.c b/fs/fat/cache.c
> index 78d501c..30c51b9 100644
> --- a/fs/fat/cache.c
> +++ b/fs/fat/cache.c
> @@ -363,7 +363,7 @@ int fat_bmap(struct inode *inode, sector_t sector, sector_t *phys,
>  
>  	*phys = 0;
>  	*mapped_blocks = 0;
> -	if ((sbi->fat_bits != 32) && (inode->i_ino == MSDOS_ROOT_INO)) {
> +	if (!IS_FAT32(sbi) && (inode->i_ino == MSDOS_ROOT_INO)) {
>  		if (sector < (sbi->dir_entries >> sbi->dir_per_block_bits)) {
>  			*phys = sector + sbi->dir_start;
>  			*mapped_blocks = 1;
> diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> index c8366cb..b0b8f44 100644
> --- a/fs/fat/dir.c
> +++ b/fs/fat/dir.c
> @@ -57,7 +57,7 @@ static inline void fat_dir_readahead(struct inode *dir, sector_t iblock,
>  	if ((iblock & (sbi->sec_per_clus - 1)) || sbi->sec_per_clus == 1)
>  		return;
>  	/* root dir of FAT12/FAT16 */
> -	if ((sbi->fat_bits != 32) && (dir->i_ino == MSDOS_ROOT_INO))
> +	if (!IS_FAT32(sbi) && (dir->i_ino == MSDOS_ROOT_INO))
>  		return;
>  
>  	bh = sb_find_get_block(sb, phys);
> @@ -1313,7 +1313,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
>  		}
>  	}
>  	if (dir->i_ino == MSDOS_ROOT_INO) {
> -		if (sbi->fat_bits != 32)
> +		if (!IS_FAT32(sbi))
>  			goto error;
>  	} else if (MSDOS_I(dir)->i_start == 0) {
>  		fat_msg(sb, KERN_ERR, "Corrupted directory (i_pos %lld)",
> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> index 11bc4a2..5b6f1c8 100644
> --- a/fs/fat/fat.h
> +++ b/fs/fat/fat.h
> @@ -142,13 +142,32 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
>  	return sb->s_fs_info;
>  }
>  
> +/*
> + * Functions that determine the variant of the FAT file system (i.e.,
> + * whether this is FAT12, FAT16 or FAT32.
> + */
> +static inline bool IS_FAT12(const struct msdos_sb_info *sbi)
> +{
> +	return sbi->fat_bits == 12;
> +}
> +
> +static inline bool IS_FAT16(const struct msdos_sb_info *sbi)
> +{
> +	return sbi->fat_bits == 16;
> +}
> +
> +static inline bool IS_FAT32(const struct msdos_sb_info *sbi)
> +{
> +	return sbi->fat_bits == 32;
> +}
> +
>  /* Maximum number of clusters */
>  static inline u32 MAX_FAT(struct super_block *sb)
>  {
>  	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>  
> -	return sbi->fat_bits == 32 ? MAX_FAT32 :
> -		sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
> +	return IS_FAT32(sbi) ? MAX_FAT32 :
> +		IS_FAT16(sbi) ? MAX_FAT16 : MAX_FAT12;
>  }
>  
>  static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
> @@ -266,7 +285,7 @@ static inline int fat_get_start(const struct msdos_sb_info *sbi,
>  				const struct msdos_dir_entry *de)
>  {
>  	int cluster = le16_to_cpu(de->start);
> -	if (sbi->fat_bits == 32)
> +	if (IS_FAT32(sbi))
>  		cluster |= (le16_to_cpu(de->starthi) << 16);
>  	return cluster;
>  }
> diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> index f58c0ca..9166d96 100644
> --- a/fs/fat/fatent.c
> +++ b/fs/fat/fatent.c
> @@ -290,19 +290,17 @@ void fat_ent_access_init(struct super_block *sb)
>  
>  	mutex_init(&sbi->fat_lock);
>  
> -	switch (sbi->fat_bits) {
> -	case 32:
> +	if (IS_FAT32(sbi)) {
>  		sbi->fatent_shift = 2;
>  		sbi->fatent_ops = &fat32_ops;
> -		break;
> -	case 16:
> +	} else if (IS_FAT16(sbi)) {
>  		sbi->fatent_shift = 1;
>  		sbi->fatent_ops = &fat16_ops;
> -		break;
> -	case 12:
> +	} else if (IS_FAT12(sbi)) {
>  		sbi->fatent_shift = -1;
>  		sbi->fatent_ops = &fat12_ops;
> -		break;
> +	} else {
> +		fat_fs_error(sb, "invalid FAT variant, %u bits", sbi->fat_bits);
>  	}
>  }
>  
> @@ -310,7 +308,7 @@ static void mark_fsinfo_dirty(struct super_block *sb)
>  {
>  	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>  
> -	if (sb_rdonly(sb) || sbi->fat_bits != 32)
> +	if (sb_rdonly(sb) || !IS_FAT32(sbi))
>  		return;
>  
>  	__mark_inode_dirty(sbi->fsinfo_inode, I_DIRTY_SYNC);
> @@ -327,7 +325,7 @@ static inline int fat_ent_update_ptr(struct super_block *sb,
>  	/* Is this fatent's blocks including this entry? */
>  	if (!fatent->nr_bhs || bhs[0]->b_blocknr != blocknr)
>  		return 0;
> -	if (sbi->fat_bits == 12) {
> +	if (IS_FAT12(sbi)) {
>  		if ((offset + 1) < sb->s_blocksize) {
>  			/* This entry is on bhs[0]. */
>  			if (fatent->nr_bhs == 2) {
> diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> index 708de6d..a84b61b 100644
> --- a/fs/fat/inode.c
> +++ b/fs/fat/inode.c
> @@ -686,7 +686,7 @@ static void fat_set_state(struct super_block *sb,
>  
>  	b = (struct fat_boot_sector *) bh->b_data;
>  
> -	if (sbi->fat_bits == 32) {
> +	if (IS_FAT32(sbi)) {
>  		if (set)
>  			b->fat32.state |= FAT_STATE_DIRTY;
>  		else
> @@ -1397,7 +1397,7 @@ static int fat_read_root(struct inode *inode)
>  	inode->i_mode = fat_make_mode(sbi, ATTR_DIR, S_IRWXUGO);
>  	inode->i_op = sbi->dir_ops;
>  	inode->i_fop = &fat_dir_operations;
> -	if (sbi->fat_bits == 32) {
> +	if (IS_FAT32(sbi)) {
>  		MSDOS_I(inode)->i_start = sbi->root_cluster;
>  		error = fat_calc_dir_size(inode);
>  		if (error < 0)
> @@ -1424,7 +1424,7 @@ static unsigned long calc_fat_clusters(struct super_block *sb)
>  	struct msdos_sb_info *sbi = MSDOS_SB(sb);
>  
>  	/* Divide first to avoid overflow */
> -	if (sbi->fat_bits != 12) {
> +	if (!IS_FAT12(sbi)) {
>  		unsigned long ent_per_sec = sb->s_blocksize * 8 / sbi->fat_bits;
>  		return ent_per_sec * sbi->fat_length;
>  	}
> @@ -1744,7 +1744,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
>  	}
>  
>  	/* interpret volume ID as a little endian 32 bit integer */
> -	if (sbi->fat_bits == 32)
> +	if (IS_FAT32(sbi))
>  		sbi->vol_id = bpb.fat32_vol_id;
>  	else /* fat 16 or 12 */
>  		sbi->vol_id = bpb.fat16_vol_id;
> @@ -1770,11 +1770,11 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
>  
>  	total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
>  
> -	if (sbi->fat_bits != 32)
> +	if (!IS_FAT32(sbi))
>  		sbi->fat_bits = (total_clusters > MAX_FAT12) ? 16 : 12;
>  
>  	/* some OSes set FAT_STATE_DIRTY and clean it on unmount. */
> -	if (sbi->fat_bits == 32)
> +	if (IS_FAT32(sbi))
>  		sbi->dirty = bpb.fat32_state & FAT_STATE_DIRTY;
>  	else /* fat 16 or 12 */
>  		sbi->dirty = bpb.fat16_state & FAT_STATE_DIRTY;
> diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> index fce0a76..5368c6a 100644
> --- a/fs/fat/misc.c
> +++ b/fs/fat/misc.c
> @@ -64,7 +64,7 @@ int fat_clusters_flush(struct super_block *sb)
>  	struct buffer_head *bh;
>  	struct fat_boot_fsinfo *fsinfo;
>  
> -	if (sbi->fat_bits != 32)
> +	if (!IS_FAT32(sbi))
>  		return 0;
>  
>  	bh = sb_bread(sb, sbi->fsinfo_sector);

-- 
OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>

Re: [PATCH v2 3/3] fat: New inline functions to determine the FAT variant (32, 16 or 12)

[Tamir Carmeli]

Sure. Published v3 with lower case chars. Fixed in both relevant patches.

On Sat, Dec 15, 2018 at 9:10 PM OGAWA Hirofumi
<hirofumi@mail.parknet.co.jp> wrote:
>
> Carmeli Tamir <carmeli.tamir@gmail.com> writes:
>
> > This patch introduces 3 new inline functions - IS_FAT12, IS_FAT16 and
> > IS_FAT32, and replaces every occurrence in the code in which the FS
> > variant (whether this is FAT12, FAT16 or FAT32) was previously checked
> > using msdos_sb_info->fat_bits.
>
> Could you use lower case chars for inline functions?
>
> > Signed-off-by: Carmeli Tamir <carmeli.tamir@gmail.com>
> > ---
> >  fs/fat/cache.c  |  2 +-
> >  fs/fat/dir.c    |  4 ++--
> >  fs/fat/fat.h    | 25 ++++++++++++++++++++++---
> >  fs/fat/fatent.c | 16 +++++++---------
> >  fs/fat/inode.c  | 12 ++++++------
> >  fs/fat/misc.c   |  2 +-
> >  6 files changed, 39 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/fat/cache.c b/fs/fat/cache.c
> > index 78d501c..30c51b9 100644
> > --- a/fs/fat/cache.c
> > +++ b/fs/fat/cache.c
> > @@ -363,7 +363,7 @@ int fat_bmap(struct inode *inode, sector_t sector, sector_t *phys,
> >
> >       *phys = 0;
> >       *mapped_blocks = 0;
> > -     if ((sbi->fat_bits != 32) && (inode->i_ino == MSDOS_ROOT_INO)) {
> > +     if (!IS_FAT32(sbi) && (inode->i_ino == MSDOS_ROOT_INO)) {
> >               if (sector < (sbi->dir_entries >> sbi->dir_per_block_bits)) {
> >                       *phys = sector + sbi->dir_start;
> >                       *mapped_blocks = 1;
> > diff --git a/fs/fat/dir.c b/fs/fat/dir.c
> > index c8366cb..b0b8f44 100644
> > --- a/fs/fat/dir.c
> > +++ b/fs/fat/dir.c
> > @@ -57,7 +57,7 @@ static inline void fat_dir_readahead(struct inode *dir, sector_t iblock,
> >       if ((iblock & (sbi->sec_per_clus - 1)) || sbi->sec_per_clus == 1)
> >               return;
> >       /* root dir of FAT12/FAT16 */
> > -     if ((sbi->fat_bits != 32) && (dir->i_ino == MSDOS_ROOT_INO))
> > +     if (!IS_FAT32(sbi) && (dir->i_ino == MSDOS_ROOT_INO))
> >               return;
> >
> >       bh = sb_find_get_block(sb, phys);
> > @@ -1313,7 +1313,7 @@ int fat_add_entries(struct inode *dir, void *slots, int nr_slots,
> >               }
> >       }
> >       if (dir->i_ino == MSDOS_ROOT_INO) {
> > -             if (sbi->fat_bits != 32)
> > +             if (!IS_FAT32(sbi))
> >                       goto error;
> >       } else if (MSDOS_I(dir)->i_start == 0) {
> >               fat_msg(sb, KERN_ERR, "Corrupted directory (i_pos %lld)",
> > diff --git a/fs/fat/fat.h b/fs/fat/fat.h
> > index 11bc4a2..5b6f1c8 100644
> > --- a/fs/fat/fat.h
> > +++ b/fs/fat/fat.h
> > @@ -142,13 +142,32 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
> >       return sb->s_fs_info;
> >  }
> >
> > +/*
> > + * Functions that determine the variant of the FAT file system (i.e.,
> > + * whether this is FAT12, FAT16 or FAT32.
> > + */
> > +static inline bool IS_FAT12(const struct msdos_sb_info *sbi)
> > +{
> > +     return sbi->fat_bits == 12;
> > +}
> > +
> > +static inline bool IS_FAT16(const struct msdos_sb_info *sbi)
> > +{
> > +     return sbi->fat_bits == 16;
> > +}
> > +
> > +static inline bool IS_FAT32(const struct msdos_sb_info *sbi)
> > +{
> > +     return sbi->fat_bits == 32;
> > +}
> > +
> >  /* Maximum number of clusters */
> >  static inline u32 MAX_FAT(struct super_block *sb)
> >  {
> >       struct msdos_sb_info *sbi = MSDOS_SB(sb);
> >
> > -     return sbi->fat_bits == 32 ? MAX_FAT32 :
> > -             sbi->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12;
> > +     return IS_FAT32(sbi) ? MAX_FAT32 :
> > +             IS_FAT16(sbi) ? MAX_FAT16 : MAX_FAT12;
> >  }
> >
> >  static inline struct msdos_inode_info *MSDOS_I(struct inode *inode)
> > @@ -266,7 +285,7 @@ static inline int fat_get_start(const struct msdos_sb_info *sbi,
> >                               const struct msdos_dir_entry *de)
> >  {
> >       int cluster = le16_to_cpu(de->start);
> > -     if (sbi->fat_bits == 32)
> > +     if (IS_FAT32(sbi))
> >               cluster |= (le16_to_cpu(de->starthi) << 16);
> >       return cluster;
> >  }
> > diff --git a/fs/fat/fatent.c b/fs/fat/fatent.c
> > index f58c0ca..9166d96 100644
> > --- a/fs/fat/fatent.c
> > +++ b/fs/fat/fatent.c
> > @@ -290,19 +290,17 @@ void fat_ent_access_init(struct super_block *sb)
> >
> >       mutex_init(&sbi->fat_lock);
> >
> > -     switch (sbi->fat_bits) {
> > -     case 32:
> > +     if (IS_FAT32(sbi)) {
> >               sbi->fatent_shift = 2;
> >               sbi->fatent_ops = &fat32_ops;
> > -             break;
> > -     case 16:
> > +     } else if (IS_FAT16(sbi)) {
> >               sbi->fatent_shift = 1;
> >               sbi->fatent_ops = &fat16_ops;
> > -             break;
> > -     case 12:
> > +     } else if (IS_FAT12(sbi)) {
> >               sbi->fatent_shift = -1;
> >               sbi->fatent_ops = &fat12_ops;
> > -             break;
> > +     } else {
> > +             fat_fs_error(sb, "invalid FAT variant, %u bits", sbi->fat_bits);
> >       }
> >  }
> >
> > @@ -310,7 +308,7 @@ static void mark_fsinfo_dirty(struct super_block *sb)
> >  {
> >       struct msdos_sb_info *sbi = MSDOS_SB(sb);
> >
> > -     if (sb_rdonly(sb) || sbi->fat_bits != 32)
> > +     if (sb_rdonly(sb) || !IS_FAT32(sbi))
> >               return;
> >
> >       __mark_inode_dirty(sbi->fsinfo_inode, I_DIRTY_SYNC);
> > @@ -327,7 +325,7 @@ static inline int fat_ent_update_ptr(struct super_block *sb,
> >       /* Is this fatent's blocks including this entry? */
> >       if (!fatent->nr_bhs || bhs[0]->b_blocknr != blocknr)
> >               return 0;
> > -     if (sbi->fat_bits == 12) {
> > +     if (IS_FAT12(sbi)) {
> >               if ((offset + 1) < sb->s_blocksize) {
> >                       /* This entry is on bhs[0]. */
> >                       if (fatent->nr_bhs == 2) {
> > diff --git a/fs/fat/inode.c b/fs/fat/inode.c
> > index 708de6d..a84b61b 100644
> > --- a/fs/fat/inode.c
> > +++ b/fs/fat/inode.c
> > @@ -686,7 +686,7 @@ static void fat_set_state(struct super_block *sb,
> >
> >       b = (struct fat_boot_sector *) bh->b_data;
> >
> > -     if (sbi->fat_bits == 32) {
> > +     if (IS_FAT32(sbi)) {
> >               if (set)
> >                       b->fat32.state |= FAT_STATE_DIRTY;
> >               else
> > @@ -1397,7 +1397,7 @@ static int fat_read_root(struct inode *inode)
> >       inode->i_mode = fat_make_mode(sbi, ATTR_DIR, S_IRWXUGO);
> >       inode->i_op = sbi->dir_ops;
> >       inode->i_fop = &fat_dir_operations;
> > -     if (sbi->fat_bits == 32) {
> > +     if (IS_FAT32(sbi)) {
> >               MSDOS_I(inode)->i_start = sbi->root_cluster;
> >               error = fat_calc_dir_size(inode);
> >               if (error < 0)
> > @@ -1424,7 +1424,7 @@ static unsigned long calc_fat_clusters(struct super_block *sb)
> >       struct msdos_sb_info *sbi = MSDOS_SB(sb);
> >
> >       /* Divide first to avoid overflow */
> > -     if (sbi->fat_bits != 12) {
> > +     if (!IS_FAT12(sbi)) {
> >               unsigned long ent_per_sec = sb->s_blocksize * 8 / sbi->fat_bits;
> >               return ent_per_sec * sbi->fat_length;
> >       }
> > @@ -1744,7 +1744,7 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
> >       }
> >
> >       /* interpret volume ID as a little endian 32 bit integer */
> > -     if (sbi->fat_bits == 32)
> > +     if (IS_FAT32(sbi))
> >               sbi->vol_id = bpb.fat32_vol_id;
> >       else /* fat 16 or 12 */
> >               sbi->vol_id = bpb.fat16_vol_id;
> > @@ -1770,11 +1770,11 @@ int fat_fill_super(struct super_block *sb, void *data, int silent, int isvfat,
> >
> >       total_clusters = (total_sectors - sbi->data_start) / sbi->sec_per_clus;
> >
> > -     if (sbi->fat_bits != 32)
> > +     if (!IS_FAT32(sbi))
> >               sbi->fat_bits = (total_clusters > MAX_FAT12) ? 16 : 12;
> >
> >       /* some OSes set FAT_STATE_DIRTY and clean it on unmount. */
> > -     if (sbi->fat_bits == 32)
> > +     if (IS_FAT32(sbi))
> >               sbi->dirty = bpb.fat32_state & FAT_STATE_DIRTY;
> >       else /* fat 16 or 12 */
> >               sbi->dirty = bpb.fat16_state & FAT_STATE_DIRTY;
> > diff --git a/fs/fat/misc.c b/fs/fat/misc.c
> > index fce0a76..5368c6a 100644
> > --- a/fs/fat/misc.c
> > +++ b/fs/fat/misc.c
> > @@ -64,7 +64,7 @@ int fat_clusters_flush(struct super_block *sb)
> >       struct buffer_head *bh;
> >       struct fat_boot_fsinfo *fsinfo;
> >
> > -     if (sbi->fat_bits != 32)
> > +     if (!IS_FAT32(sbi))
> >               return 0;
> >
> >       bh = sb_bread(sb, sbi->fsinfo_sector);
>
> --
> OGAWA Hirofumi <hirofumi@mail.parknet.co.jp>