[net-next PATCH 0/6] enic: updates to version 1.1.0.241

[net-next PATCH 0/6] enic: updates to version 1.1.0.241

[Scott Feldman]

The following series implements enic driver updates:

1/6 - Bug fix: use safe queue shutdown in dev->stop
2/6 - Bug fix: try harder to fill Rx ring on skb allocation failures
3/6 - minimize pkt filter updates to firmware
4/6 - Bug fix: align desc ring sizes to 32 descs
5/6 - feature add: add ethtool -c/C support
6/6 - whitespace cleanup; #define cleanup; more verbose err msg

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>

[net-next PATCH 1/6] enic: Bug fix: use safe queue shutdown in dev->stop

[Scott Feldman]

enic: Bug fix: use safe queue shutdown in dev->stop

Fix dev->stop shutdown bug where driver was stopping xmit queue and then
disabling intrs.  Fix is to disable intrs first and then stop the xmit
queue, otherwise an interrupt could cause the queue to be rewoken.  Also,
no need to explicitly do queue servicing because queues are cleaned and
reset back to initial state at end of dev->stop.  Servicing queues also
had the side-effect of also rewakening the xmit queue, which is not what
we want.

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
---
 drivers/net/enic/enic.h      |    2 +-
 drivers/net/enic/enic_main.c |   44 +++++++-----------------------------------
 2 files changed, 8 insertions(+), 38 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index e1c2076..dfd6024 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -34,7 +34,7 @@
 
 #define DRV_NAME		"enic"
 #define DRV_DESCRIPTION		"Cisco 10G Ethernet Driver"
-#define DRV_VERSION		"1.1.0.100"
+#define DRV_VERSION		"1.1.0.241"
 #define DRV_COPYRIGHT		"Copyright 2008-2009 Cisco Systems, Inc"
 #define PFX			DRV_NAME ": "
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index f875751..496e8b6 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -24,6 +24,7 @@
 #include <linux/types.h>
 #include <linux/init.h>
 #include <linux/workqueue.h>
+#include <linux/delay.h>
 #include <linux/pci.h>
 #include <linux/netdevice.h>
 #include <linux/etherdevice.h>
@@ -1084,34 +1085,6 @@ static int enic_rq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc,
 	return 0;
 }
 
