From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 30165A04C8; Fri, 18 Sep 2020 15:43:24 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 0775B1D943; Fri, 18 Sep 2020 15:43:24 +0200 (CEST) Received: from qrelay45.mxroute.com (qrelay45.mxroute.com [172.82.139.45]) by dpdk.org (Postfix) with ESMTP id A64461D8E9 for ; Fri, 18 Sep 2020 15:43:22 +0200 (CEST) Received: from filter003.mxroute.com ([168.235.111.26] 168-235-111-26.cloud.ramnode.com) (Authenticated sender: mN4UYu2MZsgR) by qrelay45.mxroute.com (ZoneMTA) with ESMTPA id 174a174817f000b8c7.001 for ; Fri, 18 Sep 2020 13:43:21 +0000 X-Zone-Loop: 120a52fd9179a93f13242d8134f2a254738c0187d887 X-Originating-IP: [168.235.111.26] Received: from echo.mxrouting.net (echo.mxrouting.net [116.202.222.109]) by filter003.mxroute.com (Postfix) with ESMTPS id 435C860050; Fri, 18 Sep 2020 13:43:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=ashroe.eu; s=x; h=Content-Transfer-Encoding:Content-Type:In-Reply-To:MIME-Version:Date: Message-ID:From:References:Cc:To:Subject:Sender:Reply-To:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=949QJ3wzk0whhKR1QyGVwlEU86Mn+5AKCZJICf54NOA=; b=YVbXPce+QWUWvVtgP2sKd9WY35 KOs5uyn4BptZ3hSNjRfB7q49azbYKfO/Whwr/WahotXYra/TuxVGmsraODIQP19JquoL5wghOW7TK yBp2GcWk5OoN2nJAW6IDHfatKKYTSsExJwIs+mqM9soUBPHz8HvqfHe753w5agQ7k4HjONIFJ9BMS LUGpGqMihc6WPClmI3tut56D/za07CPxetmc25uFDpAlacfAjTNVigB/bDJzZS5e3kY6wWkzOMu39 cxe/ODUyYjp1nabLJGLQDoAdaW96TAUTB92PROZk3fW8XbothjXSkyAIwZYMyJ/62KYiGQko2QKMo mV7+GJSw==; To: David Marchand , dev@dpdk.org Cc: arybchenko@solarflare.com, Neil Horman , John McNamara , Marko Kovacevic , Ferruh Yigit , Gaetan Rivet References: <20200914081727.12215-1-david.marchand@redhat.com> <20200917112823.10534-1-david.marchand@redhat.com> <20200917112823.10534-4-david.marchand@redhat.com> From: "Kinsella, Ray" Autocrypt: addr=mdr@ashroe.eu; keydata= mQINBFv8B3wBEAC+5ImcgbIvadt3axrTnt7Sxch3FsmWTTomXfB8YiuHT8KL8L/bFRQSL1f6 ASCHu3M89EjYazlY+vJUWLr0BhK5t/YI7bQzrOuYrl9K94vlLwzD19s/zB/g5YGGR5plJr0s JtJsFGEvF9LL3e+FKMRXveQxBB8A51nAHfwG0WSyx53d61DYz7lp4/Y4RagxaJoHp9lakn8j HV2N6rrnF+qt5ukj5SbbKWSzGg5HQF2t0QQ5tzWhCAKTfcPlnP0GymTBfNMGOReWivi3Qqzr S51Xo7hoGujUgNAM41sxpxmhx8xSwcQ5WzmxgAhJ/StNV9cb3HWIoE5StCwQ4uXOLplZNGnS uxNdegvKB95NHZjRVRChg/uMTGpg9PqYbTIFoPXjuk27sxZLRJRrueg4tLbb3HM39CJwSB++ YICcqf2N+GVD48STfcIlpp12/HI+EcDSThzfWFhaHDC0hyirHxJyHXjnZ8bUexI/5zATn/ux TpMbc/vicJxeN+qfaVqPkCbkS71cHKuPluM3jE8aNCIBNQY1/j87k5ELzg3qaesLo2n1krBH bKvFfAmQuUuJT84/IqfdVtrSCTabvDuNBDpYBV0dGbTwaRfE7i+LiJJclUr8lOvHUpJ4Y6a5 0cxEPxm498G12Z3NoY/mP5soItPIPtLR0rA0fage44zSPwp6cQARAQABtBxSYXkgS2luc2Vs bGEgPG1kckBhc2hyb2UuZXU+iQJUBBMBCAA+FiEEcDUDlKDJaDuJlfZfdJdaH/sCCpsFAlv8 B3wCGyMFCQlmAYAFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AACgkQdJdaH/sCCptdtRAAl0oE msa+djBVYLIsax+0f8acidtWg2l9f7kc2hEjp9h9aZCpPchQvhhemtew/nKavik3RSnLTAyn B3C/0GNlmvI1l5PFROOgPZwz4xhJKGN7jOsRrbkJa23a8ly5UXwF3Vqnlny7D3z+7cu1qq/f VRK8qFyWkAb+xgqeZ/hTcbJUWtW+l5Zb+68WGEp8hB7TuJLEWb4+VKgHTpQ4vElYj8H3Z94a 04s2PJMbLIZSgmKDASnyrKY0CzTpPXx5rSJ1q+B1FCsfepHLqt3vKSALa3ld6bJ8fSJtDUJ7 JLiU8dFZrywgDIVme01jPbjJtUScW6jONLvhI8Z2sheR71UoKqGomMHNQpZ03ViVWBEALzEt TcjWgJFn8yAmxqM4nBnZ+hE3LbMo34KCHJD4eg18ojDt3s9VrDLa+V9fNxUHPSib9FD9UX/1 +nGfU/ZABmiTuUDM7WZdXri7HaMpzDRJUKI6b+/uunF8xH/h/MHW16VuMzgI5dkOKKv1LejD dT5mA4R+2zBS+GsM0oa2hUeX9E5WwjaDzXtVDg6kYq8YvEd+m0z3M4e6diFeLS77/sAOgaYL 92UcoKD+Beym/fVuC6/55a0e12ksTmgk5/ZoEdoNQLlVgd2INtvnO+0k5BJcn66ZjKn3GbEC VqFbrnv1GnA58nEInRCTzR1k26h9nmS5Ag0EW/wHfAEQAMth1vHr3fOZkVOPfod3M6DkQir5 xJvUW5EHgYUjYCPIa2qzgIVVuLDqZgSCCinyooG5dUJONVHj3nCbITCpJp4eB3PI84RPfDcC hf/V34N/Gx5mTeoymSZDBmXT8YtvV/uJvn+LvHLO4ZJdvq5ZxmDyxfXFmkm3/lLw0+rrNdK5 pt6OnVlCqEU9tcDBezjUwDtOahyV20XqxtUttN4kQWbDRkhT+HrA9WN9l2HX91yEYC+zmF1S OhBqRoTPLrR6g4sCWgFywqztpvZWhyIicJipnjac7qL/wRS+wrWfsYy6qWLIV80beN7yoa6v ccnuy4pu2uiuhk9/edtlmFE4dNdoRf7843CV9k1yRASTlmPkU59n0TJbw+okTa9fbbQgbIb1 pWsAuicRHyLUIUz4f6kPgdgty2FgTKuPuIzJd1s8s6p2aC1qo+Obm2gnBTduB+/n1Jw+vKpt 07d+CKEKu4CWwvZZ8ktJJLeofi4hMupTYiq+oMzqH+V1k6QgNm0Da489gXllU+3EFC6W1qKj tkvQzg2rYoWeYD1Qn8iXcO4Fpk6wzylclvatBMddVlQ6qrYeTmSbCsk+m2KVrz5vIyja0o5Y yfeN29s9emXnikmNfv/dA5fpi8XCANNnz3zOfA93DOB9DBf0TQ2/OrSPGjB3op7RCfoPBZ7u AjJ9dM7VABEBAAGJAjwEGAEIACYWIQRwNQOUoMloO4mV9l90l1of+wIKmwUCW/wHfAIbDAUJ CWYBgAAKCRB0l1of+wIKm3KlD/9w/LOG5rtgtCUWPl4B3pZvGpNym6XdK8cop9saOnE85zWf u+sKWCrxNgYkYP7aZrYMPwqDvilxhbTsIJl5HhPgpTO1b0i+c0n1Tij3EElj5UCg3q8mEc17 c+5jRrY3oz77g7E3oPftAjaq1ybbXjY4K32o3JHFR6I8wX3m9wJZJe1+Y+UVrrjY65gZFxcA thNVnWKErarVQGjeNgHV4N1uF3pIx3kT1N4GSnxhoz4Bki91kvkbBhUgYfNflGURfZT3wIKK +d50jd7kqRouXUCzTdzmDh7jnYrcEFM4nvyaYu0JjSS5R672d9SK5LVIfWmoUGzqD4AVmUW8 pcv461+PXchuS8+zpltR9zajl72Q3ymlT4BTAQOlCWkD0snBoKNUB5d2EXPNV13nA0qlm4U2 GpROfJMQXjV6fyYRvttKYfM5xYKgRgtP0z5lTAbsjg9WFKq0Fndh7kUlmHjuAIwKIV4Tzo75 QO2zC0/NTaTjmrtiXhP+vkC4pcrOGNsbHuaqvsc/ZZ0siXyYsqbctj/sCd8ka2r94u+c7o4l BGaAm+FtwAfEAkXHu4y5Phuv2IRR+x1wTey1U1RaEPgN8xq0LQ1OitX4t2mQwjdPihZQBCnZ wzOrkbzlJMNrMKJpEgulmxAHmYJKgvZHXZXtLJSejFjR0GdHJcL5rwVOMWB8cg== Message-ID: <3c0817ac-1caa-44a1-2ee0-b086b43e2f77@ashroe.eu> Date: Fri, 18 Sep 2020 14:43:17 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.12.0 MIME-Version: 1.0 In-Reply-To: <20200917112823.10534-4-david.marchand@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit X-AuthUser: mdr@ashroe.eu Subject: Re: [dpdk-dev] [PATCH v3 3/6] pci: move resource mapping to the PCI bus 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 17/09/2020 12:28, David Marchand wrote: > As reported during 20.08 work for Windows, the pci_map_resource API was > built with the assumption that its flags would be passed to mmap(). > > This introduced a regression when adding the rte_mem_map API as reported > in the workaround commit 9d2b24593724 ("pci: keep API compatibility with > mmap values"). > > This API was only used in the PCI bus code, so move it there. > > There is no code change happening during the move. > The only change is in the pci_map_resource description where the > additional flags are now documented as rte_mem_map API flags: > - * The additional flags for the mapping range. > + * The additional rte_mem_map() flags for the mapping range. > > Signed-off-by: David Marchand > Acked-by: Andrew Rybchenko > --- > doc/guides/rel_notes/deprecation.rst | 11 ----- > doc/guides/rel_notes/release_20_11.rst | 6 +++ > drivers/bus/pci/linux/pci_init.h | 2 + > drivers/bus/pci/linux/pci_uio.c | 1 + > drivers/bus/pci/pci_common.c | 41 ++++++++++++++++ > drivers/bus/pci/private.h | 65 +++++++++++++++++++++++++ > lib/librte_pci/rte_pci.c | 42 ---------------- > lib/librte_pci/rte_pci.h | 66 -------------------------- > lib/librte_pci/rte_pci_version.map | 2 - > 9 files changed, 115 insertions(+), 121 deletions(-) > > diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst > index 83ba567632..8fca461045 100644 > --- a/doc/guides/rel_notes/deprecation.rst > +++ b/doc/guides/rel_notes/deprecation.rst > @@ -113,17 +113,6 @@ Deprecation Notices > us extending existing enum/define. > One solution can be using a fixed size array instead of ``.*MAX.*`` value. > > -* pci: The PCI resources map API (``pci_map_resource`` and > - ``pci_unmap_resource``) was not abstracting the Unix mmap flags (see the > - workaround for Windows support implemented in the commit > - 9d2b24593724 ("pci: keep API compatibility with mmap values")). > - This API will be removed from the public API in 20.11 and moved to the PCI > - bus driver along with the PCI resources lists and associated structures > - (``pci_map``, ``pci_msix_table``, ``mapped_pci_resource`` and > - ``mapped_pci_res_list``). > - With this removal, there won't be a need for the mentioned workaround which > - will be reverted. > - > * mbuf: Some fields will be converted to dynamic API in DPDK 20.11 > in order to reserve more space for the dynamic fields, as explained in > `this presentation `_. > diff --git a/doc/guides/rel_notes/release_20_11.rst b/doc/guides/rel_notes/release_20_11.rst > index a76e7a2941..185eeae731 100644 > --- a/doc/guides/rel_notes/release_20_11.rst > +++ b/doc/guides/rel_notes/release_20_11.rst > @@ -90,6 +90,12 @@ API Changes > * pci: Removed the ``rte_kernel_driver`` enum defined in rte_dev.h and > replaced with a private enum in the PCI subsystem. > > +* pci: Removed the PCI resources map API from the public API > + (``pci_map_resource`` and ``pci_unmap_resource``) and moved it to the > + PCI bus driver along with the PCI resources lists and associated structures > + (``pci_map``, ``pci_msix_table``, ``mapped_pci_resource`` and > + ``mapped_pci_res_list``). > + > * mbuf: Removed the unioned field ``refcnt_atomic`` from > the structures ``rte_mbuf`` and ``rte_mbuf_ext_shared_info``. > The field ``refcnt`` is remaining from the old unions. > diff --git a/drivers/bus/pci/linux/pci_init.h b/drivers/bus/pci/linux/pci_init.h > index c2e603a374..dcea726186 100644 > --- a/drivers/bus/pci/linux/pci_init.h > +++ b/drivers/bus/pci/linux/pci_init.h > @@ -7,6 +7,8 @@ > > #include > > +#include "private.h" > + > /** IO resource type: */ > #define IORESOURCE_IO 0x00000100 > #define IORESOURCE_MEM 0x00000200 > diff --git a/drivers/bus/pci/linux/pci_uio.c b/drivers/bus/pci/linux/pci_uio.c > index a354920d5f..9ab20a0b25 100644 > --- a/drivers/bus/pci/linux/pci_uio.c > +++ b/drivers/bus/pci/linux/pci_uio.c > @@ -25,6 +25,7 @@ > > #include "eal_filesystem.h" > #include "pci_init.h" > +#include "private.h" > > void *pci_map_addr = NULL; > > diff --git a/drivers/bus/pci/pci_common.c b/drivers/bus/pci/pci_common.c > index dddf2b2aad..3a2ae07958 100644 > --- a/drivers/bus/pci/pci_common.c > +++ b/drivers/bus/pci/pci_common.c > @@ -19,6 +19,7 @@ > #include > #include > #include > +#include > #include > #include > #include > @@ -79,6 +80,46 @@ pci_name_set(struct rte_pci_device *dev) > dev->device.name = dev->name; > } > > +/* map a particular resource from a file */ > +void * > +pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size, > + int additional_flags) > +{ > + void *mapaddr; > + > + /* Map the PCI memory resource of device */ > + mapaddr = rte_mem_map(requested_addr, size, > + RTE_PROT_READ | RTE_PROT_WRITE, > + RTE_MAP_SHARED | additional_flags, fd, offset); > + if (mapaddr == NULL) { > + RTE_LOG(ERR, EAL, > + "%s(): cannot map resource(%d, %p, 0x%zx, 0x%llx): %s (%p)\n", > + __func__, fd, requested_addr, size, > + (unsigned long long)offset, > + rte_strerror(rte_errno), mapaddr); > + mapaddr = MAP_FAILED; /* API uses mmap error code */ > + } else > + RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr); > + > + return mapaddr; > +} > + > +/* unmap a particular resource */ > +void > +pci_unmap_resource(void *requested_addr, size_t size) > +{ > + if (requested_addr == NULL) > + return; > + > + /* Unmap the PCI memory resource of device */ > + if (rte_mem_unmap(requested_addr, size)) { > + RTE_LOG(ERR, EAL, "%s(): cannot mem unmap(%p, %#zx): %s\n", > + __func__, requested_addr, size, > + rte_strerror(rte_errno)); > + } else > + RTE_LOG(DEBUG, EAL, " PCI memory unmapped at %p\n", > + requested_addr); > +} > /* > * Match the PCI Driver and Device using the ID Table > */ > diff --git a/drivers/bus/pci/private.h b/drivers/bus/pci/private.h > index 367cdd9a65..7c89744b66 100644 > --- a/drivers/bus/pci/private.h > +++ b/drivers/bus/pci/private.h > @@ -81,6 +81,71 @@ void rte_pci_insert_device(struct rte_pci_device *exist_pci_dev, > */ > int pci_update_device(const struct rte_pci_addr *addr); > > +/** > + * A structure describing a PCI mapping. > + */ > +struct pci_map { > + void *addr; > + char *path; > + uint64_t offset; > + uint64_t size; > + uint64_t phaddr; > +}; > + > +struct pci_msix_table { > + int bar_index; > + uint32_t offset; > + uint32_t size; > +}; > + > +/** > + * A structure describing a mapped PCI resource. > + * For multi-process we need to reproduce all PCI mappings in secondary > + * processes, so save them in a tailq. > + */ > +struct mapped_pci_resource { > + TAILQ_ENTRY(mapped_pci_resource) next; > + > + struct rte_pci_addr pci_addr; > + char path[PATH_MAX]; > + int nb_maps; > + struct pci_map maps[PCI_MAX_RESOURCE]; > + struct pci_msix_table msix_table; > +}; > + > +/** mapped pci device list */ > +TAILQ_HEAD(mapped_pci_res_list, mapped_pci_resource); > + > +/** > + * Map a particular resource from a file. > + * > + * @param requested_addr > + * The starting address for the new mapping range. > + * @param fd > + * The file descriptor. > + * @param offset > + * The offset for the mapping range. > + * @param size > + * The size for the mapping range. > + * @param additional_flags > + * The additional rte_mem_map() flags for the mapping range. > + * @return > + * - On success, the function returns a pointer to the mapped area. > + * - On error, MAP_FAILED is returned. > + */ > +void *pci_map_resource(void *requested_addr, int fd, off_t offset, > + size_t size, int additional_flags); > + > +/** > + * Unmap a particular resource. > + * > + * @param requested_addr > + * The address for the unmapping range. > + * @param size > + * The size for the unmapping range. > + */ > +void pci_unmap_resource(void *requested_addr, size_t size); > + > /** > * Map the PCI resource of a PCI device in virtual memory > * > diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c > index 1d1cbc75ac..c91be8b167 100644 > --- a/lib/librte_pci/rte_pci.c > +++ b/lib/librte_pci/rte_pci.c > @@ -144,45 +144,3 @@ rte_pci_addr_parse(const char *str, struct rte_pci_addr *addr) > return 0; > return -1; > } > - > - > -/* map a particular resource from a file */ > -void * > -pci_map_resource(void *requested_addr, int fd, off_t offset, size_t size, > - int additional_flags) > -{ > - void *mapaddr; > - > - /* Map the PCI memory resource of device */ > - mapaddr = rte_mem_map(requested_addr, size, > - RTE_PROT_READ | RTE_PROT_WRITE, > - RTE_MAP_SHARED | additional_flags, fd, offset); > - if (mapaddr == NULL) { > - RTE_LOG(ERR, EAL, > - "%s(): cannot map resource(%d, %p, 0x%zx, 0x%llx): %s (%p)\n", > - __func__, fd, requested_addr, size, > - (unsigned long long)offset, > - rte_strerror(rte_errno), mapaddr); > - mapaddr = MAP_FAILED; /* API uses mmap error code */ > - } else > - RTE_LOG(DEBUG, EAL, " PCI memory mapped at %p\n", mapaddr); > - > - return mapaddr; > -} > - > -/* unmap a particular resource */ > -void > -pci_unmap_resource(void *requested_addr, size_t size) > -{ > - if (requested_addr == NULL) > - return; > - > - /* Unmap the PCI memory resource of device */ > - if (rte_mem_unmap(requested_addr, size)) { > - RTE_LOG(ERR, EAL, "%s(): cannot mem unmap(%p, %#zx): %s\n", > - __func__, requested_addr, size, > - rte_strerror(rte_errno)); > - } else > - RTE_LOG(DEBUG, EAL, " PCI memory unmapped at %p\n", > - requested_addr); > -} > diff --git a/lib/librte_pci/rte_pci.h b/lib/librte_pci/rte_pci.h > index a03235da1f..567c8cd68d 100644 > --- a/lib/librte_pci/rte_pci.h > +++ b/lib/librte_pci/rte_pci.h > @@ -64,42 +64,6 @@ struct rte_pci_addr { > #define PCI_ANY_ID (0xffff) > #define RTE_CLASS_ANY_ID (0xffffff) > > -/** > - * A structure describing a PCI mapping. > - */ > -struct pci_map { > - void *addr; > - char *path; > - uint64_t offset; > - uint64_t size; > - uint64_t phaddr; > -}; > - > -struct pci_msix_table { > - int bar_index; > - uint32_t offset; > - uint32_t size; > -}; > - > -/** > - * A structure describing a mapped PCI resource. > - * For multi-process we need to reproduce all PCI mappings in secondary > - * processes, so save them in a tailq. > - */ > -struct mapped_pci_resource { > - TAILQ_ENTRY(mapped_pci_resource) next; > - > - struct rte_pci_addr pci_addr; > - char path[PATH_MAX]; > - int nb_maps; > - struct pci_map maps[PCI_MAX_RESOURCE]; > - struct pci_msix_table msix_table; > -}; > - > - > -/** mapped pci device list */ > -TAILQ_HEAD(mapped_pci_res_list, mapped_pci_resource); > - > /** > * Utility function to write a pci device name, this device name can later be > * used to retrieve the corresponding rte_pci_addr using eal_parse_pci_* > @@ -145,36 +109,6 @@ int rte_pci_addr_cmp(const struct rte_pci_addr *addr, > */ > int rte_pci_addr_parse(const char *str, struct rte_pci_addr *addr); > > -/** > - * Map a particular resource from a file. > - * > - * @param requested_addr > - * The starting address for the new mapping range. > - * @param fd > - * The file descriptor. > - * @param offset > - * The offset for the mapping range. > - * @param size > - * The size for the mapping range. > - * @param additional_flags > - * The additional flags for the mapping range. > - * @return > - * - On success, the function returns a pointer to the mapped area. > - * - On error, MAP_FAILED is returned. > - */ > -void *pci_map_resource(void *requested_addr, int fd, off_t offset, > - size_t size, int additional_flags); > - > -/** > - * Unmap a particular resource. > - * > - * @param requested_addr > - * The address for the unmapping range. > - * @param size > - * The size for the unmapping range. > - */ > -void pci_unmap_resource(void *requested_addr, size_t size); > - > #ifdef __cplusplus > } > #endif > diff --git a/lib/librte_pci/rte_pci_version.map b/lib/librte_pci/rte_pci_version.map > index cd77c9dc9e..1db19a5122 100644 > --- a/lib/librte_pci/rte_pci_version.map > +++ b/lib/librte_pci/rte_pci_version.map > @@ -1,8 +1,6 @@ > DPDK_21 { > global: > > - pci_map_resource; > - pci_unmap_resource; > rte_pci_addr_cmp; > rte_pci_addr_parse; > rte_pci_device_name; > Acked-by: Ray Kinsella