QEMU-Devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement
@ 2020-05-20 14:32 Eden Mikitas
  2020-05-21 16:41 ` Peter Maydell
  0 siblings, 1 reply; 9+ messages in thread
From: Eden Mikitas @ 2020-05-20 14:32 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, qemu-trivial, Alistair Francis,
	Jean-Christophe Dubois, open list:i.MX31 kzm, Peter Chubb,
	Eden Mikitas

When inserting the value retrieved (rx) from the spi slave, rx is pushed to
rx_fifo after being cast to uint8_t. rx_fifo is a fifo32, and the rx
register the driver uses is also 32 bit. This zeroes the 24 most
significant bits of rx. This proved problematic with devices that expect to
use the whole 32 bits of the rx register.
I tested this change by running `make check` and by booting linux on
sabrelite (which uses an spi flash device).

Signed-off-by: Eden Mikitas <e.mikitas@gmail.com>
---
 hw/ssi/imx_spi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 2dd9a631e1..43b2f14dd2 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -182,7 +182,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
 
         rx = 0;
 
-        while (tx_burst) {
+        while (tx_burst > 0) {
             uint8_t byte = tx & 0xff;
 
             DPRINTF("writing 0x%02x\n", (uint32_t)byte);
@@ -206,7 +206,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
         if (fifo32_is_full(&s->rx_fifo)) {
             s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
         } else {
-            fifo32_push(&s->rx_fifo, (uint8_t)rx);
+            fifo32_push(&s->rx_fifo, rx);
         }
 
         if (s->burst_length <= 0) {
-- 
2.17.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement
  2020-05-20 14:32 [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement Eden Mikitas
@ 2020-05-21 16:41 ` Peter Maydell
  2020-05-21 18:49   ` Eden Mikitas
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2020-05-21 16:41 UTC (permalink / raw)
  To: Eden Mikitas
  Cc: QEMU Trivial, Alistair Francis, QEMU Developers,
	Jean-Christophe Dubois, open list:i.MX31 (kzm),
	Peter Chubb

On Wed, 20 May 2020 at 15:33, Eden Mikitas <e.mikitas@gmail.com> wrote:
> When inserting the value retrieved (rx) from the spi slave, rx is pushed to
> rx_fifo after being cast to uint8_t. rx_fifo is a fifo32, and the rx
> register the driver uses is also 32 bit. This zeroes the 24 most
> significant bits of rx. This proved problematic with devices that expect to
> use the whole 32 bits of the rx register.
> I tested this change by running `make check` and by booting linux on
> sabrelite (which uses an spi flash device).
>
> Signed-off-by: Eden Mikitas <e.mikitas@gmail.com>
> ---
>  hw/ssi/imx_spi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 2dd9a631e1..43b2f14dd2 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -182,7 +182,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>
>          rx = 0;
>
> -        while (tx_burst) {
> +        while (tx_burst > 0) {
>              uint8_t byte = tx & 0xff;

When does this make a difference? Looking at the code
I don't think tx_burst can ever be negative at this
point, in which case the two conditions are equivalent.
What was the motivation for this change?

>              DPRINTF("writing 0x%02x\n", (uint32_t)byte);
> @@ -206,7 +206,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>          if (fifo32_is_full(&s->rx_fifo)) {
>              s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
>          } else {
> -            fifo32_push(&s->rx_fifo, (uint8_t)rx);
> +            fifo32_push(&s->rx_fifo, rx);
>          }
>
>          if (s->burst_length <= 0) {

This seems like a separate change to the first one;
in general unrelated changes should be each in their
own patch, rather than combined into a single patch.

The fifo32_push() part of this change looks correct to me.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement
  2020-05-21 16:41 ` Peter Maydell
@ 2020-05-21 18:49   ` Eden Mikitas
  2020-05-21 18:56     ` Peter Maydell
  0 siblings, 1 reply; 9+ messages in thread
From: Eden Mikitas @ 2020-05-21 18:49 UTC (permalink / raw)
  To: peter.maydell
  Cc: qemu-trivial, alistair, qemu-devel, jcd, qemu-arm, peter.chubb,
	e.mikitas

The ecspi controller is supposed to support burst length of 1 to 4096 bits,
meaning it is possible to configure it to use a value that isn't a multiple
of 8 (to my understanding). In that case, since tx_burst is always
decremented by 8, it will underflow. Sorry I didn't include an explanation.
Should I resubmit the patch?

