USB/swiotlb failure on arm64/RPi3

USB/swiotlb failure on arm64/RPi3

[Aaro Koskinen]

Hi,

Booting Debian rootfs from USB hard disk (ext4) using 64-bit 4.9 kernel
on Raspberry Pi 3 fails early in the boot as follows:

[   15.162466] systemd[1]: Detected architecture arm64.

Welcome to Debian GNU/Linux 9 (stretch)!

[   15.200822] systemd[1]: Set hostname to <raspberrypi-3>.
[   46.135227] usb 1-1.5: reset high-speed USB device number 4 using dwc2
[   76.844211] usb 1-1.5: reset high-speed USB device number 4 using dwc2
[  105.257888] systemd[1]: system-generators terminated by signal ALRM.
[  108.087234] usb 1-1.5: reset high-speed USB device number 4 using dwc2
[  138.796224] usb 1-1.5: reset high-speed USB device number 4 using dwc2
[  170.039222] usb 1-1.5: reset high-speed USB device number 4 using dwc2
[  201.261222] usb 1-1.5: reset high-speed USB device number 4 using dwc2
[  201.405906] sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x05 driverbyte=0x00
[  201.414234] sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 02 c7 c5 90 00 00 68 00
[  201.421951] blk_update_request: I/O error, dev sda, sector 46646672

Boot hangs and I/O does not recover.

I first suspected the dwc2 driver, but the cause turns out to be DMA
mapping error in usb_hcd_map_urb_for_dma(). This will cause usb_sg_wait()
to loop forever trying to re-try. On RPi3 dma_mapping_error() is:

int
swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
{
	return (dma_addr == phys_to_dma(hwdev, io_tlb_overflow_buffer));
}

On arm64, swiotlb is not initialized by default, so io_tlb_overflow_buffer
is 0. But phys_to_dma(hwdev, 0) should be a valid DMA address and not
be rejected. I tested this by initializing io_tlb_overflow_buffer with
INVALID_PHYS_ADDR, and then the boot passes and system runs fine.

Any ideas how this should be fixed properly?

A.

USB/swiotlb failure on arm64/RPi3

[Stefan Wahren]

[Add John and linux-usb]

Am 24.01.2017 um 23:52 schrieb Aaro Koskinen:
> Hi,
>
> Booting Debian rootfs from USB hard disk (ext4) using 64-bit 4.9 kernel
> on Raspberry Pi 3 fails early in the boot as follows:
>
> [   15.162466] systemd[1]: Detected architecture arm64.
>
> Welcome to Debian GNU/Linux 9 (stretch)!
>
> [   15.200822] systemd[1]: Set hostname to <raspberrypi-3>.
> [   46.135227] usb 1-1.5: reset high-speed USB device number 4 using dwc2
> [   76.844211] usb 1-1.5: reset high-speed USB device number 4 using dwc2
> [  105.257888] systemd[1]: system-generators terminated by signal ALRM.
> [  108.087234] usb 1-1.5: reset high-speed USB device number 4 using dwc2
> [  138.796224] usb 1-1.5: reset high-speed USB device number 4 using dwc2
> [  170.039222] usb 1-1.5: reset high-speed USB device number 4 using dwc2
> [  201.261222] usb 1-1.5: reset high-speed USB device number 4 using dwc2
> [  201.405906] sd 0:0:0:0: [sda] tag#0 UNKNOWN(0x2003) Result: hostbyte=0x05 driverbyte=0x00
> [  201.414234] sd 0:0:0:0: [sda] tag#0 CDB: opcode=0x28 28 00 02 c7 c5 90 00 00 68 00
> [  201.421951] blk_update_request: I/O error, dev sda, sector 46646672
>
> Boot hangs and I/O does not recover.
>
> I first suspected the dwc2 driver, but the cause turns out to be DMA
> mapping error in usb_hcd_map_urb_for_dma(). This will cause usb_sg_wait()
> to loop forever trying to re-try. On RPi3 dma_mapping_error() is:
>
> int
> swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
> {
> 	return (dma_addr == phys_to_dma(hwdev, io_tlb_overflow_buffer));
> }
>
> On arm64, swiotlb is not initialized by default, so io_tlb_overflow_buffer
> is 0. But phys_to_dma(hwdev, 0) should be a valid DMA address and not
> be rejected. I tested this by initializing io_tlb_overflow_buffer with
> INVALID_PHYS_ADDR, and then the boot passes and system runs fine.
>
> Any ideas how this should be fixed properly?
>
> A.
>
> _______________________________________________
> linux-rpi-kernel mailing list
> linux-rpi-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rpi-kernel

