Fix sdhci-of-esdhc DMA coherency

PATCH In review Russell King - ARM Linux admin 7 participants 13 messages Sep 22, 2019 – Feb 5, 2020 1 message not in the archive

Subsystems (4)

Device tree bindings Arm i.MX and Layerscape MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Russell King - ARM Linux admin

author

2019-09-22 10:23 UTC #0 lore raw

cover Fix sdhci-of-esdhc DMA coherency

Hi,

The following patch set is a result of looking at the ADMA errors observed on SolidRun's LX2160A board, caused by coherency mismatch between DT and hardware.

The first patch improves the debugging by dumping the ADMA table using the DMA address, which can be compared with the DMA address reported in the register dump. It also prints the interrupt status that we read, whereas the register dump contains the _cleared_ interrupt status.

The second patch modifies sdhci-of-esdhc to set the DMA snoop bit depending on whether DT marks the device as coherent or not. With this patch applied, driver will now set the hardware to match the contents of DT, rather than always DMA snooping. As explained by Robin, if the hardware snoops the CPU caches, but the DMA API layer does not expect this, the hardware can read stale data. The hardware state and the DMA API must always agree wrt this, for the sake of data integrity. Mismatch risks substantial filesystem corruption.

The third patch marks the LX2160A esdhc devices as dma-coherent. Other users of the driver are not touched; I do not have the information to know whether DMA snooping (coherency) is available on other SoCs using this driver. However, given the risk of filesystem corruption, it is important no matter what that the DMA API layer and the hardware agree - even if it means defaulting to being non-coherent.

 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi |  2 ++
 drivers/mmc/host/sdhci-of-esdhc.c              |  7 ++++++-
 drivers/mmc/host/sdhci.c                       | 15 ++++++++++-----
 3 files changed, 18 insertions(+), 6 deletions(-)

Signature · 3 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Russell King

2019-09-22 10:27 UTC #1 lore raw

PATCH 3/3 arm64: dts: mark lx2160a esdhc controllers dma coherent

The LX2160A esdhc controllers are setup by the driver to be DMA coherent, but without marking them as such in DT, Linux thinks they are not. This can lead to random sporadic DMA errors, even to the extent of preventing boot, such as:

mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card

These are caused by the device's descriptor fetch hitting speculatively loaded CPU cache lines that the CPU does not see through the normal, non-cacheable DMA coherent mapping that it uses for non-coherent devices.

DT and the device must agree wrt whether the device is DMA coherent or not.

Signed-off-by Russell King <[email protected]>

---
 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi +2 -0

index 36b153e3da47..508af23edef0 100644
@@ -593,6 +593,7 @@
 			reg = <0x0 0x2140000 0x0 0x10000>;
 			interrupts = <0 28 0x4>; /* Level high type */
 			clocks = <&clockgen 4 1>;
+			dma-coherent;
 			voltage-ranges = <1800 1800 3300 3300>;
 			sdhci,auto-cmd12;
 			little-endian;

@@ -605,6 +606,7 @@
 			reg = <0x0 0x2150000 0x0 0x10000>;
 			interrupts = <0 63 0x4>; /* Level high type */
 			clocks = <&clockgen 4 1>;
+			dma-coherent;
 			voltage-ranges = <1800 1800 3300 3300>;
 			sdhci,auto-cmd12;
 			broken-cd;

Signature · 1 line

2.7.4

Li Yang

Acked-by on v1 3/3

2019-09-23 16:00 UTC #2 lore raw

Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent

Quoted 31 lines · Russell King

The LX2160A esdhc controllers are setup by the driver to be DMA coherent, but without marking them as such in DT, Linux thinks they are not. This can lead to random sporadic DMA errors, even to the extent of preventing boot, such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
These are caused by the device's descriptor fetch hitting speculatively loaded CPU cache lines that the CPU does not see through the normal, non-cacheable DMA coherent mapping that it uses for non-coherent devices.

Quoted 4 lines

DT and the device must agree wrt whether the device is DMA coherent or not.
Signed-off-by Russell King <[email protected]>

