From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756243AbbCCLVf (ORCPT ); Tue, 3 Mar 2015 06:21:35 -0500 Received: from smtp-out-246.synserver.de ([212.40.185.246]:1071 "EHLO smtp-out-238.synserver.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752204AbbCCLVd (ORCPT ); Tue, 3 Mar 2015 06:21:33 -0500 X-SynServer-TrustedSrc: 1 X-SynServer-AuthUser: lars@metafoo.de X-SynServer-PPID: 765 Message-ID: <54F5992E.3060808@metafoo.de> Date: Tue, 03 Mar 2015 12:21:18 +0100 From: Lars-Peter Clausen User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.4.0 MIME-Version: 1.0 To: Robert Jarzmik CC: Vinod Koul , dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, Russell King Subject: Re: [PATCH] dmaengine: virt-dma: fix completion list manipulation References: <1425331196-7895-1-git-send-email-robert.jarzmik@free.fr> <54F4D631.6020605@metafoo.de> <87fv9n12h6.fsf@free.fr> <54F5615E.3070106@metafoo.de> <877fuy1img.fsf@free.fr> In-Reply-To: <877fuy1img.fsf@free.fr> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/03/2015 11:27 AM, Robert Jarzmik wrote: > Lars-Peter Clausen writes: > >>>> That will break all drivers which handle this currently correctly and remove the >>>> descriptor from any list before calling vchan_cookie_complete. >>> Ah, well well I don't agree. >>> >>> First, let's split the drivers which remove the descriptors and these which >>> don't : >>> >>> These which remove the descriptor: >>> dma-jz4740.c >>> fsl-edma.c >>> >>> These which don't remove the descriptor: >>> amba-pl08x.c >>> edma.c >>> img-mdc-dma.c >>> k3dma.c >>> moxart-dma.c >>> omap-dma.c >>> qcom_bam_dma.c >>> s3c24xx-dma.c >>> sa11x0-dma.c >>> sun6i-dma.c >> >> All of those remove the descriptor from the list when they start the transfer. > Ah, I didn't see that. > Isn't the descriptor lost if a terminate_all(), relying on > vchan_get_all_descriptors() and vchan_dma_desc_free_list() is used ? If the driver doesn't manually add it to the list of to be freed descriptors yes. > >>> >>> That settles the correctness I think, the correct behavior is to not remove the >>> descriptor and let it be done by vchan_cookie_complete(). >>> >>> Now for the remaining 2 drivers, we'll have : >>> - list_del(&vd->node) => vd becomes a singleton >>> - list_move_tail(&vd->node, &...desc_completed) >>> => list_del(&vd->node) : nothing changes, it's a nop >>> => list_add_tail(&vd->node, &...desc_completed) >>> >>> And the behavior remains correct after the patch, only one "list_del()" is done >>> twice for nothing. Where do you see any breakage ? >> >> Calling list_del() on a list item that is not on a list causes undefined >> behavior, which can result in memory corruption, segfaults, etc... > Ah yes, you must be thinking about the "poisoning" after the __list_del() call, > I forgot about that. > > Do you think amending all these drivers and changing their list_del() into > list_del_init() would at least prevent the "undefined behavior" ? Yes. > > I still think that their use of virt-dma is incorrect, ie. that at one point in > time a virtual descriptor has to be on exactly one list of virt-dma (excepting > transient critical sections). Well the drivers conform to the current expected behavior. It might be worth changing that, but you need to modify all the driver to conform to the new semantics, rather than just changing the API. Requiring that the descriptor is always on one of the virt-dma list is to restrictive. Some DMA controllers are able to submit multiple descriptors at the same time, these typically have a separate list to manage to active descriptors. - Lars