| Message ID | 1380905584-21665-1-git-send-email-egbert.eich@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
Dear Egbert Eich, > -----Original Message----- > From: Egbert Eich [mailto:egbert.eich@gmail.com] > Sent: Friday, October 04, 2013 6:53 PM > To: u-boot@lists.denx.de > Cc: Piotr Wilczek; Tom Rini; Egbert Eich; Egbert Eich > Subject: [Patch v3] cmd/gpt: Support gpt command for all devices > > From: Egbert Eich <eich@suse.com> > > The gpt command was only implemented for mmc devices. There is no > reason why this command should not be generalized and be applied all > other storage device classes. > This change both simplifies the implementation and eliminates a build > failure for systems that don't support mmcs. > > Signed-off-by: Egbert Eich <eich@suse.com> > --- > Changes for v2: > - Coding style cleanup. > Changes for v3: > - Removed wrong '&' > - Removed unused variable > - Fixed argument checking > Spotted by Piotr Wilczek <p.wilczek@samsung.com> > > common/cmd_gpt.c | 45 +++++++++++++++++++-------------------------- > 1 file changed, 19 insertions(+), 26 deletions(-) > > diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index a46f5cc..17b1180 > 100644 > --- a/common/cmd_gpt.c > +++ b/common/cmd_gpt.c > @@ -11,7 +11,6 @@ > #include <common.h> > #include <malloc.h> > #include <command.h> > -#include <mmc.h> > #include <part_efi.h> > #include <exports.h> > #include <linux/ctype.h> > @@ -122,7 +121,7 @@ static int set_gpt_info(block_dev_desc_t *dev_desc, > int errno = 0; > uint64_t size_ll, start_ll; > > - debug("%s: MMC lba num: 0x%x %d\n", __func__, > + debug("%s: lba num: 0x%x %d\n", __func__, > (unsigned int)dev_desc->lba, (unsigned int)dev_desc->lba); > > if (str_part == NULL) > @@ -235,25 +234,18 @@ err: > return errno; > } > > -static int gpt_mmc_default(int dev, const char *str_part) > +static int gpt_default(block_dev_desc_t *blk_dev_desc, const char > +*str_part) > { > int ret; > char *str_disk_guid; > u8 part_count = 0; > disk_partition_t *partitions = NULL; > > - struct mmc *mmc = find_mmc_device(dev); > - > - if (mmc == NULL) { > - printf("%s: mmc dev %d NOT available\n", __func__, dev); > - return CMD_RET_FAILURE; > - } > - > if (!str_part) > return -1; > > /* fill partitions */ > - ret = set_gpt_info(&mmc->block_dev, str_part, > + ret = set_gpt_info(blk_dev_desc, str_part, > &str_disk_guid, &partitions, &part_count); > if (ret) { > if (ret == -1) > @@ -266,7 +258,7 @@ static int gpt_mmc_default(int dev, const char > *str_part) > } > > /* save partitions layout to disk */ > - gpt_restore(&mmc->block_dev, str_disk_guid, partitions, > part_count); > + gpt_restore(blk_dev_desc, str_disk_guid, partitions, part_count); > free(str_disk_guid); > free(partitions); > > @@ -287,27 +279,28 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int > argc, char * const argv[]) { > int ret = CMD_RET_SUCCESS; > int dev = 0; > - char *pstr; > > if (argc < 5) > return CMD_RET_USAGE; > > /* command: 'write' */ > if ((strcmp(argv[1], "write") == 0) && (argc == 5)) { > - /* device: 'mmc' */ > - if (strcmp(argv[2], "mmc") == 0) { > - /* check if 'dev' is a number */ > - for (pstr = argv[3]; *pstr != '\0'; pstr++) > - if (!isdigit(*pstr)) { > - printf("'%s' is not a number\n", > - argv[3]); > - return CMD_RET_USAGE; > - } > - dev = (int)simple_strtoul(argv[3], NULL, 10); > - /* write to mmc */ > - if (gpt_mmc_default(dev, argv[4])) > - return CMD_RET_FAILURE; > + char *ep; > + block_dev_desc_t *blk_dev_desc; This probably should be at the beginning of the function > + dev = (int)simple_strtoul(argv[3], &ep, 10); > + if (!ep || ep[0] != '\0') { > + printf("'%s' is not a number\n", argv[3]); > + return CMD_RET_USAGE; > } > + blk_dev_desc = get_dev(argv[2], dev); > + if (!blk_dev_desc) { > + printf("%s: %s dev %d NOT available\n", > + __func__, argv[2], dev); I think it is not necessary since the mmc subsystem prints 'MMC Device not found'. > + return CMD_RET_FAILURE; > + } > + > + if (gpt_default(blk_dev_desc, argv[4])) > + return CMD_RET_FAILURE; > } else { > return CMD_RET_USAGE; > } > -- > 1.8.1.4 Except minor comments this patch looks good to me. I tested it on mmc device (Trats2) and works well. Tested-by: Piotr Wilczek <p.wilczek@samsung.com> Best regards, Piotr Wilczek
On Fri, Oct 11, 2013 at 09:13:22AM +0200, Piotr Wilczek wrote: > Dear Egbert Eich, > > > -----Original Message----- > > From: Egbert Eich [mailto:egbert.eich@gmail.com] > > Sent: Friday, October 04, 2013 6:53 PM > > To: u-boot@lists.denx.de > > Cc: Piotr Wilczek; Tom Rini; Egbert Eich; Egbert Eich > > Subject: [Patch v3] cmd/gpt: Support gpt command for all devices > > > > From: Egbert Eich <eich@suse.com> > > > > The gpt command was only implemented for mmc devices. There is no > > reason why this command should not be generalized and be applied all > > other storage device classes. > > This change both simplifies the implementation and eliminates a build > > failure for systems that don't support mmcs. > > > > Signed-off-by: Egbert Eich <eich@suse.com> > > --- > > Changes for v2: > > - Coding style cleanup. > > Changes for v3: > > - Removed wrong '&' > > - Removed unused variable > > - Fixed argument checking > > Spotted by Piotr Wilczek <p.wilczek@samsung.com> > > > > common/cmd_gpt.c | 45 +++++++++++++++++++-------------------------- > > 1 file changed, 19 insertions(+), 26 deletions(-) > > > > diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index a46f5cc..17b1180 > > 100644 > > --- a/common/cmd_gpt.c > > +++ b/common/cmd_gpt.c [..] > > > > @@ -287,27 +279,28 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int [..] > > - /* device: 'mmc' */ > > - if (strcmp(argv[2], "mmc") == 0) { > > - /* check if 'dev' is a number */ > > - for (pstr = argv[3]; *pstr != '\0'; pstr++) > > - if (!isdigit(*pstr)) { > > - printf("'%s' is not a number\n", > > - argv[3]); > > - return CMD_RET_USAGE; > > - } > > - dev = (int)simple_strtoul(argv[3], NULL, 10); > > - /* write to mmc */ > > - if (gpt_mmc_default(dev, argv[4])) > > - return CMD_RET_FAILURE; > > + char *ep; > > + block_dev_desc_t *blk_dev_desc; > This probably should be at the beginning of the function I personally prefer to keep symbols as local as possible (ie. declare them in the block they are used in if they are just used within a single block) for the following rasons: 1. It makes the code more readable ie. the definition is closeby to the location where it is used and doesn't require scrolling to the beginning of a function and the scope of the variable is is much more obvious. 2. The compiler can optimize much better as it knows that a variable can be discarded at the end of the block also by reusing stack slots stack sapce can be used much more efficiently by the compiler. I agree that in the case at hand the second argument is not too relevant, it is more a coding style issue. If there is a coding style requirement to have those definitions at the beginning of the function I will create a new patch. [..] > > + blk_dev_desc = get_dev(argv[2], dev); > > + if (!blk_dev_desc) { > > + printf("%s: %s dev %d NOT available\n", > > + __func__, argv[2], dev); > I think it is not necessary since the mmc subsystem prints 'MMC Device not > found'. I've done a quick look over the code - of all subsystems MMC seems to be the only one which prints a message when its get_dev() method is called but no device is found. Therefore I'd prefer to leave this there. > > Except minor comments this patch looks good to me. > I tested it on mmc device (Trats2) and works well. Ok, thanks! > > Tested-by: Piotr Wilczek <p.wilczek@samsung.com> > Thanks a lot for testing! Cheers, Egbert.
> -----Original Message----- > From: Egbert Eich [mailto:egbert.eich@gmail.com] > Sent: Sunday, October 13, 2013 1:14 AM > To: Piotr Wilczek > Cc: 'Egbert Eich'; u-boot@lists.denx.de; 'Tom Rini'; 'Egbert Eich' > Subject: Re: [Patch v3] cmd/gpt: Support gpt command for all devices > > On Fri, Oct 11, 2013 at 09:13:22AM +0200, Piotr Wilczek wrote: > > Dear Egbert Eich, > > > > > -----Original Message----- > > > From: Egbert Eich [mailto:egbert.eich@gmail.com] > > > Sent: Friday, October 04, 2013 6:53 PM > > > To: u-boot@lists.denx.de > > > Cc: Piotr Wilczek; Tom Rini; Egbert Eich; Egbert Eich > > > Subject: [Patch v3] cmd/gpt: Support gpt command for all devices > > > > > > From: Egbert Eich <eich@suse.com> > > > > > > The gpt command was only implemented for mmc devices. There is no > > > reason why this command should not be generalized and be applied > all > > > other storage device classes. > > > This change both simplifies the implementation and eliminates a > > > build failure for systems that don't support mmcs. > > > > > > Signed-off-by: Egbert Eich <eich@suse.com> > > > --- > > > Changes for v2: > > > - Coding style cleanup. > > > Changes for v3: > > > - Removed wrong '&' > > > - Removed unused variable > > > - Fixed argument checking > > > Spotted by Piotr Wilczek <p.wilczek@samsung.com> > > > > > > common/cmd_gpt.c | 45 +++++++++++++++++++------------------------- > - > > > 1 file changed, 19 insertions(+), 26 deletions(-) > > > > > > diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index > > > a46f5cc..17b1180 > > > 100644 > > > --- a/common/cmd_gpt.c > > > +++ b/common/cmd_gpt.c > > [..] > > > > > > > @@ -287,27 +279,28 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, > > > int > > [..] > > > > - /* device: 'mmc' */ > > > - if (strcmp(argv[2], "mmc") == 0) { > > > - /* check if 'dev' is a number */ > > > - for (pstr = argv[3]; *pstr != '\0'; pstr++) > > > - if (!isdigit(*pstr)) { > > > - printf("'%s' is not a number\n", > > > - argv[3]); > > > - return CMD_RET_USAGE; > > > - } > > > - dev = (int)simple_strtoul(argv[3], NULL, 10); > > > - /* write to mmc */ > > > - if (gpt_mmc_default(dev, argv[4])) > > > - return CMD_RET_FAILURE; > > > + char *ep; > > > + block_dev_desc_t *blk_dev_desc; > > This probably should be at the beginning of the function > > I personally prefer to keep symbols as local as possible (ie. declare > them in the block they are used in if they are just used within a > single block) for the following rasons: > 1. It makes the code more readable ie. the definition is closeby to > the location where it is used and doesn't require scrolling to > the beginning of a function and the scope of the variable is is > much more obvious. > 2. The compiler can optimize much better as it knows that a variable > can be discarded at the end of the block also by reusing stack > slots stack sapce can be used much more efficiently by the compiler. > > I agree that in the case at hand the second argument is not too > relevant, it is more a coding style issue. If there is a coding style > requirement to have those definitions at the beginning of the function > I will create a new patch. > Agreed, I think it is a coding style rule in U-boot. Let the maintainer to decide. > > [..] > > > > + blk_dev_desc = get_dev(argv[2], dev); > > > + if (!blk_dev_desc) { > > > + printf("%s: %s dev %d NOT available\n", > > > + __func__, argv[2], dev); > > I think it is not necessary since the mmc subsystem prints 'MMC > Device > > not found'. > > I've done a quick look over the code - of all subsystems MMC seems to > be the only one which prints a message when its get_dev() method is > called but no device is found. Therefore I'd prefer to leave this > there. > Ok. > > > > Except minor comments this patch looks good to me. > > I tested it on mmc device (Trats2) and works well. > > Ok, thanks! > > > > > Tested-by: Piotr Wilczek <p.wilczek@samsung.com> > > > > Thanks a lot for testing! > > Cheers, > Egbert. Best regards, Piotr Wilczek
On Fri, Oct 04, 2013 at 06:53:04PM +0200, egbert.eich@gmail.com wrote: > From: Egbert Eich <eich@suse.com> > > The gpt command was only implemented for mmc devices. There is no reason > why this command should not be generalized and be applied all other > storage device classes. > This change both simplifies the implementation and eliminates a > build failure for systems that don't support mmcs. > > Signed-off-by: Egbert Eich <eich@suse.com> > Tested-by: Piotr Wilczek <p.wilczek@samsung.com> Applied to u-boot/master with a slight change to the coding style (yes, we want variables declared at the start of the function and __maybe_unused for ones that would be hidden via #ifdef and cause a warning in some cases), thanks!
diff --git a/common/cmd_gpt.c b/common/cmd_gpt.c index a46f5cc..17b1180 100644 --- a/common/cmd_gpt.c +++ b/common/cmd_gpt.c @@ -11,7 +11,6 @@ #include <common.h> #include <malloc.h> #include <command.h> -#include <mmc.h> #include <part_efi.h> #include <exports.h> #include <linux/ctype.h> @@ -122,7 +121,7 @@ static int set_gpt_info(block_dev_desc_t *dev_desc, int errno = 0; uint64_t size_ll, start_ll; - debug("%s: MMC lba num: 0x%x %d\n", __func__, + debug("%s: lba num: 0x%x %d\n", __func__, (unsigned int)dev_desc->lba, (unsigned int)dev_desc->lba); if (str_part == NULL) @@ -235,25 +234,18 @@ err: return errno; } -static int gpt_mmc_default(int dev, const char *str_part) +static int gpt_default(block_dev_desc_t *blk_dev_desc, const char *str_part) { int ret; char *str_disk_guid; u8 part_count = 0; disk_partition_t *partitions = NULL; - struct mmc *mmc = find_mmc_device(dev); - - if (mmc == NULL) { - printf("%s: mmc dev %d NOT available\n", __func__, dev); - return CMD_RET_FAILURE; - } - if (!str_part) return -1; /* fill partitions */ - ret = set_gpt_info(&mmc->block_dev, str_part, + ret = set_gpt_info(blk_dev_desc, str_part, &str_disk_guid, &partitions, &part_count); if (ret) { if (ret == -1) @@ -266,7 +258,7 @@ static int gpt_mmc_default(int dev, const char *str_part) } /* save partitions layout to disk */ - gpt_restore(&mmc->block_dev, str_disk_guid, partitions, part_count); + gpt_restore(blk_dev_desc, str_disk_guid, partitions, part_count); free(str_disk_guid); free(partitions); @@ -287,27 +279,28 @@ static int do_gpt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]) { int ret = CMD_RET_SUCCESS; int dev = 0; - char *pstr; if (argc < 5) return CMD_RET_USAGE; /* command: 'write' */ if ((strcmp(argv[1], "write") == 0) && (argc == 5)) { - /* device: 'mmc' */ - if (strcmp(argv[2], "mmc") == 0) { - /* check if 'dev' is a number */ - for (pstr = argv[3]; *pstr != '\0'; pstr++) - if (!isdigit(*pstr)) { - printf("'%s' is not a number\n", - argv[3]); - return CMD_RET_USAGE; - } - dev = (int)simple_strtoul(argv[3], NULL, 10); - /* write to mmc */ - if (gpt_mmc_default(dev, argv[4])) - return CMD_RET_FAILURE; + char *ep; + block_dev_desc_t *blk_dev_desc; + dev = (int)simple_strtoul(argv[3], &ep, 10); + if (!ep || ep[0] != '\0') { + printf("'%s' is not a number\n", argv[3]); + return CMD_RET_USAGE; } + blk_dev_desc = get_dev(argv[2], dev); + if (!blk_dev_desc) { + printf("%s: %s dev %d NOT available\n", + __func__, argv[2], dev); + return CMD_RET_FAILURE; + } + + if (gpt_default(blk_dev_desc, argv[4])) + return CMD_RET_FAILURE; } else { return CMD_RET_USAGE; }