Here is the new version without specific I2C helpers but using the
'is_suspended' flag from the PM core. I didn't like messing with the
flag directly, so I did a helper in patch 1. So far, I like the
approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c
rejected rightfully too later transfers without further modifications.
Tested on a Renesas Lager board (R-Car H2).
I dropped a few Tested-by tags because I think this approach is too
different from V1 to keep them. I hope you guys can have a look again.
Thanks for all the testing, so far!
A branch can be found here:
git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended
If this series is acceptable, I'd suggest to take it via my i2c tree
after rc1. And then I'll provide an immutable branch for the PM tree to
pick. Let me know if this works for you.
And thanks to Renesas for funding this work!
Thanks and kind regards,
Wolfram
Wolfram Sang (9):
PM / core: add helper to return suspend status of a device
i2c: reject new transfers when adapters are suspended
i2c: synquacer: remove unused is_suspended flag
i2c: brcmstb: don't open code to reject transfers when suspended
i2c: zx2967: don't open code to reject transfers when suspended
i2c: sprd: don't use pdev as variable name for struct device *
i2c: sprd: don't open code to reject transfers when suspended
i2c: exynos5: don't open code to reject transfers when suspended
i2c: s3c2410: don't open code to reject transfers when suspended
Documentation/i2c/fault-codes | 4 ++++
drivers/i2c/busses/i2c-brcmstb.c | 22 +-------------------
drivers/i2c/busses/i2c-exynos5.c | 10 ----------
drivers/i2c/busses/i2c-s3c2410.c | 7 -------
drivers/i2c/busses/i2c-sprd.c | 32 ++++++++++--------------------
drivers/i2c/busses/i2c-synquacer.c | 5 -----
drivers/i2c/busses/i2c-zx2967.c | 8 --------
drivers/i2c/i2c-core-base.c | 3 +++
drivers/i2c/i2c-core-smbus.c | 4 ++++
include/linux/device.h | 5 +++++
10 files changed, 27 insertions(+), 73 deletions(-)
--
2.19.1
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-brcmstb.c | 22 +---------------------
1 file changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/i2c/busses/i2c-brcmstb.c b/drivers/i2c/busses/i2c-brcmstb.c
index 826d32049996..f6fd5dd638ac 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];
@@ -685,32 +681,16 @@ static int brcmstb_i2c_remove(struct platform_device *pdev)
}
#ifdef CONFIG_PM_SLEEP
-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);
-
- return 0;
-}
-
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);
-
return 0;
}
#endif
-static SIMPLE_DEV_PM_OPS(brcmstb_i2c_pm, brcmstb_i2c_suspend,
- brcmstb_i2c_resume);
+static SIMPLE_DEV_PM_OPS(brcmstb_i2c_pm, NULL, brcmstb_i2c_resume);
static const struct of_device_id brcmstb_i2c_of_match[] = {
{.compatible = "brcm,brcmstb-i2c"},
--
2.19.1
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-sprd.c | 12 ------------
1 file changed, 12 deletions(-)
diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index e266d8a713d9..fa2cae3460a6 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,6 @@ 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);
-
return pm_runtime_force_suspend(dev);
}
@@ -601,10 +593,6 @@ 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);
-
return pm_runtime_force_resume(dev);
}
--
2.19.1
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-zx2967.c | 8 --------
1 file changed, 8 deletions(-)
diff --git a/drivers/i2c/busses/i2c-zx2967.c b/drivers/i2c/busses/i2c-zx2967.c
index b8f9e020d80e..cb1f45647a8b 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,9 +466,7 @@ static int __maybe_unused zx2967_i2c_suspend(struct device *dev)
{
struct zx2967_i2c *i2c = dev_get_drvdata(dev);
- i2c->is_suspended = true;
clk_disable_unprepare(i2c->clk);
-
return 0;
}
@@ -480,9 +474,7 @@ 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);
-
return 0;
}
--
2.19.1
Using the 'is_suspended' flag from the PM core, we now reject new
transfers if the parent of the adapter is already marked suspended.
Signed-off-by: Wolfram Sang <[email protected]>
---
Documentation/i2c/fault-codes | 4 ++++
drivers/i2c/i2c-core-base.c | 3 +++
drivers/i2c/i2c-core-smbus.c | 4 ++++
3 files changed, 11 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 28460f6a60cc..3ce238b782f3 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1865,6 +1865,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
if (WARN_ON(!msgs || num < 1))
return -EINVAL;
+ if (WARN(device_is_suspended(adap->dev.parent),
+ "Accessing already suspended I2C/SMBus adapter"))
+ return -ESHUTDOWN;
if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
return -EOPNOTSUPP;
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index 9cd66cabb84f..e0f7f22feabd 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -547,6 +547,10 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
int try;
s32 res;
+ if (WARN(device_is_suspended(adapter->dev.parent),
+ "Accessing already suspended I2C/SMBus adapter"))
+ return -ESHUTDOWN;
+
/* If enabled, the following two tracepoints are conditional on
* read_write and protocol.
*/
--
2.19.1
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.19.1
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]>
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.19.1
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-exynos5.c | 10 ----------
1 file changed, 10 deletions(-)
diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index c1ce2299a76e..81b9c3cf4e75 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,10 +841,7 @@ static int exynos5_i2c_suspend_noirq(struct device *dev)
{
struct exynos5_i2c *i2c = dev_get_drvdata(dev);
- i2c->suspended = 1;
-
clk_unprepare(i2c->clk);
-
return 0;
}
@@ -871,7 +862,6 @@ static int exynos5_i2c_resume_noirq(struct device *dev)
exynos5_i2c_init(i2c);
clk_disable(i2c->clk);
- i2c->suspended = 0;
return 0;
}
--
2.19.1
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <[email protected]>
---
drivers/i2c/busses/i2c-s3c2410.c | 7 -------
1 file changed, 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-s3c2410.c b/drivers/i2c/busses/i2c-s3c2410.c
index 2f2e28d60ef5..eeccd19b5f42 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,8 +1242,6 @@ static int s3c24xx_i2c_suspend_noirq(struct device *dev)
{
struct s3c24xx_i2c *i2c = dev_get_drvdata(dev);
- i2c->suspended = 1;
-
if (!IS_ERR(i2c->sysreg))
regmap_read(i2c->sysreg, EXYNOS5_SYS_I2C_CFG, &i2c->sys_i2c_cfg);
@@ -1267,7 +1261,6 @@ static int s3c24xx_i2c_resume_noirq(struct device *dev)
return ret;
s3c24xx_i2c_init(i2c);
clk_disable(i2c->clk);
- i2c->suspended = 0;
return 0;
}
--
2.19.1
I2C core would like this to reject transfers on an already suspended
adapter.
Signed-off-by: Wolfram Sang <[email protected]>
---
include/linux/device.h | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/include/linux/device.h b/include/linux/device.h
index 1b25c7a43f4c..e63d3e06989f 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -1138,6 +1138,11 @@ static inline int device_is_registered(struct device *dev)
return dev->kobj.state_in_sysfs;
}
+static inline bool device_is_suspended(struct device *dev)
+{
+ return dev->power.is_suspended;
+}
+
static inline void device_enable_async_suspend(struct device *dev)
{
if (!dev->power.is_prepared)
--
2.19.1
Hi,
On 22-12-18 21:26, Wolfram Sang wrote:
> Here is the new version without specific I2C helpers but using the
> 'is_suspended' flag from the PM core. I didn't like messing with the
> flag directly, so I did a helper in patch 1. So far, I like the
> approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c
> rejected rightfully too later transfers without further modifications.
> Tested on a Renesas Lager board (R-Car H2).
>
> I dropped a few Tested-by tags because I think this approach is too
> different from V1 to keep them. I hope you guys can have a look again.
> Thanks for all the testing, so far!
>
> A branch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended
>
> If this series is acceptable, I'd suggest to take it via my i2c tree
> after rc1. And then I'll provide an immutable branch for the PM tree to
> pick. Let me know if this works for you.
>
> And thanks to Renesas for funding this work!
Series looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
>
> Thanks and kind regards,
>
> Wolfram
>
> Wolfram Sang (9):
> PM / core: add helper to return suspend status of a device
> i2c: reject new transfers when adapters are suspended
> i2c: synquacer: remove unused is_suspended flag
> i2c: brcmstb: don't open code to reject transfers when suspended
> i2c: zx2967: don't open code to reject transfers when suspended
> i2c: sprd: don't use pdev as variable name for struct device *
> i2c: sprd: don't open code to reject transfers when suspended
> i2c: exynos5: don't open code to reject transfers when suspended
> i2c: s3c2410: don't open code to reject transfers when suspended
>
> Documentation/i2c/fault-codes | 4 ++++
> drivers/i2c/busses/i2c-brcmstb.c | 22 +-------------------
> drivers/i2c/busses/i2c-exynos5.c | 10 ----------
> drivers/i2c/busses/i2c-s3c2410.c | 7 -------
> drivers/i2c/busses/i2c-sprd.c | 32 ++++++++++--------------------
> drivers/i2c/busses/i2c-synquacer.c | 5 -----
> drivers/i2c/busses/i2c-zx2967.c | 8 --------
> drivers/i2c/i2c-core-base.c | 3 +++
> drivers/i2c/i2c-core-smbus.c | 4 ++++
> include/linux/device.h | 5 +++++
> 10 files changed, 27 insertions(+), 73 deletions(-)
>
Hi,
Thank you for this new version.
On 22-12-18 21:26, Wolfram Sang wrote:
> Using the 'is_suspended' flag from the PM core, we now reject new
> transfers if the parent of the adapter is already marked suspended.
I've been running some tests and I'm afraid that those have
exposed multiple issues:
1) It seems that adap->dev.parent can be NULL in some cases, specifically
this patch causes the i915 driver to crash during probe on an Apollo Lake
laptop with an eDP panel. I've attached a fixup patch which fixes this.
2) There are multiple suspend stages: prepare suspend, suspend_late,
suspend_no_irq and several devices which need access to i2c-transfers
suspend from the suspend_late or even the suspend_no_irq handler.
The way this works is that first all devices are moved to the prepared
state, then a second run is done moving all devices over to suspended,
then a third run moving all devices over to suspend_late, etc.
To make this work, e.g. the i2c-designware driver has a nop suspend
callback and does the actual suspend from its suspend_late or
suspend_no_irq callback. But the is_suspended flag we are testing for
now gets set during the suspend phase. So when we are then asked to
do an i2c-transfer during the suspend_late phase we get a false-positive
triggering of the:
if (WARN(device_is_suspended(adap->dev.parent),
"Accessing already suspended I2C/SMBus adapter"))
return -ESHUTDOWN;
WARN and a return of -ESHUTDOWN, because the adapter is in the
suspended state, but it has not actually been suspended / moved
to the D3 low-power state as that happens later when we reach
e.g. the suspend_no_irq phase of the suspend.
This is not only a problem with the somewhat complex PMIC
situation on some Cherry Trail devices, but it also breaks the
i915 driver since as a PCI device the i915 device also only
really gets turned off from the suspend_no_irq phase of the suspend.
Sorry, this is something which I should have realized before, but
well I didn't.
TL;DR: really only the adapter driver knows when the device is
put in such a state that it can no longer do transfers, as it
actually turns of clks / moves it D3 / etc. Which may happen at
any of the 3 suspend phases so any "core" based solution is going
to get this wrong.
I share your desire to have the check for this shared in core code,
but I'm afraid that just is not going to work.
Regards,
Hans
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> Documentation/i2c/fault-codes | 4 ++++
> drivers/i2c/i2c-core-base.c | 3 +++
> drivers/i2c/i2c-core-smbus.c | 4 ++++
> 3 files changed, 11 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 28460f6a60cc..3ce238b782f3 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -1865,6 +1865,9 @@ int __i2c_transfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
>
> if (WARN_ON(!msgs || num < 1))
> return -EINVAL;
> + if (WARN(device_is_suspended(adap->dev.parent),
> + "Accessing already suspended I2C/SMBus adapter"))
> + return -ESHUTDOWN;
>
> if (adap->quirks && i2c_check_for_quirks(adap, msgs, num))
> return -EOPNOTSUPP;
> diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
> index 9cd66cabb84f..e0f7f22feabd 100644
> --- a/drivers/i2c/i2c-core-smbus.c
> +++ b/drivers/i2c/i2c-core-smbus.c
> @@ -547,6 +547,10 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
> int try;
> s32 res;
>
> + if (WARN(device_is_suspended(adapter->dev.parent),
> + "Accessing already suspended I2C/SMBus adapter"))
> + return -ESHUTDOWN;
> +
> /* If enabled, the following two tracepoints are conditional on
> * read_write and protocol.
> */
>
Wolfram Sang <[email protected]> 于2018年12月23日周日 上午4:26写道：
>
> This is handled by the I2C core meanwhile.
>
> Signed-off-by: Wolfram Sang <[email protected]>
> ---
> drivers/i2c/busses/i2c-zx2967.c | 8 --------
> 1 file changed, 8 deletions(-)
>
Reviewed-by: Jun Nie <[email protected]>
Hi Wolfram,
On Sat, Dec 22, 2018 at 9:26 PM Wolfram Sang
<[email protected]> wrote:
> Here is the new version without specific I2C helpers but using the
> 'is_suspended' flag from the PM core. I didn't like messing with the
> flag directly, so I did a helper in patch 1. So far, I like the
> approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c
> rejected rightfully too later transfers without further modifications.
> Tested on a Renesas Lager board (R-Car H2).
>
> I dropped a few Tested-by tags because I think this approach is too
> different from V1 to keep them. I hope you guys can have a look again.
> Thanks for all the testing, so far!
>
> A branch can be found here:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended
>
> If this series is acceptable, I'd suggest to take it via my i2c tree
> after rc1. And then I'll provide an immutable branch for the PM tree to
> pick. Let me know if this works for you.
This breaks resume of the CS2000 clock driver on Salvator-X(S) (and
presumable ULCB, too):
Accessing already suspended I2C/SMBus adapter
WARNING: CPU: 1 PID: 1186 at drivers/i2c/i2c-core-smbus.c:551
__i2c_smbus_xfer+0x38/0x66c
Modules linked in:
CPU: 1 PID: 1186 Comm: s2idle Not tainted
4.20.0-salvator-x-08442-ge5992c41ac706409 #264
Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x (DT)
pstate: 60400005 (nZCv daif +PAN -UAO)
pc : __i2c_smbus_xfer+0x38/0x66c
lr : __i2c_smbus_xfer+0x38/0x66c
sp : ffffff8013413880
x29: ffffff8013413880 x28: ffffffc6f78b4820
x27: 0000000000000010 x26: ffffff8010cf6178
x25: ffffff8013413976 x24: 0000000000000002
x23: 0000000000000016 x22: ffffffc6f7872088
x21: 0000000000000000 x20: 000000000000004f
x19: ffffffc6f7872088 x18: 000000000000000a
x17: 0000000000000000 x16: 0000000000000000
x15: 0000000000029c80 x14: 0720072007200720
x13: 0720072007200720 x12: 0720072007200720
x11: 0720072007200720 x10: 0720072007200720
x9 : ffffff801100e6d0 x8 : 6461207375424d53
x7 : ffffff8011c825c8 x6 : ffffff8011c82000
x5 : 0000000000000000 x4 : ffffff8013414000
x3 : ffffff80134136e0 x2 : 00000046ee875000
x1 : 82c6d7c720d64000 x0 : 0000000000000000
Call trace:
__i2c_smbus_xfer+0x38/0x66c
i2c_smbus_xfer+0x64/0x98
i2c_smbus_read_byte_data+0x40/0x6c
cs2000_bset.isra.1+0x2c/0x58
__cs2000_set_rate.constprop.7+0x8c/0x134
cs2000_resume+0x14/0x1c
dpm_run_callback+0x15c/0x2d8
device_resume_early+0x98/0xec
dpm_resume_early+0x3b0/0x454
suspend_devices_and_enter+0x7bc/0xbb0
The CS2000 driver uses SET_LATE_SYSTEM_SLEEP_PM_OPS().
Suspend/resume of the BD9571 driver works fine, as that one uses
SET_SYSTEM_SLEEP_PM_OPS().
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Hi,
On 26-12-18 12:01, Geert Uytterhoeven wrote:
> Hi Wolfram,
>
> On Sat, Dec 22, 2018 at 9:26 PM Wolfram Sang
> <[email protected]> wrote:
>> Here is the new version without specific I2C helpers but using the
>> 'is_suspended' flag from the PM core. I didn't like messing with the
>> flag directly, so I did a helper in patch 1. So far, I like the
>> approach. The diffstat looks nice, and i2c-rcar.c and i2c-sh_mobile.c
>> rejected rightfully too later transfers without further modifications.
>> Tested on a Renesas Lager board (R-Car H2).
>>
>> I dropped a few Tested-by tags because I think this approach is too
>> different from V1 to keep them. I hope you guys can have a look again.
>> Thanks for all the testing, so far!
>>
>> A branch can be found here:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git renesas/i2c/reject-when-suspended
>>
>> If this series is acceptable, I'd suggest to take it via my i2c tree
>> after rc1. And then I'll provide an immutable branch for the PM tree to
>> pick. Let me know if this works for you.
>
> This breaks resume of the CS2000 clock driver on Salvator-X(S) (and
> presumable ULCB, too):
>
> Accessing already suspended I2C/SMBus adapter
> WARNING: CPU: 1 PID: 1186 at drivers/i2c/i2c-core-smbus.c:551
> __i2c_smbus_xfer+0x38/0x66c
> Modules linked in:
> CPU: 1 PID: 1186 Comm: s2idle Not tainted
> 4.20.0-salvator-x-08442-ge5992c41ac706409 #264
> Hardware name: Renesas Salvator-X board based on r8a7795 ES1.x (DT)
> pstate: 60400005 (nZCv daif +PAN -UAO)
> pc : __i2c_smbus_xfer+0x38/0x66c
> lr : __i2c_smbus_xfer+0x38/0x66c
> sp : ffffff8013413880
> x29: ffffff8013413880 x28: ffffffc6f78b4820
> x27: 0000000000000010 x26: ffffff8010cf6178
> x25: ffffff8013413976 x24: 0000000000000002
> x23: 0000000000000016 x22: ffffffc6f7872088
> x21: 0000000000000000 x20: 000000000000004f
> x19: ffffffc6f7872088 x18: 000000000000000a
> x17: 0000000000000000 x16: 0000000000000000
> x15: 0000000000029c80 x14: 0720072007200720
> x13: 0720072007200720 x12: 0720072007200720
> x11: 0720072007200720 x10: 0720072007200720
> x9 : ffffff801100e6d0 x8 : 6461207375424d53
> x7 : ffffff8011c825c8 x6 : ffffff8011c82000
> x5 : 0000000000000000 x4 : ffffff8013414000
> x3 : ffffff80134136e0 x2 : 00000046ee875000
> x1 : 82c6d7c720d64000 x0 : 0000000000000000
> Call trace:
> __i2c_smbus_xfer+0x38/0x66c
> i2c_smbus_xfer+0x64/0x98
> i2c_smbus_read_byte_data+0x40/0x6c
> cs2000_bset.isra.1+0x2c/0x58
> __cs2000_set_rate.constprop.7+0x8c/0x134
> cs2000_resume+0x14/0x1c
> dpm_run_callback+0x15c/0x2d8
> device_resume_early+0x98/0xec
> dpm_resume_early+0x3b0/0x454
> suspend_devices_and_enter+0x7bc/0xbb0
>
> The CS2000 driver uses SET_LATE_SYSTEM_SLEEP_PM_OPS().
>
> Suspend/resume of the BD9571 driver works fine, as that one uses
> SET_SYSTEM_SLEEP_PM_OPS().
Ok, so this is the same thing I noticed, the approach using
the pm-core is_suspend flag only works for adapters which
suspend during the regular suspend phase and as such that
approach cannot work everywhere.
Regards,
Hans
> I share your desire to have the check for this shared in core code,
> but I'm afraid that just is not going to work.
Okay, so this series is definately not it. Probably the previous one
which exposes helpers is not a bad idea after all. Because it is
ulitmately the driver's decision when to use the helpers...
Hi,
On 03-01-19 19:59, Wolfram Sang wrote:
>
>> I share your desire to have the check for this shared in core code,
>> but I'm afraid that just is not going to work.
>
> Okay, so this series is definately not it. Probably the previous one
> which exposes helpers is not a bad idea after all. Because it is
> ulitmately the driver's decision when to use the helpers...
Agreed.
Regards,
Hans