From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f66.google.com (mail-wm0-f66.google.com [74.125.82.66]) by dpdk.org (Postfix) with ESMTP id 52D4E54AF for ; Thu, 17 Nov 2016 12:19:03 +0100 (CET) Received: by mail-wm0-f66.google.com with SMTP id m203so20168405wma.3 for ; Thu, 17 Nov 2016 03:19:03 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=xpKCzWYjyqBz97seGq8VqQ7dhTjGe4ywRs+VDd+v+3w=; b=eDyZ3aaOy9nLb3tyzX//25pae/m+/iDcwTCsZ38GOm+t1vF9peoh6L+WQqEJf+IbJ/ mV6pdZeNBX2YJmpKeP8lFCsSkM95xXQslSChsgA/xZKQRN7G6TMkS7RPhxxmXVU9tyk6 RhVAMknfY14w32+c/OzxejbXfajCAEh9mHlaOyRN/mG80vW9F9nLTKcYVot1aeKs5yUb pC/utsv4o+zpIcOWo0JpqihGMA1i7jHaVpclMtxxslcNHaoU5wDJcLRWF7KGDSXGI5kz V9UbCe2rbi2O3gwCEyOB7W0JEPsLPa3xER9QoAKQdpDxMX7VMpsU/2rwA9tDgYK7U+KA 2trg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=xpKCzWYjyqBz97seGq8VqQ7dhTjGe4ywRs+VDd+v+3w=; b=lVlpBkhWuRUl/EiGLLa8HTDEJdjHvRPtnWOmBoRbqBIOEZU/50wEV5s6TJ1CHgmjbI jIaUVgSS0RjHvG9RVzk84/tjZ3QLPGrw8ieGkAYhqXj5yCsBQ9RtRgk2pplgKHwedDKL aMdSc6WfX+8zLN3IRX8cjxa+ZbpQAuyeMrPcNoDCTyir/nIT20uA1KFZ2kQg9xy6jW93 pjA9adQWEjoT5bveb4AH7+/+pbuvHK5nASLlsEnzwq5wZUswGNLOkh7nhEoKrG/jAwGl Y2Jlrx0fep1KIY5iX/Oq0/IMu3VpfBzz0ADRStR85/kURwgRCqSLIxylbvHM+A1f+rCB IW1g== X-Gm-Message-State: ABUngvc8H4VmnsH05AOp/qsCKNDrIi2lZPJquoyt+KI+PnzK2kZcnDA4QbmISy2hh9DWWo+5rvB51Z8zCXczng== X-Received: by 10.28.66.194 with SMTP id k63mr17038979wmi.140.1479381542672; Thu, 17 Nov 2016 03:19:02 -0800 (PST) MIME-Version: 1.0 Sender: jblunck@gmail.com Received: by 10.28.191.8 with HTTP; Thu, 17 Nov 2016 03:19:01 -0800 (PST) In-Reply-To: <1479360605-20558-3-git-send-email-shreyansh.jain@nxp.com> References: <1479360605-20558-1-git-send-email-shreyansh.jain@nxp.com> <1479360605-20558-3-git-send-email-shreyansh.jain@nxp.com> From: Jan Blunck Date: Thu, 17 Nov 2016 12:19:01 +0100 X-Google-Sender-Auth: HCkkrDb3Uys6cuWvOoaXVHw5lUo Message-ID: To: Shreyansh Jain Cc: David Marchand , dev@dpdk.org Content-Type: text/plain; charset=UTF-8 Subject: Re: [dpdk-dev] [RFC PATCH 2/6] eal: introduce bus-device-driver structure X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 17 Nov 2016 11:19:03 -0000 On Thu, Nov 17, 2016 at 6:30 AM, Shreyansh Jain wrote: > A device is connected to a bus and services by a driver associated with > the bus. It is responsibility of the bus to identify the devices (scan) > and then assign each device to a matching driver. > > A PMD would allocate a rte_xxx_driver and rte_xxx_device. > rte_xxx_driver has rte_driver and rte_bus embedded. Similarly, rte_xxx_device > has rte_device and rte_bus embedded. I don't think so: the rte_xxx_device embeds the generic rte_device and references a the rte_bus that it is attached to. > When a ethernet or crypto device (rte_eth_dev, rte_cryptodev) is allocated, > it contains a reference of rte_device and rte_driver. > Each ethernet device implementation would use container_of for finding the > enclosing structure of rte_xxx_*. > > +-------------------+ > +--------------+ |rte_pci_device | > |rte_eth_dev | |+-----------------+| > |+------------+| .-------->rte_device || > ||rte_device*-----' |+-----------------+| > |+------------+| ||rte_bus || > | | |+-----------------+| > / / +-------------------+ > > Signed-off-by: Shreyansh Jain > --- > lib/librte_eal/common/include/rte_bus.h | 243 ++++++++++++++++++++++++++++++++ > lib/librte_eal/common/include/rte_dev.h | 36 ++--- > 2 files changed, 261 insertions(+), 18 deletions(-) > create mode 100644 lib/librte_eal/common/include/rte_bus.h > > diff --git a/lib/librte_eal/common/include/rte_bus.h b/lib/librte_eal/common/include/rte_bus.h > new file mode 100644 > index 0000000..dc3aeb8 > --- /dev/null > +++ b/lib/librte_eal/common/include/rte_bus.h > @@ -0,0 +1,243 @@ > +/*- > + * BSD LICENSE > + * > + * Copyright(c) 2016 NXP > + * All rights reserved. > + * > + * Redistribution and use in source and binary forms, with or without > + * modification, are permitted provided that the following conditions > + * are met: > + * > + * * Redistributions of source code must retain the above copyright > + * notice, this list of conditions and the following disclaimer. > + * * Redistributions in binary form must reproduce the above copyright > + * notice, this list of conditions and the following disclaimer in > + * the documentation and/or other materials provided with the > + * distribution. > + * * Neither the name of NXP nor the names of its > + * contributors may be used to endorse or promote products derived > + * from this software without specific prior written permission. > + * > + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS > + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT > + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR > + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT > + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, > + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT > + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, > + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY > + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT > + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE > + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. > + */ > + > +#ifndef _RTE_BUS_H_ > +#define _RTE_BUS_H_ > + > +/** > + * @file > + * > + * RTE PMD Bus Abstraction interfaces > + * > + * This file exposes APIs and Interfaces for Bus Abstraction over the devices > + * drivers in EAL. > + */ > + > +#ifdef __cplusplus > +extern "C" { > +#endif > + > +#include > +#include > + > +#include > +#include > + > + > +/** Double linked list of buses */ > +TAILQ_HEAD(rte_bus_list, rte_bus); > + > +/** > + * Bus specific scan for devices attached on the bus. > + * For each bus object, the scan would be reponsible for finding devices and > + * adding them to its private device list. > + * > + * Successful detection of a device results in rte_device object which is > + * embedded within the respective device type (rte_pci_device, for example). > + * Thereafter, PCI specific bus would need to perform > + * container_of(rte_pci_device) to obtain PCI device object. > + * > + * Scan failure of a bus is not treated as exit criteria for application. Scan > + * for all other buses would still continue. > + * > + * @param void > + * @return > + * 0 for successful scan > + * !0 (<0) for unsuccessful scan with error value > + */ > +typedef int (* bus_scan_t)(void); > + > +/** > + * Bus specific match for devices and drivers which can service them. > + * For each scanned device, during probe the match would link the devices with > + * drivers which can service the device. > + * > + * It is the work of each bus handler to obtain the specific device object > + * using container_of (or typecasting, as a less preferred way). > + * > + * @param drv > + * Driver object attached to the bus > + * @param dev > + * Device object which is being probed. > + * @return > + * 0 for successful match > + * !0 for unsuccessful match > + */ > +typedef int (* bus_match_t)(struct rte_driver *drv, struct rte_device *dev); Do you think this should do match & probe? I believe it is better to separate this into two functions to match() and probe(). The result of matching tells if the driver would want to claim the device in general. But probe()ing the device should only happen if the device isn't claimed by a driver yet and is not blacklisted. > + > +/** > + * Dump the devices on the bus. > + * Each bus type can define its own definition of information to dump. > + * > + * @param bus > + * Handle for bus, device from which are to be dumped. > + * @param f > + * Handle to output device or file. > + * @return void > + */ > +typedef void (* bus_dump_t)(struct rte_bus *bus, FILE *f); > + > +/** > + * Search for a specific device in device list of the bus > + * This would rely on the bus specific addressing. Each implementation would > + * extract its specific device type and perform address compare. > + * > + * @param dev > + * device handle to search for. > + * @return > + * rte_device handle for matched device, or NULL > + */ > +typedef struct rte_device * (* bus_device_get_t)(struct rte_device *dev); >>From my experience it is better to delegate this to a helper: int bus_for_each_dev(struct rte_bus *bus, struct rte_device *start, int (*match)(struct rte_device *dev, void *data), void *data); Based on that you can easily implement all kinds of functions like bus_find_dev(), bus_find_dev_by_name(), bus_dump(), ... > + > +struct rte_bus { > + TAILQ_ENTRY(rte_bus) next; /**< Next bus object in linked list */ > + struct rte_driver_list driver_list; /**< List of all drivers of bus */ TAILQ_HEAD? > + struct rte_device_list device_list; /**< List of all devices on bus */ TAILQ_HEAD? > + const char *name; /**< Name of the bus */ > + /* Mandatory hooks */ > + bus_scan_t *scan; /**< Hook for scanning for devices */ > + bus_match_t *match; /**< Hook for matching device & driver */ > + /* Optional hooks */ > + bus_dump_t *dump_dev; /**< Hook for dumping devices on bus */ > + bus_device_get_t *find_dev; /**< Search for a device on bus */ > +}; > + > +/** @internal > + * Add a device to a bus. > + * > + * @param bus > + * Bus on which device is to be added > + * @param dev > + * Device handle > + * @return > + * None > + */ > +void > +rte_eal_bus_add_device(struct rte_bus *bus, struct rte_device *dev); Why do we need this? From my understanding the rte_bus->scan() is adding the devices to the rte_bus->device_list. > +/** @internal > + * Remove a device from its bus. > + * > + * @param dev > + * Device handle to remove > + * @return > + * None > + */ > +void > +rte_eal_bus_remove_device(struct rte_device *dev); > + > +/** @internal > + * Associate a driver with a bus. > + * > + * @param bus > + * Bus on which driver is to be added > + * @param dev > + * Driver handle > + * @return > + * None > + */ > +void > +rte_eal_bus_add_driver(struct rte_bus *bus, struct rte_driver *drv); > + What happens if a driver is added at runtime to a bus? Does that immediately trigger a match & probe of unclaimed devices? > +/** @internal > + * Disassociate a driver from its bus. > + * > + * @param dev > + * Driver handle to remove > + * @return > + * None > + */ > +void > +rte_eal_bus_remove_driver(struct rte_driver *drv); > + > +/** > + * Register a Bus handler. > + * > + * @param driver > + * A pointer to a rte_bus structure describing the bus > + * to be registered. > + */ > +void rte_eal_bus_register(struct rte_bus *bus); > + > +/** > + * Unregister a Bus handler. > + * > + * @param driver > + * A pointer to a rte_bus structure describing the bus > + * to be unregistered. > + */ > +void rte_eal_bus_unregister(struct rte_bus *bus); > + > +/** > + * Obtain handle for bus given its name. > + * > + * @param bus_name > + * Name of the bus handle to search > + * @return > + * Pointer to Bus object if name matches any registered bus object > + * NULL, if no matching bus found > + */ > +struct rte_bus * rte_eal_get_bus(const char *bus_name); > + > +/** > + * Register a device driver. > + * > + * @param driver > + * A pointer to a rte_dev structure describing the driver > + * to be registered. > + */ > +void rte_eal_driver_register(struct rte_driver *driver); > + > +/** > + * Unregister a device driver. > + * > + * @param driver > + * A pointer to a rte_dev structure describing the driver > + * to be unregistered. > + */ > +void rte_eal_driver_unregister(struct rte_driver *driver); > + > +/** Helper for Bus registration */ > +#define RTE_PMD_REGISTER_BUS(nm, bus) \ > +RTE_INIT(businitfn_ ##nm); \ > +static void businitfn_ ##nm(void) \ > +{\ > + (bus).name = RTE_STR(nm);\ > + rte_eal_bus_register(&bus); \ > +} > + > +#ifdef __cplusplus > +} > +#endif > + > +#endif /* _RTE_BUS_H */ > diff --git a/lib/librte_eal/common/include/rte_dev.h b/lib/librte_eal/common/include/rte_dev.h > index 8840380..b08bab5 100644 > --- a/lib/librte_eal/common/include/rte_dev.h > +++ b/lib/librte_eal/common/include/rte_dev.h > @@ -116,12 +116,14 @@ TAILQ_HEAD(rte_device_list, rte_device); > > /* Forward declaration */ > struct rte_driver; > +struct rte_bus; > > /** > * A structure describing a generic device. > */ > struct rte_device { > TAILQ_ENTRY(rte_device) next; /**< Next device */ > + struct rte_bus *bus; /**< Bus on which device is placed */ > struct rte_driver *driver; /**< Associated driver */ > int numa_node; /**< NUMA node connection */ > struct rte_devargs *devargs; /**< Device user arguments */ > @@ -144,31 +146,29 @@ void rte_eal_device_insert(struct rte_device *dev); > void rte_eal_device_remove(struct rte_device *dev); > > /** > - * A structure describing a device driver. > + * @internal > + * TODO > */ > -struct rte_driver { > - TAILQ_ENTRY(rte_driver) next; /**< Next in list. */ > - const char *name; /**< Driver name. */ > - const char *alias; /**< Driver alias. */ > -}; > +typedef int (*driver_init_t)(struct rte_device *eth_dev); > > /** > - * Register a device driver. > - * > - * @param driver > - * A pointer to a rte_dev structure describing the driver > - * to be registered. > + * @internal > + * TODO > */ > -void rte_eal_driver_register(struct rte_driver *driver); > +typedef int (*driver_uninit_t)(struct rte_device *eth_dev); > > /** > - * Unregister a device driver. > - * > - * @param driver > - * A pointer to a rte_dev structure describing the driver > - * to be unregistered. > + * A structure describing a device driver. > */ > -void rte_eal_driver_unregister(struct rte_driver *driver); > +struct rte_driver { > + TAILQ_ENTRY(rte_driver) next; /**< Next in list. */ > + struct rte_bus *bus; /**< Bus which drivers services */ I think this should be TAILQ_ENTRY instead. > + const char *name; /**< Driver name. */ > + const char *alias; /**< Driver alias. */ > + driver_init_t *init; /**< Driver initialization */ > + driver_uninit_t *uninit; /**< Driver uninitialization */ Shouldn't this be probe() and remove()? > + unsigned int dev_private_size; /**< Size of device private data ??*/ I don't think that dev_private_size is really required at this level. First of all this is related to the rte_eth_dev structure and therefore it really depends on the driver_init_t (aka probe()) if it is actually allocating an rte_eth_dev or not. Anyway that is up to the drivers probe() function. > +}; > > /** > * Initalize all the registered drivers in this process > -- > 2.7.4 >