mmc: msm_sdcc: Fix possible circular locking dependency warning
Subsystems (2)
PATCH v2 1/5 mmc: msm_sdcc: Fix possible circular locking dependency warning
In the context of request processing thread, data mover lock is acquired after the host lock. In another context, in the completion handler of data mover the locks are acquired in the reverse order, resulting in possible circular lock dependency warning. Hence, schedule a tasklet to process the dma completion so as to avoid nested locks.
--- drivers/mmc/host/msm_sdcc.c | 49 +++++++++++++++++++++++++++++-------------- drivers/mmc/host/msm_sdcc.h | 3 ++ 2 files changed, 36 insertions(+), 16 deletions(-)
drivers/mmc/host/msm_sdcc.c +33 -16
@@ -189,42 +189,40 @@ msmsdcc_dma_exec_func(struct msm_dmov_cmd *cmd) }static void -msmsdcc_dma_complete_func (struct msm_dmov_cmd *cmd, -unsigned int result, -struct msm_dmov_errdata *err) +msmsdcc_dma_complete_tlet (unsigned long data) { -struct msmsdcc_dma_data *dma_data = - container_of(cmd, struct msmsdcc_dma_data, hdr); -struct msmsdcc_host *host = dma_data->host; +struct msmsdcc_host *host = (struct msmsdcc_host *)data;unsigned long flags;struct mmc_request *mrq; +struct msm_dmov_errdata err;spin_lock_irqsave (&host->lock, flags); host->dma.active =0 ; + err = host->dma.err; mrq = host->curr.mrq;BUG_ON (!mrq);WARN_ON (!mrq->data); -if (!(result &DMOV_RSLT_VALID )) { +if (!(host->dma.result &DMOV_RSLT_VALID )) {pr_err ("msmsdcc: Invalid DataMover result\n" ); goto out; } -if (result &DMOV_RSLT_DONE ) { +if (host->dma.result &DMOV_RSLT_DONE ) { host->curr.data_xfered = host->curr.xfer_size; }else {/* Error or flush */ -if (result &DMOV_RSLT_ERROR ) +if (host->dma.result &DMOV_RSLT_ERROR )pr_err ("%s: DMA error (0x%.8x)\n" , -mmc_hostname (host->mmc), result); -if (result &DMOV_RSLT_FLUSH ) +mmc_hostname (host->mmc), host->dma.result); +if (host->dma.result &DMOV_RSLT_FLUSH )pr_err ("%s: DMA channel flushed (0x%.8x)\n" , -mmc_hostname (host->mmc), result); -if (err) -pr_err ("Flush data: %.8x %.8x %.8x %.8x %.8x %.8x\n" , - err->flush[0 ], err->flush[1 ], err->flush[2 ], - err->flush[3 ], err->flush[4 ], err->flush[5 ]); +mmc_hostname (host->mmc), host->dma.result); + +pr_err ("Flush data: %.8x %.8x %.8x %.8x %.8x %.8x\n" , + err.flush[0 ], err.flush[1 ], err.flush[2 ], + err.flush[3 ], err.flush[4 ], err.flush[5 ]);if (!mrq->data->error) mrq->data->error = -EIO ; }
@@ -273,6 +271,22 @@ out:return ; } +static void +msmsdcc_dma_complete_func (struct msm_dmov_cmd *cmd, +unsigned int result, +struct msm_dmov_errdata *err) +{ +struct msmsdcc_dma_data *dma_data = + container_of(cmd, struct msmsdcc_dma_data, hdr); +struct msmsdcc_host *host = dma_data->host; + + dma_data->result = result; +if (err) +memcpy (&dma_data->err, err,sizeof (struct msm_dmov_errdata )); + +tasklet_schedule (&host->dma_tlet); +} +static int validate_dma (struct msmsdcc_host *host,struct mmc_data *data) {if (host->dma.channel ==-1 )
@@ -1118,6 +1132,9 @@ msmsdcc_probe(struct platform_device *pdev) host->dmares = dmares;spin_lock_init (&host->lock); +tasklet_init (&host->dma_tlet, msmsdcc_dma_complete_tlet, + (unsigned long )host); +/* * Setup DMA */
drivers/mmc/host/msm_sdcc.h +3 -0
@@ -172,6 +172,8 @@ struct msmsdcc_dma_data {struct msmsdcc_host *host;int busy;/* Set if DM is busy */ int active; +unsigned int result; +struct msm_dmov_errdata err; };struct msmsdcc_pio_data {
@@ -235,6 +237,7 @@ struct msmsdcc_host {int cmdpoll;struct msmsdcc_stats stats; +struct tasklet_struct dma_tlet;/* Command parameters */ unsigned int cmd_timeout;unsigned int cmd_pio_irqmask;
Signature · 5 lines
1.7.1 -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH v2 2/5 mmc: msm_sdcc: Add prog done interrupt support
Enable prog done interrupt for stop command(CMD12) that is sent after a multi-block write(CMD25). The PROG_DONE bit is set when the card has finished its programming and is ready for next data.
After every write request the card will be polled for ready status using CMD13. For a multi-block write(CMD25) before sending CMD13, stop command (CMD12) will be sent. If we enable prog done interrupt for CMD12, then CMD13 polling can be avoided. The prog done interrupt means that the card is done with its programming and is ready for next request.
--- drivers/mmc/host/msm_sdcc.c | 28 +++++++++++++++++++++++++--- drivers/mmc/host/msm_sdcc.h | 4 +++- 2 files changed, 28 insertions(+), 4 deletions(-)
drivers/mmc/host/msm_sdcc.c +25 -3
@@ -438,6 +438,11 @@ msmsdcc_start_command_deferred(struct msmsdcc_host *host, (cmd->opcode == 53)) *c |=MCI_CSPM_DATCMD ; +if (host->prog_scan && (cmd->opcode ==12 )) { + *c |=MCI_CPSM_PROGENA ; + host->prog_enable = true; + } +if (cmd == cmd->mrq->stop) *c |=MCI_CSPM_MCIABORT ;
@@ -508,6 +513,8 @@ msmsdcc_start_data(struct msmsdcc_host *host, struct mmc_data *data, host->cmd_c = c; }msm_dmov_enqueue_cmd (host->dma.channel, &host->dma.hdr); +if (data->flags &MMC_DATA_WRITE ) + host->prog_scan = true; }else {msmsdcc_writel (host, timeout,MMCIDATATIMER );
@@ -718,8 +725,23 @@ static void msmsdcc_do_cmdirq(struct msmsdcc_host *host, uint32_t status)else if (host->curr.data) {/* Non DMA */ msmsdcc_stop_data (host);msmsdcc_request_end (host, cmd->mrq); - }else /* host->data == NULL */ -msmsdcc_request_end (host, cmd->mrq); + }else {/* host->data == NULL */ +if (!cmd->error && host->prog_enable) { +if (status &MCI_PROGDONE ) { + host->prog_scan = false; + host->prog_enable = false; +msmsdcc_request_end (host, cmd->mrq); + }else { + host->curr.cmd = cmd; + } + }else { +if (host->prog_enable) { + host->prog_scan = false; + host->prog_enable = false; + } +msmsdcc_request_end (host, cmd->mrq); + } + } }else if (cmd->data)if (!(cmd->data->flags &MMC_DATA_READ )) msmsdcc_start_data(host, cmd->data,
@@ -733,7 +755,7 @@ msmsdcc_handle_irq_data(struct msmsdcc_host *host, u32 status,struct mmc_data *data = host->curr.data;if (status & (MCI_CMDSENT |MCI_CMDRESPEND |MCI_CMDCRCFAIL | -MCI_CMDTIMEOUT ) && host->curr.cmd) { +MCI_CMDTIMEOUT |MCI_PROGDONE ) && host->curr.cmd) {msmsdcc_do_cmdirq (host, status); }
drivers/mmc/host/msm_sdcc.h +3 -1
@@ -138,7 +138,7 @@#define MCI_IRQENABLE \ (MCI_CMDCRCFAILMASK|MCI_DATACRCFAILMASK|MCI_CMDTIMEOUTMASK| \ MCI_DATATIMEOUTMASK|MCI_TXUNDERRUNMASK|MCI_RXOVERRUNMASK| \ - MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATAENDMASK) + MCI_CMDRESPENDMASK|MCI_CMDSENTMASK|MCI_DATAENDMASK|MCI_PROGDONEMASK)/* * The size of the FIFO in bytes.
@@ -245,6 +245,8 @@ struct msmsdcc_host {struct mmc_command *cmd_cmd;u32 cmd_c; +bool prog_scan; + bool prog_enable; };#endif
Signature · 5 lines
1.7.1 -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH v2 3/5 mmc: msm_sdcc: Reset SDCC in case of data transfer errors
In reply to Sahitya TummalaSDCC uses an asynchronous FIFOs for data synchronization (one for TX and one for RX). For any error when DPSM (Data path state machine) is involved the transfer is terminated with the remaining data stuck inside FIFOs. Reset the controller in case of data errors to ensure that any left over data in FIFOs is flushed out and DPSM is in good state.
The following problems are observed without this reset functionality -
1. After the card is removed in an unsafe way (removed when there is an on going data transfer), the card will not be detected upon its next insertion. This is because the controller wouldn't respond to few initialization commands.
2. When an error occurs for a data transfer in non-DMA mode, sometimes we get spurious PIO interrupt after the request is processed.
--- drivers/mmc/host/msm_sdcc.c | 39 +++++++++++++++++++++++++++++++++++++++ 1 files changed, 39 insertions(+), 0 deletions(-)
drivers/mmc/host/msm_sdcc.c +39 -0
@@ -44,6 +44,7 @@#include <mach/mmc.h> #include <mach/msm_iomap.h> #include <mach/dma.h> +#include <mach/clk.h> #include "msm_sdcc.h"
@@ -126,6 +127,40 @@ static void msmsdcc_start_command(struct msmsdcc_host *host, struct mmc_command *cmd, u32 c); +static void msmsdcc_reset_and_restore (struct msmsdcc_host *host) +{ +u32 mci_clk =0 ; +u32 mci_mask0 =0 ; +int ret =0 ; + +/* Save the controller state */ + mci_clk =readl (host->base +MMCICLOCK ); + mci_mask0 =readl (host->base +MMCIMASK0 ); + +/* Reset the controller */ + ret =clk_reset (host->clk,CLK_RESET_ASSERT ); +if (ret) +pr_err ("%s: Clock assert failed at %u Hz with err %d\n" , +mmc_hostname (host->mmc), host->clk_rate, ret); + + ret =clk_reset (host->clk,CLK_RESET_DEASSERT ); +if (ret) +pr_err ("%s: Clock deassert failed at %u Hz with err %d\n" , +mmc_hostname (host->mmc), host->clk_rate, ret); + +pr_info ("%s: Controller has been re-initialiazed\n" , +mmc_hostname (host->mmc)); + +/* Restore the contoller state */ +writel (host->pwr, host->base +MMCIPOWER ); +writel (mci_clk, host->base +MMCICLOCK ); +writel (mci_mask0, host->base +MMCIMASK0 ); + ret =clk_set_rate (host->clk, host->clk_rate); +if (ret) +pr_err ("%s: Failed to set clk rate %u Hz (%d)\n" , +mmc_hostname (host->mmc), host->clk_rate, ret); +} +static void msmsdcc_request_end (struct msmsdcc_host *host,struct mmc_request *mrq) {
@@ -223,6 +258,8 @@ msmsdcc_dma_complete_tlet(unsigned long data)pr_err ("Flush data: %.8x %.8x %.8x %.8x %.8x %.8x\n" , err.flush[0 ], err.flush[1 ], err.flush[2 ], err.flush[3 ], err.flush[4 ], err.flush[5 ]); + +msmsdcc_reset_and_restore (host);if (!mrq->data->error) mrq->data->error = -EIO ; }
@@ -723,6 +760,7 @@ static void msmsdcc_do_cmdirq(struct msmsdcc_host *host, uint32_t status)msm_dmov_stop_cmd (host->dma.channel, &host->dma.hdr,0 ); else if (host->curr.data) { /* Non DMA */ +msmsdcc_reset_and_restore (host);msmsdcc_stop_data (host);msmsdcc_request_end (host, cmd->mrq); } else { /* host->data == NULL */
@@ -771,6 +809,7 @@ msmsdcc_handle_irq_data(struct msmsdcc_host *host, u32 status,msm_dmov_stop_cmd (host->dma.channel, &host->dma.hdr,0 ); else { +msmsdcc_reset_and_restore (host);if (host->curr.data)msmsdcc_stop_data (host);if (!data->stop)
Signature · 5 lines
1.7.1 -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH v2 4/5 mmc: msm_sdcc: Fix bug in PIO mode when data size is not word aligned
In reply to Sahitya TummalaThe current code for PIO doesn't transfer whole data when data size is not in multiple of 4 bytes. The last few bytes are not written to the card resulting in no DATAEND interrupt from SDCC. This patch allows data transfer for non-aligned data size in PIO mode.
--- drivers/mmc/host/msm_sdcc.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)
drivers/mmc/host/msm_sdcc.c +6 -2
@@ -613,6 +613,9 @@ msmsdcc_pio_read(struct msmsdcc_host *host, char *buffer, unsigned int remain)uint32_t *ptr = (uint32_t *) buffer;int count =0 ; +if (remain %4 ) + remain = ((remain >>2 ) +1 ) <<2 ; +while (msmsdcc_readl (host,MMCISTATUS ) &MCI_RXDATAAVLBL ) { *ptr =msmsdcc_readl (host,MMCIFIFO + (count %MCI_FIFOSIZE )); ptr++;
@@ -633,13 +636,14 @@ msmsdcc_pio_write(struct msmsdcc_host *host, char *buffer,char *ptr = buffer; do { - unsigned int count, maxcnt; + unsigned int count, maxcnt, sz; maxcnt = status & MCI_TXFIFOEMPTY ? MCI_FIFOSIZE : MCI_FIFOHALFSIZE; count = min(remain, maxcnt); - writesl(base + MMCIFIFO, ptr, count >> 2); + sz = count % 4 ? (count >> 2) + 1 : (count >> 2); + writesl(base + MMCIFIFO, ptr, sz); ptr += count; remain -= count;
Signature · 5 lines
1.7.1 -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
PATCH v2 5/5 mmc: msm_sdcc: Check for only DATA_END interrupt to end a request
In reply to Sahitya TummalaThe current code checks for both DATA_END and DATA_BLK_END bits in MCI_STATUS register and ends a request only if both are set at a time. The hardware doesn't always set DATA_BLK_END when DATA_END is set. But DATA_END status itself is sufficient condition from hardware that data transfer is done and hence, check for only DATA_END interrupt in software to end a request.
--- drivers/mmc/host/msm_sdcc.c | 15 ++++----------- drivers/mmc/host/msm_sdcc.h | 1 - 2 files changed, 4 insertions(+), 12 deletions(-)
drivers/mmc/host/msm_sdcc.c +4 -11
@@ -190,7 +190,7 @@ static void msmsdcc_stop_data(struct msmsdcc_host *host) { host->curr.data =NULL ; - host->curr.got_dataend = host->curr.got_datablkend =0 ; + host->curr.got_dataend =0 ; }uint32_t msmsdcc_fifo_addr (struct msmsdcc_host *host)
@@ -277,8 +277,7 @@ msmsdcc_dma_complete_tlet(unsigned long data) host->dma.sg =NULL ; host->dma.busy =0 ; -if ((host->curr.got_dataend && host->curr.got_datablkend) - || mrq->data->error) { +if (host->curr.got_dataend || mrq->data->error) {/* * If we've already gotten our DATAEND / DATABLKEND
@@ -506,7 +505,6 @@ msmsdcc_start_data(struct msmsdcc_host *host, struct mmc_data *data, host->curr.xfer_remain = host->curr.xfer_size; host->curr.data_xfered =0 ; host->curr.got_dataend =0 ; - host->curr.got_datablkend =0 ;memset (&host->pio,0 ,sizeof (host->pio));
@@ -827,14 +825,10 @@ msmsdcc_handle_irq_data(struct msmsdcc_host *host, u32 status,if (!host->curr.got_dataend && (status &MCI_DATAEND )) host->curr.got_dataend =1 ; -if (!host->curr.got_datablkend && (status &MCI_DATABLOCKEND )) - host->curr.got_datablkend =1 ; -/* * If DMA is still in progress, we complete via the completion handler */ -if (host->curr.got_dataend && host->curr.got_datablkend && - !host->dma.busy) { +if (host->curr.got_dataend && !host->dma.busy) {/* * There appears to be an issue in the controller where * if you request a small block transfer (< fifo size),
@@ -871,8 +865,7 @@ msmsdcc_irq(int irq, void *dev_id) do { status = msmsdcc_readl(host, MMCISTATUS); - status &= (msmsdcc_readl(host, MMCIMASK0) | - MCI_DATABLOCKENDMASK); + status &= msmsdcc_readl(host, MMCIMASK0); msmsdcc_writel(host, status, MMCICLEAR); if (status & MCI_SDIOINTR)
drivers/mmc/host/msm_sdcc.h +0 -1
@@ -190,7 +190,6 @@ struct msmsdcc_curr_req {unsigned int xfer_remain;/* Bytes remaining to send */ unsigned int data_xfered;/* Bytes acked by BLKEND irq */ int got_dataend; -int got_datablkend;int user_pages; };
Signature · 5 lines
1.7.1 -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Quoted 7 lines · Sahitya Tummala
In the context of request processing thread, data mover lock is acquired after the host lock. In another context, in the completion handler of data mover the locks are acquired in the reverse order, resulting in possible circular lock dependency warning. Hence, schedule a tasklet to process the dma completion so as to avoid nested locks.
Can you just alter the location where the locks are taken ?
This lock,
spin_lock_irqsave(&host->lock, flags);
seems not fine grained at all .. That lock is just being held over very large sections of code which it doesn't seems like it needs to be. It doesn't appear to be protecting just the data structures.
Daniel
Signature · 2 lines
Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.
Hi Daniel,
Quoted 11 lines · Daniel Walker
Quoted 7 lines · Sahitya Tummala
In the context of request processing thread, data mover lock is acquired after the host lock. In another context, in the completion handler of data mover the locks are acquired in the reverse order, resulting in possible circular lock dependency warning. Hence, schedule a tasklet to process the dma completion so as to avoid nested locks.
Can you just alter the location where the locks are taken ?
Quoted 7 lines
This lock,
spin_lock_irqsave(&host->lock, flags);seems not fine grained at all .. That lock is just being held over very large sections of code which it doesn't seems like it needs to be. It doesn't appear to be protecting just the data structures.
Yes, it seems we are using the lock in this function not just for required host data.
But to solve this circular locking issue, it looks like we don't need host lock before enqueing the data mover request. I will analyze more and confirm.
Signature · 6 lines
Thanks, Sahitya. -- Sent by a consultant of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.