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

Support for MMC storage devices to work with DFU framework.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Marek Vasut <marex@denx.de>

---
Changes for v2:
- None
---
 drivers/dfu/Makefile  |    1 +
 drivers/dfu/dfu_mmc.c |  126 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 127 insertions(+), 0 deletions(-)
 create mode 100644 drivers/dfu/dfu_mmc.c

Dear Lukasz Majewski,

> Support for MMC storage devices to work with DFU framework.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
> 

This one opens some questions.

[...]

> +int dfu_fill_entity_mmc(struct dfu_entity *dfu, char* s)
> +{
> +	char *st = NULL;
> +	int n = 0;
> +
> +	dfu->dev_type = MMC;
> +	st = dfu_extract_token(&s, &n);
> +
> +	if (!strncmp(st, "mmc", n)) {
> +		dfu->layout = RAW_ADDR;
> +
> +		dfu->data.mmc.lba_start = simple_strtoul(s, &s, 16);
> +		dfu->data.mmc.lba_size = simple_strtoul(++s, &s, 16);
> +		dfu->data.mmc.lba_blk_size = get_mmc_blk_size(dfu->dev_num);
> +

^ one line too much.

> +	} else if (!strncmp(st, "fat", n)) {
> +		dfu->layout = FAT;
> +
> +		dfu->data.mmc.dev = simple_strtoul(s, &s, 10);
> +		dfu->data.mmc.part = simple_strtoul(++s, &s, 10);
> +
> +	} else {
> +		printf("%s: Wrong memory layout!\n", __func__);
> +	}
> +
> +	dfu->read_medium = dfu_read_medium_mmc;
> +	dfu->write_medium = dfu_write_medium_mmc;
> +
> +	return 0;
> +}

Best regards,
Marek Vasut

On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> Support for MMC storage devices to work with DFU framework.
> 
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Marek Vasut <marex@denx.de>
[snip]
> +	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__);
> +	}
> +
> +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> +	run_command(cmd_buf, 0);

If we try and take the long-view here, that fatwrite/mmc write don't
perform a lot of sanity checking on input isn't good.  Lots of commands
I believe don't, but we can start somewhere.  So, lets do what Marek was
suggesting of making common/cmd_mmc.c and common/cmd_fat.c call a
sub-function that takes compile-time typecheckable inputs, and call that
here.  That opens things up for later making the user commands perform
better checking and so forth.

Hi Tom,

> On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> > Support for MMC storage devices to work with DFU framework.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Marek Vasut <marex@denx.de>
> [snip]
> > +	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__);
> > +	}
> > +
> > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > +	run_command(cmd_buf, 0);
> 
> If we try and take the long-view here, that fatwrite/mmc write don't
> perform a lot of sanity checking on input isn't good.  Lots of
> commands I believe don't, but we can start somewhere. 
Yes, indeed they don't. But I think, that it is a deeper problem.

When one looks into the cmd_mmc.c, the code is not checking the
correctness of passed data. It performs strncmp, then simple_strtoul
and with this parameter calls mmc->block_dev.block_read(). 

But I'm a bit concern if adding function:

do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) to

do_mmcops(argc, argv) {
	int i = simple_strtol(argv[]);
	return do_mmcops_check(i);
}

will help with preventing errors.

As I've written previously, data from prompt is passed in a form of
text, which is converted to the well defined type anyway (with e.g.
simple_strtoul - returns unsigned long, strict_strtoul - returns int,
simple_strtol - returns long, simple_strtoull - returns unsigned long
long). Using those functions correctly would ensure correct types
passed to e.g. mmc->block_dev.block_read().

When one create the do_mmcops_check() function, the compiler would
check if its arguments are correct (i.e. the i in the above example is
int). What is the difference between checking at compile time the
output of simple_strtoul? 

The real problem in my opinion is the lack of checking if arguments
passed as text to the do_mmcops are correct or not. This is not done
for MMC and I doubt, if adding compile time type checking (in a form of
a separate function) would solve/alleviate the problem.



> So, lets do
> what Marek was suggesting of making common/cmd_mmc.c and
> common/cmd_fat.c call a sub-function that takes compile-time
> typecheckable inputs, and call that here.  That opens things up for
> later making the user commands perform better checking and so forth.

I'd like to point to the problem with passing and then parsing
parameters as text. 

User typed parameters aren't checked.

Please correct me if I misunderstood the problem or the proposed
solution.

On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote:
> Hi Tom,
> 
> > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> > > Support for MMC storage devices to work with DFU framework.
> > > 
> > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > Cc: Marek Vasut <marex@denx.de>
> > [snip]
> > > +	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__);
> > > +	}
> > > +
> > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > +	run_command(cmd_buf, 0);
> > 
> > If we try and take the long-view here, that fatwrite/mmc write don't
> > perform a lot of sanity checking on input isn't good.  Lots of
> > commands I believe don't, but we can start somewhere. 
> Yes, indeed they don't. But I think, that it is a deeper problem.
> 
> When one looks into the cmd_mmc.c, the code is not checking the
> correctness of passed data. It performs strncmp, then simple_strtoul
> and with this parameter calls mmc->block_dev.block_read(). 
> 
> But I'm a bit concern if adding function:
> 
> do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...) to
> 
> do_mmcops(argc, argv) {
> 	int i = simple_strtol(argv[]);
> 	return do_mmcops_check(i);
> }

Well, what I was suggesting would be:

do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. }
do_mmcops_from_cmd(argc, argv) {
 ... convert user input today, maybe try and sanity check input tomorrow
 ..
}

And then dfu calls do_mmcops_real(lba_start, ...).  A further clean-up
would be to make the interface the command uses to perform checking of
the arguments passed.  Does this make sense?

On Wed, 11 Jul 2012 04:54:31 -0700
Tom Rini <trini@ti.com> wrote:

> On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote:
> > Hi Tom,
> > 
> > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> > > > Support for MMC storage devices to work with DFU framework.
> > > > 
> > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > > Cc: Marek Vasut <marex@denx.de>
> > > [snip]
> > > > +	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__);
> > > > +	}
> > > > +
> > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > +	run_command(cmd_buf, 0);
> > > 
> > > If we try and take the long-view here, that fatwrite/mmc write
> > > don't perform a lot of sanity checking on input isn't good.  Lots
> > > of commands I believe don't, but we can start somewhere. 
> > Yes, indeed they don't. But I think, that it is a deeper problem.
> > 
> > When one looks into the cmd_mmc.c, the code is not checking the
> > correctness of passed data. It performs strncmp, then simple_strtoul
> > and with this parameter calls mmc->block_dev.block_read(). 
> > 
> > But I'm a bit concern if adding function:
> > 
> > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...)
> > to
> > 
> > do_mmcops(argc, argv) {
> > 	int i = simple_strtol(argv[]);
> > 	return do_mmcops_check(i);
> > }
> 
> Well, what I was suggesting would be:
> 
> do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. }
> do_mmcops_from_cmd(argc, argv) {
>  ... convert user input today, maybe try and sanity check input
> tomorrow ..
> }
> 
> And then dfu calls do_mmcops_real(lba_start, ...).  A further clean-up
> would be to make the interface the command uses to perform checking of
> the arguments passed.  Does this make sense?
> 

Generally it is in my opinion a good way to go.

However, why we aren't first writing sanity checks for passed arguments?

We are adding one more level of abstraction, but don't think of the main
problem (checking values of passed arguments)?

Anyway we shall wait for Marek's opinion.

On Thu, Jul 12, 2012 at 02:39:27PM +0200, Lukasz Majewski wrote:
> On Wed, 11 Jul 2012 04:54:31 -0700
> Tom Rini <trini@ti.com> wrote:
> 
> > On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote:
> > > Hi Tom,
> > > 
> > > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> > > > > Support for MMC storage devices to work with DFU framework.
> > > > > 
> > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > > > Cc: Marek Vasut <marex@denx.de>
> > > > [snip]
> > > > > +	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__);
> > > > > +	}
> > > > > +
> > > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > > +	run_command(cmd_buf, 0);
> > > > 
> > > > If we try and take the long-view here, that fatwrite/mmc write
> > > > don't perform a lot of sanity checking on input isn't good.  Lots
> > > > of commands I believe don't, but we can start somewhere. 
> > > Yes, indeed they don't. But I think, that it is a deeper problem.
> > > 
> > > When one looks into the cmd_mmc.c, the code is not checking the
> > > correctness of passed data. It performs strncmp, then simple_strtoul
> > > and with this parameter calls mmc->block_dev.block_read(). 
> > > 
> > > But I'm a bit concern if adding function:
> > > 
> > > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...)
> > > to
> > > 
> > > do_mmcops(argc, argv) {
> > > 	int i = simple_strtol(argv[]);
> > > 	return do_mmcops_check(i);
> > > }
> > 
> > Well, what I was suggesting would be:
> > 
> > do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. }
> > do_mmcops_from_cmd(argc, argv) {
> >  ... convert user input today, maybe try and sanity check input
> > tomorrow ..
> > }
> > 
> > And then dfu calls do_mmcops_real(lba_start, ...).  A further clean-up
> > would be to make the interface the command uses to perform checking of
> > the arguments passed.  Does this make sense?
> > 
> 
> Generally it is in my opinion a good way to go.
> 
> However, why we aren't first writing sanity checks for passed arguments?