USB/swiotlb failure on arm64/RPi3

[Arnd Bergmann]

On Wednesday, January 25, 2017 12:52:00 AM CET Aaro Koskinen wrote:
> 
> Boot hangs and I/O does not recover.
> 
> I first suspected the dwc2 driver, but the cause turns out to be DMA
> mapping error in usb_hcd_map_urb_for_dma(). This will cause usb_sg_wait()
> to loop forever trying to re-try. On RPi3 dma_mapping_error() is:

Do you know why usb_hcd_map_urb_for_dma() fails here? Are we running
out of swiotlb bounce buffer space, or is there some other problem?

> int
> swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
> {
>         return (dma_addr == phys_to_dma(hwdev, io_tlb_overflow_buffer));
> }
> 
> On arm64, swiotlb is not initialized by default, so io_tlb_overflow_buffer
> is 0. But phys_to_dma(hwdev, 0) should be a valid DMA address and not
> be rejected. I tested this by initializing io_tlb_overflow_buffer with
> INVALID_PHYS_ADDR, and then the boot passes and system runs fine.
> 
> Any ideas how this should be fixed properly?

Why is swiotlb not initialized?

	Arnd

[PATCH] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Robin Murphy]

When bypassing SWIOTLB on small-memory systems, we need to avoid calling
into swiotlb_dma_mapping_error() in exactly the same way as we avoid
swiotlb_dma_supported(), because the former also relies on SWIOTLB state
being initialised.

Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
CC: stable at vger.kernel.org
CC: Jisheng Zhang <jszhang@marvell.com>
Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

Not a SWIOTLB problem :)

 arch/arm64/mm/dma-mapping.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e04082700bb1..0ea94c782382 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 	return 1;
 }
 
+static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
+{
+	if (swiotlb)
+		return swiotlb_dma_mapping_error(hwdev, addr);
+	return 1;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -366,7 +373,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_cpu = __swiotlb_sync_sg_for_cpu,
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = __swiotlb_dma_supported,
-	.mapping_error = swiotlb_dma_mapping_error,
+	.mapping_error = __swiotlb_dma_mapping_error,
 };
 
 static int __init atomic_pool_init(void)
-- 
2.11.0.dirty

[PATCH] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Michael Zoran]

On Wed, 2017-01-25 at 12:03 +0000, Robin Murphy wrote:
> hen bypassing SWIOTLB on small-memory systems, we need to avoid
> calling
> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
> swiotlb_dma_supported(), because the former also relies on SWIOTLB
> state
> being initialised.
> 

I didn't submit the initial ARM64 port of the RPI 3, so I don't know
much about this.  But from a third personal point of view, this seems
to side step the main issue here.

>From an ARM64 subsystem point of view, what exactly is the
correct/recommended method for ensuring the mm subsystem is initialized
correctly?

[PATCH] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Robin Murphy]

On 25/01/17 12:37, Michael Zoran wrote:
> On Wed, 2017-01-25 at 12:03 +0000, Robin Murphy wrote:
>> hen bypassing SWIOTLB on small-memory systems, we need to avoid
>> calling
>> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
>> swiotlb_dma_supported(), because the former also relies on SWIOTLB
>> state
>> being initialised.
>>
> 
> I didn't submit the initial ARM64 port of the RPI 3, so I don't know
> much about this.  But from a third personal point of view, this seems
> to side step the main issue here.

On the contrary - the main issue is that we're calling into an
uninitialised SWIOTLB, and not initialising SWIOTLB on arm64 systems
where all the RAM is below 4GB is the exact purpose of b67a8b29df7e.
This particular call was obviously overlooked in the original patch
because it happened to still work on systems with a different memory map
(or more likely without a DMA offset).

Robin.

