Re: [PATCH] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use

From: <Codrin.Ciubotariu@microchip.com>
To: <vkoul@kernel.org>
Cc: <Ludovic.Desroches@microchip.com>, <dmaengine@vger.kernel.org>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] dmaengine: at_xdmac: Fix wrongfull report of a channel as in use
Date: Mon, 21 Jan 2019 14:38:47 +0000	[thread overview]
Message-ID: <450ee975-a603-cf89-0000-13156c1cb5e8@microchip.com> (raw)
In-Reply-To: <20190120110417.GS4635@vkoul-mobl>

On 20.01.2019 13:04, Vinod Koul wrote:
> Hi Codrin,
> 
> On 17-01-19, 16:10, Codrin.Ciubotariu@microchip.com wrote:
>> From: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>>
>> 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.
> 
> This looks reasonable but am unable to see how the bug is fixed. Perhaps
> would be great to split the bug part (which can go to fixes) and remove
> the reuse of variable as a subsequent patch..

Hi Vinod,

This patch is the fix, since it moves the operations on atchan->status, 
in which the interrupt status register is saved, to a different member 
(irq_status). The AT_XDMAC_CHAN_IS_CYCLIC and AT_XDMAC_CHAN_IS_PAUSED 
bits have nothing in common with the interrupt status register.

The bug reproduces when a device_terminate_all() is called, 
(AT_XDMAC_CHAN_IS_CYCLIC cleared on atchan->status) and then a late End 
of Block interrupt arrives (AT_XDMAC_CIS_BIS), which sets bit 0 of 
atchan->status. Bit 0 is also used for AT_XDMAC_CHAN_IS_CYCLIC, so when 
a new descriptor for a cyclic transfer is created, the driver reports 
the channel as in use:

if (test_and_set_bit(AT_XDMAC_CHAN_IS_CYCLIC, &atchan->status)) {
	dev_err(chan2dev(chan), "channel currently used\n");
	return NULL;
}

I can send v2 if you consider the commit message misleading.

Best regards,
Codrin

> 
>>
>> Signed-off-by: Codrin Ciubotariu <codrin.ciubotariu@microchip.com>
>> ---
>>   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
> 