> > 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
