Messages in this thread | | | Date | Wed, 18 Jul 2012 16:23:43 -0700 | From | John Stultz <> | Subject | Re: [PATCH] alarmtimer: add .remove_dev hook to put device |
| |
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
| |