Received: by 2002:a05:6a10:2726:0:0:0:0 with SMTP id ib38csp2286838pxb; Fri, 25 Mar 2022 14:41:19 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzyd48obH/Y5MkaqjuIUn7nGaJkEiIuyl9Lm0+nEf4BlDYmPDxzf4l0dSoYkWtx0TfYLLEy X-Received: by 2002:a63:194e:0:b0:372:c757:c569 with SMTP id 14-20020a63194e000000b00372c757c569mr1204214pgz.516.1648244479581; Fri, 25 Mar 2022 14:41:19 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1648244479; cv=none; d=google.com; s=arc-20160816; b=xmqxLOP5J1ZJqNmjmjo5L7fhUneLMuWzED2uA69M92JKb/iQInLBMWhGoQfvjl76Ac jw+dW5LxtkiI2rvwoFB4DnC/MiSXWBIwUXTf9m18lzR7on26kEmu27niYtYG5z2Wdi+/ VNtbgsSApJE0AlieW/cdisLHEHdKghc5Uw2eA6Rm8fF65kyDWKQHMfz29mgFJjEK1a8u e9zqC6Ml6EeylMKt+V7gwnronP9wO5mMu9QiAoAcm+U5/XLga1yoTKwQ3Zf6j73rBVZU rnHiVR/a5kFozNmy7yaq6y0RfozcPj7SPoWGsbCWlqsLFxVBNfxoY5W3NijR9DY7G0+H OFTg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :user-agent:references:in-reply-to:date:cc:to:from:subject :message-id:dkim-signature; bh=lbD/A+mPwRg8dZJHLfabQrV74g1iS2RTgXnbR4+XnGY=; b=gnY1ShjcPIGT+/i1pNhCQqc2QMKb1Wm8gEivb1UgLomA+5Y3qIiwb8e2QayRQBjQlW pzrQKeLThDhpJ1wRUpG93kh68ZgU5SCa49iapRbjx+5P7S2ydpbbeN1MWSZoccrQ65S8 6mBlJUFuVhW8DJTUdyPAMJxoGdDLU8Nix7FJ7G6eoqyWXnYFF7oeFSZJdv8lEY528uOz XiXxO6kzpsl6bIvU6MX3JfISfMQP5EEOhA3KvdHjS6NfFCDLV6irIE+msO9z+FPfHp1N WKCZ6R87vjoCcEPyGrILX9WPHd52bKptA/quaky3QJD4NvdoD+UocQNvaWi8uB/DbzWf UvYQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@sipsolutions.net header.s=mail header.b=Ib9JNvmI; spf=softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-wireless-owner@vger.kernel.org; dmarc=pass (p=NONE sp=REJECT dis=NONE) header.from=sipsolutions.net Return-Path: Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net. [23.128.96.19]) by mx.google.com with ESMTPS id h67-20020a636c46000000b003822c7378fcsi3436402pgc.124.2022.03.25.14.41.19 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Mar 2022 14:41:19 -0700 (PDT) Received-SPF: softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19; Authentication-Results: mx.google.com; dkim=pass header.i=@sipsolutions.net header.s=mail header.b=Ib9JNvmI; spf=softfail (google.com: domain of transitioning linux-wireless-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) smtp.mailfrom=linux-wireless-owner@vger.kernel.org; dmarc=pass (p=NONE sp=REJECT dis=NONE) header.from=sipsolutions.net Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 63C88CC504; Fri, 25 Mar 2022 14:13:31 -0700 (PDT) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233344AbiCYVPB (ORCPT + 70 others); Fri, 25 Mar 2022 17:15:01 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39536 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233233AbiCYVPA (ORCPT ); Fri, 25 Mar 2022 17:15:00 -0400 Received: from sipsolutions.net (s3.sipsolutions.net [IPv6:2a01:4f8:191:4433::2]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 6BDAA34B99; Fri, 25 Mar 2022 14:13:25 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=sipsolutions.net; s=mail; h=Content-Transfer-Encoding:MIME-Version: Content-Type:References:In-Reply-To:Date:Cc:To:From:Subject:Message-ID:Sender :Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From:Resent-To: Resent-Cc:Resent-Message-ID; bh=lbD/A+mPwRg8dZJHLfabQrV74g1iS2RTgXnbR4+XnGY=; t=1648242805; x=1649452405; b=Ib9JNvmIrbaIq4qyxKnZ9Tkmka/mOeq9EwrctRUr/3VGkEK J7LUg2Py9S/VyTcFA/wF0aQ/Cvj0UZTS6PCiaXsEHS2z04WMDbHCBCju/ieGO/48w+e982A10R/zX eLQVkmMHFP3vMT1jFaV/wwWNfdk0isI/lfsKCzDaStrv+48+efPD3hx+skgIWAAeJogOxJi4X96SY nga/8Td2RNuZ05G7xisTtrW0JigPDQlSZedfh9WYXifrBhC3Ygb490+Tr/beWKqj/KJBwvViyx7X3 AtKxNNj0joHKS64Jj6Rdg4/QVEunFT7PvYrfDYMrD1B8hxSYV6a8B7/J8TI5W0qw==; Received: by sipsolutions.net with esmtpsa (TLS1.3:ECDHE_SECP256R1__RSA_PSS_RSAE_SHA256__AES_256_GCM:256) (Exim 4.95) (envelope-from ) id 1nXrF7-000VW3-JE; Fri, 25 Mar 2022 22:13:09 +0100 Message-ID: Subject: Re: [REGRESSION] Recent swiotlb DMA_FROM_DEVICE fixes break ath9k-based AP From: Johannes Berg To: Linus Torvalds Cc: Maxime Bizon , Toke =?ISO-8859-1?Q?H=F8iland-J=F8rgensen?= , 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 Date: Fri, 25 Mar 2022 22:13:08 +0100 In-Reply-To: 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> Content-Type: text/plain; charset="UTF-8" User-Agent: Evolution 3.42.4 (3.42.4-1.fc35) MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-malware-bazaar: not-scanned X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RDNS_NONE,SPF_HELO_NONE,T_SCC_BODY_TEXT_LINE autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-wireless@vger.kernel.org On Fri, 2022-03-25 at 13:47 -0700, Linus Torvalds wrote: > 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. Well I see now that you said 'cache "writeback"' in (1), and 'flush' in (2), so perhaps you were thinking of the same, and I'm just calling it "flush" and "invalidate" respectively? > 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". Hmm. Yeah, can't really disagree with that. However, this seems to be the wrong spot to flush (writeback?) the cache, as we're trying to get data from the device, not potentially overwrite the data that the device wrote because we have a dirty cacheline. Hmm. Then again, how could we possibly have a dirty cacheline? Which starts to clarify in my mind why we have a sort of (implied) ownership model: if the CPU dirties a cacheline while the device has ownership then the cache writeback might overwrite the DMA data. So it's easier to think of it as "CPU has ownership" and "device has ownership", but evidently that simple model breaks down in real-world cases such as ath9k where the CPU wants to look, but not write, and the device continues doing DMA at the same time. Now in that case the cache wouldn't be considered dirty either since the CPU was just reading, but who knows? Hence the suggestion of just invalidate, not flush. > If the core has support for "invalidate clean cache lines only", then > that's possibly a good alternative. Well if you actually did dirty the cacheline, then you have a bug one way or the other, and it's going to be really hard to debug - either you lose the CPU write, or you lose the device write, there's no way you're not losing one of them? ath9k doesn't write, of course, so hopefully the core wouldn't write back what it must think of as clean cachelines, even if the device modified the memory underneath already. So really while I agree with your semantics, I was previously privately suggesting to Toke we should probably have something like dma_single_cpu_peek() // read buffer and check if it was done dma_single_cpu_peek_finish() which really is equivalent to the current dma_sync_single_for_cpu(DMA_FROM_DEVICE) // ... dma_sync_single_for_device(DMA_FROM_DEVICE) that ath9k does, but makes it clearer that you really can't write to the buffer... but, water under the bridge, I guess. Thinking about the ownership model again - it seems that we need to at least modify that ownership model in the sense that we have *write* ownership that we pass around, not just "ownership". But if we do that, then we need to clarify which operations pass write ownership and which don't. So the operations (1) dma_sync_single_for_device(DMA_TO_DEVICE) (2) dma_sync_single_for_cpu(DMA_FROM_DEVICE) (3) dma_sync_single_for_device(DMA_FROM_DEVICE) really only (1) passes write ownership to the device, but then you can't get it back? But that cannot be true, because ath9k maps the buffer as DMA_BIDIRECTIONAL, and then eventually might want to recycle it. Actually though, perhaps passing write ownership back to the CPU isn't an operation that the DMA API needs to worry about - if the CPU has read ownership and the driver knows separately that the device is no longer accessing it, then basically the CPU already got write ownership, and passes that back uses (1)? > > 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? Yes, a few. > 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. I agree. > But yes, if we do and people come up with semantics for it, those > semantics should be clearly documented. I'm not sure? I'm wondering if this isn't just because - like me initially - people misunderstood the direction argument, or didn't understand it well enough, and then just passed the same value as for the map()/unmap()? You have to pass the size to all of them too, after all ... but I'm speculating. > 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. Makes sense. > But it sounds like you agree with my analysis, just not with some of > my bad/incorrect word choices. Yes. johannes