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

[Marek Vasut]

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.
> 
> 
> 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

We discussed it for a while ... figure out the missing typechecking and abuse of 
the command line interface is not a way to go.

> 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.

But now that we started going in the typechecking way, I'd prefer to go all the 
way. And once DFU extends properly towards other supported filesystems/devices, 
the rest of the interface can get cleaned along the way.

> 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.

Shim layer is cool -- abusing command line is not cool ;-)

> Best regards,
> 
> Wolfgang Denk