[RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Guenter Roeck]

As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
coherent_dma_mask") the NatSemi SONIC Ethernet driver is issuing the
following warning on driver initialization on Macintosh q800 systems.

SONIC ethernet @50f0a000, MAC 08:00:07:12:34:56, IRQ 3
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 macsonic_init+0x6a/0x15a
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.17.0-rc6-mac-00286-g527f47c #1
Stack from 0781fdd8:
        0781fdd8 003615b3 000181ba 000005c4 07a24cbc 00000000 00000000 000020e8
	07a24800 002c196c 0001824e 00334c06 00000204 001f782a 00000009 00000000
	00000000 003358d9 001f782a 00334c06 00000204 00000003 00000000 07a24800
	002b5cb6 000372ec 001f8b1a 07a24800 00359203 50f0a000 07a14a48 00000003
	00000000 07845c0a 0039dcca 003c835c 003c835c 0035b924 001c19de 07845c00
	07845c0a 0039dcca 001c06dc 07845c0a 0781fed8 00000007 0054d040 07845c0a
Call Trace: [<000181ba>] __warn+0xc0/0xc2
 [<000020e8>] do_one_initcall+0x0/0x140
 [<0001824e>] warn_slowpath_null+0x26/0x2c
 [<001f782a>] macsonic_init+0x6a/0x15a
 [<001f782a>] macsonic_init+0x6a/0x15a
 [<002b5cb6>] memcmp+0x0/0x2a
 [<000372ec>] printk+0x0/0x18
 [<001f8b1a>] mac_sonic_platform_probe+0x380/0x404

As per the warning the coherent_dma_mask is not set on this device.
There is nothing special about the DMA memory coherency on this hardware
so we can just set the mask to 32bits in the platform data for the FEC
ethernet devices.

Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Modeled after f61e64310b75 ("m68k: set dma and coherent masks for platform
FEC ethernets"). 

RFC: Is "nothing special about the DMA memory coherency" correect ?

 arch/m68k/mac/config.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
index 0c3275aa0197..8e0476daddb8 100644
--- a/arch/m68k/mac/config.c
+++ b/arch/m68k/mac/config.c
@@ -17,6 +17,7 @@
 #include <linux/tty.h>
 #include <linux/console.h>
 #include <linux/interrupt.h>
+#include <linux/dma-mapping.h>
 /* keyb */
 #include <linux/random.h>
 #include <linux/delay.h>
@@ -971,6 +972,15 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
 	},
 };
 
+static struct platform_device macsonic_dev = {
+	.name           = "macsonic",
+	.id             = -1,
+	.dev            = {
+		.dma_mask = &macsonic_dev.dev.coherent_dma_mask,
+		.coherent_dma_mask = DMA_BIT_MASK(32),
+	},
+};
+
 int __init mac_platform_init(void)
 {
 	u8 *swim_base;
@@ -1088,7 +1098,7 @@ int __init mac_platform_init(void)
 
 	if (macintosh_config->ether_type == MAC_ETHER_SONIC ||
 	    macintosh_config->expansion_type == MAC_EXP_PDS_COMM)
-		platform_device_register_simple("macsonic", -1, NULL, 0);
+		platform_device_register(&macsonic_dev);
 
 	if (macintosh_config->expansion_type == MAC_EXP_PDS ||
 	    macintosh_config->expansion_type == MAC_EXP_PDS_COMM)
-- 
2.7.4

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Finn Thain]

On Sat, 26 May 2018, Guenter Roeck wrote:

> As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
> coherent_dma_mask") the NatSemi SONIC Ethernet driver is issuing the
> following warning on driver initialization on Macintosh q800 systems.
> 
> SONIC ethernet @50f0a000, MAC 08:00:07:12:34:56, IRQ 3
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 macsonic_init+0x6a/0x15a
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.17.0-rc6-mac-00286-g527f47c #1
> Stack from 0781fdd8:
>         0781fdd8 003615b3 000181ba 000005c4 07a24cbc 00000000 00000000 000020e8
> 	07a24800 002c196c 0001824e 00334c06 00000204 001f782a 00000009 00000000
> 	00000000 003358d9 001f782a 00334c06 00000204 00000003 00000000 07a24800
> 	002b5cb6 000372ec 001f8b1a 07a24800 00359203 50f0a000 07a14a48 00000003
> 	00000000 07845c0a 0039dcca 003c835c 003c835c 0035b924 001c19de 07845c00
> 	07845c0a 0039dcca 001c06dc 07845c0a 0781fed8 00000007 0054d040 07845c0a
> Call Trace: [<000181ba>] __warn+0xc0/0xc2
>  [<000020e8>] do_one_initcall+0x0/0x140
>  [<0001824e>] warn_slowpath_null+0x26/0x2c
>  [<001f782a>] macsonic_init+0x6a/0x15a
>  [<001f782a>] macsonic_init+0x6a/0x15a
>  [<002b5cb6>] memcmp+0x0/0x2a
>  [<000372ec>] printk+0x0/0x18
>  [<001f8b1a>] mac_sonic_platform_probe+0x380/0x404
> 
> As per the warning the coherent_dma_mask is not set on this device.
> There is nothing special about the DMA memory coherency on this hardware
> so we can just set the mask to 32bits in the platform data for the FEC
> ethernet devices.
> 
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Acked-by: Finn Thain <fthain@telegraphics.com.au>

