Re: [dpdk-dev] [PATCH] kni: fix compile errors on Oracle Linux6.4 and RHEL6.5

["Zhang, Helin" <helin.zhang@intel.com>]

Hi Horman and Thomas

Thank you guys for the good comments for this patch!
The checking if a function is defined will be removed, and will use self-defined functions no matter they have been defined somewhere or not. This will have the same behavior of latest Linux ixgbe/igb driver.

I guess the KNI ethtool support might need to be updated soon to which is based on the latest Linux source code.

Regards,
Helin

-----Original Message-----
From: Neil Horman [mailto:nhorman@tuxdriver.com] 
Sent: Tuesday, June 10, 2014 7:23 PM
To: Zhang, Helin
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] kni: fix compile errors on Oracle Linux6.4 and RHEL6.5

On Mon, Jun 09, 2014 at 04:38:55PM +0800, Helin Zhang wrote:
> From: HELIN ZHANG <helin.zhang@intel.com>
> 
> The compile errors are as follows. The fixes came from standard Linux 
> drivers of ixgbe-3.21.2 and igb-5.1.2.
> 
> * Oracle Linux6.4
> lib/librte_eal/linuxapp/kni/ethtool/ixgbe/kcompat.h:3111:
> error: redefinition of 'ether_addr_equal'
> include/linux/etherdevice.h:180: note: previous definition of 
> 'ether_addr_equal' was here
> * RHEL6.5
> lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h:3597:
> error: redefinition of 'mmd_eee_cap_to_ethtool_sup_t'
> include/linux/mdio.h:387: note: previous definition of 
> 'mmd_eee_cap_to_ethtool_sup_t' was here
> lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h:3625:
> error: redefinition of 'mmd_eee_adv_to_ethtool_adv_t'
> include/linux/mdio.h:415: note: previous definition of 
> 'mmd_eee_adv_to_ethtool_adv_t' was here
> lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h:3653:
> error: redefinition of 'ethtool_adv_to_mmd_eee_adv_t'
> include/linux/mdio.h:443: note: previous definition of 
> 'ethtool_adv_to_mmd_eee_adv_t' was here
> 
> Signed-off-by: Helin Zhang <helin.zhang@intel.com>
> Acked-by: Cunming Liang <cunming.liang@intel.com>
> Tested-by: Waterman Cao <waterman.cao@intel.com>
> ---
>  lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h  | 29 ++++++++++++++++------
>  .../linuxapp/kni/ethtool/ixgbe/kcompat.h           |  5 ++--
>  2 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h 
> b/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
> index 4c27d5d..26bf0b2 100644
> --- a/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
> +++ b/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
> @@ -3534,12 +3534,13 @@ extern void _kc_skb_add_rx_frag(struct sk_buff 
> *, int, struct page *,  
> /*********************************************************************
> ********/  #if ( LINUX_VERSION_CODE < KERNEL_VERSION(3,5,0) )  #define 
> skb_tx_timestamp(skb) do {} while (0) -#if !(RHEL_RELEASE_CODE && 
> RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(6,4)) -static inline bool 
> ether_addr_equal(const u8 *addr1, const u8 *addr2)
> +#ifndef ether_addr_equal
> +static inline bool __kc_ether_addr_equal(const u8 *addr1, const u8 
> +*addr2)
>  {
>  	return !compare_ether_addr(addr1, addr2);  } -#endif
> +#define ether_addr_equal(_addr1, _addr2) 
> +__kc_ether_addr_equal((_addr1),(_addr2))
> +#endif /* __kc_ether_addr_equal*/
I don't see why you're wrapping this in a macro twice, just #defining ether_addr_equal to !compare_ether_addr should be sufficient here.

>  #else
>  #define HAVE_FDB_OPS
>  #define HAVE_ETHTOOL_GET_TS_INFO
> @@ -3586,7 +3587,8 @@ static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
>  #define ADVERTISED_40000baseLR4_Full	(1 << 26)
>  #endif
>  
> -#if defined(ETHTOOL_GEEE) || (RHEL_RELEASE_CODE && RHEL_RELEASE_CODE 
> <= RHEL_RELEASE_VERSION(6,4))
> +#if defined(ETHTOOL_GEEE)
> +#ifndef mmd_eee_cap_to_ethtool_sup_t
Why are we even bothering with gating this on ETHTOOL_GEEE?  Or building it in at all for that matter?  ETHTOOL_GEEE doesn't having any dependence on any hardware feature availability, we can just enable it unilaterally (if its not already defined, though doing an ifndef on a function thats defined as such is a bit wierd).  

Theres also an argument for not doing this compat code at all, as nothing currently uses it.  the KNI is the only code that could reach this path, and nothing in the KNI does so at the moment.  We could just remove this code and turn off support in the igb pmd instead, it would save some useless code space.

