From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756107AbbCCK1K (ORCPT ); Tue, 3 Mar 2015 05:27:10 -0500 Received: from smtp08.smtpout.orange.fr ([80.12.242.130]:37182 "EHLO smtp.smtpout.orange.fr" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752328AbbCCK1I (ORCPT ); Tue, 3 Mar 2015 05:27:08 -0500 X-ME-Helo: beldin X-ME-Date: Tue, 03 Mar 2015 11:27:05 +0100 X-ME-IP: 109.214.21.72 From: Robert Jarzmik To: Lars-Peter Clausen Cc: Vinod Koul , dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org 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> X-URL: http://belgarath.falguerolles.org/ Date: Tue, 03 Mar 2015 11:27:03 +0100 In-Reply-To: <54F5615E.3070106@metafoo.de> (Lars-Peter Clausen's message of "Tue, 03 Mar 2015 08:23:10 +0100") Message-ID: <877fuy1img.fsf@free.fr> User-Agent: Gnus/5.130008 (Ma Gnus v0.8) Emacs/24.3.92 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 ? >> >> 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" ? 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). Cheers. -- Robert