[v3,3/4] cleanup code which handles the Android sparse image format

Message ID 1409763954-5494-4-git-send-email-srae@broadcom.com
State Accepted
Commit 1c39d856db8b3e32fe3d5b820fbfc8ba57ab8dd7
Headers show

Commit Message

Steve Rae Sept. 3, 2014, 5:05 p.m.
- port dprintf() to debug()
- update formatting

Signed-off-by: Steve Rae <srae@broadcom.com>
---

Changes in v3:
- use original license text

Changes in v2:
- use BSD-3-Clause

 common/aboot.c | 97 +++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 56 insertions(+), 41 deletions(-)

Comments

Wolfgang Denk Sept. 4, 2014, 5:28 a.m. | #1
Dear Steve Rae,

In message <1409763954-5494-4-git-send-email-srae@broadcom.com> you wrote:
> - port dprintf() to debug()
> - update formatting
> 
> Signed-off-by: Steve Rae <srae@broadcom.com>
> ---
> 
> Changes in v3:
> - use original license text
> 
> Changes in v2:
> - use BSD-3-Clause
> 
>  common/aboot.c | 97 +++++++++++++++++++++++++++++++++-------------------------
>  1 file changed, 56 insertions(+), 41 deletions(-)
> 
> diff --git a/common/aboot.c b/common/aboot.c
> index a302c92..3611feb 100644
> --- a/common/aboot.c
> +++ b/common/aboot.c
> @@ -28,6 +28,9 @@
>   * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
>   * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>   *
> + * NOTE:
> + *   Although it is very similar, this license text is not identical
> + *   to the "BSD-3-Clause", therefore, DO NOT MODIFY THIS LICENSE TEXT!
>   */

I understand your intention of starting with the pristine file, and
then adaptng it to U-Boot, but I don't like adding a broken file in
patch 1/4 only to fix it later in patch 3/4. I think it would be
better to squash these patches.

Second, as already mentioned, we need to assign a SPDX ID for this.

Did you check with SPDX if there a matching ID?

Best regards,

Wolfgang Denk
Tom Rini Sept. 4, 2014, 3 p.m. | #2
On Thu, Sep 04, 2014 at 07:28:04AM +0200, Wolfgang Denk wrote:
> Dear Steve Rae,
> 
> In message <1409763954-5494-4-git-send-email-srae@broadcom.com> you wrote:
> > - port dprintf() to debug()
> > - update formatting
> > 
> > Signed-off-by: Steve Rae <srae@broadcom.com>
> > ---
> > 
> > Changes in v3:
> > - use original license text
> > 
> > Changes in v2:
> > - use BSD-3-Clause
> > 
> >  common/aboot.c | 97 +++++++++++++++++++++++++++++++++-------------------------
> >  1 file changed, 56 insertions(+), 41 deletions(-)
> > 
> > diff --git a/common/aboot.c b/common/aboot.c
> > index a302c92..3611feb 100644
> > --- a/common/aboot.c
> > +++ b/common/aboot.c
> > @@ -28,6 +28,9 @@
> >   * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> >   * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> >   *
> > + * NOTE:
> > + *   Although it is very similar, this license text is not identical
> > + *   to the "BSD-3-Clause", therefore, DO NOT MODIFY THIS LICENSE TEXT!
> >   */
> 
> I understand your intention of starting with the pristine file, and
> then adaptng it to U-Boot, but I don't like adding a broken file in
> patch 1/4 only to fix it later in patch 3/4. I think it would be
> better to squash these patches.

But it would make tracking things a bit harder.  We could squash 2/4 and
3/4 into one easy enough tho.

> Second, as already mentioned, we need to assign a SPDX ID for this.
> 
> Did you check with SPDX if there a matching ID?

So, we've gone round-and-round on this, and Steve is doing what I asked
him to here.  In sum, this is _not_ BSD-3, it's a one-off from it with
some interesting wording changes that mean we can't just call it BSD-3.
Since there's nothing else going to use this (and frankly I'm mildly
puzzled by how hard it is to dig up an aboot.c with sparse image support
that doesn't have this change but also does come from a google domain) I
didn't want to add a new license file for this non-standard license.
Lukasz Majewski Sept. 8, 2014, 10:49 a.m. | #3
Hi Tom,

