[PATCH] spi: spi-fsl-dspi: fix native data copy

[PATCH] spi: spi-fsl-dspi: fix native data copy

[Angelo Dureghello]

ColdFire is a big-endian cpu with a big-endian dspi hw module,
so, it uses native access, but memcpy breaks the endianness.

So, if i understand properly, by native copy we would mean
be(cpu)->be(dspi) or le(cpu)->le(dspi) accesses, so my fix
shouldn't break anything, but i couldn't test it on LS family,
so every test is really appreciated.

Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
---
 drivers/spi/spi-fsl-dspi.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
index 50e41f66a2d7..2e9f9adc5900 100644
--- a/drivers/spi/spi-fsl-dspi.c
+++ b/drivers/spi/spi-fsl-dspi.c
@@ -246,13 +246,33 @@ struct fsl_dspi {
 
 static void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata)
 {
-	memcpy(txdata, dspi->tx, dspi->oper_word_size);
+	switch (dspi->oper_word_size) {
+	case 1:
+		*txdata = *(u8 *)dspi->tx;
+		break;
+	case 2:
+		*txdata = *(u16 *)dspi->tx;
+		break;
+	case 4:
+		*txdata = *(u32 *)dspi->tx;
+		break;
+	}
 	dspi->tx += dspi->oper_word_size;
 }
 
 static void dspi_native_dev_to_host(struct fsl_dspi *dspi, u32 rxdata)
 {
-	memcpy(dspi->rx, &rxdata, dspi->oper_word_size);
+	switch (dspi->oper_word_size) {
+	case 1:
+		*(u8 *)dspi->rx = rxdata;
+		break;
+	case 2:
+		*(u16 *)dspi->rx = rxdata;
+		break;
+	case 4:
+		*(u32 *)dspi->rx = rxdata;
+		break;
+	}
 	dspi->rx += dspi->oper_word_size;
 }
 
-- 
2.26.2

Re: [PATCH] spi: spi-fsl-dspi: fix native data copy

[Vladimir Oltean]

Hi Angelo,

On Fri, 29 May 2020 at 22:53, Angelo Dureghello
<angelo.dureghello@timesys.com> wrote:
>
> ColdFire is a big-endian cpu with a big-endian dspi hw module,
> so, it uses native access, but memcpy breaks the endianness.
>
> So, if i understand properly, by native copy we would mean
> be(cpu)->be(dspi) or le(cpu)->le(dspi) accesses, so my fix
> shouldn't break anything, but i couldn't test it on LS family,
> so every test is really appreciated.
>
> Signed-off-by: Angelo Dureghello <angelo.dureghello@timesys.com>
> ---

Honestly I was expecting more breakage than that on Coldfire :)
I looked at this patch for a while before I figured out what's going on.

Let there be the program below:

#include <linux/types.h>
#include <string.h>
#include <stdio.h>

struct fsl_dspi {
    __u8 *tx;
    int oper_word_size;
};

static void dspi_native_host_to_dev_v1(struct fsl_dspi *dspi, __u32 *txdata)
{
    memcpy(txdata, dspi->tx, dspi->oper_word_size);
    dspi->tx += dspi->oper_word_size;
}

static void dspi_native_host_to_dev_v2(struct fsl_dspi *dspi, __u32 *txdata)
{
    switch (dspi->oper_word_size) {
    case 1:
        *txdata = *(__u8 *)dspi->tx;
        break;
    case 2:
        *txdata = *(__u16 *)dspi->tx;
        break;
    case 4:
        *txdata = *(__u32 *)dspi->tx;
        break;
    }
    dspi->tx += dspi->oper_word_size;
}

int main()
{
    struct fsl_dspi dspi;
    __u8 tx_buf[] = {
        0x00, 0x01, 0x02, 0x03,
        0x04, 0x05, 0x06, 0x07,
        0x08, 0x09, 0x0a, 0x0b,
    };
    __u32 txdata;

    dspi.tx = tx_buf;
    dspi.oper_word_size = 2;

    txdata = 0xdeadbeef;
    dspi_native_host_to_dev_v1(&dspi, &txdata);
    printf("txdata v1: 0x%08x\n", txdata);

    txdata = 0xdeadbeef;
    dspi_native_host_to_dev_v2(&dspi, &txdata);
    printf("txdata v2: 0x%08x\n", txdata);

    return 0;
}

On little endian, it prints:

txdata v1: 0xdead0100
txdata v2: 0x00000302

On big endian, it prints:

txdata v1: 0x0001beef
txdata v2: 0x00000203

So yeah, in your case, 0xbeef (uninitialized data) would get sent on
the wire. Your patch is correct.

Fixes: 53fadb4d90c7 ("spi: spi-fsl-dspi: Simplify bytes_per_word gymnastics")
Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Tested-by: Vladimir Oltean <vladimir.oltean@nxp.com>

>  drivers/spi/spi-fsl-dspi.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 50e41f66a2d7..2e9f9adc5900 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -246,13 +246,33 @@ struct fsl_dspi {
>
>  static void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata)
>  {
> -       memcpy(txdata, dspi->tx, dspi->oper_word_size);
> +       switch (dspi->oper_word_size) {
> +       case 1:
> +               *txdata = *(u8 *)dspi->tx;
> +               break;
> +       case 2:
> +               *txdata = *(u16 *)dspi->tx;
> +               break;
> +       case 4:
> +               *txdata = *(u32 *)dspi->tx;
> +               break;
> +       }
>         dspi->tx += dspi->oper_word_size;
>  }
>
>  static void dspi_native_dev_to_host(struct fsl_dspi *dspi, u32 rxdata)
>  {
> -       memcpy(dspi->rx, &rxdata, dspi->oper_word_size);
> +       switch (dspi->oper_word_size) {
> +       case 1:
> +               *(u8 *)dspi->rx = rxdata;
> +               break;
> +       case 2:
> +               *(u16 *)dspi->rx = rxdata;
> +               break;
> +       case 4:
> +               *(u32 *)dspi->rx = rxdata;
> +               break;
> +       }
>         dspi->rx += dspi->oper_word_size;
>  }
>
> --
> 2.26.2
>

Thanks,
-Vladimir

Re: [PATCH] spi: spi-fsl-dspi: fix native data copy

[Mark Brown]

On Fri, 29 May 2020 21:57:56 +0200, Angelo Dureghello wrote:
> ColdFire is a big-endian cpu with a big-endian dspi hw module,
> so, it uses native access, but memcpy breaks the endianness.
> 
> So, if i understand properly, by native copy we would mean
> be(cpu)->be(dspi) or le(cpu)->le(dspi) accesses, so my fix
> shouldn't break anything, but i couldn't test it on LS family,
> so every test is really appreciated.

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: spi-fsl-dspi: fix native data copy
      commit: 263b81dc6c932c8bc550d5e7bfc178d2b3fc491e

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark