[PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

tx_dma/rx_dma are already set to a dummy buffer when no
tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
address, but NULL.
This may lead to corruption of kernel memory. Fix this by
leaving tx_dma/rx_dma alone.

Do not INIT_TX_RX while controller is enabled, this is bad according
to the MPC8321 manual.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
---
 drivers/spi/spi_mpc8xxx.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index e1d8045..c312d0e 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -240,7 +240,6 @@ static void mpc8xxx_spi_change_mode(struct spi_device *spi)
 
 	/* Turn off SPI unit prior changing mode */
 	mpc8xxx_spi_write_reg(mode, cs->hw_mode & ~SPMODE_ENABLE);
-	mpc8xxx_spi_write_reg(mode, cs->hw_mode);
 
 	/* When in CPM mode, we need to reinit tx and rx. */
 	if (mspi->flags & SPI_CPM_MODE) {
@@ -257,7 +256,7 @@ static void mpc8xxx_spi_change_mode(struct spi_device *spi)
 			}
 		}
 	}
-
+	mpc8xxx_spi_write_reg(mode, cs->hw_mode);
 	local_irq_restore(flags);
 }
 
@@ -437,7 +436,7 @@ static int mpc8xxx_spi_cpm_bufs(struct mpc8xxx_spi *mspi,
 			dev_err(dev, "unable to map tx dma\n");
 			return -ENOMEM;
 		}
-	} else {
+	} else if (t->tx_buf) {
 		mspi->tx_dma = t->tx_dma;
 	}
 
@@ -448,7 +447,7 @@ static int mpc8xxx_spi_cpm_bufs(struct mpc8xxx_spi *mspi,
 			dev_err(dev, "unable to map rx dma\n");
 			goto err_rx_dma;
 		}
-	} else {
+	} else if (t->rx_buf) {
 		mspi->rx_dma = t->rx_dma;
 	}
 
-- 
1.6.4.4


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Anton Vorontsov]

On Wed, May 12, 2010 at 05:50:50PM +0200, Joakim Tjernlund wrote:
> tx_dma/rx_dma are already set to a dummy buffer when no
> tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
> address, but NULL.
> This may lead to corruption of kernel memory. Fix this by
> leaving tx_dma/rx_dma alone.
> 
> Do not INIT_TX_RX while controller is enabled, this is bad according
> to the MPC8321 manual.
> 
> Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>

Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Thanks!

> ---
>  drivers/spi/spi_mpc8xxx.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
> index e1d8045..c312d0e 100644
> --- a/drivers/spi/spi_mpc8xxx.c
> +++ b/drivers/spi/spi_mpc8xxx.c
> @@ -240,7 +240,6 @@ static void mpc8xxx_spi_change_mode(struct spi_device *spi)
>  
>  	/* Turn off SPI unit prior changing mode */
>  	mpc8xxx_spi_write_reg(mode, cs->hw_mode & ~SPMODE_ENABLE);
> -	mpc8xxx_spi_write_reg(mode, cs->hw_mode);
>  
>  	/* When in CPM mode, we need to reinit tx and rx. */
>  	if (mspi->flags & SPI_CPM_MODE) {
> @@ -257,7 +256,7 @@ static void mpc8xxx_spi_change_mode(struct spi_device *spi)
>  			}
>  		}
>  	}
> -
> +	mpc8xxx_spi_write_reg(mode, cs->hw_mode);
>  	local_irq_restore(flags);
>  }
>  
> @@ -437,7 +436,7 @@ static int mpc8xxx_spi_cpm_bufs(struct mpc8xxx_spi *mspi,
>  			dev_err(dev, "unable to map tx dma\n");
>  			return -ENOMEM;
>  		}
> -	} else {
> +	} else if (t->tx_buf) {
>  		mspi->tx_dma = t->tx_dma;
>  	}
>  
> @@ -448,7 +447,7 @@ static int mpc8xxx_spi_cpm_bufs(struct mpc8xxx_spi *mspi,
>  			dev_err(dev, "unable to map rx dma\n");
>  			goto err_rx_dma;
>  		}
> -	} else {
> +	} else if (t->rx_buf) {
>  		mspi->rx_dma = t->rx_dma;
>  	}
>  
> -- 
> 1.6.4.4

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:22:31:
>
> On Wed, May 12, 2010 at 05:50:50PM +0200, Joakim Tjernlund wrote:
> > tx_dma/rx_dma are already set to a dummy buffer when no
> > tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
> > address, but NULL.
> > This may lead to corruption of kernel memory. Fix this by
> > leaving tx_dma/rx_dma alone.
> >
> > Do not INIT_TX_RX while controller is enabled, this is bad according
> > to the MPC8321 manual.
> >
> > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
>
> Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> Thanks!
:)

