From owner-svn-src-all@freebsd.org Wed May 8 08:18:09 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 5DD8B15A167A; Wed, 8 May 2019 08:18:09 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (turbocat.net [88.99.82.50]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id F0E679115C; Wed, 8 May 2019 08:18:08 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2016.home.selasky.org (unknown [176.74.212.121]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id D55DF260321; Wed, 8 May 2019 10:18:00 +0200 (CEST) Subject: Re: svn commit: r346645 - in head/sys: compat/linuxkpi/common/include/linux compat/linuxkpi/common/src sys To: Tycho Nightingale , cem@freebsd.org Cc: src-committers , svn-src-all , svn-src-head References: <201904242030.x3OKUkgN073331@repo.freebsd.org> <6991C64C-D105-423F-B66A-4E2B2924565F@freebsd.org> From: Hans Petter Selasky Message-ID: <423baa57-2433-c300-c4a5-c60e64ba664b@selasky.org> Date: Wed, 8 May 2019 10:17:32 +0200 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:60.0) Gecko/20100101 Thunderbird/60.6.1 MIME-Version: 1.0 In-Reply-To: <6991C64C-D105-423F-B66A-4E2B2924565F@freebsd.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-Rspamd-Queue-Id: F0E679115C X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.92 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.92)[-0.919,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 May 2019 08:18:09 -0000 On 2019-05-08 03:24, Tycho Nightingale wrote: > > Hi, > >> On May 7, 2019, at 9:13 PM, Conrad Meyer wrote: >> >> Hi Tycho, >> >> On Wed, Apr 24, 2019 at 1:31 PM Tycho Nightingale wrote: >>> >>> Author: tychon >>> Date: Wed Apr 24 20:30:45 2019 >>> New Revision: 346645 >>> URL: https://svnweb.freebsd.org/changeset/base/346645 >>> >>> Log: >>> LinuxKPI should use bus_dma(9) to be compatible with an IOMMU >>> >>> Reviewed by: hselasky, kib >>> Tested by: greg@unrelenting.technology >>> Sponsored by: Dell EMC Isilon >>> Differential Revision: https://reviews.freebsd.org/D19845 >>> ... >>> Modified: head/sys/compat/linuxkpi/common/src/linux_pci.c >>> ============================================================================== >>> --- head/sys/compat/linuxkpi/common/src/linux_pci.c Wed Apr 24 19:56:02 2019 (r346644) >>> +++ head/sys/compat/linuxkpi/common/src/linux_pci.c Wed Apr 24 20:30:45 2019 (r346645) >>> ... >>> +linux_dma_map_phys(struct device *dev, vm_paddr_t phys, size_t len) >>> +{ >>> ... >>> + nseg = -1; >>> + mtx_lock(&priv->dma_lock); >>> + if (_bus_dmamap_load_phys(priv->dmat, obj->dmamap, phys, len, >>> + BUS_DMA_NOWAIT, &seg, &nseg) != 0) { >>> + bus_dmamap_destroy(priv->dmat, obj->dmamap); >>> + mtx_unlock(&priv->dma_lock); >>> + uma_zfree(linux_dma_obj_zone, obj); >>> + return (0); >>> + } >>> + mtx_unlock(&priv->dma_lock); >>> + >>> + KASSERT(++nseg == 1, ("More than one segment (nseg=%d)", nseg)); >> >> This construct is a bit odd. Coverity produces the (perhaps spurious) >> warning (CID 1401319) that the KASSERT (which can be compiled out in >> !INVARIANTS builds) has a side effect (++nseg). While true, nseg is >> never used afterwards, so perhaps we can use the equivalent expression >> with no side effect instead? I.e., something like: >> >> KASSERT(nseg == 0, ("More than one segment (nseg=%d)", nseg + 1)); >> >> Does that make sense? It is a false positive of sorts, but performing >> side effects in compiled-out assert is a pretty strong antipattern so >> I'd just as soon "fix" the warning. > > The construct is indeed a little odd and mimics how other callers of _bus_dmamap_load_phys() handle the bizarre way nseg is treated. There isn’t any reason for it and in hindsight I prefer your version — especially if it eliminates this Coverity issue. > > Tycho > I believe I already changed those asserts to what was suggested. See later commits on the same file. --HPS