fix dma_alloc_coherent on m68knommu / coldfire

fix dma_alloc_coherent on m68knommu / coldfire

[Christoph Hellwig]

Hi all,

this series ensures dma_alloc_coherent returns NULL for m68knommu and
coldfire given that these platforms can't actually allocate dma coherent
memory after fixing what is according to an old mail from Greg the
only driver that actually uses memory returned by dma_alloc_coherent on
those platforms.

[PATCH 1/2] net: fec: use dma_alloc_noncoherent for m532x

[Christoph Hellwig]

The m532x coldfire platforms can't properly implement dma_alloc_coherent
and currently just return noncoherent memory from it.  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.

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

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 28ef4d3c18789..5230698310b5e 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1580,6 +1580,10 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
 	struct page *page;
 
 #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];
@@ -3123,10 +3127,17 @@ 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];
+#ifdef CONFIG_M532x
 			dma_free_coherent(&fep->pdev->dev,
 					  txq->bd.ring_size * TSO_HEADER_SIZE,
 					  txq->tso_hdrs,
 					  txq->tso_hdrs_dma);
+#else
+			dma_free_noncoherent(&fep->pdev->dev,
+					  txq->bd.ring_size * TSO_HEADER_SIZE,
+					  txq->tso_hdrs, txq->tso_hdrs_dma,
+					  DMA_BIDIRECTIONAL);
+#endif
 		}
 
 	for (i = 0; i < fep->num_rx_queues; i++)
@@ -3157,10 +3168,18 @@ static int fec_enet_alloc_queue(struct net_device *ndev)
 		txq->tx_wake_threshold =
 			(txq->bd.ring_size - txq->tx_stop_threshold) / 2;
 
+#ifdef CONFIG_M532x
 		txq->tso_hdrs = dma_alloc_coherent(&fep->pdev->dev,
 					txq->bd.ring_size * TSO_HEADER_SIZE,
 					&txq->tso_hdrs_dma,
 					GFP_KERNEL);
+#else
+		/* m68knommu manually flushes all caches in fec_enet_rx_queue */
+		txq->tso_hdrs = dma_alloc_noncoherent(&fep->pdev->dev,
+					txq->bd.ring_size * TSO_HEADER_SIZE,
+					&txq->tso_hdrs_dma, DMA_BIDIRECTIONAL,
+					GFP_KERNEL);
+#endif
 		if (!txq->tso_hdrs) {
 			ret = -ENOMEM;
 			goto alloc_failed;
-- 
2.30.2

[PATCH 2/2] m68k: return NULL from dma_alloc_coherent for nommu or coldfire

[Christoph Hellwig]

m68knommu and coldfire platforms can't support allocating coherent DMA
memory.  Instead of just returning noncoherent memory, just fail the
allocation.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/m68k/kernel/dma.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/arch/m68k/kernel/dma.c b/arch/m68k/kernel/dma.c
index 2e192a5df949b..63618fce5dc7f 100644
--- a/arch/m68k/kernel/dma.c
+++ b/arch/m68k/kernel/dma.c
@@ -37,23 +37,13 @@ pgprot_t pgprot_dmacoherent(pgprot_t prot)
 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;
+	return NULL;
 }
 
 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));
+	WARN_ON_ONCE(1);
 }
 
 #endif /* CONFIG_MMU && !CONFIG_COLDFIRE */
-- 
2.30.2

Re: [PATCH 1/2] net: fec: use dma_alloc_noncoherent for m532x

[Geert Uytterhoeven]

Hi Christoph,

On Mon, Nov 21, 2022 at 10:56 AM Christoph Hellwig <hch@lst.de> wrote:
> The m532x coldfire platforms can't properly implement dma_alloc_coherent
> and currently just return noncoherent memory from it.  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.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Thanks for your patch!

> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1580,6 +1580,10 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>         struct page *page;
>
>  #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];
> @@ -3123,10 +3127,17 @@ 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];
> +#ifdef CONFIG_M532x

Shouldn't this be the !CONFIG_M532x path?