Who will make sure this goes to Linus?

 Jocke

BTW, I am having another problem too. An ADC doesn't work when in QE/CPM mode(CPU mode is fine)
The only thing I can think of ATM is if word size is handled differently? We
are using 16 bits words.


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Anton Vorontsov]

On Wed, May 12, 2010 at 06:28:51PM +0200, Joakim Tjernlund wrote:
> Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:22:31:
> >
> > On Wed, May 12, 2010 at 05:50:50PM +0200, Joakim Tjernlund wrote:
> > > tx_dma/rx_dma are already set to a dummy buffer when no
> > > tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
> > > address, but NULL.
> > > This may lead to corruption of kernel memory. Fix this by
> > > leaving tx_dma/rx_dma alone.
> > >
> > > Do not INIT_TX_RX while controller is enabled, this is bad according
> > > to the MPC8321 manual.
> > >
> > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> >
> > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >
> > Thanks!
> :)
> 
> Who will make sure this goes to Linus?

I guess it's Grant. Cc'ed.

>  Jocke
> 
> BTW, I am having another problem too. An ADC doesn't work when in QE/CPM mode(CPU mode is fine)
> The only thing I can think of ATM is if word size is handled differently? We
> are using 16 bits words.

No idea. Have you tried loopback mode? Create a dummy spidev
device, and try Documentation/spi/spidev_test.c's --loop switch.

Personally I tested QE/CPM mode with M25Pxx flashes. IIRC, I
also tested it with the spi_mmc driver on MPC8323E-RDB board.
Both drivers work with 8 bits words.

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:43:21:
> On Wed, May 12, 2010 at 06:28:51PM +0200, Joakim Tjernlund wrote:
> > Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:22:31:
> > >
> > > On Wed, May 12, 2010 at 05:50:50PM +0200, Joakim Tjernlund wrote:
> > > > tx_dma/rx_dma are already set to a dummy buffer when no
> > > > tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
> > > > address, but NULL.
> > > > This may lead to corruption of kernel memory. Fix this by
> > > > leaving tx_dma/rx_dma alone.
> > > >
> > > > Do not INIT_TX_RX while controller is enabled, this is bad according
> > > > to the MPC8321 manual.
> > > >
> > > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> > >
> > > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >
> > > Thanks!
> > :)
> >
> > Who will make sure this goes to Linus?
>
> I guess it's Grant. Cc'ed.
>
> >  Jocke
> >
> > BTW, I am having another problem too. An ADC doesn't work when in QE/CPM
> mode(CPU mode is fine)
> > The only thing I can think of ATM is if word size is handled differently? We
> > are using 16 bits words.
>
> No idea. Have you tried loopback mode? Create a dummy spidev
> device, and try Documentation/spi/spidev_test.c's --loop switch.
>
> Personally I tested QE/CPM mode with M25Pxx flashes. IIRC, I
> also tested it with the spi_mmc driver on MPC8323E-RDB board.
> Both drivers work with 8 bits words.

hmm, looks like wordsize == 16 is broken in the new CPM & QE mode:

root@localhost:~# ./spidev -D //dev/spidev0.3  -b 16
spi mode: 0
bits per word: 16
max speed: 500000 Hz (500 KHz)

9D 01 00 E0 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00
root@localhost:~# ./spidev -D //dev/spidev0.3
spi mode: 0
bits per word: 8
max speed: 500000 Hz (500 KHz)

01 9D C0 00 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00 00 00 00 00
00 00

