[U-Boot] [PATCH 02/12] spl: mmc: add break statements in spl_mmc_load_image()
Hans de Goede
hdegoede at redhat.com
Thu Oct 22 15:23:21 CEST 2015
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 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).
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
>>
More information about the U-Boot
mailing list