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