Notice how the bytes are swapped. This does not happen in the old CPU mode.


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:43:21:
>
> On Wed, May 12, 2010 at 06:28:51PM +0200, Joakim Tjernlund wrote:
> > Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:22:31:
> > >
> > > On Wed, May 12, 2010 at 05:50:50PM +0200, Joakim Tjernlund wrote:
> > > > tx_dma/rx_dma are already set to a dummy buffer when no
> > > > tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
> > > > address, but NULL.
> > > > This may lead to corruption of kernel memory. Fix this by
> > > > leaving tx_dma/rx_dma alone.
> > > >
> > > > Do not INIT_TX_RX while controller is enabled, this is bad according
> > > > to the MPC8321 manual.
> > > >
> > > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> > >
> > > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> > >
> > > Thanks!
> > :)
> >
> > Who will make sure this goes to Linus?
>
> I guess it's Grant. Cc'ed.
>
> >  Jocke
> >
> > BTW, I am having another problem too. An ADC doesn't work when in QE/CPM
> mode(CPU mode is fine)
> > The only thing I can think of ATM is if word size is handled differently? We
> > are using 16 bits words.
>
> No idea. Have you tried loopback mode? Create a dummy spidev
> device, and try Documentation/spi/spidev_test.c's --loop switch.
>
> Personally I tested QE/CPM mode with M25Pxx flashes. IIRC, I
> also tested it with the spi_mmc driver on MPC8323E-RDB board.
> Both drivers work with 8 bits words.

The new QE mode is so broken :(
This is spidev run with the original CPU mode somewhat shorten:

root@localhost:~# ./spidev -D /dev/spidev0.3 -b 8
spi mode: 0
bits per word: 8
max speed: 500000 Hz (500 KHz)

01 9D C0 00 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 16
spi mode: 0
bits per word: 16
max speed: 500000 Hz (500 KHz)

01 9D E0 00 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 32
spi mode: 0
bits per word: 32
max speed: 500000 Hz (500 KHz)

01 9D C0 00 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 8 -L
spi mode: 8
bits per word: 8
max speed: 500000 Hz (500 KHz)

80 B9 03 00 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 16 -L
spi mode: 8
bits per word: 16
max speed: 500000 Hz (500 KHz)

B9 80 00 07 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 32 -L
spi mode: 8
bits per word: 32
max speed: 500000 Hz (500 KHz)

00 03 B9 80 00 00


This is the new QE mode:

root@localhost:~# ./spidev -D /dev/spidev0.3 -b 8
spi mode: 0
bits per word: 8
max speed: 500000 Hz (500 KHz)

01 9D E0 00 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 16
spi mode: 0
bits per word: 16
max speed: 500000 Hz (500 KHz)

9D 01 00 C0 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 32
spi mode: 0
bits per word: 32
max speed: 500000 Hz (500 KHz)

00 C0 9D 01 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 8 -L
spi mode: 8
bits per word: 8
max speed: 500000 Hz (500 KHz)

80 B9 07 00 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 16 -L
spi mode: 8
bits per word: 16
max speed: 500000 Hz (500 KHz)

80 B9 03 00 00 00
root@localhost:~# ./spidev -D /dev/spidev0.3 -b 32 -L
spi mode: 8
bits per word: 32
max speed: 500000 Hz (500 KHz)

80 B9 03 00 00 00


Notice how the byte order is swapped for word = 16, 32
Not even sure what is right to begin with :)
Can't see how one should fix this either.

 Jocke


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Anton Vorontsov]

On Thu, May 13, 2010 at 11:36:15AM +0200, Joakim Tjernlund wrote:
[...]
> The new QE mode is so broken :(
> Notice how the byte order is swapped for word = 16, 32

*yawn*, it's not a news... :-) Remember QE-specific rx/tx_shift
stuff?

I'm sure the issue is related to how the QE SPI shift register
works. I.e. SDMA just feeds the shift register, and it's shift
register that is causing > 8 bits words to misbehave, or the
issue is in how SDMA and shift registers connected.

> Not even sure what is right to begin with :)
> Can't see how one should fix this either.

I guess by copying the whole buffer, shifting bytes back and
forth, remapping it, doing transfer, and copying data back.
I think that'll work, but no idea how efficient that will be.

Also, I'd not do that for CPM hardware, I tend to believe
that it's not affected.

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/13 11:55:45:
>
> On Thu, May 13, 2010 at 11:36:15AM +0200, Joakim Tjernlund wrote:
> [...]
> > The new QE mode is so broken :(
> > Notice how the byte order is swapped for word = 16, 32
>
> *yawn*, it's not a news... :-) Remember QE-specific rx/tx_shift
> stuff?

How can I forget, I impl. that crap :)

>
> I'm sure the issue is related to how the QE SPI shift register
> works. I.e. SDMA just feeds the shift register, and it's shift
> register that is causing > 8 bits words to misbehave, or the
> issue is in how SDMA and shift registers connected.

