RE: [PATCH v16 2/4] dmaengine: tegra: Add tegra gpcdma driver

[Akhil R]

> >>>>> +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