diff mbox

[v2,14/16] rtc: utilize new cdev_device_add helper function

Message ID 1488091097-12328-15-git-send-email-logang@deltatee.com
State Superseded
Headers show

Commit Message

Logan Gunthorpe Feb. 26, 2017, 6:38 a.m. UTC
Mostly straightforward, but we had to remove the rtc_dev_add/del_device
functions as they split up the cdev_add and the device_add.

Doing this also revealed that there was likely another subtle bug:
seeing cdev_add was done after device_register, the cdev probably
was not ready before device_add when the uevent occurs. This would
race with userspace, if it tried to use the device directly after
the uevent. This is fixed just by using the new helper function.

Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/rtc/class.c   | 14 ++++++++++----
 drivers/rtc/rtc-dev.c | 17 -----------------
 2 files changed, 10 insertions(+), 21 deletions(-)

Comments

Alexandre Belloni Feb. 27, 2017, 9:46 a.m. UTC | #1
On 25/02/2017 at 23:38:15 -0700, Logan Gunthorpe wrote:
> Mostly straightforward, but we had to remove the rtc_dev_add/del_device
> functions as they split up the cdev_add and the device_add.
> 
> Doing this also revealed that there was likely another subtle bug:
> seeing cdev_add was done after device_register, the cdev probably
> was not ready before device_add when the uevent occurs. This would
> race with userspace, if it tried to use the device directly after
> the uevent. This is fixed just by using the new helper function.
> 
> Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
Acked-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>

> ---
>  drivers/rtc/class.c   | 14 ++++++++++----
>  drivers/rtc/rtc-dev.c | 17 -----------------
>  2 files changed, 10 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
> index 74fd974..5fb4398 100644
> --- a/drivers/rtc/class.c
> +++ b/drivers/rtc/class.c
> @@ -195,6 +195,8 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
>  		goto exit_ida;
>  	}
>  
> +	device_initialize(&rtc->dev);
> +
>  	rtc->id = id;
>  	rtc->ops = ops;
>  	rtc->owner = owner;
> @@ -233,14 +235,19 @@ struct rtc_device *rtc_device_register(const char *name, struct device *dev,
>  
>  	rtc_dev_prepare(rtc);
>  
> -	err = device_register(&rtc->dev);
> +	err = cdev_device_add(&rtc->char_dev, &rtc->dev);
>  	if (err) {
> +		dev_warn(&rtc->dev, "%s: failed to add char device %d:%d\n",
> +			 rtc->name, MAJOR(rtc->dev.devt), rtc->id);
> +
>  		/* This will free both memory and the ID */
>  		put_device(&rtc->dev);
>  		goto exit;
> +	} else {
> +		dev_dbg(&rtc->dev, "%s: dev (%d:%d)\n", rtc->name,
> +			MAJOR(rtc->dev.devt), rtc->id);
>  	}
>  
> -	rtc_dev_add_device(rtc);
>  	rtc_proc_add_device(rtc);
>  
>  	dev_info(dev, "rtc core: registered %s as %s\n",
> @@ -271,9 +278,8 @@ void rtc_device_unregister(struct rtc_device *rtc)
>  	 * Remove innards of this RTC, then disable it, before
>  	 * letting any rtc_class_open() users access it again
>  	 */
> -	rtc_dev_del_device(rtc);
>  	rtc_proc_del_device(rtc);
> -	device_del(&rtc->dev);
> +	cdev_device_del(&rtc->char_dev, &rtc->dev);
>  	rtc->ops = NULL;
>  	mutex_unlock(&rtc->ops_lock);
>  	put_device(&rtc->dev);
> diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
> index a6d9434..fdd071f 100644
> --- a/drivers/rtc/rtc-dev.c
> +++ b/drivers/rtc/rtc-dev.c
> @@ -477,23 +477,6 @@ void rtc_dev_prepare(struct rtc_device *rtc)
>  
>  	cdev_init(&rtc->char_dev, &rtc_dev_fops);
>  	rtc->char_dev.owner = rtc->owner;
> -	rtc->char_dev.kobj.parent = &rtc->dev.kobj;
> -}
> -
> -void rtc_dev_add_device(struct rtc_device *rtc)
> -{
> -	if (cdev_add(&rtc->char_dev, rtc->dev.devt, 1))
> -		dev_warn(&rtc->dev, "%s: failed to add char device %d:%d\n",
> -			rtc->name, MAJOR(rtc_devt), rtc->id);
> -	else
> -		dev_dbg(&rtc->dev, "%s: dev (%d:%d)\n", rtc->name,
> -			MAJOR(rtc_devt), rtc->id);
> -}
> -
> -void rtc_dev_del_device(struct rtc_device *rtc)
> -{
> -	if (rtc->dev.devt)
> -		cdev_del(&rtc->char_dev);
>  }
>  
>  void __init rtc_dev_init(void)
> -- 
> 2.1.4
>
diff mbox

Patch

diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c
index 74fd974..5fb4398 100644
--- a/drivers/rtc/class.c
+++ b/drivers/rtc/class.c
@@ -195,6 +195,8 @@  struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 		goto exit_ida;
 	}
 
