[prev in list] [next in list] [prev in thread] [next in thread] 

List:       dmaengine
Subject:    Re: [PATCH] imx-sdma: fix pagefault when channel is disabled during interrupt
From:       thierry bultel <tbultel () pixelsurmer ! com>
Date:       2018-02-27 15:17:18
Message-ID: 63e43990-d549-53cb-ae4c-813bf3cd8085 () pixelsurmer ! com
[Download RAW message or body]

Thanks Vinod,

I had posted a V2 meanwhile.
I just posted a V3 that takes your comments into account

Best Regards,
Thierry

Le 27/02/2018 à 16:01, Vinod Koul a écrit  :
> On Tue, Feb 13, 2018 at 09:47:23AM +0100, Thierry Bultel wrote:
>> Add a spinlock and a 'enabled' boolean on channel descriptor, to avoid
>> using buffer descriptors in the interrupt context,
>> sdma_disable_channel is called meanwhile.
>> ---
>>   drivers/dma/imx-sdma.c | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c
>> index d1651a5..7eab42b 100644
>> --- a/drivers/dma/imx-sdma.c
>> +++ b/drivers/dma/imx-sdma.c
>> @@ -330,6 +330,7 @@ struct sdma_channel {
>>   	unsigned int			chn_real_count;
>>   	struct tasklet_struct		tasklet;
>>   	struct imx_dma_data		data;
>> +	unsigned int 			enabled;
> 
> description says bool but this is int?
> 
>>   };
>>   
>>   #define IMX_DMA_SG_LOOP		BIT(0)
>> @@ -588,7 +589,13 @@ static int sdma_config_ownership(struct sdma_channel *sdmac,
>>   
>>   static void sdma_enable_channel(struct sdma_engine *sdma, int channel)
>>   {
>> +	unsigned long flags;
>> +	struct sdma_channel *sdmac = &sdma->channel[channel];
> 
> Empty line here please
> 
>>   	writel(BIT(channel), sdma->regs + SDMA_H_START);
>> +
>> +	spin_lock_irqsave(&sdmac->lock, flags);
>> +	sdmac->enabled = 1;
>> +	spin_unlock_irqrestore(&sdmac->lock, flags);
>>   }
>>   
>>   /*
>> @@ -677,6 +684,15 @@ static void sdma_update_channel_loop(struct sdma_channel *sdmac)
>>   	struct sdma_buffer_descriptor *bd;
>>   	int error = 0;
>>   	enum dma_status	old_status = sdmac->status;
>> +	unsigned long flags;
>> +
> 
> this empty line is bogus
> 
>> +	int enabled;
> 
> and is required here. Empty line needs to be added to a fn after defining
> variables and before code statements
> 
>> +	spin_lock_irqsave(&sdmac->lock, flags);
>> +	enabled = sdmac->enabled;
>> +	spin_unlock_irqrestore(&sdmac->lock, flags);
>> +
>> +	if (!enabled)
>> +		return;
>>   
>>   	/*
>>   	 * loop mode. Iterate over descriptors, re-setup them and
>> @@ -930,10 +946,15 @@ static int sdma_disable_channel(struct dma_chan *chan)
>>   	struct sdma_channel *sdmac = to_sdma_chan(chan);
>>   	struct sdma_engine *sdma = sdmac->sdma;
>>   	int channel = sdmac->channel;
>> +	unsigned long flags;
>>   
>>   	writel_relaxed(BIT(channel), sdma->regs + SDMA_H_STATSTOP);
>>   	sdmac->status = DMA_ERROR;
>>   
>> +	spin_lock_irqsave(&sdmac->lock, flags);
>> +	sdmac->enabled = 0;
>> +	spin_unlock_irqrestore(&sdmac->lock, flags);
>> +
>>   	return 0;
>>   }
>>   
>> -- 
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe dmaengine" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]