Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Date:   Mon, 11 Dec 2023 16:57:57 -0800
From:   Davidlohr Bueso <dave@stgolabs.net>
To:     Romain Gantois <romain.gantois@bootlin.com>
Cc:     Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Jens Axboe <axboe@kernel.dk>, Ard Biesheuvel <ardb@kernel.org>,
        Thomas Petazzoni <thomas.petazzoni@bootlin.com>,
        Herve Codina <herve.codina@bootlin.com>,
        linux-mtd@lists.infradead.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        linux-efi@vger.kernel.org
Subject: Re: [RFC PATCH 2/6] block: partitions: efi: Fix some style issues
Message-ID: <6lhqirdaqmaqk6bpyywmnsj5oon2nvcq2wn6napir5df4md6q3@mu6w5ql45tf7>
References: <20231211151244.289349-1-romain.gantois@bootlin.com>
 <20231211151244.289349-3-romain.gantois@bootlin.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Disposition: inline
In-Reply-To: <20231211151244.289349-3-romain.gantois@bootlin.com>
User-Agent: NeoMutt/20231103
Precedence: bulk

On Mon, 11 Dec 2023, Romain Gantois wrote:

>The block layer EFI code is quite old and does not perfectly match the
>current kernel coding style. Fix some indentation and trailing whitespace
>issues in efi.c.

I agree that the styling can use some love. However, a few comments below
where such changes are really unnecessary.

...

