diff mbox series

[4/6] spi: spi-mem: reject partial cycle transfers in 8D-8D-8D mode

Message ID 20210506191829.8271-5-p.yadav@ti.com
State Superseded
Headers show
Series Avoid odd length/address read/writes in 8D-8D-8D mode. | expand

Commit Message

Pratyush Yadav May 6, 2021, 7:18 p.m. UTC
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(-)

Comments

Mark Brown May 7, 2021, 12:55 p.m. UTC | #1
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.
Pratyush Yadav May 7, 2021, 1:56 p.m. UTC | #2
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.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.
Mark Brown May 7, 2021, 3:31 p.m. UTC | #3
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.
Mark Brown May 7, 2021, 3:48 p.m. UTC | #4
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>
Pratyush Yadav May 7, 2021, 4:49 p.m. UTC | #5
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/.

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.
diff mbox series

Patch

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);