> 
> From an ARM64 subsystem point of view, what exactly is the
> correct/recommended method for ensuring the mm subsystem is initialized
> correctly?
> 
> 

[PATCH] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Arnd Bergmann]

On Wed, Jan 25, 2017 at 1:37 PM, Michael Zoran <mzoran@crowfest.net> wrote:
> On Wed, 2017-01-25 at 12:03 +0000, Robin Murphy wrote:
>> hen bypassing SWIOTLB on small-memory systems, we need to avoid
>> calling
>> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
>> swiotlb_dma_supported(), because the former also relies on SWIOTLB
>> state
>> being initialised.
>
> I didn't submit the initial ARM64 port of the RPI 3, so I don't know
> much about this.  But from a third personal point of view, this seems
> to side step the main issue here.

I think Robin's approach is fixing exactly the right part of the code.

> From an ARM64 subsystem point of view, what exactly is the
> correct/recommended method for ensuring the mm subsystem is initialized
> correctly?

It is initialized correctly, the bug was calling the wrong helper when swiotlb
is not used because we determined that we don't need it.

One concern from inspection:

> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
> +{
> +       if (swiotlb)
> +               return swiotlb_dma_mapping_error(hwdev, addr);
> +       return 1;
> +}

Shouldn't that be

     return addr == DMA_ERROR_CODE;

in the last line? Otherwise any addr is interpreted as an error, which
seems wrong. Maybe I'm missing something obvious here.

    Arnd

[PATCH] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Robin Murphy]

On 25/01/17 12:51, Arnd Bergmann wrote:
> On Wed, Jan 25, 2017 at 1:37 PM, Michael Zoran <mzoran@crowfest.net> wrote:
>> On Wed, 2017-01-25 at 12:03 +0000, Robin Murphy wrote:
>>> hen bypassing SWIOTLB on small-memory systems, we need to avoid
>>> calling
>>> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
>>> swiotlb_dma_supported(), because the former also relies on SWIOTLB
>>> state
>>> being initialised.
>>
>> I didn't submit the initial ARM64 port of the RPI 3, so I don't know
>> much about this.  But from a third personal point of view, this seems
>> to side step the main issue here.
> 
> I think Robin's approach is fixing exactly the right part of the code.
> 
>> From an ARM64 subsystem point of view, what exactly is the
>> correct/recommended method for ensuring the mm subsystem is initialized
>> correctly?
> 
> It is initialized correctly, the bug was calling the wrong helper when swiotlb
> is not used because we determined that we don't need it.
> 
> One concern from inspection:
> 
>> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
>> +{
>> +       if (swiotlb)
>> +               return swiotlb_dma_mapping_error(hwdev, addr);
>> +       return 1;
>> +}
> 
> Shouldn't that be
> 
>      return addr == DMA_ERROR_CODE;
> 
> in the last line? Otherwise any addr is interpreted as an error, which
> seems wrong. Maybe I'm missing something obvious here.

Aw crap, copy/paste/brain error - thanks.

I'll have a nice strong cup of tea, actually engage thinking mode, and
respin...

Robin.

> 
>     Arnd
> 

[PATCH] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Michael Zoran]

On Wed, 2017-01-25 at 12:54 +0000, Robin Murphy wrote:
> On 25/01/17 12:51, Arnd Bergmann wrote:
> > On Wed, Jan 25, 2017 at 1:37 PM, Michael Zoran <mzoran@crowfest.net
> > > wrote:
> > > On Wed, 2017-01-25 at 12:03 +0000, Robin Murphy wrote:
> > > > hen bypassing SWIOTLB on small-memory systems, we need to avoid
> > > > calling
> > > > into swiotlb_dma_mapping_error() in exactly the same way as we
> > > > avoid
> > > > swiotlb_dma_supported(), because the former also relies on
> > > > SWIOTLB
> > > > state
> > > > being initialised.
> > > 
> > > I didn't submit the initial ARM64 port of the RPI 3, so I don't
> > > know
> > > much about this.??But from a third personal point of view, this
> > > seems
> > > to side step the main issue here.
> > 
> > I think Robin's approach is fixing exactly the right part of the
> > code.
> > 
> > > From an ARM64 subsystem point of view, what exactly is the
> > > correct/recommended method for ensuring the mm subsystem is
> > > initialized
> > > correctly?
> > 
> > It is initialized correctly, the bug was calling the wrong helper
> > when swiotlb
> > is not used because we determined that we don't need it.
> > 
> > One concern from inspection:
> > 
> > > +static int __swiotlb_dma_mapping_error(struct device *hwdev,
> > > dma_addr_t addr)
> > > +{
> > > +???????if (swiotlb)
> > > +???????????????return swiotlb_dma_mapping_error(hwdev, addr);
> > > +???????return 1;
> > > +}
> > 
> > Shouldn't that be
> > 
> > ?????return addr == DMA_ERROR_CODE;
> > 
> > in the last line? Otherwise any addr is interpreted as an error,
> > which
> > seems wrong. Maybe I'm missing something obvious here.
> 
> Aw crap, copy/paste/brain error - thanks.
> 
> I'll have a nice strong cup of tea, actually engage thinking mode,
> and
> respin...
> 
> Robin.
> 
> > 
> > ????Arnd
> > 

