From:   "Bumwoo Lee" <bw365.lee@samsung.com>
To:     <myungjoo.ham@samsung.com>,
        "'Chanwoo Choi'" <cw00.choi@samsung.com>,
        <linux-kernel@vger.kernel.org>
In-Reply-To: <20230224100325epcms1p3e8886e278e23f610c8490cb69f1d452d@epcms1p3>
Subject: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify
 extcon register function
Date:   Thu, 2 Mar 2023 10:38:08 +0900
Message-ID: <091101d94ca7$a4ad23c0$ee076b40$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQEndo8vt+Nxhu23TcqVP1cSXfBwDwF+DXQiAV3ksbMBRRvN4rApV6YQ
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20230220054513.27385-3-bw365.lee@samsung.com>
        <20230220054513.27385-1-bw365.lee@samsung.com>
        <CGME20230220054545epcas1p34e5575ffdfd1dcef55bdee3c3563c925@epcms1p3>
        <20230224100325epcms1p3e8886e278e23f610c8490cb69f1d452d@epcms1p3>
Precedence: bulk

Hello.

As you can see, edev->cables are freed if extcon_alloc_cables() function re=
turn error handling in extcon_dev_register()
Other added functions are also same.

Because it's functionalized, apart from this, do you want to mention that i=
t should be freed within the function?=20
Please let me know your opinion.

extcon_dev_register(struct extcon_dev *edev)=7B
...

	ret =3D extcon_alloc_cables(edev);
	if (ret)
		goto err_alloc_cables;

...

err_alloc_cables:
 	if (edev->max_supported)
 		kfree(edev->cables);


Regards,
Bumwoo

-----Original Message-----
From: MyungJoo Ham <myungjoo.ham=40samsung.com>=20
Sent: Friday, February 24, 2023 7:03 PM
To: Bumwoo Lee <bw365.lee=40samsung.com>; Chanwoo Choi <cw00.choi=40samsung=
.com>; linux-kernel=40vger.kernel.org
Subject: RE: =5BPATCH v2 2/4=5D extcon: Added extcon_alloc_cables to simpli=
fy extcon register function

