diff mbox

[U-Boot,2/6] gpt: Replace the leXX_to_int() calls with ones defined at <compiler.h>

Message ID 1345795995-24656-3-git-send-email-l.majewski@samsung.com
State Superseded
Delegated to: Tom Rini
Headers show

Commit Message

Łukasz Majewski Aug. 24, 2012, 8:13 a.m. UTC 
Custom definitions of le_XX_to_int functions have been replaced with
standard ones, defined at <compiler.h>

Signed-off-by: Chang Hyun Park <chchch.park@samsung.com>
Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 disk/part_efi.c |  109 ++++++++++++++++++++-----------------------------------
 1 files changed, 40 insertions(+), 69 deletions(-)

Comments

Stephen Warren Sept. 5, 2012, 7:35 p.m. UTC | #1 
On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> Custom definitions of le_XX_to_int functions have been replaced with
> standard ones, defined at <compiler.h>
> 
> Signed-off-by: Chang Hyun Park <chchch.park@samsung.com>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Did Chang Hyun Park write this? If so, shouldn't git have generated a
"From" header for him, to reflect his authorship upstream. I'm not sure
how Kyungmin Park's S-o-b plays into this, since he's not sending the patch.
Stephen Warren Sept. 5, 2012, 7:45 p.m. UTC | #2 
On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> Custom definitions of le_XX_to_int functions have been replaced with
> standard ones, defined at <compiler.h>

> diff --git a/disk/part_efi.c b/disk/part_efi.c