Acked-by Li Yang <[email protected]>

Quoted 27 lines

---
 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi +2 -1

index 36b153e3da47..508af23edef0 100644
@@ -593,6 +593,7 @@
                        reg = <0x0 0x2140000 0x0 0x10000>;
                        interrupts = <0 28 0x4>; /* Level high type */
                        clocks = <&clockgen 4 1>;
+                       dma-coherent;
                        voltage-ranges = <1800 1800 3300 3300>;
                        sdhci,auto-cmd12;
                        little-endian;

@@ -605,6 +606,7 @@
                        reg = <0x0 0x2150000 0x0 0x10000>;
                        interrupts = <0 63 0x4>; /* Level high type */
                        clocks = <&clockgen 4 1>;
+                       dma-coherent;
                        voltage-ranges = <1800 1800 3300 3300>;
                        sdhci,auto-cmd12;
                        broken-cd;
--

2.7.4

Ulf Hansson

2019-09-23 21:26 UTC #3 lore raw

Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent

In reply to Russell King

Quoted 31 lines · Russell King

The LX2160A esdhc controllers are setup by the driver to be DMA coherent, but without marking them as such in DT, Linux thinks they are not. This can lead to random sporadic DMA errors, even to the extent of preventing boot, such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
These are caused by the device's descriptor fetch hitting speculatively loaded CPU cache lines that the CPU does not see through the normal, non-cacheable DMA coherent mapping that it uses for non-coherent devices.

Quoted 4 lines

DT and the device must agree wrt whether the device is DMA coherent or not.
Signed-off-by Russell King <[email protected]>

As I am picking up patch1 and patch2 from this series, I can also help out and pick up this one, if that is okay by people?

Kind regards
Uffe

Quoted 32 lines

---
 arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi | 2 ++
 1 file changed, 2 insertions(+)

arch/arm64/boot/dts/freescale/fsl-lx2160a.dtsi +2 -1

index 36b153e3da47..508af23edef0 100644
@@ -593,6 +593,7 @@
                        reg = <0x0 0x2140000 0x0 0x10000>;
                        interrupts = <0 28 0x4>; /* Level high type */
                        clocks = <&clockgen 4 1>;
+                       dma-coherent;
                        voltage-ranges = <1800 1800 3300 3300>;
                        sdhci,auto-cmd12;
                        little-endian;

@@ -605,6 +606,7 @@
                        reg = <0x0 0x2150000 0x0 0x10000>;
                        interrupts = <0 63 0x4>; /* Level high type */
                        clocks = <&clockgen 4 1>;
+                       dma-coherent;
                        voltage-ranges = <1800 1800 3300 3300>;
                        sdhci,auto-cmd12;
                        broken-cd;
--

2.7.4

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Leo Li

2019-09-24 03:41 UTC #4 lore raw

RE: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent

Quoted 49 lines

-----Original Message-----
From: Ulf Hansson <[email protected]>
Sent: Monday, September 23, 2019 4:27 PM
To: Russell King <[email protected]>
Cc: Robin Murphy <[email protected]>; dann frazier <[email protected]>; Will Deacon <[email protected]>; Nicolin Chen <[email protected]>; Y.b. Lu <[email protected]>; Christoph Hellwig <[email protected]>; Mark Rutland <[email protected]>; DTML <[email protected]>; Leo Li <[email protected]>; Rob Herring <[email protected]>; Shawn Guo <[email protected]>; Linux ARM <[email protected]>
Subject: Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent
Quoted 31 lines · Russell King
The LX2160A esdhc controllers are setup by the driver to be DMA coherent, but without marking them as such in DT, Linux thinks they are not. This can lead to random sporadic DMA errors, even to the extent of preventing boot, such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
These are caused by the device's descriptor fetch hitting speculatively loaded CPU cache lines that the CPU does not see through the normal, non-cacheable DMA coherent mapping that it uses for non-coherent devices.

Quoted 7 lines