I have an RPI 3 that I run in ARM64 mode all the time for personal use.
 If/when you have a patch ready, I'm more then willing to try it out
locally.

Just tell me the path of the git tree and branch that it's based on.

Thanks.

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Robin Murphy]

When bypassing SWIOTLB on small-memory systems, we need to avoid calling
into swiotlb_dma_mapping_error() in exactly the same way as we avoid
swiotlb_dma_supported(), because the former also relies on SWIOTLB state
being initialised.

Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
will only ever return the DMA-offset-adjusted physical address of the
page passed in, thus we can report success unconditionally.

Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
CC: stable at vger.kernel.org
CC: Jisheng Zhang <jszhang@marvell.com>
Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

v2: Get the return value the right way round this time... After some
    careful reasoning it really is that simple.

 arch/arm64/mm/dma-mapping.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e04082700bb1..1ffb7d5d299a 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 	return 1;
 }
 
+static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
+{
+	if (swiotlb)
+		return swiotlb_dma_mapping_error(hwdev, addr);
+	return 0;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -366,7 +373,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_cpu = __swiotlb_sync_sg_for_cpu,
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = __swiotlb_dma_supported,
-	.mapping_error = swiotlb_dma_mapping_error,
+	.mapping_error = __swiotlb_dma_mapping_error,
 };
 
 static int __init atomic_pool_init(void)
-- 
2.11.0.dirty

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Robin Murphy]

[ +Michael - FYI this is straight on top of 4.10-rc5 ]

On 25/01/17 18:31, Robin Murphy wrote:
> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
> being initialised.
> 
> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
> will only ever return the DMA-offset-adjusted physical address of the
> page passed in, thus we can report success unconditionally.
> 
> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
> CC: stable at vger.kernel.org
> CC: Jisheng Zhang <jszhang@marvell.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v2: Get the return value the right way round this time... After some
>     careful reasoning it really is that simple.
> 
>  arch/arm64/mm/dma-mapping.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e04082700bb1..1ffb7d5d299a 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>  	return 1;
>  }
>  
> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
> +{
> +	if (swiotlb)
> +		return swiotlb_dma_mapping_error(hwdev, addr);
> +	return 0;
> +}
> +
>  static struct dma_map_ops swiotlb_dma_ops = {
>  	.alloc = __dma_alloc,
>  	.free = __dma_free,
> @@ -366,7 +373,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
>  	.sync_sg_for_cpu = __swiotlb_sync_sg_for_cpu,
>  	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
>  	.dma_supported = __swiotlb_dma_supported,
> -	.mapping_error = swiotlb_dma_mapping_error,
> +	.mapping_error = __swiotlb_dma_mapping_error,
>  };
>  
>  static int __init atomic_pool_init(void)
> 

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Michael Zoran]

