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@xxxxxxxxxxxxxxxx>.

Thank you,
-- Slade's kernel patch bot

> 
> -----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.
> 
> 
>