Quoted 4 lines
DT and the device must agree wrt whether the device is DMA coherent or not.
Signed-off-by Russell King <[email protected]>
As I am picking up patch1 and patch2 from this series, I can also help out and pick up this one, if that is okay by people?

There is some concern recently from linux-next maintainer about this causing potential conflicts. https://lkml.org/lkml/2019/9/15/225

Regards,
Leo

Robin Murphy

2019-09-24 18:04 UTC #5 lore raw

Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent

Quoted 52 lines · Leo Li

Quoted 52 lines
-----Original Message-----
From: Ulf Hansson <[email protected]>
Sent: Monday, September 23, 2019 4:27 PM
To: Russell King <[email protected]>
Cc: Robin Murphy <[email protected]>; dann frazier <[email protected]>; Will Deacon <[email protected]>; Nicolin Chen <[email protected]>; Y.b. Lu <[email protected]>; Christoph Hellwig <[email protected]>; Mark Rutland <[email protected]>; DTML <[email protected]>; Leo Li <[email protected]>; Rob Herring <[email protected]>; Shawn Guo <[email protected]>; Linux ARM <[email protected]>
Subject: Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent
Quoted 34 lines · Russell King
The LX2160A esdhc controllers are setup by the driver to be DMA coherent, but without marking them as such in DT, Linux thinks they are not. This can lead to random sporadic DMA errors, even to the extent of preventing boot, such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
These are caused by the device's descriptor fetch hitting speculatively loaded CPU cache lines that the CPU does not see through the normal, non-cacheable DMA coherent mapping that it uses for non-coherent devices.
DT and the device must agree wrt whether the device is DMA coherent or not.

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Signed-off-by Russell King <[email protected]>
As I am picking up patch1 and patch2 from this series, I can also help out and pick up this one, if that is okay by people?
There is some concern recently from linux-next maintainer about this causing potential conflicts. https://lkml.org/lkml/2019/9/15/225

In principle, it should be fine for #2 and #3 to go via separate trees - #3 aligns the DT with the existing behaviour of the driver, while #2 will make the driver behave correctly whichever state the DT is in.

Robin.

Ulf Hansson

2019-09-27 18:35 UTC #6 lore raw

Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent

Quoted 59 lines · Robin Murphy

Quoted 57 lines · Leo Li
Quoted 56 lines
-----Original Message-----
From: Ulf Hansson <[email protected]>
Sent: Monday, September 23, 2019 4:27 PM
To: Russell King <[email protected]>
Cc: Robin Murphy <[email protected]>; dann frazier <[email protected]>; Will Deacon <[email protected]>; Nicolin Chen <[email protected]>; Y.b. Lu <[email protected]>; Christoph Hellwig <[email protected]>; Mark Rutland <[email protected]>; DTML <[email protected]>; Leo Li <[email protected]>; Rob Herring <[email protected]>; Shawn Guo <[email protected]>; Linux ARM <[email protected]>
Subject: Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent
Quoted 31 lines · Russell King
The LX2160A esdhc controllers are setup by the driver to be DMA coherent, but without marking them as such in DT, Linux thinks they are not. This can lead to random sporadic DMA errors, even to the extent of preventing boot, such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
These are caused by the device's descriptor fetch hitting speculatively loaded CPU cache lines that the CPU does not see through the normal, non-cacheable DMA coherent mapping that it uses for non-coherent devices.
Quoted 4 lines
DT and the device must agree wrt whether the device is DMA coherent or not.
Signed-off-by Russell King <[email protected]>
As I am picking up patch1 and patch2 from this series, I can also help out and pick up this one, if that is okay by people?

Quoted 7 lines

Quoted 1 line
There is some concern recently from linux-next maintainer about this causing potential conflicts. https://lkml.org/lkml/2019/9/15/225
In principle, it should be fine for #2 and #3 to go via separate trees - #3 aligns the DT with the existing behaviour of the driver, while #2 will make the driver behave correctly whichever state the DT is in.
Robin.

That's right, so I am leaving this patch for arm-soc people to pick up then.

Kind regards
Uffe

Shawn Guo

Applied