>                         dma_free_coherent(&fep->pdev->dev,
>                                           txq->bd.ring_size * TSO_HEADER_SIZE,
>                                           txq->tso_hdrs,
>                                           txq->tso_hdrs_dma);
> +#else
> +                       dma_free_noncoherent(&fep->pdev->dev,
> +                                         txq->bd.ring_size * TSO_HEADER_SIZE,
> +                                         txq->tso_hdrs, txq->tso_hdrs_dma,
> +                                         DMA_BIDIRECTIONAL);
> +#endif
>                 }
>
>         for (i = 0; i < fep->num_rx_queues; i++)
> @@ -3157,10 +3168,18 @@ static int fec_enet_alloc_queue(struct net_device *ndev)
>                 txq->tx_wake_threshold =
>                         (txq->bd.ring_size - txq->tx_stop_threshold) / 2;
>
> +#ifdef CONFIG_M532x

Likewise

>                 txq->tso_hdrs = dma_alloc_coherent(&fep->pdev->dev,
>                                         txq->bd.ring_size * TSO_HEADER_SIZE,
>                                         &txq->tso_hdrs_dma,
>                                         GFP_KERNEL);
> +#else
> +               /* m68knommu manually flushes all caches in fec_enet_rx_queue */
> +               txq->tso_hdrs = dma_alloc_noncoherent(&fep->pdev->dev,
> +                                       txq->bd.ring_size * TSO_HEADER_SIZE,
> +                                       &txq->tso_hdrs_dma, DMA_BIDIRECTIONAL,
> +                                       GFP_KERNEL);
> +#endif
>                 if (!txq->tso_hdrs) {
>                         ret = -ENOMEM;
>                         goto alloc_failed;

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/2] net: fec: use dma_alloc_noncoherent for m532x

[Christoph Hellwig]

On Mon, Nov 21, 2022 at 11:24:29AM +0100, Geert Uytterhoeven wrote:
> > +#ifdef CONFIG_M532x
> 
> Shouldn't this be the !CONFIG_M532x path?

Yes.

Re: [PATCH 1/2] net: fec: use dma_alloc_noncoherent for m532x

[Greg Ungerer]

Hi Christoph,

On 21/11/22 19:56, Christoph Hellwig wrote:
> The m532x coldfire platforms can't properly implement dma_alloc_coherent
> and currently just return noncoherent memory from it.  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.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>   drivers/net/ethernet/freescale/fec_main.c | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 28ef4d3c18789..5230698310b5e 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1580,6 +1580,10 @@ fec_enet_rx_queue(struct net_device *ndev, int budget, u16 queue_id)
>   	struct page *page;
>   
>   #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];
> @@ -3123,10 +3127,17 @@ 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];
> +#ifdef CONFIG_M532x
>   			dma_free_coherent(&fep->pdev->dev,
>   					  txq->bd.ring_size * TSO_HEADER_SIZE,
>   					  txq->tso_hdrs,
>   					  txq->tso_hdrs_dma);
> +#else
> +			dma_free_noncoherent(&fep->pdev->dev,
> +					  txq->bd.ring_size * TSO_HEADER_SIZE,
> +					  txq->tso_hdrs, txq->tso_hdrs_dma,
> +					  DMA_BIDIRECTIONAL);
> +#endif
>   		}
>   
>   	for (i = 0; i < fep->num_rx_queues; i++)
> @@ -3157,10 +3168,18 @@ static int fec_enet_alloc_queue(struct net_device *ndev)
>   		txq->tx_wake_threshold =
>   			(txq->bd.ring_size - txq->tx_stop_threshold) / 2;
>   
> +#ifdef CONFIG_M532x
>   		txq->tso_hdrs = dma_alloc_coherent(&fep->pdev->dev,
>   					txq->bd.ring_size * TSO_HEADER_SIZE,
>   					&txq->tso_hdrs_dma,
>   					GFP_KERNEL);

Even with this corrected this will now end up failing on all other ColdFire types
with the FEC hardware module (all the non-M532x types) once the arch_dma_alloc()
returns NULL.

Did you mean "ifndef CONFIG_COLDFIRE" here?