Not sure, how one be BE and the LE on byte level?
I wonder if one can play with?
 out_8(&mspi->pram->tfcr, CPMFCR_EB | CPMFCR_GBL);
 out_8(&mspi->pram->rfcr, CPMFCR_EB | CPMFCR_GBL);
>
> > Not even sure what is right to begin with :)
> > Can't see how one should fix this either.
>
> I guess by copying the whole buffer, shifting bytes back and
> forth, remapping it, doing transfer, and copying data back.
> I think that'll work, but no idea how efficient that will be.

Not a chance, better to hardcode 8 bit word size, then one
case will work for all sizes and reject LSB.

>
> Also, I'd not do that for CPM hardware, I tend to believe
> that it's not affected.

Sure, I can't belelive how freescale HW guys managed to screw QE SPI so badly.


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/13 11:55:45:
>
> On Thu, May 13, 2010 at 11:36:15AM +0200, Joakim Tjernlund wrote:
> [...]
> > The new QE mode is so broken :(
> > Notice how the byte order is swapped for word = 16, 32
>
> *yawn*, it's not a news... :-) Remember QE-specific rx/tx_shift
> stuff?
>
> I'm sure the issue is related to how the QE SPI shift register
> works. I.e. SDMA just feeds the shift register, and it's shift
> register that is causing > 8 bits words to misbehave, or the
> issue is in how SDMA and shift registers connected.

This is how QE SPI defines bit and byte order:

[address%4=0x0_0x1_0x2_0x3] ghijklmn__opqrstuv_wxyzabcd_efGHIJKL
                                         Example 21-1.
with LEN=4 (data size=5),REV=0, the string transmitted is:
         first_bit nmlkj last_bit
with REV=1,the string transmitted is:
         first_bit jklmn    last_bit
                                         Example 21-2.
with LEN=7 (data size=8),REV=0, the string transmitted is:
         first_bit nmlkjihg last_bit
with REV=1, the string transmitted is:
         first_bit ghijklmn last_bit
                                         Example 21-3.
with LEN=0xC (data size=13),REV=0, the string transmitted is:
         first_bit nmlkjihgvutsr last_bit
with REV=1, the string transmitted is:
         first_bit rstuvghijklmn last_bit
                                         Example 21-4.
with LEN=0xF (data size=16), REV=0, the string transmitted is:
         first_bit nmlkjihgvutsrqpo last_bit
with REV=1, the string transmitted is:
         first_bit opqrstuvghijklmn last_bit
                                         Example 21-5.
with LEN=0 (data size=32), REV=0, the string transmitted is:
         first_bit nmlkjihgvutsrqpodcbazyxwLKJIHGfe last_bit
with REV=1, the string transmitted is:
         first_bit efGHIJKLwxyzabcdopqrstuvghijklmn last_bit


Seems to correspond to what I am seeing but is this a sane definition?
If so, then the CPU mode is wrong and needs to be changed.


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/13 11:55:45:
>
> On Thu, May 13, 2010 at 11:36:15AM +0200, Joakim Tjernlund wrote:
> [...]
> > The new QE mode is so broken :(
> > Notice how the byte order is swapped for word = 16, 32
>
> *yawn*, it's not a news... :-) Remember QE-specific rx/tx_shift
> stuff?
>
> I'm sure the issue is related to how the QE SPI shift register
> works. I.e. SDMA just feeds the shift register, and it's shift
> register that is causing > 8 bits words to misbehave, or the
> issue is in how SDMA and shift registers connected.
>
> > Not even sure what is right to begin with :)
> > Can't see how one should fix this either.
>
> I guess by copying the whole buffer, shifting bytes back and
> forth, remapping it, doing transfer, and copying data back.
> I think that'll work, but no idea how efficient that will be.
>
> Also, I'd not do that for CPM hardware, I tend to believe
> that it's not affected.

Anton, would you be happy with the following patch?
Basically it works around LE endian shift for !LSB
transfers.

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index c312d0e..c995aa5 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -315,41 +315,51 @@ int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 	if (!hz)
 		hz = spi->max_speed_hz;

-	cs->rx_shift = 0;
-	cs->tx_shift = 0;
-	if (bits_per_word <= 8) {
-		cs->get_rx = mpc8xxx_spi_rx_buf_u8;
-		cs->get_tx = mpc8xxx_spi_tx_buf_u8;
-		if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
-			cs->rx_shift = 16;
-			cs->tx_shift = 24;
-		}
-	} else if (bits_per_word <= 16) {
-		cs->get_rx = mpc8xxx_spi_rx_buf_u16;
-		cs->get_tx = mpc8xxx_spi_tx_buf_u16;
-		if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
-			cs->rx_shift = 16;
-			cs->tx_shift = 16;
+	if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
+		cs->rx_shift = 0;
+		cs->tx_shift = 0;
+		if (bits_per_word <= 8) {
+			cs->get_rx = mpc8xxx_spi_rx_buf_u8;
+			cs->get_tx = mpc8xxx_spi_tx_buf_u8;
+			if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
+				cs->rx_shift = 16;
+				cs->tx_shift = 24;
+			}
+		} else if (bits_per_word <= 16) {
+			cs->get_rx = mpc8xxx_spi_rx_buf_u16;
+			cs->get_tx = mpc8xxx_spi_tx_buf_u16;
+			if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
+				cs->rx_shift = 16;
+				cs->tx_shift = 16;
+			}
+		} else if (bits_per_word <= 32) {
+			cs->get_rx = mpc8xxx_spi_rx_buf_u32;
+			cs->get_tx = mpc8xxx_spi_tx_buf_u32;
+		} else
+			return -EINVAL;
+
+		if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE &&
+		    spi->mode & SPI_LSB_FIRST) {
+			cs->tx_shift = 0;
+			if (bits_per_word <= 8)
+				cs->rx_shift = 8;
+			else
+				cs->rx_shift = 0;
 		}
