| Message ID | 20180920073112.7897-4-boris.brezillon@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 380583227c0c7f52383b0cd5c0e2de93ed31d553 |
| Headers | show |
| Series | spi: spi-mem: Minor fixes/improvements | expand |
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&data=02%7C01%7Cyogeshnarayan.gaur%40nxp.com%7C3593fc7 > 7e9d44b5b7a6e08d61ecb6c1c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0% > 7C0%7C636730256384380635&sdata=lTp45z8K1WFhI0LA7Nxca2p8pdHsQL > gXNFC1GJIi1NM%3D&reserved=0
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
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
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) {
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(-)