[v1,07/14] extcon: Use unique number for the extcon device ID
Commit Message
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
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;
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.
@@ -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) {
@@ -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;