Patchwork [U-Boot,7/9] Add a partition type target

login
register
mail settings
Submitter Pantelis Antoniou
Date Nov. 28, 2012, 12:44 p.m.
Message ID <1354106642-4587-8-git-send-email-panto@antoniou-consulting.com>
Download mbox | patch
Permalink /patch/202248/
State Superseded
Delegated to: Marek Vasut
Headers show

Comments

Marek Vasut - Nov. 28, 2012, 2:48 a.m.
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
Pantelis Antoniou - Nov. 28, 2012, 8:31 a.m.
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
Marek Vasut - Nov. 28, 2012, 9:22 a.m.
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
Pantelis Antoniou - Nov. 28, 2012, 12:44 p.m.
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(+)

Patch

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) {