fix the non-coherent coldfire dma_alloc_coherent

fix the non-coherent coldfire dma_alloc_coherent

[Christoph Hellwig]

Hi all,

this is the next attempt to not return memory that is not DMA coherent
on coldfire/m68knommu.  The last one needed more fixups in the fec
driver, which this versions includes.  On top of that I've also added
a few more cleanups to the core dma allocation code.

Jim: any work to support the set_uncached and remap method for arm32
should probably be based on this, and patch 3 should make that
selection a little easier.

[PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Christoph Hellwig]

CONFIG_DMA_GLOBAL_POOL can't be combined with other dma-coherent
allocators.  Add dependencies to Kconfig to document this, and make
kconfig complain about unment dependencies if someone tries.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/dma/Kconfig | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index f488997b071712..4524db877eba36 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -135,6 +135,8 @@ config DMA_COHERENT_POOL
 
 config DMA_GLOBAL_POOL
 	select DMA_DECLARE_COHERENT
+	depends on !ARCH_HAS_DMA_SET_UNCACHED
+	depends on !DMA_DIRECT_REMAP
 	bool
 
 config DMA_DIRECT_REMAP
-- 
2.39.2

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Geert Uytterhoeven]

Hi Christoph,

On Mon, Oct 9, 2023 at 9:41 AM Christoph Hellwig <hch@lst.de> wrote:
> CONFIG_DMA_GLOBAL_POOL can't be combined with other dma-coherent
> allocators.  Add dependencies to Kconfig to document this, and make
> kconfig complain about unment dependencies if someone tries.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks for your patch!

> --- a/kernel/dma/Kconfig
> +++ b/kernel/dma/Kconfig
> @@ -135,6 +135,8 @@ config DMA_COHERENT_POOL
>
>  config DMA_GLOBAL_POOL
>         select DMA_DECLARE_COHERENT
> +       depends on !ARCH_HAS_DMA_SET_UNCACHED
> +       depends on !DMA_DIRECT_REMAP
>         bool
>
>  config DMA_DIRECT_REMAP

riscv defconfig + CONFIG_NONPORTABLE=y + CONFIG_ARCH_R9A07G043=y:

WARNING: unmet direct dependencies detected for DMA_GLOBAL_POOL
  Depends on [n]: !ARCH_HAS_DMA_SET_UNCACHED [=n] && !DMA_DIRECT_REMAP [=y]
  Selected by [y]:
  - ARCH_R9A07G043 [=y] && SOC_RENESAS [=y] && RISCV [=y] && NONPORTABLE [=y]

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Christoph Hellwig]

On Mon, Oct 09, 2023 at 10:43:57AM +0200, Geert Uytterhoeven wrote:
> >  config DMA_DIRECT_REMAP
> 
> riscv defconfig + CONFIG_NONPORTABLE=y + CONFIG_ARCH_R9A07G043=y:
> 
> WARNING: unmet direct dependencies detected for DMA_GLOBAL_POOL
>   Depends on [n]: !ARCH_HAS_DMA_SET_UNCACHED [=n] && !DMA_DIRECT_REMAP [=y]
>   Selected by [y]:
>   - ARCH_R9A07G043 [=y] && SOC_RENESAS [=y] && RISCV [=y] && NONPORTABLE [=y]

And that's exactly what this patch is supposed to show.  RISCV must not
select DMA_DIRECT_REMAP at the same time as DMA_GLOBAL_POOL.  I though
the fix for that just went upstream?

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Geert Uytterhoeven]

Hi Christoph,

On Mon, Oct 9, 2023 at 11:16 AM Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Oct 09, 2023 at 10:43:57AM +0200, Geert Uytterhoeven wrote:
> > >  config DMA_DIRECT_REMAP
> >
> > riscv defconfig + CONFIG_NONPORTABLE=y + CONFIG_ARCH_R9A07G043=y:
> >
> > WARNING: unmet direct dependencies detected for DMA_GLOBAL_POOL
> >   Depends on [n]: !ARCH_HAS_DMA_SET_UNCACHED [=n] && !DMA_DIRECT_REMAP [=y]
> >   Selected by [y]:
> >   - ARCH_R9A07G043 [=y] && SOC_RENESAS [=y] && RISCV [=y] && NONPORTABLE [=y]
>
> And that's exactly what this patch is supposed to show.  RISCV must not
> select DMA_DIRECT_REMAP at the same time as DMA_GLOBAL_POOL.  I though
> the fix for that just went upstream?

The fix you are referring too is probably commit c1ec4b450ab729e3
("soc: renesas: Make ARCH_R9A07G043 (riscv version) depend
on NONPORTABLE") in next-20231006 and later.  It is not yet upstream.

Still, it merely makes ARCH_R9A07G043 (which selects DMA_GLOBAL_POOL)
depend on ARCH_R9A07G043.
RISCV_DMA_NONCOHERENT still selects DMA_DIRECT_REMAP, so both can end
up being enabled.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Christoph Hellwig]

On Mon, Oct 09, 2023 at 11:34:55AM +0200, Geert Uytterhoeven wrote:
> The fix you are referring too is probably commit c1ec4b450ab729e3
> ("soc: renesas: Make ARCH_R9A07G043 (riscv version) depend
> on NONPORTABLE") in next-20231006 and later.  It is not yet upstream.
> 
> Still, it merely makes ARCH_R9A07G043 (which selects DMA_GLOBAL_POOL)
> depend on ARCH_R9A07G043.
> RISCV_DMA_NONCOHERENT still selects DMA_DIRECT_REMAP, so both can end
> up being enabled.

Ok, so we need to actually fix this properly.  Lad, can you respin
the fix to not select DMA_DIRECT_REMAP, for ARCH_R9A07G043?

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Geert Uytterhoeven]

Hi Christoph,

CC soc

On Mon, Oct 9, 2023 at 11:43 AM Christoph Hellwig <hch@lst.de> wrote:
> On Mon, Oct 09, 2023 at 11:34:55AM +0200, Geert Uytterhoeven wrote:
> > The fix you are referring too is probably commit c1ec4b450ab729e3
> > ("soc: renesas: Make ARCH_R9A07G043 (riscv version) depend
> > on NONPORTABLE") in next-20231006 and later.  It is not yet upstream.
> >
> > Still, it merely makes ARCH_R9A07G043 (which selects DMA_GLOBAL_POOL)
> > depend on ARCH_R9A07G043.
> > RISCV_DMA_NONCOHERENT still selects DMA_DIRECT_REMAP, so both can end
> > up being enabled.
>
> Ok, so we need to actually fix this properly.  Lad, can you respin
> the fix to not select DMA_DIRECT_REMAP, for ARCH_R9A07G043?

ARCH_R9A07G043 does not select DMA_DIRECT_REMAP directly,
RISCV_DMA_NONCOHERENT does.  And there are other users of
RISCV_DMA_NONCOHERENT (RISCV_ISA_ZICBOM and ERRATA_THEAD_CMO).
Should the selection of DMA_DIRECT_REMAP moved to their users?

Note that the fix is already in soc/for-next, so we need coordination
with the soc people.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Robin Murphy]

