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

[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 at 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