Geert, assuming that Guenter's patch is acceptable, would you also accept 
a similar patch for the "macmace" platform device?

> ---
> Modeled after f61e64310b75 ("m68k: set dma and coherent masks for platform
> FEC ethernets"). 
> 
> RFC: Is "nothing special about the DMA memory coherency" correect ?
> 

As I understand it, "cache-coherence" is meaningless unless you have 
multiple CPU cores and caches. If there's only one CPU core, its cache 
can't be said to be "coherent" or "non-coherent". Moreover, DMA (direct 
memory access) concerns an IO device and a memory, not a cache, so the 
term "coherent dma" is bogus IMHO.

-- 

>  arch/m68k/mac/config.c | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/m68k/mac/config.c b/arch/m68k/mac/config.c
> index 0c3275aa0197..8e0476daddb8 100644
> --- a/arch/m68k/mac/config.c
> +++ b/arch/m68k/mac/config.c
> @@ -17,6 +17,7 @@
>  #include <linux/tty.h>
>  #include <linux/console.h>
>  #include <linux/interrupt.h>
> +#include <linux/dma-mapping.h>
>  /* keyb */
>  #include <linux/random.h>
>  #include <linux/delay.h>
> @@ -971,6 +972,15 @@ static const struct resource mac_scsi_ccl_rsrc[] __initconst = {
>  	},
>  };
>  
> +static struct platform_device macsonic_dev = {
> +	.name           = "macsonic",
> +	.id             = -1,
> +	.dev            = {
> +		.dma_mask = &macsonic_dev.dev.coherent_dma_mask,
> +		.coherent_dma_mask = DMA_BIT_MASK(32),
> +	},
> +};
> +
>  int __init mac_platform_init(void)
>  {
>  	u8 *swim_base;
> @@ -1088,7 +1098,7 @@ int __init mac_platform_init(void)
>  
>  	if (macintosh_config->ether_type == MAC_ETHER_SONIC ||
>  	    macintosh_config->expansion_type == MAC_EXP_PDS_COMM)
> -		platform_device_register_simple("macsonic", -1, NULL, 0);
> +		platform_device_register(&macsonic_dev);
>  
>  	if (macintosh_config->expansion_type == MAC_EXP_PDS ||
>  	    macintosh_config->expansion_type == MAC_EXP_PDS_COMM)
> -- 
> 2.7.4
> 

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Guenter Roeck]

Hi,

On Sun, May 27, 2018 at 01:01:11PM +1000, Finn Thain wrote:
> On Sat, 26 May 2018, Guenter Roeck wrote:
> 
> > As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
> > coherent_dma_mask") the NatSemi SONIC Ethernet driver is issuing the
> > following warning on driver initialization on Macintosh q800 systems.
> > 
> > SONIC ethernet @50f0a000, MAC 08:00:07:12:34:56, IRQ 3
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 macsonic_init+0x6a/0x15a
> > Modules linked in:
> > CPU: 0 PID: 1 Comm: swapper Not tainted 4.17.0-rc6-mac-00286-g527f47c #1
> > Stack from 0781fdd8:
> >         0781fdd8 003615b3 000181ba 000005c4 07a24cbc 00000000 00000000 000020e8
> > 	07a24800 002c196c 0001824e 00334c06 00000204 001f782a 00000009 00000000
> > 	00000000 003358d9 001f782a 00334c06 00000204 00000003 00000000 07a24800
> > 	002b5cb6 000372ec 001f8b1a 07a24800 00359203 50f0a000 07a14a48 00000003
> > 	00000000 07845c0a 0039dcca 003c835c 003c835c 0035b924 001c19de 07845c00
> > 	07845c0a 0039dcca 001c06dc 07845c0a 0781fed8 00000007 0054d040 07845c0a
> > Call Trace: [<000181ba>] __warn+0xc0/0xc2
> >  [<000020e8>] do_one_initcall+0x0/0x140
> >  [<0001824e>] warn_slowpath_null+0x26/0x2c
> >  [<001f782a>] macsonic_init+0x6a/0x15a
> >  [<001f782a>] macsonic_init+0x6a/0x15a
> >  [<002b5cb6>] memcmp+0x0/0x2a
> >  [<000372ec>] printk+0x0/0x18
> >  [<001f8b1a>] mac_sonic_platform_probe+0x380/0x404
> > 
> > As per the warning the coherent_dma_mask is not set on this device.
> > There is nothing special about the DMA memory coherency on this hardware
> > so we can just set the mask to 32bits in the platform data for the FEC
> > ethernet devices.
> > 
> > Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Acked-by: Finn Thain <fthain@telegraphics.com.au>
> 
> Geert, assuming that Guenter's patch is acceptable, would you also accept 
> a similar patch for the "macmace" platform device?
> 
> > ---
> > Modeled after f61e64310b75 ("m68k: set dma and coherent masks for platform
> > FEC ethernets"). 
> > 
> > RFC: Is "nothing special about the DMA memory coherency" correect ?
> > 
> 
> As I understand it, "cache-coherence" is meaningless unless you have 
> multiple CPU cores and caches. If there's only one CPU core, its cache 

Good point. Maybe it would be better to limit the warning to SMP systems
instead of (unnecessarily) fixing drivers all over the place ?

Guenter

---
>From 9eea0e1b609b69094682757285fd7f89d3930a8e Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux@roeck-us.net>
Date: Sat, 26 May 2018 21:08:39 -0700
Subject: [PATCH] dma-mapping: Warn about coherent_dma_mask only for SMP

As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
coherent_dma_mask") is unconditional, even if the affected system
is a single-CPU system where coherence is irrelevant. Limit the
warning to SMP configurations to reduce the noise.

Fixes: 205e1b7f51e4 ("dma-mapping: warn when there is no coherent_dma_mask")
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
 include/linux/dma-mapping.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index f8ab1c0f589e..ffbb39d84797 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -513,7 +513,7 @@ static inline void *dma_alloc_attrs(struct device *dev, size_t size,
 	void *cpu_addr;
 
 	BUG_ON(!ops);
-	WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_SMP) && dev && !dev->coherent_dma_mask);
 
 	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
 		return cpu_addr;
