[PATCH net 0/7] ibmvnic: Assorted bug fixes

[PATCH net 0/7] ibmvnic: Assorted bug fixes

[Sukadev Bhattiprolu]

Assorted bug fixes that we tested over the last several weeks.

Thanks to Brian King, Cris Forno, Dany Madden and Rick Lindsley for
reviews and help with testing.

Dany Madden (1):
  Revert "ibmvnic: remove duplicate napi_schedule call in open function"

Sukadev Bhattiprolu (6):
  Revert "ibmvnic: simplify reset_long_term_buff function"
  ibmvnic: clean pending indirect buffs during reset
  ibmvnic: account for bufs already saved in indir_buf
  ibmvnic: set ltb->buff to NULL after freeing
  ibmvnic: free tx_pool if tso_pool alloc fails
  ibmvnic: parenthesize a check

 drivers/net/ethernet/ibm/ibmvnic.c | 101 ++++++++++++++++++++++-------
 1 file changed, 77 insertions(+), 24 deletions(-)

-- 
2.31.1

[PATCH net 1/7] Revert "ibmvnic: simplify reset_long_term_buff function"

[Sukadev Bhattiprolu]

This reverts commit 1c7d45e7b2c29080bf6c8cd0e213cc3cbb62a054.

We tried to optimize the number of hcalls we send and skipped sending
the REQUEST_MAP calls for some maps. However during resets, we need to
resend all the maps to the VIOS since the VIOS does not remember the
old values. In fact we may have failed over to a new VIOS which will
not have any of the mappings.

When we send packets with map ids the VIOS does not know about, it
triggers a FATAL reset. While the client does recover from the FATAL
error reset, we are seeing a large number of such resets. Handling
FATAL resets is lot more unnecessary work than issuing a few more
hcalls so revert the commit and resend the maps to the VIOS.

Fixes: 1c7d45e7b2c ("ibmvnic: simplify reset_long_term_buff function")
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 46 ++++++++++++++++++++++++------
 1 file changed, 38 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index adb0d5ca9ff1..f13ad6bc67cd 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -280,12 +280,40 @@ static void free_long_term_buff(struct ibmvnic_adapter *adapter,
 	dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
 }
 
-static int reset_long_term_buff(struct ibmvnic_long_term_buff *ltb)
+static int reset_long_term_buff(struct ibmvnic_adapter *adapter,
+				struct ibmvnic_long_term_buff *ltb)
 {
-	if (!ltb->buff)
-		return -EINVAL;
+	struct device *dev = &adapter->vdev->dev;
+	int rc;
 
 	memset(ltb->buff, 0, ltb->size);
+
+	mutex_lock(&adapter->fw_lock);
+	adapter->fw_done_rc = 0;
+
+	reinit_completion(&adapter->fw_done);
+	rc = send_request_map(adapter, ltb->addr, ltb->size, ltb->map_id);
+	if (rc) {
+		mutex_unlock(&adapter->fw_lock);
+		return rc;
+	}
+
+	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done, 10000);
+	if (rc) {
+		dev_info(dev,
+			 "Reset failed, long term map request timed out or aborted\n");
+		mutex_unlock(&adapter->fw_lock);
+		return rc;
+	}
+
+	if (adapter->fw_done_rc) {
+		dev_info(dev,
+			 "Reset failed, attempting to free and reallocate buffer\n");
+		free_long_term_buff(adapter, ltb);
+		mutex_unlock(&adapter->fw_lock);
+		return alloc_long_term_buff(adapter, ltb, ltb->size);
+	}
+	mutex_unlock(&adapter->fw_lock);
 	return 0;
 }
 
@@ -507,7 +535,8 @@ static int reset_rx_pools(struct ibmvnic_adapter *adapter)
 						  rx_pool->size *
 						  rx_pool->buff_size);
 		} else {
-			rc = reset_long_term_buff(&rx_pool->long_term_buff);
+			rc = reset_long_term_buff(adapter,
+						  &rx_pool->long_term_buff);
 		}
 
 		if (rc)
@@ -630,11 +659,12 @@ static int init_rx_pools(struct net_device *netdev)
 	return 0;
 }
 
