diff mbox

[dpdk-dev,v3] nfp: report link speed using hardware info

Message ID 1480669552-10411-1-git-send-email-alejandro.lucero@netronome.com (mailing list archive)
State Changes Requested, archived
Delegated to: Ferruh Yigit
Headers

Checks

Context Check Description
checkpatch/checkpatch success coding style OK

Commit Message

Alejandro Lucero Dec. 2, 2016, 9:05 a.m. UTC 
  Previous reported speed was hardcoded.

v3: remove unsed macro
v2: using RTE_DIM instead of own macro

Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
---
 drivers/net/nfp/nfp_net.c | 28 ++++++++++++++++++++++++++--
 1 file changed, 26 insertions(+), 2 deletions(-)

Comments

Ferruh Yigit Dec. 6, 2016, 11:57 a.m. UTC | #1 
On 12/2/2016 9:05 AM, Alejandro Lucero wrote:
> Previous reported speed was hardcoded.
> 
> v3: remove unsed macro
> v2: using RTE_DIM instead of own macro
> 
> Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> ---
>  drivers/net/nfp/nfp_net.c | 28 ++++++++++++++++++++++++++--
>  1 file changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> index c6b1587..24f3164 100644
> --- a/drivers/net/nfp/nfp_net.c
> +++ b/drivers/net/nfp/nfp_net.c
> @@ -816,6 +816,17 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
>  	struct rte_eth_link link, old;
>  	uint32_t nn_link_status;
>  
> +	static const uint32_t ls_to_ethtool[] = {
> +		[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = ETH_SPEED_NUM_NONE,
> +		[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = ETH_SPEED_NUM_NONE,
> +		[NFP_NET_CFG_STS_LINK_RATE_1G]          = ETH_SPEED_NUM_1G,
> +		[NFP_NET_CFG_STS_LINK_RATE_10G]         = ETH_SPEED_NUM_10G,
> +		[NFP_NET_CFG_STS_LINK_RATE_25G]         = ETH_SPEED_NUM_25G,
> +		[NFP_NET_CFG_STS_LINK_RATE_40G]         = ETH_SPEED_NUM_40G,
> +		[NFP_NET_CFG_STS_LINK_RATE_50G]         = ETH_SPEED_NUM_50G,
> +		[NFP_NET_CFG_STS_LINK_RATE_100G]        = ETH_SPEED_NUM_100G,
> +	};
> +
>  	PMD_DRV_LOG(DEBUG, "Link update\n");
>  
>  	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> @@ -831,8 +842,21 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
>  		link.link_status = ETH_LINK_UP;
>  
>  	link.link_duplex = ETH_LINK_FULL_DUPLEX;
> -	/* Other cards can limit the tx and rx rate per VF */
> -	link.link_speed = ETH_SPEED_NUM_40G;
> +
> +	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
> +			 NFP_NET_CFG_STS_LINK_RATE_MASK;
> +
> +	if ((NFD_CFG_MAJOR_VERSION_of(hw->ver) < 4) ||
> +	    ((NFD_CFG_MINOR_VERSION_of(hw->ver) == 4) &&
> +	    (NFD_CFG_MINOR_VERSION_of(hw->ver) == 0)))
> +		link.link_speed = ETH_SPEED_NUM_40G;

Same comment from previous review:

For specific firmware version, speed is still hardcoded to 40G, can you
please mention from this and if possible its reason in commit log?

> +	else {
> +		if (nn_link_status == NFP_NET_CFG_STS_LINK_RATE_UNKNOWN ||

Again from previous review:

> This is for checking any wrong value from firmware/hardware.

I see, but removing this check will not change the logic, else branch is
taken and again same value set.

Still if you deliberately prefer to keep it, that is OK.

> +		    nn_link_status >= RTE_DIM(ls_to_ethtool))
> +			link.link_speed = ETH_SPEED_NUM_NONE;
> +		else
> +			link.link_speed = ls_to_ethtool[nn_link_status];
> +	}
>  
>  	if (old.link_status != link.link_status) {
>  		nfp_net_dev_atomic_write_link_status(dev, &link);
>
Alejandro Lucero Dec. 9, 2016, 10:08 a.m. UTC | #2 
On Tue, Dec 6, 2016 at 11:57 AM, Ferruh Yigit <ferruh.yigit@intel.com>
wrote:

> On 12/2/2016 9:05 AM, Alejandro Lucero wrote:
> > Previous reported speed was hardcoded.
> >
> > v3: remove unsed macro
> > v2: using RTE_DIM instead of own macro
> >
> > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com>
> > ---
> >  drivers/net/nfp/nfp_net.c | 28 ++++++++++++++++++++++++++--
> >  1 file changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
> > index c6b1587..24f3164 100644
> > --- a/drivers/net/nfp/nfp_net.c
> > +++ b/drivers/net/nfp/nfp_net.c
> > @@ -816,6 +816,17 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
> >       struct rte_eth_link link, old;
> >       uint32_t nn_link_status;
> >
> > +     static const uint32_t ls_to_ethtool[] = {
> > +             [NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] =
> ETH_SPEED_NUM_NONE,
> > +             [NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     =
> ETH_SPEED_NUM_NONE,
> > +             [NFP_NET_CFG_STS_LINK_RATE_1G]          = ETH_SPEED_NUM_1G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_10G]         =
> ETH_SPEED_NUM_10G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_25G]         =
> ETH_SPEED_NUM_25G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_40G]         =
> ETH_SPEED_NUM_40G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_50G]         =
> ETH_SPEED_NUM_50G,
> > +             [NFP_NET_CFG_STS_LINK_RATE_100G]        =
> ETH_SPEED_NUM_100G,
> > +     };
> > +
> >       PMD_DRV_LOG(DEBUG, "Link update\n");
> >
> >       hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > @@ -831,8 +842,21 @@ static void nfp_net_read_mac(struct nfp_net_hw *hw)
> >               link.link_status = ETH_LINK_UP;
> >
> >       link.link_duplex = ETH_LINK_FULL_DUPLEX;
> > -     /* Other cards can limit the tx and rx rate per VF */
> > -     link.link_speed = ETH_SPEED_NUM_40G;
> > +
> > +     nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT)
> &
> > +                      NFP_NET_CFG_STS_LINK_RATE_MASK;
> > +
> > +     if ((NFD_CFG_MAJOR_VERSION_of(hw->ver) < 4) ||
> > +         ((NFD_CFG_MINOR_VERSION_of(hw->ver) == 4) &&
> > +         (NFD_CFG_MINOR_VERSION_of(hw->ver) == 0)))
> > +             link.link_speed = ETH_SPEED_NUM_40G;
>
> Same comment from previous review:
>
> For specific firmware version, speed is still hardcoded to 40G, can you
> please mention from this and if possible its reason in commit log?
>
>
Well, we have old firmware still around and we need to avoid reading this
info from hardware if not supported.
But I guess I could be a more chatty about this in the commit log. I will
send another version.


