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? [...] > +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. [...] > +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. > + struct rte_device *device; /**< Backing device (HW only) */ SW port should have also a rte_device (vdev). [...] > +/** 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? > + 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. [...] > +/** > + * 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. [...] > +#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. > +/** > + * 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? End of this first review pass :)