> On Thu, Sep 04, 2014 at 07:28:04AM +0200, Wolfgang Denk wrote:
> > Dear Steve Rae,
> > 
> > In message <1409763954-5494-4-git-send-email-srae@broadcom.com> you
> > wrote:
> > > - port dprintf() to debug()
> > > - update formatting
> > > 
> > > Signed-off-by: Steve Rae <srae@broadcom.com>
> > > ---
> > > 
> > > Changes in v3:
> > > - use original license text
> > > 
> > > Changes in v2:
> > > - use BSD-3-Clause
> > > 
> > >  common/aboot.c | 97
> > > +++++++++++++++++++++++++++++++++------------------------- 1 file
> > > changed, 56 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/common/aboot.c b/common/aboot.c
> > > index a302c92..3611feb 100644
> > > --- a/common/aboot.c
> > > +++ b/common/aboot.c
> > > @@ -28,6 +28,9 @@
> > >   * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> > > SOFTWARE, EVEN IF
> > >   * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > >   *
> > > + * NOTE:
> > > + *   Although it is very similar, this license text is not
> > > identical
> > > + *   to the "BSD-3-Clause", therefore, DO NOT MODIFY THIS
> > > LICENSE TEXT! */
> > 
> > I understand your intention of starting with the pristine file, and
> > then adaptng it to U-Boot, but I don't like adding a broken file in
> > patch 1/4 only to fix it later in patch 3/4. I think it would be
> > better to squash these patches.
> 
> But it would make tracking things a bit harder.  We could squash 2/4
> and 3/4 into one easy enough tho.
> 
> > Second, as already mentioned, we need to assign a SPDX ID for this.
> > 
> > Did you check with SPDX if there a matching ID?
> 
> So, we've gone round-and-round on this, and Steve is doing what I
> asked him to here.  In sum, this is _not_ BSD-3, it's a one-off from
> it with some interesting wording changes that mean we can't just call
> it BSD-3. Since there's nothing else going to use this (and frankly
> I'm mildly puzzled by how hard it is to dig up an aboot.c with sparse
> image support that doesn't have this change but also does come from a
> google domain) I didn't want to add a new license file for this
> non-standard license.
> 

Do we have the agreement here and therefore there aren't any obstacles
to include those patches to u-boot?
Tom Rini Sept. 8, 2014, 11:10 a.m. | #4
On Mon, Sep 08, 2014 at 12:49:56PM +0200, Lukasz Majewski wrote:
> Hi Tom,
> 
> > On Thu, Sep 04, 2014 at 07:28:04AM +0200, Wolfgang Denk wrote:
> > > Dear Steve Rae,
> > > 
> > > In message <1409763954-5494-4-git-send-email-srae@broadcom.com> you
> > > wrote:
> > > > - port dprintf() to debug()
> > > > - update formatting
> > > > 
> > > > Signed-off-by: Steve Rae <srae@broadcom.com>
> > > > ---
> > > > 
> > > > Changes in v3:
> > > > - use original license text
> > > > 
> > > > Changes in v2:
> > > > - use BSD-3-Clause
> > > > 
> > > >  common/aboot.c | 97
> > > > +++++++++++++++++++++++++++++++++------------------------- 1 file
> > > > changed, 56 insertions(+), 41 deletions(-)
> > > > 
> > > > diff --git a/common/aboot.c b/common/aboot.c
> > > > index a302c92..3611feb 100644
> > > > --- a/common/aboot.c
> > > > +++ b/common/aboot.c
> > > > @@ -28,6 +28,9 @@
> > > >   * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS
> > > > SOFTWARE, EVEN IF
> > > >   * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> > > >   *
> > > > + * NOTE:
> > > > + *   Although it is very similar, this license text is not
> > > > identical
> > > > + *   to the "BSD-3-Clause", therefore, DO NOT MODIFY THIS
> > > > LICENSE TEXT! */
> > > 
> > > I understand your intention of starting with the pristine file, and
> > > then adaptng it to U-Boot, but I don't like adding a broken file in
> > > patch 1/4 only to fix it later in patch 3/4. I think it would be
> > > better to squash these patches.
> > 
> > But it would make tracking things a bit harder.  We could squash 2/4
> > and 3/4 into one easy enough tho.
> > 
> > > Second, as already mentioned, we need to assign a SPDX ID for this.
> > > 
> > > Did you check with SPDX if there a matching ID?
> > 
> > So, we've gone round-and-round on this, and Steve is doing what I
> > asked him to here.  In sum, this is _not_ BSD-3, it's a one-off from
> > it with some interesting wording changes that mean we can't just call
> > it BSD-3. Since there's nothing else going to use this (and frankly
> > I'm mildly puzzled by how hard it is to dig up an aboot.c with sparse
> > image support that doesn't have this change but also does come from a
> > google domain) I didn't want to add a new license file for this
> > non-standard license.
> > 
> 
> Do we have the agreement here and therefore there aren't any obstacles
> to include those patches to u-boot?

