Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <906167f7ca975d437ff9868333d8bf1e86b934a7.camel@perches.com>
Subject: Re: [PATCH 2/2] fat: New macros to determine the FAT variant (32,
 16 or 12)
From:   Joe Perches <joe@perches.com>
To:     Carmeli Tamir <carmeli.tamir@gmail.com>,
        hirofumi@mail.parknet.co.jp, jthumshirn@suse.de,
        sergey.senozhatsky@gmail.com, akpm@linux-foundation.org,
        axboe@kernel.dk, martin.petersen@oracle.com, bvanassche@acm.org,
        linux-kernel@vger.kernel.org
Date:   Mon, 10 Dec 2018 12:09:31 -0800
In-Reply-To: <1544470917-6443-3-git-send-email-carmeli.tamir@gmail.com>
References: <1544470917-6443-1-git-send-email-carmeli.tamir@gmail.com>
         <1544470917-6443-3-git-send-email-carmeli.tamir@gmail.com>
Content-Type: text/plain; charset="ISO-8859-1"
User-Agent: Evolution 3.30.1-1build1 
Mime-Version: 1.0
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 2018-12-10 at 14:41 -0500, Carmeli Tamir wrote:
> This patch introduces 3 new macros - 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.

Overall a nice cleanup and a couple style suggestions:

> diff --git a/fs/fat/cache.c b/fs/fat/cache.c
> index 78d501c..99962b3 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)) {

Perhaps nicer without the parens around !IS_FAT32(sbi)

[]

> diff --git a/fs/fat/fat.h b/fs/fat/fat.h
[]
> @@ -116,12 +116,16 @@ static inline struct msdos_sb_info *MSDOS_SB(struct super_block *sb)
>   * this is FAT12, FAT16 or FAT32.
>   */
>  
> -#define FAT_FIRST_ENT(s, x)     ((MSDOS_SB(s)->fat_bits == 32 ? 0x0FFFFF00 : \
> -	MSDOS_SB(s)->fat_bits == 16 ? 0xFF00 : 0xF00) | (x))
> +#define IS_FAT12(sbi) (sbi->fat_bits == 12)
> +#define IS_FAT16(sbi) (sbi->fat_bits == 16)
> +#define IS_FAT32(sbi) (sbi->fat_bits == 32)

sbi should be parenthesized or perhaps better these should be
static inline bool functions

> +
> +#define FAT_FIRST_ENT(s, x)     ((IS_FAT32(MSDOS_SB(s)) ? 0x0FFFFF00 : \
> +	IS_FAT16(MSDOS_SB(s)) ? 0xFF00 : 0xF00) | (x))

This is probably nicer to use a statement expression macro
so MSDOS_SB(s) is only evaluated once or convert this to a
static inline, but it may even better to remove it altogether
instead as it seems unused anywhere.

>  
>  /* maximum number of clusters */
> -#define MAX_FAT(s)      (MSDOS_SB(s)->fat_bits == 32 ? MAX_FAT32 : \
> -	MSDOS_SB(s)->fat_bits == 16 ? MAX_FAT16 : MAX_FAT12)
> +#define MAX_FAT(s)      (IS_FAT32(MSDOS_SB(s)) ? MAX_FAT32 : \
> +	IS_FAT16(MSDOS_SB(s)) ? MAX_FAT16 : MAX_FAT12)

Used once, so perhaps better inlined there.