> > +     else {
> > +             if (nn_link_status == NFP_NET_CFG_STS_LINK_RATE_UNKNOWN ||
>
> Again from previous review:
>
> > This is for checking any wrong value from firmware/hardware.
>
> I see, but removing this check will not change the logic, else branch is
> taken and again same value set.
>
>
OK. I think I can remove the first part of the if clause, because it is
implicit in the second part.
I guess this is what you really meant, and not just to leave the else
statement (without the else, of course). am I right?


> Still if you deliberately prefer to keep it, that is OK.
>
> > +                 nn_link_status >= RTE_DIM(ls_to_ethtool))
> > +                     link.link_speed = ETH_SPEED_NUM_NONE;
> > +             else
> > +                     link.link_speed = ls_to_ethtool[nn_link_status];
> > +     }
> >
> >       if (old.link_status != link.link_status) {
> >               nfp_net_dev_atomic_write_link_status(dev, &link);
> >
>
>
Ferruh Yigit Dec. 9, 2016, 10:11 a.m. UTC | #3 
On 12/9/2016 10:08 AM, Alejandro Lucero wrote:
> 
> 
> On Tue, Dec 6, 2016 at 11:57 AM, Ferruh Yigit <ferruh.yigit@intel.com
> <mailto:ferruh.yigit@intel.com>> wrote:
> 
>     On 12/2/2016 9:05 AM, Alejandro Lucero wrote:
>     > Previous reported speed was hardcoded.
>     >
>     > v3: remove unsed macro
>     > v2: using RTE_DIM instead of own macro
>     >
>     > Signed-off-by: Alejandro Lucero <alejandro.lucero@netronome.com
>     <mailto:alejandro.lucero@netronome.com>>
>     > ---
>     >  drivers/net/nfp/nfp_net.c | 28 ++++++++++++++++++++++++++--
>     >  1 file changed, 26 insertions(+), 2 deletions(-)
>     >
>     > diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
>     > index c6b1587..24f3164 100644
>     > --- a/drivers/net/nfp/nfp_net.c
>     > +++ b/drivers/net/nfp/nfp_net.c
>     > @@ -816,6 +816,17 @@ static void nfp_net_read_mac(struct
>     nfp_net_hw *hw)
>     >       struct rte_eth_link link, old;
>     >       uint32_t nn_link_status;
>     >
>     > +     static const uint32_t ls_to_ethtool[] = {
>     > +             [NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] =
>     ETH_SPEED_NUM_NONE,
>     > +             [NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     =
>     ETH_SPEED_NUM_NONE,
>     > +             [NFP_NET_CFG_STS_LINK_RATE_1G]          =
>     ETH_SPEED_NUM_1G,
>     > +             [NFP_NET_CFG_STS_LINK_RATE_10G]         =
>     ETH_SPEED_NUM_10G,
>     > +             [NFP_NET_CFG_STS_LINK_RATE_25G]         =
>     ETH_SPEED_NUM_25G,
>     > +             [NFP_NET_CFG_STS_LINK_RATE_40G]         =
>     ETH_SPEED_NUM_40G,
>     > +             [NFP_NET_CFG_STS_LINK_RATE_50G]         =
>     ETH_SPEED_NUM_50G,
>     > +             [NFP_NET_CFG_STS_LINK_RATE_100G]        =
>     ETH_SPEED_NUM_100G,
>     > +     };
>     > +
>     >       PMD_DRV_LOG(DEBUG, "Link update\n");
>     >
>     >       hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>     > @@ -831,8 +842,21 @@ static void nfp_net_read_mac(struct
>     nfp_net_hw *hw)
>     >               link.link_status = ETH_LINK_UP;
>     >
>     >       link.link_duplex = ETH_LINK_FULL_DUPLEX;
>     > -     /* Other cards can limit the tx and rx rate per VF */
>     > -     link.link_speed = ETH_SPEED_NUM_40G;
>     > +
>     > +     nn_link_status = (nn_link_status >>
>     NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
>     > +                      NFP_NET_CFG_STS_LINK_RATE_MASK;
>     > +
>     > +     if ((NFD_CFG_MAJOR_VERSION_of(hw->ver) < 4) ||
>     > +         ((NFD_CFG_MINOR_VERSION_of(hw->ver) == 4) &&
>     > +         (NFD_CFG_MINOR_VERSION_of(hw->ver) == 0)))
>     > +             link.link_speed = ETH_SPEED_NUM_40G;
> 
>     Same comment from previous review:
> 
>     For specific firmware version, speed is still hardcoded to 40G, can you
>     please mention from this and if possible its reason in commit log?
> 
> 
> Well, we have old firmware still around and we need to avoid reading
> this info from hardware if not supported.
> But I guess I could be a more chatty about this in the commit log. I
> will send another version.
>  
> 
>     > +     else {
>     > +             if (nn_link_status == NFP_NET_CFG_STS_LINK_RATE_UNKNOWN ||
> 
>     Again from previous review:
> 
>     > This is for checking any wrong value from firmware/hardware.
> 
>     I see, but removing this check will not change the logic, else branch is
>     taken and again same value set.
> 
> 
> OK. I think I can remove the first part of the if clause, because it is
> implicit in the second part.

Yes this is what I mean. Thanks.

> I guess this is what you really meant, and not just to leave the else
> statement (without the else, of course). am I right?
>  
> 
>     Still if you deliberately prefer to keep it, that is OK.
> 
>     > +                 nn_link_status >= RTE_DIM(ls_to_ethtool))
>     > +                     link.link_speed = ETH_SPEED_NUM_NONE;
>     > +             else
>     > +                     link.link_speed = ls_to_ethtool[nn_link_status];
>     > +     }
>     >
>     >       if (old.link_status != link.link_status) {
>     >               nfp_net_dev_atomic_write_link_status(dev, &link);
>     >
> 
>
diff mbox

Patch

diff --git a/drivers/net/nfp/nfp_net.c b/drivers/net/nfp/nfp_net.c
index c6b1587..24f3164 100644
--- a/drivers/net/nfp/nfp_net.c
+++ b/drivers/net/nfp/nfp_net.c
@@ -816,6 +816,17 @@  static void nfp_net_read_mac(struct nfp_net_hw *hw)
 	struct rte_eth_link link, old;
 	uint32_t nn_link_status;
 
+	static const uint32_t ls_to_ethtool[] = {
+		[NFP_NET_CFG_STS_LINK_RATE_UNSUPPORTED] = ETH_SPEED_NUM_NONE,
+		[NFP_NET_CFG_STS_LINK_RATE_UNKNOWN]     = ETH_SPEED_NUM_NONE,
+		[NFP_NET_CFG_STS_LINK_RATE_1G]          = ETH_SPEED_NUM_1G,
+		[NFP_NET_CFG_STS_LINK_RATE_10G]         = ETH_SPEED_NUM_10G,
+		[NFP_NET_CFG_STS_LINK_RATE_25G]         = ETH_SPEED_NUM_25G,
+		[NFP_NET_CFG_STS_LINK_RATE_40G]         = ETH_SPEED_NUM_40G,
+		[NFP_NET_CFG_STS_LINK_RATE_50G]         = ETH_SPEED_NUM_50G,
+		[NFP_NET_CFG_STS_LINK_RATE_100G]        = ETH_SPEED_NUM_100G,
+	};
+
 	PMD_DRV_LOG(DEBUG, "Link update\n");
 
 	hw = NFP_NET_DEV_PRIVATE_TO_HW(dev->data->dev_private);
@@ -831,8 +842,21 @@  static void nfp_net_read_mac(struct nfp_net_hw *hw)
 		link.link_status = ETH_LINK_UP;
 
 	link.link_duplex = ETH_LINK_FULL_DUPLEX;
-	/* Other cards can limit the tx and rx rate per VF */
-	link.link_speed = ETH_SPEED_NUM_40G;
+
+	nn_link_status = (nn_link_status >> NFP_NET_CFG_STS_LINK_RATE_SHIFT) &
+			 NFP_NET_CFG_STS_LINK_RATE_MASK;
+
+	if ((NFD_CFG_MAJOR_VERSION_of(hw->ver) < 4) ||
+	    ((NFD_CFG_MINOR_VERSION_of(hw->ver) == 4) &&
+	    (NFD_CFG_MINOR_VERSION_of(hw->ver) == 0)))
+		link.link_speed = ETH_SPEED_NUM_40G;
+	else {
+		if (nn_link_status == NFP_NET_CFG_STS_LINK_RATE_UNKNOWN ||
+		    nn_link_status >= RTE_DIM(ls_to_ethtool))
+			link.link_speed = ETH_SPEED_NUM_NONE;
+		else
+			link.link_speed = ls_to_ethtool[nn_link_status];
+	}
 
 	if (old.link_status != link.link_status) {
 		nfp_net_dev_atomic_write_link_status(dev, &link);