>@@ -213,7 +212,7 @@ static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
>	 */
>	if (ret == GPT_MBR_PROTECTIVE) {
>		sz = le32_to_cpu(mbr->partition_record[part].size_in_lba);
>-		if (sz != (uint32_t) total_sectors - 1 && sz != 0xFFFFFFFF)
>+		if (sz != (uint32_t)total_sectors - 1 && sz != 0xFFFFFFFF)

Like here.

>			pr_debug("GPT: mbr size in lba (%u) different than whole disk (%u).\n",
>				 sz, min_t(uint32_t,
>					   total_sectors - 1, 0xFFFFFFFF));
>@@ -235,17 +234,19 @@ static int is_pmbr_valid(legacy_mbr *mbr, sector_t total_sectors)
> static size_t read_lba(struct parsed_partitions *state,
>		       u64 lba, u8 *buffer, size_t count)
> {
>-	size_t totalreadcount = 0;
>	sector_t n = lba *
>		(queue_logical_block_size(state->disk->queue) / 512);
>+	size_t totalreadcount = 0;
>+	unsigned char *data;
>+	Sector sect;
>+	int copied;
>
>	if (!buffer || lba > last_lba(state->disk))
>-                return 0;
>+		return 0;
>
>	while (count) {
>-		int copied = 512;
>-		Sector sect;
>-		unsigned char *data = read_part_sector(state, n++, &sect);
>+		copied = 512;
>+		data = read_part_sector(state, n++, &sect);

ditto

>		if (!data)
>			break;
>		if (copied > count)
>@@ -253,7 +254,7 @@ static size_t read_lba(struct parsed_partitions *state,
>		memcpy(buffer, data, copied);
>		put_dev_sector(sect);
>		buffer += copied;
>-		totalreadcount +=copied;
>+		totalreadcount += copied;
>		count -= copied;
>	}
>	return totalreadcount;
>@@ -263,7 +264,7 @@ static size_t read_lba(struct parsed_partitions *state,
>  * alloc_read_gpt_entries(): reads partition entries from disk
>  * @state: disk parsed partitions
>  * @gpt: GPT header
>- *
>+ *
>  * Description: Returns ptes on success,  NULL on error.
>  * Allocates space for PTEs based on information found in @gpt.
>  * Notes: remember to free pte when you're done!
>@@ -271,14 +272,14 @@ static size_t read_lba(struct parsed_partitions *state,
> static gpt_entry *alloc_read_gpt_entries(struct parsed_partitions *state,
>					 gpt_header *gpt)
> {
>-	size_t count;
>	gpt_entry *pte;
>+	size_t count;

ditto

>
>	if (!gpt)
>		return NULL;
>
>	count = (size_t)le32_to_cpu(gpt->num_partition_entries) *
>-                le32_to_cpu(gpt->sizeof_partition_entry);
>+		le32_to_cpu(gpt->sizeof_partition_entry);
>	if (!count)
>		return NULL;
>	pte = kmalloc(count, GFP_KERNEL);
>@@ -286,9 +287,9 @@ static gpt_entry *alloc_read_gpt_entries(struct parsed_partitions *state,
>		return NULL;
>
>	if (read_lba(state, le64_to_cpu(gpt->partition_entry_lba),
>-			(u8 *) pte, count) < count) {
>+		     (u8 *)pte, count) < count) {
>		kfree(pte);
>-                pte=NULL;
>+		pte = NULL;
>		return NULL;
>	}
>	return pte;
>@@ -298,7 +299,7 @@ static gpt_entry *alloc_read_gpt_entries(struct parsed_partitions *state,
>  * alloc_read_gpt_header(): Allocates GPT header, reads into it from disk
>  * @state: disk parsed partitions
>  * @lba: the Logical Block Address of the partition table
>- *
>+ *
>  * Description: returns GPT header on success, NULL on error.   Allocates
>  * and fills a GPT header starting at @ from @state->disk.
>  * Note: remember to free gpt when finished with it.
>@@ -306,16 +307,16 @@ static gpt_entry *alloc_read_gpt_entries(struct parsed_partitions *state,
> static gpt_header *alloc_read_gpt_header(struct parsed_partitions *state,
>					 u64 lba)
> {
>+	unsigned int ssz = queue_logical_block_size(state->disk->queue);
>	gpt_header *gpt;
>-	unsigned ssz = queue_logical_block_size(state->disk->queue);

ditto

>
>	gpt = kmalloc(ssz, GFP_KERNEL);
>	if (!gpt)
>		return NULL;
>
>-	if (read_lba(state, lba, (u8 *) gpt, ssz) < ssz) {
>+	if (read_lba(state, lba, (u8 *)gpt, ssz) < ssz) {

ditto

>		kfree(gpt);
>-                gpt=NULL;
>+		gpt = NULL;
>		return NULL;
>	}
>
>@@ -486,31 +487,31 @@ compare_gpts(gpt_header *pgpt, gpt_header *agpt, u64 lastlba)
>	if (le64_to_cpu(pgpt->my_lba) != le64_to_cpu(agpt->alternate_lba)) {
>		pr_warn("GPT:Primary header LBA != Alt. header alternate_lba\n");
>		pr_warn("GPT:%lld != %lld\n",
>-		       (unsigned long long)le64_to_cpu(pgpt->my_lba),
>-                       (unsigned long long)le64_to_cpu(agpt->alternate_lba));
>+			(unsigned long long)le64_to_cpu(pgpt->my_lba),
>+			(unsigned long long)le64_to_cpu(agpt->alternate_lba));
>		error_found++;
>	}
>	if (le64_to_cpu(pgpt->alternate_lba) != le64_to_cpu(agpt->my_lba)) {
>		pr_warn("GPT:Primary header alternate_lba != Alt. header my_lba\n");
>		pr_warn("GPT:%lld != %lld\n",
>-		       (unsigned long long)le64_to_cpu(pgpt->alternate_lba),
>-                       (unsigned long long)le64_to_cpu(agpt->my_lba));
>+			(unsigned long long)le64_to_cpu(pgpt->alternate_lba),
>+			(unsigned long long)le64_to_cpu(agpt->my_lba));
>		error_found++;
>	}
>	if (le64_to_cpu(pgpt->first_usable_lba) !=
>-            le64_to_cpu(agpt->first_usable_lba)) {
>+	    le64_to_cpu(agpt->first_usable_lba)) {
>		pr_warn("GPT:first_usable_lbas don't match.\n");
>		pr_warn("GPT:%lld != %lld\n",
>-		       (unsigned long long)le64_to_cpu(pgpt->first_usable_lba),
>-                       (unsigned long long)le64_to_cpu(agpt->first_usable_lba));
>+			(unsigned long long)le64_to_cpu(pgpt->first_usable_lba),
>+			(unsigned long long)le64_to_cpu(agpt->first_usable_lba));
>		error_found++;
>	}
>	if (le64_to_cpu(pgpt->last_usable_lba) !=
>-            le64_to_cpu(agpt->last_usable_lba)) {
>+	    le64_to_cpu(agpt->last_usable_lba)) {
>		pr_warn("GPT:last_usable_lbas don't match.\n");
>		pr_warn("GPT:%lld != %lld\n",
>-		       (unsigned long long)le64_to_cpu(pgpt->last_usable_lba),
>-                       (unsigned long long)le64_to_cpu(agpt->last_usable_lba));
>+			(unsigned long long)le64_to_cpu(pgpt->last_usable_lba),
>+			(unsigned long long)le64_to_cpu(agpt->last_usable_lba));
>		error_found++;
>	}
>	if (efi_guidcmp(pgpt->disk_guid, agpt->disk_guid)) {
>@@ -518,27 +519,24 @@ compare_gpts(gpt_header *pgpt, gpt_header *agpt, u64 lastlba)
>		error_found++;
>	}
>	if (le32_to_cpu(pgpt->num_partition_entries) !=
>-            le32_to_cpu(agpt->num_partition_entries)) {
>-		pr_warn("GPT:num_partition_entries don't match: "
>-		       "0x%x != 0x%x\n",
>-		       le32_to_cpu(pgpt->num_partition_entries),
>-		       le32_to_cpu(agpt->num_partition_entries));
>+	    le32_to_cpu(agpt->num_partition_entries)) {
>+		pr_warn("GPT:num_partition_entries don't match: 0x%x != 0x%x\n",
>+			le32_to_cpu(pgpt->num_partition_entries),
>+			le32_to_cpu(agpt->num_partition_entries));
>		error_found++;
>	}
>	if (le32_to_cpu(pgpt->sizeof_partition_entry) !=
>-            le32_to_cpu(agpt->sizeof_partition_entry)) {
>-		pr_warn("GPT:sizeof_partition_entry values don't match: "
>-		       "0x%x != 0x%x\n",
>-                       le32_to_cpu(pgpt->sizeof_partition_entry),
>-		       le32_to_cpu(agpt->sizeof_partition_entry));
>+	    le32_to_cpu(agpt->sizeof_partition_entry)) {
>+		pr_warn("GPT:sizeof_partition_entry values don't match: 0x%x != 0x%x\n",
>+			le32_to_cpu(pgpt->sizeof_partition_entry),
>+			le32_to_cpu(agpt->sizeof_partition_entry));
>		error_found++;
>	}
>	if (le32_to_cpu(pgpt->partition_entry_array_crc32) !=
>-            le32_to_cpu(agpt->partition_entry_array_crc32)) {
>-		pr_warn("GPT:partition_entry_array_crc32 values don't match: "
>-		       "0x%x != 0x%x\n",
>-                       le32_to_cpu(pgpt->partition_entry_array_crc32),
>-		       le32_to_cpu(agpt->partition_entry_array_crc32));
>+	    le32_to_cpu(agpt->partition_entry_array_crc32)) {
>+		pr_warn("GPT:partition_entry_array_crc32 values don't match: 0x%x != 0x%x\n",
>+			le32_to_cpu(pgpt->partition_entry_array_crc32),
>+			le32_to_cpu(agpt->partition_entry_array_crc32));
>		error_found++;
>	}
>	if (le64_to_cpu(pgpt->alternate_lba) != lastlba) {
>@@ -581,20 +579,22 @@ compare_gpts(gpt_header *pgpt, gpt_header *agpt, u64 lastlba)
> static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
>			  gpt_entry **ptes)
> {
>+	sector_t total_sectors = get_capacity(state->disk);
>	int good_pgpt = 0, good_agpt = 0, good_pmbr = 0;
>-	gpt_header *pgpt = NULL, *agpt = NULL;
>+	const struct block_device_operations *fops;
>	gpt_entry *pptes = NULL, *aptes = NULL;
>-	legacy_mbr *legacymbr;
>+	gpt_header *pgpt = NULL, *agpt = NULL;
>	struct gendisk *disk = state->disk;
>-	const struct block_device_operations *fops = disk->fops;
>-	sector_t total_sectors = get_capacity(state->disk);
>+	legacy_mbr *legacymbr;
>	u64 lastlba;
>
>+	fops = disk->fops;

ditto

>+
>	if (!ptes)
>		return 0;
>
>	lastlba = last_lba(state->disk);
>-        if (!force_gpt) {
>+	if (!force_gpt) {
>		/* This will be added to the EFI Spec. per Intel after v1.02. */
>		legacymbr = kzalloc(sizeof(*legacymbr), GFP_KERNEL);
>		if (!legacymbr)
>@@ -609,17 +609,17 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
>
>		pr_debug("Device has a %s MBR\n",
>			 good_pmbr == GPT_MBR_PROTECTIVE ?
>-						"protective" : "hybrid");
>+			 "protective" : "hybrid");
>	}
>
>	good_pgpt = is_gpt_valid(state, GPT_PRIMARY_PARTITION_TABLE_LBA,
>				 &pgpt, &pptes);
>-        if (good_pgpt)
>+	if (good_pgpt)
>		good_agpt = is_gpt_valid(state,
>					 le64_to_cpu(pgpt->alternate_lba),
>					 &agpt, &aptes);
>-        if (!good_agpt && force_gpt)
>-                good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes);
>+	if (!good_agpt && force_gpt)
>+		good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes);
>
>	if (!good_agpt && force_gpt && fops->alternative_gpt_sector) {
>		sector_t agpt_sector;
>@@ -631,39 +631,38 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt,
>						 &agpt, &aptes);
>	}
>
>-        /* The obviously unsuccessful case */
>-        if (!good_pgpt && !good_agpt)
>-                goto fail;
>+	/* The obviously unsuccessful case */
>+	if (!good_pgpt && !good_agpt)
>+		goto fail;
>
>         compare_gpts(pgpt, agpt, lastlba);
>
>-        /* The good cases */
>-        if (good_pgpt) {
>-                *gpt  = pgpt;
>-                *ptes = pptes;
>-                kfree(agpt);
>-                kfree(aptes);
>+	/* The good cases */
>+	if (good_pgpt) {
>+		*gpt  = pgpt;
>+		*ptes = pptes;
>+		kfree(agpt);
>+		kfree(aptes);
>		if (!good_agpt)
>-                        pr_warn("Alternate GPT is invalid, using primary GPT.\n");
>-                return 1;
>-        }
>-        else if (good_agpt) {
>-                *gpt  = agpt;
>-                *ptes = aptes;
>-                kfree(pgpt);
>-                kfree(pptes);
>+			pr_warn("Alternate GPT is invalid, using primary GPT.\n");
>+		return 1;
>+	} else if (good_agpt) {
>+		*gpt  = agpt;
>+		*ptes = aptes;
>+		kfree(pgpt);
>+		kfree(pptes);
>		pr_warn("Primary GPT is invalid, using alternate GPT.\n");
>-                return 1;
>-        }
>+		return 1;
>+	}
>
>- fail:
>-        kfree(pgpt);
>-        kfree(agpt);
>-        kfree(pptes);
>-        kfree(aptes);
>-        *gpt = NULL;
>-        *ptes = NULL;
>-        return 0;
>+fail:
>+	kfree(pgpt);
>+	kfree(agpt);
>+	kfree(pptes);
>+	kfree(aptes);
>+	*gpt = NULL;
>+	*ptes = NULL;
>+	return 0;
> }
>
> /**
>@@ -712,10 +711,10 @@ static void utf16_le_to_7bit(const __le16 *in, unsigned int size, u8 *out)
>  */
> int efi_partition(struct parsed_partitions *state)
> {
>+	unsigned int ssz = queue_logical_block_size(state->disk->queue) / 512;
>	gpt_header *gpt = NULL;
>	gpt_entry *ptes = NULL;
>	u32 i;
>-	unsigned ssz = queue_logical_block_size(state->disk->queue) / 512;

ditto

>
>	if (!find_valid_gpt(state, &gpt, &ptes) || !gpt || !ptes) {
>		kfree(gpt);
>@@ -725,17 +724,17 @@ int efi_partition(struct parsed_partitions *state)
>
>	pr_debug("GUID Partition Table is valid!  Yea!\n");
>
>-	for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) && i < state->limit-1; i++) {
>+	for (i = 0; i < le32_to_cpu(gpt->num_partition_entries) && i < state->limit - 1; i++) {

ditto

>		struct partition_meta_info *info;
>-		unsigned label_max;
>+		unsigned int label_max;
>		u64 start = le64_to_cpu(ptes[i].starting_lba);
>		u64 size = le64_to_cpu(ptes[i].ending_lba) -
>-			   le64_to_cpu(ptes[i].starting_lba) + 1ULL;
>+			le64_to_cpu(ptes[i].starting_lba) + 1ULL;
>
>		if (!is_pte_valid(&ptes[i], last_lba(state->disk)))
>			continue;
>
>-		put_partition(state, i+1, start * ssz, size * ssz);
>+		put_partition(state, i + 1, start * ssz, size * ssz);
>
>		/* If this is a RAID volume, tell md */
>		if (!efi_guidcmp(ptes[i].partition_type_guid, PARTITION_LINUX_RAID_GUID))
>diff --git a/include/linux/gpt.h b/include/linux/gpt.h
>index 84b9f36b9e47..633be6bc826c 100644
>--- a/include/linux/gpt.h
>+++ b/include/linux/gpt.h
>@@ -4,7 +4,7 @@
>  * Per Intel EFI Specification v1.02
>  * http://developer.intel.com/technology/efi/efi.htm
>  *
>- * By Matt Domsch <Matt_Domsch@dell.com>  Fri Sep 22 22:15:56 CDT 2000
>+ * By Matt Domsch <Matt_Domsch@dell.com>  Fri Sep 22 22:15:56 CDT 2000
>  *   Copyright 2000,2001 Dell Inc.
>  ************************************************************/
>
>@@ -31,26 +31,26 @@
> #define GPT_PRIMARY_PARTITION_TABLE_LBA 1
>
> #define PARTITION_SYSTEM_GUID \
>-    EFI_GUID( 0xC12A7328, 0xF81F, 0x11d2, \
>-              0xBA, 0x4B, 0x00, 0xA0, 0xC9, 0x3E, 0xC9, 0x3B)
>+	EFI_GUID(0xC12A7328, 0xF81F, 0x11d2, \
>+		 0xBA, 0x4B, 0x00, 0xA0, 0xC9, 0x3E, 0xC9, 0x3B)
> #define LEGACY_MBR_PARTITION_GUID \
>-    EFI_GUID( 0x024DEE41, 0x33E7, 0x11d3, \
>-              0x9D, 0x69, 0x00, 0x08, 0xC7, 0x81, 0xF3, 0x9F)
>+	EFI_GUID(0x024DEE41, 0x33E7, 0x11d3, \
>+		 0x9D, 0x69, 0x00, 0x08, 0xC7, 0x81, 0xF3, 0x9F)
> #define PARTITION_MSFT_RESERVED_GUID \
>-    EFI_GUID( 0xE3C9E316, 0x0B5C, 0x4DB8, \
>-              0x81, 0x7D, 0xF9, 0x2D, 0xF0, 0x02, 0x15, 0xAE)
>+	EFI_GUID(0xE3C9E316, 0x0B5C, 0x4DB8, \
>+		 0x81, 0x7D, 0xF9, 0x2D, 0xF0, 0x02, 0x15, 0xAE)
> #define PARTITION_BASIC_DATA_GUID \
>-    EFI_GUID( 0xEBD0A0A2, 0xB9E5, 0x4433, \
>-              0x87, 0xC0, 0x68, 0xB6, 0xB7, 0x26, 0x99, 0xC7)
>+	EFI_GUID(0xEBD0A0A2, 0xB9E5, 0x4433, \
>+		 0x87, 0xC0, 0x68, 0xB6, 0xB7, 0x26, 0x99, 0xC7)
> #define PARTITION_LINUX_RAID_GUID \
>-    EFI_GUID( 0xa19d880f, 0x05fc, 0x4d3b, \
>-              0xa0, 0x06, 0x74, 0x3f, 0x0f, 0x84, 0x91, 0x1e)
>+	EFI_GUID(0xa19d880f, 0x05fc, 0x4d3b, \
>+		 0xa0, 0x06, 0x74, 0x3f, 0x0f, 0x84, 0x91, 0x1e)
> #define PARTITION_LINUX_SWAP_GUID \
>-    EFI_GUID( 0x0657fd6d, 0xa4ab, 0x43c4, \
>-              0x84, 0xe5, 0x09, 0x33, 0xc8, 0x4b, 0x4f, 0x4f)
>+	EFI_GUID(0x0657fd6d, 0xa4ab, 0x43c4, \
>+		 0x84, 0xe5, 0x09, 0x33, 0xc8, 0x4b, 0x4f, 0x4f)
> #define PARTITION_LINUX_LVM_GUID \
>-    EFI_GUID( 0xe6d6d379, 0xf507, 0x44c2, \
>-              0xa2, 0x3c, 0x23, 0x8f, 0x2a, 0x3d, 0xf9, 0x28)
>+	EFI_GUID(0xe6d6d379, 0xf507, 0x44c2, \
>+		 0xa2, 0x3c, 0x23, 0x8f, 0x2a, 0x3d, 0xf9, 0x28)
>
> typedef struct _gpt_header {
>	__le64 signature;
>@@ -78,7 +78,7 @@ typedef struct _gpt_header {
> typedef struct _gpt_entry_attributes {
>	u64 required_to_function:1;
>	u64 reserved:47;
>-        u64 type_guid_specific:16;
>+	u64 type_guid_specific:16;
> } __packed gpt_entry_attributes;
>
> typedef struct _gpt_entry {
>@@ -87,7 +87,7 @@ typedef struct _gpt_entry {
>	__le64 starting_lba;
>	__le64 ending_lba;
>	gpt_entry_attributes attributes;
>-	__le16 partition_name[72/sizeof(__le16)];
>+	__le16 partition_name[72 / sizeof(__le16)];
> } __packed gpt_entry;
>
> typedef struct _gpt_mbr_record {
>@@ -103,7 +103,6 @@ typedef struct _gpt_mbr_record {
>	__le32	size_in_lba;    /* used by EFI - size of pt in LBA */
> } __packed gpt_mbr_record;
>
>-
> typedef struct _legacy_mbr {
>	u8 boot_code[440];
>	__le32 unique_mbr_signature;
>--
>2.43.0
>