| Message ID | 20200514121145.28737-4-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series | sf: Cleanup | expand |
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 at chromium.org> > Cc: Vignesh R <vigneshr at ti.com> > Signed-off-by: Jagan Teki <jagan at 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 >
On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav at 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 at chromium.org> > > Cc: Vignesh R <vigneshr at ti.com> > > Signed-off-by: Jagan Teki <jagan at 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.
On 15/05/20 01:14PM, Jagan Teki wrote: > On Fri, May 15, 2020 at 12:54 PM Pratyush Yadav <p.yadav at 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 at chromium.org> > > > Cc: Vignesh R <vigneshr at ti.com> > > > Signed-off-by: Jagan Teki <jagan at 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.
On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav <p.yadav at 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 at 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 at chromium.org> > > > > Cc: Vignesh R <vigneshr at ti.com> > > > > Signed-off-by: Jagan Teki <jagan at 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.
On 18/05/20 05:36PM, Jagan Teki wrote: > On Fri, May 15, 2020 at 2:19 PM Pratyush Yadav <p.yadav at 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 at 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 at chromium.org> > > > > > Cc: Vignesh R <vigneshr at ti.com> > > > > > Signed-off-by: Jagan Teki <jagan at 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 --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;
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 at chromium.org> Cc: Vignesh R <vigneshr at ti.com> Signed-off-by: Jagan Teki <jagan at amarulasolutions.com> --- env/sf.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)