| Message ID | 1343951605-25722-1-git-send-email-vpalatin@chromium.org |
|---|---|
| State | Superseded |
| Headers | show |
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?
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
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?
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
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.
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
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
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); }
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(-)