2019-10-07 12:42 UTC #7 lore raw

Re: [PATCH 3/3] arm64: dts: mark lx2160a esdhc controllers dma coherent

In reply to Russell King

Quoted 30 lines · Russell King

The LX2160A esdhc controllers are setup by the driver to be DMA coherent, but without marking them as such in DT, Linux thinks they are not. This can lead to random sporadic DMA errors, even to the extent of preventing boot, such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
These are caused by the device's descriptor fetch hitting speculatively loaded CPU cache lines that the CPU does not see through the normal, non-cacheable DMA coherent mapping that it uses for non-coherent devices.

Quoted 4 lines

DT and the device must agree wrt whether the device is DMA coherent or not.
Signed-off-by Russell King <[email protected]>

Applied, thanks.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Y.b. Lu

2020-01-20 10:11 UTC #9 lore raw

RE: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence

Hi Russell,

Recently I got eSDHC ADMA issue on PowerPC T2080 platform. After checking, the issue is related to this patch. This patch was to make eSDHC DMA SNOOP bit set per dma-coherent. That resolved issue on LX2160A ARM64 platform. However on T2080, we are facing similar issue again. It didn't have dma-coherent in dts. Adding dma-coherent in dts, or reverting the patch could resolve the problem.

Would you please help to have a look at it too? Thanks:)

mmc0: ADMA error: 0x02000000
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002002
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
NET: Registered protocol family 10
mmc0: sdhci: Present: 0x01fd020a | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000020b8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f100f | Sig enab: 0x037f100b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002002
mmc0: sdhci: Caps: 0x34fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x003b3733
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x0000000d | ADMA Ptr: 0x00000000f35ad20c
mmc0: sdhci: ============================================
mmc0: sdhci: f35ad200: DMA 0x00000000f3587710, LEN 0x0008, Attr=0x23
mmc0: error -5 whilst initialising SD card

Best regards,
Yangbo Lu

Quoted 78 lines

-----Original Message-----
From: Russell King <[email protected]> On Behalf Of Russell King
Sent: Sunday, September 22, 2019 6:27 PM
To: Robin Murphy <[email protected]>; dann frazier <[email protected]>; Will Deacon <[email protected]>; Nicolin Chen <[email protected]>; Y.b. Lu <[email protected]>; Christoph Hellwig <[email protected]>
Cc: Adrian Hunter <[email protected]>; Ulf Hansson <[email protected]>; [email protected]
Subject: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence
We must not unconditionally set the DMA snoop bit; if the DMA API is assuming that the device is not DMA coherent, and the device snoops the CPU caches, the device can see stale cache lines brought in by speculative prefetch.
This leads to the device seeing stale data, potentially resulting in corrupted data transfers. Commonly, this results in a descriptor fetch error such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
but can lead to other errors, and potentially direct the SDHCI controller to read/write data to other memory locations (e.g. if a valid descriptor is visible to the device in a stale cache line.)
Fix this by ensuring that the DMA snoop bit corresponds with the behaviour of the DMA API. Since the driver currently only supports DT, use of_dma_is_coherent(). Note that device_get_dma_attr() can not be used as that risks re-introducing this bug if/when the driver is converted to ACPI.
Signed-off-by Russell King <[email protected]>
---
 drivers/mmc/host/sdhci-of-esdhc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci-of-esdhc.c +6 -1
index 4dd43b1adf2c..74de5e8c45c8 100644
@@ -495,7 +495,12 @@ static int esdhc_of_enable_dma(struct sdhci_host
 *host)
 		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
 
 	value = sdhci_readl(host, ESDHC_DMA_SYSCTL);
-	value |= ESDHC_DMA_SNOOP;
+
+	if (of_dma_is_coherent(dev->of_node))
+		value |= ESDHC_DMA_SNOOP;
+	else
+		value &= ~ESDHC_DMA_SNOOP;
+
 	sdhci_writel(host, value, ESDHC_DMA_SYSCTL);
 	return 0;
}
--
2.7.4

Russell King - ARM Linux admin

