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

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

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.