On Wed, 2017-01-25 at 19:14 +0000, Robin Murphy wrote:
> [ +Michael - FYI this is straight on top of 4.10-rc5 ]
> 
> On 25/01/17 18:31, Robin Murphy wrote:
> > When bypassing SWIOTLB on small-memory systems, we need to avoid
> > calling
> > into swiotlb_dma_mapping_error() in exactly the same way as we
> > avoid
> > swiotlb_dma_supported(), because the former also relies on SWIOTLB
> > state
> > being initialised.
> > 
> > Under the assumptions for which we skip SWIOTLB,
> > dma_map_{single,page}()
> > will only ever return the DMA-offset-adjusted physical address of
> > the
> > page passed in, thus we can report success unconditionally.
> > 
> > Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when
> > necessary")
> > CC: stable at vger.kernel.org
> > CC: Jisheng Zhang <jszhang@marvell.com>
> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> > 
> > v2: Get the return value the right way round this time... After
> > some
> > ????careful reasoning it really is that simple.
> > 
> > ?arch/arm64/mm/dma-mapping.c | 9 ++++++++-
> > ?1 file changed, 8 insertions(+), 1 deletion(-)
> > 

I was able to build it and it works. Cool.

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Aaro Koskinen]

Hi,

On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
> being initialised.
> 
> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
> will only ever return the DMA-offset-adjusted physical address of the
> page passed in, thus we can report success unconditionally.
> 
> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
> CC: stable at vger.kernel.org
> CC: Jisheng Zhang <jszhang@marvell.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>

Tested-by: Aaro Koskinen <aaro.koskinen@iki.fi>

Thanks,

A.

> ---
> 
> v2: Get the return value the right way round this time... After some
>     careful reasoning it really is that simple.
> 
>  arch/arm64/mm/dma-mapping.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e04082700bb1..1ffb7d5d299a 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>  	return 1;
>  }
>  
> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
> +{
> +	if (swiotlb)
> +		return swiotlb_dma_mapping_error(hwdev, addr);
> +	return 0;
> +}
> +
>  static struct dma_map_ops swiotlb_dma_ops = {
>  	.alloc = __dma_alloc,
>  	.free = __dma_free,
> @@ -366,7 +373,7 @@ static struct dma_map_ops swiotlb_dma_ops = {
>  	.sync_sg_for_cpu = __swiotlb_sync_sg_for_cpu,
>  	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
>  	.dma_supported = __swiotlb_dma_supported,
> -	.mapping_error = swiotlb_dma_mapping_error,
> +	.mapping_error = __swiotlb_dma_mapping_error,
>  };
>  
>  static int __init atomic_pool_init(void)
> -- 
> 2.11.0.dirty
> 

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Will Deacon]

On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
> being initialised.
> 
> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
> will only ever return the DMA-offset-adjusted physical address of the
> page passed in, thus we can report success unconditionally.
> 
> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
> CC: stable at vger.kernel.org
> CC: Jisheng Zhang <jszhang@marvell.com>
> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> v2: Get the return value the right way round this time... After some
>     careful reasoning it really is that simple.
> 
>  arch/arm64/mm/dma-mapping.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e04082700bb1..1ffb7d5d299a 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>  	return 1;
>  }
>  
> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
> +{
> +	if (swiotlb)
> +		return swiotlb_dma_mapping_error(hwdev, addr);
> +	return 0;
> +}

I was about to apply this, but I'm really uncomfortable with the way that
we call into swiotlb without initialising it. For example, if somebody
passes swiotlb=noforce on the command line and all of our memory is
DMA-able, then we don't call swiotlb_init but we will leave the DMA ops
intact. On a dma_map_page, we then end up in swiotlb_map_page. If, for
some reason or another, dma_capable fails (perhaps the address is out of
range), then we call map_single which will return SWIOTLB_MAP_ERROR
and subsequently phys_to_dma(dev, io_tlb_overflow_buffer);, which is
exactly what swiotlb_dma_mapping_error checks for. Except it won't get the
chance, because our swiotlb variable is false.

I can see three ways to resolve this:

1. Revert the hack that skips SWIOTLB initialisation and pay the 64m price
   (but this is configurable on the cmdline).

2. Keep the hack, but instead of skipping initialisation altogether,
   automatically adjust the bounce buffer size to a single entry. This
   shouldn't ever get used, but will allow the error paths to work.

3. We bite the bullet and implement some non-swiotlb DMA ops for the case
   when SWIOTLB is not used.

Thoughts?

Will

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Michael Zoran]

