Re: [PATCH 1/6] net: ethernet: efct: New X3 net driver

[Simon Horman <simon.horman@corigine.com>]

On Fri, Feb 10, 2023 at 06:33:16PM +0530, Harsh Jain wrote:
> This patch series adds new ethernet network driver for Alveo X3522[1].
> is a low-latency NIC with an aim to deliver the lowest possible
> latency. It accelerates a range of diverse trading strategies
> and financial applications.
> 
> Device has 2 PCI functions and each function supports 2 port, currently
> at 10GbE. This patch deals with PCI device probing and netdev creation.
> It also adds support for Firmware communication APIs used in later patches.
> 
> [1] https://www.xilinx.com/x3
> 
> Signed-off-by: Abhijit Gangurde<abhijit.gangurde@amd.com>
> Signed-off-by: Puneet Gupta <puneet.gupta@amd.com>
> Signed-off-by: Nikhil Agarwal<nikhil.agarwal@amd.com>
> Signed-off-by: Tarak Reddy<tarak.reddy@amd.com>
> Signed-off-by: Harsh Jain <h.jain@amd.com>
> ---
>  drivers/net/ethernet/amd/efct/efct_bitfield.h |  483 ++
>  drivers/net/ethernet/amd/efct/efct_common.c   | 1154 ++++
>  drivers/net/ethernet/amd/efct/efct_common.h   |  134 +
>  drivers/net/ethernet/amd/efct/efct_driver.h   |  770 +++
>  drivers/net/ethernet/amd/efct/efct_enum.h     |  130 +
>  drivers/net/ethernet/amd/efct/efct_evq.c      |  185 +
>  drivers/net/ethernet/amd/efct/efct_evq.h      |   21 +
>  drivers/net/ethernet/amd/efct/efct_io.h       |   64 +
>  drivers/net/ethernet/amd/efct/efct_netdev.c   |  459 ++
>  drivers/net/ethernet/amd/efct/efct_netdev.h   |   19 +
>  drivers/net/ethernet/amd/efct/efct_nic.c      | 1300 ++++
>  drivers/net/ethernet/amd/efct/efct_nic.h      |  104 +
>  drivers/net/ethernet/amd/efct/efct_pci.c      | 1077 +++
>  drivers/net/ethernet/amd/efct/efct_reg.h      | 1060 +++
>  drivers/net/ethernet/amd/efct/mcdi.c          | 1817 ++++++
>  drivers/net/ethernet/amd/efct/mcdi.h          |  373 ++
>  .../net/ethernet/amd/efct/mcdi_functions.c    |  642 ++
>  .../net/ethernet/amd/efct/mcdi_functions.h    |   39 +
>  drivers/net/ethernet/amd/efct/mcdi_pcol.h     | 5789 +++++++++++++++++
>  .../net/ethernet/amd/efct/mcdi_port_common.c  |  949 +++
>  .../net/ethernet/amd/efct/mcdi_port_common.h  |   98 +
>  21 files changed, 16667 insertions(+)

This is a very large patch to review.

