linux-kernel - Re: [PATCH 1/3] dmaengine: virt-dma: don't always free descriptor upon completion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <87io6zppj8.fsf@belgarion.home>
Date:	Thu, 24 Sep 2015 21:48:11 +0200
From:	Robert Jarzmik <robert.jarzmik@...e.fr>
To:	Vinod Koul <vinod.koul@...el.com>
Cc:	Daniel Mack <daniel@...que.org>,
	Haojian Zhuang <haojian.zhuang@...il.com>,
	Jun Nie <jun.nie@...aro.org>,
	Lars-Peter Clausen <lars@...afoo.de>,
	dmaengine@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/3] dmaengine: virt-dma: don't always free descriptor upon completion

Vinod Koul <vinod.koul@...el.com> writes:

> On Sun, Sep 06, 2015 at 01:40:52PM +0200, Robert Jarzmik wrote:
>> @@ -29,7 +29,7 @@ dma_cookie_t vchan_tx_submit(struct dma_async_tx_descriptor *tx)
>>  	spin_lock_irqsave(&vc->lock, flags);
>>  	cookie = dma_cookie_assign(tx);
>>  
>> -	list_add_tail(&vd->node, &vc->desc_submitted);
>> +	list_move_tail(&vd->node, &vc->desc_submitted);
>
> I am not sure I follow this, why move ?
Because it is already on the allocated list (see vchan_tx_prep()).

>> +int vchan_tx_desc_free(struct dma_async_tx_descriptor *tx)
>> +{
>> +	struct virt_dma_chan *vc = to_virt_chan(tx->chan);
>> +	struct virt_dma_desc *vd = to_virt_desc(tx);
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&vc->lock, flags);
>> +	list_del(&vd->node);
>> +	spin_unlock_irqrestore(&vc->lock, flags);
>> +
>> +	dev_dbg(vc->chan.device->dev, "vchan %p: txd %p[%x]: freeing\n",
>> +		vc, vd, vd->tx.cookie);
>> +	vc->desc_free(vd);
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(vchan_tx_desc_free);
> this one seems okay, can you add comments here and also update documentation
> for this
Ah yes, very good point, doxygen like documentation, got it.

>> @@ -96,9 +115,13 @@ void vchan_dma_desc_free_list(struct virt_dma_chan *vc, struct list_head *head)
>>  	while (!list_empty(head)) {
>>  		struct virt_dma_desc *vd = list_first_entry(head,
>>  			struct virt_dma_desc, node);
>> -		list_del(&vd->node);
>> -		dev_dbg(vc->chan.device->dev, "txd %p: freeing\n", vd);
>> -		vc->desc_free(vd);
>> +		if (dmaengine_desc_test_reuse(&vd->tx)) {
>> +			list_move_tail(&vd->node, &vc->desc_allocated);
>
> should we check this if the list is to be freed? Why would anyone call free
> except when cleaning up ?
Yes, there is a corner case : a driver does a dmaengine_terminate_all(), to stop
the dma transfers (a missed video frame for example). Then upon the sync signal,
it resubmits the reusable transfers it had.

Or said differently, reusable transfers should continue to be reusable through a
dmaengine_terminate_all(), hence this test. And there is a link with your
comment below.

>>  static inline void vchan_free_chan_resources(struct virt_dma_chan *vc)
>>  {
>> +	struct virt_dma_desc *vd;
>>  	unsigned long flags;
>>  	LIST_HEAD(head);
>>  
>>  	spin_lock_irqsave(&vc->lock, flags);
>>  	vchan_get_all_descriptors(vc, &head);
>> +	list_for_each_entry(vd, &head, node)
>> +		dmaengine_desc_clear_reuse(&vd->tx);
>
> why do we want to do this, if we are freeing them
Because if we don't clear reuse flag, vchan_dma_desc_free_list() won't really
free the descriptor, but move it to allocated list.

Cheers.

-- 
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/