Re: proposed patch

[Ben C. <bco...@de...>]

> Index: ./nodemgr.c
> ===================================================================
> --- ./nodemgr.c
> +++ ./.svn/tmp/nodemgr.c.63384.00001.tmp	2002-08-09 18:18:47.000000000 -0700
> @@ -1002,9 +1002,10 @@
>  					 addr, &buffer[0]) < 0) {
>  			HPSB_ERR("ConfigROM quadlet transaction error for node "
>  				 NODE_BUS_FMT, NODE_BUS_ARGS(nodeid));
> -			return -1;
> -		}
> -		if (buffer[0])
> +
> +			if (i == 3)
> +				return -1;
> +		} else if (buffer[0])
>  			break;
>  
>  		set_current_state(TASK_INTERRUPTIBLE);

You're going to need to explain this to me. I understand what you are
doing, but not why, or how you decided that this needed to be done.

For one, we always fail if we get an error from nodemgr_read_quadlet
everywhere else in the code. We don't ignore it for 3 times like you are
doing in this case. Not only that, but if that's what we wanted to do in
this code, the "if (i == 3)" would be kind of repetitive in the for
loop.

In nodemgr_read_quadlet we already try the read 3 times. Why would we
try 3*3 in this case? If what you are saying is that we need more time
for this case, then we should put the timing space in
nodemgr_read_quadlet, not in this one special place.

Aside from that, I'll defer to James for comments on the sbp2 code.

-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
Deqo       - http://www.deqo.com/