From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 57DD35A51 for ; Mon, 23 May 2016 15:41:07 +0200 (CEST) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84_2) (envelope-from ) id 1b4q8X-0005Wx-Lz; Mon, 23 May 2016 15:43:14 +0200 To: Yuanhan Liu , David Marchand References: <1463143859-3105-1-git-send-email-olivier.matz@6wind.com> <1463479192-2488-1-git-send-email-olivier.matz@6wind.com> <1463479192-2488-6-git-send-email-olivier.matz@6wind.com> <20160523130749.GJ5641@yliu-dev.sh.intel.com> Cc: "dev@dpdk.org" , Chao Zhu , "Xie, Huawei" From: Olivier Matz Message-ID: <5743086A.2020103@6wind.com> Date: Mon, 23 May 2016 15:40:58 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.6.0 MIME-Version: 1.0 In-Reply-To: <20160523130749.GJ5641@yliu-dev.sh.intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v2 5/7] eal/linux: mmap ioports on ppc64 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: Mon, 23 May 2016 13:41:07 -0000 Hi Yuanhan, On 05/23/2016 03:07 PM, Yuanhan Liu wrote: > On Tue, May 17, 2016 at 05:54:01PM +0200, David Marchand wrote: >>> +pci_uio_ioport_map(struct rte_pci_device *dev, int bar, >>> + struct rte_pci_ioport *p) >>> +{ >>> + FILE *f; >>> + char buf[BUFSIZ]; >>> + char filename[PATH_MAX]; >>> + uint64_t phys_addr, end_addr, flags; >>> + int fd, i; >>> + void *addr; >>> + >>> + /* open and read addresses of the corresponding resource in sysfs */ >>> + snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource", >>> + SYSFS_PCI_DEVICES, dev->addr.domain, dev->addr.bus, >>> + dev->addr.devid, dev->addr.function); >>> + f = fopen(filename, "r"); >>> + if (f == NULL) { >>> + RTE_LOG(ERR, EAL, "Cannot open sysfs resource: %s\n", >>> + strerror(errno)); >>> + return -1; >>> + } >>> + for (i = 0; i < bar + 1; i++) { >>> + if (fgets(buf, sizeof(buf), f) == NULL) { >>> + RTE_LOG(ERR, EAL, "Cannot read sysfs resource\n"); >>> + goto error; >>> + } >>> + } >>> + if (pci_parse_one_sysfs_resource(buf, sizeof(buf), &phys_addr, >>> + &end_addr, &flags) < 0) >>> + goto error; >>> + if ((flags & IORESOURCE_IO) == 0) { >>> + RTE_LOG(ERR, EAL, "BAR %d is not an IO resource\n", bar); >>> + goto error; >>> + } >>> + snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/resource%d", >>> + SYSFS_PCI_DEVICES, dev->addr.domain, dev->addr.bus, >>> + dev->addr.devid, dev->addr.function, bar); >>> + >>> + /* mmap the pci resource */ >>> + fd = open(filename, O_RDWR); >>> + if (fd < 0) { >>> + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", filename, >>> + strerror(errno)); >>> + goto error; >>> + } >>> + addr = mmap(NULL, end_addr + 1, PROT_READ | PROT_WRITE, >>> + MAP_SHARED, fd, 0); >> >> Sorry, did not catch it in v1, but a close(fd) is missing here. >> With this, I think the patchset looks good. >> >> Just missing some opinion from the virtio maintainers ? > > Apologize for being late for review. Assuming you have done proper > test, this patch set looks good to me. (well, I don't quite like > the tons of "#ifdef ... #else ..#end" block though) > > A side note is that I noticed an ABI breakage introduced in this > patch, so, this release is not a good fit? Thank you for the review. For reference, here is the report of the ABI checker for EAL: [−] struct rte_pci_ioport (2) 1 Field len has been added to this type. 1) This field will not be initialized by old clients. 2) Size of the inclusive type has been changed. NOTE: this field should be accessed only from the new library functions, otherwise it may result in crash or incorrect behavior of applications. 2 Size of this type has been changed from 16 bytes to 24 bytes. The fields or parameters of such data type may be incorrectly initialized or accessed by old client applications. [−] affected symbols (4) rte_eal_pci_ioport_map ( struct rte_pci_device* dev, int bar, struct rte_pci_ioport* p ) @@ DPDK_16.04 3rd parameter 'p' (pointer) has base type 'struct rte_pci_ioport'. rte_eal_pci_ioport_read ( struct rte_pci_ioport* p, void* data, size_t len, off_t offset ) @@ DPDK_16.04 1st parameter 'p' (pointer) has base type 'struct rte_pci_ioport'. rte_eal_pci_ioport_unmap ( struct rte_pci_ioport* p ) @@ DPDK_16.04 1st parameter 'p' (pointer) has base type 'struct rte_pci_ioport'. rte_eal_pci_ioport_write ( struct rte_pci_ioport* p, void const* data, size_t len, off_t offset ) @@ DPDK_16.04 1st parameter 'p' (pointer) has base type 'struct rte_pci_ioport'. My understanding of the comment for this structure is that it's internal to EAL: /** * A structure used to access io resources for a pci device. * rte_pci_ioport is arch, os, driver specific, and should not be used outside * of pci ioport api. */ struct rte_pci_ioport { ... } So I'd say it's ok to have it integrated for 16.07. Regards, Olivier