diff mbox series

net/nfp: fix the issue about set the vlan strip offload off

Message ID 20220419083857.409261-1-peng.zhang@corigine.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series net/nfp: fix the issue about set the vlan strip offload off |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-aarch64-unit-testing success Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/intel-Testing success Testing PASS

Commit Message

Nole Zhang April 19, 2022, 8:38 a.m. UTC 
  When set the vlan_strip_offload off, it doesn't work.
Because we can't update the msg in the nic.

This patch will fix this error.

Fixes: d4a27a3b092a ("nfp: add basic features")
Cc: stable@dpdk.org

Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
Signed-off-by: Yong Xu <yong.xu@corigine.com>
Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/nfp_common.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Comments

Ferruh Yigit May 20, 2022, 3:20 p.m. UTC | #1 
On 4/19/2022 9:38 AM, Peng Zhang wrote:
> When set the vlan_strip_offload off, it doesn't work.
> Because we can't update the msg in the nic.
> 
> This patch will fix this error.
> 
> Fixes: d4a27a3b092a ("nfp: add basic features")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
> Signed-off-by: Yong Xu <yong.xu@corigine.com>
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>

Hi Niklas, Reminder of this patch waiting for your review.

> ---
>   drivers/net/nfp/nfp_common.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
> index b26770dbfb..ab4cfda389 100644
> --- a/drivers/net/nfp/nfp_common.c
> +++ b/drivers/net/nfp/nfp_common.c
> @@ -968,19 +968,18 @@ nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>   	int ret;
>   
>   	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	new_ctrl = 0;
> +	new_ctrl = hw->ctrl;
>   
> -	/* Enable vlan strip if it is not configured yet */
> -	if ((mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
> -	    !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
> -		new_ctrl = hw->ctrl | NFP_NET_CFG_CTRL_RXVLAN;
> -
> -	/* Disable vlan strip just if it is configured */
> -	if (!(mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
> -	    (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
> -		new_ctrl = hw->ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
> +	if (mask & RTE_ETH_VLAN_STRIP_OFFLOAD) {
> +		/* Disable vlan strip just if it is configured */
> +		if (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)
> +			new_ctrl = new_ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
> +		/* Enable vlan strip if it is not configured yet */
> +		else
> +			new_ctrl = new_ctrl | NFP_NET_CFG_CTRL_RXVLAN;
> +	}
>   
> -	if (new_ctrl == 0)
> +	if (new_ctrl == hw->ctrl)
>   		return 0;
>   
>   	update = NFP_NET_CFG_UPDATE_GEN;
Niklas Söderlund May 23, 2022, 9:16 p.m. UTC | #2 
Hi Peng,

Thanks for your work.

On 2022-04-19 10:38:57 +0200, Peng Zhang wrote:
> When set the vlan_strip_offload off, it doesn't work.
> Because we can't update the msg in the nic.
> 
> This patch will fix this error.
> 
> Fixes: d4a27a3b092a ("nfp: add basic features")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
> Signed-off-by: Yong Xu <yong.xu@corigine.com>
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>

Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>

> ---
>  drivers/net/nfp/nfp_common.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
> index b26770dbfb..ab4cfda389 100644
> --- a/drivers/net/nfp/nfp_common.c
> +++ b/drivers/net/nfp/nfp_common.c
> @@ -968,19 +968,18 @@ nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>  	int ret;
>  
>  	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	new_ctrl = 0;
> +	new_ctrl = hw->ctrl;
>  
> -	/* Enable vlan strip if it is not configured yet */
> -	if ((mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
> -	    !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
> -		new_ctrl = hw->ctrl | NFP_NET_CFG_CTRL_RXVLAN;
> -
> -	/* Disable vlan strip just if it is configured */
> -	if (!(mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
> -	    (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
> -		new_ctrl = hw->ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
> +	if (mask & RTE_ETH_VLAN_STRIP_OFFLOAD) {
> +		/* Disable vlan strip just if it is configured */
> +		if (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)
> +			new_ctrl = new_ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
> +		/* Enable vlan strip if it is not configured yet */
> +		else
> +			new_ctrl = new_ctrl | NFP_NET_CFG_CTRL_RXVLAN;
> +	}
>  
> -	if (new_ctrl == 0)
> +	if (new_ctrl == hw->ctrl)
>  		return 0;
>  
>  	update = NFP_NET_CFG_UPDATE_GEN;
> -- 
> 2.27.0
>
Ferruh Yigit May 24, 2022, 12:57 p.m. UTC | #3 
On 4/19/2022 9:38 AM, Peng Zhang wrote:
> When set the vlan_strip_offload off, it doesn't work.
> Because we can't update the msg in the nic.
> 
> This patch will fix this error.
> 
> Fixes: d4a27a3b092a ("nfp: add basic features")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Peng Zhang <peng.zhang@corigine.com>
> Signed-off-by: Yong Xu <yong.xu@corigine.com>
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>   drivers/net/nfp/nfp_common.c | 21 ++++++++++-----------
>   1 file changed, 10 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
> index b26770dbfb..ab4cfda389 100644
> --- a/drivers/net/nfp/nfp_common.c
> +++ b/drivers/net/nfp/nfp_common.c
> @@ -968,19 +968,18 @@ nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask)
>   	int ret;
>   
>   	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> -	new_ctrl = 0;
> +	new_ctrl = hw->ctrl;
>   
> -	/* Enable vlan strip if it is not configured yet */
> -	if ((mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
> -	    !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
> -		new_ctrl = hw->ctrl | NFP_NET_CFG_CTRL_RXVLAN;
> -
> -	/* Disable vlan strip just if it is configured */
> -	if (!(mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
> -	    (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
> -		new_ctrl = hw->ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
> +	if (mask & RTE_ETH_VLAN_STRIP_OFFLOAD) {
> +		/* Disable vlan strip just if it is configured */
> +		if (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)
> +			new_ctrl = new_ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
> +		/* Enable vlan strip if it is not configured yet */
> +		else
> +			new_ctrl = new_ctrl | NFP_NET_CFG_CTRL_RXVLAN;
> +	}

Hi Peng,

I think both old and new implementations are not correct, although new 
one is on the right track to fix.

First 'mask' is filled with ethdev layer and can get one of below values:
RTE_ETH_VLAN_STRIP_MASK
RTE_ETH_VLAN_FILTER_MASK
RTE_ETH_VLAN_EXTEND_MASK
RTE_ETH_QINQ_STRIP_MASK

So PMDs should check 'mask' against those values.
'RTE_ETH_VLAN_*_OFFLOAD' are the values passed by application to the 
ethdev layer. (Why ethdev layer converts them to different set of 
macros, I don't know :/ )

And PMD 'vlan_offload_set()' dev_ops having one of 'RTE_ETH_VLAN_*_MASK' 
macro means that config is changed, this doesn't say yet if it is 
enabled or disabled.
'dev->data->dev_conf.rxmode' needs to be checked to decide to enable or 
disable a config.

Please check 'rte_eth_dev_set_vlan_offload()' API implementation for 
details.


>   
> -	if (new_ctrl == 0)
> +	if (new_ctrl == hw->ctrl)
>   		return 0;
>   
>   	update = NFP_NET_CFG_UPDATE_GEN;
diff mbox series

Patch

diff --git a/drivers/net/nfp/nfp_common.c b/drivers/net/nfp/nfp_common.c
index b26770dbfb..ab4cfda389 100644
--- a/drivers/net/nfp/nfp_common.c
+++ b/drivers/net/nfp/nfp_common.c
@@ -968,19 +968,18 @@  nfp_net_vlan_offload_set(struct rte_eth_dev *dev, int mask)
 	int ret;
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
-	new_ctrl = 0;
+	new_ctrl = hw->ctrl;
 
-	/* Enable vlan strip if it is not configured yet */
-	if ((mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
-	    !(hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
-		new_ctrl = hw->ctrl | NFP_NET_CFG_CTRL_RXVLAN;
-
-	/* Disable vlan strip just if it is configured */
-	if (!(mask & RTE_ETH_VLAN_STRIP_OFFLOAD) &&
-	    (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN))
-		new_ctrl = hw->ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
+	if (mask & RTE_ETH_VLAN_STRIP_OFFLOAD) {
+		/* Disable vlan strip just if it is configured */
+		if (hw->ctrl & NFP_NET_CFG_CTRL_RXVLAN)
+			new_ctrl = new_ctrl & ~NFP_NET_CFG_CTRL_RXVLAN;
+		/* Enable vlan strip if it is not configured yet */
+		else
+			new_ctrl = new_ctrl | NFP_NET_CFG_CTRL_RXVLAN;
+	}
 
-	if (new_ctrl == 0)
+	if (new_ctrl == hw->ctrl)
 		return 0;
 
 	update = NFP_NET_CFG_UPDATE_GEN;