* [PATCH] rtc: recycle id when unloading a rtc driver @ 2012-08-02 23:53 Vincent Palatin 2012-12-19 0:46 ` [rtc-linux] " Andrew Morton 0 siblings, 1 reply; 10+ messages in thread From: Vincent Palatin @ 2012-08-02 23:53 UTC (permalink / raw) To: rtc-linux; +Cc: Alessandro Zummo, linux-kernel, Vincent Palatin When calling rtc_device_unregister, we are not freeing the id used by the driver. So when doing a unload/load cycle for a RTC driver (e.g. rmmod rtc_cmos && modprobe rtc_cmos), its id is incremented by one. As a consequence, we no longer have neither an rtc0 driver nor a /proc/driver/rtc (as it only exists for the first driver). Signed-off-by: Vincent Palatin <vpalatin@chromium.org> --- drivers/rtc/class.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c index dc4c274..37b1d82 100644 --- a/drivers/rtc/class.c +++ b/drivers/rtc/class.c @@ -238,6 +238,7 @@ void rtc_device_unregister(struct rtc_device *rtc) rtc_proc_del_device(rtc); device_unregister(&rtc->dev); rtc->ops = NULL; + ida_simple_remove(&rtc_ida, rtc->id); mutex_unlock(&rtc->ops_lock); put_device(&rtc->dev); } -- 1.7.7.3 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-08-02 23:53 [PATCH] rtc: recycle id when unloading a rtc driver Vincent Palatin @ 2012-12-19 0:46 ` Andrew Morton 2012-12-19 7:37 ` Alexander Holler 0 siblings, 1 reply; 10+ messages in thread From: Andrew Morton @ 2012-12-19 0:46 UTC (permalink / raw) To: Vincent Palatin; +Cc: rtc-linux, Alessandro Zummo, linux-kernel On Thu, 2 Aug 2012 16:53:25 -0700 Vincent Palatin <vpalatin@chromium.org> wrote: > When calling rtc_device_unregister, we are not freeing the id used by the > driver. > So when doing a unload/load cycle for a RTC driver (e.g. rmmod rtc_cmos > && modprobe rtc_cmos), its id is incremented by one. As a consequence, > we no longer have neither an rtc0 driver nor a /proc/driver/rtc (as it > only exists for the first driver). > > Signed-off-by: Vincent Palatin <vpalatin@chromium.org> > --- > drivers/rtc/class.c | 1 + > 1 files changed, 1 insertions(+), 0 deletions(-) > > diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > index dc4c274..37b1d82 100644 > --- a/drivers/rtc/class.c > +++ b/drivers/rtc/class.c > @@ -238,6 +238,7 @@ void rtc_device_unregister(struct rtc_device *rtc) > rtc_proc_del_device(rtc); > device_unregister(&rtc->dev); > rtc->ops = NULL; > + ida_simple_remove(&rtc_ida, rtc->id); > mutex_unlock(&rtc->ops_lock); > put_device(&rtc->dev); > } Now I think about it, this shouldn't have been needed? That put_device() should call rtc_device_release(), which does the ida_simple_remove(). Isn't that working? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 0:46 ` [rtc-linux] " Andrew Morton @ 2012-12-19 7:37 ` Alexander Holler 2012-12-19 7:45 ` Andrew Morton 0 siblings, 1 reply; 10+ messages in thread From: Alexander Holler @ 2012-12-19 7:37 UTC (permalink / raw) To: Andrew Morton; +Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel Am 19.12.2012 01:46, schrieb Andrew Morton: > On Thu, 2 Aug 2012 16:53:25 -0700 > Vincent Palatin <vpalatin@chromium.org> wrote: > >> When calling rtc_device_unregister, we are not freeing the id used by the >> driver. >> So when doing a unload/load cycle for a RTC driver (e.g. rmmod rtc_cmos >> && modprobe rtc_cmos), its id is incremented by one. As a consequence, >> we no longer have neither an rtc0 driver nor a /proc/driver/rtc (as it >> only exists for the first driver). >> >> Signed-off-by: Vincent Palatin <vpalatin@chromium.org> >> --- >> drivers/rtc/class.c | 1 + >> 1 files changed, 1 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c >> index dc4c274..37b1d82 100644 >> --- a/drivers/rtc/class.c >> +++ b/drivers/rtc/class.c >> @@ -238,6 +238,7 @@ void rtc_device_unregister(struct rtc_device *rtc) >> rtc_proc_del_device(rtc); >> device_unregister(&rtc->dev); >> rtc->ops = NULL; >> + ida_simple_remove(&rtc_ida, rtc->id); >> mutex_unlock(&rtc->ops_lock); >> put_device(&rtc->dev); >> } > > Now I think about it, this shouldn't have been needed? > > That put_device() should call rtc_device_release(), which does the > ida_simple_remove(). Isn't that working? It is, see the mini-thread, patch and my comment here: https://lkml.org/lkml/2012/12/6/152 Maybe it would be better to move the ida_simple_remove from the rtc_device_release to rt_device_unregister as I've hinted in the above comment. That would make it easier to spot the ida_simple_remove(). Regards, Alexander ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 7:37 ` Alexander Holler @ 2012-12-19 7:45 ` Andrew Morton 2012-12-19 7:55 ` Alexander Holler 0 siblings, 1 reply; 10+ messages in thread From: Andrew Morton @ 2012-12-19 7:45 UTC (permalink / raw) To: Alexander Holler Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel On Wed, 19 Dec 2012 08:37:07 +0100 Alexander Holler <holler@ahsoftware.de> wrote: > Am 19.12.2012 01:46, schrieb Andrew Morton: > > On Thu, 2 Aug 2012 16:53:25 -0700 > > Vincent Palatin <vpalatin@chromium.org> wrote: > > > >> When calling rtc_device_unregister, we are not freeing the id used by the > >> driver. > >> So when doing a unload/load cycle for a RTC driver (e.g. rmmod rtc_cmos > >> && modprobe rtc_cmos), its id is incremented by one. As a consequence, > >> we no longer have neither an rtc0 driver nor a /proc/driver/rtc (as it > >> only exists for the first driver). > >> > >> Signed-off-by: Vincent Palatin <vpalatin@chromium.org> > >> --- > >> drivers/rtc/class.c | 1 + > >> 1 files changed, 1 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c > >> index dc4c274..37b1d82 100644 > >> --- a/drivers/rtc/class.c > >> +++ b/drivers/rtc/class.c > >> @@ -238,6 +238,7 @@ void rtc_device_unregister(struct rtc_device *rtc) > >> rtc_proc_del_device(rtc); > >> device_unregister(&rtc->dev); > >> rtc->ops = NULL; > >> + ida_simple_remove(&rtc_ida, rtc->id); > >> mutex_unlock(&rtc->ops_lock); > >> put_device(&rtc->dev); > >> } > > > > Now I think about it, this shouldn't have been needed? > > > > That put_device() should call rtc_device_release(), which does the > > ida_simple_remove(). Isn't that working? > > It is, see the mini-thread, patch and my comment here: > > https://lkml.org/lkml/2012/12/6/152 > > Maybe it would be better to move the ida_simple_remove from the > rtc_device_release to rt_device_unregister as I've hinted in the above > comment. That would make it easier to spot the ida_simple_remove(). I'm all confused. Lothar's patch simply reverts Vincent's patch. And that appears to be the correct thing to so, as the ida_simple_remove() in rtc_device_release() should be sufficient. But apparently that doesn't work, because Vincent was seeing the RTC ID's increment rather than getting reused. Is it the case that rtc_device_release() is not being called sometimes? If so, under what circumstances? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 7:45 ` Andrew Morton @ 2012-12-19 7:55 ` Alexander Holler 2012-12-19 8:27 ` Andrew Morton 0 siblings, 1 reply; 10+ messages in thread From: Alexander Holler @ 2012-12-19 7:55 UTC (permalink / raw) To: Andrew Morton Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel, Lothar Waßmann Am 19.12.2012 08:45, schrieb Andrew Morton: > On Wed, 19 Dec 2012 08:37:07 +0100 Alexander Holler <holler@ahsoftware.de> wrote: > >> Am 19.12.2012 01:46, schrieb Andrew Morton: >>> On Thu, 2 Aug 2012 16:53:25 -0700 >>> Vincent Palatin <vpalatin@chromium.org> wrote: >>> >>>> When calling rtc_device_unregister, we are not freeing the id used by the >>>> driver. >>>> So when doing a unload/load cycle for a RTC driver (e.g. rmmod rtc_cmos >>>> && modprobe rtc_cmos), its id is incremented by one. As a consequence, >>>> we no longer have neither an rtc0 driver nor a /proc/driver/rtc (as it >>>> only exists for the first driver). >>>> >>>> Signed-off-by: Vincent Palatin <vpalatin@chromium.org> >>>> --- >>>> drivers/rtc/class.c | 1 + >>>> 1 files changed, 1 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/rtc/class.c b/drivers/rtc/class.c >>>> index dc4c274..37b1d82 100644 >>>> --- a/drivers/rtc/class.c >>>> +++ b/drivers/rtc/class.c >>>> @@ -238,6 +238,7 @@ void rtc_device_unregister(struct rtc_device *rtc) >>>> rtc_proc_del_device(rtc); >>>> device_unregister(&rtc->dev); >>>> rtc->ops = NULL; >>>> + ida_simple_remove(&rtc_ida, rtc->id); >>>> mutex_unlock(&rtc->ops_lock); >>>> put_device(&rtc->dev); >>>> } >>> >>> Now I think about it, this shouldn't have been needed? >>> >>> That put_device() should call rtc_device_release(), which does the >>> ida_simple_remove(). Isn't that working? >> >> It is, see the mini-thread, patch and my comment here: >> >> https://lkml.org/lkml/2012/12/6/152 >> >> Maybe it would be better to move the ida_simple_remove from the >> rtc_device_release to rt_device_unregister as I've hinted in the above >> comment. That would make it easier to spot the ida_simple_remove(). > > I'm all confused. > > Lothar's patch simply reverts Vincent's patch. And that appears to be > the correct thing to so, as the ida_simple_remove() in > rtc_device_release() should be sufficient. > > But apparently that doesn't work, because Vincent was seeing the RTC > ID's increment rather than getting reused. > > Is it the case that rtc_device_release() is not being called sometimes? > If so, under what circumstances? Maybe something (sysfs or whatever) still has a reference to it. Vincent should check that. But I'm sure the ID will be recycled with that put_device() in unregister because I've got the same warning as Lothar did when (porperly) removing an RTC (with kernel 3.7). Regards, Alexander ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 7:55 ` Alexander Holler @ 2012-12-19 8:27 ` Andrew Morton 2012-12-19 8:55 ` Alexander Holler 0 siblings, 1 reply; 10+ messages in thread From: Andrew Morton @ 2012-12-19 8:27 UTC (permalink / raw) To: Alexander Holler Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel, Lothar Waßmann On Wed, 19 Dec 2012 08:55:57 +0100 Alexander Holler <holler@ahsoftware.de> wrote: > > > > I'm all confused. > > > > Lothar's patch simply reverts Vincent's patch. And that appears to be > > the correct thing to so, as the ida_simple_remove() in > > rtc_device_release() should be sufficient. > > > > But apparently that doesn't work, because Vincent was seeing the RTC > > ID's increment rather than getting reused. > > > > Is it the case that rtc_device_release() is not being called sometimes? > > If so, under what circumstances? > > Maybe something (sysfs or whatever) still has a reference to it. Vincent > should check that. > > But I'm sure the ID will be recycled with that put_device() in > unregister because I've got the same warning as Lothar did when > (porperly) removing an RTC (with kernel 3.7). If, as appears to be the case, rtc_device_release() is not being called then we're also leaking memory. So yes please, it would be good if someone who can reproduce the IDs-dont-decrease problem could dive in and work out why ->release() isn't begin called. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 8:27 ` Andrew Morton @ 2012-12-19 8:55 ` Alexander Holler 2012-12-19 22:37 ` Andrew Morton 2012-12-27 12:42 ` Alexander Holler 0 siblings, 2 replies; 10+ messages in thread From: Alexander Holler @ 2012-12-19 8:55 UTC (permalink / raw) To: Andrew Morton Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel, Lothar Waßmann Am 19.12.2012 09:27, schrieb Andrew Morton: > On Wed, 19 Dec 2012 08:55:57 +0100 Alexander Holler <holler@ahsoftware.de> wrote: > >>> >>> I'm all confused. >>> >>> Lothar's patch simply reverts Vincent's patch. And that appears to be >>> the correct thing to so, as the ida_simple_remove() in >>> rtc_device_release() should be sufficient. >>> >>> But apparently that doesn't work, because Vincent was seeing the RTC >>> ID's increment rather than getting reused. >>> >>> Is it the case that rtc_device_release() is not being called sometimes? >>> If so, under what circumstances? >> >> Maybe something (sysfs or whatever) still has a reference to it. Vincent >> should check that. >> >> But I'm sure the ID will be recycled with that put_device() in >> unregister because I've got the same warning as Lothar did when >> (porperly) removing an RTC (with kernel 3.7). > > If, as appears to be the case, rtc_device_release() is not being called > then we're also leaking memory. So yes please, it would be good if > someone who can reproduce the IDs-dont-decrease problem could dive in > and work out why ->release() isn't begin called. Unlikely, as I've worked hard to get one of the first drivers for pluggable RTCs into the kernel. ;) I think every sane kernel has them statically linked in and it's likely a problem of the RTC-driver Vincent experienced that with. Regards, Alexander ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 8:55 ` Alexander Holler @ 2012-12-19 22:37 ` Andrew Morton 2012-12-19 22:58 ` Alexander Holler 2012-12-27 12:42 ` Alexander Holler 1 sibling, 1 reply; 10+ messages in thread From: Andrew Morton @ 2012-12-19 22:37 UTC (permalink / raw) To: Alexander Holler Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel, Lothar Waßmann On Wed, 19 Dec 2012 09:55:02 +0100 Alexander Holler <holler@ahsoftware.de> wrote: > Am 19.12.2012 09:27, schrieb Andrew Morton: > > On Wed, 19 Dec 2012 08:55:57 +0100 Alexander Holler <holler@ahsoftware.de> wrote: > > > >>> > >>> I'm all confused. > >>> > >>> Lothar's patch simply reverts Vincent's patch. And that appears to be > >>> the correct thing to so, as the ida_simple_remove() in > >>> rtc_device_release() should be sufficient. > >>> > >>> But apparently that doesn't work, because Vincent was seeing the RTC > >>> ID's increment rather than getting reused. > >>> > >>> Is it the case that rtc_device_release() is not being called sometimes? > >>> If so, under what circumstances? > >> > >> Maybe something (sysfs or whatever) still has a reference to it. Vincent > >> should check that. > >> > >> But I'm sure the ID will be recycled with that put_device() in > >> unregister because I've got the same warning as Lothar did when > >> (porperly) removing an RTC (with kernel 3.7). > > > > If, as appears to be the case, rtc_device_release() is not being called > > then we're also leaking memory. So yes please, it would be good if > > someone who can reproduce the IDs-dont-decrease problem could dive in > > and work out why ->release() isn't begin called. > > Unlikely, as I've worked hard to get one of the first drivers for > pluggable RTCs into the kernel. ;) > I think every sane kernel has them statically linked in and it's likely > a problem of the RTC-driver Vincent experienced that with. > I think I'll do this: From: Andrew Morton <akpm@linux-foundation.org> Subject: revert "rtc: recycle id when unloading a rtc driver" Revert commit 2830a6d20139df2198d63235df7957712adb28e5 Author: Vincent Palatin <vpalatin@chromium.org> Date: Thu Oct 4 17:13:52 2012 -0700 rtc: recycle id when unloading a rtc driver We already perform the ida_simple_remove() in rtc_device_release(), which is an appropriate place. 2830a6d20 ("rtc: recycle id when unloading a rtc driver") caused the kernel to emit ida_remove called for id=0 which is not allocated. warnings when rtc_device_release() tries to release an alread-released ID. Let's restore things to their previous state and then work out why Vincent's kernel wasn't calling rtc_device_release() - presumably a bug in a specific sub-driver. Reported-by: Lothar Wa_mann <LW@KARO-electronics.de> Acked-by: Alexander Holler <holler@ahsoftware.de> Cc: Vincent Palatin <vpalatin@chromium.org> Cc: <stable@vger.kernel.org> [3.7.x] Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- drivers/rtc/class.c | 1 - 1 file changed, 1 deletion(-) diff -puN drivers/rtc/class.c~revert-rtc-recycle-id-when-unloading-a-rtc-driver drivers/rtc/class.c --- a/drivers/rtc/class.c~revert-rtc-recycle-id-when-unloading-a-rtc-driver +++ a/drivers/rtc/class.c @@ -244,7 +244,6 @@ void rtc_device_unregister(struct rtc_de rtc_proc_del_device(rtc); device_unregister(&rtc->dev); rtc->ops = NULL; - ida_simple_remove(&rtc_ida, rtc->id); mutex_unlock(&rtc->ops_lock); put_device(&rtc->dev); } _ ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 22:37 ` Andrew Morton @ 2012-12-19 22:58 ` Alexander Holler 0 siblings, 0 replies; 10+ messages in thread From: Alexander Holler @ 2012-12-19 22:58 UTC (permalink / raw) To: Andrew Morton Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel, Lothar Waßmann Am 19.12.2012 23:37, schrieb Andrew Morton: > I think I'll do this: > > > From: Andrew Morton <akpm@linux-foundation.org> > Subject: revert "rtc: recycle id when unloading a rtc driver" Thanks a lot, I just haven't seen the stuff got broken with 3.7 as I haven't played much with RTCs before (just used them). So I didn't looked at the git history and fixed it myself just to find out Lothar already had send a patch. ;) And for the symmetrie between register/unregister (moving ida_simple_remove() from release() to unregister()) I'm not exactly sure if that wouldn't break something. Also it looks like anything which still would use the ID will fail after unregister (because unregister cleans up a lot), I'm not quite sure. So just reverting it looks like a save bet. Regards, Alexander ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [rtc-linux] [PATCH] rtc: recycle id when unloading a rtc driver 2012-12-19 8:55 ` Alexander Holler 2012-12-19 22:37 ` Andrew Morton @ 2012-12-27 12:42 ` Alexander Holler 1 sibling, 0 replies; 10+ messages in thread From: Alexander Holler @ 2012-12-27 12:42 UTC (permalink / raw) To: Andrew Morton Cc: Vincent Palatin, rtc-linux, Alessandro Zummo, linux-kernel, Lothar Waßmann Am 19.12.2012 09:55, schrieb Alexander Holler: > Unlikely, as I've worked hard to get one of the first drivers for > pluggable RTCs into the kernel. ;) BTW. maybe you could have a look at the patch for that: https://lkml.org/lkml/2012/12/15/64 It should already be in your mailbox. I had to move the driver (on maintainer request) from the iio subsystem to the rtc subsystem and even if Jonathon Cameron would feed the smal patch-series through iio into the kernel, an OK from one of the RTC maintainers (or you) is needed. As the v5 of that patch suggests, it was already reviewed (by Lars-Peter Clausen) and should be in a good state. Thanks, Alexander ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2012-12-27 12:43 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-08-02 23:53 [PATCH] rtc: recycle id when unloading a rtc driver Vincent Palatin 2012-12-19 0:46 ` [rtc-linux] " Andrew Morton 2012-12-19 7:37 ` Alexander Holler 2012-12-19 7:45 ` Andrew Morton 2012-12-19 7:55 ` Alexander Holler 2012-12-19 8:27 ` Andrew Morton 2012-12-19 8:55 ` Alexander Holler 2012-12-19 22:37 ` Andrew Morton 2012-12-19 22:58 ` Alexander Holler 2012-12-27 12:42 ` Alexander Holler
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.