[PATCH bugfix] mtd: gpmi: serialize all the dma operations

[PATCH bugfix] mtd: gpmi: serialize all the dma operations

[Huang Shijie]

[1] The gpmi uses the nand_command_lp to issue the commands to NAND chips.
    It will issue a DMA operation when it handles a NAND_CMD_NONE control
    command. So when we read a page(NAND_CMD_READ0) from the NAND, we may send
    two DMA operations back-to-back.

    If we do not serialize the two DMA operations, we will meet a bug when

    1.1) we enable CONFIG_DMA_API_DEBUG, CONFIG_DMADEVICES_DEBUG,
         and CONFIG_DEBUG_SG.

    1.2) Use the following commands in an UART console and a SSH console:
         cmd 1: while true;do dd if=/dev/mtd0 of=/dev/null;done
         cmd 1: while true;do dd if=/dev/mmcblk0 of=/dev/null;done

    The kernel log shows below:
    -----------------------------------------------------------------
    kernel BUG at lib/scatterlist.c:28!
    Unable to handle kernel NULL pointer dereference at virtual address 00000000
      .........................
    [<80044a0c>] (__bug+0x18/0x24) from [<80249b74>] (sg_next+0x48/0x4c)
    [<80249b74>] (sg_next+0x48/0x4c) from [<80255398>] (debug_dma_unmap_sg+0x170/0x1a4)
    [<80255398>] (debug_dma_unmap_sg+0x170/0x1a4) from [<8004af58>] (dma_unmap_sg+0x14/0x6c)
    [<8004af58>] (dma_unmap_sg+0x14/0x6c) from [<8027e594>] (mxs_dma_tasklet+0x18/0x1c)
    [<8027e594>] (mxs_dma_tasklet+0x18/0x1c) from [<8007d444>] (tasklet_action+0x114/0x164)
    -----------------------------------------------------------------

    1.3) Assume the two DMA operations is X (first) and Y (second).
         The root cause of the bug:
         X's tasklet mxs_dma_tasklet trid to unmap the scatterlist, while Y is
         trying to set up a new DMA operation with the _SAME_ scatterlist in
         another ARM core.

[2] This patch adds a wait queue and two helpers gpmi_enter_dma/gpmi_exit_dma to
    serialize all the DMA operations.

Signed-off-by: Huang Shijie <b32955@freescale.com>
Cc: stable at vger.kernel.org
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   24 ++++++++++++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    2 ++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 71df69e..b849b92 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -392,6 +392,20 @@ void prepare_data_dma(struct gpmi_nand_data *this, enum dma_data_direction dr)
 	}
 }
 
+static void gpmi_enter_dma(struct gpmi_nand_data *this)
+{
+	/* Wait until the previous DMA is finished. */
+	wait_event(this->dma_wait, !this->dma_is_working);
+
+	this->dma_is_working = true;
+}
+
+static void gpmi_exit_dma(struct gpmi_nand_data *this)
+{
+	this->dma_is_working = false;
+	wake_up(&this->dma_wait);
+}
+
 /* This will be called after the DMA operation is finished. */
 static void dma_irq_callback(void *param)
 {
@@ -424,6 +438,7 @@ static void dma_irq_callback(void *param)
 	default:
 		pr_err("in wrong DMA operation.\n");
 	}
+	gpmi_exit_dma(this);
 }
 
 int start_dma_without_bch_irq(struct gpmi_nand_data *this,
@@ -906,6 +921,8 @@ static void gpmi_cmd_ctrl(struct mtd_info *mtd, int data, unsigned int ctrl)
 	if (!this->command_length)
 		return;
 
+	gpmi_enter_dma(this);
+
 	ret = gpmi_send_command(this);
 	if (ret)
 		pr_err("Chip: %u, Error %d\n", this->current_chip, ret);
@@ -943,6 +960,8 @@ static void gpmi_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
 	this->upper_buf	= buf;
 	this->upper_len	= len;
 
+	gpmi_enter_dma(this);
+
 	gpmi_read_data(this);
 }
 
@@ -955,6 +974,8 @@ static void gpmi_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
 	this->upper_buf	= (uint8_t *)buf;
 	this->upper_len	= len;
 
+	gpmi_enter_dma(this);
+
 	gpmi_send_data(this);
 }
 
@@ -1031,6 +1052,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	int           ret;
 
 	pr_debug("page number is : %d\n", page);
+	gpmi_enter_dma(this);
 	ret = read_page_prepare(this, buf, mtd->writesize,
 					this->payload_virt, this->payload_phys,
 					nfc_geo->payload_size,
@@ -1107,6 +1129,7 @@ static int gpmi_ecc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
 	int        ret;
 
 	pr_debug("ecc write page.\n");
+	gpmi_enter_dma(this);
 	if (this->swap_block_mark) {
 		/*
 		 * If control arrives here, we're doing block mark swapping.
@@ -1745,6 +1768,7 @@ static int gpmi_nand_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, this);
 	this->pdev  = pdev;
 	this->dev   = &pdev->dev;
+	init_waitqueue_head(&this->dma_wait);
 
 	ret = acquire_resources(this);
 	if (ret)
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index a7685e3..9597615 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -160,6 +160,8 @@ struct gpmi_nand_data {
 
 	/* for DMA operations */
 	bool			direct_dma_map_ok;
+	bool			dma_is_working;
+	wait_queue_head_t	dma_wait;
 
 	struct scatterlist	cmd_sgl;
 	char			*cmd_buffer;
-- 
1.7.2.rc3

[PATCH bugfix] mtd: gpmi: serialize all the dma operations

[Brian Norris]

Hi Huang,

On Wed, Nov 06, 2013 at 04:53:27PM +0800, Huang Shijie wrote:
> [1] The gpmi uses the nand_command_lp to issue the commands to NAND chips.
>     It will issue a DMA operation when it handles a NAND_CMD_NONE control
>     command. So when we read a page(NAND_CMD_READ0) from the NAND, we may send
>     two DMA operations back-to-back.
> 
>     If we do not serialize the two DMA operations, we will meet a bug when
> 
>     1.1) we enable CONFIG_DMA_API_DEBUG, CONFIG_DMADEVICES_DEBUG,
>          and CONFIG_DEBUG_SG.
> 
>     1.2) Use the following commands in an UART console and a SSH console:
>          cmd 1: while true;do dd if=/dev/mtd0 of=/dev/null;done
>          cmd 1: while true;do dd if=/dev/mmcblk0 of=/dev/null;done

How does mmcblk0 have anything to do with the GPMI NAND DMA?

>     The kernel log shows below:
>     -----------------------------------------------------------------
>     kernel BUG at lib/scatterlist.c:28!
>     Unable to handle kernel NULL pointer dereference at virtual address 00000000
>       .........................
>     [<80044a0c>] (__bug+0x18/0x24) from [<80249b74>] (sg_next+0x48/0x4c)
>     [<80249b74>] (sg_next+0x48/0x4c) from [<80255398>] (debug_dma_unmap_sg+0x170/0x1a4)
>     [<80255398>] (debug_dma_unmap_sg+0x170/0x1a4) from [<8004af58>] (dma_unmap_sg+0x14/0x6c)
>     [<8004af58>] (dma_unmap_sg+0x14/0x6c) from [<8027e594>] (mxs_dma_tasklet+0x18/0x1c)
>     [<8027e594>] (mxs_dma_tasklet+0x18/0x1c) from [<8007d444>] (tasklet_action+0x114/0x164)
>     -----------------------------------------------------------------
> 
>     1.3) Assume the two DMA operations is X (first) and Y (second).
>          The root cause of the bug:
>          X's tasklet mxs_dma_tasklet trid to unmap the scatterlist, while Y is
>          trying to set up a new DMA operation with the _SAME_ scatterlist in
>          another ARM core.

How are X and Y occurring concurrently? MTD/NAND has locking such that
only one I/O operation is working on the chip at one time, right?

> [2] This patch adds a wait queue and two helpers gpmi_enter_dma/gpmi_exit_dma to
>     serialize all the DMA operations.

If you really need this serialization, wouldn't a spinlock or mutex
suffice?

> Signed-off-by: Huang Shijie <b32955@freescale.com>
> Cc: stable at vger.kernel.org

Perhaps I'm missing some things, but I don't feel like the problem is
sufficiently well described, and I'm not sure this is the right
solution. But please, educate me.

Brian

[PATCH bugfix] mtd: gpmi: serialize all the dma operations

[Huang Shijie]

? 2013?11?08? 05:12, Brian Norris ??:
> Hi Huang,
>
> On Wed, Nov 06, 2013 at 04:53:27PM +0800, Huang Shijie wrote:
>> [1] The gpmi uses the nand_command_lp to issue the commands to NAND chips.
>>      It will issue a DMA operation when it handles a NAND_CMD_NONE control
>>      command. So when we read a page(NAND_CMD_READ0) from the NAND, we may send
>>      two DMA operations back-to-back.
>>
>>      If we do not serialize the two DMA operations, we will meet a bug when
>>
>>      1.1) we enable CONFIG_DMA_API_DEBUG, CONFIG_DMADEVICES_DEBUG,
>>           and CONFIG_DEBUG_SG.
>>
>>      1.2) Use the following commands in an UART console and a SSH console:
>>           cmd 1: while true;do dd if=/dev/mtd0 of=/dev/null;done
>>           cmd 1: while true;do dd if=/dev/mmcblk0 of=/dev/null;done
> How does mmcblk0 have anything to do with the GPMI NAND DMA?
Reading the mmcblk0 is just to heavy the arm core loading. Reading the 
mmcblk0 also revokes the
tasklets, and so make the tasklets of GPMI DMA not handled quickly enough.
>>      The kernel log shows below:
>>      -----------------------------------------------------------------
>>      kernel BUG at lib/scatterlist.c:28!
>>      Unable to handle kernel NULL pointer dereference at virtual address 00000000
>>        .........................
>>      [<80044a0c>] (__bug+0x18/0x24) from [<80249b74>] (sg_next+0x48/0x4c)
>>      [<80249b74>] (sg_next+0x48/0x4c) from [<80255398>] (debug_dma_unmap_sg+0x170/0x1a4)
>>      [<80255398>] (debug_dma_unmap_sg+0x170/0x1a4) from [<8004af58>] (dma_unmap_sg+0x14/0x6c)
>>      [<8004af58>] (dma_unmap_sg+0x14/0x6c) from [<8027e594>] (mxs_dma_tasklet+0x18/0x1c)
>>      [<8027e594>] (mxs_dma_tasklet+0x18/0x1c) from [<8007d444>] (tasklet_action+0x114/0x164)
>>      -----------------------------------------------------------------
>>
>>      1.3) Assume the two DMA operations is X (first) and Y (second).
>>           The root cause of the bug:
>>           X's tasklet mxs_dma_tasklet trid to unmap the scatterlist, while Y is
>>           trying to set up a new DMA operation with the _SAME_ scatterlist in
>>           another ARM core.
> How are X and Y occurring concurrently? MTD/NAND has locking such that
> only one I/O operation is working on the chip at one time, right?
X and Y can not occur concurrently. X and Y is issue when the 
nand_command_lp calls the:
chip->cmd_ctrl(mtd, NAND_CMD_NONE, ....);

When we read a page from the NAND, the X and Y is issued back to back.
>> [2] This patch adds a wait queue and two helpers gpmi_enter_dma/gpmi_exit_dma to
>>      serialize all the DMA operations.
> If you really need this serialization, wouldn't a spinlock or mutex
> suffice?
>
spinlock is too short for this case.

mutex_unlock can not be call in the interrupt context, such as the in 
the tasklet.
>> Signed-off-by: Huang Shijie<b32955@freescale.com>
>> Cc: stable at vger.kernel.org
> Perhaps I'm missing some things, but I don't feel like the problem is
> sufficiently well described, and I'm not sure this is the right
> solution. But please, educate me.
>
should i add more description in the section of the root cause?

thanks
Huang Shijie

[PATCH bugfix] mtd: gpmi: serialize all the dma operations

[Huang Shijie]

On Thu, Nov 07, 2013 at 01:12:29PM -0800, Brian Norris wrote:
> sufficiently well described, and I'm not sure this is the right
> solution. But please, educate me.
I find a better way to synchronize the two DMA operations.
I will send out the new patch after i test it.

thanks
Huang Shijie