possible bug in pxa2xx_spi.c

["iw3gtf-IWqWACnzNjyonA0d6jMUrA@public.gmane.org" <iw3gtf-IWqWACnzNjyonA0d6jMUrA@public.gmane.org>]

Hi,
I think I've found a bug in the pxa2xx_spi driver.

I'm running a linux 2.6.28.7 kernel on a pxa320 arm cpu with an m25p80 flash

connected to the cpu through the port ssp4.
If I configure the system to run in pio mode (dma disabled):

static struct pxa2xx_spi_master

pxa_ssp4_master_info = {
  .clock_enable = CKEN_SSP4, // SSP4 Peripheral clock
  .num_chipselect = 1, // Matches the number of chips 
attached to SSP4
  .enable_dma = 0, // Disables SSP4 DMA
};

static struct platform_device pxa_spi_ssp4 = {
  .name = "pxa2xx-spi", // MUST 
BE THIS VALUE, so device match driver
  .id = 4, // Bus number, MUST MATCH SSP number 1..n
  .dev = {
    .platform_data = 
&pxa_ssp4_master_info, // Passed to the controller driver
  },
};

I can correctly operate with the flash (using the mtd interface).



Nevertheless, if I enable the dma I have problems: the mtd->write(...)
function returns an absurd value in its fourth argument, the count

of successfully written bytes.

I could track this problem down to the source file:

drivers/spi/pxa2xx_spi.c

Assume we call the method 
mtd->write(...) to write 32 bytes to the flash.
The m25p80 driver prepares a struct spi_message with a couple of transfers in it and

inserts it in a message queue for the ssp controller driver.
The message is then extracted from the queue within the function

pxa2xx_spi.c:
pump_transfers(unsigned long data)

the message contains 2 transfers: the 'flash program' command and the
real 'bytes to be written' 
transfer.
The struct spi_transfer for this second transfer has the following setup:

transfer->tx_buf: 0xcf1c80c0 (a valid pointer)

transfer->rx_buf: 0x00000000
transfer->len: 32

the rx_buf is a null pointer because we don't want to read anything from the flash.
With 
this (I think valid) setup and with the dma enabled something wrong happens:
the tranfer is copied to the struct driver_data drv_data:


drv_data->tx = (void *)transfer->tx_buf;
drv_data->tx_end = drv_data->tx + transfer->len;
drv_data->rx = transfer->rx_buf;
drv_data->rx_end 
= drv_data->rx + transfer->len;

this way the field drv_data->rx gets a null and drv_data->rx_end gets 0 + 32 = 32:

...
drv_data->rx = 0

drv_data->rx_end = 32
...

sometime later the rx buffer must be dma mapped within the function:

pxa2xx_spi.c:map_dma_buffers(struct 
driver_data *drv_data)


/* Modify setup if rx buffer is null */
if (drv_data->rx == NULL) {
  *drv_data->null_dma_buf = 0;
  drv_data->rx 
= drv_data->null_dma_buf;
  drv_data->rx_map_len = 4;
} else
  drv_data->rx_map_len = drv_data->len;

being a null pointer the drv_data->rx 
gets a dummy, but dma valid, pointer value
(drv_data->null_dma_buf) and the transfer can now start with dma.
When finished the function:


pxa2xx_spi.c:dma_transfer_complete(struct driver_data *drv_data)

is called and here we have the following wrong computations:

drv_data-
>rx += drv_data->len -
  (DCMD(drv_data->rx_channel) & DCMD_LENGTH);


/* read trailing data from fifo, it does not matter how many
 * 
bytes are in the fifo just read until buffer is full
 * or fifo is empty, which ever occurs first */

 drv_data->read(drv_data);

/* return 
count of what was actually read */
 msg->actual_length += drv_data->len -
   (drv_data->rx_end - drv_data->rx);

drv_data->rx has the dummy 
pointer value (somewhere around 0xc03d6a28 for me) but
drv_data->rx_end still has the value 32!!!

drv_data->rx: 0xc03d6a28+...
drv_data-
>rx_end: 32

from this values we get an absurd result for msg->actual_length.

A fix for this problem could be to also update the drv_data-
>rx_end
field in the call to

pxa2xx_spi.c:map_dma_buffers(struct driver_data *drv_data)


/* Modify setup if rx buffer is null */
if 
(drv_data->rx == NULL) {
  *drv_data->null_dma_buf = 0;
  drv_data->rx = drv_data->null_dma_buf;
  drv_data->rx_end = drv_data->rx + 
drv_data->len; <======== fix
  drv_data->rx_map_len = 4;
} else
  drv_data->rx_map_len = drv_data->len;


I have also a second question 
about pxa2xx_spi.c: the value of the
dummy dma pointer 'drv_data->null_dma_buf' is assigned as follows in:

pxa2xx_spi.c:pxa2xx_spi_probe
(struct platform_device *pdev)


drv_data->null_dma_buf =
  (u32 *)ALIGN((u32)(drv_data + sizeof(struct driver_data)), 8);

is this address 
safe to write to? Or could we potentially overwrite
something already allocated there ?

thanks for your attention.

best regards

giorgio



Con Tiscali Tutto Incluso telefoni e navighi senza limiti A SOLI €10 AL MESE FINO ALL’ESTATE. 

Attiva entro il 18/03/09! http://abbonati.tiscali.it/promo/tuttoincluso/

------------------------------------------------------------------------------
Apps built with the Adobe(R) Flex(R) framework and Flex Builder(TM) are
powering Web 2.0 with engaging, cross-platform capabilities. Quickly and
easily build your RIAs with Flex Builder, the Eclipse(TM)based development
software that enables intelligent coding and step-through debugging.
Download the free 60 day trial. http://p.sf.net/sfu/www-adobe-com
_______________________________________________
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general