[PATCH 0/6] Implement I2C restart handler

[PATCH 0/6] Implement I2C restart handler

[Stefan Christ]

Hi,

this is a request for feedback about a lockdep-warning-free I2C
restart_handler.  I would like to know whether someone had the problem already
and how the best and mainline friendliest solution looks like.

I have an embedded board with the PMIC chip DA9063 that is also used as a
voltage regulator for DVFS. For a clean reboot/reset of the system I have to
set the bit nSHUTDOWN in the PMIC via I2C. The restart_handler call chain
is of type

    static ATOMIC_NOTIFIER_HEAD(restart_handler_list);

So any code running as a restart_handler cannot (should not) wait, for other
locks. When you use i2c_transfer/regmap you get lockdep warnings like [1].

If for example the I2C adapter lock is already held by another task, the system
blocks indefinitely. The other task maybe the dynamic voltage and frequency
scaling that is adjusting system voltages.

The first two patches fix the watchdog code. It's not allowed to trigger the
DA9063 watchdog too fast. This leads to random resets. I will port these
patches to the current master and resend them.

The subsequent patch implements a blocking and schedule-free I2C transfer
function in the i.MX6 I2C driver. For now the watchdog driver will access the
function master_xfer_blocking directly.

The next patch allows a single consumer to block an I2C adapter for other
consumers. The implementation is bad, but it should only show the idea. The
restart handler has to be the single consumer of the I2C adapter _and_ all
other consumers have to finish before the restart handler starts. Only this
driver should be allowed to acquire the I2C adapter lock now. In the restart
handler you cannot wait for other consumers to finish because they maybe gone
already.

The last patch implements the schedule-free I2C restart handler.

Any comments? 


These patchse are currently based on kernel version 4.1.x, but I will port them
to newer kernels after I got some feedback.


Stefan Christ (6):
  watchdog: da9063_wdt: don't trigger watchdog too fast
  watchdog: da9063_wdt: use delayed work to trigger
  i2c-imx: add blocking xfer function
  i2c-core: add possibility to block an adapter for a single user
  mfd: da9063: save i2c_client for later use
  watchdog: da9063_wdt: add schedule-free and race-free restart handler

 drivers/i2c/busses/i2c-imx.c    | 108 ++++++++++++++++++-------
 drivers/i2c/i2c-core.c          |   5 ++
 drivers/mfd/da9063-i2c.c        |   1 +
 drivers/watchdog/da9063_wdt.c   | 174 +++++++++++++++++++++++++++++++++++++---
 include/linux/i2c.h             |   3 +
 include/linux/mfd/da9063/core.h |   1 +
 6 files changed, 251 insertions(+), 41 deletions(-)

-- 
1.9.1

[1] lockdep warning:

[   13.797547] ===============================
[   13.801727] [ INFO: suspicious RCU usage. ]
[   13.805910] 4.1.18 #237 Not tainted
[   13.809404] -------------------------------
[   13.813593] include/linux/rcupdate.h:570 Illegal context switch in RCU read-side critical section!
[   13.822556] 
[   13.822556] other info that might help us debug this:
[   13.822556] 
[   13.830569] 
[   13.830569] rcu_scheduler_active = 1, debug_locks = 0
[   13.837096] 2 locks held by systemd-shutdow/1:
[   13.841536]  #0:  (reboot_mutex){+.+.+.}, at: [<8004df48>] SyS_reboot+0x84/0x1c8
[   13.849020]  #1:  (rcu_read_lock){......}, at: [<8004c584>] __atomic_notifier_call_chain+0x0/0x128
[   13.858069] 
[   13.858069] stack backtrace:
[   13.862436] CPU: 0 PID: 1 Comm: systemd-shutdow Not tainted 4.1.18 #237
[   13.869050] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree)
[   13.875574] Backtrace: 
[   13.878051] [<80013fbc>] (dump_backtrace) from [<800141d8>] (show_stack+0x18/0x1c)
[   13.885625]  r6:60030093 r5:00000000 r4:80b8bb4c r3:00000000
[   13.891346] [<800141c0>] (show_stack) from [<8081dba0>] (dump_stack+0xb4/0xe8)
[   13.898586] [<8081daec>] (dump_stack) from [<80069f2c>] (lockdep_rcu_suspicious+0xbc/0x11c)
[   13.906933]  r10:ae960400 r9:00000000 r8:80b6eae4 r7:809fa88c r6:0000023a r5:80a06b84
[   13.914834]  r4:af070000 r3:00000000
[   13.918449] [<80069e70>] (lockdep_rcu_suspicious) from [<808208f4>] (__schedule+0x3d4/0x6d0)
[   13.926882]  r7:80bd0ca3 r6:af070000 r5:80b69900 r4:af717900
[   13.932592] [<80820520>] (__schedule) from [<80820eec>] (preempt_schedule_common+0x28/0x44)
[   13.940947]  r10:ae960400 r9:00000000 r8:00000000 r7:00000000 r6:00000002 r5:803c8d10
[   13.948863]  r4:af056000
[   13.951415] [<80820ec4>] (preempt_schedule_common) from [<80820f48>] (_cond_resched+0x40/0x48)
[   13.960022]  r4:00000000 r3:af056000
[   13.963636] [<80820f08>] (_cond_resched) from [<80822a84>] (mutex_lock_nested+0x20/0x3ec)
[   13.971823] [<80822a64>] (mutex_lock_nested) from [<803c8d10>] (regmap_lock_mutex+0x14/0x18)
[   13.980256]  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:00000002 r5:00000013
[   13.988156]  r4:ae960400
[   13.990711] [<803c8cfc>] (regmap_lock_mutex) from [<803cb424>] (regmap_write+0x38/0x68)
[   13.998729] [<803cb3ec>] (regmap_write) from [<80562428>] (da9063_wdt_restart_handler+0x40/0x68)
[   14.007518]  r6:00000000 r5:ffffffff r4:aea440c0 r3:ae8c9c90
[   14.013229] [<805623e8>] (da9063_wdt_restart_handler) from [<8004c4f8>] (notifier_call_chain+0x54/0x94)
[   14.022626]  r5:ffffffff r4:aea440c0
[   14.026240] [<8004c4a4>] (notifier_call_chain) from [<8004c600>] (__atomic_notifier_call_chain+0x7c/0x128)
[   14.035897]  r9:80bd0c9e r8:00000000 r7:80b7e190 r6:00000000 r5:00000000 r4:ffffffff
[   14.043710] [<8004c584>] (__atomic_notifier_call_chain) from [<8004c6cc>] (atomic_notifier_call_chain+0x20/0x28)
[   14.053887]  r9:af056000 r8:80010784 r7:00772a00 r6:fee1dead r5:80b7e190 r4:00000000
[   14.061700] [<8004c6ac>] (atomic_notifier_call_chain) from [<8004dc4c>] (do_kernel_restart+0x20/0x28)
[   14.070934] [<8004dc2c>] (do_kernel_restart) from [<8001297c>] (machine_restart+0x74/0x8c)
[   14.079209] [<80012908>] (machine_restart) from [<8004dd30>] (kernel_restart+0x40/0x58)
[   14.087217]  r5:80b7e190 r4:00000000
[   14.090823] [<8004dcf0>] (kernel_restart) from [<8004df78>] (SyS_reboot+0xb4/0x1c8)
[   14.098483]  r4:01234567 r3:01234567
[   14.102106] [<8004dec4>] (SyS_reboot) from [<800105a0>] (ret_fast_syscall+0x0/0x54)
[   14.109758]  r7:00000058 r6:553d0298 r5:00000000 r4:00000000
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 1/6] watchdog: da9063_wdt: don't trigger watchdog too fast

