[PATCH v4 0/7] soundwire: Add multi link support

[PATCH v4 0/7] soundwire: Add multi link support

[Shreyas NC]

Currently, in the SoundWire subsystem, the concept of stream is
limited to a Master and one or more Slaves(Codecs). This series
aims to add support for multiple Master(s) sharing the same
reference clock and synchronized in the hardware.

This patch series adds:
 - Helpers to lock bus instances part of the stream
 - Boiler plate conversion of code to support a list of
   Master runtime
 - Support multi link bank switch to support synchronization
   between multiple masters
 - Add Intel platform ops for pre/post bank switch

changes in v4:
 - Added changes in sdw_stream_runtime structure to track Masters
   added to stream
 - Add changes to support ml_bankswitch only if both the hardware
   supports and the stream is handled by multiple masters

changes in v3:
 - Added comments to explain the trigger for multi link
   bank switch
 - Added comments in the intel_post_bank_switch ops to explain
   how synchronous trigger of bank switch is handled.

Sanyog Kale (2):
  Documentation: soundwire: Add documentation for multi link
  soundwire: Add support to lock across bus instances

Shreyas NC (4):
  soundwire: Initialize completion for defer messages
  soundwire: keep track of Masters in a stream
  soundwire: Add support for multi link bank switch
  soundwire: intel: Add pre/post bank switch ops

Vinod Koul (1):
  soundwire: Handle multiple master instances in a stream

 Documentation/driver-api/soundwire/stream.rst |  28 ++
 drivers/soundwire/bus.c                       |   6 +
 drivers/soundwire/bus.h                       |   4 +
 drivers/soundwire/intel.c                     |  65 ++++
 drivers/soundwire/stream.c                    | 483 +++++++++++++++++++-------
 include/linux/soundwire/sdw.h                 |  12 +-
 6 files changed, 467 insertions(+), 131 deletions(-)

-- 
2.7.4

[PATCH v4 1/7] Documentation: soundwire: Add documentation for multi link

[Shreyas NC]

From: Sanyog Kale <sanyog.r.kale@intel.com>

Add example and documentation to describe multi link streams

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 Documentation/driver-api/soundwire/stream.rst | 28 +++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/driver-api/soundwire/stream.rst b/Documentation/driver-api/soundwire/stream.rst
index 29121aa..19482ee 100644
--- a/Documentation/driver-api/soundwire/stream.rst
+++ b/Documentation/driver-api/soundwire/stream.rst
@@ -101,6 +101,34 @@ interface. ::
 	+--------------------+                             |                |
 							   +----------------+
 
+Example 5: Stereo Stream with L and R channel is rendered by 2 Masters, each
+rendering one channel, and is received by two different Slaves, each
+receiving one channel. Both Masters and both Slaves are using single port. ::
+
+	+---------------+                    Clock Signal  +---------------+
+	|    Master     +----------------------------------+     Slave     |
+	|   Interface   |                                  |   Interface   |
+	|       1       |                                  |       1       |
+	|               |                     Data Signal  |               |
+	|       L       +----------------------------------+       L       |
+	|     (Data)    |     Data Direction               |     (Data)    |
+	+---------------+  +----------------------->       +---------------+
+
+	+---------------+                    Clock Signal  +---------------+
+	|    Master     +----------------------------------+     Slave     |
+	|   Interface   |                                  |   Interface   |
+	|       2       |                                  |       2       |
+	|               |                     Data Signal  |               |
+	|       R       +----------------------------------+       R       |
+	|     (Data)    |     Data Direction               |     (Data)    |
+	+---------------+  +----------------------->       +---------------+
+
+Note: In multi-link cases like above, to lock, one would acquire a global
+lock and then go on locking bus instances. But, in this case the caller
+framework(ASoC DPCM) guarantees that stream operations on a card are
+always serialized. So, there is no race condition and hence no need for
+global lock.
+
 SoundWire Stream Management flow
 ================================
 
-- 
2.7.4

[PATCH v4 2/7] soundwire: Initialize completion for defer messages

[Shreyas NC]

Deferred messages are async messages used to synchronize
transitions mostly while doing a bank switch on multi links.
On successful transitions these messages are marked complete
and thereby confirming that all the buses performed bank switch
successfully.

