From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753631AbdGUHzK (ORCPT ); Fri, 21 Jul 2017 03:55:10 -0400 Received: from mga07.intel.com ([134.134.136.100]:55201 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753343AbdGUHzI (ORCPT ); Fri, 21 Jul 2017 03:55:08 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.40,388,1496127600"; d="scan'208";a="995489727" Date: Fri, 21 Jul 2017 13:27:54 +0530 From: Vinod Koul To: Dave Jiang Cc: "wharms@bfs.de" , Christophe JAILLET , "Williams, Dan J" , "dmaengine@vger.kernel.org" , "linux-kernel@vger.kernel.org" , "kernel-janitors@vger.kernel.org" Subject: Re: [PATCH] dmaengine: ioatdma: Fix error handling path in 'ioat_dma_self_test()' Message-ID: <20170721075754.GP3053@localhost> References: <20170719221648.30253-1-christophe.jaillet@wanadoo.fr> <59705A93.2090305@bfs.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 20, 2017 at 09:56:45AM -0700, Dave Jiang wrote: > > > On 07/20/2017 12:24 AM, walter harms wrote: > > > > > > Am 20.07.2017 00:16, schrieb Christophe JAILLET: > >> If the 'memcmp' fails, free allocated resources as done in all other > >> error handling paths. > >> > >> Signed-off-by: Christophe JAILLET > >> --- > >> Please review carefully, this patch looks "too obvious" to me! > >> --- > >> drivers/dma/ioat/init.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/dma/ioat/init.c b/drivers/dma/ioat/init.c > >> index ed8ed1192775..948fc1f8fb5c 100644 > >> --- a/drivers/dma/ioat/init.c > >> +++ b/drivers/dma/ioat/init.c > >> @@ -390,7 +390,7 @@ static int ioat_dma_self_test(struct ioatdma_device *ioat_dma) > >> if (memcmp(src, dest, IOAT_TEST_SIZE)) { > >> dev_err(dev, "Self-test copy failed compare, disabling\n"); > >> err = -ENODEV; > >> - goto free_resources; > >> + goto unmap_dma; > >> } > >> > >> unmap_dma: > > > > ^^^^^^^^^^ > > > > > > is the goto needed at all ? > > It's not. However, it may be better to stay there if we happen to add > additional code after the if block later on and guard against mistakes. > At least IMO. Then lets remove it please, there is no place for dead code, if we need it people can add it as part of the changes they introduce.. -- ~Vinod