Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752400AbcLEJwm (ORCPT ); Mon, 5 Dec 2016 04:52:42 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:59108 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752207AbcLEJwU (ORCPT ); Mon, 5 Dec 2016 04:52:20 -0500 DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 69D6F61566 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=sricharan@codeaurora.org From: "Sricharan" To: "'Rafael J. Wysocki'" , "'Lorenzo Pieralisi'" Cc: "'Linux PCI'" , "'Will Deacon'" , "'Sinan Kaya'" , "'Tomasz Nowicki'" , "'Joerg Roedel'" , "'ACPI Devel Maling List'" , "'Mark Salter'" , "'Marc Zyngier'" , "'Jon Masters'" , "'Eric Auger'" , "'Bjorn Helgaas'" , "'Prem Mallappa'" , , "'Rafael J. Wysocki'" , "'Linux Kernel Mailing List'" , "'open list:AMD IOMMU \(AMD-VI\)'" , "'Hanjun Guo'" , "'Suravee Suthikulpanit'" , "'Dennis Chen'" , "'Robin Murphy'" , "'Nate Watterson'" References: <20161121100148.24769-1-lorenzo.pieralisi@arm.com> <20161121100148.24769-8-lorenzo.pieralisi@arm.com> <20161202153816.GA18290@red-moon> <20161203103927.GA14953@red-moon> In-Reply-To: Subject: RE: [PATCH v9 07/16] drivers: acpi: implement acpi_dma_configure Date: Mon, 5 Dec 2016 15:22:02 +0530 Message-ID: <003001d24edd$40d7d030$c2877090$@codeaurora.org> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit X-Mailer: Microsoft Outlook 15.0 Thread-Index: AQJfsoa+tSGnNBSpvaOB61aAYN+WbAE7MSD/Amn3DHsCNd2PAAH+InhmAiUkFN6fjmB1AA== Content-Language: en-us Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5853 Lines: 117 Hi Lorenzo, > >On Sat, Dec 3, 2016 at 11:39 AM, Lorenzo Pieralisi > wrote: >> On Sat, Dec 03, 2016 at 03:11:09AM +0100, Rafael J. Wysocki wrote: >>> On Fri, Dec 2, 2016 at 4:38 PM, Lorenzo Pieralisi >>> wrote: >>> > Rafael, Mark, Suravee, >>> > >>> > On Mon, Nov 21, 2016 at 10:01:39AM +0000, Lorenzo Pieralisi wrote: >>> >> On DT based systems, the of_dma_configure() API implements DMA >>> >> configuration for a given device. On ACPI systems an API equivalent to >>> >> of_dma_configure() is missing which implies that it is currently not >>> >> possible to set-up DMA operations for devices through the ACPI generic >>> >> kernel layer. >>> >> >>> >> This patch fills the gap by introducing acpi_dma_configure/deconfigure() >>> >> calls that for now are just wrappers around arch_setup_dma_ops() and >>> >> arch_teardown_dma_ops() and also updates ACPI and PCI core code to use >>> >> the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions. >>> >> >>> >> Since acpi_dma_configure() is used to configure DMA operations, the >>> >> function initializes the dma/coherent_dma masks to sane default values >>> >> if the current masks are uninitialized (also to keep the default values >>> >> consistent with DT systems) to make sure the device has a complete >>> >> default DMA set-up. >>> > >>> > I spotted a niggle that unfortunately was hard to spot (and should not >>> > be a problem per se but better safe than sorry) and I am not comfortable >>> > with it. >>> > >>> > Following commit d0562674838c ("ACPI / scan: Parse _CCA and setup >>> > device coherency") in acpi_bind_one() we check if the acpi_device >>> > associated with a device just added supports DMA, first it was >>> > done with acpi_check_dma() and then commit 1831eff876bd ("device >>> > property: ACPI: Make use of the new DMA Attribute APIs") changed >>> > it to acpi_get_dma_attr(). >>> > >>> > The subsequent check (attr != DEV_DMA_NOT_SUPPORTED) is always true >>> > on _any_ acpi device we pass to acpi_bind_one() on x86, which was >>> > fine because we used it to call arch_setup_dma_ops(), which is a nop >>> > on x86. On ARM64 a _CCA method is required to define if a device >>> > supports DMA so (attr != DEV_DMA_NOT_SUPPORTED) may well be false. >>> > >>> > Now, acpi_bind_one() is used to bind an acpi_device to its physical >>> > node also for pseudo-devices like cpus and memory nodes. For those >>> > objects, on x86, attr will always be != DEV_DMA_NOT_SUPPORTED. >>> > >>> > So far so good, because on x86 arch_setup_dma_ops() is empty code. >>> > >>> > With this patch, I use the (attr != DEV_DMA_NOT_SUPPORTED) check >>> > to call acpi_dma_configure() which is basically a nop on x86 except >>> > that it sets up the dma_mask/coherent_dma_mask to a sane default value >>> > (after all we are setting up DMA for the device so it makes sense to >>> > initialize the masks there if they were unset since we are configuring >>> > DMA for the device in question) for the given device. >>> > >>> > Problem is, as per the explanation above, we are also setting the >>> > default dma masks for pseudo-devices (eg CPUs) that were previously >>> > untouched, it should not be a problem per-se but I am not comfortable >>> > with that, honestly it does not make much sense. >>> > >>> > An easy "fix" would be to move the default dma masks initialization out >>> > of acpi_dma_configure() (as it was in previous patch versions of this >>> > series - I moved it to acpi_dma_configure() just a consolidation point >>> > for initializing the masks instead of scattering them in every >>> > acpi_dma_configure caller) I can send this as a fix-up patch to Joerg if >>> > we think that's the right thing to do (or I can send it to Rafael later >>> > when the code is in the merged depending on the timing) just let me >>> > know please. >>> >>> Why can't arch_setup_dma_ops() set those masks too? >> >> Because the dma masks set-up is done by the caller (see >> of_dma_configure()) according to firmware configuration or >> platform data knowledge. I wanted to replicate the of_dma_configure() >> interface on ACPI for obvious reasons (on ARM systems), I stopped >> short of adding ACPI code to mirror of_dma_get_range() equivalent >> (through the _DMA object) but I am really really nervous about changing >> the code path on x86 because in theory all is fine, in practice even >> just setting the masks to sane values can have unexpected consequences, >> I just can't know (that's why I wasn't doing it in the first iterations >> of this series). >> >> Side note: DT with of_dma_configure() and ACPI with >> acpi_create_platform_device() set the default dma mask for all >> platform devices already _regardless_ of what they really are, though >> arguably acpi_bind_one() touches ways more devices. >> >> I really think that removing the default dma masks settings from >> acpi_dma_configure() is the safer thing to do for the time being (or >> moving acpi_dma_configure() to acpi_create_platform_device(), where the >> DMA masks are set-up by default by core ACPI. Mark, Suravee, what was >> the rationale behind calling arch_setup_dma_ops() in acpi_bind_one() ?) > >Alternatively, you can add one more arch wrapper that will be a no-op >on x86 and that will set up the default masks and call >arch_setup_dma_ops() on ARM. Then, you can invoke that from >acpi_dma_configure(). > >Or make the definition of acpi_dma_configure() itself depend on the >architecture. > So is it better that either removing the masks from acpi_dma_configure (or) creating the wrapper as Rafael mentioned, than moving acpi_dma_configure itself , because with something like iommu probe deferral that is tried, acpi_dma_configure is getting invoked from a device's really_probe, a different path again ? Regards, Sricharan