[PATCH] alarmtimer: add .remove_dev hook to put device

[PATCH] alarmtimer: add .remove_dev hook to put device

[Shawn Guo]

The following is a test sequence that installs a rtc module, remove it
and installs it again.

$ insmod rtc-snvs.ko
snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc0
$ hwclock
Thu Jul  5 08:53:35 2012  0.000000 seconds
$ rmmod rtc-snvs.ko
$ insmod rtc-snvs.ko
snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc1
$ hwclock
hwclock: can't open '/dev/misc/rtc': No such file or directory
$

The device is registered as rtc0 for the first time insmod, while it
becomes rtc1 with the later insmod.

It's root caused by alarmtimer which never puts the device even when
the rtc is removed.  The patch adds a .remove_dev hook to have device
properly put, so that above insmod/rmmod sequence can the rtc device
registered in a consistent behavior.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 kernel/time/alarmtimer.c |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
index aa27d39..39e8773 100644
--- a/kernel/time/alarmtimer.c
+++ b/kernel/time/alarmtimer.c
@@ -96,6 +96,17 @@ static int alarmtimer_rtc_add_device(struct device *dev,
 	return 0;
 }
 
+static void alarmtimer_rtc_remove_device(struct device *dev,
+				struct class_interface *class_intf)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&rtcdev_lock, flags);
+	rtcdev = NULL;
+	put_device(dev);
+	spin_unlock_irqrestore(&rtcdev_lock, flags);
+}
+
 static inline void alarmtimer_rtc_timer_init(void)
 {
 	rtc_timer_init(&rtctimer, NULL, NULL);
@@ -103,6 +114,7 @@ static inline void alarmtimer_rtc_timer_init(void)
 
 static struct class_interface alarmtimer_rtc_interface = {
 	.add_dev = &alarmtimer_rtc_add_device,
+	.remove_dev = &alarmtimer_rtc_remove_device,
 };
 
 static int alarmtimer_rtc_interface_setup(void)
-- 
1.7.5.4

Re: [PATCH] alarmtimer: add .remove_dev hook to put device

[Shawn Guo]

Hi John,

Is this patch a valid fix or just a noise?

Regards,
Shawn

On Thu, Jul 05, 2012 at 12:13:07AM +0800, Shawn Guo wrote:
> The following is a test sequence that installs a rtc module, remove it
> and installs it again.
> 
> $ insmod rtc-snvs.ko
> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc0
> $ hwclock
> Thu Jul  5 08:53:35 2012  0.000000 seconds
> $ rmmod rtc-snvs.ko
> $ insmod rtc-snvs.ko
> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc1
> $ hwclock
> hwclock: can't open '/dev/misc/rtc': No such file or directory
> $
> 
> The device is registered as rtc0 for the first time insmod, while it
> becomes rtc1 with the later insmod.
> 
> It's root caused by alarmtimer which never puts the device even when
> the rtc is removed.  The patch adds a .remove_dev hook to have device
> properly put, so that above insmod/rmmod sequence can the rtc device
> registered in a consistent behavior.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  kernel/time/alarmtimer.c |   12 ++++++++++++
>  1 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index aa27d39..39e8773 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -96,6 +96,17 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>  	return 0;
>  }
>  
> +static void alarmtimer_rtc_remove_device(struct device *dev,
> +				struct class_interface *class_intf)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rtcdev_lock, flags);
> +	rtcdev = NULL;
> +	put_device(dev);
> +	spin_unlock_irqrestore(&rtcdev_lock, flags);
> +}
> +
>  static inline void alarmtimer_rtc_timer_init(void)
>  {
>  	rtc_timer_init(&rtctimer, NULL, NULL);
> @@ -103,6 +114,7 @@ static inline void alarmtimer_rtc_timer_init(void)
>  
>  static struct class_interface alarmtimer_rtc_interface = {
>  	.add_dev = &alarmtimer_rtc_add_device,
> +	.remove_dev = &alarmtimer_rtc_remove_device,
>  };
>  
>  static int alarmtimer_rtc_interface_setup(void)
> -- 
> 1.7.5.4
> 
> 

Re: [PATCH] alarmtimer: add .remove_dev hook to put device

[John Stultz]

On 07/10/2012 08:46 PM, Shawn Guo wrote:
> Hi John,
>
> Is this patch a valid fix or just a noise?

Sorry! This is on my list, but I've not had a chance to get to it.

I'll try to get a closer look later this week!

Thanks for reminding me, and sorry again!
-john


> Regards,
> Shawn
>
> On Thu, Jul 05, 2012 at 12:13:07AM +0800, Shawn Guo wrote:
>> The following is a test sequence that installs a rtc module, remove it
>> and installs it again.
>>
>> $ insmod rtc-snvs.ko
>> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc0
>> $ hwclock
>> Thu Jul  5 08:53:35 2012  0.000000 seconds
>> $ rmmod rtc-snvs.ko
>> $ insmod rtc-snvs.ko
>> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc1
>> $ hwclock
>> hwclock: can't open '/dev/misc/rtc': No such file or directory
>> $
>>
>> The device is registered as rtc0 for the first time insmod, while it
>> becomes rtc1 with the later insmod.
>>
>> It's root caused by alarmtimer which never puts the device even when
>> the rtc is removed.  The patch adds a .remove_dev hook to have device
>> properly put, so that above insmod/rmmod sequence can the rtc device
>> registered in a consistent behavior.
>>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> ---
>>   kernel/time/alarmtimer.c |   12 ++++++++++++
>>   1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
>> index aa27d39..39e8773 100644
>> --- a/kernel/time/alarmtimer.c
>> +++ b/kernel/time/alarmtimer.c
>> @@ -96,6 +96,17 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>>   	return 0;
>>   }
>>   
>> +static void alarmtimer_rtc_remove_device(struct device *dev,
>> +				struct class_interface *class_intf)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&rtcdev_lock, flags);
>> +	rtcdev = NULL;
>> +	put_device(dev);
>> +	spin_unlock_irqrestore(&rtcdev_lock, flags);
>> +}
>> +
>>   static inline void alarmtimer_rtc_timer_init(void)
>>   {
>>   	rtc_timer_init(&rtctimer, NULL, NULL);
>> @@ -103,6 +114,7 @@ static inline void alarmtimer_rtc_timer_init(void)
>>   
>>   static struct class_interface alarmtimer_rtc_interface = {
>>   	.add_dev = &alarmtimer_rtc_add_device,
>> +	.remove_dev = &alarmtimer_rtc_remove_device,
>>   };
>>   
>>   static int alarmtimer_rtc_interface_setup(void)
>> -- 
>> 1.7.5.4
>>
>>

Re: [PATCH] alarmtimer: add .remove_dev hook to put device

[John Stultz]

On 07/04/2012 09:13 AM, Shawn Guo wrote:
> The following is a test sequence that installs a rtc module, remove it
> and installs it again.
>
> $ insmod rtc-snvs.ko
> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc0
> $ hwclock
> Thu Jul  5 08:53:35 2012  0.000000 seconds
> $ rmmod rtc-snvs.ko
> $ insmod rtc-snvs.ko
> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 20cc034.snvs-rtc-lp as rtc1
> $ hwclock
> hwclock: can't open '/dev/misc/rtc': No such file or directory
> $
>
> The device is registered as rtc0 for the first time insmod, while it
> becomes rtc1 with the later insmod.
>
> It's root caused by alarmtimer which never puts the device even when
> the rtc is removed.  The patch adds a .remove_dev hook to have device
> properly put, so that above insmod/rmmod sequence can the rtc device
> registered in a consistent behavior.
>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>   kernel/time/alarmtimer.c |   12 ++++++++++++
>   1 files changed, 12 insertions(+), 0 deletions(-)
>
> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
> index aa27d39..39e8773 100644
> --- a/kernel/time/alarmtimer.c
> +++ b/kernel/time/alarmtimer.c
> @@ -96,6 +96,17 @@ static int alarmtimer_rtc_add_device(struct device *dev,
>   	return 0;
>   }
>
> +static void alarmtimer_rtc_remove_device(struct device *dev,
> +				struct class_interface *class_intf)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&rtcdev_lock, flags);
> +	rtcdev = NULL;
> +	put_device(dev);
> +	spin_unlock_irqrestore(&rtcdev_lock, flags);
> +}
> +

