| Message ID | 20210506191829.8271-5-p.yadav@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Avoid odd length/address read/writes in 8D-8D-8D mode. | expand |
On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote: > In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number > of bytes cannot be transferred because it would leave a residual half > cycle at the end. Consider such a transfer invalid and reject it. > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > --- > This patch should go through the SPI tree but I have included it in this > series because if it goes in before patches 1-3, Micron MT35XU and > Cypress S28HS flashes will stop working correctly. It seems like this should probably even go in as a fix even if nothing is broken with mainline right now, it's the sort of thing some out of tree patch could easily trigger. Unless anyone objects I'll do that.
On 07/05/21 01:55PM, Mark Brown wrote: > On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote: > > In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number > > of bytes cannot be transferred because it would leave a residual half > > cycle at the end. Consider such a transfer invalid and reject it. > > > > Signed-off-by: Pratyush Yadav <p.yadav@ti.com> > > > > --- > > This patch should go through the SPI tree but I have included it in this > > series because if it goes in before patches 1-3, Micron MT35XU and > > Cypress S28HS flashes will stop working correctly. > > It seems like this should probably even go in as a fix even if nothing > is broken with mainline right now, it's the sort of thing some out of > tree patch could easily trigger. Unless anyone objects I'll do that. If it does get backported to stable branches, patches 1-3 would have to go in _before_ this one. Otherwise it will break the two 8D-8D-8D flashes: Micron MT35XU512ABA and Cypress S28HS512T. Without patch 1 8D-8D-8D mode will not be selected correctly on these two flashes. The supports_op() will reject it because of 1 data byte. This is a relatively safe patch for backporting to a stable branch. Patches 2 and 3 are a slightly different matter. They add an extra register write. But most controllers I've come across don't support 1-byte writes in 8D mode. It is likely that they are sending bogus/undefined values in the second byte and deasserting CS only after the cycle is done. So they should _in theory_ change undefined behaviour to defined behaviour. Still, they introduce an extra register write. I'm not sure how risk-tolerant you want to be for stable backports. I will leave the judgement to you or Tudor or Vignesh.
On Fri, May 07, 2021 at 07:26:33PM +0530, Pratyush Yadav wrote: > Patches 2 and 3 are a slightly different matter. They add an extra > register write. But most controllers I've come across don't support > 1-byte writes in 8D mode. It is likely that they are sending > bogus/undefined values in the second byte and deasserting CS only after > the cycle is done. So they should _in theory_ change undefined behaviour > to defined behaviour. > Still, they introduce an extra register write. I'm not sure how > risk-tolerant you want to be for stable backports. I will leave the > judgement to you or Tudor or Vignesh. Ah, given that if nobody's seeing any issues I'd probably just hold off there TBH.
On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote: > In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number > of bytes cannot be transferred because it would leave a residual half > cycle at the end. Consider such a transfer invalid and reject it. Reviwed-by: Mark Brown <broonie@kernel.org>
On 07/05/21 04:48PM, Mark Brown wrote: > On Fri, May 07, 2021 at 12:48:27AM +0530, Pratyush Yadav wrote: > > In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number > > of bytes cannot be transferred because it would leave a residual half > > cycle at the end. Consider such a transfer invalid and reject it. > > Reviwed-by: Mark Brown <broonie@kernel.org> Thanks. BTW, s/Reviwed/Reviewed/.
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 1513553e4080..ab9eefbaa1d9 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -162,7 +162,17 @@ static bool spi_mem_check_buswidth(struct spi_mem *mem, bool spi_mem_dtr_supports_op(struct spi_mem *mem, const struct spi_mem_op *op) { - if (op->cmd.nbytes != 2) + if (op->cmd.buswidth == 8 && op->cmd.nbytes % 2) + return false; + + if (op->addr.nbytes && op->addr.buswidth == 8 && op->addr.nbytes % 2) + return false; + + if (op->dummy.nbytes && op->dummy.buswidth == 8 && op->dummy.nbytes % 2) + return false; + + if (op->data.dir != SPI_MEM_NO_DATA && + op->dummy.buswidth == 8 && op->data.nbytes % 2) return false; return spi_mem_check_buswidth(mem, op);
In 8D-8D-8D mode two bytes are transferred per cycle. So an odd number of bytes cannot be transferred because it would leave a residual half cycle at the end. Consider such a transfer invalid and reject it. Signed-off-by: Pratyush Yadav <p.yadav@ti.com> --- This patch should go through the SPI tree but I have included it in this series because if it goes in before patches 1-3, Micron MT35XU and Cypress S28HS flashes will stop working correctly. drivers/spi/spi-mem.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)