All of lore.kernel.org
 help / color / mirror / Atom feed
* [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable
@ 2018-03-28 19:14 Bryan O'Donoghue
  2018-03-29  0:12   ` Trent Piepho
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Bryan O'Donoghue @ 2018-03-28 19:14 UTC (permalink / raw)
  To: alexandre.belloni, linux-kernel
  Cc: Bryan O'Donoghue, a.zummo, Pan Bian, Guy Shapiro,
	Stefan Agner, Frank Li, Shawn Guo, linux-rtc, # v3 . 16+

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

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable
  2018-03-28 19:14 [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable Bryan O'Donoghue
@ 2018-03-29  0:12   ` Trent Piepho
  2018-03-29  2:18 ` [RESEND] [PATCH] " Shawn Guo
  2018-04-03 14:56 ` Alexandre Belloni
  2 siblings, 0 replies; 10+ messages in thread
From: Trent Piepho @ 2018-03-29  0:12 UTC (permalink / raw)
  To: linux-kernel@vger.kernel.org,
	alexandre.belloni@free-electrons.com,
	pure.logic@nexus-software.ie
  Cc: shawn.guo@linaro.org, stefan@agner.ch, bianpan2016@163.com,
	a.zummo@towertech.it, stable@vger.kernel.org,
	guy.shapiro@mobi-wize.com, Frank.Li@freescale.com,
	linux-rtc@vger.kernel.org

T24gV2VkLCAyMDE4LTAzLTI4IGF0IDIwOjE0ICswMTAwLCBCcnlhbiBPJ0Rvbm9naHVlIHdyb3Rl
Og0KPiBjb21taXQgMTc5YTUwMmY4YzQ2ICgicnRjOiBzbnZzOiBhZGQgRnJlZXNjYWxlIHJ0Yy1z
bnZzIGRyaXZlciIpIGludHJvZHVjZXMNCj4gdGhlIFNOVlMgUlRDIGRyaXZlciB3aXRoIGEgZnVu
Y3Rpb24gc252c19ydGNfZW5hYmxlKCkuDQo+IA0KPiBzbnZzX3J0Y19lbmFibGUoKSBjYW4gcmV0
dXJuIGFuIGVycm9yIG9uIHRoZSBlbmFibGUgcGF0aCBob3dldmVyIHRoaXMNCj4gZHJpdmVyIGRv
ZXMgbm90IGN1cnJlbnRseSB0cmFwIHRoYXQgZmFpbHVyZSBvbiB0aGUgcHJvYmUoKSBwYXRoIGFu
ZA0KPiBjb25zZXF1ZW50bHkgaWYgZW5hYmxpbmcgdGhlIFJUQyBmYWlscyB3ZSBlbmNvdW50ZXIg
YSBsYXRlciBlcnJvciBzcGlubmluZw0KPiBmb3JldmVyIGluIHJ0Y193cml0ZV9zeW5jX2xwKCku
DQo+IA0KPiBUaGlzIHBhdGNoIGZpeGVzIGJ5IHBhcnNpbmcgdGhlIHJlc3VsdCBvZiBydGNfd3Jp
dGVfc3luY19scCgpIGFuZA0KPiBwcm9wYWdhdGluZyBib3RoIGluIHRoZSBwcm9iZSBhbmQgZWxz
ZXdoZXJlLiBJZiB0aGUgUlRDIGRvZXNuJ3Qgc3RhcnQgd2UNCj4gZG9uJ3QgcHJvY2VlZCBsb2Fk
aW5nIHRoZSBkcml2ZXIgYW5kIGRvbid0IGdldCBpbnRvIHRoaXMgbG9vcCBtZXNzIGxhdGVyDQo+
IG9uLg0KPiANCg0KSSBzZW50IGEgcGF0Y2ggYSBjb3VwbGUgd2Vla3MgYWdvIHRoYXQgYWRkcmVz
c2VkIGEgc2ltaWxhciBpc3N1ZSwgc2VlDQpodHRwczovL3BhdGNod29yay5vemxhYnMub3JnL3Bh
dGNoLzg4NzA5MC8NCg0KRG9lcyB0aGlzIGFsc28gZml4IHRoZSBwcm9ibGVtIHlvdSBzZWU/DQoN
CkkgdGhpbmsgd2hhdCB5b3UndmUgZG9uZSB3aWxsIHByZXZlbnQgdGhlIGRyaXZlciBmcm9tIGxv
YWRpbmcgaWYgdGhlDQpjbG9jayBpcyBub3Qgd29ya2luZywgYnV0IGlmIHRoZSBjbG9jayBkb2Vz
IG5vdCB0aWNrIHByb3Blcmx5IGFmdGVyDQpsb2FkaW5nLCB0aGVyZSBhcmUgc3RpbGwgcG9pbnRz
IChzbnZzX3J0Y19yZWFkX3RpbWUgZm9yIG9uZSkgdGhhdCB3aWxsDQpsb2NrIHVwIGluIHRoZSBr
ZXJuZWwu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable
@ 2018-03-29  0:12   ` Trent Piepho
  0 siblings, 0 replies; 10+ messages in thread
From: Trent Piepho @ 2018-03-29  0:12 UTC (permalink / raw)
  To: linux-kernel@vger.kernel.org,
	alexandre.belloni@free-electrons.com,
	pure.logic@nexus-software.ie
  Cc: shawn.guo@linaro.org, stefan@agner.ch, bianpan2016@163.com,
	a.zummo@towertech.it, stable@vger.kernel.org,
	guy.shapiro@mobi-wize.com, Frank.Li@freescale.com,
	linux-rtc@vger.kernel.org

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable
  2018-03-29  0:12   ` Trent Piepho
  (?)
@ 2018-03-29  1:53   ` Bryan O'Donoghue
  2018-03-30 22:59       ` Trent Piepho
  -1 siblings, 1 reply; 10+ messages in thread
From: Bryan O'Donoghue @ 2018-03-29  1:53 UTC (permalink / raw)
  To: Trent Piepho, linux-kernel@vger.kernel.org,
	alexandre.belloni@free-electrons.com
  Cc: shawn.guo@linaro.org, stefan@agner.ch, bianpan2016@163.com,
	a.zummo@towertech.it, stable@vger.kernel.org,
	guy.shapiro@mobi-wize.com, Frank.Li@freescale.com,
	linux-rtc@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable
  2018-03-28 19:14 [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable Bryan O'Donoghue
  2018-03-29  0:12   ` Trent Piepho
@ 2018-03-29  2:18 ` Shawn Guo
  2018-04-03 14:56 ` Alexandre Belloni
  2 siblings, 0 replies; 10+ messages in thread
From: Shawn Guo @ 2018-03-29  2:18 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: alexandre.belloni, linux-kernel, a.zummo, Pan Bian, Guy Shapiro,
	Stefan Agner, Frank Li, linux-rtc, # v3 . 16+

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>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable
  2018-03-29  1:53   ` Bryan O'Donoghue
@ 2018-03-30 22:59       ` Trent Piepho
  0 siblings, 0 replies; 10+ messages in thread
From: Trent Piepho @ 2018-03-30 22:59 UTC (permalink / raw)
  To: linux-kernel@vger.kernel.org,
	alexandre.belloni@free-electrons.com,
	pure.logic@nexus-software.ie
  Cc: shawn.guo@linaro.org, stefan@agner.ch, bianpan2016@163.com,
	a.zummo@towertech.it, stable@vger.kernel.org,
	guy.shapiro@mobi-wize.com, Frank.Li@freescale.com,
	linux-rtc@vger.kernel.org

T24gVGh1LCAyMDE4LTAzLTI5IGF0IDAyOjUzICswMTAwLCBCcnlhbiBPJ0Rvbm9naHVlIHdyb3Rl
Og0KPiBPbiAyOS8wMy8xOCAwMToxMiwgVHJlbnQgUGllcGhvIHdyb3RlOg0KPiANCj4gPiBJIHNl
bnQgYSBwYXRjaCBhIGNvdXBsZSB3ZWVrcyBhZ28gdGhhdCBhZGRyZXNzZWQgYSBzaW1pbGFyIGlz
c3VlLCBzZWUNCj4gPiBodHRwczovL25hMDEuc2FmZWxpbmtzLnByb3RlY3Rpb24ub3V0bG9vay5j
b20vP3VybD1odHRwcyUzQSUyRiUyRnBhdGNod29yay5vemxhYnMub3JnJTJGcGF0Y2glMkY4ODcw
OTAlMkYmZGF0YT0wMiU3QzAxJTdDdHBpZXBobyU0MGltcGluai5jb20lN0NiNjIyOTQwODAyZGY0
OWRlYWM0YzA4ZDU5NTE3ZThkOCU3QzZkZTcwZjBmNzM1NzQ1MjlhNDE1ZDhjYmI3ZTkzZTVlJTdD
MCU3QzElN0M2MzY1Nzg4NTIyODMzOTA3NDUmc2RhdGE9ZHMwb2ZwYlRxckhCakdBMThOMzQzY0pK
OXN5MlNUUTlaelN4SDJUVXl6RSUzRCZyZXNlcnZlZD0wDQo+ID4gDQo+ID4gRG9lcyB0aGlzIGFs
c28gZml4IHRoZSBwcm9ibGVtIHlvdSBzZWU/DQo+IA0KPiBJdCBicmVha3MgdGhlIGVuZGxlc3Mg
bG9vcCB0aGF0IGhhcHBlbnMgbGF0ZXIgb24gaW4gdGhlIFJUQyByZWFkIHBhdGguDQo+IA0KPiBU
aGlzIHBhdGNoIHRob3VnaCBpcyBhYm91dCBmaXhpbmcgdGhlIGJ1ZyB3aXRoIG5vdCBoYW5kbGlu
ZyB0aGUgZW5hYmxlIA0KPiBvZiB0aGUgc252c19ydGNfZW5hYmxlKCkgY29ycmVjdGx5LCB3aGlj
aCBpcyB3aHkgaXQgc2hvdWxkIGJlIGFwcGxpZWQuDQo+IA0KPiBUaGUgcmlnaHQgZmxvdyBpcyB0
byBoYW5kbGUgdGhlIGVycm9yIG9uIHNudnNfcnRjX2VuYWJsZSgpIGFzIHNvb24gYXMgaXQgDQo+
IGhhcHBlbnMgYXMgb3Bwb3NlZCB3YWl0IGZvciByZWFkKCkgdG8gLUVUSU1FRE9VVC4NCg0KVW5s
ZXNzIHRoZXJlIGFyZSBib2FyZHMgdGhhdCBkb24ndCBlbmFibGUgdGhlIDMya0h6IG9zY2lsbGF0
b3IgdW50aWwNCmFmdGVyIHRoZSBrZXJuZWwgZHJpdmVyIGxvYWRzLiAgSSB3YXMgY29uY2VybmVk
IGFib3V0IHRoYXQgc28gZGlkbid0DQphZGQgdGhlIGNoZWNrIGluIHByb2JlIHRvIHByZXZlbnQg
dGhlIGRyaXZlciBmcm9tIGxvYWRpbmcuICBJZiB0aGUNCnBvc3NpYmxlIGRpc3J1cHRpb24gb2Yg
dGhhdCBpcyBhY2NlcHRhYmxlLCB0aGVuIGl0IGRvZXMgc2VlbSBiZXR0ZXIgdG8NCmZhaWwgdG8g
bG9hZC4NCg0KSG93ZXZlciwgSSB0aGluayB0aGF0IGV2ZW4gaWYgdGhlIGRyaXZlciBmYWlscyB0
byBwcm9iZSBpZiB0aGVyZSBpcyBhDQp0aW1lb3V0IGF0IHByb2JlIHRpbWUsIGl0J3Mgc3RpbGwg
cG9zc2libGUgdG8gaGFuZyBsYXRlciBpZiB0aGVyZSBhcmUNCm5vdCBsaW1pdHMgdG8gdGhlIGhh
cmR3YXJlIHBvbGxpbmcgbG9vcHMsIHN1Y2ggYXMgdGhlIG9uZXMgSSBhZGRlZC4NCg0KPiA+IEkg
dGhpbmsgd2hhdCB5b3UndmUgZG9uZSB3aWxsIHByZXZlbnQgdGhlIGRyaXZlciBmcm9tIGxvYWRp
bmcgaWYgdGhlDQo+ID4gY2xvY2sgaXMgbm90IHdvcmtpbmcsIGJ1dCBpZiB0aGUgY2xvY2sgZG9l
cyBub3QgdGljayBwcm9wZXJseSBhZnRlcg0KPiA+IGxvYWRpbmcsIHRoZXJlIGFyZSBzdGlsbCBw
b2ludHMgKHNudnNfcnRjX3JlYWRfdGltZSBmb3Igb25lKSB0aGF0IHdpbGwNCj4gPiBsb2NrIHVw
IGluIHRoZSBrZXJuZWwuDQo+IA0KPiBJJ2xsIGRpZyBvdXQgeW91ciBwYXRjaCBhbmQgZ2l2ZSBp
dCBhIHJldmlldy4NCj4g

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable
@ 2018-03-30 22:59       ` Trent Piepho
  0 siblings, 0 replies; 10+ messages in thread
From: Trent Piepho @ 2018-03-30 22:59 UTC (permalink / raw)
  To: linux-kernel@vger.kernel.org,
	alexandre.belloni@free-electrons.com,
	pure.logic@nexus-software.ie
  Cc: shawn.guo@linaro.org, stefan@agner.ch, bianpan2016@163.com,
	a.zummo@towertech.it, stable@vger.kernel.org,
	guy.shapiro@mobi-wize.com, Frank.Li@freescale.com,
	linux-rtc@vger.kernel.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.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.
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable
  2018-03-30 22:59       ` Trent Piepho
  (?)
@ 2018-04-02 22:51       ` Bryan O'Donoghue
  2018-04-03 14:42         ` Alexandre Belloni
  -1 siblings, 1 reply; 10+ messages in thread
From: Bryan O'Donoghue @ 2018-04-02 22:51 UTC (permalink / raw)
  To: Trent Piepho, linux-kernel@vger.kernel.org,
	alexandre.belloni@free-electrons.com
  Cc: shawn.guo@linaro.org, stefan@agner.ch, bianpan2016@163.com,
	a.zummo@towertech.it, stable@vger.kernel.org,
	guy.shapiro@mobi-wize.com, Frank.Li@freescale.com,
	linux-rtc@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] rtc: snvs: Fix usage of snvs_rtc_enable
  2018-04-02 22:51       ` Bryan O'Donoghue
@ 2018-04-03 14:42         ` Alexandre Belloni
  0 siblings, 0 replies; 10+ messages in thread
From: Alexandre Belloni @ 2018-04-03 14:42 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: Trent Piepho, linux-kernel@vger.kernel.org, shawn.guo@linaro.org,
	stefan@agner.ch, bianpan2016@163.com, a.zummo@towertech.it,
	stable@vger.kernel.org, guy.shapiro@mobi-wize.com,
	Frank.Li@freescale.com, linux-rtc@vger.kernel.org

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable
  2018-03-28 19:14 [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable Bryan O'Donoghue
  2018-03-29  0:12   ` Trent Piepho
  2018-03-29  2:18 ` [RESEND] [PATCH] " Shawn Guo
@ 2018-04-03 14:56 ` Alexandre Belloni
  2 siblings, 0 replies; 10+ messages in thread
From: Alexandre Belloni @ 2018-04-03 14:56 UTC (permalink / raw)
  To: Bryan O'Donoghue
  Cc: linux-kernel, a.zummo, Pan Bian, Guy Shapiro, Stefan Agner,
	Frank Li, Shawn Guo, linux-rtc, # v3 . 16+

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2018-04-03 14:56 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-03-28 19:14 [RESEND] [PATCH] rtc: snvs: Fix usage of snvs_rtc_enable Bryan O'Donoghue
2018-03-29  0:12 ` [RESEND] " Trent Piepho
2018-03-29  0:12   ` Trent Piepho
2018-03-29  1:53   ` Bryan O'Donoghue
2018-03-30 22:59     ` Trent Piepho
2018-03-30 22:59       ` Trent Piepho
2018-04-02 22:51       ` Bryan O'Donoghue
2018-04-03 14:42         ` Alexandre Belloni
2018-03-29  2:18 ` [RESEND] [PATCH] " Shawn Guo
2018-04-03 14:56 ` Alexandre Belloni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.