So, initialize the completion structure for the same.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/bus.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index dcc0ff9..dbabd5e 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -175,6 +175,7 @@ static inline int do_transfer_defer(struct sdw_bus *bus,
 
 	defer->msg = msg;
 	defer->length = msg->len;
+	init_completion(&defer->complete);
 
 	for (i = 0; i <= retry; i++) {
 		resp = bus->ops->xfer_msg_defer(bus, msg, defer);
-- 
2.7.4

[PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Shreyas NC]

From: Sanyog Kale <sanyog.r.kale@intel.com>

Currently, the stream concept is limited to single Master and one
or more Codecs.

This patch extends the concept to support multiple Master(s)
sharing the same reference clock and synchronized in the hardware.
Modify sdw_stream_runtime to support a list of sdw_master_runtime
for the same. The existing reference to a single m_rt is removed
in the next patch.

Typically to lock, one would acquire a global lock and then lock
bus instances. In this case, the caller framework(ASoC DPCM)
guarantees that stream operations on a card are always serialized.
So, there is no race condition and hence no need for global lock.

Bus lock(s) are acquired to reconfigure the bus while the stream
is set-up.
So, we add sdw_acquire_bus_lock()/sdw_release_bus_lock() APIs which
are used only to reconfigure the bus.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/bus.h       |  2 ++
 drivers/soundwire/stream.c    | 41 +++++++++++++++++++++++++++++++++++++++++
 include/linux/soundwire/sdw.h |  4 ++++
 3 files changed, 47 insertions(+)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index 3b15c4e..b6cfbdf 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -99,6 +99,7 @@ struct sdw_slave_runtime {
  * this stream, can be zero.
  * @slave_rt_list: Slave runtime list
  * @port_list: List of Master Ports configured for this stream, can be zero.
+ * @stream_node: sdw_stream_runtime master_list node
  * @bus_node: sdw_bus m_rt_list node
  */
 struct sdw_master_runtime {
@@ -108,6 +109,7 @@ struct sdw_master_runtime {
 	unsigned int ch_count;
 	struct list_head slave_rt_list;
 	struct list_head port_list;
+	struct list_head stream_node;
 	struct list_head bus_node;
 };
 
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 8974a0f..eb942c6 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -747,6 +747,7 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
 		return NULL;
 
 	stream->name = stream_name;
+	INIT_LIST_HEAD(&stream->master_list);
 	stream->state = SDW_STREAM_ALLOCATED;
 
 	return stream;
@@ -1234,6 +1235,46 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
 	return NULL;
 }
 
+/**
+ * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
+ *
+ * @stream: SoundWire stream
+ *
+ * Acquire bus_lock for each of the master runtime(m_rt) part of this
+ * stream to reconfigure the bus.
+ */
+static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
+{
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
+
+	/* Iterate for all Master(s) in Master list */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+
+		mutex_lock(&bus->bus_lock);
+	}
+}
+
+/**
+ * sdw_release_bus_lock: Release bus lock for all Master runtime(s)
+ *
+ * @stream: SoundWire stream
+ *
+ * Release the previously held bus_lock after reconfiguring the bus.
+ */
+static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
+{
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
+
+	/* Iterate for all Master(s) in Master list */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		mutex_unlock(&bus->bus_lock);
+	}
+}
+
 static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 {
 	struct sdw_master_runtime *m_rt = stream->m_rt;
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 962971e..ccd8dcdf 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -769,6 +769,9 @@ struct sdw_stream_params {
  * @state: Current state of the stream
  * @type: Stream type PCM or PDM
  * @m_rt: Master runtime
+ * @master_list: List of Master runtime(s) in this stream.
+ * master_list can contain only one m_rt per Master instance
+ * for a stream
  */
 struct sdw_stream_runtime {
 	char *name;
@@ -776,6 +779,7 @@ struct sdw_stream_runtime {
 	enum sdw_stream_state state;
 	enum sdw_stream_type type;
 	struct sdw_master_runtime *m_rt;
+	struct list_head master_list;
 };
 
 struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name);
-- 
2.7.4

[PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Shreyas NC]

From: Vinod Koul <vkoul@kernel.org>

For each SoundWire stream operation, we need to parse master
list and operate upon all master runtime.

This is a preparatory patch to do the boilerplate conversion
of stream handling from single master runtime to handle a
list of master runtime. The code to support bank switch for
multiple master instances is added in the next patch.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/stream.c    | 309 +++++++++++++++++++++++++-----------------
 include/linux/soundwire/sdw.h |   2 -
 2 files changed, 186 insertions(+), 125 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index eb942c6..0e8a2eb5 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -681,35 +681,45 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 
 static int do_bank_switch(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
+	struct sdw_master_runtime *m_rt = NULL;
 	const struct sdw_master_ops *ops;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_bus *bus = NULL;
 	int ret = 0;
 
-	ops = bus->ops;
 
-	/* Pre-bank switch */
-	if (ops->pre_bank_switch) {
-		ret = ops->pre_bank_switch(bus);
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		ops = bus->ops;
+
+		/* Pre-bank switch */
+		if (ops->pre_bank_switch) {
+			ret = ops->pre_bank_switch(bus);
+			if (ret < 0) {
+				dev_err(bus->dev,
+					"Pre bank switch op failed: %d", ret);
+				return ret;
+			}
+		}
+
+		/* Bank switch */
+		ret = sdw_bank_switch(bus);
 		if (ret < 0) {
-			dev_err(bus->dev, "Pre bank switch op failed: %d", ret);
+			dev_err(bus->dev, "Bank switch failed: %d", ret);
 			return ret;
 		}
 	}
 
-	/* Bank switch */
-	ret = sdw_bank_switch(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Bank switch failed: %d", ret);
-		return ret;
-	}
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		ops = bus->ops;
 
-	/* Post-bank switch */
-	if (ops->post_bank_switch) {
-		ret = ops->post_bank_switch(bus);
-		if (ret < 0) {
-			dev_err(bus->dev,
+		/* Post-bank switch */
+		if (ops->post_bank_switch) {
+			ret = ops->post_bank_switch(bus);
+			if (ret < 0) {
+				dev_err(bus->dev,
 					"Post bank switch op failed: %d", ret);
+			}
 		}
 	}
 
@@ -754,6 +764,21 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
 }
 EXPORT_SYMBOL(sdw_alloc_stream);
 
+static struct sdw_master_runtime
+*sdw_find_master_rt(struct sdw_bus *bus,
+			struct sdw_stream_runtime *stream)
+{
+	struct sdw_master_runtime *m_rt = NULL;
+
+	/* Retrieve Bus handle if already available */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		if (m_rt->bus == bus)
+			return m_rt;
+	}
+
+	return NULL;
+}
+
 /**
  * sdw_alloc_master_rt() - Allocates and initialize Master runtime handle
  *
@@ -770,12 +795,11 @@ static struct sdw_master_runtime
 {
 	struct sdw_master_runtime *m_rt;
 
-	m_rt = stream->m_rt;
-
 	/*
 	 * check if Master is already allocated (as a result of Slave adding
 	 * it first), if so skip allocation and go to configure
 	 */
+	m_rt = sdw_find_master_rt(bus, stream);
 	if (m_rt)
 		goto stream_config;
 
@@ -786,7 +810,7 @@ static struct sdw_master_runtime
 	/* Initialization of Master runtime handle */
 	INIT_LIST_HEAD(&m_rt->port_list);
 	INIT_LIST_HEAD(&m_rt->slave_rt_list);
-	stream->m_rt = m_rt;
+	list_add_tail(&m_rt->stream_node, &stream->master_list);
 
 	list_add_tail(&m_rt->bus_node, &bus->m_rt_list);
 
@@ -844,17 +868,21 @@ static void sdw_slave_port_release(struct sdw_bus *bus,
 			struct sdw_stream_runtime *stream)
 {
 	struct sdw_port_runtime *p_rt, *_p_rt;
-	struct sdw_master_runtime *m_rt = stream->m_rt;
+	struct sdw_master_runtime *m_rt;
 	struct sdw_slave_runtime *s_rt;
 
-	list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
-		if (s_rt->slave != slave)
-			continue;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		list_for_each_entry(s_rt, &m_rt->slave_rt_list, m_rt_node) {
 
-		list_for_each_entry_safe(p_rt, _p_rt,
-				&s_rt->port_list, port_node) {
-			list_del(&p_rt->port_node);
-			kfree(p_rt);
+			if (s_rt->slave != slave)
+				continue;
+
+			list_for_each_entry_safe(p_rt, _p_rt,
+					&s_rt->port_list, port_node) {
+
+				list_del(&p_rt->port_node);
+				kfree(p_rt);
+			}
 		}
 	}
 }
@@ -871,16 +899,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
 			struct sdw_stream_runtime *stream)
 {
 	struct sdw_slave_runtime *s_rt, *_s_rt;
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-
-	/* Retrieve Slave runtime handle */
-	list_for_each_entry_safe(s_rt, _s_rt,
-			&m_rt->slave_rt_list, m_rt_node) {
+	struct sdw_master_runtime *m_rt;
 
-		if (s_rt->slave == slave) {
-			list_del(&s_rt->m_rt_node);
-			kfree(s_rt);
-			return;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		/* Retrieve Slave runtime handle */
+		list_for_each_entry_safe(s_rt, _s_rt,
+					&m_rt->slave_rt_list, m_rt_node) {
+
+			if (s_rt->slave == slave) {
+				list_del(&s_rt->m_rt_node);
+				kfree(s_rt);
+				return;
+			}
 		}
 	}
 }
@@ -888,6 +918,7 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
 /**
  * sdw_release_master_stream() - Free Master runtime handle
  *
+ * @m_rt: Master runtime node
  * @stream: Stream runtime handle.
  *
  * This function is to be called with bus_lock held
@@ -895,16 +926,18 @@ static void sdw_release_slave_stream(struct sdw_slave *slave,
  * handle. If this is called first then sdw_release_slave_stream() will have
  * no effect as Slave(s) runtime handle would already be freed up.
  */
-static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
+static void sdw_release_master_stream(struct sdw_master_runtime *m_rt,
+			struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
 	struct sdw_slave_runtime *s_rt, *_s_rt;
 
 	list_for_each_entry_safe(s_rt, _s_rt,
 			&m_rt->slave_rt_list, m_rt_node)
 		sdw_stream_remove_slave(s_rt->slave, stream);
 
+	list_del(&m_rt->stream_node);
 	list_del(&m_rt->bus_node);
+	kfree(m_rt);
 }
 
 /**
@@ -918,13 +951,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
 int sdw_stream_remove_master(struct sdw_bus *bus,
 		struct sdw_stream_runtime *stream)
 {
+	struct sdw_master_runtime *m_rt, *_m_rt;
+
 	mutex_lock(&bus->bus_lock);
 
-	sdw_release_master_stream(stream);
-	sdw_master_port_release(bus, stream->m_rt);
-	stream->state = SDW_STREAM_RELEASED;
-	kfree(stream->m_rt);
-	stream->m_rt = NULL;
+	list_for_each_entry_safe(m_rt, _m_rt,
+			&stream->master_list, stream_node) {
+
+		if (m_rt->bus != bus)
+			continue;
+
+		sdw_master_port_release(bus, m_rt);
+		sdw_release_master_stream(m_rt, stream);
+	}
+
+	if (list_empty(&stream->master_list))
+		stream->state = SDW_STREAM_RELEASED;
 
 	mutex_unlock(&bus->bus_lock);
 
@@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 	stream->state = SDW_STREAM_CONFIGURED;
 
 stream_error:
-	sdw_release_master_stream(stream);
+	sdw_release_master_stream(m_rt, stream);
 error:
 	mutex_unlock(&bus->bus_lock);
 	return ret;
@@ -1195,7 +1237,7 @@ int sdw_stream_add_slave(struct sdw_slave *slave,
 	 * we hit error so cleanup the stream, release all Slave(s) and
 	 * Master runtime
 	 */
-	sdw_release_master_stream(stream);
+	sdw_release_master_stream(m_rt, stream);
 error:
 	mutex_unlock(&slave->bus->bus_lock);
 	return ret;
@@ -1277,31 +1319,36 @@ static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
 
 static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	struct sdw_master_prop *prop = NULL;
 	struct sdw_bus_params params;
 	int ret;
 
-	prop = &bus->prop;
-	memcpy(&params, &bus->params, sizeof(params));
+	/* Prepare  Master(s) and Slave(s) port(s) associated with stream */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		prop = &bus->prop;
+		memcpy(&params, &bus->params, sizeof(params));
 
-	/* TODO: Support Asynchronous mode */
-	if ((prop->max_freq % stream->params.rate) != 0) {
-		dev_err(bus->dev, "Async mode not supported");
-		return -EINVAL;
-	}
+		/* TODO: Support Asynchronous mode */
+		if ((prop->max_freq % stream->params.rate) != 0) {
+			dev_err(bus->dev, "Async mode not supported");
+			return -EINVAL;
+		}
 
-	/* Increment cumulative bus bandwidth */
-	/* TODO: Update this during Device-Device support */
-	bus->params.bandwidth += m_rt->stream->params.rate *
-		m_rt->ch_count * m_rt->stream->params.bps;
+		/* Increment cumulative bus bandwidth */
+		/* TODO: Update this during Device-Device support */
+		bus->params.bandwidth += m_rt->stream->params.rate *
+			m_rt->ch_count * m_rt->stream->params.bps;
+
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			goto restore_params;
+		}
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		goto restore_params;
 	}
 
 	ret = do_bank_switch(stream);
@@ -1310,12 +1357,16 @@ static int _sdw_prepare_stream(struct sdw_stream_runtime *stream)
 		goto restore_params;
 	}
 
-	/* Prepare port(s) on the new clock configuration */
-	ret = sdw_prep_deprep_ports(m_rt, true);
-	if (ret < 0) {
-		dev_err(bus->dev, "Prepare port(s) failed ret = %d",
-				ret);
-		return ret;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+
+		/* Prepare port(s) on the new clock configuration */
+		ret = sdw_prep_deprep_ports(m_rt, true);
+		if (ret < 0) {
+			dev_err(bus->dev, "Prepare port(s) failed ret = %d",
+					ret);
+			return ret;
+		}
 	}
 
 	stream->state = SDW_STREAM_PREPARED;
@@ -1343,35 +1394,40 @@ int sdw_prepare_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
+	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_prepare_stream(stream);
 	if (ret < 0)
 		pr_err("Prepare for stream:%s failed: %d", stream->name, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_prepare_stream);
 
 static int _sdw_enable_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	int ret;
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		return ret;
-	}
+	/* Enable Master(s) and Slave(s) port(s) associated with stream */
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
 
-	/* Enable port(s) */
-	ret = sdw_enable_disable_ports(m_rt, true);
-	if (ret < 0) {
-		dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
-		return ret;
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			return ret;
+		}
+
+		/* Enable port(s) */
+		ret = sdw_enable_disable_ports(m_rt, true);
+		if (ret < 0) {
+			dev_err(bus->dev, "Enable port(s) failed ret: %d", ret);
+			return ret;
+		}
 	}
 
 	ret = do_bank_switch(stream);
@@ -1400,37 +1456,42 @@ int sdw_enable_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
+	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_enable_stream(stream);
 	if (ret < 0)
 		pr_err("Enable for stream:%s failed: %d", stream->name, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_enable_stream);
 
 static int _sdw_disable_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	int ret;
 
-	/* Disable port(s) */
-	ret = sdw_enable_disable_ports(m_rt, false);
-	if (ret < 0) {
-		dev_err(bus->dev, "Disable port(s) failed: %d", ret);
-		return ret;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		/* Disable port(s) */
+		ret = sdw_enable_disable_ports(m_rt, false);
+		if (ret < 0) {
+			dev_err(bus->dev, "Disable port(s) failed: %d", ret);
+			return ret;
+		}
 	}
-
 	stream->state = SDW_STREAM_DISABLED;
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		return ret;
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			return ret;
+		}
 	}
 
 	return do_bank_switch(stream);
@@ -1452,43 +1513,46 @@ int sdw_disable_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
+	sdw_acquire_bus_lock(stream);
 
 	ret = _sdw_disable_stream(stream);
 	if (ret < 0)
 		pr_err("Disable for stream:%s failed: %d", stream->name, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_disable_stream);
 
 static int _sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 {
-	struct sdw_master_runtime *m_rt = stream->m_rt;
-	struct sdw_bus *bus = m_rt->bus;
+	struct sdw_master_runtime *m_rt = NULL;
+	struct sdw_bus *bus = NULL;
 	int ret = 0;
 
-	/* De-prepare port(s) */
-	ret = sdw_prep_deprep_ports(m_rt, false);
-	if (ret < 0) {
-		dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
-		return ret;
-	}
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+		bus = m_rt->bus;
+		/* De-prepare port(s) */
+		ret = sdw_prep_deprep_ports(m_rt, false);
+		if (ret < 0) {
+			dev_err(bus->dev, "De-prepare port(s) failed: %d", ret);
+			return ret;
+		}
 
-	stream->state = SDW_STREAM_DEPREPARED;
+		/* TODO: Update this during Device-Device support */
+		bus->params.bandwidth -= m_rt->stream->params.rate *
+			m_rt->ch_count * m_rt->stream->params.bps;
 
-	/* TODO: Update this during Device-Device support */
-	bus->params.bandwidth -= m_rt->stream->params.rate *
-		m_rt->ch_count * m_rt->stream->params.bps;
+		/* Program params */
+		ret = sdw_program_params(bus);
+		if (ret < 0) {
+			dev_err(bus->dev, "Program params failed: %d", ret);
+			return ret;
+		}
 
-	/* Program params */
-	ret = sdw_program_params(bus);
-	if (ret < 0) {
-		dev_err(bus->dev, "Program params failed: %d", ret);
-		return ret;
 	}
 
+	stream->state = SDW_STREAM_DEPREPARED;
 	return do_bank_switch(stream);
 }
 
