| Message ID | 20221004081723.31739-1-kabel@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7 | expand |
On Tue, Oct 04, 2022 at 10:17:23AM +0200, Marek Behún wrote: > Commit ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > caused a regression on the mvebu platform, wherein devices that are > dma-coherent are marked as dma-noncoherent, because although > mvebu_hwcc_notifier() after that commit still marks then as coherent, > the arm_coherent_dma_ops() function, which is called later, overwrites > this setting, since it is being called from drivers/of/device.c with > coherency parameter determined by of_dma_is_coherent(), and the > device-trees do not declare the 'dma-coherent' property. > > Fix this by defaulting to dma-coherent for this platform in the > of_dma_is_coherent() function, if neither dma-coherent nor > dma-noncoherent is declared. Can't mvebu be part of multi-platform builds that would be broken by this change? Also if we do this, shouldn't we also remove the notifier that set ->dma_coherent?
On Tue, Oct 4, 2022, at 10:17 AM, Marek Behún wrote: > Commit ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > caused a regression on the mvebu platform, wherein devices that are > dma-coherent are marked as dma-noncoherent, because although > mvebu_hwcc_notifier() after that commit still marks then as coherent, > the arm_coherent_dma_ops() function, which is called later, overwrites > this setting, since it is being called from drivers/of/device.c with > coherency parameter determined by of_dma_is_coherent(), and the > device-trees do not declare the 'dma-coherent' property. > > Fix this by defaulting to dma-coherent for this platform in the > of_dma_is_coherent() function, if neither dma-coherent nor > dma-noncoherent is declared. > > Fixes: ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > Link: https://lore.kernel.org/linux-arm-kernel/20220930151028.0e518421@dellmb/ > Signed-off-by: Marek Behún <kabel@kernel.org> > --- > arch/arm/mach-mvebu/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig > index 9f60a6fe0eaf..846a5c6e1a5e 100644 > --- a/arch/arm/mach-mvebu/Kconfig > +++ b/arch/arm/mach-mvebu/Kconfig > @@ -23,6 +23,7 @@ config MACH_MVEBU_V7 > select ARM_CPU_SUSPEND > select MACH_MVEBU_ANY > select MVEBU_CLK_COREDIV > + select OF_DMA_DEFAULT_COHERENT > This is clearly still broken, because doing this would mark all devices on all arm32 platforms as coherent whenever MACH_MVEBU_V7 is enabled. Arnd
On 04.10.22 10:17, Marek Behún wrote: > Commit ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > caused a regression on the mvebu platform, wherein devices that are > dma-coherent are marked as dma-noncoherent, because although > mvebu_hwcc_notifier() after that commit still marks then as coherent, > the arm_coherent_dma_ops() function, which is called later, overwrites > this setting, since it is being called from drivers/of/device.c with > coherency parameter determined by of_dma_is_coherent(), and the > device-trees do not declare the 'dma-coherent' property. > > Fix this by defaulting to dma-coherent for this platform in the > of_dma_is_coherent() function, if neither dma-coherent nor > dma-noncoherent is declared. > > Fixes: ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > Link: https://lore.kernel.org/linux-arm-kernel/20220930151028.0e518421@dellmb/ Thx for taking care of this. One quick note: You might want to add a "Cc: stable@..." on the patch to ensure it's backported in a timely manner: https://lore.kernel.org/lkml/YwZmu1ZTbjVqIY%2FC@kroah.com/ Ciao, Thorsten
On Tue, Oct 04, 2022 at 11:14:55AM +0200, Thorsten Leemhuis wrote: > On 04.10.22 10:17, Marek Behún wrote: > > Commit ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > > caused a regression on the mvebu platform, wherein devices that are > > dma-coherent are marked as dma-noncoherent, because although > > mvebu_hwcc_notifier() after that commit still marks then as coherent, > > the arm_coherent_dma_ops() function, which is called later, overwrites > > this setting, since it is being called from drivers/of/device.c with > > coherency parameter determined by of_dma_is_coherent(), and the > > device-trees do not declare the 'dma-coherent' property. > > > > Fix this by defaulting to dma-coherent for this platform in the > > of_dma_is_coherent() function, if neither dma-coherent nor > > dma-noncoherent is declared. > > > > Fixes: ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > > Link: https://lore.kernel.org/linux-arm-kernel/20220930151028.0e518421@dellmb/ > > Thx for taking care of this. One quick note: You might want to add a > "Cc: stable@..." on the patch to ensure it's backported in a timely manner: > https://lore.kernel.org/lkml/YwZmu1ZTbjVqIY%2FC@kroah.com/ Sadly, this isn't a valid fix for the problem - as Christoph points out, mvebu is part of a multiplatform kernel, and selecting options that harm other platforms on a multiplatform kernel can't be allowed even if it fixes a regression. It will cause a regression for other platforms. So, this needs fixing a different way, and there are other discussions concerning an alternative approach.
On Tue, 4 Oct 2022 10:30:10 +0200 Christoph Hellwig <hch@lst.de> wrote: > On Tue, Oct 04, 2022 at 10:17:23AM +0200, Marek Behún wrote: > > Commit ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") > > caused a regression on the mvebu platform, wherein devices that are > > dma-coherent are marked as dma-noncoherent, because although > > mvebu_hwcc_notifier() after that commit still marks then as coherent, > > the arm_coherent_dma_ops() function, which is called later, overwrites > > this setting, since it is being called from drivers/of/device.c with > > coherency parameter determined by of_dma_is_coherent(), and the > > device-trees do not declare the 'dma-coherent' property. > > > > Fix this by defaulting to dma-coherent for this platform in the > > of_dma_is_coherent() function, if neither dma-coherent nor > > dma-noncoherent is declared. > > Can't mvebu be part of multi-platform builds that would be broken by > this change? OK, if selecting that isn't possible, then my opinion is that this should be handled by OF drivers. There is a precedent for this, drivers/of/address.c has function of_empty_ranges_quirk(). So maybe something like of_dma_coherency_quirk(), which will be called from of_dma_is_coherent() ? > Also if we do this, shouldn't we also remove the notifier that set > ->dma_coherent? Yes, the notifier should go away, IMO.
diff --git a/arch/arm/mach-mvebu/Kconfig b/arch/arm/mach-mvebu/Kconfig index 9f60a6fe0eaf..846a5c6e1a5e 100644 --- a/arch/arm/mach-mvebu/Kconfig +++ b/arch/arm/mach-mvebu/Kconfig @@ -23,6 +23,7 @@ config MACH_MVEBU_V7 select ARM_CPU_SUSPEND select MACH_MVEBU_ANY select MVEBU_CLK_COREDIV + select OF_DMA_DEFAULT_COHERENT config MACH_ARMADA_370 bool "Marvell Armada 370 boards"
Commit ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") caused a regression on the mvebu platform, wherein devices that are dma-coherent are marked as dma-noncoherent, because although mvebu_hwcc_notifier() after that commit still marks then as coherent, the arm_coherent_dma_ops() function, which is called later, overwrites this setting, since it is being called from drivers/of/device.c with coherency parameter determined by of_dma_is_coherent(), and the device-trees do not declare the 'dma-coherent' property. Fix this by defaulting to dma-coherent for this platform in the of_dma_is_coherent() function, if neither dma-coherent nor dma-noncoherent is declared. Fixes: ae626eb97376 ("ARM/dma-mapping: use dma-direct unconditionally") Link: https://lore.kernel.org/linux-arm-kernel/20220930151028.0e518421@dellmb/ Signed-off-by: Marek Behún <kabel@kernel.org> --- arch/arm/mach-mvebu/Kconfig | 1 + 1 file changed, 1 insertion(+)