Yes.  I plan on grabbing them shortly.
Tom Rini Sept. 17, 2014, 12:45 a.m. | #5
On Wed, Sep 03, 2014 at 10:05:53AM -0700, Steve Rae wrote:

> - port dprintf() to debug()
> - update formatting
> 
> Signed-off-by: Steve Rae <srae@broadcom.com>

Applied to u-boot/master, thanks!

Patch

diff --git a/common/aboot.c b/common/aboot.c
index a302c92..3611feb 100644
--- a/common/aboot.c
+++ b/common/aboot.c
@@ -28,6 +28,9 @@ 
  * OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
  * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  *
+ * NOTE:
+ *   Although it is very similar, this license text is not identical
+ *   to the "BSD-3-Clause", therefore, DO NOT MODIFY THIS LICENSE TEXT!
  */
 
 void cmd_flash_mmc_sparse_img(const char *arg, void *data, unsigned sz)
@@ -70,23 +73,24 @@  void cmd_flash_mmc_sparse_img(const char *arg, void *data, unsigned sz)
 	}
 
 	data += sparse_header->file_hdr_sz;
-	if(sparse_header->file_hdr_sz > sizeof(sparse_header_t))
+	if (sparse_header->file_hdr_sz > sizeof(sparse_header_t))
 	{
-		/* Skip the remaining bytes in a header that is longer than
+		/*
+		 * Skip the remaining bytes in a header that is longer than
 		 * we expected.
 		 */
 		data += (sparse_header->file_hdr_sz - sizeof(sparse_header_t));
 	}
 
-	dprintf (SPEW, "=== Sparse Image Header ===\n");
-	dprintf (SPEW, "magic: 0x%x\n", sparse_header->magic);
-	dprintf (SPEW, "major_version: 0x%x\n", sparse_header->major_version);
-	dprintf (SPEW, "minor_version: 0x%x\n", sparse_header->minor_version);
-	dprintf (SPEW, "file_hdr_sz: %d\n", sparse_header->file_hdr_sz);
-	dprintf (SPEW, "chunk_hdr_sz: %d\n", sparse_header->chunk_hdr_sz);
-	dprintf (SPEW, "blk_sz: %d\n", sparse_header->blk_sz);
-	dprintf (SPEW, "total_blks: %d\n", sparse_header->total_blks);
-	dprintf (SPEW, "total_chunks: %d\n", sparse_header->total_chunks);
+	debug("=== Sparse Image Header ===\n");
+	debug("magic: 0x%x\n", sparse_header->magic);
+	debug("major_version: 0x%x\n", sparse_header->major_version);
+	debug("minor_version: 0x%x\n", sparse_header->minor_version);
+	debug("file_hdr_sz: %d\n", sparse_header->file_hdr_sz);
+	debug("chunk_hdr_sz: %d\n", sparse_header->chunk_hdr_sz);
+	debug("blk_sz: %d\n", sparse_header->blk_sz);
+	debug("total_blks: %d\n", sparse_header->total_blks);
+	debug("total_chunks: %d\n", sparse_header->total_chunks);
 
 	/* Start processing chunks */
 	for (chunk=0; chunk<sparse_header->total_chunks; chunk++)
@@ -95,33 +99,37 @@  void cmd_flash_mmc_sparse_img(const char *arg, void *data, unsigned sz)
 		chunk_header = (chunk_header_t *) data;
 		data += sizeof(chunk_header_t);
 
-		dprintf (SPEW, "=== Chunk Header ===\n");
-		dprintf (SPEW, "chunk_type: 0x%x\n", chunk_header->chunk_type);
-		dprintf (SPEW, "chunk_data_sz: 0x%x\n", chunk_header->chunk_sz);
-		dprintf (SPEW, "total_size: 0x%x\n", chunk_header->total_sz);
+		debug("=== Chunk Header ===\n");
+		debug("chunk_type: 0x%x\n", chunk_header->chunk_type);
+		debug("chunk_data_sz: 0x%x\n", chunk_header->chunk_sz);
+		debug("total_size: 0x%x\n", chunk_header->total_sz);
 