@@ -1508,13 +1572,12 @@ int sdw_deprepare_stream(struct sdw_stream_runtime *stream)
 		return -EINVAL;
 	}
 
-	mutex_lock(&stream->m_rt->bus->bus_lock);
-
+	sdw_acquire_bus_lock(stream);
 	ret = _sdw_deprepare_stream(stream);
 	if (ret < 0)
 		pr_err("De-prepare for stream:%d failed: %d", ret, ret);
 
-	mutex_unlock(&stream->m_rt->bus->bus_lock);
+	sdw_release_bus_lock(stream);
 	return ret;
 }
 EXPORT_SYMBOL(sdw_deprepare_stream);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index ccd8dcdf..03df709 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -768,7 +768,6 @@ struct sdw_stream_params {
  * @params: Stream parameters
  * @state: Current state of the stream
  * @type: Stream type PCM or PDM
- * @m_rt: Master runtime
  * @master_list: List of Master runtime(s) in this stream.
  * master_list can contain only one m_rt per Master instance
  * for a stream
@@ -778,7 +777,6 @@ struct sdw_stream_runtime {
 	struct sdw_stream_params params;
 	enum sdw_stream_state state;
 	enum sdw_stream_type type;
-	struct sdw_master_runtime *m_rt;
 	struct list_head master_list;
 };
 
-- 
2.7.4

[PATCH v4 5/7] soundwire: keep track of Masters in a stream

[Shreyas NC]

A multi link bankswitch can be done if the hardware supports and
the stream is handled by multiple Master(s).

This preparatory patch adds support to track m_rt in a stream.

Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/stream.c    | 4 ++++
 include/linux/soundwire/sdw.h | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 0e8a2eb5..f358b92 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -759,6 +759,7 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
 	stream->name = stream_name;
 	INIT_LIST_HEAD(&stream->master_list);
 	stream->state = SDW_STREAM_ALLOCATED;
+	stream->m_rt_count = 0;
 
 	return stream;
 }
@@ -968,6 +969,7 @@ int sdw_stream_remove_master(struct sdw_bus *bus,
 	if (list_empty(&stream->master_list))
 		stream->state = SDW_STREAM_RELEASED;
 
+	stream->m_rt_count--;
 	mutex_unlock(&bus->bus_lock);
 
 	return 0;
@@ -1166,6 +1168,8 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 	if (ret)
 		goto stream_error;
 
+	stream->m_rt_count++;
+
 	stream->state = SDW_STREAM_CONFIGURED;
 
 stream_error:
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 03df709..214e146 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -771,6 +771,7 @@ struct sdw_stream_params {
  * @master_list: List of Master runtime(s) in this stream.
  * master_list can contain only one m_rt per Master instance
  * for a stream
+ * @m_rt_count: Count of Master runtime(s) in this stream
  */
 struct sdw_stream_runtime {
 	char *name;
@@ -778,6 +779,7 @@ struct sdw_stream_runtime {
 	enum sdw_stream_state state;
 	enum sdw_stream_type type;
 	struct list_head master_list;
+	int m_rt_count;
 };
 
 struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name);
-- 
2.7.4

[PATCH v4 6/7] soundwire: Add support for multi link bank switch

[Shreyas NC]

In cases of multiple Masters in a stream, synchronization
between multiple Master(s) is achieved by performing bank switch
together and using Master methods.

Add sdw_ml_bank_switch() to wait for completion of bank switch.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/bus.c       |   5 ++
 drivers/soundwire/bus.h       |   2 +
 drivers/soundwire/stream.c    | 141 ++++++++++++++++++++++++++++++++++++++----
 include/linux/soundwire/sdw.h |   4 ++
 4 files changed, 140 insertions(+), 12 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index dbabd5e..1cbfedf 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -35,6 +35,11 @@ int sdw_add_bus_master(struct sdw_bus *bus)
 	INIT_LIST_HEAD(&bus->slaves);
 	INIT_LIST_HEAD(&bus->m_rt_list);
 
+	/*
+	 * Initialize multi_link flag
+	 * TODO: populate this flag by reading property from FW node
+	 */
+	bus->multi_link = false;
 	if (bus->ops->read_prop) {
 		ret = bus->ops->read_prop(bus);
 		if (ret < 0) {
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index b6cfbdf..c77de05 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -4,6 +4,8 @@
 #ifndef __SDW_BUS_H
 #define __SDW_BUS_H
 
+#define DEFAULT_BANK_SWITCH_TIMEOUT 3000
+
 #if IS_ENABLED(CONFIG_ACPI)
 int sdw_acpi_find_slaves(struct sdw_bus *bus);
 #else
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index f358b92..fe738a8 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -626,9 +626,10 @@ static int sdw_program_params(struct sdw_bus *bus)
 	return ret;
 }
 
-static int sdw_bank_switch(struct sdw_bus *bus)
+static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count)
 {
 	int col_index, row_index;
+	bool multi_link;
 	struct sdw_msg *wr_msg;
 	u8 *wbuf = NULL;
 	int ret = 0;
@@ -638,6 +639,8 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 	if (!wr_msg)
 		return -ENOMEM;
 
+	bus->defer_msg.msg = wr_msg;
+
 	wbuf = kzalloc(sizeof(*wbuf), GFP_KERNEL);
 	if (!wbuf) {
 		ret = -ENOMEM;
@@ -658,17 +661,29 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 					SDW_MSG_FLAG_WRITE, wbuf);
 	wr_msg->ssp_sync = true;
 
-	ret = sdw_transfer(bus, wr_msg);
+	/*
+	 * Set the multi_link flag only when both the hardware supports
+	 * and there is a stream handled by multiple masters
+	 */
+	multi_link = bus->multi_link && (m_rt_count > 1);
+
+	if (multi_link)
+		ret = sdw_transfer_defer(bus, wr_msg, &bus->defer_msg);
+	else
+		ret = sdw_transfer(bus, wr_msg);
+
 	if (ret < 0) {
 		dev_err(bus->dev, "Slave frame_ctrl reg write failed");
 		goto error;
 	}
 
-	kfree(wr_msg);
-	kfree(wbuf);
-	bus->defer_msg.msg = NULL;
-	bus->params.curr_bank = !bus->params.curr_bank;
-	bus->params.next_bank = !bus->params.next_bank;
+	if (!bus->multi_link) {
+		kfree(wr_msg);
+		kfree(wbuf);
+		bus->defer_msg.msg = NULL;
+		bus->params.curr_bank = !bus->params.curr_bank;
+		bus->params.next_bank = !bus->params.next_bank;
+	}
 
 	return 0;
 
@@ -679,36 +694,87 @@ static int sdw_bank_switch(struct sdw_bus *bus)
 	return ret;
 }
 
+/**
+ * sdw_ml_sync_bank_switch: Multilink register bank switch
+ *
+ * @bus: SDW bus instance
+ *
+ * Caller function should free the buffers on error
+ */
+static int sdw_ml_sync_bank_switch(struct sdw_bus *bus)
+{
+	unsigned long time_left;
+
+	if (!bus->multi_link)
+		return 0;
+
+	/* Wait for completion of transfer */
+	time_left = wait_for_completion_timeout(&bus->defer_msg.complete,
+						bus->bank_switch_timeout);
+
+	if (!time_left) {
+		dev_err(bus->dev, "Controller Timed out on bank switch");
+		return -ETIMEDOUT;
+	}
+
+	bus->params.curr_bank = !bus->params.curr_bank;
+	bus->params.next_bank = !bus->params.next_bank;
+
+	if (bus->defer_msg.msg) {
+		kfree(bus->defer_msg.msg->buf);
+		kfree(bus->defer_msg.msg);
+	}
+
+	return 0;
+}
+
 static int do_bank_switch(struct sdw_stream_runtime *stream)
 {
 	struct sdw_master_runtime *m_rt = NULL;
 	const struct sdw_master_ops *ops;
 	struct sdw_bus *bus = NULL;
+	bool multi_link = false;
 	int ret = 0;
 
-
 	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
 		bus = m_rt->bus;
 		ops = bus->ops;
 
+		if (bus->multi_link) {
+			multi_link = true;
+			mutex_lock(&bus->msg_lock);
+		}
+
 		/* Pre-bank switch */
 		if (ops->pre_bank_switch) {
 			ret = ops->pre_bank_switch(bus);
 			if (ret < 0) {
 				dev_err(bus->dev,
 					"Pre bank switch op failed: %d", ret);
-				return ret;
+				goto msg_unlock;
 			}
 		}
 
-		/* Bank switch */
-		ret = sdw_bank_switch(bus);
+		/*
+		 * Perform Bank switch operation.
+		 * For multi link cases, the actual bank switch is
+		 * synchronized across all Masters and happens later as a
+		 * part of post_bank_switch ops.
+		 */
+		ret = sdw_bank_switch(bus, stream->m_rt_count);
 		if (ret < 0) {
 			dev_err(bus->dev, "Bank switch failed: %d", ret);
-			return ret;
+			goto error;
+
 		}
 	}
 
+	/*
+	 * For multi link cases, it is expected that the bank switch is
+	 * triggered by the post_bank_switch for the first Master in the list
+	 * and for the other Masters the post_bank_switch() should return doing
+	 * nothing.
+	 */
 	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
 		bus = m_rt->bus;
 		ops = bus->ops;
@@ -719,7 +785,47 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
 			if (ret < 0) {
 				dev_err(bus->dev,
 					"Post bank switch op failed: %d", ret);
+				goto error;
 			}
+		} else if (bus->multi_link && stream->m_rt_count > 1) {
+			dev_err(bus->dev,
+				"Post bank switch ops not implemented");
+			goto error;
+		}
+
+		/* Set the bank switch timeout to default, if not set */
+		if (!bus->bank_switch_timeout)
+			bus->bank_switch_timeout = DEFAULT_BANK_SWITCH_TIMEOUT;
+
+		/* Check if bank switch was successful */
+		ret = sdw_ml_sync_bank_switch(bus);
+		if (ret < 0) {
+			dev_err(bus->dev,
+				"multi link bank switch failed: %d", ret);
+			goto error;
+		}
+
+		mutex_unlock(&bus->msg_lock);
+	}
+
+	return ret;
+
+error:
+	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+
+		bus = m_rt->bus;
+
+		kfree(bus->defer_msg.msg->buf);
+		kfree(bus->defer_msg.msg);
+	}
+
+msg_unlock:
+
+	if (multi_link) {
+		list_for_each_entry(m_rt, &stream->master_list, stream_node) {
+			bus = m_rt->bus;
+			if (mutex_is_locked(&bus->msg_lock))
+				mutex_unlock(&bus->msg_lock);
 		}
 	}
 
