alsa-devel.alsa-project.org archive mirror
 help / color / mirror / Atom feed
* [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume
@ 2020-01-15  0:08 Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 01/10] soundwire: bus: fix race condition with probe_complete signaling Pierre-Louis Bossart
                   ` (11 more replies)
  0 siblings, 12 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Bard liao, Rander Wang

The existing mainline code for SoundWire does not handle critical race
conditions, and does not have any support for pm_runtime suspend or
clock-stop modes needed for e.g. jack detection or external VAD.

As suggested by Vinod, these patches for the bus are shared first -
with the risk that they are separated from their actual use in Intel
drivers, so reviewers might wonder why they are needed in the first
place.

For reference, the complete set of 90+ patches required for SoundWire
on Intel platforms is available here:

https://github.com/thesofproject/linux/pull/1692

These patches are not Intel-specific and are likely required for
e.g. Qualcomm-based implementations.

All the patches in this series were generated during the joint
Intel-Realtek validation effort on Intel reference designs and
form-factor devices. The support for the initialization_complete
signaling is already available in the Realtek codecs drivers merged in
the ASoC tree (rt700, rt711, rt1308, rt715)

Pierre-Louis Bossart (8):
  soundwire: bus: fix race condition with probe_complete signaling
  soundwire: bus: fix race condition with enumeration_complete signaling
  soundwire: bus: fix race condition with initialization_complete
    signaling
  soundwire: bus: add PM/no-PM versions of read/write functions
  soundwire: bus: write Slave Device Number without runtime_pm
  soundwire: bus: add helper to clear Slave status to UNATTACHED
  soundwire: bus: disable pm_runtime in sdw_slave_delete
  soundwire: bus: don't treat CMD_IGNORED as error on ClockStop

Rander Wang (2):
  soundwire: bus: fix io error when processing alert event
  soundwire: bus: add clock stop helpers

 drivers/soundwire/bus.c       | 509 ++++++++++++++++++++++++++++++++--
 drivers/soundwire/bus.h       |   9 +
 drivers/soundwire/bus_type.c  |   5 +
 drivers/soundwire/slave.c     |   4 +
 include/linux/soundwire/sdw.h |  24 ++
 5 files changed, 526 insertions(+), 25 deletions(-)

-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 01/10] soundwire: bus: fix race condition with probe_complete signaling
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 02/10] soundwire: bus: fix race condition with enumeration_complete signaling Pierre-Louis Bossart
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Rander Wang, Bard liao,
	Rander Wang

The driver probe takes care of basic initialization and is invoked
when a Slave becomes attached, after a match between the Slave DevID
registers and ACPI/DT entries.

The update_status callback is invoked when a Slave state changes,
e.g. when it is assigned a non-zero Device Number and it reports with
an ATTACHED/ALERT state.

The state change detection is usually hardware-based and based on the
SoundWire frame rate (e.g. double-digit microseconds) while the probe
is a pure software operation, which may involve a kernel module
load. In corner cases, it's possible that the state changes before the
probe completes.

This patch suggests the use of wait_for_completion to avoid races on
startup, so that the update_status callback does not rely on invalid
pointers/data structures.

Signed-off-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c      | 25 ++++++++++++++++++++++---
 drivers/soundwire/bus.h      |  1 +
 drivers/soundwire/bus_type.c |  5 +++++
 drivers/soundwire/slave.c    |  2 ++
 4 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 6106577fb3ed..4980dfd6f3a3 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -970,10 +970,29 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
 static int sdw_update_slave_status(struct sdw_slave *slave,
 				   enum sdw_slave_status status)
 {
-	if (slave->ops && slave->ops->update_status)
-		return slave->ops->update_status(slave, status);
+	unsigned long time;
 
-	return 0;
+	if (!slave->probed) {
+		/*
+		 * the slave status update is typically handled in an
+		 * interrupt thread, which can race with the driver
+		 * probe, e.g. when a module needs to be loaded.
+		 *
+		 * make sure the probe is complete before updating
+		 * status.
+		 */
+		time = wait_for_completion_timeout(&slave->probe_complete,
+				msecs_to_jiffies(DEFAULT_PROBE_TIMEOUT));
+		if (!time) {
+			dev_err(&slave->dev, "Probe not complete, timed out\n");
+			return -ETIMEDOUT;
+		}
+	}
+
+	if (!slave->ops || !slave->ops->update_status)
+		return 0;
+
+	return slave->ops->update_status(slave, status);
 }
 
 /**
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index cb482da914da..acb8d11a4c84 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -5,6 +5,7 @@
 #define __SDW_BUS_H
 
 #define DEFAULT_BANK_SWITCH_TIMEOUT 3000
+#define DEFAULT_PROBE_TIMEOUT       2000
 
 #if IS_ENABLED(CONFIG_ACPI)
 int sdw_acpi_find_slaves(struct sdw_bus *bus);
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 4a465f55039f..17f096dd6806 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -110,6 +110,11 @@ static int sdw_drv_probe(struct device *dev)
 	slave->bus->clk_stop_timeout = max_t(u32, slave->bus->clk_stop_timeout,
 					     slave->prop.clk_stop_timeout);
 
+	slave->probed = true;
+	complete(&slave->probe_complete);
+
+	dev_dbg(dev, "probe complete\n");
+
 	return 0;
 }
 
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 19919975bb6d..08db0488e02d 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -47,6 +47,8 @@ static int sdw_slave_add(struct sdw_bus *bus,
 	slave->bus = bus;
 	slave->status = SDW_SLAVE_UNATTACHED;
 	slave->dev_num = 0;
+	init_completion(&slave->probe_complete);
+	slave->probed = false;
 
 	mutex_lock(&bus->bus_lock);
 	list_add_tail(&slave->node, &bus->slaves);
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 02/10] soundwire: bus: fix race condition with enumeration_complete signaling
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 01/10] soundwire: bus: fix race condition with probe_complete signaling Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 03/10] soundwire: bus: fix race condition with initialization_complete signaling Pierre-Louis Bossart
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

This patch adds the signaling needed for Slave drivers to wait until
the enumeration completes so that race conditions when issuing
read/write commands are avoided. The calls for wait_for_completion()
will be added in codec drivers in follow-up patches.

The order between init_completion() and complete() is deterministic,
the Slave is marked as UNATTACHED either during a Master-initiated
HardReset, or when the hardware detects the Slave no longer reports as
ATTACHED.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c   | 20 ++++++++++++++++++++
 drivers/soundwire/slave.c |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 4980dfd6f3a3..a2267c3a1d2d 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -610,6 +610,26 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
 				    enum sdw_slave_status status)
 {
 	mutex_lock(&slave->bus->bus_lock);
+
+	dev_vdbg(&slave->dev,
+		 "%s: changing status slave %d status %d new status %d\n",
+		 __func__, slave->dev_num, slave->status, status);
+
+	if (status == SDW_SLAVE_UNATTACHED) {
+		dev_dbg(&slave->dev,
+			"%s: initializing completion for Slave %d\n",
+			__func__, slave->dev_num);
+
+		init_completion(&slave->enumeration_complete);
+
+	} else if ((status == SDW_SLAVE_ATTACHED) &&
+		   (slave->status == SDW_SLAVE_UNATTACHED)) {
+		dev_dbg(&slave->dev,
+			"%s: signaling completion for Slave %d\n",
+			__func__, slave->dev_num);
+
+		complete(&slave->enumeration_complete);
+	}
 	slave->status = status;
 	mutex_unlock(&slave->bus->bus_lock);
 }
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 08db0488e02d..e767a78066ee 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -46,6 +46,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
 	slave->dev.of_node = of_node_get(to_of_node(fwnode));
 	slave->bus = bus;
 	slave->status = SDW_SLAVE_UNATTACHED;
+	init_completion(&slave->enumeration_complete);
 	slave->dev_num = 0;
 	init_completion(&slave->probe_complete);
 	slave->probed = false;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 03/10] soundwire: bus: fix race condition with initialization_complete signaling
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 01/10] soundwire: bus: fix race condition with probe_complete signaling Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 02/10] soundwire: bus: fix race condition with enumeration_complete signaling Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 04/10] soundwire: bus: add PM/no-PM versions of read/write functions Pierre-Louis Bossart
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Waiting for the enumeration to be complete may not be enough for a
Slave driver, there is a possible race condition between resume
operations and initializations handled in an interrupt thread, which
can results in settings not being fully restored after system or
pm_runtime resume.

This patch builds on the changes added for enumeration_complete,
init_completion() is called when the Slave device becomes UNATTACHED,
as done with enumeration_complete.

The difference with the enumeration_complete case is that complete()
is signaled after the Slave device is fully initialized after the
.update_status() callback is called.

A Slave device driver can decide to wait on either of the two
complete() cases, depending on its initialization code and
requirements.

Signed-off-by: Rander Wang <rander.wang@linux.intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c   | 8 ++++++++
 drivers/soundwire/slave.c | 1 +
 2 files changed, 9 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index a2267c3a1d2d..ea04cf5f5bdc 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -621,6 +621,7 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
 			__func__, slave->dev_num);
 
 		init_completion(&slave->enumeration_complete);
+		init_completion(&slave->initialization_complete);
 
 	} else if ((status == SDW_SLAVE_ATTACHED) &&
 		   (slave->status == SDW_SLAVE_UNATTACHED)) {
@@ -1025,6 +1026,7 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 {
 	enum sdw_slave_status prev_status;
 	struct sdw_slave *slave;
+	bool attached_initializing;
 	int i, ret = 0;
 
 	/* first check if any Slaves fell off the bus */
@@ -1070,6 +1072,8 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 		if (!slave)
 			continue;
 
+		attached_initializing = false;
+
 		switch (status[i]) {
 		case SDW_SLAVE_UNATTACHED:
 			if (slave->status == SDW_SLAVE_UNATTACHED)
@@ -1096,6 +1100,8 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 			if (prev_status == SDW_SLAVE_ALERT)
 				break;
 
+			attached_initializing = true;
+
 			ret = sdw_initialize_slave(slave);
 			if (ret)
 				dev_err(bus->dev,
@@ -1114,6 +1120,8 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 		if (ret)
 			dev_err(slave->bus->dev,
 				"Update Slave status failed:%d\n", ret);
+		if (attached_initializing)
+			complete(&slave->initialization_complete);
 	}
 
 	return ret;
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index e767a78066ee..aace57fae7f8 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -47,6 +47,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
 	slave->bus = bus;
 	slave->status = SDW_SLAVE_UNATTACHED;
 	init_completion(&slave->enumeration_complete);
+	init_completion(&slave->initialization_complete);
 	slave->dev_num = 0;
 	init_completion(&slave->probe_complete);
 	slave->probed = false;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 04/10] soundwire: bus: add PM/no-PM versions of read/write functions
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (2 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 03/10] soundwire: bus: fix race condition with initialization_complete signaling Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 05/10] soundwire: bus: write Slave Device Number without runtime_pm Pierre-Louis Bossart
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Add support for pm_runtime with the appropriate error checks for
sdw_write/read functions, e.g. when pm_runtime is not supported.

Also expose internal functions without pm_runtime support, which are
required to perform any sort of suspend/resume operation, as well as
any enumeration tasks.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c | 68 +++++++++++++++++++++++++++++++----------
 1 file changed, 52 insertions(+), 16 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index ea04cf5f5bdc..c525b9b50453 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -317,6 +317,46 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
 	return 0;
 }
 
+/*
+ * Read/Write IO functions.
+ * no_pm versions can only be called by the bus, e.g. while enumerating or
+ * handling suspend-resume sequences.
+ * all clients need to use the pm versions
+ */
+
+static int
+sdw_nread_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+{
+	struct sdw_msg msg;
+	int ret;
+
+	ret = sdw_fill_msg(&msg, slave, addr, count,
+			   slave->dev_num, SDW_MSG_FLAG_READ, val);
+	if (ret < 0)
+		return ret;
+
+	return sdw_transfer(slave->bus, &msg);
+}
+
+static int
+sdw_nwrite_no_pm(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
+{
+	struct sdw_msg msg;
+	int ret;
+
+	ret = sdw_fill_msg(&msg, slave, addr, count,
+			   slave->dev_num, SDW_MSG_FLAG_WRITE, val);
+	if (ret < 0)
+		return ret;
+
+	return sdw_transfer(slave->bus, &msg);
+}
+
+static int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value)
+{
+	return sdw_nwrite_no_pm(slave, addr, 1, &value);
+}
+
 /**
  * sdw_nread() - Read "n" contiguous SDW Slave registers
  * @slave: SDW Slave
@@ -326,19 +366,17 @@ int sdw_fill_msg(struct sdw_msg *msg, struct sdw_slave *slave,
  */
 int sdw_nread(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
 {
-	struct sdw_msg msg;
 	int ret;
 
-	ret = sdw_fill_msg(&msg, slave, addr, count,
-			   slave->dev_num, SDW_MSG_FLAG_READ, val);
-	if (ret < 0)
-		return ret;
-
 	ret = pm_runtime_get_sync(slave->bus->dev);
-	if (ret < 0)
+	if (ret < 0 && ret != -EACCES) {
+		pm_runtime_put_noidle(slave->bus->dev);
 		return ret;
+	}
+
+	ret = sdw_nread_no_pm(slave, addr, count, val);
 
-	ret = sdw_transfer(slave->bus, &msg);
+	pm_runtime_mark_last_busy(slave->bus->dev);
 	pm_runtime_put(slave->bus->dev);
 
 	return ret;
@@ -354,19 +392,17 @@ EXPORT_SYMBOL(sdw_nread);
  */
 int sdw_nwrite(struct sdw_slave *slave, u32 addr, size_t count, u8 *val)
 {
-	struct sdw_msg msg;
 	int ret;
 
-	ret = sdw_fill_msg(&msg, slave, addr, count,
-			   slave->dev_num, SDW_MSG_FLAG_WRITE, val);
-	if (ret < 0)
-		return ret;
-
 	ret = pm_runtime_get_sync(slave->bus->dev);
-	if (ret < 0)
+	if (ret < 0 && ret != -EACCES) {
+		pm_runtime_put_noidle(slave->bus->dev);
 		return ret;
+	}
+
+	ret = sdw_nwrite_no_pm(slave, addr, count, val);
 
-	ret = sdw_transfer(slave->bus, &msg);
+	pm_runtime_mark_last_busy(slave->bus->dev);
 	pm_runtime_put(slave->bus->dev);
 
 	return ret;
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 05/10] soundwire: bus: write Slave Device Number without runtime_pm
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (3 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 04/10] soundwire: bus: add PM/no-PM versions of read/write functions Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 06/10] soundwire: bus: add helper to clear Slave status to UNATTACHED Pierre-Louis Bossart
                   ` (6 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

While handling the Device0, we can safely use sdw_write_no_pm.

This move will also helps us track that all other usages of
sdw_write() happen when the Slave is already enumerated.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index c525b9b50453..dfe27e3ca815 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -522,7 +522,7 @@ static int sdw_assign_device_num(struct sdw_slave *slave)
 	dev_num = slave->dev_num;
 	slave->dev_num = 0;
 
-	ret = sdw_write(slave, SDW_SCP_DEVNUMBER, dev_num);
+	ret = sdw_write_no_pm(slave, SDW_SCP_DEVNUMBER, dev_num);
 	if (ret < 0) {
 		dev_err(&slave->dev, "Program device_num %d failed: %d\n",
 			dev_num, ret);
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 06/10] soundwire: bus: add helper to clear Slave status to UNATTACHED
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (4 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 05/10] soundwire: bus: write Slave Device Number without runtime_pm Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 07/10] soundwire: bus: disable pm_runtime in sdw_slave_delete Pierre-Louis Bossart
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

