RE: [PATCH v4 2/7] bbdev: add device status info

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Subject: Re: [PATCH v4 2/7] bbdev: add device status info
> 
> 
> On 7/5/22 5:23 PM, Nicolas Chautru wrote:
> > Added device status information, so that the PMD can expose
> > information related to the underlying accelerator device status.
> > Minor order change in structure to fit into padding hole.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   drivers/baseband/acc100/rte_acc100_pmd.c           |  1 +
> >   drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c |  1 +
> >   drivers/baseband/fpga_lte_fec/fpga_lte_fec.c       |  1 +
> >   drivers/baseband/la12xx/bbdev_la12xx.c             |  1 +
> >   drivers/baseband/null/bbdev_null.c                 |  1 +
> >   drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
> >   lib/bbdev/rte_bbdev.c                              | 24 +++++++++++++++
> >   lib/bbdev/rte_bbdev.h                              | 35 ++++++++++++++++++++--
> >   lib/bbdev/version.map                              |  6 ++++
> >   9 files changed, 69 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> > b/drivers/baseband/acc100/rte_acc100_pmd.c
> > index de7e4bc..17ba798 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> > @@ -1060,6 +1060,7 @@
> >
> >   	/* Read and save the populated config from ACC100 registers */
> >   	fetch_acc100_config(dev);
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	/* This isn't ideal because it reports the maximum number of queues
> but
> >   	 * does not provide info on how many can be uplink/downlink or
> > different diff --git
> > a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > index 82ae6ba..57b12af 100644
> > --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > @@ -369,6 +369,7 @@
> >   	dev_info->capabilities = bbdev_capabilities;
> >   	dev_info->cpu_flag_reqs = NULL;
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	/* Calculates number of queues assigned to device */
> >   	dev_info->max_num_queues = 0;
> > diff --git a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > index 21d3529..2a330c4 100644
> > --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue {
> >   	dev_info->capabilities = bbdev_capabilities;
> >   	dev_info->cpu_flag_reqs = NULL;
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	/* Calculates number of queues assigned to device */
> >   	dev_info->max_num_queues = 0;
> > diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c
> > b/drivers/baseband/la12xx/bbdev_la12xx.c
> > index 4d1bd16..c1f88c6 100644
> > --- a/drivers/baseband/la12xx/bbdev_la12xx.c
> > +++ b/drivers/baseband/la12xx/bbdev_la12xx.c
> > @@ -100,6 +100,7 @@ struct bbdev_la12xx_params {
> >   	dev_info->capabilities = bbdev_capabilities;
> >   	dev_info->cpu_flag_reqs = NULL;
> >   	dev_info->min_alignment = 64;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	rte_bbdev_log_debug("got device info from %u", dev->data->dev_id);
> >   }
> > diff --git a/drivers/baseband/null/bbdev_null.c
> > b/drivers/baseband/null/bbdev_null.c
> > index 248e129..94a1976 100644
> > --- a/drivers/baseband/null/bbdev_null.c
> > +++ b/drivers/baseband/null/bbdev_null.c
> > @@ -82,6 +82,7 @@ struct bbdev_queue {
> >   	 * here for code completeness.
> >   	 */
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	rte_bbdev_log_debug("got device info from %u", dev->data->dev_id);
> >   }
> > diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > index af7bc41..dbc5524 100644
> > --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > @@ -254,6 +254,7 @@ struct turbo_sw_queue {
> >   	dev_info->min_alignment = 64;
> >   	dev_info->harq_buffer_size = 0;
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	rte_bbdev_log_debug("got device info from %u\n", dev->data-
> >dev_id);
> >   }
> > diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
> > 22bd894..555bda9 100644
> > --- a/lib/bbdev/rte_bbdev.c
> > +++ b/lib/bbdev/rte_bbdev.c
> > @@ -25,6 +25,8 @@
> >
> >   /* Number of supported operation types */
> >   #define BBDEV_OP_TYPE_COUNT 5
> > +/* Number of supported device status */ #define
> > +BBDEV_DEV_STATUS_COUNT 9
> >
> >   /* BBDev library logging ID */
> >   RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE); @@ -1132,3
> +1134,25
> > @@ struct rte_mempool *
> >   	rte_bbdev_log(ERR, "Invalid operation type");
> >   	return NULL;
> >   }
> > +
> > +const char *
> > +rte_bbdev_device_status_str(enum rte_bbdev_device_status status) {
> > +	static const char * const dev_sta_string[] = {
> > +		"RTE_BBDEV_DEV_NOSTATUS",
> > +		"RTE_BBDEV_DEV_NOT_SUPPORTED",
> > +		"RTE_BBDEV_DEV_RESET",
> > +		"RTE_BBDEV_DEV_CONFIGURED",
> > +		"RTE_BBDEV_DEV_ACTIVE",
> > +		"RTE_BBDEV_DEV_FATAL_ERR",
> > +		"RTE_BBDEV_DEV_RESTART_REQ",
> > +		"RTE_BBDEV_DEV_RECONFIG_REQ",
> > +		"RTE_BBDEV_DEV_CORRECT_ERR",
> > +	};
> > +
> > +	if (status < BBDEV_DEV_STATUS_COUNT)
> > +		return dev_sta_string[status];
> > +
> > +	rte_bbdev_log(ERR, "Invalid device status");
> > +	return NULL;
> > +}
> > diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
> > b88c881..9b1ffa4 100644
> > --- a/lib/bbdev/rte_bbdev.h
> > +++ b/lib/bbdev/rte_bbdev.h
> > @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf {
> >   int
> >   rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
> >
> > +/**
> > + * Flags indicate the status of the device  */ enum
> > +rte_bbdev_device_status {
> > +	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being reported */
> > +	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is not
> supported on the PMD */
> If this was 0, you may not need to explicitly set.

This helps to have the lack of status being equivalent to a cleared register.

> > +	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-configured
> state */
> > +	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is configured and
> ready to use */
> > +	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured and VF is
> being used */
> > +	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal
> uncorrectable error */
> > +	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires application to
> restart */
> > +	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires application
> to reconfigure queues */
> > +	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a correctable
> error event happened */
> Last patch was padded, do something consistent here.

We only pad if we have to. Here there is no array whose size would be dimensioned by the size of that enum.

> > +};
> > +
> >   /** Device statistics. */
> >   struct rte_bbdev_stats {
> >   	uint64_t enqueued_count;  /**< Count of all operations enqueued */
> > @@ -285,12 +300,14 @@ struct rte_bbdev_driver_info {
> >   	/** Set if device supports per-queue interrupts */
> >   	bool queue_intr_supported;
> >   	/** Minimum alignment of buffers, in bytes */
> > -	uint16_t min_alignment;
> > -	/** HARQ memory available in kB */
> > +	/** Device Status */
> > +	enum rte_bbdev_device_status device_status;
> 
> New elements should be added to the end to improve backward compatibility.

Same comment in different patch. I would like to know if there is a real recommendation from DPDK on this. I have heard opposite view as well.
In that very case we are breaking the ABI in that new serie for 22.11 (sizes and offsets are changing). 

> 
> Tom
> 
> >   	uint32_t harq_buffer_size;
> >   	/** Byte endianness (RTE_BIG_ENDIAN/RTE_LITTLE_ENDIAN)
> supported
> >   	 *  for input/output data
> >   	 */
> > +	uint16_t min_alignment;
> > +	/** HARQ memory available in kB */
> >   	uint8_t data_endianness;
> >   	/** Default queue configuration used if none is supplied  */
> >   	struct rte_bbdev_queue_conf default_queue_conf; @@ -827,6
> +844,20
> > @@ typedef void (*rte_bbdev_cb_fn)(uint16_t dev_id,
> >   rte_bbdev_queue_intr_ctl(uint16_t dev_id, uint16_t queue_id, int epfd, int
> op,
> >   		void *data);
> >
> > +/**
> > + * Converts device status from enum to string
> > + *
> > + * @param status
> > + *   Device status as enum
> > + *
> > + * @returns
> > + *   Operation type as string or NULL if op_type is invalid
> > + *
> > + */
> > +__rte_experimental
> > +const char*
> > +rte_bbdev_device_status_str(enum rte_bbdev_device_status status);
> > +
> >   #ifdef __cplusplus
> >   }
> >   #endif
> > diff --git a/lib/bbdev/version.map b/lib/bbdev/version.map index
> > cce3f3c..9ac3643 100644
> > --- a/lib/bbdev/version.map
> > +++ b/lib/bbdev/version.map
> > @@ -39,3 +39,9 @@ DPDK_22 {
> >
> >   	local: *;
> >   };
> > +
> > +EXPERIMENTAL {
> > +	global:
> > +
> > +	rte_bbdev_device_status_str;
> > +};