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

Stokes, Ian ian.stokes at intel.com
Tue Oct 30 12:42:49 UTC 2018


> > 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.h
> > tml
> >
> > 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.
> > ---
> >  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 */
> 
> This comment should be not only for full duplex case.
> Also, I'm not sure about referring to enum ofp_port_features, because it
> not used here.

Ok I'll movie it to before the if statement and remove the reference to the enums.

> 
> > +        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) {
> 
> Could you place 'else if' on the same line with '}' ?

Sure, will fix on the v2.

Thanks
Ian
> 
> > +        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



More information about the dev mailing list