* [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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).