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;
Date:   Sun, 29 Dec 2019 15:11:08 +0100
From:   Pali =?utf-8?B?Um9ow6Fy?= <pali.rohar@gmail.com>
To:     Namjae Jeon <namjae.jeon@samsung.com>
Cc:     linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
        gregkh@linuxfoundation.org, valdis.kletnieks@vt.edu, hch@lst.de,
        sj1557.seo@samsung.com, linkinjeon@gmail.com
Subject: Re: [PATCH v8 01/13] exfat: add in-memory and on-disk structures and
 headers
Message-ID: <20191229141108.ufnu6lbu7qvl5oxj@pali>
References: <20191220062419.23516-1-namjae.jeon@samsung.com>
 <CGME20191220062732epcas1p17f3b1066fb4d6496559f349f950e1751@epcas1p1.samsung.com>
 <20191220062419.23516-2-namjae.jeon@samsung.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha1;
        protocol="application/pgp-signature"; boundary="vnynbe7hoynpmjey"
Content-Disposition: inline
In-Reply-To: <20191220062419.23516-2-namjae.jeon@samsung.com>
User-Agent: NeoMutt/20170113 (1.7.2)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--vnynbe7hoynpmjey
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Friday 20 December 2019 01:24:07 Namjae Jeon wrote:
> This adds in-memory and on-disk structures and headers.
>=20
> Signed-off-by: Namjae Jeon <namjae.jeon@samsung.com>
> Signed-off-by: Sungjong Seo <sj1557.seo@samsung.com>
> ---
>  fs/exfat/exfat_fs.h  | 559 +++++++++++++++++++++++++++++++++++++++++++
>  fs/exfat/exfat_raw.h | 202 ++++++++++++++++
>  2 files changed, 761 insertions(+)
>  create mode 100644 fs/exfat/exfat_fs.h
>  create mode 100644 fs/exfat/exfat_raw.h

=2E..

> diff --git a/fs/exfat/exfat_raw.h b/fs/exfat/exfat_raw.h
> new file mode 100644
> index 000000000000..a3ccac835993
> --- /dev/null
> +++ b/fs/exfat/exfat_raw.h

=2E..

> +/* file attributes */
> +#define ATTR_READONLY		0x0001
> +#define ATTR_HIDDEN		0x0002
> +#define ATTR_SYSTEM		0x0004
> +#define ATTR_VOLUME		0x0008
> +#define ATTR_SUBDIR		0x0010
> +#define ATTR_ARCHIVE		0x0020
> +#define ATTR_EXTEND		(ATTR_READONLY | ATTR_HIDDEN | ATTR_SYSTEM | \
> +				 ATTR_VOLUME) /* 0x000F */
> +
> +#define ATTR_EXTEND_MASK	(ATTR_EXTEND | ATTR_SUBDIR | ATTR_ARCHIVE)
> +#define ATTR_RWMASK		(ATTR_HIDDEN | ATTR_SYSTEM | ATTR_VOLUME | \
> +				 ATTR_SUBDIR | ATTR_ARCHIVE)
> +
> +#define ATTR_READONLY_LE	cpu_to_le16(0x0001)
> +#define ATTR_HIDDEN_LE		cpu_to_le16(0x0002)
> +#define ATTR_SYSTEM_LE		cpu_to_le16(0x0004)
> +#define ATTR_VOLUME_LE		cpu_to_le16(0x0008)
> +#define ATTR_SUBDIR_LE		cpu_to_le16(0x0010)
> +#define ATTR_ARCHIVE_LE		cpu_to_le16(0x0020)

Hello!

This looks like copy-paste code from /* file attributes */ section
above. What about at least making these macro definitions as?

  #define ATTR_READONLY_LE	cpu_to_le16(ATTR_READONLY)
  #define ATTR_HIDDEN_LE	cpu_to_le16(ATTR_HIDDEN)
  ...

But main question is, are these _LE definitions needed at all?

Looking at the whole patch series and only ATTR_SUBDIR_LE and
ATTR_ARCHIVE_LE are used.

Is not it better to use cpu_to_le16(ATTR_READONLY) directly in code and
do not define duplicate ATTR_READONLY_LE macro at all?

> +
> +#define JUMP_BOOT_LEN			3
> +#define OEM_NAME_LEN			8
> +#define MUST_BE_ZERO_LEN		53
> +#define EXFAT_FILE_NAME_LEN		15
> +
> +/* EXFAT BIOS parameter block (64 bytes) */
> +struct bpb64 {
> +	__u8 jmp_boot[JUMP_BOOT_LEN];
> +	__u8 oem_name[OEM_NAME_LEN];
> +	__u8 res_zero[MUST_BE_ZERO_LEN];
> +};
> +
> +/* EXFAT EXTEND BIOS parameter block (56 bytes) */
> +struct bsx64 {
> +	__le64 vol_offset;
> +	__le64 vol_length;
> +	__le32 fat_offset;
> +	__le32 fat_length;
> +	__le32 clu_offset;
> +	__le32 clu_count;
> +	__le32 root_cluster;
> +	__le32 vol_serial;
> +	__u8 fs_version[2];
> +	__le16 vol_flags;
> +	__u8 sect_size_bits;
> +	__u8 sect_per_clus_bits;
> +	__u8 num_fats;
> +	__u8 phy_drv_no;
> +	__u8 perc_in_use;
> +	__u8 reserved2[7];
> +};

Should not be this structure marked as packed? Also those two below.

> +/* EXFAT PBR[BPB+BSX] (120 bytes) */
> +struct pbr64 {
> +	struct bpb64 bpb;
> +	struct bsx64 bsx;
> +};
> +
> +/* Common PBR[Partition Boot Record] (512 bytes) */
> +struct pbr {
> +	union {
> +		__u8 raw[64];
> +		struct bpb64 f64;
> +	} bpb;
> +	union {
> +		__u8 raw[56];
> +		struct bsx64 f64;
> +	} bsx;
> +	__u8 boot_code[390];
> +	__le16 signature;
> +};

--=20
Pali Roh=C3=A1r
pali.rohar@gmail.com

--vnynbe7hoynpmjey
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iF0EABECAB0WIQS4VrIQdKium2krgIWL8Mk9A+RDUgUCXgiz+QAKCRCL8Mk9A+RD
UmAYAJ9H2kwrv3K00UAzeVDfVYXvruxJkACfYgrgSvFXfP5UPzQ3HJPqmoWvgdY=
=EIYf
-----END PGP SIGNATURE-----

--vnynbe7hoynpmjey--