diff mbox series

[v2] net/iavf: preserve MAC address with i40e PF Linux driver

Message ID 20241001091254.98540-1-david.marchand@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Bruce Richardson
Headers
Series [v2] net/iavf: preserve MAC address with i40e PF Linux driver |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-marvell-Functional success Functional Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS

Commit Message

David Marchand Oct. 1, 2024, 9:12 a.m. UTC 
Following two upstream Linux kernel changes (see links), the mac address
of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
driver probes the port again (which may be triggered at any point of a
DPDK application life, like when a reset event is triggered by the PF).

A first change results in the mac address of the VF port being reset to 0
during the VIRTCHNL_OP_GET_VF_RESOURCES query.
The i40e PF driver change is pretty obscure but the iavf Linux driver does
set VIRTCHNL_VF_OFFLOAD_USO.
Announcing such a capability in the DPDK driver does not seem to be an
issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.

Then a second change in the kernel results in the VF mac address being
cleared when the VF driver remove its default mac address.
Removing (unicast or multicast) mac addresses is not done by the kernel VF
driver in general.
The reason why the DPDK driver behaves like this is undocumented
(and lost because the authors are not active anymore).
Aligning DPDK behavior to the upstream kernel driver is safer in any
case.

Cc: stable@dpdk.org

Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 drivers/net/iavf/iavf_ethdev.c | 22 +++++-----------------
 drivers/net/iavf/iavf_vchnl.c  |  1 +
 2 files changed, 6 insertions(+), 17 deletions(-)

Comments

Bruce Richardson Oct. 1, 2024, 10:08 a.m. UTC | #1 
On Tue, Oct 01, 2024 at 11:12:54AM +0200, David Marchand wrote:
> Following two upstream Linux kernel changes (see links), the mac address
> of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
> driver probes the port again (which may be triggered at any point of a
> DPDK application life, like when a reset event is triggered by the PF).
> 
> A first change results in the mac address of the VF port being reset to 0
> during the VIRTCHNL_OP_GET_VF_RESOURCES query.
> The i40e PF driver change is pretty obscure but the iavf Linux driver does
> set VIRTCHNL_VF_OFFLOAD_USO.
> Announcing such a capability in the DPDK driver does not seem to be an
> issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.
> 
> Then a second change in the kernel results in the VF mac address being
> cleared when the VF driver remove its default mac address.
> Removing (unicast or multicast) mac addresses is not done by the kernel VF
> driver in general.
> The reason why the DPDK driver behaves like this is undocumented
> (and lost because the authors are not active anymore).
> Aligning DPDK behavior to the upstream kernel driver is safer in any
> case.
> 

One question inline below.

/Bruce

> Cc: stable@dpdk.org
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  drivers/net/iavf/iavf_ethdev.c | 22 +++++-----------------
>  drivers/net/iavf/iavf_vchnl.c  |  1 +
>  2 files changed, 6 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
> index 11036bc179..97cdb1cbe0 100644
> --- a/drivers/net/iavf/iavf_ethdev.c
> +++ b/drivers/net/iavf/iavf_ethdev.c
> @@ -1044,7 +1044,7 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  		if (iavf_configure_queues(adapter,
>  				IAVF_CFG_Q_NUM_PER_BUF, index) != 0) {
>  			PMD_DRV_LOG(ERR, "configure queues failed");
> -			goto err_queue;
> +			goto error;
>  		}
>  		num_queue_pairs -= IAVF_CFG_Q_NUM_PER_BUF;
>  		index += IAVF_CFG_Q_NUM_PER_BUF;
> @@ -1052,12 +1052,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  
>  	if (iavf_configure_queues(adapter, num_queue_pairs, index) != 0) {
>  		PMD_DRV_LOG(ERR, "configure queues failed");
> -		goto err_queue;
> +		goto error;
>  	}
>  
>  	if (iavf_config_rx_queues_irqs(dev, intr_handle) != 0) {
>  		PMD_DRV_LOG(ERR, "configure irq failed");
> -		goto err_queue;
> +		goto error;
>  	}
>  	/* re-enable intr again, because efd assign may change */
>  	if (dev->data->dev_conf.intr_conf.rxq != 0) {
> @@ -1077,14 +1077,12 @@ iavf_dev_start(struct rte_eth_dev *dev)
>  
>  	if (iavf_start_queues(dev) != 0) {
>  		PMD_DRV_LOG(ERR, "enable queues failed");
> -		goto err_mac;
> +		goto error;
>  	}
>  
>  	return 0;
>  
> -err_mac:
> -	iavf_add_del_all_mac_addr(adapter, false);
> -err_queue:
> +error:
>  	return -1;
>  }
>  
> @@ -1113,16 +1111,6 @@ iavf_dev_stop(struct rte_eth_dev *dev)
>  	/* Rx interrupt vector mapping free */
>  	rte_intr_vec_list_free(intr_handle);
>  
> -	/* adminq will be disabled when vf is resetting. */
> -	if (!vf->in_reset_recovery) {
> -		/* remove all mac addrs */
> -		iavf_add_del_all_mac_addr(adapter, false);
> -
> -		/* remove all multicast addresses */
> -		iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
> -				  false);
> -	}
> -