author

2020-01-20 10:21 UTC #10 lore raw

Re: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence

Quoted 8 lines · Y.b. Lu

Hi Russell,
Recently I got eSDHC ADMA issue on PowerPC T2080 platform. After checking, the issue is related to this patch. This patch was to make eSDHC DMA SNOOP bit set per dma-coherent. That resolved issue on LX2160A ARM64 platform. However on T2080, we are facing similar issue again. It didn't have dma-coherent in dts. Adding dma-coherent in dts, or reverting the patch could resolve the problem.
Would you please help to have a look at it too? Thanks:)

All aspects of this was discussed extensively by many parties, and as far as I remember, no conclusion was reached - the discussion became rather unproductive, so I walked away from it.

I've more or less washed my hands of this in disgust that no way forward can be found, not even reverting the patch (and then I'll just carry the patch locally, so at least my machines work - shame for other ARM64 folk trying to use the LX2160A.)

Quoted 106 lines

mmc0: ADMA error: 0x02000000
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002002
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
NET: Registered protocol family 10
mmc0: sdhci: Present: 0x01fd020a | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000020b8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f100f | Sig enab: 0x037f100b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002002
mmc0: sdhci: Caps: 0x34fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x003b3733
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x0000000d | ADMA Ptr: 0x00000000f35ad20c
mmc0: sdhci: ============================================
mmc0: sdhci: f35ad200: DMA 0x00000000f3587710, LEN 0x0008, Attr=0x23
mmc0: error -5 whilst initialising SD card
Best regards,
Yangbo Lu
Quoted 78 lines
-----Original Message-----
From: Russell King <[email protected]> On Behalf Of Russell King
Sent: Sunday, September 22, 2019 6:27 PM
To: Robin Murphy <[email protected]>; dann frazier <[email protected]>; Will Deacon <[email protected]>; Nicolin Chen <[email protected]>; Y.b. Lu <[email protected]>; Christoph Hellwig <[email protected]>
Cc: Adrian Hunter <[email protected]>; Ulf Hansson <[email protected]>; [email protected]
Subject: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence
We must not unconditionally set the DMA snoop bit; if the DMA API is assuming that the device is not DMA coherent, and the device snoops the CPU caches, the device can see stale cache lines brought in by speculative prefetch.
This leads to the device seeing stale data, potentially resulting in corrupted data transfers. Commonly, this results in a descriptor fetch error such as:
mmc0: ADMA error
mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
mmc0: sdhci: Sys addr: 0x00000000 | Version: 0x00002202
mmc0: sdhci: Blk size: 0x00000008 | Blk cnt: 0x00000001
mmc0: sdhci: Argument: 0x00000000 | Trn mode: 0x00000013
mmc0: sdhci: Present: 0x01f50008 | Host ctl: 0x00000038
mmc0: sdhci: Power: 0x00000003 | Blk gap: 0x00000000
mmc0: sdhci: Wake-up: 0x00000000 | Clock: 0x000040d8
mmc0: sdhci: Timeout: 0x00000003 | Int stat: 0x00000001
mmc0: sdhci: Int enab: 0x037f108f | Sig enab: 0x037f108b
mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00002202
mmc0: sdhci: Caps: 0x35fa0000 | Caps_1: 0x0000af00
mmc0: sdhci: Cmd: 0x0000333a | Max curr: 0x00000000
mmc0: sdhci: Resp[0]: 0x00000920 | Resp[1]: 0x001d8a33
mmc0: sdhci: Resp[2]: 0x325b5900 | Resp[3]: 0x3f400e00
mmc0: sdhci: Host ctl2: 0x00000000
mmc0: sdhci: ADMA Err: 0x00000009 | ADMA Ptr: 0x000000236d43820c
mmc0: sdhci: ============================================
mmc0: error -5 whilst initialising SD card
but can lead to other errors, and potentially direct the SDHCI controller to read/write data to other memory locations (e.g. if a valid descriptor is visible to the device in a stale cache line.)
Fix this by ensuring that the DMA snoop bit corresponds with the behaviour of the DMA API. Since the driver currently only supports DT, use of_dma_is_coherent(). Note that device_get_dma_attr() can not be used as that risks re-introducing this bug if/when the driver is converted to ACPI.
Signed-off-by Russell King <[email protected]>
---
 drivers/mmc/host/sdhci-of-esdhc.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci-of-esdhc.c +6 -1
