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

[Vladimir Oltean <olteanv@xxxxxxxxx>]

Hi Angelo,

On Fri, 29 May 2020 at 22:53, Angelo Dureghello
<angelo.dureghello@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---

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@xxxxxxx>
Tested-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>

>  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