When resuming with a bus reset, we need to re-enumerate and restart
from UNATTACHED. The helper added in this patch helps implement a more
robust state machine avoiding race conditions on resume.

The unattach request is stored and will be used by Slave drivers, if
needed: Intel validation exposed a corner case where the Slave device
may transition to D3 when streaming stops, but streaming restarts
before the Master transitions to D3. In that case, the Slave status
was not cleared as UNATTACHED by the Master resuming, and the
wait_for_completion will time out.

When the slave resumes, it can check if a Master-initiated
re-enumeration and initialization took place and skip the
wait_for_completion() if there is no reason to wait.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c | 27 +++++++++++++++++++++++++++
 drivers/soundwire/bus.h |  8 ++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index dfe27e3ca815..57dec61142e5 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -1163,3 +1163,30 @@ int sdw_handle_slave_status(struct sdw_bus *bus,
 	return ret;
 }
 EXPORT_SYMBOL(sdw_handle_slave_status);
+
+void sdw_clear_slave_status(struct sdw_bus *bus, u32 request)
+{
+	struct sdw_slave *slave;
+	int i;
+
+	/* Check all non-zero devices */
+	for (i = 1; i <= SDW_MAX_DEVICES; i++) {
+		mutex_lock(&bus->bus_lock);
+		if (test_bit(i, bus->assigned) == false) {
+			mutex_unlock(&bus->bus_lock);
+			continue;
+		}
+		mutex_unlock(&bus->bus_lock);
+
+		slave = sdw_get_slave(bus, i);
+		if (!slave)
+			continue;
+
+		if (slave->status != SDW_SLAVE_UNATTACHED)
+			sdw_modify_slave_status(slave, SDW_SLAVE_UNATTACHED);
+
+		/* keep track of request, used in pm_runtime resume */
+		slave->unattach_request = request;
+	}
+}
+EXPORT_SYMBOL(sdw_clear_slave_status);
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index acb8d11a4c84..204204a26db8 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -165,4 +165,12 @@ sdw_update(struct sdw_slave *slave, u32 addr, u8 mask, u8 val)
 	return sdw_write(slave, addr, tmp);
 }
 
