From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0F188C433F5 for ; Fri, 25 Mar 2022 20:53:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232410AbiCYUz3 (ORCPT ); Fri, 25 Mar 2022 16:55:29 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48006 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232400AbiCYUz1 (ORCPT ); Fri, 25 Mar 2022 16:55:27 -0400 Received: from mail-ej1-x633.google.com (mail-ej1-x633.google.com [IPv6:2a00:1450:4864:20::633]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 39BEE42EF4 for ; Fri, 25 Mar 2022 13:53:52 -0700 (PDT) Received: by mail-ej1-x633.google.com with SMTP id pv16so17684730ejb.0 for ; Fri, 25 Mar 2022 13:53:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux-foundation.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=qTyIfzc6eWn8tPRbJvu4pItLGCLIkdX7GJakm9zDrl8=; b=gm5AHFVvThRrK7sA/BQ6fJ/iDwpQq6O0/7tOPWKWfF0gNOKqop2SvIcH4r/OClE48L VFW0eVhGjvjiyVqrrrPg6Q1OkP1qou04a5ugVipBYO9VTlVyEcQ7kokZzVjAoN7pS1Ch +u+3DjL5YncJtUcCyN3Ju5WCdBaFHsUNObffo= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=qTyIfzc6eWn8tPRbJvu4pItLGCLIkdX7GJakm9zDrl8=; b=Wd5dPjmT3gZbcrDX5/uasFfaok6hHzSRbso203SIro/Bca1X8Dv1rGkB6ooybawBi3 Y6ELiIMLsLiyEzqfPMvJsrGLhzOW6tCy8c8PNmg4xGe3EGzaWhj1TacuH/iKc2pKudz9 0fXLLklbnhtLADDDDdBEgtbhKLSgeaBnBN3jqW/DhkepAVni1jgZQJwpfOUyafo4mp5j krpYEFzmtGhSC3iR0IB+oAS2wvF3qQIcCxw9tDL1xOW5n9Js+rGzKDKDPCr4QLzhBfq/ I1zRgpShwx0be6HJksijf1FhL3d95rTMPFNkVCRMkWQWNL7A/8vfHtnYbeO0ue3Fhv7h mXbg== X-Gm-Message-State: AOAM5301hHjAw1MbDpcPJ2TcDN1Hx+aZLG26EzwlimYEzpeX6IgZ7J7o 5DBmYT4F/zYTvf2/z8a10MOepFW8d2rBZg2TxVU= X-Google-Smtp-Source: ABdhPJyNBS0A97O5n1FjdRUbFzgDiVU6WLvPDroUQUQKD5ZSxZSQWRGT61hVePaV/V5pKzFgo2IUIw== X-Received: by 2002:a17:907:6296:b0:6da:745b:7b40 with SMTP id nd22-20020a170907629600b006da745b7b40mr13965423ejc.750.1648241630464; Fri, 25 Mar 2022 13:53:50 -0700 (PDT) Received: from mail-wr1-f43.google.com (mail-wr1-f43.google.com. [209.85.221.43]) by smtp.gmail.com with ESMTPSA id k12-20020aa7c38c000000b0041939d9ccd0sm3309169edq.81.2022.03.25.13.53.49 for (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 25 Mar 2022 13:53:50 -0700 (PDT) Received: by mail-wr1-f43.google.com with SMTP id j18so12352054wrd.6 for ; Fri, 25 Mar 2022 13:53:49 -0700 (PDT) X-Received: by 2002:ac2:4203:0:b0:448:8053:d402 with SMTP id y3-20020ac24203000000b004488053d402mr9094484lfh.687.1648241278691; Fri, 25 Mar 2022 13:47:58 -0700 (PDT) MIME-Version: 1.0 References: <1812355.tdWV9SEqCh@natalenko.name> <20220324055732.GB12078@lst.de> <4386660.LvFx2qVVIh@natalenko.name> <81ffc753-72aa-6327-b87b-3f11915f2549@arm.com> <878rsza0ih.fsf@toke.dk> <4be26f5d8725cdb016c6fdd9d05cfeb69cdd9e09.camel@freebox.fr> <20220324163132.GB26098@lst.de> <871qyr9t4e.fsf@toke.dk> <31434708dcad126a8334c99ee056dcce93e507f1.camel@freebox.fr> <298f4f9ccad7c3308d3a1fd8b4b4740571305204.camel@sipsolutions.net> In-Reply-To: <298f4f9ccad7c3308d3a1fd8b4b4740571305204.camel@sipsolutions.net> From: Linus Torvalds Date: Fri, 25 Mar 2022 13:47:42 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP To: Johannes Berg Cc: Maxime Bizon , =?UTF-8?B?VG9rZSBIw7hpbGFuZC1Kw7hyZ2Vuc2Vu?= , Robin Murphy , Christoph Hellwig , Oleksandr Natalenko , Halil Pasic , Marek Szyprowski , Kalle Valo , "David S. Miller" , Jakub Kicinski , Paolo Abeni , Olha Cherevyk , iommu , linux-wireless , Netdev , Linux Kernel Mailing List , Greg Kroah-Hartman , stable Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 25, 2022 at 1:38 PM Johannes Berg wrote: > > > (2) The CPU now wants to see any state written by the device since > > the last sync > > > > This is "dma_sync_single_for_cpu(DMA_FROM_DEVICE)". > > > > A bounce-buffer implementation needs to copy *from* the bounce buffer. > > > > A cache-coherent implementation needs to do nothing. > > > > A non-coherent implementation maybe needs to do nothing (ie it > > assumes that previous ops have flushed the cache, and just accessing > > the data will bring the rigth thing back into it). Or it could just > > flush the cache. > > Doesn't that just need to *invalidate* the cache, rather than *flush* > it? Yes. I should have been more careful. That said, I think "invalidate without writeback" is a really dangerous operation (it can generate some *really* hard to debug memory state), so on the whole I think you should always strive to just do "flush-and-invalidate". If the core has support for "invalidate clean cache lines only", then that's possibly a good alternative. > > A non-coherent implementation needs to flush the cache again, bot > > not necessarily do a writeback-flush if there is some cheaper form > > (assuming it does nothing in the "CPU now wants to see any state" case > > because it depends on the data not having been in the caches) > > And similarly here, it would seem that the implementation can't _flush_ > the cache as the device might be writing concurrently (which it does in > fact do in the ath9k case), but it must invalidate the cache? Right, again, when I said "flush" I really should have said "invalidate". > I'm not sure about the (2) case, but here it seems fairly clear cut that > if you have a cache, don't expect the CPU to write to the buffer (as > evidenced by DMA_FROM_DEVICE), you wouldn't want to write out the cache > to DRAM? See above: I'd *really* want to avoid a pure "invalidate cacheline" model. The amount of debug issues that can cause is not worth it. So please flush-and-invalidate, or invalidate-non-dirty, but not just "invalidate". > Then, however, we need to define what happens if you pass > DMA_BIDIRECTIONAL to the sync_for_cpu() and sync_for_device() functions, > which adds two more cases? Or maybe we eventually just think that's not > valid at all, since you have to specify how you're (currently?) using > the buffer, which can't be DMA_BIDIRECTIONAL? Ugh. Do we actually have cases that do it? That sounds really odd for a "sync" operation. It sounds very reasonable for _allocating_ DMA, but for syncing I'm left scratching my head what the semantics would be. But yes, if we do and people come up with semantics for it, those semantics should be clearly documented. And if we don't - or people can't come up with semantics for it - we should actively warn about it and not have some code that does odd things that we don't know what they mean. But it sounds like you agree with my analysis, just not with some of my bad/incorrect word choices. Linus