[PATCH net 1/5] ibmvnic: harden interrupt handler

Move most interrupt handler processing into a tasklet, and
introduce a delay after re-enabling interrupts to fix timing
issues encountered in hardware testing.

Signed-off-by: Thomas Falcon <redacted>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 21 +++++++++++++++++++--
 drivers/net/ethernet/ibm/ibmvnic.h |  1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index c125966..09071bf 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c

@@ -3414,6 +3414,18 @@ static void ibmvnic_handle_crq(union ibmvnic_crq *crq,
 static irqreturn_t ibmvnic_interrupt(int irq, void *instance)
 {
 	struct ibmvnic_adapter *adapter = instance;
+	unsigned long flags;
+
+	spin_lock_irqsave(&adapter->crq.lock, flags);
+	vio_disable_interrupts(adapter->vdev);
+	tasklet_schedule(&adapter->tasklet);
+	spin_unlock_irqrestore(&adapter->crq.lock, flags);
+	return IRQ_HANDLED;
+}
+
+static void ibmvnic_tasklet(void *data)
+{
+	struct ibmvnic_adapter *adapter = data;
 	struct ibmvnic_crq_queue *queue = &adapter->crq;
 	struct vio_dev *vdev = adapter->vdev;
 	union ibmvnic_crq *crq;

@@ -3421,7 +3433,6 @@ static irqreturn_t ibmvnic_interrupt(int irq, void *instance)
 	bool done = false;
 
 	spin_lock_irqsave(&queue->lock, flags);
-	vio_disable_interrupts(vdev);
 	while (!done) {
 		/* Pull all the valid messages off the CRQ */
 		while ((crq = ibmvnic_next_crq(adapter)) != NULL) {

@@ -3429,6 +3440,8 @@ static irqreturn_t ibmvnic_interrupt(int irq, void *instance)
 			crq->generic.first = 0;
 		}
 		vio_enable_interrupts(vdev);
+		/* delay in case of firmware hiccup */
+		mdelay(10);
 		crq = ibmvnic_next_crq(adapter);
 		if (crq) {
 			vio_disable_interrupts(vdev);

@@ -3439,7 +3452,6 @@ static irqreturn_t ibmvnic_interrupt(int irq, void *instance)
 		}
 	}
 	spin_unlock_irqrestore(&queue->lock, flags);
-	return IRQ_HANDLED;
 }
 
 static int ibmvnic_reenable_crq_queue(struct ibmvnic_adapter *adapter)

@@ -3494,6 +3506,7 @@ static void ibmvnic_release_crq_queue(struct ibmvnic_adapter *adapter)
 
 	netdev_dbg(adapter->netdev, "Releasing CRQ\n");
 	free_irq(vdev->irq, adapter);
+	tasklet_kill(&adapter->tasklet);
 	do {
 		rc = plpar_hcall_norets(H_FREE_CRQ, vdev->unit_address);
 	} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));

@@ -3539,6 +3552,9 @@ static int ibmvnic_init_crq_queue(struct ibmvnic_adapter *adapter)
 
 	retrc = 0;
 
+	tasklet_init(&adapter->tasklet, (void *)ibmvnic_tasklet,
+		     (unsigned long)adapter);
+
 	netdev_dbg(adapter->netdev, "registering irq 0x%x\n", vdev->irq);
 	rc = request_irq(vdev->irq, ibmvnic_interrupt, 0, IBMVNIC_NAME,
 			 adapter);

@@ -3560,6 +3576,7 @@ static int ibmvnic_init_crq_queue(struct ibmvnic_adapter *adapter)
 	return retrc;
 
 req_irq_failed:
+	tasklet_kill(&adapter->tasklet);
 	do {
 		rc = plpar_hcall_norets(H_FREE_CRQ, vdev->unit_address);
 	} while (rc == H_BUSY || H_IS_LONG_BUSY(rc));

diff --git a/drivers/net/ethernet/ibm/ibmvnic.h b/drivers/net/ethernet/ibm/ibmvnic.h
index dd775d9..0d0edc3 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.h
+++ b/drivers/net/ethernet/ibm/ibmvnic.h

@@ -1049,5 +1049,6 @@ struct ibmvnic_adapter {
 
 	struct work_struct vnic_crq_init;
 	struct work_struct ibmvnic_xport;
+	struct tasklet_struct tasklet;
 	bool failover;
 };

-- 
2.7.4

When a IBM VNIC client driver requests a faulty device setting, the
server returns an acceptable value for the client to request.
This 64 bit value was incorrectly being swapped as a 32 bit value,
resulting in loss of data. This patch corrects that by using
the 64 bit swap function.

Signed-off-by: John Allen <redacted>
Signed-off-by: Thomas Falcon <redacted>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index f95f6a4..ec4eaed 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c

