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@xxxxxxxxxxxxxxxxxxxx>
---
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.
*/
>From 0ff431b48f7f2d08bbf299265c67589a598ec5d4 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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
