Re: IRDA: mcs7780.c

Re: IRDA: mcs7780.c

[Roel Kluin]

Juha Leppänen wrote:
> Hi,
> 
> In recent patch to file
> 
> drivers/net/irda/mcs7780.c

That's this one:
http://www.spinics.net/lists/netdev/msg88746.html

[PATCH] IRDA: cnt is off by 1

> I noticed when checking the code around the patch ...
> In worst case do {...} while loop is executed
> 101 times --> mcs_get_reg() is executed 101 times...
> But rval is checked only 100 times.

Right, thanks.

> If you change the order of the && expressions inside while () to
> 
> -        } while(cnt++ < 100 && (rval & MCS_IRINTX));
> +        } while((rval & MCS_IRINTX) && cnt++ < 100);

> If you also want exactly 100 maximum mcs_get_reg()
> tries ( now it is 101 ) ...

I don't think it really matters, does it?

> Happy hacking,
> 
> Mr. Juha Leppänen
> Kuopio, Finland

You too.

------------------------------>8----------------8<------------------------------
If no prior break occurs, cnt reaches 101 after the loop, so we are still able
to change speed when cnt has become 100.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
diff --git a/drivers/net/irda/mcs7780.c b/drivers/net/irda/mcs7780.c
index 7eafdca..8fdfb5a 100644
--- a/drivers/net/irda/mcs7780.c
+++ b/drivers/net/irda/mcs7780.c
@@ -583,9 +583,9 @@ static int mcs_speed_change(struct mcs_cb *mcs)
 
 	do {
 		mcs_get_reg(mcs, MCS_RESV_REG, &rval);
-	} while(cnt++ < 100 && (rval & MCS_IRINTX));
+	} while((rval & MCS_IRINTX) && cnt++ < 100);
 
-	if(cnt >= 100) {
+	if (cnt > 100) {
 		IRDA_ERROR("unable to change speed\n");
 		ret = -EIO;
 		goto error;

Re: IRDA: mcs7780.c

[Jarek Poplawski]

Roel Kluin wrote, On 02/13/2009 02:14 PM:

> Juha Leppänen wrote:
>> Hi,
>>
>> In recent patch to file
>>
>> drivers/net/irda/mcs7780.c
> 
> That's this one:
> http://www.spinics.net/lists/netdev/msg88746.html
> 
> [PATCH] IRDA: cnt is off by 1
> 
>> I noticed when checking the code around the patch ...
>> In worst case do {...} while loop is executed
>> 101 times --> mcs_get_reg() is executed 101 times...
>> But rval is checked only 100 times.
> 
> Right, thanks.
> 
>> If you change the order of the && expressions inside while () to
>>
>> -        } while(cnt++ < 100 && (rval & MCS_IRINTX));
>> +        } while((rval & MCS_IRINTX) && cnt++ < 100);
> 
>> If you also want exactly 100 maximum mcs_get_reg()
>> tries ( now it is 101 ) ...


Yes, but we don't know what an author of this code exactly
wanted. There is 1 get unused. But 100 can mean here: if
there is more than 100 it's wrong. After this current patch
we check for 100, but 101 positive get is OK, which IMHO
makes it even less readable.

> 
> I don't think it really matters, does it?


I think it should matter to write logical code, but the
practice is to fix such things in new code or during major
rewriting. Older code is usually changed only when there
is some bug (like triggering an error without counter
overflow). So, unless I miss something, I think there is
no reason here to try David's patience here. ;-)
(But even if I'm wrong - this patch is broken - it doesn't
apply to net-2.6 tree.)

Cheers,
Jarek P.

> 
>> Happy hacking,
>>
>> Mr. Juha Leppänen
>> Kuopio, Finland
> 
> You too.
> 
> ------------------------------>8----------------8<------------------------------
> If no prior break occurs, cnt reaches 101 after the loop, so we are still able
> to change speed when cnt has become 100.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/net/irda/mcs7780.c b/drivers/net/irda/mcs7780.c
> index 7eafdca..8fdfb5a 100644
> --- a/drivers/net/irda/mcs7780.c
> +++ b/drivers/net/irda/mcs7780.c
> @@ -583,9 +583,9 @@ static int mcs_speed_change(struct mcs_cb *mcs)
>  
>  	do {
>  		mcs_get_reg(mcs, MCS_RESV_REG, &rval);
> -	} while(cnt++ < 100 && (rval & MCS_IRINTX));
> +	} while((rval & MCS_IRINTX) && cnt++ < 100);
>  
> -	if(cnt >= 100) {
> +	if (cnt > 100) {
>  		IRDA_ERROR("unable to change speed\n");
>  		ret = -EIO;
>  		goto error;
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>