-- 
2.7.4

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Guenter Roeck]

Hi,

On Sun, May 27, 2018 at 01:01:11PM +1000, Finn Thain wrote:
> On Sat, 26 May 2018, Guenter Roeck wrote:
> 
> > As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
> > coherent_dma_mask") the NatSemi SONIC Ethernet driver is issuing the
> > following warning on driver initialization on Macintosh q800 systems.
> > 
> > SONIC ethernet @50f0a000, MAC 08:00:07:12:34:56, IRQ 3
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 macsonic_init+0x6a/0x15a
> > Modules linked in:
> > CPU: 0 PID: 1 Comm: swapper Not tainted 4.17.0-rc6-mac-00286-g527f47c #1
> > Stack from 0781fdd8:
> >         0781fdd8 003615b3 000181ba 000005c4 07a24cbc 00000000 00000000 000020e8
> > 	07a24800 002c196c 0001824e 00334c06 00000204 001f782a 00000009 00000000
> > 	00000000 003358d9 001f782a 00334c06 00000204 00000003 00000000 07a24800
> > 	002b5cb6 000372ec 001f8b1a 07a24800 00359203 50f0a000 07a14a48 00000003
> > 	00000000 07845c0a 0039dcca 003c835c 003c835c 0035b924 001c19de 07845c00
> > 	07845c0a 0039dcca 001c06dc 07845c0a 0781fed8 00000007 0054d040 07845c0a
> > Call Trace: [<000181ba>] __warn+0xc0/0xc2
> >  [<000020e8>] do_one_initcall+0x0/0x140
> >  [<0001824e>] warn_slowpath_null+0x26/0x2c
> >  [<001f782a>] macsonic_init+0x6a/0x15a
> >  [<001f782a>] macsonic_init+0x6a/0x15a
> >  [<002b5cb6>] memcmp+0x0/0x2a
> >  [<000372ec>] printk+0x0/0x18
> >  [<001f8b1a>] mac_sonic_platform_probe+0x380/0x404
> > 
> > As per the warning the coherent_dma_mask is not set on this device.
> > There is nothing special about the DMA memory coherency on this hardware
> > so we can just set the mask to 32bits in the platform data for the FEC
> > ethernet devices.
> > 
> > Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
> 
> Acked-by: Finn Thain <fthain-PnMVE5gNl/W9dxLkCovUHLpzq4S04n8Q@public.gmane.org>
> 
> Geert, assuming that Guenter's patch is acceptable, would you also accept 
> a similar patch for the "macmace" platform device?
> 
> > ---
> > Modeled after f61e64310b75 ("m68k: set dma and coherent masks for platform
> > FEC ethernets"). 
> > 
> > RFC: Is "nothing special about the DMA memory coherency" correect ?
> > 
> 
> As I understand it, "cache-coherence" is meaningless unless you have 
> multiple CPU cores and caches. If there's only one CPU core, its cache 

Good point. Maybe it would be better to limit the warning to SMP systems
instead of (unnecessarily) fixing drivers all over the place ?

Guenter

---
>From 9eea0e1b609b69094682757285fd7f89d3930a8e Mon Sep 17 00:00:00 2001
From: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
Date: Sat, 26 May 2018 21:08:39 -0700
Subject: [PATCH] dma-mapping: Warn about coherent_dma_mask only for SMP

As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
coherent_dma_mask") is unconditional, even if the affected system
is a single-CPU system where coherence is irrelevant. Limit the
warning to SMP configurations to reduce the noise.

Fixes: 205e1b7f51e4 ("dma-mapping: warn when there is no coherent_dma_mask")
Signed-off-by: Guenter Roeck <linux-0h96xk9xTtrk1uMJSBkQmQ@public.gmane.org>
---
 include/linux/dma-mapping.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index f8ab1c0f589e..ffbb39d84797 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -513,7 +513,7 @@ static inline void *dma_alloc_attrs(struct device *dev, size_t size,
 	void *cpu_addr;
 
 	BUG_ON(!ops);
-	WARN_ON_ONCE(dev && !dev->coherent_dma_mask);
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_SMP) && dev && !dev->coherent_dma_mask);
 
 	if (dma_alloc_from_dev_coherent(dev, size, dma_handle, &cpu_addr))
 		return cpu_addr;
