diff mbox

WAN: bit and/or confusion

Message ID 4A86BAF3.5060609@gmail.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

roel kluin Aug. 15, 2009, 1:41 p.m. UTC 
Fix the tests that check whether Frame* bits are not set

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
> we need
> 
> 	else if (!(skb->data[pkt_len] & FrameCrc))

> vfr is "valid frame".  0 is invalid.
> 
> rab is "receive message aborted".  The data sheet doesn't actually say
> if the bit is active-high or active-low (grr).  

Maybe someone could test this?


--
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

Comments

Francois Romieu Aug. 15, 2009, 2:13 p.m. UTC | #1 
Roel Kluin <roel.kluin@gmail.com> :
[...]
> Maybe someone could test this?

I may undust a pair of card and a split box but do not hold your breath.
Krzysztof Halasa Aug. 15, 2009, 6:46 p.m. UTC | #2 
Roel Kluin <roel.kluin@gmail.com> writes:

> +++ b/drivers/net/wan/dscc4.c
> @@ -663,9 +663,9 @@ static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv,
>  	} else {
>  		if (skb->data[pkt_len] & FrameRdo)
>  			dev->stats.rx_fifo_errors++;
> -		else if (!(skb->data[pkt_len] | ~FrameCrc))
> +		else if (!(skb->data[pkt_len] & FrameCrc))
>  			dev->stats.rx_crc_errors++;

This looks like a correct fix.

> -		else if (!(skb->data[pkt_len] | ~(FrameVfr | FrameRab)))
> +		else if (!(skb->data[pkt_len] & (FrameVfr | FrameRab)))
>  			dev->stats.rx_length_errors++;

This test requires both FrameVfr and FrameRab to be true (zero). Perhaps
it should be:

> +		else if ((skb->data[pkt_len] & (FrameVfr | FrameRab)) != FrameVfr | FrameRab)

>  		else
>  			dev->stats.rx_errors++;

rx_errors is incremented only on remaining errors. I think most drivers
increment rx_errors on all RX errors, and simultaneously rx_*_errors
when needed.


Perhaps something like the following should be better?

		u8 status = ~skb->data[pkt_len];

		if (status == 0)
			looks_good...;
		else {
			if (status & FrameRab)
				...
			if (status & FrameVfr)
				...
			etc.
			rx_errors++;
		}

I don't have the hardware and can't test (donations of such hw welcome).
diff mbox

Patch

diff --git a/drivers/net/wan/dscc4.c b/drivers/net/wan/dscc4.c
index 8face5d..b686050 100644
--- a/drivers/net/wan/dscc4.c
+++ b/drivers/net/wan/dscc4.c
@@ -663,9 +663,9 @@  static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv,
 	} else {
 		if (skb->data[pkt_len] & FrameRdo)
 			dev->stats.rx_fifo_errors++;
-		else if (!(skb->data[pkt_len] | ~FrameCrc))
+		else if (!(skb->data[pkt_len] & FrameCrc))
 			dev->stats.rx_crc_errors++;
-		else if (!(skb->data[pkt_len] | ~(FrameVfr | FrameRab)))
+		else if (!(skb->data[pkt_len] & (FrameVfr | FrameRab)))
 			dev->stats.rx_length_errors++;
 		else
 			dev->stats.rx_errors++;