On Sunday 20 February 2005 05:34 pm, Francois Romieu wrote:
[...]

quoted

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.  

quoted

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

[...]

quoted

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