-- 
2.7.4

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Michael Schmitz]

Hi Finn,

was your patch to implement this in arch_setup_pdev_archdata() rejected?
That should have fixed the warning already ...

Am 27.05.2018 um 15:01 schrieb Finn Thain:
> On Sat, 26 May 2018, Guenter Roeck wrote:
> 
>> As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
>> coherent_dma_mask") the NatSemi SONIC Ethernet driver is issuing the
>> following warning on driver initialization on Macintosh q800 systems.
>>
>> SONIC ethernet @50f0a000, MAC 08:00:07:12:34:56, IRQ 3
>> ------------[ cut here ]------------
>> WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 macsonic_init+0x6a/0x15a
>> Modules linked in:
>> CPU: 0 PID: 1 Comm: swapper Not tainted 4.17.0-rc6-mac-00286-g527f47c #1
>> Stack from 0781fdd8:
>>         0781fdd8 003615b3 000181ba 000005c4 07a24cbc 00000000 00000000 000020e8
>> 	07a24800 002c196c 0001824e 00334c06 00000204 001f782a 00000009 00000000
>> 	00000000 003358d9 001f782a 00334c06 00000204 00000003 00000000 07a24800
>> 	002b5cb6 000372ec 001f8b1a 07a24800 00359203 50f0a000 07a14a48 00000003
>> 	00000000 07845c0a 0039dcca 003c835c 003c835c 0035b924 001c19de 07845c00
>> 	07845c0a 0039dcca 001c06dc 07845c0a 0781fed8 00000007 0054d040 07845c0a
>> Call Trace: [<000181ba>] __warn+0xc0/0xc2
>>  [<000020e8>] do_one_initcall+0x0/0x140
>>  [<0001824e>] warn_slowpath_null+0x26/0x2c
>>  [<001f782a>] macsonic_init+0x6a/0x15a
>>  [<001f782a>] macsonic_init+0x6a/0x15a
>>  [<002b5cb6>] memcmp+0x0/0x2a
>>  [<000372ec>] printk+0x0/0x18
>>  [<001f8b1a>] mac_sonic_platform_probe+0x380/0x404
>>
>> As per the warning the coherent_dma_mask is not set on this device.
>> There is nothing special about the DMA memory coherency on this hardware
>> so we can just set the mask to 32bits in the platform data for the FEC
>> ethernet devices.
>>
>> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> 
> Acked-by: Finn Thain <fthain@telegraphics.com.au>
> 
> Geert, assuming that Guenter's patch is acceptable, would you also accept 
> a similar patch for the "macmace" platform device?
> 
>> ---
>> Modeled after f61e64310b75 ("m68k: set dma and coherent masks for platform
>> FEC ethernets"). 
>>
>> RFC: Is "nothing special about the DMA memory coherency" correect ?
>>
> 
> As I understand it, "cache-coherence" is meaningless unless you have 
> multiple CPU cores and caches. If there's only one CPU core, its cache 
> can't be said to be "coherent" or "non-coherent". Moreover, DMA (direct 
> memory access) concerns an IO device and a memory, not a cache, so the 
> term "coherent dma" is bogus IMHO.

DMA does concern the CPU cache insofar as (without explicit cache
maintenance) the CPU cache may hold stale data after a DMA write, or
data to be used in a DMA read may not have been written back from cache
to memory. As far as I understand it, the generic DMA API does handle
these cache maintenance aspects without a need for action at the driver
level. But you're correct that the question of coherent memory does not
arise on uniprocessor systems, so the whole warning could have been
avoided.

Unless 'coherent memory' in this context means no explicit cache
maintenance is required (CPU does bus snooping - which of our platforms
fully support that)?

Cheers,

	Michael

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Finn Thain]

On Sun, 27 May 2018, Michael Schmitz wrote:

> That should have fixed the warning already ...

It's still not fixed (hence my "acked-by" for Geunter's patch).

-- 

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Michael Schmitz]

Hi Finn,

Am 27.05.2018 um 17:49 schrieb Finn Thain:
> On Sun, 27 May 2018, Michael Schmitz wrote:
> 
>> That should have fixed the warning already ...
> 
> It's still not fixed (hence my "acked-by" for Geunter's patch).
> 

Odd - does link order still matter even though the
arch_setup_dev_archdata() function from the core platform code is
declared as a weak symbol?

I'll see what I can find out on elgar ...

Cheers,

	Michael

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Finn Thain]

On Mon, 28 May 2018, Michael Schmitz wrote:

> Hi Finn,
> 
> Am 27.05.2018 um 17:49 schrieb Finn Thain:
> > On Sun, 27 May 2018, Michael Schmitz wrote:
> > 
> >> That should have fixed the warning already ...
> > 
> > It's still not fixed (hence my "acked-by" for Geunter's patch).
> > 
> 
> Odd - does link order still matter even though the 
> arch_setup_dev_archdata() function from the core platform code is 
> declared as a weak symbol?
> 
> I'll see what I can find out on elgar ...
> 