@@ -1170,6 +1276,17 @@ int sdw_stream_add_master(struct sdw_bus *bus,
 
 	stream->m_rt_count++;
 
+	/*
+	 * For multi link streams, add the second master only if
+	 * the bus supports it.
+	 * Check if bus->multi_link is set
+	 */
+	if (!bus->multi_link && stream->m_rt_count > 1) {
+		dev_err(bus->dev,
+			"Multilink not supported, link %d", bus->link_id);
+		goto stream_error;
+	}
+
 	stream->state = SDW_STREAM_CONFIGURED;
 
 stream_error:
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 214e146..df31391 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -678,6 +678,9 @@ struct sdw_master_ops {
  * @defer_msg: Defer message
  * @clk_stop_timeout: Clock stop timeout computed
  * @bank_switch_timeout: Bank switch timeout computed
+ * @multi_link: Store bus property that indicates if multi links
+ * are supported. This flag is populated by drivers after reading
+ * appropriate firmware (ACPI/DT).
  */
 struct sdw_bus {
 	struct device *dev;
@@ -694,6 +697,7 @@ struct sdw_bus {
 	struct sdw_defer defer_msg;
 	unsigned int clk_stop_timeout;
 	u32 bank_switch_timeout;
+	bool multi_link;
 };
 
 int sdw_add_bus_master(struct sdw_bus *bus);
-- 
2.7.4

[PATCH v4 7/7] soundwire: intel: Add pre/post bank switch ops

[Shreyas NC]

To support multi link on Intel platforms, we need to update
SDW SHIM registers.

So, add pre/post bank switch ops for the same in Intel driver.

Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
---
 drivers/soundwire/intel.c | 65 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 65 insertions(+)

diff --git a/drivers/soundwire/intel.c b/drivers/soundwire/intel.c
index 0a8990e..da7f91f 100644
--- a/drivers/soundwire/intel.c
+++ b/drivers/soundwire/intel.c
@@ -398,6 +398,69 @@ static int intel_config_stream(struct sdw_intel *sdw,
 }
 
 /*
+ * bank switch routines
+ */
+
+static int intel_pre_bank_switch(struct sdw_bus *bus)
+{
+	struct sdw_cdns *cdns = bus_to_cdns(bus);
+	struct sdw_intel *sdw = cdns_to_intel(cdns);
+	void __iomem *shim = sdw->res->shim;
+	int sync_reg;
+
+	/* Write to register only for multi-link */
+	if (!bus->multi_link)
+		return 0;
+
+	/* Read SYNC register */
+	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
+	sync_reg |= SDW_SHIM_SYNC_CMDSYNC << sdw->instance;
+	intel_writel(shim, SDW_SHIM_SYNC, sync_reg);
+
+	return 0;
+}
+
+static int intel_post_bank_switch(struct sdw_bus *bus)
+{
+	struct sdw_cdns *cdns = bus_to_cdns(bus);
+	struct sdw_intel *sdw = cdns_to_intel(cdns);
+	void __iomem *shim = sdw->res->shim;
+	int sync_reg, ret;
+
+	/* Write to register only for multi-link */
+	if (!bus->multi_link)
+		return 0;
+
+	/* Read SYNC register */
+	sync_reg = intel_readl(shim, SDW_SHIM_SYNC);
+
+	/*
+	 * post_bank_switch() ops is called from the bus in loop for
+	 * all the Masters in the steam with the expectation that
+	 * we trigger the bankswitch for the only first Master in the list
+	 * and do nothing for the other Masters
+	 *
+	 * So, set the SYNCGO bit only if CMDSYNC bit is set for any Master.
+	 */
+	if (!(sync_reg & SDW_SHIM_SYNC_CMDSYNC_MASK))
+		return 0;
+
+	/*
+	 * Set SyncGO bit to synchronously trigger a bank switch for
+	 * all the masters. A write to SYNCGO bit clears CMDSYNC bit for all
+	 * the Masters.
+	 */
+	sync_reg |= SDW_SHIM_SYNC_SYNCGO;
+
+	ret = intel_clear_bit(shim, SDW_SHIM_SYNC, sync_reg,
+					SDW_SHIM_SYNC_SYNCGO);
+	if (ret < 0)
+		dev_err(sdw->cdns.dev, "Post bank switch failed: %d", ret);
+
+	return ret;
+}
+
+/*
  * DAI routines
  */
 
@@ -750,6 +813,8 @@ static struct sdw_master_ops sdw_intel_ops = {
 	.xfer_msg_defer = cdns_xfer_msg_defer,
 	.reset_page_addr = cdns_reset_page_addr,
 	.set_bus_conf = cdns_bus_conf,
+	.pre_bank_switch = intel_pre_bank_switch,
+	.post_bank_switch = intel_post_bank_switch,
 };
 
 /*
-- 
2.7.4

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Takashi Iwai]

On Mon, 25 Jun 2018 12:58:56 +0200,
Shreyas NC wrote:
> 
> From: Sanyog Kale <sanyog.r.kale@intel.com>
> 
> Currently, the stream concept is limited to single Master and one
> or more Codecs.
> 
> This patch extends the concept to support multiple Master(s)
> sharing the same reference clock and synchronized in the hardware.
> Modify sdw_stream_runtime to support a list of sdw_master_runtime
> for the same. The existing reference to a single m_rt is removed
> in the next patch.
> 
> Typically to lock, one would acquire a global lock and then lock
> bus instances. In this case, the caller framework(ASoC DPCM)
> guarantees that stream operations on a card are always serialized.
> So, there is no race condition and hence no need for global lock.
> 
> Bus lock(s) are acquired to reconfigure the bus while the stream
> is set-up.
> So, we add sdw_acquire_bus_lock()/sdw_release_bus_lock() APIs which
> are used only to reconfigure the bus.
> 
> Signed-off-by: Sanyog Kale <sanyog.r.kale@intel.com>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> Signed-off-by: Shreyas NC <shreyas.nc@intel.com>
> ---
>  drivers/soundwire/bus.h       |  2 ++
>  drivers/soundwire/stream.c    | 41 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/soundwire/sdw.h |  4 ++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
> index 3b15c4e..b6cfbdf 100644
> --- a/drivers/soundwire/bus.h
> +++ b/drivers/soundwire/bus.h
> @@ -99,6 +99,7 @@ struct sdw_slave_runtime {
>   * this stream, can be zero.
>   * @slave_rt_list: Slave runtime list
>   * @port_list: List of Master Ports configured for this stream, can be zero.
> + * @stream_node: sdw_stream_runtime master_list node
>   * @bus_node: sdw_bus m_rt_list node
>   */
>  struct sdw_master_runtime {
> @@ -108,6 +109,7 @@ struct sdw_master_runtime {
>  	unsigned int ch_count;
>  	struct list_head slave_rt_list;
>  	struct list_head port_list;
> +	struct list_head stream_node;
>  	struct list_head bus_node;
>  };
>  
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 8974a0f..eb942c6 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -747,6 +747,7 @@ struct sdw_stream_runtime *sdw_alloc_stream(char *stream_name)
>  		return NULL;
>  
>  	stream->name = stream_name;
> +	INIT_LIST_HEAD(&stream->master_list);
>  	stream->state = SDW_STREAM_ALLOCATED;
>  
>  	return stream;
> @@ -1234,6 +1235,46 @@ struct sdw_dpn_prop *sdw_get_slave_dpn_prop(struct sdw_slave *slave,
>  	return NULL;
>  }
>  
> +/**
> + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> + *
> + * @stream: SoundWire stream
> + *
> + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> + * stream to reconfigure the bus.
> + */
> +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt = NULL;
> +	struct sdw_bus *bus = NULL;
> +
> +	/* Iterate for all Master(s) in Master list */
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +
> +		mutex_lock(&bus->bus_lock);
> +	}
> +}

So it's nested locks?  Then you'd need some more trick to deal with
the lockdep.  I guess you'll get the false-positive deadlock detection
by this code when the mutex lock debug is enabled.

Also, is the linked order assured not to lead to a real deadlock?

> +/**
> + * sdw_release_bus_lock: Release bus lock for all Master runtime(s)
> + *
> + * @stream: SoundWire stream
> + *
> + * Release the previously held bus_lock after reconfiguring the bus.
> + */
> +static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
> +{
> +	struct sdw_master_runtime *m_rt = NULL;
> +	struct sdw_bus *bus = NULL;
> +
> +	/* Iterate for all Master(s) in Master list */
> +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> +		bus = m_rt->bus;
> +		mutex_unlock(&bus->bus_lock);
> +	}

... and this looks bad.  The loop for unlocking should be traversed
reversely.


thanks,

Takashi

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Shreyas NC]

> > +/**
> > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > + *
> > + * @stream: SoundWire stream
> > + *
> > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > + * stream to reconfigure the bus.
> > + */
> > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > +{
> > +	struct sdw_master_runtime *m_rt = NULL;
> > +	struct sdw_bus *bus = NULL;
> > +
> > +	/* Iterate for all Master(s) in Master list */
> > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > +		bus = m_rt->bus;
> > +
> > +		mutex_lock(&bus->bus_lock);
> > +	}
> > +}
> 
> So it's nested locks?  Then you'd need some more trick to deal with
> the lockdep.  I guess you'll get the false-positive deadlock detection
> by this code when the mutex lock debug is enabled.
> 
> Also, is the linked order assured not to lead to a real deadlock?
>

Hi Takashi,

Thanks for the review :)

A multi link SoundWire stream consists of a list of Master runtimes and
more importantly only one master runtime per SoundWire bus instance.

So, these mutexes are actually different mutex locks(one per bus instance)
and are not nested.

In SDW we have a bus instance per Master (link). In multi-link case, a
stream may have multiple Masters, thus we need to lock all bus instances
before we operate on them.

Now since these are invoked from a stream (pcm ops) they will be always
serialized and DPCM ensures we are never racing.

We did add this note here and in Documentation to make it explicit.

> > +/**
> > + * sdw_release_bus_lock: Release bus lock for all Master runtime(s)
> > + *
> > + * @stream: SoundWire stream
> > + *
> > + * Release the previously held bus_lock after reconfiguring the bus.
> > + */
> > +static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
> > +{
> > +	struct sdw_master_runtime *m_rt = NULL;
> > +	struct sdw_bus *bus = NULL;
> > +
> > +	/* Iterate for all Master(s) in Master list */
> > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > +		bus = m_rt->bus;
> > +		mutex_unlock(&bus->bus_lock);
> > +	}
> 
> ... and this looks bad.  The loop for unlocking should be traversed
> reversely.
> 

Yes in principle I agree locking should be in reverse, but as explained
above in this case, it does not matter much :)

--Shreyas

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Takashi Iwai]

