[U-Boot] [PATCH v2 4/7] dfu: MMC specific routines for DFU operation

[Lukasz Majewski]

Dear Wolfgang Denk,

> Dear Lukasz Majewski,
> 
> In message <1341416922-13792-5-git-send-email-l.majewski at samsung.com>
> you wrote:
> > Support for MMC storage devices to work with DFU framework.
> 
> Sorry for jumping in late.
> 
> 
> > +	switch (dfu->layout) {
> > +	case RAW_ADDR:
> > +		sprintf(cmd_buf, "mmc write 0x%x %x %x", (unsigned
> > int) buf,
> > +			dfu->data.mmc.lba_start,
> > dfu->data.mmc.lba_size);
> > +		break;
> > +	case FAT:
> > +		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx",
> > +			dfu->data.mmc.dev, dfu->data.mmc.part,
> > +			(unsigned int) buf, dfu->name, *len);
> > +		break;
> > +	default:
> > +		printf("%s: Wrong layout!\n", __func__);
> > +	}
> 
> In case of error, you should always print what the unexpected data
> was.  The end user who receives an "Wrong layout!" error is probably
> pretty much surpised and doesn't know what he did wrong.   If you
> print instead: "EXT2 layout not supported (yet)" he would know exactly
> what to change to make it work.

Ok, this can be easily corrected.
> 
> 
> There has been some dicussion already if using this sprintf() /
> run_command() approach is good or not, or how it should be fixed.
> 
> It appears, all this discussion forgot to take into account that
> patch 3/7 dfu: DFU backend implementation promised to add "platform
> and storage independent operation of DFU."  Here we are breaking this
> promise.
>
> And by adding special functions to the FAT file system code thing sget
> just worse, as now not only the DFU code needs to be extended when we
> want to use this with any other file system type, but suddenly this
> also bleeds into all supported file system code.
> 
> 
> I am aware that the current implementation suffers from the fact that
> we don't have a unified access to file systems - each comes with it's
> own set of commands and more or less (in)compatible command line and
> function call interfaces.  This will hopefully improve in the context
> of the device model rework, but we don't want to block you until then.
> But considering that this is going to change in a foreseeable future,
> it also makes littel sense to invest big efforts into generic code
> that covers all potential future uses.
> 
> Here is my poposal:
> 
> Let's introduce a thin shim layer to abstract the file system
> interface from the accesses you need here.  As far as I can see here,
> you need exactly 4 functions:
> 
> - block_read()
> - block_write()
> - file_read()
> - file_write()
> 
> These names could be function pointers to implement the device I/O in
> a both device and file system agnostic way; you can implement
> specific versions of the functions like mmc_block_read(),
> usb_block_read(), ... etc. and use the settings of dfu_device_type
> and dfu_layout to set the pointers to these functions.
> 
> For this shim layer I would actually prefer the original approach
> (shown above) to use sprintf() to build commands based on the existing
> command interface - this is the minimal intrusive way to implement
> this for now.  With this approach, no modifications to file system
> and/or device driver code are needed, and we still maintain full
> flexibility here in the DFU code.  Yes, error checking may not be
> perfect, and we my not win a price for elegance of design either, but
> we should get mostly clean, working code quickly.
> 

So I suppose, that you are proposing something like this (pseudo code):

int mmc_block_write() {
	sprintf(cmd_buf, "mmc write 0x%x %x %x")
	run_command(cmd_buf, 0);
}

int mmc_file_write(enum fs_type) {
	switch (fs_type)
	case FAT:
		sprintf(cmd_buf, "fatwrite mmc %d:%d 0x%x %s %lx")
	break;
	case EXT4:
		sprintf(cmd_buf, "ext4write mmc %d:%d 0x%x %s %lx")
	break;
	run_command(cmd_buf);
}

int dfu_write_medium_mmc(struct dfu_entity *dfu, void *buf, long *len)
{
	ALLOC_CACHE_ALIGN_BUFFER(char, cmd_buf, DFU_CMD_BUF_SIZE);

	memset(cmd_buf, '\0', sizeof(cmd_buf));

	switch (dfu->layout) {
	case RAW_ADDR:
		mmc_block_write()
		break;
	case FAT:
	case EXT3:
	case EXT4:
		mmc_file_write(dfu->layout);
		break;
	default:
		printf("%s: Wrong layout %s!\n", __func__,
		layout_table[dfu->layout]); }	

	return 0;
}


> 
> The advantage I see for this code is that we have separated all
> this device interface stuff from both the generic DFU code and from
> the device and file system code as well.  As soon as we have a better
> implementation below, all we need to adjust (or potentially even
> remove) is this shim layer.
> 


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group