Finally, here is the implementation Hans and I agreed on. Plus, all potential
users I could spot already converted. Renesas R-Car driver was added on top.
This series was tested on a Renesas Lager board (R-Car H2). I had to hack some
error cases into the code to verify the workings. Thanks for all the tests so
far. Of course, more testing never hurts ;)
Please comment, review, test... a branch can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/core-pm-helpers
Thanks,
Wolfram
Changes since RFC:
* don't use bitfields anymore but an unsigned long flags variable which is only
meant for flags to be changed when the adapter lock is held
* split the genereic accessor into _mark_suspended() and _mark_resumed()
* added kerneldoc and noted that using these helpers is optional
* documented -ESHUTDOWN as the error code when trying to transfer using an
already suspended adapter
* added acks from last series. Changes were minor enough to keep them. Please let
me know if you disagree.
Wolfram Sang (10):
i2c: add suspended flag and accessors for i2c adapters
i2c: reject new transfers when adapters are suspended
i2c: synquacer: remove unused is_suspended flag
i2c: brcmstb: use core helper to mark adapter suspended
i2c: zx2967: use core helper to mark adapter suspended
i2c: sprd: don't use pdev as variable name for struct device *
i2c: sprd: use core helper to mark adapter suspended
i2c: exynos5: use core helper to mark adapter suspended
i2c: s3c2410: use core helper to mark adapter suspended
i2c: rcar: add suspend/resume support
Documentation/i2c/fault-codes | 4 ++++
drivers/i2c/busses/i2c-brcmstb.c | 13 ++-----------
drivers/i2c/busses/i2c-exynos5.c | 11 ++---------
drivers/i2c/busses/i2c-rcar.c | 25 +++++++++++++++++++++++++
drivers/i2c/busses/i2c-s3c2410.c | 8 ++------
drivers/i2c/busses/i2c-sprd.c | 34 ++++++++++++----------------------
drivers/i2c/busses/i2c-synquacer.c | 5 -----
drivers/i2c/busses/i2c-zx2967.c | 8 ++------
drivers/i2c/i2c-core-base.c | 3 +++
include/linux/i2c.h | 34 ++++++++++++++++++++++++++++++++++
10 files changed, 86 insertions(+), 59 deletions(-)
--
2.11.0
A few drivers open code handling of suspended adapters. It could be
handled by the core, though, to ensure generic handling. This patch adds
the flag and accessor functions. The usage of these helpers is optional,
though. See the kerneldoc in this patch.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/i2c-core-base.c | 1 +
include/linux/i2c.h | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 28460f6a60cc..2c465455b2f6 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1232,6 +1232,7 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
if (!adap->lock_ops)
adap->lock_ops = &i2c_adapter_lock_ops;
+ adap->locked_flags = 0;
rt_mutex_init(&adap->bus_lock);
rt_mutex_init(&adap->mux_lock);
mutex_init(&adap->userspace_clients_lock);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 65b4eaed1d96..b7401c55dc83 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -680,6 +680,8 @@ struct i2c_adapter {
int timeout; /* in jiffies */
int retries;
struct device dev; /* the adapter device */
+ unsigned long locked_flags; /* owned by the I2C core */
+#define I2C_ALF_IS_SUSPENDED 0
int nr;
char name[48];
@@ -762,6 +764,38 @@ i2c_unlock_bus(struct i2c_adapter *adapter, unsigned int flags)
adapter->lock_ops->unlock_bus(adapter, flags);
}
+/**
+ * i2c_mark_adapter_suspended - Report suspended state of the adapter to the core
+ * @adap: Adapter to mark as suspended
+ *
+ * When using this helper to mark an adapter as suspended, the core will reject
+ * further transfers to this adapter. The usage of this helper is optional but
+ * recommended for devices having distinct handlers for systems suspend and
+ * runtime suspend. More complex devices are free to implement custom solutions
+ * to reject transfers when suspended.
+ */
+static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
+{
+ i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+ set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
+ i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+}
+
+/**
+ * i2c_mark_adapter_resumed - Report resumed state of the adapter to the core
+ * @adap: Adapter to mark as resumed
+ *
+ * When using this helper to mark an adapter as resumed, the core will allow
+ * further transfers to this adapter. See also further notes to
+ * @i2c_mark_adapter_suspended().
+ */
+static inline void i2c_mark_adapter_resumed(struct i2c_adapter *adap)
+{
+ i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+ clear_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
+ i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
+}
+
/*flags for the client struct: */
#define I2C_CLIENT_PEC 0x04 /* Use Packet Error Checking */
#define I2C_CLIENT_TEN 0x10 /* we have a ten bit chip address */
--
2.11.0
Because the adapter will be set up before every transaction anyhow, we
just need to mark it as suspended to the I2C core.
Signed-off-by: Hiromitsu Yamasaki <[email protected]>
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-rcar.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/i2c/busses/i2c-rcar.c b/drivers/i2c/busses/i2c-rcar.c
index 254e6219e538..1d6390eed861 100644
--- a/drivers/i2c/busses/i2c-rcar.c
+++ b/drivers/i2c/busses/i2c-rcar.c
@@ -1017,10 +1017,35 @@ static int rcar_i2c_remove(struct platform_device *pdev)
return 0;
}
+#ifdef CONFIG_PM_SLEEP
+static int rcar_i2c_suspend(struct device *dev)
+{
+ struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
+
+ i2c_mark_adapter_suspended(&priv->adap);
+ return 0;
+}
+
+static int rcar_i2c_resume(struct device *dev)
+{
+ struct rcar_i2c_priv *priv = dev_get_drvdata(dev);
+
+ i2c_mark_adapter_resumed(&priv->adap);
+ return 0;
+}
+
+static SIMPLE_DEV_PM_OPS(rcar_i2c_pm_ops, rcar_i2c_suspend, rcar_i2c_resume);
+
+#define DEV_PM_OPS (&rcar_i2c_pm_ops)
+#else
+#define DEV_PM_OPS NULL
+#endif /* CONFIG_PM_SLEEP */
+
static struct platform_driver rcar_i2c_driver = {
.driver = {
.name = "i2c-rcar",
.of_match_table = rcar_i2c_dt_ids,
+ .pm = DEV_PM_OPS,
},
.probe = rcar_i2c_probe,
.remove = rcar_i2c_remove,
--
2.11.0
Rejecting transfers should be handled by the core. Also, this will
ensure proper locking which was forgotten in this open coded version
and make sure resume mark is set after enabling clocks (not before).
Signed-off-by: Wolfram Sang <[email protected]>
Acked-by: Shawn Guo <[email protected]>
---
drivers/i2c/busses/i2c-zx2967.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-zx2967.c b/drivers/i2c/busses/i2c-zx2967.c
index b8f9e020d80e..7b98d97da3c6 100644
--- a/drivers/i2c/busses/i2c-zx2967.c
+++ b/drivers/i2c/busses/i2c-zx2967.c
@@ -66,7 +66,6 @@ struct zx2967_i2c {
int msg_rd;
u8 *cur_trans;
u8 access_cnt;
- bool is_suspended;
int error;
};
@@ -313,9 +312,6 @@ static int zx2967_i2c_xfer(struct i2c_adapter *adap,
int ret;
int i;
- if (i2c->is_suspended)
- return -EBUSY;
-
zx2967_set_addr(i2c, msgs->addr);
for (i = 0; i < num; i++) {
@@ -470,7 +466,7 @@ static int __maybe_unused zx2967_i2c_suspend(struct device *dev)
{
struct zx2967_i2c *i2c = dev_get_drvdata(dev);
- i2c->is_suspended = true;
+ i2c_mark_adapter_suspended(&i2c->adap);
clk_disable_unprepare(i2c->clk);
return 0;
@@ -480,8 +476,8 @@ static int __maybe_unused zx2967_i2c_resume(struct device *dev)
{
struct zx2967_i2c *i2c = dev_get_drvdata(dev);
- i2c->is_suspended = false;
clk_prepare_enable(i2c->clk);
+ i2c_mark_adapter_resumed(&i2c->adap);
return 0;
}
--
2.11.0
The pointer to a device is usually named 'dev'. These 'pdev' here look
much like copy&paste errors. Fix them to avoid confusion.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-sprd.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index a94e724f51dc..e266d8a713d9 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -586,40 +586,40 @@ static int sprd_i2c_remove(struct platform_device *pdev)
return 0;
}
-static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
+static int __maybe_unused sprd_i2c_suspend_noirq(struct device *dev)
{
- struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+ struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
i2c_dev->is_suspended = true;
i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
- return pm_runtime_force_suspend(pdev);
+ return pm_runtime_force_suspend(dev);
}
-static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
+static int __maybe_unused sprd_i2c_resume_noirq(struct device *dev)
{
- struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+ struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
i2c_dev->is_suspended = false;
i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
- return pm_runtime_force_resume(pdev);
+ return pm_runtime_force_resume(dev);
}
-static int __maybe_unused sprd_i2c_runtime_suspend(struct device *pdev)
+static int __maybe_unused sprd_i2c_runtime_suspend(struct device *dev)
{
- struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+ struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
clk_disable_unprepare(i2c_dev->clk);
return 0;
}
-static int __maybe_unused sprd_i2c_runtime_resume(struct device *pdev)
+static int __maybe_unused sprd_i2c_runtime_resume(struct device *dev)
{
- struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
+ struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
int ret;
ret = clk_prepare_enable(i2c_dev->clk);
--
2.11.0
Rejecting transfers should be handled by the core.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-sprd.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index e266d8a713d9..961123529678 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -86,7 +86,6 @@ struct sprd_i2c {
u32 count;
int irq;
int err;
- bool is_suspended;
};
static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
@@ -284,9 +283,6 @@ static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
int im, ret;
- if (i2c_dev->is_suspended)
- return -EBUSY;
-
ret = pm_runtime_get_sync(i2c_dev->dev);
if (ret < 0)
return ret;
@@ -590,10 +586,7 @@ static int __maybe_unused sprd_i2c_suspend_noirq(struct device *dev)
{
struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
- i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
- i2c_dev->is_suspended = true;
- i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
-
+ i2c_mark_adapter_suspended(&i2c_dev->adap);
return pm_runtime_force_suspend(dev);
}
@@ -601,10 +594,7 @@ static int __maybe_unused sprd_i2c_resume_noirq(struct device *dev)
{
struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
- i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
- i2c_dev->is_suspended = false;
- i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
-
+ i2c_mark_adapter_resumed(&i2c_dev->adap);
return pm_runtime_force_resume(dev);
}
--
2.11.0
Rejecting transfers should be handled by the core. Also, this will
ensure proper locking which was forgotten in this open coded version.
Signed-off-by: Wolfram Sang <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
---
drivers/i2c/busses/i2c-s3c2410.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 2f2e28d60ef5..53bc021f4a5a 100644
--- a/drivers/i2c/busses/i2c-s3c2410.c
+++ b/drivers/i2c/busses/i2c-s3c2410.c
@@ -104,7 +104,6 @@ enum s3c24xx_i2c_state {
struct s3c24xx_i2c {
wait_queue_head_t wait;
kernel_ulong_t quirks;
- unsigned int suspended:1;
struct i2c_msg *msg;
unsigned int msg_num;
@@ -703,9 +702,6 @@ static int s3c24xx_i2c_doxfer(struct s3c24xx_i2c *i2c,
unsigned long timeout;
int ret;
- if (i2c->suspended)
- return -EIO;
-
ret = s3c24xx_i2c_set_master(i2c);
if (ret != 0) {
dev_err(i2c->dev, "cannot get bus (error %d)\n", ret);
@@ -1246,7 +1242,7 @@ static int s3c24xx_i2c_suspend_noirq(struct device *dev)
{
struct s3c24xx_i2c *i2c = dev_get_drvdata(dev);
- i2c->suspended = 1;
+ i2c_mark_adapter_suspended(&i2c->adap);
if (!IS_ERR(i2c->sysreg))
regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &i2c->sys_i2c_cfg);
@@ -1267,7 +1263,7 @@ static int s3c24xx_i2c_resume_noirq(struct device *dev)
return ret;
s3c24xx_i2c_init(i2c);
clk_disable(i2c->clk);
- i2c->suspended = 0;
+ i2c_mark_adapter_resumed(&i2c->adap);
return 0;
}
--
2.11.0
On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> +{
> + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> + set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> +}
This looks like a duplication of the is_suspended flag in struct dev_pm_info.
Any reason why you can't use that? If so, it would be good to document the
reason in the commit message.
If the point is to constrain refusal of transfers in suspended state to
certain drivers, those drivers could opt in to that functionality by
setting a flag, and the i2c core could then gate refusal based on
that flag and the is_suspended flag in struct dev_pm_info.
Also, why is it necessary to take a lock to perform an atomic bitop?
(Sorry if that's a dumb question, seems non-obvious to me.)
Thanks,
Lukas
Hi,
On 19-12-18 18:22, Lukas Wunner wrote:
> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
>> +{
>> + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>> + set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
>> + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>> +}
>
> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
> Any reason why you can't use that? If so, it would be good to document the
> reason in the commit message.
Oh, that is a very good point and that one only gets set on system suspend
and not on resume suspend, working around the problems with the i2c-designware
driver.
I think this might be as simple as adding:
if (WARN_ON(adap->dev.parent->power.is_suspended))
return -ESHUTDOWN;
To __i2c_transfer (and document the -ESHUTDOWN) combined with removing
all the now no longer DIY code from various bus drivers.
Regards,
Hans
>
> If the point is to constrain refusal of transfers in suspended state to
> certain drivers, those drivers could opt in to that functionality by
> setting a flag, and the i2c core could then gate refusal based on
> that flag and the is_suspended flag in struct dev_pm_info.
>
> Also, why is it necessary to take a lock to perform an atomic bitop?
> (Sorry if that's a dumb question, seems non-obvious to me.)
>
> Thanks,
>
> Lukas
>
Using the new 'is_suspended' flag, we now reject new transfers if the
adapter is already marked suspended.
Signed-off-by: Wolfram Sang <[email protected]>
---
Documentation/i2c/fault-codes | 4 ++++
drivers/i2c/i2c-core-base.c | 2 ++
2 files changed, 6 insertions(+)
diff --git a/Documentation/i2c/fault-codes b/Documentation/i2c/fault-codes
index 47c25abb7d52..0cee0fc545b4 100644
--- a/Documentation/i2c/fault-codes
+++ b/Documentation/i2c/fault-codes
@@ -112,6 +112,10 @@ EPROTO
case is when the length of an SMBus block data response
(from the SMBus slave) is outside the range 1-32 bytes.
+ESHUTDOWN
+ Returned when a transfer was requested using an adapter
+ which is already suspended.
+
ETIMEDOUT
This is returned by drivers when an operation took too much
time, and was aborted before it completed.
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 2c465455b2f6..926ca0a7477f 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1866,6 +1866,8 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
if (WARN_ON(!msgs || num < 1))
return -EINVAL;
+ if (WARN_ON(test_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags)))
+ return -ESHUTDOWN;
if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
return -EOPNOTSUPP;
--
2.11.0
Rejecting transfers should be handled by the core.
Signed-off-by: Wolfram Sang <[email protected]>
Reviewed-by: Kamal Dasu <[email protected]>
---
drivers/i2c/busses/i2c-brcmstb.c | 13 ++-----------
1 file changed, 2 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 826d32049996..f4d862234980 100644
--- a/drivers/i2c/busses/i2c-brcmstb.c
+++ b/drivers/i2c/busses/i2c-brcmstb.c
@@ -170,7 +170,6 @@ struct brcmstb_i2c_dev {
struct bsc_regs *bsc_regmap;
struct i2c_adapter adapter;
struct completion done;
- bool is_suspended;
u32 clk_freq_hz;
int data_regsz;
};
@@ -467,9 +466,6 @@ static int brcmstb_i2c_xfer(struct i2c_adapter *adapter,
int xfersz = brcmstb_i2c_get_xfersz(dev);
u32 cond, cond_per_msg;
- if (dev->is_suspended)
- return -EBUSY;
-
/* Loop through all messages */
for (i = 0; i < num; i++) {
pmsg = &msgs[i];
@@ -689,10 +685,7 @@ static int brcmstb_i2c_suspend(struct device *dev)
{
struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
- i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
- i2c_dev->is_suspended = true;
- i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
-
+ i2c_mark_adapter_suspended(&i2c_dev->adapter);
return 0;
}
@@ -700,10 +693,8 @@ static int brcmstb_i2c_resume(struct device *dev)
{
struct brcmstb_i2c_dev *i2c_dev = dev_get_drvdata(dev);
- i2c_lock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
brcmstb_i2c_set_bsc_reg_defaults(i2c_dev);
- i2c_dev->is_suspended = false;
- i2c_unlock_bus(&i2c_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
+ i2c_mark_adapter_resumed(&i2c_dev->adapter);
return 0;
}
--
2.11.0
This flag was defined and checked but never set a value. Remove it.
Signed-off-by: Wolfram Sang <[email protected]>
Acked-by: Ard Biesheuvel <[email protected]>
---
drivers/i2c/busses/i2c-synquacer.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/i2c/busses/i2c-synquacer.c b/drivers/i2c/busses/i2c-synquacer.c
index 2184b7c3580e..d18b0941b71a 100644
--- a/drivers/i2c/busses/i2c-synquacer.c
+++ b/drivers/i2c/busses/i2c-synquacer.c
@@ -144,8 +144,6 @@ struct synquacer_i2c {
u32 timeout_ms;
enum i2c_state state;
struct i2c_adapter adapter;
-
- bool is_suspended;
};
static inline int is_lastmsg(struct synquacer_i2c *i2c)
@@ -316,9 +314,6 @@ static int synquacer_i2c_doxfer(struct synquacer_i2c *i2c,
unsigned long timeout;
int ret;
- if (i2c->is_suspended)
- return -EBUSY;
-
synquacer_i2c_hw_init(i2c);
bsr = readb(i2c->base + SYNQUACER_I2C_REG_BSR);
if (bsr & SYNQUACER_I2C_BSR_BB) {
--
2.11.0
Rejecting transfers should be handled by the core. Also, this will
ensure proper locking which was forgotten in this open coded version.
Signed-off-by: Wolfram Sang <[email protected]>
Tested-by: Marek Szyprowski <[email protected]>
---
drivers/i2c/busses/i2c-exynos5.c | 11 ++---------
1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index c1ce2299a76e..41de4ee409b6 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -183,7 +183,6 @@ enum i2c_type_exynos {
struct exynos5_i2c {
struct i2c_adapter adap;
- unsigned int suspended:1;
struct i2c_msg *msg;
struct completion msg_complete;
@@ -715,11 +714,6 @@ static int exynos5_i2c_xfer(struct i2c_adapter *adap,
struct exynos5_i2c *i2c = adap->algo_data;
int i, ret;
- if (i2c->suspended) {
- dev_err(i2c->dev, "HS-I2C is not initialized.\n");
- return -EIO;
- }
-
ret = clk_enable(i2c->clk);
if (ret)
return ret;
@@ -847,8 +841,7 @@ static int exynos5_i2c_suspend_noirq(struct device *dev)
{
struct exynos5_i2c *i2c = dev_get_drvdata(dev);
- i2c->suspended = 1;
-
+ i2c_mark_adapter_suspended(&i2c->adap);
clk_unprepare(i2c->clk);
return 0;
@@ -871,7 +864,7 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
exynos5_i2c_init(i2c);
clk_disable(i2c->clk);
- i2c->suspended = 0;
+ i2c_mark_adapter_resumed(&i2c->adap);
return 0;
}
--
2.11.0
Hi Lukas, Hans,
On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
> Hi,
>
> On 19-12-18 18:22, Lukas Wunner wrote:
> > On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> > > +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> > > +{
> > > + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> > > + set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> > > + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> > > +}
> >
> > This looks like a duplication of the is_suspended flag in struct dev_pm_info.
> > Any reason why you can't use that? If so, it would be good to document the
> > reason in the commit message.
>
> Oh, that is a very good point and that one only gets set on system suspend
> and not on resume suspend, working around the problems with the i2c-designware
Just to make it clear: you mean runtime suspend, not resume suspend, or?
> driver.
>
> I think this might be as simple as adding:
>
> if (WARN_ON(adap->dev.parent->power.is_suspended))
> return -ESHUTDOWN;
I have seen this flag but decided against it. One reason is because it
is marked as "PM core only". The other reason is that it doesn't know
about the adapter lock. It might get set while a transfer is on going.
Or even right after the suggested if-block above. The code from this
series gets the mutex first which ensures that on going transfers are
completed and no new ones are started in parallel.
Unless I am totally overlooking something...
Thanks,
Wolfram
Hi,
On 19-12-18 23:33, Wolfram Sang wrote:
> Hi Lukas, Hans,
>
> On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 19-12-18 18:22, Lukas Wunner wrote:
>>> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
>>>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
>>>> +{
>>>> + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>> + set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
>>>> + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>> +}
>>>
>>> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
>>> Any reason why you can't use that? If so, it would be good to document the
>>> reason in the commit message.
>>
>> Oh, that is a very good point and that one only gets set on system suspend
>> and not on resume suspend, working around the problems with the i2c-designware
>
> Just to make it clear: you mean runtime suspend, not resume suspend, or?
Yes I mean runtime-suspend, sorry.
>> driver.
>>
>> I think this might be as simple as adding:
>>
>> if (WARN_ON(adap->dev.parent->power.is_suspended))
>> return -ESHUTDOWN;
>
> I have seen this flag but decided against it. One reason is because it
> is marked as "PM core only".
Right and we definitely should not be touching it, but reading it should
be fine.
> The other reason is that it doesn't know
> about the adapter lock. It might get set while a transfer is on going.
> Or even right after the suggested if-block above. The code from this
> series gets the mutex first which ensures that on going transfers are
> completed and no new ones are started in parallel.
>
> Unless I am totally overlooking something...
No you are right, there is a race here, but I don't think we are likely to
hit that race. Normally there won't be any ongoing i2c-transfers during
a system suspend and more over, the goal of adding this check is to help
find problems, so even if the check sometimes does not trigger because
of the race that is not really a big deal.
I think we need to get really unlucky to have both a suspend ordering
problem in the first case (already a somewhat rare thing) combined with
hitting this race in such a way *each time* that we don't trigger the
WARN_ON.
To me this seems a case of perfect being the enemy of good. When we
first started discussing this you wanted to not have to modify the
adapter/bus drivers for the check, using adap->dev.parent->power.is_suspended
gives us that and it will also work for complex cases like
the i2c-designware case, so I believe the benefits outway the downsides.
Regards,
Hans
On Thursday, December 20, 2018 11:00:29 AM CET Hans de Goede wrote:
> Hi,
>
> On 19-12-18 23:33, Wolfram Sang wrote:
> > Hi Lukas, Hans,
> >
> > On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 19-12-18 18:22, Lukas Wunner wrote:
> >>> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
> >>>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
> >>>> +{
> >>>> + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> >>>> + set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
> >>>> + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
> >>>> +}
> >>>
> >>> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
> >>> Any reason why you can't use that? If so, it would be good to document the
> >>> reason in the commit message.
> >>
> >> Oh, that is a very good point and that one only gets set on system suspend
> >> and not on resume suspend, working around the problems with the i2c-designware
> >
> > Just to make it clear: you mean runtime suspend, not resume suspend, or?
>
> Yes I mean runtime-suspend, sorry.
The power.is_suspended flag is about system-wide suspend, however.
Hi Wolfram,
On Thu, 20 Dec 2018 at 00:48, Wolfram Sang
<[email protected]> wrote:
>
> Rejecting transfers should be handled by the core.
>
> Signed-off-by: Wolfram Sang <[email protected]>
Great to see the I2C core can handle this issue.
Reviewed-by: Baolin Wang <[email protected]>
> ---
> drivers/i2c/busses/i2c-sprd.c | 14 ++------------
> 1 file changed, 2 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index e266d8a713d9..961123529678 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -86,7 +86,6 @@ struct sprd_i2c {
> u32 count;
> int irq;
> int err;
> - bool is_suspended;
> };
>
> static void sprd_i2c_set_count(struct sprd_i2c *i2c_dev, u32 count)
> @@ -284,9 +283,6 @@ static int sprd_i2c_master_xfer(struct i2c_adapter *i2c_adap,
> struct sprd_i2c *i2c_dev = i2c_adap->algo_data;
> int im, ret;
>
> - if (i2c_dev->is_suspended)
> - return -EBUSY;
> -
> ret = pm_runtime_get_sync(i2c_dev->dev);
> if (ret < 0)
> return ret;
> @@ -590,10 +586,7 @@ static int __maybe_unused sprd_i2c_suspend_noirq(struct device *dev)
> {
> struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
>
> - i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> - i2c_dev->is_suspended = true;
> - i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> -
> + i2c_mark_adapter_suspended(&i2c_dev->adap);
> return pm_runtime_force_suspend(dev);
> }
>
> @@ -601,10 +594,7 @@ static int __maybe_unused sprd_i2c_resume_noirq(struct device *dev)
> {
> struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
>
> - i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> - i2c_dev->is_suspended = false;
> - i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> -
> + i2c_mark_adapter_resumed(&i2c_dev->adap);
> return pm_runtime_force_resume(dev);
> }
>
> --
> 2.11.0
>
--
Baolin Wang
Best Regards
Hi Wolfram,
On Thu, 20 Dec 2018 at 00:48, Wolfram Sang
<[email protected]> wrote:
>
> The pointer to a device is usually named 'dev'. These 'pdev' here look
> much like copy&paste errors. Fix them to avoid confusion.
>
> Signed-off-by: Wolfram Sang <[email protected]>
Thanks for fixing the copy&paste errors.
Reviewed-by: Baolin Wang <[email protected]>
> ---
> drivers/i2c/busses/i2c-sprd.c | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
> index a94e724f51dc..e266d8a713d9 100644
> --- a/drivers/i2c/busses/i2c-sprd.c
> +++ b/drivers/i2c/busses/i2c-sprd.c
> @@ -586,40 +586,40 @@ static int sprd_i2c_remove(struct platform_device *pdev)
> return 0;
> }
>
> -static int __maybe_unused sprd_i2c_suspend_noirq(struct device *pdev)
> +static int __maybe_unused sprd_i2c_suspend_noirq(struct device *dev)
> {
> - struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> + struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
>
> i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = true;
> i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
>
> - return pm_runtime_force_suspend(pdev);
> + return pm_runtime_force_suspend(dev);
> }
>
> -static int __maybe_unused sprd_i2c_resume_noirq(struct device *pdev)
> +static int __maybe_unused sprd_i2c_resume_noirq(struct device *dev)
> {
> - struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> + struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
>
> i2c_lock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
> i2c_dev->is_suspended = false;
> i2c_unlock_bus(&i2c_dev->adap, I2C_LOCK_ROOT_ADAPTER);
>
> - return pm_runtime_force_resume(pdev);
> + return pm_runtime_force_resume(dev);
> }
>
> -static int __maybe_unused sprd_i2c_runtime_suspend(struct device *pdev)
> +static int __maybe_unused sprd_i2c_runtime_suspend(struct device *dev)
> {
> - struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> + struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
>
> clk_disable_unprepare(i2c_dev->clk);
>
> return 0;
> }
>
> -static int __maybe_unused sprd_i2c_runtime_resume(struct device *pdev)
> +static int __maybe_unused sprd_i2c_runtime_resume(struct device *dev)
> {
> - struct sprd_i2c *i2c_dev = dev_get_drvdata(pdev);
> + struct sprd_i2c *i2c_dev = dev_get_drvdata(dev);
> int ret;
>
> ret = clk_prepare_enable(i2c_dev->clk);
> --
> 2.11.0
>
--
Baolin Wang
Best Regards
Hi,
On 20-12-18 22:09, Rafael J. Wysocki wrote:
> On Thursday, December 20, 2018 11:00:29 AM CET Hans de Goede wrote:
>> Hi,
>>
>> On 19-12-18 23:33, Wolfram Sang wrote:
>>> Hi Lukas, Hans,
>>>
>>> On Wed, Dec 19, 2018 at 07:36:54PM +0100, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 19-12-18 18:22, Lukas Wunner wrote:
>>>>> On Wed, Dec 19, 2018 at 05:48:17PM +0100, Wolfram Sang wrote:
>>>>>> +static inline void i2c_mark_adapter_suspended(struct i2c_adapter *adap)
>>>>>> +{
>>>>>> + i2c_lock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>>>> + set_bit(I2C_ALF_IS_SUSPENDED, &adap->locked_flags);
>>>>>> + i2c_unlock_bus(adap, I2C_LOCK_ROOT_ADAPTER);
>>>>>> +}
>>>>>
>>>>> This looks like a duplication of the is_suspended flag in struct dev_pm_info.
>>>>> Any reason why you can't use that? If so, it would be good to document the
>>>>> reason in the commit message.
>>>>
>>>> Oh, that is a very good point and that one only gets set on system suspend
>>>> and not on resume suspend, working around the problems with the i2c-designware
>>>
>>> Just to make it clear: you mean runtime suspend, not resume suspend, or?
>>
>> Yes I mean runtime-suspend, sorry.
>
> The power.is_suspended flag is about system-wide suspend, however.
Right, which is why it is good for us to use, when runtime-suspend the
i2c-adapter drivers transfer function will do a runtime_pm_get and all
is well, we want to check for someone trying to do i2c-transfers on
the adapter while it is system-suspended, since then the runtime_pm_get
is a no-op and things fail.
So for this use case it is a good thing that power.is_suspended flag is about
system-wide suspend (which is what I was trying to say in the first place).
Regards,
Hans
> > > I think this might be as simple as adding:
> > >
> > > if (WARN_ON(adap->dev.parent->power.is_suspended))
> > > return -ESHUTDOWN;
Peter, I think this should work for muxes, too, or? The i2c_transfer()
call to the mux will not be rejected, but it will be later when we reach
the root adapter. And then the error code will be pushed down the tree
until we arrive at the mux again. So, the rejection will not happen at
the earliest time, but it will happen. Is my understanding correct?
> > I have seen this flag but decided against it. One reason is because it
> > is marked as "PM core only".
>
> Right and we definitely should not be touching it, but reading it should
> be fine.
Seems like it. So far, no rejection from the other PM people :)
> No you are right, there is a race here, but I don't think we are likely to
> hit that race. Normally there won't be any ongoing i2c-transfers during
> a system suspend and more over, the goal of adding this check is to help
> find problems, so even if the check sometimes does not trigger because
> of the race that is not really a big deal.
You are right that the impact of a missed detection is not fatal. That
helps. The low likeliness was not an argument for me, though, because
detecting rare things is very important IMO. Because, well, they are
rare and especially hard to debug.
> I think we need to get really unlucky to have both a suspend ordering
> problem in the first case (already a somewhat rare thing) combined with
> hitting this race in such a way *each time* that we don't trigger the
> WARN_ON.
And here you convinced me: even if we miss a detection once, I agree it
is super super unlikely to hit the race every time.
> To me this seems a case of perfect being the enemy of good. When we
> first started discussing this you wanted to not have to modify the
> adapter/bus drivers for the check, using adap->dev.parent->power.is_suspended
> gives us that and it will also work for complex cases like
> the i2c-designware case, so I believe the benefits outway the downsides.
I'll try it.
Hi Peter,
> Yes, I agree with that analysis. All mux actions eventually end up with
Good!
> an __i2c_transfer() call on the relevant root adapter. Hmm, but not *all*
> calls. How about SMBus adapters? Should there not be a similar WARN_ON
> in __i2c_smbus_xfer?
Yes, there should. Yeah, that's what I like about our way of hacking; I
somewhen thought to not forget about SMBus, nevertheless forgot about it
again, and then there is someone else to remind me \o/
Thanks,
Wolfram
On 2018-12-21 15:50, Wolfram Sang wrote:
>
>>>> I think this might be as simple as adding:
>>>>
>>>> if (WARN_ON(adap->dev.parent->power.is_suspended))
>>>> return -ESHUTDOWN;
>
> Peter, I think this should work for muxes, too, or? The i2c_transfer()
> call to the mux will not be rejected, but it will be later when we reach
> the root adapter. And then the error code will be pushed down the tree
> until we arrive at the mux again. So, the rejection will not happen at
> the earliest time, but it will happen. Is my understanding correct?
Yes, I agree with that analysis. All mux actions eventually end up with
an __i2c_transfer() call on the relevant root adapter. Hmm, but not *all*
calls. How about SMBus adapters? Should there not be a similar WARN_ON
in __i2c_smbus_xfer?
But maybe that's not applicable for some reason? Just asking...
Cheers,
Peter
On Wed, Dec 19, 2018 at 05:48:16PM +0100, Wolfram Sang wrote:
> Finally, here is the implementation Hans and I agreed on. Plus, all potential
> users I could spot already converted. Renesas R-Car driver was added on top.
> This series was tested on a Renesas Lager board (R-Car H2). I had to hack some
> error cases into the code to verify the workings. Thanks for all the tests so
> far. Of course, more testing never hurts ;)
>
> Please comment, review, test... a branch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/core-pm-helpers
>
> Thanks,
>
> Wolfram
>
> Changes since RFC:
>
> * don't use bitfields anymore but an unsigned long flags variable which is only
> meant for flags to be changed when the adapter lock is held
>
> * split the genereic accessor into _mark_suspended() and _mark_resumed()
>
> * added kerneldoc and noted that using these helpers is optional
>
> * documented -ESHUTDOWN as the error code when trying to transfer using an
> already suspended adapter
>
> * added acks from last series. Changes were minor enough to keep them. Please let
> me know if you disagree.
>
>
> Wolfram Sang (10):
> i2c: add suspended flag and accessors for i2c adapters
> i2c: reject new transfers when adapters are suspended
> i2c: synquacer: remove unused is_suspended flag
> i2c: brcmstb: use core helper to mark adapter suspended
> i2c: zx2967: use core helper to mark adapter suspended
> i2c: sprd: don't use pdev as variable name for struct device *
> i2c: sprd: use core helper to mark adapter suspended
> i2c: exynos5: use core helper to mark adapter suspended
> i2c: s3c2410: use core helper to mark adapter suspended
> i2c: rcar: add suspend/resume support
After a long road, I applied this series now to for-next. I only
squashed patches 1+2 to have the functionality in one place. And fixed a
typo in patch 1. Thanks to the testers and reviewers, and especially
Hans, for the input and discussions.
This will now get a full test cycle in -next. Let's see what happens...
And let's move on to how to add 'very late transfers without irqs' on
top of this series.
Happy hacking,
Wolfram