[ovs-dev] [PATCH v1 1/2] netdev-dpdk: Fix netdev_dpdk_get_features().

Flavio Leitner fbl at sysclose.org
Tue Oct 30 16:37:13 UTC 2018


On Thu, Oct 25, 2018 at 03:17:40PM +0100, Ian Stokes wrote:
> This commit fixes netdev_dpdk_get_features() by initializing a bitmap
> that represents current features to zero and accounting for non defined
> link speed values in the OpenFlow spec.
> 
> The current approach for retrieving netdev dpdk features uses a
> pointer allocated in the stack without being initialized. As such there
> is no guarantee that the bitmap will be accurate. Fix this by declaring
> and initializing local variable 'feature' to be used when building the
> bitmap, with its value then assigned to the pointer. Also account for
> link speeds not defined in the OpenFlow spec by defaulting to
> NETDEV_F_OTHER for undefined link speeds.
> 
> Fixes: 8a9562d21a40 ("dpif-netdev: Add DPDK netdev.")
> Signed-off-by: Ian Stokes <ian.stokes at intel.com>
> ---
> Flavio, this patch is based on suggestions from you
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2018-September/351809.html
> 
> I'd like to add
> 
> Co-authored-by: Flavio Leitner <fbl at sysclose.org>
> Signed-off-by: Flavio Leitner <fbl at sysclose.org>
> 
> if you agree to sign off on this.

Yup, sounds good to me.
Thanks Ian!
fbl


> ---
>  lib/netdev-dpdk.c | 65 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 40 insertions(+), 25 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index f91aa27cd..35eb30f8d 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -2707,43 +2707,58 @@ netdev_dpdk_get_features(const struct netdev *netdev,
>  {
>      struct netdev_dpdk *dev = netdev_dpdk_cast(netdev);
>      struct rte_eth_link link;
> +    uint32_t feature = 0;
>  
>      ovs_mutex_lock(&dev->mutex);
>      link = dev->link;
>      ovs_mutex_unlock(&dev->mutex);
>  
> -    if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
> -        if (link.link_speed == ETH_SPEED_NUM_10M) {
> -            *current = NETDEV_F_10MB_HD;
> -        }
> -        if (link.link_speed == ETH_SPEED_NUM_100M) {
> -            *current = NETDEV_F_100MB_HD;
> -        }
> -        if (link.link_speed == ETH_SPEED_NUM_1G) {
> -            *current = NETDEV_F_1GB_HD;
> -        }
> -    } else if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
> -        if (link.link_speed == ETH_SPEED_NUM_10M) {
> -            *current = NETDEV_F_10MB_FD;
> -        }
> -        if (link.link_speed == ETH_SPEED_NUM_100M) {
> -            *current = NETDEV_F_100MB_FD;
> -        }
> -        if (link.link_speed == ETH_SPEED_NUM_1G) {
> -            *current = NETDEV_F_1GB_FD;
> -        }
> -        if (link.link_speed == ETH_SPEED_NUM_10G) {
> -            *current = NETDEV_F_10GB_FD;
> +    if (link.link_duplex == ETH_LINK_FULL_DUPLEX) {
> +        switch (link.link_speed) {
> +        /* OpenFlow defined values: see enum ofp_port_features */
> +        case ETH_SPEED_NUM_10M:
> +            feature |= NETDEV_F_10MB_FD;
> +            break;
> +        case ETH_SPEED_NUM_100M:
> +            feature |= NETDEV_F_100MB_FD;
> +            break;
> +        case ETH_SPEED_NUM_1G:
> +            feature |= NETDEV_F_1GB_FD;
> +            break;
> +        case ETH_SPEED_NUM_10G:
> +            feature |= NETDEV_F_10GB_FD;
> +            break;
> +        case ETH_SPEED_NUM_40G:
> +            feature |= NETDEV_F_40GB_FD;
> +            break;
> +        case ETH_SPEED_NUM_100G:
> +            feature |= NETDEV_F_100GB_FD;
> +            break;
> +        default:
> +            feature |= NETDEV_F_OTHER;
>          }
> -        if (link.link_speed == ETH_SPEED_NUM_40G) {
> -            *current = NETDEV_F_40GB_FD;
> +    }
> +    else if (link.link_duplex == ETH_LINK_HALF_DUPLEX) {
> +        switch (link.link_speed) {
> +        case ETH_SPEED_NUM_10M:
> +            feature |= NETDEV_F_10MB_HD;
> +            break;
> +        case ETH_SPEED_NUM_100M:
> +            feature |= NETDEV_F_100MB_HD;
> +            break;
> +        case ETH_SPEED_NUM_1G:
> +            feature |= NETDEV_F_1GB_HD;
> +            break;
> +        default:
> +            feature |= NETDEV_F_OTHER;
>          }
>      }
>  
>      if (link.link_autoneg) {
> -        *current |= NETDEV_F_AUTONEG;
> +        feature |= NETDEV_F_AUTONEG;
>      }
>  
> +    *current = feature;
>      *advertised = *supported = *peer = 0;
>  
>      return 0;
> -- 
> 2.13.6
> 

-- 
Flavio



More information about the dev mailing list