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

[Thomas Monjalon]

17/07/2017 18:28, Stephen Hemminger:
> On Mon, 17 Jul 2017 19:14:16 +0300
> Andrew Rybchenko <arybchenko at 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 at 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

Right.
The comment is wrong.
rte_eth_link.link_autoneg must uses ETH_LINK_[FIXED/AUTONEG]
ETH_LINK_SPEED_[AUTONEG/FIXED] is for rte_eth_conf.link_speeds
and for rte_eth_dev_info.speed_capa.

So in this patch, you should set link.link_autoneg = ETH_LINK_FIXED.

I will send a patch to fix the comment in ethdev.