On Fri, Mar 3, 2017 at 6:46 AM, Michal Simek <mon...@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+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