[Stefan Christ]

Triggering the watchdog faster than T_WDMIN=256ms leads to resets of the
da9063 chip. The datasheet says that the watchdog must only be triggered
in the timeframe T_WDMIN to T_WDMAX. The T_WDMAX is configured in the
driver.

This problem was already mentioned in the patch:

    http://comments.gmane.org/gmane.linux.watchdog/1708

You also could say that this behavior is a feature. When the userspace
goes wild, triggering the watchdog to fast, the system is reseted. But
there is currently no information in the watchdog ABI to report a
minimum wait time between two watchdog heartbeats.

The code is taken from the watchdog driver da9062_wdt.c.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/watchdog/da9063_wdt.c | 45 +++++++++++++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 4 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index e2fe2eb..f7bdfb1 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -35,13 +35,36 @@ static const unsigned int wdt_timeout[] = { 0, 2, 4, 8, 16, 32, 65, 131 };
 #define DA9063_WDT_MIN_TIMEOUT		wdt_timeout[DA9063_TWDSCALE_MIN]
 #define DA9063_WDT_MAX_TIMEOUT		wdt_timeout[DA9063_TWDSCALE_MAX]
 #define DA9063_WDG_TIMEOUT		wdt_timeout[3]
+#define DA9063_RESET_PROTECTION_MS	256
 
 struct da9063_watchdog {
 	struct da9063 *da9063;
 	struct watchdog_device wdtdev;
 	struct notifier_block restart_handler;
+	unsigned long j_time_stamp;
 };
 
+static void da9063_set_window_start(struct da9063_watchdog *wdt)
+{
+	wdt->j_time_stamp = jiffies;
+}
+
+static void da9063_apply_window_protection(struct da9063_watchdog *wdt)
+{
+	unsigned long delay = msecs_to_jiffies(DA9063_RESET_PROTECTION_MS);
+	unsigned long timeout = wdt->j_time_stamp + delay;
+	unsigned long now = jiffies;
+	unsigned int diff_ms;
+
+	/* if time-limit has not elapsed then wait for remainder */
+	if (time_before(now, timeout)) {
+		diff_ms = jiffies_to_msecs(timeout-now);
+		dev_dbg(wdt->da9063->dev,
+			"Kicked too quickly. Delaying %u msecs\n", diff_ms);
+		msleep(diff_ms);
+	}
+}
+
 static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
 {
 	unsigned int i;
@@ -54,10 +77,18 @@ static unsigned int da9063_wdt_timeout_to_sel(unsigned int secs)
 	return DA9063_TWDSCALE_MAX;
 }
 
-static int _da9063_wdt_set_timeout(struct da9063 *da9063, unsigned int regval)
+static int _da9063_wdt_set_timeout(struct da9063_watchdog *wdt, unsigned int regval)
 {
-	return regmap_update_bits(da9063->regmap, DA9063_REG_CONTROL_D,
+	int ret;
+
+	da9063_apply_window_protection(wdt);
+
+	ret = regmap_update_bits(wdt->da9063->regmap, DA9063_REG_CONTROL_D,
 				  DA9063_TWDSCALE_MASK, regval);
+
+	da9063_set_window_start(wdt);
+
+	return ret;
 }
 
 static int da9063_wdt_start(struct watchdog_device *wdd)
@@ -67,7 +98,7 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
 	int ret;
 
 	selector = da9063_wdt_timeout_to_sel(wdt->wdtdev.timeout);
-	ret = _da9063_wdt_set_timeout(wdt->da9063, selector);
+	ret = _da9063_wdt_set_timeout(wdt, selector);
 	if (ret)
 		dev_err(wdt->da9063->dev, "Watchdog failed to start (err = %d)\n",
 			ret);
@@ -94,12 +125,16 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
 	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
 	int ret;
 
+	da9063_apply_window_protection(wdt);
+
 	ret = regmap_write(wdt->da9063->regmap, DA9063_REG_CONTROL_F,
 			   DA9063_WATCHDOG);
 	if (ret)
 		dev_alert(wdt->da9063->dev, "Failed to ping the watchdog (err = %d)\n",
 			  ret);
 
+	da9063_set_window_start(wdt);
+
 	return ret;
 }
 
@@ -111,7 +146,7 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 	int ret;
 
 	selector = da9063_wdt_timeout_to_sel(timeout);
-	ret = _da9063_wdt_set_timeout(wdt->da9063, selector);
+	ret = _da9063_wdt_set_timeout(wdt, selector);
 	if (ret)
 		dev_err(wdt->da9063->dev, "Failed to set watchdog timeout (err = %d)\n",
 			ret);
@@ -192,6 +227,8 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 		dev_err(wdt->da9063->dev,
 			"Failed to register restart handler (err = %d)\n", ret);
 
+	da9063_set_window_start(wdt);
+
 	return 0;
 }
 
-- 
1.9.1