>  create mode 100644 drivers/net/ethernet/amd/efct/efct_bitfield.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_common.c
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_common.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_driver.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_enum.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_evq.c
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_evq.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_io.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_netdev.c
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_netdev.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_nic.c
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_nic.h
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_pci.c
>  create mode 100644 drivers/net/ethernet/amd/efct/efct_reg.h
>  create mode 100644 drivers/net/ethernet/amd/efct/mcdi.c
>  create mode 100644 drivers/net/ethernet/amd/efct/mcdi.h
>  create mode 100644 drivers/net/ethernet/amd/efct/mcdi_functions.c
>  create mode 100644 drivers/net/ethernet/amd/efct/mcdi_functions.h
>  create mode 100644 drivers/net/ethernet/amd/efct/mcdi_pcol.h
>  create mode 100644 drivers/net/ethernet/amd/efct/mcdi_port_common.c
>  create mode 100644 drivers/net/ethernet/amd/efct/mcdi_port_common.h
> 
> diff --git a/drivers/net/ethernet/amd/efct/efct_bitfield.h b/drivers/net/ethernet/amd/efct/efct_bitfield.h
> new file mode 100644
> index 000000000000..bd67e0fa08f9
> --- /dev/null
> +++ b/drivers/net/ethernet/amd/efct/efct_bitfield.h
> @@ -0,0 +1,483 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + ****************************************************************************
> + * Driver for AMD/Xilinx network controllers and boards
> + * Copyright (C) 2021, Xilinx, Inc.
> + * Copyright (C) 2022-2023, Advanced Micro Devices, Inc.
> + */
> +
> +#ifndef EFCT_BITFIELD_H
> +#define EFCT_BITFIELD_H
> +#include <linux/types.h>
> +
> +/* Efct bitfield access
> + * NIC uses bitfield of upto 128 bits wide.  Since there is no
> + * native 128-bit datatype on most systems, and 64-bit datatypes
> + * are inefficient on 32-bit systems and vice versa,
> + * we wrap accesses in a way that uses the most efficient
> + * datatype.
> + *
> + * The NICs are PCI devices and therefore little-endian.  Since most
> + * of the quantities that we deal with are DMAed to/from host memory,
> + * we define our datatypes (union efct_oword, union efct_dword and
> + * union efct_qword) to be little-endian.
> + */
> +
> +/* Lowest bit numbers and widths */
> +#define EFCT_DUMMY_FIELD_LBN 0
> +#define EFCT_DUMMY_FIELD_WIDTH 0
> +#define EFCT_WORD_0_LBN 0
> +#define EFCT_WORD_0_WIDTH 16
> +#define EFCT_WORD_1_LBN 16
> +#define EFCT_WORD_1_WIDTH 16
> +#define EFCT_DWORD_0_LBN 0
> +#define EFCT_DWORD_0_WIDTH 32
> +#define EFCT_DWORD_1_LBN 32
> +#define EFCT_DWORD_1_WIDTH 32
> +#define EFCT_DWORD_2_LBN 64
> +#define EFCT_DWORD_2_WIDTH 32
> +#define EFCT_DWORD_3_LBN 96
> +#define EFCT_DWORD_3_WIDTH 32
> +#define EFCT_QWORD_0_LBN 0
> +#define EFCT_QWORD_0_WIDTH 64

nit: The above would be easier to read if the values were vertically aligned
nit 2: blank line here

> +/* Specified attribute (e.g. LBN) of the specified field */
> +#define EFCT_VAL(field, attribute) field ## _ ## attribute

> +/* Low bit number of the specified field */
> +#define EFCT_LOW_BIT(field) EFCT_VAL(field, LBN)
> +/* Bit width of the specified field */
> +#define EFCT_WIDTH(field) EFCT_VAL(field, WIDTH)
> +/* High bit number of the specified field */
> +#define EFCT_HIGH_BIT(field) (EFCT_LOW_BIT(field) + EFCT_WIDTH(field) - 1)

nit2: and here

> +/* Mask equal in width to the specified field.
> + *
> + * For example, a field with width 5 would have a mask of 0x1f.
> + *
> + * The maximum width mask that can be generated is 64 bits.
> + */
> +#define EFCT_MASK64(width)			\
> +	((width) == 64 ? ~((u64)0) :		\
> +	 (((((u64)1) << (width))) - 1))

Maybe:

#define EFCT_MASK64_B(width) ((width) ? GENMASK_ULL((width) - 1, 0) : 0ULL)

> +
> +/* Mask equal in width to the specified field.
> + *
> + * For example, a field with width 5 would have a mask of 0x1f.
> + *
> + * The maximum width mask that can be generated is 32 bits.  Use
> + * EFCT_MASK64 for higher width fields.
> + */
> +#define EFCT_MASK32(width)			\
> +	((width) == 32 ? ~((u32)0) :		\
>  +	 (((((u32)1) << (width))) - 1))

Maybe:

#define EFCT_MASK32_B(width) ((width) ? GENMASK((width) - 1, 0) : 0UL)

