28.01.2020 17:02, Jon Hunter пишет: > > On 16/01/2020 20:10, Dmitry Osipenko wrote: >> 15.01.2020 12:00, Jon Hunter пишет: >>> >>> On 14/01/2020 20:33, Dmitry Osipenko wrote: >>>> 14.01.2020 18:09, Jon Hunter пишет: >>>>> >>>>> On 12/01/2020 17:29, Dmitry Osipenko wrote: >>>>>> I was doing some experiments with I2C and noticed that Tegra APB DMA >>>>>> driver crashes sometime after I2C DMA transfer termination. The crash >>>>>> happens because tegra_dma_terminate_all() bails out immediately if pending >>>>>> list is empty, thus it doesn't release the half-completed descriptors >>>>>> which are getting re-used before ISR tasklet kicks-in. >>>>> >>>>> Can you elaborate a bit more on how these are getting re-used? What is >>>>> the sequence of events which results in the panic? I believe that this >>>>> was also reported in the past [0] and so I don't doubt there is an issue >>>>> here, but would like to completely understand this. >>>>> >>>>> Thanks! >>>>> Jon >>>>> >>>>> [0] https://lore.kernel.org/patchwork/patch/675349/ >>>>> >>>> >>>> In my case it happens in the touchscreen driver during of the >>>> touchscreen's interrupt handling (in a threaded IRQ handler) + CPU is >>>> under load and there is other interrupts activity. So what happens here >>>> is that the TS driver issues one I2C transfer, which fails with >>>> (apparently bogus) timeout (because DMA descriptor is completed and >>>> removed from the pending list, but tasklet not executed yet), and then >>>> TS immediately issues another I2C transfer that re-uses the >>>> yet-incompleted descriptor. That's my understanding. >>> >>> OK, but what is the exact sequence that it allowing it to re-use the >>> incompleted descriptor? >> >> TDMA driver DMA Client >> >> 1. >> dmaengine_prep() >> >> 2. >> tegra_dma_desc_get() >> dma_desc = kzalloc() >> ... >> tegra_dma_prep_slave_sg() >> INIT_LIST_HEAD(&dma_desc->tx_list); >> INIT_LIST_HEAD(&dma_desc->cb_node); >> list_add_tail(sgreq->node, >> dma_desc->tx_list) >> >> 3. >> dma_async_issue_pending() >> >> 4. >> tegra_dma_tx_submit() >> list_splice_tail_init(dma_desc->tx_list, >> tdc->pending_sg_req) >> >> 5. >> tegra_dma_isr() >> ... >> handle_once_dma_done() >> ... >> sgreq = list_first_entry(tdc->pending_sg_req) >> list_del(sgreq->node); >> ... >> list_add_tail(dma_desc->cb_node, >> tdc->cb_desc); >> list_add_tail(dma_desc->node, >> tdc->free_dma_desc); > > Isn't this the problem here, that we have placed this on the free list > before we are actually done? > > It seems to me that there could still be a potential race condition > between the ISR and the tasklet running. Yes, this should be addressed by the patch #3 "dmaengine: tegra-apb: Prevent race conditions of tasklet vs free list".