Re: [RFC 1/6] watchdog: da9063_wdt: don't trigger watchdog too fast

[Guenter Roeck]

On 07/05/2016 06:22 AM, Stefan Christ wrote:
> Triggering the watchdog faster than T_WDMIN=256ms leads to resets of the
> da9063 chip. The datasheet says that the watchdog must only be triggered
> in the timeframe T_WDMIN to T_WDMAX. The T_WDMAX is configured in the
> driver.
>
> This problem was already mentioned in the patch:
>
>      http://comments.gmane.org/gmane.linux.watchdog/1708
>
> You also could say that this behavior is a feature. When the userspace
> goes wild, triggering the watchdog to fast, the system is reseted. But
> there is currently no information in the watchdog ABI to report a
> minimum wait time between two watchdog heartbeats.
>

There isn't ? What is the problem with min_hw_heartbeat_ms which was
introduced exactly for that purpose ?

Thanks,
Guenter

Re: [RFC 1/6] watchdog: da9063_wdt: don't trigger watchdog too fast

[Stefan Christ]

Hi Guenter,

On Tue, Jul 05, 2016 at 08:33:35AM -0700, Guenter Roeck wrote:
> On 07/05/2016 06:22 AM, Stefan Christ wrote:
> >Triggering the watchdog faster than T_WDMIN=256ms leads to resets of the
> >da9063 chip. The datasheet says that the watchdog must only be triggered
> >in the timeframe T_WDMIN to T_WDMAX. The T_WDMAX is configured in the
> >driver.
> >
> >This problem was already mentioned in the patch:
> >
> >     http://comments.gmane.org/gmane.linux.watchdog/1708
> >
> >You also could say that this behavior is a feature. When the userspace
> >goes wild, triggering the watchdog to fast, the system is reseted. But
> >there is currently no information in the watchdog ABI to report a
> >minimum wait time between two watchdog heartbeats.
> >
> 
> There isn't ? What is the problem with min_hw_heartbeat_ms which was
> introduced exactly for that purpose ?
> 

Ah your right. Sorry, I didn't noticed it. I'm sending a v2 patch for it.
Thanks.

Mit freundlichen Grüßen / Kind regards,
	Stefan Christ

> Thanks,
> Guenter
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 2/6] watchdog: da9063_wdt: use delayed work to trigger

[Stefan Christ]

Use a delayed worker to trigger the watchdog asynchronously. This avoids
blocking the caller, when the watchdog is triggered faster than 256ms.
For example the systemd init daemon triggers it at each iteration of its
mainloop.

Using schedule_delayed_work() also consolidates multiple watchdog
trigger requests in a 256ms timeframe into a single watchdog trigger
event.

TODO There is a race condition between call schedule_delayed_work() and
resetting the window start time with da9063_set_window_start().  This
leads to warnings like:

    da9063 0-0058: Kicked too quickly. Delaying 260 msecs

These are harmless, but can be fixed with correct locking.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/watchdog/da9063_wdt.c | 54 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 9 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index f7bdfb1..a32f2cd 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -41,6 +41,7 @@ struct da9063_watchdog {
 	struct da9063 *da9063;
 	struct watchdog_device wdtdev;
 	struct notifier_block restart_handler;
+	struct delayed_work ping_work;
 	unsigned long j_time_stamp;
 };
 
@@ -49,18 +50,28 @@ static void da9063_set_window_start(struct da9063_watchdog *wdt)
 	wdt->j_time_stamp = jiffies;
 }
 
-static void da9063_apply_window_protection(struct da9063_watchdog *wdt)
+static unsigned int da9063_get_wait_time(struct da9063_watchdog *wdt)
 {
 	unsigned long delay = msecs_to_jiffies(DA9063_RESET_PROTECTION_MS);
 	unsigned long timeout = wdt->j_time_stamp + delay;
 	unsigned long now = jiffies;
-	unsigned int diff_ms;
 
 	/* if time-limit has not elapsed then wait for remainder */
 	if (time_before(now, timeout)) {
-		diff_ms = jiffies_to_msecs(timeout-now);
-		dev_dbg(wdt->da9063->dev,
-			"Kicked too quickly. Delaying %u msecs\n", diff_ms);
+		return timeout - now;
+	}
+
+	return 0; /* Watchdog can be pinged at once */
+}
+
+static void da9063_apply_window_protection(struct da9063_watchdog *wdt)
+{
+	unsigned int diff_ms = jiffies_to_msecs(da9063_get_wait_time(wdt));
+
+	/* if time-limit has not elapsed then wait for remainder */
+	if (diff_ms) {
+		dev_warn(wdt->da9063->dev,
+			 "Kicked too quickly. Delaying %u msecs\n", diff_ms);
 		msleep(diff_ms);
 	}
 }
@@ -103,6 +114,8 @@ static int da9063_wdt_start(struct watchdog_device *wdd)
 		dev_err(wdt->da9063->dev, "Watchdog failed to start (err = %d)\n",
 			ret);
 
+	da9063_set_window_start(wdt);
+
 	return ret;
 }
 
@@ -111,6 +124,12 @@ static int da9063_wdt_stop(struct watchdog_device *wdd)
 	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
 	int ret;
 
+	/*
+	 * Cancel current ping request and wait for it to finished if it's
+	 * currently running.
+	 */
+	cancel_delayed_work_sync(&wdt->ping_work);
+
 	ret = regmap_update_bits(wdt->da9063->regmap, DA9063_REG_CONTROL_D,
 				 DA9063_TWDSCALE_MASK, DA9063_TWDSCALE_DISABLE);
 	if (ret)
@@ -120,9 +139,14 @@ static int da9063_wdt_stop(struct watchdog_device *wdd)
 	return ret;
 }
 
-static int da9063_wdt_ping(struct watchdog_device *wdd)
+static void da9063_wdt_ping_work_now(struct work_struct *work)
 {
-	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
+	struct delayed_work *workd = container_of(work,
+						struct delayed_work,
+						work);
+	struct da9063_watchdog *wdt = container_of(workd,
+						struct da9063_watchdog,
+						ping_work);
 	int ret;
 
 	da9063_apply_window_protection(wdt);
@@ -134,8 +158,16 @@ static int da9063_wdt_ping(struct watchdog_device *wdd)
 			  ret);
 
 	da9063_set_window_start(wdt);
