linux-kernel - Re: [PATCH] nfp: convert nfp_eth_set_bit

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 4 Oct 2017 11:44:59 -0700
From:   Matthias Kaehlcke <mka@...omium.org>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     Manoj Gupta <manojgupta@...omium.org>,
        "David S . Miller" <davem@...emloft.net>,
        Simon Horman <simon.horman@...ronome.com>,
        Dirk van der Merwe <dirk.vandermerwe@...ronome.com>,
        oss-drivers@...ronome.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Renato Golin <renato.golin@...aro.org>,
        Guenter Roeck <groeck@...omium.org>,
        Doug Anderson <dianders@...omium.org>
Subject: Re: [PATCH] nfp: convert nfp_eth_set_bit_config() into a macro

El Wed, Oct 04, 2017 at 11:17:24AM -0700 Jakub Kicinski ha dit:

> On Wed, 4 Oct 2017 10:42:42 -0700, Manoj Gupta wrote:
> > Hi Jakub,
> > 
> > I had discussed about supporting this code with some clang developers.
> > However, the consensus was this code relies on a specific GCC optimizer
> > behavior and Clang does not share the same behavior by design.
> 
> Hm.  I find surprising that constant propagation to inlined functions
> is considered something GCC-specific rather than obvious/basic.
> 
> > Note that even GCC can't compile this code at -O0.
> 
> Yes, that makes me feel uncomfortable...  Could you try this?

The kernel as a whole does not build with -O0, so I couldn't try
that. I know Manoj (who isn't a kernel dev) isolated the
compiletime_assert macros and encountered the same 'undefined
reference' errors with gcc -O0 that we are seeing with clang.
This is due to:

 "if you use it in an inlined function and pass an argument of the
 function as the argument to the built-in, GCC never returns 1 when
 you call the inline function with a string constant or compound
 literal (see Compound Literals) and does not return 1 when you pass a
 constant numeric value to the inline function unless you specify the
 -O option."

https://gcc.gnu.org/onlinedocs/gcc-7.2.0/gcc/Other-Builtins.html

> diff --git a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> index f6f7c085f8e0..47251396fcae 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfpcore/nfp_nsp_eth.c
> @@ -469,10 +469,10 @@ int nfp_eth_set_configured(struct nfp_cpp *cpp, unsigned int idx, bool configed)
>  	return nfp_eth_config_commit_end(nsp);
>  }
>  
> -/* Force inline, FIELD_* macroes require masks to be compilation-time known */
> -static __always_inline int
> +static int
>  nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
> -		       const u64 mask, unsigned int val, const u64 ctrl_bit)
> +		       const u64 mask, const unsigned int shift,
> +		       unsigned int val, const u64 ctrl_bit)
>  {
>  	union eth_table_entry *entries = nfp_nsp_config_entries(nsp);
>  	unsigned int idx = nfp_nsp_config_idx(nsp);
> @@ -489,11 +489,11 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>  
>  	/* Check if we are already in requested state */
>  	reg = le64_to_cpu(entries[idx].raw[raw_idx]);
> -	if (val == FIELD_GET(mask, reg))
> +	if (val == (reg & mask) >> shift)
>  		return 0;
>  
>  	reg &= ~mask;
> -	reg |= FIELD_PREP(mask, val);
> +	reg |= (val << shift) & mask;
>  	entries[idx].raw[raw_idx] = cpu_to_le64(reg);
>  
>  	entries[idx].control |= cpu_to_le64(ctrl_bit);
> @@ -503,6 +503,13 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>  	return 0;
>  }
>  
> +#define NFP_ETH_SET_BIT_CONFIG(nsp, raw_idx, mask, val, ctrl_bit)	\
> +	({								\
> +		__BF_FIELD_CHECK(mask, 0ULL, val, "NFP_ETH_SET_BIT_CONFIG: "); \
> +		nfp_eth_set_bit_config(nsp, raw_idx, mask, __bf_shf(mask), \
> +				       val, ctrl_bit);			\
> +	})
> +
>  /**
>   * __nfp_eth_set_aneg() - set PHY autonegotiation control bit
>   * @nsp:	NFP NSP handle returned from nfp_eth_config_start()
> @@ -515,7 +522,7 @@ nfp_eth_set_bit_config(struct nfp_nsp *nsp, unsigned int raw_idx,
>   */
>  int __nfp_eth_set_aneg(struct nfp_nsp *nsp, enum nfp_eth_aneg mode)
>  {
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
>  				      NSP_ETH_STATE_ANEG, mode,
>  				      NSP_ETH_CTRL_SET_ANEG);
>  }
> @@ -544,7 +551,7 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
>  		return -EINVAL;
>  	}
>  
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_STATE,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_STATE,
>  				      NSP_ETH_STATE_RATE, rate,
>  				      NSP_ETH_CTRL_SET_RATE);
>  }
> @@ -561,6 +568,6 @@ int __nfp_eth_set_speed(struct nfp_nsp *nsp, unsigned int speed)
>   */
>  int __nfp_eth_set_split(struct nfp_nsp *nsp, unsigned int lanes)
>  {
> -	return nfp_eth_set_bit_config(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
> +	return NFP_ETH_SET_BIT_CONFIG(nsp, NSP_ETH_RAW_PORT, NSP_ETH_PORT_LANES,
>  				      lanes, NSP_ETH_CTRL_SET_LANES);
>  }