-	} else if (bits_per_word <= 32) {
-		cs->get_rx = mpc8xxx_spi_rx_buf_u32;
-		cs->get_tx = mpc8xxx_spi_tx_buf_u32;
-	} else
-		return -EINVAL;

-	if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE &&
-			spi->mode & SPI_LSB_FIRST) {
-		cs->tx_shift = 0;
-		if (bits_per_word <= 8)
-			cs->rx_shift = 8;
-		else
-			cs->rx_shift = 0;
-	}
+		mpc8xxx_spi->rx_shift = cs->rx_shift;
+		mpc8xxx_spi->tx_shift = cs->tx_shift;
+		mpc8xxx_spi->get_rx = cs->get_rx;
+		mpc8xxx_spi->get_tx = cs->get_tx;

-	mpc8xxx_spi->rx_shift = cs->rx_shift;
-	mpc8xxx_spi->tx_shift = cs->tx_shift;
-	mpc8xxx_spi->get_rx = cs->get_rx;
-	mpc8xxx_spi->get_tx = cs->get_tx;
+	} else if (mpc8xxx_spi->flags & SPI_QE) {
+		/* Note: 32 bits word, LSB works iff
+		 *  tfcr/rfcr is set to CPMFCR_GBL */
+		if (spi->mode & SPI_LSB_FIRST &&
+			bits_per_word != 8)
+			return -EINVAL;
+		bits_per_word = 8;
+	}

 	if (bits_per_word == 32)
 		bits_per_word = 0;


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Anton Vorontsov]

On Thu, May 13, 2010 at 05:55:22PM +0200, Joakim Tjernlund wrote:
> Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/13 11:55:45:
> >
> > On Thu, May 13, 2010 at 11:36:15AM +0200, Joakim Tjernlund wrote:
> > [...]
> > > The new QE mode is so broken :(
> > > Notice how the byte order is swapped for word = 16, 32
> >
> > *yawn*, it's not a news... :-) Remember QE-specific rx/tx_shift
> > stuff?
> >
> > I'm sure the issue is related to how the QE SPI shift register
> > works. I.e. SDMA just feeds the shift register, and it's shift
> > register that is causing > 8 bits words to misbehave, or the
> > issue is in how SDMA and shift registers connected.
> >
> > > Not even sure what is right to begin with :)
> > > Can't see how one should fix this either.
> >
> > I guess by copying the whole buffer, shifting bytes back and
> > forth, remapping it, doing transfer, and copying data back.
> > I think that'll work, but no idea how efficient that will be.
> >
> > Also, I'd not do that for CPM hardware, I tend to believe
> > that it's not affected.
> 
> Anton, would you be happy with the following patch?

Sure.

Just a few cosmetic suggestions, feel free to ignore. ;-)