> -/* Convert char[8] in little endian format to the host format integer
> - */
> -static inline unsigned long long le64_to_int(unsigned char *le64)
> -{

So this original function takes a pointer to the value ...

>  	/* Check the first_usable_lba and last_usable_lba are within the disk. */
>  	lastlba = (unsigned long long)dev_desc->lba;
> -	if (le64_to_int(pgpt_head->first_usable_lba) > lastlba) {
> +	if (le64_to_cpu(pgpt_head->first_usable_lba) > lastlba) {

At this point in the series, first_usable_lba is a char[8], so this is
passing the address of the first byte to both the original function
le64_to_int(), and the replacement function le64_to_cpu(). However,
le64_to_cpu() expects to receive the 64-bit value to swap, not a pointer
to it - from compiler.h:

#define _uswap_64(x, sfx) \
        ((((x) & 0xff00000000000000##sfx) >> 56) | \
...
# define uswap_64(x) _uswap_64(x, ull)

le:
# define le64_to_cpu(x)         (x)
be:
# define le64_to_cpu(x)         uswap_64(x)

So I think this patch breaks the code, and then the next patch fixes it,
since it changes the type of first_usable_lba from char[8] to __le64?
Łukasz Majewski Sept. 6, 2012, 10:15 a.m. UTC | #3 
Hi Stephen,

> On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> > Custom definitions of le_XX_to_int functions have been replaced with
> > standard ones, defined at <compiler.h>
> 
> > diff --git a/disk/part_efi.c b/disk/part_efi.c
> 
> > -/* Convert char[8] in little endian format to the host format
> > integer
> > - */
> > -static inline unsigned long long le64_to_int(unsigned char *le64)
> > -{
> 
> So this original function takes a pointer to the value ...
> 
> >  	/* Check the first_usable_lba and last_usable_lba are
> > within the disk. */ lastlba = (unsigned long long)dev_desc->lba;
> > -	if (le64_to_int(pgpt_head->first_usable_lba) > lastlba) {
> > +	if (le64_to_cpu(pgpt_head->first_usable_lba) > lastlba) {
> 
> At this point in the series, first_usable_lba is a char[8], so this is
> passing the address of the first byte to both the original function
> le64_to_int(), and the replacement function le64_to_cpu(). However,
> le64_to_cpu() expects to receive the 64-bit value to swap, not a
> pointer to it - from compiler.h:
> 
> #define _uswap_64(x, sfx) \
>         ((((x) & 0xff00000000000000##sfx) >> 56) | \
> ...
> # define uswap_64(x) _uswap_64(x, ull)
> 
> le:
> # define le64_to_cpu(x)         (x)
> be:
> # define le64_to_cpu(x)         uswap_64(x)
> 
> So I think this patch breaks the code, and then the next patch fixes
> it, since it changes the type of first_usable_lba from char[8] to
> __le64?

First of all, thanks for very thorough review. 

You are right, my fault. Patch 2/6 and 3/6 shall be squashed together.
When squashed, no errors and warnings appear.

I've unnecessary separated them to show this two step change. For the
sake of bisecting - those shall be squashed.
Łukasz Majewski Sept. 6, 2012, 10:20 a.m. UTC | #4 
Hi Stephen,

> On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
> > Custom definitions of le_XX_to_int functions have been replaced with
> > standard ones, defined at <compiler.h>
> > 
> > Signed-off-by: Chang Hyun Park <chchch.park@samsung.com>
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> 
> Did Chang Hyun Park write this? If so, shouldn't git have generated a
> "From" header for him, to reflect his authorship upstream. I'm not
> sure how Kyungmin Park's S-o-b plays into this, since he's not
> sending the patch.

My work (for those patches) was based on Mr. Chang Hyum Park patches.
However I had to modify his work for patches submission.

Therefore I've added Mr. Chang in a first place to credit his effort to
this patch series creation.

Shall I do it in a different way?
Stephen Warren Sept. 6, 2012, 6:53 p.m. UTC | #5 
On 09/06/2012 04:20 AM, Lukasz Majewski wrote:
> Hi Stephen,
> 
>> On 08/24/2012 02:13 AM, Lukasz Majewski wrote:
>>> Custom definitions of le_XX_to_int functions have been replaced with
>>> standard ones, defined at <compiler.h>
>>>
>>> Signed-off-by: Chang Hyun Park <chchch.park@samsung.com>
>>> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>
>> Did Chang Hyun Park write this? If so, shouldn't git have generated a
>> "From" header for him, to reflect his authorship upstream. I'm not
>> sure how Kyungmin Park's S-o-b plays into this, since he's not
>> sending the patch.
> 
> My work (for those patches) was based on Mr. Chang Hyum Park patches.
> However I had to modify his work for patches submission.
> 
> Therefore I've added Mr. Chang in a first place to credit his effort to
> this patch series creation.
> 
> Shall I do it in a different way?

To be honest, I'm not really sure. When I've done this, I've kept the
original author in the git "author" field to show this, and then listed
what I changed in the commit description. Whether that's the right way
to go perhaps depends on the size of your changes. If your changes are
so large that it doesn't make sense to do that, perhaps his S-o-b should
be removed and replaced with a simple note "based on work by Chang Hyun
Park <chchch.park@samsung.com>"?
Łukasz Majewski Sept. 12, 2012, 2:42 p.m. UTC | #6 
Hi Chang Hyun,

> Hello Lukasz, Stephen, 
> I see there has been quite a few mails going back and forth on the
> mailing list on the "gpt: Replace the leXX_to_int() calls with ones
> defined at <compiler.h>"I haven't been receiving the e-mails because
> my samsung e-mail account has expired.(Internship expired)(I just
> happened to run into the mails from a google search) Is there
> anything I should catch up on? Anything I should be doing?And on
> which fork is this commit being pushed onto? Thank you.
> 

I've changed you as an author of the commit. I will also add your
"private" e-mail to CC when I send gpt v2, so stay tunned :-)
diff mbox

Patch

diff --git a/disk/part_efi.c b/disk/part_efi.c
index 02927a0..86e7f33 100644
--- a/disk/part_efi.c
+++ b/disk/part_efi.c
@@ -44,34 +44,6 @@ 
     defined(CONFIG_MMC) || \
     defined(CONFIG_SYSTEMACE)
 
-/* Convert char[2] in little endian format to the host format integer
- */
-static inline unsigned short le16_to_int(unsigned char *le16)
-{
-	return ((le16[1] << 8) + le16[0]);
-}
-
-/* Convert char[4] in little endian format to the host format integer
- */
-static inline unsigned long le32_to_int(unsigned char *le32)
-{
-	return ((le32[3] << 24) + (le32[2] << 16) + (le32[1] << 8) + le32[0]);
-}
-
-/* Convert char[8] in little endian format to the host format integer
- */
-static inline unsigned long long le64_to_int(unsigned char *le64)
-{
-	return (((unsigned long long)le64[7] << 56) +
-		((unsigned long long)le64[6] << 48) +
-		((unsigned long long)le64[5] << 40) +
-		((unsigned long long)le64[4] << 32) +
-		((unsigned long long)le64[3] << 24) +
-		((unsigned long long)le64[2] << 16) +
-		((unsigned long long)le64[1] << 8) +
-		(unsigned long long)le64[0]);
-}
-
 /**
  * efi_crc32() - EFI version of crc32 function
  * @buf: buffer to calculate crc32 of
@@ -79,7 +51,7 @@  static inline unsigned long long le64_to_int(unsigned char *le64)
  *
  * Description: Returns EFI-style CRC32 value for @buf
  */
-static inline unsigned long efi_crc32(const void *buf, unsigned long len)
+static inline u32 efi_crc32(const void *buf, u32 len)
 {
 	return crc32(0, buf, len);
 }
@@ -137,13 +109,13 @@  void print_part_efi(block_dev_desc_t * dev_desc)
 	debug("%s: gpt-entry at %p\n", __func__, gpt_pte);
 
 	printf("Part\tName\t\t\tStart LBA\tEnd LBA\n");
-	for (i = 0; i < le32_to_int(gpt_head->num_partition_entries); i++) {
+	for (i = 0; i < le32_to_cpu(gpt_head->num_partition_entries); i++) {
 
 		if (is_pte_valid(&gpt_pte[i])) {
 			printf("%3d\t%-18s\t0x%08llX\t0x%08llX\n", (i + 1),
 				print_efiname(&gpt_pte[i]),
-				le64_to_int(gpt_pte[i].starting_lba),
-				le64_to_int(gpt_pte[i].ending_lba));
+			       (u64) le64_to_cpu(gpt_pte[i].starting_lba),
+			       (u64) le64_to_cpu(gpt_pte[i].ending_lba));
 		} else {
 			break;	/* Stop at the first non valid PTE */
 		}
@@ -174,9 +146,9 @@  int get_partition_info_efi(block_dev_desc_t * dev_desc, int part,
 	}
 
 	/* The ulong casting limits the maximum disk size to 2 TB */
-	info->start = (ulong) le64_to_int(gpt_pte[part - 1].starting_lba);
+	info->start = (u64) le64_to_cpu(gpt_pte[part - 1].starting_lba);
 	/* The ending LBA is inclusive, to calculate size, add 1 to it */
-	info->size = ((ulong)le64_to_int(gpt_pte[part - 1].ending_lba) + 1)
+	info->size = ((u64)le64_to_cpu(gpt_pte[part - 1].ending_lba) + 1)
 		     - info->start;
 	info->blksz = GPT_BLOCK_SIZE;
 
@@ -215,7 +187,7 @@  int test_part_efi(block_dev_desc_t * dev_desc)
 static int pmbr_part_valid(struct partition *part)
 {
 	if (part->sys_ind == EFI_PMBR_OSTYPE_EFI_GPT &&
-		le32_to_int(part->start_sect) == 1UL) {
+		le32_to_cpu(part->start_sect) == 1UL) {
 		return 1;
 	}
 
@@ -234,9 +206,8 @@  static int is_pmbr_valid(legacy_mbr * mbr)
 {
 	int i = 0;
 
-	if (!mbr || le16_to_int(mbr->signature) != MSDOS_MBR_SIGNATURE) {
+	if (!mbr || le16_to_cpu(mbr->signature) != MSDOS_MBR_SIGNATURE)
 		return 0;
-	}
 
 	for (i = 0; i < 4; i++) {
 		if (pmbr_part_valid(&mbr->partition_record[i])) {
@@ -259,8 +230,8 @@  static int is_pmbr_valid(legacy_mbr * mbr)
 static int is_gpt_valid(block_dev_desc_t * dev_desc, unsigned long long lba,
 			gpt_header * pgpt_head, gpt_entry ** pgpt_pte)
 {
-	unsigned char crc32_backup[4] = { 0 };
-	unsigned long calc_crc32;
+	u32 crc32_backup = 0;
+	u32 calc_crc32;
 	unsigned long long lastlba;
 
 	if (!dev_desc || !pgpt_head) {
@@ -275,54 +246,54 @@  static int is_gpt_valid(block_dev_desc_t * dev_desc, unsigned long long lba,
 	}
 
 	/* Check the GPT header signature */
-	if (le64_to_int(pgpt_head->signature) != GPT_HEADER_SIGNATURE) {
+	if (le64_to_cpu(pgpt_head->signature) != GPT_HEADER_SIGNATURE) {
 		printf("GUID Partition Table Header signature is wrong:"
 			"0x%llX != 0x%llX\n",
-			(unsigned long long)le64_to_int(pgpt_head->signature),
-			(unsigned long long)GPT_HEADER_SIGNATURE);
+			(u64) le64_to_cpu(pgpt_head->signature),
+			(u64) GPT_HEADER_SIGNATURE);
 		return 0;
 	}
 
 	/* Check the GUID Partition Table CRC */
-	memcpy(crc32_backup, pgpt_head->header_crc32, sizeof(crc32_backup));
-	memset(pgpt_head->header_crc32, 0, sizeof(pgpt_head->header_crc32));
+	memcpy(&crc32_backup, &pgpt_head->header_crc32, sizeof(crc32_backup));
+	memset(&pgpt_head->header_crc32, 0, sizeof(pgpt_head->header_crc32));
 
 	calc_crc32 = efi_crc32((const unsigned char *)pgpt_head,
-		le32_to_int(pgpt_head->header_size));
+		le32_to_cpu(pgpt_head->header_size));
 
-	memcpy(pgpt_head->header_crc32, crc32_backup, sizeof(crc32_backup));
+	memcpy(&pgpt_head->header_crc32, &crc32_backup, sizeof(crc32_backup));
 
-	if (calc_crc32 != le32_to_int(crc32_backup)) {
+	if (calc_crc32 != le32_to_cpu(crc32_backup)) {
 		printf("GUID Partition Table Header CRC is wrong:"
-			"0x%08lX != 0x%08lX\n",
-			le32_to_int(crc32_backup), calc_crc32);
+			"0x%x != 0x%x\n",
+		       (u32) le32_to_cpu(crc32_backup), calc_crc32);
 		return 0;
 	}
 
 	/* Check that the my_lba entry points to the LBA that contains the GPT */
-	if (le64_to_int(pgpt_head->my_lba) != lba) {
+	if (le64_to_cpu(pgpt_head->my_lba) != lba) {
 		printf("GPT: my_lba incorrect: %llX != %llX\n",
-			(unsigned long long)le64_to_int(pgpt_head->my_lba),
-			(unsigned long long)lba);
+			(u64)le64_to_cpu(pgpt_head->my_lba),
+			(u64)lba);
 		return 0;
 	}
 
 	/* Check the first_usable_lba and last_usable_lba are within the disk. */
 	lastlba = (unsigned long long)dev_desc->lba;
-	if (le64_to_int(pgpt_head->first_usable_lba) > lastlba) {
+	if (le64_to_cpu(pgpt_head->first_usable_lba) > lastlba) {
 		printf("GPT: first_usable_lba incorrect: %llX > %llX\n",
-			le64_to_int(pgpt_head->first_usable_lba), lastlba);
+		       (u64) le64_to_cpu(pgpt_head->first_usable_lba), lastlba);
 		return 0;
 	}
-	if (le64_to_int(pgpt_head->last_usable_lba) > lastlba) {
+	if (le64_to_cpu(pgpt_head->last_usable_lba) > lastlba) {
 		printf("GPT: last_usable_lba incorrect: %llX > %llX\n",
-			le64_to_int(pgpt_head->last_usable_lba), lastlba);
+		       (u64) le64_to_cpu(pgpt_head->last_usable_lba), lastlba);
 		return 0;
 	}
 
 	debug("GPT: first_usable_lba: %llX last_usable_lba %llX last lba %llX\n",
-		le64_to_int(pgpt_head->first_usable_lba),
-		le64_to_int(pgpt_head->last_usable_lba), lastlba);
+	      (u64) le64_to_cpu(pgpt_head->first_usable_lba),
+	      (u64) le64_to_cpu(pgpt_head->last_usable_lba), lastlba);
 
 	/* Read and allocate Partition Table Entries */
 	*pgpt_pte = alloc_read_gpt_entries(dev_desc, pgpt_head);
@@ -333,13 +304,13 @@  static int is_gpt_valid(block_dev_desc_t * dev_desc, unsigned long long lba,
 
 	/* Check the GUID Partition Table Entry Array CRC */
 	calc_crc32 = efi_crc32((const unsigned char *)*pgpt_pte,
-		le32_to_int(pgpt_head->num_partition_entries) *
-		le32_to_int(pgpt_head->sizeof_partition_entry));
+		le32_to_cpu(pgpt_head->num_partition_entries) *
+		le32_to_cpu(pgpt_head->sizeof_partition_entry));
 
-	if (calc_crc32 != le32_to_int(pgpt_head->partition_entry_array_crc32)) {
+	if (calc_crc32 != le32_to_cpu(pgpt_head->partition_entry_array_crc32)) {
 		printf("GUID Partition Table Entry Array CRC is wrong:"
-			"0x%08lX != 0x%08lX\n",
-			le32_to_int(pgpt_head->partition_entry_array_crc32),
+			"0x%x != 0x%x\n",
+		       (u32)le32_to_cpu(pgpt_head->partition_entry_array_crc32),
 			calc_crc32);
 
 		free(*pgpt_pte);
@@ -370,12 +341,12 @@  static gpt_entry *alloc_read_gpt_entries(block_dev_desc_t * dev_desc,
 		return NULL;
 	}
 
-	count = le32_to_int(pgpt_head->num_partition_entries) *
-		le32_to_int(pgpt_head->sizeof_partition_entry);
+	count = le32_to_cpu(pgpt_head->num_partition_entries) *
+		le32_to_cpu(pgpt_head->sizeof_partition_entry);
 
-	debug("%s: count = %lu * %lu = %u\n", __func__,
-		le32_to_int(pgpt_head->num_partition_entries),
-		le32_to_int(pgpt_head->sizeof_partition_entry), count);
+	debug("%s: count = %u * %u = %u\n", __func__,
+	      (u32) le32_to_cpu(pgpt_head->num_partition_entries),
+	      (u32) le32_to_cpu(pgpt_head->sizeof_partition_entry), count);
 
 	/* Allocate memory for PTE, remember to FREE */
 	if (count != 0) {
@@ -390,7 +361,7 @@  static gpt_entry *alloc_read_gpt_entries(block_dev_desc_t * dev_desc,
 
 	/* Read GPT Entries from device */
 	if (dev_desc->block_read (dev_desc->dev,
-		(unsigned long)le64_to_int(pgpt_head->partition_entry_lba),
+		(u64) le64_to_cpu(pgpt_head->partition_entry_lba),
 		(lbaint_t) (count / GPT_BLOCK_SIZE), pte)
 		!= (count / GPT_BLOCK_SIZE)) {