| Message ID | 31e332fa-f152-1eff-39fb-91f332b84757@tul.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 >
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
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);
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(-)