[U-Boot] crash when doing `sf probe` multiple times due to API miscommunication
the `sf probe` command does: static int do_spi_flash_probe(...) { ... new = spi_flash_probe(bus, cs, speed, mode); if (flash) spi_flash_free(flash); flash = new; ... }
looks good ... if the user ran `sf probe` once already, then we need to free that structure ...
however, let's take a look at spi_flash_probe ... struct spi_flash *spi_flash_probe(...) { ... case 0x01: flash = spi_flash_probe_atmel(spi, idcode); ... case 0x1F: flash = spi_flash_probe_spansion(spi, idcode); ... case 0x20: flash = spi_flash_probe_stmicro(spi, idcode); ... return flash; }
and so we descend another level ... struct spi_flash *spi_flash_probe_stmicro(...) { ... stm = malloc(sizeof(struct stmicro_spi_flash)); ... return &stm->flash; } struct spi_flash *spi_flash_probe_atmel(...) { ... asf = malloc(sizeof(struct atmel_spi_flash)); ... return &asf->flash; }
clearly this isnt lining up. the `sf` command expects to be given back malloced memory, not a pointer to the middle of a malloc. so calling free() on the pointer returned is invalid (and in my case, crashes the board most of the time). -mike
On Thursday 11 December 2008 15:52:25 Mike Frysinger wrote:
the `sf probe` command does: static int do_spi_flash_probe(...) { ... new = spi_flash_probe(bus, cs, speed, mode); if (flash) spi_flash_free(flash); flash = new; ... }
oh, i left out one last function ... void spi_flash_free(struct spi_flash *flash) { spi_free_slave(flash->spi); free(flash); } and we see the wrong "free(flash)" ...
maybe if the spi_flash structure provided a pointer to its parent, then we could do "free(flash->parent)" ... -mike
From: Brad Bozarth bflinux@yumbrad.com
Higher spi flash layers expect to be given back a pointer that was malloced so that it can free the result, but the lower layers return a pointer that is in the middle of the malloced memory. Reorder the members of the lower spi structures so that things work out.
Signed-off-by: Brad Bozarth bflinux@yumbrad.com Signed-off-by: Mike Frysinger vapier@gentoo.org CC: Haavard Skinnemoen haavard.skinnemoen@atmel.com --- Jason McMullan: your winbond driver will need a similar change: drivers/mtd/spi/winbond.c +/* spi_flash needs to be first so upper layers can free() it */ struct winbond_spi_flash { - const struct winbond_spi_flash_params *params; struct spi_flash flash; + const struct winbond_spi_flash_params *params; };
drivers/mtd/spi/atmel.c | 3 ++- drivers/mtd/spi/stmicro.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/mtd/spi/atmel.c b/drivers/mtd/spi/atmel.c index 10fcf0c..a5f51ca 100644 --- a/drivers/mtd/spi/atmel.c +++ b/drivers/mtd/spi/atmel.c @@ -39,9 +39,10 @@ struct atmel_spi_flash_params { const char *name; };
+/* spi_flash needs to be first so upper layers can free() it */ struct atmel_spi_flash { - const struct atmel_spi_flash_params *params; struct spi_flash flash; + const struct atmel_spi_flash_params *params; };
static inline struct atmel_spi_flash * diff --git a/drivers/mtd/spi/stmicro.c b/drivers/mtd/spi/stmicro.c index 86324e4..e7dda91 100644 --- a/drivers/mtd/spi/stmicro.c +++ b/drivers/mtd/spi/stmicro.c @@ -64,9 +64,10 @@ struct stmicro_spi_flash_params { const char *name; };
+/* spi_flash needs to be first so upper layers can free() it */ struct stmicro_spi_flash { - const struct stmicro_spi_flash_params *params; struct spi_flash flash; + const struct stmicro_spi_flash_params *params; };
static inline struct stmicro_spi_flash *to_stmicro_spi_flash(struct spi_flash
Mike Frysinger wrote:
From: Brad Bozarth bflinux@yumbrad.com
Higher spi flash layers expect to be given back a pointer that was malloced so that it can free the result, but the lower layers return a pointer that is in the middle of the malloced memory. Reorder the members of the lower spi structures so that things work out.
Signed-off-by: Brad Bozarth bflinux@yumbrad.com Signed-off-by: Mike Frysinger vapier@gentoo.org CC: Haavard Skinnemoen haavard.skinnemoen@atmel.com
Acked-by: Haavard Skinnemoen haavard.skinnemoen@atmel.com
On Monday 05 January 2009 03:17:00 Haavard Skinnemoen wrote:
Mike Frysinger wrote:
From: Brad Bozarth bflinux@yumbrad.com
Higher spi flash layers expect to be given back a pointer that was malloced so that it can free the result, but the lower layers return a pointer that is in the middle of the malloced memory. Reorder the members of the lower spi structures so that things work out.
Signed-off-by: Brad Bozarth bflinux@yumbrad.com Signed-off-by: Mike Frysinger vapier@gentoo.org CC: Haavard Skinnemoen haavard.skinnemoen@atmel.com
Acked-by: Haavard Skinnemoen haavard.skinnemoen@atmel.com
Wolfgang: this should be in for the next release ... -mike
Dear Mike Frysinger,
In message 1230867947-17100-1-git-send-email-vapier@gentoo.org you wrote:
From: Brad Bozarth bflinux@yumbrad.com
Higher spi flash layers expect to be given back a pointer that was malloced so that it can free the result, but the lower layers return a pointer that is in the middle of the malloced memory. Reorder the members of the lower spi structures so that things work out.
Signed-off-by: Brad Bozarth bflinux@yumbrad.com Signed-off-by: Mike Frysinger vapier@gentoo.org CC: Haavard Skinnemoen haavard.skinnemoen@atmel.com
Jason McMullan: your winbond driver will need a similar change: drivers/mtd/spi/winbond.c +/* spi_flash needs to be first so upper layers can free() it */ struct winbond_spi_flash {
- const struct winbond_spi_flash_params *params; struct spi_flash flash;
- const struct winbond_spi_flash_params *params;
};drivers/mtd/spi/atmel.c | 3 ++- drivers/mtd/spi/stmicro.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-)
Applied, thanks.
Best regards,
Wolfgang Denk
participants (3)
-
Haavard Skinnemoen -
Mike Frysinger -
Wolfgang Denk