On Thu, 2017-01-26 at 12:52 +0000, Will Deacon wrote:
> On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
> > When bypassing SWIOTLB on small-memory systems, we need to avoid
> > calling
> > into swiotlb_dma_mapping_error() in exactly the same way as we
> > avoid
> > swiotlb_dma_supported(), because the former also relies on SWIOTLB
> > state
> > being initialised.
> > 
> > Under the assumptions for which we skip SWIOTLB,
> > dma_map_{single,page}()
> > will only ever return the DMA-offset-adjusted physical address of
> > the
> > page passed in, thus we can report success unconditionally.
> > 
> > Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when
> > necessary")
> > CC: stable at vger.kernel.org
> > CC: Jisheng Zhang <jszhang@marvell.com>
> > Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
> > Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> > ---
> > 
> > v2: Get the return value the right way round this time... After
> > some
> > ????careful reasoning it really is that simple.
> > 
> > ?arch/arm64/mm/dma-mapping.c | 9 ++++++++-
> > ?1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-
> > mapping.c
> > index e04082700bb1..1ffb7d5d299a 100644
> > --- a/arch/arm64/mm/dma-mapping.c
> > +++ b/arch/arm64/mm/dma-mapping.c
> > @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct
> > device *hwdev, u64 mask)
> > ?	return 1;
> > ?}
> > ?
> > +static int __swiotlb_dma_mapping_error(struct device *hwdev,
> > dma_addr_t addr)
> > +{
> > +	if (swiotlb)
> > +		return swiotlb_dma_mapping_error(hwdev, addr);
> > +	return 0;
> > +}
> 
> I was about to apply this, but I'm really uncomfortable with the way
> that
> we call into swiotlb without initialising it. For example, if
> somebody
> passes swiotlb=noforce on the command line and all of our memory is
> DMA-able, then we don't call swiotlb_init but we will leave the DMA
> ops
> intact. On a dma_map_page, we then end up in swiotlb_map_page. If,
> for
> some reason or another, dma_capable fails (perhaps the address is out
> of
> range), then we call map_single which will return SWIOTLB_MAP_ERROR
> and subsequently phys_to_dma(dev, io_tlb_overflow_buffer);, which is
> exactly what swiotlb_dma_mapping_error checks for. Except it won't
> get the
> chance, because our swiotlb variable is false.
> 
> I can see three ways to resolve this:
> 
> 1. Revert the hack that skips SWIOTLB initialisation and pay the 64m
> price
> ???(but this is configurable on the cmdline).
> 
> 2. Keep the hack, but instead of skipping initialisation altogether,
> ???automatically adjust the bounce buffer size to a single entry.
> This
> ???shouldn't ever get used, but will allow the error paths to work.
> 
> 3. We bite the bullet and implement some non-swiotlb DMA ops for the
> case
> ???when SWIOTLB is not used.
> 
> Thoughts?
> 
> Will
> 

I'm learning about the DMA APIs since I'm new here and just trying to
understand...

On the RPI 3, all the memory is DMA able if I understand.  All the DMA
APIs needs to do is just flush the various caches.  

To keep things as simple as possible, why not just have a seperate dma-
ops table for the simple case where all the functions are no-ops except
for the needed cache flushing?

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Robin Murphy]

