Re: [PATCH 2/3] r8169: code clean-up

[Jon Mason <jdmason@us.ibm.com>]

On Sunday 20 February 2005 05:34 pm, Francois Romieu wrote:
[...]
> > True, but this problem existed before I made the jumbo frames changes.  The 
> 
> AFAIKS, if the rx_copybreak fails, be it because the packet is too big or
> because the allocation failed, the current skb is sent to the upper layers.

You are correct, I misunderstood the error path of the previous rx_copy routine.  

> > static inline int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
> >                               struct RxDesc *desc, struct rtl8169_private *tp)
> > {
> >         u32 status = le32_to_cpu(desc->opts1);
> > 
> >         if ((pkt_size > rx_copybreak) &&
> >                         ((status & FirstFrag) && (status & LastFrag)))
> >                 return -1;
> > 
> >         if (status & FirstFrag) {
> >                 struct sk_buff *skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
> >                 u32 len = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
> > 
> >                 if (skb) {
> >                         skb_reserve(skb, NET_IP_ALIGN);
> >                         memcpy(skb->data, sk_buff[0]->tail, len);
> >                         skb_put(skb, len);
> >                 } else {
> >                         printk(KERN_INFO "no rx skb allocated\n");
> >                         if (pkt_size <= rx_copybreak) 
> >                                 return -1;
> >                 }
> > 
> >                 tp->new_skb = skb;
> >         }
> > 
> > Is this acceptable?
> 
> I'll think more until I can figure I got the whole picture (only copy the
> first frament ?). I would have expected something like the mess below:
> 
> static int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
>       struct RxDesc *desc, struct rtl8169_private *tp)
> {
>  u32 status = le32_to_cpu(desc->opts1);
>  struct sk_buff *skb;
>  int ret = -1;
> 
>  if ((pkt_size > rx_copybreak) && (status & FirstFrag) &&
>      (status & LastFrag)) {
>   goto out;
>  }
> 
>  if (pkt_size < rx_copybreak) {
>   skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
>   if (!skb)
>    goto out;
>   skb_reserve(skb, NET_IP_ALIGN);
>  } else if (status & FirstFrag)
>   skb = tp->new_skb = sk_buff[0];
> 
>  if ((pkt_size < rx_copybreak) || !(status & FirstFrag)) {
>   memcpy(skb->data, sk_buff[0]->tail, pkt_size);
>   rtl8169_return_to_asic(desc, tp->rx_buf_sz);
>   *sk_buff = skb;
>   ret = 0;
>  }
> 
>  skb_put(skb, pkt_size);
> 
> out:
>  return ret;
> }
> 
> At this point I'd expect "How do you handle rtl8169_rx_copy() return ?" 
> (or "yuck").

My main problem with the above patch is that is assumes that there will be a maximum of 2 descriptors per jumbo frame (which isn't the case).  This might have been caused by my incomplete patch given above.  This is what I currently have:
static inline int rtl8169_rx_copy(struct sk_buff **sk_buff, int pkt_size,
                                struct RxDesc *desc, struct rtl8169_private *tp)
{
        u32 status = le32_to_cpu(desc->opts1);

        if ((pkt_size > rx_copybreak) &&
                        ((status & FirstFrag) && (status & LastFrag)))
                return -1;

        if (status & FirstFrag) {
                u32 len = (pkt_size > rx_copybreak) ? tp->desc_part : pkt_size;
                struct sk_buff *skb = dev_alloc_skb(pkt_size + NET_IP_ALIGN);
                if (skb) {
                        skb_reserve(skb, NET_IP_ALIGN);
                        memcpy(skb->data, sk_buff[0]->tail, len);
                        skb_put(skb, len);
                } else {
                        printk(KERN_INFO "no rx skb allocated\n");
                        if (pkt_size <= rx_copybreak)
                                return -1;
                }

                tp->new_skb = skb;
        }

        if (!(status & FirstFrag) && !(status & LastFrag) && tp->new_skb) {
                memcpy(tp->new_skb->tail, sk_buff[0]->tail, tp->desc_part);
                skb_put(tp->new_skb, tp->desc_part);
        }

        if (status & LastFrag) {
                if (pkt_size > rx_copybreak && tp->new_skb) {
                        memcpy(tp->new_skb->tail, sk_buff[0]->tail,
                                        pkt_size - tp->new_skb->len);

                        skb_put(tp->new_skb, pkt_size - tp->new_skb->len);
                }

                *sk_buff = tp->new_skb;
        }

        rtl8169_return_to_asic(desc, tp->rx_buf_sz);

        return 0;
}



> [...]
> > If I am still not making sense, I can be more verbose.
> 
> Enlight me with the code, just to be sure.

Working (but still hackish) patch:
--- drivers/net/r8169.c.0220    2005-02-20 22:45:07.000000000 -0600
+++ drivers/net/r8169.c 2005-02-20 22:43:27.000000000 -0600
@@ -1663,7 +1663,8 @@ static void rtl8169_free_rx_skb(struct r
 {
        struct pci_dev *pdev = tp->pci_dev;

-       pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
+       //pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->rx_buf_sz,
+       pci_unmap_single(pdev, le64_to_cpu(desc->addr), tp->desc_part+ETH_HLEN,
                         PCI_DMA_FROMDEVICE);
        dev_kfree_skb(*sk_buff);
        *sk_buff = NULL;
@@ -1694,14 +1695,14 @@ static int rtl8169_alloc_rx_skb(struct r
        dma_addr_t mapping;
        int ret = 0;

-       skb = dev_alloc_skb(rx_buf_sz + NET_IP_ALIGN);
+       skb = dev_alloc_skb(tp->desc_part + ETH_HLEN + NET_IP_ALIGN);
        if (!skb)
                goto err_out;

        skb_reserve(skb, NET_IP_ALIGN);
        *sk_buff = skb;

-       mapping = pci_map_single(tp->pci_dev, skb->tail, rx_buf_sz,
+       mapping = pci_map_single(tp->pci_dev, skb->tail, tp->desc_part+ETH_HLEN,
                                 PCI_DMA_FROMDEVICE);

        rtl8169_give_to_asic(desc, mapping, rx_buf_sz);
@@ -2225,7 +2226,7 @@ rtl8169_rx_interrupt(struct net_device *
                        rtl8169_rx_csum(skb, desc);

                        pci_dma_sync_single_for_cpu(tp->pci_dev,
-                               le64_to_cpu(desc->addr), tp->rx_buf_sz,
+                               le64_to_cpu(desc->addr), tp->desc_part+ETH_HLEN,
                                PCI_DMA_FROMDEVICE);

                        if (rtl8169_rx_copy(&skb, pkt_size, desc, tp)) {
@@ -2235,7 +2236,7 @@ rtl8169_rx_interrupt(struct net_device *
                        }

                        pci_action(tp->pci_dev, le64_to_cpu(desc->addr),
-                                  tp->rx_buf_sz, PCI_DMA_FROMDEVICE);
+                                  tp->desc_part+ETH_HLEN, PCI_DMA_FROMDEVICE);

                        if (skb && (status & LastFrag)) {
                                skb->dev = dev;


A more complete patch to follow shortly (as I want to try and fix my tabs problem).