Any one of the numerous patches/rfcs/suggestions that I sent will avoid 
the WARN splat.

When I said "it's still not fixed", what I meant to say was, "it's still 
not fixed in mainline and no proposed fix was accepted to the best of my 
knowledge".

-- 

> Cheers,
> 
> 	Michael
> 

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Christoph Hellwig]

On Sat, May 26, 2018 at 09:15:05PM -0700, Guenter Roeck wrote:
> Good point. Maybe it would be better to limit the warning to SMP systems
> instead of (unnecessarily) fixing drivers all over the place ?

No.  The coherent_dma_mask is the mask used for dma_alloc_coherent and
dma_alloc_attrs.  It has nothing to do with cache coherent in SMP
systems.

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Christoph Hellwig]

On Sat, May 26, 2018 at 09:15:05PM -0700, Guenter Roeck wrote:
> Good point. Maybe it would be better to limit the warning to SMP systems
> instead of (unnecessarily) fixing drivers all over the place ?

No.  The coherent_dma_mask is the mask used for dma_alloc_coherent and
dma_alloc_attrs.  It has nothing to do with cache coherent in SMP
systems.

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Geert Uytterhoeven]

On Mon, May 28, 2018 at 7:26 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
> On Mon, 28 May 2018, Michael Schmitz wrote:
>> Am 27.05.2018 um 17:49 schrieb Finn Thain:
>> > On Sun, 27 May 2018, Michael Schmitz wrote:
>> >
>> >> That should have fixed the warning already ...
>> >
>> > It's still not fixed (hence my "acked-by" for Geunter's patch).
>> >
>>
>> Odd - does link order still matter even though the
>> arch_setup_dev_archdata() function from the core platform code is
>> declared as a weak symbol?
>>
>> I'll see what I can find out on elgar ...
>>
>
> Any one of the numerous patches/rfcs/suggestions that I sent will avoid
> the WARN splat.
>
> When I said "it's still not fixed", what I meant to say was, "it's still
> not fixed in mainline and no proposed fix was accepted to the best of my
> knowledge".

Indeed.

Do we have a consensus on the way forward? The merge window for
v4.18 will open soon.

Thanks!

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: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Michael Schmitz]

Hi Geert,

my preference would be Finn's patch introducing a m68k
arch_setup_pdev_archdata(). It nicely preserves what bus code sets up
prior to registering a platform device (important for Zorro devices
using platform or mfd devices), and allows overriding by drivers that
need it.

If ever a kernel-wide consensus is reached to move this setup to the
generic arch_setup_pdev_archdata(), we can still back out his patch at
that time. Finn found one counterexample where absence of DMA mask
signaled 'do not use DMA', so I think moving the DMA mask setup to
generic code is unlikely to happen any time.

Just my $0.02 ...

Cheers,

  MIchael

On Mon, May 28, 2018 at 10:15 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Mon, May 28, 2018 at 7:26 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
>> On Mon, 28 May 2018, Michael Schmitz wrote:
>>> Am 27.05.2018 um 17:49 schrieb Finn Thain:
>>> > On Sun, 27 May 2018, Michael Schmitz wrote:
>>> >
>>> >> That should have fixed the warning already ...
>>> >
>>> > It's still not fixed (hence my "acked-by" for Geunter's patch).
>>> >
>>>
>>> Odd - does link order still matter even though the
>>> arch_setup_dev_archdata() function from the core platform code is
>>> declared as a weak symbol?
>>>
>>> I'll see what I can find out on elgar ...
>>>
>>
>> Any one of the numerous patches/rfcs/suggestions that I sent will avoid
>> the WARN splat.
>>
>> When I said "it's still not fixed", what I meant to say was, "it's still
>> not fixed in mainline and no proposed fix was accepted to the best of my
>> knowledge".
>
> Indeed.
>
> Do we have a consensus on the way forward? The merge window for
> v4.18 will open soon.
>
> Thanks!
>
> 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: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Finn Thain]

On Mon, 28 May 2018, Geert Uytterhoeven wrote:

> 
> Do we have a consensus on the way forward?

My prefered solution remains the two driver patches that I originally 
submitted, which you objected to:

https://lkml.org/lkml/2018/5/3/10
https://lkml.org/lkml/2018/5/3/9

So there is no consensus yet.

To my mind, the existing code is pretty clear: drivers should set the 
(default) mask. See also,

$ egrep -r -A1 "coherent_dma_mask.*expected" */
drivers/of/device.c:     * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
drivers/of/device.c-     * setup the correct supported mask.
--
drivers/acpi/arm64/iort.c:       * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
drivers/acpi/arm64/iort.c-       * setup the correct supported mask.
$ 

And drivers/usb/dwc2/platform.c:

        /*
         * Use reasonable defaults so platforms don't have to provide these.
         */
        if (!dev->dev.dma_mask)
                dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
        retval = dma_set_coherent_mask(&dev->dev, DMA_BIT_MASK(32));

And FWIW,

$ egrep -wlr "dma_set_mask_and_coherent|dma_set_coherent_mask|dma_coerce_mask_and_coherent" drivers/ | wc -l
196

I suppose that a driver should avoid lengthening an existing device mask.

