[RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable

[RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable

[Bryan O'Donoghue]

commit 179a502f8c46 ("rtc: snvs: add Freescale rtc-snvs driver") introduces
the SNVS RTC driver with a function snvs_rtc_enable().

snvs_rtc_enable() can return an error on the enable path however this
driver does not currently trap that failure on the probe() path and
consequently if enabling the RTC fails we encounter a later error spinning
forever in rtc_write_sync_lp().

[   36.093481] [<c010d630>] (__irq_svc) from [<c0c2e9ec>] (_raw_spin_unlock_irqrestore+0x34/0x44)
[   36.102122] [<c0c2e9ec>] (_raw_spin_unlock_irqrestore) from [<c072e32c>] (regmap_read+0x4c/0x5c)
[   36.110938] [<c072e32c>] (regmap_read) from [<c085d0f4>] (rtc_write_sync_lp+0x6c/0x98)
[   36.118881] [<c085d0f4>] (rtc_write_sync_lp) from [<c085d160>] (snvs_rtc_alarm_irq_enable+0x40/0x4c)
[   36.128041] [<c085d160>] (snvs_rtc_alarm_irq_enable) from [<c08567b4>] (rtc_timer_do_work+0xd8/0x1a8)
[   36.137291] [<c08567b4>] (rtc_timer_do_work) from [<c01441b8>] (process_one_work+0x28c/0x76c)
[   36.145840] [<c01441b8>] (process_one_work) from [<c01446cc>] (worker_thread+0x34/0x58c)
[   36.153961] [<c01446cc>] (worker_thread) from [<c014aee4>] (kthread+0x138/0x150)
[   36.161388] [<c014aee4>] (kthread) from [<c0107e14>] (ret_from_fork+0x14/0x20)
[   36.168635] rcu_sched kthread starved for 2602 jiffies! g496 c495 f0x2 RCU_GP_WAIT_FQS(3) ->state=0x0 ->cpu=0
[   36.178564] rcu_sched       R  running task        0     8      2 0x00000000
[   36.185664] [<c0c288b0>] (__schedule) from [<c0c29134>] (schedule+0x3c/0xa0)
[   36.192739] [<c0c29134>] (schedule) from [<c0c2db80>] (schedule_timeout+0x78/0x4e0)
[   36.200422] [<c0c2db80>] (schedule_timeout) from [<c01a7ab0>] (rcu_gp_kthread+0x648/0x1864)
[   36.208800] [<c01a7ab0>] (rcu_gp_kthread) from [<c014aee4>] (kthread+0x138/0x150)
[   36.216309] [<c014aee4>] (kthread) from [<c0107e14>] (ret_from_fork+0x14/0x20)

This patch fixes by parsing the result of rtc_write_sync_lp() and
propagating both in the probe and elsewhere. If the RTC doesn't start we
don't proceed loading the driver and don't get into this loop mess later
on.

Fixes: 179a502f8c46 ("rtc: snvs: add Freescale rtc-snvs driver")

Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
Cc: a.zummo@towertech.it
Cc: alexandre.belloni@free-electrons.com
Cc: Pan Bian <bianpan2016@163.com>
Cc: Guy Shapiro <guy.shapiro@mobi-wize.com>
Cc: Stefan Agner <stefan@agner.ch>
Cc: Frank Li <Frank.Li@freescale.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: linux-rtc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: <stable@vger.kernel.org> # v3.16+
---
 drivers/rtc/rtc-snvs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/rtc/rtc-snvs.c b/drivers/rtc/rtc-snvs.c
index d8ef9e0..9af591d 100644
--- a/drivers/rtc/rtc-snvs.c
+++ b/drivers/rtc/rtc-snvs.c
@@ -132,20 +132,23 @@ static int snvs_rtc_set_time(struct device *dev, struct rtc_time *tm)
 {
 	struct snvs_rtc_data *data = dev_get_drvdata(dev);
 	unsigned long time;
+	int ret;
 
 	rtc_tm_to_time(tm, &time);
 
 	/* Disable RTC first */
-	snvs_rtc_enable(data, false);
+	ret = snvs_rtc_enable(data, false);
+	if (ret)
+		return ret;
 
 	/* Write 32-bit time to 47-bit timer, leaving 15 LSBs blank */
 	regmap_write(data->regmap, data->offset + SNVS_LPSRTCLR, time << CNTR_TO_SECS_SH);
 	regmap_write(data->regmap, data->offset + SNVS_LPSRTCMR, time >> (32 - CNTR_TO_SECS_SH));
 
 	/* Enable RTC again */
-	snvs_rtc_enable(data, true);
+	ret = snvs_rtc_enable(data, true);
 
-	return 0;
+	return ret;
 }
 
 static int snvs_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
@@ -288,7 +291,11 @@ static int snvs_rtc_probe(struct platform_device *pdev)
 	regmap_write(data->regmap, data->offset + SNVS_LPSR, 0xffffffff);
 
 	/* Enable RTC */
-	snvs_rtc_enable(data, true);
+	ret = snvs_rtc_enable(data, true);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to enable rtc %d\n", ret);
+		goto error_rtc_device_register;
+	}
 
 	device_init_wakeup(&pdev->dev, true);
 
-- 
2.7.4

Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable

[Trent Piepho]

On Wed, 2018-03-28 at 20:14 +0100, Bryan O'Donoghue wrote:
> commit 179a502f8c46 ("rtc: snvs: add Freescale rtc-snvs driver") introduces
> the SNVS RTC driver with a function snvs_rtc_enable().
> 
> snvs_rtc_enable() can return an error on the enable path however this
> driver does not currently trap that failure on the probe() path and
> consequently if enabling the RTC fails we encounter a later error spinning
> forever in rtc_write_sync_lp().
> 
> This patch fixes by parsing the result of rtc_write_sync_lp() and
> propagating both in the probe and elsewhere. If the RTC doesn't start we
> don't proceed loading the driver and don't get into this loop mess later
> on.
> 

I sent a patch a couple weeks ago that addressed a similar issue, see
https://patchwork.ozlabs.org/patch/887090/

Does this also fix the problem you see?

I think what you've done will prevent the driver from loading if the
clock is not working, but if the clock does not tick properly after
loading, there are still points (snvs_rtc_read_time for one) that will
lock up in the kernel.

Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable

[Bryan O'Donoghue]

On 29/03/18 01:12, Trent Piepho wrote:

> I sent a patch a couple weeks ago that addressed a similar issue, see
> https://patchwork.ozlabs.org/patch/887090/
> 
> Does this also fix the problem you see?

It breaks the endless loop that happens later on in the RTC read path.

This patch though is about fixing the bug with not handling the enable 
of the snvs_rtc_enable() correctly, which is why it should be applied.

The right flow is to handle the error on snvs_rtc_enable() as soon as it 
happens as opposed wait for read() to -ETIMEDOUT.

> I think what you've done will prevent the driver from loading if the
> clock is not working, but if the clock does not tick properly after
> loading, there are still points (snvs_rtc_read_time for one) that will
> lock up in the kernel.

I'll dig out your patch and give it a review.

---
bod

Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable

[Trent Piepho]

On Thu, 2018-03-29 at 02:53 +0100, Bryan O'Donoghue wrote:
> On 29/03/18 01:12, Trent Piepho wrote:
> 
> > I sent a patch a couple weeks ago that addressed a similar issue, see
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F887090%2F&data=02%7C01%7Ctpiepho%40impinj.com%7Cb622940802df49deac4c08d59517e8d8%7C6de70f0f73574529a415d8cbb7e93e5e%7C0%7C1%7C636578852283390745&sdata=ds0ofpbTqrHBjGA18N343cJJ9sy2STQ9ZzSxH2TUyzE%3D&reserved=0
> > 
> > Does this also fix the problem you see?
> 
> It breaks the endless loop that happens later on in the RTC read path.
> 
> This patch though is about fixing the bug with not handling the enable 
> of the snvs_rtc_enable() correctly, which is why it should be applied.
> 
> The right flow is to handle the error on snvs_rtc_enable() as soon as it 
> happens as opposed wait for read() to -ETIMEDOUT.

Unless there are boards that don't enable the 32kHz oscillator until
after the kernel driver loads.  I was concerned about that so didn't
add the check in probe to prevent the driver from loading.  If the
possible disruption of that is acceptable, then it does seem better to
fail to load.

However, I think that even if the driver fails to probe if there is a
timeout at probe time, it's still possible to hang later if there are
not limits to the hardware polling loops, such as the ones I added.

> > I think what you've done will prevent the driver from loading if the
> > clock is not working, but if the clock does not tick properly after
> > loading, there are still points (snvs_rtc_read_time for one) that will
> > lock up in the kernel.
> 
> I'll dig out your patch and give it a review.
> 

Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable

[Bryan O'Donoghue]

On 30/03/18 23:59, Trent Piepho wrote:
> However, I think that even if the driver fails to probe if there is a
> timeout at probe time, it's still possible to hang later if there are
> not limits to the hardware polling loops, such as the ones I added.

I agree with your patch in principle for this the reason you've outlined.

My patch though should still be applied to fix non-starting @ source.

/aside - I don't have your patch in my inbox - if you could resend and 
cc me I'll review it for you.

---
bod

Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable

[Alexandre Belloni]

Hi,

On 02/04/2018 at 23:51:12 +0100, Bryan O'Donoghue wrote:
> On 30/03/18 23:59, Trent Piepho wrote:
> > However, I think that even if the driver fails to probe if there is a
> > timeout at probe time, it's still possible to hang later if there are
> > not limits to the hardware polling loops, such as the ones I added.
> 
> I agree with your patch in principle for this the reason you've outlined.
> 
> My patch though should still be applied to fix non-starting @ source.
> 
> /aside - I don't have your patch in my inbox - if you could resend and cc me
> I'll review it for you.
> 

It is available in mbox format here:
http://patchwork.ozlabs.org/patch/887090/mbox/

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable

[Shawn Guo]

On Wed, Mar 28, 2018 at 08:14:05PM +0100, Bryan O'Donoghue wrote:
> commit 179a502f8c46 ("rtc: snvs: add Freescale rtc-snvs driver") introduces
> the SNVS RTC driver with a function snvs_rtc_enable().
> 
> snvs_rtc_enable() can return an error on the enable path however this
> driver does not currently trap that failure on the probe() path and
> consequently if enabling the RTC fails we encounter a later error spinning
> forever in rtc_write_sync_lp().
> 
> [   36.093481] [<c010d630>] (__irq_svc) from [<c0c2e9ec>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> [   36.102122] [<c0c2e9ec>] (_raw_spin_unlock_irqrestore) from [<c072e32c>] (regmap_read+0x4c/0x5c)
> [   36.110938] [<c072e32c>] (regmap_read) from [<c085d0f4>] (rtc_write_sync_lp+0x6c/0x98)
> [   36.118881] [<c085d0f4>] (rtc_write_sync_lp) from [<c085d160>] (snvs_rtc_alarm_irq_enable+0x40/0x4c)
> [   36.128041] [<c085d160>] (snvs_rtc_alarm_irq_enable) from [<c08567b4>] (rtc_timer_do_work+0xd8/0x1a8)
> [   36.137291] [<c08567b4>] (rtc_timer_do_work) from [<c01441b8>] (process_one_work+0x28c/0x76c)
> [   36.145840] [<c01441b8>] (process_one_work) from [<c01446cc>] (worker_thread+0x34/0x58c)
> [   36.153961] [<c01446cc>] (worker_thread) from [<c014aee4>] (kthread+0x138/0x150)
> [   36.161388] [<c014aee4>] (kthread) from [<c0107e14>] (ret_from_fork+0x14/0x20)
> [   36.168635] rcu_sched kthread starved for 2602 jiffies! g496 c495 f0x2 RCU_GP_WAIT_FQS(3) ->state=0x0 ->cpu=0
> [   36.178564] rcu_sched       R  running task        0     8      2 0x00000000
> [   36.185664] [<c0c288b0>] (__schedule) from [<c0c29134>] (schedule+0x3c/0xa0)
> [   36.192739] [<c0c29134>] (schedule) from [<c0c2db80>] (schedule_timeout+0x78/0x4e0)
> [   36.200422] [<c0c2db80>] (schedule_timeout) from [<c01a7ab0>] (rcu_gp_kthread+0x648/0x1864)
> [   36.208800] [<c01a7ab0>] (rcu_gp_kthread) from [<c014aee4>] (kthread+0x138/0x150)
> [   36.216309] [<c014aee4>] (kthread) from [<c0107e14>] (ret_from_fork+0x14/0x20)
> 
> This patch fixes by parsing the result of rtc_write_sync_lp() and
> propagating both in the probe and elsewhere. If the RTC doesn't start we
> don't proceed loading the driver and don't get into this loop mess later
> on.
> 
> Fixes: 179a502f8c46 ("rtc: snvs: add Freescale rtc-snvs driver")
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> Cc: a.zummo@towertech.it
> Cc: alexandre.belloni@free-electrons.com
> Cc: Pan Bian <bianpan2016@163.com>
> Cc: Guy Shapiro <guy.shapiro@mobi-wize.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Frank Li <Frank.Li@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>

Acked-by: Shawn Guo <shawn.guo@linaro.org>

Re: [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable

[Alexandre Belloni]

On 28/03/2018 20:14:05+0100, Bryan O'Donoghue wrote:
> commit 179a502f8c46 ("rtc: snvs: add Freescale rtc-snvs driver") introduces
> the SNVS RTC driver with a function snvs_rtc_enable().
> 
> snvs_rtc_enable() can return an error on the enable path however this
> driver does not currently trap that failure on the probe() path and
> consequently if enabling the RTC fails we encounter a later error spinning
> forever in rtc_write_sync_lp().
> 
> [   36.093481] [<c010d630>] (__irq_svc) from [<c0c2e9ec>] (_raw_spin_unlock_irqrestore+0x34/0x44)
> [   36.102122] [<c0c2e9ec>] (_raw_spin_unlock_irqrestore) from [<c072e32c>] (regmap_read+0x4c/0x5c)
> [   36.110938] [<c072e32c>] (regmap_read) from [<c085d0f4>] (rtc_write_sync_lp+0x6c/0x98)
> [   36.118881] [<c085d0f4>] (rtc_write_sync_lp) from [<c085d160>] (snvs_rtc_alarm_irq_enable+0x40/0x4c)
> [   36.128041] [<c085d160>] (snvs_rtc_alarm_irq_enable) from [<c08567b4>] (rtc_timer_do_work+0xd8/0x1a8)
> [   36.137291] [<c08567b4>] (rtc_timer_do_work) from [<c01441b8>] (process_one_work+0x28c/0x76c)
> [   36.145840] [<c01441b8>] (process_one_work) from [<c01446cc>] (worker_thread+0x34/0x58c)
> [   36.153961] [<c01446cc>] (worker_thread) from [<c014aee4>] (kthread+0x138/0x150)
> [   36.161388] [<c014aee4>] (kthread) from [<c0107e14>] (ret_from_fork+0x14/0x20)
> [   36.168635] rcu_sched kthread starved for 2602 jiffies! g496 c495 f0x2 RCU_GP_WAIT_FQS(3) ->state=0x0 ->cpu=0
> [   36.178564] rcu_sched       R  running task        0     8      2 0x00000000
> [   36.185664] [<c0c288b0>] (__schedule) from [<c0c29134>] (schedule+0x3c/0xa0)
> [   36.192739] [<c0c29134>] (schedule) from [<c0c2db80>] (schedule_timeout+0x78/0x4e0)
> [   36.200422] [<c0c2db80>] (schedule_timeout) from [<c01a7ab0>] (rcu_gp_kthread+0x648/0x1864)
> [   36.208800] [<c01a7ab0>] (rcu_gp_kthread) from [<c014aee4>] (kthread+0x138/0x150)
> [   36.216309] [<c014aee4>] (kthread) from [<c0107e14>] (ret_from_fork+0x14/0x20)
> 
> This patch fixes by parsing the result of rtc_write_sync_lp() and
> propagating both in the probe and elsewhere. If the RTC doesn't start we
> don't proceed loading the driver and don't get into this loop mess later
> on.
> 
> Fixes: 179a502f8c46 ("rtc: snvs: add Freescale rtc-snvs driver")
> 
> Signed-off-by: Bryan O'Donoghue <pure.logic@nexus-software.ie>
> Cc: a.zummo@towertech.it
> Cc: alexandre.belloni@free-electrons.com
> Cc: Pan Bian <bianpan2016@163.com>
> Cc: Guy Shapiro <guy.shapiro@mobi-wize.com>
> Cc: Stefan Agner <stefan@agner.ch>
> Cc: Frank Li <Frank.Li@freescale.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> Cc: linux-rtc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: <stable@vger.kernel.org> # v3.16+
> ---
>  drivers/rtc/rtc-snvs.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
Applied, thanks.

-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com