| Message ID | 20230201113603.293758-4-miquel.raynal@bootlin.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: read while write support | expand |
On 2/1/23 11:35, Miquel Raynal wrote: > The ->prepare()/->unprepare() hooks are now legacy, we no longer accept > new drivers supporting them. The only remaining controllers using them > acquires a per-chip mutex, which should not interfere with the rest of > the operation done in the core. As a result, we should be safe to > reorganize these helpers to first perform the preparation, before > acquiring the core locks. This is necessary in order to be able to > improve the locking mechanism in the core (coming next). No side effects > are expected. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > drivers/mtd/spi-nor/core.c | 23 ++++++++++------------- > 1 file changed, 10 insertions(+), 13 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 3845de9c874c..01932dbaa5b9 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1071,27 +1071,24 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor) > } > } > > -int spi_nor_lock_and_prep(struct spi_nor *nor) > +int spi_nor_prep_and_lock(struct spi_nor *nor) > { > int ret = 0; > > - mutex_lock(&nor->lock); > - > - if (nor->controller_ops && nor->controller_ops->prepare) { > + if (nor->controller_ops && nor->controller_ops->prepare) > ret = nor->controller_ops->prepare(nor); > - if (ret) { > - mutex_unlock(&nor->lock); > - return ret; > - } > - } > + > + mutex_lock(&nor->lock); > + > return ret; > } > > void spi_nor_unlock_and_unprep(struct spi_nor *nor) > { > + mutex_unlock(&nor->lock); > + > if (nor->controller_ops && nor->controller_ops->unprepare) > nor->controller_ops->unprepare(nor); > - mutex_unlock(&nor->lock); > } > > static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr) > @@ -1447,7 +1444,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) > addr = instr->addr; > len = instr->len; > > - ret = spi_nor_lock_and_prep(nor); > + ret = spi_nor_prep_and_lock(nor); > if (ret) > return ret; > > @@ -1707,7 +1704,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, > > dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); > > - ret = spi_nor_lock_and_prep(nor); > + ret = spi_nor_prep_and_lock(nor); > if (ret) > return ret; > > @@ -1753,7 +1750,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, > > dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); > > - ret = spi_nor_lock_and_prep(nor); > + ret = spi_nor_prep_and_lock(nor); > if (ret) > return ret; >
On 3/17/23 03:39, Tudor Ambarus wrote: > > > On 2/1/23 11:35, Miquel Raynal wrote: >> The ->prepare()/->unprepare() hooks are now legacy, we no longer accept >> new drivers supporting them. The only remaining controllers using them >> acquires a per-chip mutex, which should not interfere with the rest of >> the operation done in the core. As a result, we should be safe to >> reorganize these helpers to first perform the preparation, before >> acquiring the core locks. This is necessary in order to be able to >> improve the locking mechanism in the core (coming next). No side effects >> are expected. >> >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org> No, I take back my R-b. The reorder is fine, but you missed to update otp.c and swp.c, see below. > >> --- >> drivers/mtd/spi-nor/core.c | 23 ++++++++++------------- >> 1 file changed, 10 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c >> index 3845de9c874c..01932dbaa5b9 100644 >> --- a/drivers/mtd/spi-nor/core.c >> +++ b/drivers/mtd/spi-nor/core.c >> @@ -1071,27 +1071,24 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor) >> } >> } >> >> -int spi_nor_lock_and_prep(struct spi_nor *nor) >> +int spi_nor_prep_and_lock(struct spi_nor *nor) $ git grep spi_nor_lock_and_prep drivers/mtd/spi-nor/core.h:int spi_nor_lock_and_prep(struct spi_nor *nor); drivers/mtd/spi-nor/otp.c: ret = spi_nor_lock_and_prep(nor); drivers/mtd/spi-nor/otp.c: ret = spi_nor_lock_and_prep(nor); drivers/mtd/spi-nor/otp.c: ret = spi_nor_lock_and_prep(nor); drivers/mtd/spi-nor/otp.c: ret = spi_nor_lock_and_prep(nor); drivers/mtd/spi-nor/sst.c: ret = spi_nor_lock_and_prep(nor); drivers/mtd/spi-nor/swp.c: ret = spi_nor_lock_and_prep(nor); drivers/mtd/spi-nor/swp.c: ret = spi_nor_lock_and_prep(nor); drivers/mtd/spi-nor/swp.c: ret = spi_nor_lock_and_prep(nor); Please make sure you try a build after each patch, you'll affect bisects: arm-linux-gnueabi-ld: drivers/mtd/spi-nor/swp.o: in function `spi_nor_unlock': swp.c:(.text+0x6d0): undefined reference to `spi_nor_lock_and_prep' arm-linux-gnueabi-ld: drivers/mtd/spi-nor/swp.o: in function `spi_nor_is_locked': swp.c:(.text+0x72c): undefined reference to `spi_nor_lock_and_prep' arm-linux-gnueabi-ld: drivers/mtd/spi-nor/swp.o: in function `spi_nor_lock': swp.c:(.text+0x788): undefined reference to `spi_nor_lock_and_prep' arm-linux-gnueabi-ld: drivers/mtd/spi-nor/swp.o: in function `spi_nor_try_unlock_all': swp.c:(.text+0x804): undefined reference to `spi_nor_lock_and_prep' arm-linux-gnueabi-ld: drivers/mtd/spi-nor/otp.o: in function `spi_nor_mtd_otp_info': otp.c:(.text+0x30): undefined reference to `spi_nor_lock_and_prep' arm-linux-gnueabi-ld: drivers/mtd/spi-nor/otp.o:otp.c:(.text+0x138): more undefined references to `spi_nor_lock_and_prep' follow make[1]: *** [scripts/Makefile.vmlinux:34: vmlinux] Error 1 make: *** [Makefile:1252: vmlinux] Error 2 >> { >> int ret = 0; >> >> - mutex_lock(&nor->lock); >> - >> - if (nor->controller_ops && nor->controller_ops->prepare) { >> + if (nor->controller_ops && nor->controller_ops->prepare) >> ret = nor->controller_ops->prepare(nor); >> - if (ret) { >> - mutex_unlock(&nor->lock); >> - return ret; >> - } >> - } >> + >> + mutex_lock(&nor->lock); >> + >> return ret; >> } >> >> void spi_nor_unlock_and_unprep(struct spi_nor *nor) >> { >> + mutex_unlock(&nor->lock); >> + >> if (nor->controller_ops && nor->controller_ops->unprepare) >> nor->controller_ops->unprepare(nor); >> - mutex_unlock(&nor->lock); >> } >> >> static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr) >> @@ -1447,7 +1444,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) >> addr = instr->addr; >> len = instr->len; >> >> - ret = spi_nor_lock_and_prep(nor); >> + ret = spi_nor_prep_and_lock(nor); >> if (ret) >> return ret; >> >> @@ -1707,7 +1704,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, >> >> dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); >> >> - ret = spi_nor_lock_and_prep(nor); >> + ret = spi_nor_prep_and_lock(nor); >> if (ret) >> return ret; >> >> @@ -1753,7 +1750,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, >> >> dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); >> >> - ret = spi_nor_lock_and_prep(nor); >> + ret = spi_nor_prep_and_lock(nor); >> if (ret) >> return ret; >>
Hi Tudor, tudor.ambarus@linaro.org wrote on Fri, 17 Mar 2023 03:51:37 +0000: > On 3/17/23 03:39, Tudor Ambarus wrote: > > > > > > On 2/1/23 11:35, Miquel Raynal wrote: > >> The ->prepare()/->unprepare() hooks are now legacy, we no longer accept > >> new drivers supporting them. The only remaining controllers using them > >> acquires a per-chip mutex, which should not interfere with the rest of > >> the operation done in the core. As a result, we should be safe to > >> reorganize these helpers to first perform the preparation, before > >> acquiring the core locks. This is necessary in order to be able to > >> improve the locking mechanism in the core (coming next). No side effects > >> are expected. > >> > >> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org> > > No, I take back my R-b. The reorder is fine, but you missed to update > otp.c and swp.c, see below. /me guilty Not sure how that slipped in. Anyhow, I'll go through the missing files ofc. Thanks, Miquèl
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 3845de9c874c..01932dbaa5b9 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1071,27 +1071,24 @@ static void spi_nor_set_4byte_opcodes(struct spi_nor *nor) } } -int spi_nor_lock_and_prep(struct spi_nor *nor) +int spi_nor_prep_and_lock(struct spi_nor *nor) { int ret = 0; - mutex_lock(&nor->lock); - - if (nor->controller_ops && nor->controller_ops->prepare) { + if (nor->controller_ops && nor->controller_ops->prepare) ret = nor->controller_ops->prepare(nor); - if (ret) { - mutex_unlock(&nor->lock); - return ret; - } - } + + mutex_lock(&nor->lock); + return ret; } void spi_nor_unlock_and_unprep(struct spi_nor *nor) { + mutex_unlock(&nor->lock); + if (nor->controller_ops && nor->controller_ops->unprepare) nor->controller_ops->unprepare(nor); - mutex_unlock(&nor->lock); } static u32 spi_nor_convert_addr(struct spi_nor *nor, loff_t addr) @@ -1447,7 +1444,7 @@ static int spi_nor_erase(struct mtd_info *mtd, struct erase_info *instr) addr = instr->addr; len = instr->len; - ret = spi_nor_lock_and_prep(nor); + ret = spi_nor_prep_and_lock(nor); if (ret) return ret; @@ -1707,7 +1704,7 @@ static int spi_nor_read(struct mtd_info *mtd, loff_t from, size_t len, dev_dbg(nor->dev, "from 0x%08x, len %zd\n", (u32)from, len); - ret = spi_nor_lock_and_prep(nor); + ret = spi_nor_prep_and_lock(nor); if (ret) return ret; @@ -1753,7 +1750,7 @@ static int spi_nor_write(struct mtd_info *mtd, loff_t to, size_t len, dev_dbg(nor->dev, "to 0x%08x, len %zd\n", (u32)to, len); - ret = spi_nor_lock_and_prep(nor); + ret = spi_nor_prep_and_lock(nor); if (ret) return ret;
The ->prepare()/->unprepare() hooks are now legacy, we no longer accept new drivers supporting them. The only remaining controllers using them acquires a per-chip mutex, which should not interfere with the rest of the operation done in the core. As a result, we should be safe to reorganize these helpers to first perform the preparation, before acquiring the core locks. This is necessary in order to be able to improve the locking mechanism in the core (coming next). No side effects are expected. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/spi-nor/core.c | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-)