| Message ID | 1426749501-6251-2-git-send-email-cyril.bur@au1.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
On Thu, 2015-03-19 at 18:18 +1100, Cyril Bur wrote: > During init libflash calls low level functions without checking. > > libflash states to backends that if they implement all the higher level > functions the lower level functions are optional (from libflash-priv.h): > If all functions of the high level interface are > implemented then the low level one is optional. A > controller can implement some of the high level one > in which case the missing ones will be handled by > libflash using the low level interface. > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> I think you've missed fl_wpage. You might also want to change flash_configure: it currently checks ct->cmd_wr before calling flash_set_4b, which would be redundant with this patch. Regards, Daniel > --- > libflash/libflash.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/libflash/libflash.c b/libflash/libflash.c > index a229668..ce6a899 100644 > --- a/libflash/libflash.c > +++ b/libflash/libflash.c > @@ -91,6 +91,10 @@ int fl_wren(struct spi_flash_ctrl *ct) > int i, rc; > uint8_t stat; > > + /* If lower level interface not implmented, just return */ > + if (!ct->cmd_wr) > + return 0; > + > /* Some flashes need it to be hammered */ > for (i = 0; i < 1000; i++) { > rc = ct->cmd_wr(ct, CMD_WREN, false, 0, NULL, 0); > @@ -674,6 +678,11 @@ static int flash_set_4b(struct flash_chip *c, bool enable) > /* Ignore the error & move on (could be wrprotect chip) */ > } > > + /* Don't have low level interface, assume all is well */ > + if (!ct->cmd_wr) > + return 0; > + > + > /* Ignore error in case chip is write protected */ > > return ct->cmd_wr(ct, enable ? CMD_EN4B : CMD_EX4B, false, 0, NULL, 0); > @@ -757,7 +766,6 @@ static int flash_configure(struct flash_chip *c) > return rc; > } > } > - > /* Set controller to 3b mode if mode switch is supported */ > if (ct->set_4b) { > FL_DBG("LIBFLASH: Disabling controller 4B mode...\n");
On Thu, 2015-03-26 at 11:44 +1100, Daniel Axtens wrote: > On Thu, 2015-03-19 at 18:18 +1100, Cyril Bur wrote: > > During init libflash calls low level functions without checking. > > > > libflash states to backends that if they implement all the higher level > > functions the lower level functions are optional (from libflash-priv.h): > > If all functions of the high level interface are > > implemented then the low level one is optional. A > > controller can implement some of the high level one > > in which case the missing ones will be handled by > > libflash using the low level interface. > > > > Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> > > > I think you've missed fl_wpage. > It's not clear to me if fl_wpage() needs it, it is only called from flash_write() which has already checked to see if it can use the high level interface and also done: if (!ct->cmd_wr) return FLASH_ERR_CTRL_CMD_UNSUPPORTED; before it calls fl_wpage(). > You might also want to change flash_configure: it currently checks > ct->cmd_wr before calling flash_set_4b, which would be redundant with > this patch. Actually better yet, I can remove both the checks and add one to the start of flash_set_4b() which should cover everything. Just to explain a bit, I want this patch to be as least invasive as possible because my backend isn't flash at all (a rewrite of libflash/libffs is underway to address this problem) and its possible that there are reasons why these codepathes exist, and 'normal' backends might want them, I don't want to start simply returning (success or failure) if the low levels don't exist, it could start masking bigger issues. I just need libflash to not segfault when it's used with the file backend :). In a perfect world once there is a better way of using libffs with files this patch should probably be reverted and the problem re-examined. I'll resend updated version. Thanks for review, Cyril > Regards, > Daniel > > > --- > > libflash/libflash.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/libflash/libflash.c b/libflash/libflash.c > > index a229668..ce6a899 100644 > > --- a/libflash/libflash.c > > +++ b/libflash/libflash.c > > @@ -91,6 +91,10 @@ int fl_wren(struct spi_flash_ctrl *ct) > > int i, rc; > > uint8_t stat; > > > > + /* If lower level interface not implmented, just return */ > > + if (!ct->cmd_wr) > > + return 0; > > + > > /* Some flashes need it to be hammered */ > > for (i = 0; i < 1000; i++) { > > rc = ct->cmd_wr(ct, CMD_WREN, false, 0, NULL, 0); > > @@ -674,6 +678,11 @@ static int flash_set_4b(struct flash_chip *c, bool enable) > > /* Ignore the error & move on (could be wrprotect chip) */ > > } > > > > + /* Don't have low level interface, assume all is well */ > > + if (!ct->cmd_wr) > > + return 0; > > + > > + > > /* Ignore error in case chip is write protected */ > > > > return ct->cmd_wr(ct, enable ? CMD_EN4B : CMD_EX4B, false, 0, NULL, 0); > > @@ -757,7 +766,6 @@ static int flash_configure(struct flash_chip *c) > > return rc; > > } > > } > > - > > /* Set controller to 3b mode if mode switch is supported */ > > if (ct->set_4b) { > > FL_DBG("LIBFLASH: Disabling controller 4B mode...\n"); >
diff --git a/libflash/libflash.c b/libflash/libflash.c index a229668..ce6a899 100644 --- a/libflash/libflash.c +++ b/libflash/libflash.c @@ -91,6 +91,10 @@ int fl_wren(struct spi_flash_ctrl *ct) int i, rc; uint8_t stat; + /* If lower level interface not implmented, just return */ + if (!ct->cmd_wr) + return 0; + /* Some flashes need it to be hammered */ for (i = 0; i < 1000; i++) { rc = ct->cmd_wr(ct, CMD_WREN, false, 0, NULL, 0); @@ -674,6 +678,11 @@ static int flash_set_4b(struct flash_chip *c, bool enable) /* Ignore the error & move on (could be wrprotect chip) */ } + /* Don't have low level interface, assume all is well */ + if (!ct->cmd_wr) + return 0; + + /* Ignore error in case chip is write protected */ return ct->cmd_wr(ct, enable ? CMD_EN4B : CMD_EX4B, false, 0, NULL, 0); @@ -757,7 +766,6 @@ static int flash_configure(struct flash_chip *c) return rc; } } - /* Set controller to 3b mode if mode switch is supported */ if (ct->set_4b) { FL_DBG("LIBFLASH: Disabling controller 4B mode...\n");
During init libflash calls low level functions without checking. libflash states to backends that if they implement all the higher level functions the lower level functions are optional (from libflash-priv.h): If all functions of the high level interface are implemented then the low level one is optional. A controller can implement some of the high level one in which case the missing ones will be handled by libflash using the low level interface. Signed-off-by: Cyril Bur <cyril.bur@au1.ibm.com> --- libflash/libflash.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)