@@ -2388,10 +2388,10 @@ static void handle_request_cap_rsp(union ibmvnic_crq *crq,
 	case PARTIALSUCCESS:
 		dev_info(dev, "req=%lld, rsp=%ld in %s queue, retrying.\n",
 			 *req_value,
-			 (long int)be32_to_cpu(crq->request_capability_rsp.
+			 (long int)be64_to_cpu(crq->request_capability_rsp.
 					       number), name);
 		release_sub_crqs_no_irqs(adapter);
-		*req_value = be32_to_cpu(crq->request_capability_rsp.number);
+		*req_value = be64_to_cpu(crq->request_capability_rsp.number);
 		init_sub_crqs(adapter, 1);
 		return;
 	default:

-- 
1.8.3.1

In the current driver, the MTU is set to the maximum value
capable for the backing device. This patch sets the MTU to the
default value for a Linux net device. It also corrects a
discrepancy between MTU values received from firmware, which includes
the ethernet header length, and net device MTU values. Finally, it removes
redundant min/max MTU assignments after device initialization.

Signed-off-by: Thomas Falcon <redacted>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index 48debde2..f95f6a4 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c

@@ -1496,7 +1496,7 @@ static void init_sub_crqs(struct ibmvnic_adapter *adapter, int retry)
 		adapter->req_rx_queues = adapter->opt_rx_comp_queues;
 		adapter->req_rx_add_queues = adapter->max_rx_add_queues;
 
-		adapter->req_mtu = adapter->max_mtu;
+		adapter->req_mtu = adapter->netdev->mtu + ETH_HLEN;
 	}
 
 	total_queues = adapter->req_tx_queues + adapter->req_rx_queues;

@@ -2626,12 +2626,12 @@ static void handle_query_cap_rsp(union ibmvnic_crq *crq,
 		break;
 	case MIN_MTU:
 		adapter->min_mtu = be64_to_cpu(crq->query_capability.number);
-		netdev->min_mtu = adapter->min_mtu;
+		netdev->min_mtu = adapter->min_mtu - ETH_HLEN;
 		netdev_dbg(netdev, "min_mtu = %lld\n", adapter->min_mtu);
 		break;
 	case MAX_MTU:
 		adapter->max_mtu = be64_to_cpu(crq->query_capability.number);
-		netdev->max_mtu = adapter->max_mtu;
+		netdev->max_mtu = adapter->max_mtu - ETH_HLEN;
 		netdev_dbg(netdev, "max_mtu = %lld\n", adapter->max_mtu);
 		break;
 	case MAX_MULTICAST_FILTERS:

@@ -3672,9 +3672,7 @@ static void handle_crq_init_rsp(struct work_struct *work)
 		goto task_failed;
 
 	netdev->real_num_tx_queues = adapter->req_tx_queues;
-	netdev->mtu = adapter->req_mtu;
-	netdev->min_mtu = adapter->min_mtu;
-	netdev->max_mtu = adapter->max_mtu;
+	netdev->mtu = adapter->req_mtu - ETH_HLEN;
 
 	if (adapter->failover) {
 		adapter->failover = false;

@@ -3814,7 +3812,7 @@ static int ibmvnic_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	}
 
 	netdev->real_num_tx_queues = adapter->req_tx_queues;
-	netdev->mtu = adapter->req_mtu;
+	netdev->mtu = adapter->req_mtu - ETH_HLEN;
 
 	rc = register_netdev(netdev);
 	if (rc) {

-- 
1.8.3.1

Error reports received from firmware were not being converted from
big endian values, leading to bogus error codes reported on little
endian systems.

Signed-off-by: Thomas Falcon <redacted>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index ec4eaed..ec6c5fe 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c

@@ -2185,12 +2185,12 @@ static void handle_error_info_rsp(union ibmvnic_crq *crq,
 
 	if (!found) {
 		dev_err(dev, "Couldn't find error id %x\n",
-			crq->request_error_rsp.error_id);
+			be32_to_cpu(crq->request_error_rsp.error_id));
 		return;
 	}
 
 	dev_err(dev, "Detailed info for error id %x:",
-		crq->request_error_rsp.error_id);
+		be32_to_cpu(crq->request_error_rsp.error_id));
 
 	for (i = 0; i < error_buff->len; i++) {
 		pr_cont("%02x", (int)error_buff->buff[i]);

@@ -2269,8 +2269,8 @@ static void handle_error_indication(union ibmvnic_crq *crq,
 	dev_err(dev, "Firmware reports %serror id %x, cause %d\n",
 		crq->error_indication.
 		    flags & IBMVNIC_FATAL_ERROR ? "FATAL " : "",
-		crq->error_indication.error_id,
-		crq->error_indication.error_cause);
+		be32_to_cpu(crq->error_indication.error_id),
+		be16_to_cpu(crq->error_indication.error_cause));
 
 	error_buff = kmalloc(sizeof(*error_buff), GFP_ATOMIC);
 	if (!error_buff)

-- 
1.8.3.1

Initialize this completion structure before requesting that
a buffer be long-term mapped . This fix resolves a bug where firmware
sends a response before the structure is initialized.

Signed-off-by: John Allen <redacted>
Signed-off-by: Nathan Fontenot <redacted>
Signed-off-by: Thomas Falcon <redacted>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index ec6c5fe..d1ffc61 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c

@@ -189,9 +189,9 @@ static int alloc_long_term_buff(struct ibmvnic_adapter *adapter,
 	}
 	ltb->map_id = adapter->map_id;
 	adapter->map_id++;
+	init_completion(&adapter->fw_done);
 	send_request_map(adapter, ltb->addr,
 			 ltb->size, ltb->map_id);
-	init_completion(&adapter->fw_done);
 	wait_for_completion(&adapter->fw_done);
 	return 0;
 }

-- 
1.8.3.1

From: Thomas Falcon <redacted>
Date: Wed, 25 Jan 2017 15:02:19 -0600

Move most interrupt handler processing into a tasklet, and
introduce a delay after re-enabling interrupts to fix timing
issues encountered in hardware testing.

Signed-off-by: Thomas Falcon <redacted>

I don't think you have any idea what the real problem is.  This looks
like a hack, at best.  Next patch you'll increase the delay to "20",
right?  And if that doesn't work you'll try "40".

Or if you do know the reason, you need to explain it in detail in this
commit message so that we can properly evaluate this patch.

Furthermore, if you're going to move all of your packet processing
into software interrupt context, you might as well use NAPI polling
which is a purposefully built piece of infrastructure for doing
exactly this.

From: Thomas Falcon <redacted>
Date: Wed, 25 Jan 2017 15:02:20 -0600

In the current driver, the MTU is set to the maximum value
capable for the backing device. This patch sets the MTU to the
default value for a Linux net device.

Why are you doing this?

What happens to users who depend upon the current behavior.

They will break, and that isn't acceptable.

On 01/25/2017 10:04 PM, David Miller wrote:

From: Thomas Falcon <redacted>
Date: Wed, 25 Jan 2017 15:02:19 -0600

quoted

Move most interrupt handler processing into a tasklet, and
introduce a delay after re-enabling interrupts to fix timing
issues encountered in hardware testing.

Signed-off-by: Thomas Falcon <redacted>

I don't think you have any idea what the real problem is.  This looks
like a hack, at best.  Next patch you'll increase the delay to "20",
right?  And if that doesn't work you'll try "40".

Or if you do know the reason, you need to explain it in detail in this
commit message so that we can properly evaluate this patch.

You're right, I should have given more explanation in the commit message about the bug encountered and our reasons for this sort of fix.  The issue is that there are some scenarios during the device init where multiple messages are sent by firmware in one interrupt request. 

We have observed behavior where messages are delayed, resulting in the interrupt handler completing before the delayed messages can be processed, fouling up the device bring-up in the device probing and elsewhere.  The goal of the delay is to buy some time for the hypervisor to forward all the CRQ messages from the firmware.

Furthermore, if you're going to move all of your packet processing
into software interrupt context, you might as well use NAPI polling
which is a purposefully built piece of infrastructure for doing
exactly this.

This interrupt handler doesn't handle packet processing, but communications between the driver and firmware for device settings and resource allocation.  Packet processing is done with different interrupts that do use NAPI polling.

On 01/25/2017 10:05 PM, David Miller wrote:

From: Thomas Falcon <redacted>
Date: Wed, 25 Jan 2017 15:02:20 -0600

quoted

In the current driver, the MTU is set to the maximum value
capable for the backing device. This patch sets the MTU to the
default value for a Linux net device.

Why are you doing this?

What happens to users who depend upon the current behavior.

They will break, and that isn't acceptable.

The current behavior was already broken.  We were seeing firmware errors when sending jumbo sized packets .  We plan to add proper support for jumbo sized packets as soon as possible.  

From: Thomas Falcon <redacted>
Date: Thu, 26 Jan 2017 10:44:22 -0600

On 01/25/2017 10:04 PM, David Miller wrote:

quoted

From: Thomas Falcon <redacted>
Date: Wed, 25 Jan 2017 15:02:19 -0600

quoted

Move most interrupt handler processing into a tasklet, and
introduce a delay after re-enabling interrupts to fix timing
issues encountered in hardware testing.

Signed-off-by: Thomas Falcon <redacted>

I don't think you have any idea what the real problem is.  This looks
like a hack, at best.  Next patch you'll increase the delay to "20",
right?  And if that doesn't work you'll try "40".

Or if you do know the reason, you need to explain it in detail in this
commit message so that we can properly evaluate this patch.

You're right, I should have given more explanation in the commit message about the bug encountered and our reasons for this sort of fix.  The issue is that there are some scenarios during the device init where multiple messages are sent by firmware in one interrupt request. 

We have observed behavior where messages are delayed, resulting in the interrupt handler completing before the delayed messages can be processed, fouling up the device bring-up in the device probing and elsewhere.  The goal of the delay is to buy some time for the hypervisor to forward all the CRQ messages from the firmware.

Then isn't there an event you can sleep and wait for, or a piece of state for
you to poll and test for in a timeout based loop?

This delay is a bad solution for the problem of waiting for A to happen
before you do B.

From: Thomas Falcon <redacted>
Date: Thu, 26 Jan 2017 10:44:31 -0600

On 01/25/2017 10:05 PM, David Miller wrote:

quoted

From: Thomas Falcon <redacted>
Date: Wed, 25 Jan 2017 15:02:20 -0600

quoted

In the current driver, the MTU is set to the maximum value
capable for the backing device. This patch sets the MTU to the
default value for a Linux net device.

Why are you doing this?

What happens to users who depend upon the current behavior.

They will break, and that isn't acceptable.

The current behavior was already broken.  We were seeing firmware errors when sending jumbo sized packets .  We plan to add proper support for jumbo sized packets as soon as possible.  

Need to be explained, with a full detailed history of how things got this way,
in your commit message.

 

On Wed, 25 Jan 2017 15:02:19 -0600
Thomas Falcon [off-list ref] wrote:

 static irqreturn_t ibmvnic_interrupt(int irq, void *instance)
 {
 	struct ibmvnic_adapter *adapter = instance;
+	unsigned long flags;
+
+	spin_lock_irqsave(&adapter->crq.lock, flags);
+	vio_disable_interrupts(adapter->vdev);
+	tasklet_schedule(&adapter->tasklet);
+	spin_unlock_irqrestore(&adapter->crq.lock, flags);
+	return IRQ_HANDLED;
+}
+

Why not use NAPI? rather than a tasklet

On 01/26/2017 11:56 AM, David Miller wrote:

From: Thomas Falcon <redacted>
Date: Thu, 26 Jan 2017 10:44:22 -0600

quoted

On 01/25/2017 10:04 PM, David Miller wrote:

quoted

From: Thomas Falcon <redacted>
Date: Wed, 25 Jan 2017 15:02:19 -0600

quoted

Move most interrupt handler processing into a tasklet, and
introduce a delay after re-enabling interrupts to fix timing
issues encountered in hardware testing.

Signed-off-by: Thomas Falcon <redacted>

I don't think you have any idea what the real problem is.  This looks
like a hack, at best.  Next patch you'll increase the delay to "20",
right?  And if that doesn't work you'll try "40".

Or if you do know the reason, you need to explain it in detail in this
commit message so that we can properly evaluate this patch.

You're right, I should have given more explanation in the commit message about the bug encountered and our reasons for this sort of fix.  The issue is that there are some scenarios during the device init where multiple messages are sent by firmware in one interrupt request. 

We have observed behavior where messages are delayed, resulting in the interrupt handler completing before the delayed messages can be processed, fouling up the device bring-up in the device probing and elsewhere.  The goal of the delay is to buy some time for the hypervisor to forward all the CRQ messages from the firmware.

Then isn't there an event you can sleep and wait for, or a piece of state for
you to poll and test for in a timeout based loop?

This delay is a bad solution for the problem of waiting for A to happen
before you do B.

Understood.  We will come up with a better fix.  Thanks for your attention.

On 01/26/2017 12:28 PM, Stephen Hemminger wrote:

On Wed, 25 Jan 2017 15:02:19 -0600
Thomas Falcon [off-list ref] wrote:

quoted

 static irqreturn_t ibmvnic_interrupt(int irq, void *instance)
 {
 	struct ibmvnic_adapter *adapter = instance;
+	unsigned long flags;
+
+	spin_lock_irqsave(&adapter->crq.lock, flags);
+	vio_disable_interrupts(adapter->vdev);
+	tasklet_schedule(&adapter->tasklet);
+	spin_unlock_irqrestore(&adapter->crq.lock, flags);
+	return IRQ_HANDLED;
+}
+

Why not use NAPI? rather than a tasklet

This interrupt function doesn't process packets, but message passing between firmware and driver for determining device capabilities and available resources, such as the number TX and RX queues.