ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement

[PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement

Posted by Eden Mikitas 5 years, 5 months ago

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

Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement

Posted by Peter Maydell 5 years, 5 months ago

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

Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement

Posted by Eden Mikitas 5 years, 5 months ago

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
>

Re: [PATCH] ssi/imx_spi: Removed unnecessary cast and fixed condition in while statement

Posted by Peter Maydell 5 years, 5 months ago

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

[PATCH 0/2] hw/ssi/imx_spi: 2 Fixes to flush txfifo function in imx_spi

Posted by Eden Mikitas 5 years, 5 months ago

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>

Re: [PATCH 0/2] hw/ssi/imx_spi: 2 Fixes to flush txfifo function in imx_spi

Posted by Peter Maydell 5 years, 5 months ago

On Fri, 22 May 2020 at 12:50, Eden Mikitas <e.mikitas@gmail.com> wrote:
>
>
> 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.



Applied to target-arm.next, thanks.

-- PMM

[PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow

Posted by Eden Mikitas 5 years, 5 months ago

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

Re: [PATCH 1/2] hw/ssi/imx_spi: changed while statement to prevent underflow

Posted by Alistair Francis 5 years, 5 months ago

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

[PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave

Posted by Eden Mikitas 5 years, 5 months ago

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

Re: [PATCH 2/2] hw/ssi/imx_spi: Removed unnecessary cast of rx data received from slave

Posted by Alistair Francis 5 years, 5 months ago

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