-static void enic_rq_drop_buf(struct vnic_rq *rq,
-	struct cq_desc *cq_desc, struct vnic_rq_buf *buf,
-	int skipped, void *opaque)
-{
-	struct enic *enic = vnic_dev_priv(rq->vdev);
-	struct sk_buff *skb = buf->os_buf;
-
-	if (skipped)
-		return;
-
-	pci_unmap_single(enic->pdev, buf->dma_addr,
-		buf->len, PCI_DMA_FROMDEVICE);
-
-	dev_kfree_skb_any(skb);
-}
-
-static int enic_rq_service_drop(struct vnic_dev *vdev, struct cq_desc *cq_desc,
-	u8 type, u16 q_number, u16 completed_index, void *opaque)
-{
-	struct enic *enic = vnic_dev_priv(vdev);
-
-	vnic_rq_service(&enic->rq[q_number], cq_desc,
-		completed_index, VNIC_RQ_RETURN_DESC,
-		enic_rq_drop_buf, opaque);
-
-	return 0;
-}
-
 static int enic_poll(struct napi_struct *napi, int budget)
 {
 	struct enic *enic = container_of(napi, struct enic, napi);
@@ -1409,16 +1382,18 @@ static int enic_stop(struct net_device *netdev)
 	unsigned int i;
 	int err;
 
+	for (i = 0; i < enic->intr_count; i++)
+		vnic_intr_mask(&enic->intr[i]);
+
 	del_timer_sync(&enic->notify_timer);
 
 	spin_lock(&enic->devcmd_lock);
 	vnic_dev_disable(enic->vdev);
 	spin_unlock(&enic->devcmd_lock);
 	napi_disable(&enic->napi);
-	netif_stop_queue(netdev);
-
-	for (i = 0; i < enic->intr_count; i++)
-		vnic_intr_mask(&enic->intr[i]);
+	netif_tx_disable(netdev);
+	msleep(10);
+	netif_carrier_off(netdev);
 
 	for (i = 0; i < enic->wq_count; i++) {
 		err = vnic_wq_disable(&enic->wq[i]);
@@ -1436,11 +1411,6 @@ static int enic_stop(struct net_device *netdev)
 	spin_unlock(&enic->devcmd_lock);
 	enic_free_intr(enic);
 
-	(void)vnic_cq_service(&enic->cq[ENIC_CQ_RQ],
-		-1, enic_rq_service_drop, NULL);
-	(void)vnic_cq_service(&enic->cq[ENIC_CQ_WQ],
-		-1, enic_wq_service, NULL);
-
 	for (i = 0; i < enic->wq_count; i++)
 		vnic_wq_clean(&enic->wq[i], enic_free_wq_buf);
 	for (i = 0; i < enic->rq_count; i++)

Re: [net-next PATCH 1/6] enic: Bug fix: use safe queue shutdown in dev->stop

[Ben Hutchings]

On Fri, 2009-12-18 at 18:09 -0800, Scott Feldman wrote:
> enic: Bug fix: use safe queue shutdown in dev->stop
> 
> Fix dev->stop shutdown bug where driver was stopping xmit queue and then
> disabling intrs.  Fix is to disable intrs first and then stop the xmit
> queue, otherwise an interrupt could cause the queue to be rewoken.  Also,
> no need to explicitly do queue servicing because queues are cleaned and
> reset back to initial state at end of dev->stop.  Servicing queues also
> had the side-effect of also rewakening the xmit queue, which is not what
> we want.
[...]
> @@ -1409,16 +1382,18 @@ static int enic_stop(struct net_device *netdev)
>  	unsigned int i;
>  	int err;
>  
> +	for (i = 0; i < enic->intr_count; i++)
> +		vnic_intr_mask(&enic->intr[i]);
> +

I think you need to use synchronize_irq() after this.

>  	del_timer_sync(&enic->notify_timer);
>  
>  	spin_lock(&enic->devcmd_lock);
>  	vnic_dev_disable(enic->vdev);
>  	spin_unlock(&enic->devcmd_lock);
>  	napi_disable(&enic->napi);
> -	netif_stop_queue(netdev);
> -
> -	for (i = 0; i < enic->intr_count; i++)
> -		vnic_intr_mask(&enic->intr[i]);
> +	netif_tx_disable(netdev);
> +	msleep(10);

This sleep is suspicious.

> +	netif_carrier_off(netdev);
[...]

And this should be unnecessary.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [net-next PATCH 1/6] enic: Bug fix: use safe queue shutdown in dev->stop

[Scott Feldman]

On 12/19/09 7:34 AM, "Ben Hutchings" <bhutchings@solarflare.com> wrote:

> On Fri, 2009-12-18 at 18:09 -0800, Scott Feldman wrote:
>> enic: Bug fix: use safe queue shutdown in dev->stop
>> 
>> Fix dev->stop shutdown bug where driver was stopping xmit queue and then
>> disabling intrs.  Fix is to disable intrs first and then stop the xmit
>> queue, otherwise an interrupt could cause the queue to be rewoken.  Also,
>> no need to explicitly do queue servicing because queues are cleaned and
>> reset back to initial state at end of dev->stop.  Servicing queues also
>> had the side-effect of also rewakening the xmit queue, which is not what
>> we want.
> [...]
>> @@ -1409,16 +1382,18 @@ static int enic_stop(struct net_device *netdev)
>> unsigned int i;
>> int err;
>>  
>> + for (i = 0; i < enic->intr_count; i++)
>> +  vnic_intr_mask(&enic->intr[i]);
>> +
> 
> I think you need to use synchronize_irq() after this.

I think you're right.  That's what I want: all intrs masked and no IRQs
running.  I also need to add a PIO read to flush the PIO write to mask.

I want intrs shut down from here on.
 
>> del_timer_sync(&enic->notify_timer);
>>  
>> spin_lock(&enic->devcmd_lock);
>> vnic_dev_disable(enic->vdev);
>> spin_unlock(&enic->devcmd_lock);
>> napi_disable(&enic->napi);
>> - netif_stop_queue(netdev);
>> -
>> - for (i = 0; i < enic->intr_count; i++)
>> -  vnic_intr_mask(&enic->intr[i]);
>> + netif_tx_disable(netdev);
>> + msleep(10);
> 
> This sleep is suspicious.

Ya, that can go.  Netif_tx_disable grabs netif_tx_lock, so we should be
synced with xmit_hard_start.  Forgot to delete that when we had
netif_stop_queue.
 
>> + netif_carrier_off(netdev);
> [...]
> 
> And this should be unnecessary.

We don't have any way to get true link status when the interface is down, so
this forces us to always report link status down when the interface is down.

-scott

[net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Scott Feldman]

enic: Bug fix: try harder to fill Rx ring on skb allocation failures

During probe(), make sure we get at least one skb on the Rx ring. 
Otherwise abort the interface load.  Also, if we get skb allocation
failures in NAPI poll while trying to replenish the ring, try again
later so we don't end up starving out the Rx ring completely.

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
---
 drivers/net/enic/enic_main.c |   54 ++++++++++++++++++++++++++----------------
 1 files changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 496e8b6..0265b25 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -1092,6 +1092,7 @@ static int enic_poll(struct napi_struct *napi, int budget)
 	unsigned int rq_work_to_do = budget;
 	unsigned int wq_work_to_do = -1; /* no limit */
 	unsigned int  work_done, rq_work_done, wq_work_done;
+	int err;
 
 	/* Service RQ (first) and WQ
 	 */
@@ -1115,16 +1116,19 @@ static int enic_poll(struct napi_struct *napi, int budget)
 			0 /* don't unmask intr */,
 			0 /* don't reset intr timer */);
 
-	if (rq_work_done > 0) {
+	err = vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
 
-		/* Replenish RQ
-		 */
+	/* Buffer allocation failed. Stay in polling
+	 * mode so we can try to fill the ring again.
+	 */
 
-		vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
+	if (err)
+		rq_work_done = rq_work_to_do;
 
-	} else {
+	if (rq_work_done < rq_work_to_do) {
 
-		/* If no work done, flush all LROs and exit polling
+		/* Some work done, but not enough to stay in polling,
+		 * flush all LROs and exit polling
 		 */
 
 		if (netdev->features & NETIF_F_LRO)
@@ -1143,6 +1147,7 @@ static int enic_poll_msix(struct napi_struct *napi, int budget)
 	struct net_device *netdev = enic->netdev;
 	unsigned int work_to_do = budget;
 	unsigned int work_done;
+	int err;
 
 	/* Service RQ
 	 */
@@ -1150,25 +1155,30 @@ static int enic_poll_msix(struct napi_struct *napi, int budget)
 	work_done = vnic_cq_service(&enic->cq[ENIC_CQ_RQ],
 		work_to_do, enic_rq_service, NULL);
 
-	if (work_done > 0) {
-
-		/* Replenish RQ
-		 */
-
-		vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
-
-		/* Return intr event credits for this polling
-		 * cycle.  An intr event is the completion of a
-		 * RQ packet.
-		 */
+	/* Return intr event credits for this polling
+	 * cycle.  An intr event is the completion of a
+	 * RQ packet.
+	 */
 
+	if (work_done > 0)
 		vnic_intr_return_credits(&enic->intr[ENIC_MSIX_RQ],
 			work_done,
 			0 /* don't unmask intr */,
 			0 /* don't reset intr timer */);
-	} else {
 
-		/* If no work done, flush all LROs and exit polling
+	err = vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
+
+	/* Buffer allocation failed. Stay in polling mode
+	 * so we can try to fill the ring again.
+	 */
+
+	if (err)
+		work_done = work_to_do;
+
+	if (work_done < work_to_do) {
+
+		/* Some work done, but not enough to stay in polling,
+		 * flush all LROs and exit polling
 		 */
 
 		if (netdev->features & NETIF_F_LRO)
@@ -1333,11 +1343,13 @@ static int enic_open(struct net_device *netdev)
 	}
 
 	for (i = 0; i < enic->rq_count; i++) {
-		err = vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
-		if (err) {
+		vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
+		/* Need at least one buffer on ring to get going */
+		if (vnic_rq_desc_used(&enic->rq[i]) == 0) {
 			printk(KERN_ERR PFX
 				"%s: Unable to alloc receive buffers.\n",
 				netdev->name);
+			err = -ENOMEM;
 			goto err_out_notify_unset;
 		}
 	}

Re: [net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Simon Horman]

On Fri, Dec 18, 2009 at 06:09:46PM -0800, Scott Feldman wrote:
> enic: Bug fix: try harder to fill Rx ring on skb allocation failures
> 
> During probe(), make sure we get at least one skb on the Rx ring. 
> Otherwise abort the interface load.  Also, if we get skb allocation
> failures in NAPI poll while trying to replenish the ring, try again
> later so we don't end up starving out the Rx ring completely.
> 
> Signed-off-by: Scott Feldman <scofeldm@cisco.com>
> Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
> ---
>  drivers/net/enic/enic_main.c |   54 ++++++++++++++++++++++++++----------------
>  1 files changed, 33 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 496e8b6..0265b25 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -1092,6 +1092,7 @@ static int enic_poll(struct napi_struct *napi, int budget)
>  	unsigned int rq_work_to_do = budget;
>  	unsigned int wq_work_to_do = -1; /* no limit */
>  	unsigned int  work_done, rq_work_done, wq_work_done;
> +	int err;
>  
>  	/* Service RQ (first) and WQ
>  	 */
> @@ -1115,16 +1116,19 @@ static int enic_poll(struct napi_struct *napi, int budget)
>  			0 /* don't unmask intr */,
>  			0 /* don't reset intr timer */);
>  
> -	if (rq_work_done > 0) {
> +	err = vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
>  
> -		/* Replenish RQ
> -		 */
> +	/* Buffer allocation failed. Stay in polling
> +	 * mode so we can try to fill the ring again.
> +	 */
>  
> -		vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
> +	if (err)
> +		rq_work_done = rq_work_to_do;

Is it intentional for rq_work_done = rq_work_to_do to become the
return value?

>  
> -	} else {
> +	if (rq_work_done < rq_work_to_do) {
>  
> -		/* If no work done, flush all LROs and exit polling
> +		/* Some work done, but not enough to stay in polling,
> +		 * flush all LROs and exit polling
>  		 */
>  
>  		if (netdev->features & NETIF_F_LRO)
> @@ -1143,6 +1147,7 @@ static int enic_poll_msix(struct napi_struct *napi, int budget)
>  	struct net_device *netdev = enic->netdev;
>  	unsigned int work_to_do = budget;
>  	unsigned int work_done;
> +	int err;
>  
>  	/* Service RQ
>  	 */
> @@ -1150,25 +1155,30 @@ static int enic_poll_msix(struct napi_struct *napi, int budget)
>  	work_done = vnic_cq_service(&enic->cq[ENIC_CQ_RQ],
>  		work_to_do, enic_rq_service, NULL);
>  
> -	if (work_done > 0) {
> -
> -		/* Replenish RQ
> -		 */
> -
> -		vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
> -
> -		/* Return intr event credits for this polling
> -		 * cycle.  An intr event is the completion of a
> -		 * RQ packet.
> -		 */
> +	/* Return intr event credits for this polling
> +	 * cycle.  An intr event is the completion of a
> +	 * RQ packet.
> +	 */
>  
> +	if (work_done > 0)
>  		vnic_intr_return_credits(&enic->intr[ENIC_MSIX_RQ],
>  			work_done,
>  			0 /* don't unmask intr */,
>  			0 /* don't reset intr timer */);
> -	} else {
>  
> -		/* If no work done, flush all LROs and exit polling
> +	err = vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
> +
> +	/* Buffer allocation failed. Stay in polling mode
> +	 * so we can try to fill the ring again.
> +	 */
> +
> +	if (err)
> +		work_done = work_to_do;

Again, is it intended for this to be the return value of the function?

> +
> +	if (work_done < work_to_do) {
> +
> +		/* Some work done, but not enough to stay in polling,
> +		 * flush all LROs and exit polling
>  		 */
>  
>  		if (netdev->features & NETIF_F_LRO)
> @@ -1333,11 +1343,13 @@ static int enic_open(struct net_device *netdev)
>  	}
>  
>  	for (i = 0; i < enic->rq_count; i++) {
> -		err = vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
> -		if (err) {
> +		vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
> +		/* Need at least one buffer on ring to get going */
> +		if (vnic_rq_desc_used(&enic->rq[i]) == 0) {
>  			printk(KERN_ERR PFX
>  				"%s: Unable to alloc receive buffers.\n",
>  				netdev->name);
> +			err = -ENOMEM;
>  			goto err_out_notify_unset;
>  		}
>  	}

My brain may well have switched off for the day,
but its unclear to me how &enic->rq[i] could ever be NULL.

Also, in the case where a failure occurs for i > 0,
it it necessary to unwind the previous rq allocations?

Re: [net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Scott Feldman]

On 12/19/09 2:41 AM, "Simon Horman" <horms@verge.net.au> wrote:

> On Fri, Dec 18, 2009 at 06:09:46PM -0800, Scott Feldman wrote:
>> enic: Bug fix: try harder to fill Rx ring on skb allocation failures
>> 
>> During probe(), make sure we get at least one skb on the Rx ring.
>> Otherwise abort the interface load.  Also, if we get skb allocation
>> failures in NAPI poll while trying to replenish the ring, try again
>> later so we don't end up starving out the Rx ring completely.
>> 
>> @@ -1115,16 +1116,19 @@ static int enic_poll(struct napi_struct *napi, int
>> budget)
>> 0 /* don't unmask intr */,
>> 0 /* don't reset intr timer */);
>>  
>> - if (rq_work_done > 0) {
>> + err = vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
>>  
>> -  /* Replenish RQ
>> -   */
>> + /* Buffer allocation failed. Stay in polling
>> +  * mode so we can try to fill the ring again.
>> +  */
>>  
>> -  vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
>> + if (err)
>> +  rq_work_done = rq_work_to_do;
> 
> Is it intentional for rq_work_done = rq_work_to_do to become the
> return value?

That was intentional.  If the replacement skb allocation fails, we're
returning like we did a full budget's worth of work so we stay scheduled and
hopefully the next polling pass we'll get the allocations.  Before this fix,
there is a corner case which isn't covered: if hw has used all descs and
gens an intr and we get into polling and the replacement alloc fails, then
Rx is hung.  Hw is desc starved and we're not going to get any more intrs:
game over.

I was looking at tg3.c and it looks like it does napi_schedule() if the
allocation fails.  Would this be a better option than setting work_done =
budget?
 
>> @@ -1333,11 +1343,13 @@ static int enic_open(struct net_device *netdev)
>> }
>>  
>> for (i = 0; i < enic->rq_count; i++) {
>> -  err = vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
>> -  if (err) {
>> +  vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
>> +  /* Need at least one buffer on ring to get going */
>> +  if (vnic_rq_desc_used(&enic->rq[i]) == 0) {
>> printk(KERN_ERR PFX
>> "%s: Unable to alloc receive buffers.\n",
>> netdev->name);
>> +   err = -ENOMEM;
>> goto err_out_notify_unset;
>> }
>> }
> 
> My brain may well have switched off for the day,
> but its unclear to me how &enic->rq[i] could ever be NULL.

You missed a ")"
 
> Also, in the case where a failure occurs for i > 0,
> it it necessary to unwind the previous rq allocations?

Yes, good catch.  We'll fix that.

-scott

Re: [net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Simon Horman]

On Sat, Dec 19, 2009 at 12:39:03PM -0800, Scott Feldman wrote:
> On 12/19/09 2:41 AM, "Simon Horman" <horms@verge.net.au> wrote:
> 
> > On Fri, Dec 18, 2009 at 06:09:46PM -0800, Scott Feldman wrote:
> >> enic: Bug fix: try harder to fill Rx ring on skb allocation failures
> >> 
> >> During probe(), make sure we get at least one skb on the Rx ring.
> >> Otherwise abort the interface load.  Also, if we get skb allocation
> >> failures in NAPI poll while trying to replenish the ring, try again
> >> later so we don't end up starving out the Rx ring completely.
> >> 
> >> @@ -1115,16 +1116,19 @@ static int enic_poll(struct napi_struct *napi, int
> >> budget)
> >> 0 /* don't unmask intr */,
> >> 0 /* don't reset intr timer */);
> >>  
> >> - if (rq_work_done > 0) {
> >> + err = vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
> >>  
> >> -  /* Replenish RQ
> >> -   */
> >> + /* Buffer allocation failed. Stay in polling
> >> +  * mode so we can try to fill the ring again.
> >> +  */
> >>  
> >> -  vnic_rq_fill(&enic->rq[0], enic->rq_alloc_buf);
> >> + if (err)
> >> +  rq_work_done = rq_work_to_do;
> > 
> > Is it intentional for rq_work_done = rq_work_to_do to become the
> > return value?
> 
> That was intentional.  If the replacement skb allocation fails, we're
> returning like we did a full budget's worth of work so we stay scheduled and
> hopefully the next polling pass we'll get the allocations.  Before this fix,
> there is a corner case which isn't covered: if hw has used all descs and
> gens an intr and we get into polling and the replacement alloc fails, then
> Rx is hung.  Hw is desc starved and we're not going to get any more intrs:
> game over.

Ok, understood. Though doesn't this fix just narrow the scope for that
failure? It seems that it could still occur in a pathological case.

> I was looking at tg3.c and it looks like it does napi_schedule() if the
> allocation fails.  Would this be a better option than setting work_done =
> budget?

I don't think that calling napi_schedule() would help for this case as
it looks like it only schedules the poll routine if its not already running.

>  
> >> @@ -1333,11 +1343,13 @@ static int enic_open(struct net_device *netdev)
> >> }
> >>  
> >> for (i = 0; i < enic->rq_count; i++) {
> >> -  err = vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
> >> -  if (err) {
> >> +  vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
> >> +  /* Need at least one buffer on ring to get going */
> >> +  if (vnic_rq_desc_used(&enic->rq[i]) == 0) {
> >> printk(KERN_ERR PFX
> >> "%s: Unable to alloc receive buffers.\n",
> >> netdev->name);
> >> +   err = -ENOMEM;
> >> goto err_out_notify_unset;
> >> }
> >> }
> > 
> > My brain may well have switched off for the day,
> > but its unclear to me how &enic->rq[i] could ever be NULL.
> 
> You missed a ")"

Yes, I did. And the more I stared at it, the more I missed it :-(

>  
> > Also, in the case where a failure occurs for i > 0,
> > it it necessary to unwind the previous rq allocations?
> 
> Yes, good catch.  We'll fix that.
> 
> -scott
> 
> --
> 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

Re: [net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Scott Feldman]

On 12/19/09 5:15 PM, "Simon Horman" <horms@verge.net.au> wrote:

> On Sat, Dec 19, 2009 at 12:39:03PM -0800, Scott Feldman wrote:
>>>> +  rq_work_done = rq_work_to_do;
>>> 
>>> Is it intentional for rq_work_done = rq_work_to_do to become the
>>> return value?
>> 
>> That was intentional.  If the replacement skb allocation fails, we're
>> returning like we did a full budget's worth of work so we stay scheduled and
>> hopefully the next polling pass we'll get the allocations.  Before this fix,
>> there is a corner case which isn't covered: if hw has used all descs and
>> gens an intr and we get into polling and the replacement alloc fails, then
>> Rx is hung.  Hw is desc starved and we're not going to get any more intrs:
>> game over.
> 
> Ok, understood. Though doesn't this fix just narrow the scope for that
> failure? It seems that it could still occur in a pathological case.

Oh!  The intention was to fix it absolutely.  Please expand on the
pathological case your thinking about.  We need this 100% solved, but now
you've got me worried...  ;)

-scott

Re: [net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Simon Horman]

On Sat, Dec 19, 2009 at 05:29:43PM -0800, Scott Feldman wrote:
> On 12/19/09 5:15 PM, "Simon Horman" <horms@verge.net.au> wrote:
> 
> > On Sat, Dec 19, 2009 at 12:39:03PM -0800, Scott Feldman wrote:
> >>>> +  rq_work_done = rq_work_to_do;
> >>> 
> >>> Is it intentional for rq_work_done = rq_work_to_do to become the
> >>> return value?
> >> 
> >> That was intentional.  If the replacement skb allocation fails, we're
> >> returning like we did a full budget's worth of work so we stay scheduled and
> >> hopefully the next polling pass we'll get the allocations.  Before this fix,
> >> there is a corner case which isn't covered: if hw has used all descs and
> >> gens an intr and we get into polling and the replacement alloc fails, then
> >> Rx is hung.  Hw is desc starved and we're not going to get any more intrs:
> >> game over.
> > 
> > Ok, understood. Though doesn't this fix just narrow the scope for that
> > failure? It seems that it could still occur in a pathological case.
> 
> Oh!  The intention was to fix it absolutely.  Please expand on the
> pathological case your thinking about.  We need this 100% solved, but now
> you've got me worried...  ;)

Perhaps I am misunderstanding the problem, but what if the budget in
net_rx_action() is exhausted before a call to enic_poll*() successfully
allocs?

Re: [net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Scott Feldman]

On 12/19/09 2:41 AM, "Simon Horman" <horms@verge.net.au> wrote:

> On Fri, Dec 18, 2009 at 06:09:46PM -0800, Scott Feldman wrote:
>> for (i = 0; i < enic->rq_count; i++) {
>> -  err = vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
>> -  if (err) {
>> +  vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
>> +  /* Need at least one buffer on ring to get going */
>> +  if (vnic_rq_desc_used(&enic->rq[i]) == 0) {
>> printk(KERN_ERR PFX
>> "%s: Unable to alloc receive buffers.\n",
>> netdev->name);
>> +   err = -ENOMEM;
>> goto err_out_notify_unset;
>> }
>> }
> 
> Also, in the case where a failure occurs for i > 0,
> it it necessary to unwind the previous rq allocations?

Sorry Simon, I replied wrongly on this one.  The code only cares if _all_
allocations failed (i.e. The ring is empty), and takes the err path.  Since
nothing was allocated, the err path has nothing to cleanup.

If at lease one skb was allocated, then we don't go down the err path, even
if not all of the allocations succeeded.  Bottom line is we only need one
skb on the ring to get the ball rolling.  If we start out with a full
rings-worth of skbs (the expected case), then that's even better.

-scott

Re: [net-next PATCH 2/6] enic: Bug fix: try harder to fill Rx ring on skb allocation failures

[Simon Horman]

On Sat, Dec 19, 2009 at 05:51:31PM -0800, Scott Feldman wrote:
> On 12/19/09 2:41 AM, "Simon Horman" <horms@verge.net.au> wrote:
> 
> > On Fri, Dec 18, 2009 at 06:09:46PM -0800, Scott Feldman wrote:
> >> for (i = 0; i < enic->rq_count; i++) {
> >> -  err = vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
> >> -  if (err) {
> >> +  vnic_rq_fill(&enic->rq[i], enic->rq_alloc_buf);
> >> +  /* Need at least one buffer on ring to get going */
> >> +  if (vnic_rq_desc_used(&enic->rq[i]) == 0) {
> >> printk(KERN_ERR PFX
> >> "%s: Unable to alloc receive buffers.\n",
> >> netdev->name);
> >> +   err = -ENOMEM;
> >> goto err_out_notify_unset;
> >> }
> >> }
> > 
> > Also, in the case where a failure occurs for i > 0,
> > it it necessary to unwind the previous rq allocations?
> 
> Sorry Simon, I replied wrongly on this one.  The code only cares if _all_
> allocations failed (i.e. The ring is empty), and takes the err path.  Since
> nothing was allocated, the err path has nothing to cleanup.
> 
> If at lease one skb was allocated, then we don't go down the err path, even
> if not all of the allocations succeeded.  Bottom line is we only need one
> skb on the ring to get the ball rolling.  If we start out with a full
> rings-worth of skbs (the expected case), then that's even better.

Understood. Thanks for the clarification.

[net-next PATCH 3/6] enic: minimize pkt filter updates to firmware

[Scott Feldman]

enic: minimize pkt filter updates to firmware

In set_multicast(), only push pkt filter changes down to firmware if
pkt filter actually changes.

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
---
 drivers/net/enic/enic.h      |    1 +
 drivers/net/enic/enic_main.c |    8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index dfd6024..b09b091 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -89,6 +89,7 @@ struct enic {
 	spinlock_t devcmd_lock;
 	u8 mac_addr[ETH_ALEN];
 	u8 mc_addr[ENIC_MULTICAST_PERFECT_FILTERS][ETH_ALEN];
+	unsigned int flags;
 	unsigned int mc_count;
 	int csum_rx_enabled;
 	u32 port_mtu;
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 0265b25..2708ecf 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -772,6 +772,7 @@ static void enic_set_multicast_list(struct net_device *netdev)
 	int promisc = (netdev->flags & IFF_PROMISC) ? 1 : 0;
 	int allmulti = (netdev->flags & IFF_ALLMULTI) ||
 	    (netdev->mc_count > ENIC_MULTICAST_PERFECT_FILTERS);
+	unsigned int flags = netdev->flags | (allmulti ? IFF_ALLMULTI : 0);
 	u8 mc_addr[ENIC_MULTICAST_PERFECT_FILTERS][ETH_ALEN];
 	unsigned int mc_count = netdev->mc_count;
 	unsigned int i, j;
@@ -781,8 +782,11 @@ static void enic_set_multicast_list(struct net_device *netdev)
 
 	spin_lock(&enic->devcmd_lock);
 
-	vnic_dev_packet_filter(enic->vdev, directed,
-		multicast, broadcast, promisc, allmulti);
+	if (enic->flags != flags) {
+		enic->flags = flags;
+		vnic_dev_packet_filter(enic->vdev, directed,
+			multicast, broadcast, promisc, allmulti);
+	}
 
 	/* Is there an easier way?  Trying to minimize to
 	 * calls to add/del multicast addrs.  We keep the

[net-next PATCH 4/6] enic: Bug fix: align desc ring sizes to 32 descs

[Scott Feldman]

enic: Bug fix: align desc ring sizes to 32 descs

Previous driver was aligning ring sizes to 16 descs, but hardware actually
wants desc ring sizes to be aligned to 32 descs.

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
---
 drivers/net/enic/enic_res.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
index 3211114..a605da1 100644
--- a/drivers/net/enic/enic_res.c
+++ b/drivers/net/enic/enic_res.c
@@ -74,13 +74,13 @@ int enic_get_vnic_config(struct enic *enic)
 		min_t(u32, ENIC_MAX_WQ_DESCS,
 		max_t(u32, ENIC_MIN_WQ_DESCS,
 		c->wq_desc_count));
-	c->wq_desc_count &= 0xfffffff0; /* must be aligned to groups of 16 */
+	c->wq_desc_count &= 0xffffffe0; /* must be aligned to groups of 32 */
 
 	c->rq_desc_count =
 		min_t(u32, ENIC_MAX_RQ_DESCS,
 		max_t(u32, ENIC_MIN_RQ_DESCS,
 		c->rq_desc_count));
-	c->rq_desc_count &= 0xfffffff0; /* must be aligned to groups of 16 */
+	c->rq_desc_count &= 0xffffffe0; /* must be aligned to groups of 32 */
 
 	if (c->mtu == 0)
 		c->mtu = 1500;

[net-next PATCH 5/6] enic: feature add: add ethtool -c/C support

[Scott Feldman]

enic: feature add: add ethtool -c/C support



Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
---
 drivers/net/enic/enic.h      |    2 +
 drivers/net/enic/enic_main.c |   83 +++++++++++++++++++++++++++++++++++++++++-
 drivers/net/enic/enic_res.c  |   12 ++++--
 drivers/net/enic/vnic_enet.h |    5 +++
 drivers/net/enic/vnic_intr.c |    8 ++++
 drivers/net/enic/vnic_intr.h |    2 +
 6 files changed, 104 insertions(+), 8 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index b09b091..68d7a66 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -93,6 +93,8 @@ struct enic {
 	unsigned int mc_count;
 	int csum_rx_enabled;
 	u32 port_mtu;
+	u32 rx_coalesce_usecs;
+	u32 tx_coalesce_usecs;
 
 	/* work queue cache line section */
 	____cacheline_aligned struct vnic_wq wq[ENIC_WQ_MAX];
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 2708ecf..664d132 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -262,7 +262,81 @@ static void enic_set_msglevel(struct net_device *netdev, u32 value)
 	enic->msg_enable = value;
 }
 
-static const struct ethtool_ops enic_ethtool_ops = {
+static int enic_get_coalesce(struct net_device *netdev,
+	struct ethtool_coalesce *ecmd)
+{
+	struct enic *enic = netdev_priv(netdev);
+
+	ecmd->tx_coalesce_usecs = enic->tx_coalesce_usecs;
+	ecmd->rx_coalesce_usecs = enic->rx_coalesce_usecs;
+
+	return 0;
+}
+
+static int enic_set_coalesce(struct net_device *netdev,
+	struct ethtool_coalesce *ecmd)
+{
+	struct enic *enic = netdev_priv(netdev);
+	u32 tx_coalesce_usecs;
+	u32 rx_coalesce_usecs;
+
+	/* Only rx-usecs and tx-usecs can be set */
+	if (ecmd->rx_max_coalesced_frames || ecmd->rx_coalesce_usecs_irq ||
+	    ecmd->rx_max_coalesced_frames_irq ||
+	    ecmd->tx_max_coalesced_frames || ecmd->tx_coalesce_usecs_irq ||
+	    ecmd->tx_max_coalesced_frames_irq ||
+	    ecmd->stats_block_coalesce_usecs ||
+	    ecmd->use_adaptive_rx_coalesce || ecmd->use_adaptive_tx_coalesce ||
+	    ecmd->pkt_rate_low ||
+	    ecmd->rx_coalesce_usecs_low || ecmd->rx_max_coalesced_frames_low ||
+	    ecmd->tx_coalesce_usecs_low || ecmd->tx_max_coalesced_frames_low ||
+	    ecmd->pkt_rate_high ||
+	    ecmd->rx_coalesce_usecs_high ||
+	    ecmd->rx_max_coalesced_frames_high ||
+	    ecmd->tx_coalesce_usecs_high ||
+	    ecmd->tx_max_coalesced_frames_high ||
+	    ecmd->rate_sample_interval)
+		return -EINVAL;
+
+	tx_coalesce_usecs = min_t(u32,
+		INTR_COALESCE_HW_TO_USEC(VNIC_INTR_TIMER_MAX),
+		ecmd->tx_coalesce_usecs);
+	rx_coalesce_usecs = min_t(u32,
+		INTR_COALESCE_HW_TO_USEC(VNIC_INTR_TIMER_MAX),
+		ecmd->rx_coalesce_usecs);
+
+	switch (vnic_dev_get_intr_mode(enic->vdev)) {
+	case VNIC_DEV_INTR_MODE_INTX:
+		if (tx_coalesce_usecs != rx_coalesce_usecs)
+			return -EINVAL;
+
+		vnic_intr_coalescing_timer_set(&enic->intr[ENIC_INTX_WQ_RQ],
+			INTR_COALESCE_USEC_TO_HW(tx_coalesce_usecs));
+		break;
+	case VNIC_DEV_INTR_MODE_MSI:
+		if (tx_coalesce_usecs != rx_coalesce_usecs)
+			return -EINVAL;
+
+		vnic_intr_coalescing_timer_set(&enic->intr[0],
+			INTR_COALESCE_USEC_TO_HW(tx_coalesce_usecs));
+		break;
+	case VNIC_DEV_INTR_MODE_MSIX:
+		vnic_intr_coalescing_timer_set(&enic->intr[ENIC_MSIX_WQ],
+			INTR_COALESCE_USEC_TO_HW(tx_coalesce_usecs));
+		vnic_intr_coalescing_timer_set(&enic->intr[ENIC_MSIX_RQ],
+			INTR_COALESCE_USEC_TO_HW(rx_coalesce_usecs));
+		break;
+	default:
+		break;
+	}
+
+	enic->tx_coalesce_usecs = tx_coalesce_usecs;
+	enic->rx_coalesce_usecs = rx_coalesce_usecs;
+
+	return 0;
+}
+
+static struct ethtool_ops enic_ethtool_ops = {
 	.get_settings = enic_get_settings,
 	.get_drvinfo = enic_get_drvinfo,
 	.get_msglevel = enic_get_msglevel,
@@ -279,6 +353,8 @@ static const struct ethtool_ops enic_ethtool_ops = {
 	.set_sg = ethtool_op_set_sg,
 	.get_tso = ethtool_op_get_tso,
 	.set_tso = enic_set_tso,
+	.get_coalesce = enic_get_coalesce,
+	.set_coalesce = enic_set_coalesce,
 	.get_flags = ethtool_op_get_flags,
 	.set_flags = ethtool_op_set_flags,
 };
@@ -364,12 +440,12 @@ static void enic_mtu_check(struct enic *enic)
 	u32 mtu = vnic_dev_mtu(enic->vdev);
 
 	if (mtu && mtu != enic->port_mtu) {
+		enic->port_mtu = mtu;
 		if (mtu < enic->netdev->mtu)
 			printk(KERN_WARNING PFX
 				"%s: interface MTU (%d) set higher "
 				"than switch port MTU (%d)\n",
 				enic->netdev->name, enic->netdev->mtu, mtu);
-		enic->port_mtu = mtu;
 	}
 }
 
@@ -1972,6 +2048,9 @@ static int __devinit enic_probe(struct pci_dev *pdev,
 		goto err_out_dev_deinit;
 	}
 
+	enic->tx_coalesce_usecs = enic->config.intr_timer_usec;
+	enic->rx_coalesce_usecs =  enic->tx_coalesce_usecs;
+
 	netdev->netdev_ops = &enic_netdev_ops;
 	netdev->watchdog_timeo = 2 * HZ;
 	netdev->ethtool_ops = &enic_ethtool_ops;
diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
index a605da1..02839bf 100644
--- a/drivers/net/enic/enic_res.c
+++ b/drivers/net/enic/enic_res.c
@@ -66,9 +66,9 @@ int enic_get_vnic_config(struct enic *enic)
 	GET_CONFIG(wq_desc_count);
 	GET_CONFIG(rq_desc_count);
 	GET_CONFIG(mtu);
-	GET_CONFIG(intr_timer);
 	GET_CONFIG(intr_timer_type);
 	GET_CONFIG(intr_mode);
+	GET_CONFIG(intr_timer_usec);
 
 	c->wq_desc_count =
 		min_t(u32, ENIC_MAX_WQ_DESCS,
@@ -88,15 +88,17 @@ int enic_get_vnic_config(struct enic *enic)
 		max_t(u16, ENIC_MIN_MTU,
 		c->mtu));
 
-	c->intr_timer = min_t(u16, VNIC_INTR_TIMER_MAX, c->intr_timer);
+	c->intr_timer_usec = min_t(u32,
+		INTR_COALESCE_HW_TO_USEC(VNIC_INTR_TIMER_MAX),
+		c->intr_timer_usec);
 
 	printk(KERN_INFO PFX "vNIC MAC addr %pM wq/rq %d/%d\n",
 		enic->mac_addr, c->wq_desc_count, c->rq_desc_count);
 	printk(KERN_INFO PFX "vNIC mtu %d csum tx/rx %d/%d tso/lro %d/%d "
-		"intr timer %d\n",
+		"intr timer %d usec\n",
 		c->mtu, ENIC_SETTING(enic, TXCSUM),
 		ENIC_SETTING(enic, RXCSUM), ENIC_SETTING(enic, TSO),
-		ENIC_SETTING(enic, LRO), c->intr_timer);
+		ENIC_SETTING(enic, LRO), c->intr_timer_usec);
 
 	return 0;
 }
@@ -303,7 +305,7 @@ void enic_init_vnic_resources(struct enic *enic)
 
 	for (i = 0; i < enic->intr_count; i++) {
 		vnic_intr_init(&enic->intr[i],
-			enic->config.intr_timer,
+			INTR_COALESCE_USEC_TO_HW(enic->config.intr_timer_usec),
 			enic->config.intr_timer_type,
 			mask_on_assertion);
 	}
diff --git a/drivers/net/enic/vnic_enet.h b/drivers/net/enic/vnic_enet.h
index 6332ac9..8eeb675 100644
--- a/drivers/net/enic/vnic_enet.h
+++ b/drivers/net/enic/vnic_enet.h
@@ -20,6 +20,10 @@
 #ifndef _VNIC_ENIC_H_
 #define _VNIC_ENIC_H_
 
+/* Hardware intr coalesce timer is in units of 1.5us */
+#define INTR_COALESCE_USEC_TO_HW(usec) ((usec) * 2/3)
+#define INTR_COALESCE_HW_TO_USEC(usec) ((usec) * 3/2)
+
 /* Device-specific region: enet configuration */
 struct vnic_enet_config {
 	u32 flags;
@@ -30,6 +34,7 @@ struct vnic_enet_config {
 	u8 intr_timer_type;
 	u8 intr_mode;
 	char devname[16];
+	u32 intr_timer_usec;
 };
 
 #define VENETF_TSO		0x1	/* TSO enabled */
diff --git a/drivers/net/enic/vnic_intr.c b/drivers/net/enic/vnic_intr.c
index 1f8786d..3934309 100644
--- a/drivers/net/enic/vnic_intr.c
+++ b/drivers/net/enic/vnic_intr.c
@@ -50,12 +50,18 @@ int vnic_intr_alloc(struct vnic_dev *vdev, struct vnic_intr *intr,
 void vnic_intr_init(struct vnic_intr *intr, unsigned int coalescing_timer,
 	unsigned int coalescing_type, unsigned int mask_on_assertion)
 {
-	iowrite32(coalescing_timer, &intr->ctrl->coalescing_timer);
+	vnic_intr_coalescing_timer_set(intr, coalescing_timer);
 	iowrite32(coalescing_type, &intr->ctrl->coalescing_type);
 	iowrite32(mask_on_assertion, &intr->ctrl->mask_on_assertion);
 	iowrite32(0, &intr->ctrl->int_credits);
 }
 
+void vnic_intr_coalescing_timer_set(struct vnic_intr *intr,
+	unsigned int coalescing_timer)
+{
+	iowrite32(coalescing_timer, &intr->ctrl->coalescing_timer);
+}
+
 void vnic_intr_clean(struct vnic_intr *intr)
 {
 	iowrite32(0, &intr->ctrl->int_credits);
diff --git a/drivers/net/enic/vnic_intr.h b/drivers/net/enic/vnic_intr.h
index 9a53604..0c1e094 100644
--- a/drivers/net/enic/vnic_intr.h
+++ b/drivers/net/enic/vnic_intr.h
@@ -101,6 +101,8 @@ int vnic_intr_alloc(struct vnic_dev *vdev, struct vnic_intr *intr,
 	unsigned int index);
 void vnic_intr_init(struct vnic_intr *intr, unsigned int coalescing_timer,
 	unsigned int coalescing_type, unsigned int mask_on_assertion);
+void vnic_intr_coalescing_timer_set(struct vnic_intr *intr,
+	unsigned int coalescing_timer);
 void vnic_intr_clean(struct vnic_intr *intr);
 
 #endif /* _VNIC_INTR_H_ */

Re: [net-next PATCH 5/6] enic: feature add: add ethtool -c/C support

[Simon Horman]

On Fri, Dec 18, 2009 at 06:10:02PM -0800, Scott Feldman wrote:
> enic: feature add: add ethtool -c/C support
> 
> 
> 
> Signed-off-by: Scott Feldman <scofeldm@cisco.com>
> Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
> ---
>  drivers/net/enic/enic.h      |    2 +
>  drivers/net/enic/enic_main.c |   83 +++++++++++++++++++++++++++++++++++++++++-
>  drivers/net/enic/enic_res.c  |   12 ++++--
>  drivers/net/enic/vnic_enet.h |    5 +++
>  drivers/net/enic/vnic_intr.c |    8 ++++
>  drivers/net/enic/vnic_intr.h |    2 +
>  6 files changed, 104 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
> index b09b091..68d7a66 100644
> --- a/drivers/net/enic/enic.h
> +++ b/drivers/net/enic/enic.h
> @@ -93,6 +93,8 @@ struct enic {
>  	unsigned int mc_count;
>  	int csum_rx_enabled;
>  	u32 port_mtu;
> +	u32 rx_coalesce_usecs;
> +	u32 tx_coalesce_usecs;
>  
>  	/* work queue cache line section */
>  	____cacheline_aligned struct vnic_wq wq[ENIC_WQ_MAX];
> diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
> index 2708ecf..664d132 100644
> --- a/drivers/net/enic/enic_main.c
> +++ b/drivers/net/enic/enic_main.c
> @@ -262,7 +262,81 @@ static void enic_set_msglevel(struct net_device *netdev, u32 value)
>  	enic->msg_enable = value;
>  }
>  
> -static const struct ethtool_ops enic_ethtool_ops = {
> +static int enic_get_coalesce(struct net_device *netdev,
> +	struct ethtool_coalesce *ecmd)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +
> +	ecmd->tx_coalesce_usecs = enic->tx_coalesce_usecs;
> +	ecmd->rx_coalesce_usecs = enic->rx_coalesce_usecs;
> +
> +	return 0;
> +}
> +
> +static int enic_set_coalesce(struct net_device *netdev,
> +	struct ethtool_coalesce *ecmd)
> +{
> +	struct enic *enic = netdev_priv(netdev);
> +	u32 tx_coalesce_usecs;
> +	u32 rx_coalesce_usecs;
> +
> +	/* Only rx-usecs and tx-usecs can be set */
> +	if (ecmd->rx_max_coalesced_frames || ecmd->rx_coalesce_usecs_irq ||
> +	    ecmd->rx_max_coalesced_frames_irq ||
> +	    ecmd->tx_max_coalesced_frames || ecmd->tx_coalesce_usecs_irq ||
> +	    ecmd->tx_max_coalesced_frames_irq ||
> +	    ecmd->stats_block_coalesce_usecs ||
> +	    ecmd->use_adaptive_rx_coalesce || ecmd->use_adaptive_tx_coalesce ||
> +	    ecmd->pkt_rate_low ||
> +	    ecmd->rx_coalesce_usecs_low || ecmd->rx_max_coalesced_frames_low ||
> +	    ecmd->tx_coalesce_usecs_low || ecmd->tx_max_coalesced_frames_low ||
> +	    ecmd->pkt_rate_high ||
> +	    ecmd->rx_coalesce_usecs_high ||
> +	    ecmd->rx_max_coalesced_frames_high ||
> +	    ecmd->tx_coalesce_usecs_high ||
> +	    ecmd->tx_max_coalesced_frames_high ||
> +	    ecmd->rate_sample_interval)
> +		return -EINVAL;

This seems rather verbose.  Does it really matter if there is junk in other
ecmd fields?  They seem to be otherwise ignored. Removing the check above
would seem to be consistent with other enic_set_*() functions.

> +
> +	tx_coalesce_usecs = min_t(u32,
> +		INTR_COALESCE_HW_TO_USEC(VNIC_INTR_TIMER_MAX),
> +		ecmd->tx_coalesce_usecs);
> +	rx_coalesce_usecs = min_t(u32,
> +		INTR_COALESCE_HW_TO_USEC(VNIC_INTR_TIMER_MAX),
> +		ecmd->rx_coalesce_usecs);
> +
> +	switch (vnic_dev_get_intr_mode(enic->vdev)) {
> +	case VNIC_DEV_INTR_MODE_INTX:
> +		if (tx_coalesce_usecs != rx_coalesce_usecs)
> +			return -EINVAL;
> +
> +		vnic_intr_coalescing_timer_set(&enic->intr[ENIC_INTX_WQ_RQ],
> +			INTR_COALESCE_USEC_TO_HW(tx_coalesce_usecs));
> +		break;
> +	case VNIC_DEV_INTR_MODE_MSI:
> +		if (tx_coalesce_usecs != rx_coalesce_usecs)
> +			return -EINVAL;
> +
> +		vnic_intr_coalescing_timer_set(&enic->intr[0],
> +			INTR_COALESCE_USEC_TO_HW(tx_coalesce_usecs));
> +		break;
> +	case VNIC_DEV_INTR_MODE_MSIX:
> +		vnic_intr_coalescing_timer_set(&enic->intr[ENIC_MSIX_WQ],
> +			INTR_COALESCE_USEC_TO_HW(tx_coalesce_usecs));
> +		vnic_intr_coalescing_timer_set(&enic->intr[ENIC_MSIX_RQ],
> +			INTR_COALESCE_USEC_TO_HW(rx_coalesce_usecs));
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	enic->tx_coalesce_usecs = tx_coalesce_usecs;
> +	enic->rx_coalesce_usecs = rx_coalesce_usecs;
> +
> +	return 0;
> +}
> +
> +static struct ethtool_ops enic_ethtool_ops = {
>  	.get_settings = enic_get_settings,
>  	.get_drvinfo = enic_get_drvinfo,
>  	.get_msglevel = enic_get_msglevel,
> @@ -279,6 +353,8 @@ static const struct ethtool_ops enic_ethtool_ops = {
>  	.set_sg = ethtool_op_set_sg,
>  	.get_tso = ethtool_op_get_tso,
>  	.set_tso = enic_set_tso,
> +	.get_coalesce = enic_get_coalesce,
> +	.set_coalesce = enic_set_coalesce,
>  	.get_flags = ethtool_op_get_flags,
>  	.set_flags = ethtool_op_set_flags,
>  };
> @@ -364,12 +440,12 @@ static void enic_mtu_check(struct enic *enic)
>  	u32 mtu = vnic_dev_mtu(enic->vdev);
>  
>  	if (mtu && mtu != enic->port_mtu) {
> +		enic->port_mtu = mtu;
>  		if (mtu < enic->netdev->mtu)
>  			printk(KERN_WARNING PFX
>  				"%s: interface MTU (%d) set higher "
>  				"than switch port MTU (%d)\n",
>  				enic->netdev->name, enic->netdev->mtu, mtu);
> -		enic->port_mtu = mtu;
>  	}
>  }

This hunk seems unrelated to the rest of the patch.
Am I missing something?

> @@ -1972,6 +2048,9 @@ static int __devinit enic_probe(struct pci_dev *pdev,
>  		goto err_out_dev_deinit;
>  	}
>  
> +	enic->tx_coalesce_usecs = enic->config.intr_timer_usec;
> +	enic->rx_coalesce_usecs =  enic->tx_coalesce_usecs;
> +

It looks like there is an space after =

>  	netdev->netdev_ops = &enic_netdev_ops;
>  	netdev->watchdog_timeo = 2 * HZ;
>  	netdev->ethtool_ops = &enic_ethtool_ops;
> diff --git a/drivers/net/enic/enic_res.c b/drivers/net/enic/enic_res.c
> index a605da1..02839bf 100644
> --- a/drivers/net/enic/enic_res.c
> +++ b/drivers/net/enic/enic_res.c
> @@ -66,9 +66,9 @@ int enic_get_vnic_config(struct enic *enic)
>  	GET_CONFIG(wq_desc_count);
>  	GET_CONFIG(rq_desc_count);
>  	GET_CONFIG(mtu);
> -	GET_CONFIG(intr_timer);
>  	GET_CONFIG(intr_timer_type);
>  	GET_CONFIG(intr_mode);
> +	GET_CONFIG(intr_timer_usec);
>  
>  	c->wq_desc_count =
>  		min_t(u32, ENIC_MAX_WQ_DESCS,
> @@ -88,15 +88,17 @@ int enic_get_vnic_config(struct enic *enic)
>  		max_t(u16, ENIC_MIN_MTU,
>  		c->mtu));
>  
> -	c->intr_timer = min_t(u16, VNIC_INTR_TIMER_MAX, c->intr_timer);
> +	c->intr_timer_usec = min_t(u32,
> +		INTR_COALESCE_HW_TO_USEC(VNIC_INTR_TIMER_MAX),
> +		c->intr_timer_usec);
>  
>  	printk(KERN_INFO PFX "vNIC MAC addr %pM wq/rq %d/%d\n",
>  		enic->mac_addr, c->wq_desc_count, c->rq_desc_count);
>  	printk(KERN_INFO PFX "vNIC mtu %d csum tx/rx %d/%d tso/lro %d/%d "
> -		"intr timer %d\n",
> +		"intr timer %d usec\n",
>  		c->mtu, ENIC_SETTING(enic, TXCSUM),
>  		ENIC_SETTING(enic, RXCSUM), ENIC_SETTING(enic, TSO),
> -		ENIC_SETTING(enic, LRO), c->intr_timer);
> +		ENIC_SETTING(enic, LRO), c->intr_timer_usec);
>  
>  	return 0;
>  }
> @@ -303,7 +305,7 @@ void enic_init_vnic_resources(struct enic *enic)
>  
>  	for (i = 0; i < enic->intr_count; i++) {
>  		vnic_intr_init(&enic->intr[i],
> -			enic->config.intr_timer,
> +			INTR_COALESCE_USEC_TO_HW(enic->config.intr_timer_usec),
>  			enic->config.intr_timer_type,
>  			mask_on_assertion);
>  	}
> diff --git a/drivers/net/enic/vnic_enet.h b/drivers/net/enic/vnic_enet.h
> index 6332ac9..8eeb675 100644
> --- a/drivers/net/enic/vnic_enet.h
> +++ b/drivers/net/enic/vnic_enet.h
> @@ -20,6 +20,10 @@
>  #ifndef _VNIC_ENIC_H_
>  #define _VNIC_ENIC_H_
>  
> +/* Hardware intr coalesce timer is in units of 1.5us */
> +#define INTR_COALESCE_USEC_TO_HW(usec) ((usec) * 2/3)
> +#define INTR_COALESCE_HW_TO_USEC(usec) ((usec) * 3/2)
> +
>  /* Device-specific region: enet configuration */
>  struct vnic_enet_config {
>  	u32 flags;
> @@ -30,6 +34,7 @@ struct vnic_enet_config {
>  	u8 intr_timer_type;
>  	u8 intr_mode;
>  	char devname[16];
> +	u32 intr_timer_usec;
>  };
>  
>  #define VENETF_TSO		0x1	/* TSO enabled */
> diff --git a/drivers/net/enic/vnic_intr.c b/drivers/net/enic/vnic_intr.c
> index 1f8786d..3934309 100644
> --- a/drivers/net/enic/vnic_intr.c
> +++ b/drivers/net/enic/vnic_intr.c
> @@ -50,12 +50,18 @@ int vnic_intr_alloc(struct vnic_dev *vdev, struct vnic_intr *intr,
>  void vnic_intr_init(struct vnic_intr *intr, unsigned int coalescing_timer,
>  	unsigned int coalescing_type, unsigned int mask_on_assertion)
>  {
> -	iowrite32(coalescing_timer, &intr->ctrl->coalescing_timer);
> +	vnic_intr_coalescing_timer_set(intr, coalescing_timer);
>  	iowrite32(coalescing_type, &intr->ctrl->coalescing_type);
>  	iowrite32(mask_on_assertion, &intr->ctrl->mask_on_assertion);
>  	iowrite32(0, &intr->ctrl->int_credits);
>  }
>  
> +void vnic_intr_coalescing_timer_set(struct vnic_intr *intr,
> +	unsigned int coalescing_timer)
> +{
> +	iowrite32(coalescing_timer, &intr->ctrl->coalescing_timer);
> +}
> +
>  void vnic_intr_clean(struct vnic_intr *intr)
>  {
>  	iowrite32(0, &intr->ctrl->int_credits);
> diff --git a/drivers/net/enic/vnic_intr.h b/drivers/net/enic/vnic_intr.h
> index 9a53604..0c1e094 100644
> --- a/drivers/net/enic/vnic_intr.h
> +++ b/drivers/net/enic/vnic_intr.h
> @@ -101,6 +101,8 @@ int vnic_intr_alloc(struct vnic_dev *vdev, struct vnic_intr *intr,
>  	unsigned int index);
>  void vnic_intr_init(struct vnic_intr *intr, unsigned int coalescing_timer,
>  	unsigned int coalescing_type, unsigned int mask_on_assertion);
> +void vnic_intr_coalescing_timer_set(struct vnic_intr *intr,
> +	unsigned int coalescing_timer);
>  void vnic_intr_clean(struct vnic_intr *intr);
>  
>  #endif /* _VNIC_INTR_H_ */
> 
> --
> 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

Re: [net-next PATCH 5/6] enic: feature add: add ethtool -c/C support

[Scott Feldman]

Thanks for the review comments Simon.

On 12/19/09 2:10 AM, "Simon Horman" <horms@verge.net.au> wrote:

> On Fri, Dec 18, 2009 at 06:10:02PM -0800, Scott Feldman wrote:
>> + /* Only rx-usecs and tx-usecs can be set */
>> + if (ecmd->rx_max_coalesced_frames || ecmd->rx_coalesce_usecs_irq ||
>> +     ecmd->rx_max_coalesced_frames_irq ||
>> +     ecmd->tx_max_coalesced_frames || ecmd->tx_coalesce_usecs_irq ||
>> +     ecmd->tx_max_coalesced_frames_irq ||
>> +     ecmd->stats_block_coalesce_usecs ||
>> +     ecmd->use_adaptive_rx_coalesce || ecmd->use_adaptive_tx_coalesce ||
>> +     ecmd->pkt_rate_low ||
>> +     ecmd->rx_coalesce_usecs_low || ecmd->rx_max_coalesced_frames_low ||
>> +     ecmd->tx_coalesce_usecs_low || ecmd->tx_max_coalesced_frames_low ||
>> +     ecmd->pkt_rate_high ||
>> +     ecmd->rx_coalesce_usecs_high ||
>> +     ecmd->rx_max_coalesced_frames_high ||
>> +     ecmd->tx_coalesce_usecs_high ||
>> +     ecmd->tx_max_coalesced_frames_high ||
>> +     ecmd->rate_sample_interval)
>> +  return -EINVAL;
> 
> This seems rather verbose.  Does it really matter if there is junk in other
> ecmd fields?  They seem to be otherwise ignored. Removing the check above
> would seem to be consistent with other enic_set_*() functions.

Ya, we can do that.
 
>> if (mtu && mtu != enic->port_mtu) {
>> +  enic->port_mtu = mtu;
>> if (mtu < enic->netdev->mtu)
>> printk(KERN_WARNING PFX
>> "%s: interface MTU (%d) set higher "
>> "than switch port MTU (%d)\n",
>> enic->netdev->name, enic->netdev->mtu, mtu);
>> -  enic->port_mtu = mtu;
>> }
>>  }
> 
> This hunk seems unrelated to the rest of the patch.
> Am I missing something?

Oops, that change snuck in there.  That line got moved due to some backward
compat changes, which have been stripped away, so it looks like that line is
moving for no reason.  It's a benign change so let's make the change to be
consistent with our internal code base.
 
>> + enic->tx_coalesce_usecs = enic->config.intr_timer_usec;
>> + enic->rx_coalesce_usecs =  enic->tx_coalesce_usecs;
>> +
> 
> It looks like there is an space after =

Got it.

-scott

Re: [net-next PATCH 5/6] enic: feature add: add ethtool -c/C support

[Simon Horman]

On Sat, Dec 19, 2009 at 11:33:54AM -0800, Scott Feldman wrote:
> Thanks for the review comments Simon.
> 
> On 12/19/09 2:10 AM, "Simon Horman" <horms@verge.net.au> wrote:
> 
> > On Fri, Dec 18, 2009 at 06:10:02PM -0800, Scott Feldman wrote:
> >> + /* Only rx-usecs and tx-usecs can be set */
> >> + if (ecmd->rx_max_coalesced_frames || ecmd->rx_coalesce_usecs_irq ||
> >> +     ecmd->rx_max_coalesced_frames_irq ||
> >> +     ecmd->tx_max_coalesced_frames || ecmd->tx_coalesce_usecs_irq ||
> >> +     ecmd->tx_max_coalesced_frames_irq ||
> >> +     ecmd->stats_block_coalesce_usecs ||
> >> +     ecmd->use_adaptive_rx_coalesce || ecmd->use_adaptive_tx_coalesce ||
> >> +     ecmd->pkt_rate_low ||
> >> +     ecmd->rx_coalesce_usecs_low || ecmd->rx_max_coalesced_frames_low ||
> >> +     ecmd->tx_coalesce_usecs_low || ecmd->tx_max_coalesced_frames_low ||
> >> +     ecmd->pkt_rate_high ||
> >> +     ecmd->rx_coalesce_usecs_high ||
> >> +     ecmd->rx_max_coalesced_frames_high ||
> >> +     ecmd->tx_coalesce_usecs_high ||
> >> +     ecmd->tx_max_coalesced_frames_high ||
> >> +     ecmd->rate_sample_interval)
> >> +  return -EINVAL;
> > 
> > This seems rather verbose.  Does it really matter if there is junk in other
> > ecmd fields?  They seem to be otherwise ignored. Removing the check above
> > would seem to be consistent with other enic_set_*() functions.
> 
> Ya, we can do that.
>  
> >> if (mtu && mtu != enic->port_mtu) {
> >> +  enic->port_mtu = mtu;
> >> if (mtu < enic->netdev->mtu)
> >> printk(KERN_WARNING PFX
> >> "%s: interface MTU (%d) set higher "
> >> "than switch port MTU (%d)\n",
> >> enic->netdev->name, enic->netdev->mtu, mtu);
> >> -  enic->port_mtu = mtu;
> >> }
> >>  }
> > 
> > This hunk seems unrelated to the rest of the patch.
> > Am I missing something?
> 
> Oops, that change snuck in there.  That line got moved due to some backward
> compat changes, which have been stripped away, so it looks like that line is
> moving for no reason.  It's a benign change so let's make the change to be
> consistent with our internal code base.

Sure, it doesn't seem to be doing any harm.

> >> + enic->tx_coalesce_usecs = enic->config.intr_timer_usec;
> >> + enic->rx_coalesce_usecs =  enic->tx_coalesce_usecs;
> >> +
> > 
> > It looks like there is an space after =
> 
> Got it.
> 
> -scott
> 
> --
> 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

[net-next PATCH 6/6] enic: whitespace cleanup; #define cleanup; more verbose err msg

[Scott Feldman]

enic: whitespace cleanup; #define cleanup; more verbose err msg

Some misc changes to cleanup whitespace issues and fix/remove some #define
HW defintions.

1) fix some whitespace issues
2) more verbose err msg when resources aren't available to configure vnic
3) remove unused #define
4) fix RSS #define rss hash types

Signed-off-by: Scott Feldman <scofeldm@cisco.com>
Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
---
 drivers/net/enic/enic_main.c |    5 +++--
 drivers/net/enic/vnic_dev.c  |    1 -
 drivers/net/enic/vnic_nic.h  |   12 ++++++------
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index 664d132..0ef3ab3 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -750,7 +750,7 @@ static inline void enic_queue_wq_skb(struct enic *enic,
 
 /* netif_tx_lock held, process context with BHs disabled, or BH */
 static netdev_tx_t enic_hard_start_xmit(struct sk_buff *skb,
-					      struct net_device *netdev)
+	struct net_device *netdev)
 {
 	struct enic *enic = netdev_priv(netdev);
 	struct vnic_wq *wq = &enic->wq[0];
@@ -1824,7 +1824,8 @@ int enic_dev_init(struct enic *enic)
 	err = enic_set_intr_mode(enic);
 	if (err) {
 		printk(KERN_ERR PFX
-			"Failed to set intr mode, aborting.\n");
+			"Failed to set intr mode based on resource "
+			"counts and system capabilities, aborting.\n");
 		return err;
 	}
 
diff --git a/drivers/net/enic/vnic_dev.c b/drivers/net/enic/vnic_dev.c
index 29a48e8..69b9b70 100644
--- a/drivers/net/enic/vnic_dev.c
+++ b/drivers/net/enic/vnic_dev.c
@@ -36,7 +36,6 @@ struct vnic_res {
 };
 
 #define VNIC_DEV_CAP_INIT	0x0001
-#define VNIC_DEV_CAP_PERBI	0x0002
 
 struct vnic_dev {
 	void *priv;
diff --git a/drivers/net/enic/vnic_nic.h b/drivers/net/enic/vnic_nic.h
index eeaf329..cf80ab4 100644
--- a/drivers/net/enic/vnic_nic.h
+++ b/drivers/net/enic/vnic_nic.h
@@ -41,12 +41,12 @@
 #define NIC_CFG_IG_VLAN_STRIP_EN_MASK_FIELD	1UL
 #define NIC_CFG_IG_VLAN_STRIP_EN_SHIFT		24
 
-#define NIC_CFG_RSS_HASH_TYPE_IPV4		(1 << 0)
-#define NIC_CFG_RSS_HASH_TYPE_TCP_IPV4		(1 << 1)
-#define NIC_CFG_RSS_HASH_TYPE_IPV6		(1 << 2)
-#define NIC_CFG_RSS_HASH_TYPE_TCP_IPV6		(1 << 3)
-#define NIC_CFG_RSS_HASH_TYPE_IPV6_EX		(1 << 4)
-#define NIC_CFG_RSS_HASH_TYPE_TCP_IPV6_EX	(1 << 5)
+#define NIC_CFG_RSS_HASH_TYPE_IPV4		(1 << 1)
+#define NIC_CFG_RSS_HASH_TYPE_TCP_IPV4		(1 << 2)
+#define NIC_CFG_RSS_HASH_TYPE_IPV6		(1 << 3)
+#define NIC_CFG_RSS_HASH_TYPE_TCP_IPV6		(1 << 4)
+#define NIC_CFG_RSS_HASH_TYPE_IPV6_EX		(1 << 5)
+#define NIC_CFG_RSS_HASH_TYPE_TCP_IPV6_EX	(1 << 6)
 
 static inline void vnic_set_nic_cfg(u32 *nic_cfg,
 	u8 rss_default_cpu, u8 rss_hash_type,

[net-next PATCH 0/6] enic: updates to version 1.1.0.241a

[Scott Feldman]

Resending with updates to address issues raised by Ben Hutchings and
Simon Horman.

Please apply to net-next.

1/6 - Bug fix: use safe queue shutdown in dev->stop
2/6 - Bug fix: try harder to fill Rx ring on skb allocation failures
3/6 - minimize pkt filter updates to firmware
4/6 - Bug fix: align desc ring sizes to 32 descs
5/6 - feature add: add ethtool -c/C support
6/6 - whitespace cleanup; #define cleanup; more verbose err msg

Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
Signed-off-by: Scott Feldman <scofeldm@cisco.com>

[net-next PATCH 1/6] enic: Bug fix: use safe queue shutdown in dev->stop

[Scott Feldman]

From: Scott Feldman <scofeldm@cisco.com>

Fix dev->stop shutdown bug where driver was stopping xmit queue and then
disabling intrs.  Fix is to disable intrs first and then stop the xmit
queue, otherwise an interrupt could cause the queue to be rewoken.  Also,
no need to explicitly do queue servicing because queues are cleaned and
reset back to initial state at end of dev->stop.  Servicing queues also
had the side-effect of also rewakening the xmit queue, which is not what
we want.

Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
Signed-off-by: Scott Feldman <scofeldm@cisco.com>
---
 drivers/net/enic/enic.h      |    2 +
 drivers/net/enic/enic_main.c |   62 +++++++++++++++++-------------------------
 drivers/net/enic/vnic_intr.h |    1 +
 3 files changed, 27 insertions(+), 38 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index e1c2076..8dd0105 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -34,7 +34,7 @@
 
 #define DRV_NAME		"enic"
 #define DRV_DESCRIPTION		"Cisco 10G Ethernet Driver"
-#define DRV_VERSION		"1.1.0.100"
+#define DRV_VERSION		"1.1.0.241a"
 #define DRV_COPYRIGHT		"Copyright 2008-2009 Cisco Systems, Inc"
 #define PFX			DRV_NAME ": "
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index f875751..b4a11be 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -1084,34 +1084,6 @@ static int enic_rq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc,
 	return 0;
 }
 
-static void enic_rq_drop_buf(struct vnic_rq *rq,
-	struct cq_desc *cq_desc, struct vnic_rq_buf *buf,
-	int skipped, void *opaque)
-{
-	struct enic *enic = vnic_dev_priv(rq->vdev);
-	struct sk_buff *skb = buf->os_buf;
-
-	if (skipped)
-		return;
-
-	pci_unmap_single(enic->pdev, buf->dma_addr,
-		buf->len, PCI_DMA_FROMDEVICE);
-
-	dev_kfree_skb_any(skb);
-}
-
-static int enic_rq_service_drop(struct vnic_dev *vdev, struct cq_desc *cq_desc,
-	u8 type, u16 q_number, u16 completed_index, void *opaque)
-{
-	struct enic *enic = vnic_dev_priv(vdev);
-
-	vnic_rq_service(&enic->rq[q_number], cq_desc,
-		completed_index, VNIC_RQ_RETURN_DESC,
-		enic_rq_drop_buf, opaque);
-
-	return 0;
-}
-
 static int enic_poll(struct napi_struct *napi, int budget)
 {
 	struct enic *enic = container_of(napi, struct enic, napi);
@@ -1304,6 +1276,24 @@ static int enic_request_intr(struct enic *enic)
 	return err;
 }
 
+static void enic_synchronize_irqs(struct enic *enic)
+{
+	unsigned int i;
+
+	switch (vnic_dev_get_intr_mode(enic->vdev)) {
+	case VNIC_DEV_INTR_MODE_INTX:
+	case VNIC_DEV_INTR_MODE_MSI:
+		synchronize_irq(enic->pdev->irq);
+		break;
+	case VNIC_DEV_INTR_MODE_MSIX:
+		for (i = 0; i < enic->intr_count; i++)
+			synchronize_irq(enic->msix_entry[i].vector);
+		break;
+	default:
+		break;
+	}
+}
+
 static int enic_notify_set(struct enic *enic)
 {
 	int err;
@@ -1409,16 +1399,19 @@ static int enic_stop(struct net_device *netdev)
 	unsigned int i;
 	int err;
 
+	for (i = 0; i < enic->intr_count; i++)
+		vnic_intr_mask(&enic->intr[i]);
+
+	enic_synchronize_irqs(enic);
+
 	del_timer_sync(&enic->notify_timer);
 
 	spin_lock(&enic->devcmd_lock);
 	vnic_dev_disable(enic->vdev);
 	spin_unlock(&enic->devcmd_lock);
 	napi_disable(&enic->napi);
-	netif_stop_queue(netdev);
-
-	for (i = 0; i < enic->intr_count; i++)
-		vnic_intr_mask(&enic->intr[i]);
+	netif_carrier_off(netdev);
+	netif_tx_disable(netdev);
 
 	for (i = 0; i < enic->wq_count; i++) {
 		err = vnic_wq_disable(&enic->wq[i]);
@@ -1436,11 +1429,6 @@ static int enic_stop(struct net_device *netdev)
 	spin_unlock(&enic->devcmd_lock);
 	enic_free_intr(enic);
 
-	(void)vnic_cq_service(&enic->cq[ENIC_CQ_RQ],
-		-1, enic_rq_service_drop, NULL);
-	(void)vnic_cq_service(&enic->cq[ENIC_CQ_WQ],
-		-1, enic_wq_service, NULL);
-
 	for (i = 0; i < enic->wq_count; i++)
 		vnic_wq_clean(&enic->wq[i], enic_free_wq_buf);
 	for (i = 0; i < enic->rq_count; i++)
diff --git a/drivers/net/enic/vnic_intr.h b/drivers/net/enic/vnic_intr.h
index 9a53604..f79a722 100644
--- a/drivers/net/enic/vnic_intr.h
+++ b/drivers/net/enic/vnic_intr.h
@@ -61,6 +61,7 @@ static inline void vnic_intr_unmask(struct vnic_intr *intr)
 static inline void vnic_intr_mask(struct vnic_intr *intr)
 {
 	iowrite32(1, &intr->ctrl->mask);
+	(void)ioread32(&intr->ctrl->mask);
 }
 
 static inline void vnic_intr_return_credits(struct vnic_intr *intr,

[net-next PATCH 0/6] enic: updates to version 1.1.0.241a

[Scott Feldman]

Resending with updates to address issues raised by Ben Hutchings and
Simon Horman and then Ben Hutchings again.

Please apply to net-next.

1/6 - Bug fix: use safe queue shutdown in dev->stop
2/6 - Bug fix: try harder to fill Rx ring on skb allocation failures
3/6 - minimize pkt filter updates to firmware
4/6 - Bug fix: align desc ring sizes to 32 descs
5/6 - feature add: add ethtool -c/C support
6/6 - whitespace cleanup; #define cleanup; more verbose err msg

Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
Signed-off-by: Scott Feldman <scofeldm@cisco.com>

[net-next PATCH 1/6] enic: Bug fix: use safe queue shutdown in dev->stop

[Scott Feldman]

From: Scott Feldman <scofeldm@cisco.com>

Fix dev->stop shutdown bug where driver was stopping xmit queue and then
disabling intrs.  Fix is to disable intrs first and then stop the xmit
queue, otherwise an interrupt could cause the queue to be rewoken.  Also,
no need to explicitly do queue servicing because queues are cleaned and
reset back to initial state at end of dev->stop.  Servicing queues also
had the side-effect of also rewakening the xmit queue, which is not what
we want.

Signed-off-by: Vasanthy Kolluri <vkolluri@cisco.com>
Signed-off-by: Scott Feldman <scofeldm@cisco.com>
---
 drivers/net/enic/enic.h      |    2 +
 drivers/net/enic/enic_main.c |   62 +++++++++++++++++-------------------------
 drivers/net/enic/vnic_intr.h |    1 +
 3 files changed, 27 insertions(+), 38 deletions(-)

diff --git a/drivers/net/enic/enic.h b/drivers/net/enic/enic.h
index e1c2076..8dd0105 100644
--- a/drivers/net/enic/enic.h
+++ b/drivers/net/enic/enic.h
@@ -34,7 +34,7 @@
 
 #define DRV_NAME		"enic"
 #define DRV_DESCRIPTION		"Cisco 10G Ethernet Driver"
-#define DRV_VERSION		"1.1.0.100"
+#define DRV_VERSION		"1.1.0.241a"
 #define DRV_COPYRIGHT		"Copyright 2008-2009 Cisco Systems, Inc"
 #define PFX			DRV_NAME ": "
 
diff --git a/drivers/net/enic/enic_main.c b/drivers/net/enic/enic_main.c
index f875751..b4a11be 100644
--- a/drivers/net/enic/enic_main.c
+++ b/drivers/net/enic/enic_main.c
@@ -1084,34 +1084,6 @@ static int enic_rq_service(struct vnic_dev *vdev, struct cq_desc *cq_desc,
 	return 0;
 }
 
-static void enic_rq_drop_buf(struct vnic_rq *rq,
-	struct cq_desc *cq_desc, struct vnic_rq_buf *buf,
-	int skipped, void *opaque)
-{
-	struct enic *enic = vnic_dev_priv(rq->vdev);
-	struct sk_buff *skb = buf->os_buf;
-
-	if (skipped)
-		return;
-
-	pci_unmap_single(enic->pdev, buf->dma_addr,
-		buf->len, PCI_DMA_FROMDEVICE);
-
-	dev_kfree_skb_any(skb);
-}
-
-static int enic_rq_service_drop(struct vnic_dev *vdev, struct cq_desc *cq_desc,
-	u8 type, u16 q_number, u16 completed_index, void *opaque)
-{
-	struct enic *enic = vnic_dev_priv(vdev);
-
-	vnic_rq_service(&enic->rq[q_number], cq_desc,
-		completed_index, VNIC_RQ_RETURN_DESC,
-		enic_rq_drop_buf, opaque);
-
-	return 0;
-}
-
 static int enic_poll(struct napi_struct *napi, int budget)
 {
 	struct enic *enic = container_of(napi, struct enic, napi);
@@ -1304,6 +1276,24 @@ static int enic_request_intr(struct enic *enic)
 	return err;
 }
 
+static void enic_synchronize_irqs(struct enic *enic)
+{
+	unsigned int i;
+
+	switch (vnic_dev_get_intr_mode(enic->vdev)) {
+	case VNIC_DEV_INTR_MODE_INTX:
+	case VNIC_DEV_INTR_MODE_MSI:
+		synchronize_irq(enic->pdev->irq);
+		break;
+	case VNIC_DEV_INTR_MODE_MSIX:
+		for (i = 0; i < enic->intr_count; i++)
+			synchronize_irq(enic->msix_entry[i].vector);
+		break;
+	default:
+		break;
+	}
+}
+
 static int enic_notify_set(struct enic *enic)
 {
 	int err;
@@ -1409,16 +1399,19 @@ static int enic_stop(struct net_device *netdev)
 	unsigned int i;
 	int err;
 
+	for (i = 0; i < enic->intr_count; i++)
+		vnic_intr_mask(&enic->intr[i]);
+
+	enic_synchronize_irqs(enic);
+
 	del_timer_sync(&enic->notify_timer);
 
 	spin_lock(&enic->devcmd_lock);
 	vnic_dev_disable(enic->vdev);
 	spin_unlock(&enic->devcmd_lock);
 	napi_disable(&enic->napi);
-	netif_stop_queue(netdev);
-
-	for (i = 0; i < enic->intr_count; i++)
-		vnic_intr_mask(&enic->intr[i]);
+	netif_carrier_off(netdev);
+	netif_tx_disable(netdev);
 
 	for (i = 0; i < enic->wq_count; i++) {
 		err = vnic_wq_disable(&enic->wq[i]);
@@ -1436,11 +1429,6 @@ static int enic_stop(struct net_device *netdev)
 	spin_unlock(&enic->devcmd_lock);
 	enic_free_intr(enic);
 
-	(void)vnic_cq_service(&enic->cq[ENIC_CQ_RQ],
-		-1, enic_rq_service_drop, NULL);
-	(void)vnic_cq_service(&enic->cq[ENIC_CQ_WQ],
-		-1, enic_wq_service, NULL);
-
 	for (i = 0; i < enic->wq_count; i++)
 		vnic_wq_clean(&enic->wq[i], enic_free_wq_buf);
 	for (i = 0; i < enic->rq_count; i++)
diff --git a/drivers/net/enic/vnic_intr.h b/drivers/net/enic/vnic_intr.h
index 9a53604..f79a722 100644
--- a/drivers/net/enic/vnic_intr.h
+++ b/drivers/net/enic/vnic_intr.h
@@ -61,6 +61,7 @@ static inline void vnic_intr_unmask(struct vnic_intr *intr)
 static inline void vnic_intr_mask(struct vnic_intr *intr)
 {
 	iowrite32(1, &intr->ctrl->mask);
+	(void)ioread32(&intr->ctrl->mask);
 }
 
 static inline void vnic_intr_return_credits(struct vnic_intr *intr,