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
The alloc groups is functionalized from extcon_dev_register.
Signed-off-by: Bumwoo Lee <[email protected]>
---
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
The mutual exclusive part is functionalized from extcon_dev_register.
Signed-off-by: Bumwoo Lee <[email protected]>
---
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
The cable allocation part is functionalized from extcon_dev_register.
Signed-off-by: Bumwoo Lee <[email protected]>
---
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
>--------- Original Message ---------
>Sender : 이범우 <[email protected]>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 <[email protected]>
>---
> 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.
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 <[email protected]>
Sent: Friday, February 24, 2023 7:03 PM
To: Bumwoo Lee <[email protected]>; Chanwoo Choi <[email protected]>; [email protected]
Subject: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
>--------- Original Message ---------
>Sender : 이범우 <[email protected]>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 <[email protected]>
>---
> 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.
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
<[email protected]>.
Thank you,
-- Slade's kernel patch bot
>
> -----Original Message-----
> From: MyungJoo Ham <[email protected]>
> Sent: Friday, February 24, 2023 7:03 PM
> To: Bumwoo Lee <[email protected]>; Chanwoo Choi <[email protected]>; [email protected]
> Subject: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify extcon register function
>
>> --------- Original Message ---------
>> Sender : 이범우 <[email protected]>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 <[email protected]>
>> ---
>> 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.
>
>
>
>
>
>--------- Original Message ---------
>Sender : 이범우 <[email protected]>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
> -----Original Message-----
> From: MyungJoo Ham <[email protected]>
> Sent: Thursday, March 2, 2023 3:33 PM
> To: Bumwoo Lee <[email protected]>; Chanwoo Choi
> <[email protected]>; [email protected]
> Subject: RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to
> simplify extcon register function
>
> >
> >
> >--------- Original Message ---------
> >Sender : 이범우 <[email protected]>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
> -----Original Message-----
> From: Bumwoo Lee <[email protected]>
> Sent: Thursday, March 2, 2023 4:12 PM
> To: '[email protected]' <[email protected]>; 'Chanwoo Choi'
> <[email protected]>; '[email protected]' <linux-
> [email protected]>
> Subject: RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to
> simplify extcon register function
>
>
> > -----Original Message-----
> > From: MyungJoo Ham <[email protected]>
> > Sent: Thursday, March 2, 2023 3:33 PM
> > To: Bumwoo Lee <[email protected]>; Chanwoo Choi
> > <[email protected]>; [email protected]
> > Subject: RE: RE: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to
> > simplify extcon register function
> >
> > >
> > >
> > >--------- Original Message ---------
> > >Sender : 이범우 <[email protected]>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