+/*
+ * At the moment we only track Master-initiated hw_reset.
+ * Additional fields can be added as needed
+ */
+#define SDW_UNATTACH_REQUEST_MASTER_RESET	BIT(0)
+
+void sdw_clear_slave_status(struct sdw_bus *bus, u32 request);
+
 #endif /* __SDW_BUS_H */
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 07/10] soundwire: bus: disable pm_runtime in sdw_slave_delete
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (5 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 06/10] soundwire: bus: add helper to clear Slave status to UNATTACHED Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 08/10] soundwire: bus: fix io error when processing alert event Pierre-Louis Bossart
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

Before removing the slave device, disable pm_runtime to prevent any
race condition with the resume being executed after the bus and slave
devices are removed.

Since this pm_runtime_disable() is handled in common routines,
implementations of Slave drivers do not need to call it in their
.remove() routine.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 57dec61142e5..33bb273454cf 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -113,6 +113,8 @@ static int sdw_delete_slave(struct device *dev, void *data)
 	struct sdw_slave *slave = dev_to_sdw_dev(dev);
 	struct sdw_bus *bus = slave->bus;
 
+	pm_runtime_disable(dev);
+
 	sdw_slave_debugfs_exit(slave);
 
 	mutex_lock(&bus->bus_lock);
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 08/10] soundwire: bus: fix io error when processing alert event
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (6 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 07/10] soundwire: bus: disable pm_runtime in sdw_slave_delete Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 09/10] soundwire: bus: add clock stop helpers Pierre-Louis Bossart
                   ` (3 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Rander Wang, Bard liao,
	Rander Wang

From: Rander Wang <rander.wang@intel.com>

There are two types of io errors when processing alert event.

a) the Master detects an ALERT status for e.g. a jack event and
invokes the implementation-defined function in the Slave driver to
check the jack status. At this time the codec is just suspended, so io
registers can't be accessed.

b) when waking up from clock stop mode1 state, where the bus needs a
complete re-enumeration, Slave registers can't be accessed until the
enumeration is complete.

This patch resumes the Slave device and waits for initialization
complete when processing slave alert event, so that registers on the
Slave can be accessed without timeouts or io errors.

Signed-off-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 33bb273454cf..23bc24c8e9d1 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -890,12 +890,19 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
 
 	sdw_modify_slave_status(slave, SDW_SLAVE_ALERT);
 
+	ret = pm_runtime_get_sync(&slave->dev);
+	if (ret < 0 && ret != -EACCES) {
+		dev_err(&slave->dev, "Failed to resume device: %d\n", ret);
+		pm_runtime_put_noidle(slave->bus->dev);
+		return ret;
+	}
+
 	/* Read Instat 1, Instat 2 and Instat 3 registers */
 	ret = sdw_read(slave, SDW_SCP_INT1);
 	if (ret < 0) {
 		dev_err(slave->bus->dev,
 			"SDW_SCP_INT1 read failed:%d\n", ret);
-		return ret;
+		goto io_err;
 	}
 	buf = ret;
 
@@ -903,7 +910,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
 	if (ret < 0) {
 		dev_err(slave->bus->dev,
 			"SDW_SCP_INT2/3 read failed:%d\n", ret);
-		return ret;
+		goto io_err;
 	}
 
 	do {
@@ -983,7 +990,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
 		if (ret < 0) {
 			dev_err(slave->bus->dev,
 				"SDW_SCP_INT1 write failed:%d\n", ret);
-			return ret;
+			goto io_err;
 		}
 
 		/*
@@ -994,7 +1001,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
 		if (ret < 0) {
 			dev_err(slave->bus->dev,
 				"SDW_SCP_INT1 read failed:%d\n", ret);
-			return ret;
+			goto io_err;
 		}
 		_buf = ret;
 
@@ -1002,7 +1009,7 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
 		if (ret < 0) {
 			dev_err(slave->bus->dev,
 				"SDW_SCP_INT2/3 read failed:%d\n", ret);
-			return ret;
+			goto io_err;
 		}
 
 		/* Make sure no interrupts are pending */
@@ -1023,6 +1030,10 @@ static int sdw_handle_slave_alerts(struct sdw_slave *slave)
 	if (count == SDW_READ_INTR_CLEAR_RETRY)
 		dev_warn(slave->bus->dev, "Reached MAX_RETRY on alert read\n");
 
+io_err:
+	pm_runtime_mark_last_busy(&slave->dev);
+	pm_runtime_put_autosuspend(&slave->dev);
+
 	return ret;
 }
 
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 09/10] soundwire: bus: add clock stop helpers
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (7 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 08/10] soundwire: bus: fix io error when processing alert event Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-01-15  0:08 ` [alsa-devel] [PATCH 10/10] soundwire: bus: don't treat CMD_IGNORED as error on ClockStop Pierre-Louis Bossart
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Rander Wang, Bard liao,
	Rander Wang