On 2023-10-09 10:51, Geert Uytterhoeven wrote:
> Hi Christoph,
> 
> CC soc
> 
> On Mon, Oct 9, 2023 at 11:43 AM Christoph Hellwig <hch@lst.de> wrote:
>> On Mon, Oct 09, 2023 at 11:34:55AM +0200, Geert Uytterhoeven wrote:
>>> The fix you are referring too is probably commit c1ec4b450ab729e3
>>> ("soc: renesas: Make ARCH_R9A07G043 (riscv version) depend
>>> on NONPORTABLE") in next-20231006 and later.  It is not yet upstream.
>>>
>>> Still, it merely makes ARCH_R9A07G043 (which selects DMA_GLOBAL_POOL)
>>> depend on ARCH_R9A07G043.
>>> RISCV_DMA_NONCOHERENT still selects DMA_DIRECT_REMAP, so both can end
>>> up being enabled.
>>
>> Ok, so we need to actually fix this properly.  Lad, can you respin
>> the fix to not select DMA_DIRECT_REMAP, for ARCH_R9A07G043?
> 
> ARCH_R9A07G043 does not select DMA_DIRECT_REMAP directly,
> RISCV_DMA_NONCOHERENT does.  And there are other users of
> RISCV_DMA_NONCOHERENT (RISCV_ISA_ZICBOM and ERRATA_THEAD_CMO).
> Should the selection of DMA_DIRECT_REMAP moved to their users?

No, the selection of DMA_GLOBAL_POOL should be removed from 
RISV_DMA_NONCOHERENT and selected directly by ARCH_R9A07G043 (along with 
any of the other implied symbols it needs). Or if as suggested this 
physical-attribute-remap wackiness is due to show up on more platforms 
as well, maybe have a common config for that which selects 
DMA_GLOBAL_POOL plus the relevant cache maintenance extensions as an 
equivalent to RISCV_DMA_NONCOHERENT, and can itself explicitly depend on 
NONPORTABLE for clarity.

Thanks,
Robin.

> Note that the fix is already in soc/for-next, so we need coordination
> with the soc people.
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
> 

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Geert Uytterhoeven]

Hi Robin,

On Mon, Oct 9, 2023 at 12:04 PM Robin Murphy <robin.murphy@arm.com> wrote:
> On 2023-10-09 10:51, Geert Uytterhoeven wrote:
> > On Mon, Oct 9, 2023 at 11:43 AM Christoph Hellwig <hch@lst.de> wrote:
> >> On Mon, Oct 09, 2023 at 11:34:55AM +0200, Geert Uytterhoeven wrote:
> >>> The fix you are referring too is probably commit c1ec4b450ab729e3
> >>> ("soc: renesas: Make ARCH_R9A07G043 (riscv version) depend
> >>> on NONPORTABLE") in next-20231006 and later.  It is not yet upstream.
> >>>
> >>> Still, it merely makes ARCH_R9A07G043 (which selects DMA_GLOBAL_POOL)
> >>> depend on ARCH_R9A07G043.
> >>> RISCV_DMA_NONCOHERENT still selects DMA_DIRECT_REMAP, so both can end
> >>> up being enabled.
> >>
> >> Ok, so we need to actually fix this properly.  Lad, can you respin
> >> the fix to not select DMA_DIRECT_REMAP, for ARCH_R9A07G043?
> >
> > ARCH_R9A07G043 does not select DMA_DIRECT_REMAP directly,
> > RISCV_DMA_NONCOHERENT does.  And there are other users of
> > RISCV_DMA_NONCOHERENT (RISCV_ISA_ZICBOM and ERRATA_THEAD_CMO).
> > Should the selection of DMA_DIRECT_REMAP moved to their users?
>
> No, the selection of DMA_GLOBAL_POOL should be removed from
> RISV_DMA_NONCOHERENT and selected directly by ARCH_R9A07G043 (along with
> any of the other implied symbols it needs). Or if as suggested this
> physical-attribute-remap wackiness is due to show up on more platforms
> as well, maybe have a common config for that which selects
> DMA_GLOBAL_POOL plus the relevant cache maintenance extensions as an
> equivalent to RISCV_DMA_NONCOHERENT, and can itself explicitly depend on
> NONPORTABLE for clarity.

RISCV_DMA_NONCOHERENT does not select DMA_GLOBAL_POOL,
ARCH_R9A07G043 selects DMA_GLOBAL_POOL.
RISCV_DMA_NONCOHERENT does select DMA_DIRECT_REMAP if MMU.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Christoph Hellwig]

On Mon, Oct 09, 2023 at 01:10:26PM +0200, Geert Uytterhoeven wrote:
> RISCV_DMA_NONCOHERENT does not select DMA_GLOBAL_POOL,
> ARCH_R9A07G043 selects DMA_GLOBAL_POOL.
> RISCV_DMA_NONCOHERENT does select DMA_DIRECT_REMAP if MMU.

Yeah, and we'll basically need to split RISCV_DMA_NONCOHERENT into
an option for each type of non-coherent support.  This is why we
should never have added support for any of the non-standard versions,
as it's turning into a giant mess.

Re: [PATCH 1/6] dma-direct: add depdenencies to CONFIG_DMA_GLOBAL_POOL

[Robin Murphy]

On 09/10/2023 1:48 pm, Christoph Hellwig wrote:
> On Mon, Oct 09, 2023 at 01:10:26PM +0200, Geert Uytterhoeven wrote:
>> RISCV_DMA_NONCOHERENT does not select DMA_GLOBAL_POOL,
>> ARCH_R9A07G043 selects DMA_GLOBAL_POOL.
>> RISCV_DMA_NONCOHERENT does select DMA_DIRECT_REMAP if MMU.

Bleh, guess I should have known better than to trust my Monday morning 
memory without double-checking the code :)

> Yeah, and we'll basically need to split RISCV_DMA_NONCOHERENT into
> an option for each type of non-coherent support.  This is why we
> should never have added support for any of the non-standard versions,
> as it's turning into a giant mess.

Indeed the main point I was trying to get at is for ARCH_R9A07G043 (or 
rather possibly ERRATA_ANDES_CMO) to not select RISCV_DMA_NONCOHERENT in 
its current form, since that ending up selecting DMA_DIRECT_REMAP on a 
platform which can't support it is the thing that's most obviously wrong.

Thanks,
Robin.

[PATCH 2/6] dma-direct: add a CONFIG_ARCH_DMA_ALLOC symbol

[Christoph Hellwig]

Instead of using arch_dma_alloc if none of the generic coherent
allocators are used, require the architectures to explicitly opt into
providing it.  This will used to deal with the case of m68knommu and
coldfire where we can't do any coherent allocations whatsoever, and
also makes it clear that arch_dma_alloc is a last resort.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/arm/Kconfig    |  1 +
 arch/m68k/Kconfig   |  1 +
 arch/parisc/Kconfig |  1 +
 kernel/dma/Kconfig  |  9 +++++++++
 kernel/dma/direct.c | 12 ++----------
 5 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 9557808e8937b1..a3fdf584278f86 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -4,6 +4,7 @@ config ARM
 	default y
 	select ARCH_32BIT_OFF_T
 	select ARCH_CORRECT_STACKTRACE_ON_KRETPROBE if HAVE_KRETPROBES && FRAME_POINTER && !ARM_UNWIND
+	select ARCH_DMA_ALLOC if MMU
 	select ARCH_HAS_BINFMT_FLAT
 	select ARCH_HAS_CPU_FINALIZE_INIT if MMU
 	select ARCH_HAS_CURRENT_STACK_POINTER
diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 3e318bf9504c5b..0430b8ba6b5cc6 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -3,6 +3,7 @@ config M68K
 	bool
 	default y
 	select ARCH_32BIT_OFF_T
+	select ARCH_DMA_ALLOC if !MMU || COLDFIRE
 	select ARCH_HAS_BINFMT_FLAT
 	select ARCH_HAS_CPU_FINALIZE_INIT if MMU
 	select ARCH_HAS_CURRENT_STACK_POINTER
