svn commit: r346645 - in head/sys: compat/linuxkpi/common/include/linux compat/linuxkpi/common/src sys

[Hans Petter Selasky]

On 2019-05-08 03:24, Tycho Nightingale wrote:
> 
> 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
> 

I believe I already changed those asserts to what was suggested. See 
later commits on the same file.

--HPS