Re: [PATCH v1 3/7] dmaengine: tegra-apb: Prevent race conditions on channel's freeing

[Dmitry Osipenko <digetx@gmail.com>]

30.12.2019 23:45, Michał Mirosław пишет:
> On Sat, Dec 28, 2019 at 11:46:36PM +0300, Dmitry Osipenko wrote:
>> It's unsafe to check the channel's "busy" state without taking a lock,
>> it is also unsafe to assume that tasklet isn't in-fly.
> 
> 'in-flight'. Also, the patch seems to have two independent bug-fixes
> in it. Second one doesn't look right, at least not without an explanation.

Technically, this all shouldn't be needed at all since it should be a
responsibility of the DMA client drivers to make sure that channel is
idling before releasing it. But, AFAIK, the behavior of channel's
releasing isn't strictly defined by the DMA API, so it should be better
to keep the original behavior in place.

> First:
> 
>> -	if (tdc->busy)
>> -		tegra_dma_terminate_all(dc);
>> +	tegra_dma_terminate_all(dc);
> 
> Second:
> 
>> +	tasklet_kill(&tdc->tasklet);

Yes, it could be a separate change. Actually, this is not a fix, but a
clean-up change that simply stops tasklet instead of trying to work
around the fact that tasklet could be scheduled at the time channel's
freeing.

>>  	spin_lock_irqsave(&tdc->lock, flags);

I now see that missed to remove this locking since it's not needed now,
given that tasklet is already stopped after killing it by the above change.

I'll update and split this patch into two in v3.

>>  	list_splice_init(&tdc->pending_sg_req, &sg_req_list);
>> @@ -1543,7 +1543,6 @@ static int tegra_dma_probe(struct platform_device *pdev)
>>  		struct tegra_dma_channel *tdc = &tdma->channels[i];
>>  
>>  		free_irq(tdc->irq, tdc);
>> -		tasklet_kill(&tdc->tasklet);
>>  	}
>>  
>>  	pm_runtime_disable(&pdev->dev);
>> @@ -1563,7 +1562,6 @@ static int tegra_dma_remove(struct platform_device *pdev)
>>  	for (i = 0; i < tdma->chip_data->nr_channels; ++i) {
>>  		tdc = &tdma->channels[i];
>>  		free_irq(tdc->irq, tdc);
>> -		tasklet_kill(&tdc->tasklet);
>>  	}
>>  
>>  	pm_runtime_disable(&pdev->dev);

Thank you very much for taking a look at it!