-static int reset_one_tx_pool(struct ibmvnic_tx_pool *tx_pool)
+static int reset_one_tx_pool(struct ibmvnic_adapter *adapter,
+			     struct ibmvnic_tx_pool *tx_pool)
 {
 	int rc, i;
 
-	rc = reset_long_term_buff(&tx_pool->long_term_buff);
+	rc = reset_long_term_buff(adapter, &tx_pool->long_term_buff);
 	if (rc)
 		return rc;
 
@@ -661,10 +691,10 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
 
 	tx_scrqs = adapter->num_active_tx_pools;
 	for (i = 0; i < tx_scrqs; i++) {
-		rc = reset_one_tx_pool(&adapter->tso_pool[i]);
+		rc = reset_one_tx_pool(adapter, &adapter->tso_pool[i]);
 		if (rc)
 			return rc;
-		rc = reset_one_tx_pool(&adapter->tx_pool[i]);
+		rc = reset_one_tx_pool(adapter, &adapter->tx_pool[i]);
 		if (rc)
 			return rc;
 	}
-- 
2.31.1

[PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Sukadev Bhattiprolu]

From: Dany Madden <drt@linux.ibm.com>

This reverts commit 7c451f3ef676c805a4b77a743a01a5c21a250a73.

When a vnic interface is taken down and then up, connectivity is not
restored. We bisected it to this commit. Reverting this commit until
we can fully investigate the issue/benefit of the change.

Fixes: 7c451f3ef676 ("ibmvnic: remove duplicate napi_schedule call in open function")
Reported-by: Cristobal Forno <cforno12@linux.ibm.com>
Reported-by: Abdul Haleem <abdhalee@in.ibm.com>
Signed-off-by: Dany Madden <drt@linux.ibm.com>
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index f13ad6bc67cd..fe1627ea9762 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -1234,6 +1234,11 @@ static int __ibmvnic_open(struct net_device *netdev)
 
 	netif_tx_start_all_queues(netdev);
 
+	if (prev_state == VNIC_CLOSED) {
+		for (i = 0; i < adapter->req_rx_queues; i++)
+			napi_schedule(&adapter->napi[i]);
+	}
+
 	adapter->state = VNIC_OPEN;
 	return rc;
 }
-- 
2.31.1

[PATCH net 3/7] ibmvnic: clean pending indirect buffs during reset

[Sukadev Bhattiprolu]

We batch subordinate command response queue (scrq) descriptors that we
need to send to the VIOS using an "indirect" buffer. If after we queue
one or more scrqs in the indirect buffer encounter an error (say fail
to allocate an skb), we leave the queued scrq descriptors in the
indirect buffer until the next call to ibmvnic_xmit().

On the next call to ibmvnic_xmit(), it is possible that the adapter is
going through a reset and it is possible that the long term  buffers
have been unmapped on the VIOS side. If we proceed to flush (send) the
packets that are in the indirect buffer, we will end up using the old
map ids and this can cause the VIOS to trigger an unnecessary FATAL
error reset.

Instead of flushing packets remaining on the indirect_buff, discard
(clean) them instead.

Fixes: 0d973388185d4 ("ibmvnic: Introduce xmit_more support using batched subCRQ hcalls")
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index fe1627ea9762..fa402e20c137 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -106,6 +106,8 @@ static void release_crq_queue(struct ibmvnic_adapter *);
 static int __ibmvnic_set_mac(struct net_device *, u8 *);
 static int init_crq_queue(struct ibmvnic_adapter *adapter);
 static int send_query_phys_parms(struct ibmvnic_adapter *adapter);