Looking at this a little closer, you need to validate that the device 
being removed is the actual rtcdev before setting it to null.

Otherwise if there's two rtc drivers, and the 1st is the current rtcdev, 
you'll clear rtcdev if the second rtc driver is removed.

I'll go ahead and fix this up for you.

thanks
-john

Re: [PATCH] alarmtimer: add .remove_dev hook to put device

[John Stultz]

On 07/18/2012 04:16 PM, John Stultz wrote:
> On 07/04/2012 09:13 AM, Shawn Guo wrote:
>> The following is a test sequence that installs a rtc module, remove it
>> and installs it again.
>>
>> $ insmod rtc-snvs.ko
>> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 
>> 20cc034.snvs-rtc-lp as rtc0
>> $ hwclock
>> Thu Jul  5 08:53:35 2012  0.000000 seconds
>> $ rmmod rtc-snvs.ko
>> $ insmod rtc-snvs.ko
>> snvs_rtc 20cc034.snvs-rtc-lp: rtc core: registered 
>> 20cc034.snvs-rtc-lp as rtc1
>> $ hwclock
>> hwclock: can't open '/dev/misc/rtc': No such file or directory
>> $
>>
>> The device is registered as rtc0 for the first time insmod, while it
>> becomes rtc1 with the later insmod.
>>
>> It's root caused by alarmtimer which never puts the device even when
>> the rtc is removed.  The patch adds a .remove_dev hook to have device
>> properly put, so that above insmod/rmmod sequence can the rtc device
>> registered in a consistent behavior.
>>
>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
>> ---
>>   kernel/time/alarmtimer.c |   12 ++++++++++++
>>   1 files changed, 12 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c
>> index aa27d39..39e8773 100644
>> --- a/kernel/time/alarmtimer.c
>> +++ b/kernel/time/alarmtimer.c
>> @@ -96,6 +96,17 @@ static int alarmtimer_rtc_add_device(struct device 
>> *dev,
>>       return 0;
>>   }
>>
>> +static void alarmtimer_rtc_remove_device(struct device *dev,
>> +                struct class_interface *class_intf)
>> +{
>> +    unsigned long flags;
>> +
>> +    spin_lock_irqsave(&rtcdev_lock, flags);
>> +    rtcdev = NULL;
>> +    put_device(dev);
>> +    spin_unlock_irqrestore(&rtcdev_lock, flags);
>> +}
>> +
>
> Looking at this a little closer, you need to validate that the device 
> being removed is the actual rtcdev before setting it to null.
>
> Otherwise if there's two rtc drivers, and the 1st is the current 
> rtcdev, you'll clear rtcdev if the second rtc driver is removed.
>
> I'll go ahead and fix this up for you.

Actually, this change opens up a bunch of other races, as any caller of 
alarmtimer_get_rtcdev() could have the rtcdevice removed under it.

We'll need to have proper reference counting w/ get/put calls, probably 
also adding a alarmtimer_put_rtcdev() interface.

So for now I'm dropping this from my tree.  Do you think you might be 
able to take another stab at this?

Sorry again for the delayed review here.

thanks
-john

Re: [PATCH] alarmtimer: add .remove_dev hook to put device

[Shawn Guo]

On 19 July 2012 07:23, John Stultz <johnstul@us.ibm.com> wrote:
> Actually, this change opens up a bunch of other races, as any caller of
> alarmtimer_get_rtcdev() could have the rtcdevice removed under it.
>
> We'll need to have proper reference counting w/ get/put calls, probably also
> adding a alarmtimer_put_rtcdev() interface.
>
> So for now I'm dropping this from my tree.  Do you think you might be able
> to take another stab at this?
>
No.  You can take the patch as a bug report and fix it yourself in the
best way you can think of :)

Regards,
Shawn

Re: [PATCH] alarmtimer: add .remove_dev hook to put device

[John Stultz]

On 07/18/2012 06:23 PM, Shawn Guo wrote:
> On 19 July 2012 07:23, John Stultz <johnstul@us.ibm.com> wrote:
>> Actually, this change opens up a bunch of other races, as any caller of
>> alarmtimer_get_rtcdev() could have the rtcdevice removed under it.
>>
>> We'll need to have proper reference counting w/ get/put calls, probably also
>> adding a alarmtimer_put_rtcdev() interface.
>>
>> So for now I'm dropping this from my tree.  Do you think you might be able
>> to take another stab at this?
>>
> No.  You can take the patch as a bug report and fix it yourself in the
> best way you can think of :)
Ok. I'll try to find some time to do that.

Thanks!
-john