lkml.org 
[lkml]   [2022]   [Mar]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP
    On Fri, 25 Mar 2022 11:27:41 +0000
    Robin Murphy <robin.murphy@arm.com> wrote:

    > What muddies the waters a bit is that the opposite combination
    > sync_for_cpu(DMA_TO_DEVICE) really *should* always be a no-op, and I for
    > one have already made the case for eliding that in code elsewhere, but
    > it doesn't necessarily hold for the inverse here, hence why I'm not sure
    > there even is a robust common solution for peeking at a live
    > DMA_FROM_DEVICE buffer.

    In https://lkml.org/lkml/2022/3/24/739 I also argued, that a robust
    common solution for a peeking at a live DMA_FROM_DEVICE buffer is
    probably not possible, at least not with the current programming model
    as described by Documentation/core-api/dma-api.rst.

    Namely AFAIU the programming model is based on exclusive ownership: the
    buffer is either owned by the device, which means CPU(s) are not allowed
    to *access* it, or it is owned by the CPU(s), and the device is not
    allowed to *access* it. Do we agree on this?

    Considering what Linus said here https://lkml.org/lkml/2022/3/24/775
    I understand that: if the idea that dma_sync_*_for_{cpu,device} always
    transfers ownership to the cpu and device respectively is abandoned,
    and we re-define ownership in a sense that only the owner may write,
    but non-owner is allowed to read, then it may be possible to make the
    scenario under discussion work.

    The scenario in pseudo code:

    /* when invoked device might be doing DMA into buf */
    rx_buf_complete(buf)
    {
    prepare_peek(buf, DMA_FROM_DEVICE);
    if (!is_ready(buf)) {
    /*let device gain the buffer again*/
    peek_done_not_ready(buf, DMA_FROM_DEVICE);
    return false;
    }
    peek_done_ready(buf, DMA_FROM_DEVICE);
    process_buff(buf, DMA_FROM_DEVICE); is
    }

    IMHO it is pretty obvious, that prepare_peek() has to update the
    cpu copy of the data *without* transferring ownership to the CPU. Since
    the owner is still the device, it is legit for the device to keep
    modifying the buffer via DMA. In case of the swiotlb, we would copy the
    content of the bounce buffer to the orig buffer possibly after
    invalidating
    caches, and for non-swiotlb we would do invalidate caches. So
    prepare_peek() could be actually something like,
    dma_sync_single_for_cpu(buf, DMA_FROM_DEVICE,
    DMA_ATTR_NO_OWNERSHIP_TRANSFER)
    which would most end up being functionally the same, as without the
    flag, since my guess is that the ownership is only tracked in our heads.

    For peek_done_not_ready() there is conceptually nothing to do, because
    the device retained ownership. Thus would either have to mandate
    peek_done_not_ready() being a nop, or non-existent, (that is
    what Toke's patch does in the specific case), or we would have to
    mandate that dma_sync_*_for_*() has no side effects under certain. The
    former looks simpler to me, especially with swiotlb. But we are also
    fine if the cache ain't dirty, because the CPU didn't write (as pointed
    out by Linus) and we were to detect that, and avoid flushing a clean
    cache, or if we were to track ownership and to avoid flushing caches
    because no ownership transfer. But to avoid these bad flushes, at least
    for swiotlb, we would either have to track cache ownership, or even
    worse track dirtiness (for which we would have to extend the API, and
    make the drivers tell us that the cache, i.e. the original buffer got
    dirtied).

    Since the device has ownership when peek_done_not_ready() is invoked,
    we might need to transfer ownership to the CPU in peek_done_ready().
    This could again be a dma_sync_for_cpu() with a flag, which when supplied
    tells the dma API that no sync (cache invalidate) is needed because the
    driver guarantees, that the whole mapping was sufficiently sync-ed by
    prepare_peek(). Please notice, that the whole scheme is based on the
    driver knowing that the whole DMA is done by examining the buffer, and
    it decides based on whatever it sees.

    Some of the ongoing discussion seem so ignore this whole ownership biz.
    My feeling is: the notion of ownership useful. If both sides end up
    modifying (and eventually flushing) we are in trouble IMHO, an ownership
    avoids that. But if the conclusion ends up being, that ownership does
    not matter, then we should make sure it is purged from the documentation,
    because otherwise it will confuse the hell out of people who read
    documentations and care about programming models. People like me.

    Regards,
    Halil

    \
     
     \ /
      Last update: 2022-03-26 00:43    [W:4.279 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site