From mboxrd@z Thu Jan 1 00:00:00 1970 From: arnd@arndb.de (Arnd Bergmann) Date: Mon, 16 Nov 2015 20:57:41 +0100 Subject: [RFC] ARM64: simplify dma_get_ops In-Reply-To: <20151116183941.GF6556@e104818-lin.cambridge.arm.com> References: <4270550.cGd11OgA5n@wuerfel> <20151116183941.GF6556@e104818-lin.cambridge.arm.com> Message-ID: <5118146.8KqrJiJrDe@wuerfel> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Monday 16 November 2015 18:39:41 Catalin Marinas wrote: > On Mon, Nov 16, 2015 at 05:25:48PM +0100, Arnd Bergmann wrote: > > Including linux/acpi.h from asm/dma-mapping.h causes tons of compile-time > > warnings, e.g. > > > > drivers/isdn/mISDN/dsp_ecdis.h:43:0: warning: "FALSE" redefined > > drivers/isdn/mISDN/dsp_ecdis.h:44:0: warning: "TRUE" redefined > > drivers/net/fddi/skfp/h/targetos.h:62:0: warning: "TRUE" redefined > > drivers/net/fddi/skfp/h/targetos.h:63:0: warning: "FALSE" redefined > > > > However, it looks like the dependency should not even there as > > I do not see why __generic_dma_ops() cares about whether we have > > an ACPI based system or not. > > > > The current behavior is to fall back to the global dma_ops when > > a device has not set its own dma_ops, but only for DT based systems. > > This seems dangerous, as a random device might have different > > requirements regarding IOMMU or coherency, so we should really > > never have that fallback and just forbid DMA when we have not > > initialized DMA for a device. > > I think this is from the days when we didn't have an > arch_setup_dma_ops() to be called from the DT code. Makes sense. > I did placed a WARN_ON() to see who's getting the dummy_dma_ops and it > triggered this call trace: > > WARNING: at /work/Linux/linux-2.6-aarch64/arch/arm64/include/asm/dma-mapping.h:40 > Modules linked in: > > CPU: 3 PID: 1 Comm: swapper/0 Tainted: G W 4.4.0-rc1+ #601 > Hardware name: Juno (DT) > task: ffffffc9768a0000 ti: ffffffc9768a8000 task.ti: ffffffc9768a8000 > PC is at ohci_platform_probe+0x2b8/0x518 > LR is at ohci_platform_probe+0x2c/0x518 > Call trace: > [] ohci_platform_probe+0x2b8/0x518 > [] platform_drv_probe+0x54/0xb8 > [] driver_probe_device+0x1ec/0x2f0 > [] __driver_attach+0x9c/0xa0 > [] bus_for_each_dev+0x60/0xa0 > [] driver_attach+0x20/0x28 > [] bus_add_driver+0x1d0/0x238 > [] driver_register+0x60/0xf8 > [] __platform_driver_register+0x40/0x48 > [] ohci_platform_init+0x50/0x60 > [] do_one_initcall+0x90/0x1a0 > [] kernel_init_freeable+0x154/0x1f8 > [] kernel_init+0x10/0xe0 > [] ret_from_fork+0x10/0x40 > ohci-platform: probe of 7ffb0000.ohci failed with error -5 > > I need to check whether we break anything by no longer returning > swiotlb_dma_ops. Interesting. Note that this driver calls 'dma_coerce_mask_and_coherent()', which can be regarded a bug by itself, because it overrides the platform specific DMA mask. > > This removes the global dma_ops variable and the special-casing > > for ACPI, and just returns the dma ops that got set for the > > device, or the dummy_dma_ops if none were present. > > > > The original code has apparently been copied from arm32 where we > > rely on it for ISA devices things like the floppy controller, but > > we should have no such devices on ARM64. > > IIRC, this was required for DT before we had the arch_set_dma_ops() > (though I may be wrong, I haven't checked the logs). The dummy ops were > introduced with ACPI to avoid any default, I think this was related to > the _CCA property. Right. > > @@ -985,7 +977,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > > struct iommu_ops *iommu, bool coherent) > > { > > if (!acpi_disabled && !dev->archdata.dma_ops) > > - dev->archdata.dma_ops = dma_ops; > > + dev->archdata.dma_ops = &swiotlb_dma_ops; > > Why do we still keep the !acpi_disabled check here? If I remove it, the > WARN_ON() above disappears. Ah, good. That must be my mistake then. This looks much better. On a related note, we should also urgently fix the arch_setup_dma_ops() function to no longer ignore the base and size arguments. For dma_mase, we can simply WARN_ON(dma_base != 0), so we can implement support for that whenever we need it, but for the size we need to prevent drivers from calling dma_set_mask() with an argument larger than the size we pass in here, unless the size is also larger than max_pfn. Arnd From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752286AbbKPT6c (ORCPT ); Mon, 16 Nov 2015 14:58:32 -0500 Received: from mout.kundenserver.de ([212.227.126.135]:49983 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751139AbbKPT6a (ORCPT ); Mon, 16 Nov 2015 14:58:30 -0500 From: Arnd Bergmann To: Catalin Marinas Cc: Will Deacon , Suravee Suthikulpanit , "Rafael J. Wysocki" , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Mark Salter Subject: Re: [RFC] ARM64: simplify dma_get_ops Date: Mon, 16 Nov 2015 20:57:41 +0100 Message-ID: <5118146.8KqrJiJrDe@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <20151116183941.GF6556@e104818-lin.cambridge.arm.com> References: <4270550.cGd11OgA5n@wuerfel> <20151116183941.GF6556@e104818-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:akUOLTncSskpEcS4HA5dDnbKEUX2ObQwgbt2e8ijkcH8km465Oq LG0D8EABiJh5J5UEnOQYYet/ToPtvZAG+8vw9kHLYhB2UBz1M0++m8AFxA1gwW2Bhzaf43a tE2Ko4B0qdWR2zZrMRuKYgCLoxG+FT5ojsKxWcacoCmHqztcIeBKKLHOpxCxbA0qDtVUNLn a82lEKxwZDexi/FZ5H5Bg== X-UI-Out-Filterresults: notjunk:1;V01:K0:F0V6/og7Y3I=:oJneriEybiduYNh+b1sMGS //so5s7mlbvGfRLRNv3PoYjiRTEfJ3yPhfyPst+BnfXz2T8xQSUtC6eebXyhMImVblYjAIPiX iD+V3VEhYyMCO039xIpOh5LILFWGqG9VCDqQYuXrdaP7YJY113eON1FUwYs2YV1pvau5bryzd SVncXidhdVsCDIkzXu8Iqr+5LZhwCMktGro7CuedpACIudQCS1k1czyULSKRBjCIiY3ihAkgD DJFJVLSQzFdiEQKxMmeVef0Odu3dWXoDwo+Dr+N++LW6DLnomeXVhhYenAbXBe4Kjkqc0FXU2 QMQ7FPzGsKO8hFJO+PMDsSyzuU5Emz3cbvAy5SmfC2HLb333uDRWldsvfQnbr0smASX3V2lxA ES48Mc4qzmGdk1Itg0WhImeZb/mc1a9EWEfzVOwJYMoq7D+f5e6S5EWghOBGts3kWRxmw4hiZ y2VzER0XZxp8Knkf7NH1GFMA8ESS3nx/0sn5kgCx8AdPEAlNYmGIkSD43+J3QByuzx6yNqgX4 52fD+9oEDokl2iN7cbZdS905CuuYOJTeHrlkgK8uDDxDJ+6Zbw2doVgHFnrL3QCmWuZ+PbqPp 0arGeFKqWZO6v77v1jZmiE8TaMPHA9y1HMQYNYG2EbmkJtfagDai5YjDUI0rME6beegiNj81i fX4i7DAw4ylnQxn9KImGS7wJA1X2ek6gl1rZ/UpioKD3Is9HDSk06C+sAjPNEiuEB23d/8gQf Hy2ivh26cs6yV6yD Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday 16 November 2015 18:39:41 Catalin Marinas wrote: > On Mon, Nov 16, 2015 at 05:25:48PM +0100, Arnd Bergmann wrote: > > Including linux/acpi.h from asm/dma-mapping.h causes tons of compile-time > > warnings, e.g. > > > > drivers/isdn/mISDN/dsp_ecdis.h:43:0: warning: "FALSE" redefined > > drivers/isdn/mISDN/dsp_ecdis.h:44:0: warning: "TRUE" redefined > > drivers/net/fddi/skfp/h/targetos.h:62:0: warning: "TRUE" redefined > > drivers/net/fddi/skfp/h/targetos.h:63:0: warning: "FALSE" redefined > > > > However, it looks like the dependency should not even there as > > I do not see why __generic_dma_ops() cares about whether we have > > an ACPI based system or not. > > > > The current behavior is to fall back to the global dma_ops when > > a device has not set its own dma_ops, but only for DT based systems. > > This seems dangerous, as a random device might have different > > requirements regarding IOMMU or coherency, so we should really > > never have that fallback and just forbid DMA when we have not > > initialized DMA for a device. > > I think this is from the days when we didn't have an > arch_setup_dma_ops() to be called from the DT code. Makes sense. > I did placed a WARN_ON() to see who's getting the dummy_dma_ops and it > triggered this call trace: > > WARNING: at /work/Linux/linux-2.6-aarch64/arch/arm64/include/asm/dma-mapping.h:40 > Modules linked in: > > CPU: 3 PID: 1 Comm: swapper/0 Tainted: G W 4.4.0-rc1+ #601 > Hardware name: Juno (DT) > task: ffffffc9768a0000 ti: ffffffc9768a8000 task.ti: ffffffc9768a8000 > PC is at ohci_platform_probe+0x2b8/0x518 > LR is at ohci_platform_probe+0x2c/0x518 > Call trace: > [] ohci_platform_probe+0x2b8/0x518 > [] platform_drv_probe+0x54/0xb8 > [] driver_probe_device+0x1ec/0x2f0 > [] __driver_attach+0x9c/0xa0 > [] bus_for_each_dev+0x60/0xa0 > [] driver_attach+0x20/0x28 > [] bus_add_driver+0x1d0/0x238 > [] driver_register+0x60/0xf8 > [] __platform_driver_register+0x40/0x48 > [] ohci_platform_init+0x50/0x60 > [] do_one_initcall+0x90/0x1a0 > [] kernel_init_freeable+0x154/0x1f8 > [] kernel_init+0x10/0xe0 > [] ret_from_fork+0x10/0x40 > ohci-platform: probe of 7ffb0000.ohci failed with error -5 > > I need to check whether we break anything by no longer returning > swiotlb_dma_ops. Interesting. Note that this driver calls 'dma_coerce_mask_and_coherent()', which can be regarded a bug by itself, because it overrides the platform specific DMA mask. > > This removes the global dma_ops variable and the special-casing > > for ACPI, and just returns the dma ops that got set for the > > device, or the dummy_dma_ops if none were present. > > > > The original code has apparently been copied from arm32 where we > > rely on it for ISA devices things like the floppy controller, but > > we should have no such devices on ARM64. > > IIRC, this was required for DT before we had the arch_set_dma_ops() > (though I may be wrong, I haven't checked the logs). The dummy ops were > introduced with ACPI to avoid any default, I think this was related to > the _CCA property. Right. > > @@ -985,7 +977,7 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size, > > struct iommu_ops *iommu, bool coherent) > > { > > if (!acpi_disabled && !dev->archdata.dma_ops) > > - dev->archdata.dma_ops = dma_ops; > > + dev->archdata.dma_ops = &swiotlb_dma_ops; > > Why do we still keep the !acpi_disabled check here? If I remove it, the > WARN_ON() above disappears. Ah, good. That must be my mistake then. This looks much better. On a related note, we should also urgently fix the arch_setup_dma_ops() function to no longer ignore the base and size arguments. For dma_mase, we can simply WARN_ON(dma_base != 0), so we can implement support for that whenever we need it, but for the size we need to prevent drivers from calling dma_set_mask() with an argument larger than the size we pass in here, unless the size is also larger than max_pfn. Arnd