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

[Ferruh Yigit]

On 12/9/2016 10:08 AM, Alejandro Lucero wrote:
> 
> 
> On Tue, Dec 6, 2016 at 11:57 AM, Ferruh Yigit <ferruh.yigit at intel.com
> <mailto:ferruh.yigit at 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 at netronome.com
>     <mailto:alejandro.lucero at 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);
>     >
> 
>