Patchwork [net-next] drivers/net/ethernet/micrel/ks8851_mll: Implement basic statistics

login
register
mail settings
Submitter Choi, David
Date Jan. 29, 2013, 6:24 p.m.
Message ID <FD9AD8C5375B924CABC56D982DB3A802079D382B@EXMB1.micrel.com>
Download mbox | patch
Permalink /patch/216617/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Choi, David - Jan. 29, 2013, 6:24 p.m.
From: David J. Choi <david.choi@micrel.com>
 
Implement to collect basic statistical information about Ethernet packets.
 
Signed-off-by: David J. Choi <david.choi@micrel.com>
---


---
--
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
Joe Perches - Jan. 29, 2013, 6:42 p.m.
On Tue, 2013-01-29 at 18:24 +0000, Choi, David wrote:
> From: David J. Choi <david.choi@micrel.com>

Hello David.

When you resubmit a patch, please use a different
header for each resubmission.

This email subject line should have been

	[PATCH V3] ks8851_mll: Implement basic statistics

Also, don't use the complete path in the subject.
Read Documentation/SubmittingPatches Section 1, #15.

Please use git format-patch and git-send-email.

Please use a changelog describing what changes
you've made for each version.  The changelog is
generally placed after the --- separator that
git format-email produces.

More comments interspersed.

