[PATCH v2 0/4] Simplify extcon_dev_register function.

[PATCH v2 0/4] Simplify extcon_dev_register function.

[Bumwoo Lee]

It was modified to increase readability.

Changes from v1:
added return value handling.

Bumwoo Lee (4):
  extcon: Removed redundant null checking for class
  extcon: Added extcon_alloc_cables to simplify extcon register function
  extcon: Added extcon_alloc_muex to simplify extcon register function
  extcon: Added extcon_alloc_groups to simplify extcon register function

 drivers/extcon/extcon.c | 289 ++++++++++++++++++++++------------------
 1 file changed, 163 insertions(+), 126 deletions(-)

-- 
2.35.1

[PATCH v2 1/4] extcon: Removed redundant null checking for class

[Bumwoo Lee]

create_extcon_class() is already Null checking.

Signed-off-by: Bumwoo Lee <bw365.lee@samsung.com>
---
 drivers/extcon/extcon.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index e1c71359b605..adcf01132f70 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1012,12 +1012,13 @@ ATTRIBUTE_GROUPS(extcon);
 
 static int create_extcon_class(void)
 {
-	if (!extcon_class) {
-		extcon_class = class_create(THIS_MODULE, "extcon");
-		if (IS_ERR(extcon_class))
-			return PTR_ERR(extcon_class);
-		extcon_class->dev_groups = extcon_groups;
-	}
+	if (extcon_class)
+		return 0;
+
+	extcon_class = class_create(THIS_MODULE, "extcon");
+	if (IS_ERR(extcon_class))
+		return PTR_ERR(extcon_class);
+	extcon_class->dev_groups = extcon_groups;
 
 	return 0;
 }
@@ -1088,11 +1089,9 @@ int extcon_dev_register(struct extcon_dev *edev)
 	int ret, index = 0;
 	static atomic_t edev_no = ATOMIC_INIT(-1);
 
-	if (!extcon_class) {
-		ret = create_extcon_class();
-		if (ret < 0)
-			return ret;
-	}
+	ret = create_extcon_class();
+	if (ret < 0)
+		return ret;
 
 	if (!edev || !edev->supported_cable)
 		return -EINVAL;
-- 
2.35.1

[PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

[Bumwoo Lee]

The cable allocation part is functionalized from extcon_dev_register.

Signed-off-by: Bumwoo Lee <bw365.lee@samsung.com>
---
 drivers/extcon/extcon.c | 104 +++++++++++++++++++++++-----------------
 1 file changed, 59 insertions(+), 45 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index adcf01132f70..3c2f540785e8 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1070,6 +1070,61 @@ void extcon_dev_free(struct extcon_dev *edev)
 }
 EXPORT_SYMBOL_GPL(extcon_dev_free);
 
+/**
+ * extcon_alloc_cables() - alloc the cables for extcon device
+ * @edev:	extcon device which has cables
+ *
+ * Returns 0 if success or error number if fail.
+ */
+static int extcon_alloc_cables(struct extcon_dev *edev)
+{
+	int index;
+	char *str;
+	struct extcon_cable *cable;
+
+	if (!edev->max_supported)
+		return 0;
+
+	edev->cables = kcalloc(edev->max_supported,
+			       sizeof(struct extcon_cable),
+			       GFP_KERNEL);
+	if (!edev->cables)
+		return -ENOMEM;
+
+	for (index = 0; index < edev->max_supported; index++) {
+		cable = &edev->cables[index];
+
+		str = kasprintf(GFP_KERNEL, "cable.%d", index);
+		if (!str) {
+			for (index--; index >= 0; index--) {
+				cable = &edev->cables[index];
+				kfree(cable->attr_g.name);
+			}
+			return -ENOMEM;
+		}
+
+		cable->edev = edev;
+		cable->cable_index = index;
+		cable->attrs[0] = &cable->attr_name.attr;
+		cable->attrs[1] = &cable->attr_state.attr;
+		cable->attrs[2] = NULL;
+		cable->attr_g.name = str;
+		cable->attr_g.attrs = cable->attrs;
+
+		sysfs_attr_init(&cable->attr_name.attr);
+		cable->attr_name.attr.name = "name";
+		cable->attr_name.attr.mode = 0444;
+		cable->attr_name.show = cable_name_show;
+
+		sysfs_attr_init(&cable->attr_state.attr);
+		cable->attr_state.attr.name = "state";
+		cable->attr_state.attr.mode = 0444;
+		cable->attr_state.show = cable_state_show;
+	}
+
+	return 0;
+}
+
 /**
  * extcon_dev_register() - Register an new extcon device
  * @edev:	the extcon device to be registered
@@ -1117,50 +1172,9 @@ int extcon_dev_register(struct extcon_dev *edev)
 	dev_set_name(&edev->dev, "extcon%lu",
 			(unsigned long)atomic_inc_return(&edev_no));
 
-	if (edev->max_supported) {
-		char *str;
-		struct extcon_cable *cable;
-
-		edev->cables = kcalloc(edev->max_supported,
-				       sizeof(struct extcon_cable),
-				       GFP_KERNEL);
-		if (!edev->cables) {
-			ret = -ENOMEM;
-			goto err_sysfs_alloc;
-		}
-		for (index = 0; index < edev->max_supported; index++) {
-			cable = &edev->cables[index];
-
-			str = kasprintf(GFP_KERNEL, "cable.%d", index);
-			if (!str) {
-				for (index--; index >= 0; index--) {
-					cable = &edev->cables[index];
-					kfree(cable->attr_g.name);
-				}
-				ret = -ENOMEM;
-
-				goto err_alloc_cables;
-			}
-
-			cable->edev = edev;
-			cable->cable_index = index;
-			cable->attrs[0] = &cable->attr_name.attr;
-			cable->attrs[1] = &cable->attr_state.attr;
-			cable->attrs[2] = NULL;
-			cable->attr_g.name = str;
-			cable->attr_g.attrs = cable->attrs;
-
-			sysfs_attr_init(&cable->attr_name.attr);
-			cable->attr_name.attr.name = "name";
-			cable->attr_name.attr.mode = 0444;
-			cable->attr_name.show = cable_name_show;
-
-			sysfs_attr_init(&cable->attr_state.attr);
-			cable->attr_state.attr.name = "state";
-			cable->attr_state.attr.mode = 0444;
-			cable->attr_state.show = cable_state_show;
-		}
-	}
+	ret = extcon_alloc_cables(edev);
+	if (ret)
+		goto err_alloc_cables;
 
 	if (edev->max_supported && edev->mutually_exclusive) {
 		char *name;
@@ -1282,7 +1296,7 @@ int extcon_dev_register(struct extcon_dev *edev)
 err_alloc_cables:
 	if (edev->max_supported)
 		kfree(edev->cables);
-err_sysfs_alloc:
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(extcon_dev_register);
-- 
2.35.1

[PATCH v2 3/4] extcon: Added extcon_alloc_muex to simplify extcon register function

[Bumwoo Lee]

The mutual exclusive part is functionalized from extcon_dev_register.

Signed-off-by: Bumwoo Lee <bw365.lee@samsung.com>
---
 drivers/extcon/extcon.c | 106 ++++++++++++++++++++++------------------
 1 file changed, 58 insertions(+), 48 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 3c2f540785e8..2aec34909843 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1125,6 +1125,60 @@ static int extcon_alloc_cables(struct extcon_dev *edev)
 	return 0;
 }
 
+/**
+ * extcon_alloc_muex() - alloc the mutual exclusive for extcon device
+ * @edev:	extcon device
+ *
+ * Returns 0 if success or error number if fail.
+ */
+static int extcon_alloc_muex(struct extcon_dev *edev)
+{
+	char *name;
+	int index;
+
+	if (!(edev->max_supported && edev->mutually_exclusive))
+		return 0;
+
+	/* Count the size of mutually_exclusive array */
+	for (index = 0; edev->mutually_exclusive[index]; index++)
+		;
+
+	edev->attrs_muex = kcalloc(index + 1,
+				   sizeof(struct attribute *),
+				   GFP_KERNEL);
+	if (!edev->attrs_muex)
+		return -ENOMEM;
+
+	edev->d_attrs_muex = kcalloc(index,
+				     sizeof(struct device_attribute),
+				     GFP_KERNEL);
+	if (!edev->d_attrs_muex) {
+		kfree(edev->attrs_muex);
+		return -ENOMEM;
+	}
+
+	for (index = 0; edev->mutually_exclusive[index]; index++) {
+		name = kasprintf(GFP_KERNEL, "0x%x",
+				 edev->mutually_exclusive[index]);
+		if (!name) {
+			for (index--; index >= 0; index--)
+				kfree(edev->d_attrs_muex[index].attr.name);
+
+			kfree(edev->d_attrs_muex);
+			kfree(edev->attrs_muex);
+			return -ENOMEM;
+		}
+		sysfs_attr_init(&edev->d_attrs_muex[index].attr);
+		edev->d_attrs_muex[index].attr.name = name;
+		edev->d_attrs_muex[index].attr.mode = 0000;
+		edev->attrs_muex[index] = &edev->d_attrs_muex[index].attr;
+	}
+	edev->attr_g_muex.name = muex_name;
+	edev->attr_g_muex.attrs = edev->attrs_muex;
+
+	return 0;
+}
+
 /**
  * extcon_dev_register() - Register an new extcon device
  * @edev:	the extcon device to be registered
@@ -1176,53 +1230,9 @@ int extcon_dev_register(struct extcon_dev *edev)
 	if (ret)
 		goto err_alloc_cables;
 
-	if (edev->max_supported && edev->mutually_exclusive) {
-		char *name;
-
-		/* Count the size of mutually_exclusive array */
-		for (index = 0; edev->mutually_exclusive[index]; index++)
-			;
-
-		edev->attrs_muex = kcalloc(index + 1,
-					   sizeof(struct attribute *),
-					   GFP_KERNEL);
-		if (!edev->attrs_muex) {
-			ret = -ENOMEM;
-			goto err_muex;
-		}
-
-		edev->d_attrs_muex = kcalloc(index,
-					     sizeof(struct device_attribute),
-					     GFP_KERNEL);
-		if (!edev->d_attrs_muex) {
-			ret = -ENOMEM;
-			kfree(edev->attrs_muex);
-			goto err_muex;
-		}
-
-		for (index = 0; edev->mutually_exclusive[index]; index++) {
-			name = kasprintf(GFP_KERNEL, "0x%x",
-					 edev->mutually_exclusive[index]);
-			if (!name) {
-				for (index--; index >= 0; index--) {
-					kfree(edev->d_attrs_muex[index].attr.
-					      name);
-				}
-				kfree(edev->d_attrs_muex);
-				kfree(edev->attrs_muex);
-				ret = -ENOMEM;
-				goto err_muex;
-			}
-			sysfs_attr_init(&edev->d_attrs_muex[index].attr);
-			edev->d_attrs_muex[index].attr.name = name;
-			edev->d_attrs_muex[index].attr.mode = 0000;
-			edev->attrs_muex[index] = &edev->d_attrs_muex[index]
-							.attr;
-		}
-		edev->attr_g_muex.name = muex_name;
-		edev->attr_g_muex.attrs = edev->attrs_muex;
-
-	}
+	ret = extcon_alloc_muex(edev);
+	if (ret)
+		goto err_alloc_muex;
 
 	if (edev->max_supported) {
 		edev->extcon_dev_type.groups =
@@ -1290,7 +1300,7 @@ int extcon_dev_register(struct extcon_dev *edev)
 		kfree(edev->d_attrs_muex);
 		kfree(edev->attrs_muex);
 	}