On Tue, 26 Jun 2018 10:22:01 +0200,
Shreyas NC wrote:
> 
> > > +/**
> > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > + *
> > > + * @stream: SoundWire stream
> > > + *
> > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > + * stream to reconfigure the bus.
> > > + */
> > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > +{
> > > +	struct sdw_master_runtime *m_rt = NULL;
> > > +	struct sdw_bus *bus = NULL;
> > > +
> > > +	/* Iterate for all Master(s) in Master list */
> > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > +		bus = m_rt->bus;
> > > +
> > > +		mutex_lock(&bus->bus_lock);
> > > +	}
> > > +}
> > 
> > So it's nested locks?  Then you'd need some more trick to deal with
> > the lockdep.  I guess you'll get the false-positive deadlock detection
> > by this code when the mutex lock debug is enabled.
> > 
> > Also, is the linked order assured not to lead to a real deadlock?
> >
> 
> Hi Takashi,
> 
> Thanks for the review :)
> 
> A multi link SoundWire stream consists of a list of Master runtimes and
> more importantly only one master runtime per SoundWire bus instance.
> 
> So, these mutexes are actually different mutex locks(one per bus instance)
> and are not nested.

You take a mutex lock inside a mutex lock, so they are nested.
If they take the very same lock, it's called a "deadlock" instead.

> In SDW we have a bus instance per Master (link). In multi-link case, a
> stream may have multiple Masters, thus we need to lock all bus instances
> before we operate on them.
> 
> Now since these are invoked from a stream (pcm ops) they will be always
> serialized and DPCM ensures we are never racing.
> 
> We did add this note here and in Documentation to make it explicit.

Well, my question is whether the order to take the multiple locks is
always assured.  You're calling like:

	list_for_each_entry(m_rt, &stream->master_list, stream_node)
		mutex_lock();

And it's a linked-list.  If a stream has a link of masters like
M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
to a deadlock with the concurrent calls above.

> > > +/**
> > > + * sdw_release_bus_lock: Release bus lock for all Master runtime(s)
> > > + *
> > > + * @stream: SoundWire stream
> > > + *
> > > + * Release the previously held bus_lock after reconfiguring the bus.
> > > + */
> > > +static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
> > > +{
> > > +	struct sdw_master_runtime *m_rt = NULL;
> > > +	struct sdw_bus *bus = NULL;
> > > +
> > > +	/* Iterate for all Master(s) in Master list */
> > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > +		bus = m_rt->bus;
> > > +		mutex_unlock(&bus->bus_lock);
> > > +	}
> > 
> > ... and this looks bad.  The loop for unlocking should be traversed
> > reversely.
> > 
> 
> Yes in principle I agree locking should be in reverse, but as explained
> above in this case, it does not matter much :)

It does matter when you dealing with the multiple nested mutexes...


thanks,

Takashi

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Shreyas NC]

On Tue, Jun 26, 2018 at 10:34:17AM +0200, Takashi Iwai wrote:
> On Tue, 26 Jun 2018 10:22:01 +0200,
> Shreyas NC wrote:
> > 
> > > > +/**
> > > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > > + *
> > > > + * @stream: SoundWire stream
> > > > + *
> > > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > > + * stream to reconfigure the bus.
> > > > + */
> > > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > > +{
> > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > +	struct sdw_bus *bus = NULL;
> > > > +
> > > > +	/* Iterate for all Master(s) in Master list */
> > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > +		bus = m_rt->bus;
> > > > +
> > > > +		mutex_lock(&bus->bus_lock);
> > > > +	}
> > > > +}
> > > 
> > > So it's nested locks?  Then you'd need some more trick to deal with
> > > the lockdep.  I guess you'll get the false-positive deadlock detection
> > > by this code when the mutex lock debug is enabled.
> > > 
> > > Also, is the linked order assured not to lead to a real deadlock?
> > >
> > 
> > Hi Takashi,
> > 
> > Thanks for the review :)
> > 
> > A multi link SoundWire stream consists of a list of Master runtimes and
> > more importantly only one master runtime per SoundWire bus instance.
> > 
> > So, these mutexes are actually different mutex locks(one per bus instance)
> > and are not nested.
> 
> You take a mutex lock inside a mutex lock, so they are nested.
> If they take the very same lock, it's called a "deadlock" instead.
> 

Ok, myy bad, I misunderstood the comment :(

I forgot to add that I did check with mutex debug enabled and lockdep did
not complain though :)

> > In SDW we have a bus instance per Master (link). In multi-link case, a
> > stream may have multiple Masters, thus we need to lock all bus instances
> > before we operate on them.
> > 
> > Now since these are invoked from a stream (pcm ops) they will be always
> > serialized and DPCM ensures we are never racing.
> > 
> > We did add this note here and in Documentation to make it explicit.
> 
> Well, my question is whether the order to take the multiple locks is
> always assured.  You're calling like:
> 
> 	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> 		mutex_lock();
> 
> And it's a linked-list.  If a stream has a link of masters like
> M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
> to a deadlock with the concurrent calls above.
> 

These are called from PCM stream ops context and the DPCM holds
lock(fe->card->mutex) which serializes these operations.
So, in the scenario you have mentioned, we would not have
concurrent calls to this function.

> > > > +/**
> > > > + * sdw_release_bus_lock: Release bus lock for all Master runtime(s)
> > > > + *
> > > > + * @stream: SoundWire stream
> > > > + *
> > > > + * Release the previously held bus_lock after reconfiguring the bus.
> > > > + */
> > > > +static void sdw_release_bus_lock(struct sdw_stream_runtime *stream)
> > > > +{
> > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > +	struct sdw_bus *bus = NULL;
> > > > +
> > > > +	/* Iterate for all Master(s) in Master list */
> > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > +		bus = m_rt->bus;
> > > > +		mutex_unlock(&bus->bus_lock);
> > > > +	}
> > > 
> > > ... and this looks bad.  The loop for unlocking should be traversed
> > > reversely.
> > > 
> > 
> > Yes in principle I agree locking should be in reverse, but as explained
> > above in this case, it does not matter much :)
> 
> It does matter when you dealing with the multiple nested mutexes...
> 

Ok

--Shreyas

-- 

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Takashi Iwai]

On Tue, 26 Jun 2018 11:23:59 +0200,
Shreyas NC wrote:
> 
> On Tue, Jun 26, 2018 at 10:34:17AM +0200, Takashi Iwai wrote:
> > On Tue, 26 Jun 2018 10:22:01 +0200,
> > Shreyas NC wrote:
> > > 
> > > > > +/**
> > > > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > > > + *
> > > > > + * @stream: SoundWire stream
> > > > > + *
> > > > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > > > + * stream to reconfigure the bus.
> > > > > + */
> > > > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > > > +{
> > > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > > +	struct sdw_bus *bus = NULL;
> > > > > +
> > > > > +	/* Iterate for all Master(s) in Master list */
> > > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > > +		bus = m_rt->bus;
> > > > > +
> > > > > +		mutex_lock(&bus->bus_lock);
> > > > > +	}
> > > > > +}
> > > > 
> > > > So it's nested locks?  Then you'd need some more trick to deal with
> > > > the lockdep.  I guess you'll get the false-positive deadlock detection
> > > > by this code when the mutex lock debug is enabled.
> > > > 
> > > > Also, is the linked order assured not to lead to a real deadlock?
> > > >
> > > 
> > > Hi Takashi,
> > > 
> > > Thanks for the review :)
> > > 
> > > A multi link SoundWire stream consists of a list of Master runtimes and
> > > more importantly only one master runtime per SoundWire bus instance.
> > > 
> > > So, these mutexes are actually different mutex locks(one per bus instance)
> > > and are not nested.
> > 
> > You take a mutex lock inside a mutex lock, so they are nested.
> > If they take the very same lock, it's called a "deadlock" instead.
> > 
> 
> Ok, myy bad, I misunderstood the comment :(
> 
> I forgot to add that I did check with mutex debug enabled and lockdep did
> not complain though :)

You didn't test the actual concurrent calls because of FE's mutex
below, right?


> > > In SDW we have a bus instance per Master (link). In multi-link case, a
> > > stream may have multiple Masters, thus we need to lock all bus instances
> > > before we operate on them.
> > > 
> > > Now since these are invoked from a stream (pcm ops) they will be always
> > > serialized and DPCM ensures we are never racing.
> > > 
> > > We did add this note here and in Documentation to make it explicit.
> > 
> > Well, my question is whether the order to take the multiple locks is
> > always assured.  You're calling like:
> > 
> > 	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> > 		mutex_lock();
> > 
> > And it's a linked-list.  If a stream has a link of masters like
> > M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
> > to a deadlock with the concurrent calls above.
> > 
> 
> These are called from PCM stream ops context and the DPCM holds
> lock(fe->card->mutex) which serializes these operations.
> So, in the scenario you have mentioned, we would not have
> concurrent calls to this function.

The implementation of soundwire bus is basically independent from ASoC
or whatever else.  That is, any other drivers may use this API, and
it'll be busted.


Takashi

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Shreyas NC]

On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote:
> On Tue, 26 Jun 2018 11:23:59 +0200,
> Shreyas NC wrote:
> > 
> > On Tue, Jun 26, 2018 at 10:34:17AM +0200, Takashi Iwai wrote:
> > > On Tue, 26 Jun 2018 10:22:01 +0200,
> > > Shreyas NC wrote:
> > > > 
> > > > > > +/**
> > > > > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > > > > + *
> > > > > > + * @stream: SoundWire stream
> > > > > > + *
> > > > > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > > > > + * stream to reconfigure the bus.
> > > > > > + */
> > > > > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > > > > +{
> > > > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > > > +	struct sdw_bus *bus = NULL;
> > > > > > +
> > > > > > +	/* Iterate for all Master(s) in Master list */
> > > > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > > > +		bus = m_rt->bus;
> > > > > > +
> > > > > > +		mutex_lock(&bus->bus_lock);
> > > > > > +	}
> > > > > > +}
> > > > > 
> > > > > So it's nested locks?  Then you'd need some more trick to deal with
> > > > > the lockdep.  I guess you'll get the false-positive deadlock detection
> > > > > by this code when the mutex lock debug is enabled.
> > > > > 
> > > > > Also, is the linked order assured not to lead to a real deadlock?
> > > > >
> > > > 
> > > > Hi Takashi,
> > > > 
> > > > Thanks for the review :)
> > > > 
> > > > A multi link SoundWire stream consists of a list of Master runtimes and
> > > > more importantly only one master runtime per SoundWire bus instance.
> > > > 
> > > > So, these mutexes are actually different mutex locks(one per bus instance)
> > > > and are not nested.
> > > 
> > > You take a mutex lock inside a mutex lock, so they are nested.
> > > If they take the very same lock, it's called a "deadlock" instead.
> > > 
> > 
> > Ok, myy bad, I misunderstood the comment :(
> > 
> > I forgot to add that I did check with mutex debug enabled and lockdep did
> > not complain though :)
> 
> You didn't test the actual concurrent calls because of FE's mutex
> below, right?
> 

Yes