> +++ net-next/drivers/net/ethernet/micrel/ks8851_mll.c	2013-01-29 10:06:09.000000000 -0800
> @@ -793,19 +793,35 @@ static void ks_rcv(struct ks_net *ks, st
>  	frame_hdr = ks->frame_head_info;
>  	while (ks->frame_cnt--) {
>  		skb = netdev_alloc_skb(netdev, frame_hdr->len + 16);
> -		if (likely(skb && (frame_hdr->sts & RXFSHR_RXFV) &&
> -			(frame_hdr->len < RX_BUF_SIZE) && frame_hdr->len)) {
> +		if (unlikely(!skb)) {
> +			/* discard the packet from the device */
> +			ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
> +			netdev->stats.rx_dropped++;
> +		} else if (likely((frame_hdr->sts & RXFSHR_RXFV) &&
> +				frame_hdr->len > 0 &&
> +				frame_hdr->len <= RX_BUF_SIZE)) {

OK, <= here

>  			skb_reserve(skb, 2);
>  			/* read data block including CRC 4 bytes */
>  			ks_read_qmu(ks, (u16 *)skb->data, frame_hdr->len);
> -			skb_put(skb, frame_hdr->len);
> +
> +			/* exclude the size of CRC */
> +			skb_put(skb, frame_hdr->len - 4);
>  			skb->protocol = eth_type_trans(skb, netdev);
>  			netif_rx(skb);
> +			netdev->stats.rx_packets++;
> +
> +			/* crc field */
> +			netdev->stats.rx_bytes += frame_hdr->len - 4;
>  		} else {
> -			pr_err("%s: err:skb alloc\n", __func__);
> -			ks_wrreg16(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_RRXEF));
> -			if (skb)
> -				dev_kfree_skb_irq(skb);
> +			/* discard the packet from the device */
> +			ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
> +			netdev->stats.rx_dropped++;
> +			if (frame_hdr->len >= RX_BUF_SIZE || !frame_hdr->len)

but >= here and !frame_hdr->len not frame_hdr-len == 0
Please be consistent.

> +				netdev->stats.rx_length_errors++;
> +			if (!(frame_hdr->sts & RXFSHR_RXFV))
> +				netdev->stats.rx_frame_errors++;
> +
> +			dev_kfree_skb_irq(skb);
>  		}
>  		frame_hdr++;
>  	}

It seems that a 0 length frame or an over length
frame is pretty unlikely and those should probably
be tested and accounted for before the
netdev_alloc_skb is done so that what seems an
unnecessary alloc/free is avoided.

--
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
David Miller - Jan. 29, 2013, 6:55 p.m.
From: "Choi, David" <David.Choi@Micrel.Com>
Date: Tue, 29 Jan 2013 18:24:57 +0000

Do not post new versions of patches as replies to other emails or
threads, always use fresh, new list postings to post a patch.

> +		} else if (likely((frame_hdr->sts & RXFSHR_RXFV) &&
> +				frame_hdr->len > 0 &&
> +				frame_hdr->len <= RX_BUF_SIZE)) {

Not indented properly, if you're using purely TAB characters and no
space characters at all, chances are you're doing it wrong.

Conditionals are to be indented like this:

	if (A &&
	    B ||
	    C)

Specifically, the first character on the second and subsequent
lines must line up with the first column after the openning
parenthesis on the first line.

You must use whatever combination of TAB and space characters
are necessary to achieve this.

Several text editors, such as emacs, can be configured to do
all of this automatically for you when you press TAB.
--
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
Joe Perches - Jan. 29, 2013, 7:09 p.m.
On Tue, 2013-01-29 at 13:55 -0500, David Miller wrote:
> Do not post new versions of patches as replies to other emails or
> threads, always use fresh, new list postings to post a patch.

I think replying with In-Reply-To: context is better
than starting a new thread without that In-Reply-To.

If the subject changes with version number, what
difference does it make?

Is there some case that patchwork doesn't handle well?



--
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
David Miller - Jan. 29, 2013, 7:18 p.m.
From: Joe Perches <joe@perches.com>
Date: Tue, 29 Jan 2013 11:09:18 -0800

> On Tue, 2013-01-29 at 13:55 -0500, David Miller wrote:
>> Do not post new versions of patches as replies to other emails or
>> threads, always use fresh, new list postings to post a patch.
> 
> I think replying with In-Reply-To: context is better
> than starting a new thread without that In-Reply-To.
> 
> If the subject changes with version number, what
> difference does it make?
> 
> Is there some case that patchwork doesn't handle well?

I just don't want to see patches in the middle of threads, it
confuses where it's just an RFC take or a real submission.
--
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

Patch

--- net-next/drivers/net/ethernet/micrel/ks8851_mll.c.orig	2013-01-22 17:25:59.000000000 -0800
+++ net-next/drivers/net/ethernet/micrel/ks8851_mll.c	2013-01-29 10:06:09.000000000 -0800
@@ -793,19 +793,35 @@  static void ks_rcv(struct ks_net *ks, st
 	frame_hdr = ks->frame_head_info;
 	while (ks->frame_cnt--) {
 		skb = netdev_alloc_skb(netdev, frame_hdr->len + 16);
-		if (likely(skb && (frame_hdr->sts & RXFSHR_RXFV) &&
-			(frame_hdr->len < RX_BUF_SIZE) && frame_hdr->len)) {
+		if (unlikely(!skb)) {
+			/* discard the packet from the device */
+			ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
+			netdev->stats.rx_dropped++;
+		} else if (likely((frame_hdr->sts & RXFSHR_RXFV) &&
+				frame_hdr->len > 0 &&
+				frame_hdr->len <= RX_BUF_SIZE)) {
 			skb_reserve(skb, 2);
 			/* read data block including CRC 4 bytes */
 			ks_read_qmu(ks, (u16 *)skb->data, frame_hdr->len);
-			skb_put(skb, frame_hdr->len);
+
+			/* exclude the size of CRC */
+			skb_put(skb, frame_hdr->len - 4);
 			skb->protocol = eth_type_trans(skb, netdev);
 			netif_rx(skb);
+			netdev->stats.rx_packets++;
+
+			/* crc field */
+			netdev->stats.rx_bytes += frame_hdr->len - 4;
 		} else {
-			pr_err("%s: err:skb alloc\n", __func__);
-			ks_wrreg16(ks, KS_RXQCR, (ks->rc_rxqcr | RXQCR_RRXEF));
-			if (skb)
-				dev_kfree_skb_irq(skb);
+			/* discard the packet from the device */
+			ks_wrreg16(ks, KS_RXQCR, ks->rc_rxqcr | RXQCR_RRXEF);
+			netdev->stats.rx_dropped++;
+			if (frame_hdr->len >= RX_BUF_SIZE || !frame_hdr->len)
+				netdev->stats.rx_length_errors++;
+			if (!(frame_hdr->sts & RXFSHR_RXFV))
+				netdev->stats.rx_frame_errors++;
+
+			dev_kfree_skb_irq(skb);
 		}
 		frame_hdr++;
 	}
@@ -876,6 +892,8 @@  static irqreturn_t ks_irq(int irq, void 
 		pmecr &= ~PMECR_WKEVT_MASK;
 		ks_wrreg16(ks, KS_PMECR, pmecr | PMECR_WKEVT_LINK);
 	}
+	if (unlikely(status & IRQ_RXOI))
+		ks->netdev->stats.rx_over_errors++;
 
 	/* this should be the last in IRQ handler*/
 	ks_restore_cmd_reg(ks);
@@ -1015,6 +1033,8 @@  static int ks_start_xmit(struct sk_buff 
 
 	if (likely(ks_tx_fifo_space(ks) >= skb->len + 12)) {
 		ks_write_qmu(ks, skb->data, skb->len);
+		netdev->stats.tx_bytes += skb->len;
+		netdev->stats.tx_packets++;
 		dev_kfree_skb(skb);
 	} else
 		retv = NETDEV_TX_BUSY;