Hi Thomas, Thanks for reviewing.. Kindly find my reply in-line below.. > -----Original Message----- > From: Thomas Monjalon [mailto:tho...@monjalon.net] > Sent: Thursday 21 September 2017 15:56 > To: Mokhtar, Amr <amr.mokh...@intel.com> > Cc: dev@dpdk.org > Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev) > > 25/08/2017 15:46, Amr Mokhtar: > > +/** > > + * Configure a device. > > + * This function must be called on a device before setting up the > > +queues and > > + * starting the device. It can also be called when a device is in the > > +stopped > > + * state. If any device queues have been configured their > > +configuration will be > > + * cleared by a call to this function. > > + * > > + * @param dev_id > > + * The identifier of the device. > > + * @param num_queues > > + * Number of queues to configure on device. > > + * @param conf > > + * The device configuration. If NULL, a default configuration will be > > used. > > + * > > + * @return > > + * - 0 on success > > + * - EINVAL if num_queues is invalid, 0 or greater than maximum > > + * - EBUSY if the identified device has already started > > + * - ENOMEM if unable to allocate memory > > + */ > > +int > > +rte_bbdev_configure(uint8_t dev_id, uint16_t num_queues, > > + const struct rte_bbdev_conf *conf); > > I am not convinced by the "configure all" function in ethdev. > We break the ABI each time we add a new feature to configure. > And it does not really help to have all configurations in one struct. > Would you mind to split the struct rte_bbdev_conf and split the function > accordingly?
There is nothing to split tbh. The only parameter it has is the socket_id. And in fact, it's optional, can be null. The only config we need is num_queues. I don't see in the near future that we may need to add more config params. As a side, in the time of the implementation we were trying to avoid any diversions from the current design ideology of ethdev and cryptodev. Can we leave it for consideration with future releases? > > [...] > > +struct rte_bbdev_info { > > + int socket_id; /**< NUMA socket that device is on */ > > + const char *dev_name; /**< Unique device name */ > > + const struct rte_pci_device *pci_dev; /**< PCI information */ > > + unsigned int num_queues; /**< Number of queues currently configured > */ > > + struct rte_bbdev_conf conf; /**< Current device configuration */ > > + bool started; /**< Set if device is currently started */ > > + struct rte_bbdev_driver_info drv; /**< Info from device driver */ > > +}; > > As Stephen said, PCI must not appear in this API. > Please use the bus abstraction. Done. > > [...] > > +struct __rte_cache_aligned rte_bbdev { > > + rte_bbdev_enqueue_ops_t enqueue_ops; /**< Enqueue function */ > > + rte_bbdev_dequeue_ops_t dequeue_ops; /**< Dequeue function */ > > + const struct rte_bbdev_ops *dev_ops; /**< Functions exported by PMD > */ > > + struct rte_bbdev_data *data; /**< Pointer to device data */ > > + bool attached; /**< If device is currently attached or not */ > > What "attached" means? > I'm afraid you are trying to manage hotplug in the wrong layer. Hotplug is not supported in the current release. > > > + struct rte_device *device; /**< Backing device (HW only) */ > > SW port should have also a rte_device (vdev). Right. Fixed the comment. > > [...] > > +/** Data input and output buffer for Turbo operations */ struct > > +rte_bbdev_op_data { > > Why there is no "turbo" word in the name of this struct? To keep it a generic input/output data descriptor, that suits any type of baseband operation not only for turbo. > > > + struct rte_mbuf *data; > > + /**< First mbuf segment with input/output data. */ > > + uint32_t offset; > > + /**< The starting point for the Turbo input/output, in bytes, from the > > + * start of the data in the data buffer. It must be smaller than > > + * data_len of the mbuf's first segment! > > + */ > > + uint32_t length; > > + /**< For input operations: the length, in bytes, of the source buffer > > + * on which the Turbo encode/decode will be computed. > > + * For output operations: the length, in bytes, of the output buffer > > + * of the Turbo operation. > > + */ > > +}; > > [...] > > +/** Structure specifying a single operation */ struct rte_bbdev_op { > > + enum rte_bbdev_op_type type; /**< Type of this operation */ > > + int status; /**< Status of operation that was performed */ > > + struct rte_mempool *mempool; /**< Mempool which op instance is in > */ > > + void *opaque_data; /**< Opaque pointer for user data */ > > + /** > > + * Anonymous union of operation-type specific parameters. When > allocated > > + * using rte_bbdev_op_pool_create(), space is allocated for the > > + * parameters at the end of each rte_bbdev_op structure, and the > > + * pointers here point to it. > > + */ > > + RTE_STD_C11 > > + union { > > + void *generic; > > + struct rte_bbdev_op_turbo_dec *turbo_dec; > > + struct rte_bbdev_op_turbo_enc *turbo_enc; > > + }; > > +}; > > I am not sure it is a good idea to fit every operations in the same struct > and the > same functions. Due to the fact that our design adopts this idea that a device can support both the encode and decode operations. Then, at the time of PMD registration, the enqueue functions is allocated. This enqueue() function is common for both operations. This fitted operation structure is essential for the driver to decide on the operation. > > [...] > > +/** > > + * Helper macro for logging > > + * > > + * @param level > > + * Log level: EMERG, ALERT, CRIT, ERR, WARNING, NOTICE, INFO, or > DEBUG > > + * @param fmt > > + * The format string, as in printf(3). > > + * @param ... > > + * The variable arguments required by the format string. > > + * > > + * @return > > + * - 0 on success > > + * - Negative on error > > + */ > > +#define rte_bbdev_log(level, fmt, ...) \ > > + RTE_LOG(level, BBDEV, fmt "\n", ##__VA_ARGS__) > > This is the legacy log system. > Please use dynamic log type. Done. > > [...] > > +#ifdef RTE_LIBRTE_BBDEV_DEBUG > > +#define rte_bbdev_log_verbose(fmt, ...) rte_bbdev_log_debug(fmt, > > +##__VA_ARGS__) #else #define rte_bbdev_log_verbose(fmt, ...) #endif > > With the new log functions, you do not need to disable debug log at > compilation > time. Right. > > > +/** > > + * Initialisation params structure that can be used by software > > +based drivers */ struct rte_bbdev_init_params { > > + int socket_id; /**< Base band null device socket */ > > + uint16_t queues_num; /**< Base band null device queues number */ }; > > + > > +/** > > + * Parse generic parameters that could be used for software based devices. > > + * > > + * @param params > > + * Pointer to structure that will hold the parsed parameters. > > + * @param input_args > > + * Pointer to arguments to be parsed. > > + * > > + * @return > > + * - 0 on success > > + * - EINVAL if invalid parameter pointer is provided > > + * - EFAULT if unable to parse provided arguments > > + */ > > +int > > +rte_bbdev_parse_params(struct rte_bbdev_init_params *params, > > + const char *input_args); > > I do not understand the intent of these parameters. > Are they common to every PMDs? > Or could they be moved in software PMDs? That was an old design approach, but this now moved and became the responsibility of the soft PMD. > > End of this first review pass :) Thanks!!