On Mon, 17 Jul 2017 19:14:16 +0300 Andrew Rybchenko <arybche...@solarflare.com> wrote:
> On 07/17/2017 07:01 PM, Stephen Hemminger wrote: > > On Sun, 16 Jul 2017 15:33:26 +0300 > > Andrew Rybchenko <arybche...@solarflare.com> wrote: > > > >>> + link.link_autoneg = ETH_LINK_SPEED_FIXED; > >> As I understand link_autoneg is 1 bit field with boolean semantics. I.e. > >> 0/false - no autoneg, 1/true - autoneg. > >> It looks like it has wrong comment: > >> uint16_t link_autoneg : 1; /**< ETH_LINK_SPEED_[AUTONEG/FIXED] > >> */ > >> > >> since > >> #define ETH_LINK_SPEED_AUTONEG (0 << 0) /**< Autonegotiate (all speeds) > >> */ > >> #define ETH_LINK_SPEED_FIXED (1 << 0) /**< Disable autoneg (fixed > >> speed) */ > >> > >> whereas > >> #define ETH_LINK_FIXED 0 /**< No autonegotiation. */ > >> #define ETH_LINK_AUTONEG 1 /**< Autonegotiated. */ > >> > >> In general this attempt to introduce bug is the result of wrong comment > >> which is caused by very similar > >> defines with opposite values. > > Orignal observation was because some drivers (vmxnet3) were setting autoneg > > = fixed > > and others were not. Turns out it makes no difference > > since FIXED == 0, the old code and new code have same effect. > > May be I miss something, but ETH_LINK_SPEED_FIXED==1, but > ETH_LINK_FIXED==0. > So, initially it was 0 (fixed speed), but now it is 1 (autoneg). My understanding is that ETH_SPEED_XXX and ETH_LINK_FIXED are for rte_eth_link structure; and ETH_LINK_SPEED_XXX values are for dev_info->speed_capa Would be good to use some kind of typedef for this, maybe introduce a bitfield for speed_capa and get rid of the ETH_LINK_SPEED_XXX.
