[PATCH net-next] ibmveth: Always stop tx queues during close

[PATCH net-next] ibmveth: Always stop tx queues during close

[Nick Child]

netif_stop_all_queues must be called before calling H_FREE_LOGICAL_LAN.
As a result, we can remove the pool_config field from the ibmveth
adapter structure.

Some device configuration changes call ibmveth_close in order to free
the current resources held by the device. These functions then make
their changes and call ibmveth_open to reallocate and reserve resources
for the device.

Prior to this commit, the flag pool_config was used to tell ibmveth_close
that it should not halt the transmit queue. pool_config was introduced in
commit 860f242eb534 ("[PATCH] ibmveth change buffer pools dynamically")
to avoid interrupting the tx flow when making rx config changes. Since
then, other commits adopted this approach, even if making tx config
changes.

The issue with this approach was that the hypervisor freed all of
the devices control structures after the hcall H_FREE_LOGICAL_LAN
was performed but the transmit queues were never stopped. So the higher
layers in the network stack would continue transmission but any
H_SEND_LOGICAL_LAN hcall would fail with H_PARAMETER until the
hypervisor's structures for the device were allocated with the
H_REGISTER_LOGICAL_LAN hcall in ibmveth_open.

So, instead of trying to keep the transmit queues alive during network
configuration changes, just stop the queues, make necessary changes then
restart the queues.

Signed-off-by: Nick Child <nnac123@linux.ibm.com>
---
 drivers/net/ethernet/ibm/ibmveth.c | 18 +-----------------
 drivers/net/ethernet/ibm/ibmveth.h |  1 -
 2 files changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
index 3b14dc93f59d..7d79006250ae 100644
--- a/drivers/net/ethernet/ibm/ibmveth.c
+++ b/drivers/net/ethernet/ibm/ibmveth.c
@@ -690,8 +690,7 @@ static int ibmveth_close(struct net_device *netdev)
 
 	napi_disable(&adapter->napi);
 
-	if (!adapter->pool_config)
-		netif_tx_stop_all_queues(netdev);
+	netif_tx_stop_all_queues(netdev);
 
 	h_vio_signal(adapter->vdev->unit_address, VIO_IRQ_DISABLE);
 
@@ -799,9 +798,7 @@ static int ibmveth_set_csum_offload(struct net_device *dev, u32 data)
 
 	if (netif_running(dev)) {
 		restart = 1;
-		adapter->pool_config = 1;
 		ibmveth_close(dev);
-		adapter->pool_config = 0;
 	}
 
 	set_attr = 0;
@@ -883,9 +880,7 @@ static int ibmveth_set_tso(struct net_device *dev, u32 data)
 
 	if (netif_running(dev)) {
 		restart = 1;
-		adapter->pool_config = 1;
 		ibmveth_close(dev);
-		adapter->pool_config = 0;
 	}
 
 	set_attr = 0;
@@ -1535,9 +1530,7 @@ static int ibmveth_change_mtu(struct net_device *dev, int new_mtu)
 	   only the buffer pools necessary to hold the new MTU */
 	if (netif_running(adapter->netdev)) {
 		need_restart = 1;
-		adapter->pool_config = 1;
 		ibmveth_close(adapter->netdev);
-		adapter->pool_config = 0;
 	}
 
 	/* Look for an active buffer pool that can hold the new MTU */
@@ -1701,7 +1694,6 @@ static int ibmveth_probe(struct vio_dev *dev, const struct vio_device_id *id)
 	adapter->vdev = dev;
 	adapter->netdev = netdev;
 	adapter->mcastFilterSize = be32_to_cpu(*mcastFilterSize_p);
-	adapter->pool_config = 0;
 	ibmveth_init_link_settings(netdev);
 
 	netif_napi_add_weight(netdev, &adapter->napi, ibmveth_poll, 16);
@@ -1841,9 +1833,7 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
 					return -ENOMEM;
 				}
 				pool->active = 1;
-				adapter->pool_config = 1;
 				ibmveth_close(netdev);
-				adapter->pool_config = 0;
 				if ((rc = ibmveth_open(netdev)))
 					return rc;
 			} else {
@@ -1869,10 +1859,8 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
 			}
 
 			if (netif_running(netdev)) {
-				adapter->pool_config = 1;
 				ibmveth_close(netdev);
 				pool->active = 0;
-				adapter->pool_config = 0;
 				if ((rc = ibmveth_open(netdev)))
 					return rc;
 			}
