On Mon, 2015-09-21 at 23:01 +0200, Francois Romieu wrote:
> 
> Can you pile a patch to replace BUG with WARN_ON_ONCE(1) ?

Let's avoid having three copies of the same damn code, while we're at
it... http://git.infradead.org/users/dwmw2/linux-8139cp.git has this
and the appropriate minor fixes to subsequent patches in the series.

What do you think of finally enabling hw csum and TSO by default, btw?

Subject: [PATCH 4½/7] 8139cp: Reduce duplicate csum/tso code in cp_start_xmit()

We calculate the value of the opts1 descriptor field in three different
places. With two different behaviours when given an invalid packet to
be checksummed — none of them correct. Sort that out.

Signed-off-by: David Woodhouse <david.woodho...@intel.com>
---
 drivers/net/ethernet/realtek/8139cp.c | 61 ++++++++++++-----------------------
 1 file changed, 20 insertions(+), 41 deletions(-)

diff --git a/drivers/net/ethernet/realtek/8139cp.c 
b/drivers/net/ethernet/realtek/8139cp.c
index 3b219aa..a2c471d 100644
--- a/drivers/net/ethernet/realtek/8139cp.c
+++ b/drivers/net/ethernet/realtek/8139cp.c
@@ -740,7 +740,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 {
        struct cp_private *cp = netdev_priv(dev);
        unsigned entry;
-       u32 eor, flags;
+       u32 eor, opts1;
        unsigned long intr_flags;
        __le32 opts2;
        int mss = 0;
@@ -760,6 +760,21 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
        mss = skb_shinfo(skb)->gso_size;
 
        opts2 = cpu_to_le32(cp_tx_vlan_tag(skb));
+       opts1 = DescOwn;
+       if (mss)
+               opts1 |= LargeSend | ((mss & MSSMask) << MSSShift);
+       else if (skb->ip_summed == CHECKSUM_PARTIAL) {
+               const struct iphdr *ip = ip_hdr(skb);
+               if (ip->protocol == IPPROTO_TCP)
+                       opts1 |= IPCS | TCPCS;
+               else if (ip->protocol == IPPROTO_UDP)
+                       opts1 |= IPCS | UDPCS;
+               else {
+                       WARN_ONCE(1,
+                                 "Net bug: asked to checksum invalid Legacy IP 
packet\n");
+                       goto out_dma_error;
+               }
+       }
 
        if (skb_shinfo(skb)->nr_frags == 0) {
                struct cp_desc *txd = &cp->tx_ring[entry];
@@ -775,21 +790,9 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
                txd->addr = cpu_to_le64(mapping);
                wmb();
 
-               flags = eor | len | DescOwn | FirstFrag | LastFrag;
-
-               if (mss)
-                       flags |= LargeSend | ((mss & MSSMask) << MSSShift);
-               else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-                       const struct iphdr *ip = ip_hdr(skb);
-                       if (ip->protocol == IPPROTO_TCP)
-                               flags |= IPCS | TCPCS;
-                       else if (ip->protocol == IPPROTO_UDP)
-                               flags |= IPCS | UDPCS;
-                       else
-                               WARN_ON(1);     /* we need a WARN() */
-               }
+               opts1 |= eor | len | FirstFrag | LastFrag;
 
-               txd->opts1 = cpu_to_le32(flags);
+               txd->opts1 = cpu_to_le32(opts1);
                wmb();
 
                cp->tx_skb[entry] = skb;
@@ -800,7 +803,6 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
                u32 first_len, first_eor, ctrl;
                dma_addr_t first_mapping;
                int frag, first_entry = entry;
-               const struct iphdr *ip = ip_hdr(skb);
 
                /* We must give this initial chunk to the device last.
                 * Otherwise we could race with the device.
@@ -832,19 +834,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
 
                        eor = (entry == (CP_TX_RING_SIZE - 1)) ? RingEnd : 0;
 
-                       ctrl = eor | len | DescOwn;
-
-                       if (mss)
-                               ctrl |= LargeSend |
-                                       ((mss & MSSMask) << MSSShift);
-                       else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-                               if (ip->protocol == IPPROTO_TCP)
-                                       ctrl |= IPCS | TCPCS;
-                               else if (ip->protocol == IPPROTO_UDP)
-                                       ctrl |= IPCS | UDPCS;
-                               else
-                                       BUG();
-                       }
+                       ctrl = opts1 | eor | len;
 
                        if (frag == skb_shinfo(skb)->nr_frags - 1)
                                ctrl |= LastFrag;
@@ -864,18 +854,7 @@ static netdev_tx_t cp_start_xmit (struct sk_buff *skb,
                txd->addr = cpu_to_le64(first_mapping);
                wmb();
 
-               ctrl = first_eor | first_len | FirstFrag | DescOwn;
-               if (mss)
-                       ctrl |= LargeSend | (mss & MSSMask) << MSSShift);
-               else if (skb->ip_summed == CHECKSUM_PARTIAL) {
-                       if (ip->protocol == IPPROTO_TCP)
-                               ctrl |= IPCS | TCPCS;
-                       else if (ip->protocol == IPPROTO_UDP)
-                               ctrl |= IPCS | UDPCS;
-                       else
-                               BUG();
-               }
-
+               ctrl = opts1 | first_eor | first_len | FirstFrag;
                txd->opts1 = cpu_to_le32(ctrl);
                wmb();
 
-- 
2.4.3



-- 
David Woodhouse                            Open Source Technology Centre
david.woodho...@intel.com                              Intel Corporation

Attachment: smime.p7s
Description: S/MIME cryptographic signature

Reply via email to