> 
> > > > In SDW we have a bus instance per Master (link). In multi-link case, a
> > > > stream may have multiple Masters, thus we need to lock all bus instances
> > > > before we operate on them.
> > > > 
> > > > Now since these are invoked from a stream (pcm ops) they will be always
> > > > serialized and DPCM ensures we are never racing.
> > > > 
> > > > We did add this note here and in Documentation to make it explicit.
> > > 
> > > Well, my question is whether the order to take the multiple locks is
> > > always assured.  You're calling like:
> > > 
> > > 	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> > > 		mutex_lock();
> > > 
> > > And it's a linked-list.  If a stream has a link of masters like
> > > M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
> > > to a deadlock with the concurrent calls above.
> > > 
> > 
> > These are called from PCM stream ops context and the DPCM holds
> > lock(fe->card->mutex) which serializes these operations.
> > So, in the scenario you have mentioned, we would not have
> > concurrent calls to this function.
> 
> The implementation of soundwire bus is basically independent from ASoC
> or whatever else.  That is, any other drivers may use this API, and
> it'll be busted.
> 

Hmmh, yes. The only way we could think of to protect this is to use a global
lock which we wanted to avoid.
We did give this a thought and since today no other subsytem
can use this, we went ahead with the way it is today.
Any suggestions on how to go about this ?

--Shreyas
-- 

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Takashi Iwai]

On Tue, 26 Jun 2018 11:59:32 +0200,
Shreyas NC wrote:
> 
> On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote:
> > On Tue, 26 Jun 2018 11:23:59 +0200,
> > Shreyas NC wrote:
> > > 
> > > On Tue, Jun 26, 2018 at 10:34:17AM +0200, Takashi Iwai wrote:
> > > > On Tue, 26 Jun 2018 10:22:01 +0200,
> > > > Shreyas NC wrote:
> > > > > 
> > > > > > > +/**
> > > > > > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > > > > > + *
> > > > > > > + * @stream: SoundWire stream
> > > > > > > + *
> > > > > > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > > > > > + * stream to reconfigure the bus.
> > > > > > > + */
> > > > > > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > > > > > +{
> > > > > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > > > > +	struct sdw_bus *bus = NULL;
> > > > > > > +
> > > > > > > +	/* Iterate for all Master(s) in Master list */
> > > > > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > > > > +		bus = m_rt->bus;
> > > > > > > +
> > > > > > > +		mutex_lock(&bus->bus_lock);
> > > > > > > +	}
> > > > > > > +}
> > > > > > 
> > > > > > So it's nested locks?  Then you'd need some more trick to deal with
> > > > > > the lockdep.  I guess you'll get the false-positive deadlock detection
> > > > > > by this code when the mutex lock debug is enabled.
> > > > > > 
> > > > > > Also, is the linked order assured not to lead to a real deadlock?
> > > > > >
> > > > > 
> > > > > Hi Takashi,
> > > > > 
> > > > > Thanks for the review :)
> > > > > 
> > > > > A multi link SoundWire stream consists of a list of Master runtimes and
> > > > > more importantly only one master runtime per SoundWire bus instance.
> > > > > 
> > > > > So, these mutexes are actually different mutex locks(one per bus instance)
> > > > > and are not nested.
> > > > 
> > > > You take a mutex lock inside a mutex lock, so they are nested.
> > > > If they take the very same lock, it's called a "deadlock" instead.
> > > > 
> > > 
> > > Ok, myy bad, I misunderstood the comment :(
> > > 
> > > I forgot to add that I did check with mutex debug enabled and lockdep did
> > > not complain though :)
> > 
> > You didn't test the actual concurrent calls because of FE's mutex
> > below, right?
> > 
> 
> Yes
> 
> > 
> > > > > In SDW we have a bus instance per Master (link). In multi-link case, a
> > > > > stream may have multiple Masters, thus we need to lock all bus instances
> > > > > before we operate on them.
> > > > > 
> > > > > Now since these are invoked from a stream (pcm ops) they will be always
> > > > > serialized and DPCM ensures we are never racing.
> > > > > 
> > > > > We did add this note here and in Documentation to make it explicit.
> > > > 
> > > > Well, my question is whether the order to take the multiple locks is
> > > > always assured.  You're calling like:
> > > > 
> > > > 	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> > > > 		mutex_lock();
> > > > 
> > > > And it's a linked-list.  If a stream has a link of masters like
> > > > M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
> > > > to a deadlock with the concurrent calls above.
> > > > 
> > > 
> > > These are called from PCM stream ops context and the DPCM holds
> > > lock(fe->card->mutex) which serializes these operations.
> > > So, in the scenario you have mentioned, we would not have
> > > concurrent calls to this function.
> > 
> > The implementation of soundwire bus is basically independent from ASoC
> > or whatever else.  That is, any other drivers may use this API, and
> > it'll be busted.
> > 
> 
> Hmmh, yes. The only way we could think of to protect this is to use a global
> lock which we wanted to avoid.
> We did give this a thought and since today no other subsytem
> can use this, we went ahead with the way it is today.
> Any suggestions on how to go about this ?

If so, you have to give an explicit big fat warning for the usage in
the function description.


Takashi

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Shreyas NC]

On Tue, Jun 26, 2018 at 12:16:42PM +0200, Takashi Iwai wrote:
> On Tue, 26 Jun 2018 11:59:32 +0200,
> Shreyas NC wrote:
> > 
> > On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote:
> > > On Tue, 26 Jun 2018 11:23:59 +0200,
> > > Shreyas NC wrote:
> > > > 
> > > > On Tue, Jun 26, 2018 at 10:34:17AM +0200, Takashi Iwai wrote:
> > > > > On Tue, 26 Jun 2018 10:22:01 +0200,
> > > > > Shreyas NC wrote:
> > > > > > 
> > > > > > > > +/**
> > > > > > > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > > > > > > + *
> > > > > > > > + * @stream: SoundWire stream
> > > > > > > > + *
> > > > > > > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > > > > > > + * stream to reconfigure the bus.
> > > > > > > > + */
> > > > > > > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > > > > > > +{
> > > > > > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > > > > > +	struct sdw_bus *bus = NULL;
> > > > > > > > +
> > > > > > > > +	/* Iterate for all Master(s) in Master list */
> > > > > > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > > > > > +		bus = m_rt->bus;
> > > > > > > > +
> > > > > > > > +		mutex_lock(&bus->bus_lock);
> > > > > > > > +	}
> > > > > > > > +}
> > > > > > > 
> > > > > > > So it's nested locks?  Then you'd need some more trick to deal with
> > > > > > > the lockdep.  I guess you'll get the false-positive deadlock detection
> > > > > > > by this code when the mutex lock debug is enabled.
> > > > > > > 
> > > > > > > Also, is the linked order assured not to lead to a real deadlock?
> > > > > > >
> > > > > > 
> > > > > > Hi Takashi,
> > > > > > 
> > > > > > Thanks for the review :)
> > > > > > 
> > > > > > A multi link SoundWire stream consists of a list of Master runtimes and
> > > > > > more importantly only one master runtime per SoundWire bus instance.
> > > > > > 
> > > > > > So, these mutexes are actually different mutex locks(one per bus instance)
> > > > > > and are not nested.
> > > > > 
> > > > > You take a mutex lock inside a mutex lock, so they are nested.
> > > > > If they take the very same lock, it's called a "deadlock" instead.
> > > > > 
> > > > 
> > > > Ok, myy bad, I misunderstood the comment :(
> > > > 
> > > > I forgot to add that I did check with mutex debug enabled and lockdep did
> > > > not complain though :)
> > > 
> > > You didn't test the actual concurrent calls because of FE's mutex
> > > below, right?
> > > 
> > 
> > Yes
> > 
> > > 
> > > > > > In SDW we have a bus instance per Master (link). In multi-link case, a
> > > > > > stream may have multiple Masters, thus we need to lock all bus instances
> > > > > > before we operate on them.
> > > > > > 
> > > > > > Now since these are invoked from a stream (pcm ops) they will be always
> > > > > > serialized and DPCM ensures we are never racing.
> > > > > > 
> > > > > > We did add this note here and in Documentation to make it explicit.
> > > > > 
> > > > > Well, my question is whether the order to take the multiple locks is
> > > > > always assured.  You're calling like:
> > > > > 
> > > > > 	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> > > > > 		mutex_lock();
> > > > > 
> > > > > And it's a linked-list.  If a stream has a link of masters like
> > > > > M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
> > > > > to a deadlock with the concurrent calls above.
> > > > > 
> > > > 
> > > > These are called from PCM stream ops context and the DPCM holds
> > > > lock(fe->card->mutex) which serializes these operations.
> > > > So, in the scenario you have mentioned, we would not have
> > > > concurrent calls to this function.
> > > 
> > > The implementation of soundwire bus is basically independent from ASoC
> > > or whatever else.  That is, any other drivers may use this API, and
> > > it'll be busted.
> > > 
> > 
> > Hmmh, yes. The only way we could think of to protect this is to use a global
> > lock which we wanted to avoid.
> > We did give this a thought and since today no other subsytem
> > can use this, we went ahead with the way it is today.
> > Any suggestions on how to go about this ?
> 
> If so, you have to give an explicit big fat warning for the usage in
> the function description.
> 

Ok, I had added it in the commit log and documentation.
It does make sense to add it in the function description.

Thanks!

--Shreyas
-- 

Re: [PATCH v4 3/7] soundwire: Add support to lock across bus instances

[Takashi Iwai]

