| Message ID | 1445515280-21389-3-git-send-email-nikita@compulab.co.il |
|---|---|
| State | Superseded |
| Delegated to: | Pantelis Antoniou |
| Headers | show |
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. > } > + > + hang(); > } > Regards, Hans
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 >
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 >>
Hi, On Thu, Oct 22, 2015 at 03:23:21PM +0200, Hans de Goede wrote: > 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: > >>> 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. Right. OK, noted for V2. > > Otherwise you have: > > hang(); > } > hang(); > > Which is just ugly code IMHO. > > Regards, > > Hans > > > > > >>> } > >>>+ > >>>+ hang(); > >>> } > >>> > >> > >>Regards, > >> > >>Hans > >> >
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(); } + + hang(); }
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(-)