From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olivier Matz Subject: Re: [PATCH v2 5/7] eal/linux: mmap ioports on ppc64 Date: Mon, 23 May 2016 15:40:58 +0200 Message-ID: <5743086A.2020103@6wind.com> 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> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: "dev@dpdk.org" , Chao Zhu , "Xie, Huawei" To: Yuanhan Liu , David Marchand 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) In-Reply-To: <20160523130749.GJ5641@yliu-dev.sh.intel.com> List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 s= ysfs */ >>> + snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/reso= urce", >>> + SYSFS_PCI_DEVICES, dev->addr.domain, dev->addr.bus, >>> + dev->addr.devid, dev->addr.function); >>> + f =3D fopen(filename, "r"); >>> + if (f =3D=3D NULL) { >>> + RTE_LOG(ERR, EAL, "Cannot open sysfs resource: %s\n", >>> + strerror(errno)); >>> + return -1; >>> + } >>> + for (i =3D 0; i < bar + 1; i++) { >>> + if (fgets(buf, sizeof(buf), f) =3D=3D 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) =3D=3D 0) { >>> + RTE_LOG(ERR, EAL, "BAR %d is not an IO resource\n", b= ar); >>> + goto error; >>> + } >>> + snprintf(filename, sizeof(filename), "%s/" PCI_PRI_FMT "/reso= urce%d", >>> + SYSFS_PCI_DEVICES, dev->addr.domain, dev->addr.bus, >>> + dev->addr.devid, dev->addr.function, bar); >>> + >>> + /* mmap the pci resource */ >>> + fd =3D open(filename, O_RDWR); >>> + if (fd < 0) { >>> + RTE_LOG(ERR, EAL, "Cannot open %s: %s\n", filename, >>> + strerror(errno)); >>> + goto error; >>> + } >>> + addr =3D 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 ? >=20 > 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) >=20 > 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: [=E2=88=92] 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. =09 The fields or parameters of such data type may be incorrectly initialized or accessed by old client applications. [=E2=88=92] 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