| Message ID | 1345757510-6756-4-git-send-email-robherring2@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
On 08/23/2012 03:31 PM, Rob Herring wrote: > From: Rob Herring <rob.herring@calxeda.com> > > All block device related commands (scsiboot, fatload, ext2ls, etc.) have > simliar duplicated device and partition parsing and selection code. This > adds a common function to replace various implementations. > > The new function has some enhancements over current versions. If no device > or partition is specified on the command line, the bootdevice env variable > will be used (scsiboot does this). If the partition is not specified and > the device has partitions, then the first bootable partition will be used. > If a bootable partition is not found, the first valid partition is used. > The ret value is not needed since part will be zero when no partition is > found. Two thoughts on this patch: First, if I write "mmc 0" right now, command will always attempt to access precisely partion 1, whereas after this patch, they will search for the first bootable, or valid, partition. This is a change in behavior. It's a pretty reasonable change, but I wonder if it might cause problems somewhere. Instead, perhaps this new feature should be explicitly requested, supporting the following device/partition specifications: # existing: dev 0:0 # whole device dev 0:n # n >= 1: explicit partition dev 0 # partition 1 # new: dev 0:valid # first valid partition dev 0:bootable # first bootable partition dev 0:default # first bootable partition if there is one, # else first valid That would allow scripts to be very explicit about whether they wanted this new functionality. Second, if I run a slew of ext2load commands: ext2load mmc 0:bootable ${scriptaddr} boot.scr source ${scriptaddr} # script does: ext2load mmc 0:bootable ${kernel_addr} zImage ext2load mmc 0:bootable ${initrd_addr} initrd.bin ext2load mmc 0:bootable ${fdt_addr} foo.dtb Then there are two disadvantages: 1) I believe the partition table is read and decoded and search for every one of those ext2load commands. Slightly inefficient. 2) There's no permanent record of the partition number, so this couldn't be e.g. used to construct a kernel command-line etc. Instead, I wonder if get_device_and_partition() should just support the existing 3 device specification options, and we introduce a new command to determine which partition to boot from, e.g.: # writes result to "bootpart" variable # or get-default or get-first-valid part get-first-bootable mmc 0 bootpart ext2load mmc 0:${bootpart} ${scriptaddr} boot.scr source ${scriptaddr} # script does: ext2load mmc 0:${bootpart} ${kernel_addr} zImage ext2load mmc 0:${bootpart} ${initrd_addr} initrd.bin ext2load mmc 0:${bootpart} ${fdt_addr} foo.dtb That solves those issues. Does anyone have any comment on the two approaches? (although perhaps e.g. ext2load always re-reads the partition table anyway, so perhaps that advantage is moot?) Aside from that, this series looks conceptually reasonable at a quick glance. I'd be happy to provide an equivalent to patch 2 for GPT/EFI partition tables.
On 08/23/2012 05:36 PM, Stephen Warren wrote: > On 08/23/2012 03:31 PM, Rob Herring wrote: >> From: Rob Herring <rob.herring@calxeda.com> >> >> All block device related commands (scsiboot, fatload, ext2ls, etc.) have >> simliar duplicated device and partition parsing and selection code. This >> adds a common function to replace various implementations. >> >> The new function has some enhancements over current versions. If no device >> or partition is specified on the command line, the bootdevice env variable >> will be used (scsiboot does this). If the partition is not specified and >> the device has partitions, then the first bootable partition will be used. >> If a bootable partition is not found, the first valid partition is used. >> The ret value is not needed since part will be zero when no partition is >> found. > > Two thoughts on this patch: > > First, if I write "mmc 0" right now, command will always attempt to > access precisely partion 1, whereas after this patch, they will search > for the first bootable, or valid, partition. This is a change in > behavior. It's a pretty reasonable change, but I wonder if it might > cause problems somewhere. > > Instead, perhaps this new feature should be explicitly requested, > supporting the following device/partition specifications: > > # existing: > dev 0:0 # whole device > dev 0:n # n >= 1: explicit partition > dev 0 # partition 1 > # new: > dev 0:valid # first valid partition > dev 0:bootable # first bootable partition > dev 0:default # first bootable partition if there is one, > # else first valid I'm not sure we need to distinguish valid vs. bootable. Returning the first valid partition was really just to maintain somewhat backwards compatible behavior. Perhaps just "0:-" would be sufficient. > > That would allow scripts to be very explicit about whether they wanted > this new functionality. > > Second, if I run a slew of ext2load commands: > > ext2load mmc 0:bootable ${scriptaddr} boot.scr > source ${scriptaddr} > # script does: > ext2load mmc 0:bootable ${kernel_addr} zImage > ext2load mmc 0:bootable ${initrd_addr} initrd.bin > ext2load mmc 0:bootable ${fdt_addr} foo.dtb > > Then there are two disadvantages: > > 1) I believe the partition table is read and decoded and search for > every one of those ext2load commands. Slightly inefficient. It was already multiple times per command with the command function calling get_partition_info and then the filesystem code calling it again internally as well. Now it is only 1 time at least. I would think the 1st partition being bootable is the common case. > 2) There's no permanent record of the partition number, so this couldn't > be e.g. used to construct a kernel command-line etc. You mean to setup rootfs? I don't think we want u-boot to do that. Or what would be the use? > Instead, I wonder if get_device_and_partition() should just support the > existing 3 device specification options, and we introduce a new command > to determine which partition to boot from, e.g.: > > # writes result to "bootpart" variable > # or get-default or get-first-valid > part get-first-bootable mmc 0 bootpart > > ext2load mmc 0:${bootpart} ${scriptaddr} boot.scr > source ${scriptaddr} > # script does: > ext2load mmc 0:${bootpart} ${kernel_addr} zImage > ext2load mmc 0:${bootpart} ${initrd_addr} initrd.bin > ext2load mmc 0:${bootpart} ${fdt_addr} foo.dtb > > That solves those issues. Does anyone have any comment on the two > approaches? I'm really open to either way. Another option would be for the first command run to set bootpart and then re-use that value on subsequent commands. Rob > > (although perhaps e.g. ext2load always re-reads the partition table > anyway, so perhaps that advantage is moot?) > > Aside from that, this series looks conceptually reasonable at a quick > glance. I'd be happy to provide an equivalent to patch 2 for GPT/EFI > partition tables. >
On 08/23/2012 07:57 PM, Rob Herring wrote: > On 08/23/2012 05:36 PM, Stephen Warren wrote: >> On 08/23/2012 03:31 PM, Rob Herring wrote: >>> From: Rob Herring <rob.herring@calxeda.com> >>> >>> All block device related commands (scsiboot, fatload, ext2ls, etc.) have >>> simliar duplicated device and partition parsing and selection code. This >>> adds a common function to replace various implementations. >>> >>> The new function has some enhancements over current versions. If no device >>> or partition is specified on the command line, the bootdevice env variable >>> will be used (scsiboot does this). If the partition is not specified and >>> the device has partitions, then the first bootable partition will be used. >>> If a bootable partition is not found, the first valid partition is used. >>> The ret value is not needed since part will be zero when no partition is >>> found. >> >> Two thoughts on this patch: >> >> First, if I write "mmc 0" right now, command will always attempt to >> access precisely partion 1, whereas after this patch, they will search >> for the first bootable, or valid, partition. This is a change in >> behavior. It's a pretty reasonable change, but I wonder if it might >> cause problems somewhere. >> >> Instead, perhaps this new feature should be explicitly requested, >> supporting the following device/partition specifications: >> >> # existing: >> dev 0:0 # whole device >> dev 0:n # n >= 1: explicit partition >> dev 0 # partition 1 >> # new: >> dev 0:valid # first valid partition >> dev 0:bootable # first bootable partition >> dev 0:default # first bootable partition if there is one, >> # else first valid > > I'm not sure we need to distinguish valid vs. bootable. Returning the > first valid partition was really just to maintain somewhat backwards > compatible behavior. > > Perhaps just "0:-" would be sufficient. I guess that syntax would be fine if we don't need to distinguish all the cases. "-" isn't that descriptive though, and I've only seen it mean "nothing" in U-Boot commands. So, bike-shedding a bit, it doesn't seem exactly correct. Perhaps just "auto"? >> That would allow scripts to be very explicit about whether they wanted >> this new functionality. >> >> Second, if I run a slew of ext2load commands: >> >> ext2load mmc 0:bootable ${scriptaddr} boot.scr >> source ${scriptaddr} >> # script does: >> ext2load mmc 0:bootable ${kernel_addr} zImage >> ext2load mmc 0:bootable ${initrd_addr} initrd.bin >> ext2load mmc 0:bootable ${fdt_addr} foo.dtb >> >> Then there are two disadvantages: >> >> 1) I believe the partition table is read and decoded and search for >> every one of those ext2load commands. Slightly inefficient. > > It was already multiple times per command with the command function > calling get_partition_info and then the filesystem code calling it again > internally as well. Now it is only 1 time at least. I would think the > 1st partition being bootable is the common case. > >> 2) There's no permanent record of the partition number, so this couldn't >> be e.g. used to construct a kernel command-line etc. > > You mean to setup rootfs? I don't think we want u-boot to do that. Or > what would be the use? I can imagine a boot.scr that does: setenv bootargs root=/dev/mmcblk0p${bootpart} But then, you may as well use the partition UUID feature instead of that, so that boot.scr doesn't need to know the kernel's device name. >> Instead, I wonder if get_device_and_partition() should just support the >> existing 3 device specification options, and we introduce a new command >> to determine which partition to boot from, e.g.: >> >> # writes result to "bootpart" variable >> # or get-default or get-first-valid >> part get-first-bootable mmc 0 bootpart >> >> ext2load mmc 0:${bootpart} ${scriptaddr} boot.scr >> source ${scriptaddr} >> # script does: >> ext2load mmc 0:${bootpart} ${kernel_addr} zImage >> ext2load mmc 0:${bootpart} ${initrd_addr} initrd.bin >> ext2load mmc 0:${bootpart} ${fdt_addr} foo.dtb >> >> That solves those issues. Does anyone have any comment on the two >> approaches? > > I'm really open to either way. > > Another option would be for the first command run to set bootpart and > then re-use that value on subsequent commands. That could work too, although commands using environment variables seems a little implicit/hidden.
On Thu, Aug 23, 2012 at 08:51:50PM -0600, Stephen Warren wrote: > On 08/23/2012 07:57 PM, Rob Herring wrote: > > On 08/23/2012 05:36 PM, Stephen Warren wrote: > >> On 08/23/2012 03:31 PM, Rob Herring wrote: > >>> From: Rob Herring <rob.herring@calxeda.com> > >>> > >>> All block device related commands (scsiboot, fatload, ext2ls, etc.) have > >>> simliar duplicated device and partition parsing and selection code. This > >>> adds a common function to replace various implementations. > >>> > >>> The new function has some enhancements over current versions. If no device > >>> or partition is specified on the command line, the bootdevice env variable > >>> will be used (scsiboot does this). If the partition is not specified and > >>> the device has partitions, then the first bootable partition will be used. > >>> If a bootable partition is not found, the first valid partition is used. > >>> The ret value is not needed since part will be zero when no partition is > >>> found. > >> > >> Two thoughts on this patch: > >> > >> First, if I write "mmc 0" right now, command will always attempt to > >> access precisely partion 1, whereas after this patch, they will search > >> for the first bootable, or valid, partition. This is a change in > >> behavior. It's a pretty reasonable change, but I wonder if it might > >> cause problems somewhere. > >> > >> Instead, perhaps this new feature should be explicitly requested, > >> supporting the following device/partition specifications: > >> > >> # existing: > >> dev 0:0 # whole device > >> dev 0:n # n >= 1: explicit partition > >> dev 0 # partition 1 > >> # new: > >> dev 0:valid # first valid partition > >> dev 0:bootable # first bootable partition > >> dev 0:default # first bootable partition if there is one, > >> # else first valid > > > > I'm not sure we need to distinguish valid vs. bootable. Returning the > > first valid partition was really just to maintain somewhat backwards > > compatible behavior. > > > > Perhaps just "0:-" would be sufficient. > > I guess that syntax would be fine if we don't need to distinguish all > the cases. "-" isn't that descriptive though, and I've only seen it mean > "nothing" in U-Boot commands. So, bike-shedding a bit, it doesn't seem > exactly correct. Perhaps just "auto"? "auto" sounds like a good idea to me as well. [snip] > >> Instead, I wonder if get_device_and_partition() should just support the > >> existing 3 device specification options, and we introduce a new command > >> to determine which partition to boot from, e.g.: > >> > >> # writes result to "bootpart" variable > >> # or get-default or get-first-valid > >> part get-first-bootable mmc 0 bootpart > >> > >> ext2load mmc 0:${bootpart} ${scriptaddr} boot.scr > >> source ${scriptaddr} > >> # script does: > >> ext2load mmc 0:${bootpart} ${kernel_addr} zImage > >> ext2load mmc 0:${bootpart} ${initrd_addr} initrd.bin > >> ext2load mmc 0:${bootpart} ${fdt_addr} foo.dtb > >> > >> That solves those issues. Does anyone have any comment on the two > >> approaches? > > > > I'm really open to either way. > > > > Another option would be for the first command run to set bootpart and > > then re-use that value on subsequent commands. > > That could work too, although commands using environment variables seems > a little implicit/hidden. Agreed. We should be very careful when changing behavior. Adding a new command so that folks can use this as they see fit sounds like the best idea. I shudder to think what the partition table on some SD cards I have around looks like.
diff --git a/disk/part.c b/disk/part.c index 76f3939..1284e1a 100644 --- a/disk/part.c +++ b/disk/part.c @@ -70,7 +70,7 @@ static const struct block_drvr block_drvr[] = { DECLARE_GLOBAL_DATA_PTR; -block_dev_desc_t *get_dev(char* ifname, int dev) +block_dev_desc_t *get_dev(const char *ifname, int dev) { const struct block_drvr *drvr = block_drvr; block_dev_desc_t* (*reloc_get_dev)(int dev); @@ -97,7 +97,7 @@ block_dev_desc_t *get_dev(char* ifname, int dev) return NULL; } #else -block_dev_desc_t *get_dev(char* ifname, int dev) +block_dev_desc_t *get_dev(const char *ifname, int dev) { return NULL; } @@ -288,6 +288,7 @@ void init_part (block_dev_desc_t * dev_desc) return; } #endif + dev_desc->part_type = PART_TYPE_UNKNOWN; } @@ -433,3 +434,88 @@ void print_part (block_dev_desc_t * dev_desc) #endif #endif + +int get_device_and_partition(const char *ifname, const char *dev_str, + block_dev_desc_t **dev_desc, + disk_partition_t *info) +{ + int ret; + char *ep; + int dev; + block_dev_desc_t *desc; + int p; + int part = 0; + char *part_str; + disk_partition_t *best_part = NULL; + + if (dev_str) + dev = simple_strtoul(dev_str, &ep, 16); + + if (!dev_str || (dev_str == ep)) { + dev_str = getenv("bootdevice"); + if (dev_str) + dev = simple_strtoul(dev_str, &ep, 16); + if (!dev_str || (dev_str == ep)) + goto err; + } + + desc = get_dev(ifname, dev); + if (!desc || (desc->type == DEV_TYPE_UNKNOWN)) + goto err; + + if (desc->part_type == PART_TYPE_UNKNOWN) { + /* disk doesn't use partition table */ + if (!desc->lba) { + printf("**Bad disk size - %s %d:0 **\n", ifname, dev); + return -1; + } + info->start = 0; + info->size = desc->lba; + info->blksz = desc->blksz; + + *dev_desc = desc; + return 0; + } + + part_str = strchr(dev_str, ':'); + if (part_str) { + part = (int)simple_strtoul(++part_str, NULL, 16); + ret = get_partition_info(desc, part, info); + } else { + /* find the first bootable partition. If none are bootable, + * fall back to the first valid partition */ + for (p = 1; p < 16; p++) { + ret = get_partition_info(desc, p, info); + if (ret) + continue; + + if (!best_part || info->bootable) { + best_part = info; + part = p; + } + + if (info->bootable) + break; + } + info = best_part; + if (part) + ret = 0; + } + if (ret) { + printf("** Invalid partition %d, use `dev[:part]' **\n", part); + return -1; + } + if (strncmp((char *)info->type, BOOT_PART_TYPE, sizeof(info->type)) != 0) { + printf("** Invalid partition type \"%.32s\"" + " (expect \"" BOOT_PART_TYPE "\")\n", + info->type); + return -1; + } + + *dev_desc = desc; + return part; + + err: + puts("** Invalid boot device, use `dev[:part]' **\n"); + return -1; +} diff --git a/include/part.h b/include/part.h index 447f69d..a6d06f3 100644 --- a/include/part.h +++ b/include/part.h @@ -98,7 +98,7 @@ typedef struct disk_partition { /* Misc _get_dev functions */ #ifdef CONFIG_PARTITIONS -block_dev_desc_t* get_dev(char* ifname, int dev); +block_dev_desc_t *get_dev(const char *ifname, int dev); block_dev_desc_t* ide_get_dev(int dev); block_dev_desc_t* sata_get_dev(int dev); block_dev_desc_t* scsi_get_dev(int dev); @@ -112,8 +112,12 @@ int get_partition_info (block_dev_desc_t * dev_desc, int part, disk_partition_t void print_part (block_dev_desc_t *dev_desc); void init_part (block_dev_desc_t *dev_desc); void dev_print(block_dev_desc_t *dev_desc); +int get_device_and_partition(const char *ifname, const char *dev_str, + block_dev_desc_t **dev_desc, + disk_partition_t *info); #else -static inline block_dev_desc_t* get_dev(char* ifname, int dev) { return NULL; } +static inline block_dev_desc_t *get_dev(const char *ifname, int dev) +{ return NULL; } static inline block_dev_desc_t* ide_get_dev(int dev) { return NULL; } static inline block_dev_desc_t* sata_get_dev(int dev) { return NULL; } static inline block_dev_desc_t* scsi_get_dev(int dev) { return NULL; } @@ -127,6 +131,11 @@ static inline int get_partition_info (block_dev_desc_t * dev_desc, int part, static inline void print_part (block_dev_desc_t *dev_desc) {} static inline void init_part (block_dev_desc_t *dev_desc) {} static inline void dev_print(block_dev_desc_t *dev_desc) {} +static inline int get_device_and_partition(const char *ifname, + const char *dev_str, + block_dev_desc_t **dev_desc, + disk_partition_t *info) +{ *dev_desc = NULL; return -1; } #endif #ifdef CONFIG_MAC_PARTITION