Since an arch gets to apply limits in the dma ops it implements, why would 
arch code also have to set a limit in the form of default platform device 
masks? Powerpc seems to be the only arch that does this.

The same line of reasoning suggests that the problematic WARN_ON should 
not appear in include/linux/ in the first place. If it is needed by 
certain architectures, it should be in arch/x.

I would send a patch to revert commit 205e1b7f51e4 ("dma-mapping: warn 
when there is no coherent_dma_mask") if I thought that arch code was not 
somehow relying on it. But I'll leave that up to Chrisoph.

-- 

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Michael Schmitz]

Hi Finn,

Am 29.05.2018 um 14:15 schrieb Finn Thain:
> 
> Since an arch gets to apply limits in the dma ops it implements, why would 
> arch code also have to set a limit in the form of default platform device 
> masks? Powerpc seems to be the only arch that does this.

One of Christoph's recent patches removed most of arches' dma ops,
replacing them by one generic implementation instead. m68k was one of
the affected arches. I concede his patch series is experimental still
and not in mainline, but may be included at some time.

Otherwise - your patches, so if you prefer to have each driver handle
this on an as-needed basis, I'm not objecting.

Cheers,

	Michael

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Finn Thain]

On Tue, 29 May 2018, Michael Schmitz wrote:

> > 
> > Since an arch gets to apply limits in the dma ops it implements, why 
> > would arch code also have to set a limit in the form of default 
> > platform device masks? Powerpc seems to be the only arch that does 
> > this.
> 
> One of Christoph's recent patches removed most of arches' dma ops, 
> replacing them by one generic implementation instead. m68k was one of 
> the affected arches. I concede his patch series is experimental still 
> and not in mainline, but may be included at some time.

I found some patches here,
http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/generic-dma-noncoherent.2

Looks like m68k_dma_alloc() gets renamed arch_dma_alloc() and the generic 
ops don't use the dma masks.

Maybe I'm looking at the wrong patches?

-- 

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Christoph Hellwig]

On Tue, May 29, 2018 at 07:59:37AM +1200, Michael Schmitz wrote:
> Hi Geert,
> 
> my preference would be Finn's patch introducing a m68k
> arch_setup_pdev_archdata(). It nicely preserves what bus code sets up
> prior to registering a platform device (important for Zorro devices
> using platform or mfd devices), and allows overriding by drivers that
> need it.

Let's go with those for now.

> 
> If ever a kernel-wide consensus is reached to move this setup to the
> generic arch_setup_pdev_archdata(), we can still back out his patch at
> that time. Finn found one counterexample where absence of DMA mask
> signaled 'do not use DMA', so I think moving the DMA mask setup to
> generic code is unlikely to happen any time.

This is the right thing to do in the long run, and I'll spend some time
auditing all dma_mask users for the next merge window.

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Christoph Hellwig]

On Tue, May 29, 2018 at 03:38:23PM +1000, Finn Thain wrote:
> On Tue, 29 May 2018, Michael Schmitz wrote:
> 
> > > 
> > > Since an arch gets to apply limits in the dma ops it implements, why 
> > > would arch code also have to set a limit in the form of default 
> > > platform device masks? Powerpc seems to be the only arch that does 
> > > this.
> > 
> > One of Christoph's recent patches removed most of arches' dma ops, 
> > replacing them by one generic implementation instead. m68k was one of 
> > the affected arches. I concede his patch series is experimental still 
> > and not in mainline, but may be included at some time.
> 
> I found some patches here,
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/generic-dma-noncoherent.2
> 
> Looks like m68k_dma_alloc() gets renamed arch_dma_alloc() and the generic 
> ops don't use the dma masks.

This is for the 'coherent' allocations, and literatelly nothing changes
for those yet.

But for the mapping path this switches m68k to use the dma-direct
path, wrapped by dma-noncoherent.  And these do check dev->dma_mask,
although only really for sanity checking.

Btw, can I get a review and testing for the above?  They aren't really
experimental any more, and I'd like to move architectures over as soon
as possible.

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Finn Thain]

On Tue, 29 May 2018, Christoph Hellwig wrote:

> Btw, can I get a review and testing for the above? They aren't really 
> experimental any more, and I'd like to move architectures over as soon 
> as possible.

When I boot the generic-dma-noncoherent.2 branch (5f4613b2dcd4) in qemu 
and run "ifconfig eth0 up" I get an oops (see below).

The crash goes away when I add a patch to initialize the macsonic device 
dma masks, but it appears the new code is not as robust as the old code.

