diff mbox series

[3/5] env: sf: Preserve and free the previous flash

Message ID	20200514121145.28737-4-jagan@amarulasolutions.com
State	New
Headers	Received-SPF: pass (google.com: domain of jagan@amarulasolutions.com designates 209.85.220.65 as permitted sender) client-ip=209.85.220.65; From: Jagan Teki <jagan@amarulasolutions.com> To: u-boot@lists.denx.de Cc: linux-amarula@amarulasolutions.com, Jagan Teki <jagan@amarulasolutions.com>, Simon Glass <sjg@chromium.org>, Vignesh R <vigneshr@ti.com> Subject: [PATCH 3/5] env: sf: Preserve and free the previous flash Date: Thu, 14 May 2020 17:41:43 +0530 Message-Id: <20200514121145.28737-4-jagan@amarulasolutions.com> In-Reply-To: <20200514121145.28737-1-jagan@amarulasolutions.com> References: <20200514121145.28737-1-jagan@amarulasolutions.com> MIME-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Precedence: list Mailing-list: list linux-amarula@amarulasolutions.com; contact linux-amarula+owners@amarulasolutions.com
Series	sf: Cleanup \| [0/5] sf: Cleanup [1/5] mtd: spi: Call sst_write in _write ops [2/5] cmd: sf Drop reassignment of new into flash [3/5] env: sf: Preserve and free the previous flash [4/5] mtd: sf: Drop plat from sf_probe [5/5] mtd: spi: Use IS_ENABLED to prevent ifdef

Commit Message

Jagan Teki May 14, 2020, 12:11 p.m. UTC

env_flash is a global flash pointer, and the probe would
happen only if env_flash is NULL, but there is no checking
and free the pointer if is not NULL.

So, this patch frees the env_flash if it's not NULL, and
get the probed flash in new flash pointer and finally
assign into env_flash.

Note: Similar approach has been followed and tested in
cmd/sf.c

Cc: Simon Glass <sjg@chromium.org>
Cc: Vignesh R <vigneshr@ti.com>
Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 env/sf.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

Comments

'Abel Vesa' via Amarula Linux May 15, 2020, 7:24 a.m. UTC | #1

Hi Jagan,

On 14/05/20 05:41PM, Jagan Teki wrote:
> env_flash is a global flash pointer, and the probe would
> happen only if env_flash is NULL, but there is no checking
> and free the pointer if is not NULL.
> 
> So, this patch frees the env_flash if it's not NULL, and
> get the probed flash in new flash pointer and finally
> assign into env_flash.
> 
> Note: Similar approach has been followed and tested in
> cmd/sf.c
> 
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Vignesh R <vigneshr@ti.com>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>  env/sf.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/env/sf.c b/env/sf.c
> index 64c57f2cdf..af59c8375c 100644
> --- a/env/sf.c
> +++ b/env/sf.c
> @@ -50,15 +50,17 @@ static int setup_flash_device(void)
>  
>  	env_flash = dev_get_uclass_priv(new);
>  #else
> +	struct spi_flash *new;
>  
> -	if (!env_flash) {
> -		env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> -			CONFIG_ENV_SPI_CS,
> -			CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> -		if (!env_flash) {
> -			env_set_default("spi_flash_probe() failed", 0);
> -			return -EIO;
> -		}
> +	if (env_flash)
> +		spi_flash_free(env_flash);
> +
> +	new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,

Why not assign env_flash directly here? You do it in the very next 
statement anyway. I don't think the extra 'new' variable is needed.

> +			      CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> +	env_flash = new;
> +	if (!new) {
> +		env_set_default("spi_flash_probe() failed", 0);
> +		return -EIO;
>  	}
>  #endif
>  	return 0;
> -- 
> 2.20.1
>

Jagan Teki May 15, 2020, 7:44 a.m. UTC | #2

On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> Hi Jagan,
>
> On 14/05/20 05:41PM, Jagan Teki wrote:
> > env_flash is a global flash pointer, and the probe would
> > happen only if env_flash is NULL, but there is no checking
> > and free the pointer if is not NULL.
> >
> > So, this patch frees the env_flash if it's not NULL, and
> > get the probed flash in new flash pointer and finally
> > assign into env_flash.
> >
> > Note: Similar approach has been followed and tested in
> > cmd/sf.c
> >
> > Cc: Simon Glass <sjg@chromium.org>
> > Cc: Vignesh R <vigneshr@ti.com>
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >  env/sf.c | 18 ++++++++++--------
> >  1 file changed, 10 insertions(+), 8 deletions(-)
> >
> > diff --git a/env/sf.c b/env/sf.c
> > index 64c57f2cdf..af59c8375c 100644
> > --- a/env/sf.c
> > +++ b/env/sf.c
> > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> >
> >       env_flash = dev_get_uclass_priv(new);
> >  #else
> > +     struct spi_flash *new;
> >
> > -     if (!env_flash) {
> > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > -                     CONFIG_ENV_SPI_CS,
> > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > -             if (!env_flash) {
> > -                     env_set_default("spi_flash_probe() failed", 0);
> > -                     return -EIO;
> > -             }
> > +     if (env_flash)
> > +             spi_flash_free(env_flash);
> > +
> > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
>
> Why not assign env_flash directly here? You do it in the very next
> statement anyway. I don't think the extra 'new' variable is needed.

If flash pointer is used free it, before probing a new flash and
storing it in flash. In this scenario it requires local new pointer,
it was known observation we have faced in cmd/sf.c

Jagan.

'Abel Vesa' via Amarula Linux May 15, 2020, 8:49 a.m. UTC | #3

On 15/05/20 01:14PM, Jagan Teki wrote:
> On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > Hi Jagan,
> >
> > On 14/05/20 05:41PM, Jagan Teki wrote:
> > > env_flash is a global flash pointer, and the probe would
> > > happen only if env_flash is NULL, but there is no checking
> > > and free the pointer if is not NULL.
> > >
> > > So, this patch frees the env_flash if it's not NULL, and
> > > get the probed flash in new flash pointer and finally
> > > assign into env_flash.
> > >
> > > Note: Similar approach has been followed and tested in
> > > cmd/sf.c
> > >
> > > Cc: Simon Glass <sjg@chromium.org>
> > > Cc: Vignesh R <vigneshr@ti.com>
> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > ---
> > >  env/sf.c | 18 ++++++++++--------
> > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/env/sf.c b/env/sf.c
> > > index 64c57f2cdf..af59c8375c 100644
> > > --- a/env/sf.c
> > > +++ b/env/sf.c
> > > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> > >
> > >       env_flash = dev_get_uclass_priv(new);
> > >  #else
> > > +     struct spi_flash *new;
> > >
> > > -     if (!env_flash) {
> > > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > > -                     CONFIG_ENV_SPI_CS,
> > > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > > -             if (!env_flash) {
> > > -                     env_set_default("spi_flash_probe() failed", 0);
> > > -                     return -EIO;
> > > -             }
> > > +     if (env_flash)
> > > +             spi_flash_free(env_flash);
> > > +
> > > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
> >
> > Why not assign env_flash directly here? You do it in the very next
> > statement anyway. I don't think the extra 'new' variable is needed.
> 
> If flash pointer is used free it, before probing a new flash and
> storing it in flash.

Understood.

> In this scenario it requires local new pointer,
> it was known observation we have faced in cmd/sf.c
 
What difference does using the local pointer make though? We don't do 
anything with it. We just assign it to env_flash in the next statement. 
This is similar to doing:

  a = foo();
  b = a;
  if (a)
  	...

The below snippet is equivalent:

  b = foo();
  if (b)
  	...

We can get rid of 'a' this way.

Jagan Teki May 18, 2020, 12:06 p.m. UTC | #4

On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav <p.yadav@ti.com> wrote:
>
> On 15/05/20 01:14PM, Jagan Teki wrote:
> > On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> > >
> > > Hi Jagan,
> > >
> > > On 14/05/20 05:41PM, Jagan Teki wrote:
> > > > env_flash is a global flash pointer, and the probe would
> > > > happen only if env_flash is NULL, but there is no checking
> > > > and free the pointer if is not NULL.
> > > >
> > > > So, this patch frees the env_flash if it's not NULL, and
> > > > get the probed flash in new flash pointer and finally
> > > > assign into env_flash.
> > > >
> > > > Note: Similar approach has been followed and tested in
> > > > cmd/sf.c
> > > >
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Cc: Vignesh R <vigneshr@ti.com>
> > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > ---
> > > >  env/sf.c | 18 ++++++++++--------
> > > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/env/sf.c b/env/sf.c
> > > > index 64c57f2cdf..af59c8375c 100644
> > > > --- a/env/sf.c
> > > > +++ b/env/sf.c
> > > > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> > > >
> > > >       env_flash = dev_get_uclass_priv(new);
> > > >  #else
> > > > +     struct spi_flash *new;
> > > >
> > > > -     if (!env_flash) {
> > > > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > > > -                     CONFIG_ENV_SPI_CS,
> > > > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > > > -             if (!env_flash) {
> > > > -                     env_set_default("spi_flash_probe() failed", 0);
> > > > -                     return -EIO;
> > > > -             }
> > > > +     if (env_flash)
> > > > +             spi_flash_free(env_flash);
> > > > +
> > > > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
> > >
> > > Why not assign env_flash directly here? You do it in the very next
> > > statement anyway. I don't think the extra 'new' variable is needed.
> >
> > If flash pointer is used free it, before probing a new flash and
> > storing it in flash.
>
> Understood.
>
> > In this scenario it requires local new pointer,
> > it was known observation we have faced in cmd/sf.c
>
> What difference does using the local pointer make though? We don't do
> anything with it. We just assign it to env_flash in the next statement.
> This is similar to doing:
>
>   a = foo();
>   b = a;
>   if (a)
>         ...
>
> The below snippet is equivalent:
>
>   b = foo();
>   if (b)
>         ...
>
> We can get rid of 'a' this way.

The scenario here, seems different here we need to take care of free
the old pointer and update new pointer with a global pointer like
below.

if (b)
    free(b);
a = foo();
b = a;
if (!a)
   fail;

So, you mean to say as below?

if (b)
    free(b);
b = foo();
if (!b)
     fail;

Jagan.

'Abel Vesa' via Amarula Linux May 18, 2020, 3:41 p.m. UTC | #5

On 18/05/20 05:36PM, Jagan Teki wrote:
> On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> >
> > On 15/05/20 01:14PM, Jagan Teki wrote:
> > > On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav@ti.com> wrote:
> > > >
> > > > Hi Jagan,
> > > >
> > > > On 14/05/20 05:41PM, Jagan Teki wrote:
> > > > > env_flash is a global flash pointer, and the probe would
> > > > > happen only if env_flash is NULL, but there is no checking
> > > > > and free the pointer if is not NULL.
> > > > >
> > > > > So, this patch frees the env_flash if it's not NULL, and
> > > > > get the probed flash in new flash pointer and finally
> > > > > assign into env_flash.
> > > > >
> > > > > Note: Similar approach has been followed and tested in
> > > > > cmd/sf.c
> > > > >
> > > > > Cc: Simon Glass <sjg@chromium.org>
> > > > > Cc: Vignesh R <vigneshr@ti.com>
> > > > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > > > > ---
> > > > >  env/sf.c | 18 ++++++++++--------
> > > > >  1 file changed, 10 insertions(+), 8 deletions(-)
> > > > >
> > > > > diff --git a/env/sf.c b/env/sf.c
> > > > > index 64c57f2cdf..af59c8375c 100644
> > > > > --- a/env/sf.c
> > > > > +++ b/env/sf.c
> > > > > @@ -50,15 +50,17 @@ static int setup_flash_device(void)
> > > > >
> > > > >       env_flash = dev_get_uclass_priv(new);
> > > > >  #else
> > > > > +     struct spi_flash *new;
> > > > >
> > > > > -     if (!env_flash) {
> > > > > -             env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
> > > > > -                     CONFIG_ENV_SPI_CS,
> > > > > -                     CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
> > > > > -             if (!env_flash) {
> > > > > -                     env_set_default("spi_flash_probe() failed", 0);
> > > > > -                     return -EIO;
> > > > > -             }
> > > > > +     if (env_flash)
> > > > > +             spi_flash_free(env_flash);
> > > > > +
> > > > > +     new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
> > > >
> > > > Why not assign env_flash directly here? You do it in the very next
> > > > statement anyway. I don't think the extra 'new' variable is needed.
> > >
> > > If flash pointer is used free it, before probing a new flash and
> > > storing it in flash.
> >
> > Understood.
> >
> > > In this scenario it requires local new pointer,
> > > it was known observation we have faced in cmd/sf.c
> >
> > What difference does using the local pointer make though? We don't do
> > anything with it. We just assign it to env_flash in the next statement.
> > This is similar to doing:
> >
> >   a = foo();
> >   b = a;
> >   if (a)
> >         ...
> >
> > The below snippet is equivalent:
> >
> >   b = foo();
> >   if (b)
> >         ...
> >
> > We can get rid of 'a' this way.
> 
> The scenario here, seems different here we need to take care of free
> the old pointer and update new pointer with a global pointer like
> below.
> 
> if (b)
>     free(b);
> a = foo();
> b = a;
> if (!a)
>    fail;
> 
> So, you mean to say as below?
> 
> if (b)
>     free(b);
> b = foo();
> if (!b)
>      fail;

Yes, I mean this. Since the pointer b is freed/unassigned, the old 
pointer value is of no use to us. Getting rid of a (or new in case of 
this patch) makes the code a bit cleaner.

diff mbox series

Patch

diff --git a/env/sf.c b/env/sf.c
index 64c57f2cdf..af59c8375c 100644
--- a/env/sf.c
+++ b/env/sf.c
@@ -50,15 +50,17 @@  static int setup_flash_device(void)
 
 	env_flash = dev_get_uclass_priv(new);
 #else
+	struct spi_flash *new;
 
-	if (!env_flash) {
-		env_flash = spi_flash_probe(CONFIG_ENV_SPI_BUS,
-			CONFIG_ENV_SPI_CS,
-			CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
-		if (!env_flash) {
-			env_set_default("spi_flash_probe() failed", 0);
-			return -EIO;
-		}
+	if (env_flash)
+		spi_flash_free(env_flash);
+
+	new = spi_flash_probe(CONFIG_ENV_SPI_BUS, CONFIG_ENV_SPI_CS,
+			      CONFIG_ENV_SPI_MAX_HZ, CONFIG_ENV_SPI_MODE);
+	env_flash = new;
+	if (!new) {
+		env_set_default("spi_flash_probe() failed", 0);
+		return -EIO;
 	}
 #endif
 	return 0;