WAN: bit and/or confusion

PATCH Applied Roel Kluin 5 participants 11 messages Aug 14, 2009 – Aug 31, 2009

Subsystems (2)

Networking Network drivers

lore

Share & tools

lore

Roel Kluin

author

2009-08-14 12:47 UTC #0 lore raw

PATCH 1/1 WAN: bit and/or confusion

Fix the tests that check whether Frame* bits are not set

Signed-off-by Roel Kluin <[email protected]>

---
// vi drivers/net/wan/dscc4.c +307
#define FrameVfr 0x80
#define FrameRdo 0x40
#define FrameCrc 0x20
#define FrameRab 0x10

drivers/net/wan/dscc4.c +2 -2

index 8face5d..dd3c64a 100644
@@ -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++;

David Miller

maintainer Applied

2009-08-14 23:31 UTC #1 lore raw

From: Roel Kluin <[email protected]>
Date: Fri, 14 Aug 2009 14:51:46 +0200

Quoted 3 lines

Fix the tests that check whether Frame* bits are not set
Signed-off-by Roel Kluin <[email protected]>

Applied.

Andrew Morton

2009-08-14 23:37 UTC #2 lore raw

In reply to Roel Kluin

Quoted 15 lines · On Fri, 14 Aug 2009 14:51:46 +0200 Roel Kluin <[email protected]> wrote:

Fix the tests that check whether Frame* bits are not set
Signed-off-by Roel Kluin <[email protected]>
---
// vi drivers/net/wan/dscc4.c +307
#define FrameVfr 0x80
#define FrameRdo 0x40
#define FrameCrc 0x20
#define FrameRab 0x10
drivers/net/wan/dscc4.c +0 -0
index 8face5d..dd3c64a 100644

drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 667

Quoted 6 lines

drivers/net/wan/dscc4.c +1 -1

@@ -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++;

that's

	if (!(x & 0xffffffdf))

which seems peculiar. Should it have been

	else if (skb->data[pkt_len] & FrameCrc)

	else if (!(skb->data[pkt_len] & FrameCrc))

Quoted 5 lines

drivers/net/wan/dscc4.c +1 -1

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

David Miller

maintainer

2009-08-14 23:41 UTC #3 lore raw

From: Andrew Morton <[email protected]>
Date: Fri, 14 Aug 2009 16:36:44 -0700

Quoted 15 lines

drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 667
Quoted 7 lines · On Fri, 14 Aug 2009 14:51:46 +0200 Roel Kluin <[email protected]> wrote:
drivers/net/wan/dscc4.c +1 -1
@@ -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++;
that's
	if (!(x & 0xffffffdf))

Quoted 7 lines

which seems peculiar. Should it have been
	else if (skb->data[pkt_len] & FrameCrc)
or
	else if (!(skb->data[pkt_len] & FrameCrc))

Indeed, I can't tell which variant would be correct.

I'm reverting until someone with a datasheet for this chip speaks up :-)

Andrew Morton

2009-08-14 23:58 UTC #4 lore raw

Quoted 22 lines · On Fri, 14 Aug 2009 16:41:23 -0700 (PDT) David Miller <[email protected]>…

From: Andrew Morton <[email protected]>
Date: Fri, 14 Aug 2009 16:36:44 -0700
Quoted 19 lines
drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 667
Quoted 7 lines · On Fri, 14 Aug 2009 14:51:46 +0200 Roel Kluin <[email protected]> wrote:
drivers/net/wan/dscc4.c +1 -1
@@ -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++;
that's
	if (!(x & 0xffffffdf))
which seems peculiar. Should it have been
	else if (skb->data[pkt_len] & FrameCrc)

Quoted 8 lines

Quoted 3 lines
or
	else if (!(skb->data[pkt_len] & FrameCrc))
Indeed, I can't tell which variant would be correct.
I'm reverting until someone with a datasheet for this chip speaks up :-)

http://www.datasheet.in/download.php?id=39415

Page 383 and 384 say that bit 5 (CRC) is zero if the rx frame contained errors.

So we need

	else if (!(skb->data[pkt_len] & FrameCrc))

drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 668

Quoted 2 lines

drivers/net/wan/dscc4.c +1 -1

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

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

Roel Kluin