diff --git a/arch/parisc/Kconfig b/arch/parisc/Kconfig
index a15ab147af2e07..30a4916fa9b0cc 100644
--- a/arch/parisc/Kconfig
+++ b/arch/parisc/Kconfig
@@ -3,6 +3,7 @@ config PARISC
 	def_bool y
 	select ALTERNATE_USER_ADDRESS_SPACE
 	select ARCH_32BIT_OFF_T if !64BIT
+	select ARCH_DMA_ALLOC if PA11
 	select ARCH_MIGHT_HAVE_PC_PARPORT
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index 4524db877eba36..515d2063b509ac 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -144,6 +144,15 @@ config DMA_DIRECT_REMAP
 	select DMA_COHERENT_POOL
 	select DMA_NONCOHERENT_MMAP
 
+#
+# Fallback to arch code for DMA allocations.  This should eventually go away.
+#
+config ARCH_DMA_ALLOC
+	depends on !ARCH_HAS_DMA_SET_UNCACHED
+	depends on !DMA_DIRECT_REMAP
+	depends on !DMA_GLOBAL_POOL
+	bool
+
 config DMA_CMA
 	bool "DMA Contiguous Memory Allocator"
 	depends on HAVE_DMA_CONTIGUOUS && CMA
diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 9596ae1aa0dacf..ec410af1d8a14e 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -220,13 +220,7 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 		return dma_direct_alloc_no_mapping(dev, size, dma_handle, gfp);
 
 	if (!dev_is_dma_coherent(dev)) {
-		/*
-		 * Fallback to the arch handler if it exists.  This should
-		 * eventually go away.
-		 */
-		if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&
-		    !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
-		    !IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) &&
+		if (IS_ENABLED(CONFIG_ARCH_DMA_ALLOC) &&
 		    !is_swiotlb_for_alloc(dev))
 			return arch_dma_alloc(dev, size, dma_handle, gfp,
 					      attrs);
@@ -330,9 +324,7 @@ void dma_direct_free(struct device *dev, size_t size,
 		return;
 	}
 
-	if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED) &&
-	    !IS_ENABLED(CONFIG_DMA_DIRECT_REMAP) &&
-	    !IS_ENABLED(CONFIG_DMA_GLOBAL_POOL) &&
+	if (IS_ENABLED(CONFIG_ARCH_DMA_ALLOC) &&
 	    !dev_is_dma_coherent(dev) &&
 	    !is_swiotlb_for_alloc(dev)) {
 		arch_dma_free(dev, size, cpu_addr, dma_addr, attrs);
-- 
2.39.2

[PATCH 3/6] dma-direct: simplify the use atomic pool logic in dma_direct_alloc

[Christoph Hellwig]

The logic in dma_direct_alloc when to use the atomic pool vs remapping
grew a bit unreadable.  Consolidate it into a single check, and clean
up the set_uncached vs remap logic a bit as well.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/dma/direct.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index ec410af1d8a14e..1327d04fa32a25 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -234,27 +234,22 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 					dma_handle);
 
 		/*
-		 * Otherwise remap if the architecture is asking for it.  But
-		 * given that remapping memory is a blocking operation we'll
-		 * instead have to dip into the atomic pools.
+		 * Otherwise we require the architecture to either be able to
+		 * mark arbitrary parts of the kernel direct mapping uncached,
+		 * or remapped it uncached.
 		 */
+		set_uncached = IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED);
 		remap = IS_ENABLED(CONFIG_DMA_DIRECT_REMAP);
-		if (remap) {
-			if (dma_direct_use_pool(dev, gfp))
-				return dma_direct_alloc_from_pool(dev, size,
-						dma_handle, gfp);
-		} else {
-			if (!IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED))
-				return NULL;
-			set_uncached = true;
-		}
+		if (!set_uncached && !remap)
+			return NULL;
 	}
 
 	/*
-	 * Decrypting memory may block, so allocate the memory from the atomic
-	 * pools if we can't block.
+	 * Remapping or decrypting memory may block, allocate the memory from
+	 * the atomic pools instead if we aren't allowed block.
 	 */
-	if (force_dma_unencrypted(dev) && dma_direct_use_pool(dev, gfp))
+	if ((remap || force_dma_unencrypted(dev)) &&
+	    dma_direct_use_pool(dev, gfp))
 		return dma_direct_alloc_from_pool(dev, size, dma_handle, gfp);
 
 	/* we always manually zero the memory once we are done */
-- 
2.39.2

[PATCH 4/6] dma-direct: warn when coherent allocations aren't supported

[Christoph Hellwig]