> Basically it works around LE endian shift for !LSB
> transfers.
> 
> diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
> index c312d0e..c995aa5 100644
> --- a/drivers/spi/spi_mpc8xxx.c
> +++ b/drivers/spi/spi_mpc8xxx.c
> @@ -315,41 +315,51 @@ int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
>  	if (!hz)
>  		hz = spi->max_speed_hz;
> 
> -	cs->rx_shift = 0;
> -	cs->tx_shift = 0;
> -	if (bits_per_word <= 8) {
> -		cs->get_rx = mpc8xxx_spi_rx_buf_u8;
> -		cs->get_tx = mpc8xxx_spi_tx_buf_u8;
> -		if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
> -			cs->rx_shift = 16;
> -			cs->tx_shift = 24;
> -		}
> -	} else if (bits_per_word <= 16) {
> -		cs->get_rx = mpc8xxx_spi_rx_buf_u16;
> -		cs->get_tx = mpc8xxx_spi_tx_buf_u16;
> -		if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
> -			cs->rx_shift = 16;
> -			cs->tx_shift = 16;
> +	if (!(mpc8xxx_spi->flags & SPI_CPM_MODE)) {
> +		cs->rx_shift = 0;
> +		cs->tx_shift = 0;
> +		if (bits_per_word <= 8) {
> +			cs->get_rx = mpc8xxx_spi_rx_buf_u8;
> +			cs->get_tx = mpc8xxx_spi_tx_buf_u8;
> +			if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
> +				cs->rx_shift = 16;
> +				cs->tx_shift = 24;
> +			}
> +		} else if (bits_per_word <= 16) {
> +			cs->get_rx = mpc8xxx_spi_rx_buf_u16;
> +			cs->get_tx = mpc8xxx_spi_tx_buf_u16;
> +			if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE) {
> +				cs->rx_shift = 16;
> +				cs->tx_shift = 16;
> +			}
> +		} else if (bits_per_word <= 32) {
> +			cs->get_rx = mpc8xxx_spi_rx_buf_u32;
> +			cs->get_tx = mpc8xxx_spi_tx_buf_u32;
> +		} else
> +			return -EINVAL;
> +
> +		if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE &&
> +		    spi->mode & SPI_LSB_FIRST) {
> +			cs->tx_shift = 0;
> +			if (bits_per_word <= 8)
> +				cs->rx_shift = 8;
> +			else
> +				cs->rx_shift = 0;

While you're at it, can you move this monstrous stuff to its
own function? That'll save one indent level, and will make the
patch nicier to read (let's hope), i.e.

if (!(mpc8xxx_spi->flags & SPI_CPM_MODE))
	rc = mspi_apply_cpu_mode_quirks(cs);
else if (mpc8xxx_spi->flags & SPI_QE)
	rc = mspi_apply_qe_mode_quirks(cs);

if (rc)
	return rc;

>  		}
> -	} else if (bits_per_word <= 32) {
> -		cs->get_rx = mpc8xxx_spi_rx_buf_u32;
> -		cs->get_tx = mpc8xxx_spi_tx_buf_u32;
> -	} else
> -		return -EINVAL;
> 
> -	if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE &&
> -			spi->mode & SPI_LSB_FIRST) {
> -		cs->tx_shift = 0;
> -		if (bits_per_word <= 8)
> -			cs->rx_shift = 8;
> -		else
> -			cs->rx_shift = 0;
> -	}
> +		mpc8xxx_spi->rx_shift = cs->rx_shift;
> +		mpc8xxx_spi->tx_shift = cs->tx_shift;
> +		mpc8xxx_spi->get_rx = cs->get_rx;
> +		mpc8xxx_spi->get_tx = cs->get_tx;
> 
> -	mpc8xxx_spi->rx_shift = cs->rx_shift;
> -	mpc8xxx_spi->tx_shift = cs->tx_shift;
> -	mpc8xxx_spi->get_rx = cs->get_rx;
> -	mpc8xxx_spi->get_tx = cs->get_tx;
> +	} else if (mpc8xxx_spi->flags & SPI_QE) {
> +		/* Note: 32 bits word, LSB works iff
> +		 *  tfcr/rfcr is set to CPMFCR_GBL */
> +		if (spi->mode & SPI_LSB_FIRST &&
> +			bits_per_word != 8)

                ^^^ one more tab here?

> +			return -EINVAL;
> +		bits_per_word = 8;
> +	}
> 
>  	if (bits_per_word == 32)
>  		bits_per_word = 0;
> 

Thanks!

-- 
Anton Vorontsov
email: cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
irc://irc.freenode.net/bd2

------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/13 18:18:51:

