diff mbox series

[1/4] spi: dw: differentiate between unsupported and invalid requests

Message ID 20240522145255.995778-2-miquel.raynal@bootlin.com (mailing list archive)
State Accepted
Commit 19a9aa9302276d49a4c4890f656359808d3a0151
Headers show
Series spi: differentiate between unsupported and invalid requests | expand

Commit Message

Miquel Raynal May 22, 2024, 2:52 p.m. UTC
The driver does not support dirmap write operations, return -EOPTNOTSUPP
in this case.

Most controllers have a maximum linear mapping area. Requests beyond
this limit can be considered invalid, rather than unsupported.

From a caller (and reviewer) point of view, distinguising between the
two may be helpful because somehow one can be "fixed" while the other
will always be refused no matter how hard we try.

As part of a wider work to bring spi-nand continuous reads, it was
useful to easily catch the upper limit direct mapping boundaries for
each controller, with the idea of enlarging this area from a page to an
eraseblock, without risking too many regressions.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/spi/spi-dw-bt1.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Comments

Serge Semin May 31, 2024, 1:39 p.m. UTC | #1
On Wed, May 22, 2024 at 04:52:52PM +0200, Miquel Raynal wrote:
> The driver does not support dirmap write operations, return -EOPTNOTSUPP
> in this case.
> 
> Most controllers have a maximum linear mapping area. Requests beyond
> this limit can be considered invalid, rather than unsupported.
> 
> From a caller (and reviewer) point of view, distinguising between the
> two may be helpful because somehow one can be "fixed" while the other
> will always be refused no matter how hard we try.
> 
> As part of a wider work to bring spi-nand continuous reads, it was
> useful to easily catch the upper limit direct mapping boundaries for
> each controller, with the idea of enlarging this area from a page to an
> eraseblock, without risking too many regressions.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/spi/spi-dw-bt1.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-bt1.c b/drivers/spi/spi-dw-bt1.c
> index 5391bcac305c..4577e8096cd9 100644
> --- a/drivers/spi/spi-dw-bt1.c
> +++ b/drivers/spi/spi-dw-bt1.c
> @@ -55,13 +55,15 @@ static int dw_spi_bt1_dirmap_create(struct spi_mem_dirmap_desc *desc)
>  	    !dwsbt1->dws.mem_ops.supports_op(desc->mem, &desc->info.op_tmpl))
>  		return -EOPNOTSUPP;
> 
> +	if (desc->info.op_tmpl.data.dir != SPI_MEM_DATA_IN)
> +		return -EOPNOTSUPP;
> +
>  	/*
>  	 * Make sure the requested region doesn't go out of the physically
> -	 * mapped flash memory bounds and the operation is read-only.
> +	 * mapped flash memory bounds.
>  	 */
> -	if (desc->info.offset + desc->info.length > dwsbt1->map_len ||
> -	    desc->info.op_tmpl.data.dir != SPI_MEM_DATA_IN)
> -		return -EOPNOTSUPP;
> +	if (desc->info.offset + desc->info.length > dwsbt1->map_len)
> +		return -EINVAL;

Seems reasonable. Indeed the out of bounds situation is better
described by the EINVAL error. So the change looks good to me:

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

But note the error number won't be propagated that far and will be
overwritten in spi_mem_dirmap_create() with either zero or
-EOPNOTSUPP.

-Serge(y)

>  
>  	return 0;
>  }
> -- 
> 2.40.1
>
diff mbox series

Patch

diff --git a/drivers/spi/spi-dw-bt1.c b/drivers/spi/spi-dw-bt1.c
index 5391bcac305c..4577e8096cd9 100644
--- a/drivers/spi/spi-dw-bt1.c
+++ b/drivers/spi/spi-dw-bt1.c
@@ -55,13 +55,15 @@  static int dw_spi_bt1_dirmap_create(struct spi_mem_dirmap_desc *desc)
 	    !dwsbt1->dws.mem_ops.supports_op(desc->mem, &desc->info.op_tmpl))
 		return -EOPNOTSUPP;
 
+	if (desc->info.op_tmpl.data.dir != SPI_MEM_DATA_IN)
+		return -EOPNOTSUPP;
+
 	/*
 	 * Make sure the requested region doesn't go out of the physically
-	 * mapped flash memory bounds and the operation is read-only.
+	 * mapped flash memory bounds.
 	 */
-	if (desc->info.offset + desc->info.length > dwsbt1->map_len ||
-	    desc->info.op_tmpl.data.dir != SPI_MEM_DATA_IN)
-		return -EOPNOTSUPP;
+	if (desc->info.offset + desc->info.length > dwsbt1->map_len)
+		return -EINVAL;
 
 	return 0;
 }