From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
	id S1751481AbbJJGLx (ORCPT <rfc822;w@1wt.eu>);
	Sat, 10 Oct 2015 02:11:53 -0400
Received: from szxga03-in.huawei.com ([119.145.14.66]:27205 "EHLO
	szxga03-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1750775AbbJJGLv (ORCPT
	<rfc822;linux-kernel@vger.kernel.org>);
	Sat, 10 Oct 2015 02:11:51 -0400
Subject: Re: [PATCH 2/3] staging: android: ion: Add ion driver for Hi6220 SoC
 platform
To: Dan Carpenter <dan.carpenter@oracle.com>
References: <1444290913-76936-1-git-send-email-puck.chen@hisilicon.com>
 <1444290913-76936-2-git-send-email-puck.chen@hisilicon.com>
 <20151009085332.GS7340@mwanda> <20151009085848.GT7340@mwanda>
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>
From: chenfeng <puck.chen@hisilicon.com>
Message-ID: <5618AB25.908@hisilicon.com>
Date: Sat, 10 Oct 2015 14:07:33 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101
 Thunderbird/38.2.0
MIME-Version: 1.0
In-Reply-To: <20151009085848.GT7340@mwanda>
Content-Type: text/plain; charset="windows-1252"
Content-Transfer-Encoding: 7bit
X-Originating-IP: [10.142.192.172]
X-CFilter-Loop: Reflected
X-Mirapoint-Virus-RAPID-Raw: score=unknown(0),
	refid=str=0001.0A090201.5618AB3A.009F,ss=1,re=0.000,recu=0.000,reip=0.000,cl=1,cld=1,fgs=0,
	ip=0.0.0.0,
	so=2013-05-26 15:14:31,
	dmn=2013-03-21 17:37:32
X-Mirapoint-Loop-Id: 4c5896509abf17de31d2813d6363f3d4
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org



On 2015/10/9 16:58, Dan Carpenter wrote:
> 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.
> 
Thanks.
I will modify this next version.

> regards,
> dan carpenter
> 
> 
> .
>