> From: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> To: Joakim Tjernlund <joakim.tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>, spi-devel-general-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
> Date: 2010/05/13 18:18
> Subject: Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.
>
> On Thu, May 13, 2010 at 05:55:22PM +0200, Joakim Tjernlund wrote:
> > Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/13 11:55:45:
> > >
> > > On Thu, May 13, 2010 at 11:36:15AM +0200, Joakim Tjernlund wrote:
> > > [...]
> > > > The new QE mode is so broken :(
> > > > Notice how the byte order is swapped for word = 16, 32
> > >
> > > *yawn*, it's not a news... :-) Remember QE-specific rx/tx_shift
> > > stuff?
> > >
> > > I'm sure the issue is related to how the QE SPI shift register
> > > works. I.e. SDMA just feeds the shift register, and it's shift
> > > register that is causing > 8 bits words to misbehave, or the
> > > issue is in how SDMA and shift registers connected.
> > >
> > > > Not even sure what is right to begin with :)
> > > > Can't see how one should fix this either.
> > >
> > > I guess by copying the whole buffer, shifting bytes back and
> > > forth, remapping it, doing transfer, and copying data back.
> > > I think that'll work, but no idea how efficient that will be.
> > >
> > > Also, I'd not do that for CPM hardware, I tend to believe
> > > that it's not affected.
> >
> > Anton, would you be happy with the following patch?
>
> Sure.
>
> Just a few cosmetic suggestions, feel free to ignore. ;-)

would look like this, OK?

diff --git a/drivers/spi/spi_mpc8xxx.c b/drivers/spi/spi_mpc8xxx.c
index c312d0e..34e7d87 100644
--- a/drivers/spi/spi_mpc8xxx.c
+++ b/drivers/spi/spi_mpc8xxx.c
@@ -285,36 +285,12 @@ static void mpc8xxx_spi_chipselect(struct spi_device *spi, int value)
 	}
 }

-static
-int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
+static int
+mspi_apply_cpu_mode_quirks(struct spi_mpc8xxx_cs *cs,
+			   struct spi_device *spi,
+			   struct mpc8xxx_spi *mpc8xxx_spi,
+			   int bits_per_word)
 {
-	struct mpc8xxx_spi *mpc8xxx_spi;
-	u8 bits_per_word, pm;
-	u32 hz;
-	struct spi_mpc8xxx_cs	*cs = spi->controller_state;
-
-	mpc8xxx_spi = spi_master_get_devdata(spi->master);
-
-	if (t) {
-		bits_per_word = t->bits_per_word;
-		hz = t->speed_hz;
-	} else {
-		bits_per_word = 0;
-		hz = 0;
-	}
-
-	/* spi_transfer level calls that work per-word */
-	if (!bits_per_word)
-		bits_per_word = spi->bits_per_word;
-
-	/* Make sure its a bit width we support [4..16, 32] */
-	if ((bits_per_word < 4)
-	    || ((bits_per_word > 16) && (bits_per_word != 32)))
-		return -EINVAL;
-
-	if (!hz)
-		hz = spi->max_speed_hz;
-
 	cs->rx_shift = 0;
 	cs->tx_shift = 0;
 	if (bits_per_word <= 8) {
@@ -338,19 +314,76 @@ int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
 		return -EINVAL;

 	if (mpc8xxx_spi->flags & SPI_QE_CPU_MODE &&
-			spi->mode & SPI_LSB_FIRST) {
+	    spi->mode & SPI_LSB_FIRST) {
 		cs->tx_shift = 0;
 		if (bits_per_word <= 8)
 			cs->rx_shift = 8;
 		else
 			cs->rx_shift = 0;
 	}
-
 	mpc8xxx_spi->rx_shift = cs->rx_shift;
 	mpc8xxx_spi->tx_shift = cs->tx_shift;
 	mpc8xxx_spi->get_rx = cs->get_rx;
 	mpc8xxx_spi->get_tx = cs->get_tx;

+	return bits_per_word;
+}
+
+static int
+mspi_apply_qe_mode_quirks(struct spi_mpc8xxx_cs *cs,
+			  struct spi_device *spi,
+			  int bits_per_word)
+{
+	/* Note: 32 bits word, LSB works iff
+	 *  tfcr/rfcr is set to CPMFCR_GBL */
+	if (spi->mode & SPI_LSB_FIRST &&
+	    bits_per_word != 8)
+		return -EINVAL;
+	return 8;
+}
+
+static
+int mpc8xxx_spi_setup_transfer(struct spi_device *spi, struct spi_transfer *t)
+{
+	struct mpc8xxx_spi *mpc8xxx_spi;
+	int bits_per_word;
+	u8 pm;
+	u32 hz;
+	struct spi_mpc8xxx_cs	*cs = spi->controller_state;
+
+	mpc8xxx_spi = spi_master_get_devdata(spi->master);
+
+	if (t) {
+		bits_per_word = t->bits_per_word;
+		hz = t->speed_hz;
+	} else {
+		bits_per_word = 0;
+		hz = 0;
+	}
+
+	/* spi_transfer level calls that work per-word */
+	if (!bits_per_word)
+		bits_per_word = spi->bits_per_word;
+
+	/* Make sure its a bit width we support [4..16, 32] */
+	if ((bits_per_word < 4)
+	    || ((bits_per_word > 16) && (bits_per_word != 32)))
+		return -EINVAL;
+
+	if (!hz)
+		hz = spi->max_speed_hz;
+
+	if (!(mpc8xxx_spi->flags & SPI_CPM_MODE))
+		bits_per_word = mspi_apply_cpu_mode_quirks(cs, spi,
+							   mpc8xxx_spi,
+							   bits_per_word);
+	else if (mpc8xxx_spi->flags & SPI_QE)
+		bits_per_word = mspi_apply_qe_mode_quirks(cs, spi,
+							  bits_per_word);
+
+	if (bits_per_word < 0)
+		return bits_per_word;
+
 	if (bits_per_word == 32)
 		bits_per_word = 0;
 	else


------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Grant Likely]

On Wed, May 12, 2010 at 6:43 PM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, May 12, 2010 at 06:28:51PM +0200, Joakim Tjernlund wrote:
>> Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:22:31:
>> >
>> > On Wed, May 12, 2010 at 05:50:50PM +0200, Joakim Tjernlund wrote:
>> > > tx_dma/rx_dma are already set to a dummy buffer when no
>> > > tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
>> > > address, but NULL.
>> > > This may lead to corruption of kernel memory. Fix this by
>> > > leaving tx_dma/rx_dma alone.
>> > >
>> > > Do not INIT_TX_RX while controller is enabled, this is bad according
>> > > to the MPC8321 manual.
>> > >
>> > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
>> >
>> > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >
>> > Thanks!
>> :)
>>
>> Who will make sure this goes to Linus?
>
> I guess it's Grant. Cc'ed.

