[PATCH] mtd_dataflash use command allocated on stack

[PATCH] mtd_dataflash use command allocated on stack

[Vitja Makarov]

Hi!


Currently mtd_dataflash driver has command in their private structure.
That is only 4 bytes, sometimes actually 8 is needed.
Also it could be accessed twice before lock is taken.

This patch moves command to stack.

vitja.

Signed-off-by: Vitja Makarov <vitja.makarov@gmail.com>
--- mtd_dataflash.c=	2008-03-03 10:30:35.000000000 +0300
+++ mtd_dataflash.c	2008-04-04 08:47:24.000000000 +0400
@@ -82,7 +82,6 @@
 
 
 struct dataflash {
-	u8			command[4];
 	char			name[24];
 
 	unsigned		partitioned:1;
@@ -150,7 +149,7 @@
 	struct spi_transfer	x = { .tx_dma = 0, };
 	struct spi_message	msg;
 	unsigned		blocksize = priv->page_size << 3;
-	u8			*command;
+	u8			command[4];
 
 	DEBUG(MTD_DEBUG_LEVEL2, "%s: erase addr=0x%x len 0x%x\n",
 			spi->dev.bus_id,
@@ -164,7 +163,7 @@
 
 	spi_message_init(&msg);
 
-	x.tx_buf = command = priv->command;
+	x.tx_buf = command;
 	x.len = 4;
 	spi_message_add_tail(&x, &msg);
 
@@ -234,7 +233,7 @@
 	struct spi_transfer	x[2] = { { .tx_dma = 0, }, };
 	struct spi_message	msg;
 	unsigned int		addr;
-	u8			*command;
+	u8			command[8];
 	int			status;
 
 	DEBUG(MTD_DEBUG_LEVEL2, "%s: read 0x%x..0x%x\n",
@@ -252,8 +251,6 @@
 	addr = (((unsigned)from / priv->page_size) << priv->page_offset)
 		+ ((unsigned)from % priv->page_size);
 
-	command = priv->command;
-
 	DEBUG(MTD_DEBUG_LEVEL3, "READ: (%x) %x %x %x\n",
 		command[0], command[1], command[2], command[3]);
 
@@ -311,7 +308,7 @@
 	size_t			remaining = len;
 	u_char			*writebuf = (u_char *) buf;
 	int			status = -EINVAL;
-	u8			*command;
+	u8			command[8];
 
 	DEBUG(MTD_DEBUG_LEVEL2, "%s: write 0x%x..0x%x\n",
 		spi->dev.bus_id, (unsigned)to, (unsigned)(to + len));
@@ -326,7 +323,7 @@
 
 	spi_message_init(&msg);
 
-	x[0].tx_buf = command = priv->command;
+	x[0].tx_buf = command;
 	x[0].len = 4;
 	spi_message_add_tail(&x[0], &msg);
 

Re: [PATCH] mtd_dataflash use command allocated on stack

[David Woodhouse]

On Fri, 2008-04-04 at 09:02 +0400, Vitja Makarov wrote:
> Hi!
> 
> 
> Currently mtd_dataflash driver has command in their private structure.
> That is only 4 bytes, sometimes actually 8 is needed.
> Also it could be accessed twice before lock is taken.
> 
> This patch moves command to stack.

Is that allowed? The SPI code might DMA from it, surely?

-- 
dwmw2

Re: [PATCH] mtd_dataflash use command allocated on stack

[Vitja Makarov]

2008/4/23 David Woodhouse <dwmw2@infradead.org>:
> On Fri, 2008-04-04 at 09:02 +0400, Vitja Makarov wrote:
>  > Hi!
>  >
>  >
>  > Currently mtd_dataflash driver has command in their private structure.
>  > That is only 4 bytes, sometimes actually 8 is needed.
>  > Also it could be accessed twice before lock is taken.
>  >
>  > This patch moves command to stack.
>
>  Is that allowed? The SPI code might DMA from it, surely?
>
>  --
>  dwmw2
>
>


I'm using this driver with dma enabled on blackfin arch and that seems to work.
Btw mutex is taken before priv->command is accessed so no race could be here.
But I still think that having command 4 bytes when it's actually 8 (4
+ 4 don't cares) isn't a good idea.
If you are asking about could spi DMA from stack, I think so, as
spi_sync() is called before ret, but there still are cache issues,
dataflash or better spi driver should call invalidate/flush for the
given address if needed, I moved it to spi_bfin code, and that seems
to work for me wo modifyng original dataflash driver.

Re: [PATCH] mtd_dataflash use command allocated on stack

[Haavard Skinnemoen]

Sorry about the late response...I'm currently getting up to speed with
various mailing lists.

On Wed, 23 Apr 2008 08:00:34 +0400
"Vitja Makarov" <vitja.makarov@gmail.com> wrote:

> If you are asking about could spi DMA from stack, I think so, as
> spi_sync() is called before ret, but there still are cache issues,
> dataflash or better spi driver should call invalidate/flush for the
> given address if needed, I moved it to spi_bfin code, and that seems
> to work for me wo modifyng original dataflash driver.

NO! DMA from stack is NOT safe!

The problem isn't missing cache flushing -- any driver that uses DMA
must do that anyway -- but that the flushed lines get sucked right back
in when the function returns.

Also note that DMA from stack may work _sometimes_, but you never know
when the buffer happens to share a cache line with some return
address...

Haavard