* [PATCH v2 0/4] Simplify extcon_dev_register function.
[not found] <CGME20230220054545epcas1p4a2e170c1c67a675c6a54496417171d51@epcas1p4.samsung.com>
@ 2023-02-20 5:45 ` Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 1/4] extcon: Removed redundant null checking for class Bumwoo Lee
` (3 more replies)
0 siblings, 4 replies; 11+ messages in thread
From: Bumwoo Lee @ 2023-02-20 5:45 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel; +Cc: 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/4] extcon: Removed redundant null checking for class
2023-02-20 5:45 ` [PATCH v2 0/4] Simplify extcon_dev_register function Bumwoo Lee
@ 2023-02-20 5:45 ` Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function Bumwoo Lee
` (2 subsequent siblings)
3 siblings, 0 replies; 11+ messages in thread
From: Bumwoo Lee @ 2023-02-20 5:45 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
2023-02-20 5:45 ` [PATCH v2 0/4] Simplify extcon_dev_register function Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 1/4] extcon: Removed redundant null checking for class Bumwoo Lee
@ 2023-02-20 5:45 ` Bumwoo Lee
2023-02-24 10:03 ` MyungJoo Ham
2023-02-20 5:45 ` [PATCH v2 3/4] extcon: Added extcon_alloc_muex " Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 4/4] extcon: Added extcon_alloc_groups " Bumwoo Lee
3 siblings, 1 reply; 11+ messages in thread
From: Bumwoo Lee @ 2023-02-20 5:45 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 3/4] extcon: Added extcon_alloc_muex to simplify extcon register function
2023-02-20 5:45 ` [PATCH v2 0/4] Simplify extcon_dev_register function Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 1/4] extcon: Removed redundant null checking for class Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function Bumwoo Lee
@ 2023-02-20 5:45 ` Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 4/4] extcon: Added extcon_alloc_groups " Bumwoo Lee
3 siblings, 0 replies; 11+ messages in thread
From: Bumwoo Lee @ 2023-02-20 5:45 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 4/4] extcon: Added extcon_alloc_groups to simplify extcon register function
2023-02-20 5:45 ` [PATCH v2 0/4] Simplify extcon_dev_register function Bumwoo Lee
` (2 preceding siblings ...)
2023-02-20 5:45 ` [PATCH v2 3/4] extcon: Added extcon_alloc_muex " Bumwoo Lee
@ 2023-02-20 5:45 ` Bumwoo Lee
3 siblings, 0 replies; 11+ messages in thread
From: Bumwoo Lee @ 2023-02-20 5:45 UTC (permalink / raw)
To: MyungJoo Ham, Chanwoo Choi, linux-kernel; +Cc: 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
2023-02-20 5:45 ` [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function Bumwoo Lee
@ 2023-02-24 10:03 ` MyungJoo Ham
2023-03-02 1:38 ` Bumwoo Lee
0 siblings, 1 reply; 11+ messages in thread
From: MyungJoo Ham @ 2023-02-24 10:03 UTC (permalink / raw)
To: Bumwoo Lee, Chanwoo Choi, linux-kernel@vger.kernel.org
>--------- 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
2023-02-24 10:03 ` MyungJoo Ham
@ 2023-03-02 1:38 ` Bumwoo Lee
2023-03-02 3:44 ` Slade's Kernel Patch Bot
2023-03-02 6:33 ` MyungJoo Ham
0 siblings, 2 replies; 11+ messages in thread
From: Bumwoo Lee @ 2023-03-02 1:38 UTC (permalink / raw)
To: myungjoo.ham, 'Chanwoo Choi', linux-kernel
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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
2023-03-02 1:38 ` Bumwoo Lee
@ 2023-03-02 3:44 ` Slade's Kernel Patch Bot
2023-03-02 6:33 ` MyungJoo Ham
1 sibling, 0 replies; 11+ messages in thread
From: Slade's Kernel Patch Bot @ 2023-03-02 3:44 UTC (permalink / raw)
To: Bumwoo Lee
Cc: 'Chanwoo Choi', myungjoo.ham, linux-kernel, Slade Watkins
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.
>
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
2023-03-02 1:38 ` Bumwoo Lee
2023-03-02 3:44 ` Slade's Kernel Patch Bot
@ 2023-03-02 6:33 ` MyungJoo Ham
2023-03-02 7:12 ` Bumwoo Lee
2023-03-02 8:33 ` Bumwoo Lee
1 sibling, 2 replies; 11+ messages in thread
From: MyungJoo Ham @ 2023-03-02 6:33 UTC (permalink / raw)
To: Bumwoo Lee, Chanwoo Choi, linux-kernel@vger.kernel.org
>
>
>--------- 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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
2023-03-02 6:33 ` MyungJoo Ham
@ 2023-03-02 7:12 ` Bumwoo Lee
2023-03-02 8:33 ` Bumwoo Lee
1 sibling, 0 replies; 11+ messages in thread
From: Bumwoo Lee @ 2023-03-02 7:12 UTC (permalink / raw)
To: myungjoo.ham, 'Chanwoo Choi', linux-kernel
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
2023-03-02 6:33 ` MyungJoo Ham
2023-03-02 7:12 ` Bumwoo Lee
@ 2023-03-02 8:33 ` Bumwoo Lee
1 sibling, 0 replies; 11+ messages in thread
From: Bumwoo Lee @ 2023-03-02 8:33 UTC (permalink / raw)
To: myungjoo.ham, 'Chanwoo Choi', linux-kernel
> -----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
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2023-03-02 8:33 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <CGME20230220054545epcas1p4a2e170c1c67a675c6a54496417171d51@epcas1p4.samsung.com>
2023-02-20 5:45 ` [PATCH v2 0/4] Simplify extcon_dev_register function Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 1/4] extcon: Removed redundant null checking for class Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function Bumwoo Lee
2023-02-24 10:03 ` MyungJoo Ham
2023-03-02 1:38 ` Bumwoo Lee
2023-03-02 3:44 ` Slade's Kernel Patch Bot
2023-03-02 6:33 ` MyungJoo Ham
2023-03-02 7:12 ` Bumwoo Lee
2023-03-02 8:33 ` Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 3/4] extcon: Added extcon_alloc_muex " Bumwoo Lee
2023-02-20 5:45 ` [PATCH v2 4/4] extcon: Added extcon_alloc_groups " Bumwoo Lee
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox