From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 22 Oct 2015 15:23:21 +0200
Subject: [U-Boot] [PATCH 02/12] spl: mmc: add break statements in
 spl_mmc_load_image()
In-Reply-To: <20151022130829.GA27303@skynet>
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> <20151022130829.GA27303@skynet>
Message-ID: <5628E349.6040009@redhat.com>
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

Hi,

On 22-10-15 15:08, Nikita Kiryanov wrote:
> 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).

Easy to fix, put the default: in the #ifdef too.

Otherwise you have:

		hang();
	}
	hang();

Which is just ugly code IMHO.

Regards,

Hans

	
>
>>>   	}
>>> +
>>> +	hang();
>>>   }
>>>
>>
>> Regards,
>>
>> Hans
>>