Unable to handle kernel NULL pointer dereference at virtual address   (null)
Oops: 00000000
Modules linked in:
PC: [<003352e6>] dma_direct_map_page+0xc0/0xf8
SR: 2004  SP: f93954f5  a2: 1e988f30
d0: 1e9c7950    d1: 1e9c7000    d2: fff0b1c8    d3: f4e569c7
d4: 000005f0    d5: 00000002    a0: 1e85a20a    a1: 00000000
Process ifconfig (pid: 46, task=87dc4b31)
Frame format=7 eff addr=00000000 ssw=0505 faddr=00000000
wb 1 stat/addr/data: 0000 00000000 00000000
wb 2 stat/addr/data: 0000 00000000 00000000
wb 3 stat/addr/data: 0000 00000000 00000000
push data: 00000000 00000000 00000000 00000000
Stack from 1e9b9c74:
        00000000 00000000 000005f0 00000002 0001e9c7 0000003d 1e85a20a 00951ffc
        1e9c7950 1e9b9cd8 00335636 1e85a20a 00951ffc 00000950 000005f0 00000002
        00000000 00000000 00000017 003d38e0 0027b11e 0001e9c7 1e9783e0 1e9784d0
        00381f10 1e9b9d2c 0025c11a 1e85a20a 00951ffc 00000950 000005f0 00000002
        00000000 00001043 00001002 00000041 8000b543 00000000 1e95f00c 1e978000
        0037c3a0 1e97802f 00000950 1e85a20a 0007a71c 1e9b9d44 1e9b9d44 0028d4b0
Call Trace: [<0001e9c7>] resource_list_create_entry+0x11/0x46
 [<00335636>] dma_noncoherent_map_page+0x32/0xfc
 [<0027b11e>] skb_put+0x0/0x6a
 [<0001e9c7>] resource_list_create_entry+0x11/0x46
 [<0025c11a>] macsonic_open+0x124/0x360
 [<00001043>] kernel_pg_dir+0x43/0x1000
 [<00001002>] kernel_pg_dir+0x2/0x1000
 [<0007a71c>] wb_update_bandwidth+0x40/0x4c
 [<0028d4b0>] __dev_open+0x8e/0x108
 [<0028d026>] dev_set_rx_mode+0x0/0x3e
 [<0028d676>] __dev_change_flags+0x14c/0x19e
 [<00001002>] kernel_pg_dir+0x2/0x1000
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<00021378>] ns_capable_common+0x30/0x96
 [<0028d6e8>] dev_change_flags+0x20/0x56
 [<00001043>] kernel_pg_dir+0x43/0x1000
 [<00001043>] kernel_pg_dir+0x43/0x1000
 [<002fad14>] devinet_ioctl+0x610/0x754
 [<00001043>] kernel_pg_dir+0x43/0x1000
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<002fc3c6>] inet_ioctl+0x1a2/0x250
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<00291c50>] dev_get_by_name_rcu+0x68/0xa0
 [<00291c50>] dev_get_by_name_rcu+0x68/0xa0
 [<00008913>] via_nubus_irq+0x9d/0xd0
 [<002b17ce>] dev_ioctl+0x334/0x4ba
 [<002b1826>] dev_ioctl+0x38c/0x4ba
 [<00273b44>] sock_ioctl+0x132/0x3f4
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<00001002>] kernel_pg_dir+0x2/0x1000
 [<00028000>] getrusage+0x25a/0x3ee
 [<000c1594>] vfs_ioctl+0x20/0x36
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<000c197e>] do_vfs_ioctl+0x70/0x6ae
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<000bf7e0>] user_path_at_empty+0x2c/0x34
 [<0003471e>] __put_cred+0x2a/0x78
 [<00034b2a>] put_cred_rcu+0x0/0x92
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<000c1fec>] ksys_ioctl+0x30/0x6c
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<000c203e>] sys_ioctl+0x16/0x1a
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<00002b7c>] syscall+0x8/0xc
 [<00008914>] via_nubus_irq+0x9e/0xd0
 [<0000c00f>] cu_norm+0x9/0x28
Code: 9781 6514 4280 4cee 18fc ffdc 4e5e 4e75 <2211> 2429 0004 60e2 2f02 2f01 2f2e 0014 486e fffc 4879 0038 1efc 4879 003e a45d
Disabling lock debugging due to kernel taint

-- 

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Finn Thain]

On Mon, 28 May 2018, Geert Uytterhoeven wrote:

> 
> Do we have a consensus on the way forward? The merge window for v4.18 
> will open soon.

I'm content to let you and Christoph decide how best to deal with this. 
Don't wait for me to find consensus ;-)

-- 

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Michael Schmitz]

Hi Finn,

On Tue, May 29, 2018 at 5:38 PM, Finn Thain <fthain@telegraphics.com.au> wrote:
> I found some patches here,
> http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/generic-dma-noncoherent.2

That's the most recent IIRC. Haven't begun looking at that yet - still stuck at

git://git.infradead.org/users/hch/misc.git

and waiting for a SCSI disk to be installed on elgar.

Christoph - any merit in testing that old repo?

Cheers,

  Michael

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Greg Ungerer]

Hi Geert,

On 28/05/18 20:15, Geert Uytterhoeven wrote:
> On Mon, May 28, 2018 at 7:26 AM, Finn Thain <fthain@telegraphics.com.au> wrote:
>> On Mon, 28 May 2018, Michael Schmitz wrote:
>>> Am 27.05.2018 um 17:49 schrieb Finn Thain:
>>>> On Sun, 27 May 2018, Michael Schmitz wrote:
>>>>
>>>>> That should have fixed the warning already ...
>>>>
>>>> It's still not fixed (hence my "acked-by" for Geunter's patch).
>>>>
>>>
>>> Odd - does link order still matter even though the
>>> arch_setup_dev_archdata() function from the core platform code is
>>> declared as a weak symbol?
>>>
>>> I'll see what I can find out on elgar ...
>>>
>>
>> Any one of the numerous patches/rfcs/suggestions that I sent will avoid
>> the WARN splat.
>>
>> When I said "it's still not fixed", what I meant to say was, "it's still
>> not fixed in mainline and no proposed fix was accepted to the best of my
>> knowledge".
> 
> Indeed.
> 
> Do we have a consensus on the way forward? The merge window for
> v4.18 will open soon.

