Re: [alsa-devel] [PATCH v3 03/13] soundwire: Add support for port management

On 04/16/2018 10:23 AM, Vinod Koul wrote:

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

Add Soundwire port data structures and APIS for initialization and release of ports.

Signed-off-by: Sanyog Kale sanyog.r.kale@intel.com Signed-off-by: Shreyas NC shreyas.nc@intel.com Signed-off-by: Vinod Koul vinod.koul@intel.com

---

drivers/soundwire/bus.h | 25 +++++++ drivers/soundwire/stream.c | 149 +++++++++++++++++++++++++++++++++++++++++- include/linux/soundwire/sdw.h | 67 +++++++++++++++++++ 3 files changed, 239 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h index 2e5043de9a4b..39e6811e435c 100644 --- a/drivers/soundwire/bus.h +++ b/drivers/soundwire/bus.h @@ -46,6 +46,27 @@ struct sdw_msg { };

/**

• • sdw_port_runtime: Runtime port parameters for Master or Slave
• • @num: Port number. For audio streams, valid port number ranges from
• • [1,14]
• • @ch_mask: Channel mask
• • @transport_params: Transport parameters
• • @port_params: Port parameters
• • @port_node: List node for Master or Slave port_list
• • SoundWire spec has no mention of ports for Master interface but the
• • concept is logically extended.
• */

+struct sdw_port_runtime {

• int num;
• int ch_mask;
• struct sdw_transport_params transport_params;
• struct sdw_port_params port_params;
• struct list_head port_node;

+};

+/**

• sdw_slave_runtime: Runtime Stream parameters for Slave
• @slave: Slave handle

@@ -53,12 +74,14 @@ struct sdw_msg {

• @ch_count: Number of channels handled by the Slave for
• this stream
• @m_rt_node: sdw_master_runtime list node

• • @port_list: List of Slave Ports configured for this stream

  */ struct sdw_slave_runtime { struct sdw_slave *slave; enum sdw_data_direction direction; unsigned int ch_count; struct list_head m_rt_node;

• struct list_head port_list; };

  /**

@@ -70,6 +93,7 @@ struct sdw_slave_runtime {

• @ch_count: Number of channels handled by the Master for
• this stream, can be zero.
• @slave_rt_list: Slave runtime list

• • @port_list: List of Master Ports configured for this stream, can be zero.
  • @bus_node: sdw_bus m_rt_list node
  */ struct sdw_master_runtime {

@@ -78,6 +102,7 @@ struct sdw_master_runtime { enum sdw_data_direction direction; unsigned int ch_count; struct list_head slave_rt_list;

• struct list_head port_list; struct list_head bus_node; };

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 7ec1a3e4b2d6..7e2b3512d8e2 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -81,6 +81,7 @@ static struct sdw_master_runtime return NULL;

/* 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;

@@ -115,6 +116,7 @@ static struct sdw_slave_runtime if (!s_rt) return NULL;

• INIT_LIST_HEAD(&s_rt->port_list);

  s_rt->ch_count = stream_config->ch_count; s_rt->direction = stream_config->direction;

@@ -123,6 +125,41 @@ static struct sdw_slave_runtime return s_rt; }

+static void sdw_master_port_release(struct sdw_bus *bus,

• 	struct sdw_master_runtime *m_rt)
  

+{

• struct sdw_port_runtime *p_rt, *_p_rt;
• list_for_each_entry_safe(p_rt, _p_rt,

• 	&m_rt->port_list, port_node) {
  

• list_del(&p_rt->port_node);
  

• kfree(p_rt);
  

• }

+}

+static void sdw_slave_port_release(struct sdw_bus *bus,

• struct sdw_slave *slave,
  

• struct sdw_stream_runtime *stream)
  

+{

• struct sdw_port_runtime *p_rt, *_p_rt;
• struct sdw_master_runtime *m_rt = stream->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_safe(p_rt, _p_rt,
  

• 		&s_rt->port_list, port_node) {
  

• 	list_del(&p_rt->port_node);
  

• 	kfree(p_rt);
  

• }
  

• }

+}

• /**
  • sdw_release_slave_stream: Free Slave(s) runtime handle

@@ -167,7 +204,7 @@ static void sdw_release_master_stream(struct sdw_stream_runtime *stream)

• @bus: SDW Bus instance
• @stream: SoundWire stream

• • This removes and frees master_rt from a stream

• • This removes and frees port_rt and master_rt from a stream
  */ int sdw_stream_remove_master(struct sdw_bus *bus, struct sdw_stream_runtime *stream)

@@ -175,6 +212,7 @@ int sdw_stream_remove_master(struct sdw_bus *bus, 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;

@@ -191,13 +229,14 @@ EXPORT_SYMBOL(sdw_stream_remove_master);

• @slave: SDW Slave instance
• @stream: SoundWire stream

• • This removes and frees slave_rt from a stream

• • This removes and frees port_rt and slave_rt from a stream

  */ int sdw_stream_remove_slave(struct sdw_slave *slave, struct sdw_stream_runtime *stream) { mutex_lock(&slave->bus->bus_lock);

• sdw_slave_port_release(slave->bus, slave, stream);

Humm, does this work if the sdw_stream_remove_master() is called first? It'll call sdw_release_slave_stream() so you have lost the s_rt pointer by the time you want to call sdw_slave_port_release() so will never free the ports?

You will also have lost the stream->m_rt at that point. I believe the slave ports should be freed from sdw_release_slave_stream(). This call to sdw_slave_port_release comes too late if the master takes the initiative first to clean house.