[U-Boot] [PATCH 02/12] spl: mmc: add break statements in spl_mmc_load_image()
Nikita Kiryanov
nikita at compulab.co.il
Thu Oct 22 15:08:29 CEST 2015
On Thu, Oct 22, 2015 at 02:37:11PM +0200, Hans de Goede wrote:
> Hi,
>
> On 22-10-15 14:01, Nikita Kiryanov wrote:
> >The original intention of the mmc load_image() function was to try multiple
> >boot modes before failing. This is evident by the lack of break statements
> >in the switch, and the following line in the default case:
> >puts("spl: mmc: no boot mode left to try\n");
> >
> >This implementation is problematic because:
> >- The availability of alternative boot modes is very arbitrary since it
> >depends on the specific order of the switch cases. If your boot mode happens to
> >be the first case, then you'll have a bunch of other boot modes as alternatives.
> >If it happens to be the last case, then you have none.
> >- Opting in/out is tied to config options, so the only way for you to prevent an
> >alternative boot mode from being attempted is to give up on the feature completely.
> >- This implementation makes the code more complicated and difficult to
> >understand.
> >
> >Address these issues by inserting a break statements between the cases to make the
> >function try only one boot mode.
> >
> >Signed-off-by: Nikita Kiryanov <nikita at compulab.co.il>
> >Cc: Igor Grinberg <grinberg at compulab.co.il>
> >Cc: Paul Kocialkowski <contact at paulk.fr>
> >Cc: Pantelis Antoniou <panto at antoniou-consulting.com>
> >Cc: Tom Rini <trini at konsulko.com>
> >Cc: Simon Glass <sjg at chromium.org>
> >---
> > common/spl/spl_mmc.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> >diff --git a/common/spl/spl_mmc.c b/common/spl/spl_mmc.c
> >index ce58c58..e831970 100644
> >--- a/common/spl/spl_mmc.c
> >+++ b/common/spl/spl_mmc.c
> >@@ -164,6 +164,7 @@ void spl_mmc_load_image(void)
> > if (!err)
> > return;
> > #endif
> >+ break;
> > case MMCSD_MODE_FS:
> > debug("spl: mmc boot mode: fs\n");
> >
> >@@ -203,6 +204,7 @@ void spl_mmc_load_image(void)
> > #endif
> > #endif
> > #endif
> >+ break;
> > #ifdef CONFIG_SUPPORT_EMMC_BOOT
> > case MMCSD_MODE_EMMCBOOT:
> > /*
> >@@ -240,15 +242,15 @@ void spl_mmc_load_image(void)
> > if (!err)
> > return;
> > #endif
> >+ break;
> > #endif
> > case MMCSD_MODE_UNDEFINED:
> > default:
> > #ifdef CONFIG_SPL_LIBCOMMON_SUPPORT
> >- if (err)
> >- puts("spl: mmc: no boot mode left to try\n");
> >- else
> >- puts("spl: mmc: wrong boot mode\n");
> >+ puts("spl: mmc: wrong boot mode\n");
> > #endif
> > hang();
>
> The above hang() can be removed now.
>
Not true. If CONFIG_SPL_LIBCOMMON_SUPPORT is not defined this part will
be just:
default:
}
Which is a compile error (label at the end of compound statement).
> > }
> >+
> >+ hang();
> > }
> >
>
> Regards,
>
> Hans
>
More information about the U-Boot
mailing list