> >>>>> +static int tegra_dma_terminate_all(struct dma_chan *dc)
> >>>>> +{
> >>>>> + struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> >>>>> + unsigned long flags;
> >>>>> + LIST_HEAD(head);
> >>>>> + int err;
> >>>>> +
> >>>>> + if (tdc->dma_desc) {
> >>>>
> >>>> Needs locking protection against racing with the interrupt handler.
> >>> tegra_dma_stop_client() waits for the in-flight transfer
> >>> to complete and prevents any additional transfer to start.
> >>> Wouldn't it manage the race? Do you see any potential issue there?
> >>
> >> You should consider interrupt handler like a process running in a
> >> parallel thread. The interrupt handler sets tdc->dma_desc to NULL, hence
> >> you'll get NULL dereference in tegra_dma_stop_client().
> >
> > Is it better if I remove the below part from tegra_dma_stop_client() so
> > that dma_desc is not accessed at all?
> >
> > + wcount = tdc_read(tdc, TEGRA_GPCDMA_CHAN_XFER_COUNT);
> > + tdc->dma_desc->bytes_transferred +=
> > + tdc->dma_desc->bytes_requested - (wcount * 4);
> >
> > Because I don't see a point in updating the value there. dma_desc is set
> > to NULL in the next step in terminate_all() anyway.
>
> That isn't going help you much because you also can't release DMA
> descriptor while interrupt handler still may be running and using that
> descriptor.
Does the below functions look good to resolve the issue, provided
tegra_dma_stop_client() doesn't access dma_desc?
+static int tegra_dma_terminate_all(struct dma_chan *dc)
+{
+ struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
+ unsigned long flags;
+ LIST_HEAD(head);
+ int err;
+
+ err = tegra_dma_stop_client(tdc);
+ if (err)
+ return err;
+
+ tegra_dma_stop(tdc);
+
+ spin_lock_irqsave(&tdc->vc.lock, flags);
+ tegra_dma_sid_free(tdc);
+ tdc->dma_desc = NULL;
+
+ vchan_get_all_descriptors(&tdc->vc, &head);
+ spin_unlock_irqrestore(&tdc->vc.lock, flags);
+
+ vchan_dma_desc_free_list(&tdc->vc, &head);
+
+ return 0;
+}
+static irqreturn_t tegra_dma_isr(int irq, void *dev_id)
+{
+ struct tegra_dma_channel *tdc = dev_id;
+ struct tegra_dma_desc *dma_desc = tdc->dma_desc;
+ struct tegra_dma_sg_req *sg_req;
+ u32 status;
+
+ /* Check channel error status register */
+ status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_ERR_STATUS);
+ if (status) {
+ tegra_dma_chan_decode_error(tdc, status);
+ tegra_dma_dump_chan_regs(tdc);
+ tdc_write(tdc, TEGRA_GPCDMA_CHAN_ERR_STATUS, 0xFFFFFFFF);
+ }
+
+ status = tdc_read(tdc, TEGRA_GPCDMA_CHAN_STATUS);
+ if (!(status & TEGRA_GPCDMA_STATUS_ISE_EOC))
+ return IRQ_HANDLED;
+
+ tdc_write(tdc, TEGRA_GPCDMA_CHAN_STATUS,
+ TEGRA_GPCDMA_STATUS_ISE_EOC);
+
+ spin_lock(&tdc->vc.lock);
+ if (!dma_desc)
+ goto irq_done;
+
+ sg_req = dma_desc->sg_req;
+ dma_desc->bytes_transferred += sg_req[dma_desc->sg_idx - 1].len;
+
+ if (dma_desc->sg_idx == dma_desc->sg_count) {
+ tegra_dma_xfer_complete(tdc);
+ } else if (dma_desc->cyclic) {
+ vchan_cyclic_callback(&dma_desc->vd);
+ tegra_dma_configure_next_sg(tdc);
+ } else {
+ tegra_dma_start(tdc);
+ }
+
+irq_done:
+ spin_unlock(&tdc->vc.lock);
+ return IRQ_HANDLED;
+}
Thanks,
Akhil