Log a warning once when dma_alloc_coherent fails because the platform
does not support coherent allocations at all.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 kernel/dma/direct.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/kernel/dma/direct.c b/kernel/dma/direct.c
index 1327d04fa32a25..fddfea3b2fe173 100644
--- a/kernel/dma/direct.c
+++ b/kernel/dma/direct.c
@@ -240,8 +240,10 @@ void *dma_direct_alloc(struct device *dev, size_t size,
 		 */
 		set_uncached = IS_ENABLED(CONFIG_ARCH_HAS_DMA_SET_UNCACHED);
 		remap = IS_ENABLED(CONFIG_DMA_DIRECT_REMAP);
-		if (!set_uncached && !remap)
+		if (!set_uncached && !remap) {
+			pr_warn_once("coherent DMA allocations not supported on this platform.\n");
 			return NULL;
+		}
 	}
 
 	/*
-- 
2.39.2

[PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Christoph Hellwig]

The coldfire platforms can't properly implement dma_alloc_coherent and
currently just return noncoherent memory from dma_alloc_coherent.

The fec driver than works around this with a flush of all caches in the
receive path. Make this hack a little less bad by using the explicit
dma_alloc_noncoherent API and documenting the hacky cache flushes so
that the DMA API level hack can be removed.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/net/ethernet/freescale/fec_main.c | 84 ++++++++++++++++++++---
 1 file changed, 75 insertions(+), 9 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 77c8e9cfb44562..aa032ea0ebf0c2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -406,6 +406,70 @@ static void fec_dump(struct net_device *ndev)
 	} while (bdp != txq->bd.base);
 }
 
+/*
+ * Coldfire does not support DMA coherent allocations, and has historically used
+ * a band-aid with a manual flush in fec_enet_rx_queue.
+ */
+#ifdef CONFIG_COLDFIRE
+static void *fec_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
+		gfp_t gfp)
+{
+	return dma_alloc_noncoherent(dev, size, handle, DMA_BIDIRECTIONAL, gfp);
+}
+
+static void fec_dma_free(struct device *dev, size_t size, void *cpu_addr,
+		dma_addr_t handle)
+{
+	dma_free_noncoherent(dev, size, cpu_addr, handle, DMA_BIDIRECTIONAL);
+}
+#else /* CONFIG_COLDFIRE */
+static void *fec_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
+		gfp_t gfp)
+{
+	return dma_alloc_coherent(dev, size, handle, gfp);
+}
+
+static void fec_dma_free(struct device *dev, size_t size, void *cpu_addr,
+		dma_addr_t handle)
+{
+	dma_free_coherent(dev, size, cpu_addr, handle);
+}
+#endif /* !CONFIG_COLDFIRE */
+
+struct fec_dma_devres {
+	size_t		size;
+	void		*vaddr;
+	dma_addr_t	dma_handle;
+};
+
+static void fec_dmam_release(struct device *dev, void *res)
+{
+	struct fec_dma_devres *this = res;
+
+	fec_dma_free(dev, this->size, this->vaddr, this->dma_handle);
+}
+
+static void *fec_dmam_alloc(struct device *dev, size_t size, dma_addr_t *handle,
+		gfp_t gfp)
+{
+	struct fec_dma_devres *dr;
+	void *vaddr;
+
+	dr = devres_alloc(fec_dmam_release, sizeof(*dr), gfp);
+	if (!dr)
+		return NULL;
+	vaddr = fec_dma_alloc(dev, size, handle, gfp);
+	if (!vaddr) {
+		devres_free(dr);
+		return NULL;
+	}
+	dr->vaddr = vaddr;
+	dr->dma_handle = *handle;
+	dr->size = size;
+	devres_add(dev, dr);
+	return vaddr;
+}
+
 static inline bool is_ipv4_pkt(struct sk_buff *skb)
 {
 	return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4;
@@ -1661,6 +1725,10 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
 #endif
 
 #ifdef CONFIG_M532x
+	/*
+	 * Hacky flush of all caches instead of using the DMA API for the TSO
+	 * headers.
+	 */
 	flush_cache_all();
 #endif
 	rxq = fep->rx_queue[queue_id];
@@ -3288,10 +3356,9 @@ static void fec_enet_free_queue(struct net_device *ndev)
 	for (i = 0; i < fep->num_tx_queues; i++)
 		if (fep->tx_queue[i] && fep->tx_queue[i]->tso_hdrs) {
 			txq = fep->tx_queue[i];
-			dma_free_coherent(&fep->pdev->dev,
-					  txq->bd.ring_size * TSO_HEADER_SIZE,
-					  txq->tso_hdrs,
-					  txq->tso_hdrs_dma);
+			fec_dma_free(&fep->pdev->dev,
+				     txq->bd.ring_size * TSO_HEADER_SIZE,
+				     txq->tso_hdrs, txq->tso_hdrs_dma);
 		}
 
 	for (i = 0; i < fep->num_rx_queues; i++)
@@ -3321,10 +3388,9 @@ static int fec_enet_alloc_queue(struct net_device *ndev)
 		txq->tx_stop_threshold = FEC_MAX_SKB_DESCS;
 		txq->tx_wake_threshold = FEC_MAX_SKB_DESCS + 2 * MAX_SKB_FRAGS;
 
-		txq->tso_hdrs = dma_alloc_coherent(&fep->pdev->dev,
+		txq->tso_hdrs = fec_dma_alloc(&fep->pdev->dev,
 					txq->bd.ring_size * TSO_HEADER_SIZE,
-					&txq->tso_hdrs_dma,
-					GFP_KERNEL);
+					&txq->tso_hdrs_dma, GFP_KERNEL);
 		if (!txq->tso_hdrs) {
 			ret = -ENOMEM;
 			goto alloc_failed;
@@ -4043,8 +4109,8 @@ static int fec_enet_init(struct net_device *ndev)
 	bd_size = (fep->total_tx_ring_size + fep->total_rx_ring_size) * dsize;
 
 	/* Allocate memory for buffer descriptors. */
-	cbd_base = dmam_alloc_coherent(&fep->pdev->dev, bd_size, &bd_dma,
-				       GFP_KERNEL);
+	cbd_base = fec_dmam_alloc(&fep->pdev->dev, bd_size, &bd_dma,
+				  GFP_KERNEL);
 	if (!cbd_base) {
 		ret = -ENOMEM;
 		goto free_queue_mem;
-- 
2.39.2

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Robin Murphy]

On 2023-10-09 08:41, Christoph Hellwig wrote:
> The coldfire platforms can't properly implement dma_alloc_coherent and
> currently just return noncoherent memory from dma_alloc_coherent.
> 
> The fec driver than works around this with a flush of all caches in the
> receive path. Make this hack a little less bad by using the explicit
> dma_alloc_noncoherent API and documenting the hacky cache flushes so
> that the DMA API level hack can be removed.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/net/ethernet/freescale/fec_main.c | 84 ++++++++++++++++++++---
>   1 file changed, 75 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 77c8e9cfb44562..aa032ea0ebf0c2 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -406,6 +406,70 @@ static void fec_dump(struct net_device *ndev)
>   	} while (bdp != txq->bd.base);
>   }
>   
> +/*
> + * Coldfire does not support DMA coherent allocations, and has historically used
> + * a band-aid with a manual flush in fec_enet_rx_queue.
> + */
> +#ifdef CONFIG_COLDFIRE

It looks a bit odd that this ends up applying to all of Coldfire, while 
the associated cache flush only applies to the M532x platform, which 
implies that we'd now be relying on the non-coherent allocation actually 
being coherent on other Coldfire platforms.

