Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   =?UTF-8?B?7J2067KU7Jqw?= <bw365.lee@samsung.com>
To:     "'Chanwoo Choi'" <cwchoi00@gmail.com>,
        "'MyungJoo Ham'" <myungjoo.ham@samsung.com>,
        "'Chanwoo Choi'" <cw00.choi@samsung.com>,
        <linux-kernel@vger.kernel.org>
In-Reply-To: <6f07e65b-339a-1ff7-1959-9ea4758d5244@gmail.com>
Subject: RE: [PATCH] extcon: modify extcon device to be created after driver
 data is set
Date:   Wed, 27 Apr 2022 11:12:26 +0900
Message-ID: <001e01d859dc$3d4d6870$b7e83950$@samsung.com>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Thread-Index: AQN4yMk10aQ8kdeD9OhaFXryx5/BvwITpb9EAcNqtAqpo24wsA==
Content-Language: ko
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <CGME20220331030336epcas1p3b8bf7b1841d4b246030b57b668312169@epcas1p3.samsung.com>
        <20220331030324.42621-1-bw365.lee@samsung.com>
        <6f07e65b-339a-1ff7-1959-9ea4758d5244@gmail.com>
Precedence: bulk

Hi chanwoo.

Thank you for your review.

Actually, I thought the same thing as you first.
However, it was a problem because dev->devres_lock was not initialized and =
devm_kcalloc was used, and this patch was created instead of using kcalloc =
instead of devm_kcalloc.

In your opinion, if I use kcalloc instead of devm_kcalloc, there will be no=
 problem.
But, I think the code of the following part should be supplemented.

1. alloc edev->nh when edev->max_supported is none zero.
=40=40 -1224,19 +1224,14 =40=40 int extcon_dev_register(struct extcon_dev *=
edev)
+       if (edev->max_supported) =7B
+               edev->nh =3D kcalloc(edev->max_supported, sizeof(*edev->nh)=
,
+                               GFP_KERNEL);
+               if (=21edev->nh) =7B
+                       ret =3D -ENOMEM;
+                       goto err_alloc_nh;
+               =7D
        =7D
2. free evdev->nh when device unregister.
=40=40 -1314,6 +1318,7 =40=40 void extcon_dev_unregister(struct extcon_dev =
*edev)
        if (edev->max_supported) =7B
                kfree(edev->extcon_dev_type.groups);
                kfree(edev->cables);
+               kfree(edev->nh);
        =7D

I will re-raise commit after testing.
Please let me know if you  have additional concern.

Best Regards,
Bumwoo Lee.
-----Original Message-----
From: Chanwoo Choi <cwchoi00=40gmail.com>=20
Sent: Wednesday, April 27, 2022 2:09 AM
To: bumwoo lee <bw365.lee=40samsung.com>; MyungJoo Ham <myungjoo.ham=40sams=
ung.com>; Chanwoo Choi <cw00.choi=40samsung.com>; linux-kernel=40vger.kerne=
l.org
Subject: Re: =5BPATCH=5D extcon: modify extcon device to be created after d=
river data is set

Hi Bumwoo,

Firstly, I'm sorry for late replay.

On 22. 3. 31. 12:03, bumwoo lee wrote:
> Currently, someone can invoke the sysfs such as state_show()=20
> intermittently before set_drvdata() is done.
> And it can be a cause of kernel Oops because of edev is Null at that time=
.
> So modified the driver registration to after setting drviver data.
>=20
> - Oops's backtrace.
>=20
> Backtrace:
> =5B<c067865c>=5D (state_show) from =5B<c05222e8>=5D (dev_attr_show)=20
> =5B<c05222c0>=5D (dev_attr_show) from =5B<c02c66e0>=5D (sysfs_kf_seq_show=
)=20
> =5B<c02c6648>=5D (sysfs_kf_seq_show) from =5B<c02c496c>=5D (kernfs_seq_sh=
ow)=20
> =5B<c02c4938>=5D (kernfs_seq_show) from =5B<c025e2a0>=5D (seq_read)=20
> =5B<c025e11c>=5D (seq_read) from =5B<c02c50a0>=5D (kernfs_fop_read)=20
> =5B<c02c5064>=5D (kernfs_fop_read) from =5B<c0231cac>=5D (__vfs_read)=20
> =5B<c0231c5c>=5D (__vfs_read) from =5B<c0231ee0>=5D (vfs_read) =5B<c0231e=
34>=5D=20
> (vfs_read) from =5B<c0232464>=5D (ksys_read) =5B<c02323f0>=5D (ksys_read)=
 from=20
