Re: [dpdk-dev] [PATCH v5 3/4] ethdev: redesign link speed config API

[Marc Sune <marcdevel@gmail.com>]

2015-10-05 12:59 GMT+02:00 Neil Horman <nhorman@tuxdriver.com>:

> On Sun, Oct 04, 2015 at 11:12:46PM +0200, Marc Sune wrote:
> > This patch redesigns the API to set the link speed/s configure
> > for an ethernet port. Specifically:
> >
> > - it allows to define a set of advertised speeds for
> >   auto-negociation.
> > - it allows to disable link auto-negociation (single fixed speed).
> > - default: auto-negociate all supported speeds.
> >
> > Other changes:
> >
> > * Added utility MACROs ETH_SPEED_NUM_XXX with the numeric
> >   values of all supported link speeds, in Mbps.
> > * Converted link_speed to uint32_t to accomodate 100G speeds
> >   (bug).
> > * Added autoneg flag in struct rte_eth_link to indicate if
> >   link speed was a result of auto-negociation or was fixed
> >   by configuration.
> > * Added utility function to convert numeric speeds to bitmap
> >   fields.
> > * Adapted testpmd to the new link API.
> >
> > Signed-off-by: Marc Sune <marcdevel@gmail.com>
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> b/lib/librte_ether/rte_ethdev.c
> > index f593f6e..29b2960 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1072,6 +1072,55 @@ rte_eth_dev_check_mq_mode(uint8_t port_id,
> uint16_t nb_rx_q, uint16_t nb_tx_q,
> >  }
> >
> >  int
> > +rte_eth_speed_to_bm_flag(uint32_t speed, int duplex, uint32_t *flag)
> > +{
> > +     switch (speed) {
> > +     case ETH_SPEED_NUM_10M:
> > +             *flag = (duplex) ? ETH_LINK_SPEED_10M :
> > +
>  ETH_LINK_SPEED_10M_HD;
> > +             break;
> > +     case ETH_SPEED_NUM_100M:
> > +             *flag = (duplex) ? ETH_LINK_SPEED_100M :
> > +
>  ETH_LINK_SPEED_100M_HD;
> > +             break;
> > +     case ETH_SPEED_NUM_1G:
> > +             *flag = ETH_LINK_SPEED_1G;
> > +             break;
> > +     case ETH_SPEED_NUM_2_5G:
> > +             *flag = ETH_LINK_SPEED_2_5G;
> > +             break;
> > +     case ETH_SPEED_NUM_5G:
> > +             *flag = ETH_LINK_SPEED_5G;
> > +             break;
> > +     case ETH_SPEED_NUM_10G:
> > +             *flag = ETH_LINK_SPEED_10G;
> > +             break;
> > +     case ETH_SPEED_NUM_20G:
> > +             *flag = ETH_LINK_SPEED_20G;
> > +             break;
> > +     case ETH_SPEED_NUM_25G:
> > +             *flag = ETH_LINK_SPEED_25G;
> > +             break;
> > +     case ETH_SPEED_NUM_40G:
> > +             *flag = ETH_LINK_SPEED_40G;
> > +             break;
> > +     case ETH_SPEED_NUM_50G:
> > +             *flag = ETH_LINK_SPEED_50G;
> > +             break;
> > +     case ETH_SPEED_NUM_56G:
> > +             *flag = ETH_LINK_SPEED_56G;
> > +             break;
> > +     case ETH_SPEED_NUM_100G:
> > +             *flag = ETH_LINK_SPEED_100G;
> > +             break;
> > +     default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
>
> This nees to go in the appropriate version.map file for shared library
> building.
>
> > +int
> >  rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t
> nb_tx_q,
> >                     const struct rte_eth_conf *dev_conf)
> >  {
> > diff --git a/lib/librte_ether/rte_ethdev.h
> b/lib/librte_ether/rte_ethdev.h
> > index 951a423..bd333e4 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -238,26 +238,59 @@ struct rte_eth_stats {
> >  };
> >
> >  /**
> > + * Device supported speeds bitmap flags
> > + */
> > +#define ETH_LINK_SPEED_AUTONEG               (0 << 0)  /*<
> Autonegociate (all speeds)  */
> > +#define ETH_LINK_SPEED_NO_AUTONEG    (1 << 0)  /*< Disable autoneg
> (fixed speed)  */
> > +#define ETH_LINK_SPEED_10M_HD                (1 << 1)  /*< 10 Mbps
> half-duplex */
> > +#define ETH_LINK_SPEED_10M           (1 << 2)  /*< 10 Mbps full-duplex
> */
> > +#define ETH_LINK_SPEED_100M_HD               (1 << 3)  /*< 100 Mbps
> half-duplex */
> > +#define ETH_LINK_SPEED_100M          (1 << 4)  /*< 100 Mbps full-duplex
> */
> > +#define ETH_LINK_SPEED_1G            (1 << 5)  /*< 1 Gbps */
> > +#define ETH_LINK_SPEED_2_5G          (1 << 6)  /*< 2.5 Gbps */
> > +#define ETH_LINK_SPEED_5G            (1 << 7)  /*< 5 Gbps */
> > +#define ETH_LINK_SPEED_10G           (1 << 8)  /*< 10 Mbps */
> > +#define ETH_LINK_SPEED_20G           (1 << 9)  /*< 20 Gbps */
> > +#define ETH_LINK_SPEED_25G           (1 << 10)  /*< 25 Gbps */
> > +#define ETH_LINK_SPEED_40G           (1 << 11)  /*< 40 Gbps */
> > +#define ETH_LINK_SPEED_50G           (1 << 12)  /*< 50 Gbps */
> > +#define ETH_LINK_SPEED_56G           (1 << 13)  /*< 56 Gbps */
> > +#define ETH_LINK_SPEED_100G          (1 << 14)  /*< 100 Gbps */
> > +
> > +/**
> > + * Ethernet numeric link speeds in Mbps
> > + */
> > +#define ETH_SPEED_NUM_NONE   0      /*< Not defined */
> > +#define ETH_SPEED_NUM_10M    10     /*< 10 Mbps */
> > +#define ETH_SPEED_NUM_100M   100    /*< 100 Mbps */
> > +#define ETH_SPEED_NUM_1G     1000   /*< 1 Gbps */
> > +#define ETH_SPEED_NUM_2_5G   2500   /*< 2.5 Gbps */
> > +#define ETH_SPEED_NUM_5G     5000   /*< 5 Gbps */
> > +#define ETH_SPEED_NUM_10G    10000  /*< 10 Mbps */
> > +#define ETH_SPEED_NUM_20G    20000  /*< 20 Gbps */
> > +#define ETH_SPEED_NUM_25G    25000  /*< 25 Gbps */
> > +#define ETH_SPEED_NUM_40G    40000  /*< 40 Gbps */
> > +#define ETH_SPEED_NUM_50G    50000  /*< 50 Gbps */
> > +#define ETH_SPEED_NUM_56G    56000  /*< 56 Gbps */
> > +#define ETH_SPEED_NUM_100G   100000 /*< 100 Gbps */
> > +
> > +/**
> >   * A structure used to retrieve link-level information of an Ethernet
> port.
> >   */
> >  struct rte_eth_link {
> > -     uint16_t link_speed;      /**< ETH_LINK_SPEED_[10, 100, 1000,
> 10000] */
> > -     uint16_t link_duplex;     /**< ETH_LINK_[HALF_DUPLEX, FULL_DUPLEX]
> */
> > -     uint8_t  link_status : 1; /**< 1 -> link up, 0 -> link down */
> > -}__attribute__((aligned(8)));     /**< aligned for atomic64 read/write
> */
> > -
> > -#define ETH_LINK_SPEED_AUTONEG  0       /**< Auto-negotiate link speed.
> */
> > -#define ETH_LINK_SPEED_10       10      /**< 10 megabits/second. */
> > -#define ETH_LINK_SPEED_100      100     /**< 100 megabits/second. */
> > -#define ETH_LINK_SPEED_1000     1000    /**< 1 gigabits/second. */
> > -#define ETH_LINK_SPEED_10000    10000   /**< 10 gigabits/second. */
> > -#define ETH_LINK_SPEED_10G      10000   /**< alias of 10
> gigabits/second. */
> > -#define ETH_LINK_SPEED_20G      20000   /**< 20 gigabits/second. */
> > -#define ETH_LINK_SPEED_40G      40000   /**< 40 gigabits/second. */
> > +     uint32_t link_speed;       /**< Link speed (ETH_SPEED_NUM_) */
> > +     uint16_t link_duplex;      /**< 1 -> full duplex, 0 -> half duplex
> */
> > +     uint8_t link_autoneg : 1;  /**< 1 -> link speed has been autoneg */
> > +     uint8_t link_status  : 1;  /**< 1 -> link up, 0 -> link down */
> > +} __attribute__((aligned(8)));      /**< aligned for atomic64
> read/write */
> >
> > -#define ETH_LINK_AUTONEG_DUPLEX 0       /**< Auto-negotiate duplex. */
> > -#define ETH_LINK_HALF_DUPLEX    1       /**< Half-duplex connection. */
> > -#define ETH_LINK_FULL_DUPLEX    2       /**< Full-duplex connection. */
> > +/* Utility constants */
> > +#define ETH_LINK_HALF_DUPLEX    0    /**< Half-duplex connection. */
> > +#define ETH_LINK_FULL_DUPLEX    1    /**< Full-duplex connection. */
> > +#define ETH_LINK_SPEED_FIXED    0    /**< Link speed was not
> autonegociated. */
> > +#define ETH_LINK_SPEED_NEG      1    /**< Link speed was
> autonegociated. */
> > +#define ETH_LINK_DOWN                0       /**< Link is down. */
> > +#define ETH_LINK_UP          1       /**< Link is up. */
> >
> >  /**
> >   * A structure used to configure the ring threshold registers of an
> RX/TX
> > @@ -747,10 +780,14 @@ struct rte_intr_conf {
> >   * configuration settings may be needed.
> >   */
> >  struct rte_eth_conf {
> > -     uint16_t link_speed;
> > -     /**< ETH_LINK_SPEED_10[0|00|000], or 0 for autonegotation */
> > -     uint16_t link_duplex;
> > -     /**< ETH_LINK_[HALF_DUPLEX|FULL_DUPLEX], or 0 for autonegotation */
> > +     uint32_t link_speeds; /**< bitmap of ETH_LINK_SPEED_XXX of speeds
> to be
> > +                             used. ETH_LINK_SPEED_NO_AUTONEG disables
> link
> > +                             autonegociation, and a unique speed shall
> be
> > +                             set. Otherwise, the bitmap defines the set
> of
> > +                             speeds to be advertised. If the special
> value
> > +                             ETH_LINK_SPEED_AUTONEG (0) is used, all
> speeds
> > +                             supported are advertised.
> > +                             */
> This structure is allocated by applications, and the link_speed/duplex
> field may
> be accessed by them.  As such this is an ABI change and should go through
> the
> ABI process.  Arguably, while its not really supposed to be done, given
> that
> there are so many changes comming to rte_eth_dev for 2.2, I could see an
> argument for sliding this in with those changes, so we didn't have to wait
> for
> an additional relelase cycle.
>

If I understand you correctly you are arguing that this patch should go
through the process of NEXT_ABI -> unique implementation. I keep saying I
do not understand what is the benefit of doing so when the ABI for 2.2 is
going to change anyway (even other patches will modify ethdev ABI).

Having a strict rule for ABI changes between minor releases (e.g. 2.1.0 and
2.1.1) is a must or if the next release is announced to be ABI compatible.
Other than that I don't see it, specially since users have to recompile
anyway their binaries due to (other) ABI changes.

But maybe I am missing something?


> The removal of the Speed/Duplex macros should also be noted.
>

Agree. I will add it into the doc

Marc