-err_muex:
+err_alloc_muex:
 	for (index = 0; index < edev->max_supported; index++)
 		kfree(edev->cables[index].attr_g.name);
 err_alloc_cables:
-- 
2.35.1

[PATCH v2 4/4] extcon: Added extcon_alloc_groups to simplify extcon register function

[Bumwoo Lee]

The alloc groups is functionalized from extcon_dev_register.

Signed-off-by: Bumwoo Lee <bw365.lee@samsung.com>
---
 drivers/extcon/extcon.c | 58 +++++++++++++++++++++++++----------------
 1 file changed, 36 insertions(+), 22 deletions(-)

diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
index 2aec34909843..919d77539039 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
@@ -1179,6 +1179,39 @@ static int extcon_alloc_muex(struct extcon_dev *edev)
 	return 0;
 }
 
+/**
+ * extcon_alloc_groups() - alloc the groups for extcon device
+ * @edev:	extcon device
+ *
+ * Returns 0 if success or error number if fail.
+ */
+static int extcon_alloc_groups(struct extcon_dev *edev)
+{
+	int index;
+
+	if (!edev->max_supported)
+		return 0;
+
+	edev->extcon_dev_type.groups = kcalloc(edev->max_supported + 2,
+			sizeof(struct attribute_group *),
+			GFP_KERNEL);
+	if (!edev->extcon_dev_type.groups)
+		return -ENOMEM;
+
+	edev->extcon_dev_type.name = dev_name(&edev->dev);
+	edev->extcon_dev_type.release = dummy_sysfs_dev_release;
+
+	for (index = 0; index < edev->max_supported; index++)
+		edev->extcon_dev_type.groups[index] = &edev->cables[index].attr_g;
+
+	if (edev->mutually_exclusive)
+		edev->extcon_dev_type.groups[index] = &edev->attr_g_muex;
+
+	edev->dev.type = &edev->extcon_dev_type;
+
+	return 0;
+}
+
 /**
  * extcon_dev_register() - Register an new extcon device
  * @edev:	the extcon device to be registered
@@ -1234,28 +1267,9 @@ int extcon_dev_register(struct extcon_dev *edev)
 	if (ret)
 		goto err_alloc_muex;
 
-	if (edev->max_supported) {
-		edev->extcon_dev_type.groups =
-			kcalloc(edev->max_supported + 2,
-				sizeof(struct attribute_group *),
-				GFP_KERNEL);
-		if (!edev->extcon_dev_type.groups) {
-			ret = -ENOMEM;
-			goto err_alloc_groups;
-		}
-
-		edev->extcon_dev_type.name = dev_name(&edev->dev);
-		edev->extcon_dev_type.release = dummy_sysfs_dev_release;
-
-		for (index = 0; index < edev->max_supported; index++)
-			edev->extcon_dev_type.groups[index] =
-				&edev->cables[index].attr_g;
-		if (edev->mutually_exclusive)
-			edev->extcon_dev_type.groups[index] =
-				&edev->attr_g_muex;
-
-		edev->dev.type = &edev->extcon_dev_type;
-	}
+	ret = extcon_alloc_groups(edev);
+	if (ret)
+		goto err_alloc_groups;
 
 	spin_lock_init(&edev->lock);
 	if (edev->max_supported) {
-- 
2.35.1

RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

[MyungJoo Ham]

>--------- Original Message ---------
>Sender : 이범우 <bw365.lee@samsung.com>Product S/W Lab(VD)/삼성전자
>Date : 2023-02-20 14:45 (GMT+9)
>Title : [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
> 
>The cable allocation part is functionalized from extcon_dev_register.
>
>Signed-off-by: Bumwoo Lee <bw365.lee@samsung.com>
>---
> drivers/extcon/extcon.c | 104 +++++++++++++++++++++++-----------------
> 1 file changed, 59 insertions(+), 45 deletions(-)
>
>diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
>index adcf01132f70..3c2f540785e8 100644
>--- a/drivers/extcon/extcon.c
>+++ b/drivers/extcon/extcon.c
>@@ -1070,6 +1070,61 @@ void extcon_dev_free(struct extcon_dev *edev)
> }
> EXPORT_SYMBOL_GPL(extcon_dev_free);
> 
>+/**
>+ * extcon_alloc_cables() - alloc the cables for extcon device
>+ * @edev:        extcon device which has cables
>+ *
>+ * Returns 0 if success or error number if fail.
>+ */
>+static int extcon_alloc_cables(struct extcon_dev *edev)
>+{
>+        int index;
>+        char *str;
>+        struct extcon_cable *cable;
>+
>+        if (!edev->max_supported)
>+                return 0;
>+
>+        edev->cables = kcalloc(edev->max_supported,
>+                               sizeof(struct extcon_cable),
>+                               GFP_KERNEL);
>+        if (!edev->cables)
>+                return -ENOMEM;
>+
>+        for (index = 0; index < edev->max_supported; index++) {
>+                cable = &edev->cables[index];
>+
>+                str = kasprintf(GFP_KERNEL, "cable.%d", index);
>+                if (!str) {
>+                        for (index--; index >= 0; index--) {
>+                                cable = &edev->cables[index];
>+                                kfree(cable->attr_g.name);
>+                        }
>+                        return -ENOMEM;

You have a memory leak.
edev->cables is allocated and
you are not freeing it.

In the previous code, it was freed by
having different err-goto labels.

Please check if you have similar errors
in other patches of this series.

...

>@@ -1282,7 +1296,7 @@ int extcon_dev_register(struct extcon_dev *edev)
> err_alloc_cables:
>         if (edev->max_supported)
>                 kfree(edev->cables);
>-err_sysfs_alloc:
>+
>         return ret;
> }
> EXPORT_SYMBOL_GPL(extcon_dev_register);
>-- 
>2.35.1
>
>

Cheers,
MyungJoo.

RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

[Bumwoo Lee]

Hello.

As you can see, edev->cables are freed if extcon_alloc_cables() function return 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 it should be freed within the function? 
Please let me know your opinion.

extcon_dev_register(struct extcon_dev *edev){
...

	ret = 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@samsung.com> 
Sent: Friday, February 24, 2023 7:03 PM
To: Bumwoo Lee <bw365.lee@samsung.com>; Chanwoo Choi <cw00.choi@samsung.com>; linux-kernel@vger.kernel.org
Subject: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

>--------- Original Message ---------
>Sender : 이범우 <bw365.lee@samsung.com>Product S/W Lab(VD)/삼성전자 Date : 
>2023-02-20 14:45 (GMT+9) Title : [PATCH v2 2/4] extcon: Added 
>extcon_alloc_cables to simplify extcon register function
> 
>The cable allocation part is functionalized from extcon_dev_register.
>
>Signed-off-by: Bumwoo Lee <bw365.lee@samsung.com>
>---
> drivers/extcon/extcon.c | 104 +++++++++++++++++++++++-----------------
> 1 file changed, 59 insertions(+), 45 deletions(-)
>
>diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index 
>adcf01132f70..3c2f540785e8 100644
>--- a/drivers/extcon/extcon.c
>+++ b/drivers/extcon/extcon.c
>@@ -1070,6 +1070,61 @@ void extcon_dev_free(struct extcon_dev *edev)  }  
>EXPORT_SYMBOL_GPL(extcon_dev_free);
> 
>+/**
>+ * extcon_alloc_cables() - alloc the cables for extcon device
>+ * @edev:        extcon device which has cables
>+ *
>+ * Returns 0 if success or error number if fail.
>+ */
>+static int extcon_alloc_cables(struct extcon_dev *edev) {
>+        int index;
>+        char *str;
>+        struct extcon_cable *cable;
>+
>+        if (!edev->max_supported)
>+                return 0;
>+
>+        edev->cables = kcalloc(edev->max_supported,
>+                               sizeof(struct extcon_cable),
>+                               GFP_KERNEL);
>+        if (!edev->cables)
>+                return -ENOMEM;
>+
>+        for (index = 0; index < edev->max_supported; index++) {
>+                cable = &edev->cables[index];
>+
>+                str = kasprintf(GFP_KERNEL, "cable.%d", index);
>+                if (!str) {
>+                        for (index--; index >= 0; index--) {
>+                                cable = &edev->cables[index];
>+                                kfree(cable->attr_g.name);
>+                        }
>+                        return -ENOMEM;

You have a memory leak.
edev->cables is allocated and
you are not freeing it.

In the previous code, it was freed by
having different err-goto labels.

Please check if you have similar errors
in other patches of this series.

...

>@@ -1282,7 +1296,7 @@ int extcon_dev_register(struct extcon_dev *edev)
> err_alloc_cables:
>         if (edev->max_supported)
>                 kfree(edev->cables);
>-err_sysfs_alloc:
>+
>         return ret;
> }
> EXPORT_SYMBOL_GPL(extcon_dev_register);
>--
>2.35.1
>
>

Cheers,
MyungJoo.

Re: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

[Slade's Kernel Patch Bot]

On 3/1/23 20:38, Bumwoo Lee wrote:
> Hello.
> 
> As you can see, edev->cables are freed if extcon_alloc_cables() function return 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 it should be freed within the function? 
> Please let me know your opinion.
> 
> extcon_dev_register(struct extcon_dev *edev){
> ...
> 
> 	ret = extcon_alloc_cables(edev);
> 	if (ret)
> 		goto err_alloc_cables;
> 
> ...
> 
> err_alloc_cables:
>  	if (edev->max_supported)
>  		kfree(edev->cables);
> 
> 
> Regards,
> Bumwoo

This is Slade's kernel patch bot. When scanning his mailbox, I came across
this message, which appears to be a top-post. Please do not top-post on Linux
mailing lists.

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

Please bottom-post to Linux mailing lists in the future. See also:
https://daringfireball.net/2007/07/on_top

If you believe this is an error, please address a message to Slade Watkins
<srw@sladewatkins.net>.

Thank you,
-- Slade's kernel patch bot

> 
> -----Original Message-----
> From: MyungJoo Ham <myungjoo.ham@samsung.com> 
> Sent: Friday, February 24, 2023 7:03 PM
> To: Bumwoo Lee <bw365.lee@samsung.com>; Chanwoo Choi <cw00.choi@samsung.com>; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
> 
>> --------- Original Message ---------
>> Sender : 이범우 <bw365.lee@samsung.com>Product S/W Lab(VD)/삼성전자 Date : 
>> 2023-02-20 14:45 (GMT+9) Title : [PATCH v2 2/4] extcon: Added 
>> extcon_alloc_cables to simplify extcon register function
>>
>> The cable allocation part is functionalized from extcon_dev_register.
>>
>> Signed-off-by: Bumwoo Lee <bw365.lee@samsung.com>
>> ---
>> drivers/extcon/extcon.c | 104 +++++++++++++++++++++++-----------------
>> 1 file changed, 59 insertions(+), 45 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index 
>> adcf01132f70..3c2f540785e8 100644
>> --- a/drivers/extcon/extcon.c
>> +++ b/drivers/extcon/extcon.c
>> @@ -1070,6 +1070,61 @@ void extcon_dev_free(struct extcon_dev *edev)  }  
>> EXPORT_SYMBOL_GPL(extcon_dev_free);
>>
>> +/**
>> + * extcon_alloc_cables() - alloc the cables for extcon device
>> + * @edev:        extcon device which has cables
>> + *
>> + * Returns 0 if success or error number if fail.
>> + */
>> +static int extcon_alloc_cables(struct extcon_dev *edev) {
>> +        int index;
>> +        char *str;
>> +        struct extcon_cable *cable;
>> +
>> +        if (!edev->max_supported)
>> +                return 0;
>> +
>> +        edev->cables = kcalloc(edev->max_supported,
>> +                               sizeof(struct extcon_cable),
>> +                               GFP_KERNEL);
>> +        if (!edev->cables)
>> +                return -ENOMEM;
>> +
>> +        for (index = 0; index < edev->max_supported; index++) {
>> +                cable = &edev->cables[index];
>> +
>> +                str = kasprintf(GFP_KERNEL, "cable.%d", index);
>> +                if (!str) {
>> +                        for (index--; index >= 0; index--) {
>> +                                cable = &edev->cables[index];
>> +                                kfree(cable->attr_g.name);
>> +                        }
>> +                        return -ENOMEM;
> 
> You have a memory leak.
> edev->cables is allocated and
> you are not freeing it.
> 
> In the previous code, it was freed by
> having different err-goto labels.
> 
> Please check if you have similar errors
> in other patches of this series.
> 
> ...
> 
>> @@ -1282,7 +1296,7 @@ int extcon_dev_register(struct extcon_dev *edev)
>> err_alloc_cables:
>>         if (edev->max_supported)
>>                 kfree(edev->cables);
>> -err_sysfs_alloc:
>> +
>>         return ret;
>> }
>> EXPORT_SYMBOL_GPL(extcon_dev_register);
>> --
>> 2.35.1
>>
>>
> 
> Cheers,
> MyungJoo.
> 
> 
> 

RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

[MyungJoo Ham]

> 
> 
>--------- Original Message ---------
>Sender : 이범우 <bw365.lee@samsung.com>Product S/W Lab(VD)/삼성전자
>Date : 2023-03-02 10:38 (GMT+9)
>Title : RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
> 
>Hello.
>
>As you can see, edev->cables are freed if extcon_alloc_cables() function return 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 it should be freed within the function? 
>Please let me know your opinion.
>
>extcon_dev_register(struct extcon_dev *edev){
>...
>
>        ret = extcon_alloc_cables(edev);
>        if (ret)
>                goto err_alloc_cables;
>
>...
>
>err_alloc_cables:
>         if (edev->max_supported)
>                 kfree(edev->cables);
>
>
>Regards,
>Bumwoo

In such a case, you are doing kfree(NULL); with the following:

>+static int extcon_alloc_cables(struct extcon_dev *edev) {
...
>+        if (!edev->max_supported)
>+                return 0;
>+
>+        edev->cables = kcalloc(edev->max_supported,
>+                               sizeof(struct extcon_cable),
>+                               GFP_KERNEL);
>+        if (!edev->cables)
>+                return -ENOMEM;




Cheers,
MyungJoo

RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

[Bumwoo Lee]

> -----Original Message-----
> From: MyungJoo Ham <myungjoo.ham@samsung.com>
> Sent: Thursday, March 2, 2023 3:33 PM
> To: Bumwoo Lee <bw365.lee@samsung.com>; Chanwoo Choi
> <cw00.choi@samsung.com>; linux-kernel@vger.kernel.org
> Subject: RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to
> simplify extcon register function
> 
> >
> >
> >--------- Original Message ---------
> >Sender : 이범우 <bw365.lee@samsung.com>Product S/W Lab(VD)/삼성전자 Date :
> >2023-03-02 10:38 (GMT+9) Title : RE: [PATCH v2 2/4] extcon: Added
> >extcon_alloc_cables to simplify extcon register function
> >
> >Hello.
> >
> >As you can see, edev->cables are freed if extcon_alloc_cables()
> >function return 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
> it should be freed within the function?
> >Please let me know your opinion.
> >
> >extcon_dev_register(struct extcon_dev *edev){ ...
> >
> >        ret = extcon_alloc_cables(edev);
> >        if (ret)
> >                goto err_alloc_cables;
> >
> >...
> >
> >err_alloc_cables:
> >         if (edev->max_supported)
> >                 kfree(edev->cables);
> >
> >
> >Regards,
> >Bumwoo
> 
> In such a case, you are doing kfree(NULL); with the following:

Yes, you are right.
But Kfree() is checking NULL internally. So it does not a problem I think.
Anyway I added kfree(edev->cables) before exit extcon_alloc_cables() like below on v3 patches.

static int extcon_alloc_cables(struct extcon_dev *edev) {
...
                         for (index--; index >= 0; index--) {
                                 cable = &edev->cables[index];
                                 kfree(cable->attr_g.name);
                         }
 
+                        kfree(edev->cables);
                         return -ENOMEM;
                 }



> >+static int extcon_alloc_cables(struct extcon_dev *edev) {
> ...
> >+        if (!edev->max_supported)
> >+                return 0;
> >+
> >+        edev->cables = kcalloc(edev->max_supported,
> >+                               sizeof(struct extcon_cable),
> >+                               GFP_KERNEL);
> >+        if (!edev->cables)
> >+                return -ENOMEM;
> 
> 
> 
> 
> Cheers,
> MyungJoo
> 

Regards,
Bumwoo

RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function

[Bumwoo Lee]

> -----Original Message-----
> From: Bumwoo Lee <bw365.lee@samsung.com>
> Sent: Thursday, March 2, 2023 4:12 PM
> To: 'myungjoo.ham@samsung.com' <myungjoo.ham@samsung.com>; 'Chanwoo Choi'
> <cw00.choi@samsung.com>; 'linux-kernel@vger.kernel.org' <linux-
> kernel@vger.kernel.org>
> Subject: RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to
> simplify extcon register function
> 
> 
> > -----Original Message-----
> > From: MyungJoo Ham <myungjoo.ham@samsung.com>
> > Sent: Thursday, March 2, 2023 3:33 PM
> > To: Bumwoo Lee <bw365.lee@samsung.com>; Chanwoo Choi
> > <cw00.choi@samsung.com>; linux-kernel@vger.kernel.org
> > Subject: RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to
> > simplify extcon register function
> >
> > >
> > >
> > >--------- Original Message ---------
> > >Sender : 이범우 <bw365.lee@samsung.com>Product S/W Lab(VD)/삼성전자 Date :
> > >2023-03-02 10:38 (GMT+9) Title : RE: [PATCH v2 2/4] extcon: Added
> > >extcon_alloc_cables to simplify extcon register function
> > >
> > >Hello.
> > >
> > >As you can see, edev->cables are freed if extcon_alloc_cables()
> > >function return 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
> > it should be freed within the function?
> > >Please let me know your opinion.
> > >
> > >extcon_dev_register(struct extcon_dev *edev){ ...
> > >
> > >        ret = extcon_alloc_cables(edev);
> > >        if (ret)
> > >                goto err_alloc_cables;
> > >
> > >...
> > >
> > >err_alloc_cables:
> > >         if (edev->max_supported)
> > >                 kfree(edev->cables);
> > >
> > >
> > >Regards,
> > >Bumwoo
> >
> > In such a case, you are doing kfree(NULL); with the following:
> 
> Yes, you are right.
> But Kfree() is checking NULL internally. So it does not a problem I think.
> Anyway I added kfree(edev->cables) before exit extcon_alloc_cables() like
> below on v3 patches.
> 
> static int extcon_alloc_cables(struct extcon_dev *edev) { ...
>                          for (index--; index >= 0; index--) {
>                                  cable = &edev->cables[index];
>                                  kfree(cable->attr_g.name);
>                          }
> 
> +                        kfree(edev->cables);
>                          return -ENOMEM;
>                  }
> 

In order to avoid unnecessary kfree(NULL), the position will be moved like below on v4 patch.

err_alloc_muex:
	for (index = 0; index < edev->max_supported; index++)
        	kfree(edev->cables[index].attr_g.name);
+	if (edev->max_supported)
+		kfree(edev->cables);
err_alloc_cables:
-	if (edev->max_supported)
-		kfree(edev->cables); 
        return ret;


> Regards,
> Bumwoo