| 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, 06 Apr 2022 10:54:37 +0200
From: Matthias Schiffer <matthias.schiffer@...tq-group.com>
To: Pratyush Yadav <p.yadav@...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 Wed, 2022-04-06 at 01:07 +0530, Pratyush Yadav wrote:
> 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.
Okay, I tried to split my patch, however I found
that cqspi_set_protocol() is not the right place to check for
unsupported operations at all - if the driver ever reaches a path where
cqspi_set_protocol() returns -EINVAL, something has already gone wrong
(and the attemption operation will fail altogether rather than falling
back to a slower command).
I think that these checks - including the check that only 8-8-8 is
supported with DTR - should actually happen in cqspi_supports_mem_op().
Does that sound right?
Regards,
Matthias
>
> > 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
> > > >
Powered by blists - more mailing lists