Skip to contentSkip to conversation

mmc-uclass: spl: upriv->mmc override by host descriptor address

PATCH New Jagan Teki 2 participants 2 messages Nov 3, 2017 – Nov 10, 2017 MMC, SD and SDIO

lore
Share & tools
lore

Jagan Teki

author
#0 lore raw

PATCH 1/1 mmc-uclass: spl: upriv->mmc override by host descriptor address

This specific issue observed with SPL_DM_MMC in falcon mode on rk3288 which used dw_mmc.c driver.

Bug details:
-----------
based on the falcon configuration, SPL trying to read the kernel from specified sectors, while mmc sending multi-block command(CMD18) the
host descriptor address here next_addr(from dw_mmc.c, on below Bug log at blk_cnt = 938) is trying to override upriv(which further corrupting upriv->mmc) since after mmc pointing to wrong address which is causing next commands(CMD12)
is unable to get the host pointer since it's reading wrong address which eventually block the booting at mmc stage.

Bug log:
-------
For blk_cnt = 946
Before: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0xff7160fc
dwmci_set_idma_desc: addr = 0x274dfc0, next_addr = 0x7e080
After: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0xff7160fc For blk_cnt = 938
Before: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0xff7160fc
dwmci_set_idma_desc: addr = 0x274efc0, next_addr = 0x7e0c0
After: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0x274efc0

Based on the above information, this patch is trying to allocate mmc_uclass_priv using .priv_auto_alloc_size so if it is zero, the respective uclass driver is responsible for allocating any data required. So in this scenario memory is not override between upriv->mmc and host description.

mmc_uclass_priv with ..priv_auto_alloc_size:
--------------------------------------------
For blk_cnt = 938
Before: mmc_get_mmc_dev: dev = 0xff71608c, upriv = 0x0, mmc = 0xff716100
dwmci_set_idma_desc: addr = 0x274efc0, next_addr = 0x7e0c0
After: mmc_get_mmc_dev: dev = 0xff71608c, upriv = 0x0, mmc = 0xff716100

---
Note:
- debug code:
https://paste.ubuntu.com/25879159/
- Bug log:
https://paste.ubuntu.com/25879138/
- Fix log:
https://paste.ubuntu.com/25879139/

 drivers/mmc/mmc-uclass.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mmc/mmc-uclass.c +4 -0
index 5dda20c..cdb0d28 100644
@@ -299,5 +299,9 @@ UCLASS_DRIVER(mmc) = {
 	.id		= UCLASS_MMC,
 	.name		= "mmc",
 	.flags		= DM_UC_FLAG_SEQ_ALIAS,
+#ifdef CONFIG_SPL_BUILD
+	.priv_auto_alloc_size = sizeof(struct mmc_uclass_priv),
+#else
 	.per_device_auto_alloc_size = sizeof(struct mmc_uclass_priv),
+#endif
 };
Signature · 1 line
2.7.4
#1 lore raw

PATCH 1/1 mmc-uclass: spl: upriv->mmc override by host descriptor address

Hi Jagan,

Quoted 64 lines · Jagan Teki

This specific issue observed with SPL_DM_MMC in falcon mode on rk3288 which used dw_mmc.c driver.

Bug details:
-----------
based on the falcon configuration, SPL trying to read the kernel from specified sectors, while mmc sending multi-block command(CMD18) the
host descriptor address here next_addr(from dw_mmc.c, on below Bug log at blk_cnt = 938) is trying to override upriv(which further corrupting upriv->mmc) since after mmc pointing to wrong address which is causing next commands(CMD12)
is unable to get the host pointer since it's reading wrong address which eventually block the booting at mmc stage.

Bug log:
-------
For blk_cnt = 946
Before: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0xff7160fc
dwmci_set_idma_desc: addr = 0x274dfc0, next_addr = 0x7e080
After: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0xff7160fc For blk_cnt = 938
Before: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0xff7160fc
dwmci_set_idma_desc: addr = 0x274efc0, next_addr = 0x7e0c0
After: mmc_get_mmc_dev: dev = 0xff716088, upriv = 0x7e088, mmc = 0x274efc0

Based on the above information, this patch is trying to allocate mmc_uclass_priv using .priv_auto_alloc_size so if it is zero, the respective uclass driver is responsible for allocating any data required. So in this scenario memory is not override between upriv->mmc and host description.

mmc_uclass_priv with ..priv_auto_alloc_size:
--------------------------------------------
For blk_cnt = 938
Before: mmc_get_mmc_dev: dev = 0xff71608c, upriv = 0x0, mmc = 0xff716100
dwmci_set_idma_desc: addr = 0x274efc0, next_addr = 0x7e0c0
After: mmc_get_mmc_dev: dev = 0xff71608c, upriv = 0x0, mmc = 0xff716100

---
Note:
- debug code:
https://paste.ubuntu.com/25879159/
- Bug log:
https://paste.ubuntu.com/25879138/
- Fix log:
https://paste.ubuntu.com/25879139/

 drivers/mmc/mmc-uclass.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/mmc/mmc-uclass.c +4 -1
index 5dda20c..cdb0d28 100644
@@ -299,5 +299,9 @@ UCLASS_DRIVER(mmc) = {
        .id             = UCLASS_MMC,
        .name           = "mmc",
        .flags          = DM_UC_FLAG_SEQ_ALIAS,
+#ifdef CONFIG_SPL_BUILD
+       .priv_auto_alloc_size = sizeof(struct mmc_uclass_priv),
+#else
        .per_device_auto_alloc_size = sizeof(struct mmc_uclass_priv),
+#endif
 };
--
Quoted 2 lines

2.7.4

This is so strange, I don't really understand it.

But priv_auto_alloc_size should be used with dev_get_uclass_priv() which is what the code is using.

priv_auto_alloc_size is used for the uclass' priv-> pointer. We don't need the uclass to store anything as far as I can tell.

So I am not sure why your fix works, but it seems wrong to me.

But as I say, it is strange, and perhaps there is something else wrong.

Regards,
Simon