From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 3261D9A8D for ; Fri, 10 Apr 2015 21:25:36 +0200 (CEST) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga101.jf.intel.com with ESMTP; 10 Apr 2015 12:25:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,558,1422950400"; d="scan'208";a="678425815" Received: from orsmsx106.amr.corp.intel.com ([10.22.225.133]) by orsmga001.jf.intel.com with ESMTP; 10 Apr 2015 12:25:34 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by ORSMSX106.amr.corp.intel.com (10.22.225.133) with Microsoft SMTP Server (TLS) id 14.3.224.2; Fri, 10 Apr 2015 12:25:33 -0700 Received: from fmsmsx113.amr.corp.intel.com ([169.254.13.111]) by fmsmsx115.amr.corp.intel.com ([169.254.4.233]) with mapi id 14.03.0224.002; Fri, 10 Apr 2015 12:25:33 -0700 From: "Wiles, Keith" To: Neil Horman Thread-Topic: [dpdk-dev] [RFC PATCH 2/4] Add the new common device header and C file. Thread-Index: AQHQcrvJoW7ZNZSE5ESdCCHUD/HJvZ1GxEaA Date: Fri, 10 Apr 2015 19:25:32 +0000 Message-ID: References: <1428526720-50221-1-git-send-email-keith.wiles@intel.com> <1428526720-50221-3-git-send-email-keith.wiles@intel.com> <20150409115301.GB26201@hmsreliant.think-freely.org> In-Reply-To: <20150409115301.GB26201@hmsreliant.think-freely.org> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.252.195.97] Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [RFC PATCH 2/4] Add the new common device header and C file. 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: Fri, 10 Apr 2015 19:25:37 -0000 On 4/9/15, 6:53 AM, "Neil Horman" wrote: >On Wed, Apr 08, 2015 at 03:58:38PM -0500, Keith Wiles wrote: >> Move a number of device specific define, structures and functions >> into a generic device base set of files for all device not just >>Ethernet. >>=20 >> Signed-off-by: Keith Wiles >> --- >> lib/librte_eal/common/eal_common_device.c | 185 +++++++ >> lib/librte_eal/common/include/rte_common_device.h | 617 >>++++++++++++++++++++++ >> 2 files changed, 802 insertions(+) >> create mode 100644 lib/librte_eal/common/eal_common_device.c >> create mode 100644 lib/librte_eal/common/include/rte_common_device.h >>=20 >> diff --git a/lib/librte_eal/common/eal_common_device.c >>b/lib/librte_eal/common/eal_common_device.c >> new file mode 100644 >> index 0000000..a9ef925 >> --- /dev/null >> +++ b/lib/librte_eal/common/eal_common_device.c >> @@ -0,0 +1,185 @@ >> +/*- >> + * BSD LICENSE >> + * >> + * Copyright(c) 2010-2014 Intel Corporation. All rights reserved. >> + * Copyright(c) 2014 6WIND S.A. >> + * 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 Intel Corporation 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. >> + */ >> + >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include "rte_common_device.h" >> + >> +void * >> +rte_dev_add_callback(struct rte_dev_rxtx_callback ** cbp, >> + void * fn, void *user_param) >> +{ >> + struct rte_dev_rxtx_callback *cb; >> + >> + cb =3D rte_zmalloc(NULL, sizeof(*cb), 0); >> + >> + if (cb =3D=3D NULL) { >> + rte_errno =3D ENOMEM; >> + return NULL; >> + } >> + >> + cb->fn.vp =3D fn; >> + cb->param =3D user_param; >> + cb->next =3D *cbp; >> + *cbp =3D cb; >> + return cb; >> +} >> + >> +int >> +rte_dev_remove_callback(struct rte_dev_rxtx_callback ** cbp, >> + struct rte_dev_rxtx_callback *user_cb) >> +{ >> + struct rte_dev_rxtx_callback *cb =3D *cbp; >> + struct rte_dev_rxtx_callback *prev_cb; >> + >> + /* Reset head pointer and remove user cb if first in the list. */ >> + if (cb =3D=3D user_cb) { >> + *cbp =3D user_cb->next; >> + return 0; >> + } >> + >> + /* Remove the user cb from the callback list. */ >> + do { >> + prev_cb =3D cb; >> + cb =3D cb->next; >> + >> + if (cb =3D=3D user_cb) { >> + prev_cb->next =3D user_cb->next; >> + return 0; >> + } >> + } while (cb !=3D NULL); >> + >> + /* Callback wasn't found. */ >> + return (-EINVAL); >> +} >> + >> +int >> +rte_dev_callback_register(struct rte_dev_cb_list * cb_list, >> + rte_spinlock_t * lock, >> + enum rte_dev_event_type event, >> + rte_dev_cb_fn cb_fn, void *cb_arg) >> +{ >> + struct rte_dev_callback *cb; >> + >> + rte_spinlock_lock(lock); >> + >> + TAILQ_FOREACH(cb, cb_list, next) { >> + if (cb->cb_fn =3D=3D cb_fn && >> + cb->cb_arg =3D=3D cb_arg && >> + cb->event =3D=3D event) { >> + break; >> + } >> + } >> + >> + /* create a new callback. */ >> + if (cb =3D=3D NULL && (cb =3D rte_zmalloc("INTR_USER_CALLBACK", >> + sizeof(struct rte_dev_callback), 0)) !=3D NULL) { >> + cb->cb_fn =3D cb_fn; >> + cb->cb_arg =3D cb_arg; >> + cb->event =3D event; >> + TAILQ_INSERT_TAIL(cb_list, cb, next); >> + } >> + >> + rte_spinlock_unlock(lock); >> + return ((cb =3D=3D NULL) ? -ENOMEM : 0); >> +} >> + >> +int >> +rte_dev_callback_unregister(struct rte_dev_cb_list * cb_list, >> + rte_spinlock_t * lock, >> + enum rte_dev_event_type event, >> + rte_dev_cb_fn cb_fn, void *cb_arg) >> +{ >> + int ret; >> + struct rte_dev_callback *cb, *next; >> + >> + rte_spinlock_lock(lock); >> + >> + ret =3D 0; >> + for (cb =3D TAILQ_FIRST(cb_list); cb !=3D NULL; cb =3D next) { >> + >> + next =3D TAILQ_NEXT(cb, next); >> + >> + if (cb->cb_fn !=3D cb_fn || cb->event !=3D event || >> + (cb->cb_arg !=3D (void *)-1 && >> + cb->cb_arg !=3D cb_arg)) >> + continue; >> + >> + /* >> + * if this callback is not executing right now, >> + * then remove it. >> + */ >> + if (cb->active =3D=3D 0) { >> + TAILQ_REMOVE(cb_list, cb, next); >> + rte_free(cb); >> + } else { >> + ret =3D -EAGAIN; >> + } >> + } >> + >> + rte_spinlock_unlock(lock); >> + return (ret); >> +} >> + >> +void >> +rte_dev_callback_process(struct rte_dev_cb_list * cb_list, uint8_t >>port_id, >> + enum rte_dev_event_type event, rte_spinlock_t *lock) >> +{ >> + struct rte_dev_callback *cb; >> + struct rte_dev_callback dev_cb; >> + >> + rte_spinlock_lock(lock); >> + TAILQ_FOREACH(cb, cb_list, next) { >> + if (cb->cb_fn =3D=3D NULL || cb->event !=3D event) >> + continue; >> + dev_cb =3D *cb; >> + cb->active =3D 1; >> + rte_spinlock_unlock(lock); >> + dev_cb.cb_fn(port_id, dev_cb.event, dev_cb.cb_arg); >> + rte_spinlock_lock(lock); >> + cb->active =3D 0; >> + } >> + rte_spinlock_unlock(lock); >> +} > > >This is a bit...odd. The rte_eth callbacks had some context because you >knew >when the callback was going to be issued (at the end of an rx and tx >operation). >Here, by making the process routine generic, you're removed that context, >and so >the caller has no guarantee as to when their callbacks will be called. >If its >to be done at the end of the generic transmit and routine functions, that >would >be fine, but I don't see that happening here. No able to follow you here as this routine is called from the rte_ethdev.c _rte_eth_dev_callback_process(). We could remove the _rte_eth_dev_callback_process function and have the rest of the code call this routine instead. No context is lost here and the callbacks are called in the same location as before. >=20 >> diff --git a/lib/librte_eal/common/include/rte_common_device.h >>b/lib/librte_eal/common/include/rte_common_device.h >> new file mode 100644 >> index 0000000..5baefb6 >> --- /dev/null >> +++ b/lib/librte_eal/common/include/rte_common_device.h >> @@ -0,0 +1,617 @@ >> +/*- >> + * BSD LICENSE >> + * >> + * Copyright(c) 2010-2014 Intel Corporation. All rights reserved. >> + * 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 Intel Corporation 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, WHPDF 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_COMMON_DEVICE_H_ >> +#define _RTE_COMMON_DEVICE_H_ >> + >> +/** >> + * @file >> + * >> + * Common Device Helpers in RTE >> + */ >> + >> +#ifdef __cplusplus >> +extern "C" { >> +#endif >> + >> +#include >> +#include >> +#include >> +#include >> + >> +/* Macros for checking for restricting functions to primary instance >>only */ >> +#define PROC_PRIMARY_OR_ERR_RET(retval) do { \ >> + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) { \ >> + PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ >> + return (retval); \ >> + } \ >> +} while(0) >> +#define PROC_PRIMARY_OR_RET() do { \ >> + if (rte_eal_process_type() !=3D RTE_PROC_PRIMARY) { \ >> + PMD_DEBUG_TRACE("Cannot run in secondary processes\n"); \ >> + return; \ >> + } \ >> +} while(0) >> + >> +/* Macros to check for invalid function pointers in dev_ops structure >>*/ >> +#define FUNC_PTR_OR_ERR_RET(func, retval) do { \ >> + if ((func) =3D=3D NULL) { \ >> + PMD_DEBUG_TRACE("Function not supported\n"); \ >> + return (retval); \ >> + } \ >> +} while(0) >> +#define FUNC_PTR_OR_RET(func) do { \ >> + if ((func) =3D=3D NULL) { \ >> + PMD_DEBUG_TRACE("Function not supported\n"); \ >> + return; \ >> + } \ >> +} while(0) >> + >> +enum { >> + DEV_DETACHED =3D 0, >> + DEV_ATTACHED >> +}; >> + >> +/* >> + * The device type >> + */ >> +enum rte_dev_type { >> + RTE_DEV_UNKNOWN, /**< unknown device type */ >> + RTE_DEV_PCI, >> + /**< Physical function and Virtual function of PCI devices */ >> + RTE_DEV_VIRTUAL, /**< non hardware device */ >> + RTE_DEV_MAX /**< max value of this enum */ >> +}; >> + >> +/** >> + * The device event type for interrupt, and maybe others in the future. >> + */ >> +enum rte_dev_event_type { >> + RTE_DEV_EVENT_UNKNOWN, /**< unknown event type */ >> + RTE_DEV_EVENT_INTR_LSC, /**< lsc interrupt event */ >> + RTE_DEV_EVENT_MAX /**< max value of this enum */ >> +}; >> + >> +struct rte_dev_callback; >> + >> +/** @internal Structure to keep track of registered callback */ >> +TAILQ_HEAD(rte_dev_cb_list, rte_dev_callback); >> + >> +struct rte_mbuf; >> + >> +typedef uint16_t (*dev_rx_burst_t)(void *rxq, >> + struct rte_mbuf **rx_pkts, >> + uint16_t nb_pkts); >> +/**< @internal Retrieve input packets from a receive queue of a >>device. */ >> + >> +typedef uint16_t (*dev_tx_burst_t)(void *txq, >> + struct rte_mbuf **tx_pkts, >> + uint16_t nb_pkts); >> +/**< @internal Send output packets on a transmit queue of a device. */ >> + >> +typedef void (*rte_dev_cb_fn)(uint8_t port_id, \ >> + enum rte_dev_event_type event, void *cb_arg); >> +/**< user application callback to be registered for interrupts */ >> + >> +#define RTE_DEV_NAME_MAX_LEN (32) >> + >> +/** >> + * @internal >> + * Common set of members for all devices included at the top of >>'struct rte_xyz_dev' >> + */ >> +#define RTE_COMMON_DEV(_t) >> \ >> + dev_rx_burst_t rx_pkt_burst; /**< Pointer to PMD >>receive function. */ \ >> + dev_tx_burst_t tx_pkt_burst; /**< Pointer to PMD >>transmit function. */ \ >> + struct rte_##_t##dev_data *data; /**< Pointer to device >>data */ \ >> + struct rte_##_t##global *gbl_data; /**< Pointer to device >>global data */ \ >> + const struct _t##driver *driver; /**< Driver for this >>device */ \ >> + struct _t##dev_ops *dev_ops; /**< Functions >>exported by PMD */ \ > >This doesn't make any sense to me. You've made a generic macro that >creates >what is ostensibly supposed to be common code point, but then you include >some >string concatenation so that the common parts are actually unique to a >given >device class. Why would you do that? At that point the common parts >aren't >common by definition, and should go in a structure specific to that >device class The reason for these being specific to the device is the device may have extra members private to the device. I could have added a private pointer to the structure to make then generic to allow the device to have its own private members. > >> + struct rte_pci_device *pci_dev; /**< PCI info. >>supplied by probing */ \ >> + /** User application callback for interrupts if present */ >> \ >> + struct rte_dev_cb_list link_intr_cbs; >> \ >> + /** =20 >> \ >> + * User-supplied functions called from rx_burst to post-process >> \ >> + * received packets before passing them to the user >> \ >> + */ =20 >> \ >> + struct rte_dev_rxtx_callback **post_rx_burst_cbs; >> \ >> + /** =20 >> \ >> + * User-supplied functions called from tx_burst to pre-process >> \ >> + * received packets before passing them to the driver for >>transmission. \ >> + */ =20 >> \ >> + struct rte_dev_rxtx_callback **pre_tx_burst_cbs; >> \ >> + enum rte_dev_type dev_type; /**< Flag indicating the >>device type */ \ >> + uint8_t attached /**< Flag indicating >>the port is attached */ >> + > >I'm also confused here. Looking at this, this is effectively a complete >renaming of the rte_eth_dev device structure. Once again, why not just >leave >rte_eth_dev alone, and develop the crypto device from scratch? If your >intent >is for it to have so much in common with an ethernet device that you can >effectively just rename the existing structure, why not just call it an >ethernet >device? My goal was not to have everything common to a ethdev, but to move items out of the ethdev we can have in common. I feel you are not understanding something and I can not figure out what it is. If I remove the concat part maybe this will get my point across better. > >> +/** >> + * @internal >> + * Common set of members for all devices included at the top of >>'struct rte_xyz_dev_data' >> + */ >> +#define RTE_COMMON_DEV_DATA >> \ >> + char name[RTE_DEV_NAME_MAX_LEN]; /**< Unique identifier name */ >> \ >> + =20 >> \ >> + void **rx_queues; /**< Array of pointers to RX >>queues. */ \ >> + void **tx_queues; /**< Array of pointers to TX >>queues. */ \ >> + uint16_t nb_rx_queues; /**< Number of RX queues. */ >> \ >> + uint16_t nb_tx_queues; /**< Number of TX queues. */ >> \ >> + =20 >> \ >> + uint16_t mtu; /**< Maximum Transmission Unit. */ >> \ >> + uint8_t unit_id; /**< Unit/Port ID for this >>instance */ \ >> + uint8_t _pad0; /* alignment filler */ >> \ >> + =20 >> \ >> + /* 64bit alignment starts here */ >> \ >> + void *dev_private; /**< PMD-specific private data */ >> \ >> + uint64_t rx_mbuf_alloc_failed; /**< RX ring mbuf allocation >>failures. */ \ >> + uint32_t min_rx_buf_size; /**< Common rx buffer size handled >>by all queues */ \ >> + uint32_t _pad1 /**< Align to a 64bit boundary */ >> + >> +/** >> + * @internal >> + * Common set of members for all devices included at the top of >>'struct rte_xyz_dev_info' >> + */ >> +#define RTE_COMMON_DEV_INFO >> \ >> + struct rte_pci_device *pci_dev; /**< Device PCI >>information. */ \ >> + const char *driver_name; /**< Device Driver name. >>*/ \ >> + unsigned int if_index; /**< Index to bound host >>interface, or 0 if none. */ \ >> + /* Use if_indextoname() to translate into an interface name. >>*/ \ >> + uint32_t _pad0 >> + >> +#define port_id unit_id >> + >> +/** >> + * @internal >> + * Common set of members for all devices included at the top of >>'struct rte_xyz_global' >> + */ >> +#define RTE_COMMON_GLOBAL(_t) >> \ >> + struct rte_##_t##dev * devs;/**< Device information array */ >>\ >> + struct rte_##_t##dev_data * data; /**< Device private data */ >>\ >Again, if you're going to make something common, It doesn't make sense to >then >give it a name that is going to be unique to the device class >instantiating the >macro. OK, will try to update the code to be non-specific to a given device. > >> + uint8_t nb_ports; /**< Number of ports/units found */ >> \ >> + uint8_t max_ports; /**< Max number of ports or units */ >> \ >> + uint16_t dflt_mtu; /**< Default MTU if needed by device >>*/\ >> + uint16_t dev_size; /**< Internal size of device struct >>*/ \ >> + uint16_t data_size; /**< Internal size of data structure >>*/\ >> + const char * mz_dev_data /**< String constant for device data >>*/ >> + >> +/** >> + * @internal >> + * Common overlay type structures with all devices. >> + */ >> +struct rte_dev_info { >> + RTE_COMMON_DEV_INFO; >> +}; >> + >> +/** >> + * @internal >> + * The generic data structure associated with each device. >> + * >> + * Pointers to burst-oriented packet receive and transmit functions are >> + * located at the beginning of the structure, along with the pointer to >> + * where all the data elements for the particular device are stored in >>shared >> + * memory. This split allows the function pointer and driver data to >>be per- >> + * process, while the actual configuration data for the device is >>shared. >> + */ >> +struct rte_dev { >> + RTE_COMMON_DEV(); >> +}; >> + >> +/** >> + * @internal >> + * The data part, with no function pointers, associated with each >>device. >> + * >> + * This structure is safe to place in shared memory to be common among >>different >> + * processes in a multi-process configuration. >> + */ >> +struct rte_dev_data { >> + RTE_COMMON_DEV_DATA; >> +}; >> + >> +/** >> + * @internal >> + * This structure is attempting to mirror the rte_xyz_global >>structures. >> + * >> + * Be careful as this structure is over layered on top of the xyzdev >>structures. >> + */ >> +struct rte_dev_global { >> + RTE_COMMON_GLOBAL(); >> +}; >> + >I don't understand, you created macros that allow for naming specifics, >then >don't use it? If your intent was for that data to be generically named >(which >would make sense), then you should remove the parameter from the macro so >that >can't change. > >> +/** >> + * Get the rte_pkt_dev structure device pointer for the device. >> + * >> + * @param gbl pointer to device specific global structure. >> + * @param port_id Port ID value to select the device structure. >> + * >> + * @return >> + * - The rte_pkt_dev structure pointer for the given port ID. >> + */ >> +static inline void * >> +rte_get_dev(void * _gbl, uint8_t port_id) >> +{ >> + struct rte_dev_global * gbl =3D (struct rte_dev_global *)_gbl; >> + >> + return (void *)((uint8_t *)gbl->devs + (port_id * gbl->dev_size)); >> +} >> + >How will this be helpful? the caller will need to know what type of >struct to >cast this to (since it can be named different for different dev classes). >doesn't that sort of thing undo the notion of commonality? Not really the point was to have a routine able to return the correct device structure pointer and the developer should understand which global structure he is passing into the routine and what type is being returned. > >Neil >