>  /**
>   * mmd_eee_cap_to_ethtool_sup_t
>   * @eee_cap: value of the MMD EEE Capability register @@ -3594,7 
> +3596,7 @@ static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
>   * A small helper function that translates MMD EEE Capability (3.20) bits
>   * to ethtool supported settings.
>   */
> -static inline u32 mmd_eee_cap_to_ethtool_sup_t(u16 eee_cap)
> +static inline u32 __kc_mmd_eee_cap_to_ethtool_sup_t(u16 eee_cap)
>  {
>  	u32 supported = 0;
>  
> @@ -3613,7 +3615,11 @@ static inline u32 
> mmd_eee_cap_to_ethtool_sup_t(u16 eee_cap)
>  
>  	return supported;
>  }
> +#define mmd_eee_cap_to_ethtool_sup_t(eee_cap) \
> +	__kc_mmd_eee_cap_to_ethtool_sup_t(eee_cap)
> +#endif /* __kc_mmd_eee_cap_to_ethtool_sup_t */
>  
> +#ifndef mmd_eee_adv_to_ethtool_adv_t
>  /**
>   * mmd_eee_adv_to_ethtool_adv_t
>   * @eee_adv: value of the MMD EEE Advertisement/Link Partner Ability 
> registers @@ -3622,7 +3628,7 @@ static inline u32 mmd_eee_cap_to_ethtool_sup_t(u16 eee_cap)
>   * and MMD EEE Link Partner Ability (7.61) bits to ethtool advertisement
>   * settings.
>   */
> -static inline u32 mmd_eee_adv_to_ethtool_adv_t(u16 eee_adv)
> +static inline u32 __kc_mmd_eee_adv_to_ethtool_adv_t(u16 eee_adv)
>  {
>  	u32 adv = 0;
>  
> @@ -3641,7 +3647,11 @@ static inline u32 
> mmd_eee_adv_to_ethtool_adv_t(u16 eee_adv)
>  
>  	return adv;
>  }
> +#define mmd_eee_adv_to_ethtool_adv_t(eee_adv) \
> +	__kc_mmd_eee_adv_to_ethtool_adv_t(eee_adv)
> +#endif /* mmd_eee_adv_to_ethtool_adv_t */
>  
> +#ifndef ethtool_adv_to_mmd_eee_adv_t
>  /**
>   * ethtool_adv_to_mmd_eee_adv_t
>   * @adv: the ethtool advertisement settings @@ -3650,7 +3660,7 @@ 
> static inline u32 mmd_eee_adv_to_ethtool_adv_t(u16 eee_adv)
>   * to EEE advertisements for the MMD EEE Advertisement (7.60) and
>   * MMD EEE Link Partner Ability (7.61) registers.
>   */
> -static inline u16 ethtool_adv_to_mmd_eee_adv_t(u32 adv)
> +static inline u16 __kc_ethtool_adv_to_mmd_eee_adv_t(u32 adv)
>  {
>  	u16 reg = 0;
>  
> @@ -3669,7 +3679,10 @@ static inline u16 
> ethtool_adv_to_mmd_eee_adv_t(u32 adv)
>  
>  	return reg;
>  }
> -#endif
> +#define ethtool_adv_to_mmd_eee_adv_t(adv) \
> +__kc_ethtool_adv_to_mmd_eee_adv_t(adv)
> +#endif /* ethtool_adv_to_mmd_eee_adv_t */ #endif /* */
>  
>  #ifndef pci_pcie_type
>  #if ( LINUX_VERSION_CODE < KERNEL_VERSION(2,6,24) ) diff --git 
> a/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/kcompat.h 
> b/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/kcompat.h
> index 4126d14..4e97886 100644
> --- a/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/kcompat.h
> +++ b/lib/librte_eal/linuxapp/kni/ethtool/ixgbe/kcompat.h
> @@ -3107,11 +3107,12 @@ typedef netdev_features_t 
> kni_netdev_features_t;
>  
>  
> /*********************************************************************
> ********/  #if ( LINUX_VERSION_CODE < KERNEL_VERSION(3,5,0) ) -#if 
> !(RHEL_RELEASE_CODE && RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(6,4)) 
> -static inline bool ether_addr_equal(const u8 *addr1, const u8 *addr2)
> +#ifndef ether_addr_equal
> +static inline bool __kc_ether_addr_equal(const u8 *addr1, const u8 
> +*addr2)
>  {
>  	return !compare_ether_addr(addr1, addr2);  }
> +#define ether_addr_equal(_addr1, _addr2) 
> +__kc_ether_addr_equal((_addr1),(_addr2))
>  #endif
>  #else
>  #define HAVE_FDB_OPS
> --
> 1.8.1.4
> 
>