+	device_initialize(&rtc->dev);
+
 	rtc->id = id;
 	rtc->ops = ops;
 	rtc->owner = owner;
@@ -233,14 +235,19 @@  struct rtc_device *rtc_device_register(const char *name, struct device *dev,
 
 	rtc_dev_prepare(rtc);
 
-	err = device_register(&rtc->dev);
+	err = cdev_device_add(&rtc->char_dev, &rtc->dev);
 	if (err) {
+		dev_warn(&rtc->dev, "%s: failed to add char device %d:%d\n",
+			 rtc->name, MAJOR(rtc->dev.devt), rtc->id);
+
 		/* This will free both memory and the ID */
 		put_device(&rtc->dev);
 		goto exit;
+	} else {
+		dev_dbg(&rtc->dev, "%s: dev (%d:%d)\n", rtc->name,
+			MAJOR(rtc->dev.devt), rtc->id);
 	}
 
-	rtc_dev_add_device(rtc);
 	rtc_proc_add_device(rtc);
 
 	dev_info(dev, "rtc core: registered %s as %s\n",
@@ -271,9 +278,8 @@  void rtc_device_unregister(struct rtc_device *rtc)
 	 * Remove innards of this RTC, then disable it, before
 	 * letting any rtc_class_open() users access it again
 	 */
-	rtc_dev_del_device(rtc);
 	rtc_proc_del_device(rtc);
-	device_del(&rtc->dev);
+	cdev_device_del(&rtc->char_dev, &rtc->dev);
 	rtc->ops = NULL;
 	mutex_unlock(&rtc->ops_lock);
 	put_device(&rtc->dev);
diff --git a/drivers/rtc/rtc-dev.c b/drivers/rtc/rtc-dev.c
index a6d9434..fdd071f 100644
--- a/drivers/rtc/rtc-dev.c
+++ b/drivers/rtc/rtc-dev.c
@@ -477,23 +477,6 @@  void rtc_dev_prepare(struct rtc_device *rtc)
 
 	cdev_init(&rtc->char_dev, &rtc_dev_fops);
 	rtc->char_dev.owner = rtc->owner;
-	rtc->char_dev.kobj.parent = &rtc->dev.kobj;
-}
-
-void rtc_dev_add_device(struct rtc_device *rtc)
-{
-	if (cdev_add(&rtc->char_dev, rtc->dev.devt, 1))
-		dev_warn(&rtc->dev, "%s: failed to add char device %d:%d\n",
-			rtc->name, MAJOR(rtc_devt), rtc->id);
-	else
-		dev_dbg(&rtc->dev, "%s: dev (%d:%d)\n", rtc->name,
-			MAJOR(rtc_devt), rtc->id);
-}
-
-void rtc_dev_del_device(struct rtc_device *rtc)
-{
-	if (rtc->dev.devt)
-		cdev_del(&rtc->char_dev);
 }
 
 void __init rtc_dev_init(void)