Simply because I didn't want to ask you to do a lot more unrelated work
:)  If you want to split and check the mmc (and fatwrite) argueuments
and then make the DFU series depend on that, by all means please do so!

> We are adding one more level of abstraction, but don't think of the main
> problem (checking values of passed arguments)?
> 
> Anyway we shall wait for Marek's opinion.

Yes, a good idea as well.

Dear Tom Rini,

> On Thu, Jul 12, 2012 at 02:39:27PM +0200, Lukasz Majewski wrote:
> > On Wed, 11 Jul 2012 04:54:31 -0700
> > 
> > Tom Rini <trini@ti.com> wrote:
> > > On Tue, Jul 10, 2012 at 12:38:54PM +0200, Lukasz Majewski wrote:
> > > > Hi Tom,
> > > > 
> > > > > On Wed, Jul 04, 2012 at 05:48:39PM +0200, Lukasz Majewski wrote:
> > > > > > Support for MMC storage devices to work with DFU framework.
> > > > > > 
> > > > > > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > > > > > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > > > > > Cc: Marek Vasut <marex@denx.de>
> > > > > 
> > > > > [snip]
> > > > > 
> > > > > > +	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__);
> > > > > > +	}
> > > > > > +
> > > > > > +	debug("%s: %s 0x%p\n", __func__, cmd_buf, cmd_buf);
> > > > > > +	run_command(cmd_buf, 0);
> > > > > 
> > > > > If we try and take the long-view here, that fatwrite/mmc write
> > > > > don't perform a lot of sanity checking on input isn't good.  Lots
> > > > > of commands I believe don't, but we can start somewhere.
> > > > 
> > > > Yes, indeed they don't. But I think, that it is a deeper problem.
> > > > 
> > > > When one looks into the cmd_mmc.c, the code is not checking the
> > > > correctness of passed data. It performs strncmp, then simple_strtoul
> > > > and with this parameter calls mmc->block_dev.block_read().
> > > > 
> > > > But I'm a bit concern if adding function:
> > > > 
> > > > do_mmcops_check(unsigned int lba_start, unsigned int lba_end, ...)
> > > > to
> > > > 
> > > > do_mmcops(argc, argv) {
> > > > 
> > > > 	int i = simple_strtol(argv[]);
> > > > 	return do_mmcops_check(i);
> > > > 
> > > > }
> > > 
> > > Well, what I was suggesting would be:
> > > 
> > > do_mmcops_real(uint lba_start, ...) { .. most of do_mmcops today .. }
> > > do_mmcops_from_cmd(argc, argv) {
> > > 
> > >  ... convert user input today, maybe try and sanity check input
> > > 
> > > tomorrow ..
> > > }
> > > 
> > > And then dfu calls do_mmcops_real(lba_start, ...).  A further clean-up
> > > would be to make the interface the command uses to perform checking of
> > > the arguments passed.  Does this make sense?
> > 
> > Generally it is in my opinion a good way to go.
> > 
> > However, why we aren't first writing sanity checks for passed arguments?
> 
> Simply because I didn't want to ask you to do a lot more unrelated work
> 
> :)  If you want to split and check the mmc (and fatwrite) argueuments
> 
> and then make the DFU series depend on that, by all means please do so!

Would be cool indeed.

> > We are adding one more level of abstraction, but don't think of the main
> > problem (checking values of passed arguments)?
> > 
> > Anyway we shall wait for Marek's opinion.
> 
> Yes, a good idea as well.

My opinion is that if you'll do the sanity checks, that'd be good. We're right 
before .07 release anyway, so the patches will hit the next merge window. Are 
you up for doing a lot of unrelated work to make this proper?

Best regards,
Marek Vasut

>> > Generally it is in my opinion a good way to go.
>> >
>> > However, why we aren't first writing sanity checks for passed arguments?
>>
>> Simply because I didn't want to ask you to do a lot more unrelated work
>>
>> :)  If you want to split and check the mmc (and fatwrite) argueuments
>>
>> and then make the DFU series depend on that, by all means please do so!
>
> Would be cool indeed.
>
>> > We are adding one more level of abstraction, but don't think of the main
>> > problem (checking values of passed arguments)?
>> >
>> > Anyway we shall wait for Marek's opinion.
>>
>> Yes, a good idea as well.
>
> My opinion is that if you'll do the sanity checks, that'd be good. We're right
> before .07 release anyway, so the patches will hit the next merge window. Are
> you up for doing a lot of unrelated work to make this proper?


I agree with the general sense that adding sanity checking would be
good. Just because I was too lazy to add them, doesn't mean I was
right. ;)

Andy

Dear Lukasz Majewski,

In message <1341416922-13792-5-git-send-email-l.majewski@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 - 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.


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,

Wolfgang Denk

Dear Wolfgang Denk,

