| Message ID | 20221215081241.407098-5-miquel.raynal@bootlin.com |
|---|---|
| State | Superseded |
| Delegated to: | Ambarus Tudor |
| Headers | show |
| Series | mtd: spi-nor: read while write support | expand |
Hi, Miquel, On 12/15/22 08:12, Miquel Raynal wrote: > The ->prepare()/->unprepare() hooks are now legacy, and there are only > two controllers left supporting them. In both cases, the implementation now there's only one, hisi-sfc.c. > acquires a mutex, which is somehow redundant with the spi-nor main lock I see a HIFMC_MAX_CHIP_NUM with value 2, the controller seems to be able to operate 2 flashes in parallel and that's why the internal mutex. > that we acquire as well in the spi_nor_[un]lock_and_[un]prep() helpers. > > While the mutex taken in the core is necessary, the helper can be > reorganized to first do the preparation, then acquire the core > lock. This is necessary in order to be able to improve the locking > mechanism in the core and should have no side effect. > the change seems fine for hisi, and since we're no longer adding drivers under spi-nor/controllers we should be good. > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/spi-nor/core.c | 15 ++++++--------- > 1 file changed, 6 insertions(+), 9 deletions(-) > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > index 38a57aac6754..de77ca55f74d 100644 > --- a/drivers/mtd/spi-nor/core.c > +++ b/drivers/mtd/spi-nor/core.c > @@ -1074,23 +1074,20 @@ int spi_nor_lock_and_prep(struct spi_nor *nor) you'll have to rename it to spi_nor_prep_and_lock to reflect the order of ops. > { > 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; > } > Cheers, ta
Hi Tudor, tudor.ambarus@linaro.org wrote on Tue, 31 Jan 2023 05:11:33 +0000: > Hi, Miquel, > > On 12/15/22 08:12, Miquel Raynal wrote: > > The ->prepare()/->unprepare() hooks are now legacy, and there are only > > two controllers left supporting them. In both cases, the implementation > > now there's only one, hisi-sfc.c. Indeed. > > > acquires a mutex, which is somehow redundant with the spi-nor main lock > > I see a HIFMC_MAX_CHIP_NUM with value 2, the controller seems to be able > to operate 2 flashes in parallel and that's why the internal mutex. > > > that we acquire as well in the spi_nor_[un]lock_and_[un]prep() helpers. > > > > While the mutex taken in the core is necessary, the helper can be > > reorganized to first do the preparation, then acquire the core > > lock. This is necessary in order to be able to improve the locking > > mechanism in the core and should have no side effect. > > > > the change seems fine for hisi, and since we're no longer adding drivers > under spi-nor/controllers we should be good. > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/mtd/spi-nor/core.c | 15 ++++++--------- > > 1 file changed, 6 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c > > index 38a57aac6754..de77ca55f74d 100644 > > --- a/drivers/mtd/spi-nor/core.c > > +++ b/drivers/mtd/spi-nor/core.c > > @@ -1074,23 +1074,20 @@ int spi_nor_lock_and_prep(struct spi_nor *nor) > > you'll have to rename it to spi_nor_prep_and_lock to reflect the order > of ops. Sure. > > > { > > 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; > > } > > > > Cheers, > ta Thanks, Miquèl
diff --git a/drivers/mtd/spi-nor/core.c b/drivers/mtd/spi-nor/core.c index 38a57aac6754..de77ca55f74d 100644 --- a/drivers/mtd/spi-nor/core.c +++ b/drivers/mtd/spi-nor/core.c @@ -1074,23 +1074,20 @@ int spi_nor_lock_and_prep(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)
The ->prepare()/->unprepare() hooks are now legacy, and there are only two controllers left supporting them. In both cases, the implementation acquires a mutex, which is somehow redundant with the spi-nor main lock that we acquire as well in the spi_nor_[un]lock_and_[un]prep() helpers. While the mutex taken in the core is necessary, the helper can be reorganized to first do the preparation, then acquire the core lock. This is necessary in order to be able to improve the locking mechanism in the core and should have no side effect. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/spi-nor/core.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-)