From: Rander Wang <rander.wang@intel.com>

SoundWire supports two clock stop modes. Add support to handle the
clock stop modes and add pm_runtime calls in the bus.

Credits: this patch is based on an earlier internal contribution by
Vinod Koul, Sanyog Kale, Shreyas Nc and Hardik Shah.

Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Rander Wang <rander.wang@intel.com>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c       | 332 ++++++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h |  24 +++
 2 files changed, 356 insertions(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 23bc24c8e9d1..3395abd2ed39 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -2,6 +2,7 @@
 // Copyright(c) 2015-17 Intel Corporation.
 
 #include <linux/acpi.h>
+#include <linux/delay.h>
 #include <linux/mod_devicetable.h>
 #include <linux/pm_runtime.h>
 #include <linux/soundwire/sdw_registers.h>
@@ -359,6 +360,52 @@ static int sdw_write_no_pm(struct sdw_slave *slave, u32 addr, u8 value)
 	return sdw_nwrite_no_pm(slave, addr, 1, &value);
 }
 
+static int
+sdw_bread_no_pm(struct sdw_bus *bus, u16 dev_num, u32 addr)
+{
+	struct sdw_msg msg;
+	u8 buf;
+	int ret;
+
+	ret = sdw_fill_msg(&msg, NULL, addr, 1, dev_num,
+			   SDW_MSG_FLAG_READ, &buf);
+	if (ret)
+		return ret;
+
+	ret = sdw_transfer(bus, &msg);
+	if (ret < 0)
+		return ret;
+	else
+		return buf;
+}
+
+static int
+sdw_bwrite_no_pm(struct sdw_bus *bus, u16 dev_num, u32 addr, u8 value)
+{
+	struct sdw_msg msg;
+	int ret;
+
+	ret = sdw_fill_msg(&msg, NULL, addr, 1, dev_num,
+			   SDW_MSG_FLAG_WRITE, &value);
+	if (ret)
+		return ret;
+
+	return sdw_transfer(bus, &msg);
+}
+
+static int
+sdw_read_no_pm(struct sdw_slave *slave, u32 addr)
+{
+	u8 buf;
+	int ret;
+
+	ret = sdw_nread_no_pm(slave, addr, 1, &buf);
+	if (ret < 0)
+		return ret;
+	else
+		return buf;
+}
+
 /**
  * sdw_nread() - Read "n" contiguous SDW Slave registers
  * @slave: SDW Slave
@@ -673,6 +720,291 @@ static void sdw_modify_slave_status(struct sdw_slave *slave,
 	mutex_unlock(&slave->bus->bus_lock);
 }
 
+static enum sdw_clk_stop_mode sdw_get_clk_stop_mode(struct sdw_slave *slave)
+{
+	enum sdw_clk_stop_mode mode;
+
+	/*
+	 * Query for clock stop mode if Slave implements
+	 * ops->get_clk_stop_mode, else read from property.
+	 */
+	if (slave->ops && slave->ops->get_clk_stop_mode) {
+		mode = slave->ops->get_clk_stop_mode(slave);
+	} else {
+		if (slave->prop.clk_stop_mode1)
+			mode = SDW_CLK_STOP_MODE1;
+		else
+			mode = SDW_CLK_STOP_MODE0;
+	}
+
+	return mode;
+}
+
+static int sdw_slave_clk_stop_callback(struct sdw_slave *slave,
+				       enum sdw_clk_stop_mode mode,
+				       enum sdw_clk_stop_type type)
+{
+	int ret;
+
+	if (slave->ops && slave->ops->clk_stop) {
+		ret = slave->ops->clk_stop(slave, mode, type);
+		if (ret < 0) {
+			dev_err(&slave->dev,
+				"Clk Stop type =%d failed: %d\n", type, ret);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int sdw_slave_clk_stop_prepare(struct sdw_slave *slave,
+				      enum sdw_clk_stop_mode mode,
+				      bool prepare)
+{
+	bool wake_en;
+	u32 val = 0;
+	int ret;
+
+	wake_en = slave->prop.wake_capable;
+
+	if (prepare) {
+		val = SDW_SCP_SYSTEMCTRL_CLK_STP_PREP;
+
+		if (mode == SDW_CLK_STOP_MODE1)
+			val |= SDW_SCP_SYSTEMCTRL_CLK_STP_MODE1;
+
+		if (wake_en)
+			val |= SDW_SCP_SYSTEMCTRL_WAKE_UP_EN;
+	} else {
+		val = sdw_read_no_pm(slave, SDW_SCP_SYSTEMCTRL);
+
+		val &= ~(SDW_SCP_SYSTEMCTRL_CLK_STP_PREP);
+	}
+
+	ret = sdw_write_no_pm(slave, SDW_SCP_SYSTEMCTRL, val);
+
+	if (ret != 0)
+		dev_err(&slave->dev,
+			"Clock Stop prepare failed for slave: %d", ret);
+
+	return ret;
+}
+
+static int sdw_bus_wait_for_clk_prep_deprep(struct sdw_bus *bus, u16 dev_num)
+{
+	int retry = bus->clk_stop_timeout;
+	int val;
+
+	do {
+		val = sdw_bread_no_pm(bus, dev_num, SDW_SCP_STAT) &
+			SDW_SCP_STAT_CLK_STP_NF;
+		if (!val) {
+			dev_info(bus->dev, "clock stop prep/de-prep done slave:%d",
+				 dev_num);
+			return 0;
+		}
+
+		usleep_range(1000, 1500);
+		retry--;
+	} while (retry);
+
+	dev_err(bus->dev, "clock stop prep/de-prep failed slave:%d",
+		dev_num);
+
+	return -ETIMEDOUT;
+}
+
+/**
+ * sdw_bus_prep_clk_stop: prepare Slave(s) for clock stop
+ *
+ * @bus: SDW bus instance
+ *
+ * Query Slave for clock stop mode and prepare for that mode.
+ */
+int sdw_bus_prep_clk_stop(struct sdw_bus *bus)
+{
+	enum sdw_clk_stop_mode slave_mode;
+	bool simple_clk_stop = true;
+	struct sdw_slave *slave;
+	bool is_slave = false;
+	int ret = 0;
+
+	/*
+	 * In order to save on transition time, prepare
+	 * each Slave and then wait for all Slave(s) to be
+	 * prepared for clock stop.
+	 */
+	list_for_each_entry(slave, &bus->slaves, node) {
+		if (!slave->dev_num)
+			continue;
+
+		/* Identify if Slave(s) are available on Bus */
+		is_slave = true;
+
+		if (slave->status != SDW_SLAVE_ATTACHED &&
+		    slave->status != SDW_SLAVE_ALERT)
+			continue;
+
+		slave_mode = sdw_get_clk_stop_mode(slave);
+		slave->curr_clk_stop_mode = slave_mode;
+
+		ret = sdw_slave_clk_stop_callback(slave, slave_mode,
+						  SDW_CLK_PRE_PREPARE);
+		if (ret < 0) {
+			dev_err(&slave->dev,
+				"pre-prepare failed:%d", ret);
+			return ret;
+		}
+
+		ret = sdw_slave_clk_stop_prepare(slave,
+						 slave_mode, true);
+		if (ret < 0) {
+			dev_err(&slave->dev,
+				"pre-prepare failed:%d", ret);
+			return ret;
+		}
+
+		if (slave_mode == SDW_CLK_STOP_MODE1)
+			simple_clk_stop = false;
+	}
+
+	if (is_slave && !simple_clk_stop) {
+		ret = sdw_bus_wait_for_clk_prep_deprep(bus,
+						       SDW_BROADCAST_DEV_NUM);
+		if (ret < 0)
+			return ret;
+	}
+
+	/* Inform slaves that prep is done */
+	list_for_each_entry(slave, &bus->slaves, node) {
+		if (!slave->dev_num)
+			continue;
+
+		if (slave->status != SDW_SLAVE_ATTACHED &&
+		    slave->status != SDW_SLAVE_ALERT)
+			continue;
+
+		slave_mode = slave->curr_clk_stop_mode;
+
+		if (slave_mode == SDW_CLK_STOP_MODE1) {
+			ret = sdw_slave_clk_stop_callback(slave,
+							  slave_mode,
+							  SDW_CLK_POST_PREPARE);
+
+			if (ret < 0) {
+				dev_err(&slave->dev,
+					"post-prepare failed:%d", ret);
+			}
+		}
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL(sdw_bus_prep_clk_stop);
+
+/**
+ * sdw_bus_clk_stop: stop bus clock
+ *
+ * @bus: SDW bus instance
+ *
+ * After preparing the Slaves for clock stop, stop the clock by broadcasting
+ * write to SCP_CTRL register.
+ */
+int sdw_bus_clk_stop(struct sdw_bus *bus)
+{
+	int ret;
+
+	/*
+	 * broadcast clock stop now, attached Slaves will ACK this,
+	 * unattached will ignore
+	 */
+	ret = sdw_bwrite_no_pm(bus, SDW_BROADCAST_DEV_NUM,
+			       SDW_SCP_CTRL, SDW_SCP_CTRL_CLK_STP_NOW);
+	if (ret < 0) {
+		dev_err(bus->dev,
+			"ClockStopNow Broadcast message failed %d", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sdw_bus_clk_stop);
+
+/**
+ * sdw_bus_exit_clk_stop: Exit clock stop mode
+ *
+ * @bus: SDW bus instance
+ *
+ * This De-prepares the Slaves by exiting Clock Stop Mode 0. For the Slaves
+ * exiting Clock Stop Mode 1, they will be de-prepared after they enumerate
+ * back.
+ */
+int sdw_bus_exit_clk_stop(struct sdw_bus *bus)
+{
+	enum sdw_clk_stop_mode mode;
+	bool simple_clk_stop = true;
+	struct sdw_slave *slave;
+	bool is_slave = false;
+	int ret;
+
+	/*
+	 * In order to save on transition time, de-prepare
+	 * each Slave and then wait for all Slave(s) to be
+	 * de-prepared after clock resume.
+	 */
+	list_for_each_entry(slave, &bus->slaves, node) {
+		if (!slave->dev_num)
+			continue;
+
+		/* Identify if Slave(s) are available on Bus */
+		is_slave = true;
+
+		if (slave->status != SDW_SLAVE_ATTACHED &&
+		    slave->status != SDW_SLAVE_ALERT)
+			continue;
+
+		mode = slave->curr_clk_stop_mode;
+
+		if (mode == SDW_CLK_STOP_MODE1) {
+			simple_clk_stop = false;
+			continue;
+		}
+
+		ret = sdw_slave_clk_stop_callback(slave, mode,
+						  SDW_CLK_PRE_DEPREPARE);
+		if (ret < 0)
+			dev_warn(&slave->dev,
+				 "clk stop deprep failed:%d", ret);
+
+		ret = sdw_slave_clk_stop_prepare(slave, mode,
+						 false);
+
+		if (ret < 0)
+			dev_warn(&slave->dev,
+				 "clk stop deprep failed:%d", ret);
+	}
+
+	if (is_slave && !simple_clk_stop)
+		sdw_bus_wait_for_clk_prep_deprep(bus, SDW_BROADCAST_DEV_NUM);
+
+	list_for_each_entry(slave, &bus->slaves, node) {
+		if (!slave->dev_num)
+			continue;
+
+		if (slave->status != SDW_SLAVE_ATTACHED &&
+		    slave->status != SDW_SLAVE_ALERT)
+			continue;
+
+		mode = slave->curr_clk_stop_mode;
+		sdw_slave_clk_stop_callback(slave, mode,
+					    SDW_CLK_POST_DEPREPARE);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(sdw_bus_exit_clk_stop);
+
 int sdw_configure_dpn_intr(struct sdw_slave *slave,
 			   int port, bool enable, int mask)
 {
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index b451bb622335..b8427df034ce 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -79,6 +79,21 @@ enum sdw_slave_status {
 	SDW_SLAVE_RESERVED = 3,
 };
 
+/**
+ * enum sdw_clk_stop_type: clock stop operations
+ *
+ * @SDW_CLK_PRE_PREPARE: pre clock stop prepare
+ * @SDW_CLK_POST_PREPARE: post clock stop prepare
+ * @SDW_CLK_PRE_DEPREPARE: pre clock stop de-prepare
+ * @SDW_CLK_POST_DEPREPARE: post clock stop de-prepare
+ */
+enum sdw_clk_stop_type {
+	       SDW_CLK_PRE_PREPARE = 0,
+	       SDW_CLK_POST_PREPARE,
+	       SDW_CLK_PRE_DEPREPARE,
+	       SDW_CLK_POST_DEPREPARE,
+};
+
 /**
  * enum sdw_command_response - Command response as defined by SDW spec
  * @SDW_CMD_OK: cmd was successful
@@ -533,6 +548,11 @@ struct sdw_slave_ops {
 	int (*port_prep)(struct sdw_slave *slave,
 			 struct sdw_prepare_ch *prepare_ch,
 			 enum sdw_port_prep_ops pre_ops);
+	int (*get_clk_stop_mode)(struct sdw_slave *slave);
+	int (*clk_stop)(struct sdw_slave *slave,
+			enum sdw_clk_stop_mode mode,
+			enum sdw_clk_stop_type type);
+
 };
 
 /**
@@ -575,6 +595,7 @@ struct sdw_slave {
 #endif
 	struct list_head node;
 	struct completion *port_ready;
+	enum sdw_clk_stop_mode curr_clk_stop_mode;
 	u16 dev_num;
 	u16 dev_num_sticky;
 	bool probed;
@@ -892,6 +913,9 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream);
 int sdw_enable_stream(struct sdw_stream_runtime *stream);
 int sdw_disable_stream(struct sdw_stream_runtime *stream);
 int sdw_deprepare_stream(struct sdw_stream_runtime *stream);
+int sdw_bus_prep_clk_stop(struct sdw_bus *bus);
+int sdw_bus_clk_stop(struct sdw_bus *bus);
+int sdw_bus_exit_clk_stop(struct sdw_bus *bus);
 
 /* messaging and data APIs */
 
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* [alsa-devel] [PATCH 10/10] soundwire: bus: don't treat CMD_IGNORED as error on ClockStop
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (8 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 09/10] soundwire: bus: add clock stop helpers Pierre-Louis Bossart
@ 2020-01-15  0:08 ` Pierre-Louis Bossart
  2020-02-10 14:30 ` [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
  2020-02-25 10:27 ` Vinod Koul
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-01-15  0:08 UTC (permalink / raw)
  To: alsa-devel
  Cc: Pierre-Louis Bossart, tiwai, gregkh, linux-kernel,
	Ranjani Sridharan, vkoul, broonie, srinivas.kandagatla, jank,
	slawomir.blauciak, Sanyog Kale, Bard liao, Rander Wang

If a SoundWire link is enabled, but there are no Slave devices exposed
in firmware tables for this link, or no Slaves in ATTACHED or ALERT
mode, the CMD_IGNORED/-ENODATA error code on a broadcast write is
perfectly legit.

Filter this case to report errors and let the caller deal with the
CMD_IGNORED case.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 3395abd2ed39..13887713f311 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -922,8 +922,12 @@ int sdw_bus_clk_stop(struct sdw_bus *bus)
 	ret = sdw_bwrite_no_pm(bus, SDW_BROADCAST_DEV_NUM,
 			       SDW_SCP_CTRL, SDW_SCP_CTRL_CLK_STP_NOW);
 	if (ret < 0) {
-		dev_err(bus->dev,
-			"ClockStopNow Broadcast message failed %d", ret);
+		if (ret == -ENODATA)
+			dev_dbg(bus->dev,
+				"ClockStopNow Broadcast msg ignored %d", ret);
+		else
+			dev_err(bus->dev,
+				"ClockStopNow Broadcast msg failed %d", ret);
 		return ret;
 	}
 
-- 
2.20.1

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (9 preceding siblings ...)
  2020-01-15  0:08 ` [alsa-devel] [PATCH 10/10] soundwire: bus: don't treat CMD_IGNORED as error on ClockStop Pierre-Louis Bossart
@ 2020-02-10 14:30 ` Pierre-Louis Bossart
  2020-02-25 10:27 ` Vinod Koul
  11 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-02-10 14:30 UTC (permalink / raw)
  To: alsa-devel
  Cc: tiwai, gregkh, linux-kernel, Ranjani Sridharan, vkoul, broonie,
	srinivas.kandagatla, jank, slawomir.blauciak, Bard liao,
	Rander Wang



On 1/14/20 6:08 PM, Pierre-Louis Bossart wrote:
> The existing mainline code for SoundWire does not handle critical race
> conditions, and does not have any support for pm_runtime suspend or
> clock-stop modes needed for e.g. jack detection or external VAD.
> 
> As suggested by Vinod, these patches for the bus are shared first -
> with the risk that they are separated from their actual use in Intel
> drivers, so reviewers might wonder why they are needed in the first
> place.
> 
> For reference, the complete set of 90+ patches required for SoundWire
> on Intel platforms is available here:
> 
> https://github.com/thesofproject/linux/pull/1692
> 
> These patches are not Intel-specific and are likely required for
> e.g. Qualcomm-based implementations.
> 
> All the patches in this series were generated during the joint
> Intel-Realtek validation effort on Intel reference designs and
> form-factor devices. The support for the initialization_complete
> signaling is already available in the Realtek codecs drivers merged in
> the ASoC tree (rt700, rt711, rt1308, rt715)

there's been no feedback since January 14, can we move on with the 
reviews now that r.6-rc1 is out?
Thanks!

> 
> Pierre-Louis Bossart (8):
>    soundwire: bus: fix race condition with probe_complete signaling
>    soundwire: bus: fix race condition with enumeration_complete signaling
>    soundwire: bus: fix race condition with initialization_complete
>      signaling
>    soundwire: bus: add PM/no-PM versions of read/write functions
>    soundwire: bus: write Slave Device Number without runtime_pm
>    soundwire: bus: add helper to clear Slave status to UNATTACHED
>    soundwire: bus: disable pm_runtime in sdw_slave_delete
>    soundwire: bus: don't treat CMD_IGNORED as error on ClockStop
> 
> Rander Wang (2):
>    soundwire: bus: fix io error when processing alert event
>    soundwire: bus: add clock stop helpers
> 
>   drivers/soundwire/bus.c       | 509 ++++++++++++++++++++++++++++++++--
>   drivers/soundwire/bus.h       |   9 +
>   drivers/soundwire/bus_type.c  |   5 +
>   drivers/soundwire/slave.c     |   4 +
>   include/linux/soundwire/sdw.h |  24 ++
>   5 files changed, 526 insertions(+), 25 deletions(-)
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
https://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

* Re: [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume
  2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
                   ` (10 preceding siblings ...)
  2020-02-10 14:30 ` [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
@ 2020-02-25 10:27 ` Vinod Koul
  2020-02-25 15:23   ` Pierre-Louis Bossart
  11 siblings, 1 reply; 14+ messages in thread
From: Vinod Koul @ 2020-02-25 10:27 UTC (permalink / raw)
  To: Pierre-Louis Bossart
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak, Bard liao,
	Rander Wang

On 14-01-20, 18:08, Pierre-Louis Bossart wrote:
> The existing mainline code for SoundWire does not handle critical race
> conditions, and does not have any support for pm_runtime suspend or
> clock-stop modes needed for e.g. jack detection or external VAD.
> 
> As suggested by Vinod, these patches for the bus are shared first -
> with the risk that they are separated from their actual use in Intel
> drivers, so reviewers might wonder why they are needed in the first
> place.
> 
> For reference, the complete set of 90+ patches required for SoundWire
> on Intel platforms is available here:
> 
> https://github.com/thesofproject/linux/pull/1692
> 
> These patches are not Intel-specific and are likely required for
> e.g. Qualcomm-based implementations.
> 
> All the patches in this series were generated during the joint
> Intel-Realtek validation effort on Intel reference designs and
> form-factor devices. The support for the initialization_complete
> signaling is already available in the Realtek codecs drivers merged in
> the ASoC tree (rt700, rt711, rt1308, rt715)

Applied all, thanks

-- 
~Vinod

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

* Re: [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume
  2020-02-25 10:27 ` Vinod Koul
@ 2020-02-25 15:23   ` Pierre-Louis Bossart
  0 siblings, 0 replies; 14+ messages in thread
From: Pierre-Louis Bossart @ 2020-02-25 15:23 UTC (permalink / raw)
  To: Vinod Koul
  Cc: alsa-devel, tiwai, gregkh, linux-kernel, Ranjani Sridharan,
	broonie, srinivas.kandagatla, jank, slawomir.blauciak, Bard liao,
	Rander Wang



On 2/25/20 4:27 AM, Vinod Koul wrote:
> On 14-01-20, 18:08, Pierre-Louis Bossart wrote:
>> The existing mainline code for SoundWire does not handle critical race
>> conditions, and does not have any support for pm_runtime suspend or
>> clock-stop modes needed for e.g. jack detection or external VAD.
>>
>> As suggested by Vinod, these patches for the bus are shared first -
>> with the risk that they are separated from their actual use in Intel
>> drivers, so reviewers might wonder why they are needed in the first
>> place.
>>
>> For reference, the complete set of 90+ patches required for SoundWire
>> on Intel platforms is available here:
>>
>> https://github.com/thesofproject/linux/pull/1692
>>
>> These patches are not Intel-specific and are likely required for
>> e.g. Qualcomm-based implementations.
>>
>> All the patches in this series were generated during the joint
>> Intel-Realtek validation effort on Intel reference designs and
>> form-factor devices. The support for the initialization_complete
>> signaling is already available in the Realtek codecs drivers merged in
>> the ASoC tree (rt700, rt711, rt1308, rt715)
> 
> Applied all, thanks

Thanks Vinod, I'll now prepare the update for the transition away from 
platform devices (minor update needed on the RFC already reviewed by Greg).

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

end of thread, other threads:[~2020-02-25 15:24 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-15  0:08 [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 01/10] soundwire: bus: fix race condition with probe_complete signaling Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 02/10] soundwire: bus: fix race condition with enumeration_complete signaling Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 03/10] soundwire: bus: fix race condition with initialization_complete signaling Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 04/10] soundwire: bus: add PM/no-PM versions of read/write functions Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 05/10] soundwire: bus: write Slave Device Number without runtime_pm Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 06/10] soundwire: bus: add helper to clear Slave status to UNATTACHED Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 07/10] soundwire: bus: disable pm_runtime in sdw_slave_delete Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 08/10] soundwire: bus: fix io error when processing alert event Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 09/10] soundwire: bus: add clock stop helpers Pierre-Louis Bossart
2020-01-15  0:08 ` [alsa-devel] [PATCH 10/10] soundwire: bus: don't treat CMD_IGNORED as error on ClockStop Pierre-Louis Bossart
2020-02-10 14:30 ` [alsa-devel] [PATCH 00/10] soundwire: bus: fix race conditions, add suspend-resume Pierre-Louis Bossart
2020-02-25 10:27 ` Vinod Koul
2020-02-25 15:23   ` Pierre-Louis Bossart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).