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

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

[Helin Zhang]

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*/
 #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
 /**
  * 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

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

[Thomas Monjalon]

Hi Helin,

2014-06-09 16:38, Helin Zhang:
>  #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

It should be always true as it is a function (not known by the preprocessor).

> +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*/

So it is always replacing ether_addr_equal() by a kcompat equivalent for old kernels,
even if ether_addr_equal() is already defined as a C function.
Just to confirm: is it really what we want?

[...]
> -#endif
> +#define ethtool_adv_to_mmd_eee_adv_t(adv) \
> +__kc_ethtool_adv_to_mmd_eee_adv_t(adv)

An indentation is missing here.

> +#endif /* ethtool_adv_to_mmd_eee_adv_t */
> +#endif /* */

Why an empty comment?

Thanks
-- 
Thomas

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

[Neil Horman]

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
> 
> 

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

[Zhang, Helin]

Hi Thomas

Were you talking about the "#ifndef ether_addr_equal "?
The compile errors I copied in that patch shows that the function has already been defined in include/linux/etherdevice.h on Oracle Linux6.4. In this case, it seems not always true as you said.
The method came from the standard Linux driver, so I think it should be useful and has been reviewed by Linux kernel community.

Oops, it seems that there are useless code which I need to remove. I will update it later! Thank you very much for the pointing!

Regards,
Helin

-----Original Message-----
From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
Sent: Tuesday, June 10, 2014 6:03 PM
To: Zhang, Helin
Cc: dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH] kni: fix compile errors on Oracle Linux6.4 and RHEL6.5

Hi Helin,

2014-06-09 16:38, Helin Zhang:
>  #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

It should be always true as it is a function (not known by the preprocessor).

> +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*/

So it is always replacing ether_addr_equal() by a kcompat equivalent for old kernels, even if ether_addr_equal() is already defined as a C function.
Just to confirm: is it really what we want?

[...]
> -#endif
> +#define ethtool_adv_to_mmd_eee_adv_t(adv) \
> +__kc_ethtool_adv_to_mmd_eee_adv_t(adv)

An indentation is missing here.

> +#endif /* ethtool_adv_to_mmd_eee_adv_t */ #endif /* */

Why an empty comment?

Thanks
--
Thomas

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

[Thomas Monjalon]

Hi Helin,

Please answer below the question.

2014-06-10 14:59, Zhang, Helin:
> Were you talking about the "#ifndef ether_addr_equal "?

yes

> The compile errors I copied in that patch shows that the function
> has already been defined in include/linux/etherdevice.h on Oracle
> Linux6.4. In this case, it seems not always true as you said.

No, ether_addr_equal() is defined for the processor but not for the preprocessor.

> The method came from the standard Linux driver, so I think
> it should be useful and has been reviewed by Linux kernel community.

No, I think that this code come from the opaque Intel development
of the sourceforge driver.
On kernel.org, there is no need for kcompat glue as drivers are in-tree.

> Oops, it seems that there are useless code which I need to remove.
> I will update it later! Thank you very much for the pointing!

Thanks
-- 
Thomas

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com] 
> Sent: Tuesday, June 10, 2014 6:03 PM
> To: Zhang, Helin
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] kni: fix compile errors on Oracle Linux6.4 and RHEL6.5
> 
> Hi Helin,
> 
> 2014-06-09 16:38, Helin Zhang:
> >  #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
> 
> It should be always true as it is a function (not known by the preprocessor).
> 
> > +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*/
> 
> So it is always replacing ether_addr_equal() by a kcompat equivalent for old kernels, even if ether_addr_equal() is already defined as a C function.
> Just to confirm: is it really what we want?
> 
> [...]
> > -#endif
> > +#define ethtool_adv_to_mmd_eee_adv_t(adv) \
> > +__kc_ethtool_adv_to_mmd_eee_adv_t(adv)
> 
> An indentation is missing here.
> 
> > +#endif /* ethtool_adv_to_mmd_eee_adv_t */ #endif /* */
> 
> Why an empty comment?
> 
> Thanks
> --
> Thomas

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

[Zhang, Helin]

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
> 
>