From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from out1-smtp.messagingengine.com (out1-smtp.messagingengine.com [66.111.4.25]) by dpdk.org (Postfix) with ESMTP id E96611B19D for ; Thu, 21 Sep 2017 16:56:07 +0200 (CEST) Received: from compute1.internal (compute1.nyi.internal [10.202.2.41]) by mailout.nyi.internal (Postfix) with ESMTP id D273A20EA9; Thu, 21 Sep 2017 10:56:06 -0400 (EDT) Received: from frontend1 ([10.202.2.160]) by compute1.internal (MEProxy); Thu, 21 Sep 2017 10:56:06 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:content-transfer-encoding:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-me-sender :x-me-sender:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=jMsQeF7QKtxyikE 52DoVfpKF+AOxvVXJvUJA+HP4uHY=; b=BRf+1zy8X2HPKTHAwSYW9GmIpcGIDpG CwMxUnEjKETwERkKMEwXDdf2T8ACtAIb0icINNCusTfY/5S14alPkPr6qDRypHi2 JVYtxIXfPwpMydAf70RwBPoEUpbzMXSYmi/rJTIqSfBMM+ezXY/CRz2I0QL0XjN7 7KWQd8cTmN2A= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-transfer-encoding:content-type :date:from:in-reply-to:message-id:mime-version:references :subject:to:x-me-sender:x-me-sender:x-sasl-enc:x-sasl-enc; s= fm1; bh=jMsQeF7QKtxyikE52DoVfpKF+AOxvVXJvUJA+HP4uHY=; b=bI5zmDrb jPh6OxqMV+Omcy8PGkAnJNIxyfPxb4e33QXCrQibjaS/Rki8x7gnUSuUftN9GCjr zcPrbBVyUYTSNdRppQxj89x20dV3cb/S4OgAT42yIAYd8xvWZkartfFOAk6btBO8 W+EZVhyEMoRewYKf/6lGBlIzPLUqvTD2tMJkRrXedI+Ngb77FvE8+34WIiw9H//e 6CQmoHDLws6QrDlJkphIrljvNl4Si/VsdE2dw4pWOWac8Pz9j8xxYcOF0FGT4M2P GEnjdcG5F6n4FdLbNhwYbXOkky+ihb/o+adils2PLZpDSdUYjclqa9ht6a4itkHy 2Z9OBkd+QhuENw== X-ME-Sender: X-Sasl-enc: 25ar6T09NGgUBGPAOWFRtOifp/UPTH1umhgxgaFgVlJj 1506005766 Received: from xps.localnet (184.203.134.77.rev.sfr.net [77.134.203.184]) by mail.messagingengine.com (Postfix) with ESMTPA id 78C0C7E675; Thu, 21 Sep 2017 10:56:06 -0400 (EDT) From: Thomas Monjalon To: Amr Mokhtar Cc: dev@dpdk.org Date: Thu, 21 Sep 2017 16:56:05 +0200 Message-ID: <5517187.euiR5LjUcT@xps> In-Reply-To: <1503668796-65832-1-git-send-email-amr.mokhtar@intel.com> References: <1503668796-65832-1-git-send-email-amr.mokhtar@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Subject: Re: [dpdk-dev] [RFC] Wireless Base Band Device (bbdev) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 21 Sep 2017 14:56:08 -0000 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 :)