| Message ID | 20200529195756.184677-1-angelo.dureghello@timesys.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 263b81dc6c932c8bc550d5e7bfc178d2b3fc491e |
| Headers | show |
| Series | spi: spi-fsl-dspi: fix native data copy | expand |
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
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
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; }
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(-)