Question on this: while I understand we don't want to remove the default
mac address, should all other non-default macs not still be removed?
David Marchand Oct. 1, 2024, 1:07 p.m. UTC | #2 
On Tue, Oct 1, 2024 at 12:09 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
> > @@ -1113,16 +1111,6 @@ iavf_dev_stop(struct rte_eth_dev *dev)
> >       /* Rx interrupt vector mapping free */
> >       rte_intr_vec_list_free(intr_handle);
> >
> > -     /* adminq will be disabled when vf is resetting. */
> > -     if (!vf->in_reset_recovery) {
> > -             /* remove all mac addrs */
> > -             iavf_add_del_all_mac_addr(adapter, false);
> > -
> > -             /* remove all multicast addresses */
> > -             iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
> > -                               false);
> > -     }
> > -
>
> Question on this: while I understand we don't want to remove the default
> mac address, should all other non-default macs not still be removed?

I would say that we don't care.
Right after this, the VF driver sends a VF reset (following your
recent change), and the PF flushes any mac associated to the VF from
hw filters.

The kernel VF driver does nothing about mac addresses when uninitialising.
Bruce Richardson Oct. 1, 2024, 1:14 p.m. UTC | #3 
On Tue, Oct 01, 2024 at 11:12:54AM +0200, David Marchand wrote:
> Following two upstream Linux kernel changes (see links), the mac address
> of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
> driver probes the port again (which may be triggered at any point of a
> DPDK application life, like when a reset event is triggered by the PF).
> 
> A first change results in the mac address of the VF port being reset to 0
> during the VIRTCHNL_OP_GET_VF_RESOURCES query.
> The i40e PF driver change is pretty obscure but the iavf Linux driver does
> set VIRTCHNL_VF_OFFLOAD_USO.
> Announcing such a capability in the DPDK driver does not seem to be an
> issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.
> 
> Then a second change in the kernel results in the VF mac address being
> cleared when the VF driver remove its default mac address.
> Removing (unicast or multicast) mac addresses is not done by the kernel VF
> driver in general.
> The reason why the DPDK driver behaves like this is undocumented
> (and lost because the authors are not active anymore).
> Aligning DPDK behavior to the upstream kernel driver is safer in any
> case.
> 
> Cc: stable@dpdk.org
> 
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
> Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Seems reasonable to me.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Bruce Richardson Oct. 15, 2024, 5:03 p.m. UTC | #4 
On Tue, Oct 01, 2024 at 02:14:36PM +0100, Bruce Richardson wrote:
> On Tue, Oct 01, 2024 at 11:12:54AM +0200, David Marchand wrote:
> > Following two upstream Linux kernel changes (see links), the mac address
> > of a iavf port, serviced by a i40e PF driver, is lost when the DPDK iavf
> > driver probes the port again (which may be triggered at any point of a
> > DPDK application life, like when a reset event is triggered by the PF).
> > 
> > A first change results in the mac address of the VF port being reset to 0
> > during the VIRTCHNL_OP_GET_VF_RESOURCES query.
> > The i40e PF driver change is pretty obscure but the iavf Linux driver does
> > set VIRTCHNL_VF_OFFLOAD_USO.
> > Announcing such a capability in the DPDK driver does not seem to be an
> > issue, so do the same in DPDK to keep the legacy behavior of a fixed mac.
> > 
> > Then a second change in the kernel results in the VF mac address being
> > cleared when the VF driver remove its default mac address.
> > Removing (unicast or multicast) mac addresses is not done by the kernel VF
> > driver in general.
> > The reason why the DPDK driver behaves like this is undocumented
> > (and lost because the authors are not active anymore).
> > Aligning DPDK behavior to the upstream kernel driver is safer in any
> > case.
> > 
> > Cc: stable@dpdk.org
> > 
> > Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fed0d9f13266
> > Link: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ceb29474bbbc
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > ---
> 
> Seems reasonable to me.
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Applied to dpdk-next-net-intel.