On Tue, 26 Jun 2018 12:22:01 +0200,
Shreyas NC wrote:
> 
> On Tue, Jun 26, 2018 at 12:16:42PM +0200, Takashi Iwai wrote:
> > On Tue, 26 Jun 2018 11:59:32 +0200,
> > Shreyas NC wrote:
> > > 
> > > On Tue, Jun 26, 2018 at 11:46:35AM +0200, Takashi Iwai wrote:
> > > > On Tue, 26 Jun 2018 11:23:59 +0200,
> > > > Shreyas NC wrote:
> > > > > 
> > > > > On Tue, Jun 26, 2018 at 10:34:17AM +0200, Takashi Iwai wrote:
> > > > > > On Tue, 26 Jun 2018 10:22:01 +0200,
> > > > > > Shreyas NC wrote:
> > > > > > > 
> > > > > > > > > +/**
> > > > > > > > > + * sdw_acquire_bus_lock: Acquire bus lock for all Master runtime(s)
> > > > > > > > > + *
> > > > > > > > > + * @stream: SoundWire stream
> > > > > > > > > + *
> > > > > > > > > + * Acquire bus_lock for each of the master runtime(m_rt) part of this
> > > > > > > > > + * stream to reconfigure the bus.
> > > > > > > > > + */
> > > > > > > > > +static void sdw_acquire_bus_lock(struct sdw_stream_runtime *stream)
> > > > > > > > > +{
> > > > > > > > > +	struct sdw_master_runtime *m_rt = NULL;
> > > > > > > > > +	struct sdw_bus *bus = NULL;
> > > > > > > > > +
> > > > > > > > > +	/* Iterate for all Master(s) in Master list */
> > > > > > > > > +	list_for_each_entry(m_rt, &stream->master_list, stream_node) {
> > > > > > > > > +		bus = m_rt->bus;
> > > > > > > > > +
> > > > > > > > > +		mutex_lock(&bus->bus_lock);
> > > > > > > > > +	}
> > > > > > > > > +}
> > > > > > > > 
> > > > > > > > So it's nested locks?  Then you'd need some more trick to deal with
> > > > > > > > the lockdep.  I guess you'll get the false-positive deadlock detection
> > > > > > > > by this code when the mutex lock debug is enabled.
> > > > > > > > 
> > > > > > > > Also, is the linked order assured not to lead to a real deadlock?
> > > > > > > >
> > > > > > > 
> > > > > > > Hi Takashi,
> > > > > > > 
> > > > > > > Thanks for the review :)
> > > > > > > 
> > > > > > > A multi link SoundWire stream consists of a list of Master runtimes and
> > > > > > > more importantly only one master runtime per SoundWire bus instance.
> > > > > > > 
> > > > > > > So, these mutexes are actually different mutex locks(one per bus instance)
> > > > > > > and are not nested.
> > > > > > 
> > > > > > You take a mutex lock inside a mutex lock, so they are nested.
> > > > > > If they take the very same lock, it's called a "deadlock" instead.
> > > > > > 
> > > > > 
> > > > > Ok, myy bad, I misunderstood the comment :(
> > > > > 
> > > > > I forgot to add that I did check with mutex debug enabled and lockdep did
> > > > > not complain though :)
> > > > 
> > > > You didn't test the actual concurrent calls because of FE's mutex
> > > > below, right?
> > > > 
> > > 
> > > Yes
> > > 
> > > > 
> > > > > > > In SDW we have a bus instance per Master (link). In multi-link case, a
> > > > > > > stream may have multiple Masters, thus we need to lock all bus instances
> > > > > > > before we operate on them.
> > > > > > > 
> > > > > > > Now since these are invoked from a stream (pcm ops) they will be always
> > > > > > > serialized and DPCM ensures we are never racing.
> > > > > > > 
> > > > > > > We did add this note here and in Documentation to make it explicit.
> > > > > > 
> > > > > > Well, my question is whether the order to take the multiple locks is
> > > > > > always assured.  You're calling like:
> > > > > > 
> > > > > > 	list_for_each_entry(m_rt, &stream->master_list, stream_node)
> > > > > > 		mutex_lock();
> > > > > > 
> > > > > > And it's a linked-list.  If a stream has a link of masters like
> > > > > > M1->M2->M3 while another stream has a link like M2->M1->M3, it'll lead
> > > > > > to a deadlock with the concurrent calls above.
> > > > > > 
> > > > > 
> > > > > These are called from PCM stream ops context and the DPCM holds
> > > > > lock(fe->card->mutex) which serializes these operations.
> > > > > So, in the scenario you have mentioned, we would not have
> > > > > concurrent calls to this function.
> > > > 
> > > > The implementation of soundwire bus is basically independent from ASoC
> > > > or whatever else.  That is, any other drivers may use this API, and
> > > > it'll be busted.
> > > > 
> > > 
> > > Hmmh, yes. The only way we could think of to protect this is to use a global
> > > lock which we wanted to avoid.
> > > We did give this a thought and since today no other subsytem
> > > can use this, we went ahead with the way it is today.
> > > Any suggestions on how to go about this ?
> > 
> > If so, you have to give an explicit big fat warning for the usage in
> > the function description.
> > 
> 
> Ok, I had added it in the commit log and documentation.
> It does make sense to add it in the function description.

Yes.  It needs a clear sign showing that the concurrency has to be
resolved in the caller side.


Takashi

Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Pierre-Louis Bossart]

