[PATCH 0/3] spi: spi-mem: Minor fixes/improvements

[PATCH 0/3] spi: spi-mem: Minor fixes/improvements

[Boris Brezillon]

Hello Mark,

Here is a patchset which, IMO, is improving spi-mem support. The first
2 patches are fixing/improving the kerneldoc header associated to the
spi_mem_op struct, and the last one is adding checks on the spi_mem_op
object passed to spi_mem_{exec,supports}_op().

Regards,

Boris

Boris Brezillon (3):
  spi: spi-mem: Add missing description for data.nbytes field
  spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header
  spi: spi-mem: Add extra sanity checks on the op param

 drivers/spi/spi-mem.c       | 54 ++++++++++++++++++++++++++++++++++++++++-----
 include/linux/spi/spi-mem.h |  7 +++---
 2 files changed, 52 insertions(+), 9 deletions(-)

-- 
2.14.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 0/3] spi: spi-mem: Minor fixes/improvements

[Boris Brezillon]

Hello Mark,

Here is a patchset which, IMO, is improving spi-mem support. The first
2 patches are fixing/improving the kerneldoc header associated to the
spi_mem_op struct, and the last one is adding checks on the spi_mem_op
object passed to spi_mem_{exec,supports}_op().

Regards,

Boris

Boris Brezillon (3):
  spi: spi-mem: Add missing description for data.nbytes field
  spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header
  spi: spi-mem: Add extra sanity checks on the op param

 drivers/spi/spi-mem.c       | 54 ++++++++++++++++++++++++++++++++++++++++-----
 include/linux/spi/spi-mem.h |  7 +++---
 2 files changed, 52 insertions(+), 9 deletions(-)

-- 
2.14.1

[PATCH 1/3] spi: spi-mem: Add missing description for data.nbytes field

[Boris Brezillon]

Add a description for spi_mem_op.data.nbytes to the kerneldoc header.

Fixes: c36ff266dc82 ("spi: Extend the core to ease integration of SPI memory controllers")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 include/linux/spi/spi-mem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index b2bd4b4127c4..aeb87c02cf1d 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -81,6 +81,8 @@ enum spi_mem_data_dir {
  * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
  * @data.buswidth: number of IO lanes used to send/receive the data
  * @data.dir: direction of the transfer
+ * @data.nbytes: number of data bytes to send/receive. Can be zero if the
+ *		 operation does not involve transferring data
  * @data.buf.in: input buffer
  * @data.buf.out: output buffer
  */
-- 
2.14.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/3] spi: spi-mem: Add missing description for data.nbytes field

[Boris Brezillon]

Add a description for spi_mem_op.data.nbytes to the kerneldoc header.