>--------- Original Message ---------
>Sender : =EC=9D=B4=EB=B2=94=EC=9A=B0=20<bw365.lee=40samsung.com>Product=20=
S/W=20Lab(VD)/=EC=82=BC=EC=84=B1=EC=A0=84=EC=9E=90=20Date=20:=20=0D=0A>2023=
-02-20=2014:45=20(GMT+9)=20Title=20:=20=5BPATCH=20v2=202/4=5D=20extcon:=20A=
dded=20=0D=0A>extcon_alloc_cables=20to=20simplify=20extcon=20register=20fun=
ction=0D=0A>=20=0D=0A>The=20cable=20allocation=20part=20is=20functionalized=
=20from=20extcon_dev_register.=0D=0A>=0D=0A>Signed-off-by:=20Bumwoo=20Lee=
=20<bw365.lee=40samsung.com>=0D=0A>---=0D=0A>=20drivers/extcon/extcon.c=20=
=7C=20104=20+++++++++++++++++++++++-----------------=0D=0A>=201=20file=20ch=
anged,=2059=20insertions(+),=2045=20deletions(-)=0D=0A>=0D=0A>diff=20--git=
=20a/drivers/extcon/extcon.c=20b/drivers/extcon/extcon.c=20index=20=0D=0A>a=
dcf01132f70..3c2f540785e8=20100644=0D=0A>---=20a/drivers/extcon/extcon.c=0D=
=0A>+++=20b/drivers/extcon/extcon.c=0D=0A>=40=40=20-1070,6=20+1070,61=20=40=
=40=20void=20extcon_dev_free(struct=20extcon_dev=20*edev)=20=20=7D=20=20=0D=
=0A>EXPORT_SYMBOL_GPL(extcon_dev_free);=0D=0A>=20=0D=0A>+/**=0D=0A>+=20*=20=
extcon_alloc_cables()=20-=20alloc=20the=20cables=20for=20extcon=20device=0D=
=0A>+=20*=20=40edev:=20=20=20=20=20=20=20=20extcon=20device=20which=20has=
=20cables=0D=0A>+=20*=0D=0A>+=20*=20Returns=200=20if=20success=20or=20error=
=20number=20if=20fail.=0D=0A>+=20*/=0D=0A>+static=20int=20extcon_alloc_cabl=
es(struct=20extcon_dev=20*edev)=20=7B=0D=0A>+=20=20=20=20=20=20=20=20int=20=
index;=0D=0A>+=20=20=20=20=20=20=20=20char=20*str;=0D=0A>+=20=20=20=20=20=
=20=20=20struct=20extcon_cable=20*cable;=0D=0A>+=0D=0A>+=20=20=20=20=20=20=
=20=20if=20(=21edev->max_supported)=0D=0A>+=20=20=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20return=200;=0D=0A>+=0D=0A>+=20=20=20=20=20=20=20=20edev->=
cables=20=3D=20kcalloc(edev->max_supported,=0D=0A>+=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20sizeof=
(struct=20extcon_cable),=0D=0A>+=20=20=20=20=20=20=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20GFP_KERNEL);=0D=0A>+=20=
=20=20=20=20=20=20=20if=20(=21edev->cables)=0D=0A>+=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20=20=20return=20-ENOMEM;=0D=0A>+=0D=0A>+=20=20=20=20=20=20=
=20=20for=20(index=20=3D=200;=20index=20<=20edev->max_supported;=20index++)=
=20=7B=0D=0A>+=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20cable=20=3D=
=20&edev->cables=5Bindex=5D;=0D=0A>+=0D=0A>+=20=20=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20str=20=3D=20kasprintf(GFP_KERNEL,=20=22cable.%d=22,=20ind=
ex);=0D=0A>+=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20if=20(=21str)=
=20=7B=0D=0A>+=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=
=20=20=20=20for=20(index--;=20index=20>=3D=200;=20index--)=20=7B=0D=0A>+=20=
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20cable=20=3D=20&edev->cables=5Bindex=5D;=0D=0A>+=20=20=20=
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=
=20=20=20=20kfree(cable->attr_g.name);=0D=0A>+=20=20=20=20=20=20=20=20=20=
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=7D=0D=0A>+=20=20=20=20=20=20=
=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20=20return=20-ENOMEM;=0D=
=0A=0D=0AYou=20have=20a=20memory=20leak.=0D=0Aedev->cables=20is=20allocated=
=20and=0D=0Ayou=20are=20not=20freeing=20it.=0D=0A=0D=0AIn=20the=20previous=
=20code,=20it=20was=20freed=20by=0D=0Ahaving=20different=20err-goto=20label=
s.=0D=0A=0D=0APlease=20check=20if=20you=20have=20similar=20errors=0D=0Ain=
=20other=20patches=20of=20this=20series.=0D=0A=0D=0A...=0D=0A=0D=0A>=40=40=
=20-1282,7=20+1296,7=20=40=40=20int=20extcon_dev_register(struct=20extcon_d=
ev=20*edev)=0D=0A>=20err_alloc_cables:=0D=0A>=20=20=20=20=20=20=20=20=20if=
=20(edev->max_supported)=0D=0A>=20=20=20=20=20=20=20=20=20=20=20=20=20=20=
=20=20=20kfree(edev->cables);=0D=0A>-err_sysfs_alloc:=0D=0A>+=0D=0A>=20=20=
=20=20=20=20=20=20=20return=20ret;=0D=0A>=20=7D=0D=0A>=20EXPORT_SYMBOL_GPL(=
extcon_dev_register);=0D=0A>--=0D=0A>2.35.1=0D=0A>=0D=0A>=0D=0A=0D=0ACheers=
,=0D=0AMyungJoo.=0D=0A=0D=0A=0D=0A