[U-Boot] [PATCH 06/13] fastboot: Move fastboot response functions to fastboot core

[Maxime Ripard]

On Fri, Sep 04, 2015 at 01:20:44PM -0400, Tom Rini wrote:
> On Mon, Aug 31, 2015 at 04:46:06PM +0200, Maxime Ripard wrote:
> 
> > The functions and a few define to generate a fastboot message to be sent
> > back to the host were so far duplicated among the users.
> > 
> > Move them all to a common place.
> [snip]
> > diff --git a/common/aboot.c b/common/aboot.c
> > index 18ff30ee6d11..37ad50efc50a 100644
> > --- a/common/aboot.c
> > +++ b/common/aboot.c
> > @@ -277,7 +277,7 @@ void store_sparse_image(sparse_storage_t *storage,
> >  
> >  	sparse_header = sparse_parse_header(&data);
> >  	if (!sparse_header) {
> > -		fastboot_fail("sparse header issue\n");
> > +		printf("sparse header issue\n");
> >  		return;
> >  	}
> >  
> > @@ -288,7 +288,6 @@ void store_sparse_image(sparse_storage_t *storage,
> >  	if (sparse_header->blk_sz % storage->block_sz) {
> >  		printf("%s: Sparse image block size issue [%u]\n",
> >  		       __func__, sparse_header->blk_sz);
> > -		fastboot_fail("sparse image block size issue");
> >  		return;
> >  	}
> >  
> > @@ -299,7 +298,7 @@ void store_sparse_image(sparse_storage_t *storage,
> >  	for (chunk = 0; chunk < sparse_header->total_chunks; chunk++) {
> >  		chunk_header = sparse_parse_chunk(sparse_header, &data);
> >  		if (!chunk_header) {
> > -			fastboot_fail("Unknown chunk type");
> > +			printf("Unknown chunk type");
> >  			return;
> >  		}
> >  
> > @@ -314,7 +313,6 @@ void store_sparse_image(sparse_storage_t *storage,
> >  		if (blk + blkcnt > storage->start + storage->size) {
> >  			printf("%s: Request would exceed partition size!\n",
> >  			       __func__);
> > -			fastboot_fail("Request would exceed partition size!");
> >  			return;
> >  		}
> >  
> > @@ -331,7 +329,6 @@ void store_sparse_image(sparse_storage_t *storage,
> >  			if (buffer_blks != buffer_blk_cnt) {
> >  				printf("%s: Write %d failed %d\n",
> >  				       __func__, i, buffer_blks);
> > -				fastboot_fail("flash write failure");
> >  				return;
> >  			}
> >  
> > @@ -348,9 +345,10 @@ void store_sparse_image(sparse_storage_t *storage,
> >  	printf("........ wrote %u bytes to '%s'\n", bytes_written,
> >  	       storage->name);
> >  
> > -	if (total_blocks != sparse_header->total_blks)
> > -		fastboot_fail("sparse image write failure");
> > +	if (total_blocks != sparse_header->total_blks) {
> > +		printf("sparse image write failure");
> > +		return;
> > +	}
> >  
> > -	fastboot_okay("");
> >  	return;
> >  }
> 
> Why in the case of this image do we not need to do fastboot_fail/okay
> now?  It's not obvious from the rest of the changes to me, thanks!

I took that shortcut while refactoring and forgot to fix it...

The issue is that we don't have access to the response buffer in those
functions. We could pass it as an argument, but that would:

  - Prevent the calling function (that "owns" the buffer pointer) to
    use fastboot_fail / fastboot_okay, which feels a bit wrong.

  - On a more theorical point, the sparse image format should be
    decoupled from the fastboot protocol, hence not really rely of
    these functions.

What we can do though, is simply return an error code, and the storage
layers could use fastboot_okay / fastboot_fail themselves. Does that
sound good?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150906/3f9b2ab8/attachment.sig>