From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shreyansh Jain Subject: Re: [PATCH v1 1/5] rawdev: introduce raw device library support Date: Mon, 8 Jan 2018 19:39:36 +0530 Message-ID: References: <20180102125749.2379-1-shreyansh.jain@nxp.com> <20180102125749.2379-2-shreyansh.jain@nxp.com> <348A99DA5F5B7549AA880327E580B435892EFE92@IRSMSX101.ger.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Cc: Hemant Agrawal , "Xu, Rosen" , "dev@dpdk.org" To: "Trahe, Fiona" Return-path: Received: from NAM02-CY1-obe.outbound.protection.outlook.com (mail-cys01nam02on0051.outbound.protection.outlook.com [104.47.37.51]) by dpdk.org (Postfix) with ESMTP id C41841B1A3 for ; Mon, 8 Jan 2018 14:55:23 +0100 (CET) In-Reply-To: <348A99DA5F5B7549AA880327E580B435892EFE92@IRSMSX101.ger.corp.intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hello Fiona, On Saturday 06 January 2018 07:10 PM, Trahe, Fiona wrote: > Hi Shreyansh, > > This looks like a useful generic device, thanks. Some comments below. Thanks for taking interest and sending your review. I have some responses inline.... (And I have shortened the original email) [...] >> +#include "rte_rawdev.h" >> +#include "rte_rawdev_pmd.h" >> + >> +/* dynamic log identifier */ >> +int librawdev_logtype; >> + >> +/* Maximum rawdevices supported by system. >> + */ >> +#define RTE_MAX_RAWDEVPORTS 10 > [Fiona] Typo in comment above? There's RTE_RAWDEV_MAX_DEVS, RTE_MAX_RAWDEVS and RTE_MAX_RAWDEVPORTS. Are all 3 necessary and what's the relationship between ports and devs? This is a stupid mistake by me. It should be only RTE_RAWDEV_MAX_DEVS. RTE_MAX_RAWDEVS is useless and I will remove RTE_MAX_RAWDEVPORTS. They are intend the same thing - number of max devices supported. > [...] >> + >> +/** >> + * Allocate and set up a raw queue for a raw device. >> + * >> + * @param dev_id >> + * The identifier of the device. >> + * @param queue_id >> + * The index of the raw queue to setup. The value must be in the range >> + * [0, nb_raw_queues - 1] previously supplied to rte_rawdev_configure(). >> + * >> + * @see rte_rawdev_queue_conf_get() >> + * >> + * @return >> + * - 0: Success, raw queue correctly set up. >> + * - <0: raw queue configuration failed >> + */ > [Fiona] cut and paste error above - should be release. Indeed. Thanks for pointing out. I will fix this. > >> +int >> +rte_rawdev_queue_release(uint16_t dev_id, uint16_t queue_id); >> +/** >> + * Get the number of raw queues on a specific raw device >> + * >> + * @param dev_id >> + * Raw device identifier. >> + * @return >> + * - The number of configured raw queues >> + */ >> +uint16_t [...] >> + >> +/** >> + * Allocates a new rawdev slot for an raw device and returns the pointer >> + * to that slot for the driver to use. >> + * >> + * @param name >> + * Unique identifier name for each device >> + * @dev_priv_size >> + * Private data allocated within rte_rawdev object. >> + * @param socket_id >> + * Socket to allocate resources on. >> + * @return >> + * - Slot in the rte_dev_devices array for a new device; >> + */ >> +struct rte_rawdev * >> +rte_rawdev_pmd_allocate(const char *name, size_t dev_private_size, >> + int socket_id); > [Fiona] The driver must allocate a unique name for each device, and the application presumably must search through all devices using dev_count and dev_info_get for each > until it finds a name it expects? But will the application always know the name chosen by the PMD? e.g. driver type xyz might find 10 devices and call them xyz_0, xyz_1, xyz_2, etc > The application wants to look for any or all xyz devices so must know the naming format used by the PMD. > Would it be useful to have 2 parts to the name, a type and an instance, to facilitate finding all devices of a specific type? let me state what I have understood: There are two types of devices: 1. which are scanned through a bus (PCI ...) 2. which are created through vdev (devargs, vdev_init) for those which are scanned through a bus, it is easy to append a "type_" string during device naming. for those which are added through command line, this pattern would have to be choosen by the application/user. further, a rawdevice doesn't have a specific type. So, type would be purely be defined by the driver (scan) or the device name itself (vdev_init). So, eventually the "type_" field would be left out for driver or application to decide. framework (lib/librte_rawdev) would never override/append to it. Is this understanding correct? I will send a v2 shortly with your comments. I will also try and think through your suggestion about name containing "type_" - I do think it is useful but not really sure how would it define semantics between driver and application. - Shreyansh