>
> >              DPRINTF("writing 0x%02x\n", (uint32_t)byte);
> > @@ -206,7 +206,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
> >          if (fifo32_is_full(&s->rx_fifo)) {
> >              s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
> >          } else {
> > -            fifo32_push(&s->rx_fifo, (uint8_t)rx);
> > +            fifo32_push(&s->rx_fifo, rx);
> >          }
> >
> >          if (s->burst_length <= 0) {
>
> This seems like a separate change to the first one;
> in general unrelated changes should be each in their
> own patch, rather than combined into a single patch.

Should I resubmit this as a patch?

>
> The fifo32_push() part of this change looks correct to me.
>
> thanks
> -- PMM
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement
  2020-05-21 18:49   ` Eden Mikitas
@ 2020-05-21 18:56     ` Peter Maydell
  2020-05-22 11:50       ` [PATCH 0/2] hw/ssi/imx_spi: 2 Fixes to flush txfifo function in imx_spi Eden Mikitas
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Maydell @ 2020-05-21 18:56 UTC (permalink / raw)
  To: Eden Mikitas
  Cc: QEMU Trivial, Alistair Francis, QEMU Developers,
	Jean-Christophe DUBOIS, qemu-arm, Peter Chubb

On Thu, 21 May 2020 at 19:49, Eden Mikitas <e.mikitas@gmail.com> wrote:
>
> The ecspi controller is supposed to support burst length of 1 to 4096 bits,
> meaning it is possible to configure it to use a value that isn't a multiple
> of 8 (to my understanding). In that case, since tx_burst is always
> decremented by 8, it will underflow. Sorry I didn't include an explanation.

Ah, yes, I misread the code. On the first time into the loop
tx_burst must be positive, but this is a while() condition
and on subsequent loops we might end up negative. So all
the code changes in this patch are correct, they just need
to be split into two commits I think.

> > This seems like a separate change to the first one;
> > in general unrelated changes should be each in their
> > own patch, rather than combined into a single patch.
>
> Should I resubmit this as a patch?

Yes, if you could submit a 2-patch patch series where
one patch is the fix for handling burst lengths which aren't
multiples of 8, and the other is the fix for saving all the
data into the rx fifo rather than just the low byte, that
would be great.

thanks
-- PMM


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 0/2] hw/ssi/imx_spi: 2 Fixes to flush txfifo function in imx_spi
  2020-05-21 18:56     ` Peter Maydell
@ 2020-05-22 11:50       ` Eden Mikitas
  2020-05-22 11:50         ` [PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow Eden Mikitas
  2020-05-22 11:50         ` [PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave Eden Mikitas
  0 siblings, 2 replies; 9+ messages in thread
From: Eden Mikitas @ 2020-05-22 11:50 UTC (permalink / raw)
  To: peter.maydell
  Cc: qemu-trivial, alistair, qemu-devel, jcd, qemu-arm, peter.chubb,
	e.mikitas


This patch series contains 2 fixes to the imx_spi_flush_txfifo function.

The first fix prevents a case in which calling imx_spi_flush_txfifo while the 
controller is configured (by the guest driver) to use a burst length that isn't 
a multiple of 8 will cause an infinite loop.

The second fix makes the spi controller compatible with slaves + guest drivers
that expect to make transaction larger than 8 bits by removing an unnecessary 
cast.

This patch series was tested by running `make check` and by booting linux on
a sabrelite machine (which uses an spi flash)

Signed-off-by: Eden Mikitas <e.mikitas@gmail.com>



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow
  2020-05-22 11:50       ` [PATCH 0/2] hw/ssi/imx_spi: 2 Fixes to flush txfifo function in imx_spi Eden Mikitas
@ 2020-05-22 11:50         ` Eden Mikitas
  2020-05-22 15:34           ` Alistair Francis
  2020-05-22 11:50         ` [PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave Eden Mikitas
  1 sibling, 1 reply; 9+ messages in thread
From: Eden Mikitas @ 2020-05-22 11:50 UTC (permalink / raw)
  To: peter.maydell
  Cc: qemu-trivial, alistair, qemu-devel, jcd, qemu-arm, peter.chubb,
	e.mikitas

The while statement in question only checked if tx_burst is not 0.
tx_burst is a signed int, which is assigned the value put by the
guest driver in ECSPI_CONREG. The burst length can be anywhere
between 1 and 4096, and since tx_burst is always decremented by 8
it could possibly underflow, causing an infinite loop.

Signed-off-by: Eden Mikitas <e.mikitas@gmail.com>
---
 hw/ssi/imx_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 2dd9a631e1..6fef5c7958 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -182,7 +182,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
 
         rx = 0;
 
-        while (tx_burst) {
+        while (tx_burst > 0) {
             uint8_t byte = tx & 0xff;
 
             DPRINTF("writing 0x%02x\n", (uint32_t)byte);
-- 
2.17.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave
  2020-05-22 11:50       ` [PATCH 0/2] hw/ssi/imx_spi: 2 Fixes to flush txfifo function in imx_spi Eden Mikitas
  2020-05-22 11:50         ` [PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow Eden Mikitas
@ 2020-05-22 11:50         ` Eden Mikitas
  2020-05-22 15:36           ` Alistair Francis
  1 sibling, 1 reply; 9+ messages in thread
From: Eden Mikitas @ 2020-05-22 11:50 UTC (permalink / raw)
  To: peter.maydell
  Cc: qemu-trivial, alistair, qemu-devel, jcd, qemu-arm, peter.chubb,
	e.mikitas

When inserting the value retrieved (rx) from the spi slave, rx is pushed to
rx_fifo after being cast to uint8_t. rx_fifo is a fifo32, and the rx
register the driver uses is also 32 bit. This zeroes the 24 most
significant bits of rx. This proved problematic with devices that expect to
use the whole 32 bits of the rx register.

Signed-off-by: Eden Mikitas <e.mikitas@gmail.com>
---
 hw/ssi/imx_spi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
index 6fef5c7958..43b2f14dd2 100644
--- a/hw/ssi/imx_spi.c
+++ b/hw/ssi/imx_spi.c
@@ -206,7 +206,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
         if (fifo32_is_full(&s->rx_fifo)) {
             s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
         } else {
-            fifo32_push(&s->rx_fifo, (uint8_t)rx);
+            fifo32_push(&s->rx_fifo, rx);
         }
 
         if (s->burst_length <= 0) {
-- 
2.17.1



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow
  2020-05-22 11:50         ` [PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow Eden Mikitas
@ 2020-05-22 15:34           ` Alistair Francis
  0 siblings, 0 replies; 9+ messages in thread
From: Alistair Francis @ 2020-05-22 15:34 UTC (permalink / raw)
  To: Eden Mikitas
  Cc: Peter Maydell, QEMU Trivial, Alistair Francis,
	qemu-devel@nongnu.org Developers, Jean-Christophe Dubois,
	qemu-arm, Peter Chubb

On Fri, May 22, 2020 at 4:51 AM Eden Mikitas <e.mikitas@gmail.com> wrote:
>
> The while statement in question only checked if tx_burst is not 0.
> tx_burst is a signed int, which is assigned the value put by the
> guest driver in ECSPI_CONREG. The burst length can be anywhere
> between 1 and 4096, and since tx_burst is always decremented by 8
> it could possibly underflow, causing an infinite loop.
>
> Signed-off-by: Eden Mikitas <e.mikitas@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/ssi/imx_spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 2dd9a631e1..6fef5c7958 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -182,7 +182,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>
>          rx = 0;
>
> -        while (tx_burst) {
> +        while (tx_burst > 0) {
>              uint8_t byte = tx & 0xff;
>
>              DPRINTF("writing 0x%02x\n", (uint32_t)byte);
> --
> 2.17.1
>
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave
  2020-05-22 11:50         ` [PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave Eden Mikitas
@ 2020-05-22 15:36           ` Alistair Francis
  0 siblings, 0 replies; 9+ messages in thread
From: Alistair Francis @ 2020-05-22 15:36 UTC (permalink / raw)
  To: Eden Mikitas
  Cc: Peter Maydell, QEMU Trivial, Alistair Francis,
	qemu-devel@nongnu.org Developers, Jean-Christophe Dubois,
	qemu-arm, Peter Chubb

On Fri, May 22, 2020 at 4:52 AM Eden Mikitas <e.mikitas@gmail.com> wrote:
>
> When inserting the value retrieved (rx) from the spi slave, rx is pushed to
> rx_fifo after being cast to uint8_t. rx_fifo is a fifo32, and the rx
> register the driver uses is also 32 bit. This zeroes the 24 most
> significant bits of rx. This proved problematic with devices that expect to
> use the whole 32 bits of the rx register.
>
> Signed-off-by: Eden Mikitas <e.mikitas@gmail.com>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  hw/ssi/imx_spi.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/ssi/imx_spi.c b/hw/ssi/imx_spi.c
> index 6fef5c7958..43b2f14dd2 100644
> --- a/hw/ssi/imx_spi.c
> +++ b/hw/ssi/imx_spi.c
> @@ -206,7 +206,7 @@ static void imx_spi_flush_txfifo(IMXSPIState *s)
>          if (fifo32_is_full(&s->rx_fifo)) {
>              s->regs[ECSPI_STATREG] |= ECSPI_STATREG_RO;
>          } else {
> -            fifo32_push(&s->rx_fifo, (uint8_t)rx);
> +            fifo32_push(&s->rx_fifo, rx);
>          }
>
>          if (s->burst_length <= 0) {
> --
> 2.17.1
>
>


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-20 14:32 [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement Eden Mikitas
2020-05-21 16:41 ` Peter Maydell
2020-05-21 18:49   ` Eden Mikitas
2020-05-21 18:56     ` Peter Maydell
2020-05-22 11:50       ` [PATCH 0/2] hw/ssi/imx_spi: 2 Fixes to flush txfifo function in imx_spi Eden Mikitas
2020-05-22 11:50         ` [PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow Eden Mikitas
2020-05-22 15:34           ` Alistair Francis
2020-05-22 11:50         ` [PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave Eden Mikitas
2020-05-22 15:36           ` Alistair Francis

QEMU-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/qemu-devel/0 qemu-devel/git/0.git
	git clone --mirror https://lore.kernel.org/qemu-devel/1 qemu-devel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 qemu-devel qemu-devel/ https://lore.kernel.org/qemu-devel \
		qemu-devel@nongnu.org
	public-inbox-index qemu-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.nongnu.qemu-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git