-		if(sparse_header->chunk_hdr_sz > sizeof(chunk_header_t))
+		if (sparse_header->chunk_hdr_sz > sizeof(chunk_header_t))
 		{
-			/* Skip the remaining bytes in a header that is longer than
-			 * we expected.
+			/*
+			 * Skip the remaining bytes in a header that is longer
+			 * than we expected.
 			 */
-			data += (sparse_header->chunk_hdr_sz - sizeof(chunk_header_t));
+			data += (sparse_header->chunk_hdr_sz -
+				 sizeof(chunk_header_t));
 		}
 
 		chunk_data_sz = sparse_header->blk_sz * chunk_header->chunk_sz;
 		switch (chunk_header->chunk_type)
 		{
 			case CHUNK_TYPE_RAW:
-			if(chunk_header->total_sz != (sparse_header->chunk_hdr_sz +
-											chunk_data_sz))
+			if (chunk_header->total_sz !=
+			    (sparse_header->chunk_hdr_sz + chunk_data_sz))
 			{
-				fastboot_fail("Bogus chunk size for chunk type Raw");
+				fastboot_fail(
+					"Bogus chunk size for chunk type Raw");
 				return;
 			}
 
-			if(mmc_write(ptn + ((uint64_t)total_blocks*sparse_header->blk_sz),
-						chunk_data_sz,
-						(unsigned int*)data))
+			if (mmc_write(ptn +
+				      ((uint64_t)total_blocks *
+						 sparse_header->blk_sz),
+				      chunk_data_sz, (unsigned int *)data))
 			{
 				fastboot_fail("flash write failure");
 				return;
@@ -131,17 +139,22 @@  void cmd_flash_mmc_sparse_img(const char *arg, void *data, unsigned sz)
 			break;
 
 			case CHUNK_TYPE_FILL:
-			if(chunk_header->total_sz != (sparse_header->chunk_hdr_sz +
-											sizeof(uint32_t)))
+			if (chunk_header->total_sz !=
+			    (sparse_header->chunk_hdr_sz + sizeof(uint32_t)))
 			{
-				fastboot_fail("Bogus chunk size for chunk type FILL");
+				fastboot_fail(
+					"Bogus chunk size for chunk type FILL");
 				return;
 			}
 
-			fill_buf = (uint32_t *)memalign(CACHE_LINE, ROUNDUP(sparse_header->blk_sz, CACHE_LINE));
+			fill_buf = (uint32_t *)
+				   memalign(CACHE_LINE,
+					    ROUNDUP(sparse_header->blk_sz,
+						    CACHE_LINE));
 			if (!fill_buf)
 			{
-				fastboot_fail("Malloc failed for: CHUNK_TYPE_FILL");
+				fastboot_fail(
+					"Malloc failed for: CHUNK_TYPE_FILL");
 				return;
 			}
 
@@ -156,9 +169,10 @@  void cmd_flash_mmc_sparse_img(const char *arg, void *data, unsigned sz)
 
 			for (i = 0; i < chunk_blk_cnt; i++)
 			{
-				if(mmc_write(ptn + ((uint64_t)total_blocks*sparse_header->blk_sz),
-							sparse_header->blk_sz,
-							fill_buf))
+				if (mmc_write(ptn +
+					      ((uint64_t)total_blocks *
+							 sparse_header->blk_sz),
+					      sparse_header->blk_sz, fill_buf))
 				{
 					fastboot_fail("flash write failure");
 					free(fill_buf);
@@ -176,9 +190,11 @@  void cmd_flash_mmc_sparse_img(const char *arg, void *data, unsigned sz)
 			break;
 
 			case CHUNK_TYPE_CRC:
-			if(chunk_header->total_sz != sparse_header->chunk_hdr_sz)
+			if (chunk_header->total_sz !=
+			    sparse_header->chunk_hdr_sz)
 			{
-				fastboot_fail("Bogus chunk size for chunk type Dont Care");
+				fastboot_fail(
+					"Bogus chunk size for chunk type Dont Care");
 				return;
 			}
 			total_blocks += chunk_header->chunk_sz;
@@ -186,19 +202,18 @@  void cmd_flash_mmc_sparse_img(const char *arg, void *data, unsigned sz)
 			break;
 
 			default:
-			dprintf(CRITICAL, "Unkown chunk type: %x\n",chunk_header->chunk_type);
+			debug("Unkown chunk type: %x\n",
+			      chunk_header->chunk_type);
 			fastboot_fail("Unknown chunk type");
 			return;
 		}
 	}
 
-	dprintf(INFO, "Wrote %d blocks, expected to write %d blocks\n",
-					total_blocks, sparse_header->total_blks);
+	debug("Wrote %d blocks, expected to write %d blocks\n",
+	      total_blocks, sparse_header->total_blks);
 
-	if(total_blocks != sparse_header->total_blks)
-	{
+	if (total_blocks != sparse_header->total_blks)
 		fastboot_fail("sparse image write failure");
-	}
 
 	fastboot_okay("");
 	return;