diff mbox series

[v4,3/8] mtd: spi-nor: Reorder the preparation vs locking steps

Message ID 20230201113603.293758-4-miquel.raynal@bootlin.com (mailing list archive)
State New, archived
Headers show
Series mtd: spi-nor: read while write support | expand

Commit Message

Miquel Raynal Feb. 1, 2023, 11:35 a.m. UTC
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(-)

Comments

Tudor Ambarus March 17, 2023, 3:39 a.m. UTC | #1
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;
>
Tudor Ambarus March 17, 2023, 3:51 a.m. UTC | #2
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;
>>
Miquel Raynal March 24, 2023, 3:28 p.m. UTC | #3
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 mbox series

Patch

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;