* [PATCH v2 1/9] PM / core: add helper to return suspend status of a device
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 2/9] i2c: reject new transfers when adapters are suspended Wolfram Sang
` (9 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang
I2C core would like this to reject transfers on an already suspended
adapter.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/9] i2c: reject new transfers when adapters are suspended
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 1/9] PM / core: add helper to return suspend status of a device Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-24 21:32 ` Hans de Goede
2018-12-22 20:26 ` [PATCH v2 3/9] i2c: synquacer: remove unused is_suspended flag Wolfram Sang
` (8 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Wolfram Sang
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 <wsa+renesas@sang-engineering.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/9] i2c: reject new transfers when adapters are suspended
2018-12-22 20:26 ` [PATCH v2 2/9] i2c: reject new transfers when adapters are suspended Wolfram Sang
@ 2018-12-24 21:32 ` Hans de Goede
2019-01-03 18:59 ` Wolfram Sang
0 siblings, 1 reply; 17+ messages in thread
From: Hans de Goede @ 2018-12-24 21:32 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linux-renesas-soc, linux-pm, linux-arm-kernel, linux-kernel,
Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 4370 bytes --]
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 <wsa+renesas@sang-engineering.com>
> ---
> 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.
> */
>
[-- Attachment #2: 0001-FIXUP-i2c-reject-new-transfers-when-adapters-are-sus.patch --]
[-- Type: text/x-patch, Size: 1450 bytes --]
From 0ff431b48f7f2d08bbf299265c67589a598ec5d4 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Mon, 24 Dec 2018 22:00:31 +0100
Subject: [PATCH] FIXUP: i2c: reject new transfers when adapters are suspended
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
drivers/i2c/i2c-core-base.c | 2 +-
drivers/i2c/i2c-core-smbus.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 3ce238b782f3..e2fae7ec6c95 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -1865,7 +1865,7 @@ 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),
+ if (WARN(adap->dev.parent && device_is_suspended(adap->dev.parent),
"Accessing already suspended I2C/SMBus adapter"))
return -ESHUTDOWN;
diff --git a/drivers/i2c/i2c-core-smbus.c b/drivers/i2c/i2c-core-smbus.c
index e0f7f22feabd..1376620ae204 100644
--- a/drivers/i2c/i2c-core-smbus.c
+++ b/drivers/i2c/i2c-core-smbus.c
@@ -547,7 +547,8 @@ s32 __i2c_smbus_xfer(struct i2c_adapter *adapter, u16 addr,
int try;
s32 res;
- if (WARN(device_is_suspended(adapter->dev.parent),
+ if (WARN(adapter->dev.parent &&
+ device_is_suspended(adapter->dev.parent),
"Accessing already suspended I2C/SMBus adapter"))
return -ESHUTDOWN;
--
2.20.1
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/9] i2c: reject new transfers when adapters are suspended
2018-12-24 21:32 ` Hans de Goede
@ 2019-01-03 18:59 ` Wolfram Sang
2019-01-03 20:49 ` Hans de Goede
0 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2019-01-03 18:59 UTC (permalink / raw)
To: Hans de Goede
Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-pm,
linux-arm-kernel, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 316 bytes --]
> 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...
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/9] i2c: reject new transfers when adapters are suspended
2019-01-03 18:59 ` Wolfram Sang
@ 2019-01-03 20:49 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2019-01-03 20:49 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wolfram Sang, linux-i2c, linux-renesas-soc, linux-pm,
linux-arm-kernel, linux-kernel
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 3/9] i2c: synquacer: remove unused is_suspended flag
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 1/9] PM / core: add helper to return suspend status of a device Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 2/9] i2c: reject new transfers when adapters are suspended Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 4/9] i2c: brcmstb: don't open code to reject transfers when suspended Wolfram Sang
` (7 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Ard Biesheuvel
This flag was defined and checked but never set a value. Remove it.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Acked-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 4/9] i2c: brcmstb: don't open code to reject transfers when suspended
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (2 preceding siblings ...)
2018-12-22 20:26 ` [PATCH v2 3/9] i2c: synquacer: remove unused is_suspended flag Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 5/9] i2c: zx2967: " Wolfram Sang
` (6 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Kamal Dasu, Brian Norris,
Gregory Fong, Florian Fainelli, bcm-kernel-feedback-list
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 5/9] i2c: zx2967: don't open code to reject transfers when suspended
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (3 preceding siblings ...)
2018-12-22 20:26 ` [PATCH v2 4/9] i2c: brcmstb: don't open code to reject transfers when suspended Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-26 1:06 ` Jun Nie
2018-12-22 20:26 ` [PATCH v2 6/9] i2c: sprd: don't use pdev as variable name for struct device * Wolfram Sang
` (5 subsequent siblings)
10 siblings, 1 reply; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Jun Nie, Shawn Guo
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 5/9] i2c: zx2967: don't open code to reject transfers when suspended
2018-12-22 20:26 ` [PATCH v2 5/9] i2c: zx2967: " Wolfram Sang
@ 2018-12-26 1:06 ` Jun Nie
0 siblings, 0 replies; 17+ messages in thread
From: Jun Nie @ 2018-12-26 1:06 UTC (permalink / raw)
To: Wolfram Sang
Cc: linux-i2c, linux-renesas-soc, linux-pm, Hans de Goede,
linux-arm-kernel, Linux Kernel Mailing List, Shawn Guo
Wolfram Sang <wsa+renesas@sang-engineering.com> 于2018年12月23日周日 上午4:26写道:
>
> This is handled by the I2C core meanwhile.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/i2c/busses/i2c-zx2967.c | 8 --------
> 1 file changed, 8 deletions(-)
>
Reviewed-by: Jun Nie <jun.nie@linaro.org>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 6/9] i2c: sprd: don't use pdev as variable name for struct device *
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (4 preceding siblings ...)
2018-12-22 20:26 ` [PATCH v2 5/9] i2c: zx2967: " Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 7/9] i2c: sprd: don't open code to reject transfers when suspended Wolfram Sang
` (4 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Orson Zhai, Baolin Wang,
Chunyan Zhang
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 <wsa+renesas@sang-engineering.com>
Reviewed-by: Baolin Wang <baolin.wang@linaro.org>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 7/9] i2c: sprd: don't open code to reject transfers when suspended
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (5 preceding siblings ...)
2018-12-22 20:26 ` [PATCH v2 6/9] i2c: sprd: don't use pdev as variable name for struct device * Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 8/9] i2c: exynos5: " Wolfram Sang
` (3 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Orson Zhai, Baolin Wang,
Chunyan Zhang
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 8/9] i2c: exynos5: don't open code to reject transfers when suspended
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (6 preceding siblings ...)
2018-12-22 20:26 ` [PATCH v2 7/9] i2c: sprd: don't open code to reject transfers when suspended Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-22 20:26 ` [PATCH v2 9/9] i2c: s3c2410: " Wolfram Sang
` (2 subsequent siblings)
10 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Kukjin Kim, Krzysztof Kozlowski,
linux-samsung-soc
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 9/9] i2c: s3c2410: don't open code to reject transfers when suspended
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (7 preceding siblings ...)
2018-12-22 20:26 ` [PATCH v2 8/9] i2c: exynos5: " Wolfram Sang
@ 2018-12-22 20:26 ` Wolfram Sang
2018-12-24 8:55 ` [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Hans de Goede
2018-12-26 11:01 ` Geert Uytterhoeven
10 siblings, 0 replies; 17+ messages in thread
From: Wolfram Sang @ 2018-12-22 20:26 UTC (permalink / raw)
To: linux-i2c
Cc: linux-renesas-soc, linux-pm, Hans de Goede, linux-arm-kernel,
linux-kernel, Wolfram Sang, Kukjin Kim, Krzysztof Kozlowski,
linux-samsung-soc
This is handled by the I2C core meanwhile.
Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
---
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/9] i2c: move handling of suspended adapters to the core
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (8 preceding siblings ...)
2018-12-22 20:26 ` [PATCH v2 9/9] i2c: s3c2410: " Wolfram Sang
@ 2018-12-24 8:55 ` Hans de Goede
2018-12-26 11:01 ` Geert Uytterhoeven
10 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2018-12-24 8:55 UTC (permalink / raw)
To: Wolfram Sang, linux-i2c
Cc: linux-renesas-soc, linux-pm, linux-arm-kernel, linux-kernel
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 <hdegoede@redhat.com>
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(-)
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/9] i2c: move handling of suspended adapters to the core
2018-12-22 20:26 [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Wolfram Sang
` (9 preceding siblings ...)
2018-12-24 8:55 ` [PATCH v2 0/9] i2c: move handling of suspended adapters to the core Hans de Goede
@ 2018-12-26 11:01 ` Geert Uytterhoeven
2018-12-26 11:46 ` Hans de Goede
10 siblings, 1 reply; 17+ messages in thread
From: Geert Uytterhoeven @ 2018-12-26 11:01 UTC (permalink / raw)
To: Wolfram Sang
Cc: Linux I2C, Linux PM list, Linux Kernel Mailing List,
Linux-Renesas, Hans de Goede, Linux ARM
Hi Wolfram,
On Sat, Dec 22, 2018 at 9:26 PM Wolfram Sang
<wsa+renesas@sang-engineering.com> 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 -- geert@linux-m68k.org
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 0/9] i2c: move handling of suspended adapters to the core
2018-12-26 11:01 ` Geert Uytterhoeven
@ 2018-12-26 11:46 ` Hans de Goede
0 siblings, 0 replies; 17+ messages in thread
From: Hans de Goede @ 2018-12-26 11:46 UTC (permalink / raw)
To: Geert Uytterhoeven, Wolfram Sang
Cc: Linux I2C, Linux PM list, Linux Kernel Mailing List,
Linux-Renesas, Linux ARM
Hi,
On 26-12-18 12:01, Geert Uytterhoeven wrote:
> Hi Wolfram,
>
> On Sat, Dec 22, 2018 at 9:26 PM Wolfram Sang
> <wsa+renesas@sang-engineering.com> 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
^ permalink raw reply [flat|nested] 17+ messages in thread