diff mbox series

[v6,02/27] net/nfp: fix the promiscuous mode control functions

Message ID 1666339323-24016-3-git-send-email-chaoyong.he@corigine.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers
Series add the basic rte_flow offload support of nfp PMD |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chaoyong He Oct. 21, 2022, 8:01 a.m. UTC 
  The original functions of promiscuous mode can't process the
representor port rightly, revise the logic to do that.

Fixes: e1124c4f8a45 ("net/nfp: add flower representor framework")

Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
---
 drivers/net/nfp/flower/nfp_flower_representor.c | 26 +++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit Oct. 21, 2022, 1:03 p.m. UTC | #1 
On 10/21/2022 9:01 AM, Chaoyong He wrote:
> The original functions of promiscuous mode can't process the
> representor port rightly, revise the logic to do that.
> 
> Fixes: e1124c4f8a45 ("net/nfp: add flower representor framework")
> 
> Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> ---
>   drivers/net/nfp/flower/nfp_flower_representor.c | 26 +++++++++++++++++++++++--
>   1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c
> index 0e60f50..cecdf46 100644
> --- a/drivers/net/nfp/flower/nfp_flower_representor.c
> +++ b/drivers/net/nfp/flower/nfp_flower_representor.c
> @@ -415,6 +415,9 @@
>   static int
>   nfp_flower_repr_promiscuous_enable(struct rte_eth_dev *dev)
>   {
> +	int ret;
> +	uint32_t update;
> +	uint32_t new_ctrl;
>   	struct nfp_net_hw *pf_hw;
>   	struct nfp_flower_representor *repr;
>   
> @@ -431,12 +434,23 @@
>   		return 0;
>   	}
>   
> -	return nfp_net_promisc_enable(pf_hw->eth_dev);
> +	new_ctrl = pf_hw->ctrl | NFP_NET_CFG_CTRL_PROMISC;
> +	update = NFP_NET_CFG_UPDATE_GEN;
> +	ret = nfp_net_reconfig(pf_hw, new_ctrl, update);
> +	if (ret < 0)
> +		return ret;
> +
> +	pf_hw->ctrl = new_ctrl;
> +

With this change 'nfp_net_promisc_enable()' seems not used at all in the 
driver.

And the difference between above code and 'nfp_net_promisc_enable()' is 
that capability check is removed, so it looks like hack for flower, 
instead why not fix the issue in the 'nfp_net_promisc_enable()'?

Same comment for 'nfp_net_promisc_disable()'.

> +	return 0;
>   }
>   
>   static int
>   nfp_flower_repr_promiscuous_disable(struct rte_eth_dev *dev)
>   {
> +	int ret;
> +	uint32_t update;
> +	uint32_t new_ctrl;
>   	struct nfp_net_hw *pf_hw;
>   	struct nfp_flower_representor *repr;
>   
> @@ -448,7 +462,15 @@
>   		return 0;
>   	}
>   
> -	return nfp_net_promisc_disable(pf_hw->eth_dev);
> +	new_ctrl = pf_hw->ctrl & ~NFP_NET_CFG_CTRL_PROMISC;
> +	update = NFP_NET_CFG_UPDATE_GEN;
> +	ret = nfp_net_reconfig(pf_hw, new_ctrl, update);
> +	if (ret < 0)
> +		return ret;
> +
> +	pf_hw->ctrl = new_ctrl;
> +
> +	return 0;
>   }
>   
>   static int
Chaoyong He Oct. 22, 2022, 6:49 a.m. UTC | #2 
> On 10/21/2022 9:01 AM, Chaoyong He wrote:
> > The original functions of promiscuous mode can't process the
> > representor port rightly, revise the logic to do that.
> >
> > Fixes: e1124c4f8a45 ("net/nfp: add flower representor framework")
> >
> > Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> > ---
> >   drivers/net/nfp/flower/nfp_flower_representor.c | 26
> +++++++++++++++++++++++--
> >   1 file changed, 24 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c
> > b/drivers/net/nfp/flower/nfp_flower_representor.c
> > index 0e60f50..cecdf46 100644
> > --- a/drivers/net/nfp/flower/nfp_flower_representor.c
> > +++ b/drivers/net/nfp/flower/nfp_flower_representor.c
> > @@ -415,6 +415,9 @@
> >   static int
> >   nfp_flower_repr_promiscuous_enable(struct rte_eth_dev *dev)
> >   {
> > +	int ret;
> > +	uint32_t update;
> > +	uint32_t new_ctrl;
> >   	struct nfp_net_hw *pf_hw;
> >   	struct nfp_flower_representor *repr;
> >
> > @@ -431,12 +434,23 @@
> >   		return 0;
> >   	}
> >
> > -	return nfp_net_promisc_enable(pf_hw->eth_dev);
> > +	new_ctrl = pf_hw->ctrl | NFP_NET_CFG_CTRL_PROMISC;
> > +	update = NFP_NET_CFG_UPDATE_GEN;
> > +	ret = nfp_net_reconfig(pf_hw, new_ctrl, update);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pf_hw->ctrl = new_ctrl;
> > +
> 
> With this change 'nfp_net_promisc_enable()' seems not used at all in the
> driver.
> 
Actually, 'nfp_net_promisc_enable()' is still be used by the VF and when the NIC
using CoreNIC firmware app.
In the `static const struct eth_dev_ops nfp_netvf_eth_dev_ops[]` of file 'nfp_ethdev_vf.c'
and `static const struct eth_dev_ops nfp_net_eth_dev_ops[]` of file 'nfp_ethdev.c'.

