Re: rtc ds3232 call trace in kernel

Re: rtc ds3232 call trace in kernel

[Akinobu Mita]

2016-04-18 15:15 GMT+09:00 Qianyu Gong <qianyu.gong@nxp.com>:
> Hi Akinobu,
>
>
>
> I got an rtc call trace when booting 4.6 kernel on our board and I found it
>
> was caused by this patch:
>
>
>
> commit fc1dcb0b39dbb10d3290f2fcd6e154670f699166
>
> Author: Akinobu Mita <akinobu.mita@gmail.com>
>
> Date:   Mon Mar 7 00:27:53 2016 +0900
>
>
>
>     rtc: ds3232: use rtc->ops_lock to protect alarm operations
>
>
>
>     ds3232->mutex is used to protect for alarm operations which
>
>     need to access status and control registers.
>
>
>
>     But we can use rtc->ops_lock instead.  rtc->ops_lock is held when most
>
>     of rtc_class_ops methods are called, so we only need to explicitly
>
>     acquire it from irq handler in order to protect form concurrent
>
>     accesses.
>
>
>
>     Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com
>
>
>
> The problem is that rtc->ops_lock would be accessed in ds3232_irq()
>
> without being initialized as rtc_device_register() is called too late.

You have already identified the root cause of this issue.

> As I’m not familiar with rtc things, could I just revert the patch or you
> already
>
> have a solution to this problem? Thanks in advance.

Could you check if the problem is resolved by moving the call of
devm_rtc_device_register() from the end of ds3232_probe() to just
before registering irq handler?

Re: rtc ds3232 call trace in kernel

[Alexandre Belloni]

On 18/04/2016 at 06:15:40 +0000, Qianyu Gong wrote :
> Hi Akinobu,
> 
> I got an rtc call trace when booting 4.6 kernel on our board and I found it
> was caused by this patch:
> 
> commit fc1dcb0b39dbb10d3290f2fcd6e154670f699166
> Author: Akinobu Mita <akinobu.mita@gmail.com>
> Date:   Mon Mar 7 00:27:53 2016 +0900
> 
>     rtc: ds3232: use rtc->ops_lock to protect alarm operations
> 
>     ds3232->mutex is used to protect for alarm operations which
>     need to access status and control registers.
> 
>     But we can use rtc->ops_lock instead.  rtc->ops_lock is held when most
>     of rtc_class_ops methods are called, so we only need to explicitly
>     acquire it from irq handler in order to protect form concurrent
>     accesses.
> 
>     Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com
> 
> The problem is that rtc->ops_lock would be accessed in ds3232_irq()
> without being initialized as rtc_device_register() is called too late.
> 
> As I'm not familiar with rtc things, could I just revert the patch or you already
> have a solution to this problem? Thanks in advance.
> 
> 

Well, many RTC drivers actually suffer from that. I'm planning to change
the infrastructure to handle this case better.
Meanwhile, this is worth fixing.


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

RE: rtc ds3232 call trace in kernel

[Qianyu Gong]

> -----Original Message-----
> From: Akinobu Mita [mailto:akinobu.mita@gmail.com]
> Sent: Monday, April 18, 2016 9:02 PM
> To: Qianyu Gong <qianyu.gong@nxp.com>
> Cc: alexandre.belloni@free-electrons.com; Mingkai Hu <mingkai.hu@nxp.com>;
> rtc-linux@googlegroups.com; linux-kernel@vger.kernel.org
> Subject: Re: rtc ds3232 call trace in kernel
> 
> 2016-04-18 15:15 GMT+09:00 Qianyu Gong <qianyu.gong@nxp.com>:
> > Hi Akinobu,
> >
> >
> >
> > I got an rtc call trace when booting 4.6 kernel on our board and I
> > found it
> >
> > was caused by this patch:
> >
> >
> >
> > commit fc1dcb0b39dbb10d3290f2fcd6e154670f699166
> >
> > Author: Akinobu Mita <akinobu.mita@gmail.com>
> >
> > Date:   Mon Mar 7 00:27:53 2016 +0900
> >
> >
> >
> >     rtc: ds3232: use rtc->ops_lock to protect alarm operations
> >
> >
> >
> >     ds3232->mutex is used to protect for alarm operations which
> >
> >     need to access status and control registers.
> >
> >
> >
> >     But we can use rtc->ops_lock instead.  rtc->ops_lock is held when
> > most
> >
> >     of rtc_class_ops methods are called, so we only need to explicitly
> >
> >     acquire it from irq handler in order to protect form concurrent
> >
> >     accesses.
> >
> >
> >
> >     Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com
> >
> >
> >
> > The problem is that rtc->ops_lock would be accessed in ds3232_irq()
> >
> > without being initialized as rtc_device_register() is called too late.
> 
> You have already identified the root cause of this issue.
> 
> > As I’m not familiar with rtc things, could I just revert the patch or
> > you already
> >
> > have a solution to this problem? Thanks in advance.
> 
> Could you check if the problem is resolved by moving the call of
> devm_rtc_device_register() from the end of ds3232_probe() to just before
> registering irq handler?

