Hi, > On May 7, 2019, at 9:13 PM, Conrad Meyer <c...@freebsd.org> wrote: > > Hi Tycho, > > On Wed, Apr 24, 2019 at 1:31 PM Tycho Nightingale <tyc...@freebsd.org> 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 _______________________________________________ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
