Re: [flashrom] [PATCH 5/6] improve output in case run_opcode fails

[Stefan Tauner]

On Fri,  1 Apr 2011 14:33:19 +0200
Stefan Tauner  wrote:
> diff --git a/ichspi.c b/ichspi.c
> index b24b6a0..0ad4f20 100644
> --- a/ichspi.c
> +++ b/ichspi.c
> ...
> @@ -974,7 +973,19 @@ int ich_spi_send_command(unsigned int writecnt,
> unsigned int readcnt, 
>    result = run_opcode(*opcode, addr, count, data);
>    if (result) {
> msg_pdbg("run OPCODE 0x%02x failed\n",
>
> opcode->opcode);
> msg_pdbg("run OPCODE 0x%02x failed at address
>
> 0x%06x.\n",
> 	 opcode->opcode, addr);
> /* Print out the data array if it contains data to
>
> write.
>  * Errors are detected before the received data is
>
> read back into
>  * the array so it won't make sense to print it
>
> then. */
> if (((opcode->spi_type ==
>
> SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) ||
>      (opcode->spi_type ==
>
> SPI_OPCODE_TYPE_WRITE_NO_ADDRESS))) {
> 	msg_pspew("The data was:\n");
> 	uint8_t i;
> 	for(i=0; i<count; i++){
> 		msg_pspew("%3d: 0x%02x\n", i,
>
> data[i]);
> 	}
> }
> }

self-NAK :)

uint8_t is too small and could result in an endless loop. oops. :(
this should not happen normally, because devices supported by ichspi.c
allow only 16B or 64B data lengths, but we could be called with
something bigger i guess.
how should this be solved?
since "count" will get truncated implicitly when it is used as
parameter for "run_opcode" we could do so explicitly before the loop.
or an if around the loop checking for count<256.

also i would like to include the count like so:
"run OPCODE 0x%02x failed at address 0x%06x (length %d).\n"

RFC :)

[PATCH 5/6] improve output in case run_opcode fails

[Michael Karcher]

Am Samstag, den 30.04.2011, 00:06 +0200 schrieb Stefan Tauner:
> > 	uint8_t i;
> > 	for(i=0; i<count; i++){
> > 		msg_pspew("%3d: 0x%02x\n", i,
> >
> > data[i]);
> > 	}
> > }
> > }
>
> self-NAK :)
>
> uint8_t is too small and could result in an endless loop. oops. :(
> this should not happen normally, because devices supported by ichspi.c
> allow only 16B or 64B data lengths, but we could be called with
> something bigger i guess.
> how should this be solved?

use the same type as count for i. In this case: int.

Regards,
  Michael Karcher

[PATCH] improve output in case run_opcode fails

[Stefan Tauner]

Signed-off-by: Stefan Tauner 
---
 ichspi.c |   22 +++++++++++++++++++---
 1 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/ichspi.c b/ichspi.c
index 5c66ddd..1086a67 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -758,8 +758,7 @@ static int ich9_run_opcode(OPCODE op, uint32_t offset,
    /* FIXME make sure we do not needlessly cause transaction errors. */
    temp32 = REGREAD32(ICH9_REG_SSFS);
    if (temp32 & SSFS_FCERR) {
-		msg_perr("Transaction error for opcode 0x%02x!\n",
-			 op.opcode);
+		msg_perr("Transaction error!\n");
    	/* keep reserved bits */
    	temp32 &= SSFS_RESERVED_MASK | SSFC_RESERVED_MASK;
    	/* Clear the transaction error. */
@@ -934,7 +933,24 @@ int ich_spi_send_command(unsigned int writecnt, unsigned int readcnt,

result = run_opcode(*opcode, addr, count, data);
    if (result) {
-		msg_pdbg("run OPCODE 0x%02x failed\n", opcode->opcode);
+		msg_pdbg("Running OPCODE 0x%02x failed ", opcode->opcode);
+		if ((opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) ||
+		    (opcode->spi_type == SPI_OPCODE_TYPE_READ_WITH_ADDRESS)) {
+			msg_pdbg("at address 0x%06x ", addr);
+		}
+		msg_pdbg("(payload length was %d).\n", count);
+
+		/* Print out the data array if it contains data to write.
+		 * Errors are detected before the received data is read back into
+		 * the array so it won't make sense to print it then. */
+		if ((opcode->spi_type == SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS) ||
+		    (opcode->spi_type == SPI_OPCODE_TYPE_WRITE_NO_ADDRESS)) {
+			msg_pspew("The data was:\n");
+			int i;
+			for(i=0; i<count; i++){
+				msg_pspew("%3d: 0x%02x\n", i, data[i]);
+			}
+		}
    }

return result;