Thanks,
/Bruce
diff mbox series

Patch

diff --git a/drivers/net/iavf/iavf_ethdev.c b/drivers/net/iavf/iavf_ethdev.c
index 11036bc179..97cdb1cbe0 100644
--- a/drivers/net/iavf/iavf_ethdev.c
+++ b/drivers/net/iavf/iavf_ethdev.c
@@ -1044,7 +1044,7 @@  iavf_dev_start(struct rte_eth_dev *dev)
 		if (iavf_configure_queues(adapter,
 				IAVF_CFG_Q_NUM_PER_BUF, index) != 0) {
 			PMD_DRV_LOG(ERR, "configure queues failed");
-			goto err_queue;
+			goto error;
 		}
 		num_queue_pairs -= IAVF_CFG_Q_NUM_PER_BUF;
 		index += IAVF_CFG_Q_NUM_PER_BUF;
@@ -1052,12 +1052,12 @@  iavf_dev_start(struct rte_eth_dev *dev)
 
 	if (iavf_configure_queues(adapter, num_queue_pairs, index) != 0) {
 		PMD_DRV_LOG(ERR, "configure queues failed");
-		goto err_queue;
+		goto error;
 	}
 
 	if (iavf_config_rx_queues_irqs(dev, intr_handle) != 0) {
 		PMD_DRV_LOG(ERR, "configure irq failed");
-		goto err_queue;
+		goto error;
 	}
 	/* re-enable intr again, because efd assign may change */
 	if (dev->data->dev_conf.intr_conf.rxq != 0) {
@@ -1077,14 +1077,12 @@  iavf_dev_start(struct rte_eth_dev *dev)
 
 	if (iavf_start_queues(dev) != 0) {
 		PMD_DRV_LOG(ERR, "enable queues failed");
-		goto err_mac;
+		goto error;
 	}
 
 	return 0;
 
-err_mac:
-	iavf_add_del_all_mac_addr(adapter, false);
-err_queue:
+error:
 	return -1;
 }
 
@@ -1113,16 +1111,6 @@  iavf_dev_stop(struct rte_eth_dev *dev)
 	/* Rx interrupt vector mapping free */
 	rte_intr_vec_list_free(intr_handle);
 
-	/* adminq will be disabled when vf is resetting. */
-	if (!vf->in_reset_recovery) {
-		/* remove all mac addrs */
-		iavf_add_del_all_mac_addr(adapter, false);
-
-		/* remove all multicast addresses */
-		iavf_add_del_mc_addr_list(adapter, vf->mc_addrs, vf->mc_addrs_num,
-				  false);
-	}
-
 	iavf_stop_queues(dev);
 
 	adapter->stopped = 1;
diff --git a/drivers/net/iavf/iavf_vchnl.c b/drivers/net/iavf/iavf_vchnl.c
index 69420bc9b6..065ab3594c 100644
--- a/drivers/net/iavf/iavf_vchnl.c
+++ b/drivers/net/iavf/iavf_vchnl.c
@@ -710,6 +710,7 @@  iavf_get_vf_resource(struct iavf_adapter *adapter)
 		VIRTCHNL_VF_OFFLOAD_ADV_RSS_PF |
 		VIRTCHNL_VF_OFFLOAD_FSUB_PF |
 		VIRTCHNL_VF_OFFLOAD_REQ_QUEUES |
+		VIRTCHNL_VF_OFFLOAD_USO |
 		VIRTCHNL_VF_OFFLOAD_CRC |
 		VIRTCHNL_VF_OFFLOAD_VLAN_V2 |
 		VIRTCHNL_VF_LARGE_NUM_QPAIRS |