From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934045AbbJII70 (ORCPT ); Fri, 9 Oct 2015 04:59:26 -0400 Received: from aserp1040.oracle.com ([141.146.126.69]:38285 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933229AbbJII7Y (ORCPT ); Fri, 9 Oct 2015 04:59:24 -0400 Date: Fri, 9 Oct 2015 11:58:48 +0300 From: Dan Carpenter To: Chen Feng Cc: dan.zhao@hisilicon.com, w.f@huawei.com, linuxarm@huawei.com, paul.gortmaker@windriver.com, sumit.semwal@linaro.org, devel@driverdev.osuosl.org, xuyiping@hisilicon.com, tapaswenipathak@gmail.com, tranmanphong@gmail.com, z.liuxinliang@hisilicon.com, kong.kongxinwei@hisilicon.com, qijiwen@hisilicon.com, weidong2@hisilicon.com, suzhuangluan@hisilicon.com, riandrews@android.com, gioh.kim@lge.com, gregkh@linuxfoundation.org, peter.panshilin@hisilicon.com, linux-kernel@vger.kernel.org, arve@android.com, saberlily.xia@hisilicon.com Subject: Re: [PATCH 2/3] staging: android: ion: Add ion driver for Hi6220 SoC platform Message-ID: <20151009085848.GT7340@mwanda> References: <1444290913-76936-1-git-send-email-puck.chen@hisilicon.com> <1444290913-76936-2-git-send-email-puck.chen@hisilicon.com> <20151009085332.GS7340@mwanda> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151009085332.GS7340@mwanda> User-Agent: Mutt/1.5.21 (2010-09-15) X-Source-IP: userv0022.oracle.com [156.151.31.74] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Oct 09, 2015 at 11:53:32AM +0300, Dan Carpenter wrote: > > +out: > > Labels named "out" are bug prone because handling everything is harder > than using named labels and unwinding one step at a time. The bug here > is that we don't call ion_device_destroy(). > > > + for (i = 0; i < num_heaps; ++i) > > + ion_heap_destroy(heaps[i]); > > + return err; > > Write it like this: > > err_free_heaps: > for (i = 0; i < num_heaps; ++i) > ion_heap_destroy(heaps[i]); > err_free_idev: > ion_device_destroy(idev); > > return err; > > > +} > > + > > +static int hi6220_ion_remove(struct platform_device *pdev) > > +{ > > + int i; > > + > > + ion_device_destroy(idev); > > + for (i = 0; i < num_heaps; i++) { > > + if (!heaps[i]) > > + continue; > > We don't really need this NULL check and it isn't there in the > hi6220_ion_probe() unwind code. > > > + ion_heap_destroy(heaps[i]); > > + heaps[i] = NULL; > > + } > > + Really the unwind from probe() and the remove() function should have similar code. For example, is it important to set heaps[i] to NULL? If so then we should do it in the probe function as well. If not then we could leave it out of the remove function. Also the ion_device_destroy(idev) should be after freeing heaps in the remove function. regards, dan carpenter