> =5B<c02324fc>=5D (sys_read) =5B<c02324e4>=5D (sys_read) from =5B<c00091d0=
>=5D=20
> (__sys_trace_return)
>=20
> Signed-off-by: bumwoo lee <bw365.lee=40samsung.com>
> ---
>   drivers/extcon/extcon.c =7C 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>=20
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index=20
> d27cd9f88f86..74fee04bd764 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> =40=40 -1224,18 +1224,13 =40=40 int extcon_dev_register(struct extcon_dev=
 *edev)
>   		edev->dev.type =3D &edev->extcon_dev_type;
>   	=7D
>  =20
> -	ret =3D device_register(&edev->dev);
> -	if (ret) =7B
> -		put_device(&edev->dev);
> -		goto err_dev;
> -	=7D
> +	device_initialize(&edev->dev);
>  =20
>   	spin_lock_init(&edev->lock);
>   	edev->nh =3D devm_kcalloc(&edev->dev, edev->max_supported,
>   				sizeof(*edev->nh), GFP_KERNEL);

Actually, I'm not sure that it is proper to use 'edev->dev'
by devm_kcalloc before device_add. When error happen between device_initial=
ize and device_add, I think that it is not properly handled such as free th=
e allocated memory automatically.

>   	if (=21edev->nh) =7B
>   		ret =3D -ENOMEM;
> -		device_unregister(&edev->dev);
>   		goto err_dev;
>   	=7D
>  =20
> =40=40 -1251,9 +1246,14 =40=40 int extcon_dev_register(struct extcon_dev =
*edev)
>   	list_add(&edev->entry, &extcon_dev_list);
>   	mutex_unlock(&extcon_dev_list_lock);
>  =20
> +	ret =3D device_add(&edev->dev);
> +	if (ret)
> +		goto err_dev;

On this case, need to delete entry from extcon_dev_list by using list_del.

> +
>   	return 0;
>  =20
>   err_dev:
> +	put_device(&edev->dev);
>   	if (edev->max_supported)
>   		kfree(edev->extcon_dev_type.groups);
>   err_alloc_groups:


I recommend that move device_register() position after dev_set_drvdata.
And then use kcalloc instead of devm_kcalloc as following:


diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c index d27cd9=
f88f86..80e5bfec1131 100644
--- a/drivers/extcon/extcon.c
+++ b/drivers/extcon/extcon.c
=40=40 -1224,19 +1224,11 =40=40 int extcon_dev_register(struct extcon_dev *=
edev)
                 edev->dev.type =3D &edev->extcon_dev_type;
         =7D

-       ret =3D device_register(&edev->dev);
-       if (ret) =7B
-               put_device(&edev->dev);
-               goto err_dev;
-       =7D
-
         spin_lock_init(&edev->lock);
-       edev->nh =3D devm_kcalloc(&edev->dev, edev->max_supported,
-                               sizeof(*edev->nh), GFP_KERNEL);
+       edev->nh =3D kcalloc(edev->max_supported, sizeof(*edev->nh),
GFP_KERNEL);
         if (=21edev->nh) =7B
                 ret =3D -ENOMEM;
-               device_unregister(&edev->dev);
-               goto err_dev;
+               goto err_alloc_nh;
         =7D

         for (index =3D 0; index < edev->max_supported; index++) =40=40 -12=
47,6 +1239,12 =40=40 int extcon_dev_register(struct extcon_dev *edev)
         dev_set_drvdata(&edev->dev, edev);
         edev->state =3D 0;

+       ret =3D device_register(&edev->dev);
+       if (ret) =7B
+               put_device(&edev->dev);
+               goto err_dev;
+       =7D
+
         mutex_lock(&extcon_dev_list_lock);
         list_add(&edev->entry, &extcon_dev_list);
         mutex_unlock(&extcon_dev_list_lock);
=40=40 -1254,6 +1252,9 =40=40 int extcon_dev_register(struct extcon_dev *ed=
ev)
         return 0;

  err_dev:
+       if (edev->max_supported)
+               kfree(edev->nh);
+err_alloc_nh:
         if (edev->max_supported)
                 kfree(edev->extcon_dev_type.groups);
  err_alloc_groups:
=40=40 -1314,6 +1315,7 =40=40 void extcon_dev_unregister(struct extcon_dev =
*edev)
         if (edev->max_supported) =7B
                 kfree(edev->extcon_dev_type.groups);
                 kfree(edev->cables);
+               kfree(edev->nh);
         =7D

         put_device(&edev->dev);


--
Best Regards,
Samsung Electronics
Chanwoo Choi