USB/swiotlb failure on arm64/RPi3

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

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?

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.

From an ARM64 subsystem point of view, what exactly is the

Quoted 7 lines · Robin Murphy

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?

Quoted 6 lines · Robin Murphy

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?

Quoted 11 lines · Michael Zoran

Quoted 6 lines · Robin Murphy

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.

Quoted 3 lines

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:

arch/arm64/mm/dma-mapping.c static int __swiotlb_dma_supported(struct device *hwdev, u64 mask) · line 360

Quoted 6 lines

arch/arm64/mm/dma-mapping.c +6 -0

+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

Quoted 42 lines · Arnd Bergmann

On Wed, Jan 25, 2017 at 1:37 PM, Michael Zoran <[email protected]

Quoted 11 lines

wrote:

Quoted 8 lines · Robin Murphy

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.

Quoted 5 lines

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.

Quoted 4 lines

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:

Review

Quoted 7 lines

+static int __swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t addr)

(quoted diff) +5 -0

+{
+???????if (swiotlb)
+???????????????return swiotlb_dma_mapping_error(hwdev, addr);
+???????return 1;
+}

Shouldn't that be

Quoted 5 lines

?????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.

Quoted 3 lines

????Arnd

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 <[email protected]>

Reported-by Aaro Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

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(-)

arch/arm64/mm/dma-mapping.c +8 -1

index e04082700bb1..1ffb7d5d299a 100644
@@ -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)

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

Quoted 29 lines · 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 <[email protected]>

Reported-by Aaro Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

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(-)

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 <[email protected]>

Reported-by Aaro Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

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(-)

arch/arm64/mm/dma-mapping.c +8 -1

index e04082700bb1..1ffb7d5d299a 100644
@@ -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)

Signature · 1 line

2.11.0.dirty

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 <[email protected]>

Reported-by Aaro Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

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(-)

arch/arm64/mm/dma-mapping.c +0 -0

index e04082700bb1..1ffb7d5d299a 100644

Quoted 41 lines · 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 <[email protected]>

Reported-by Aaro Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

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(-)

arch/arm64/mm/dma- renamed from arch/arm64/mm/dma-mapping.c +0 -0

mapping.c
index e04082700bb1..1ffb7d5d299a 100644

arch/arm64/mm/dma-mapping.c +0 -0

@@ -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)

arch/arm64/mm/dma-mapping.c static int __swiotlb_dma_supported(struct device *hwdev, u64 mask) · line 360

Quoted 5 lines

arch/arm64/mm/dma-mapping.c +5 -0

+{
+	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

Quoted 27 lines · 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 <[email protected]>

Reported-by Aaro Koskinen <[email protected]>

Signed-off-by Robin Murphy <[email protected]>

---

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(-)

arch/arm64/mm/dma-mapping.c +0 -0

index e04082700bb1..1ffb7d5d299a 100644

arch/arm64/mm/dma-mapping.c static int __swiotlb_dma_supported(struct device *hwdev, u64 mask) · line 360

Quoted 9 lines

arch/arm64/mm/dma-mapping.c +6 -0

@@ -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

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.

Quoted 2 lines · Will Deacon

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.