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

>--------- Original Message ---------
>Sender : 이범우 <bw365.lee@xxxxxxxxxxx>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@xxxxxxxxxxx>
>---
> 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.