[PATCH 1/2] i2c: designware: Lock the adapter while setting the suspended flag

* [PATCH 1/2] i2c: designware: Lock the adapter while setting the suspended flag
@ 2022-02-23 13:48 Hans de Goede
  2022-02-23 13:48 ` [PATCH 2/2] i2c: designware: Use the i2c_mark_adapter_suspended/resumed() helpers Hans de Goede
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Hans de Goede @ 2022-02-23 13:48 UTC (permalink / raw)
  To: Jarkko Nikula, Wolfram Sang, Andy Shevchenko, Mika Westerberg
  Cc: Hans de Goede, linux-i2c

Lock the adapter while setting the suspended flag, to ensure that other
locked code always sees the change immediately, rather then possibly using
a stale value.

This involves splitting the suspend/resume callbacks into separate runtime
and normal suspend/resume calls. This is necessary because i2c_dw_xfer()
will get called by the i2c-core with the adapter locked and it in turn
calls the runtime-resume callback through pm_runtime_get_sync().

So the runtime versions of the suspend/resume callbacks cannot take
the adapter-lock. Note this patch simply makes the runtime suspend/resume
callbacks not deal with the suspended flag at all. During runtime the
pm_runtime_get_sync() from i2c_dw_xfer() will always ensure that the
adapter is resumed when necessary.

The suspended flag check is only necessary to check proper suspend/resume
ordering during normal suspend/resume which makes the pm_runtime_get_sync()
call a no-op.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/i2c-designware-pcidrv.c  | 31 +++++++++++++++++----
 drivers/i2c/busses/i2c-designware-platdrv.c | 31 +++++++++++++++++----
 2 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c
index ef4250f8852b..9553d7075223 100644
--- a/drivers/i2c/busses/i2c-designware-pcidrv.c
+++ b/drivers/i2c/busses/i2c-designware-pcidrv.c
@@ -213,14 +213,30 @@ static struct dw_pci_controller dw_pci_controllers[] = {
 	},
 };
 
+static int __maybe_unused i2c_dw_pci_runtime_suspend(struct device *dev)
+{
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
+
+	i_dev->disable(i_dev);
+	return 0;
+}
+
 static int __maybe_unused i2c_dw_pci_suspend(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
+	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 	i_dev->suspended = true;
-	i_dev->disable(i_dev);
+	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 
-	return 0;
+	return i2c_dw_pci_runtime_suspend(dev);
+}
+
+static int __maybe_unused i2c_dw_pci_runtime_resume(struct device *dev)
+{
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
+
+	return i_dev->init(i_dev);
 }
 
 static int __maybe_unused i2c_dw_pci_resume(struct device *dev)
@@ -228,14 +244,19 @@ static int __maybe_unused i2c_dw_pci_resume(struct device *dev)
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 	int ret;
 
-	ret = i_dev->init(i_dev);
+	ret = i2c_dw_pci_runtime_resume(dev);
+
+	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 	i_dev->suspended = false;
+	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 
 	return ret;
 }
 
-static UNIVERSAL_DEV_PM_OPS(i2c_dw_pm_ops, i2c_dw_pci_suspend,
-			    i2c_dw_pci_resume, NULL);
+static const struct dev_pm_ops i2c_dw_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_dw_pci_suspend, i2c_dw_pci_resume)
+	SET_RUNTIME_PM_OPS(i2c_dw_pci_runtime_suspend, i2c_dw_pci_runtime_resume, NULL)
+};
 
 static int i2c_dw_pci_probe(struct pci_dev *pdev,
 			    const struct pci_device_id *id)
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 2bd81abc86f6..8e45f65bab73 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -368,12 +368,10 @@ static void dw_i2c_plat_complete(struct device *dev)
 #endif
 
 #ifdef CONFIG_PM
-static int dw_i2c_plat_suspend(struct device *dev)
+static int dw_i2c_plat_runtime_suspend(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
-	i_dev->suspended = true;
-
 	if (i_dev->shared_with_punit)
 		return 0;
 
@@ -383,7 +381,18 @@ static int dw_i2c_plat_suspend(struct device *dev)
 	return 0;
 }
 
-static int dw_i2c_plat_resume(struct device *dev)
+static int dw_i2c_plat_suspend(struct device *dev)
+{
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
+
+	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
+	i_dev->suspended = true;
+	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
+
+	return dw_i2c_plat_runtime_suspend(dev);
+}
+
+static int dw_i2c_plat_runtime_resume(struct device *dev)
 {
 	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
 
@@ -391,7 +400,19 @@ static int dw_i2c_plat_resume(struct device *dev)
 		i2c_dw_prepare_clk(i_dev, true);
 
 	i_dev->init(i_dev);
+
+	return 0;
+}
+
+static int dw_i2c_plat_resume(struct device *dev)
+{
+	struct dw_i2c_dev *i_dev = dev_get_drvdata(dev);
+
+	dw_i2c_plat_runtime_resume(dev);
+
+	i2c_lock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 	i_dev->suspended = false;
+	i2c_unlock_bus(&i_dev->adapter, I2C_LOCK_ROOT_ADAPTER);
 
 	return 0;
 }
@@ -400,7 +421,7 @@ static const struct dev_pm_ops dw_i2c_dev_pm_ops = {
 	.prepare = dw_i2c_plat_prepare,
 	.complete = dw_i2c_plat_complete,
 	SET_LATE_SYSTEM_SLEEP_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume)
-	SET_RUNTIME_PM_OPS(dw_i2c_plat_suspend, dw_i2c_plat_resume, NULL)
+	SET_RUNTIME_PM_OPS(dw_i2c_plat_runtime_suspend, dw_i2c_plat_runtime_resume, NULL)
 };
 
 #define DW_I2C_DEV_PMOPS (&dw_i2c_dev_pm_ops)
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread