Patchwork rtc: recycle id when unloading a rtc driver

login
register
mail settings
Submitter Vincent Palatin
Date Aug. 2, 2012, 11:53 p.m.
Message ID <1343951605-25722-1-git-send-email-vpalatin@chromium.org>
Download mbox | patch
Permalink /patch/174819/
State New
Headers show

Comments

Vincent Palatin - Aug. 2, 2012, 11:53 p.m.
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(-)
Andrew Morton - Dec. 19, 2012, 12:46 a.m.
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?
Alexander Holler - Dec. 19, 2012, 7:37 a.m.
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
Andrew Morton - Dec. 19, 2012, 7:45 a.m.
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?
Alexander Holler - Dec. 19, 2012, 7:55 a.m.
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
Andrew Morton - Dec. 19, 2012, 8:27 a.m.
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.
Alexander Holler - Dec. 19, 2012, 8:55 a.m.
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
Alexander Holler - Dec. 27, 2012, 12:42 p.m.
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

Patch

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);
 	}