Would it work to do something like this to make sure dma-direct does the 
right thing on such platforms (which presumably don't have caches?), and 
then reduce the scope of this FEC hack accordingly, to clean things up 
even better?

diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
index b826e9c677b2..1851fa3fe077 100644
--- a/arch/m68k/Kconfig.cpu
+++ b/arch/m68k/Kconfig.cpu
@@ -27,6 +27,7 @@ config COLDFIRE
  	select CPU_HAS_NO_BITFIELDS
  	select CPU_HAS_NO_CAS
  	select CPU_HAS_NO_MULDIV64
+	select DMA_DEFAULT_COHERENT if !MMU && !M523x
  	select GENERIC_CSUM
  	select GPIOLIB
  	select HAVE_LEGACY_CLK


Thanks,
Robin.

> +static void *fec_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
> +		gfp_t gfp)
> +{
> +	return dma_alloc_noncoherent(dev, size, handle, DMA_BIDIRECTIONAL, gfp);
> +}
> +
> +static void fec_dma_free(struct device *dev, size_t size, void *cpu_addr,
> +		dma_addr_t handle)
> +{
> +	dma_free_noncoherent(dev, size, cpu_addr, handle, DMA_BIDIRECTIONAL);
> +}
> +#else /* CONFIG_COLDFIRE */
> +static void *fec_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
> +		gfp_t gfp)
> +{
> +	return dma_alloc_coherent(dev, size, handle, gfp);
> +}
> +
> +static void fec_dma_free(struct device *dev, size_t size, void *cpu_addr,
> +		dma_addr_t handle)
> +{
> +	dma_free_coherent(dev, size, cpu_addr, handle);
> +}
> +#endif /* !CONFIG_COLDFIRE */
> +
> +struct fec_dma_devres {
> +	size_t		size;
> +	void		*vaddr;
> +	dma_addr_t	dma_handle;
> +};
> +
> +static void fec_dmam_release(struct device *dev, void *res)
> +{
> +	struct fec_dma_devres *this = res;
> +
> +	fec_dma_free(dev, this->size, this->vaddr, this->dma_handle);
> +}
> +
> +static void *fec_dmam_alloc(struct device *dev, size_t size, dma_addr_t *handle,
> +		gfp_t gfp)
> +{
> +	struct fec_dma_devres *dr;
> +	void *vaddr;
> +
> +	dr = devres_alloc(fec_dmam_release, sizeof(*dr), gfp);
> +	if (!dr)
> +		return NULL;
> +	vaddr = fec_dma_alloc(dev, size, handle, gfp);
> +	if (!vaddr) {
> +		devres_free(dr);
> +		return NULL;
> +	}
> +	dr->vaddr = vaddr;
> +	dr->dma_handle = *handle;
> +	dr->size = size;
> +	devres_add(dev, dr);
> +	return vaddr;
> +}
> +
>   static inline bool is_ipv4_pkt(struct sk_buff *skb)
>   {
>   	return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4;
> @@ -1661,6 +1725,10 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>   #endif
>   
>   #ifdef CONFIG_M532x
> +	/*
> +	 * Hacky flush of all caches instead of using the DMA API for the TSO
> +	 * headers.
> +	 */
>   	flush_cache_all();
>   #endif
>   	rxq = fep->rx_queue[queue_id];
> @@ -3288,10 +3356,9 @@ static void fec_enet_free_queue(struct net_device *ndev)
>   	for (i = 0; i < fep->num_tx_queues; i++)
>   		if (fep->tx_queue[i] && fep->tx_queue[i]->tso_hdrs) {
>   			txq = fep->tx_queue[i];
> -			dma_free_coherent(&fep->pdev->dev,
> -					  txq->bd.ring_size * TSO_HEADER_SIZE,
> -					  txq->tso_hdrs,
> -					  txq->tso_hdrs_dma);
> +			fec_dma_free(&fep->pdev->dev,
> +				     txq->bd.ring_size * TSO_HEADER_SIZE,
> +				     txq->tso_hdrs, txq->tso_hdrs_dma);
>   		}
>   
>   	for (i = 0; i < fep->num_rx_queues; i++)
> @@ -3321,10 +3388,9 @@ static int fec_enet_alloc_queue(struct net_device *ndev)
>   		txq->tx_stop_threshold = FEC_MAX_SKB_DESCS;
>   		txq->tx_wake_threshold = FEC_MAX_SKB_DESCS + 2 * MAX_SKB_FRAGS;
>   
> -		txq->tso_hdrs = dma_alloc_coherent(&fep->pdev->dev,
> +		txq->tso_hdrs = fec_dma_alloc(&fep->pdev->dev,
>   					txq->bd.ring_size * TSO_HEADER_SIZE,
> -					&txq->tso_hdrs_dma,
> -					GFP_KERNEL);
> +					&txq->tso_hdrs_dma, GFP_KERNEL);
>   		if (!txq->tso_hdrs) {
>   			ret = -ENOMEM;
>   			goto alloc_failed;
> @@ -4043,8 +4109,8 @@ static int fec_enet_init(struct net_device *ndev)
>   	bd_size = (fep->total_tx_ring_size + fep->total_rx_ring_size) * dsize;
>   
>   	/* Allocate memory for buffer descriptors. */
> -	cbd_base = dmam_alloc_coherent(&fep->pdev->dev, bd_size, &bd_dma,
> -				       GFP_KERNEL);
> +	cbd_base = fec_dmam_alloc(&fep->pdev->dev, bd_size, &bd_dma,
> +				  GFP_KERNEL);
>   	if (!cbd_base) {
>   		ret = -ENOMEM;
>   		goto free_queue_mem;

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Christoph Hellwig]

On Mon, Oct 09, 2023 at 11:29:12AM +0100, Robin Murphy wrote:
> It looks a bit odd that this ends up applying to all of Coldfire, while the 
> associated cache flush only applies to the M532x platform, which implies 
> that we'd now be relying on the non-coherent allocation actually being 
> coherent on other Coldfire platforms.
>
> Would it work to do something like this to make sure dma-direct does the 
> right thing on such platforms (which presumably don't have caches?), and 
> then reduce the scope of this FEC hack accordingly, to clean things up even 
> better?

Probably.  Actually Greg comment something along the lines last
time, and mentioned something about just instruction vs instruction
and data cache.

>
> diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
> index b826e9c677b2..1851fa3fe077 100644
> --- a/arch/m68k/Kconfig.cpu
> +++ b/arch/m68k/Kconfig.cpu
> @@ -27,6 +27,7 @@ config COLDFIRE
>  	select CPU_HAS_NO_BITFIELDS
>  	select CPU_HAS_NO_CAS
>  	select CPU_HAS_NO_MULDIV64
> +	select DMA_DEFAULT_COHERENT if !MMU && !M523x

Although it would probably make more sense to simply not select
CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE and
CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU for these platforms and not
build the non-coherent code at all.  This should also include
all coldfire platforms with mmu (M54xx/M548x/M5441x).  Then
again for many of the coldfire platforms the Kconfig allows
to select CACHE_WRITETHRU/CACHE_COPYBACK which looks related.

Greg, any chance you could help out with the caching modes on
coldfire and legacy m68knommu?

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Greg Ungerer]

Hi Christoph,

On 9/10/23 22:58, Christoph Hellwig wrote:
> On Mon, Oct 09, 2023 at 11:29:12AM +0100, Robin Murphy wrote:
>> It looks a bit odd that this ends up applying to all of Coldfire, while the
>> associated cache flush only applies to the M532x platform, which implies
>> that we'd now be relying on the non-coherent allocation actually being
>> coherent on other Coldfire platforms.
>>
>> Would it work to do something like this to make sure dma-direct does the
>> right thing on such platforms (which presumably don't have caches?), and
>> then reduce the scope of this FEC hack accordingly, to clean things up even
>> better?
> 
> Probably.  Actually Greg comment something along the lines last
> time, and mentioned something about just instruction vs instruction
> and data cache.

I just elaborated on that point a little in response to Robin's email.

>>
>> diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
>> index b826e9c677b2..1851fa3fe077 100644
>> --- a/arch/m68k/Kconfig.cpu
>> +++ b/arch/m68k/Kconfig.cpu
>> @@ -27,6 +27,7 @@ config COLDFIRE
>>   	select CPU_HAS_NO_BITFIELDS
>>   	select CPU_HAS_NO_CAS
>>   	select CPU_HAS_NO_MULDIV64
>> +	select DMA_DEFAULT_COHERENT if !MMU && !M523x
> 
> Although it would probably make more sense to simply not select
> CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE and
> CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU for these platforms and not
> build the non-coherent code at all.  This should also include
> all coldfire platforms with mmu (M54xx/M548x/M5441x).  Then
> again for many of the coldfire platforms the Kconfig allows
> to select CACHE_WRITETHRU/CACHE_COPYBACK which looks related.
> 
> Greg, any chance you could help out with the caching modes on
> coldfire and legacy m68knommu?

Sure, yep. I am not aware that the legacy 68000 or 68328 had any caches
at all.

The cache modes change a bit through out the various ColdFire family series, but
can be broken down roughly into 2 groups.

1.  Version 2 cores (so everything named 52xx). Early members (5206, 5206e, 5272)
     had instruction cache only. Later members (5208, 5271/5275, 523x, etc) had
     a selectable instruction or data or both cache arrangement. Kconfig lets you
     select which you want - the default is instruction cache only.

2.  Version 3 and 4 cores (so everything named 53xx and 54xx). They have a unified
     instruction and data cache. Data caching can be selected to be write-through
     (the default) or write-back.

Some of the version 4 cores also have an MMU.

The M532x hack in the fec driver is to deal with its unified cache, and it is the
only ColdFire version 3 or 4 SoC that has the fec hardware module (thus no others
listed out there). I suspect if you select data cache on the version 2 cores that have
it would break in the fec driver too.

Regards
Greg

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Geert Uytterhoeven]

Hi Greg,

On Tue, Oct 10, 2023 at 4:45 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
> On 9/10/23 22:58, Christoph Hellwig wrote:
> > On Mon, Oct 09, 2023 at 11:29:12AM +0100, Robin Murphy wrote:
> >> It looks a bit odd that this ends up applying to all of Coldfire, while the
> >> associated cache flush only applies to the M532x platform, which implies
> >> that we'd now be relying on the non-coherent allocation actually being
> >> coherent on other Coldfire platforms.
> >>
> >> Would it work to do something like this to make sure dma-direct does the
> >> right thing on such platforms (which presumably don't have caches?), and
> >> then reduce the scope of this FEC hack accordingly, to clean things up even
> >> better?
> >
> > Probably.  Actually Greg comment something along the lines last
> > time, and mentioned something about just instruction vs instruction
> > and data cache.
>
> I just elaborated on that point a little in response to Robin's email.
>
> >>
> >> diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
> >> index b826e9c677b2..1851fa3fe077 100644
> >> --- a/arch/m68k/Kconfig.cpu
> >> +++ b/arch/m68k/Kconfig.cpu
> >> @@ -27,6 +27,7 @@ config COLDFIRE
> >>      select CPU_HAS_NO_BITFIELDS
> >>      select CPU_HAS_NO_CAS
> >>      select CPU_HAS_NO_MULDIV64
> >> +    select DMA_DEFAULT_COHERENT if !MMU && !M523x
> >
> > Although it would probably make more sense to simply not select
> > CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE and
> > CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU for these platforms and not
> > build the non-coherent code at all.  This should also include
> > all coldfire platforms with mmu (M54xx/M548x/M5441x).  Then
> > again for many of the coldfire platforms the Kconfig allows
> > to select CACHE_WRITETHRU/CACHE_COPYBACK which looks related.
> >
> > Greg, any chance you could help out with the caching modes on
> > coldfire and legacy m68knommu?
>
> Sure, yep. I am not aware that the legacy 68000 or 68328 had any caches
> at all.

68000 (and derivatives like 68*328) does not have any cache.
68360 (which is no longer supported by Linux) is based on CPU32, and
does not seem to have any caches, although the documentation does
mention the use of "external cache memories".

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Geert Uytterhoeven]

On Mon, Oct 16, 2023 at 11:12 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Oct 10, 2023 at 4:45 PM Greg Ungerer <gerg@linux-m68k.org> wrote:
> > On 9/10/23 22:58, Christoph Hellwig wrote:
> > > On Mon, Oct 09, 2023 at 11:29:12AM +0100, Robin Murphy wrote:
> > >> It looks a bit odd that this ends up applying to all of Coldfire, while the
> > >> associated cache flush only applies to the M532x platform, which implies
> > >> that we'd now be relying on the non-coherent allocation actually being
> > >> coherent on other Coldfire platforms.
> > >>
> > >> Would it work to do something like this to make sure dma-direct does the
> > >> right thing on such platforms (which presumably don't have caches?), and
> > >> then reduce the scope of this FEC hack accordingly, to clean things up even
> > >> better?
> > >
> > > Probably.  Actually Greg comment something along the lines last
> > > time, and mentioned something about just instruction vs instruction
> > > and data cache.
> >
> > I just elaborated on that point a little in response to Robin's email.
> >
> > >>
> > >> diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
> > >> index b826e9c677b2..1851fa3fe077 100644
> > >> --- a/arch/m68k/Kconfig.cpu
> > >> +++ b/arch/m68k/Kconfig.cpu
> > >> @@ -27,6 +27,7 @@ config COLDFIRE
> > >>      select CPU_HAS_NO_BITFIELDS
> > >>      select CPU_HAS_NO_CAS
> > >>      select CPU_HAS_NO_MULDIV64
> > >> +    select DMA_DEFAULT_COHERENT if !MMU && !M523x
> > >
> > > Although it would probably make more sense to simply not select
> > > CONFIG_ARCH_HAS_SYNC_DMA_FOR_DEVICE and
> > > CONFIG_ARCH_HAS_SYNC_DMA_FOR_CPU for these platforms and not
> > > build the non-coherent code at all.  This should also include
> > > all coldfire platforms with mmu (M54xx/M548x/M5441x).  Then
> > > again for many of the coldfire platforms the Kconfig allows
> > > to select CACHE_WRITETHRU/CACHE_COPYBACK which looks related.
> > >
> > > Greg, any chance you could help out with the caching modes on
> > > coldfire and legacy m68knommu?
> >
> > Sure, yep. I am not aware that the legacy 68000 or 68328 had any caches
> > at all.
>
> 68000 (and derivatives like 68*328) does not have any cache.
> 68360 (which is no longer supported by Linux) is based on CPU32, and
> does not seem to have any caches, although the documentation does
> mention the use of "external cache memories".

As M68K selects NO_DMA if !MMU && !COLDFIRE anyway, this doesn't
matter for legacy^Wclassic m68knommu.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Greg Ungerer]

Hi Robin,

On 9/10/23 20:29, Robin Murphy wrote:
> On 2023-10-09 08:41, Christoph Hellwig wrote:
>> The coldfire platforms can't properly implement dma_alloc_coherent and
>> currently just return noncoherent memory from dma_alloc_coherent.
>>
>> The fec driver than works around this with a flush of all caches in the
>> receive path. Make this hack a little less bad by using the explicit
>> dma_alloc_noncoherent API and documenting the hacky cache flushes so
>> that the DMA API level hack can be removed.
>>
>> Signed-off-by: Christoph Hellwig <hch@lst.de>
>> ---
>>   drivers/net/ethernet/freescale/fec_main.c | 84 ++++++++++++++++++++---
>>   1 file changed, 75 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
>> index 77c8e9cfb44562..aa032ea0ebf0c2 100644
>> --- a/drivers/net/ethernet/freescale/fec_main.c
>> +++ b/drivers/net/ethernet/freescale/fec_main.c
>> @@ -406,6 +406,70 @@ static void fec_dump(struct net_device *ndev)
>>       } while (bdp != txq->bd.base);
>>   }
>> +/*
>> + * Coldfire does not support DMA coherent allocations, and has historically used
>> + * a band-aid with a manual flush in fec_enet_rx_queue.
>> + */
>> +#ifdef CONFIG_COLDFIRE
> 
> It looks a bit odd that this ends up applying to all of Coldfire, while the associated cache flush only applies to the M532x platform, which implies that we'd now be relying on the non-coherent allocation actually being coherent on other Coldfire platforms.
> 
> Would it work to do something like this to make sure dma-direct does the right thing on such platforms (which presumably don't have caches?), and then reduce the scope of this FEC hack accordingly, to clean things up even better?
> 
> diff --git a/arch/m68k/Kconfig.cpu b/arch/m68k/Kconfig.cpu
> index b826e9c677b2..1851fa3fe077 100644
> --- a/arch/m68k/Kconfig.cpu
> +++ b/arch/m68k/Kconfig.cpu
> @@ -27,6 +27,7 @@ config COLDFIRE
>       select CPU_HAS_NO_BITFIELDS
>       select CPU_HAS_NO_CAS
>       select CPU_HAS_NO_MULDIV64
> +    select DMA_DEFAULT_COHERENT if !MMU && !M523x

That should be M532x.

I am pretty sure the code as-is today is broken for the case of using
the split cache arrangement (so both instruction and data cache) for any
of the version 2 cores too (denoted by the HAVE_CACHE_SPLIT option).
But that has probably not been picked up because the default on those
has always been instruction cache only.

The reason for the special case for the M532x series is that it is a version 3
core and they have a unified instruction and data cache. The 523x series is the
only version 3 core that Linux supports that has the FEC hardware module.

Regards
Greg


>       select GENERIC_CSUM
>       select GPIOLIB
>       select HAVE_LEGACY_CLK
> 
> 
> Thanks,
> Robin.
> 
>> +static void *fec_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>> +        gfp_t gfp)
>> +{
>> +    return dma_alloc_noncoherent(dev, size, handle, DMA_BIDIRECTIONAL, gfp);
>> +}
>> +
>> +static void fec_dma_free(struct device *dev, size_t size, void *cpu_addr,
>> +        dma_addr_t handle)
>> +{
>> +    dma_free_noncoherent(dev, size, cpu_addr, handle, DMA_BIDIRECTIONAL);
>> +}
>> +#else /* CONFIG_COLDFIRE */
>> +static void *fec_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>> +        gfp_t gfp)
>> +{
>> +    return dma_alloc_coherent(dev, size, handle, gfp);
>> +}
>> +
>> +static void fec_dma_free(struct device *dev, size_t size, void *cpu_addr,
>> +        dma_addr_t handle)
>> +{
>> +    dma_free_coherent(dev, size, cpu_addr, handle);
>> +}
>> +#endif /* !CONFIG_COLDFIRE */
>> +
>> +struct fec_dma_devres {
>> +    size_t        size;
>> +    void        *vaddr;
>> +    dma_addr_t    dma_handle;
>> +};
>> +
>> +static void fec_dmam_release(struct device *dev, void *res)
>> +{
>> +    struct fec_dma_devres *this = res;
>> +
>> +    fec_dma_free(dev, this->size, this->vaddr, this->dma_handle);
>> +}
>> +
>> +static void *fec_dmam_alloc(struct device *dev, size_t size, dma_addr_t *handle,
>> +        gfp_t gfp)
>> +{
>> +    struct fec_dma_devres *dr;
>> +    void *vaddr;
>> +
>> +    dr = devres_alloc(fec_dmam_release, sizeof(*dr), gfp);
>> +    if (!dr)
>> +        return NULL;
>> +    vaddr = fec_dma_alloc(dev, size, handle, gfp);
>> +    if (!vaddr) {
>> +        devres_free(dr);
>> +        return NULL;
>> +    }
>> +    dr->vaddr = vaddr;
>> +    dr->dma_handle = *handle;
>> +    dr->size = size;
>> +    devres_add(dev, dr);
>> +    return vaddr;
>> +}
>> +
>>   static inline bool is_ipv4_pkt(struct sk_buff *skb)
>>   {
>>       return skb->protocol == htons(ETH_P_IP) && ip_hdr(skb)->version == 4;
>> @@ -1661,6 +1725,10 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>>   #endif
>>   #ifdef CONFIG_M532x
>> +    /*
>> +     * Hacky flush of all caches instead of using the DMA API for the TSO
>> +     * headers.
>> +     */
>>       flush_cache_all();
>>   #endif
>>       rxq = fep->rx_queue[queue_id];
>> @@ -3288,10 +3356,9 @@ static void fec_enet_free_queue(struct net_device *ndev)
>>       for (i = 0; i < fep->num_tx_queues; i++)
>>           if (fep->tx_queue[i] && fep->tx_queue[i]->tso_hdrs) {
>>               txq = fep->tx_queue[i];
>> -            dma_free_coherent(&fep->pdev->dev,
>> -                      txq->bd.ring_size * TSO_HEADER_SIZE,
>> -                      txq->tso_hdrs,
>> -                      txq->tso_hdrs_dma);
>> +            fec_dma_free(&fep->pdev->dev,
>> +                     txq->bd.ring_size * TSO_HEADER_SIZE,
>> +                     txq->tso_hdrs, txq->tso_hdrs_dma);
>>           }
>>       for (i = 0; i < fep->num_rx_queues; i++)
>> @@ -3321,10 +3388,9 @@ static int fec_enet_alloc_queue(struct net_device *ndev)
>>           txq->tx_stop_threshold = FEC_MAX_SKB_DESCS;
>>           txq->tx_wake_threshold = FEC_MAX_SKB_DESCS + 2 * MAX_SKB_FRAGS;
>> -        txq->tso_hdrs = dma_alloc_coherent(&fep->pdev->dev,
>> +        txq->tso_hdrs = fec_dma_alloc(&fep->pdev->dev,
>>                       txq->bd.ring_size * TSO_HEADER_SIZE,
>> -                    &txq->tso_hdrs_dma,
>> -                    GFP_KERNEL);
>> +                    &txq->tso_hdrs_dma, GFP_KERNEL);
>>           if (!txq->tso_hdrs) {
>>               ret = -ENOMEM;
>>               goto alloc_failed;
>> @@ -4043,8 +4109,8 @@ static int fec_enet_init(struct net_device *ndev)
>>       bd_size = (fep->total_tx_ring_size + fep->total_rx_ring_size) * dsize;
>>       /* Allocate memory for buffer descriptors. */
>> -    cbd_base = dmam_alloc_coherent(&fep->pdev->dev, bd_size, &bd_dma,
>> -                       GFP_KERNEL);
>> +    cbd_base = fec_dmam_alloc(&fep->pdev->dev, bd_size, &bd_dma,
>> +                  GFP_KERNEL);
>>       if (!cbd_base) {
>>           ret = -ENOMEM;
>>           goto free_queue_mem;

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Christoph Hellwig]

On Wed, Oct 11, 2023 at 12:20:57AM +1000, Greg Ungerer wrote:
> That should be M532x.
>
> I am pretty sure the code as-is today is broken for the case of using
> the split cache arrangement (so both instruction and data cache) for any
> of the version 2 cores too (denoted by the HAVE_CACHE_SPLIT option).
> But that has probably not been picked up because the default on those
> has always been instruction cache only.
>
> The reason for the special case for the M532x series is that it is a version 3
> core and they have a unified instruction and data cache. The 523x series is the
> only version 3 core that Linux supports that has the FEC hardware module.

So what config option should we check for supporting coherent allocations
and which not having the hack in fec?

Here is my guesses based on the above:

in m68k support coherent allocations with no work if

CONFIG_COLDIFRE is set and neither CONFIG_CACHE_D or CONFIG_CACHE_BOTH
is set.

in the fec driver do the alloc_noncoherent and global cache flush
hack if:

COMFIG_COLDFIRE && (CONFIG_CACHE_D || CONFIG_CACHE_BOTH)

?

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Greg Ungerer]

On 11/10/23 15:52, Christoph Hellwig wrote:
> On Wed, Oct 11, 2023 at 12:20:57AM +1000, Greg Ungerer wrote:
>> That should be M532x.
>>
>> I am pretty sure the code as-is today is broken for the case of using
>> the split cache arrangement (so both instruction and data cache) for any
>> of the version 2 cores too (denoted by the HAVE_CACHE_SPLIT option).
>> But that has probably not been picked up because the default on those
>> has always been instruction cache only.
>>
>> The reason for the special case for the M532x series is that it is a version 3
>> core and they have a unified instruction and data cache. The 523x series is the
>> only version 3 core that Linux supports that has the FEC hardware module.
> 
> So what config option should we check for supporting coherent allocations
> and which not having the hack in fec?
> 
> Here is my guesses based on the above:
> 
> in m68k support coherent allocations with no work if
> 
> CONFIG_COLDIFRE is set and neither CONFIG_CACHE_D or CONFIG_CACHE_BOTH
> is set.

I think this needs to be CONFIG_COLDFIRE is set and none of CONFIG_HAVE_CACHE_CB or
CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set.



> in the fec driver do the alloc_noncoherent and global cache flush
> hack if:
> 
> COMFIG_COLDFIRE && (CONFIG_CACHE_D || CONFIG_CACHE_BOTH)

And then this becomes:

CONFIG_COLDFIRE && (CONFIG_HAVE_CACHE_CB || CONFIG_CACHE_D || CONFIG_CACHE_BOTH)

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Michael Schmitz]