> +
> +/* A doubleword (i.e. 4 byte) datatype - little-endian in HW */
> +union efct_dword {
> +	__le32 word32;
> +};

I see where you are going with consistency for
efct_dword, efct_qword and efct_oword.
But does a union with one element provide value?

> +
> +/* A quadword (i.e. 8 byte) datatype - little-endian in HW */
> +union efct_qword {
> +	__le64 u64[1];
> +	__le32 u32[2];
> +	union efct_dword dword[2];

dword is an alias for .ue2. I'm not sure I see the value in this.

> +};
> +
> +/* An octword (eight-word, i.e. 16 byte) datatype - little-endian in HW */
> +union efct_oword {
> +	__le64 u64[2];
> +	union efct_qword qword[2];
> +	__le32 u32[4];
> +	union efct_dword dword[4];

ditto

> +};
> +
> +/* Format string and value expanders for printk */
> +#define EFCT_DWORD_FMT "%08x"
> +#define EFCT_OWORD_FMT "%08x:%08x:%08x:%08x"
> +#define EFCT_DWORD_VAL(dword)				\
> +	((u32)le32_to_cpu((dword).word32))
> +
> +/* Extract bit field portion [low,high) from the native-endian element
> + * which contains bits [min,max).
> + * For example, suppose "element" represents the high 32 bits of a
> + * 64-bit value, and we wish to extract the bits belonging to the bit
> + * field occupying bits 28-45 of this 64-bit value.
> + * Then EFCT_EXTRACT ( element, 32, 63, 28, 45 ) would give
> + *
> + *   ( element ) << 4
> + * The result will contain the relevant bits filled in the range
> + * [0,high-low), with garbage in bits [high-low+1,...).

In this case [0,min-low) will be zero filled.
But maybe that never happens so it's not important that it is spelt out.

Garbage bits sounds fun.

Did you consider implementing this using GET_FIELD and GENMASK_ULL?

> + */
> +#define EFCT_EXTRACT_NATIVE(native_element, min, max, low, high)		\
> +	((low) > (max) || (high) < (min) ? 0 :				\
> +	 (low) > (min) ?						\
> +	 (native_element) >> ((low) - (min)) :				\
> +	 (native_element) << ((min) - (low)))
> +
> +/* Extract bit field portion [low,high) from the 64-bit little-endian
> + * element which contains bits [min,max)
> + */
> +#define EFCT_EXTRACT64(element, min, max, low, high)			\
> +	EFCT_EXTRACT_NATIVE(le64_to_cpu(element), min, max, low, high)
> +
> +/* Extract bit field portion [low,high) from the 32-bit little-endian
> + * element which contains bits [min,max)
> + */
> +#define EFCT_EXTRACT32(element, min, max, low, high)			\
> +	EFCT_EXTRACT_NATIVE(le32_to_cpu(element), min, max, low, high)
> +
> +#define EFCT_EXTRACT_OWORD64(oword, low, high)				\
> +	((EFCT_EXTRACT64((oword).u64[0], 0, 63, low, high) |		\
> +	  EFCT_EXTRACT64((oword).u64[1], 64, 127, low, high)) &		\
> +	 EFCT_MASK64((high) + 1 - (low)))
> +
> +#define EFCT_EXTRACT_QWORD64(qword, low, high)				\
> +	(EFCT_EXTRACT64((qword).u64[0], 0, 63, low, high) &		\
> +	 EFCT_MASK64((high) + 1 - (low)))
> +
> +#define EFCT_EXTRACT_OWORD32(oword, low, high)				\
> +	((EFCT_EXTRACT32((oword).u32[0], 0, 31, low, high) |		\
> +	  EFCT_EXTRACT32((oword).u32[1], 32, 63, low, high) |		\
> +	  EFCT_EXTRACT32((oword).u32[2], 64, 95, low, high) |		\
> +	  EFCT_EXTRACT32((oword).u32[3], 96, 127, low, high)) &		\
> +	 EFCT_MASK32((high) + 1 - (low)))
> +
> +#define EFCT_EXTRACT_QWORD32(qword, low, high)				\
> +	((EFCT_EXTRACT32((qword).u32[0], 0, 31, low, high) |		\
> +	  EFCT_EXTRACT32((qword).u32[1], 32, 63, low, high)) &		\
> +	 EFCT_MASK32((high) + 1 - (low)))
> +
> +#define EFCT_EXTRACT_DWORD(dword, low, high)			\
> +	(EFCT_EXTRACT32((dword).word32, 0, 31, low, high) &	\
> +	 EFCT_MASK32((high) + 1 - (low)))
> +
> +#define EFCT_OWORD_FIELD64(oword, field)				\
> +	EFCT_EXTRACT_OWORD64(oword, EFCT_LOW_BIT(field),		\
> +			    EFCT_HIGH_BIT(field))
> +
> +#define EFCT_QWORD_FIELD64(qword, field)				\
> +	EFCT_EXTRACT_QWORD64(qword, EFCT_LOW_BIT(field),		\
> +			    EFCT_HIGH_BIT(field))
> +
> +#define EFCT_OWORD_FIELD32(oword, field)				\
> +	EFCT_EXTRACT_OWORD32(oword, EFCT_LOW_BIT(field),		\
> +			    EFCT_HIGH_BIT(field))
> +
> +#define EFCT_QWORD_FIELD32(qword, field)				\
> +	EFCT_EXTRACT_QWORD32(qword, EFCT_LOW_BIT(field),		\
> +			    EFCT_HIGH_BIT(field))
> +
> +#define EFCT_DWORD_FIELD(dword, field)				\
> +	EFCT_EXTRACT_DWORD(dword, EFCT_LOW_BIT(field),		\
> +			  EFCT_HIGH_BIT(field))
> +
> +#define EFCT_OWORD_IS_ZERO64(oword)					\
> +	(((oword).u64[0] | (oword).u64[1]) == (__force __le64)0)
> +
> +#define EFCT_QWORD_IS_ZERO64(qword)					\
> +	(((qword).u64[0]) == (__force __le64)0)
> +
> +#define EFCT_OWORD_IS_ZERO32(oword)					     \
> +	(((oword).u32[0] | (oword).u32[1] | (oword).u32[2] | (oword).u32[3]) \
> +	 == (__force __le32)0)
> +
> +#define EFCT_QWORD_IS_ZERO32(qword)					\
> +	(((qword).u32[0] | (qword).u32[1]) == (__force __le32)0)
> +
> +#define EFCT_DWORD_IS_ZERO(dword)					\
> +	(((dword).u32[0]) == (__force __le32)0)
> +
> +#define EFCT_OWORD_IS_ALL_ONES64(oword)					\
> +	(((oword).u64[0] & (oword).u64[1]) == ~((__force __le64)0))
> +
> +#define EFCT_QWORD_IS_ALL_ONES64(qword)					\
> +	((qword).u64[0] == ~((__force __le64)0))
> +
> +#define EFCT_OWORD_IS_ALL_ONES32(oword)					\
> +	(((oword).u32[0] & (oword).u32[1] & (oword).u32[2] & (oword).u32[3]) \
> +	 == ~((__force __le32)0))
> +
> +#define EFCT_QWORD_IS_ALL_ONES32(qword)					\
> +	(((qword).u32[0] & (qword).u32[1]) == ~((__force __le32)0))
> +
> +#define EFCT_DWORD_IS_ALL_ONES(dword)					\
> +	((dword).u32[0] == ~((__force __le32)0))
> +
> +#if BITS_PER_LONG == 64
> +#define EFCT_OWORD_FIELD		EFCT_OWORD_FIELD64
> +#define EFCT_QWORD_FIELD		EFCT_QWORD_FIELD64
> +#define EFCT_OWORD_IS_ZERO	EFCT_OWORD_IS_ZERO64
> +#define EFCT_QWORD_IS_ZERO	EFCT_QWORD_IS_ZERO64
> +#define EFCT_OWORD_IS_ALL_ONES	EFCT_OWORD_IS_ALL_ONES64
> +#define EFCT_QWORD_IS_ALL_ONES	EFCT_QWORD_IS_ALL_ONES64
> +#else
> +#define EFCT_OWORD_FIELD		EFCT_OWORD_FIELD32
> +#define EFCT_QWORD_FIELD		EFCT_QWORD_FIELD32
> +#define EFCT_OWORD_IS_ZERO	EFCT_OWORD_IS_ZERO32
> +#define EFCT_QWORD_IS_ZERO	EFCT_QWORD_IS_ZERO32
> +#define EFCT_OWORD_IS_ALL_ONES	EFCT_OWORD_IS_ALL_ONES32
> +#define EFCT_QWORD_IS_ALL_ONES	EFCT_QWORD_IS_ALL_ONES32
> +#endif
> +
> +/* Construct bit field portion
> + * Creates the portion of the bit field [low,high) that lies within
> + * the range [min,max).
> + */
> +#define EFCT_INSERT_NATIVE64(min, max, low, high, value)		\
> +	((((low) > (max)) || ((high) < (min))) ? 0 :			\
> +	 (((low) > (min)) ?						\
> +	  (((u64)(value)) << ((low) - (min))) :		\
> +	  (((u64)(value)) >> ((min) - (low)))))
> +
> +#define EFCT_INSERT_NATIVE32(min, max, low, high, value)		\
> +	((((low) > (max)) || ((high) < (min))) ? 0 :			\
> +	 (((low) > (min)) ?						\
> +	  (((u32)(value)) << ((low) - (min))) :		\
> +	  (((u32)(value)) >> ((min) - (low)))))
> +
> +#define EFCT_INSERT_NATIVE(min, max, low, high, value)		\
> +	(((((max) - (min)) >= 32) || (((high) - (low)) >= 32)) ?	\
> +	 EFCT_INSERT_NATIVE64(min, max, low, high, value) :	\
> +	 EFCT_INSERT_NATIVE32(min, max, low, high, value))
> +
> +/* Construct bit field portion
> + * Creates the portion of the named bit field that lies within the
> + * range [min,max).
> + */
> +#define EFCT_INSERT_FIELD_NATIVE(min, max, field, value)		\
> +	EFCT_INSERT_NATIVE(min, max, EFCT_LOW_BIT(field),		\
> +			  EFCT_HIGH_BIT(field), value)
> +
> +/* Construct bit field
> + * Creates the portion of the named bit fields that lie within the
> + * range [min,max).
> + */
> +#define EFCT_INSERT_FIELDS_NATIVE(_min, _max,				\
> +				 field1, value1,			\
> +				 field2, value2,			\
> +				 field3, value3,			\
> +				 field4, value4,			\
> +				 field5, value5,			\
> +				 field6, value6,			\
> +				 field7, value7,			\
> +				 field8, value8,			\
> +				 field9, value9,			\
> +				 field10, value10)			\
> +	({typeof(_min) (min) = (_min);					\
> +	  typeof(_max) (max) = (_max);			\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field1, (value1)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field2, (value2)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field3, (value3)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field4, (value4)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field5, (value5)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field6, (value6)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field7, (value7)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field8, (value8)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field9, (value9)) |	\
> +	 EFCT_INSERT_FIELD_NATIVE((min), (max), field10, (value10)); })
> +
> +#define EFCT_INSERT_FIELDS64(...)				\
> +	cpu_to_le64(EFCT_INSERT_FIELDS_NATIVE(__VA_ARGS__))
> +
> +#define EFCT_INSERT_FIELDS32(...)				\
> +	cpu_to_le32(EFCT_INSERT_FIELDS_NATIVE(__VA_ARGS__))
> +
> +#define EFCT_POPULATE_OWORD64(oword, ...) do {				\
> +	(oword).u64[0] = EFCT_INSERT_FIELDS64(0, 63, __VA_ARGS__);	\
> +	(oword).u64[1] = EFCT_INSERT_FIELDS64(64, 127, __VA_ARGS__);	\
> +	} while (0)
> +
> +#define EFCT_POPULATE_QWORD64(qword, ...) (qword).u64[0] = EFCT_INSERT_FIELDS64(0, 63, __VA_ARGS__)
> +
> +#define EFCT_POPULATE_OWORD32(oword, ...) do {				\
> +	(oword).u32[0] = EFCT_INSERT_FIELDS32(0, 31, __VA_ARGS__);	\
> +	(oword).u32[1] = EFCT_INSERT_FIELDS32(32, 63, __VA_ARGS__);	\
> +	(oword).u32[2] = EFCT_INSERT_FIELDS32(64, 95, __VA_ARGS__);	\
> +	(oword).u32[3] = EFCT_INSERT_FIELDS32(96, 127, __VA_ARGS__);	\
> +	} while (0)
> +
> +#define EFCT_POPULATE_QWORD32(qword, ...) do {				\
> +	(qword).u32[0] = EFCT_INSERT_FIELDS32(0, 31, __VA_ARGS__);	\
> +	(qword).u32[1] = EFCT_INSERT_FIELDS32(32, 63, __VA_ARGS__);	\
> +	} while (0)
> +
> +#define EFCT_POPULATE_DWORD(dword, ...) (dword).word32 = EFCT_INSERT_FIELDS32(0, 31, __VA_ARGS__)
> +
> +#if BITS_PER_LONG == 64
> +#define EFCT_POPULATE_OWORD EFCT_POPULATE_OWORD64
> +#define EFCT_POPULATE_QWORD EFCT_POPULATE_QWORD64
> +#else
> +#define EFCT_POPULATE_OWORD EFCT_POPULATE_OWORD32
> +#define EFCT_POPULATE_QWORD EFCT_POPULATE_QWORD32
> +#endif
> +
> +/* Populate an octword field with various numbers of arguments */
> +#define EFCT_POPULATE_OWORD_10 EFCT_POPULATE_OWORD
> +#define EFCT_POPULATE_OWORD_9(oword, ...) \
> +	EFCT_POPULATE_OWORD_10(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_8(oword, ...) \
> +	EFCT_POPULATE_OWORD_9(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_7(oword, ...) \
> +	EFCT_POPULATE_OWORD_8(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_6(oword, ...) \
> +	EFCT_POPULATE_OWORD_7(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_5(oword, ...) \
> +	EFCT_POPULATE_OWORD_6(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_4(oword, ...) \
> +	EFCT_POPULATE_OWORD_5(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_3(oword, ...) \
> +	EFCT_POPULATE_OWORD_4(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_2(oword, ...) \
> +	EFCT_POPULATE_OWORD_3(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_OWORD_1(oword, ...) \
> +	EFCT_POPULATE_OWORD_2(oword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_ZERO_OWORD(oword) \
> +	EFCT_POPULATE_OWORD_1(oword, EFCT_DUMMY_FIELD, 0)
> +#define EFCT_SET_OWORD(oword) \
> +	EFCT_POPULATE_OWORD_4(oword, \
> +			     EFCT_DWORD_0, 0xffffffff, \
> +			     EFCT_DWORD_1, 0xffffffff, \
> +			     EFCT_DWORD_2, 0xffffffff, \
> +			     EFCT_DWORD_3, 0xffffffff)
> +
> +/* Populate a quadword field with various numbers of arguments */
> +#define EFCT_POPULATE_QWORD_10 EFCT_POPULATE_QWORD
> +#define EFCT_POPULATE_QWORD_9(qword, ...) \
> +	EFCT_POPULATE_QWORD_10(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_8(qword, ...) \
> +	EFCT_POPULATE_QWORD_9(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_7(qword, ...) \
> +	EFCT_POPULATE_QWORD_8(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_6(qword, ...) \
> +	EFCT_POPULATE_QWORD_7(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_5(qword, ...) \
> +	EFCT_POPULATE_QWORD_6(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_4(qword, ...) \
> +	EFCT_POPULATE_QWORD_5(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_3(qword, ...) \
> +	EFCT_POPULATE_QWORD_4(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_2(qword, ...) \
> +	EFCT_POPULATE_QWORD_3(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_QWORD_1(qword, ...) \
> +	EFCT_POPULATE_QWORD_2(qword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_ZERO_QWORD(qword) \
> +	EFCT_POPULATE_QWORD_1(qword, EFCT_DUMMY_FIELD, 0)
> +#define EFCT_SET_QWORD(qword) \
> +	EFCT_POPULATE_QWORD_2(qword, \
> +			     EFCT_DWORD_0, 0xffffffff, \
> +			     EFCT_DWORD_1, 0xffffffff)
> +/* Populate a dword field with various numbers of arguments */
> +#define EFCT_POPULATE_DWORD_10 EFCT_POPULATE_DWORD
> +#define EFCT_POPULATE_DWORD_9(dword, ...) \
> +	EFCT_POPULATE_DWORD_10(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_8(dword, ...) \
> +	EFCT_POPULATE_DWORD_9(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_7(dword, ...) \
> +	EFCT_POPULATE_DWORD_8(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_6(dword, ...) \
> +	EFCT_POPULATE_DWORD_7(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_5(dword, ...) \
> +	EFCT_POPULATE_DWORD_6(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_4(dword, ...) \
> +	EFCT_POPULATE_DWORD_5(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_3(dword, ...) \
> +	EFCT_POPULATE_DWORD_4(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_2(dword, ...) \
> +	EFCT_POPULATE_DWORD_3(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_POPULATE_DWORD_1(dword, ...) \
> +	EFCT_POPULATE_DWORD_2(dword, EFCT_DUMMY_FIELD, 0, __VA_ARGS__)
> +#define EFCT_ZERO_DWORD(dword) \
> +	EFCT_POPULATE_DWORD_1(dword, EFCT_DUMMY_FIELD, 0)
> +#define EFCT_SET_DWORD(dword) \
> +	EFCT_POPULATE_DWORD_1(dword, EFCT_DWORD_0, 0xffffffff)
> +
> +/* Modify a named field within an already-populated structure.  Used
> + * for read-modify-write operations.
> + */
> +
> +#define EFCT_INSERT64(min, max, low, high, value)			\
> +	cpu_to_le64(EFCT_INSERT_NATIVE(min, max, low, high, value))
> +
> +#define EFCT_INSERT32(min, max, low, high, value)			\
> +	cpu_to_le32(EFCT_INSERT_NATIVE(min, max, low, high, value))
> +
> +#define EFCT_INPLACE_MASK64(min, max, low, high)				\
> +	EFCT_INSERT64(min, max, low, high, EFCT_MASK64((high) + 1 - (low)))
> +
> +#define EFCT_INPLACE_MASK32(min, max, low, high)				\
> +	EFCT_INSERT32(min, max, low, high, EFCT_MASK32((high) + 1 - (low)))
> +
> +#define EFCT_SET_OWORD64(oword, low, high, value) do {			\
> +	(oword).u64[0] = (((oword).u64[0]				\
> +			   & ~EFCT_INPLACE_MASK64(0,  63, low, high))	\
> +			  | EFCT_INSERT64(0,  63, low, high, value));	\
> +	(oword).u64[1] = (((oword).u64[1]				\
> +			   & ~EFCT_INPLACE_MASK64(64, 127, low, high))	\
> +			  | EFCT_INSERT64(64, 127, low, high, value));	\
> +	} while (0)
> +
> +#define EFCT_SET_QWORD64(qword, low, high, value)			\
> +	(qword).u64[0] = (((qword).u64[0]				\
> +			   & ~EFCT_INPLACE_MASK64(0, 63, low, high))	\
> +			  | EFCT_INSERT64(0, 63, low, high, value))	\
> +
> +#define EFCT_SET_OWORD32(oword, low, high, value) do {			\
> +	(oword).u32[0] = (((oword).u32[0]				\
> +			   & ~EFCT_INPLACE_MASK32(0, 31, low, high))	\
> +			  | EFCT_INSERT32(0, 31, low, high, value));	\
> +	(oword).u32[1] = (((oword).u32[1]				\
> +			   & ~EFCT_INPLACE_MASK32(32, 63, low, high))	\
> +			  | EFCT_INSERT32(32, 63, low, high, value));	\
> +	(oword).u32[2] = (((oword).u32[2]				\
> +			   & ~EFCT_INPLACE_MASK32(64, 95, low, high))	\
> +			  | EFCT_INSERT32(64, 95, low, high, value));	\
> +	(oword).u32[3] = (((oword).u32[3]				\
> +			   & ~EFCT_INPLACE_MASK32(96, 127, low, high))	\
> +			  | EFCT_INSERT32(96, 127, low, high, value));	\
> +	} while (0)
> +
> +#define EFCT_SET_QWORD32(qword, low, high, value) do {			\
> +	(qword).u32[0] = (((qword).u32[0]				\
> +			   & ~EFCT_INPLACE_MASK32(0, 31, low, high))	\
> +			  | EFCT_INSERT32(0, 31, low, high, value));	\
> +	(qword).u32[1] = (((qword).u32[1]				\
> +			   & ~EFCT_INPLACE_MASK32(32, 63, low, high))	\
> +			  | EFCT_INSERT32(32, 63, low, high, value));	\
> +	} while (0)
> +
> +#define EFCT_SET_DWORD32(dword, low, high, value)			\
> +	  (dword).word32 = (((dword).word32				\
> +			   & ~EFCT_INPLACE_MASK32(0, 31, low, high))	\
> +			  | EFCT_INSERT32(0, 31, low, high, value))	\
> +
> +#define EFCT_SET_OWORD_FIELD64(oword, field, value)			\
> +	EFCT_SET_OWORD64(oword, EFCT_LOW_BIT(field),			\
> +			 EFCT_HIGH_BIT(field), value)
> +
> +#define EFCT_SET_QWORD_FIELD64(qword, field, value)			\
> +	EFCT_SET_QWORD64(qword, EFCT_LOW_BIT(field),			\
> +			 EFCT_HIGH_BIT(field), value)
> +
> +#define EFCT_SET_OWORD_FIELD32(oword, field, value)			\
> +	EFCT_SET_OWORD32(oword, EFCT_LOW_BIT(field),			\
> +			 EFCT_HIGH_BIT(field), value)
> +
> +#define EFCT_SET_QWORD_FIELD32(qword, field, value)			\
> +	EFCT_SET_QWORD32(qword, EFCT_LOW_BIT(field),			\
> +			 EFCT_HIGH_BIT(field), value)
> +
> +#define EFCT_SET_DWORD_FIELD(dword, field, value)			\
> +	EFCT_SET_DWORD32(dword, EFCT_LOW_BIT(field),			\
> +			 EFCT_HIGH_BIT(field), value)
> +
> +#if BITS_PER_LONG == 64
> +#define EFCT_SET_OWORD_FIELD EFCT_SET_OWORD_FIELD64
> +#define EFCT_SET_QWORD_FIELD EFCT_SET_QWORD_FIELD64
> +#else
> +#define EFCT_SET_OWORD_FIELD EFCT_SET_OWORD_FIELD32
> +#define EFCT_SET_QWORD_FIELD EFCT_SET_QWORD_FIELD32
> +#endif

EFCT_SET_OWORD_FIELD and EFCT_SET_QWORD_FIELD are appear to
be unused in this patchst. I did not look carefully for other
unused defines.

> +
> +/* Static initialiser */
> +#define EFCT_OWORD32(a, b, c, d)				\
> +	{ .u32 = { cpu_to_le32(a), cpu_to_le32(b),	\
> +		   cpu_to_le32(c), cpu_to_le32(d) } }
> +
> +#endif /* EFCT_BITFIELD_H */

I'm stopping my review here (because I've run out of time for now).