+}
 
-	return ret;
+static int da9063_wdt_ping(struct watchdog_device *wdd)
+{
+	struct da9063_watchdog *wdt = watchdog_get_drvdata(wdd);
+	unsigned int delay = da9063_get_wait_time(wdt); /* in jiffies */
+
+	schedule_delayed_work(&wdt->ping_work, delay);
+
+	return 0;
 }
 
 static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
@@ -204,6 +236,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	wdt->da9063 = da9063;
+	da9063_set_window_start(wdt);
 
 	wdt->wdtdev.info = &da9063_watchdog_info;
 	wdt->wdtdev.ops = &da9063_watchdog_ops;
@@ -227,7 +260,7 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 		dev_err(wdt->da9063->dev,
 			"Failed to register restart handler (err = %d)\n", ret);
 
-	da9063_set_window_start(wdt);
+	INIT_DELAYED_WORK(&(wdt->ping_work), da9063_wdt_ping_work_now);
 
 	return 0;
 }
@@ -236,6 +269,9 @@ static int da9063_wdt_remove(struct platform_device *pdev)
 {
 	struct da9063_watchdog *wdt = dev_get_drvdata(&pdev->dev);
 
+	/* Wait for delayed worker to finish. */
+	cancel_delayed_work_sync(&wdt->ping_work);
+
 	unregister_restart_handler(&wdt->restart_handler);
 
 	watchdog_unregister_device(&wdt->wdtdev);
-- 
1.9.1

[RFC 3/6] i2c-imx: add blocking xfer function

[Stefan Christ]

Implement a blocking i2c xfer function that does not call schedule() and
does not rely on acquiring locks. The caller must ensure that there is
no concurrent user.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/i2c/busses/i2c-imx.c | 108 ++++++++++++++++++++++++++++++++-----------
 include/linux/i2c.h          |   2 +
 2 files changed, 82 insertions(+), 28 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index a53a7dd..49104b0 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -412,7 +412,7 @@ static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
 /** Functions for IMX I2C adapter driver ***************************************
 *******************************************************************************/
 
-static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
+static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy, bool blocking)
 {
 	unsigned long orig_jiffies = jiffies;
 	unsigned int temp;
@@ -438,15 +438,41 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy)
 				"<%s> I2C bus is busy\n", __func__);
 			return -ETIMEDOUT;
 		}
-		schedule();
+		if (!blocking) {
+			schedule();
+		} else {
+			printk("%s: delay\n", __func__);
+			udelay(100);
+		}
 	}
 
 	return 0;
 }
 
-static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx, bool blocking)
 {
-	wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+	if (!blocking) {
+		wait_event_timeout(i2c_imx->queue, i2c_imx->i2csr & I2SR_IIF, HZ / 10);
+	} else {
+		int counter = 0;
+		unsigned int reg;
+
+		while (1) {
+			printk("%s: try %d\n", __func__, counter);
+			reg = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+			i2c_imx->i2csr = reg;
+			if (reg & I2SR_IIF)
+				break;
+
+			if (counter > 1000) {
+				dev_err(&i2c_imx->adapter.dev, "<%s> TXR timeout\n", __func__);
+				return -EIO;
+			}
+			udelay(100);
+			counter++;
+		}
+		imx_i2c_write_reg(0, i2c_imx, IMX_I2C_I2SR);
+	}
 
 	if (unlikely(!(i2c_imx->i2csr & I2SR_IIF))) {
 		dev_dbg(&i2c_imx->adapter.dev, "<%s> Timeout\n", __func__);
@@ -511,7 +537,7 @@ static void i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx)
 #endif
 }
 
-static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
+static int i2c_imx_start(struct imx_i2c_struct *i2c_imx, bool blocking)
 {
 	unsigned int temp = 0;
 	int result;
@@ -535,18 +561,24 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
 	temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 	temp |= I2CR_MSTA;
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-	result = i2c_imx_bus_busy(i2c_imx, 1);
+	result = i2c_imx_bus_busy(i2c_imx, 1, blocking);
 	if (result)
 		return result;
 	i2c_imx->stopped = 0;
 
-	temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+	if (!blocking) {
+		temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
+	} else {
+		temp |= I2CR_MTX | I2CR_TXAK;
+		temp &= ~I2CR_IIEN; /* Disable interrupt */
+	}
+
 	temp &= ~I2CR_DMAEN;
 	imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
 	return result;
 }
 
-static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
+static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx, bool blocking)
 {
 	unsigned int temp = 0;
 
@@ -568,7 +600,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx)
 	}
 
 	if (!i2c_imx->stopped) {
-		i2c_imx_bus_busy(i2c_imx, 0);
+		i2c_imx_bus_busy(i2c_imx, 0, blocking);
 		i2c_imx->stopped = 1;
 	}
 
@@ -653,7 +685,7 @@ static int i2c_imx_dma_write(struct imx_i2c_struct *i2c_imx,
 	/* The last data byte must be transferred by the CPU. */
 	imx_i2c_write_reg(msgs->buf[msgs->len-1],
 				i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, false);
 	if (result)
 		return result;
 
@@ -716,7 +748,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 
 	msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
 	/* read n byte data */
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, false);
 	if (result)
 		return result;
 
@@ -729,7 +761,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 		temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 		temp &= ~(I2CR_MSTA | I2CR_MTX);
 		imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-		i2c_imx_bus_busy(i2c_imx, 0);
+		i2c_imx_bus_busy(i2c_imx, 0, false);
 		i2c_imx->stopped = 1;
 	} else {
 		/*
@@ -748,7 +780,7 @@ static int i2c_imx_dma_read(struct imx_i2c_struct *i2c_imx,
 	return 0;
 }
 
-static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
+static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bool blocking)
 {
 	int i, result;
 
@@ -757,7 +789,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 
 	/* write slave address */
 	imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, blocking);
 	if (result)
 		return result;
 	result = i2c_imx_acked(i2c_imx);
@@ -771,7 +803,7 @@ static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
 			"<%s> write byte: B%d=0x%X\n",
 			__func__, i, msgs->buf[i]);
 		imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
-		result = i2c_imx_trx_complete(i2c_imx);
+		result = i2c_imx_trx_complete(i2c_imx, blocking);
 		if (result)
 			return result;
 		result = i2c_imx_acked(i2c_imx);