On 26/01/17 12:52, Will Deacon wrote:
> On Wed, Jan 25, 2017 at 06:31:31PM +0000, Robin Murphy wrote:
>> When bypassing SWIOTLB on small-memory systems, we need to avoid calling
>> into swiotlb_dma_mapping_error() in exactly the same way as we avoid
>> swiotlb_dma_supported(), because the former also relies on SWIOTLB state
>> being initialised.
>>
>> Under the assumptions for which we skip SWIOTLB, dma_map_{single,page}()
>> will only ever return the DMA-offset-adjusted physical address of the
>> page passed in, thus we can report success unconditionally.
>>
>> Fixes: b67a8b29df7e ("arm64: mm: only initialize swiotlb when necessary")
>> CC: stable at vger.kernel.org
>> CC: Jisheng Zhang <jszhang@marvell.com>
>> Reported-by: Aaro Koskinen <aaro.koskinen@iki.fi>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> v2: Get the return value the right way round this time... After some
>>     careful reasoning it really is that simple.
>>
>>  arch/arm64/mm/dma-mapping.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
>> index e04082700bb1..1ffb7d5d299a 100644
>> --- a/arch/arm64/mm/dma-mapping.c
>> +++ b/arch/arm64/mm/dma-mapping.c
>> @@ -352,6 +352,13 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
>>  	return 1;
>>  }
>>  
>> +static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)
>> +{
>> +	if (swiotlb)
>> +		return swiotlb_dma_mapping_error(hwdev, addr);
>> +	return 0;
>> +}
> 
> I was about to apply this, but I'm really uncomfortable with the way that
> we call into swiotlb without initialising it. For example, if somebody
> passes swiotlb=noforce on the command line and all of our memory is
> DMA-able, then we don't call swiotlb_init but we will leave the DMA ops
> intact. On a dma_map_page, we then end up in swiotlb_map_page. If, for
> some reason or another, dma_capable fails (perhaps the address is out of
> range), then we call map_single which will return SWIOTLB_MAP_ERROR
> and subsequently phys_to_dma(dev, io_tlb_overflow_buffer);, which is
> exactly what swiotlb_dma_mapping_error checks for. Except it won't get the
> chance, because our swiotlb variable is false.

Right. Or to look at it another way (in isolation), this patch as-is at
least moves us from a real observed problem to a theoretical problem
involving a theoretical device :P

However, I do agree that skirting danger by calling into uninitialised
SWIOTLB state on the assumption that it 'should' be OK is grotty as
hell, and having had a closer look I found another sweet nugget - if
someone calls dma_alloc_coherent() in non-blocking context, for a
sufficiently large order that the initial __get_free_pages() call from
swiotlb_alloc_coherent() fails (hey, small-memory systems *are* going to
suffer fragmentation), then we'll end up poking around in yet more
uninitialised internals trying to allocate out of the non-existent
bounce buffer.

> I can see three ways to resolve this:
> 
> 1. Revert the hack that skips SWIOTLB initialisation and pay the 64m price
>    (but this is configurable on the cmdline).
> 
> 2. Keep the hack, but instead of skipping initialisation altogether,
>    automatically adjust the bounce buffer size to a single entry. This
>    shouldn't ever get used, but will allow the error paths to work.

Since the hack's been present in two kernel releases already, one of
them long-term stable, I suspect 2 is the only viable option of those,
although it does look to require juggling two different SWIOTLB init
functions, which appears fiddly at a glance.

> 3. We bite the bullet and implement some non-swiotlb DMA ops for the case
>    when SWIOTLB is not used.

I'd already started thinking along those lines in the process of writing
this patch - I'm happy to take that on, although it might be a wee bit
tight for 4.11 now.

Robin.

> 
> Thoughts?
> 
> Will
> 

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Aaro Koskinen]

Hi,

On Thu, Jan 26, 2017 at 03:20:47PM +0000, Robin Murphy wrote:
> hell, and having had a closer look I found another sweet nugget - if
> someone calls dma_alloc_coherent() in non-blocking context, for a
> sufficiently large order that the initial __get_free_pages() call from
> swiotlb_alloc_coherent() fails (hey, small-memory systems *are* going to
> suffer fragmentation), then we'll end up poking around in yet more
> uninitialised internals trying to allocate out of the non-existent
> bounce buffer.

I think this shouldn't happen anymore after 524dabe1c68e ("arm64: Fix
swiotlb fallback allocation").

A.

[PATCH v2] arm64: dma-mapping: Fix dma_mapping_error() when bypassing SWIOTLB

[Will Deacon]

On Thu, Jan 26, 2017 at 03:20:47PM +0000, Robin Murphy wrote:
> On 26/01/17 12:52, Will Deacon wrote:
> > 3. We bite the bullet and implement some non-swiotlb DMA ops for the case
> >    when SWIOTLB is not used.
> 
> I'd already started thinking along those lines in the process of writing
> this patch - I'm happy to take that on, although it might be a wee bit
> tight for 4.11 now.

Ok, so I've merged this patch as-is for 4.11 because it is an improvement
over what we had before. Let's look at using separate ops in the long run.

Will