+static void ibmvnic_tx_scrq_clean_buffer(struct ibmvnic_adapter *adapter,
+					 struct ibmvnic_sub_crq_queue *tx_scrq);
 
 struct ibmvnic_stat {
 	char name[ETH_GSTRING_LEN];
@@ -691,6 +693,7 @@ static int reset_tx_pools(struct ibmvnic_adapter *adapter)
 
 	tx_scrqs = adapter->num_active_tx_pools;
 	for (i = 0; i < tx_scrqs; i++) {
+		ibmvnic_tx_scrq_clean_buffer(adapter, adapter->tx_scrq[i]);
 		rc = reset_one_tx_pool(adapter, &adapter->tso_pool[i]);
 		if (rc)
 			return rc;
@@ -1643,7 +1646,8 @@ static void ibmvnic_tx_scrq_clean_buffer(struct ibmvnic_adapter *adapter,
 	ind_bufp->index = 0;
 	if (atomic_sub_return(entries, &tx_scrq->used) <=
 	    (adapter->req_tx_entries_per_subcrq / 2) &&
-	    __netif_subqueue_stopped(adapter->netdev, queue_num)) {
+	    __netif_subqueue_stopped(adapter->netdev, queue_num) &&
+	    !test_bit(0, &adapter->resetting)) {
 		netif_wake_subqueue(adapter->netdev, queue_num);
 		netdev_dbg(adapter->netdev, "Started queue %d\n",
 			   queue_num);
@@ -1713,7 +1717,6 @@ static netdev_tx_t ibmvnic_xmit(struct sk_buff *skb, struct net_device *netdev)
 		tx_send_failed++;
 		tx_dropped++;
 		ret = NETDEV_TX_OK;
-		ibmvnic_tx_scrq_flush(adapter, tx_scrq);
 		goto out;
 	}
 
@@ -3276,6 +3279,7 @@ static void release_sub_crqs(struct ibmvnic_adapter *adapter, bool do_h_free)
 
 			netdev_dbg(adapter->netdev, "Releasing tx_scrq[%d]\n",
 				   i);
+			ibmvnic_tx_scrq_clean_buffer(adapter, adapter->tx_scrq[i]);
 			if (adapter->tx_scrq[i]->irq) {
 				free_irq(adapter->tx_scrq[i]->irq,
 					 adapter->tx_scrq[i]);
-- 
2.31.1

[PATCH net 4/7] ibmvnic: account for bufs already saved in indir_buf

[Sukadev Bhattiprolu]

This fixes a crash in replenish_rx_pool() when called from ibmvnic_poll()
after a previous call to replenish_rx_pool() encountered an error when
allocating a socket buffer.

Thanks to Rick Lindsley and Dany Madden for helping debug the crash.

Fixes: 4f0b6812e9b9 ("ibmvnic: Introduce batched RX buffer descriptor transmission")
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index fa402e20c137..b1d7caaa4fb7 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -351,7 +351,14 @@ static void replenish_rx_pool(struct ibmvnic_adapter *adapter,
 
 	rx_scrq = adapter->rx_scrq[pool->index];
 	ind_bufp = &rx_scrq->ind_buf;
-	for (i = 0; i < count; ++i) {
+
+	/* netdev_skb_alloc() could have failed after we saved a few skbs
+	 * in the indir_buf and we would not have sent them to VIOS yet.
+	 * To account for them, start the loop at ind_bufp->index rather
+	 * than 0. If we pushed all the skbs to VIOS, ind_bufp->index will
+	 * be 0.
+	 */
+	for (i = ind_bufp->index; i < count; ++i) {
 		skb = netdev_alloc_skb(adapter->netdev, pool->buff_size);
 		if (!skb) {
 			dev_err(dev, "Couldn't replenish rx buff\n");
-- 
2.31.1

[PATCH net 5/7] ibmvnic: set ltb->buff to NULL after freeing

[Sukadev Bhattiprolu]

free_long_term_buff() checks ltb->buff to decide whether we have a long
term buffer to free. So set ltb->buff to NULL afer freeing. While here,
also clear ->map_id, fix up some coding style and log an error.

Fixes: 9c4eaabd1bb39 ("Check CRQ command return codes")
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index b1d7caaa4fb7..b56406ca90c0 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -234,12 +234,11 @@ static int alloc_long_term_buff(struct ibmvnic_adapter *adapter,
 	mutex_lock(&adapter->fw_lock);
 	adapter->fw_done_rc = 0;
 	reinit_completion(&adapter->fw_done);
-	rc = send_request_map(adapter, ltb->addr,
-			      ltb->size, ltb->map_id);
+
+	rc = send_request_map(adapter, ltb->addr, ltb->size, ltb->map_id);
 	if (rc) {
-		dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
-		mutex_unlock(&adapter->fw_lock);
-		return rc;
+		dev_err(dev, "send_request_map failed, rc = %d\n", rc);
+		goto out;
 	}
 
 	rc = ibmvnic_wait_for_completion(adapter, &adapter->fw_done, 10000);
@@ -247,20 +246,23 @@ static int alloc_long_term_buff(struct ibmvnic_adapter *adapter,
 		dev_err(dev,
 			"Long term map request aborted or timed out,rc = %d\n",
 			rc);
-		dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
-		mutex_unlock(&adapter->fw_lock);
-		return rc;
+		goto out;
 	}
 
 	if (adapter->fw_done_rc) {
 		dev_err(dev, "Couldn't map long term buffer,rc = %d\n",
 			adapter->fw_done_rc);
+		rc = -1;
+		goto out;
+	}
+	rc = 0;
+out:
+	if (rc) {
 		dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
-		mutex_unlock(&adapter->fw_lock);
-		return -1;
+		ltb->buff = NULL;
 	}
 	mutex_unlock(&adapter->fw_lock);
-	return 0;
+	return rc;
 }
 
 static void free_long_term_buff(struct ibmvnic_adapter *adapter,
@@ -280,6 +282,8 @@ static void free_long_term_buff(struct ibmvnic_adapter *adapter,
 	    adapter->reset_reason != VNIC_RESET_TIMEOUT)
 		send_request_unmap(adapter, ltb->map_id);
 	dma_free_coherent(dev, ltb->size, ltb->buff, ltb->addr);
+	ltb->buff = NULL;
+	ltb->map_id = 0;
 }
 
 static int reset_long_term_buff(struct ibmvnic_adapter *adapter,
-- 
2.31.1

[PATCH net 6/7] ibmvnic: free tx_pool if tso_pool alloc fails

[Sukadev Bhattiprolu]

Free tx_pool and clear it, if allocation of tso_pool fails.

release_tx_pools() assumes we have both tx and tso_pools if ->tx_pool is
non-NULL. If allocation of tso_pool fails in init_tx_pools(), the assumption
will not be true and we would end up dereferencing ->tx_buff, ->free_map
fields from a NULL pointer.

Fixes: 3205306c6b8d ("ibmvnic: Update TX pool initialization routine")
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmvnic.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
index b56406ca90c0..363a5d5503ad 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -801,8 +801,11 @@ static int init_tx_pools(struct net_device *netdev)
 
 	adapter->tso_pool = kcalloc(tx_subcrqs,
 				    sizeof(struct ibmvnic_tx_pool), GFP_KERNEL);
-	if (!adapter->tso_pool)
+	if (!adapter->tso_pool) {
+		kfree(adapter->tx_pool);
+		adapter->tx_pool = NULL;
 		return -1;
+	}
 
 	adapter->num_active_tx_pools = tx_subcrqs;
 
-- 
2.31.1

[PATCH net 7/7] ibmvnic: parenthesize a check

[Sukadev Bhattiprolu]

Parenthesize a check to be more explicit and to fix a sparse warning
seen on some distros.

Fixes: 91dc5d2553fbf ("ibmvnic: fix miscellaneous checks")
Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
---
 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 363a5d5503ad..697b9714fc76 100644
--- a/drivers/net/ethernet/ibm/ibmvnic.c
+++ b/drivers/net/ethernet/ibm/ibmvnic.c
@@ -3367,7 +3367,7 @@ static int enable_scrq_irq(struct ibmvnic_adapter *adapter,
 		/* H_EOI would fail with rc = H_FUNCTION when running
 		 * in XIVE mode which is expected, but not an error.
 		 */
-		if (rc && rc != H_FUNCTION)
+		if (rc && (rc != H_FUNCTION))
 			dev_err(dev, "H_EOI FAILED irq 0x%llx. rc=%ld\n",
 				val, rc);
 	}
-- 
2.31.1

Re: [PATCH net 7/7] ibmvnic: parenthesize a check

[Lijun Pan]

On Wed, Jun 23, 2021 at 11:17 PM Sukadev Bhattiprolu
<sukadev@linux.ibm.com> wrote:
>
> Parenthesize a check to be more explicit and to fix a sparse warning
> seen on some distros.
>
> Fixes: 91dc5d2553fbf ("ibmvnic: fix miscellaneous checks")
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> ---
>  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 363a5d5503ad..697b9714fc76 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -3367,7 +3367,7 @@ static int enable_scrq_irq(struct ibmvnic_adapter *adapter,
>                 /* H_EOI would fail with rc = H_FUNCTION when running
>                  * in XIVE mode which is expected, but not an error.
>                  */
> -               if (rc && rc != H_FUNCTION)
> +               if (rc && (rc != H_FUNCTION))

It is unnecessary and this is not an issue with the latest sparse.
Just curious what was the version on that distro?

Re: [PATCH net 1/7] Revert "ibmvnic: simplify reset_long_term_buff function"

[Lijun Pan]

On Wed, Jun 23, 2021 at 11:16 PM Sukadev Bhattiprolu
<sukadev@linux.ibm.com> wrote:
>
> This reverts commit 1c7d45e7b2c29080bf6c8cd0e213cc3cbb62a054.
>
> We tried to optimize the number of hcalls we send and skipped sending
> the REQUEST_MAP calls for some maps. However during resets, we need to
> resend all the maps to the VIOS since the VIOS does not remember the
> old values. In fact we may have failed over to a new VIOS which will
> not have any of the mappings.
>
> When we send packets with map ids the VIOS does not know about, it
> triggers a FATAL reset. While the client does recover from the FATAL
> error reset, we are seeing a large number of such resets. Handling
> FATAL resets is lot more unnecessary work than issuing a few more
> hcalls so revert the commit and resend the maps to the VIOS.
>

This was not an issue when the original optimization code was committed.
VIOS changes over time and it is proprietary code, so people don't really know
what it changes every time. If you believe the verbose hcall is really
necessary,
you'd better document that in the function/source code. This patch may
be reverted again
some time later when the verbose calling isn't needed.

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Lijun Pan]

On Wed, Jun 23, 2021 at 11:16 PM Sukadev Bhattiprolu
<sukadev@linux.ibm.com> wrote:
>
> From: Dany Madden <drt@linux.ibm.com>
>
> This reverts commit 7c451f3ef676c805a4b77a743a01a5c21a250a73.
>
> When a vnic interface is taken down and then up, connectivity is not
> restored. We bisected it to this commit. Reverting this commit until
> we can fully investigate the issue/benefit of the change.
>

The reverted patch shouldn't be the real cause of the problem.
It is very likely VIOS does not forward the rx packets so that the rx interrupt
isn't raised.

> Fixes: 7c451f3ef676 ("ibmvnic: remove duplicate napi_schedule call in open function")
> Reported-by: Cristobal Forno <cforno12@linux.ibm.com>
> Reported-by: Abdul Haleem <abdhalee@in.ibm.com>
> Signed-off-by: Dany Madden <drt@linux.ibm.com>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.ibm.com>
> ---
>  drivers/net/ethernet/ibm/ibmvnic.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
> index f13ad6bc67cd..fe1627ea9762 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -1234,6 +1234,11 @@ static int __ibmvnic_open(struct net_device *netdev)
>
>         netif_tx_start_all_queues(netdev);
>
> +       if (prev_state == VNIC_CLOSED) {
> +               for (i = 0; i < adapter->req_rx_queues; i++)
> +                       napi_schedule(&adapter->napi[i]);
> +       }
> +

interrupt_rx will schedule the napi, so not necessary here.

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Rick Lindsley]

On 6/23/21 11:20 PM, Lijun Pan wrote:
> On Wed, Jun 23, 2021 at 11:16 PM Sukadev Bhattiprolu

> It is very likely VIOS does not forward the rx packets so that the rx interrupt
> isn't raised.

On what do you base this position? Do you have some concrete data that indicates this?

As noted, bisect led us here.  With the previous patch applied, bug occurs.  With the previous patch removed, it does not.  As the description says, we are reverting the patch until it can be determined whether the problem is the patch itself or that the patch better magnifies some other problem.

Rick

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Johaan Smith]

On Wed, Jun 23, 2021 at 11:17 PM Sukadev Bhattiprolu
<sukadev@linux.ibm.com> wrote:
>
> From: Dany Madden <drt@linux.ibm.com>
>
> This reverts commit 7c451f3ef676c805a4b77a743a01a5c21a250a73.
>
> When a vnic interface is taken down and then up, connectivity is not
> restored. We bisected it to this commit. Reverting this commit until
> we can fully investigate the issue/benefit of the change.
>

Sometimes git bisect may lead us to the false positives. Full investigation is
always the best solution if it is not too hard.

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Rick Lindsley]

On 6/23/21 11:20 PM, Lijun Pan wrote:

>> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c b/drivers/net/ethernet/ibm/ibmvnic.c
>> index f13ad6bc67cd..fe1627ea9762 100644
>> --- a/drivers/net/ethernet/ibm/ibmvnic.c
>> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
>> @@ -1234,6 +1234,11 @@ static int __ibmvnic_open(struct net_device *netdev)
>>
>>          netif_tx_start_all_queues(netdev);
>>
>> +       if (prev_state == VNIC_CLOSED) {
>> +               for (i = 0; i < adapter->req_rx_queues; i++)
>> +                       napi_schedule(&adapter->napi[i]);
>> +       }
>> +
> 
> interrupt_rx will schedule the napi, so not necessary here.

You keep saying this, but yet there is some case with the original patch that leaves napi unscheduled and the device unresponsive.  Until that is better understood, this patch should be reverted - especially when you consider that calling napi_schedule() when the rx_queue is already scheduled is harmless.  The original patch did not address any specific problem, but did introduce one.

Rick

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Rick Lindsley]

On 6/24/21 12:02 AM, Johaan Smith wrote:
> On Wed, Jun 23, 2021 at 11:17 PM Sukadev Bhattiprolu

> Sometimes git bisect may lead us to the false positives. Full investigation is
> always the best solution if it is not too hard.

I'd be happy to view evidence that points at another patch, if someone has some.
But the original patch did not address any observed problem:

      "So there is no need for do_reset to call napi_schedule again
       at the end of the function though napi_schedule will neglect
       the request if napi is already scheduled."

Given that it fixed no problem but appears to have introduced one, the efficient
action is to revert it for now.

Re: [PATCH net 1/7] Revert "ibmvnic: simplify reset_long_term_buff function"

[Sukadev Bhattiprolu]

Lijun Pan [lijunp213@gmail.com] wrote:
> On Wed, Jun 23, 2021 at 11:16 PM Sukadev Bhattiprolu
> <sukadev@linux.ibm.com> wrote:
> >
> > This reverts commit 1c7d45e7b2c29080bf6c8cd0e213cc3cbb62a054.
> >
> > We tried to optimize the number of hcalls we send and skipped sending
> > the REQUEST_MAP calls for some maps. However during resets, we need to
> > resend all the maps to the VIOS since the VIOS does not remember the
> > old values. In fact we may have failed over to a new VIOS which will
> > not have any of the mappings.
> >
> > When we send packets with map ids the VIOS does not know about, it
> > triggers a FATAL reset. While the client does recover from the FATAL
> > error reset, we are seeing a large number of such resets. Handling
> > FATAL resets is lot more unnecessary work than issuing a few more
> > hcalls so revert the commit and resend the maps to the VIOS.
> >
> 
> This was not an issue when the original optimization code was committed.
> VIOS changes over time and it is proprietary code, so people don't really know
> what it changes every time.

All the more reason to be careful about ripping out code.

>If you believe the verbose hcall is really necessary,
> you'd better document that in the function/source code.

It was necessary and present until you removed it. I am reverting it
after lot of debugging and with sufficient explanation. Feel free to
submit a new patch.

>This patch may be reverted again
> some time later when the verbose calling isn't needed.

Hopefully not without sufficient testing.

Sukadev

Sukadev

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Lijun Pan]

On Thu, Jun 24, 2021 at 2:05 AM Johaan Smith <johaansmith74@gmail.com> wrote:
>
> On Wed, Jun 23, 2021 at 11:17 PM Sukadev Bhattiprolu
> <sukadev@linux.ibm.com> wrote:
> >
> > From: Dany Madden <drt@linux.ibm.com>
> >
> > This reverts commit 7c451f3ef676c805a4b77a743a01a5c21a250a73.
> >
> > When a vnic interface is taken down and then up, connectivity is not
> > restored. We bisected it to this commit. Reverting this commit until
> > we can fully investigate the issue/benefit of the change.
> >
>
> Sometimes git bisect may lead us to the false positives. Full investigation is
> always the best solution if it is not too hard.

Agreed.

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Lijun Pan]

On Thu, Jun 24, 2021 at 2:29 AM Rick Lindsley
<ricklind@linux.vnet.ibm.com> wrote:
>
> On 6/24/21 12:02 AM, Johaan Smith wrote:
> > On Wed, Jun 23, 2021 at 11:17 PM Sukadev Bhattiprolu
>
> > Sometimes git bisect may lead us to the false positives. Full investigation is
> > always the best solution if it is not too hard.
>
> I'd be happy to view evidence that points at another patch, if someone has some.
> But the original patch did not address any observed problem:
>
>       "So there is no need for do_reset to call napi_schedule again
>        at the end of the function though napi_schedule will neglect
>        the request if napi is already scheduled."
>
> Given that it fixed no problem but appears to have introduced one, the efficient
> action is to revert it for now.
>

With this reverted patch, there are lots of errors after bringing
device down then up, e.g.,
"ibmvnic 30000003 env3: rx buffer returned with rc 6".
That seems something wrong with the handling of set_link_state
DOWN/UP. It is either the communication protocol or the VIOS itself.

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Dany Madden]

On 2021-06-24 10:05, Lijun Pan wrote:
> On Thu, Jun 24, 2021 at 2:29 AM Rick Lindsley
> <ricklind@linux.vnet.ibm.com> wrote:
>> 
>> On 6/24/21 12:02 AM, Johaan Smith wrote:
>> > On Wed, Jun 23, 2021 at 11:17 PM Sukadev Bhattiprolu
>> 
>> > Sometimes git bisect may lead us to the false positives. Full investigation is
>> > always the best solution if it is not too hard.
>> 
>> I'd be happy to view evidence that points at another patch, if someone 
>> has some.
>> But the original patch did not address any observed problem:
>> 
>>       "So there is no need for do_reset to call napi_schedule again
>>        at the end of the function though napi_schedule will neglect
>>        the request if napi is already scheduled."
>> 
>> Given that it fixed no problem but appears to have introduced one, the 
>> efficient
>> action is to revert it for now.
>> 
> 
> With this reverted patch, there are lots of errors after bringing
> device down then up, e.g.,
> "ibmvnic 30000003 env3: rx buffer returned with rc 6".
> That seems something wrong with the handling of set_link_state
> DOWN/UP. It is either the communication protocol or the VIOS itself.

Did the driver bring the link back up with the patch is reverted? When 
link is down, vnic server returns rx buffers back to the client. This 
error message is printed when debug is turned on, driver's way to log 
what happened.

Re: [PATCH net 0/7] ibmvnic: Assorted bug fixes

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Wed, 23 Jun 2021 21:13:09 -0700 you wrote:
> Assorted bug fixes that we tested over the last several weeks.
> 
> Thanks to Brian King, Cris Forno, Dany Madden and Rick Lindsley for
> reviews and help with testing.
> 
> Dany Madden (1):
>   Revert "ibmvnic: remove duplicate napi_schedule call in open function"
> 
> [...]

Here is the summary with links:
  - [net,1/7] Revert "ibmvnic: simplify reset_long_term_buff function"
    https://git.kernel.org/netdev/net/c/0ec13aff058a
  - [net,2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"
    https://git.kernel.org/netdev/net/c/2ca220f92878
  - [net,3/7] ibmvnic: clean pending indirect buffs during reset
    https://git.kernel.org/netdev/net/c/65d6470d139a
  - [net,4/7] ibmvnic: account for bufs already saved in indir_buf
    https://git.kernel.org/netdev/net/c/72368f8b2b9e
  - [net,5/7] ibmvnic: set ltb->buff to NULL after freeing
    https://git.kernel.org/netdev/net/c/552a33729f1a
  - [net,6/7] ibmvnic: free tx_pool if tso_pool alloc fails
    https://git.kernel.org/netdev/net/c/f6ebca8efa52
  - [net,7/7] ibmvnic: parenthesize a check
    https://git.kernel.org/netdev/net/c/154b3b2a6ffc

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

Re: [PATCH net 2/7] Revert "ibmvnic: remove duplicate napi_schedule call in open function"

[Rick Lindsley]

On 6/24/21 10:05 AM, Lijun Pan wrote:

> With this reverted patch, there are lots of errors after bringing
> device down then up, e.g.,
> "ibmvnic 30000003 env3: rx buffer returned with rc 6".

Ok, thanks.  Can you provide some more details about your test?  When you
ran this test, did you include the rest of the patches in this series,
only some, or no others at all?  I assume it was based on the contents
of net from 6/23 or 6/24 - is that correct? Can you also describe the
hardware you ran your test on?  And lastly, would you briefly describe
the nature of your test?

The message you quoted is only seen with debug turned on, and as Dany
remarked, can be expected and normal in some recovery situations. It's
always possible you've found a case undetected by our testing, so the
above information can help us better understand what you saw.