| Message ID | 1354106642-4587-8-git-send-email-panto@antoniou-consulting.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
Dear Pantelis Antoniou, > Dealing with raw block numbers with the dfu is very annoying. > Introduce a partition method. > > Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > --- > drivers/dfu/dfu_mmc.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c > index 5d504df..3733b21 100644 > --- a/drivers/dfu/dfu_mmc.c > +++ b/drivers/dfu/dfu_mmc.c > @@ -153,6 +153,10 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu, void > *buf, long *len) > > int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s) > { > + int dev, part; > + struct mmc *mmc; > + block_dev_desc_t *blk_dev; > + disk_partition_t partinfo; > char *st; > > dfu->dev_type = DFU_DEV_MMC; > @@ -166,8 +170,33 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char > *s) dfu->layout = DFU_FS_FAT; > } else if (!strcmp(st, "ext4")) { > dfu->layout = DFU_FS_EXT4; > + } else if (!strcmp(st, "part")) { > + > + dfu->layout = DFU_RAW_ADDR; > + > + dev = simple_strtoul(s, &s, 10); > + part = simple_strtoul(++s, &s, 10); ++s ... this is unreadable and definitelly prone to breakage. > + > + mmc = find_mmc_device(dev); > + if (mmc == NULL || mmc_init(mmc)) { > + printf("%s: could not find mmc device #%d!\n", __func__, dev); > + return -1; > + } > + > + blk_dev = &mmc->block_dev; > + if (get_partition_info(blk_dev, part, &partinfo) != 0) { > + printf("%s: could not find partition #%d on mmc device #%d!\n", > + __func__, part, dev); > + return -1; Try using regular errno.h ... fix all around. > + } > + > + dfu->data.mmc.lba_start = partinfo.start; > + dfu->data.mmc.lba_size = partinfo.size; > + dfu->data.mmc.lba_blk_size = partinfo.blksz; > + > } else { > printf("%s: Memory layout (%s) not supported!\n", __func__, st); > + return -1; This is new ... does it fit into this patch at all? > } > > if (dfu->layout == DFU_FS_EXT4 || dfu->layout == DFU_FS_FAT) { Best regards, Marek Vasut
Hi Marek, On Nov 28, 2012, at 4:48 AM, Marek Vasut wrote: > Dear Pantelis Antoniou, > >> Dealing with raw block numbers with the dfu is very annoying. >> Introduce a partition method. >> >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> >> --- >> drivers/dfu/dfu_mmc.c | 29 +++++++++++++++++++++++++++++ >> 1 file changed, 29 insertions(+) >> >> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c >> index 5d504df..3733b21 100644 >> --- a/drivers/dfu/dfu_mmc.c >> +++ b/drivers/dfu/dfu_mmc.c >> @@ -153,6 +153,10 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu, void >> *buf, long *len) >> >> int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s) >> { >> + int dev, part; >> + struct mmc *mmc; >> + block_dev_desc_t *blk_dev; >> + disk_partition_t partinfo; >> char *st; >> >> dfu->dev_type = DFU_DEV_MMC; >> @@ -166,8 +170,33 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char >> *s) dfu->layout = DFU_FS_FAT; >> } else if (!strcmp(st, "ext4")) { >> dfu->layout = DFU_FS_EXT4; >> + } else if (!strcmp(st, "part")) { >> + >> + dfu->layout = DFU_RAW_ADDR; >> + >> + dev = simple_strtoul(s, &s, 10); >> + part = simple_strtoul(++s, &s, 10); > > ++s ... this is unreadable and definitelly prone to breakage. > Just following what the existing code does. Look around this line. Perhaps we should modify that too. >> + >> + mmc = find_mmc_device(dev); >> + if (mmc == NULL || mmc_init(mmc)) { >> + printf("%s: could not find mmc device #%d!\n", __func__, > dev); >> + return -1; >> + } >> + >> + blk_dev = &mmc->block_dev; >> + if (get_partition_info(blk_dev, part, &partinfo) != 0) { >> + printf("%s: could not find partition #%d on mmc device > #%d!\n", >> + __func__, part, dev); >> + return -1; > > Try using regular errno.h ... fix all around. > The whole file doesn't use errno.h at all. Changing this single thing will stick out like a sore thumb. >> + } >> + >> + dfu->data.mmc.lba_start = partinfo.start; >> + dfu->data.mmc.lba_size = partinfo.size; >> + dfu->data.mmc.lba_blk_size = partinfo.blksz; >> + >> } else { >> printf("%s: Memory layout (%s) not supported!\n", __func__, st); >> + return -1; > > This is new ... does it fit into this patch at all? > It should, it's an error condition and the code just blindly continued. >> } >> >> if (dfu->layout == DFU_FS_EXT4 || dfu->layout == DFU_FS_FAT) { > > Best regards, > Marek Vasut Regards -- Pantelis
Dear Pantelis Antoniou, > Hi Marek, > > On Nov 28, 2012, at 4:48 AM, Marek Vasut wrote: > > Dear Pantelis Antoniou, > > > >> Dealing with raw block numbers with the dfu is very annoying. > >> Introduce a partition method. > >> > >> Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> > >> --- > >> drivers/dfu/dfu_mmc.c | 29 +++++++++++++++++++++++++++++ > >> 1 file changed, 29 insertions(+) > >> > >> diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c > >> index 5d504df..3733b21 100644 > >> --- a/drivers/dfu/dfu_mmc.c > >> +++ b/drivers/dfu/dfu_mmc.c > >> @@ -153,6 +153,10 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu, > >> void *buf, long *len) > >> > >> int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s) > >> { > >> + int dev, part; > >> + struct mmc *mmc; > >> + block_dev_desc_t *blk_dev; > >> + disk_partition_t partinfo; > >> > >> char *st; > >> > >> dfu->dev_type = DFU_DEV_MMC; > >> > >> @@ -166,8 +170,33 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, > >> char *s) dfu->layout = DFU_FS_FAT; > >> > >> } else if (!strcmp(st, "ext4")) { > >> > >> dfu->layout = DFU_FS_EXT4; > >> > >> + } else if (!strcmp(st, "part")) { > >> + > >> + dfu->layout = DFU_RAW_ADDR; > >> + > >> + dev = simple_strtoul(s, &s, 10); > >> + part = simple_strtoul(++s, &s, 10); > > > > ++s ... this is unreadable and definitelly prone to breakage. > > Just following what the existing code does. Look around this line. > Perhaps we should modify that too. Yes, that'd be really nice ... but in a separate patch please. > >> + > >> + mmc = find_mmc_device(dev); > >> + if (mmc == NULL || mmc_init(mmc)) { > >> + printf("%s: could not find mmc device #%d!\n", __func__, > > > > dev); > > > >> + return -1; > >> + } > >> + > >> + blk_dev = &mmc->block_dev; > >> + if (get_partition_info(blk_dev, part, &partinfo) != 0) { > >> + printf("%s: could not find partition #%d on mmc device > > > > #%d!\n", > > > >> + __func__, part, dev); > >> + return -1; > > > > Try using regular errno.h ... fix all around. > > The whole file doesn't use errno.h at all. Changing this single thing will > stick out like a sore thumb. :-( > >> + } > >> + > >> + dfu->data.mmc.lba_start = partinfo.start; > >> + dfu->data.mmc.lba_size = partinfo.size; > >> + dfu->data.mmc.lba_blk_size = partinfo.blksz; > >> + > >> > >> } else { > >> > >> printf("%s: Memory layout (%s) not supported!\n", __func__, st); > >> > >> + return -1; > > > > This is new ... does it fit into this patch at all? > > It should, it's an error condition and the code just blindly continued. Even more :-(( > >> } > >> > >> if (dfu->layout == DFU_FS_EXT4 || dfu->layout == DFU_FS_FAT) { > > > > Best regards, > > Marek Vasut > > Regards > > -- Pantelis
diff --git a/drivers/dfu/dfu_mmc.c b/drivers/dfu/dfu_mmc.c index 5d504df..3733b21 100644 --- a/drivers/dfu/dfu_mmc.c +++ b/drivers/dfu/dfu_mmc.c @@ -153,6 +153,10 @@ int dfu_read_medium_mmc(struct dfu_entity *dfu, void *buf, long *len) int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s) { + int dev, part; + struct mmc *mmc; + block_dev_desc_t *blk_dev; + disk_partition_t partinfo; char *st; dfu->dev_type = DFU_DEV_MMC; @@ -166,8 +170,33 @@ int dfu_fill_entity_mmc(struct dfu_entity *dfu, char *s) dfu->layout = DFU_FS_FAT; } else if (!strcmp(st, "ext4")) { dfu->layout = DFU_FS_EXT4; + } else if (!strcmp(st, "part")) { + + dfu->layout = DFU_RAW_ADDR; + + dev = simple_strtoul(s, &s, 10); + part = simple_strtoul(++s, &s, 10); + + mmc = find_mmc_device(dev); + if (mmc == NULL || mmc_init(mmc)) { + printf("%s: could not find mmc device #%d!\n", __func__, dev); + return -1; + } + + blk_dev = &mmc->block_dev; + if (get_partition_info(blk_dev, part, &partinfo) != 0) { + printf("%s: could not find partition #%d on mmc device #%d!\n", + __func__, part, dev); + return -1; + } + + dfu->data.mmc.lba_start = partinfo.start; + dfu->data.mmc.lba_size = partinfo.size; + dfu->data.mmc.lba_blk_size = partinfo.blksz; + } else { printf("%s: Memory layout (%s) not supported!\n", __func__, st); + return -1; } if (dfu->layout == DFU_FS_EXT4 || dfu->layout == DFU_FS_FAT) {
Dealing with raw block numbers with the dfu is very annoying. Introduce a partition method. Signed-off-by: Pantelis Antoniou <panto@antoniou-consulting.com> --- drivers/dfu/dfu_mmc.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+)