Yeah, it's me.  I'm just traveling so I'm a little slow on the uptake
right now.  I've picked it up.

g.

------------------------------------------------------------------------------

Re: [PATCH] spi: spi_mpc8xxx.c: fix potential memory corruption.

[Joakim Tjernlund]

glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org wrote on 2010/05/13 23:13:52:
>
> On Wed, May 12, 2010 at 6:43 PM, Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > On Wed, May 12, 2010 at 06:28:51PM +0200, Joakim Tjernlund wrote:
> >> Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote on 2010/05/12 18:22:31:
> >> >
> >> > On Wed, May 12, 2010 at 05:50:50PM +0200, Joakim Tjernlund wrote:
> >> > > tx_dma/rx_dma are already set to a dummy buffer when no
> >> > > tx/rx buffer and t->tx_dma/t->rx_dma does not contain a dma
> >> > > address, but NULL.
> >> > > This may lead to corruption of kernel memory. Fix this by
> >> > > leaving tx_dma/rx_dma alone.
> >> > >
> >> > > Do not INIT_TX_RX while controller is enabled, this is bad according
> >> > > to the MPC8321 manual.
> >> > >
> >> > > Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund-SNLAxHN9vbcOP4wsBPIw7w@public.gmane.org>
> >> >
> >> > Acked-by: Anton Vorontsov <cbouatmailru-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> >
> >> > Thanks!
> >> :)
> >>
> >> Who will make sure this goes to Linus?
> >
> > I guess it's Grant. Cc'ed.
>
> Yeah, it's me.  I'm just traveling so I'm a little slow on the uptake
> right now.  I've picked it up.

Thanks, I think it should go to Linus now.

I also just sent another patch, fixing brain dead Little Endian problem
in QE mode. You might want to consider that one too for early inclusion as
potential QE SPI users might start using word size > 8 bits and compensate
for LE in their app.

Anton should first ack it though.

 Jocke


------------------------------------------------------------------------------