> -----Original Message-----
> From: Ferruh Yigit <ferruh.yi...@amd.com>
> Sent: Thursday, October 26, 2023 12:30 AM
> To: Brandes, Shai <shaib...@amazon.com>
> Cc: dev@dpdk.org; Beider, Ron <rbei...@amazon.com>; Atrash, Wajeeh
> <atrwa...@amazon.com>; Bernstein, Amit <amitb...@amazon.com>
> Subject: RE: [EXTERNAL] [PATCH v2 1/5] net/ena: hal upgrade
>
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
>
>
>
> 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.
[Brandes, Shai] agree
>
>
> > + * 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.
[Brandes, Shai] fixed
>
>
>
> > * **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.
>
> <...>
>
[Brandes, Shai] agree, fixed
> > 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.
>
[Brandes, Shai] removed all, no problem
>
> <...>
>
> > @@ -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
>
> <...>
[Brandes, Shai] changed, thanks for the notice
>
> > 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?
[Brandes, Shai] good catch, indeed was removed accidently
