From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755229AbbCXQc0 (ORCPT ); Tue, 24 Mar 2015 12:32:26 -0400 Received: from mga09.intel.com ([134.134.136.24]:25903 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752767AbbCXQcU (ORCPT ); Tue, 24 Mar 2015 12:32:20 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.11,459,1422950400"; d="scan'208";a="669899120" Date: Tue, 24 Mar 2015 21:58:16 +0530 From: Vinod Koul To: Appana Durga Kedareswara Rao Cc: "dan.j.williams@intel.com" , Michal Simek , Soren Brinkmann , "dmaengine@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Anirudha Sarangi , Srikanth Vemula , Srikanth Thokala Subject: Re: [PATCH v5] dma: Add Xilinx AXI Direct Memory Access Engine driver support Message-ID: <20150324162815.GD32683@intel.com> References: <6f3b34935c0f405199cacd5312e972ac@BN1BFFO11FD017.protection.gbl> <20150317110739.GE32683@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Mar 23, 2015 at 04:24:26PM +0000, Appana Durga Kedareswara Rao wrote: > > > +static enum dma_status xilinx_dma_tx_status(struct dma_chan *dchan, > > > + dma_cookie_t cookie, > > > + struct dma_tx_state *txstate) { > > > + struct xilinx_dma_chan *chan = to_xilinx_chan(dchan); > > > + enum dma_status ret; > > > + unsigned long flags; > > > + > > > + ret = dma_cookie_status(dchan, cookie, txstate); > > > + if (ret != DMA_COMPLETE) { > > txstate can be null > > Ok will modify. > It will be something like below > > ret = dma_cookie_status(dchan, cookie, txstate); > if (ret == DMA_COMPLETE || !txstate) > return ret; > Calculate residue. > > Please correct me if I am wrong Thats right > > > +static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan > > > +*chan) { > > > + struct xilinx_dma_tx_descriptor *desc; > > > + struct xilinx_dma_tx_segment *segment, *next; > > > + struct xilinx_dma_desc_hw *hw; > > > + unsigned long flags; > > > + u32 residue = 0; > > > + > > > + spin_lock_irqsave(&chan->lock, flags); > > > + > > > + desc = chan->active_desc; > > > + if (!desc) { > > > + dev_dbg(chan->dev, "no running descriptors\n"); > > > + goto out_unlock; > > > + } > > > + > > > + if (chan->has_sg) { > > > + list_for_each_entry_safe(segment, next, &desc->segments, > > node) { > > > + hw = &segment->hw; > > > + residue += (hw->control - hw->status) & > > > + XILINX_DMA_MAX_TRANS_LEN; > > > + } > > why are we calculating residue here? > > > This API is called from Interrupt handler when the BD(Buffer Descriptor) is > Successfully transmitted. > Thought of calculating residue here is more accurate than calculating the residue in the > tx_status. Nope, you need to report reside when asked not precompute! > This is while preparing the descs, but I see your point. I will fix > it in my next version of the patch. > > > > > > + async_tx_ack(&desc->async_tx); > > why? > > It is not required? > As far as I know we should ack the descriptor for Slave dma case right? > ( https://www.kernel.org/doc/Documentation/crypto/async-tx-api.txt ) No you dont, for slave you need to read Documentation/dmaengine/ > > > +static int xilinx_dma_remove(struct platform_device *pdev) { > > > + struct xilinx_dma_device *xdev = platform_get_drvdata(pdev); > > > + int i; > > > + > > > + of_dma_controller_free(pdev->dev.of_node); > > > + dma_async_device_unregister(&xdev->common); > > > + > > > + for (i = 0; i < XILINX_DMA_MAX_CHANS_PER_DEVICE; i++) > > > + if (xdev->chan[i]) > > > + xilinx_dma_chan_remove(xdev->chan[i]); > > > + > > at this point your irq is active and tasklet cna still be scheduled > > We are freeing the IRQ and killing the tasklet in the chan_remove API why irq is still active at this point of time? I missed this bit, if you are freeing irq explcitly then it should be okay > I didn't get you. > Could you please explain a bit? -- ~Vinod