| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 6 Apr 2022 01:07:21 +0530
From: Pratyush Yadav <p.yadav@...com>
To: Matthias Schiffer <matthias.schiffer@...tq-group.com>
CC: Mark Brown <broonie@...nel.org>,
Tudor Ambarus <tudor.ambarus@...rochip.com>,
Vignesh Raghavendra <vigneshr@...com>,
Ramuthevar Vadivel Murugan
<vadivel.muruganx.ramuthevar@...ux.intel.com>,
<linux-spi@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] spi: cadence-quadspi: fix protocol setup for non-1-1-X
operations
On 01/04/22 12:20PM, Matthias Schiffer wrote:
> On Fri, 2022-04-01 at 15:36 +0530, Pratyush Yadav wrote:
> > Hi Matthias,
> >
> > On 31/03/22 01:08PM, Matthias Schiffer wrote:
> > > cqspi_set_protocol() only set the data width, but ignored the
> > > command
> > > and address width (except for 8-8-8 DTR ops), leading to corruption
> > > of
> > > all transfers using 1-X-X or X-X-X ops. Fix by setting the other
> > > two
> > > widths as well.
> > >
> > > While we're at it, simplify the code a bit by replacing the
> > > CQSPI_INST_TYPE_* constants with ilog2().
> > >
> > > Tested on a TI AM64x with a Macronix MX25U51245G QSPI flash with 1-
> > > 4-4
> > > read and write operations.
> > >
> > > Fixes: a314f6367787 ("mtd: spi-nor: Convert cadence-quadspi to use
> > > spi-mem framework")
> >
> > I think a fixes tag is wrong here. The old driver did not support 1-
> > X-X
> > modes either. So you are not fixing anything, you are adding a new
> > feature. I don't think we should backport this patch to stable.
>
>
> Giving a precise fixes tag is a bit difficult. The referenced commit
> made the driver (accidentally) accept commands like 1-4-4 without
> handing them correctly, causing data corruption for flashs that support
> these modes. The data corruption is fixed by my patch.
Ah, you're right. I missed the fact that the original driver explicitly
checked for 1-1-1, 1-1-4, and 1-1-8, and returned an error for the rest.
So your patch does indeed fix a bug.
>
> As the change was unintended, one option would be to split this patch
> into two parts: One fix patch that makes cqspi_set_protocol() -EINVAL
> again for all commands that are not 1-1-X, and one feature patch that
> adds actual support for these commands.
>
> My thought process was that making these commands work correctly can't
> break anything that is not already broken in current stable kernels.
> But if you prefer the minimal change, I can send a v2 that splits the
> patch.
Yes, but as I commented below, I would prefer you rework the driver to
drop cqspi_set_protocol() entirely. For doing that the 2 patch approach
would work best so we don't end up getting the new code backported to
stable as well.
>
> Regards,
> Matthias
>
>
>
> >
> > > Signed-off-by: Matthias Schiffer <matthias.schiffer@...tq-group.com
> > > >
> > > ---
> > > drivers/spi/spi-cadence-quadspi.c | 46 ++++++++-------------------
> > > ----
> > > 1 file changed, 12 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/drivers/spi/spi-cadence-quadspi.c b/drivers/spi/spi-
> > > cadence-quadspi.c
> > > index b0c9f62ccefb..616ada891974 100644
> > > --- a/drivers/spi/spi-cadence-quadspi.c
> > > +++ b/drivers/spi/spi-cadence-quadspi.c
> > > @@ -19,6 +19,7 @@
> > > #include <linux/iopoll.h>
> > > #include <linux/jiffies.h>
> > > #include <linux/kernel.h>
> > > +#include <linux/log2.h>
> > > #include <linux/module.h>
> > > #include <linux/of_device.h>
> > > #include <linux/of.h>
> > > @@ -102,12 +103,6 @@ struct cqspi_driver_platdata {
> > > #define CQSPI_TIMEOUT_MS 500
> > > #define CQSPI_READ_TIMEOUT_MS 10
> > >
> > > -/* Instruction type */
> > > -#define CQSPI_INST_TYPE_SINGLE 0
> > > -#define CQSPI_INST_TYPE_DUAL 1
> > > -#define CQSPI_INST_TYPE_QUAD 2
> > > -#define CQSPI_INST_TYPE_OCTAL 3
> > > -
> > > #define CQSPI_DUMMY_CLKS_PER_BYTE 8
> > > #define CQSPI_DUMMY_BYTES_MAX 4
> > > #define CQSPI_DUMMY_CLKS_MAX 31
> > > @@ -376,10 +371,6 @@ static unsigned int cqspi_calc_dummy(const
> > > struct spi_mem_op *op, bool dtr)
> > > static int cqspi_set_protocol(struct cqspi_flash_pdata *f_pdata,
> > > const struct spi_mem_op *op)
> > > {
> > > - f_pdata->inst_width = CQSPI_INST_TYPE_SINGLE;
> > > - f_pdata->addr_width = CQSPI_INST_TYPE_SINGLE;
> > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > > -
> > > /*
> > > * For an op to be DTR, cmd phase along with every other non-
> > > empty
> > > * phase should have dtr field set to 1. If an op phase has
> > > zero
> > > @@ -389,32 +380,23 @@ static int cqspi_set_protocol(struct
> > > cqspi_flash_pdata *f_pdata,
> > > (!op->addr.nbytes || op->addr.dtr) &&
> > > (!op->data.nbytes || op->data.dtr);
> > >
> > > - switch (op->data.buswidth) {
> > > - case 0:
> > > - break;
> > > - case 1:
> > > - f_pdata->data_width = CQSPI_INST_TYPE_SINGLE;
> > > - break;
> > > - case 2:
> > > - f_pdata->data_width = CQSPI_INST_TYPE_DUAL;
> > > - break;
> > > - case 4:
> > > - f_pdata->data_width = CQSPI_INST_TYPE_QUAD;
> > > - break;
> > > - case 8:
> > > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> > > - break;
> > > - default:
> > > - return -EINVAL;
> > > - }
> > > + f_pdata->inst_width = 0;
> > > + if (op->cmd.buswidth)
> > > + f_pdata->inst_width = ilog2(op->cmd.buswidth);
> > > +
> > > + f_pdata->addr_width = 0;
> > > + if (op->addr.buswidth)
> > > + f_pdata->addr_width = ilog2(op->addr.buswidth);
> > > +
> > > + f_pdata->data_width = 0;
> > > + if (op->data.buswidth)
> > > + f_pdata->data_width = ilog2(op->data.buswidth);
> >
> > Honestly, I think we should get rid of cqspi_set_protocol() entirely.
> > I
> > see no need to store f_pdata->{instr,addr,data}_width since we
> > recalculate those for each op execution anyway. So why not just use
> > the
> > spi_mem_op to get those values directly and be rid of all this mess?
> >
> > >
> > > /* Right now we only support 8-8-8 DTR mode. */
> > > if (f_pdata->dtr) {
> > > switch (op->cmd.buswidth) {
> > > case 0:
> > > - break;
> > > case 8:
> > > - f_pdata->inst_width = CQSPI_INST_TYPE_OCTAL;
> > > break;
> > > default:
> > > return -EINVAL;
> > > @@ -422,9 +404,7 @@ static int cqspi_set_protocol(struct
> > > cqspi_flash_pdata *f_pdata,
> > >
> > > switch (op->addr.buswidth) {
> > > case 0:
> > > - break;
> > > case 8:
> > > - f_pdata->addr_width = CQSPI_INST_TYPE_OCTAL;
> > > break;
> > > default:
> > > return -EINVAL;
> > > @@ -432,9 +412,7 @@ static int cqspi_set_protocol(struct
> > > cqspi_flash_pdata *f_pdata,
> > >
> > > switch (op->data.buswidth) {
> > > case 0:
> > > - break;
> > > case 8:
> > > - f_pdata->data_width = CQSPI_INST_TYPE_OCTAL;
> > > break;
> > > default:
> > > return -EINVAL;
> > > --
> > > 2.25.1
> > >
>
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
Powered by blists - more mailing lists