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.