[net-next PATCH 1/5] igb: allow tx of pre-formatted vlan tagged packets

[net-next PATCH 1/5] igb: allow tx of pre-formatted vlan tagged packets

[Jeff Kirsher]

From: Arthur Jones <ajones@riverbed.com>

When the 82575 is fed 802.1q packets, it chokes with
an error of the form:

igb 0000:08:00.1 partial checksum but proto=81!

As the logic there was not smart enough to look into
the vlan header to pick out the encapsulated protocol.

There are times when we'd like to send these packets
out without having to configure a vlan on the interface.
Here we check for the vlan tag and allow the packet to
go out with the correct hardware checksum.

Thanks to Kand Ly <kand@riverbed.com> for discovering the
issue and the coming up with a solution.  This patch is
based upon his work.

Signed-off-by: Arthur Jones <ajones@riverbed.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_main.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 7c4481b..c14d569 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -3008,6 +3008,17 @@ static inline bool igb_tx_csum_adv(struct igb_adapter *adapter,
 		tu_cmd |= (E1000_TXD_CMD_DEXT | E1000_ADVTXD_DTYP_CTXT);
 
 		if (skb->ip_summed == CHECKSUM_PARTIAL) {
+			__be16 protocol;
+
+			if (skb->protocol == cpu_to_be16(ETH_P_8021Q)) {
+				const struct vlan_ethhdr *vhdr =
+				          (const struct vlan_ethhdr*)skb->data;
+
+				protocol = vhdr->h_vlan_encapsulated_proto;
+			} else {
+				protocol = skb->protocol;
+			}
+
 			switch (skb->protocol) {
 			case cpu_to_be16(ETH_P_IP):
 				tu_cmd |= E1000_ADVTXD_TUCMD_IPV4;

[net-next PATCH 2/5] igb: remove IGB_DESC_UNUSED since it is better handled by a function call

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

This patch removes IGB_DESC_UNUSED and replaces it with a function call
instead in order to cleanup some of the ugliness introduced by the macro.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb.h      |    4 ----
 drivers/net/igb/igb_main.c |   25 ++++++++++++++++++-------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/net/igb/igb.h b/drivers/net/igb/igb.h
index e18ac1b..4e8464b 100644
--- a/drivers/net/igb/igb.h
+++ b/drivers/net/igb/igb.h
@@ -182,10 +182,6 @@ struct igb_ring {
 	char name[IFNAMSIZ + 5];
 };
 
-#define IGB_DESC_UNUSED(R) \
-	((((R)->next_to_clean > (R)->next_to_use) ? 0 : (R)->count) + \
-	(R)->next_to_clean - (R)->next_to_use - 1)
-
 #define E1000_RX_DESC_ADV(R, i)	    \
 	(&(((union e1000_adv_rx_desc *)((R).desc))[i]))
 #define E1000_TX_DESC_ADV(R, i)	    \
diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index c14d569..05bb7a7 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -278,6 +278,17 @@ static char *igb_get_time_str(struct igb_adapter *adapter,
 #endif
 
 /**
+ * igb_desc_unused - calculate if we have unused descriptors
+ **/
+static int igb_desc_unused(struct igb_ring *ring)
+{
+	if (ring->next_to_clean > ring->next_to_use)
+		return ring->next_to_clean - ring->next_to_use - 1;
+
+	return ring->count + ring->next_to_clean - ring->next_to_use - 1;
+}
+
+/**
  * igb_init_module - Driver Registration Routine
  *
  * igb_init_module is the first routine called when the driver is
@@ -873,12 +884,12 @@ static void igb_configure(struct igb_adapter *adapter)
 
 	igb_rx_fifo_flush_82575(&adapter->hw);
 
-	/* call IGB_DESC_UNUSED which always leaves
+	/* call igb_desc_unused which always leaves
 	 * at least 1 descriptor unused to make sure
 	 * next_to_use != next_to_clean */
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		struct igb_ring *ring = &adapter->rx_ring[i];
-		igb_alloc_rx_buffers_adv(ring, IGB_DESC_UNUSED(ring));
+		igb_alloc_rx_buffers_adv(ring, igb_desc_unused(ring));
 	}
 
 
@@ -2661,7 +2672,7 @@ link_up:
 	igb_update_adaptive(&adapter->hw);
 
 	if (!netif_carrier_ok(netdev)) {
-		if (IGB_DESC_UNUSED(tx_ring) + 1 < tx_ring->count) {
+		if (igb_desc_unused(tx_ring) + 1 < tx_ring->count) {
 			/* We've lost link, so the controller stops DMA,
 			 * but we've got queued Tx work that's never going
 			 * to get done, so reset controller to flush Tx.
@@ -3199,7 +3210,7 @@ static int __igb_maybe_stop_tx(struct net_device *netdev,
 
 	/* We need to check again in a case another CPU has just
 	 * made room available. */
-	if (IGB_DESC_UNUSED(tx_ring) < size)
+	if (igb_desc_unused(tx_ring) < size)
 		return -EBUSY;
 
 	/* A reprieve! */
@@ -3211,7 +3222,7 @@ static int __igb_maybe_stop_tx(struct net_device *netdev,
 static int igb_maybe_stop_tx(struct net_device *netdev,
 			     struct igb_ring *tx_ring, int size)
 {
-	if (IGB_DESC_UNUSED(tx_ring) >= size)
+	if (igb_desc_unused(tx_ring) >= size)
 		return 0;
 	return __igb_maybe_stop_tx(netdev, tx_ring, size);
 }
@@ -4310,7 +4321,7 @@ static bool igb_clean_tx_irq(struct igb_ring *tx_ring)
 
 	if (unlikely(count &&
 		     netif_carrier_ok(netdev) &&
-		     IGB_DESC_UNUSED(tx_ring) >= IGB_TX_QUEUE_WAKE)) {
+		     igb_desc_unused(tx_ring) >= IGB_TX_QUEUE_WAKE)) {
 		/* Make sure that anybody stopping the queue after this
 		 * sees the new next_to_clean.
 		 */
@@ -4587,7 +4598,7 @@ next_desc:
 	}
 
 	rx_ring->next_to_clean = i;
-	cleaned_count = IGB_DESC_UNUSED(rx_ring);
+	cleaned_count = igb_desc_unused(rx_ring);
 
 	if (cleaned_count)
 		igb_alloc_rx_buffers_adv(rx_ring, cleaned_count);

[net-next PATCH 3/5] igb: update driver to use setup_timer function

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

igb was previously setting up all of the timer members itself.  It is
easier to just call setup_timer and reduce the calls to one line.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_main.c |   11 ++++-------
 1 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 05bb7a7..3dd3a5a 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -1312,13 +1312,10 @@ static int __devinit igb_probe(struct pci_dev *pdev,
 		goto err_eeprom;
 	}
 
-	init_timer(&adapter->watchdog_timer);
-	adapter->watchdog_timer.function = &igb_watchdog;
-	adapter->watchdog_timer.data = (unsigned long) adapter;
-
-	init_timer(&adapter->phy_info_timer);
-	adapter->phy_info_timer.function = &igb_update_phy_info;
-	adapter->phy_info_timer.data = (unsigned long) adapter;
+	setup_timer(&adapter->watchdog_timer, &igb_watchdog,
+	            (unsigned long) adapter);
+	setup_timer(&adapter->phy_info_timer, &igb_update_phy_info,
+	            (unsigned long) adapter);
 
 	INIT_WORK(&adapter->reset_task, igb_reset_task);
 	INIT_WORK(&adapter->watchdog_task, igb_watchdog_task);

[net-next PATCH 4/5] igb: rework igb_set_multi so that vfs are properly updated

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

Currently if there are no multicast addresses programmed into the PF then
the VFs cannot have their multicast filters reset.  This change makes it so
the code path that updates vf multicast is always called along with the pf
updates.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_main.c |   18 ++++++++----------
 1 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index 3dd3a5a..a977bd7 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -2464,7 +2464,7 @@ static void igb_set_multi(struct net_device *netdev)
 	struct e1000_hw *hw = &adapter->hw;
 	struct e1000_mac_info *mac = &hw->mac;
 	struct dev_mc_list *mc_ptr;
-	u8  *mta_list;
+	u8  *mta_list = NULL;
 	u32 rctl;
 	int i;
 
@@ -2485,17 +2485,15 @@ static void igb_set_multi(struct net_device *netdev)
 	}
 	wr32(E1000_RCTL, rctl);
 
-	if (!netdev->mc_count) {
-		/* nothing to program, so clear mc list */
-		igb_update_mc_addr_list(hw, NULL, 0, 1,
-					mac->rar_entry_count);
-		return;
+	if (netdev->mc_count) {
+		mta_list = kzalloc(netdev->mc_count * 6, GFP_ATOMIC);
+		if (!mta_list) {
+			dev_err(&adapter->pdev->dev,
+			        "failed to allocate multicast filter list\n");
+			return;
+		}
 	}
 
-	mta_list = kzalloc(netdev->mc_count * 6, GFP_ATOMIC);
-	if (!mta_list)
-		return;
-
 	/* The shared function expects a packed array of only addresses. */
 	mc_ptr = netdev->mc_list;
 

[net-next PATCH 5/5] igb: cleanup tx dma so map & unmap use matching calls

[Jeff Kirsher]

From: Alexander Duyck <alexander.h.duyck@intel.com>

The igb driver was using map_single to map the skbs and then unmap_page to
unmap them.  This update changes that so instead uses skb_dma_map and
skb_dma_unmap.

In addition the next_to_watch member of the buffer_info struct was being
set uneccesarily.  I removed the spots where it was set without being needed.

Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
---

 drivers/net/igb/igb_main.c |   66 +++++++++++++++++++++++---------------------
 1 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
index a977bd7..d6f7af8 100644
--- a/drivers/net/igb/igb_main.c
+++ b/drivers/net/igb/igb_main.c
@@ -2257,19 +2257,14 @@ static void igb_free_all_tx_resources(struct igb_adapter *adapter)
 static void igb_unmap_and_free_tx_resource(struct igb_adapter *adapter,
 					   struct igb_buffer *buffer_info)
 {
-	if (buffer_info->dma) {
-		pci_unmap_page(adapter->pdev,
-				buffer_info->dma,
-				buffer_info->length,
-				PCI_DMA_TODEVICE);
-		buffer_info->dma = 0;
-	}
+	buffer_info->dma = 0;
 	if (buffer_info->skb) {
+		skb_dma_unmap(&adapter->pdev->dev, buffer_info->skb,
+		              DMA_TO_DEVICE);
 		dev_kfree_skb_any(buffer_info->skb);
 		buffer_info->skb = NULL;
 	}
 	buffer_info->time_stamp = 0;
-	buffer_info->next_to_watch = 0;
 	/* buffer_info must be completely set up in the transmit path */
 }
 
@@ -3078,25 +3073,33 @@ static inline int igb_tx_map_adv(struct igb_adapter *adapter,
 	unsigned int len = skb_headlen(skb);
 	unsigned int count = 0, i;
 	unsigned int f;
+	dma_addr_t *map;
 
 	i = tx_ring->next_to_use;
 
+	if (skb_dma_map(&adapter->pdev->dev, skb, DMA_TO_DEVICE)) {
+		dev_err(&adapter->pdev->dev, "TX DMA map failed\n");
+		return 0;
+	}
+
+	map = skb_shinfo(skb)->dma_maps;
+
 	buffer_info = &tx_ring->buffer_info[i];
 	BUG_ON(len >= IGB_MAX_DATA_PER_TXD);
 	buffer_info->length = len;
 	/* set time_stamp *before* dma to help avoid a possible race */
 	buffer_info->time_stamp = jiffies;
 	buffer_info->next_to_watch = i;
-	buffer_info->dma = pci_map_single(adapter->pdev, skb->data, len,
-					  PCI_DMA_TODEVICE);
+	buffer_info->dma = map[count];
 	count++;
-	i++;
-	if (i == tx_ring->count)
-		i = 0;
 
 	for (f = 0; f < skb_shinfo(skb)->nr_frags; f++) {
 		struct skb_frag_struct *frag;
 
+		i++;
+		if (i == tx_ring->count)
+			i = 0;
+
 		frag = &skb_shinfo(skb)->frags[f];
 		len = frag->size;
 
@@ -3105,19 +3108,10 @@ static inline int igb_tx_map_adv(struct igb_adapter *adapter,
 		buffer_info->length = len;
 		buffer_info->time_stamp = jiffies;
 		buffer_info->next_to_watch = i;
-		buffer_info->dma = pci_map_page(adapter->pdev,
-						frag->page,
-						frag->page_offset,
-						len,
-						PCI_DMA_TODEVICE);
-
+		buffer_info->dma = map[count];
 		count++;
-		i++;
-		if (i == tx_ring->count)
-			i = 0;
 	}
 
-	i = ((i == 0) ? tx_ring->count - 1 : i - 1);
 	tx_ring->buffer_info[i].skb = skb;
 	tx_ring->buffer_info[first].next_to_watch = i;
 
@@ -3230,6 +3224,7 @@ static int igb_xmit_frame_ring_adv(struct sk_buff *skb,
 	unsigned int first;
 	unsigned int tx_flags = 0;
 	u8 hdr_len = 0;
+	int count = 0;
 	int tso = 0;
 	union skb_shared_tx *shtx;
 
@@ -3291,14 +3286,23 @@ static int igb_xmit_frame_ring_adv(struct sk_buff *skb,
 	         (skb->ip_summed == CHECKSUM_PARTIAL))
 		tx_flags |= IGB_TX_FLAGS_CSUM;
 
-	igb_tx_queue_adv(adapter, tx_ring, tx_flags,
-			 igb_tx_map_adv(adapter, tx_ring, skb, first),
-			 skb->len, hdr_len);
-
-	netdev->trans_start = jiffies;
-
-	/* Make sure there is space in the ring for the next send. */
-	igb_maybe_stop_tx(netdev, tx_ring, MAX_SKB_FRAGS + 4);
+	/*
+	 * count reflects descriptors mapped, if 0 then mapping error
+	 * has occured and we need to rewind the descriptor queue
+	 */
+	count = igb_tx_map_adv(adapter, tx_ring, skb, first);
+
+	if (count) {
+		igb_tx_queue_adv(adapter, tx_ring, tx_flags, count,
+			         skb->len, hdr_len);
+		netdev->trans_start = jiffies;
+		/* Make sure there is space in the ring for the next send. */
+		igb_maybe_stop_tx(netdev, tx_ring, MAX_SKB_FRAGS + 4);
+	} else {
+		dev_kfree_skb_any(skb);
+		tx_ring->buffer_info[first].time_stamp = 0;
+		tx_ring->next_to_use = first;
+	}
 
 	return NETDEV_TX_OK;
 }

Re: [net-next PATCH 1/5] igb: allow tx of pre-formatted vlan tagged packets

[Arthur Jones]

Hi Jeff, ...

On Fri, Mar 20, 2009 at 03:16:31AM -0700, Jeff Kirsher wrote:
>  [...]
> 
>  drivers/net/igb/igb_main.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
> index 7c4481b..c14d569 100644
> --- a/drivers/net/igb/igb_main.c
> +++ b/drivers/net/igb/igb_main.c
> @@ -3008,6 +3008,17 @@ static inline bool igb_tx_csum_adv(struct igb_adapter *adapter,
>  		tu_cmd |= (E1000_TXD_CMD_DEXT | E1000_ADVTXD_DTYP_CTXT);
>  
>  		if (skb->ip_summed == CHECKSUM_PARTIAL) {
> +			__be16 protocol;
> +
> +			if (skb->protocol == cpu_to_be16(ETH_P_8021Q)) {
> +				const struct vlan_ethhdr *vhdr =
> +				          (const struct vlan_ethhdr*)skb->data;
> +
> +				protocol = vhdr->h_vlan_encapsulated_proto;
> +			} else {
> +				protocol = skb->protocol;
> +			}
> +
>  			switch (skb->protocol) {
                                ^^^^^^^^^^^^^
This has the same merge error as the e1000e version,
it should be just protocol here, not skb->protocol...

Arthur

Re: [net-next PATCH 1/5] igb: allow tx of pre-formatted vlan tagged packets

[Jeff Kirsher]

On Fri, Mar 20, 2009 at 8:27 AM, Arthur Jones <ajones@riverbed.com> wrote:
> Hi Jeff, ...
>
> On Fri, Mar 20, 2009 at 03:16:31AM -0700, Jeff Kirsher wrote:
>>  [...]
>>
>>  drivers/net/igb/igb_main.c |   11 +++++++++++
>>  1 files changed, 11 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/net/igb/igb_main.c b/drivers/net/igb/igb_main.c
>> index 7c4481b..c14d569 100644
>> --- a/drivers/net/igb/igb_main.c
>> +++ b/drivers/net/igb/igb_main.c
>> @@ -3008,6 +3008,17 @@ static inline bool igb_tx_csum_adv(struct igb_adapter *adapter,
>>               tu_cmd |= (E1000_TXD_CMD_DEXT | E1000_ADVTXD_DTYP_CTXT);
>>
>>               if (skb->ip_summed == CHECKSUM_PARTIAL) {
>> +                     __be16 protocol;
>> +
>> +                     if (skb->protocol == cpu_to_be16(ETH_P_8021Q)) {
>> +                             const struct vlan_ethhdr *vhdr =
>> +                                       (const struct vlan_ethhdr*)skb->data;
>> +
>> +                             protocol = vhdr->h_vlan_encapsulated_proto;
>> +                     } else {
>> +                             protocol = skb->protocol;
>> +                     }
>> +
>>                       switch (skb->protocol) {
>                                ^^^^^^^^^^^^^
> This has the same merge error as the e1000e version,
> it should be just protocol here, not skb->protocol...
>
> Arthur
> --

Sorry, when I merged your patch into my tree, that is when the merge
error occurred.

-- 
Cheers,
Jeff

Re: [net-next PATCH 1/5] igb: allow tx of pre-formatted vlan tagged packets

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 20 Mar 2009 03:16:31 -0700

> When the 82575 is fed 802.1q packets, it chokes with
> an error of the form:
> 
> igb 0000:08:00.1 partial checksum but proto=81!
 ...
> Signed-off-by: Arthur Jones <ajones@riverbed.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
> +
>  			switch (skb->protocol) {
>  			case cpu_to_be16(ETH_P_IP):
>  				tu_cmd |= E1000_ADVTXD_TUCMD_IPV4;

Applied with skb->protocol fixed to be just plain 'protocol'.

Re: [net-next PATCH 2/5] igb: remove IGB_DESC_UNUSED since it is better handled by a function call

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 20 Mar 2009 03:16:50 -0700

> This patch removes IGB_DESC_UNUSED and replaces it with a function call
> instead in order to cleanup some of the ugliness introduced by the macro.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.

Re: [net-next PATCH 3/5] igb: update driver to use setup_timer function

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 20 Mar 2009 03:17:08 -0700

> igb was previously setting up all of the timer members itself.  It is
> easier to just call setup_timer and reduce the calls to one line.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.

Re: [net-next PATCH 4/5] igb: rework igb_set_multi so that vfs are properly updated

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 20 Mar 2009 03:17:25 -0700

> Currently if there are no multicast addresses programmed into the PF then
> the VFs cannot have their multicast filters reset.  This change makes it so
> the code path that updates vf multicast is always called along with the pf
> updates.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.

Re: [net-next PATCH 5/5] igb: cleanup tx dma so map & unmap use matching calls

[David Miller]

From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Fri, 20 Mar 2009 03:17:43 -0700

> The igb driver was using map_single to map the skbs and then unmap_page to
> unmap them.  This update changes that so instead uses skb_dma_map and
> skb_dma_unmap.
> 
> In addition the next_to_watch member of the buffer_info struct was being
> set uneccesarily.  I removed the spots where it was set without being needed.
> 
> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com>
> Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com>

Applied.