Yes. It works(no call trace now). 

Regards,
Qianyu

RE: rtc ds3232 call trace in kernel

[Qianyu Gong]

> -----Original Message-----
> From: Alexandre Belloni [mailto:alexandre.belloni@free-electrons.com]
> Sent: Tuesday, April 19, 2016 12:22 AM
> To: Qianyu Gong <qianyu.gong@nxp.com>
> Cc: akinobu.mita@gmail.com; Mingkai Hu <mingkai.hu@nxp.com>; rtc-
> linux@googlegroups.com; linux-kernel@vger.kernel.org
> Subject: Re: rtc ds3232 call trace in kernel
> 
> On 18/04/2016 at 06:15:40 +0000, Qianyu Gong wrote :
> > Hi Akinobu,
> >
> > I got an rtc call trace when booting 4.6 kernel on our board and I
> > found it was caused by this patch:
> >
> > commit fc1dcb0b39dbb10d3290f2fcd6e154670f699166
> > Author: Akinobu Mita <akinobu.mita@gmail.com>
> > Date:   Mon Mar 7 00:27:53 2016 +0900
> >
> >     rtc: ds3232: use rtc->ops_lock to protect alarm operations
> >
> >     ds3232->mutex is used to protect for alarm operations which
> >     need to access status and control registers.
> >
> >     But we can use rtc->ops_lock instead.  rtc->ops_lock is held when most
> >     of rtc_class_ops methods are called, so we only need to explicitly
> >     acquire it from irq handler in order to protect form concurrent
> >     accesses.
> >
> >     Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com
> >
> > The problem is that rtc->ops_lock would be accessed in ds3232_irq()
> > without being initialized as rtc_device_register() is called too late.
> >
> > As I'm not familiar with rtc things, could I just revert the patch or
> > you already have a solution to this problem? Thanks in advance.
> >
> >
> 
> Well, many RTC drivers actually suffer from that. I'm planning to change the
> infrastructure to handle this case better.
> Meanwhile, this is worth fixing.
> 
> 
> --
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering http://free-electrons.com

Thanks. I followed Akinobu's suggest and put devm_rtc_device_register() just
before registering the irq handler. The call trace disappeared. Looks like it's 
working well.


Regards,
Qianyu

Re: rtc ds3232 call trace in kernel

[Akinobu Mita]

2016-04-19 11:36 GMT+09:00 Qianyu Gong <qianyu.gong@nxp.com>:
>
>> -----Original Message-----
>> From: Akinobu Mita [mailto:akinobu.mita@gmail.com]
>> Sent: Monday, April 18, 2016 9:02 PM
>> To: Qianyu Gong <qianyu.gong@nxp.com>
>> Cc: alexandre.belloni@free-electrons.com; Mingkai Hu <mingkai.hu@nxp.com>;
>> rtc-linux@googlegroups.com; linux-kernel@vger.kernel.org
>> Subject: Re: rtc ds3232 call trace in kernel
>>
>> 2016-04-18 15:15 GMT+09:00 Qianyu Gong <qianyu.gong@nxp.com>:
>> > Hi Akinobu,
>> >
>> >
>> >
>> > I got an rtc call trace when booting 4.6 kernel on our board and I
>> > found it
>> >
>> > was caused by this patch:
>> >
>> >
>> >
>> > commit fc1dcb0b39dbb10d3290f2fcd6e154670f699166
>> >
>> > Author: Akinobu Mita <akinobu.mita@gmail.com>
>> >
>> > Date:   Mon Mar 7 00:27:53 2016 +0900
>> >
>> >
>> >
>> >     rtc: ds3232: use rtc->ops_lock to protect alarm operations
>> >
>> >
>> >
>> >     ds3232->mutex is used to protect for alarm operations which
>> >
>> >     need to access status and control registers.
>> >
>> >
>> >
>> >     But we can use rtc->ops_lock instead.  rtc->ops_lock is held when
>> > most
>> >
>> >     of rtc_class_ops methods are called, so we only need to explicitly
>> >
>> >     acquire it from irq handler in order to protect form concurrent
>> >
>> >     accesses.
>> >
>> >
>> >
>> >     Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> >
>> > Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com
>> >
>> >
>> >
>> > The problem is that rtc->ops_lock would be accessed in ds3232_irq()
>> >
>> > without being initialized as rtc_device_register() is called too late.
>>
>> You have already identified the root cause of this issue.
>>
>> > As I’m not familiar with rtc things, could I just revert the patch or
>> > you already
>> >
>> > have a solution to this problem? Thanks in advance.
>>
>> Could you check if the problem is resolved by moving the call of
>> devm_rtc_device_register() from the end of ds3232_probe() to just before
>> registering irq handler?
>
> Yes. It works(no call trace now).