Fixes: c36ff266dc82 ("spi: Extend the core to ease integration of SPI memory controllers")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 include/linux/spi/spi-mem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index b2bd4b4127c4..aeb87c02cf1d 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -81,6 +81,8 @@ enum spi_mem_data_dir {
  * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
  * @data.buswidth: number of IO lanes used to send/receive the data
  * @data.dir: direction of the transfer
+ * @data.nbytes: number of data bytes to send/receive. Can be zero if the
+ *		 operation does not involve transferring data
  * @data.buf.in: input buffer
  * @data.buf.out: output buffer
  */
-- 
2.14.1

[PATCH 2/3] spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header

[Boris Brezillon]

We'd better have that documented in the kerneldoc header, so that it's
exposed to the doc generated by Sphinx.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 include/linux/spi/spi-mem.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index aeb87c02cf1d..69ee30456864 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -83,8 +83,8 @@ enum spi_mem_data_dir {
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
  *		 operation does not involve transferring data
- * @data.buf.in: input buffer
- * @data.buf.out: output buffer
+ * @data.buf.in: input buffer (must be DMA-able)
+ * @data.buf.out: output buffer (must be DMA-able)
  */
 struct spi_mem_op {
 	struct {
@@ -107,7 +107,6 @@ struct spi_mem_op {
 		u8 buswidth;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
-		/* buf.{in,out} must be DMA-able. */
 		union {
 			void *in;
 			const void *out;
-- 
2.14.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/3] spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header

[Boris Brezillon]

We'd better have that documented in the kerneldoc header, so that it's
exposed to the doc generated by Sphinx.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 include/linux/spi/spi-mem.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index aeb87c02cf1d..69ee30456864 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -83,8 +83,8 @@ enum spi_mem_data_dir {
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
  *		 operation does not involve transferring data
- * @data.buf.in: input buffer
- * @data.buf.out: output buffer
+ * @data.buf.in: input buffer (must be DMA-able)
+ * @data.buf.out: output buffer (must be DMA-able)
  */
 struct spi_mem_op {
 	struct {
@@ -107,7 +107,6 @@ struct spi_mem_op {
 		u8 buswidth;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
-		/* buf.{in,out} must be DMA-able. */
 		union {
 			void *in;
 			const void *out;
-- 
2.14.1

[PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Boris Brezillon]

Some combinations are simply not valid and should be rejected before
the op is passed to the SPI controller driver.

Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
spi_mem_supports_op() to make sure the spi-mem operation is valid.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/spi/spi-mem.c | 54 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index eb72dba71d83..cc3d425aae56 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -12,6 +12,8 @@
 
 #include "internals.h"
 
+#define SPI_MEM_MAX_BUSWIDTH		4
+
 /**
  * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
  *					  memory operation
@@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct spi_mem *mem,
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
 
+static bool spi_mem_buswidth_is_valid(u8 buswidth)
+{
+	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
+		return false;
+
+	return true;
+}
+
+static int spi_mem_check_op(const struct spi_mem_op *op)
+{
+	if (!op->cmd.buswidth)
+		return -EINVAL;
+
+	if ((op->addr.nbytes && !op->addr.buswidth) ||
+	    (op->dummy.nbytes && !op->dummy.buswidth) ||
+	    (op->data.nbytes && !op->data.buswidth))
+		return -EINVAL;
+
+	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->data.buswidth))
+		return -EINVAL;
+
+	return 0;
+}
+
+static bool spi_mem_internal_supports_op(struct spi_mem *mem,
+					 const struct spi_mem_op *op)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
+		return ctlr->mem_ops->supports_op(mem, op);
+
+	return spi_mem_default_supports_op(mem, op);
+}
+
 /**
  * spi_mem_supports_op() - Check if a memory device and the controller it is
  *			   connected to support a specific memory operation
@@ -166,12 +206,10 @@ EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
  */
 bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
-	struct spi_controller *ctlr = mem->spi->controller;
-
-	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
-		return ctlr->mem_ops->supports_op(mem, op);
+	if (spi_mem_check_op(op))
+		return false;
 
-	return spi_mem_default_supports_op(mem, op);
+	return spi_mem_internal_supports_op(mem, op);
 }
 EXPORT_SYMBOL_GPL(spi_mem_supports_op);
 
@@ -196,7 +234,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	u8 *tmpbuf;
 	int ret;
 
-	if (!spi_mem_supports_op(mem, op))
+	ret = spi_mem_check_op(op);
+	if (ret)
+		return ret;
+
+	if (!spi_mem_internal_supports_op(mem, op))
 		return -ENOTSUPP;
 
 	if (ctlr->mem_ops) {
-- 
2.14.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Boris Brezillon]

Some combinations are simply not valid and should be rejected before
the op is passed to the SPI controller driver.

Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
spi_mem_supports_op() to make sure the spi-mem operation is valid.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
---
 drivers/spi/spi-mem.c | 54 +++++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index eb72dba71d83..cc3d425aae56 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -12,6 +12,8 @@
 
 #include "internals.h"
 
+#define SPI_MEM_MAX_BUSWIDTH		4
+
 /**
  * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
  *					  memory operation
@@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct spi_mem *mem,
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
 
+static bool spi_mem_buswidth_is_valid(u8 buswidth)
+{
+	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
+		return false;
+
+	return true;
+}
+
+static int spi_mem_check_op(const struct spi_mem_op *op)
+{
+	if (!op->cmd.buswidth)
+		return -EINVAL;
+
+	if ((op->addr.nbytes && !op->addr.buswidth) ||
+	    (op->dummy.nbytes && !op->dummy.buswidth) ||
+	    (op->data.nbytes && !op->data.buswidth))
+		return -EINVAL;
+
+	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->data.buswidth))
+		return -EINVAL;
+
+	return 0;
+}
+
+static bool spi_mem_internal_supports_op(struct spi_mem *mem,
+					 const struct spi_mem_op *op)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
+		return ctlr->mem_ops->supports_op(mem, op);
+
+	return spi_mem_default_supports_op(mem, op);
+}
+
 /**
  * spi_mem_supports_op() - Check if a memory device and the controller it is
  *			   connected to support a specific memory operation
@@ -166,12 +206,10 @@ EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
  */
 bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
-	struct spi_controller *ctlr = mem->spi->controller;
-
-	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
-		return ctlr->mem_ops->supports_op(mem, op);
+	if (spi_mem_check_op(op))
+		return false;
 
-	return spi_mem_default_supports_op(mem, op);
+	return spi_mem_internal_supports_op(mem, op);
 }
 EXPORT_SYMBOL_GPL(spi_mem_supports_op);
 
@@ -196,7 +234,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	u8 *tmpbuf;
 	int ret;
 
-	if (!spi_mem_supports_op(mem, op))
+	ret = spi_mem_check_op(op);
+	if (ret)
+		return ret;
+
+	if (!spi_mem_internal_supports_op(mem, op))
 		return -ENOTSUPP;
 
 	if (ctlr->mem_ops) {
-- 
2.14.1

RE: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Yogesh Narayan Gaur]

Hi Boris,

> -----Original Message-----
> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of
> Boris Brezillon
> Sent: Thursday, September 20, 2018 1:01 PM
> To: Mark Brown <broonie@kernel.org>; linux-spi@vger.kernel.org
> Cc: Marek Vasut <marek.vasut@gmail.com>; Richard Weinberger
> <richard@nod.at>; Boris Brezillon <boris.brezillon@bootlin.com>; linux-
> mtd@lists.infradead.org; Brian Norris <computersforpeace@gmail.com>; David
> Woodhouse <dwmw2@infradead.org>
> Subject: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param
> 
> Some combinations are simply not valid and should be rejected before the op is
> passed to the SPI controller driver.
> 
> Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
> spi_mem_supports_op() to make sure the spi-mem operation is valid.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/spi/spi-mem.c | 54
> +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index
> eb72dba71d83..cc3d425aae56 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -12,6 +12,8 @@
> 
>  #include "internals.h"
> 
> +#define SPI_MEM_MAX_BUSWIDTH		4
> +
>  /**
>   * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to
> a
>   *					  memory operation
> @@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct
> spi_mem *mem,  }  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
> 
> +static bool spi_mem_buswidth_is_valid(u8 buswidth) {
> +	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)

Isn't check for lower limit should be '< 1'?

--
Regards
Yogesh Gaur

> +		return false;
> +
> +	return true;
> +}
> +
> +static int spi_mem_check_op(const struct spi_mem_op *op) {
> +	if (!op->cmd.buswidth)
> +		return -EINVAL;
> +
> +	if ((op->addr.nbytes && !op->addr.buswidth) ||
> +	    (op->dummy.nbytes && !op->dummy.buswidth) ||
> +	    (op->data.nbytes && !op->data.buswidth))
> +		return -EINVAL;
> +
> +	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->data.buswidth))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static bool spi_mem_internal_supports_op(struct spi_mem *mem,
> +					 const struct spi_mem_op *op)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +
> +	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
> +		return ctlr->mem_ops->supports_op(mem, op);
> +
> +	return spi_mem_default_supports_op(mem, op); }
> +
>  /**
>   * spi_mem_supports_op() - Check if a memory device and the controller it is
>   *			   connected to support a specific memory operation
> @@ -166,12 +206,10 @@
> EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>   */
>  bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op
> *op)  {
> -	struct spi_controller *ctlr = mem->spi->controller;
> -
> -	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
> -		return ctlr->mem_ops->supports_op(mem, op);
> +	if (spi_mem_check_op(op))
> +		return false;
> 
> -	return spi_mem_default_supports_op(mem, op);
> +	return spi_mem_internal_supports_op(mem, op);
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_supports_op);
> 
> @@ -196,7 +234,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
>  	u8 *tmpbuf;
>  	int ret;
> 
> -	if (!spi_mem_supports_op(mem, op))
> +	ret = spi_mem_check_op(op);
> +	if (ret)
> +		return ret;
> +
> +	if (!spi_mem_internal_supports_op(mem, op))
>  		return -ENOTSUPP;
> 
>  	if (ctlr->mem_ops) {
> --
> 2.14.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fmailman%2Flistinfo%2Flinux-
> mtd%2F&amp;data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C3593fc7
> 7e9d44b5b7a6e08d61ecb6c1c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7C636730256384380635&amp;sdata=lTp45z8K1WFhI0LA7Nxca2p8pdHsQL
> gXNFC1GJIi1NM%3D&amp;reserved=0

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Yogesh Narayan Gaur]

Hi Boris,

> -----Original Message-----
> From: linux-mtd [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of
> Boris Brezillon
> Sent: Thursday, September 20, 2018 1:01 PM
> To: Mark Brown <broonie@kernel.org>; linux-spi@vger.kernel.org
> Cc: Marek Vasut <marek.vasut@gmail.com>; Richard Weinberger
> <richard@nod.at>; Boris Brezillon <boris.brezillon@bootlin.com>; linux-
> mtd@lists.infradead.org; Brian Norris <computersforpeace@gmail.com>; David
> Woodhouse <dwmw2@infradead.org>
> Subject: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param
> 
> Some combinations are simply not valid and should be rejected before the op is
> passed to the SPI controller driver.
> 
> Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
> spi_mem_supports_op() to make sure the spi-mem operation is valid.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
> ---
>  drivers/spi/spi-mem.c | 54
> +++++++++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 48 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index
> eb72dba71d83..cc3d425aae56 100644
> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -12,6 +12,8 @@
> 
>  #include "internals.h"
> 
> +#define SPI_MEM_MAX_BUSWIDTH		4
> +
>  /**
>   * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to
> a
>   *					  memory operation
> @@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct
> spi_mem *mem,  }  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
> 
> +static bool spi_mem_buswidth_is_valid(u8 buswidth) {
> +	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)

Isn't check for lower limit should be '< 1'?

--
Regards
Yogesh Gaur

> +		return false;
> +
> +	return true;
> +}
> +
> +static int spi_mem_check_op(const struct spi_mem_op *op) {
> +	if (!op->cmd.buswidth)
> +		return -EINVAL;
> +
> +	if ((op->addr.nbytes && !op->addr.buswidth) ||
> +	    (op->dummy.nbytes && !op->dummy.buswidth) ||
> +	    (op->data.nbytes && !op->data.buswidth))
> +		return -EINVAL;
> +
> +	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
> +	    spi_mem_buswidth_is_valid(op->data.buswidth))
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static bool spi_mem_internal_supports_op(struct spi_mem *mem,
> +					 const struct spi_mem_op *op)
> +{
> +	struct spi_controller *ctlr = mem->spi->controller;
> +
> +	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
> +		return ctlr->mem_ops->supports_op(mem, op);
> +
> +	return spi_mem_default_supports_op(mem, op); }
> +
>  /**
>   * spi_mem_supports_op() - Check if a memory device and the controller it is
>   *			   connected to support a specific memory operation
> @@ -166,12 +206,10 @@
> EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>   */
>  bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op
> *op)  {
> -	struct spi_controller *ctlr = mem->spi->controller;
> -
> -	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
> -		return ctlr->mem_ops->supports_op(mem, op);
> +	if (spi_mem_check_op(op))
> +		return false;
> 
> -	return spi_mem_default_supports_op(mem, op);
> +	return spi_mem_internal_supports_op(mem, op);
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_supports_op);
> 
> @@ -196,7 +234,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const
> struct spi_mem_op *op)
>  	u8 *tmpbuf;
>  	int ret;
> 
> -	if (!spi_mem_supports_op(mem, op))
> +	ret = spi_mem_check_op(op);
> +	if (ret)
> +		return ret;
> +
> +	if (!spi_mem_internal_supports_op(mem, op))
>  		return -ENOTSUPP;
> 
>  	if (ctlr->mem_ops) {
> --
> 2.14.1
> 
> 
> ______________________________________________________
> Linux MTD discussion mailing list
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fmailman%2Flistinfo%2Flinux-
> mtd%2F&amp;data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C3593fc7
> 7e9d44b5b7a6e08d61ecb6c1c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%
> 7C0%7C636730256384380635&amp;sdata=lTp45z8K1WFhI0LA7Nxca2p8pdHsQL
> gXNFC1GJIi1NM%3D&amp;reserved=0

Re: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Boris Brezillon]

Hi Yogesh,

On Thu, 20 Sep 2018 10:31:16 +0000
Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com> wrote:

> > 
> > +static bool spi_mem_buswidth_is_valid(u8 buswidth) {
> > +	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)  
> 
> Isn't check for lower limit should be '< 1'?

buswidth == 0 is valid if you have nothing to send...

> 
> --
> Regards
> Yogesh Gaur
> 
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static int spi_mem_check_op(const struct spi_mem_op *op) {
> > +	if (!op->cmd.buswidth)
> > +		return -EINVAL;
> > +
> > +	if ((op->addr.nbytes && !op->addr.buswidth) ||
> > +	    (op->dummy.nbytes && !op->dummy.buswidth) ||
> > +	    (op->data.nbytes && !op->data.buswidth))
> > +		return -EINVAL;

... and I make sure op->data.buswidth != 0 when required here.

Thanks for the review by the way.

Boris

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Boris Brezillon]

Hi Yogesh,

On Thu, 20 Sep 2018 10:31:16 +0000
Yogesh Narayan Gaur <yogeshnarayan.gaur@nxp.com> wrote:

> > 
> > +static bool spi_mem_buswidth_is_valid(u8 buswidth) {
> > +	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)  
> 
> Isn't check for lower limit should be '< 1'?

buswidth == 0 is valid if you have nothing to send...

> 
> --
> Regards
> Yogesh Gaur
> 
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > +static int spi_mem_check_op(const struct spi_mem_op *op) {
> > +	if (!op->cmd.buswidth)
> > +		return -EINVAL;
> > +
> > +	if ((op->addr.nbytes && !op->addr.buswidth) ||
> > +	    (op->dummy.nbytes && !op->dummy.buswidth) ||
> > +	    (op->data.nbytes && !op->data.buswidth))
> > +		return -EINVAL;

... and I make sure op->data.buswidth != 0 when required here.

Thanks for the review by the way.

Boris

Applied "spi: spi-mem: Add extra sanity checks on the op param" to the spi tree

[Mark Brown]

The patch

   spi: spi-mem: Add extra sanity checks on the op param

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 380583227c0c7f52383b0cd5c0e2de93ed31d553 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@bootlin.com>
Date: Thu, 20 Sep 2018 09:31:12 +0200
Subject: [PATCH] spi: spi-mem: Add extra sanity checks on the op param

Some combinations are simply not valid and should be rejected before
the op is passed to the SPI controller driver.

Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
spi_mem_supports_op() to make sure the spi-mem operation is valid.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mem.c | 54 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index eb72dba71d83..cc3d425aae56 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -12,6 +12,8 @@
 
 #include "internals.h"
 
+#define SPI_MEM_MAX_BUSWIDTH		4
+
 /**
  * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
  *					  memory operation
@@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct spi_mem *mem,
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
 
+static bool spi_mem_buswidth_is_valid(u8 buswidth)
+{
+	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
+		return false;
+
+	return true;
+}
+
+static int spi_mem_check_op(const struct spi_mem_op *op)
+{
+	if (!op->cmd.buswidth)
+		return -EINVAL;
+
+	if ((op->addr.nbytes && !op->addr.buswidth) ||
+	    (op->dummy.nbytes && !op->dummy.buswidth) ||
+	    (op->data.nbytes && !op->data.buswidth))
+		return -EINVAL;
+
+	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->data.buswidth))
+		return -EINVAL;
+
+	return 0;
+}
+
+static bool spi_mem_internal_supports_op(struct spi_mem *mem,
+					 const struct spi_mem_op *op)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
+		return ctlr->mem_ops->supports_op(mem, op);
+
+	return spi_mem_default_supports_op(mem, op);
+}
+
 /**
  * spi_mem_supports_op() - Check if a memory device and the controller it is
  *			   connected to support a specific memory operation
@@ -166,12 +206,10 @@ EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
  */
 bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
-	struct spi_controller *ctlr = mem->spi->controller;
-
-	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
-		return ctlr->mem_ops->supports_op(mem, op);
+	if (spi_mem_check_op(op))
+		return false;
 
-	return spi_mem_default_supports_op(mem, op);
+	return spi_mem_internal_supports_op(mem, op);
 }
 EXPORT_SYMBOL_GPL(spi_mem_supports_op);
 
@@ -196,7 +234,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	u8 *tmpbuf;
 	int ret;
 
-	if (!spi_mem_supports_op(mem, op))
+	ret = spi_mem_check_op(op);
+	if (ret)
+		return ret;
+
+	if (!spi_mem_internal_supports_op(mem, op))
 		return -ENOTSUPP;
 
 	if (ctlr->mem_ops) {
-- 
2.19.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Applied "spi: spi-mem: Add extra sanity checks on the op param" to the spi tree

[Mark Brown]

The patch

   spi: spi-mem: Add extra sanity checks on the op param

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 380583227c0c7f52383b0cd5c0e2de93ed31d553 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@bootlin.com>
Date: Thu, 20 Sep 2018 09:31:12 +0200
Subject: [PATCH] spi: spi-mem: Add extra sanity checks on the op param

Some combinations are simply not valid and should be rejected before
the op is passed to the SPI controller driver.

Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
spi_mem_supports_op() to make sure the spi-mem operation is valid.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-mem.c | 54 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c
index eb72dba71d83..cc3d425aae56 100644
--- a/drivers/spi/spi-mem.c
+++ b/drivers/spi/spi-mem.c
@@ -12,6 +12,8 @@
 
 #include "internals.h"
 
+#define SPI_MEM_MAX_BUSWIDTH		4
+
 /**
  * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
  *					  memory operation
@@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct spi_mem *mem,
 }
 EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
 
+static bool spi_mem_buswidth_is_valid(u8 buswidth)
+{
+	if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
+		return false;
+
+	return true;
+}
+
+static int spi_mem_check_op(const struct spi_mem_op *op)
+{
+	if (!op->cmd.buswidth)
+		return -EINVAL;
+
+	if ((op->addr.nbytes && !op->addr.buswidth) ||
+	    (op->dummy.nbytes && !op->dummy.buswidth) ||
+	    (op->data.nbytes && !op->data.buswidth))
+		return -EINVAL;
+
+	if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->addr.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
+	    spi_mem_buswidth_is_valid(op->data.buswidth))
+		return -EINVAL;
+
+	return 0;
+}
+
+static bool spi_mem_internal_supports_op(struct spi_mem *mem,
+					 const struct spi_mem_op *op)
+{
+	struct spi_controller *ctlr = mem->spi->controller;
+
+	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
+		return ctlr->mem_ops->supports_op(mem, op);
+
+	return spi_mem_default_supports_op(mem, op);
+}
+
 /**
  * spi_mem_supports_op() - Check if a memory device and the controller it is
  *			   connected to support a specific memory operation
@@ -166,12 +206,10 @@ EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
  */
 bool spi_mem_supports_op(struct spi_mem *mem, const struct spi_mem_op *op)
 {
-	struct spi_controller *ctlr = mem->spi->controller;
-
-	if (ctlr->mem_ops && ctlr->mem_ops->supports_op)
-		return ctlr->mem_ops->supports_op(mem, op);
+	if (spi_mem_check_op(op))
+		return false;
 
-	return spi_mem_default_supports_op(mem, op);
+	return spi_mem_internal_supports_op(mem, op);
 }
 EXPORT_SYMBOL_GPL(spi_mem_supports_op);
 
@@ -196,7 +234,11 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op)
 	u8 *tmpbuf;
 	int ret;
 
-	if (!spi_mem_supports_op(mem, op))
+	ret = spi_mem_check_op(op);
+	if (ret)
+		return ret;
+
+	if (!spi_mem_internal_supports_op(mem, op))
 		return -ENOTSUPP;
 
 	if (ctlr->mem_ops) {
-- 
2.19.0

Applied "spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header" to the spi tree

[Mark Brown]

The patch

   spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From c949a8e8b43f2c75567269bcc9a50d704ae3c420 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@bootlin.com>
Date: Thu, 20 Sep 2018 09:31:11 +0200
Subject: [PATCH] spi: spi-mem: Move the DMA-able constraint doc to the
 kerneldoc header

We'd better have that documented in the kerneldoc header, so that it's
exposed to the doc generated by Sphinx.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/spi/spi-mem.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 0cfbb1ad8d96..7195fbc234aa 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -81,8 +81,8 @@ enum spi_mem_data_dir {
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
  *		 operation does not involve transferring data
- * @data.buf.in: input buffer
- * @data.buf.out: output buffer
+ * @data.buf.in: input buffer (must be DMA-able)
+ * @data.buf.out: output buffer (must be DMA-able)
  */
 struct spi_mem_op {
 	struct {
@@ -105,7 +105,6 @@ struct spi_mem_op {
 		u8 buswidth;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
-		/* buf.{in,out} must be DMA-able. */
 		union {
 			void *in;
 			const void *out;
-- 
2.19.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Applied "spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header" to the spi tree

[Mark Brown]

The patch

   spi: spi-mem: Move the DMA-able constraint doc to the kerneldoc header

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From c949a8e8b43f2c75567269bcc9a50d704ae3c420 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@bootlin.com>
Date: Thu, 20 Sep 2018 09:31:11 +0200
Subject: [PATCH] spi: spi-mem: Move the DMA-able constraint doc to the
 kerneldoc header

We'd better have that documented in the kerneldoc header, so that it's
exposed to the doc generated by Sphinx.

Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/spi/spi-mem.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 0cfbb1ad8d96..7195fbc234aa 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -81,8 +81,8 @@ enum spi_mem_data_dir {
  * @data.dir: direction of the transfer
  * @data.nbytes: number of data bytes to send/receive. Can be zero if the
  *		 operation does not involve transferring data
- * @data.buf.in: input buffer
- * @data.buf.out: output buffer
+ * @data.buf.in: input buffer (must be DMA-able)
+ * @data.buf.out: output buffer (must be DMA-able)
  */
 struct spi_mem_op {
 	struct {
@@ -105,7 +105,6 @@ struct spi_mem_op {
 		u8 buswidth;
 		enum spi_mem_data_dir dir;
 		unsigned int nbytes;
-		/* buf.{in,out} must be DMA-able. */
 		union {
 			void *in;
 			const void *out;
-- 
2.19.0

Applied "spi: spi-mem: Add missing description for data.nbytes field" to the spi tree

[Mark Brown]

The patch

   spi: spi-mem: Add missing description for data.nbytes field

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 60489f085574157c343fc62a32f997fe7346a659 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@bootlin.com>
Date: Thu, 20 Sep 2018 09:31:10 +0200
Subject: [PATCH] spi: spi-mem: Add missing description for data.nbytes field

Add a description for spi_mem_op.data.nbytes to the kerneldoc header.

Fixes: c36ff266dc82 ("spi: Extend the core to ease integration of SPI memory controllers")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/spi/spi-mem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 62722fb7472d..0cfbb1ad8d96 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -79,6 +79,8 @@ enum spi_mem_data_dir {
  * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
  * @data.buswidth: number of IO lanes used to send/receive the data
  * @data.dir: direction of the transfer
+ * @data.nbytes: number of data bytes to send/receive. Can be zero if the
+ *		 operation does not involve transferring data
  * @data.buf.in: input buffer
  * @data.buf.out: output buffer
  */
-- 
2.19.0


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Applied "spi: spi-mem: Add missing description for data.nbytes field" to the spi tree

[Mark Brown]

The patch

   spi: spi-mem: Add missing description for data.nbytes field

has been applied to the spi tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 60489f085574157c343fc62a32f997fe7346a659 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@bootlin.com>
Date: Thu, 20 Sep 2018 09:31:10 +0200
Subject: [PATCH] spi: spi-mem: Add missing description for data.nbytes field

Add a description for spi_mem_op.data.nbytes to the kerneldoc header.

Fixes: c36ff266dc82 ("spi: Extend the core to ease integration of SPI memory controllers")
Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/spi/spi-mem.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
index 62722fb7472d..0cfbb1ad8d96 100644
--- a/include/linux/spi/spi-mem.h
+++ b/include/linux/spi/spi-mem.h
@@ -79,6 +79,8 @@ enum spi_mem_data_dir {
  * @dummy.buswidth: number of IO lanes used to transmit the dummy bytes
  * @data.buswidth: number of IO lanes used to send/receive the data
  * @data.dir: direction of the transfer
+ * @data.nbytes: number of data bytes to send/receive. Can be zero if the
+ *		 operation does not involve transferring data
  * @data.buf.in: input buffer
  * @data.buf.out: output buffer
  */
-- 
2.19.0

Re: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Geert Uytterhoeven]

Hi Boris,

On Thu, Sep 20, 2018 at 9:32 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Some combinations are simply not valid and should be rejected before
> the op is passed to the SPI controller driver.
>
> Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
> spi_mem_supports_op() to make sure the spi-mem operation is valid.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

This is now commit 380583227c0c7f52 ("spi: spi-mem: Add extra sanity checks
on the op param") in spi/for-next, and causes probing of the QSPI FLASH
on r8a7791/koelsch to fail with:

    m25p80 spi0.0: error -22 reading 9f
    m25p80: probe of spi0.0 failed with error -22

Reverting the commit revives the FLASH:

    m25p80 spi0.0: s25fl512s (65536 Kbytes)
    3 fixed-partitions partitions found on MTD device spi0.0
    Creating 3 MTD partitions on "spi0.0":
    0x000000000000-0x000000080000 : "loader"
    0x000000080000-0x000000600000 : "user"
    0x000000600000-0x000004000000 : "flash"

> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -12,6 +12,8 @@
>
>  #include "internals.h"
>
> +#define SPI_MEM_MAX_BUSWIDTH           4
> +
>  /**
>   * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
>   *                                       memory operation
> @@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct spi_mem *mem,
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>
> +static bool spi_mem_buswidth_is_valid(u8 buswidth)
> +{
> +       if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
> +               return false;
> +
> +       return true;
> +}
> +
> +static int spi_mem_check_op(const struct spi_mem_op *op)
> +{
> +       if (!op->cmd.buswidth)
> +               return -EINVAL;
> +
> +       if ((op->addr.nbytes && !op->addr.buswidth) ||
> +           (op->dummy.nbytes && !op->dummy.buswidth) ||
> +           (op->data.nbytes && !op->data.buswidth))
> +               return -EINVAL;
> +
> +       if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->addr.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->data.buswidth))

Adding debug code shows:

    op->cmd.buswidth = 1
    op->addr.buswidth = 0
    op->dummy.buswidth = 0
    op->data.buswidth = 1

so they're all valid, leading to a failure?!?
Looks like the logic is completely reversed, will send a patch...

> +               return -EINVAL;
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 3/3] spi: spi-mem: Add extra sanity checks on the op param

[Geert Uytterhoeven]

Hi Boris,

On Thu, Sep 20, 2018 at 9:32 AM Boris Brezillon
<boris.brezillon@bootlin.com> wrote:
> Some combinations are simply not valid and should be rejected before
> the op is passed to the SPI controller driver.
>
> Add an spi_mem_check_op() helper and use it in spi_mem_exec_op() and
> spi_mem_supports_op() to make sure the spi-mem operation is valid.
>
> Signed-off-by: Boris Brezillon <boris.brezillon@bootlin.com>

This is now commit 380583227c0c7f52 ("spi: spi-mem: Add extra sanity checks
on the op param") in spi/for-next, and causes probing of the QSPI FLASH
on r8a7791/koelsch to fail with:

    m25p80 spi0.0: error -22 reading 9f
    m25p80: probe of spi0.0 failed with error -22

Reverting the commit revives the FLASH:

    m25p80 spi0.0: s25fl512s (65536 Kbytes)
    3 fixed-partitions partitions found on MTD device spi0.0
    Creating 3 MTD partitions on "spi0.0":
    0x000000000000-0x000000080000 : "loader"
    0x000000080000-0x000000600000 : "user"
    0x000000600000-0x000004000000 : "flash"

> --- a/drivers/spi/spi-mem.c
> +++ b/drivers/spi/spi-mem.c
> @@ -12,6 +12,8 @@
>
>  #include "internals.h"
>
> +#define SPI_MEM_MAX_BUSWIDTH           4
> +
>  /**
>   * spi_controller_dma_map_mem_op_data() - DMA-map the buffer attached to a
>   *                                       memory operation
> @@ -149,6 +151,44 @@ static bool spi_mem_default_supports_op(struct spi_mem *mem,
>  }
>  EXPORT_SYMBOL_GPL(spi_mem_default_supports_op);
>
> +static bool spi_mem_buswidth_is_valid(u8 buswidth)
> +{
> +       if (hweight8(buswidth) > 1 || buswidth > SPI_MEM_MAX_BUSWIDTH)
> +               return false;
> +
> +       return true;
> +}
> +
> +static int spi_mem_check_op(const struct spi_mem_op *op)
> +{
> +       if (!op->cmd.buswidth)
> +               return -EINVAL;
> +
> +       if ((op->addr.nbytes && !op->addr.buswidth) ||
> +           (op->dummy.nbytes && !op->dummy.buswidth) ||
> +           (op->data.nbytes && !op->data.buswidth))
> +               return -EINVAL;
> +
> +       if (spi_mem_buswidth_is_valid(op->cmd.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->addr.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->dummy.buswidth) ||
> +           spi_mem_buswidth_is_valid(op->data.buswidth))

Adding debug code shows:

    op->cmd.buswidth = 1
    op->addr.buswidth = 0
    op->dummy.buswidth = 0
    op->data.buswidth = 1

so they're all valid, leading to a failure?!?
Looks like the logic is completely reversed, will send a patch...

> +               return -EINVAL;
> +
> +       return 0;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds