[PATCH 1/4] mmc: pxamci: Use the right flags for DMA callback init

[PATCH 1/4] mmc: pxamci: Use the right flags for DMA callback init

[Petr Cvek]

The MMC_DATA_READ and the MMC_DATA_WRITE flags for the mmc request are not
mutually exclusive (two different bits). Change the callback initialization
code to use the proper one.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/mmc/host/pxamci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index c763b404510f..80bc8065b50f 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -235,7 +235,7 @@ static void pxamci_setup_data(struct pxamci_host *host, struct mmc_data *data)
 		return;
 	}
 
-	if (!(data->flags & MMC_DATA_READ)) {
+	if (data->flags & MMC_DATA_WRITE) {
 		tx->callback = pxamci_dma_irq;
 		tx->callback_param = host;
 	}
-- 
2.11.0

[PATCH 1/4] mmc: pxamci: Use the right flags for DMA callback init

[Petr Cvek]

Dne 19.4.2017 v 01:16 Petr Cvek napsal(a):
> The MMC_DATA_READ and the MMC_DATA_WRITE flags for the mmc request are not
> mutually exclusive (two different bits). Change the callback initialization
> code to use the proper one.
> 
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>

It seems I forgot to patch a code in the pxamci_setup_data():

	config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
	config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
	config.src_addr = host->res->start + MMC_RXFIFO;
	config.dst_addr = host->res->start + MMC_TXFIFO;
	config.src_maxburst = 32;
	config.dst_maxburst = 32;

	if (data->flags & MMC_DATA_READ) {
		host->dma_dir = DMA_FROM_DEVICE;
		direction = DMA_DEV_TO_MEM;
		chan = host->dma_chan_rx;
	} else {
		host->dma_dir = DMA_TO_DEVICE;
		direction = DMA_MEM_TO_DEV;
		chan = host->dma_chan_tx;
	}

I will add it in v2 series.

Just curious: why does the MMC_DATA_READ/MMC_DATA_WRITE flag occupies two bits anyway is there a hardware which can do both request at the same time?

BTW that config.* block should be IMO split and moved into that if(){} too (as the PXA27x do only one DMA direction at the time).

Petr

[PATCH 2/4] mmc: pxamci: Enhance error checking

[Petr Cvek]

The pxamci_dma_irq() and pxamci_data_done() should print errors if they
obtains invalid parameters. Make the pxamci_dma_irq() call with
the MMC_DATA_READ flag a fault as the DMA callback is used only for write.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/mmc/host/pxamci.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 80bc8065b50f..48c26d848e9f 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -354,13 +354,22 @@ static int pxamci_data_done(struct pxamci_host *host, unsigned int stat)
 	struct mmc_data *data = host->data;
 	struct dma_chan *chan;
 
-	if (!data)
+	if (!data) {
+		pr_err("%s: Missing data structure\n",
+			mmc_hostname(host->mmc));
 		return 0;
+	}
 
 	if (data->flags & MMC_DATA_READ)
 		chan = host->dma_chan_rx;
-	else
+	else if (data->flags & MMC_DATA_WRITE)
 		chan = host->dma_chan_tx;
+	else {
+		pr_err("%s: Unknown data direction, flags=%08x\n",
+			mmc_hostname(host->mmc), data->flags);
+		return 0;
+	}
+
 	dma_unmap_sg(chan->device->dev,
 		     data->sg, data->sg_len, host->dma_dir);
 
@@ -558,21 +567,27 @@ static void pxamci_dma_irq(void *param)
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	if (!host->data)
+	if (!host->data) {
+		pr_err("%s: Missing data structure\n",
+			mmc_hostname(host->mmc));
 		goto out_unlock;
+	}
 
-	if (host->data->flags & MMC_DATA_READ)
-		chan = host->dma_chan_rx;
-	else
-		chan = host->dma_chan_tx;
+	if (!(host->data->flags & MMC_DATA_WRITE)) {
+		pr_err("%s: DMA callback is only for tx channel, flags=%x\n",
+			mmc_hostname(host->mmc), host->data->flags);
+		goto out_unlock;
+	}
+
+	chan = host->dma_chan_tx;
 
 	status = dmaengine_tx_status(chan, host->dma_cookie, &state);
 
 	if (likely(status == DMA_COMPLETE)) {
 		writel(BUF_PART_FULL, host->base + MMC_PRTBUF);
 	} else {
-		pr_err("%s: DMA error on %s channel\n", mmc_hostname(host->mmc),
-			host->data->flags & MMC_DATA_READ ? "rx" : "tx");
+		pr_err("%s: Invalid DMA status %i\n", mmc_hostname(host->mmc),
+			status);
 		host->data->error = -EIO;
 		pxamci_data_done(host, 0);
 	}
-- 
2.11.0

[PATCH 2/4] mmc: pxamci: Enhance error checking

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> The pxamci_dma_irq() and pxamci_data_done() should print errors if they
> obtains invalid parameters. Make the pxamci_dma_irq() call with
> the MMC_DATA_READ flag a fault as the DMA callback is used only for write.
>
> Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
> ---
>  drivers/mmc/host/pxamci.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
> index 80bc8065b50f..48c26d848e9f 100644
> --- a/drivers/mmc/host/pxamci.c
> +++ b/drivers/mmc/host/pxamci.c
> @@ -354,13 +354,22 @@ static int pxamci_data_done(struct pxamci_host *host, unsigned int stat)
>  	struct mmc_data *data = host->data;
>  	struct dma_chan *chan;
>  
> -	if (!data)
> +	if (!data) {
> +		pr_err("%s: Missing data structure\n",
> +			mmc_hostname(host->mmc));
I'd rather have :
    dev_err(mmc_dev(mmc), ...)

>  		return 0;
> +	}
>  
>  	if (data->flags & MMC_DATA_READ)
>  		chan = host->dma_chan_rx;
> -	else
> +	else if (data->flags & MMC_DATA_WRITE)
>  		chan = host->dma_chan_tx;
> +	else {
> +		pr_err("%s: Unknown data direction, flags=%08x\n",
> +			mmc_hostname(host->mmc), data->flags);
Ditto.

> +	if (!host->data) {
> +		pr_err("%s: Missing data structure\n",
> +			mmc_hostname(host->mmc));
Ditto.

>  		goto out_unlock;
> +	}
>  
> -	if (host->data->flags & MMC_DATA_READ)
> -		chan = host->dma_chan_rx;
> -	else
> -		chan = host->dma_chan_tx;
> +	if (!(host->data->flags & MMC_DATA_WRITE)) {
> +		pr_err("%s: DMA callback is only for tx channel, flags=%x\n",
> +			mmc_hostname(host->mmc), host->data->flags);
Ditto.

> +		goto out_unlock;
> +	}
> +
> +	chan = host->dma_chan_tx;
>  
>  	status = dmaengine_tx_status(chan, host->dma_cookie, &state);
>  
>  	if (likely(status == DMA_COMPLETE)) {
>  		writel(BUF_PART_FULL, host->base + MMC_PRTBUF);
>  	} else {
> -		pr_err("%s: DMA error on %s channel\n", mmc_hostname(host->mmc),
> -			host->data->flags & MMC_DATA_READ ? "rx" : "tx");
> +		pr_err("%s: Invalid DMA status %i\n", mmc_hostname(host->mmc),
> +			status);
Ditto.

Cheers.

-- 
Robert

[PATCH 3/4] mmc: pxamci: Disable DATA_TRAN_DONE interrupt sooner

[Petr Cvek]

Disable the DATA_TRAN_DONE interrupt as soon as possible in the handler.

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/mmc/host/pxamci.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 48c26d848e9f..570735a10127 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -354,6 +354,8 @@ static int pxamci_data_done(struct pxamci_host *host, unsigned int stat)
 	struct mmc_data *data = host->data;
 	struct dma_chan *chan;
 
+	pxamci_disable_irq(host, DATA_TRAN_DONE);
+
 	if (!data) {
 		pr_err("%s: Missing data structure\n",
 			mmc_hostname(host->mmc));
@@ -389,8 +391,6 @@ static int pxamci_data_done(struct pxamci_host *host, unsigned int stat)
 	else
 		data->bytes_xfered = 0;
 
-	pxamci_disable_irq(host, DATA_TRAN_DONE);
-
 	host->data = NULL;
 	if (host->mrq->stop) {
 		pxamci_stop_clock(host);
-- 
2.11.0

[PATCH 3/4] mmc: pxamci: Disable DATA_TRAN_DONE interrupt sooner

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Disable the DATA_TRAN_DONE interrupt as soon as possible in the handler.
Yeah, but why, please explain.
If that's only to "reduce" the race occurrence, then I'd rather have this patch
dropped. Otherwise if there is a compelling reason let's see ...

Cheers.

-- 
Robert

[PATCH 3/4] mmc: pxamci: Disable DATA_TRAN_DONE interrupt sooner

[Petr Cvek]

Dne 19.4.2017 v 21:14 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> Disable the DATA_TRAN_DONE interrupt as soon as possible in the handler.
> Yeah, but why, please explain.
> If that's only to "reduce" the race occurrence, then I'd rather have this patch
> dropped. Otherwise if there is a compelling reason let's see ...
> 

I created it during researching where put the IRQ enable in the callback and then I left it that way the data done interrupt will be disabled if there was an error at the beginning of the pxamci_data_done(). 

I dropped the patch and re-tested and it works (in the case the tests at the start of the pxamci_data_done() fail there will be probably some irq hell though :-D). 

> Cheers.
> 

[PATCH 4/4] mmc: pxamci: Fix race condition between pxamci_dma_irq() and pxamci_irq()

[Petr Cvek]

The data write requests may require an FIFO flush when the DMA transaction
ends. This is handled by a DMA callback pxamci_dma_irq(). After flushing
the FIFO the MCI controller generates the DATA_TRAN_DONE interrupt.

Problem is the DATA_TRAN_DONE interrupt will be generated when the write
data length is divisible by the FIFO size (no flush is required). And in
this case the DMA callback can be called long time after the
DATA_TRAN_DONE interrupt (as the DMA callback is realised by a tasklet,
it can even stack). When the DMA callback is finally called there can
already be a different type of the transaction (another data read or write
request).

The dmaengine_tx_status() will be called for a wrong DMA transaction and
in some case it returns DMA_IN_PROGRESS, which the code recognize as
an error and ends a running DMA and halts the MCI controller.

The problem presents itself under heavy (interrupt) load with a high MCI
traffic with this message:

	mmc0: DMA error on tx channel

The fix must obey these situations:
 - Any command will erase the FIFO
 - Data writes divisible by the FIFO size will (probably) automatically
   generate a DATA_TRAN_DONE interrupt
 - Data writes with a nonzero FIFO remainder must be flushed and then MCI
   generates a DATA_TRAN_DONE interrupt
 - Data reads do not require a flush but they will generate
   a DATA_TRAN_DONE interrupt

The fix changes the DATA_TRAN_DONE interrupt enable from read/write
requests to read requests. The DATA_TRAN_DONE interrupt for a write
request is enabled in the DMA callback, this assures  a DATA_TRAN_DONE
interrupt will be always called after a callback (with or without an FIFO
flush).

Signed-off-by: Petr Cvek <petr.cvek@tul.cz>
---
 drivers/mmc/host/pxamci.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/pxamci.c b/drivers/mmc/host/pxamci.c
index 570735a10127..08713bb6c716 100644
--- a/drivers/mmc/host/pxamci.c
+++ b/drivers/mmc/host/pxamci.c
@@ -335,7 +335,9 @@ static int pxamci_cmd_done(struct pxamci_host *host, unsigned int stat)
 
 	pxamci_disable_irq(host, END_CMD_RES);
 	if (host->data && !cmd->error) {
-		pxamci_enable_irq(host, DATA_TRAN_DONE);
+		if (host->data->flags & MMC_DATA_READ)
+			pxamci_enable_irq(host, DATA_TRAN_DONE);
+
 		/*
 		 * workaround for erratum #91, if doing write
 		 * enable DMA late
@@ -585,6 +587,9 @@ static void pxamci_dma_irq(void *param)
 
 	if (likely(status == DMA_COMPLETE)) {
 		writel(BUF_PART_FULL, host->base + MMC_PRTBUF);
+
+		/* NOTICE pxamci_irq() is dependent on pxamci_dma_irq() */
+		pxamci_enable_irq(host, DATA_TRAN_DONE);
 	} else {
 		pr_err("%s: Invalid DMA status %i\n", mmc_hostname(host->mmc),
 			status);
-- 
2.11.0

[PATCH 4/4] mmc: pxamci: Fix race condition between pxamci_dma_irq() and pxamci_irq()

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> The data write requests may require an FIFO flush when the DMA transaction
> ends. This is handled by a DMA callback pxamci_dma_irq(). After flushing
> the FIFO the MCI controller generates the DATA_TRAN_DONE interrupt.
>
> Problem is the DATA_TRAN_DONE interrupt will be generated when the write
> data length is divisible by the FIFO size (no flush is required). And in
> this case the DMA callback can be called long time after the
> DATA_TRAN_DONE interrupt (as the DMA callback is realised by a tasklet,
> it can even stack). When the DMA callback is finally called there can
> already be a different type of the transaction (another data read or write
> request).
>
> The dmaengine_tx_status() will be called for a wrong DMA transaction and
> in some case it returns DMA_IN_PROGRESS, which the code recognize as
> an error and ends a running DMA and halts the MCI controller.
>
> The problem presents itself under heavy (interrupt) load with a high MCI
> traffic with this message:
>
> 	mmc0: DMA error on tx channel
>
> The fix must obey these situations:
>  - Any command will erase the FIFO
>  - Data writes divisible by the FIFO size will (probably) automatically
>    generate a DATA_TRAN_DONE interrupt
>  - Data writes with a nonzero FIFO remainder must be flushed and then MCI
>    generates a DATA_TRAN_DONE interrupt
>  - Data reads do not require a flush but they will generate
>    a DATA_TRAN_DONE interrupt
>
> The fix changes the DATA_TRAN_DONE interrupt enable from read/write
> requests to read requests. The DATA_TRAN_DONE interrupt for a write
> request is enabled in the DMA callback, this assures  a DATA_TRAN_DONE
> interrupt will be always called after a callback (with or without an FIFO
> flush).

I'm a bit concerned with the way this patch works.
What bothers me is the re-enabling of the interrupt source in the DMA completion
path, ie. in pxamci_dma_irq().
For example, imagine :
 - the tran_done bit is left set (for whatever reason)
 - a new transation is queued
 - the DMA finishes, but not the last request
 - the pxamci_enable_irq() enables the interrupt, which fires right away even if
   the tran_done for this interrupt wasn't yet set

I will need a bit more time to think this one through, as I'm not yet set about
all the consequences. That shouldn't prevent you from pushing for reviews of
these patches of course, as I think this serie (or an equivalent) is required to
fix the current race condition.

As this is the last patch, I wonder if this serie is bisectable, especially is
patch 1/4 self contained ?

Cheers.

--
Robert

[PATCH 4/4] mmc: pxamci: Fix race condition between pxamci_dma_irq() and pxamci_irq()

[Petr Cvek]

Dne 19.4.2017 v 21:22 Robert Jarzmik napsal(a):
> Petr Cvek <petr.cvek@tul.cz> writes:
> 
>> The data write requests may require an FIFO flush when the DMA transaction
>> ends. This is handled by a DMA callback pxamci_dma_irq(). After flushing
>> the FIFO the MCI controller generates the DATA_TRAN_DONE interrupt.
>>
>> Problem is the DATA_TRAN_DONE interrupt will be generated when the write
>> data length is divisible by the FIFO size (no flush is required). And in
>> this case the DMA callback can be called long time after the
>> DATA_TRAN_DONE interrupt (as the DMA callback is realised by a tasklet,
>> it can even stack). When the DMA callback is finally called there can
>> already be a different type of the transaction (another data read or write
>> request).
>>
>> The dmaengine_tx_status() will be called for a wrong DMA transaction and
>> in some case it returns DMA_IN_PROGRESS, which the code recognize as
>> an error and ends a running DMA and halts the MCI controller.
>>
>> The problem presents itself under heavy (interrupt) load with a high MCI
>> traffic with this message:
>>
>> 	mmc0: DMA error on tx channel
>>
>> The fix must obey these situations:
>>  - Any command will erase the FIFO
>>  - Data writes divisible by the FIFO size will (probably) automatically
>>    generate a DATA_TRAN_DONE interrupt
>>  - Data writes with a nonzero FIFO remainder must be flushed and then MCI
>>    generates a DATA_TRAN_DONE interrupt
>>  - Data reads do not require a flush but they will generate
>>    a DATA_TRAN_DONE interrupt
>>
>> The fix changes the DATA_TRAN_DONE interrupt enable from read/write
>> requests to read requests. The DATA_TRAN_DONE interrupt for a write
>> request is enabled in the DMA callback, this assures  a DATA_TRAN_DONE
>> interrupt will be always called after a callback (with or without an FIFO
>> flush).
> 
> I'm a bit concerned with the way this patch works.
> What bothers me is the re-enabling of the interrupt source in the DMA completion
> path, ie. in pxamci_dma_irq().
> For example, imagine :
>  - the tran_done bit is left set (for whatever reason)

You mean DATA_TRAN_DONE flag? The datasheet (page for reg MMC_STAT) says it is cleared at the start of the every command sequence. So inside pxamci_data_done() it can stay only activated in the case of an error (after dropping the third patch of this patchset "Disable DATA_TRAN_DONE interrupt sooner") or in the case of no STOP command.

>  - a new transation is queued

In the case of pxa27x the TX DMA (the one with a callback and patched behavior) will start only after the end of the command sequence. But I see, there could be a problem outside the PXA27x SoC (PXA3xx?) as in that case the DMA is started (dma_async_issue_pending()) before the command sequence.

>  - the DMA finishes, but not the last request

That would be a problem on itself because the MCI flushes the FIFO at the every command. Anyway if the DATA_TRAN_DONE flag was left as "1" from the previous transaction. It should cause an interrupt in the original driver version before applying my patch too. If I'm right (and it is possible I'm not) your situation is independent to my patch.

>  - the pxamci_enable_irq() enables the interrupt, which fires right away even if
>    the tran_done for this interrupt wasn't yet set
> 

Maybe we can add some sanity checks in the pxamci_finish_request() or at the start of pxamci_request().

> I will need a bit more time to think this one through, as I'm not yet set about
> all the consequences. That shouldn't prevent you from pushing for reviews of
> these patches of course, as I think this serie (or an equivalent) is required to
> fix the current race condition.

Well during the bug hunting and reading the documentation I was wondering if a hardware could be designed to be incompatible with the kernel subsystems ;-).

> 
> As this is the last patch, I wonder if this serie is bisectable, especially is
> patch 1/4 self contained ?

Applying only the patch 1/4 itself should not change the behavior at all if the mmc core sends only "01" and "10" in these bit fields. The exact way I've found it was that I traced the callback and found it assigns:

	chan = host->dma_chan_rx;

even though it is called only for the TX. I have checked the flags definitions and found they are not a single bit and changed it to more descriptive if(){} (why negate the read if there is write AND they are not mutually exclusive AND callback is for write).

The patches 1, 1+2, 1+2+3 should boot, but the MMC will of course fail as only the 4 repairs it. Do you want me to send them independently? (or I can sort them that the race condition repair is the first one)

> 
> Cheers.
> 
> --
> Robert
> 

[PATCH 4/4] mmc: pxamci: Fix race condition between pxamci_dma_irq() and pxamci_irq()

[Robert Jarzmik]

Petr Cvek <petr.cvek@tul.cz> writes:

> Dne 19.4.2017 v 21:22 Robert Jarzmik napsal(a):
>> Petr Cvek <petr.cvek@tul.cz> writes:

Hi Petr,

As promised, I though it a bit more, and I have a counter proposal, which looks
simpler (if it works for you).

Why not remove completely the call to pxamci_data_done() from pxamci_dma_irq() ?
The pxamci_dma_irq() will only remain for the partial full write, and for the
dev_err() part, but won't act on command completion, that part being full dealt
with by pxamci_data_done().

I still seeing a small race window, in that :
 - DATA_TRAN_DONE is asserted for a MMC read transaction, because the MMC FIFO
   was just emptied by the DMA IP
 - imagine the memory is not yet written to by the DMA IP (ie. this is the race
   window, the DATA being help in DMA IP internal FIFO)
 - the pxamci_data_done() is called, and it calls dma_unmap_sg(), flushing the
   cache
 - the consumer reads this last data bit, which is still the old data, and then
   the DMA finishes ...

I know the probability is almost 0 for this scenario to happen given the
timings, it's just to add fuel to the technical exchange.

> The patches 1, 1+2, 1+2+3 should boot, but the MMC will of course fail as only
> the 4 repairs it. Do you want me to send them independently? (or I can sort
> them that the race condition repair is the first one)
No no, I like it the way it is, and as far as Ulf is happy as his tree will
carry them, I'm happy too.

Cheers.

--
Robert