For whatever it is worth I thought Finn's patch was the best approach
(https://lkml.org/lkml/2018/5/17/333, "m68k: Set default dma mask for
platform device").

We seem to be hitting quite a few places (within m68k) that otherwise
need individual fixes. There is no immediate need to revert existing
changes that have already been applied if we use this now either
(like my FEC fix, commit f61e64310b75 "m68k: set dma and coherent
masks for platform FEC ethernets").

Regards
Greg

Re: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Geert Uytterhoeven]

Hi Greg,

On Wed, May 30, 2018 at 2:28 AM, Greg Ungerer <gerg@linux-m68k.org> wrote:
> On 28/05/18 20:15, Geert Uytterhoeven wrote:
>> On Mon, May 28, 2018 at 7:26 AM, Finn Thain <fthain@telegraphics.com.au>
>> wrote:
>>> On Mon, 28 May 2018, Michael Schmitz wrote:
>>>> Am 27.05.2018 um 17:49 schrieb Finn Thain:
>>>>> On Sun, 27 May 2018, Michael Schmitz wrote:
>>>>>> That should have fixed the warning already ...
>>>>>
>>>>> It's still not fixed (hence my "acked-by" for Geunter's patch).
>>>>
>>>> Odd - does link order still matter even though the
>>>> arch_setup_dev_archdata() function from the core platform code is
>>>> declared as a weak symbol?
>>>>
>>>> I'll see what I can find out on elgar ...
>>>
>>> Any one of the numerous patches/rfcs/suggestions that I sent will avoid
>>> the WARN splat.
>>>
>>> When I said "it's still not fixed", what I meant to say was, "it's still
>>> not fixed in mainline and no proposed fix was accepted to the best of my
>>> knowledge".
>>
>> Indeed.
>>
>> Do we have a consensus on the way forward? The merge window for
>> v4.18 will open soon.
>
> For whatever it is worth I thought Finn's patch was the best approach
> (https://lkml.org/lkml/2018/5/17/333, "m68k: Set default dma mask for
> platform device").

FTR: done.

> We seem to be hitting quite a few places (within m68k) that otherwise
> need individual fixes. There is no immediate need to revert existing
> changes that have already been applied if we use this now either
> (like my FEC fix, commit f61e64310b75 "m68k: set dma and coherent
> masks for platform FEC ethernets").

Indeed.

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: [RFC PATCH] m68k: set dma and coherent masks for Macintosh SONIC based ethernet

[Greg Ungerer]

Hi Geert,

On 31/05/18 05:55, Geert Uytterhoeven wrote:
> On Wed, May 30, 2018 at 2:28 AM, Greg Ungerer <gerg@linux-m68k.org> wrote:
>> On 28/05/18 20:15, Geert Uytterhoeven wrote:
>>> On Mon, May 28, 2018 at 7:26 AM, Finn Thain <fthain@telegraphics.com.au>
>>> wrote:
>>>> On Mon, 28 May 2018, Michael Schmitz wrote:
>>>>> Am 27.05.2018 um 17:49 schrieb Finn Thain:
>>>>>> On Sun, 27 May 2018, Michael Schmitz wrote:
>>>>>>> That should have fixed the warning already ...
>>>>>>
>>>>>> It's still not fixed (hence my "acked-by" for Geunter's patch).
>>>>>
>>>>> Odd - does link order still matter even though the
>>>>> arch_setup_dev_archdata() function from the core platform code is
>>>>> declared as a weak symbol?
>>>>>
>>>>> I'll see what I can find out on elgar ...
>>>>
>>>> Any one of the numerous patches/rfcs/suggestions that I sent will avoid
>>>> the WARN splat.
>>>>
>>>> When I said "it's still not fixed", what I meant to say was, "it's still
>>>> not fixed in mainline and no proposed fix was accepted to the best of my
>>>> knowledge".
>>>
>>> Indeed.
>>>
>>> Do we have a consensus on the way forward? The merge window for
>>> v4.18 will open soon.
>>
>> For whatever it is worth I thought Finn's patch was the best approach
>> (https://lkml.org/lkml/2018/5/17/333, "m68k: Set default dma mask for
>> platform device").
> 
> FTR: done.

Feel free to add my acked by if you like:

Acked-by: Greg Ungerer <gerg@linux-m68k.org>

Regards
Greg


>> We seem to be hitting quite a few places (within m68k) that otherwise
>> need individual fixes. There is no immediate need to revert existing
>> changes that have already been applied if we use this now either
>> (like my FEC fix, commit f61e64310b75 "m68k: set dma and coherent
>> masks for platform FEC ethernets").
> 
> Indeed.
> 
> Gr{oetje,eeting}s,
> 
>                          Geert
>