@@ -793,7 +825,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 
 	/* write slave address */
 	imx_i2c_write_reg((msgs->addr << 1) | 0x01, i2c_imx, IMX_I2C_I2DR);
-	result = i2c_imx_trx_complete(i2c_imx);
+	result = i2c_imx_trx_complete(i2c_imx, false);
 	if (result)
 		return result;
 	result = i2c_imx_acked(i2c_imx);
@@ -824,7 +856,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 	for (i = 0; i < msgs->len; i++) {
 		u8 len = 0;
 
-		result = i2c_imx_trx_complete(i2c_imx);
+		result = i2c_imx_trx_complete(i2c_imx, false);
 		if (result)
 			return result;
 		/*
@@ -852,7 +884,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 				temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 				temp &= ~(I2CR_MSTA | I2CR_MTX);
 				imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-				i2c_imx_bus_busy(i2c_imx, 0);
+				i2c_imx_bus_busy(i2c_imx, 0, false);
 				i2c_imx->stopped = 1;
 			} else {
 				/*
@@ -884,8 +916,8 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs, bo
 	return 0;
 }
 
-static int i2c_imx_xfer(struct i2c_adapter *adapter,
-						struct i2c_msg *msgs, int num)
+static int i2c_imx_xfer_s(struct i2c_adapter *adapter,
+						struct i2c_msg *msgs, int num, bool blocking)
 {
 	unsigned int i, temp;
 	int result;
@@ -895,7 +927,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 	dev_dbg(&i2c_imx->adapter.dev, "<%s>\n", __func__);
 
 	/* Start I2C transfer */
-	result = i2c_imx_start(i2c_imx);
+	result = i2c_imx_start(i2c_imx, blocking);
 	if (result)
 		goto fail0;
 
@@ -910,7 +942,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 			temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
 			temp |= I2CR_RSTA;
 			imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
-			result =  i2c_imx_bus_busy(i2c_imx, 1);
+			result =  i2c_imx_bus_busy(i2c_imx, 1, blocking);
 			if (result)
 				goto fail0;
 		}
@@ -935,12 +967,19 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 			(temp & I2SR_RXAK ? 1 : 0));
 #endif
 		if (msgs[i].flags & I2C_M_RD)