author

2009-08-15 13:36 UTC #5 lore raw

Fix the tests that check whether Frame* bits are not set

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 8 lines

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?

drivers/net/wan/dscc4.c +2 -2

index 8face5d..b686050 100644
@@ -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++;

Francois Romieu

2009-08-15 14:09 UTC #6 lore raw

Roel Kluin <[email protected]> :
[...]

Quoted 1 line

Maybe someone could test this?

I may undust a pair of card and a split box but do not hold your breath.

Signature · 1 line

Ueimor

Krzysztof Halasa

2009-08-15 18:46 UTC #7 lore raw

In reply to Roel Kluin

drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 667

Quoted 8 lines · Roel Kluin

+++ b/drivers/net/wan/dscc4.c

drivers/net/wan/dscc4.c +1 -1

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

drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 669

Quoted 3 lines

drivers/net/wan/dscc4.c +1 -1

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

drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 671

Quoted 4 lines

drivers/net/wan/dscc4.c +1 -0

+		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).

Signature · 1 line

Krzysztof Halasa

Roel Kluin

author

2009-08-20 14:00 UTC #8 lore raw

Fix the tests that check whether Frame* bits are not set

Signed-off-by Roel Kluin <[email protected]>

---

Quoted 8 lines

Review
Quoted 3 lines
(quoted diff) +1 -1
-		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:
Review
Quoted 1 line
(quoted diff) +1 -0
+		else if ((skb->data[pkt_len] & (FrameVfr | FrameRab)) != FrameVfr | FrameRab)

Ok, Francois Romieu, could you test this?

Quoted 10 lines

Review
Quoted 2 lines
(quoted diff) +0 -0
 		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?

Quoted 12 lines

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

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

I don't understand your suggestion - why status == 0? doesn't the patch below do what you want instead?

Roel

drivers/net/wan/dscc4.c +4 -4

index 8face5d..88534b6 100644
@@ -663,12 +663,12 @@ 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)) !=
+				FrameVfr | FrameRab)
 			dev->stats.rx_length_errors++;
-		else
-			dev->stats.rx_errors++;
+		dev->stats.rx_errors++;
 		dev_kfree_skb_irq(skb);
 	}
 refill:

Krzysztof Halasa

2009-08-20 14:25 UTC #9 lore raw

Quoted 15 lines · Roel Kluin

Quoted 14 lines

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++;
		}

Quoted 2 lines

I don't understand your suggestion - why status == 0? doesn't the patch below do what you want instead?

Because I think (didn't read the manual) that these (inverted) bits represent specific errors. So I suggested inverting them, then treating as separate error bits, it should be easier to read.

That's only a suggestion of course (unless someone sends me the/a card - non-returnable "donations" only please - and I can work on it personally).

IOW all (inverted) bits = 1 => all bits zero after inversion => no errors. This changes functionality a bit, and would need to be checked. Otherwise, you could test: if ((status & FrameOk) == 0) then looks_good().

Quoted 11 lines

drivers/net/wan/dscc4.c +1 -1

index 8face5d..88534b6 100644
@@ -663,12 +663,12 @@ 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++;

drivers/net/wan/dscc4.c static inline void dscc4_rx_skb(struct dscc4_dev_priv *dpriv, · line 674

Quoted 10 lines

drivers/net/wan/dscc4.c +3 -3

-		else if (!(skb->data[pkt_len] | ~(FrameVfr | FrameRab)))
+		else if ((skb->data[pkt_len] & (FrameVfr | FrameRab)) !=
+				FrameVfr | FrameRab)
 			dev->stats.rx_length_errors++;
-		else
-			dev->stats.rx_errors++;
+		dev->stats.rx_errors++;
 		dev_kfree_skb_irq(skb);
 	}
 refill:

Guess it would do.

Signature · 1 line

Krzysztof Halasa

David Miller

maintainer Applied

2009-08-31 05:02 UTC #10 lore raw

In reply to Roel Kluin

From: Roel Kluin <[email protected]>
Date: Thu, 20 Aug 2009 16:04:40 +0200

Quoted 3 lines

Fix the tests that check whether Frame* bits are not set
Signed-off-by Roel Kluin <[email protected]>

Applied to net-next-2.6, thanks.