diff mbox series

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

Message ID 20230220054513.27385-3-bw365.lee@samsung.com
State New
Headers
Series Simplify extcon_dev_register function. |

Commit Message

Bumwoo Lee Feb. 20, 2023, 5:45 a.m. UTC 
  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(-)

Comments

MyungJoo Ham Feb. 24, 2023, 10:03 a.m. UTC | #1 
>--------- 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.
Bumwoo Lee March 2, 2023, 1:38 a.m. UTC | #2 
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.
Slade's Kernel Patch Bot March 2, 2023, 3:44 a.m. UTC | #3 
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.
> 
> 
>
MyungJoo Ham March 2, 2023, 6:33 a.m. UTC | #4 
> 
> 
>--------- 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
Bumwoo Lee March 2, 2023, 7:12 a.m. UTC | #5 
> -----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
Bumwoo Lee March 2, 2023, 8:33 a.m. UTC | #6 
> -----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
diff mbox series

Patch

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);