From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S965992AbdCXOtD (ORCPT ); Fri, 24 Mar 2017 10:49:03 -0400 Received: from pandora.armlinux.org.uk ([78.32.30.218]:48590 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S965973AbdCXOs6 (ORCPT ); Fri, 24 Mar 2017 10:48:58 -0400 Date: Fri, 24 Mar 2017 14:48:39 +0000 From: Russell King - ARM Linux To: Geliang Tang Cc: Joerg Roedel , Robin Murphy , Will Deacon , Andrew Morton , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH] arm/dma-mapping: use for_each_sg Message-ID: <20170324144838.GH7909@n2100.armlinux.org.uk> References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Mar 24, 2017 at 10:12:23PM +0800, Geliang Tang wrote: > Use for_each_sg() instead of open-coding it. > > Signed-off-by: Geliang Tang No. > diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c > index 63eabb0..e551351 100644 > --- a/arch/arm/mm/dma-mapping.c > +++ b/arch/arm/mm/dma-mapping.c > @@ -1720,7 +1720,7 @@ static int __map_sg_chunk(struct device *dev, struct scatterlist *sg, > if (iova == DMA_ERROR_CODE) > return -ENOMEM; > > - for (count = 0, s = sg; count < (size >> PAGE_SHIFT); s = sg_next(s)) { > + for_each_sg(sg, s, size >> PAGE_SHIFT, count) { This is _not_ equivalent. #define for_each_sg(sglist, sg, nr, __i) \ for (__i = 0, sg = (sglist); __i < (nr); __i++, sg = sg_next(sg)) Putting the arguments into that gives: for (count = 0, s = sg; count < (size >> PAGE_SHIFT); count++, s = sg_next(s)) whereas the old code is: for (count = 0, s = sg; count < (size >> PAGE_SHIFT); s = sg_next(s)) Spot the difference? "count++". This is because "size >> PAGE_SHIFT" is not the number of scatterlist entries, it's the number of pages to map. Please be more careful in the future to ensure that, when cleaning up code, that the code is indeed equivalent. Subtle errors like this are sometimes incredibly difficult to spot. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.