-			result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg);
-		else {
-			if (i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD)
-				result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
+			if (!blocking)
+				result = i2c_imx_read(i2c_imx, &msgs[i], is_lastmsg);
 			else
-				result = i2c_imx_write(i2c_imx, &msgs[i]);
+				return -EPERM; /* not implemented */
+		else {
+			if (!blocking) {
+				if (i2c_imx->dma && msgs[i].len >= DMA_THRESHOLD)
+					result = i2c_imx_dma_write(i2c_imx, &msgs[i]);
+				else
+					result = i2c_imx_write(i2c_imx, &msgs[i], blocking);
+			} else {
+				result = i2c_imx_write(i2c_imx, &msgs[i], blocking);
+			}
 		}
 		if (result)
 			goto fail0;
@@ -948,7 +987,7 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter,
 
 fail0:
 	/* Stop I2C transfer */
-	i2c_imx_stop(i2c_imx);
+	i2c_imx_stop(i2c_imx, blocking);
 
 	dev_dbg(&i2c_imx->adapter.dev, "<%s> exit with: %s: %d\n", __func__,
 		(result < 0) ? "error" : "success msg",
@@ -956,6 +995,18 @@ fail0:
 	return (result < 0) ? result : num;
 }
 
+static int i2c_imx_xfer(struct i2c_adapter *adapter,
+						struct i2c_msg *msgs, int num)
+{
+	return i2c_imx_xfer_s(adapter, msgs, num, false);
+}
+
+static int i2c_imx_xfer_blocking(struct i2c_adapter *adapter,
+						struct i2c_msg *msgs, int num)
+{
+	return i2c_imx_xfer_s(adapter, msgs, num, true);
+}
+
 static u32 i2c_imx_func(struct i2c_adapter *adapter)
 {
 	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
@@ -964,6 +1015,7 @@ static u32 i2c_imx_func(struct i2c_adapter *adapter)
 
 static struct i2c_algorithm i2c_imx_algo = {
 	.master_xfer	= i2c_imx_xfer,
+	.master_xfer_blocking	= i2c_imx_xfer_blocking,
 	.functionality	= i2c_imx_func,
 };
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index e83a738..8461841 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -397,6 +397,8 @@ struct i2c_algorithm {
 	   processed, or a negative value on error */
 	int (*master_xfer)(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			   int num);
+	int (*master_xfer_blocking)(struct i2c_adapter *adap, struct i2c_msg *msgs,
+			   int num);
 	int (*smbus_xfer) (struct i2c_adapter *adap, u16 addr,
 			   unsigned short flags, char read_write,
 			   u8 command, int size, union i2c_smbus_data *data);
-- 
1.9.1

[RFC 4/6] i2c-core: add possibility to block an adapter for a single user

[Stefan Christ]

TODO use mutex and mutex_try_lock instead of boolean flag. Avoid that
multiple consumers can block the adapter at the same time.

This flag is only intended to be used in the shutdown process in which
no cleanup is required.

Signed-off-by: Stefan Christ <s.christ-guT5V/WYfQezQB+pC5nmwQ@public.gmane.org>
---
 drivers/i2c/i2c-core.c | 5 +++++
 include/linux/i2c.h    | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/i2c/i2c-core.c b/drivers/i2c/i2c-core.c
index 987c124..9db7130 100644
--- a/drivers/i2c/i2c-core.c
+++ b/drivers/i2c/i2c-core.c
@@ -2068,6 +2068,11 @@ int i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
 {
 	int ret;
 
+	if (adap->blocked) {
+		dev_info(&adap->dev, "No more i2c communication is allowed! pid = %d\n", current->pid);
+		return -EPERM;
+	}
+
 	/* REVISIT the fault reporting model here is weak:
 	 *
 	 *  - When we get an error after receiving N bytes from a slave,
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 8461841..282ec71 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -519,6 +519,7 @@ struct i2c_adapter {
 
 	struct i2c_bus_recovery_info *bus_recovery_info;
 	const struct i2c_adapter_quirks *quirks;
+	bool blocked;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC 6/6] watchdog: da9063_wdt: add schedule-free and race-free restart handler

[Stefan Christ]

Signed-off-by: Stefan Christ <s.christ-guT5V/WYfQezQB+pC5nmwQ@public.gmane.org>
---
 drivers/watchdog/da9063_wdt.c | 87 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 81 insertions(+), 6 deletions(-)

diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
index a32f2cd..b9feef9 100644
--- a/drivers/watchdog/da9063_wdt.c
+++ b/drivers/watchdog/da9063_wdt.c
@@ -18,6 +18,7 @@
 #include <linux/uaccess.h>
 #include <linux/slab.h>
 #include <linux/delay.h>
+#include <linux/i2c.h>
 #include <linux/mfd/da9063/registers.h>
 #include <linux/mfd/da9063/core.h>
 #include <linux/reboot.h>
@@ -41,6 +42,7 @@ struct da9063_watchdog {
 	struct da9063 *da9063;
 	struct watchdog_device wdtdev;
 	struct notifier_block restart_handler;
+	struct notifier_block reboot_notifier;
 	struct delayed_work ping_work;
 	unsigned long j_time_stamp;
 };
@@ -188,19 +190,83 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
 	return ret;
 }
 
+static int da9063_wdt_reboot_notifier(struct notifier_block *this, unsigned long val, void *v)
+{
+	struct da9063_watchdog *wdt = container_of(this,
+						   struct da9063_watchdog,
+						   reboot_notifier);
+	struct i2c_adapter *adap = wdt->da9063->i2c->adapter;
+
+	/*
+	 * First block the I2C bus for other drivers. Other consumers are not
+	 * allowed to access the bus now.
+	 */
+	adap->blocked = true;
+
+	/*
+	 * Then acquire adapter lock. This will wait until all other consumers
+	 * have finished.  After that no more writes are possible for other
+	 * drivers.
+	 */
+	i2c_lock_adapter(adap);
+
+	/*
+	 * Now the I2C adapter can be used in contexts that are not allowed to
+	 * wait for locks or call schedule() because there is no other
+	 * consumer.
+	 */
+	return NOTIFY_DONE;
+}
+
 static int da9063_wdt_restart_handler(struct notifier_block *this,
 				      unsigned long mode, void *cmd)
 {
 	struct da9063_watchdog *wdt = container_of(this,
 						   struct da9063_watchdog,
 						   restart_handler);
-	int ret;
+	int ret, i;
+	struct i2c_adapter *adap = wdt->da9063->i2c->adapter;
+	unsigned char data[3] = {DA9063_REG_CONTROL_F,
+				 DA9063_SHUTDOWN,
+				 0x0};
+	struct i2c_msg msg = { .addr = wdt->da9063->i2c->addr,
+			       .flags = 0,
+			       .len = sizeof(data),
+			       .buf = data };
+
+	/* Very special calling context:
+	 *  - other CPU cores already stopped
+	 *  - But tasks on first cpu still running
+	 *  - Other tasks may have acquired locks that will never be released
+	 *  - Cleanup doesn't matter anymore. Execution may stop at any
+	 *    instruction now.
+	 *  - Current task can stall the CPU. Not to worried about
+	 *    concurrent access from other tasks or CPUs.
+	 *  - You have to avoid any kernel internal function which calls
+	 *     schedule().  Otherwise other tasks will run and interfere.
+	 */
 
-	ret = regmap_write(wdt->da9063->regmap, DA9063_REG_CONTROL_F,
-			   DA9063_SHUTDOWN);
-	if (ret)
-		dev_alert(wdt->da9063->dev, "Failed to shutdown (err = %d)\n",
-			  ret);
+	if (!adap->algo->master_xfer_blocking) {
+		printk("%s: I2C adapter does not support blocking transfer. Cannot use it!",
+				__func__);
+		return NOTIFY_DONE;
+	}
+
+
+	for (i = 0; i < 3; i++) {
+		ret = adap->algo->master_xfer_blocking(adap, &msg, 1);
+		if (ret == 0)
+			break;
+
+		printk("%s: blocking i2c transfer failed, ret =%d\n", __func__, ret);
+		if (ret != -EAGAIN) /* dont' retry, some other error */
+			break;
+
+		udelay(100);
+		printk("%s: try %d failed. Retry!\n", __func__, i);
+	}
+
+	udelay(500); /* wait for reset to assert... */
 
 	return NOTIFY_DONE;
 }
@@ -253,6 +319,13 @@ static int da9063_wdt_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	wdt->reboot_notifier.notifier_call = da9063_wdt_reboot_notifier;
+	wdt->reboot_notifier.priority = 0; /* be the last notifier */
+	ret = register_reboot_notifier(&wdt->reboot_notifier);
+	if (ret)
+		dev_err(wdt->da9063->dev,
+			"Failed to register reboot notifier (err = %d)\n", ret);
+
 	wdt->restart_handler.notifier_call = da9063_wdt_restart_handler;
 	wdt->restart_handler.priority = 128;
 	ret = register_restart_handler(&wdt->restart_handler);
@@ -272,6 +345,8 @@ static int da9063_wdt_remove(struct platform_device *pdev)
 	/* Wait for delayed worker to finish. */
 	cancel_delayed_work_sync(&wdt->ping_work);
 
+	unregister_reboot_notifier(&wdt->reboot_notifier);
+
 	unregister_restart_handler(&wdt->restart_handler);
 
 	watchdog_unregister_device(&wdt->wdtdev);
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 6/6] watchdog: da9063_wdt: add schedule-free and race-free restart handler

[Guenter Roeck]

On 07/05/2016 06:22 AM, Stefan Christ wrote:
> Signed-off-by: Stefan Christ <s.christ@phytec.de>
> ---

I am not sure if I like the idea of bypassing the reboot handler functionality
implemented in the watchdog core. First preference should be to fix it if there
is a use case which is not covered. If that does not work, I would expect
a detailed explanation of the reason, not a patch with no explanation whatsoever.

Guenter

>   drivers/watchdog/da9063_wdt.c | 87 ++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 81 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/watchdog/da9063_wdt.c b/drivers/watchdog/da9063_wdt.c
> index a32f2cd..b9feef9 100644
> --- a/drivers/watchdog/da9063_wdt.c
> +++ b/drivers/watchdog/da9063_wdt.c
> @@ -18,6 +18,7 @@
>   #include <linux/uaccess.h>
>   #include <linux/slab.h>
>   #include <linux/delay.h>
> +#include <linux/i2c.h>
>   #include <linux/mfd/da9063/registers.h>
>   #include <linux/mfd/da9063/core.h>
>   #include <linux/reboot.h>
> @@ -41,6 +42,7 @@ struct da9063_watchdog {
>   	struct da9063 *da9063;
>   	struct watchdog_device wdtdev;
>   	struct notifier_block restart_handler;
> +	struct notifier_block reboot_notifier;
>   	struct delayed_work ping_work;
>   	unsigned long j_time_stamp;
>   };
> @@ -188,19 +190,83 @@ static int da9063_wdt_set_timeout(struct watchdog_device *wdd,
>   	return ret;
>   }
>
> +static int da9063_wdt_reboot_notifier(struct notifier_block *this, unsigned long val, void *v)
> +{
> +	struct da9063_watchdog *wdt = container_of(this,
> +						   struct da9063_watchdog,
> +						   reboot_notifier);
> +	struct i2c_adapter *adap = wdt->da9063->i2c->adapter;
> +
> +	/*
> +	 * First block the I2C bus for other drivers. Other consumers are not
> +	 * allowed to access the bus now.
> +	 */
> +	adap->blocked = true;
> +
> +	/*
> +	 * Then acquire adapter lock. This will wait until all other consumers
> +	 * have finished.  After that no more writes are possible for other
> +	 * drivers.
> +	 */
> +	i2c_lock_adapter(adap);
> +
> +	/*
> +	 * Now the I2C adapter can be used in contexts that are not allowed to
> +	 * wait for locks or call schedule() because there is no other
> +	 * consumer.
> +	 */
> +	return NOTIFY_DONE;
> +}
> +
>   static int da9063_wdt_restart_handler(struct notifier_block *this,
>   				      unsigned long mode, void *cmd)
>   {
>   	struct da9063_watchdog *wdt = container_of(this,
>   						   struct da9063_watchdog,
>   						   restart_handler);
> -	int ret;
> +	int ret, i;
> +	struct i2c_adapter *adap = wdt->da9063->i2c->adapter;
> +	unsigned char data[3] = {DA9063_REG_CONTROL_F,
> +				 DA9063_SHUTDOWN,
> +				 0x0};
> +	struct i2c_msg msg = { .addr = wdt->da9063->i2c->addr,
> +			       .flags = 0,
> +			       .len = sizeof(data),
> +			       .buf = data };
> +
> +	/* Very special calling context:
> +	 *  - other CPU cores already stopped
> +	 *  - But tasks on first cpu still running
> +	 *  - Other tasks may have acquired locks that will never be released
> +	 *  - Cleanup doesn't matter anymore. Execution may stop at any
> +	 *    instruction now.
> +	 *  - Current task can stall the CPU. Not to worried about
> +	 *    concurrent access from other tasks or CPUs.
> +	 *  - You have to avoid any kernel internal function which calls
> +	 *     schedule().  Otherwise other tasks will run and interfere.
> +	 */
>
> -	ret = regmap_write(wdt->da9063->regmap, DA9063_REG_CONTROL_F,
> -			   DA9063_SHUTDOWN);
> -	if (ret)
> -		dev_alert(wdt->da9063->dev, "Failed to shutdown (err = %d)\n",
> -			  ret);
> +	if (!adap->algo->master_xfer_blocking) {
> +		printk("%s: I2C adapter does not support blocking transfer. Cannot use it!",
> +				__func__);
> +		return NOTIFY_DONE;
> +	}
> +
> +
> +	for (i = 0; i < 3; i++) {
> +		ret = adap->algo->master_xfer_blocking(adap, &msg, 1);
> +		if (ret == 0)
> +			break;
> +
> +		printk("%s: blocking i2c transfer failed, ret =%d\n", __func__, ret);
> +		if (ret != -EAGAIN) /* dont' retry, some other error */
> +			break;
> +
> +		udelay(100);
> +		printk("%s: try %d failed. Retry!\n", __func__, i);
> +	}
> +
> +	udelay(500); /* wait for reset to assert... */
>
>   	return NOTIFY_DONE;
>   }
> @@ -253,6 +319,13 @@ static int da9063_wdt_probe(struct platform_device *pdev)
>   	if (ret)
>   		return ret;
>
> +	wdt->reboot_notifier.notifier_call = da9063_wdt_reboot_notifier;
> +	wdt->reboot_notifier.priority = 0; /* be the last notifier */
> +	ret = register_reboot_notifier(&wdt->reboot_notifier);
> +	if (ret)
> +		dev_err(wdt->da9063->dev,
> +			"Failed to register reboot notifier (err = %d)\n", ret);
> +
>   	wdt->restart_handler.notifier_call = da9063_wdt_restart_handler;
>   	wdt->restart_handler.priority = 128;
>   	ret = register_restart_handler(&wdt->restart_handler);
> @@ -272,6 +345,8 @@ static int da9063_wdt_remove(struct platform_device *pdev)
>   	/* Wait for delayed worker to finish. */
>   	cancel_delayed_work_sync(&wdt->ping_work);
>
> +	unregister_reboot_notifier(&wdt->reboot_notifier);
> +
>   	unregister_restart_handler(&wdt->restart_handler);
>
>   	watchdog_unregister_device(&wdt->wdtdev);
>

[RFC 5/6] mfd: da9063: save i2c_client for later use

[Stefan Christ]

Save i2c_client for later use in watchdog restart handler. The regmap
interface cannot be used there.

Signed-off-by: Stefan Christ <s.christ@phytec.de>
---
 drivers/mfd/da9063-i2c.c        | 1 +
 include/linux/mfd/da9063/core.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/mfd/da9063-i2c.c b/drivers/mfd/da9063-i2c.c
index 6f3a7c0..c0f53d2 100644
--- a/drivers/mfd/da9063-i2c.c
+++ b/drivers/mfd/da9063-i2c.c
@@ -224,6 +224,7 @@ static int da9063_i2c_probe(struct i2c_client *i2c,
 	i2c_set_clientdata(i2c, da9063);
 	da9063->dev = &i2c->dev;
 	da9063->chip_irq = i2c->irq;
+	da9063->i2c = i2c;
 
 	if (da9063->variant_code == PMIC_DA9063_AD) {
 		da9063_regmap_config.rd_table = &da9063_ad_readable_table;
diff --git a/include/linux/mfd/da9063/core.h b/include/linux/mfd/da9063/core.h
index 79f4d82..1d1ae5d 100644
--- a/include/linux/mfd/da9063/core.h
+++ b/include/linux/mfd/da9063/core.h
@@ -83,6 +83,7 @@ struct da9063 {
 
 	/* Control interface */
 	struct regmap	*regmap;
+	struct i2c_client *i2c;
 
 	/* Interrupts */
 	int		chip_irq;
-- 
1.9.1

Re: [PATCH 0/6] Implement I2C restart handler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

Hi,

> this is a request for feedback about a lockdep-warning-free I2C
> restart_handler.  I would like to know whether someone had the problem already
> and how the best and mainline friendliest solution looks like.
> I have an embedded board with the PMIC chip DA9063 that is also used as a
> voltage regulator for DVFS. For a clean reboot/reset of the system I have to
> set the bit nSHUTDOWN in the PMIC via I2C. The restart_handler call chain
> is of type
> 
>     static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
> 
> So any code running as a restart_handler cannot (should not) wait, for other
> locks. When you use i2c_transfer/regmap you get lockdep warnings like [1].

Yes, this problem has come up occasionally. And it is not only about
sleeping. IRQs are disabled, too.

My idea was to introduce master_xfer_irqless or similar to struct
i2c_algorithm. I even discussed this shortly with Mark Brown how to
interact with regmap; but this was too long ago, so I forgot what we
agreed on :(

> Any comments? 

I just skimmed very lightly over the patchset. Yet, seeing an I2C client
messing directly with 'struct adapter' breaks so many abstractions, it
hits my instant-NACK nerve ;)

Thanks,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 0/6] Implement I2C restart handler

[Guenter Roeck]

On 07/05/2016 07:58 AM, Wolfram Sang wrote:
> Hi,
>
>> this is a request for feedback about a lockdep-warning-free I2C
>> restart_handler.  I would like to know whether someone had the problem already
>> and how the best and mainline friendliest solution looks like.
>> I have an embedded board with the PMIC chip DA9063 that is also used as a
>> voltage regulator for DVFS. For a clean reboot/reset of the system I have to
>> set the bit nSHUTDOWN in the PMIC via I2C. The restart_handler call chain
>> is of type
>>
>>      static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
>>
>> So any code running as a restart_handler cannot (should not) wait, for other
>> locks. When you use i2c_transfer/regmap you get lockdep warnings like [1].
>
> Yes, this problem has come up occasionally. And it is not only about
> sleeping. IRQs are disabled, too.
>
> My idea was to introduce master_xfer_irqless or similar to struct
> i2c_algorithm. I even discussed this shortly with Mark Brown how to
> interact with regmap; but this was too long ago, so I forgot what we
> agreed on :(
>
>> Any comments?
>
> I just skimmed very lightly over the patchset. Yet, seeing an I2C client
> messing directly with 'struct adapter' breaks so many abstractions, it
> hits my instant-NACK nerve ;)
>

My NACK would be for two other reasons: First, for bypassing the watchdog core
for handling the minimum time between heartbeats, second for bypassing
the watchdog core to implement a reboot handler. Both are inadequately
or not at all explained.

Guenter

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] Implement I2C restart handler

[Stefan Christ]

Hi,

On Tue, Jul 05, 2016 at 11:58:41PM +0900, Wolfram Sang wrote:
> Hi,
> 
> > this is a request for feedback about a lockdep-warning-free I2C
> > restart_handler.  I would like to know whether someone had the problem already
> > and how the best and mainline friendliest solution looks like.
> > I have an embedded board with the PMIC chip DA9063 that is also used as a
> > voltage regulator for DVFS. For a clean reboot/reset of the system I have to
> > set the bit nSHUTDOWN in the PMIC via I2C. The restart_handler call chain
> > is of type
> > 
> >     static ATOMIC_NOTIFIER_HEAD(restart_handler_list);
> > 
> > So any code running as a restart_handler cannot (should not) wait, for other
> > locks. When you use i2c_transfer/regmap you get lockdep warnings like [1].
> 
> Yes, this problem has come up occasionally. And it is not only about
> sleeping. IRQs are disabled, too.
> 
> My idea was to introduce master_xfer_irqless or similar to struct
> i2c_algorithm. I even discussed this shortly with Mark Brown how to
> interact with regmap; but this was too long ago, so I forgot what we
> agreed on :(

Great. I was looking for exactly this kind of feedback.

The term 'master_xfer_irqless' does not bring up anything on the internet and I
didn't find anything on the list linux-i2c. Can you point me to the discussion,
so I can read it up and work on it?

> > Any comments? 
> 
> I just skimmed very lightly over the patchset. Yet, seeing an I2C client
> messing directly with 'struct adapter' breaks so many abstractions, it
> hits my instant-NACK nerve ;)

Yes, accessing the function directly is really ugly. I was thinking about
augmenting the I2C interface and the regmap interface with 'sleep-free' and
'irq-less' functions or flags, but this requires changing a lot of code. So I
was just taking this shortcut. For follow up patches I will rework this.

Mit freundlichen Grüßen / Kind regards,
	Stefan Christ

> Thanks,
> 
>    Wolfram
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/6] Implement I2C restart handler

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 927 bytes --]


> > My idea was to introduce master_xfer_irqless or similar to struct
> > i2c_algorithm. I even discussed this shortly with Mark Brown how to
> > interact with regmap; but this was too long ago, so I forgot what we
> > agreed on :(
> 
> Great. I was looking for exactly this kind of feedback.
> 
> The term 'master_xfer_irqless' does not bring up anything on the internet and I
> didn't find anything on the list linux-i2c. Can you point me to the discussion,
> so I can read it up and work on it?

Sadly not. It was just an idea I had in my mind and some talking with
Mark at FOSDEM. Then priorities changed and I worked on something else.
You might want to look for threads where people implemented 'irqless' or
'polling' i2c transfers. I recall at least one discussion in the last
year, but can't seem to find it now :( Dunno if there is something
worthwhile in there, though.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]