Re: [flashrom] [PATCH 1/1] Fix programmer-centric probe

[Stefan Tauner]

On Thu, 22 Dec 2011 01:38:09 +0100
Michael Karcher  wrote:
> As reported by Stefan Tauner on IRC, the new programmer-centric logic
> is broken by re-using occupied members of the flashes array when changing
> to the next programmer. This fixes it
> Signed-off-by: Michael Karcher
>
> cli_classic.c |   15 +++++++++------
>  1 files changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/cli_classic.c b/cli_classic.c
> index 543b644..10ed0f4 100644
> --- a/cli_classic.c
> +++ b/cli_classic.c
> @@ -451,14 +451,17 @@ int main(int argc, char *argv[])
>
> for (j = 0; j < registered_programmer_count; j++) {
>    	startchip = 0;
> for (i = 0; i < ARRAY_SIZE(flashes); i++) {
> do {
> startchip = probe_flash(&registered_programmers[j],
> 				startchip, &flashes[i], 0);
> 	if (startchip == -1)
> 		break;
> 	chipcount++;
> 	startchip++;
> 				startchip, 

^ white space at EOL
> 				&flashes[chipcount], 0);
> 	if (startchip != -1)
> 	{

^ should be in the same line as the "if"
> 		chipcount++;
> 		startchip++;
> 	}
> }
> while( startchip != -1 && chipcount < ARRAY_SIZE(flashes) );

^ spaces inside the while braces but not on the outside. wrong way
around afaics.
> }
>
> if (chipcount > 1) {

the semantics look ok, but i dont get the change of the inner "if".
startchip is reset anyway, chipcount is not touched in either version.
why did you change it? see below for my version (a cheap ripoff of your
patch, of course).

i still hate the index tossed around in the probing process. in theory
the segregation between the caller of probe_flash and itself is
advantageous because the caller decides what to do on a chip match. but
in practice this has bitten us a few times, is awfully readable because
the important paths are spread over two pretty complicated functions and
does not buy us anything(! imho). allocating a few simple list elements
does not cost anything relative to the costs of probing the chips.

i would just return a null terminated single linked list and be happy.
or even save the pointer to the head of that list in struct
registered_programmer and just return success.
carldani: btw why is the "void *data" not in the common part of
registered_programmer (like buses_supported) but inside each of the 3
programmer types?

Signed-off-by: Michael Karcher 
and if need be:
Signed-off-by: Stefan Tauner 
---
 cli_classic.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/cli_classic.c b/cli_classic.c
index 543b644..c495714 100644
--- a/cli_classic.c
+++ b/cli_classic.c
@@ -451,14 +451,17 @@ int main(int argc, char *argv[])

for (j = 0; j < registered_programmer_count; j++) {
    	startchip = 0;
-		for (i = 0; i < ARRAY_SIZE(flashes); i++) {
+		do {
    		startchip = probe_flash(&registered_programmers[j],
-						startchip, &flashes[i], 0);
+						startchip,
+						&flashes[chipcount], 0);
    		if (startchip == -1)
    			break;
+
    		chipcount++;
    		startchip++;
    	}
+		while(chipcount < ARRAY_SIZE(flashes));
    }

if (chipcount > 1) {