On Thu, Jan 17, 2019 at 05:10:38PM +0100, Codrin Ciubotariu - M19940 wrote:
> From: Codrin Ciubotariu <codrin.ciubotariu@xxxxxxxxxxxxx>
>
> atchan->status is used for two things:
> - pass channel interrupts status from interrupt handler to tasklet;
> - channel information like whether it is cyclic or paused;
>
> Since these operations have nothing in common, this patch adds a
> different struct member to keep the interrupts status.
>
> Fixes a bug in which a channel is wrongfully reported as in use when
> trying to obtain a new descriptior for a previously used cyclic channel.
>
> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@xxxxxxxxxxxxx>
Acked-by: Ludovic Desroches <ludovic.desroches@xxxxxxxxxxxxx>
As Vinod suggested, you may change the commit message to emphasize
that the bug comes from the fact that a single variable is used to store
different information.
The Fixes: tag should be added too.
Regards
Ludovic
> ---
> drivers/dma/at_xdmac.c | 19 ++++++++++---------
> 1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/dma/at_xdmac.c b/drivers/dma/at_xdmac.c
> index 4e557684f792..fe69dccfa0c0 100644
> --- a/drivers/dma/at_xdmac.c
> +++ b/drivers/dma/at_xdmac.c
> @@ -203,6 +203,7 @@ struct at_xdmac_chan {
> u32 save_cim;
> u32 save_cnda;
> u32 save_cndc;
> + u32 irq_status;
> unsigned long status;
> struct tasklet_struct tasklet;
> struct dma_slave_config sconfig;
> @@ -1580,8 +1581,8 @@ static void at_xdmac_tasklet(unsigned long data)
> struct at_xdmac_desc *desc;
> u32 error_mask;
>
> - dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08lx\n",
> - __func__, atchan->status);
> + dev_dbg(chan2dev(&atchan->chan), "%s: status=0x%08x\n",
> + __func__, atchan->irq_status);
>
> error_mask = AT_XDMAC_CIS_RBEIS
> | AT_XDMAC_CIS_WBEIS
> @@ -1589,15 +1590,15 @@ static void at_xdmac_tasklet(unsigned long data)
>
> if (at_xdmac_chan_is_cyclic(atchan)) {
> at_xdmac_handle_cyclic(atchan);
> - } else if ((atchan->status & AT_XDMAC_CIS_LIS)
> - || (atchan->status & error_mask)) {
> + } else if ((atchan->irq_status & AT_XDMAC_CIS_LIS)
> + || (atchan->irq_status & error_mask)) {
> struct dma_async_tx_descriptor *txd;
>
> - if (atchan->status & AT_XDMAC_CIS_RBEIS)
> + if (atchan->irq_status & AT_XDMAC_CIS_RBEIS)
> dev_err(chan2dev(&atchan->chan), "read bus error!!!");
> - if (atchan->status & AT_XDMAC_CIS_WBEIS)
> + if (atchan->irq_status & AT_XDMAC_CIS_WBEIS)
> dev_err(chan2dev(&atchan->chan), "write bus error!!!");
> - if (atchan->status & AT_XDMAC_CIS_ROIS)
> + if (atchan->irq_status & AT_XDMAC_CIS_ROIS)
> dev_err(chan2dev(&atchan->chan), "request overflow error!!!");
>
> spin_lock(&atchan->lock);
> @@ -1652,7 +1653,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
> atchan = &atxdmac->chan[i];
> chan_imr = at_xdmac_chan_read(atchan, AT_XDMAC_CIM);
> chan_status = at_xdmac_chan_read(atchan, AT_XDMAC_CIS);
> - atchan->status = chan_status & chan_imr;
> + atchan->irq_status = chan_status & chan_imr;
> dev_vdbg(atxdmac->dma.dev,
> "%s: chan%d: imr=0x%x, status=0x%x\n",
> __func__, i, chan_imr, chan_status);
> @@ -1666,7 +1667,7 @@ static irqreturn_t at_xdmac_interrupt(int irq, void *dev_id)
> at_xdmac_chan_read(atchan, AT_XDMAC_CDA),
> at_xdmac_chan_read(atchan, AT_XDMAC_CUBC));
>
> - if (atchan->status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
> + if (atchan->irq_status & (AT_XDMAC_CIS_RBEIS | AT_XDMAC_CIS_WBEIS))
> at_xdmac_write(atxdmac, AT_XDMAC_GD, atchan->mask);
>
> tasklet_schedule(&atchan->tasklet);
> --
> 2.17.1
>