@@ -1883,9 +1871,7 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
 			return -EINVAL;
 		} else {
 			if (netif_running(netdev)) {
-				adapter->pool_config = 1;
 				ibmveth_close(netdev);
-				adapter->pool_config = 0;
 				pool->size = value;
 				if ((rc = ibmveth_open(netdev)))
 					return rc;
@@ -1898,9 +1884,7 @@ static ssize_t veth_pool_store(struct kobject *kobj, struct attribute *attr,
 			return -EINVAL;
 		} else {
 			if (netif_running(netdev)) {
-				adapter->pool_config = 1;
 				ibmveth_close(netdev);
-				adapter->pool_config = 0;
 				pool->buff_size = value;
 				if ((rc = ibmveth_open(netdev)))
 					return rc;
diff --git a/drivers/net/ethernet/ibm/ibmveth.h b/drivers/net/ethernet/ibm/ibmveth.h
index daf6f615c03f..4f8357187292 100644
--- a/drivers/net/ethernet/ibm/ibmveth.h
+++ b/drivers/net/ethernet/ibm/ibmveth.h
@@ -146,7 +146,6 @@ struct ibmveth_adapter {
     dma_addr_t filter_list_dma;
     struct ibmveth_buff_pool rx_buff_pool[IBMVETH_NUM_BUFF_POOLS];
     struct ibmveth_rx_q rx_queue;
-    int pool_config;
     int rx_csum;
     int large_send;
     bool is_active_trunk;
-- 
2.31.1

Re: [PATCH net-next] ibmveth: Always stop tx queues during close

[Jakub Kicinski]

On Mon, 17 Oct 2022 10:17:43 -0500 Nick Child wrote:
> The issue with this approach was that the hypervisor freed all of
> the devices control structures after the hcall H_FREE_LOGICAL_LAN
> was performed but the transmit queues were never stopped. So the higher
> layers in the network stack would continue transmission but any
> H_SEND_LOGICAL_LAN hcall would fail with H_PARAMETER until the
> hypervisor's structures for the device were allocated with the
> H_REGISTER_LOGICAL_LAN hcall in ibmveth_open.

Sounds like we should treat this one as a fix as well?
How far back is it safe to backport?

Re: [PATCH net-next] ibmveth: Always stop tx queues during close

[Nick Child]

Hello Jakub,

Thanks for the review,

On 10/18/22 13:47, Jakub Kicinski wrote:
> On Mon, 17 Oct 2022 10:17:43 -0500 Nick Child wrote:
>> The issue with this approach was that the hypervisor freed all of
>> the devices control structures after the hcall H_FREE_LOGICAL_LAN
>> was performed but the transmit queues were never stopped. So the higher
>> layers in the network stack would continue transmission but any
>> H_SEND_LOGICAL_LAN hcall would fail with H_PARAMETER until the
>> hypervisor's structures for the device were allocated with the
>> H_REGISTER_LOGICAL_LAN hcall in ibmveth_open.
> 
> Sounds like we should treat this one as a fix as well?

Agreed. I will resend with `net` tag.


> How far back is it safe to backport?
The issue is introduced in commit 860f242eb534 ("[PATCH] ibmveth change 
buffer pools dynamically") which first appears in v3.19.
Please let me know if I should resend with "Fixes:".

Since the aforementioned commit, multiple other patches have mimicked 
the behavior. Because the same bad logic is used in multiple places, 
their will be multiple merge conflicts depending on the LTS branch. From
what I can tell the patch will differ between the one posted here 
(v6.1), LTS v4.14-5.15, and LTS v4.9.

Since the issue only results in annoying "failed to send" driver 
messages and is under no pressure to get backported from end users, we 
are okay if it does not get backported. That being said, if you would 
like to see it backported, I can send you the patches that would apply
cleanly to v4.9 and v4.14-v5.15.

If there is anything else I can do please let me know.

Thanks again,
Nick Child

Re: [PATCH net-next] ibmveth: Always stop tx queues during close

[Jakub Kicinski]

On Tue, 18 Oct 2022 16:00:14 -0500 Nick Child wrote:
> Since the issue only results in annoying "failed to send" driver 
> messages and is under no pressure to get backported from end users, we 
> are okay if it does not get backported. That being said, if you would 
> like to see it backported, I can send you the patches that would apply
> cleanly to v4.9 and v4.14-v5.15.

If it's just a error message in the logs net-next is fine.
Please repost with that clarified in the commit message, tho.