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

[Michal Simek <michal.simek@xilinx.com>]

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.