Message-ID: <6bae12cb-f15e-e50f-7c77-016f94aa8fdd@sladewatkins.net>
Date:   Wed, 1 Mar 2023 22:44:06 -0500
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.8.0
Subject: Re: [PATCH v2 2/4] extcon: Added extcon_alloc_cables to simplify
 extcon register function
Content-Language: en-US, en-AU
To:     Bumwoo Lee <bw365.lee@samsung.com>
References: <20230220054513.27385-3-bw365.lee@samsung.com>
 <20230220054513.27385-1-bw365.lee@samsung.com>
 <CGME20230220054545epcas1p34e5575ffdfd1dcef55bdee3c3563c925@epcms1p3>
 <20230224100325epcms1p3e8886e278e23f610c8490cb69f1d452d@epcms1p3>
 <091101d94ca7$a4ad23c0$ee076b40$@samsung.com>
Cc:     'Chanwoo Choi' <cw00.choi@samsung.com>, myungjoo.ham@samsung.com,
        linux-kernel@vger.kernel.org, Slade Watkins <srw@sladewatkins.net>
From:   Slade's Kernel Patch Bot <KernelPatchBot@sladewatkins.net>
In-Reply-To: <091101d94ca7$a4ad23c0$ee076b40$@samsung.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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