* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-03 21:30 ` Marcin Wojtas
@ 2022-10-03 21:35 ` Pali Rohár
2022-10-03 22:03 ` Marcin Wojtas
2022-10-04 7:10 ` Christoph Hellwig
` (2 subsequent siblings)
3 siblings, 1 reply; 43+ messages in thread
From: Pali Rohár @ 2022-10-03 21:35 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Marek Behún, Russell King (Oracle),
Christoph Hellwig, Robin Murphy, Arnd Bergmann, Andre Przywara,
Marc Zyngier, Linus Torvalds, Andrew Lunn, Gregory Clement,
Greg Kroah-Hartman, iommu, linux-arm-kernel
Hello!
On Monday 03 October 2022 23:30:31 Marcin Wojtas wrote:
> Hi Marek,
>
>
> pon., 3 paź 2022 o 17:33 Marek Behún <kabel@kernel.org> napisał(a):
> >
> > On Mon, 3 Oct 2022 15:11:44 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >
> > > On Mon, Oct 03, 2022 at 09:30:37AM +0200, Christoph Hellwig wrote:
> > > > On Fri, Sep 30, 2022 at 05:02:05PM +0200, Marek Behún wrote:
> > > > > It seems that the null pointer dereference comes from the data variable
> > > > > having zero value. We assign
> > > > > data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
> > > >
> > > > I never see any assignment to ->buf_cookie in the driver, what am
> > > > I missing?
> > >
> > > I think Marek's setup (like my setups) use the hardware buffer manager,
> > > and it's hardware that fills in the "buf_cookie", which is supposed to
> > > be the virtual address of the buffer.
> > >
> > > Each buffer supplied to the hardware buffer manager is supposed to
> > > contain the virtual address in the first 32-bit word in that buffer.
> > >
> > > This is done by mvneta_bm_construct():
> > >
> > > /* In order to update buf_cookie field of RX descriptor properly,
> > > * BM hardware expects buf virtual address to be placed in the
> > > * first four bytes of mapped buffer.
> > > */
> > > *(u32 *)buf = (u32)buf;
> > >
> > > immediately prior to dma_map_single(..., DMA_FROM_DEVICE) is called.
> > >
> > > If I had to guess, I would suggest that this write is being lost via
> > > cache invalidation, and given that the hardware BM both reads and
> > > writes this buffer, DMA_FROM_DEVICE is not correct, it should be
> > > DMA_BIDIRECTIONAL.
> > >
>
> I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> the driver - the CPU doesn't access the payload afterward. The BM only
> pushes the pointers back and forth between internal SRAM ('internal
> pool' - BPPI) to DRAM ('external pool' - BPPE) and the descriptors,
> but afaik it should not touch the buffer contents. But maybe somehow
> it affects the coherency and DMA_BIDIRECTIONAL are indeed required...
> About the coherency itself - please see my comment below.
>
> Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
Hm... When you are talking about IO coherency... This reminds me that
there is a HW bug on A38x with undocumented errata related to IO
coherency. It is not mentioned in the official A38x errata document but
kernel has implemented some kind of workaround for it. But last time
when I tried to understand it I had feeling that it was implemented
incorrectly and maybe does not do what it is expected to do?
Maybe it should be revisited, now?
> > > Changing that is probably going to need DMA_FROM_DEVICE also changed
> > > elsewhere in the mvneta_bm and mvneta driver.
> > >
> > > I'm not in a position where I could test that out. Marek?
> > >
> >
> > Hello Russell,
> >
> > thanks for your suggestion!
> >
> > Adding Pali, since he has some information (see at the end of this
> > message).
> >
> > The attached patch seems to solve the null-pointer dereference.
>
> Did you manage to measure performance impact?
>
> I have one overall concern here. On all kinds of A38x-based boards I
> worked on, by default, the firmware set all devices (e.g. network,
> AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> used). Can you please try adding 'dma-coherent;' property under the
> 'internal-regs' node?
>
> > I booted into single user mode and enabled eth2. Before it caused the
> > NULL pointer dereference after link got up, not it does not happen.
> >
> > But I am still encountering the freeze after booting into system.
> >
> > Maybe these are different bugs?
> >
> > I am thinking whether we don't need something similar like
> > 7bea67a99430 ("ARM: dts integrator: Fix DMA ranges")
> > also for mvebu.
> > I seem to remember Pali talking about how the ranges defined in some
> > upstream mvebu-tree, using MBUS_ID() macros, are incorrect.
> > Pali, what do you remember about this?
> >
>
> Afaik there should be no issues around MBUS configuration for non-PCIE devices.
>
> Best regards,
> Marcin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-03 21:35 ` Pali Rohár
@ 2022-10-03 22:03 ` Marcin Wojtas
0 siblings, 0 replies; 43+ messages in thread
From: Marcin Wojtas @ 2022-10-03 22:03 UTC (permalink / raw)
To: Pali Rohár
Cc: Marek Behún, Russell King (Oracle),
Christoph Hellwig, Robin Murphy, Arnd Bergmann, Andre Przywara,
Marc Zyngier, Linus Torvalds, Andrew Lunn, Gregory Clement,
Greg Kroah-Hartman, iommu, linux-arm-kernel
Hi,
pon., 3 paź 2022 o 23:35 Pali Rohár <pali@kernel.org> napisał(a):
>
> Hello!
>
> On Monday 03 October 2022 23:30:31 Marcin Wojtas wrote:
> > Hi Marek,
> >
> >
> > pon., 3 paź 2022 o 17:33 Marek Behún <kabel@kernel.org> napisał(a):
> > >
> > > On Mon, 3 Oct 2022 15:11:44 +0100
> > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > >
> > > > On Mon, Oct 03, 2022 at 09:30:37AM +0200, Christoph Hellwig wrote:
> > > > > On Fri, Sep 30, 2022 at 05:02:05PM +0200, Marek Behún wrote:
> > > > > > It seems that the null pointer dereference comes from the data variable
> > > > > > having zero value. We assign
> > > > > > data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
> > > > >
> > > > > I never see any assignment to ->buf_cookie in the driver, what am
> > > > > I missing?
> > > >
> > > > I think Marek's setup (like my setups) use the hardware buffer manager,
> > > > and it's hardware that fills in the "buf_cookie", which is supposed to
> > > > be the virtual address of the buffer.
> > > >
> > > > Each buffer supplied to the hardware buffer manager is supposed to
> > > > contain the virtual address in the first 32-bit word in that buffer.
> > > >
> > > > This is done by mvneta_bm_construct():
> > > >
> > > > /* In order to update buf_cookie field of RX descriptor properly,
> > > > * BM hardware expects buf virtual address to be placed in the
> > > > * first four bytes of mapped buffer.
> > > > */
> > > > *(u32 *)buf = (u32)buf;
> > > >
> > > > immediately prior to dma_map_single(..., DMA_FROM_DEVICE) is called.
> > > >
> > > > If I had to guess, I would suggest that this write is being lost via
> > > > cache invalidation, and given that the hardware BM both reads and
> > > > writes this buffer, DMA_FROM_DEVICE is not correct, it should be
> > > > DMA_BIDIRECTIONAL.
> > > >
> >
> > I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> > the driver - the CPU doesn't access the payload afterward. The BM only
> > pushes the pointers back and forth between internal SRAM ('internal
> > pool' - BPPI) to DRAM ('external pool' - BPPE) and the descriptors,
> > but afaik it should not touch the buffer contents. But maybe somehow
> > it affects the coherency and DMA_BIDIRECTIONAL are indeed required...
> > About the coherency itself - please see my comment below.
> >
> > Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
>
> Hm... When you are talking about IO coherency... This reminds me that
> there is a HW bug on A38x with undocumented errata related to IO
> coherency. It is not mentioned in the official A38x errata document but
> kernel has implemented some kind of workaround for it. But last time
> when I tried to understand it I had feeling that it was implemented
> incorrectly and maybe does not do what it is expected to do?
>
> Maybe it should be revisited, now?
It mentions problems with deadlocks when outer syncs are performed:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-mvebu/coherency.c?h=v6.0#n183
A couple of lines above __arm_ioremap_caller is overriden, but IMO
it's not related to what we see in our issue.
If that may help - despite lack of 'dma-coherent' property in DT, the
FreeBSD works with full IO cache-coherency (e.g. cache maintenance in
NETA drivers are effectively nop's) on Clearfog and Clearfog-like
devices in production. So it's definitely possible, and I'd like to
check that in Linux (I may also have a setup this week available).
Best regards,
Marcin
>
> > > > Changing that is probably going to need DMA_FROM_DEVICE also changed
> > > > elsewhere in the mvneta_bm and mvneta driver.
> > > >
> > > > I'm not in a position where I could test that out. Marek?
> > > >
> > >
> > > Hello Russell,
> > >
> > > thanks for your suggestion!
> > >
> > > Adding Pali, since he has some information (see at the end of this
> > > message).
> > >
> > > The attached patch seems to solve the null-pointer dereference.
> >
> > Did you manage to measure performance impact?
> >
> > I have one overall concern here. On all kinds of A38x-based boards I
> > worked on, by default, the firmware set all devices (e.g. network,
> > AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> > reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> > [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> > used). Can you please try adding 'dma-coherent;' property under the
> > 'internal-regs' node?
> >
> > > I booted into single user mode and enabled eth2. Before it caused the
> > > NULL pointer dereference after link got up, not it does not happen.
> > >
> > > But I am still encountering the freeze after booting into system.
> > >
> > > Maybe these are different bugs?
> > >
> > > I am thinking whether we don't need something similar like
> > > 7bea67a99430 ("ARM: dts integrator: Fix DMA ranges")
> > > also for mvebu.
> > > I seem to remember Pali talking about how the ranges defined in some
> > > upstream mvebu-tree, using MBUS_ID() macros, are incorrect.
> > > Pali, what do you remember about this?
> > >
> >
> > Afaik there should be no issues around MBUS configuration for non-PCIE devices.
> >
> > Best regards,
> > Marcin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-03 21:30 ` Marcin Wojtas
2022-10-03 21:35 ` Pali Rohár
@ 2022-10-04 7:10 ` Christoph Hellwig
2022-10-04 8:15 ` Marek Behún
2022-10-04 9:56 ` REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally" Robin Murphy
2022-10-04 7:25 ` Russell King (Oracle)
2022-10-04 8:26 ` Marek Behún
3 siblings, 2 replies; 43+ messages in thread
From: Christoph Hellwig @ 2022-10-04 7:10 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Marek Behún, Russell King (Oracle),
pali, Christoph Hellwig, Robin Murphy, Arnd Bergmann,
Andre Przywara, Marc Zyngier, Linus Torvalds, Andrew Lunn,
Gregory Clement, Greg Kroah-Hartman, iommu, linux-arm-kernel
On Mon, Oct 03, 2022 at 11:30:31PM +0200, Marcin Wojtas wrote:
> I have one overall concern here. On all kinds of A38x-based boards I
> worked on, by default, the firmware set all devices (e.g. network,
> AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> used). Can you please try adding 'dma-coherent;' property under the
> 'internal-regs' node?
Robin mentioned something similar earlier. This almost smalls like
we somehow manage to mark these device non-coherent by accident now.
The interesting part of the bisected commit is the change to
mvebu_hwcc_notifier that used to force the DMA OPS to
arm_coherent_dma_ops, but now just sets the ->dma_coherent flags,
which seems to get overriden somehow again. Maybe the notifier is
run before arch_setup_dma_ops, even if that seems odd? As that is
the only thing I could think of, maybe try this patch:
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index 089c9c644cce2..76789650e2596 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -1770,7 +1770,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
const struct iommu_ops *iommu, bool coherent)
{
dev->archdata.dma_coherent = coherent;
- dev->dma_coherent = coherent;
+
+ if (coherent)
+ dev->dma_coherent = true;
/*
* Don't override the dma_ops if they have already been set. Ideally
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 7:10 ` Christoph Hellwig
@ 2022-10-04 8:15 ` Marek Behún
2022-10-04 8:17 ` [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7 Marek Behún
2022-10-04 9:56 ` REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally" Robin Murphy
1 sibling, 1 reply; 43+ messages in thread
From: Marek Behún @ 2022-10-04 8:15 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Marcin Wojtas, Russell King (Oracle),
pali, Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
On Tue, 4 Oct 2022 09:10:00 +0200
Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Oct 03, 2022 at 11:30:31PM +0200, Marcin Wojtas wrote:
> > I have one overall concern here. On all kinds of A38x-based boards I
> > worked on, by default, the firmware set all devices (e.g. network,
> > AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> > reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> > [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> > used). Can you please try adding 'dma-coherent;' property under the
> > 'internal-regs' node?
>
> Robin mentioned something similar earlier. This almost smalls like
> we somehow manage to mark these device non-coherent by accident now.
>
> The interesting part of the bisected commit is the change to
> mvebu_hwcc_notifier that used to force the DMA OPS to
> arm_coherent_dma_ops, but now just sets the ->dma_coherent flags,
> which seems to get overriden somehow again. Maybe the notifier is
> run before arch_setup_dma_ops, even if that seems odd? As that is
> the only thing I could think of, maybe try this patch:
>
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 089c9c644cce2..76789650e2596 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1770,7 +1770,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> const struct iommu_ops *iommu, bool coherent)
> {
> dev->archdata.dma_coherent = coherent;
> - dev->dma_coherent = coherent;
> +
> + if (coherent)
> + dev->dma_coherent = true;
>
> /*
> * Don't override the dma_ops if they have already been set. Ideally
>
Indeed this fixes the issue, and indeed arch_setup_dma_ops is called
later than mvebu_hwcc_notifier.
bash-5.1# dmesg | grep -Ee '(mvebu_hwcc|arch_setup)' | grep ethernet
[ 0.009350] mvebu-coherency: mvebu_hwcc_notifier f1070000.ethernet
[ 0.009434] mvebu-coherency: mvebu_hwcc_notifier f1030000.ethernet
[ 0.009523] mvebu-coherency: mvebu_hwcc_notifier f1034000.ethernet
[ 1.859657] arch_setup_dma_ops f1070000.ethernet coherent=0
[ 1.874852] arch_setup_dma_ops f1030000.ethernet coherent=0
[ 1.889770] arch_setup_dma_ops f1034000.ethernet coherent=0
But mvebu_hwcc_notifier is called even for device not in internal-regs:\
bash-5.1# dmesg | grep -Ee '(mvebu_hwcc|arch_setup)'
[ 0.006475] mvebu-coherency: mvebu_hwcc_notifier pmu
[ 0.006536] mvebu-coherency: mvebu_hwcc_notifier soc
[ 0.007902] mvebu-coherency: mvebu_hwcc_notifier fff00000.bootrom
[ 0.007946] mvebu-coherency: mvebu_hwcc_notifier soc:internal-regs
...
[ 0.010790] mvebu-coherency: mvebu_hwcc_notifier soc:pcie
This probably means that to fix it, we just need to
select OF_DMA_DEFAULT_COHERENT
in
config MACH_MVEBU_V7
?
That way it will work with existing device-trees.
Marek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7
2022-10-04 8:15 ` Marek Behún
@ 2022-10-04 8:17 ` Marek Behún
2022-10-04 8:30 ` Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 43+ messages in thread
From: Marek Behún @ 2022-10-04 8:17 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Marek Behún, Marcin Wojtas, Russell King (Oracle),
pali, Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
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
config MACH_ARMADA_370
bool "Marvell Armada 370 boards"
--
2.35.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 43+ messages in thread* Re: [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7
2022-10-04 8:17 ` [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7 Marek Behún
@ 2022-10-04 8:30 ` Christoph Hellwig
2022-10-04 12:54 ` Marek Behún
2022-10-04 8:30 ` Arnd Bergmann
2022-10-04 9:14 ` Thorsten Leemhuis
2 siblings, 1 reply; 43+ messages in thread
From: Christoph Hellwig @ 2022-10-04 8:30 UTC (permalink / raw)
To: Marek Behún
Cc: Christoph Hellwig, Marcin Wojtas, Russell King (Oracle),
pali, Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
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?
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7
2022-10-04 8:30 ` Christoph Hellwig
@ 2022-10-04 12:54 ` Marek Behún
0 siblings, 0 replies; 43+ messages in thread
From: Marek Behún @ 2022-10-04 12:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Marcin Wojtas, Russell King (Oracle),
pali, Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7
2022-10-04 8:17 ` [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7 Marek Behún
2022-10-04 8:30 ` Christoph Hellwig
@ 2022-10-04 8:30 ` Arnd Bergmann
2022-10-04 9:14 ` Thorsten Leemhuis
2 siblings, 0 replies; 43+ messages in thread
From: Arnd Bergmann @ 2022-10-04 8:30 UTC (permalink / raw)
To: Marek Behún, Christoph Hellwig
Cc: Marcin Wojtas, Russell King, Pali Rohár, Robin Murphy,
Andre Przywara, Marc Zyngier, Linus Torvalds, Andrew Lunn,
Gregory Clement, Greg Kroah-Hartman, iommu, linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7
2022-10-04 8:17 ` [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7 Marek Behún
2022-10-04 8:30 ` Christoph Hellwig
2022-10-04 8:30 ` Arnd Bergmann
@ 2022-10-04 9:14 ` Thorsten Leemhuis
2022-10-04 9:22 ` Russell King (Oracle)
2 siblings, 1 reply; 43+ messages in thread
From: Thorsten Leemhuis @ 2022-10-04 9:14 UTC (permalink / raw)
To: Marek Behún, Christoph Hellwig
Cc: Marcin Wojtas, Russell King (Oracle),
pali, Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: [PATCH] ARM: mvebu: select OF_DMA_DEFAULT_COHERENT if MACH_MVEBU_V7
2022-10-04 9:14 ` Thorsten Leemhuis
@ 2022-10-04 9:22 ` Russell King (Oracle)
0 siblings, 0 replies; 43+ messages in thread
From: Russell King (Oracle) @ 2022-10-04 9:22 UTC (permalink / raw)
To: Thorsten Leemhuis
Cc: Marek Behún, Christoph Hellwig, Marcin Wojtas, pali,
Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 7:10 ` Christoph Hellwig
2022-10-04 8:15 ` Marek Behún
@ 2022-10-04 9:56 ` Robin Murphy
1 sibling, 0 replies; 43+ messages in thread
From: Robin Murphy @ 2022-10-04 9:56 UTC (permalink / raw)
To: Christoph Hellwig, Marcin Wojtas
Cc: Marek Behún, Russell King (Oracle),
pali, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
On 2022-10-04 08:10, Christoph Hellwig wrote:
> On Mon, Oct 03, 2022 at 11:30:31PM +0200, Marcin Wojtas wrote:
>> I have one overall concern here. On all kinds of A38x-based boards I
>> worked on, by default, the firmware set all devices (e.g. network,
>> AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
>> reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
>> [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
>> used). Can you please try adding 'dma-coherent;' property under the
>> 'internal-regs' node?
>
> Robin mentioned something similar earlier. This almost smalls like
> we somehow manage to mark these device non-coherent by accident now.
>
> The interesting part of the bisected commit is the change to
> mvebu_hwcc_notifier that used to force the DMA OPS to
> arm_coherent_dma_ops, but now just sets the ->dma_coherent flags,
> which seems to get overriden somehow again. Maybe the notifier is
> run before arch_setup_dma_ops, even if that seems odd? As that is
> the only thing I could think of, maybe try this patch:
>
>
> diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
> index 089c9c644cce2..76789650e2596 100644
> --- a/arch/arm/mm/dma-mapping.c
> +++ b/arch/arm/mm/dma-mapping.c
> @@ -1770,7 +1770,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
> const struct iommu_ops *iommu, bool coherent)
> {
> dev->archdata.dma_coherent = coherent;
> - dev->dma_coherent = coherent;
> +
> + if (coherent)
> + dev->dma_coherent = true;
>
> /*
> * Don't override the dma_ops if they have already been set. Ideally
>
That seems reasonable to me - it preserves the equivalent behaviour of
respecting platform overrides, which thankfully only go one way. In
terms of the intent it might be more properly phrased as "if
(!dev->dma_coherent) dev->dma_coherent = coherent" but whether that's
worth the extra characters is debatable.
The only other possibility might be to change mvebu_hwcc_notifier() to
apply its override at the BUS_NOTIFY_BIND_DRIVER stage, after
dma_configure() has run. The other notifier for highbank looks
unaffected since that one's just following whatever the DT says rather
than trying to override anything.
Thanks,
Robin.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-03 21:30 ` Marcin Wojtas
2022-10-03 21:35 ` Pali Rohár
2022-10-04 7:10 ` Christoph Hellwig
@ 2022-10-04 7:25 ` Russell King (Oracle)
2022-10-04 8:30 ` Marcin Wojtas
2022-10-04 8:26 ` Marek Behún
3 siblings, 1 reply; 43+ messages in thread
From: Russell King (Oracle) @ 2022-10-04 7:25 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Marek Behún, pali, Christoph Hellwig, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Mon, Oct 03, 2022 at 11:30:31PM +0200, Marcin Wojtas wrote:
> I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> the driver - the CPU doesn't access the payload afterward.
Please look at the bigger picture rather than concentrating on what
happens when a packet is received.
The issue is that the CPU writes to the buffer prior to handing the
buffer over to the hardware, and then the BM reads from the buffer.
This is DMA _to_ the device no matter how you look at it.
The BM later writes the received packet to the buffer. This is DMA
_from_ the device.
So, we have DMA in both directions, hence it really is bidirectional,
and using DMA_FROM_DEVICE in the RX path _is_ incorrect.
Architectures _can_ and _do_ invalidate the data cache when mapping a
buffer for DMA_FROM_DEVICE and any writes in the data cache for that
buffer will be discarded. If the writes don't hit the data cache, then
they will be unaffected by this.
IMHO, having read the docs on the buffer manager, the use of
DMA_FROM_DEVICE in mvneta in this path is a long-standing bug - it
should be bidirectional for the reason I state above - the hardware
both reads and writes the buffer that is passed to it, expecting to
read data written by the CPU initially.
> Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
That will make no difference to this - this is not about barriers, it's
about caches.
> I have one overall concern here. On all kinds of A38x-based boards I
> worked on, by default, the firmware set all devices (e.g. network,
> AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> used). Can you please try adding 'dma-coherent;' property under the
> 'internal-regs' node?
I did notice that there was no dma-coherent markings in DT, which means
that the DMA API will assume the device is non-coherent, and cache
maintenance will be performed. If it is dma-coherent, then cache
maintenance won't be performed, and DT needs to be updated to indicate
this.
If firmware is making the devices DMA coherent, and it's under firmware
control, then shouldn't firmware also be updating the kernel's device
tree to indicate how it's configured the hardware coherency?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 7:25 ` Russell King (Oracle)
@ 2022-10-04 8:30 ` Marcin Wojtas
2022-10-04 9:08 ` Russell King (Oracle)
0 siblings, 1 reply; 43+ messages in thread
From: Marcin Wojtas @ 2022-10-04 8:30 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Marek Behún, pali, Christoph Hellwig, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
wt., 4 paź 2022 o 09:25 Russell King (Oracle) <linux@armlinux.org.uk>
napisał(a):
>
> On Mon, Oct 03, 2022 at 11:30:31PM +0200, Marcin Wojtas wrote:
> > I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> > the driver - the CPU doesn't access the payload afterward.
>
> Please look at the bigger picture rather than concentrating on what
> happens when a packet is received.
>
> The issue is that the CPU writes to the buffer prior to handing the
> buffer over to the hardware, and then the BM reads from the buffer.
> This is DMA _to_ the device no matter how you look at it.
>
> The BM later writes the received packet to the buffer. This is DMA
> _from_ the device.
>
> So, we have DMA in both directions, hence it really is bidirectional,
> and using DMA_FROM_DEVICE in the RX path _is_ incorrect.
>
> Architectures _can_ and _do_ invalidate the data cache when mapping a
> buffer for DMA_FROM_DEVICE and any writes in the data cache for that
> buffer will be discarded. If the writes don't hit the data cache, then
> they will be unaffected by this.
>
> IMHO, having read the docs on the buffer manager, the use of
> DMA_FROM_DEVICE in mvneta in this path is a long-standing bug - it
> should be bidirectional for the reason I state above - the hardware
> both reads and writes the buffer that is passed to it, expecting to
> read data written by the CPU initially.
Thanks for the explanation and I agree with your reasoning. Therefore
the below should be sufficient if we use HW BM and non-coherent
setting:
--- a/drivers/net/ethernet/marvell/mvneta_bm.c
+++ b/drivers/net/ethernet/marvell/mvneta_bm.c
@@ -103,7 +103,7 @@ int mvneta_bm_construct(struct hwbm_pool
*hwbm_pool, void *buf)
*/
*(u32 *)buf = (u32)buf;
phys_addr = dma_map_single(&priv->pdev->dev, buf, bm_pool->buf_size,
- DMA_FROM_DEVICE);
+ DMA_BIDIRECTIONAL);
if (unlikely(dma_mapping_error(&priv->pdev->dev, phys_addr)))
return -ENOMEM;
Marek - can you please confirm that?
>
> > Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
>
> That will make no difference to this - this is not about barriers, it's
> about caches.
>
Sure.
> > I have one overall concern here. On all kinds of A38x-based boards I
> > worked on, by default, the firmware set all devices (e.g. network,
> > AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> > reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> > [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> > used). Can you please try adding 'dma-coherent;' property under the
> > 'internal-regs' node?
>
> I did notice that there was no dma-coherent markings in DT, which means
> that the DMA API will assume the device is non-coherent, and cache
> maintenance will be performed. If it is dma-coherent, then cache
> maintenance won't be performed, and DT needs to be updated to indicate
> this.
>
> If firmware is making the devices DMA coherent, and it's under firmware
> control, then shouldn't firmware also be updating the kernel's device
> tree to indicate how it's configured the hardware coherency?
>
Imo there are too many boxes out there and updating firmware in the
field is rather no-go. We already handle this in kernel / DT in big
extent, so I think we should stick to that path.
Thanks,
Marcin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 8:30 ` Marcin Wojtas
@ 2022-10-04 9:08 ` Russell King (Oracle)
2022-10-04 12:36 ` Marek Behún
0 siblings, 1 reply; 43+ messages in thread
From: Russell King (Oracle) @ 2022-10-04 9:08 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Marek Behún, pali, Christoph Hellwig, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Tue, Oct 04, 2022 at 10:30:40AM +0200, Marcin Wojtas wrote:
> Thanks for the explanation and I agree with your reasoning. Therefore
> the below should be sufficient if we use HW BM and non-coherent
> setting:
>
> --- a/drivers/net/ethernet/marvell/mvneta_bm.c
> +++ b/drivers/net/ethernet/marvell/mvneta_bm.c
> @@ -103,7 +103,7 @@ int mvneta_bm_construct(struct hwbm_pool
> *hwbm_pool, void *buf)
> */
> *(u32 *)buf = (u32)buf;
> phys_addr = dma_map_single(&priv->pdev->dev, buf, bm_pool->buf_size,
> - DMA_FROM_DEVICE);
> + DMA_BIDIRECTIONAL);
> if (unlikely(dma_mapping_error(&priv->pdev->dev, phys_addr)))
> return -ENOMEM;
>
> Marek - can you please confirm that?
This is insufficient. Marek's patch is the correct version.
The DMA API requires that the direction argument is the same for
mapping, unmapping and syncing a region.
> > I did notice that there was no dma-coherent markings in DT, which means
> > that the DMA API will assume the device is non-coherent, and cache
> > maintenance will be performed. If it is dma-coherent, then cache
> > maintenance won't be performed, and DT needs to be updated to indicate
> > this.
> >
> > If firmware is making the devices DMA coherent, and it's under firmware
> > control, then shouldn't firmware also be updating the kernel's device
> > tree to indicate how it's configured the hardware coherency?
> >
>
> Imo there are too many boxes out there and updating firmware in the
> field is rather no-go. We already handle this in kernel / DT in big
> extent, so I think we should stick to that path.
Seemingly, we don't handle it "well enough" in the kernel, since with
Christoph's change, we end up with the devices being non-coherent.
Yes, the kernel needs fixing to work with older firmware, but I think
firmware should also be fixed.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 9:08 ` Russell King (Oracle)
@ 2022-10-04 12:36 ` Marek Behún
2022-10-04 12:59 ` Marcin Wojtas
0 siblings, 1 reply; 43+ messages in thread
From: Marek Behún @ 2022-10-04 12:36 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Marcin Wojtas, pali, Christoph Hellwig, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Tue, 4 Oct 2022 10:08:15 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Tue, Oct 04, 2022 at 10:30:40AM +0200, Marcin Wojtas wrote:
> > Thanks for the explanation and I agree with your reasoning. Therefore
> > the below should be sufficient if we use HW BM and non-coherent
> > setting:
> >
> > --- a/drivers/net/ethernet/marvell/mvneta_bm.c
> > +++ b/drivers/net/ethernet/marvell/mvneta_bm.c
> > @@ -103,7 +103,7 @@ int mvneta_bm_construct(struct hwbm_pool
> > *hwbm_pool, void *buf)
> > */
> > *(u32 *)buf = (u32)buf;
> > phys_addr = dma_map_single(&priv->pdev->dev, buf, bm_pool->buf_size,
> > - DMA_FROM_DEVICE);
> > + DMA_BIDIRECTIONAL);
> > if (unlikely(dma_mapping_error(&priv->pdev->dev, phys_addr)))
> > return -ENOMEM;
> >
> > Marek - can you please confirm that?
>
> This is insufficient. Marek's patch is the correct version.
>
> The DMA API requires that the direction argument is the same for
> mapping, unmapping and syncing a region.
And now I am wondering about whether this didn't also cause the buffer
manager not working on Armada 3720. Marvell just gave an erratum that
HWBM is broken on 3720, but maybe they didn't notice this and just gave
up.
Also mvneta works only with one CPU on 3720 (see usage of
pp->neta_armada3700 in mvneta.c), which I also still don't know why,
and maybe it is related to this.
Marek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 12:36 ` Marek Behún
@ 2022-10-04 12:59 ` Marcin Wojtas
2022-10-04 18:51 ` Pali Rohár
0 siblings, 1 reply; 43+ messages in thread
From: Marcin Wojtas @ 2022-10-04 12:59 UTC (permalink / raw)
To: Marek Behún
Cc: Russell King (Oracle),
pali, Christoph Hellwig, Robin Murphy, Arnd Bergmann,
Andre Przywara, Marc Zyngier, Linus Torvalds, Andrew Lunn,
Gregory Clement, Greg Kroah-Hartman, iommu, linux-arm-kernel
wt., 4 paź 2022 o 14:37 Marek Behún <kabel@kernel.org> napisał(a):
>
> On Tue, 4 Oct 2022 10:08:15 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
>
> > On Tue, Oct 04, 2022 at 10:30:40AM +0200, Marcin Wojtas wrote:
> > > Thanks for the explanation and I agree with your reasoning. Therefore
> > > the below should be sufficient if we use HW BM and non-coherent
> > > setting:
> > >
> > > --- a/drivers/net/ethernet/marvell/mvneta_bm.c
> > > +++ b/drivers/net/ethernet/marvell/mvneta_bm.c
> > > @@ -103,7 +103,7 @@ int mvneta_bm_construct(struct hwbm_pool
> > > *hwbm_pool, void *buf)
> > > */
> > > *(u32 *)buf = (u32)buf;
> > > phys_addr = dma_map_single(&priv->pdev->dev, buf, bm_pool->buf_size,
> > > - DMA_FROM_DEVICE);
> > > + DMA_BIDIRECTIONAL);
> > > if (unlikely(dma_mapping_error(&priv->pdev->dev, phys_addr)))
> > > return -ENOMEM;
> > >
> > > Marek - can you please confirm that?
> >
> > This is insufficient. Marek's patch is the correct version.
> >
> > The DMA API requires that the direction argument is the same for
> > mapping, unmapping and syncing a region.
>
> And now I am wondering about whether this didn't also cause the buffer
> manager not working on Armada 3720. Marvell just gave an erratum that
> HWBM is broken on 3720, but maybe they didn't notice this and just gave
> up.
I implemented inital version of this support, which was never
published > 5y ago (I checked my oldest repos, but the code is gone
unfortunately). I don't recall exact HW issue justification - I
remember it was not related to coherency though, but the decision was
to drop it.
>
> Also mvneta works only with one CPU on 3720 (see usage of
> pp->neta_armada3700 in mvneta.c), which I also still don't know why,
> and maybe it is related to this.
>
The single-core processing is result of the problems with per-CPU IRQ
routing for the NETA controllers (never to be fixed eventually), not
related to coherency whatsoever.
Best regards,
Marcin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 12:59 ` Marcin Wojtas
@ 2022-10-04 18:51 ` Pali Rohár
2022-10-04 19:35 ` Marcin Wojtas
0 siblings, 1 reply; 43+ messages in thread
From: Pali Rohár @ 2022-10-04 18:51 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Marek Behún, Russell King (Oracle),
Christoph Hellwig, Robin Murphy, Arnd Bergmann, Andre Przywara,
Marc Zyngier, Linus Torvalds, Andrew Lunn, Gregory Clement,
Greg Kroah-Hartman, iommu, linux-arm-kernel
On Tuesday 04 October 2022 14:59:29 Marcin Wojtas wrote:
> wt., 4 paź 2022 o 14:37 Marek Behún <kabel@kernel.org> napisał(a):
> > And now I am wondering about whether this didn't also cause the buffer
> > manager not working on Armada 3720. Marvell just gave an erratum that
> > HWBM is broken on 3720, but maybe they didn't notice this and just gave
> > up.
>
> I implemented inital version of this support, which was never
> published > 5y ago (I checked my oldest repos, but the code is gone
> unfortunately). I don't recall exact HW issue justification - I
> remember it was not related to coherency though, but the decision was
> to drop it.
>
> >
> > Also mvneta works only with one CPU on 3720 (see usage of
> > pp->neta_armada3700 in mvneta.c), which I also still don't know why,
> > and maybe it is related to this.
> >
>
> The single-core processing is result of the problems with per-CPU IRQ
> routing for the NETA controllers (never to be fixed eventually), not
> related to coherency whatsoever.
>
> Best regards,
> Marcin
FYI this is described in A3720 errata document.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 18:51 ` Pali Rohár
@ 2022-10-04 19:35 ` Marcin Wojtas
0 siblings, 0 replies; 43+ messages in thread
From: Marcin Wojtas @ 2022-10-04 19:35 UTC (permalink / raw)
To: Pali Rohár
Cc: Marek Behún, Russell King (Oracle),
Christoph Hellwig, Robin Murphy, Arnd Bergmann, Andre Przywara,
Marc Zyngier, Linus Torvalds, Andrew Lunn, Gregory Clement,
Greg Kroah-Hartman, iommu, linux-arm-kernel
wt., 4 paź 2022 o 20:51 Pali Rohár <pali@kernel.org> napisał(a):
>
> On Tuesday 04 October 2022 14:59:29 Marcin Wojtas wrote:
> > wt., 4 paź 2022 o 14:37 Marek Behún <kabel@kernel.org> napisał(a):
> > > And now I am wondering about whether this didn't also cause the buffer
> > > manager not working on Armada 3720. Marvell just gave an erratum that
> > > HWBM is broken on 3720, but maybe they didn't notice this and just gave
> > > up.
> >
> > I implemented inital version of this support, which was never
> > published > 5y ago (I checked my oldest repos, but the code is gone
> > unfortunately). I don't recall exact HW issue justification - I
> > remember it was not related to coherency though, but the decision was
> > to drop it.
> >
> > >
> > > Also mvneta works only with one CPU on 3720 (see usage of
> > > pp->neta_armada3700 in mvneta.c), which I also still don't know why,
> > > and maybe it is related to this.
> > >
> >
> > The single-core processing is result of the problems with per-CPU IRQ
> > routing for the NETA controllers (never to be fixed eventually), not
> > related to coherency whatsoever.
> >
> > Best regards,
> > Marcin
>
> FYI this is described in A3720 errata document.
Thanks. I don't have this doc, I have to trust my long-term memory :)
Best regards,
Marcin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-03 21:30 ` Marcin Wojtas
` (2 preceding siblings ...)
2022-10-04 7:25 ` Russell King (Oracle)
@ 2022-10-04 8:26 ` Marek Behún
2022-10-04 8:36 ` Marcin Wojtas
3 siblings, 1 reply; 43+ messages in thread
From: Marek Behún @ 2022-10-04 8:26 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Russell King (Oracle),
pali, Christoph Hellwig, Robin Murphy, Arnd Bergmann,
Andre Przywara, Marc Zyngier, Linus Torvalds, Andrew Lunn,
Gregory Clement, Greg Kroah-Hartman, iommu, linux-arm-kernel
On Mon, 3 Oct 2022 23:30:31 +0200
Marcin Wojtas <mw@semihalf.com> wrote:
> Hi Marek,
>
>
> pon., 3 paź 2022 o 17:33 Marek Behún <kabel@kernel.org> napisał(a):
> >
> > On Mon, 3 Oct 2022 15:11:44 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >
> > > On Mon, Oct 03, 2022 at 09:30:37AM +0200, Christoph Hellwig wrote:
> > > > On Fri, Sep 30, 2022 at 05:02:05PM +0200, Marek Behún wrote:
> > > > > It seems that the null pointer dereference comes from the data variable
> > > > > having zero value. We assign
> > > > > data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
> > > >
> > > > I never see any assignment to ->buf_cookie in the driver, what am
> > > > I missing?
> > >
> > > I think Marek's setup (like my setups) use the hardware buffer manager,
> > > and it's hardware that fills in the "buf_cookie", which is supposed to
> > > be the virtual address of the buffer.
> > >
> > > Each buffer supplied to the hardware buffer manager is supposed to
> > > contain the virtual address in the first 32-bit word in that buffer.
> > >
> > > This is done by mvneta_bm_construct():
> > >
> > > /* In order to update buf_cookie field of RX descriptor properly,
> > > * BM hardware expects buf virtual address to be placed in the
> > > * first four bytes of mapped buffer.
> > > */
> > > *(u32 *)buf = (u32)buf;
> > >
> > > immediately prior to dma_map_single(..., DMA_FROM_DEVICE) is called.
> > >
> > > If I had to guess, I would suggest that this write is being lost via
> > > cache invalidation, and given that the hardware BM both reads and
> > > writes this buffer, DMA_FROM_DEVICE is not correct, it should be
> > > DMA_BIDIRECTIONAL.
> > >
>
> I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> the driver - the CPU doesn't access the payload afterward. The BM only
> pushes the pointers back and forth between internal SRAM ('internal
> pool' - BPPI) to DRAM ('external pool' - BPPE) and the descriptors,
> but afaik it should not touch the buffer contents. But maybe somehow
> it affects the coherency and DMA_BIDIRECTIONAL are indeed required...
> About the coherency itself - please see my comment below.
>
> Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
>
> > > Changing that is probably going to need DMA_FROM_DEVICE also changed
> > > elsewhere in the mvneta_bm and mvneta driver.
> > >
> > > I'm not in a position where I could test that out. Marek?
> > >
> >
> > Hello Russell,
> >
> > thanks for your suggestion!
> >
> > Adding Pali, since he has some information (see at the end of this
> > message).
> >
> > The attached patch seems to solve the null-pointer dereference.
>
> Did you manage to measure performance impact?
I did not measure any performance impacts. But DMA directianlity
within mvneta seems to be a different bug, as Russell replied, and maybe
was just revealed by this.
> I have one overall concern here. On all kinds of A38x-based boards I
> worked on, by default, the firmware set all devices (e.g. network,
> AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> used). Can you please try adding 'dma-coherent;' property under the
> 'internal-regs' node?
Yes, adding dma-coherent solves this issue. See other emails. The
proper solution IMO is to default to dma-coherent on the platform,
which can be done in a simple way (I've sent a patch). We want to be
compatible with older device-trees.
Marek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 8:26 ` Marek Behún
@ 2022-10-04 8:36 ` Marcin Wojtas
2022-10-20 18:22 ` Russell King (Oracle)
0 siblings, 1 reply; 43+ messages in thread
From: Marcin Wojtas @ 2022-10-04 8:36 UTC (permalink / raw)
To: Marek Behún
Cc: Russell King (Oracle),
pali, Christoph Hellwig, Robin Murphy, Arnd Bergmann,
Andre Przywara, Marc Zyngier, Linus Torvalds, Andrew Lunn,
Gregory Clement, Greg Kroah-Hartman, iommu, linux-arm-kernel
wt., 4 paź 2022 o 10:26 Marek Behún <kabel@kernel.org> napisał(a):
>
> On Mon, 3 Oct 2022 23:30:31 +0200
> Marcin Wojtas <mw@semihalf.com> wrote:
>
> > Hi Marek,
> >
> >
> > pon., 3 paź 2022 o 17:33 Marek Behún <kabel@kernel.org> napisał(a):
> > >
> > > On Mon, 3 Oct 2022 15:11:44 +0100
> > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > >
> > > > On Mon, Oct 03, 2022 at 09:30:37AM +0200, Christoph Hellwig wrote:
> > > > > On Fri, Sep 30, 2022 at 05:02:05PM +0200, Marek Behún wrote:
> > > > > > It seems that the null pointer dereference comes from the data variable
> > > > > > having zero value. We assign
> > > > > > data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
> > > > >
> > > > > I never see any assignment to ->buf_cookie in the driver, what am
> > > > > I missing?
> > > >
> > > > I think Marek's setup (like my setups) use the hardware buffer manager,
> > > > and it's hardware that fills in the "buf_cookie", which is supposed to
> > > > be the virtual address of the buffer.
> > > >
> > > > Each buffer supplied to the hardware buffer manager is supposed to
> > > > contain the virtual address in the first 32-bit word in that buffer.
> > > >
> > > > This is done by mvneta_bm_construct():
> > > >
> > > > /* In order to update buf_cookie field of RX descriptor properly,
> > > > * BM hardware expects buf virtual address to be placed in the
> > > > * first four bytes of mapped buffer.
> > > > */
> > > > *(u32 *)buf = (u32)buf;
> > > >
> > > > immediately prior to dma_map_single(..., DMA_FROM_DEVICE) is called.
> > > >
> > > > If I had to guess, I would suggest that this write is being lost via
> > > > cache invalidation, and given that the hardware BM both reads and
> > > > writes this buffer, DMA_FROM_DEVICE is not correct, it should be
> > > > DMA_BIDIRECTIONAL.
> > > >
> >
> > I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> > the driver - the CPU doesn't access the payload afterward. The BM only
> > pushes the pointers back and forth between internal SRAM ('internal
> > pool' - BPPI) to DRAM ('external pool' - BPPE) and the descriptors,
> > but afaik it should not touch the buffer contents. But maybe somehow
> > it affects the coherency and DMA_BIDIRECTIONAL are indeed required...
> > About the coherency itself - please see my comment below.
> >
> > Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
> >
> > > > Changing that is probably going to need DMA_FROM_DEVICE also changed
> > > > elsewhere in the mvneta_bm and mvneta driver.
> > > >
> > > > I'm not in a position where I could test that out. Marek?
> > > >
> > >
> > > Hello Russell,
> > >
> > > thanks for your suggestion!
> > >
> > > Adding Pali, since he has some information (see at the end of this
> > > message).
> > >
> > > The attached patch seems to solve the null-pointer dereference.
> >
> > Did you manage to measure performance impact?
>
> I did not measure any performance impacts. But DMA directianlity
> within mvneta seems to be a different bug, as Russell replied, and maybe
> was just revealed by this.
>
> > I have one overall concern here. On all kinds of A38x-based boards I
> > worked on, by default, the firmware set all devices (e.g. network,
> > AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> > reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> > [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> > used). Can you please try adding 'dma-coherent;' property under the
> > 'internal-regs' node?
>
> Yes, adding dma-coherent solves this issue. See other emails. The
> proper solution IMO is to default to dma-coherent on the platform,
> which can be done in a simple way (I've sent a patch). We want to be
> compatible with older device-trees.
>
Thanks a lot for testing. I agree we must maintain the backward
compatibility with older DT's. To summarize, I think we should end up
with 3 patches:
1. Update arch/arm/mm/dma-mapping.c as suggested by Christoph.
2. Add 'dma-coherent' in armada-38x.dtsi.
3. Fix DMA attribute in mvneta_bm_construct().
In case any help is needed from my side, please let me know.
Best regards,
Marcin
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-04 8:36 ` Marcin Wojtas
@ 2022-10-20 18:22 ` Russell King (Oracle)
2022-10-20 19:10 ` Marek Behún
` (3 more replies)
0 siblings, 4 replies; 43+ messages in thread
From: Russell King (Oracle) @ 2022-10-20 18:22 UTC (permalink / raw)
To: Marcin Wojtas
Cc: Marek Behún, pali, Christoph Hellwig, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Tue, Oct 04, 2022 at 10:36:05AM +0200, Marcin Wojtas wrote:
> wt., 4 paź 2022 o 10:26 Marek Behún <kabel@kernel.org> napisał(a):
> >
> > On Mon, 3 Oct 2022 23:30:31 +0200
> > Marcin Wojtas <mw@semihalf.com> wrote:
> >
> > > Hi Marek,
> > >
> > >
> > > pon., 3 paź 2022 o 17:33 Marek Behún <kabel@kernel.org> napisał(a):
> > > >
> > > > On Mon, 3 Oct 2022 15:11:44 +0100
> > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > >
> > > > > On Mon, Oct 03, 2022 at 09:30:37AM +0200, Christoph Hellwig wrote:
> > > > > > On Fri, Sep 30, 2022 at 05:02:05PM +0200, Marek Behún wrote:
> > > > > > > It seems that the null pointer dereference comes from the data variable
> > > > > > > having zero value. We assign
> > > > > > > data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
> > > > > >
> > > > > > I never see any assignment to ->buf_cookie in the driver, what am
> > > > > > I missing?
> > > > >
> > > > > I think Marek's setup (like my setups) use the hardware buffer manager,
> > > > > and it's hardware that fills in the "buf_cookie", which is supposed to
> > > > > be the virtual address of the buffer.
> > > > >
> > > > > Each buffer supplied to the hardware buffer manager is supposed to
> > > > > contain the virtual address in the first 32-bit word in that buffer.
> > > > >
> > > > > This is done by mvneta_bm_construct():
> > > > >
> > > > > /* In order to update buf_cookie field of RX descriptor properly,
> > > > > * BM hardware expects buf virtual address to be placed in the
> > > > > * first four bytes of mapped buffer.
> > > > > */
> > > > > *(u32 *)buf = (u32)buf;
> > > > >
> > > > > immediately prior to dma_map_single(..., DMA_FROM_DEVICE) is called.
> > > > >
> > > > > If I had to guess, I would suggest that this write is being lost via
> > > > > cache invalidation, and given that the hardware BM both reads and
> > > > > writes this buffer, DMA_FROM_DEVICE is not correct, it should be
> > > > > DMA_BIDIRECTIONAL.
> > > > >
> > >
> > > I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> > > the driver - the CPU doesn't access the payload afterward. The BM only
> > > pushes the pointers back and forth between internal SRAM ('internal
> > > pool' - BPPI) to DRAM ('external pool' - BPPE) and the descriptors,
> > > but afaik it should not touch the buffer contents. But maybe somehow
> > > it affects the coherency and DMA_BIDIRECTIONAL are indeed required...
> > > About the coherency itself - please see my comment below.
> > >
> > > Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
> > >
> > > > > Changing that is probably going to need DMA_FROM_DEVICE also changed
> > > > > elsewhere in the mvneta_bm and mvneta driver.
> > > > >
> > > > > I'm not in a position where I could test that out. Marek?
> > > > >
> > > >
> > > > Hello Russell,
> > > >
> > > > thanks for your suggestion!
> > > >
> > > > Adding Pali, since he has some information (see at the end of this
> > > > message).
> > > >
> > > > The attached patch seems to solve the null-pointer dereference.
> > >
> > > Did you manage to measure performance impact?
> >
> > I did not measure any performance impacts. But DMA directianlity
> > within mvneta seems to be a different bug, as Russell replied, and maybe
> > was just revealed by this.
> >
> > > I have one overall concern here. On all kinds of A38x-based boards I
> > > worked on, by default, the firmware set all devices (e.g. network,
> > > AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> > > reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> > > [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> > > used). Can you please try adding 'dma-coherent;' property under the
> > > 'internal-regs' node?
> >
> > Yes, adding dma-coherent solves this issue. See other emails. The
> > proper solution IMO is to default to dma-coherent on the platform,
> > which can be done in a simple way (I've sent a patch). We want to be
> > compatible with older device-trees.
> >
>
> Thanks a lot for testing. I agree we must maintain the backward
> compatibility with older DT's. To summarize, I think we should end up
> with 3 patches:
> 1. Update arch/arm/mm/dma-mapping.c as suggested by Christoph.
> 2. Add 'dma-coherent' in armada-38x.dtsi.
> 3. Fix DMA attribute in mvneta_bm_construct().
>
> In case any help is needed from my side, please let me know.
Is it possible that this would also cause data corruption reading from
a SATA card on armada-38x platforms?
I'm getting with 6.0:
[ 1.410115] EXT4-fs error (device sda1): htree_dirblock_to_tree:1093: inode #256: block 8797: comm systemd: bad entry in directory: rec_len % 4 != 0 - offset=0, inode=33188, rec_len=35097, size=4096 fake=0
which appears to be due to reading bad data from the SATA device -
what this tells me in inode and rec_len is not what is actually on
the device in question.
Booting back to 5.19 gives a clean filesystem which has no errors,
so it isn't filesystem corruption, it is an inability for 6.0 to
read data correctly from the device.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-20 18:22 ` Russell King (Oracle)
@ 2022-10-20 19:10 ` Marek Behún
2022-10-21 16:25 ` Linus Torvalds
` (2 subsequent siblings)
3 siblings, 0 replies; 43+ messages in thread
From: Marek Behún @ 2022-10-20 19:10 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Marcin Wojtas, pali, Christoph Hellwig, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Thu, 20 Oct 2022 19:22:07 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> On Tue, Oct 04, 2022 at 10:36:05AM +0200, Marcin Wojtas wrote:
> > wt., 4 paź 2022 o 10:26 Marek Behún <kabel@kernel.org> napisał(a):
> > >
> > > On Mon, 3 Oct 2022 23:30:31 +0200
> > > Marcin Wojtas <mw@semihalf.com> wrote:
> > >
> > > > Hi Marek,
> > > >
> > > >
> > > > pon., 3 paź 2022 o 17:33 Marek Behún <kabel@kernel.org> napisał(a):
> > > > >
> > > > > On Mon, 3 Oct 2022 15:11:44 +0100
> > > > > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> > > > >
> > > > > > On Mon, Oct 03, 2022 at 09:30:37AM +0200, Christoph Hellwig wrote:
> > > > > > > On Fri, Sep 30, 2022 at 05:02:05PM +0200, Marek Behún wrote:
> > > > > > > > It seems that the null pointer dereference comes from the data variable
> > > > > > > > having zero value. We assign
> > > > > > > > data = (u8 *)(uintptr_t)rx_desc->buf_cookie;
> > > > > > >
> > > > > > > I never see any assignment to ->buf_cookie in the driver, what am
> > > > > > > I missing?
> > > > > >
> > > > > > I think Marek's setup (like my setups) use the hardware buffer manager,
> > > > > > and it's hardware that fills in the "buf_cookie", which is supposed to
> > > > > > be the virtual address of the buffer.
> > > > > >
> > > > > > Each buffer supplied to the hardware buffer manager is supposed to
> > > > > > contain the virtual address in the first 32-bit word in that buffer.
> > > > > >
> > > > > > This is done by mvneta_bm_construct():
> > > > > >
> > > > > > /* In order to update buf_cookie field of RX descriptor properly,
> > > > > > * BM hardware expects buf virtual address to be placed in the
> > > > > > * first four bytes of mapped buffer.
> > > > > > */
> > > > > > *(u32 *)buf = (u32)buf;
> > > > > >
> > > > > > immediately prior to dma_map_single(..., DMA_FROM_DEVICE) is called.
> > > > > >
> > > > > > If I had to guess, I would suggest that this write is being lost via
> > > > > > cache invalidation, and given that the hardware BM both reads and
> > > > > > writes this buffer, DMA_FROM_DEVICE is not correct, it should be
> > > > > > DMA_BIDIRECTIONAL.
> > > > > >
> > > >
> > > > I think the DMA_FROM_DEVICE is used rather properly in the RX path of
> > > > the driver - the CPU doesn't access the payload afterward. The BM only
> > > > pushes the pointers back and forth between internal SRAM ('internal
> > > > pool' - BPPI) to DRAM ('external pool' - BPPE) and the descriptors,
> > > > but afaik it should not touch the buffer contents. But maybe somehow
> > > > it affects the coherency and DMA_BIDIRECTIONAL are indeed required...
> > > > About the coherency itself - please see my comment below.
> > > >
> > > > Another thought - when writing to *buf (memory normal) shouldn't we add a dsb()?
> > > >
> > > > > > Changing that is probably going to need DMA_FROM_DEVICE also changed
> > > > > > elsewhere in the mvneta_bm and mvneta driver.
> > > > > >
> > > > > > I'm not in a position where I could test that out. Marek?
> > > > > >
> > > > >
> > > > > Hello Russell,
> > > > >
> > > > > thanks for your suggestion!
> > > > >
> > > > > Adding Pali, since he has some information (see at the end of this
> > > > > message).
> > > > >
> > > > > The attached patch seems to solve the null-pointer dereference.
> > > >
> > > > Did you manage to measure performance impact?
> > >
> > > I did not measure any performance impacts. But DMA directianlity
> > > within mvneta seems to be a different bug, as Russell replied, and maybe
> > > was just revealed by this.
> > >
> > > > I have one overall concern here. On all kinds of A38x-based boards I
> > > > worked on, by default, the firmware set all devices (e.g. network,
> > > > AHCI, XHCI) on MBUS as fully IO cache coherent - it should be
> > > > reflected in the MVNETA_WIN_BASE(w) registers attribute field. Bits
> > > > [15:8] should be set to 0x1D (or 0x1E if there is a second DRAM CS
> > > > used). Can you please try adding 'dma-coherent;' property under the
> > > > 'internal-regs' node?
> > >
> > > Yes, adding dma-coherent solves this issue. See other emails. The
> > > proper solution IMO is to default to dma-coherent on the platform,
> > > which can be done in a simple way (I've sent a patch). We want to be
> > > compatible with older device-trees.
> > >
> >
> > Thanks a lot for testing. I agree we must maintain the backward
> > compatibility with older DT's. To summarize, I think we should end up
> > with 3 patches:
> > 1. Update arch/arm/mm/dma-mapping.c as suggested by Christoph.
> > 2. Add 'dma-coherent' in armada-38x.dtsi.
> > 3. Fix DMA attribute in mvneta_bm_construct().
> >
> > In case any help is needed from my side, please let me know.
>
> Is it possible that this would also cause data corruption reading from
> a SATA card on armada-38x platforms?
>
> I'm getting with 6.0:
>
> [ 1.410115] EXT4-fs error (device sda1): htree_dirblock_to_tree:1093: inode #256: block 8797: comm systemd: bad entry in directory: rec_len % 4 != 0 - offset=0, inode=33188, rec_len=35097, size=4096 fake=0
>
> which appears to be due to reading bad data from the SATA device -
> what this tells me in inode and rec_len is not what is actually on
> the device in question.
>
> Booting back to 5.19 gives a clean filesystem which has no errors,
> so it isn't filesystem corruption, it is an inability for 6.0 to
> read data correctly from the device.
>
It's very probable. Don't use 6.0 or apply Christoph's patches.
Marek
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-20 18:22 ` Russell King (Oracle)
2022-10-20 19:10 ` Marek Behún
@ 2022-10-21 16:25 ` Linus Torvalds
2022-10-21 16:30 ` Christoph Hellwig
2022-10-23 11:58 ` Klaus Kudielka
3 siblings, 0 replies; 43+ messages in thread
From: Linus Torvalds @ 2022-10-21 16:25 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Marcin Wojtas, Marek Behún, pali, Christoph Hellwig,
Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Thu, Oct 20, 2022 at 11:22 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> I'm getting with 6.0:
>
> [ 1.410115] EXT4-fs error (device sda1): htree_dirblock_to_tree:1093: inode #256: block 8797: comm systemd: bad entry in directory: rec_len % 4 != 0 - offset=0, inode=33188, rec_len=35097, size=4096 fake=0
Russell, can you confirm that 6.0.3 (that Greg just released and
should contain the fix from Christoph) and my current development tree
are both ok?
I assume you already tested with Christoph's patch separately, I'm
just trying to make sure that this is all good in the base stable /
development trees.
Linus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-20 18:22 ` Russell King (Oracle)
2022-10-20 19:10 ` Marek Behún
2022-10-21 16:25 ` Linus Torvalds
@ 2022-10-21 16:30 ` Christoph Hellwig
2022-10-21 18:21 ` Russell King (Oracle)
2022-10-23 11:58 ` Klaus Kudielka
3 siblings, 1 reply; 43+ messages in thread
From: Christoph Hellwig @ 2022-10-21 16:30 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: Marcin Wojtas, Marek Behún, pali, Christoph Hellwig,
Robin Murphy, Arnd Bergmann, Andre Przywara, Marc Zyngier,
Linus Torvalds, Andrew Lunn, Gregory Clement, Greg Kroah-Hartman,
iommu, linux-arm-kernel
On Thu, Oct 20, 2022 at 07:22:07PM +0100, Russell King (Oracle) wrote:
> Is it possible that this would also cause data corruption reading from
> a SATA card on armada-38x platforms?
armada-38x eems to be a mvebu platform, so on the account yes.
I'm not sure what SATA host driver is used there, but that must also be
doing weird thing with the DMA API for the issue to hit.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-21 16:30 ` Christoph Hellwig
@ 2022-10-21 18:21 ` Russell King (Oracle)
0 siblings, 0 replies; 43+ messages in thread
From: Russell King (Oracle) @ 2022-10-21 18:21 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Marcin Wojtas, Marek Behún, pali, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Fri, Oct 21, 2022 at 06:30:50PM +0200, Christoph Hellwig wrote:
> On Thu, Oct 20, 2022 at 07:22:07PM +0100, Russell King (Oracle) wrote:
> > Is it possible that this would also cause data corruption reading from
> > a SATA card on armada-38x platforms?
>
> armada-38x eems to be a mvebu platform, so on the account yes.
> I'm not sure what SATA host driver is used there, but that must also be
> doing weird thing with the DMA API for the issue to hit.
None of the devices on armada-38x mark themselves as DMA-coherent, but
I believe they are all DMA-coherent - certainly in
arch/arm/mach-mvebu/coherency.c, all platform devices will be marked
as DMA-coherent via the notifier, which basically means all devices
declared in DT need to be DMA-coherent.
Since the crypto engine, SD, USB and so on and so forth all use DMA,
it's highly likely that all those were broken as well and this isn't
limited to just mvneta as originally reported. Basically the entire
platform got broken, which really isn't good.
At least in this case, I have a test platform I try new kernels out on
before putting it on my connectivity-critical hardware - such as the
machine that my public connectivity comes through!
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread
* Re: REGRESSION in 6.0-rc7 caused by patch "ARM/dma-mapping: use dma-direct unconditionally"
2022-10-20 18:22 ` Russell King (Oracle)
` (2 preceding siblings ...)
2022-10-21 16:30 ` Christoph Hellwig
@ 2022-10-23 11:58 ` Klaus Kudielka
3 siblings, 0 replies; 43+ messages in thread
From: Klaus Kudielka @ 2022-10-23 11:58 UTC (permalink / raw)
To: Russell King (Oracle), Marcin Wojtas
Cc: Marek Behún, pali, Christoph Hellwig, Robin Murphy,
Arnd Bergmann, Andre Przywara, Marc Zyngier, Linus Torvalds,
Andrew Lunn, Gregory Clement, Greg Kroah-Hartman, iommu,
linux-arm-kernel
On Thu, 2022-10-20 at 19:22 +0100, Russell King (Oracle) wrote:
> Is it possible that this would also cause data corruption reading from
> a SATA card on armada-38x platforms?
>
> I'm getting with 6.0:
>
> [ 1.410115] EXT4-fs error (device sda1): htree_dirblock_to_tree:1093:
> inode #256: block 8797: comm systemd: bad entry in directory: rec_len % 4
> != 0 - offset=0, inode=33188, rec_len=35097, size=4096 fake=0
>
> which appears to be due to reading bad data from the SATA device -
> what this tells me in inode and rec_len is not what is actually on
> the device in question.
>
> Booting back to 5.19 gives a clean filesystem which has no errors,
> so it isn't filesystem corruption, it is an inability for 6.0 to
> read data correctly from the device.
>
For reference, I am running 6.0.0, with the following two patches applied
on top:
ARM/dma-mappіng: don't override ->dma_coherent when set from a bus notifier
ARM/dma-mapping: remove the dma_coherent member of struct dev_archdata
Turris Omnia (Armada 385), EXT4 rootfs on a Kingston UV500 mSATA drive.
I have 16 days uptime and am error-free, as far as I can tell. Definitely,
not a single EXT4-fs error.
Regards, Klaus
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 43+ messages in thread