index 4dd43b1adf2c..74de5e8c45c8 100644
@@ -495,7 +495,12 @@ static int esdhc_of_enable_dma(struct sdhci_host
 *host)
 		dma_set_mask_and_coherent(dev, DMA_BIT_MASK(40));
 
 	value = sdhci_readl(host, ESDHC_DMA_SYSCTL);
-	value |= ESDHC_DMA_SNOOP;
+
+	if (of_dma_is_coherent(dev->of_node))
+		value |= ESDHC_DMA_SNOOP;
+	else
+		value &= ~ESDHC_DMA_SNOOP;
+
 	sdhci_writel(host, value, ESDHC_DMA_SYSCTL);
 	return 0;
}
--
2.7.4

Signature · 3 lines

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up

Robin Murphy

2020-01-20 13:34 UTC #11 lore raw

Re: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence

In reply to Y.b. Lu

Quoted 6 lines · Y.b. Lu

Hi Russell,
Recently I got eSDHC ADMA issue on PowerPC T2080 platform. After checking, the issue is related to this patch. This patch was to make eSDHC DMA SNOOP bit set per dma-coherent. That resolved issue on LX2160A ARM64 platform. However on T2080, we are facing similar issue again. It didn't have dma-coherent in dts. Adding dma-coherent in dts, or reverting the patch could resolve the problem.

Arguably updating the DTS would be the most accurate option, since it would be describing the hardware more correctly, however if there are reasons for that not being sufficient (e.g. DTBs baked into firmware, or worries of confusing some other DT consumer) then something like the below seems reasonable (albeit a little crude) IMO.

Robin.

----->8-----
From fafad319893b4168fcccc5445543caf876a0be2d Mon Sep 17 00:00:00 2001
Message-Id: <fafad319893b4168fcccc5445543caf876a0be2d.1579526755.git.robin.murphy@arm.com>
From: Robin Murphy <[email protected]>
Date: Mon, 20 Jan 2020 13:11:59 +0000
Subject: [PATCH] mmc: sdhci-of-esdhc: Restore coherency for PPC platforms

Historically, not all PPC platforms have supported per-device coherency, and some may rely on platform-level assumptions rather than explicitly specifying the "dma-coherent" propert in their DT. Although the eSDHC driver needs to tie in to per-device coherency to work correctly on arm/arm64 platforms, this has apparently caused problems for PPC, so restore the previous behaviour there with a special case.

Fixes 121bd08b029e ("mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence")

Reported-by Yangbo Lu <[email protected]>

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

---
 drivers/mmc/host/sdhci-of-esdhc.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/mmc/host/sdhci-of-esdhc.c +2 -1

index 500f70a6ee42..a2599368b2bd 100644
@@ -527,7 +527,8 @@ static int esdhc_of_enable_dma(struct sdhci_host *host)
 
 	value = sdhci_readl(host, ESDHC_DMA_SYSCTL);
 
-	if (of_dma_is_coherent(dev->of_node))
+	/* Historically, PPC has always assumed coherency here */
+	if (IS_ENABLED(CONFIG_PPC) || of_dma_is_coherent(dev->of_node))
 		value |= ESDHC_DMA_SNOOP;
 	else
 		value &= ~ESDHC_DMA_SNOOP;

Signature · 1 line

2.23.0.dirty

Y.b. Lu

2020-02-05 05:56 UTC #12 lore raw

RE: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence

Hi Robin,

Thanks lot for your suggestion. I prefer your fix-up patch to treat PowerPC as a special case.

Hi Adrian and Uffe,

Do you think it is ok? Should I help to re-send that patch to mmc mailing list for reviewing. Thanks.

