On 10/25/2023 2:36 PM, shaib...@amazon.com wrote:
> From: Shai Brandes <shaib...@amazon.com>
>
> ENA maintains a HAL that is shared by all supported host drivers.
> Main features introduced to the HAL:
> [1] Reworked the mechanism that queries the performance metrics
> from the device.
> [2] Added support for a new metric that allows monitoring the
> available tracked connections.
> [3] Added support for a new statistic that counts RX drops due
> to insufficient buffers provided by host.
> [4] Added support for Scalable Reliable Datagram (SRD) metrics
> from ENA Express.
> [5] Added support for querying the LLQ entry size recommendation
> from the device.
> [6] Added support for PTP hardware clock (PHC) feature that
> provides enhanced accuracy (Not supported by the driver).
> [7] Added support for new reset reasons for a suspected CPU
> starvation and for completion descriptor inconsistency.
> [8] Aligned all return error code to a common notation.
> [9] Removed an obsolete queue tail pointer update API.
>
> Signed-off-by: Shai Brandes <shaib...@amazon.com>
> Reviewed-by: Amit Bernstein <amitb...@amazon.com>
> ---
> doc/guides/rel_notes/release_23_11.rst | 4 +
> drivers/net/ena/base/ena_com.c | 499 +++++++++++++++---
> drivers/net/ena/base/ena_com.h | 197 ++++++-
> .../net/ena/base/ena_defs/ena_admin_defs.h | 198 ++++++-
> .../net/ena/base/ena_defs/ena_eth_io_defs.h | 18 +-
> drivers/net/ena/base/ena_defs/ena_gen_info.h | 4 +-
> drivers/net/ena/base/ena_defs/ena_regs_defs.h | 12 +
> drivers/net/ena/base/ena_eth_com.c | 45 +-
> drivers/net/ena/base/ena_eth_com.h | 30 +-
> drivers/net/ena/base/ena_plat.h | 8 +-
> drivers/net/ena/base/ena_plat_dpdk.h | 49 +-
> drivers/net/ena/ena_ethdev.c | 16 +-
> 12 files changed, 915 insertions(+), 165 deletions(-)
>
> diff --git a/doc/guides/rel_notes/release_23_11.rst
> b/doc/guides/rel_notes/release_23_11.rst
> index 0a6fc76a9d..e3b0ba58c9 100644
> --- a/doc/guides/rel_notes/release_23_11.rst
> +++ b/doc/guides/rel_notes/release_23_11.rst
> @@ -144,6 +144,10 @@ New Features
>
> * Added support for Network Service Header (NSH) flow matching.
>
> +* **Updated Amazon Elastic Network Adapter ena net driver.**
> +
>
What do you think clarify as:
Updated Amazon ena (Elastic Network Adapter) net driver.
> + * Upgraded ENA HAL to latest version.
> +
>
Updates should be ordered in vendor in the driver group, so can you
please sort it based on 'Amazon'? With current release notes, it makes
before Intel driver updates.
> * **Updated Solarflare net driver.**
>
> * Added support for transfer flow action ``INDIRECT`` with subtype
> ``VXLAN_ENCAP``.
> diff --git a/drivers/net/ena/base/ena_com.c b/drivers/net/ena/base/ena_com.c
> index 5ca36ab6d9..880d047956 100644
> --- a/drivers/net/ena/base/ena_com.c
> +++ b/drivers/net/ena/base/ena_com.c
> @@ -38,6 +38,12 @@
>
> #define ENA_MAX_ADMIN_POLL_US 5000
>
> +/* PHC definitions */
> +#define ENA_PHC_DEFAULT_EXPIRE_TIMEOUT_USEC 20
> +#define ENA_PHC_DEFAULT_BLOCK_TIMEOUT_USEC 1000
> +#define ENA_PHC_TIMESTAMP_ERROR 0xFFFFFFFFFFFFFFFF
> +#define ENA_PHC_REQ_ID_OFFSET 0xDEAD
> +
>
> /*****************************************************************************/
>
> /*****************************************************************************/
>
> /*****************************************************************************/
> @@ -70,7 +76,7 @@ static int ena_com_mem_addr_set(struct ena_com_dev *ena_dev,
> dma_addr_t addr)
> {
> if ((addr & GENMASK_ULL(ena_dev->dma_addr_bits - 1, 0)) != addr) {
> - ena_trc_err(ena_dev, "DMA address has more bits than the device
> supports\n");
> + ena_trc_err(ena_dev, "DMA address has more bits that the device
> supports\n");
>
Original wording looks better to me, 'than' instead of 'that', but we
can get another opinion from a native English speaker.
<...>
> diff --git a/drivers/net/ena/base/ena_plat.h b/drivers/net/ena/base/ena_plat.h
> index 2583823080..a3649e0cb6 100644
> --- a/drivers/net/ena/base/ena_plat.h
> +++ b/drivers/net/ena/base/ena_plat.h
> @@ -14,14 +14,16 @@
> #else
> #include <ena_plat_dpdk.h>
> #endif
> +#elif defined(_WIN32)
> +#include <ena_plat_windows.h>
> #elif defined(__FreeBSD__)
> -#if defined(_KERNEL)
> +#if defined(__KERNEL__)
> #include <ena_plat_fbsd.h>
> #else
> #include <ena_plat_dpdk.h>
> #endif
> -#elif defined(_WIN32)
> -#include <ena_plat_windows.h>
> +#elif defined(__APPLE__)
> +#include <ena_plat_macos.h>
> #else
> #error "Invalid platform"
> #endif
>
As far as I can see only ena_plat_dpdk.h exists, other ena_plat_*.h
files not exists at all in dpdk driver,
would it be possible to strip those lines for the dpdk version of the
ena_plat.h to not confuse about the support.
If this creates additional maintanence burden, OK to continue as it is.
<...>
> @@ -107,6 +110,7 @@ extern int ena_logtype_com;
> #define BITS_PER_LONG_LONG (__SIZEOF_LONG_LONG__ * 8)
> #define U64_C(x) x ## ULL
> #define BIT(nr) (1UL << (nr))
> +#define BIT64(nr) BIT(nr)
>
Can use existing RTE_BIT64 / RTE_BIT32 macros
<...>
> diff --git a/drivers/net/ena/ena_ethdev.c b/drivers/net/ena/ena_ethdev.c
> index 7345e480f8..b764442dbb 100644
> --- a/drivers/net/ena/ena_ethdev.c
> +++ b/drivers/net/ena/ena_ethdev.c
> @@ -1171,7 +1171,6 @@ static int ena_start(struct rte_eth_dev *dev)
> struct ena_adapter *adapter = dev->data->dev_private;
> uint64_t ticks;
> int rc = 0;
> - uint16_t i;
>
> /* Cannot allocate memory in secondary process */
> if (rte_eal_process_type() != RTE_PROC_PRIMARY) {
> @@ -1209,11 +1208,6 @@ static int ena_start(struct rte_eth_dev *dev)
> ++adapter->dev_stats.dev_start;
> adapter->state = ENA_ADAPTER_STATE_RUNNING;
>
> - for (i = 0; i < dev->data->nb_rx_queues; i++)
> - dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
> - for (i = 0; i < dev->data->nb_tx_queues; i++)
> - dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STARTED;
> -
> return 0;
>
> err_rss_init:
> @@ -1229,7 +1223,6 @@ static int ena_stop(struct rte_eth_dev *dev)
> struct ena_com_dev *ena_dev = &adapter->ena_dev;
> struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> struct rte_intr_handle *intr_handle = pci_dev->intr_handle;
> - uint16_t i;
> int rc;
>
> /* Cannot free memory in secondary process */
> @@ -1261,11 +1254,6 @@ static int ena_stop(struct rte_eth_dev *dev)
> adapter->state = ENA_ADAPTER_STATE_STOPPED;
> dev->data->dev_started = 0;
>
> - for (i = 0; i < dev->data->nb_rx_queues; i++)
> - dev->data->rx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
> - for (i = 0; i < dev->data->nb_tx_queues; i++)
> - dev->data->tx_queue_state[i] = RTE_ETH_QUEUE_STATE_STOPPED;
> -
> return 0;
> }
>
Above queue state related changes are coming from other patch in this
release, I guess removing them is a mistake, can you please double check?
