[dpdk-dev] [PATCH v11 0/8] ethdev: 100G and link speed API refactoring

Marc marcdevel at gmail.com
Sun Mar 27 21:39:30 CEST 2016


On 27 March 2016 at 11:53, Thomas Monjalon <thomas.monjalon at 6wind.com>
wrote:

> 2016-03-26 11:24, Marc:
> > On 26 March 2016 at 09:08, Thomas Monjalon <thomas.monjalon at 6wind.com>
> > wrote:
> > > 2016-03-25 22:30, Marc:
> > > > From v9 to v10 patchset the values ETH_LINK_SPEED_AUTONEG and
> > > ETH_LINK_SPEED_FIXED were flipped. Reverting this makes it work:
> > > >
> > > > marc at Beluga:~/personal/dpdk/tools$ git diff
> > > > diff --git a/lib/librte_ether/rte_ethdev.h
> > > b/lib/librte_ether/rte_ethdev.h
> > > > index ef2502a..fb247a7 100644
> > > > --- a/lib/librte_ether/rte_ethdev.h
> > > > +++ b/lib/librte_ether/rte_ethdev.h
> > > > @@ -244,8 +244,8 @@ struct rte_eth_stats {
> > > >  /**
> > > >   * Device supported speeds bitmap flags
> > > >   */
> > > > -#define ETH_LINK_SPEED_FIXED    (0 <<  0)  /**< Disable autoneg
> (fixed
> > > speed) */
> > > > -#define ETH_LINK_SPEED_AUTONEG  (1 <<  0)  /**< Autonegotiate (all
> > > speeds) */
> > > > +#define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all
> > > speeds) */
> > > > +#define ETH_LINK_SPEED_FIXED    (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 */
> > > >
> > > > I think having autoneg == 0 is better. Do you agree Thomas?
> > >
> > > No I do not agree, because this flag is now used also for
> > > rte_eth_link.link_autoneg.
> > > So it is more logic to set it to 1.
> >
> > Having to explicitly specify ETH_LINK_SPEED_AUTONEG in link_speeds during
> > port configuration for achieving auto-negociation, which is what 98% of
> > applications will use, seems anti-natural to me and breaks existing
> > applications.
>
> Yes, you're right. We have to avoid apps modifications.
> By keeping autoneg the default behaviour (value 0), we avoid issues.
>
> > The only benefit of your approach is not to have another macro #define
> > ETH_LINK_AUTONEG 1, which is marginal IMHO.
>
> Yes, you're right.
> I suggest adding 2 macros for rte_eth_link.link_autoneg:
> #define ETH_LINK_FIXED      0 /**< No autonegotiation. */
> #define ETH_LINK_AUTONEG    1 /**< Autonegotiated. */
>
> and keep these ones to use with rte_eth_conf.link_speeds and
> rte_eth_dev_info.speed_capa:
> #define ETH_LINK_SPEED_AUTONEG  (0 <<  0)  /**< Autonegotiate (all speeds)
> */
> #define ETH_LINK_SPEED_FIXED    (1 <<  0)  /**< Disable autoneg (fixed
> speed) */
>
> Opinions?
>


Agree on all of them. I can add them in the (hopefully) final v14, once all
or main drivers have been tested.

Regards
marc


More information about the dev mailing list