svn commit: r346645 - in head/sys: compat/linuxkpi/common/include/linux compat/linuxkpi/common/src sys
Tycho Nightingale
tychon at freebsd.org
Wed May 8 01:25:06 UTC 2019
Hi,
> On May 7, 2019, at 9:13 PM, Conrad Meyer <cem at freebsd.org> wrote:
>
> Hi Tycho,
>
> On Wed, Apr 24, 2019 at 1:31 PM Tycho Nightingale <tychon at 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 at 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
More information about the svn-src-all
mailing list