[U-Boot] [PATCH] mmc-uclass: spl: upriv->mmc override by host descriptor address

[Simon Glass]

Hi Jagan,

On 3 November 2017 at 06:05, Jagan Teki <jagannadh.teki at gmail.com> wrote:
> 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
>
> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com>
> ---
> 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(+)
>
> diff --git a/drivers/mmc/mmc-uclass.c b/drivers/mmc/mmc-uclass.c
> index 5dda20c..cdb0d28 100644
> --- a/drivers/mmc/mmc-uclass.c
> +++ b/drivers/mmc/mmc-uclass.c
> @@ -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
>  };
> --
> 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