Thanks for testing.  Do you mind submitting your patch to
rtc-linux@googlegroups.com ?

RE: rtc ds3232 call trace in kernel

[Qianyu Gong]

> -----Original Message-----
> From: Akinobu Mita [mailto:akinobu.mita@gmail.com]
> Sent: Tuesday, April 19, 2016 11:58 AM
> To: Qianyu Gong <qianyu.gong@nxp.com>
> Cc: alexandre.belloni@free-electrons.com; Mingkai Hu <mingkai.hu@nxp.com>;
> rtc-linux@googlegroups.com; linux-kernel@vger.kernel.org
> Subject: Re: rtc ds3232 call trace in kernel
> 
> 2016-04-19 11:36 GMT+09:00 Qianyu Gong <qianyu.gong@nxp.com>:
> >
> >> -----Original Message-----
> >> From: Akinobu Mita [mailto:akinobu.mita@gmail.com]
> >> Sent: Monday, April 18, 2016 9:02 PM
> >> To: Qianyu Gong <qianyu.gong@nxp.com>
> >> Cc: alexandre.belloni@free-electrons.com; Mingkai Hu
> >> <mingkai.hu@nxp.com>; rtc-linux@googlegroups.com;
> >> linux-kernel@vger.kernel.org
> >> Subject: Re: rtc ds3232 call trace in kernel
> >>
> >> 2016-04-18 15:15 GMT+09:00 Qianyu Gong <qianyu.gong@nxp.com>:
> >> > Hi Akinobu,
> >> >
> >> >
> >> >
> >> > I got an rtc call trace when booting 4.6 kernel on our board and I
> >> > found it
> >> >
> >> > was caused by this patch:
> >> >
> >> >
> >> >
> >> > commit fc1dcb0b39dbb10d3290f2fcd6e154670f699166
> >> >
> >> > Author: Akinobu Mita <akinobu.mita@gmail.com>
> >> >
> >> > Date:   Mon Mar 7 00:27:53 2016 +0900
> >> >
> >> >
> >> >
> >> >     rtc: ds3232: use rtc->ops_lock to protect alarm operations
> >> >
> >> >
> >> >
> >> >     ds3232->mutex is used to protect for alarm operations which
> >> >
> >> >     need to access status and control registers.
> >> >
> >> >
> >> >
> >> >     But we can use rtc->ops_lock instead.  rtc->ops_lock is held
> >> > when most
> >> >
> >> >     of rtc_class_ops methods are called, so we only need to
> >> > explicitly
> >> >
> >> >     acquire it from irq handler in order to protect form concurrent
> >> >
> >> >     accesses.
> >> >
> >> >
> >> >
> >> >     Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> >
> >> > Signed-off-by: Alexandre Belloni
> >> > <alexandre.belloni@free-electrons.com
> >> >
> >> >
> >> >
> >> > The problem is that rtc->ops_lock would be accessed in ds3232_irq()
> >> >
> >> > without being initialized as rtc_device_register() is called too late.
> >>
> >> You have already identified the root cause of this issue.
> >>
> >> > As I’m not familiar with rtc things, could I just revert the patch
> >> > or you already
> >> >
> >> > have a solution to this problem? Thanks in advance.
> >>
> >> Could you check if the problem is resolved by moving the call of
> >> devm_rtc_device_register() from the end of ds3232_probe() to just
> >> before registering irq handler?
> >
> > Yes. It works(no call trace now).
> 
> Thanks for testing.  Do you mind submitting your patch to rtc-
> linux@googlegroups.com ?

No problem.:)

Regards,
Qianyu