Best regards,
Yangbo Lu

Quoted 75 lines

-----Original Message-----
From: Robin Murphy <[email protected]>
Sent: Monday, January 20, 2020 9:34 PM
To: Y.b. Lu <[email protected]>; Russell King <[email protected]>
Cc: Adrian Hunter <[email protected]>; Ulf Hansson <[email protected]>; [email protected]; dann frazier <[email protected]>; Will Deacon <[email protected]>; Nicolin Chen <[email protected]>; Christoph Hellwig <[email protected]>
Subject: Re: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence
Quoted 3 lines · Y.b. Lu
Hi Russell,
Recently I got eSDHC ADMA issue on PowerPC T2080 platform. After
checking, the issue is related to this patch.
Quoted 1 line
This patch was to make eSDHC DMA SNOOP bit set per dma-coherent. That
resolved issue on LX2160A ARM64 platform.
Quoted 1 line
However on T2080, we are facing similar issue again. It didn't have
dma-coherent in dts.
Quoted 1 line
Adding dma-coherent in dts, or reverting the patch could resolve the
problem.
Arguably updating the DTS would be the most accurate option, since it would be describing the hardware more correctly, however if there are reasons for that not being sufficient (e.g. DTBs baked into firmware, or worries of confusing some other DT consumer) then something like the below seems reasonable (albeit a little crude) IMO.
Robin.
----->8-----
From fafad319893b4168fcccc5445543caf876a0be2d Mon Sep 17 00:00:00
2001
Message-Id:
<fafad319893b4168fcccc5445543caf876a0be2d.1579526755.git.robin.murph [email protected]>
From: Robin Murphy <[email protected]>
Date: Mon, 20 Jan 2020 13:11:59 +0000
Subject: [PATCH] mmc: sdhci-of-esdhc: Restore coherency for PPC platforms
Historically, not all PPC platforms have supported per-device coherency, and some may rely on platform-level assumptions rather than explicitly specifying the "dma-coherent" propert in their DT. Although the eSDHC driver needs to tie in to per-device coherency to work correctly on arm/arm64 platforms, this has apparently caused problems for PPC, so restore the previous behaviour there with a special case.
Fixes 121bd08b029e ("mmc: sdhci-of-esdhc: set DMA snooping based on
DMA
coherence")
Reported-by Yangbo Lu <[email protected]>
Signed-off-by Robin Murphy <[email protected]>
---
  drivers/mmc/host/sdhci-of-esdhc.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)
drivers/mmc/host/sdhci-of-esdhc.c +2 -1
index 500f70a6ee42..a2599368b2bd 100644
@@ -527,7 +527,8 @@ static int esdhc_of_enable_dma(struct sdhci_host
 *host)
 
  	value = sdhci_readl(host, ESDHC_DMA_SYSCTL);
 
-	if (of_dma_is_coherent(dev->of_node))
+	/* Historically, PPC has always assumed coherency here */
+	if (IS_ENABLED(CONFIG_PPC) || of_dma_is_coherent(dev->of_node))
  		value |= ESDHC_DMA_SNOOP;
  	else
value &= ~ESDHC_DMA_SNOOP;
--
2.23.0.dirty

Ulf Hansson

2020-02-05 07:57 UTC #13 lore raw

Re: [PATCH 2/3] mmc: sdhci-of-esdhc: set DMA snooping based on DMA coherence

Quoted 9 lines · Y.b. Lu

Hi Robin,
Thanks lot for your suggestion. I prefer your fix-up patch to treat PowerPC as a special case.
Hi Adrian and Uffe,
Do you think it is ok? Should I help to re-send that patch to mmc mailing list for reviewing. Thanks.

commit dabf6b36b83a18d57e3d4b9d50544ed040d86255
Author: Michael Ellerman <[email protected]>
Date: Sun Jan 26 22:52:47 2020 +1100
of: Add OF_DMA_DEFAULT_COHERENT & select it on powerpc

This one is already in Linus' tree, I suppose that is sufficient.

[...]

Kind regards
Uffe