From mboxrd@z Thu Jan  1 00:00:00 1970
From: Nikita Kiryanov <nikita@compulab.co.il>
Date: Thu, 22 Oct 2015 16:08:29 +0300
Subject: [U-Boot] [PATCH 02/12] spl: mmc: add break statements in
 spl_mmc_load_image()
In-Reply-To: <5628D877.6080403@redhat.com>
References: <1445515280-21389-1-git-send-email-nikita@compulab.co.il>
 <1445515280-21389-3-git-send-email-nikita@compulab.co.il>
 <5628D877.6080403@redhat.com>
Message-ID: <20151022130829.GA27303@skynet>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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@compulab.co.il>
> >Cc: Igor Grinberg <grinberg@compulab.co.il>
> >Cc: Paul Kocialkowski <contact@paulk.fr>
> >Cc: Pantelis Antoniou <panto@antoniou-consulting.com>
> >Cc: Tom Rini <trini@konsulko.com>
> >Cc: Simon Glass <sjg@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
>