> Dear Lukasz Majewski,
> 
> In message <1341416922-13792-5-git-send-email-l.majewski@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

Dear Marek Vasut,

In message <201207271443.45189.marex@denx.de> you wrote:
>
> > 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.

I agree that this is not the preferred way for a permanent solution,
but for now I cxonsider it acceptable, as it allows to easily deal
with the inconsistent API of the misc file system handlers.  For
example instead of

	sprintf(cmd_buf, "fatread mmc %d:%d 0x%x %s %lx", ...

we can even do something like

	sprintf(cmd_buf, "%s %s  %d:%d 0x%x %s %lx", 
		command, device, ...

and set command as needed to "fatread" or "ext2load" or whatever, and
device can be set to "mmc" or "usb" or ...

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

You can implement the type checking code when you hav ea stable, well
define API for storage devices and file systems.  For now we don't
have that, and it makes ZERO sense to add special code just for that
just to throw it away in a few weeks.

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

Whether this is abuse or clever use remains to be defined, and this
is probably largely a matter of taste.  In any case, this will be a
small piece of code that is of clearly transient nature, to be
replaced or removed as soon as we can do better.

Best regards,

Wolfgang Denk

Dear Wolfgang Denk,

> Dear Marek Vasut,
> 
> In message <201207271443.45189.marex@denx.de> you wrote:
> > > 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.
> 
> I agree that this is not the preferred way for a permanent solution,
> but for now I cxonsider it acceptable, as it allows to easily deal
> with the inconsistent API of the misc file system handlers.  For
> example instead of
> 
> 	sprintf(cmd_buf, "fatread mmc %d:%d 0x%x %s %lx", ...
> 
> we can even do something like
> 
> 	sprintf(cmd_buf, "%s %s  %d:%d 0x%x %s %lx",
> 		command, device, ...
> 
> and set command as needed to "fatread" or "ext2load" or whatever, and
> device can be set to "mmc" or "usb" or ...

Given that they have slightly different syntax, this is crazy too.

> > > 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.
> 
> You can implement the type checking code when you hav ea stable, well
> define API for storage devices and file systems.  For now we don't
> have that, and it makes ZERO sense to add special code just for that
> just to throw it away in a few weeks.

Ad API -- adding Pavel to CC. Hope his GMail works better than mine (mine 
doesn't, crashed again, damned google stuff). Pavel, can you provide us with how 
the API will look maybe?

> > Shim layer is cool -- abusing command line is not cool ;-)
> 
> Whether this is abuse or clever use remains to be defined, and this
> is probably largely a matter of taste.  In any case, this will be a
> small piece of code that is of clearly transient nature, to be
> replaced or removed as soon as we can do better.
> 
> Best regards,
> 
> Wolfgang Denk

Well ... after this discussion, I feel like I'll just go dig me a grave again.

Best regards,
Marek Vasut

Dear Wolfgang Denk,

> Dear Lukasz Majewski,
> 
> In message <1341416922-13792-5-git-send-email-l.majewski@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.
>

Dear Marek Vasut,

In message <201207271515.36085.marex@denx.de> you wrote:
> 
> > 	sprintf(cmd_buf, "%s %s  %d:%d 0x%x %s %lx",
> > 		command, device, ...
> > 
> > and set command as needed to "fatread" or "ext2load" or whatever, and
> > device can be set to "mmc" or "usb" or ...
> 
> Given that they have slightly different syntax, this is crazy too.

Draw the line between clever and crazy, genius and idiot :-)


> Well ... after this discussion, I feel like I'll just go dig me a grave again.

Why?  I think the discussion has been beneficial - we isolated a poor
piece of code, discussed how it should be improved, and now ry to find
an intermediate solution that adds not too much work for Lukasz while
still keeing the existing code clean and allowing for easy adaption to
the new DM interfaces - three worthwile goals at once.

Best regards,

Wolfgang Denk

Dear Lukasz,

In message <20120727153345.008fde41@amdc308.digital.local> you wrote:
> 
> So I suppose, that you are proposing something like this (pseudo code):

Yes, very close.

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

I suggest just use 

	int dfu_write_medium(struct dfu_entity *dfu, void *buf, long *len)

here; you can then so something like

	switch (dfu->dev_type) {
	case MMC:	block_write = mmc_block_write;
			file_write  = mmc_file_write;
			break;
#ifdef NAND_SUPPORT_AVAILABLE
	case NAND:	block_write = nand_block_write;
			file_write  = nand_block_write;
			break;
#endif
	...

and use block_write() resp. file_write() in the rest.  So the code is
really trivial to extend for other storage devices and file systems.

I feel we are very close, thanks!

Let's see if Pavel adds some comments about the best API to chose to
be as compatible with possible with the upcoming block device layer,
and then we can go for it.


Best regards,

Wolfgang Denk