[PATCH] spl: mmc: Use correct MMC device when loading image

[PATCH] spl: mmc: Use correct MMC device when loading image

[Harald Seiler]

When attempting to load images from multiple MMC devices in sequence,
spl_mmc_load() chooses the wrong device from the second attempt onwards.

The reason is that MMC initialization is only done on its first call and
spl_mmc_load() will then continue using this same device for all future
calls.

Fix this by checking the devnum of the "cached" device struct against
the one which is requested.  If they match, use the cached one but if
they do not match, initialize the new device.

This fixes specifying multiple MMC devices in the SPL's boot order to
fall back when U-Boot Proper is corrupted or missing on the first
attempted MMC device.

Fixes: e1eb6ada4e38 ("spl: Make image loader infrastructure more universal")
Signed-off-by: Harald Seiler <hws@denx.de>
---
 common/spl/spl_mmc.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index e1a7d25bd0..22c8a8097c 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -371,9 +371,11 @@ int spl_mmc_load(struct spl_image_info *spl_image,
 	u32 boot_mode;
 	int err = 0;
 	__maybe_unused int part = 0;
+	int mmc_dev;
 
-	/* Perform peripheral init only once */
-	if (!mmc) {
+	/* Perform peripheral init only once for an mmc device */
+	mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
+	if (!mmc || mmc->block_dev.devnum != mmc_dev) {
 		err = spl_mmc_find_device(&mmc, bootdev->boot_device);
 		if (err)
 			return err;
-- 
2.36.1

Re: [PATCH] spl: mmc: Use correct MMC device when loading image

[Marek Vasut]

On 7/6/22 12:58, Harald Seiler wrote:
> When attempting to load images from multiple MMC devices in sequence,
> spl_mmc_load() chooses the wrong device from the second attempt onwards.
> 
> The reason is that MMC initialization is only done on its first call and
> spl_mmc_load() will then continue using this same device for all future
> calls.
> 
> Fix this by checking the devnum of the "cached" device struct against
> the one which is requested.  If they match, use the cached one but if
> they do not match, initialize the new device.
> 
> This fixes specifying multiple MMC devices in the SPL's boot order to
> fall back when U-Boot Proper is corrupted or missing on the first
> attempted MMC device.
> 
> Fixes: e1eb6ada4e38 ("spl: Make image loader infrastructure more universal")
> Signed-off-by: Harald Seiler <hws@denx.de>

Reviewed-by: Marek Vasut <marex@denx.de>

Re: [PATCH] spl: mmc: Use correct MMC device when loading image

[Quentin Schulz]

Hi Harald,

On 7/6/22 12:58, Harald Seiler wrote:
> When attempting to load images from multiple MMC devices in sequence,
> spl_mmc_load() chooses the wrong device from the second attempt onwards.
> 
> The reason is that MMC initialization is only done on its first call and
> spl_mmc_load() will then continue using this same device for all future
> calls.
> 
> Fix this by checking the devnum of the "cached" device struct against
> the one which is requested.  If they match, use the cached one but if
> they do not match, initialize the new device.
> 
> This fixes specifying multiple MMC devices in the SPL's boot order to
> fall back when U-Boot Proper is corrupted or missing on the first
> attempted MMC device.
> 
> Fixes: e1eb6ada4e38 ("spl: Make image loader infrastructure more universal")
> Signed-off-by: Harald Seiler <hws@denx.de>
> ---
>   common/spl/spl_mmc.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index e1a7d25bd0..22c8a8097c 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -371,9 +371,11 @@ int spl_mmc_load(struct spl_image_info *spl_image,
>   	u32 boot_mode;
>   	int err = 0;
>   	__maybe_unused int part = 0;
> +	int mmc_dev;
>   
> -	/* Perform peripheral init only once */
> -	if (!mmc) {
> +	/* Perform peripheral init only once for an mmc device */
> +	mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
> +	if (!mmc || mmc->block_dev.devnum != mmc_dev) {

block_dev field only exists when CONFIG_BLK is not defined, c.f. 
https://elixir.bootlin.com/u-boot/latest/source/include/mmc.h#L705

Considering the commit introducing this, c.f. 
https://source.denx.de/u-boot/u-boot/-/commit/33fb211dd2706e666db4008801dc0d5903fd82f6, 
I can suggest the following diff (which makes it compile with 
rk3399-puma_defconfig):

diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
index e0e1a80536..c40b436a11 100644
--- a/common/spl/spl_mmc.c
+++ b/common/spl/spl_mmc.c
@@ -409,10 +409,17 @@ int spl_mmc_load(struct spl_image_info *spl_image,
         int err = 0;
         __maybe_unused int part = 0;
         int mmc_dev;
+       struct blk_desc *block_dev;

         /* Perform peripheral init only once for an mmc device */
         mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
-       if (!mmc || mmc->block_dev.devnum != mmc_dev) {
+#if !CONFIG_IS_ENABLED(BLK)
+       block_dev = &mmc->block_dev;
+#else
+       block_dev = dev_get_uclass_plat(mmc->dev);
+#endif
+
+       if (!mmc || block_dev->devnum != mmc_dev) {
                 err = spl_mmc_find_device(&mmc, bootdev->boot_device);
                 if (err)
                         return err;

Note: Only build tested.

Cheers,
Quentin

Re: [PATCH] spl: mmc: Use correct MMC device when loading image

[Harald Seiler]

Hi,

On Mon, 2022-07-11 at 12:18 +0200, Quentin Schulz wrote:
> Hi Harald,
> 
> On 7/6/22 12:58, Harald Seiler wrote:
> > When attempting to load images from multiple MMC devices in sequence,
> > spl_mmc_load() chooses the wrong device from the second attempt onwards.
> > 
> > The reason is that MMC initialization is only done on its first call and
> > spl_mmc_load() will then continue using this same device for all future
> > calls.
> > 
> > Fix this by checking the devnum of the "cached" device struct against
> > the one which is requested.  If they match, use the cached one but if
> > they do not match, initialize the new device.
> > 
> > This fixes specifying multiple MMC devices in the SPL's boot order to
> > fall back when U-Boot Proper is corrupted or missing on the first
> > attempted MMC device.
> > 
> > Fixes: e1eb6ada4e38 ("spl: Make image loader infrastructure more universal")
> > Signed-off-by: Harald Seiler <hws@denx.de>
> > ---
> >   common/spl/spl_mmc.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> > index e1a7d25bd0..22c8a8097c 100644
> > --- a/common/spl/spl_mmc.c
> > +++ b/common/spl/spl_mmc.c
> > @@ -371,9 +371,11 @@ int spl_mmc_load(struct spl_image_info *spl_image,
> >   	u32 boot_mode;
> >   	int err = 0;
> >   	__maybe_unused int part = 0;
> > +	int mmc_dev;
> >   
> > -	/* Perform peripheral init only once */
> > -	if (!mmc) {
> > +	/* Perform peripheral init only once for an mmc device */
> > +	mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
> > +	if (!mmc || mmc->block_dev.devnum != mmc_dev) {
> 
> block_dev field only exists when CONFIG_BLK is not defined, c.f. 
> https://elixir.bootlin.com/u-boot/latest/source/include/mmc.h#L705
> 
> Considering the commit introducing this, c.f. 
> https://source.denx.de/u-boot/u-boot/-/commit/33fb211dd2706e666db4008801dc0d5903fd82f6, 
> I can suggest the following diff (which makes it compile with 
> rk3399-puma_defconfig):
> 
> diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> index e0e1a80536..c40b436a11 100644
> --- a/common/spl/spl_mmc.c
> +++ b/common/spl/spl_mmc.c
> @@ -409,10 +409,17 @@ int spl_mmc_load(struct spl_image_info *spl_image,
>          int err = 0;
>          __maybe_unused int part = 0;
>          int mmc_dev;
> +       struct blk_desc *block_dev;
> 
>          /* Perform peripheral init only once for an mmc device */
>          mmc_dev = spl_mmc_get_device_index(bootdev->boot_device);
> -       if (!mmc || mmc->block_dev.devnum != mmc_dev) {
> +#if !CONFIG_IS_ENABLED(BLK)
> +       block_dev = &mmc->block_dev;
> +#else
> +       block_dev = dev_get_uclass_plat(mmc->dev);
> +#endif
> +
> +       if (!mmc || block_dev->devnum != mmc_dev) {
>                  err = spl_mmc_find_device(&mmc, bootdev->boot_device);
>                  if (err)
>                          return err;
> 
> Note: Only build tested.

Thanks for reporting.  It's not quite that simple because `mmc` is NULL
on first call to spl_mmc_load().  I'll send a v2 to fix this.

-- 
Harald