Hi Greg,

On 12/10/23 02:09, Greg Ungerer wrote:
>
> I think this needs to be CONFIG_COLDFIRE is set and none of 
> CONFIG_HAVE_CACHE_CB or
> CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set.
>
>
>
>> in the fec driver do the alloc_noncoherent and global cache flush
>> hack if:
>>
>> COMFIG_COLDFIRE && (CONFIG_CACHE_D || CONFIG_CACHE_BOTH)
>
> And then this becomes:
>
> CONFIG_COLDFIRE && (CONFIG_HAVE_CACHE_CB || CONFIG_CACHE_D || 
> CONFIG_CACHE_BOTH)

You appear to have dropped a '!' there ...

Cheers,

     Michael

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Greg Ungerer]

Hi Michael,

On 12/10/23 04:21, Michael Schmitz wrote:
> Hi Greg,
> 
> On 12/10/23 02:09, Greg Ungerer wrote:
>>
>> I think this needs to be CONFIG_COLDFIRE is set and none of CONFIG_HAVE_CACHE_CB or
>> CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set.
>>
>>
>>
>>> in the fec driver do the alloc_noncoherent and global cache flush
>>> hack if:
>>>
>>> COMFIG_COLDFIRE && (CONFIG_CACHE_D || CONFIG_CACHE_BOTH)
>>
>> And then this becomes:
>>
>> CONFIG_COLDFIRE && (CONFIG_HAVE_CACHE_CB || CONFIG_CACHE_D || CONFIG_CACHE_BOTH)
> 
> You appear to have dropped a '!' there ...