>   /**
> @@ -918,13 +951,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
>   int sdw_stream_remove_master(struct sdw_bus *bus,
>   		struct sdw_stream_runtime *stream)
>   {
> +	struct sdw_master_runtime *m_rt, *_m_rt;
> +
>   	mutex_lock(&bus->bus_lock);
>   
> -	sdw_release_master_stream(stream);
> -	sdw_master_port_release(bus, stream->m_rt);
> -	stream->state = SDW_STREAM_RELEASED;
> -	kfree(stream->m_rt);
> -	stream->m_rt = NULL;
> +	list_for_each_entry_safe(m_rt, _m_rt,
> +			&stream->master_list, stream_node) {
> +
> +		if (m_rt->bus != bus)
> +			continue;
> +
> +		sdw_master_port_release(bus, m_rt);
> +		sdw_release_master_stream(m_rt, stream);
> +	}
> +
> +	if (list_empty(&stream->master_list))
> +		stream->state = SDW_STREAM_RELEASED;
When a master is removed, there is an explicit test to make sure the 
stream state changes when there are no masters left in the list, but...

>   
>   	mutex_unlock(&bus->bus_lock);
>   
> @@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
>   	stream->state = SDW_STREAM_CONFIGURED;
... it's not symmetrical for the add_master case. The stream state 
changes on the first added master. In addition the stream state changes 
both when a slave is added and a master is added.
Is this intentional or not - and are there side effects resulting from 
this inconsistency?

Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Shreyas NC]

On Mon, Jul 02, 2018 at 03:22:07PM -0500, Pierre-Louis Bossart wrote:
> 
> >  /**
> >@@ -918,13 +951,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
> >  int sdw_stream_remove_master(struct sdw_bus *bus,
> >  		struct sdw_stream_runtime *stream)
> >  {
> >+	struct sdw_master_runtime *m_rt, *_m_rt;
> >+
> >  	mutex_lock(&bus->bus_lock);
> >-	sdw_release_master_stream(stream);
> >-	sdw_master_port_release(bus, stream->m_rt);
> >-	stream->state = SDW_STREAM_RELEASED;
> >-	kfree(stream->m_rt);
> >-	stream->m_rt = NULL;
> >+	list_for_each_entry_safe(m_rt, _m_rt,
> >+			&stream->master_list, stream_node) {
> >+
> >+		if (m_rt->bus != bus)
> >+			continue;
> >+
> >+		sdw_master_port_release(bus, m_rt);
> >+		sdw_release_master_stream(m_rt, stream);
> >+	}
> >+
> >+	if (list_empty(&stream->master_list))
> >+		stream->state = SDW_STREAM_RELEASED;
> When a master is removed, there is an explicit test to make sure the stream
> state changes when there are no masters left in the list, but...
>
> >  	mutex_unlock(&bus->bus_lock);
> >@@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
> >  	stream->state = SDW_STREAM_CONFIGURED;
> ... it's not symmetrical for the add_master case. The stream state changes
> on the first added master. In addition the stream state changes both when a
> slave is added and a master is added.
> Is this intentional or not - and are there side effects resulting from this
> inconsistency?
>

For remove_master, we already know the number of Masters in the stream and
hence we change the state to RELEASED only when there are no Masters
left in stream.

But, in the add_master case, we have no idea on how many master instances
are part of the stream and hence how many times add_master would be called.
So, we change the stream state to CONFIGURED when the first Master is added.
I can add a comment if that helps :)

Yes, it is added intentionally (or rather because we could not think of any
other suitable way) and we dont see any side effects. Anything
that you could think of?

IIRC, we had this discussion when the stream series was posted. But I am
unable to find those mails :(

Thanks for the review!

--Shreyas
-- 

Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Pierre-Louis Bossart]

On 7/2/18 8:13 PM, Shreyas NC wrote:
> On Mon, Jul 02, 2018 at 03:22:07PM -0500, Pierre-Louis Bossart wrote:
>>
>>>   /**
>>> @@ -918,13 +951,22 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)
>>>   int sdw_stream_remove_master(struct sdw_bus *bus,
>>>   		struct sdw_stream_runtime *stream)
>>>   {
>>> +	struct sdw_master_runtime *m_rt, *_m_rt;
>>> +
>>>   	mutex_lock(&bus->bus_lock);
>>> -	sdw_release_master_stream(stream);
>>> -	sdw_master_port_release(bus, stream->m_rt);
>>> -	stream->state = SDW_STREAM_RELEASED;
>>> -	kfree(stream->m_rt);
>>> -	stream->m_rt = NULL;
>>> +	list_for_each_entry_safe(m_rt, _m_rt,
>>> +			&stream->master_list, stream_node) {
>>> +
>>> +		if (m_rt->bus != bus)
>>> +			continue;
>>> +
>>> +		sdw_master_port_release(bus, m_rt);
>>> +		sdw_release_master_stream(m_rt, stream);
>>> +	}
>>> +
>>> +	if (list_empty(&stream->master_list))
>>> +		stream->state = SDW_STREAM_RELEASED;
>> When a master is removed, there is an explicit test to make sure the stream
>> state changes when there are no masters left in the list, but...
>>
>>>   	mutex_unlock(&bus->bus_lock);
>>> @@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus *bus,
>>>   	stream->state = SDW_STREAM_CONFIGURED;
>> ... it's not symmetrical for the add_master case. The stream state changes
>> on the first added master. In addition the stream state changes both when a
>> slave is added and a master is added.
>> Is this intentional or not - and are there side effects resulting from this
>> inconsistency?
>>
> 
> For remove_master, we already know the number of Masters in the stream and
> hence we change the state to RELEASED only when there are no Masters
> left in stream.
> 
> But, in the add_master case, we have no idea on how many master instances
> are part of the stream and hence how many times add_master would be called.
> So, we change the stream state to CONFIGURED when the first Master is added.
> I can add a comment if that helps :)

I get the point, but you currently change the state for the first slave 
that's added, so there is an inconsistency here (even before we add the 
multi-master support).
If I wanted to split hair I'd also note it's almost like the state is 
CONFIGURING rather than CONFIGURED if you don't really control when all 
configurations are complete at the bus level and depend on external 
transitions (e.g. DAPM/ALSA initiated) to go in PREPARED mode.

> 
> Yes, it is added intentionally (or rather because we could not think of any
> other suitable way) and we dont see any side effects. Anything
> that you could think of?

We should check what happens if the stream is removed before being 
enabled, or all cases of testing stream->state.

> 
> IIRC, we had this discussion when the stream series was posted. But I am
> unable to find those mails :(

I've seen SoundWire patches for the last 2 years and don't have the 
memory of all decisions and directions either...

> 
> Thanks for the review!
> 
> --Shreyas
> 

Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Nc, Shreyas]

> >>> +	struct sdw_master_runtime *m_rt, *_m_rt;
> >>> +
> >>>   	mutex_lock(&bus->bus_lock);
> >>> -	sdw_release_master_stream(stream);
> >>> -	sdw_master_port_release(bus, stream->m_rt);
> >>> -	stream->state = SDW_STREAM_RELEASED;
> >>> -	kfree(stream->m_rt);
> >>> -	stream->m_rt = NULL;
> >>> +	list_for_each_entry_safe(m_rt, _m_rt,
> >>> +			&stream->master_list, stream_node) {
> >>> +
> >>> +		if (m_rt->bus != bus)
> >>> +			continue;
> >>> +
> >>> +		sdw_master_port_release(bus, m_rt);
> >>> +		sdw_release_master_stream(m_rt, stream);
> >>> +	}
> >>> +
> >>> +	if (list_empty(&stream->master_list))
> >>> +		stream->state = SDW_STREAM_RELEASED;
> >> When a master is removed, there is an explicit test to make sure the
> >> stream state changes when there are no masters left in the list, but...
> >>
> >>>   	mutex_unlock(&bus->bus_lock);
> >>> @@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus
> *bus,
> >>>   	stream->state = SDW_STREAM_CONFIGURED;
> >> ... it's not symmetrical for the add_master case. The stream state
> >> changes on the first added master. In addition the stream state
> >> changes both when a slave is added and a master is added.
> >> Is this intentional or not - and are there side effects resulting
> >> from this inconsistency?
> >>
> >
> > For remove_master, we already know the number of Masters in the stream
> > and hence we change the state to RELEASED only when there are no
> > Masters left in stream.
> >
> > But, in the add_master case, we have no idea on how many master
> > instances are part of the stream and hence how many times add_master
> would be called.
> > So, we change the stream state to CONFIGURED when the first Master is
> added.
> > I can add a comment if that helps :)
> 
> I get the point, but you currently change the state for the first slave that's
> added, so there is an inconsistency here (even before we add the multi-master
> support).

When we add a slave, we check if there is an existing m_rt for that bus instance
and if It does not exist we create a m_rt for that master instance, add the slave
runtime and change the state to CONFIGURED. So technically we still change
the state after adding the first m_rt.

> If I wanted to split hair I'd also note it's almost like the state is CONFIGURING
> rather than CONFIGURED if you don't really control when all configurations are
> complete at the bus level and depend on external transitions (e.g. DAPM/ALSA
> initiated) to go in PREPARED mode.
> 
> >
> > Yes, it is added intentionally (or rather because we could not think
> > of any other suitable way) and we dont see any side effects. Anything
> > that you could think of?
> 
> We should check what happens if the stream is removed before being enabled,
> or all cases of testing stream->state.

While releasing the stream we unconditionally clean up things without checking
stream state. So, we should be fine that way. But, I can check these :)

--Shreyas

Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Pierre-Louis Bossart]

On 07/03/2018 11:03 AM, Nc, Shreyas wrote:
>>>>> +	struct sdw_master_runtime *m_rt, *_m_rt;
>>>>> +
>>>>>    	mutex_lock(&bus->bus_lock);
>>>>> -	sdw_release_master_stream(stream);
>>>>> -	sdw_master_port_release(bus, stream->m_rt);
>>>>> -	stream->state = SDW_STREAM_RELEASED;
>>>>> -	kfree(stream->m_rt);
>>>>> -	stream->m_rt = NULL;
>>>>> +	list_for_each_entry_safe(m_rt, _m_rt,
>>>>> +			&stream->master_list, stream_node) {
>>>>> +
>>>>> +		if (m_rt->bus != bus)
>>>>> +			continue;
>>>>> +
>>>>> +		sdw_master_port_release(bus, m_rt);
>>>>> +		sdw_release_master_stream(m_rt, stream);
>>>>> +	}
>>>>> +
>>>>> +	if (list_empty(&stream->master_list))
>>>>> +		stream->state = SDW_STREAM_RELEASED;
>>>> When a master is removed, there is an explicit test to make sure the
>>>> stream state changes when there are no masters left in the list, but...
>>>>
>>>>>    	mutex_unlock(&bus->bus_lock);
>>>>> @@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus
>> *bus,
>>>>>    	stream->state = SDW_STREAM_CONFIGURED;
>>>> ... it's not symmetrical for the add_master case. The stream state
>>>> changes on the first added master. In addition the stream state
>>>> changes both when a slave is added and a master is added.
>>>> Is this intentional or not - and are there side effects resulting
>>>> from this inconsistency?
>>>>
>>> For remove_master, we already know the number of Masters in the stream
>>> and hence we change the state to RELEASED only when there are no
>>> Masters left in stream.
>>>
>>> But, in the add_master case, we have no idea on how many master
>>> instances are part of the stream and hence how many times add_master
>> would be called.
>>> So, we change the stream state to CONFIGURED when the first Master is
>> added.
>>> I can add a comment if that helps :)
>> I get the point, but you currently change the state for the first slave that's
>> added, so there is an inconsistency here (even before we add the multi-master
>> support).
> When we add a slave, we check if there is an existing m_rt for that bus instance
> and if It does not exist we create a m_rt for that master instance, add the slave
> runtime and change the state to CONFIGURED. So technically we still change
> the state after adding the first m_rt.

If you made no assumption on the order in which slaves and masters are 
added then you need this assignment to CONFIGURED twice. But if you know 
the slaves are added first then the second assignment is irrelevant.
It's hard to memorize really since there are parts where you assume 
things are triggered by external events and others where there is no 
assumption on API use...

>
>> If I wanted to split hair I'd also note it's almost like the state is CONFIGURING
>> rather than CONFIGURED if you don't really control when all configurations are
>> complete at the bus level and depend on external transitions (e.g. DAPM/ALSA
>> initiated) to go in PREPARED mode.
>>
>>> Yes, it is added intentionally (or rather because we could not think
>>> of any other suitable way) and we dont see any side effects. Anything
>>> that you could think of?
>> We should check what happens if the stream is removed before being enabled,
>> or all cases of testing stream->state.
> While releasing the stream we unconditionally clean up things without checking
> stream state. So, we should be fine that way. But, I can check these :)
A quick check for stream->state gives me this:

     stream->state = SDW_STREAM_CONFIGURED;

 >>> so here it's a success case but we fallback to an error case...

stream_error:
     sdw_release_master_stream(m_rt, stream);
error:
     mutex_unlock(&bus->bus_lock);
     return ret;

You are probably missing a

     stream->state = SDW_STREAM_CONFIGURED;
     goto error; //<<< Add this

as done for the slave?

Wondering how this ever worked, you may want to triple-check the regular 
stream management before adding the multi-master stuff?

>
> --Shreyas
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

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

Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Nc, Shreyas]

> On 07/03/2018 11:03 AM, Nc, Shreyas wrote:
> >>>>> +	struct sdw_master_runtime *m_rt, *_m_rt;
> >>>>> +
> >>>>>    	mutex_lock(&bus->bus_lock);
> >>>>> -	sdw_release_master_stream(stream);
> >>>>> -	sdw_master_port_release(bus, stream->m_rt);
> >>>>> -	stream->state = SDW_STREAM_RELEASED;
> >>>>> -	kfree(stream->m_rt);
> >>>>> -	stream->m_rt = NULL;
> >>>>> +	list_for_each_entry_safe(m_rt, _m_rt,
> >>>>> +			&stream->master_list, stream_node) {
> >>>>> +
> >>>>> +		if (m_rt->bus != bus)
> >>>>> +			continue;
> >>>>> +
> >>>>> +		sdw_master_port_release(bus, m_rt);
> >>>>> +		sdw_release_master_stream(m_rt, stream);
> >>>>> +	}
> >>>>> +
> >>>>> +	if (list_empty(&stream->master_list))
> >>>>> +		stream->state = SDW_STREAM_RELEASED;
> >>>> When a master is removed, there is an explicit test to make sure
> >>>> the stream state changes when there are no masters left in the list, but...
> >>>>
> >>>>>    	mutex_unlock(&bus->bus_lock);
> >>>>> @@ -1127,7 +1169,7 @@ int sdw_stream_add_master(struct sdw_bus
> >> *bus,
> >>>>>    	stream->state = SDW_STREAM_CONFIGURED;
> >>>> ... it's not symmetrical for the add_master case. The stream state
> >>>> changes on the first added master. In addition the stream state
> >>>> changes both when a slave is added and a master is added.
> >>>> Is this intentional or not - and are there side effects resulting
> >>>> from this inconsistency?
> >>>>
> >>> For remove_master, we already know the number of Masters in the
> >>> stream and hence we change the state to RELEASED only when there are
> >>> no Masters left in stream.
> >>>
> >>> But, in the add_master case, we have no idea on how many master
> >>> instances are part of the stream and hence how many times add_master
> >> would be called.
> >>> So, we change the stream state to CONFIGURED when the first Master
> >>> is
> >> added.
> >>> I can add a comment if that helps :)
> >> I get the point, but you currently change the state for the first
> >> slave that's added, so there is an inconsistency here (even before we
> >> add the multi-master support).
> > When we add a slave, we check if there is an existing m_rt for that
> > bus instance and if It does not exist we create a m_rt for that master
> > instance, add the slave runtime and change the state to CONFIGURED. So
> > technically we still change the state after adding the first m_rt.
> 
> If you made no assumption on the order in which slaves and masters are added
> then you need this assignment to CONFIGURED twice. But if you know the slaves
> are added first then the second assignment is irrelevant.
> It's hard to memorize really since there are parts where you assume things are
> triggered by external events and others where there is no assumption on API
> use...

Ok, will fix this. 
Vinod, would you like to have a patch for fixing this in the same series or a
patch later on ?

> 
> >
> >> If I wanted to split hair I'd also note it's almost like the state is
> >> CONFIGURING rather than CONFIGURED if you don't really control when
> >> all configurations are complete at the bus level and depend on
> >> external transitions (e.g. DAPM/ALSA
> >> initiated) to go in PREPARED mode.
> >>
> >>> Yes, it is added intentionally (or rather because we could not think
> >>> of any other suitable way) and we dont see any side effects.
> >>> Anything that you could think of?
> >> We should check what happens if the stream is removed before being
> >> enabled, or all cases of testing stream->state.
> > While releasing the stream we unconditionally clean up things without
> > checking stream state. So, we should be fine that way. But, I can
> > check these :)
> A quick check for stream->state gives me this:
> 
>      stream->state = SDW_STREAM_CONFIGURED;
> 
>  >>> so here it's a success case but we fallback to an error case...
> 
> stream_error:
>      sdw_release_master_stream(m_rt, stream);
> error:
>      mutex_unlock(&bus->bus_lock);
>      return ret;
> 
> You are probably missing a
> 
>      stream->state = SDW_STREAM_CONFIGURED;
>      goto error; //<<< Add this
> 
> as done for the slave?
> 
> Wondering how this ever worked, you may want to triple-check the regular
> stream management before adding the multi-master stuff?

Yes, you are right. It works because of the change being a part of another change
that is yet to be up streamed :(

Wondering how this slipped, this should have been part of the stream series itself.
Will fix this up :)

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

Re: [PATCH v4 4/7] soundwire: Handle multiple master instances in a stream

[Vinod]

On 04-07-18, 00:17, Nc, Shreyas wrote:
> > On 07/03/2018 11:03 AM, Nc, Shreyas wrote:

> Ok, will fix this. 
> Vinod, would you like to have a patch for fixing this in the same series or a
> patch later on ?

It is a fix, so early on please :)
-- 
~Vinod