diff mbox

[U-Boot] mmc:fix Call mmc_init() when executing mmc_get_dev()

Message ID 1333463043-11495-1-git-send-email-l.majewski@samsung.com
State Superseded, archived
Delegated to: Andy Fleming
Headers show

Commit Message

Łukasz Majewski April 3, 2012, 2:24 p.m. UTC
This code adds call to mmc_init(), for partition related commands (e.g.
fatls, fatinfo etc.).

It is safe to call mmc_init() multiple times since mmc->has_init flag
prevents from multiple initialization.

The FAT related code calls get_dev high level method and then uses
elements from mmc->block_dev, which is uninitialized until the mmc_init
(and thereof mmc_startup) is called.

This problem appears on boards, which don't use mmc as the default
place for envs

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Cc: Andy Fleming <afleming@gmail.com>

---
Test HW:
- GONI S5PC110
- Universal C210 (Exynos4)
---
 drivers/mmc/mmc.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

Comments

Minkyu Kang April 18, 2012, 2:15 a.m. UTC | #1
Hi Lukasz,

On 3 April 2012 23:24, Lukasz Majewski <l.majewski@samsung.com> wrote:
> This code adds call to mmc_init(), for partition related commands (e.g.
> fatls, fatinfo etc.).
>
> It is safe to call mmc_init() multiple times since mmc->has_init flag
> prevents from multiple initialization.
>
> The FAT related code calls get_dev high level method and then uses
> elements from mmc->block_dev, which is uninitialized until the mmc_init
> (and thereof mmc_startup) is called.
>
> This problem appears on boards, which don't use mmc as the default
> place for envs
>
> Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Cc: Andy Fleming <afleming@gmail.com>
>
> ---
> Test HW:
> - GONI S5PC110
> - Universal C210 (Exynos4)
> ---
>  drivers/mmc/mmc.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> index 618960e..1fa90e7 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -1305,8 +1305,12 @@ int mmc_register(struct mmc *mmc)
>  block_dev_desc_t *mmc_get_dev(int dev)
>  {
>        struct mmc *mmc = find_mmc_device(dev);
> +       if (mmc) {
> +               mmc_init(mmc);
> +               return &mmc->block_dev;
> +       }
>
> -       return mmc ? &mmc->block_dev : NULL;
> +       return NULL;
>  }
>  #endif
>

I think

	if (!mmc)
		return NULL;

	mmc_init(mmc);
	return &mmc->block_dev;

is better.
How you think?

Thanks.
Minkyu Kang.
Łukasz Majewski April 18, 2012, 7:28 a.m. UTC | #2
Hi, Minkyu

> Hi Lukasz,
> 
> On 3 April 2012 23:24, Lukasz Majewski <l.majewski@samsung.com> wrote:
> > This code adds call to mmc_init(), for partition related commands
> > (e.g. fatls, fatinfo etc.).
> >
> > It is safe to call mmc_init() multiple times since mmc->has_init
> > flag prevents from multiple initialization.
> >
> > The FAT related code calls get_dev high level method and then uses
> > elements from mmc->block_dev, which is uninitialized until the
> > mmc_init (and thereof mmc_startup) is called.
> >
> > This problem appears on boards, which don't use mmc as the default
> > place for envs
> >
> > Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Cc: Andy Fleming <afleming@gmail.com>
> >
> > ---
> > Test HW:
> > - GONI S5PC110
> > - Universal C210 (Exynos4)
> > ---
> >  drivers/mmc/mmc.c |    6 +++++-
> >  1 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
> > index 618960e..1fa90e7 100644
> > --- a/drivers/mmc/mmc.c
> > +++ b/drivers/mmc/mmc.c
> > @@ -1305,8 +1305,12 @@ int mmc_register(struct mmc *mmc)
> >  block_dev_desc_t *mmc_get_dev(int dev)
> >  {
> >        struct mmc *mmc = find_mmc_device(dev);
> > +       if (mmc) {
> > +               mmc_init(mmc);
> > +               return &mmc->block_dev;
> > +       }
> >
> > -       return mmc ? &mmc->block_dev : NULL;
> > +       return NULL;
> >  }
> >  #endif
> >
> 
> I think
> 
> 	if (!mmc)
> 		return NULL;
> 
> 	mmc_init(mmc);
> 	return &mmc->block_dev;
> 
> is better.
> How you think?

Yes, it seems also more readable for me.
But anyway Andy shall express his opinion.
diff mbox

Patch

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 618960e..1fa90e7 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -1305,8 +1305,12 @@  int mmc_register(struct mmc *mmc)
 block_dev_desc_t *mmc_get_dev(int dev)
 {
 	struct mmc *mmc = find_mmc_device(dev);
+	if (mmc) {
+		mmc_init(mmc);
+		return &mmc->block_dev;
+	}
 
-	return mmc ? &mmc->block_dev : NULL;
+	return NULL;
 }
 #endif