[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f0a40f3a-2ff6-5529-ad84-1c66f7e381ea@gmail.com>
Date: Mon, 3 Apr 2023 23:52:46 +0900
From: Chanwoo Choi <cwchoi00@...il.com>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
Bumwoo Lee <bw365.lee@...sung.com>,
linux-kernel@...r.kernel.org
Cc: MyungJoo Ham <myungjoo.ham@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>
Subject: Re: [PATCH v1 07/14] extcon: Use unique number for the extcon device
ID
On 23. 3. 22. 23:39, Andy Shevchenko wrote:
> The use of atomic variable is still racy when we do not control which
> device has been unregistered and there is a (theoretical) possibility
> of the overflow that may cause a duplicate extcon device ID number
> to be allocated next time a device is registered.
>
> Replace above mentioned approach by using IDA framework.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
> drivers/extcon/extcon.c | 15 ++++++++++++---
> drivers/extcon/extcon.h | 2 ++
> 2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c
> index 14e66e21597f..0d261ec6c473 100644
> --- a/drivers/extcon/extcon.c
> +++ b/drivers/extcon/extcon.c
> @@ -16,6 +16,7 @@
>
> #include <linux/module.h>
> #include <linux/types.h>
> +#include <linux/idr.h>
> #include <linux/init.h>
> #include <linux/device.h>
> #include <linux/fs.h>
> @@ -238,6 +239,7 @@ struct extcon_cable {
>
> static struct class *extcon_class;
>
> +static DEFINE_IDA(extcon_dev_ids);
> static LIST_HEAD(extcon_dev_list);
> static DEFINE_MUTEX(extcon_dev_list_lock);
>
> @@ -1248,7 +1250,6 @@ static int extcon_alloc_groups(struct extcon_dev *edev)
> int extcon_dev_register(struct extcon_dev *edev)
> {
> int ret, index = 0;
> - static atomic_t edev_no = ATOMIC_INIT(-1);
>
> ret = create_extcon_class();
> if (ret < 0)
> @@ -1275,8 +1276,14 @@ int extcon_dev_register(struct extcon_dev *edev)
> dev_err(&edev->dev, "extcon device name is null\n");
> return -EINVAL;
> }
> - dev_set_name(&edev->dev, "extcon%lu",
> - (unsigned long)atomic_inc_return(&edev_no));
> +
> + ret = ida_simple_get(&extcon_dev_ids, 0, INT_MAX, GFP_KERNEL);
ida_simple_get and ida_simple_remove are deprecated on
commit 3264ceec8f17 ("lib/idr.c: document that ida_simple_{get,remove}()
are deprecated"). Instead of them, better to use ida_alloc and ida_free
according to the comments.
> + if (ret < 0)
> + return ret;
> +
> + edev->id = ret;
> +
> + dev_set_name(&edev->dev, "extcon%d", edev->id);
>
> ret = extcon_alloc_cables(edev);
> if (ret < 0)
> @@ -1368,6 +1375,8 @@ void extcon_dev_unregister(struct extcon_dev *edev)
> return;
> }
>
> + ida_simple_remove(&extcon_dev_ids, edev->id);
ditto.
> +
> device_unregister(&edev->dev);
>
> if (edev->mutually_exclusive && edev->max_supported) {
> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h
> index 15616446140d..877c0860e300 100644
> --- a/drivers/extcon/extcon.h
> +++ b/drivers/extcon/extcon.h
> @@ -20,6 +20,7 @@
> * {0x3, 0x6, 0x5, 0}. If it is {0xFFFFFFFF, 0}, there
> * can be no simultaneous connections.
> * @dev: Device of this extcon.
> + * @id: Unique device ID of this extcon.
> * @state: Attach/detach state of this extcon. Do not provide at
> * register-time.
> * @nh_all: Notifier for the state change events for all supported
> @@ -46,6 +47,7 @@ struct extcon_dev {
>
> /* Internal data. Please do not set. */
> struct device dev;
> + int id;
> struct raw_notifier_head nh_all;
> struct raw_notifier_head *nh;
> struct list_head entry;
--
Best Regards,
Samsung Electronics
Chanwoo Choi
Powered by blists - more mailing lists