diff mbox

[U-Boot,V2] disk: make get_partition_info() always available to disk.c

Message ID 1348267614-22976-1-git-send-email-swarren@wwwdotorg.org
State Accepted
Delegated to: Tom Rini
Headers show

Commit Message

Stephen Warren Sept. 21, 2012, 10:46 p.m. UTC 
From: Stephen Warren <swarren@nvidia.com>

Now that get_device_and_partition() always calls get_partition_info()
when disk.c is compiled, we must always compile the function, rather
than ifdef it away.

The implementation must be conditional based on CONFIG_CMD_* etc., since
that's what e.g. part_dos.c uses to ifdef out get_partition_info_dos();
CONFIG_DOS_PARTITION can be enabled even without those commands being
enabled.

Technically, this change is required before Rob's "disk/part: introduce
get_device_and_partition" patch. However, at least when the compiler
optimizer is turned on, it isn't required before then in practice,
since get_device_and_partition() calls get_dev(), which is stubbed out
in disk.c under exactly the same conditions that get_partition_info()
is not compiled, and hence the compiler never generates code for the
call to the missing function. However, in my later patch "disk:
get_device_and_partition() "auto" partition and cleanup", the optimizer
doesn't succeed at this, and may attempt to reference the undefined
function.

Signed-off-by: Stephen Warren <swarren@nvidia.com>
---
v2: Add CONFIG_CMD_* etc. ifdefs around the implementation.

Rob, I wonder if you shouldn't squash this into your series. Then, I'll
need to rebase mine on your again since this causes a few nasty conflicts
with my series.
---
 disk/part.c |  118 +++++++++++++++++++++++++++++++---------------------------
 1 files changed, 63 insertions(+), 55 deletions(-)

Comments

Tom Rini Sept. 21, 2012, 10:51 p.m. UTC | #1 
On Fri, Sep 21, 2012 at 04:46:54PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren@nvidia.com>
> 
> Now that get_device_and_partition() always calls get_partition_info()
> when disk.c is compiled, we must always compile the function, rather
> than ifdef it away.
> 
> The implementation must be conditional based on CONFIG_CMD_* etc., since
> that's what e.g. part_dos.c uses to ifdef out get_partition_info_dos();
> CONFIG_DOS_PARTITION can be enabled even without those commands being
> enabled.
> 
> Technically, this change is required before Rob's "disk/part: introduce
> get_device_and_partition" patch. However, at least when the compiler
> optimizer is turned on, it isn't required before then in practice,
> since get_device_and_partition() calls get_dev(), which is stubbed out
> in disk.c under exactly the same conditions that get_partition_info()
> is not compiled, and hence the compiler never generates code for the
> call to the missing function. However, in my later patch "disk:
> get_device_and_partition() "auto" partition and cleanup", the optimizer
> doesn't succeed at this, and may attempt to reference the undefined
> function.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>
> ---
> v2: Add CONFIG_CMD_* etc. ifdefs around the implementation.
> 
> Rob, I wonder if you shouldn't squash this into your series. Then, I'll
> need to rebase mine on your again since this causes a few nasty conflicts
> with my series.

I _really_ want to see incremental changes.  It's one of the good
practices of the kernel folks and I'd like to see us do it as well as
much as we can.
Stephen Warren Sept. 21, 2012, 10:53 p.m. UTC | #2 
On 09/21/2012 04:51 PM, Tom Rini wrote:
> On Fri, Sep 21, 2012 at 04:46:54PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren@nvidia.com>
>> 
>> Now that get_device_and_partition() always calls
>> get_partition_info() when disk.c is compiled, we must always
>> compile the function, rather than ifdef it away.
>> 
>> The implementation must be conditional based on CONFIG_CMD_*
>> etc., since that's what e.g. part_dos.c uses to ifdef out
>> get_partition_info_dos(); CONFIG_DOS_PARTITION can be enabled
>> even without those commands being enabled.
>> 
>> Technically, this change is required before Rob's "disk/part:
>> introduce get_device_and_partition" patch. However, at least when
>> the compiler optimizer is turned on, it isn't required before
>> then in practice, since get_device_and_partition() calls
>> get_dev(), which is stubbed out in disk.c under exactly the same
>> conditions that get_partition_info() is not compiled, and hence
>> the compiler never generates code for the call to the missing
>> function. However, in my later patch "disk: 
>> get_device_and_partition() "auto" partition and cleanup", the
>> optimizer doesn't succeed at this, and may attempt to reference
>> the undefined function.
>> 
>> Signed-off-by: Stephen Warren <swarren@nvidia.com> --- v2: Add
>> CONFIG_CMD_* etc. ifdefs around the implementation.
>> 
>> Rob, I wonder if you shouldn't squash this into your series.
>> Then, I'll need to rebase mine on your again since this causes a
>> few nasty conflicts with my series.
> 
> I _really_ want to see incremental changes.  It's one of the good 
> practices of the kernel folks and I'd like to see us do it as well
> as much as we can.

OK, well in that case, you can just apply the patch standalone before
you apply Rob's series then. I think we'll both need to rebase to
avoid conflict issues those - e.g. I edited the function that got
moved in my patch series and had to manually re-apply the change to
the new code location after I created this patch.
Tom Rini Sept. 25, 2012, 11:16 p.m. UTC | #3 
On Fri, Sep 21, 2012 at 12:46:54PM -0000, Stephen Warren wrote:

> From: Stephen Warren <swarren@nvidia.com>
> 
> Now that get_device_and_partition() always calls get_partition_info()
> when disk.c is compiled, we must always compile the function, rather
> than ifdef it away.
> 
> The implementation must be conditional based on CONFIG_CMD_* etc., since
> that's what e.g. part_dos.c uses to ifdef out get_partition_info_dos();
> CONFIG_DOS_PARTITION can be enabled even without those commands being
> enabled.
> 
> Technically, this change is required before Rob's "disk/part: introduce
> get_device_and_partition" patch. However, at least when the compiler
> optimizer is turned on, it isn't required before then in practice,
> since get_device_and_partition() calls get_dev(), which is stubbed out
> in disk.c under exactly the same conditions that get_partition_info()
> is not compiled, and hence the compiler never generates code for the
> call to the missing function. However, in my later patch "disk:
> get_device_and_partition() "auto" partition and cleanup", the optimizer
> doesn't succeed at this, and may attempt to reference the undefined
> function.
> 
> Signed-off-by: Stephen Warren <swarren@nvidia.com>

Applied to u-boot/master, thanks!
diff mbox

Patch

diff --git a/disk/part.c b/disk/part.c
index 76f3939..f659cc3 100644
--- a/disk/part.c
+++ b/disk/part.c
@@ -291,61 +291,6 @@  void init_part (block_dev_desc_t * dev_desc)
 }
 
 
-int get_partition_info (block_dev_desc_t *dev_desc, int part
-					, disk_partition_t *info)
-{
-	switch (dev_desc->part_type) {
-#ifdef CONFIG_MAC_PARTITION
-	case PART_TYPE_MAC:
-		if (get_partition_info_mac(dev_desc,part,info) == 0) {
-			PRINTF ("## Valid MAC partition found ##\n");
-			return (0);
-		}
-		break;
-#endif
-
-#ifdef CONFIG_DOS_PARTITION
-	case PART_TYPE_DOS:
-		if (get_partition_info_dos(dev_desc,part,info) == 0) {
-			PRINTF ("## Valid DOS partition found ##\n");
-			return (0);
-		}
-		break;
-#endif
-
-#ifdef CONFIG_ISO_PARTITION
-	case PART_TYPE_ISO:
-		if (get_partition_info_iso(dev_desc,part,info) == 0) {
-			PRINTF ("## Valid ISO boot partition found ##\n");
-			return (0);
-		}
-		break;
-#endif
-
-#ifdef CONFIG_AMIGA_PARTITION
-	case PART_TYPE_AMIGA:
-	    if (get_partition_info_amiga(dev_desc, part, info) == 0)
-	    {
-		PRINTF ("## Valid Amiga partition found ##\n");
-		return (0);
-	    }
-	    break;
-#endif
-
-#ifdef CONFIG_EFI_PARTITION
-	case PART_TYPE_EFI:
-		if (get_partition_info_efi(dev_desc,part,info) == 0) {
-			PRINTF ("## Valid EFI partition found ##\n");
-			return (0);
-		}
-		break;
-#endif
-	default:
-		break;
-	}
-	return (-1);
-}
-
 static void print_part_header (const char *type, block_dev_desc_t * dev_desc)
 {
 	puts ("\nPartition Map for ");
@@ -433,3 +378,66 @@  void print_part (block_dev_desc_t * dev_desc)
 #endif
 
 #endif
+
+int get_partition_info(block_dev_desc_t *dev_desc, int part
+					, disk_partition_t *info)
+{
+#if defined(CONFIG_CMD_IDE) || \
+	defined(CONFIG_CMD_SATA) || \
+	defined(CONFIG_CMD_SCSI) || \
+	defined(CONFIG_CMD_USB) || \
+	defined(CONFIG_MMC) || \
+	defined(CONFIG_SYSTEMACE)
+
+	switch (dev_desc->part_type) {
+#ifdef CONFIG_MAC_PARTITION
+	case PART_TYPE_MAC:
+		if (get_partition_info_mac(dev_desc, part, info) == 0) {
+			PRINTF("## Valid MAC partition found ##\n");
+			return 0;
+		}
+		break;
+#endif
+
+#ifdef CONFIG_DOS_PARTITION
+	case PART_TYPE_DOS:
+		if (get_partition_info_dos(dev_desc, part, info) == 0) {
+			PRINTF("## Valid DOS partition found ##\n");
+			return 0;
+		}
+		break;
+#endif
+
+#ifdef CONFIG_ISO_PARTITION
+	case PART_TYPE_ISO:
+		if (get_partition_info_iso(dev_desc, part, info) == 0) {
+			PRINTF("## Valid ISO boot partition found ##\n");
+			return 0;
+		}
+		break;
+#endif
+
+#ifdef CONFIG_AMIGA_PARTITION
+	case PART_TYPE_AMIGA:
+		if (get_partition_info_amiga(dev_desc, part, info) == 0) {
+			PRINTF("## Valid Amiga partition found ##\n");
+			return 0;
+		}
+		break;
+#endif
+
+#ifdef CONFIG_EFI_PARTITION
+	case PART_TYPE_EFI:
+		if (get_partition_info_efi(dev_desc, part, info) == 0) {
+			PRINTF("## Valid EFI partition found ##\n");
+			return 0;
+		}
+		break;
+#endif
+	default:
+		break;
+	}
+#endif
+
+	return -1;
+}