> +#else
> +		/* m68knommu manually flushes all caches in fec_enet_rx_queue */
> +		txq->tso_hdrs = dma_alloc_noncoherent(&fep->pdev->dev,
> +					txq->bd.ring_size * TSO_HEADER_SIZE,
> +					&txq->tso_hdrs_dma, DMA_BIDIRECTIONAL,
> +					GFP_KERNEL);
> +#endif
>   		if (!txq->tso_hdrs) {
>   			ret = -ENOMEM;
>   			goto alloc_failed;

And what about the dmam_alloc_coherent() call in fec_enet_init()?
Does that need changing too?

Regards
Greg

Re: [PATCH 1/2] net: fec: use dma_alloc_noncoherent for m532x

[Christoph Hellwig]

On Thu, Dec 01, 2022 at 03:41:34PM +1000, Greg Ungerer wrote:
>>     #ifdef CONFIG_M532x
>> +	/*
>> +	 * Hacky flush of all caches instead of using the DMA API for the TSO
>> +	 * headers.
>> +	 */
>>   	flush_cache_all();
>
> Even with this corrected this will now end up failing on all other ColdFire types
> with the FEC hardware module (all the non-M532x types) once the arch_dma_alloc()
> returns NULL.
>
> Did you mean "ifndef CONFIG_COLDFIRE" here?

How did these work before given that the cache flush is conditional
on CONFIG_M532x?

>> +#else
>> +		/* m68knommu manually flushes all caches in fec_enet_rx_queue */
>> +		txq->tso_hdrs = dma_alloc_noncoherent(&fep->pdev->dev,
>> +					txq->bd.ring_size * TSO_HEADER_SIZE,
>> +					&txq->tso_hdrs_dma, DMA_BIDIRECTIONAL,
>> +					GFP_KERNEL);
>> +#endif
>>   		if (!txq->tso_hdrs) {
>>   			ret = -ENOMEM;
>>   			goto alloc_failed;
>
> And what about the dmam_alloc_coherent() call in fec_enet_init()?
> Does that need changing too?

If that's actually use by the FEC implementations on coldire: yes.
But maybe I need even more help on how the cache flushing is suppoѕed
to actually work here.

Re: [PATCH 1/2] net: fec: use dma_alloc_noncoherent for m532x

[Greg Ungerer]

On 2/12/22 17:02, Christoph Hellwig wrote:
> On Thu, Dec 01, 2022 at 03:41:34PM +1000, Greg Ungerer wrote:
>>>      #ifdef CONFIG_M532x
>>> +	/*
>>> +	 * Hacky flush of all caches instead of using the DMA API for the TSO
>>> +	 * headers.
>>> +	 */
>>>    	flush_cache_all();
>>
>> Even with this corrected this will now end up failing on all other ColdFire types
>> with the FEC hardware module (all the non-M532x types) once the arch_dma_alloc()
>> returns NULL.
>>
>> Did you mean "ifndef CONFIG_COLDFIRE" here?
> 
> How did these work before given that the cache flush is conditional
> on CONFIG_M532x?

The case for the 5272 is that it only has instruction cache, no data cache.
That was the first supported part, and the one I have used the most over
the years (though not so much recently). So it should be ok.

I am not convinced about the other version 2 cores either. They all have
both instruction and data cache - they can be flexibily configured to be
all instruction, or a mix of instruction and data - but the default is
both.

I don't have a 532x platform, so I have never done any work on that.
The flush_cache_all() for that seems dubious to me.


>>> +#else
>>> +		/* m68knommu manually flushes all caches in fec_enet_rx_queue */
>>> +		txq->tso_hdrs = dma_alloc_noncoherent(&fep->pdev->dev,
>>> +					txq->bd.ring_size * TSO_HEADER_SIZE,
>>> +					&txq->tso_hdrs_dma, DMA_BIDIRECTIONAL,
>>> +					GFP_KERNEL);
>>> +#endif
>>>    		if (!txq->tso_hdrs) {
>>>    			ret = -ENOMEM;
>>>    			goto alloc_failed;
>>
>> And what about the dmam_alloc_coherent() call in fec_enet_init()?
>> Does that need changing too?
> 
> If that's actually use by the FEC implementations on coldire: yes.

It is. Testing these changes failed at boot without changing this one too.


> But maybe I need even more help on how the cache flushing is suppoѕed
> to actually work here.

Regards
Greg