| Message ID | 1314924284-25554-5-git-send-email-boojin.kim@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2011-09-02 at 09:44 +0900, Boojin Kim wrote: > Origianl code carries out the start operation after flush operation. Orignal > But start operation is not required for DMA_TERMINATE_ALL command. > So, This patch removes the unnecessary start operation and only carries out this should be all lowercase > the flush oeration for handling DMA_TERMINATE_ALL command. operation ^^^^^^^^ > > Signed-off-by: Boojin Kim <boojin.kim@samsung.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Acked-by: Vinod Koul <vinod.koul@intel.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > --- > drivers/dma/pl330.c | 11 +++++++---- > 1 files changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c > index e7f9d1d..59943ec 100644 > --- a/drivers/dma/pl330.c > +++ b/drivers/dma/pl330.c > @@ -262,10 +262,11 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan) > static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg) > { > struct dma_pl330_chan *pch = to_pchan(chan); > - struct dma_pl330_desc *desc; > + struct dma_pl330_desc *desc, *_dt; > unsigned long flags; > struct dma_pl330_dmac *pdmac = pch->dmac; > struct dma_slave_config *slave_config; > + LIST_HEAD(list); > > switch (cmd) { > case DMA_TERMINATE_ALL: > @@ -275,12 +276,14 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned > pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); > > /* Mark all desc done */ > - list_for_each_entry(desc, &pch->work_list, node) > + list_for_each_entry_safe(desc, _dt, &pch->work_list , node) { > desc->status = DONE; > + pch->completed = desc->txd.cookie; > + list_move_tail(&desc->node, &list); > + } > > + list_splice_tail_init(&list, &pdmac->desc_pool); > spin_unlock_irqrestore(&pch->lock, flags); > - > - pl330_tasklet((unsigned long) pch); > break; > case DMA_SLAVE_CONFIG: > slave_config = (struct dma_slave_config *)arg; If this is the only fix required in entire series, I can fix this up while applying
On Fri, Sep 2, 2011 at 6:14 AM, Boojin Kim <boojin.kim@samsung.com> wrote: > Origianl code carries out the start operation after flush operation. > But start operation is not required for DMA_TERMINATE_ALL command. > So, This patch removes the unnecessary start operation and only carries out > the flush oeration for handling DMA_TERMINATE_ALL command. Not exactly. The 'start' is impotent when called from this path because there is nothing left queued after the call to 'flush'. pl330_tasklet() is called because that is a common function that does the house-keeping acc to what's presently in 'work' and 'done' lists. Though as a side-effect, your patch does avoid doing callbacks on submitted xfers during DMA_TERMINATE_ALL - which may or may not be the right thing to do. I remember seeing some dmac drivers doing the callbacks while some specifically avoiding them during DMA_TERMINATE_ALL. While I personally prefer the former, I guess it's for maintainers to decide. Vinod ?
On Tue, Sep 06, 2011 at 05:52:19PM +0530, Jassi Brar wrote: > On Fri, Sep 2, 2011 at 6:14 AM, Boojin Kim <boojin.kim@samsung.com> wrote: > > Origianl code carries out the start operation after flush operation. > > But start operation is not required for DMA_TERMINATE_ALL command. > > So, This patch removes the unnecessary start operation and only carries out > > the flush oeration for handling DMA_TERMINATE_ALL command. > > Not exactly. The 'start' is impotent when called from this path because there > is nothing left queued after the call to 'flush'. > pl330_tasklet() is called because that is a common function that does the > house-keeping acc to what's presently in 'work' and 'done' lists. > > Though as a side-effect, your patch does avoid doing callbacks on submitted > xfers during DMA_TERMINATE_ALL - which may or may not be the right thing > to do. It is defined that DMA_TERMINATE_ALL does not call the callbacks: 1. int dmaengine_terminate_all(struct dma_chan *chan) This causes all activity for the DMA channel to be stopped, and may discard data in the DMA FIFO which hasn't been fully transferred. No callback functions will be called for any incomplete transfers.
On 6 September 2011 17:57, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > On Tue, Sep 06, 2011 at 05:52:19PM +0530, Jassi Brar wrote: >> On Fri, Sep 2, 2011 at 6:14 AM, Boojin Kim <boojin.kim@samsung.com> wrote: >> > Origianl code carries out the start operation after flush operation. >> > But start operation is not required for DMA_TERMINATE_ALL command. >> > So, This patch removes the unnecessary start operation and only carries out >> > the flush oeration for handling DMA_TERMINATE_ALL command. >> >> Not exactly. The 'start' is impotent when called from this path because there >> is nothing left queued after the call to 'flush'. >> pl330_tasklet() is called because that is a common function that does the >> house-keeping acc to what's presently in 'work' and 'done' lists. >> >> Though as a side-effect, your patch does avoid doing callbacks on submitted >> xfers during DMA_TERMINATE_ALL - which may or may not be the right thing >> to do. > > It is defined that DMA_TERMINATE_ALL does not call the callbacks: > > 1. int dmaengine_terminate_all(struct dma_chan *chan) > > This causes all activity for the DMA channel to be stopped, and may > discard data in the DMA FIFO which hasn't been fully transferred. > No callback functions will be called for any incomplete transfers. > Ok, thanks for the info. Boojin Kim, please re-write the changelog to state only preventing callbacks during DMA_TERMINATE_ALL as the reason.
Jassi Brar wrote: > > On 6 September 2011 17:57, Russell King - ARM Linux > <linux@arm.linux.org.uk> wrote: > > On Tue, Sep 06, 2011 at 05:52:19PM +0530, Jassi Brar wrote: > >> On Fri, Sep 2, 2011 at 6:14 AM, Boojin Kim <boojin.kim@samsung.com> wrote: > >> > Origianl code carries out the start operation after flush operation. > >> > But start operation is not required for DMA_TERMINATE_ALL command. > >> > So, This patch removes the unnecessary start operation and only carries out > >> > the flush oeration for handling DMA_TERMINATE_ALL command. > >> > >> Not exactly. The 'start' is impotent when called from this path because there > >> is nothing left queued after the call to 'flush'. > >> pl330_tasklet() is called because that is a common function that does the > >> house-keeping acc to what's presently in 'work' and 'done' lists. > >> > >> Though as a side-effect, your patch does avoid doing callbacks on submitted > >> xfers during DMA_TERMINATE_ALL - which may or may not be the right thing > >> to do. > > > > It is defined that DMA_TERMINATE_ALL does not call the callbacks: > > > > 1. int dmaengine_terminate_all(struct dma_chan *chan) > > > > This causes all activity for the DMA channel to be stopped, and may > > discard data in the DMA FIFO which hasn't been fully transferred. > > No callback functions will be called for any incomplete transfers. > > > Ok, thanks for the info. > > Boojin Kim, please re-write the changelog to state only preventing > callbacks during DMA_TERMINATE_ALL as the reason. As above, we don't need callback as well start operation in DMA_TERMIANTE_ALL command and her patch just removed them for DMA_TERMINATE_ALL here. So current git message seems to have proper behavior of patch. Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
On 7 September 2011 12:53, Kukjin Kim <kgene.kim@samsung.com> wrote: > Jassi Brar wrote: >> >> On 6 September 2011 17:57, Russell King - ARM Linux >> <linux@arm.linux.org.uk> wrote: >> > On Tue, Sep 06, 2011 at 05:52:19PM +0530, Jassi Brar wrote: >> >> On Fri, Sep 2, 2011 at 6:14 AM, Boojin Kim <boojin.kim@samsung.com> wrote: >> >> > Origianl code carries out the start operation after flush operation. >> >> > But start operation is not required for DMA_TERMINATE_ALL command. >> >> > So, This patch removes the unnecessary start operation and only carries out >> >> > the flush oeration for handling DMA_TERMINATE_ALL command. >> >> >> >> Not exactly. The 'start' is impotent when called from this path because there >> >> is nothing left queued after the call to 'flush'. >> >> pl330_tasklet() is called because that is a common function that does the >> >> house-keeping acc to what's presently in 'work' and 'done' lists. >> >> >> >> Though as a side-effect, your patch does avoid doing callbacks on submitted >> >> xfers during DMA_TERMINATE_ALL - which may or may not be the right thing >> >> to do. >> > >> > It is defined that DMA_TERMINATE_ALL does not call the callbacks: >> > >> > 1. int dmaengine_terminate_all(struct dma_chan *chan) >> > >> > This causes all activity for the DMA channel to be stopped, and may >> > discard data in the DMA FIFO which hasn't been fully transferred. >> > No callback functions will be called for any incomplete transfers. >> > >> Ok, thanks for the info. >> >> Boojin Kim, please re-write the changelog to state only preventing >> callbacks during DMA_TERMINATE_ALL as the reason. > > As above, we don't need callback as well start operation in DMA_TERMIANTE_ALL command and her patch just removed them for DMA_TERMINATE_ALL here. So current git message seems to have proper behavior of patch. > Nopes. No modification to current code is needed if only the following is to be the reason. { Origianl code carries out the start operation after flush operation. But start operation is not required for DMA_TERMINATE_ALL command. So, This patch removes the unnecessary start operation and only carries out the flush oeration for handling DMA_TERMINATE_ALL command. } The patch, however, has unintended good effect of "preventing callbacks from DMA_TERMINATE_ALL". The only valid reason, and which is no way implied by the changelog.
diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c index e7f9d1d..59943ec 100644 --- a/drivers/dma/pl330.c +++ b/drivers/dma/pl330.c @@ -262,10 +262,11 @@ static int pl330_alloc_chan_resources(struct dma_chan *chan) static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned long arg) { struct dma_pl330_chan *pch = to_pchan(chan); - struct dma_pl330_desc *desc; + struct dma_pl330_desc *desc, *_dt; unsigned long flags; struct dma_pl330_dmac *pdmac = pch->dmac; struct dma_slave_config *slave_config; + LIST_HEAD(list); switch (cmd) { case DMA_TERMINATE_ALL: @@ -275,12 +276,14 @@ static int pl330_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, unsigned pl330_chan_ctrl(pch->pl330_chid, PL330_OP_FLUSH); /* Mark all desc done */ - list_for_each_entry(desc, &pch->work_list, node) + list_for_each_entry_safe(desc, _dt, &pch->work_list , node) { desc->status = DONE; + pch->completed = desc->txd.cookie; + list_move_tail(&desc->node, &list); + } + list_splice_tail_init(&list, &pdmac->desc_pool); spin_unlock_irqrestore(&pch->lock, flags); - - pl330_tasklet((unsigned long) pch); break; case DMA_SLAVE_CONFIG: slave_config = (struct dma_slave_config *)arg;