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);
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.
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
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
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.o...
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.
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
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/
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.
linux-stable-mirror@lists.linaro.org
-
Alexandre Belloni -
Bryan O'Donoghue -
Shawn Guo -
Trent Piepho