Not sure I follow. This is the opposite of the case above. The noncoherent alloc
and cache flush should be performed if ColdFire and any of CONFIG_HAVE_CACHE_CB,
CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set - since that means there is data
caching involved.

Regards
Greg

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Christoph Hellwig]

On Thu, Oct 12, 2023 at 11:25:00PM +1000, Greg Ungerer wrote:
> Not sure I follow. This is the opposite of the case above. The noncoherent alloc
> and cache flush should be performed if ColdFire and any of CONFIG_HAVE_CACHE_CB,
> CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set - since that means there is data
> caching involved.

FYI, this is what I ended up with this morning:

http://git.infradead.org/users/hch/misc.git/commitdiff/ea7c8c5ca3f158f88594f4f1c9a52735115f9aca

Whole branch:

http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-coherent-deps

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Greg Ungerer]

Hi Christoph,

On 13/10/23 00:00, Christoph Hellwig wrote:
> On Thu, Oct 12, 2023 at 11:25:00PM +1000, Greg Ungerer wrote:
>> Not sure I follow. This is the opposite of the case above. The noncoherent alloc
>> and cache flush should be performed if ColdFire and any of CONFIG_HAVE_CACHE_CB,
>> CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set - since that means there is data
>> caching involved.
> 
> FYI, this is what I ended up with this morning:
> 
> http://git.infradead.org/users/hch/misc.git/commitdiff/ea7c8c5ca3f158f88594f4f1c9a52735115f9aca

