[rtc-linux] RTC used as a module

[rtc-linux] RTC used as a module

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 1641 bytes --]

Hi guys,

I am trying to find out reason for this behavior. If rtc-zynqmp is used
as module for the first time it is correctly probed based on aliases
setting. (rtc5 for log below). But then driver is removed and add again
rtc5 is still not freed. rtc_device_release() is not called that's why
rtc->id can't be used again.

sh-4.3# modprobe rtc-zynqmp
[   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
ffa60000.rtc as rtc5
sh-4.3# rmmod rtc-zynqmp
sh-4.3# modprobe rtc-zynqmp
[   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
[   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
ffa60000.rtc as rtc0


I didn't try different rtc drivers but it looks like that someone keeps
reference that's why release function is not called.

Can someone check this with different RTC module?
Or is this expected behavior?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs


-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [rtc-linux] RTC used as a module

[Alexandre Belloni]

Hi Michal,

I've been thinking about this issue (and meaning to actually test).

On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
> Hi guys,
> 
> I am trying to find out reason for this behavior. If rtc-zynqmp is used
> as module for the first time it is correctly probed based on aliases
> setting. (rtc5 for log below). But then driver is removed and add again
> rtc5 is still not freed. rtc_device_release() is not called that's why
> rtc->id can't be used again.
> 
> sh-4.3# modprobe rtc-zynqmp
> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
> ffa60000.rtc as rtc5
> sh-4.3# rmmod rtc-zynqmp
> sh-4.3# modprobe rtc-zynqmp
> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
> ffa60000.rtc as rtc0
> 
> 
> I didn't try different rtc drivers but it looks like that someone keeps
> reference that's why release function is not called.
> 
> Can someone check this with different RTC module?

I'll do that soon.

> Or is this expected behavior?
> 

I don't think so :)

Can you clarify a few things:
 - is this the only rtc with a driver on your system ?
 - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
   checked the code and I don't think this is the issue but it is worth
testing.

Thanks,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [rtc-linux] RTC used as a module

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 4469 bytes --]

Hi Alexandre,

On 23.2.2017 13:14, Alexandre Belloni wrote:
> Hi Michal,
> 
> I've been thinking about this issue (and meaning to actually test).
> 
> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
>> Hi guys,
>>
>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
>> as module for the first time it is correctly probed based on aliases
>> setting. (rtc5 for log below). But then driver is removed and add again
>> rtc5 is still not freed. rtc_device_release() is not called that's why
>> rtc->id can't be used again.
>>
>> sh-4.3# modprobe rtc-zynqmp
>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
>> ffa60000.rtc as rtc5
>> sh-4.3# rmmod rtc-zynqmp
>> sh-4.3# modprobe rtc-zynqmp
>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
>> ffa60000.rtc as rtc0
>>
>>
>> I didn't try different rtc drivers but it looks like that someone keeps
>> reference that's why release function is not called.
>>
>> Can someone check this with different RTC module?
> 
> I'll do that soon.

Have you tried it?

> 
>> Or is this expected behavior?
>>
> 
> I don't think so :)
> 
> Can you clarify a few things:
>  - is this the only rtc with a driver on your system ?

yes only one.

>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
>    checked the code and I don't think this is the issue but it is worth
> testing.

This is the setting.

CONFIG_RTC_HCTOSYS=y
CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
CONFIG_RTC_SYSTOHC=y
CONFIG_RTC_SYSTOHC_DEVICE="rtc0"

What I see is that rtc_device_release is not called which is that
function which contain ida_simple_remove.

I have alias for rtc0 (rtc0 = &rtc;)
And here is a log with debug. As you see when driver is removed for the
first time rtc_device_release is not called. When this is done for the
second time rtc_device_release is called properly and the same device
allocated via ida is used.

root@linaro-alip:~# dmesg | grep rtc
[    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
[    3.422099] hctosys: unable to open rtc device (rtc0)
[    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
ffa60000.rtc as rtc0
root@linaro-alip:~#
root@linaro-alip:~# lsmod
Module                  Size  Used by
rtc_zynqmp             16384  0
uio_pdrv_genirq        16384  0
root@linaro-alip:~# rmmod rtc_zynqmp
[   26.805172] rtc_core: devm_rtc_device_unregister
[   26.809794] rtc_core: devm_rtc_device_release
[   26.814129] rtc_core: rtc_device_unregister
root@linaro-alip:~# modprobe rtc_zynqmp
[   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
[   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
ffa60000.rtc as rtc1
root@linaro-alip:~# rmmod rtc_zynqmp
[  122.140487] rtc_core: devm_rtc_device_unregister
[  122.145110] rtc_core: devm_rtc_device_release
[  122.149443] rtc_core: rtc_device_unregister
[  122.153770] rtc_core: rtc_device_release
root@linaro-alip:~# modprobe rtc_zynqmp
[  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
[  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
ffa60000.rtc as rtc1
root@linaro-alip:~# rmmod rtc_zynqmp
[  125.876188] rtc_core: devm_rtc_device_release
[  125.880554] rtc_core: rtc_device_unregister
[  125.885163] rtc_core: rtc_device_release
root@linaro-alip:~#

I have also done experiment to setup rtc alias to rtc1 and this ID is
used only once. It looks like something is not released properly.

This is out of tree driver but I can't see any specific code which could
improve behavior.
I have tested i2c rtc mcp79410 and there is not a visible problem but it
is not MMIO.

Thanks,
Michal

---
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs


-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [rtc-linux] RTC used as a module

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 5858 bytes --]

Hi Alexandre,

On 3.3.2017 10:41, Michal Simek wrote:
> Hi Alexandre,
> 
> On 23.2.2017 13:14, Alexandre Belloni wrote:
>> Hi Michal,
>>
>> I've been thinking about this issue (and meaning to actually test).
>>
>> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
>>> Hi guys,
>>>
>>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
>>> as module for the first time it is correctly probed based on aliases
>>> setting. (rtc5 for log below). But then driver is removed and add again
>>> rtc5 is still not freed. rtc_device_release() is not called that's why
>>> rtc->id can't be used again.
>>>
>>> sh-4.3# modprobe rtc-zynqmp
>>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>> ffa60000.rtc as rtc5
>>> sh-4.3# rmmod rtc-zynqmp
>>> sh-4.3# modprobe rtc-zynqmp
>>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
>>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>> ffa60000.rtc as rtc0
>>>
>>>
>>> I didn't try different rtc drivers but it looks like that someone keeps
>>> reference that's why release function is not called.
>>>
>>> Can someone check this with different RTC module?
>>
>> I'll do that soon.
> 
> Have you tried it?
> 
>>
>>> Or is this expected behavior?
>>>
>>
>> I don't think so :)
>>
>> Can you clarify a few things:
>>  - is this the only rtc with a driver on your system ?
> 
> yes only one.
> 
>>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
>>    checked the code and I don't think this is the issue but it is worth
>> testing.
> 
> This is the setting.
> 
> CONFIG_RTC_HCTOSYS=y
> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
> CONFIG_RTC_SYSTOHC=y
> CONFIG_RTC_SYSTOHC_DEVICE="rtc0"
> 
> What I see is that rtc_device_release is not called which is that
> function which contain ida_simple_remove.
> 
> I have alias for rtc0 (rtc0 = &rtc;)
> And here is a log with debug. As you see when driver is removed for the
> first time rtc_device_release is not called. When this is done for the
> second time rtc_device_release is called properly and the same device
> allocated via ida is used.
> 
> root@linaro-alip:~# dmesg | grep rtc
> [    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
> [    3.422099] hctosys: unable to open rtc device (rtc0)
> [    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
> ffa60000.rtc as rtc0
> root@linaro-alip:~#
> root@linaro-alip:~# lsmod
> Module                  Size  Used by
> rtc_zynqmp             16384  0
> uio_pdrv_genirq        16384  0
> root@linaro-alip:~# rmmod rtc_zynqmp
> [   26.805172] rtc_core: devm_rtc_device_unregister
> [   26.809794] rtc_core: devm_rtc_device_release
> [   26.814129] rtc_core: rtc_device_unregister
> root@linaro-alip:~# modprobe rtc_zynqmp
> [   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
> [   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
> ffa60000.rtc as rtc1
> root@linaro-alip:~# rmmod rtc_zynqmp
> [  122.140487] rtc_core: devm_rtc_device_unregister
> [  122.145110] rtc_core: devm_rtc_device_release
> [  122.149443] rtc_core: rtc_device_unregister
> [  122.153770] rtc_core: rtc_device_release
> root@linaro-alip:~# modprobe rtc_zynqmp
> [  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
> [  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
> ffa60000.rtc as rtc1
> root@linaro-alip:~# rmmod rtc_zynqmp
> [  125.876188] rtc_core: devm_rtc_device_release
> [  125.880554] rtc_core: rtc_device_unregister
> [  125.885163] rtc_core: rtc_device_release
> root@linaro-alip:~#
> 
> I have also done experiment to setup rtc alias to rtc1 and this ID is
> used only once. It looks like something is not released properly.
> 
> This is out of tree driver but I can't see any specific code which could
> improve behavior.
> I have tested i2c rtc mcp79410 and there is not a visible problem but it
> is not MMIO.

This is the function which is making the difference.

It was added by John Stultz:
"timers: Introduce in-kernel alarm-timer interface"
(sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)

Probe function has device_init_wakeup(&pdev->dev, 1);
That's why reference is taken (get_device() below)

static int alarmtimer_rtc_add_device(struct device *dev,
				struct class_interface *class_intf)
{
	unsigned long flags;
	struct rtc_device *rtc = to_rtc_device(dev);

	if (rtcdev)
		return -EBUSY;

	pr_info("%s\n", __func__);

	if (!rtc->ops->set_alarm)
		return -1;

	pr_info("%s\n", __func__);

	if (!device_may_wakeup(rtc->dev.parent))
		return -1;

	spin_lock_irqsave(&rtcdev_lock, flags);
	if (!rtcdev) {
		rtcdev = rtc;
		/* hold a reference so it doesn't go away */
		get_device(dev);
	}
	spin_unlock_irqrestore(&rtcdev_lock, flags);
	return 0;
}

And in remove function device_init_wakeup(&pdev->dev, 0);
But because there is still a reference rtc_device_release() is not
called and ida is not freed.

Generic question which I think should be asked if in this situation
driver should be modular or not.
If driver can be modular then somewhere should be put_device().

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs


-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [rtc-linux] RTC used as a module

[Alexandre Belloni]

On 03/03/2017 at 15:46:36 +0100, Michal Simek wrote:
> This is the function which is making the difference.
> 
> It was added by John Stultz:
> "timers: Introduce in-kernel alarm-timer interface"
> (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
> 
> Probe function has device_init_wakeup(&pdev->dev, 1);
> That's why reference is taken (get_device() below)
> 
> static int alarmtimer_rtc_add_device(struct device *dev,
> 				struct class_interface *class_intf)
> {
> 	unsigned long flags;
> 	struct rtc_device *rtc = to_rtc_device(dev);
> 
> 	if (rtcdev)
> 		return -EBUSY;
> 
> 	pr_info("%s\n", __func__);
> 
> 	if (!rtc->ops->set_alarm)
> 		return -1;
> 
> 	pr_info("%s\n", __func__);
> 
> 	if (!device_may_wakeup(rtc->dev.parent))
> 		return -1;
> 
> 	spin_lock_irqsave(&rtcdev_lock, flags);
> 	if (!rtcdev) {
> 		rtcdev = rtc;
> 		/* hold a reference so it doesn't go away */
> 		get_device(dev);
> 	}
> 	spin_unlock_irqrestore(&rtcdev_lock, flags);
> 	return 0;
> }
> 
> And in remove function device_init_wakeup(&pdev->dev, 0);
> But because there is still a reference rtc_device_release() is not
> called and ida is not freed.
> 
> Generic question which I think should be asked if in this situation
> driver should be modular or not.
> If driver can be modular then somewhere should be put_device().
> 

>From a quick look, I'd say the issue is coming from
8bc0dafb5cf38a19484dfb16e2c6d29e85820046 which removed the call to
rtc_class_open so you can actually unload the module.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

[-- Attachment #1: Type: text/plain, Size: 2695 bytes --]

Hi

2017-03-03 16:09 GMT+01:00 Alexandre Belloni <
alexandre.belloni@free-electrons.com>:

> On 03/03/2017 at 15:46:36 +0100, Michal Simek wrote:
> > This is the function which is making the difference.
> >
> > It was added by John Stultz:
> > "timers: Introduce in-kernel alarm-timer interface"
> > (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
> >
> > Probe function has device_init_wakeup(&pdev->dev, 1);
> > That's why reference is taken (get_device() below)
> >
> > static int alarmtimer_rtc_add_device(struct device *dev,
> >                               struct class_interface *class_intf)
> > {
> >       unsigned long flags;
> >       struct rtc_device *rtc = to_rtc_device(dev);
> >
> >       if (rtcdev)
> >               return -EBUSY;
> >
> >       pr_info("%s\n", __func__);
> >
> >       if (!rtc->ops->set_alarm)
> >               return -1;
> >
> >       pr_info("%s\n", __func__);
> >
> >       if (!device_may_wakeup(rtc->dev.parent))
> >               return -1;
> >
> >       spin_lock_irqsave(&rtcdev_lock, flags);
> >       if (!rtcdev) {
> >               rtcdev = rtc;
> >               /* hold a reference so it doesn't go away */
> >               get_device(dev);
> >       }
> >       spin_unlock_irqrestore(&rtcdev_lock, flags);
> >       return 0;
> > }
> >
> > And in remove function device_init_wakeup(&pdev->dev, 0);
> > But because there is still a reference rtc_device_release() is not
> > called and ida is not freed.
> >
> > Generic question which I think should be asked if in this situation
> > driver should be modular or not.
> > If driver can be modular then somewhere should be put_device().
> >
>
> From a quick look, I'd say the issue is coming from
> 8bc0dafb5cf38a19484dfb16e2c6d29e85820046 which removed the call to
> rtc_class_open so you can actually unload the module.
>
>
This is from 2011 which is kind of surprising.

M


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 4267 bytes --]

Re: [rtc-linux] RTC used as a module

[John Stultz]

On Fri, Mar 3, 2017 at 6:46 AM, Michal Simek <monstr@monstr.eu> wrote:
> Hi Alexandre,
>
> On 3.3.2017 10:41, Michal Simek wrote:
>> Hi Alexandre,
>>
>> On 23.2.2017 13:14, Alexandre Belloni wrote:
>>> Hi Michal,
>>>
>>> I've been thinking about this issue (and meaning to actually test).
>>>
>>> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
>>>> Hi guys,
>>>>
>>>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
>>>> as module for the first time it is correctly probed based on aliases
>>>> setting. (rtc5 for log below). But then driver is removed and add again
>>>> rtc5 is still not freed. rtc_device_release() is not called that's why
>>>> rtc->id can't be used again.
>>>>
>>>> sh-4.3# modprobe rtc-zynqmp
>>>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>> ffa60000.rtc as rtc5
>>>> sh-4.3# rmmod rtc-zynqmp
>>>> sh-4.3# modprobe rtc-zynqmp
>>>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
>>>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>> ffa60000.rtc as rtc0
>>>>
>>>>
>>>> I didn't try different rtc drivers but it looks like that someone keeps
>>>> reference that's why release function is not called.
>>>>
>>>> Can someone check this with different RTC module?
>>>
>>> I'll do that soon.
>>
>> Have you tried it?
>>
>>>
>>>> Or is this expected behavior?
>>>>
>>>
>>> I don't think so :)
>>>
>>> Can you clarify a few things:
>>>  - is this the only rtc with a driver on your system ?
>>
>> yes only one.
>>
>>>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
>>>    checked the code and I don't think this is the issue but it is worth
>>> testing.
>>
>> This is the setting.
>>
>> CONFIG_RTC_HCTOSYS=y
>> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
>> CONFIG_RTC_SYSTOHC=y
>> CONFIG_RTC_SYSTOHC_DEVICE="rtc0"
>>
>> What I see is that rtc_device_release is not called which is that
>> function which contain ida_simple_remove.
>>
>> I have alias for rtc0 (rtc0 = &rtc;)
>> And here is a log with debug. As you see when driver is removed for the
>> first time rtc_device_release is not called. When this is done for the
>> second time rtc_device_release is called properly and the same device
>> allocated via ida is used.
>>
>> root@linaro-alip:~# dmesg | grep rtc
>> [    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
>> [    3.422099] hctosys: unable to open rtc device (rtc0)
>> [    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
>> ffa60000.rtc as rtc0
>> root@linaro-alip:~#
>> root@linaro-alip:~# lsmod
>> Module                  Size  Used by
>> rtc_zynqmp             16384  0
>> uio_pdrv_genirq        16384  0
>> root@linaro-alip:~# rmmod rtc_zynqmp
>> [   26.805172] rtc_core: devm_rtc_device_unregister
>> [   26.809794] rtc_core: devm_rtc_device_release
>> [   26.814129] rtc_core: rtc_device_unregister
>> root@linaro-alip:~# modprobe rtc_zynqmp
>> [   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>> [   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
>> ffa60000.rtc as rtc1
>> root@linaro-alip:~# rmmod rtc_zynqmp
>> [  122.140487] rtc_core: devm_rtc_device_unregister
>> [  122.145110] rtc_core: devm_rtc_device_release
>> [  122.149443] rtc_core: rtc_device_unregister
>> [  122.153770] rtc_core: rtc_device_release
>> root@linaro-alip:~# modprobe rtc_zynqmp
>> [  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>> [  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
>> ffa60000.rtc as rtc1
>> root@linaro-alip:~# rmmod rtc_zynqmp
>> [  125.876188] rtc_core: devm_rtc_device_release
>> [  125.880554] rtc_core: rtc_device_unregister
>> [  125.885163] rtc_core: rtc_device_release
>> root@linaro-alip:~#
>>
>> I have also done experiment to setup rtc alias to rtc1 and this ID is
>> used only once. It looks like something is not released properly.
>>
>> This is out of tree driver but I can't see any specific code which could
>> improve behavior.
>> I have tested i2c rtc mcp79410 and there is not a visible problem but it
>> is not MMIO.
>
> This is the function which is making the difference.
>
> It was added by John Stultz:
> "timers: Introduce in-kernel alarm-timer interface"
> (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
>
> Probe function has device_init_wakeup(&pdev->dev, 1);
> That's why reference is taken (get_device() below)
>
> static int alarmtimer_rtc_add_device(struct device *dev,
>                                 struct class_interface *class_intf)
> {
>         unsigned long flags;
>         struct rtc_device *rtc = to_rtc_device(dev);
>
>         if (rtcdev)
>                 return -EBUSY;
>
>         pr_info("%s\n", __func__);
>
>         if (!rtc->ops->set_alarm)
>                 return -1;
>
>         pr_info("%s\n", __func__);
>
>         if (!device_may_wakeup(rtc->dev.parent))
>                 return -1;
>
>         spin_lock_irqsave(&rtcdev_lock, flags);
>         if (!rtcdev) {
>                 rtcdev = rtc;
>                 /* hold a reference so it doesn't go away */
>                 get_device(dev);
>         }
>         spin_unlock_irqrestore(&rtcdev_lock, flags);
>         return 0;
> }
>
> And in remove function device_init_wakeup(&pdev->dev, 0);
> But because there is still a reference rtc_device_release() is not
> called and ida is not freed.
>
> Generic question which I think should be asked if in this situation
> driver should be modular or not.
> If driver can be modular then somewhere should be put_device().

So off the top of my head (sorry, packing for a trip at the moment), I
think this was because the alarmtimer subsystem needs to hold onto the
rtcdev so it doesn't disappear under it, as it will be needed when
setting any alarmtimers on suspend.

Part of the issue is the alarmtimer interface will provide an error to
userspace if there's no rtcdev, so we don't want a situation where
timers successfully set then silently never fire because the rtc was
removed.

That said, there may very well be bugs in how it was handled, and I've
not looked closely at it recently.

thanks
-john

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

On 3.3.2017 21:04, John Stultz wrote:
> On Fri, Mar 3, 2017 at 6:46 AM, Michal Simek <monstr@monstr.eu> wrote:
>> Hi Alexandre,
>>
>> On 3.3.2017 10:41, Michal Simek wrote:
>>> Hi Alexandre,
>>>
>>> On 23.2.2017 13:14, Alexandre Belloni wrote:
>>>> Hi Michal,
>>>>
>>>> I've been thinking about this issue (and meaning to actually test).
>>>>
>>>> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
>>>>> Hi guys,
>>>>>
>>>>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
>>>>> as module for the first time it is correctly probed based on aliases
>>>>> setting. (rtc5 for log below). But then driver is removed and add again
>>>>> rtc5 is still not freed. rtc_device_release() is not called that's why
>>>>> rtc->id can't be used again.
>>>>>
>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>> ffa60000.rtc as rtc5
>>>>> sh-4.3# rmmod rtc-zynqmp
>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
>>>>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>> ffa60000.rtc as rtc0
>>>>>
>>>>>
>>>>> I didn't try different rtc drivers but it looks like that someone keeps
>>>>> reference that's why release function is not called.
>>>>>
>>>>> Can someone check this with different RTC module?
>>>>
>>>> I'll do that soon.
>>>
>>> Have you tried it?
>>>
>>>>
>>>>> Or is this expected behavior?
>>>>>
>>>>
>>>> I don't think so :)
>>>>
>>>> Can you clarify a few things:
>>>>  - is this the only rtc with a driver on your system ?
>>>
>>> yes only one.
>>>
>>>>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
>>>>    checked the code and I don't think this is the issue but it is worth
>>>> testing.
>>>
>>> This is the setting.
>>>
>>> CONFIG_RTC_HCTOSYS=y
>>> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
>>> CONFIG_RTC_SYSTOHC=y
>>> CONFIG_RTC_SYSTOHC_DEVICE="rtc0"
>>>
>>> What I see is that rtc_device_release is not called which is that
>>> function which contain ida_simple_remove.
>>>
>>> I have alias for rtc0 (rtc0 = &rtc;)
>>> And here is a log with debug. As you see when driver is removed for the
>>> first time rtc_device_release is not called. When this is done for the
>>> second time rtc_device_release is called properly and the same device
>>> allocated via ida is used.
>>>
>>> root@linaro-alip:~# dmesg | grep rtc
>>> [    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
>>> [    3.422099] hctosys: unable to open rtc device (rtc0)
>>> [    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>> ffa60000.rtc as rtc0
>>> root@linaro-alip:~#
>>> root@linaro-alip:~# lsmod
>>> Module                  Size  Used by
>>> rtc_zynqmp             16384  0
>>> uio_pdrv_genirq        16384  0
>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>> [   26.805172] rtc_core: devm_rtc_device_unregister
>>> [   26.809794] rtc_core: devm_rtc_device_release
>>> [   26.814129] rtc_core: rtc_device_unregister
>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>> [   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>> [   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>> ffa60000.rtc as rtc1
>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>> [  122.140487] rtc_core: devm_rtc_device_unregister
>>> [  122.145110] rtc_core: devm_rtc_device_release
>>> [  122.149443] rtc_core: rtc_device_unregister
>>> [  122.153770] rtc_core: rtc_device_release
>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>> [  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>> [  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>> ffa60000.rtc as rtc1
>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>> [  125.876188] rtc_core: devm_rtc_device_release
>>> [  125.880554] rtc_core: rtc_device_unregister
>>> [  125.885163] rtc_core: rtc_device_release
>>> root@linaro-alip:~#
>>>
>>> I have also done experiment to setup rtc alias to rtc1 and this ID is
>>> used only once. It looks like something is not released properly.
>>>
>>> This is out of tree driver but I can't see any specific code which could
>>> improve behavior.
>>> I have tested i2c rtc mcp79410 and there is not a visible problem but it
>>> is not MMIO.
>>
>> This is the function which is making the difference.
>>
>> It was added by John Stultz:
>> "timers: Introduce in-kernel alarm-timer interface"
>> (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
>>
>> Probe function has device_init_wakeup(&pdev->dev, 1);
>> That's why reference is taken (get_device() below)
>>
>> static int alarmtimer_rtc_add_device(struct device *dev,
>>                                 struct class_interface *class_intf)
>> {
>>         unsigned long flags;
>>         struct rtc_device *rtc = to_rtc_device(dev);
>>
>>         if (rtcdev)
>>                 return -EBUSY;
>>
>>         pr_info("%s\n", __func__);
>>
>>         if (!rtc->ops->set_alarm)
>>                 return -1;
>>
>>         pr_info("%s\n", __func__);
>>
>>         if (!device_may_wakeup(rtc->dev.parent))
>>                 return -1;
>>
>>         spin_lock_irqsave(&rtcdev_lock, flags);
>>         if (!rtcdev) {
>>                 rtcdev = rtc;
>>                 /* hold a reference so it doesn't go away */
>>                 get_device(dev);
>>         }
>>         spin_unlock_irqrestore(&rtcdev_lock, flags);
>>         return 0;
>> }
>>
>> And in remove function device_init_wakeup(&pdev->dev, 0);
>> But because there is still a reference rtc_device_release() is not
>> called and ida is not freed.
>>
>> Generic question which I think should be asked if in this situation
>> driver should be modular or not.
>> If driver can be modular then somewhere should be put_device().
> 
> So off the top of my head (sorry, packing for a trip at the moment), I
> think this was because the alarmtimer subsystem needs to hold onto the
> rtcdev so it doesn't disappear under it, as it will be needed when
> setting any alarmtimers on suspend.
> 
> Part of the issue is the alarmtimer interface will provide an error to
> userspace if there's no rtcdev, so we don't want a situation where
> timers successfully set then silently never fire because the rtc was
> removed.

right I fully understand that.

> 
> That said, there may very well be bugs in how it was handled, and I've
> not looked closely at it recently.

On the other hand you can have multiple alarms in the system and you can
remove them at run time. It means I would expect that you can remove rtc
if alarm is not set. When you run application driver is in use and you
can't do it.

Anyway by saying this I expect every driver which setup
device_init_wakeup(..,1); and device_init_wakeup(..,0) in remove has the
same issue.
Solution is definitely not making rtc as a module which is in my case
rtc-zynqmp an option but not the best one.

Thanks,
Michal

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

Hi John and Alexandre,

On 6.3.2017 15:56, Michal Simek wrote:
> On 3.3.2017 21:04, John Stultz wrote:
>> On Fri, Mar 3, 2017 at 6:46 AM, Michal Simek <monstr@monstr.eu> wrote:
>>> Hi Alexandre,
>>>
>>> On 3.3.2017 10:41, Michal Simek wrote:
>>>> Hi Alexandre,
>>>>
>>>> On 23.2.2017 13:14, Alexandre Belloni wrote:
>>>>> Hi Michal,
>>>>>
>>>>> I've been thinking about this issue (and meaning to actually test).
>>>>>
>>>>> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
>>>>>> Hi guys,
>>>>>>
>>>>>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
>>>>>> as module for the first time it is correctly probed based on aliases
>>>>>> setting. (rtc5 for log below). But then driver is removed and add again
>>>>>> rtc5 is still not freed. rtc_device_release() is not called that's why
>>>>>> rtc->id can't be used again.
>>>>>>
>>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>> ffa60000.rtc as rtc5
>>>>>> sh-4.3# rmmod rtc-zynqmp
>>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
>>>>>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>> ffa60000.rtc as rtc0
>>>>>>
>>>>>>
>>>>>> I didn't try different rtc drivers but it looks like that someone keeps
>>>>>> reference that's why release function is not called.
>>>>>>
>>>>>> Can someone check this with different RTC module?
>>>>>
>>>>> I'll do that soon.
>>>>
>>>> Have you tried it?
>>>>
>>>>>
>>>>>> Or is this expected behavior?
>>>>>>
>>>>>
>>>>> I don't think so :)
>>>>>
>>>>> Can you clarify a few things:
>>>>>  - is this the only rtc with a driver on your system ?
>>>>
>>>> yes only one.
>>>>
>>>>>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
>>>>>    checked the code and I don't think this is the issue but it is worth
>>>>> testing.
>>>>
>>>> This is the setting.
>>>>
>>>> CONFIG_RTC_HCTOSYS=y
>>>> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
>>>> CONFIG_RTC_SYSTOHC=y
>>>> CONFIG_RTC_SYSTOHC_DEVICE="rtc0"
>>>>
>>>> What I see is that rtc_device_release is not called which is that
>>>> function which contain ida_simple_remove.
>>>>
>>>> I have alias for rtc0 (rtc0 = &rtc;)
>>>> And here is a log with debug. As you see when driver is removed for the
>>>> first time rtc_device_release is not called. When this is done for the
>>>> second time rtc_device_release is called properly and the same device
>>>> allocated via ida is used.
>>>>
>>>> root@linaro-alip:~# dmesg | grep rtc
>>>> [    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
>>>> [    3.422099] hctosys: unable to open rtc device (rtc0)
>>>> [    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>> ffa60000.rtc as rtc0
>>>> root@linaro-alip:~#
>>>> root@linaro-alip:~# lsmod
>>>> Module                  Size  Used by
>>>> rtc_zynqmp             16384  0
>>>> uio_pdrv_genirq        16384  0
>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>> [   26.805172] rtc_core: devm_rtc_device_unregister
>>>> [   26.809794] rtc_core: devm_rtc_device_release
>>>> [   26.814129] rtc_core: rtc_device_unregister
>>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>>> [   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>>> [   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>> ffa60000.rtc as rtc1
>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>> [  122.140487] rtc_core: devm_rtc_device_unregister
>>>> [  122.145110] rtc_core: devm_rtc_device_release
>>>> [  122.149443] rtc_core: rtc_device_unregister
>>>> [  122.153770] rtc_core: rtc_device_release
>>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>>> [  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>>> [  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>> ffa60000.rtc as rtc1
>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>> [  125.876188] rtc_core: devm_rtc_device_release
>>>> [  125.880554] rtc_core: rtc_device_unregister
>>>> [  125.885163] rtc_core: rtc_device_release
>>>> root@linaro-alip:~#
>>>>
>>>> I have also done experiment to setup rtc alias to rtc1 and this ID is
>>>> used only once. It looks like something is not released properly.
>>>>
>>>> This is out of tree driver but I can't see any specific code which could
>>>> improve behavior.
>>>> I have tested i2c rtc mcp79410 and there is not a visible problem but it
>>>> is not MMIO.
>>>
>>> This is the function which is making the difference.
>>>
>>> It was added by John Stultz:
>>> "timers: Introduce in-kernel alarm-timer interface"
>>> (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
>>>
>>> Probe function has device_init_wakeup(&pdev->dev, 1);
>>> That's why reference is taken (get_device() below)
>>>
>>> static int alarmtimer_rtc_add_device(struct device *dev,
>>>                                 struct class_interface *class_intf)
>>> {
>>>         unsigned long flags;
>>>         struct rtc_device *rtc = to_rtc_device(dev);
>>>
>>>         if (rtcdev)
>>>                 return -EBUSY;
>>>
>>>         pr_info("%s\n", __func__);
>>>
>>>         if (!rtc->ops->set_alarm)
>>>                 return -1;
>>>
>>>         pr_info("%s\n", __func__);
>>>
>>>         if (!device_may_wakeup(rtc->dev.parent))
>>>                 return -1;
>>>
>>>         spin_lock_irqsave(&rtcdev_lock, flags);
>>>         if (!rtcdev) {
>>>                 rtcdev = rtc;
>>>                 /* hold a reference so it doesn't go away */
>>>                 get_device(dev);
>>>         }
>>>         spin_unlock_irqrestore(&rtcdev_lock, flags);
>>>         return 0;
>>> }
>>>
>>> And in remove function device_init_wakeup(&pdev->dev, 0);
>>> But because there is still a reference rtc_device_release() is not
>>> called and ida is not freed.
>>>
>>> Generic question which I think should be asked if in this situation
>>> driver should be modular or not.
>>> If driver can be modular then somewhere should be put_device().
>>
>> So off the top of my head (sorry, packing for a trip at the moment), I
>> think this was because the alarmtimer subsystem needs to hold onto the
>> rtcdev so it doesn't disappear under it, as it will be needed when
>> setting any alarmtimers on suspend.
>>
>> Part of the issue is the alarmtimer interface will provide an error to
>> userspace if there's no rtcdev, so we don't want a situation where
>> timers successfully set then silently never fire because the rtc was
>> removed.
> 
> right I fully understand that.
> 
>>
>> That said, there may very well be bugs in how it was handled, and I've
>> not looked closely at it recently.
> 
> On the other hand you can have multiple alarms in the system and you can
> remove them at run time. It means I would expect that you can remove rtc
> if alarm is not set. When you run application driver is in use and you
> can't do it.
> 
> Anyway by saying this I expect every driver which setup
> device_init_wakeup(..,1); and device_init_wakeup(..,0) in remove has the
> same issue.
> Solution is definitely not making rtc as a module which is in my case
> rtc-zynqmp an option but not the best one.

I think it is good time to continue on this.

Thanks,
Michal

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

On 16.3.2017 16:35, Michal Simek wrote:
> Hi John and Alexandre,
> 
> On 6.3.2017 15:56, Michal Simek wrote:
>> On 3.3.2017 21:04, John Stultz wrote:
>>> On Fri, Mar 3, 2017 at 6:46 AM, Michal Simek <monstr@monstr.eu> wrote:
>>>> Hi Alexandre,
>>>>
>>>> On 3.3.2017 10:41, Michal Simek wrote:
>>>>> Hi Alexandre,
>>>>>
>>>>> On 23.2.2017 13:14, Alexandre Belloni wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> I've been thinking about this issue (and meaning to actually test).
>>>>>>
>>>>>> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
>>>>>>> Hi guys,
>>>>>>>
>>>>>>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
>>>>>>> as module for the first time it is correctly probed based on aliases
>>>>>>> setting. (rtc5 for log below). But then driver is removed and add again
>>>>>>> rtc5 is still not freed. rtc_device_release() is not called that's why
>>>>>>> rtc->id can't be used again.
>>>>>>>
>>>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>>>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>>> ffa60000.rtc as rtc5
>>>>>>> sh-4.3# rmmod rtc-zynqmp
>>>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>>>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
>>>>>>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>>> ffa60000.rtc as rtc0
>>>>>>>
>>>>>>>
>>>>>>> I didn't try different rtc drivers but it looks like that someone keeps
>>>>>>> reference that's why release function is not called.
>>>>>>>
>>>>>>> Can someone check this with different RTC module?
>>>>>>
>>>>>> I'll do that soon.
>>>>>
>>>>> Have you tried it?
>>>>>
>>>>>>
>>>>>>> Or is this expected behavior?
>>>>>>>
>>>>>>
>>>>>> I don't think so :)
>>>>>>
>>>>>> Can you clarify a few things:
>>>>>>  - is this the only rtc with a driver on your system ?
>>>>>
>>>>> yes only one.
>>>>>
>>>>>>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
>>>>>>    checked the code and I don't think this is the issue but it is worth
>>>>>> testing.
>>>>>
>>>>> This is the setting.
>>>>>
>>>>> CONFIG_RTC_HCTOSYS=y
>>>>> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
>>>>> CONFIG_RTC_SYSTOHC=y
>>>>> CONFIG_RTC_SYSTOHC_DEVICE="rtc0"
>>>>>
>>>>> What I see is that rtc_device_release is not called which is that
>>>>> function which contain ida_simple_remove.
>>>>>
>>>>> I have alias for rtc0 (rtc0 = &rtc;)
>>>>> And here is a log with debug. As you see when driver is removed for the
>>>>> first time rtc_device_release is not called. When this is done for the
>>>>> second time rtc_device_release is called properly and the same device
>>>>> allocated via ida is used.
>>>>>
>>>>> root@linaro-alip:~# dmesg | grep rtc
>>>>> [    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
>>>>> [    3.422099] hctosys: unable to open rtc device (rtc0)
>>>>> [    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>> ffa60000.rtc as rtc0
>>>>> root@linaro-alip:~#
>>>>> root@linaro-alip:~# lsmod
>>>>> Module                  Size  Used by
>>>>> rtc_zynqmp             16384  0
>>>>> uio_pdrv_genirq        16384  0
>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>>> [   26.805172] rtc_core: devm_rtc_device_unregister
>>>>> [   26.809794] rtc_core: devm_rtc_device_release
>>>>> [   26.814129] rtc_core: rtc_device_unregister
>>>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>>>> [   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>>>> [   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>> ffa60000.rtc as rtc1
>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>>> [  122.140487] rtc_core: devm_rtc_device_unregister
>>>>> [  122.145110] rtc_core: devm_rtc_device_release
>>>>> [  122.149443] rtc_core: rtc_device_unregister
>>>>> [  122.153770] rtc_core: rtc_device_release
>>>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>>>> [  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>>>> [  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>> ffa60000.rtc as rtc1
>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>>> [  125.876188] rtc_core: devm_rtc_device_release
>>>>> [  125.880554] rtc_core: rtc_device_unregister
>>>>> [  125.885163] rtc_core: rtc_device_release
>>>>> root@linaro-alip:~#
>>>>>
>>>>> I have also done experiment to setup rtc alias to rtc1 and this ID is
>>>>> used only once. It looks like something is not released properly.
>>>>>
>>>>> This is out of tree driver but I can't see any specific code which could
>>>>> improve behavior.
>>>>> I have tested i2c rtc mcp79410 and there is not a visible problem but it
>>>>> is not MMIO.
>>>>
>>>> This is the function which is making the difference.
>>>>
>>>> It was added by John Stultz:
>>>> "timers: Introduce in-kernel alarm-timer interface"
>>>> (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
>>>>
>>>> Probe function has device_init_wakeup(&pdev->dev, 1);
>>>> That's why reference is taken (get_device() below)
>>>>
>>>> static int alarmtimer_rtc_add_device(struct device *dev,
>>>>                                 struct class_interface *class_intf)
>>>> {
>>>>         unsigned long flags;
>>>>         struct rtc_device *rtc = to_rtc_device(dev);
>>>>
>>>>         if (rtcdev)
>>>>                 return -EBUSY;
>>>>
>>>>         pr_info("%s\n", __func__);
>>>>
>>>>         if (!rtc->ops->set_alarm)
>>>>                 return -1;
>>>>
>>>>         pr_info("%s\n", __func__);
>>>>
>>>>         if (!device_may_wakeup(rtc->dev.parent))
>>>>                 return -1;
>>>>
>>>>         spin_lock_irqsave(&rtcdev_lock, flags);
>>>>         if (!rtcdev) {
>>>>                 rtcdev = rtc;
>>>>                 /* hold a reference so it doesn't go away */
>>>>                 get_device(dev);
>>>>         }
>>>>         spin_unlock_irqrestore(&rtcdev_lock, flags);
>>>>         return 0;
>>>> }
>>>>
>>>> And in remove function device_init_wakeup(&pdev->dev, 0);
>>>> But because there is still a reference rtc_device_release() is not
>>>> called and ida is not freed.
>>>>
>>>> Generic question which I think should be asked if in this situation
>>>> driver should be modular or not.
>>>> If driver can be modular then somewhere should be put_device().
>>>
>>> So off the top of my head (sorry, packing for a trip at the moment), I
>>> think this was because the alarmtimer subsystem needs to hold onto the
>>> rtcdev so it doesn't disappear under it, as it will be needed when
>>> setting any alarmtimers on suspend.
>>>
>>> Part of the issue is the alarmtimer interface will provide an error to
>>> userspace if there's no rtcdev, so we don't want a situation where
>>> timers successfully set then silently never fire because the rtc was
>>> removed.
>>
>> right I fully understand that.
>>
>>>
>>> That said, there may very well be bugs in how it was handled, and I've
>>> not looked closely at it recently.
>>
>> On the other hand you can have multiple alarms in the system and you can
>> remove them at run time. It means I would expect that you can remove rtc
>> if alarm is not set. When you run application driver is in use and you
>> can't do it.
>>
>> Anyway by saying this I expect every driver which setup
>> device_init_wakeup(..,1); and device_init_wakeup(..,0) in remove has the
>> same issue.
>> Solution is definitely not making rtc as a module which is in my case
>> rtc-zynqmp an option but not the best one.
> 
> I think it is good time to continue on this.

Any update on this one?

M



-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

Hi John and Alexandre,

On 7.4.2017 09:12, Michal Simek wrote:
> On 16.3.2017 16:35, Michal Simek wrote:
>> Hi John and Alexandre,
>>
>> On 6.3.2017 15:56, Michal Simek wrote:
>>> On 3.3.2017 21:04, John Stultz wrote:
>>>> On Fri, Mar 3, 2017 at 6:46 AM, Michal Simek <monstr@monstr.eu> wrote:
>>>>> Hi Alexandre,
>>>>>
>>>>> On 3.3.2017 10:41, Michal Simek wrote:
>>>>>> Hi Alexandre,
>>>>>>
>>>>>> On 23.2.2017 13:14, Alexandre Belloni wrote:
>>>>>>> Hi Michal,
>>>>>>>
>>>>>>> I've been thinking about this issue (and meaning to actually test).
>>>>>>>
>>>>>>> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
>>>>>>>> Hi guys,
>>>>>>>>
>>>>>>>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
>>>>>>>> as module for the first time it is correctly probed based on aliases
>>>>>>>> setting. (rtc5 for log below). But then driver is removed and add again
>>>>>>>> rtc5 is still not freed. rtc_device_release() is not called that's why
>>>>>>>> rtc->id can't be used again.
>>>>>>>>
>>>>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>>>>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>>>> ffa60000.rtc as rtc5
>>>>>>>> sh-4.3# rmmod rtc-zynqmp
>>>>>>>> sh-4.3# modprobe rtc-zynqmp
>>>>>>>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
>>>>>>>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>>>> ffa60000.rtc as rtc0
>>>>>>>>
>>>>>>>>
>>>>>>>> I didn't try different rtc drivers but it looks like that someone keeps
>>>>>>>> reference that's why release function is not called.
>>>>>>>>
>>>>>>>> Can someone check this with different RTC module?
>>>>>>>
>>>>>>> I'll do that soon.
>>>>>>
>>>>>> Have you tried it?
>>>>>>
>>>>>>>
>>>>>>>> Or is this expected behavior?
>>>>>>>>
>>>>>>>
>>>>>>> I don't think so :)
>>>>>>>
>>>>>>> Can you clarify a few things:
>>>>>>>  - is this the only rtc with a driver on your system ?
>>>>>>
>>>>>> yes only one.
>>>>>>
>>>>>>>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
>>>>>>>    checked the code and I don't think this is the issue but it is worth
>>>>>>> testing.
>>>>>>
>>>>>> This is the setting.
>>>>>>
>>>>>> CONFIG_RTC_HCTOSYS=y
>>>>>> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
>>>>>> CONFIG_RTC_SYSTOHC=y
>>>>>> CONFIG_RTC_SYSTOHC_DEVICE="rtc0"
>>>>>>
>>>>>> What I see is that rtc_device_release is not called which is that
>>>>>> function which contain ida_simple_remove.
>>>>>>
>>>>>> I have alias for rtc0 (rtc0 = &rtc;)
>>>>>> And here is a log with debug. As you see when driver is removed for the
>>>>>> first time rtc_device_release is not called. When this is done for the
>>>>>> second time rtc_device_release is called properly and the same device
>>>>>> allocated via ida is used.
>>>>>>
>>>>>> root@linaro-alip:~# dmesg | grep rtc
>>>>>> [    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
>>>>>> [    3.422099] hctosys: unable to open rtc device (rtc0)
>>>>>> [    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>> ffa60000.rtc as rtc0
>>>>>> root@linaro-alip:~#
>>>>>> root@linaro-alip:~# lsmod
>>>>>> Module                  Size  Used by
>>>>>> rtc_zynqmp             16384  0
>>>>>> uio_pdrv_genirq        16384  0
>>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>>>> [   26.805172] rtc_core: devm_rtc_device_unregister
>>>>>> [   26.809794] rtc_core: devm_rtc_device_release
>>>>>> [   26.814129] rtc_core: rtc_device_unregister
>>>>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>>>>> [   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>>>>> [   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>> ffa60000.rtc as rtc1
>>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>>>> [  122.140487] rtc_core: devm_rtc_device_unregister
>>>>>> [  122.145110] rtc_core: devm_rtc_device_release
>>>>>> [  122.149443] rtc_core: rtc_device_unregister
>>>>>> [  122.153770] rtc_core: rtc_device_release
>>>>>> root@linaro-alip:~# modprobe rtc_zynqmp
>>>>>> [  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
>>>>>> [  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
>>>>>> ffa60000.rtc as rtc1
>>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
>>>>>> [  125.876188] rtc_core: devm_rtc_device_release
>>>>>> [  125.880554] rtc_core: rtc_device_unregister
>>>>>> [  125.885163] rtc_core: rtc_device_release
>>>>>> root@linaro-alip:~#
>>>>>>
>>>>>> I have also done experiment to setup rtc alias to rtc1 and this ID is
>>>>>> used only once. It looks like something is not released properly.
>>>>>>
>>>>>> This is out of tree driver but I can't see any specific code which could
>>>>>> improve behavior.
>>>>>> I have tested i2c rtc mcp79410 and there is not a visible problem but it
>>>>>> is not MMIO.
>>>>>
>>>>> This is the function which is making the difference.
>>>>>
>>>>> It was added by John Stultz:
>>>>> "timers: Introduce in-kernel alarm-timer interface"
>>>>> (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
>>>>>
>>>>> Probe function has device_init_wakeup(&pdev->dev, 1);
>>>>> That's why reference is taken (get_device() below)
>>>>>
>>>>> static int alarmtimer_rtc_add_device(struct device *dev,
>>>>>                                 struct class_interface *class_intf)
>>>>> {
>>>>>         unsigned long flags;
>>>>>         struct rtc_device *rtc = to_rtc_device(dev);
>>>>>
>>>>>         if (rtcdev)
>>>>>                 return -EBUSY;
>>>>>
>>>>>         pr_info("%s\n", __func__);
>>>>>
>>>>>         if (!rtc->ops->set_alarm)
>>>>>                 return -1;
>>>>>
>>>>>         pr_info("%s\n", __func__);
>>>>>
>>>>>         if (!device_may_wakeup(rtc->dev.parent))
>>>>>                 return -1;
>>>>>
>>>>>         spin_lock_irqsave(&rtcdev_lock, flags);
>>>>>         if (!rtcdev) {
>>>>>                 rtcdev = rtc;
>>>>>                 /* hold a reference so it doesn't go away */
>>>>>                 get_device(dev);
>>>>>         }
>>>>>         spin_unlock_irqrestore(&rtcdev_lock, flags);
>>>>>         return 0;
>>>>> }
>>>>>
>>>>> And in remove function device_init_wakeup(&pdev->dev, 0);
>>>>> But because there is still a reference rtc_device_release() is not
>>>>> called and ida is not freed.
>>>>>
>>>>> Generic question which I think should be asked if in this situation
>>>>> driver should be modular or not.
>>>>> If driver can be modular then somewhere should be put_device().
>>>>
>>>> So off the top of my head (sorry, packing for a trip at the moment), I
>>>> think this was because the alarmtimer subsystem needs to hold onto the
>>>> rtcdev so it doesn't disappear under it, as it will be needed when
>>>> setting any alarmtimers on suspend.
>>>>
>>>> Part of the issue is the alarmtimer interface will provide an error to
>>>> userspace if there's no rtcdev, so we don't want a situation where
>>>> timers successfully set then silently never fire because the rtc was
>>>> removed.
>>>
>>> right I fully understand that.
>>>
>>>>
>>>> That said, there may very well be bugs in how it was handled, and I've
>>>> not looked closely at it recently.
>>>
>>> On the other hand you can have multiple alarms in the system and you can
>>> remove them at run time. It means I would expect that you can remove rtc
>>> if alarm is not set. When you run application driver is in use and you
>>> can't do it.
>>>
>>> Anyway by saying this I expect every driver which setup
>>> device_init_wakeup(..,1); and device_init_wakeup(..,0) in remove has the
>>> same issue.
>>> Solution is definitely not making rtc as a module which is in my case
>>> rtc-zynqmp an option but not the best one.
>>
>> I think it is good time to continue on this.
> 
> Any update on this one?

It is quite some time from my last ping that's hwy I am sending new one.
If this is not going to be resolve I will send a patch which will simply
disable module functionality which is reasonable workaround for now.

Thanks,
Michal

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Alexandre Belloni]

Hi Michal,

I've just send a patch to fix this issue (and avoid your other patch).

Could you test it? (I did test on an atmel platform)

Sorry it took so long!

On 29/06/2017 at 09:34:14 +0200, Michal Simek wrote:
> Hi John and Alexandre,
> 
> On 7.4.2017 09:12, Michal Simek wrote:
> > On 16.3.2017 16:35, Michal Simek wrote:
> >> Hi John and Alexandre,
> >>
> >> On 6.3.2017 15:56, Michal Simek wrote:
> >>> On 3.3.2017 21:04, John Stultz wrote:
> >>>> On Fri, Mar 3, 2017 at 6:46 AM, Michal Simek <monstr@monstr.eu> wrote:
> >>>>> Hi Alexandre,
> >>>>>
> >>>>> On 3.3.2017 10:41, Michal Simek wrote:
> >>>>>> Hi Alexandre,
> >>>>>>
> >>>>>> On 23.2.2017 13:14, Alexandre Belloni wrote:
> >>>>>>> Hi Michal,
> >>>>>>>
> >>>>>>> I've been thinking about this issue (and meaning to actually test).
> >>>>>>>
> >>>>>>> On 08/12/2016 at 14:02:36 +0100, Michal Simek wrote:
> >>>>>>>> Hi guys,
> >>>>>>>>
> >>>>>>>> I am trying to find out reason for this behavior. If rtc-zynqmp is used
> >>>>>>>> as module for the first time it is correctly probed based on aliases
> >>>>>>>> setting. (rtc5 for log below). But then driver is removed and add again
> >>>>>>>> rtc5 is still not freed. rtc_device_release() is not called that's why
> >>>>>>>> rtc->id can't be used again.
> >>>>>>>>
> >>>>>>>> sh-4.3# modprobe rtc-zynqmp
> >>>>>>>> [   42.468565] rtc_zynqmp ffa60000.rtc: rtc core: registered
> >>>>>>>> ffa60000.rtc as rtc5
> >>>>>>>> sh-4.3# rmmod rtc-zynqmp
> >>>>>>>> sh-4.3# modprobe rtc-zynqmp
> >>>>>>>> [   48.648222] rtc_zynqmp ffa60000.rtc: /aliases ID 5 not available
> >>>>>>>> [   48.654280] rtc_zynqmp ffa60000.rtc: rtc core: registered
> >>>>>>>> ffa60000.rtc as rtc0
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> I didn't try different rtc drivers but it looks like that someone keeps
> >>>>>>>> reference that's why release function is not called.
> >>>>>>>>
> >>>>>>>> Can someone check this with different RTC module?
> >>>>>>>
> >>>>>>> I'll do that soon.
> >>>>>>
> >>>>>> Have you tried it?
> >>>>>>
> >>>>>>>
> >>>>>>>> Or is this expected behavior?
> >>>>>>>>
> >>>>>>>
> >>>>>>> I don't think so :)
> >>>>>>>
> >>>>>>> Can you clarify a few things:
> >>>>>>>  - is this the only rtc with a driver on your system ?
> >>>>>>
> >>>>>> yes only one.
> >>>>>>
> >>>>>>>  - is it selected as the RTC_SYSTOHC_DEVICE or RTC_HCTOSYS_DEVICE. I've
> >>>>>>>    checked the code and I don't think this is the issue but it is worth
> >>>>>>> testing.
> >>>>>>
> >>>>>> This is the setting.
> >>>>>>
> >>>>>> CONFIG_RTC_HCTOSYS=y
> >>>>>> CONFIG_RTC_HCTOSYS_DEVICE="rtc0"
> >>>>>> CONFIG_RTC_SYSTOHC=y
> >>>>>> CONFIG_RTC_SYSTOHC_DEVICE="rtc0"
> >>>>>>
> >>>>>> What I see is that rtc_device_release is not called which is that
> >>>>>> function which contain ida_simple_remove.
> >>>>>>
> >>>>>> I have alias for rtc0 (rtc0 = &rtc;)
> >>>>>> And here is a log with debug. As you see when driver is removed for the
> >>>>>> first time rtc_device_release is not called. When this is done for the
> >>>>>> second time rtc_device_release is called properly and the same device
> >>>>>> allocated via ida is used.
> >>>>>>
> >>>>>> root@linaro-alip:~# dmesg | grep rtc
> >>>>>> [    3.372715] [drm] Cannot find any crtc or sizes - going 1024x768
> >>>>>> [    3.422099] hctosys: unable to open rtc device (rtc0)
> >>>>>> [    6.578930] rtc_zynqmp ffa60000.rtc: rtc core: registered
> >>>>>> ffa60000.rtc as rtc0
> >>>>>> root@linaro-alip:~#
> >>>>>> root@linaro-alip:~# lsmod
> >>>>>> Module                  Size  Used by
> >>>>>> rtc_zynqmp             16384  0
> >>>>>> uio_pdrv_genirq        16384  0
> >>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
> >>>>>> [   26.805172] rtc_core: devm_rtc_device_unregister
> >>>>>> [   26.809794] rtc_core: devm_rtc_device_release
> >>>>>> [   26.814129] rtc_core: rtc_device_unregister
> >>>>>> root@linaro-alip:~# modprobe rtc_zynqmp
> >>>>>> [   35.879655] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
> >>>>>> [   35.886002] rtc_zynqmp ffa60000.rtc: rtc core: registered
> >>>>>> ffa60000.rtc as rtc1
> >>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
> >>>>>> [  122.140487] rtc_core: devm_rtc_device_unregister
> >>>>>> [  122.145110] rtc_core: devm_rtc_device_release
> >>>>>> [  122.149443] rtc_core: rtc_device_unregister
> >>>>>> [  122.153770] rtc_core: rtc_device_release
> >>>>>> root@linaro-alip:~# modprobe rtc_zynqmp
> >>>>>> [  123.646002] rtc_zynqmp ffa60000.rtc: /aliases ID 0 not available
> >>>>>> [  123.652249] rtc_zynqmp ffa60000.rtc: rtc core: registered
> >>>>>> ffa60000.rtc as rtc1
> >>>>>> root@linaro-alip:~# rmmod rtc_zynqmp
> >>>>>> [  125.876188] rtc_core: devm_rtc_device_release
> >>>>>> [  125.880554] rtc_core: rtc_device_unregister
> >>>>>> [  125.885163] rtc_core: rtc_device_release
> >>>>>> root@linaro-alip:~#
> >>>>>>
> >>>>>> I have also done experiment to setup rtc alias to rtc1 and this ID is
> >>>>>> used only once. It looks like something is not released properly.
> >>>>>>
> >>>>>> This is out of tree driver but I can't see any specific code which could
> >>>>>> improve behavior.
> >>>>>> I have tested i2c rtc mcp79410 and there is not a visible problem but it
> >>>>>> is not MMIO.
> >>>>>
> >>>>> This is the function which is making the difference.
> >>>>>
> >>>>> It was added by John Stultz:
> >>>>> "timers: Introduce in-kernel alarm-timer interface"
> >>>>> (sha1:  ff3ead96d17f47ee70c294a5cc2cce9b61e82f0f)
> >>>>>
> >>>>> Probe function has device_init_wakeup(&pdev->dev, 1);
> >>>>> That's why reference is taken (get_device() below)
> >>>>>
> >>>>> static int alarmtimer_rtc_add_device(struct device *dev,
> >>>>>                                 struct class_interface *class_intf)
> >>>>> {
> >>>>>         unsigned long flags;
> >>>>>         struct rtc_device *rtc = to_rtc_device(dev);
> >>>>>
> >>>>>         if (rtcdev)
> >>>>>                 return -EBUSY;
> >>>>>
> >>>>>         pr_info("%s\n", __func__);
> >>>>>
> >>>>>         if (!rtc->ops->set_alarm)
> >>>>>                 return -1;
> >>>>>
> >>>>>         pr_info("%s\n", __func__);
> >>>>>
> >>>>>         if (!device_may_wakeup(rtc->dev.parent))
> >>>>>                 return -1;
> >>>>>
> >>>>>         spin_lock_irqsave(&rtcdev_lock, flags);
> >>>>>         if (!rtcdev) {
> >>>>>                 rtcdev = rtc;
> >>>>>                 /* hold a reference so it doesn't go away */
> >>>>>                 get_device(dev);
> >>>>>         }
> >>>>>         spin_unlock_irqrestore(&rtcdev_lock, flags);
> >>>>>         return 0;
> >>>>> }
> >>>>>
> >>>>> And in remove function device_init_wakeup(&pdev->dev, 0);
> >>>>> But because there is still a reference rtc_device_release() is not
> >>>>> called and ida is not freed.
> >>>>>
> >>>>> Generic question which I think should be asked if in this situation
> >>>>> driver should be modular or not.
> >>>>> If driver can be modular then somewhere should be put_device().
> >>>>
> >>>> So off the top of my head (sorry, packing for a trip at the moment), I
> >>>> think this was because the alarmtimer subsystem needs to hold onto the
> >>>> rtcdev so it doesn't disappear under it, as it will be needed when
> >>>> setting any alarmtimers on suspend.
> >>>>
> >>>> Part of the issue is the alarmtimer interface will provide an error to
> >>>> userspace if there's no rtcdev, so we don't want a situation where
> >>>> timers successfully set then silently never fire because the rtc was
> >>>> removed.
> >>>
> >>> right I fully understand that.
> >>>
> >>>>
> >>>> That said, there may very well be bugs in how it was handled, and I've
> >>>> not looked closely at it recently.
> >>>
> >>> On the other hand you can have multiple alarms in the system and you can
> >>> remove them at run time. It means I would expect that you can remove rtc
> >>> if alarm is not set. When you run application driver is in use and you
> >>> can't do it.
> >>>
> >>> Anyway by saying this I expect every driver which setup
> >>> device_init_wakeup(..,1); and device_init_wakeup(..,0) in remove has the
> >>> same issue.
> >>> Solution is definitely not making rtc as a module which is in my case
> >>> rtc-zynqmp an option but not the best one.
> >>
> >> I think it is good time to continue on this.
> > 
> > Any update on this one?
> 
> It is quite some time from my last ping that's hwy I am sending new one.
> If this is not going to be resolve I will send a patch which will simply
> disable module functionality which is reasonable workaround for now.
> 
> Thanks,
> Michal
> 

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 954 bytes --]

Hi,

On 21.8.2017 00:03, Alexandre Belloni wrote:
> Hi Michal,
> 
> I've just send a patch to fix this issue (and avoid your other patch).
> 
> Could you test it? (I did test on an atmel platform)
> 
> Sorry it took so long!

not a problem but it looks like it is still just temporary solution. I
would expect that you can't unload module when this alarm is used. It if
it is not used then you should be able to remove it.

Thanks,
Michal

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [rtc-linux] RTC used as a module

[Alexandre Belloni]

On 21/08/2017 at 12:50:34 +0200, Michal Simek wrote:
> Hi,
> 
> On 21.8.2017 00:03, Alexandre Belloni wrote:
> > Hi Michal,
> > 
> > I've just send a patch to fix this issue (and avoid your other patch).
> > 
> > Could you test it? (I did test on an atmel platform)
> > 
> > Sorry it took so long!
> 
> not a problem but it looks like it is still just temporary solution. I
> would expect that you can't unload module when this alarm is used. It if
> it is not used then you should be able to remove it.
> 

Yes, the alarmtimer handling needs to be made more flexible, especially
in the rtc device selection.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

On 21.8.2017 14:13, Alexandre Belloni wrote:
> On 21/08/2017 at 12:50:34 +0200, Michal Simek wrote:
>> Hi,
>>
>> On 21.8.2017 00:03, Alexandre Belloni wrote:
>>> Hi Michal,
>>>
>>> I've just send a patch to fix this issue (and avoid your other patch).
>>>
>>> Could you test it? (I did test on an atmel platform)
>>>
>>> Sorry it took so long!
>>
>> not a problem but it looks like it is still just temporary solution. I
>> would expect that you can't unload module when this alarm is used. It if
>> it is not used then you should be able to remove it.
>>
> 
> Yes, the alarmtimer handling needs to be made more flexible, especially
> in the rtc device selection.

ok. What about to extend that commit message for the patch you sent to
explicitly say that this is temporary solution and what should be the
right fix?

Maybe you can also add link that patch which introduced this issue.
It was in that patch I sent.

Thanks,
Michal



-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Alexandre Belloni]

On 21/08/2017 at 14:19:07 +0200, Michal Simek wrote:
> On 21.8.2017 14:13, Alexandre Belloni wrote:
> > On 21/08/2017 at 12:50:34 +0200, Michal Simek wrote:
> >> Hi,
> >>
> >> On 21.8.2017 00:03, Alexandre Belloni wrote:
> >>> Hi Michal,
> >>>
> >>> I've just send a patch to fix this issue (and avoid your other patch).
> >>>
> >>> Could you test it? (I did test on an atmel platform)
> >>>
> >>> Sorry it took so long!
> >>
> >> not a problem but it looks like it is still just temporary solution. I
> >> would expect that you can't unload module when this alarm is used. It if
> >> it is not used then you should be able to remove it.
> >>
> > 
> > Yes, the alarmtimer handling needs to be made more flexible, especially
> > in the rtc device selection.
> 
> ok. What about to extend that commit message for the patch you sent to
> explicitly say that this is temporary solution and what should be the
> right fix?
> 
> Maybe you can also add link that patch which introduced this issue.
> It was in that patch I sent.
> 

You didn't pinpoint the correct patch. The patch introducing the issue
you were seeing (i.e. being able to remove the module) is
8bc0dafb5cf38a19484dfb16e2c6d29e85820046. Before this patch,
rtc_class_open() was called and this prevented the module from being
removed (it does try_module_get). For now, this is how the feature has
been implemented since 2011 and nobody cared enough to change that
behaviour.



-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

[-- Attachment #1.1: Type: text/plain, Size: 2359 bytes --]

On 21.8.2017 14:47, Alexandre Belloni wrote:
> On 21/08/2017 at 14:19:07 +0200, Michal Simek wrote:
>> On 21.8.2017 14:13, Alexandre Belloni wrote:
>>> On 21/08/2017 at 12:50:34 +0200, Michal Simek wrote:
>>>> Hi,
>>>>
>>>> On 21.8.2017 00:03, Alexandre Belloni wrote:
>>>>> Hi Michal,
>>>>>
>>>>> I've just send a patch to fix this issue (and avoid your other patch).
>>>>>
>>>>> Could you test it? (I did test on an atmel platform)
>>>>>
>>>>> Sorry it took so long!
>>>>
>>>> not a problem but it looks like it is still just temporary solution. I
>>>> would expect that you can't unload module when this alarm is used. It if
>>>> it is not used then you should be able to remove it.
>>>>
>>>
>>> Yes, the alarmtimer handling needs to be made more flexible, especially
>>> in the rtc device selection.
>>
>> ok. What about to extend that commit message for the patch you sent to
>> explicitly say that this is temporary solution and what should be the
>> right fix?
>>
>> Maybe you can also add link that patch which introduced this issue.
>> It was in that patch I sent.
>>
> 
> You didn't pinpoint the correct patch. The patch introducing the issue
> you were seeing (i.e. being able to remove the module) is
> 8bc0dafb5cf38a19484dfb16e2c6d29e85820046. Before this patch,
> rtc_class_open() was called and this prevented the module from being
> removed (it does try_module_get). For now, this is how the feature has
> been implemented since 2011 and nobody cared enough to change that
> behaviour.

ok but still isn't worth to mentioned it in commit message?

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs


-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [rtc-linux] RTC used as a module

[Alexandre Belloni]

On 21/08/2017 at 14:47:20 +0200, Alexandre Belloni wrote:
> On 21/08/2017 at 14:19:07 +0200, Michal Simek wrote:
> > On 21.8.2017 14:13, Alexandre Belloni wrote:
> > > On 21/08/2017 at 12:50:34 +0200, Michal Simek wrote:
> > >> Hi,
> > >>
> > >> On 21.8.2017 00:03, Alexandre Belloni wrote:
> > >>> Hi Michal,
> > >>>
> > >>> I've just send a patch to fix this issue (and avoid your other patch).
> > >>>
> > >>> Could you test it? (I did test on an atmel platform)
> > >>>
> > >>> Sorry it took so long!
> > >>
> > >> not a problem but it looks like it is still just temporary solution. I
> > >> would expect that you can't unload module when this alarm is used. It if
> > >> it is not used then you should be able to remove it.
> > >>
> > > 
> > > Yes, the alarmtimer handling needs to be made more flexible, especially
> > > in the rtc device selection.
> > 
> > ok. What about to extend that commit message for the patch you sent to
> > explicitly say that this is temporary solution and what should be the
> > right fix?
> > 
> > Maybe you can also add link that patch which introduced this issue.
> > It was in that patch I sent.
> > 
> 
> You didn't pinpoint the correct patch. The patch introducing the issue
> you were seeing (i.e. being able to remove the module) is
> 8bc0dafb5cf38a19484dfb16e2c6d29e85820046. Before this patch,
> rtc_class_open() was called and this prevented the module from being
> removed (it does try_module_get). For now, this is how the feature has
> been implemented since 2011 and nobody cared enough to change that
> behaviour.
> 

To be clear, I'm not saying this should not be done. I'm just try to
explain that this would not be a fix but rather a feature improvement.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Re: [rtc-linux] RTC used as a module

[Michal Simek]

On 21.8.2017 14:53, Alexandre Belloni wrote:
> On 21/08/2017 at 14:47:20 +0200, Alexandre Belloni wrote:
>> On 21/08/2017 at 14:19:07 +0200, Michal Simek wrote:
>>> On 21.8.2017 14:13, Alexandre Belloni wrote:
>>>> On 21/08/2017 at 12:50:34 +0200, Michal Simek wrote:
>>>>> Hi,
>>>>>
>>>>> On 21.8.2017 00:03, Alexandre Belloni wrote:
>>>>>> Hi Michal,
>>>>>>
>>>>>> I've just send a patch to fix this issue (and avoid your other patch).
>>>>>>
>>>>>> Could you test it? (I did test on an atmel platform)
>>>>>>
>>>>>> Sorry it took so long!
>>>>>
>>>>> not a problem but it looks like it is still just temporary solution. I
>>>>> would expect that you can't unload module when this alarm is used. It if
>>>>> it is not used then you should be able to remove it.
>>>>>
>>>>
>>>> Yes, the alarmtimer handling needs to be made more flexible, especially
>>>> in the rtc device selection.
>>>
>>> ok. What about to extend that commit message for the patch you sent to
>>> explicitly say that this is temporary solution and what should be the
>>> right fix?
>>>
>>> Maybe you can also add link that patch which introduced this issue.
>>> It was in that patch I sent.
>>>
>>
>> You didn't pinpoint the correct patch. The patch introducing the issue
>> you were seeing (i.e. being able to remove the module) is
>> 8bc0dafb5cf38a19484dfb16e2c6d29e85820046. Before this patch,
>> rtc_class_open() was called and this prevented the module from being
>> removed (it does try_module_get). For now, this is how the feature has
>> been implemented since 2011 and nobody cared enough to change that
>> behaviour.
>>
> 
> To be clear, I'm not saying this should not be done. I'm just try to
> explain that this would not be a fix but rather a feature improvement.


I am not quite sure I agree with this description. It doesn't mean that
none care about this from 2011 that this is correct behavior.
You know much more than I in this area but it seems to me quite normal
that if you have module and features which implement and you are not
using them that you can remove module.
It means your patch is a temporary solution for bug introduced in 2011
and found in 2016/2017.

Thanks,
Michal




-- 
You received this message because you are subscribed to "rtc-linux".
Membership options at http://groups.google.com/group/rtc-linux .
Please read http://groups.google.com/group/rtc-linux/web/checklist
before submitting a driver.
--- 
You received this message because you are subscribed to the Google Groups "rtc-linux" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rtc-linux+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.