diff mbox series

[v1,07/14] extcon: Use unique number for the extcon device ID

Message ID 20230322144005.40368-8-andriy.shevchenko@linux.intel.com
State New
Headers
Series extcon: Core cleanups and documentation fixes |

Commit Message

Andy Shevchenko March 22, 2023, 2:39 p.m. UTC 
  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@linux.intel.com>
---
 drivers/extcon/extcon.c | 15 ++++++++++++---
 drivers/extcon/extcon.h |  2 ++
 2 files changed, 14 insertions(+), 3 deletions(-)

Comments

Chanwoo Choi April 3, 2023, 2:52 p.m. UTC | #1 
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@linux.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;
Andy Shevchenko April 5, 2023, 3:03 p.m. UTC | #2 
On Mon, Apr 03, 2023 at 11:52:46PM +0900, Chanwoo Choi wrote:
> On 23. 3. 22. 23:39, Andy Shevchenko wrote:

...

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

Done for v2.

...

> > +	ida_simple_remove(&extcon_dev_ids, edev->id);
> 
> ditto.

Ditto.
diff mbox series

Patch

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);
+	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);
+
 	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;