That looks nice.

> Whole branch:
> 
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/dma-coherent-deps

I'll try and give this some testing next week.

Thanks
Greg

Re: [PATCH 5/6] net: fec: use dma_alloc_noncoherent for m532x

[Michael Schmitz]

Hi Greg,

On 13/10/23 02:25, Greg Ungerer wrote:
> Hi Michael,
>
> On 12/10/23 04:21, Michael Schmitz wrote:
>> Hi Greg,
>>
>> On 12/10/23 02:09, Greg Ungerer wrote:
>>>
>>> I think this needs to be CONFIG_COLDFIRE is set and none of 
>>> CONFIG_HAVE_CACHE_CB or
>>> CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set.
>>>
>>>
>>>> in the fec driver do the alloc_noncoherent and global cache flush
>>>> hack if:
>>>>
>>>> COMFIG_COLDFIRE && (CONFIG_CACHE_D || CONFIG_CACHE_BOTH)
>>>
>>> And then this becomes:
>>>
>>> CONFIG_COLDFIRE && (CONFIG_HAVE_CACHE_CB || CONFIG_CACHE_D || 
>>> CONFIG_CACHE_BOTH)
>>
>> You appear to have dropped a '!' there ...
>
> Not sure I follow. This is the opposite of the case above. The 
> noncoherent alloc
> and cache flush should be performed if ColdFire and any of 
> CONFIG_HAVE_CACHE_CB,
> CONFIG_CACHE_D or CONFIG_CACHE_BOTH are set - since that means there 
> is data
> caching involved.

Now I see - I had read that definition to correspond to the 'select 
coherent allocations on m68k' case, not the latter 'use non-coherent 
allocations in the fec driver'.

Your definitions are correct as written (and for all I can see, 
Christoph's implementation is correct, too).

Apologies for the noise - I blame jet leg for getting my wires crossed ...

Cheers,

     Michael

>
> Regards
> Greg
>

[PATCH 6/6] m68k: don't provide arch_dma_alloc for nommu/coldfire

[Christoph Hellwig]

m68knommu and coldfire can't provide coherent DMA allocations at all.
Currently they simply return normal kernel memory from
dma_alloc_coherent, which is broken and breaks the API contract.  Now
that the only DMA capable driver on these systems has been switched
to use explicitly non-coherent allocations we can drop this hack and
return NULL from dma_alloc_coherent.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/m68k/Kconfig      |  1 -
 arch/m68k/kernel/dma.c | 23 -----------------------
 2 files changed, 24 deletions(-)

diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 0430b8ba6b5cc6..3e318bf9504c5b 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -3,7 +3,6 @@ config M68K
 	bool
 	default y
 	select ARCH_32BIT_OFF_T
-	select ARCH_DMA_ALLOC if !MMU || COLDFIRE
 	select ARCH_HAS_BINFMT_FLAT
 	select ARCH_HAS_CPU_FINALIZE_INIT if MMU
 	select ARCH_HAS_CURRENT_STACK_POINTER
diff --git a/arch/m68k/kernel/dma.c b/arch/m68k/kernel/dma.c
index 2e192a5df949bb..eb164ef1a45ebd 100644
--- a/arch/m68k/kernel/dma.c
+++ b/arch/m68k/kernel/dma.c
@@ -33,29 +33,6 @@ pgprot_t pgprot_dmacoherent(pgprot_t prot)
 	}
 	return prot;
 }
-#else
-void *arch_dma_alloc(struct device *dev, size_t size, dma_addr_t *dma_handle,
-		gfp_t gfp, unsigned long attrs)
-{
-	void *ret;
-
-	if (dev == NULL || (*dev->dma_mask < 0xffffffff))
-		gfp |= GFP_DMA;
-	ret = (void *)__get_free_pages(gfp, get_order(size));
-
-	if (ret != NULL) {
-		memset(ret, 0, size);
-		*dma_handle = virt_to_phys(ret);
-	}
-	return ret;
-}
-
-void arch_dma_free(struct device *dev, size_t size, void *vaddr,
-		dma_addr_t dma_handle, unsigned long attrs)
-{
-	free_pages((unsigned long)vaddr, get_order(size));
-}
-
 #endif /* CONFIG_MMU && !CONFIG_COLDFIRE */
 
 void arch_sync_dma_for_device(phys_addr_t handle, size_t size,
-- 
2.39.2

Re: fix the non-coherent coldfire dma_alloc_coherent

[Geert Uytterhoeven]

CC Greg

On Mon, Oct 9, 2023 at 9:41 AM Christoph Hellwig <hch@lst.de> wrote:
> this is the next attempt to not return memory that is not DMA coherent
> on coldfire/m68knommu.  The last one needed more fixups in the fec
> driver, which this versions includes.  On top of that I've also added
> a few more cleanups to the core dma allocation code.
>
> Jim: any work to support the set_uncached and remap method for arm32
> should probably be based on this, and patch 3 should make that
> selection a little easier.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: fix the non-coherent coldfire dma_alloc_coherent

[Christoph Hellwig]

On Mon, Oct 09, 2023 at 10:39:18AM +0200, Geert Uytterhoeven wrote:
> CC Greg

Ooops.  I was pretty sure I added him as he is one of the most
relevant persons for this.  Sorry.