Re: [dpdk-dev] [RFC 02/14] virtio: use eth_link_read/write (and bug fix)

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 17 Jul 2017 19:14:16 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 07/17/2017 07:01 PM, Stephen Hemminger wrote:
> > On Sun, 16 Jul 2017 15:33:26 +0300
> > Andrew Rybchenko <arybchenko@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.