> And the difference between above code and 'nfp_net_promisc_enable()' is
> that capability check is removed, so it looks like hack for flower, instead why
> not fix the issue in the 'nfp_net_promisc_enable()'?
> 
> Same comment for 'nfp_net_promisc_disable()'.
> 
Yes, it's a good idea to fix this issue in the 'nfp_ent_promisc_enable()' and 'nfp_net_promisc_disable()'.
I will send a v7 patch for this one, thanks.

> > +	return 0;
> >   }
> >
> >   static int
> >   nfp_flower_repr_promiscuous_disable(struct rte_eth_dev *dev)
> >   {
> > +	int ret;
> > +	uint32_t update;
> > +	uint32_t new_ctrl;
> >   	struct nfp_net_hw *pf_hw;
> >   	struct nfp_flower_representor *repr;
> >
> > @@ -448,7 +462,15 @@
> >   		return 0;
> >   	}
> >
> > -	return nfp_net_promisc_disable(pf_hw->eth_dev);
> > +	new_ctrl = pf_hw->ctrl & ~NFP_NET_CFG_CTRL_PROMISC;
> > +	update = NFP_NET_CFG_UPDATE_GEN;
> > +	ret = nfp_net_reconfig(pf_hw, new_ctrl, update);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	pf_hw->ctrl = new_ctrl;
> > +
> > +	return 0;
> >   }
> >
> >   static int
diff mbox series

Patch

diff --git a/drivers/net/nfp/flower/nfp_flower_representor.c b/drivers/net/nfp/flower/nfp_flower_representor.c
index 0e60f50..cecdf46 100644
--- a/drivers/net/nfp/flower/nfp_flower_representor.c
+++ b/drivers/net/nfp/flower/nfp_flower_representor.c
@@ -415,6 +415,9 @@ 
 static int
 nfp_flower_repr_promiscuous_enable(struct rte_eth_dev *dev)
 {
+	int ret;
+	uint32_t update;
+	uint32_t new_ctrl;
 	struct nfp_net_hw *pf_hw;
 	struct nfp_flower_representor *repr;
 
@@ -431,12 +434,23 @@ 
 		return 0;
 	}
 
-	return nfp_net_promisc_enable(pf_hw->eth_dev);
+	new_ctrl = pf_hw->ctrl | NFP_NET_CFG_CTRL_PROMISC;
+	update = NFP_NET_CFG_UPDATE_GEN;
+	ret = nfp_net_reconfig(pf_hw, new_ctrl, update);
+	if (ret < 0)
+		return ret;
+
+	pf_hw->ctrl = new_ctrl;
+
+	return 0;
 }
 
 static int
 nfp_flower_repr_promiscuous_disable(struct rte_eth_dev *dev)
 {
+	int ret;
+	uint32_t update;
+	uint32_t new_ctrl;
 	struct nfp_net_hw *pf_hw;
 	struct nfp_flower_representor *repr;
 
@@ -448,7 +462,15 @@ 
 		return 0;
 	}
 
-	return nfp_net_promisc_disable(pf_hw->eth_dev);
+	new_ctrl = pf_hw->ctrl & ~NFP_NET_CFG_CTRL_PROMISC;
+	update = NFP_NET_CFG_UPDATE_GEN;
+	ret = nfp_net_reconfig(pf_hw, new_ctrl, update);
+	if (ret < 0)
+		return ret;
+
+	pf_hw->ctrl = new_ctrl;
+
+	return 0;
 }
 
 static int