[PATCH 0/2] soundwire: fix port_ready[] dynamic allocation in

[PATCH 0/2] soundwire: fix port_ready[] dynamic allocation in

[Bard Liao]

The existing code allocates memory for the total number of ports.
This only works if the ports are contiguous, but will break if e.g. a
Devices uses port0, 1, and 14. The port_ready[] array would contain 3
elements, which would lead to an out-of-bounds access. Conversely in
other cases, the wrong port index would be used leading to timeouts on
prepare.

This can be fixed by allocating for the worst-case of 15
ports (DP0..DP14). In addition since the number is now fixed, we can
use an array instead of a dynamic allocation.

Pierre-Louis Bossart (2):
  soundwire: add definition for maximum number of ports
  soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC
    codecs

 drivers/soundwire/mipi_disco.c  | 18 +-----------------
 drivers/soundwire/slave.c       |  4 ++++
 include/linux/soundwire/sdw.h   |  5 +++--
 sound/soc/codecs/max98373-sdw.c | 15 +--------------
 sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
 sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
 sound/soc/codecs/rt700-sdw.c    | 15 +--------------
 sound/soc/codecs/rt711-sdw.c    | 15 +--------------
 sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
 9 files changed, 14 insertions(+), 120 deletions(-)

-- 
2.17.1

[PATCH 0/2] soundwire: fix port_ready[] dynamic allocation in

[Bard Liao]

The existing code allocates memory for the total number of ports.
This only works if the ports are contiguous, but will break if e.g. a
Devices uses port0, 1, and 14. The port_ready[] array would contain 3
elements, which would lead to an out-of-bounds access. Conversely in
other cases, the wrong port index would be used leading to timeouts on
prepare.

This can be fixed by allocating for the worst-case of 15
ports (DP0..DP14). In addition since the number is now fixed, we can
use an array instead of a dynamic allocation.

Pierre-Louis Bossart (2):
  soundwire: add definition for maximum number of ports
  soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC
    codecs

 drivers/soundwire/mipi_disco.c  | 18 +-----------------
 drivers/soundwire/slave.c       |  4 ++++
 include/linux/soundwire/sdw.h   |  5 +++--
 sound/soc/codecs/max98373-sdw.c | 15 +--------------
 sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
 sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
 sound/soc/codecs/rt700-sdw.c    | 15 +--------------
 sound/soc/codecs/rt711-sdw.c    | 15 +--------------
 sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
 9 files changed, 14 insertions(+), 120 deletions(-)

-- 
2.17.1

[PATCH 1/2] soundwire: add definition for maximum number of ports

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

A Device may have at most 15 physical ports (DP0, DP1..DP14).

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 include/linux/soundwire/sdw.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 76052f12c9f7..0aa4c6af7554 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -38,7 +38,8 @@ struct sdw_slave;
 #define SDW_FRAME_CTRL_BITS		48
 #define SDW_MAX_DEVICES			11
 
-#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
+#define SDW_MAX_PORTS			15
+#define SDW_VALID_PORT_RANGE(n)		((n) < SDW_MAX_PORTS && (n) >= 1)
 
 enum {
 	SDW_PORT_DIRN_SINK = 0,
-- 
2.17.1

[PATCH 1/2] soundwire: add definition for maximum number of ports

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

A Device may have at most 15 physical ports (DP0, DP1..DP14).

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 include/linux/soundwire/sdw.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 76052f12c9f7..0aa4c6af7554 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -38,7 +38,8 @@ struct sdw_slave;
 #define SDW_FRAME_CTRL_BITS		48
 #define SDW_MAX_DEVICES			11
 
-#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
+#define SDW_MAX_PORTS			15
+#define SDW_VALID_PORT_RANGE(n)		((n) < SDW_MAX_PORTS && (n) >= 1)
 
 enum {
 	SDW_PORT_DIRN_SINK = 0,
-- 
2.17.1

Re: [PATCH 1/2] soundwire: add definition for maximum number of ports

[Vinod Koul]

On 18-08-20, 01:47, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> A Device may have at most 15 physical ports (DP0, DP1..DP14).
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  include/linux/soundwire/sdw.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 76052f12c9f7..0aa4c6af7554 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -38,7 +38,8 @@ struct sdw_slave;
>  #define SDW_FRAME_CTRL_BITS		48
>  #define SDW_MAX_DEVICES			11
>  
> -#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
> +#define SDW_MAX_PORTS			15
> +#define SDW_VALID_PORT_RANGE(n)		((n) < SDW_MAX_PORTS && (n) >= 1)

What is the use of this one if we are allocating all ports always, Also,
I dont see it used in second patch?

>  
>  enum {
>  	SDW_PORT_DIRN_SINK = 0,
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 1/2] soundwire: add definition for maximum number of ports

[Vinod Koul]

On 18-08-20, 01:47, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> A Device may have at most 15 physical ports (DP0, DP1..DP14).
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  include/linux/soundwire/sdw.h | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 76052f12c9f7..0aa4c6af7554 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -38,7 +38,8 @@ struct sdw_slave;
>  #define SDW_FRAME_CTRL_BITS		48
>  #define SDW_MAX_DEVICES			11
>  
> -#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
> +#define SDW_MAX_PORTS			15
> +#define SDW_VALID_PORT_RANGE(n)		((n) < SDW_MAX_PORTS && (n) >= 1)

What is the use of this one if we are allocating all ports always, Also,
I dont see it used in second patch?

>  
>  enum {
>  	SDW_PORT_DIRN_SINK = 0,
> -- 
> 2.17.1

-- 
~Vinod

RE: [PATCH 1/2] soundwire: add definition for maximum number of ports

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Tuesday, August 18, 2020 2:36 PM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>; Lin,
> Mengdong <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH 1/2] soundwire: add definition for maximum number of
> ports
> 
> On 18-08-20, 01:47, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >
> > A Device may have at most 15 physical ports (DP0, DP1..DP14).
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com>
> > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > Reviewed-by: Guennadi Liakhovetski
> > <guennadi.liakhovetski@linux.intel.com>
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > ---
> >  include/linux/soundwire/sdw.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/soundwire/sdw.h
> > b/include/linux/soundwire/sdw.h index 76052f12c9f7..0aa4c6af7554
> > 100644
> > --- a/include/linux/soundwire/sdw.h
> > +++ b/include/linux/soundwire/sdw.h
> > @@ -38,7 +38,8 @@ struct sdw_slave;
> >  #define SDW_FRAME_CTRL_BITS		48
> >  #define SDW_MAX_DEVICES			11
> >
> > -#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
> > +#define SDW_MAX_PORTS			15
> > +#define SDW_VALID_PORT_RANGE(n)		((n) <
> SDW_MAX_PORTS && (n) >= 1)
> 
> What is the use of this one if we are allocating all ports always, Also, I dont
> see it used in second patch?

It is used in drivers/soundwire/stream.c and drivers/soundwire/debugfs.c.

> 
> >
> >  enum {
> >  	SDW_PORT_DIRN_SINK = 0,
> > --
> > 2.17.1
> 
> --
> ~Vinod

RE: [PATCH 1/2] soundwire: add definition for maximum number of ports

[Liao, Bard]

> -----Original Message-----
> From: Vinod Koul <vkoul@kernel.org>
> Sent: Tuesday, August 18, 2020 2:36 PM
> To: Bard Liao <yung-chuan.liao@linux.intel.com>
> Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>; Lin,
> Mengdong <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> Subject: Re: [PATCH 1/2] soundwire: add definition for maximum number of
> ports
> 
> On 18-08-20, 01:47, Bard Liao wrote:
> > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> >
> > A Device may have at most 15 physical ports (DP0, DP1..DP14).
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <pierre-louis.bossart@linux.intel.com>
> > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > Reviewed-by: Guennadi Liakhovetski
> > <guennadi.liakhovetski@linux.intel.com>
> > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > ---
> >  include/linux/soundwire/sdw.h | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/soundwire/sdw.h
> > b/include/linux/soundwire/sdw.h index 76052f12c9f7..0aa4c6af7554
> > 100644
> > --- a/include/linux/soundwire/sdw.h
> > +++ b/include/linux/soundwire/sdw.h
> > @@ -38,7 +38,8 @@ struct sdw_slave;
> >  #define SDW_FRAME_CTRL_BITS		48
> >  #define SDW_MAX_DEVICES			11
> >
> > -#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
> > +#define SDW_MAX_PORTS			15
> > +#define SDW_VALID_PORT_RANGE(n)		((n) <
> SDW_MAX_PORTS && (n) >= 1)
> 
> What is the use of this one if we are allocating all ports always, Also, I dont
> see it used in second patch?

It is used in drivers/soundwire/stream.c and drivers/soundwire/debugfs.c.

> 
> >
> >  enum {
> >  	SDW_PORT_DIRN_SINK = 0,
> > --
> > 2.17.1
> 
> --
> ~Vinod

Re: [PATCH 1/2] soundwire: add definition for maximum number of ports

[Vinod Koul]

On 18-08-20, 06:53, Liao, Bard wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Tuesday, August 18, 2020 2:36 PM
> > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> > broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>; Lin,
> > Mengdong <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > Subject: Re: [PATCH 1/2] soundwire: add definition for maximum number of
> > ports
> > 
> > On 18-08-20, 01:47, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > >
> > > A Device may have at most 15 physical ports (DP0, DP1..DP14).
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > > Reviewed-by: Guennadi Liakhovetski
> > > <guennadi.liakhovetski@linux.intel.com>
> > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > ---
> > >  include/linux/soundwire/sdw.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/soundwire/sdw.h
> > > b/include/linux/soundwire/sdw.h index 76052f12c9f7..0aa4c6af7554
> > > 100644
> > > --- a/include/linux/soundwire/sdw.h
> > > +++ b/include/linux/soundwire/sdw.h
> > > @@ -38,7 +38,8 @@ struct sdw_slave;
> > >  #define SDW_FRAME_CTRL_BITS		48
> > >  #define SDW_MAX_DEVICES			11
> > >
> > > -#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
> > > +#define SDW_MAX_PORTS			15
> > > +#define SDW_VALID_PORT_RANGE(n)		((n) <
> > SDW_MAX_PORTS && (n) >= 1)
> > 
> > What is the use of this one if we are allocating all ports always, Also, I dont
> > see it used in second patch?
> 
> It is used in drivers/soundwire/stream.c and drivers/soundwire/debugfs.c.

Ah overlooked that it is modified, pls ignore this comment

-- 
~Vinod

Re: [PATCH 1/2] soundwire: add definition for maximum number of ports

[Vinod Koul]

On 18-08-20, 06:53, Liao, Bard wrote:
> > -----Original Message-----
> > From: Vinod Koul <vkoul@kernel.org>
> > Sent: Tuesday, August 18, 2020 2:36 PM
> > To: Bard Liao <yung-chuan.liao@linux.intel.com>
> > Cc: alsa-devel@alsa-project.org; linux-kernel@vger.kernel.org; tiwai@suse.de;
> > broonie@kernel.org; gregkh@linuxfoundation.org; jank@cadence.com;
> > srinivas.kandagatla@linaro.org; rander.wang@linux.intel.com;
> > ranjani.sridharan@linux.intel.com; hui.wang@canonical.com; pierre-
> > louis.bossart@linux.intel.com; Kale, Sanyog R <sanyog.r.kale@intel.com>; Lin,
> > Mengdong <mengdong.lin@intel.com>; Liao, Bard <bard.liao@intel.com>
> > Subject: Re: [PATCH 1/2] soundwire: add definition for maximum number of
> > ports
> > 
> > On 18-08-20, 01:47, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > >
> > > A Device may have at most 15 physical ports (DP0, DP1..DP14).
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > <pierre-louis.bossart@linux.intel.com>
> > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > > Reviewed-by: Guennadi Liakhovetski
> > > <guennadi.liakhovetski@linux.intel.com>
> > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > ---
> > >  include/linux/soundwire/sdw.h | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/linux/soundwire/sdw.h
> > > b/include/linux/soundwire/sdw.h index 76052f12c9f7..0aa4c6af7554
> > > 100644
> > > --- a/include/linux/soundwire/sdw.h
> > > +++ b/include/linux/soundwire/sdw.h
> > > @@ -38,7 +38,8 @@ struct sdw_slave;
> > >  #define SDW_FRAME_CTRL_BITS		48
> > >  #define SDW_MAX_DEVICES			11
> > >
> > > -#define SDW_VALID_PORT_RANGE(n)		((n) <= 14 && (n) >= 1)
> > > +#define SDW_MAX_PORTS			15
> > > +#define SDW_VALID_PORT_RANGE(n)		((n) <
> > SDW_MAX_PORTS && (n) >= 1)
> > 
> > What is the use of this one if we are allocating all ports always, Also, I dont
> > see it used in second patch?
> 
> It is used in drivers/soundwire/stream.c and drivers/soundwire/debugfs.c.

Ah overlooked that it is modified, pls ignore this comment

-- 
~Vinod

[PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The existing code allocates memory for the total number of ports.
This only works if the ports are contiguous, but will break if e.g. a
Devices uses port0, 1, and 14. The port_ready[] array would contain 3
elements, which would lead to an out-of-bounds access. Conversely in
other cases, the wrong port index would be used leading to timeouts on
prepare.

This can be fixed by allocating for the worst-case of 15
ports (DP0..DP14). In addition since the number is now fixed, we can
use an array instead of a dynamic allocation.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/mipi_disco.c  | 18 +-----------------
 drivers/soundwire/slave.c       |  4 ++++
 include/linux/soundwire/sdw.h   |  2 +-
 sound/soc/codecs/max98373-sdw.c | 15 +--------------
 sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
 sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
 sound/soc/codecs/rt700-sdw.c    | 15 +--------------
 sound/soc/codecs/rt711-sdw.c    | 15 +--------------
 sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
 9 files changed, 12 insertions(+), 119 deletions(-)

diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index 4ae62b452b8c..55a9c51c84c1 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -289,7 +289,7 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 	struct sdw_slave_prop *prop = &slave->prop;
 	struct device *dev = &slave->dev;
 	struct fwnode_handle *port;
-	int num_of_ports, nval, i, dp0 = 0;
+	int nval;
 
 	device_property_read_u32(dev, "mipi-sdw-sw-interface-revision",
 				 &prop->mipi_revision);
@@ -352,7 +352,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 			return -ENOMEM;
 
 		sdw_slave_read_dp0(slave, port, prop->dp0_prop);
-		dp0 = 1;
 	}
 
 	/*
@@ -383,21 +382,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 	sdw_slave_read_dpn(slave, prop->sink_dpn_prop, nval,
 			   prop->sink_ports, "sink");
 
-	/* some ports are bidirectional so check total ports by ORing */
-	nval = prop->source_ports | prop->sink_ports;
-	num_of_ports = hweight32(nval) + dp0; /* add DP0 */
-
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					 sizeof(*slave->port_ready),
-					 GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	return 0;
 }
 EXPORT_SYMBOL(sdw_slave_read_prop);
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 0839445ee07b..a762ee24e6fa 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -25,6 +25,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
 {
 	struct sdw_slave *slave;
 	int ret;
+	int i;
 
 	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
 	if (!slave)
@@ -58,6 +59,9 @@ static int sdw_slave_add(struct sdw_bus *bus,
 	init_completion(&slave->probe_complete);
 	slave->probed = false;
 
+	for (i = 0; i < SDW_MAX_PORTS; i++)
+		init_completion(&slave->port_ready[i]);
+
 	mutex_lock(&bus->bus_lock);
 	list_add_tail(&slave->node, &bus->slaves);
 	mutex_unlock(&bus->bus_lock);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 0aa4c6af7554..63e71645fd13 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -619,7 +619,7 @@ struct sdw_slave {
 	struct dentry *debugfs;
 #endif
 	struct list_head node;
-	struct completion *port_ready;
+	struct completion port_ready[SDW_MAX_PORTS];
 	enum sdw_clk_stop_mode curr_clk_stop_mode;
 	u16 dev_num;
 	u16 dev_num_sticky;
diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c
index 5fe724728e84..a3ec92775ea7 100644
--- a/sound/soc/codecs/max98373-sdw.c
+++ b/sound/soc/codecs/max98373-sdw.c
@@ -282,7 +282,7 @@ static const struct dev_pm_ops max98373_pm = {
 static int max98373_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -295,7 +295,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
 	prop->clk_stop_timeout = 20;
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports = nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					  sizeof(*prop->src_dpn_prop),
 					  GFP_KERNEL);
@@ -315,7 +314,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					   sizeof(*prop->sink_dpn_prop),
 					   GFP_KERNEL);
@@ -333,17 +331,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					 sizeof(*slave->port_ready),
-					 GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
index b0ba0d2acbdd..09c69dbab12a 100644
--- a/sound/soc/codecs/rt1308-sdw.c
+++ b/sound/soc/codecs/rt1308-sdw.c
@@ -118,7 +118,7 @@ static int rt1308_clock_config(struct device *dev)
 static int rt1308_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -131,7 +131,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
 
 	/* for sink */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->sink_dpn_prop),
 						GFP_KERNEL);
@@ -149,17 +148,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
index 94bf6bee78e6..b7c97aba7f17 100644
--- a/sound/soc/codecs/rt5682-sdw.c
+++ b/sound/soc/codecs/rt5682-sdw.c
@@ -537,7 +537,7 @@ static int rt5682_update_status(struct sdw_slave *slave,
 static int rt5682_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -549,7 +549,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0x2;		/* BITMAP: 00000010 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					  sizeof(*prop->src_dpn_prop),
 					  GFP_KERNEL);
@@ -569,7 +568,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					   sizeof(*prop->sink_dpn_prop),
 					   GFP_KERNEL);
@@ -587,17 +585,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					 sizeof(*slave->port_ready),
-					 GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
index 4d14048d1197..b19fbcc12c69 100644
--- a/sound/soc/codecs/rt700-sdw.c
+++ b/sound/soc/codecs/rt700-sdw.c
@@ -333,7 +333,7 @@ static int rt700_update_status(struct sdw_slave *slave,
 static int rt700_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -345,7 +345,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0xA; /* BITMAP:  00001010 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->src_dpn_prop),
 						GFP_KERNEL);
@@ -365,7 +364,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->sink_dpn_prop),
 						GFP_KERNEL);
@@ -383,17 +381,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt711-sdw.c b/sound/soc/codecs/rt711-sdw.c
index 45b928954b58..dc4a2b482462 100644
--- a/sound/soc/codecs/rt711-sdw.c
+++ b/sound/soc/codecs/rt711-sdw.c
@@ -337,7 +337,7 @@ static int rt711_update_status(struct sdw_slave *slave,
 static int rt711_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -349,7 +349,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0x8; /* BITMAP:  00001000 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->src_dpn_prop),
 						GFP_KERNEL);
@@ -369,7 +368,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->sink_dpn_prop),
 						GFP_KERNEL);
@@ -387,17 +385,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt715-sdw.c b/sound/soc/codecs/rt715-sdw.c
index d11b23d6b240..d8ed07305ffc 100644
--- a/sound/soc/codecs/rt715-sdw.c
+++ b/sound/soc/codecs/rt715-sdw.c
@@ -431,7 +431,7 @@ static int rt715_update_status(struct sdw_slave *slave,
 static int rt715_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -443,7 +443,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0x0;	/* BITMAP:  00000000 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					sizeof(*prop->src_dpn_prop),
 					GFP_KERNEL);
@@ -460,36 +459,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* do this again for sink now */
-	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
-	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
-					sizeof(*prop->sink_dpn_prop),
-					GFP_KERNEL);
-	if (!prop->sink_dpn_prop)
-		return -ENOMEM;
-
-	dpn = prop->sink_dpn_prop;
-	i = 0;
-	addr = prop->sink_ports;
-	for_each_set_bit(bit, &addr, 32) {
-		dpn[i].num = bit;
-		dpn[i].simple_ch_prep_sm = true;
-		dpn[i].ch_prep_timeout = 10;
-		i++;
-	}
-
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
-- 
2.17.1

[PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Bard Liao]

From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The existing code allocates memory for the total number of ports.
This only works if the ports are contiguous, but will break if e.g. a
Devices uses port0, 1, and 14. The port_ready[] array would contain 3
elements, which would lead to an out-of-bounds access. Conversely in
other cases, the wrong port index would be used leading to timeouts on
prepare.

This can be fixed by allocating for the worst-case of 15
ports (DP0..DP14). In addition since the number is now fixed, we can
use an array instead of a dynamic allocation.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
---
 drivers/soundwire/mipi_disco.c  | 18 +-----------------
 drivers/soundwire/slave.c       |  4 ++++
 include/linux/soundwire/sdw.h   |  2 +-
 sound/soc/codecs/max98373-sdw.c | 15 +--------------
 sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
 sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
 sound/soc/codecs/rt700-sdw.c    | 15 +--------------
 sound/soc/codecs/rt711-sdw.c    | 15 +--------------
 sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
 9 files changed, 12 insertions(+), 119 deletions(-)

diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
index 4ae62b452b8c..55a9c51c84c1 100644
--- a/drivers/soundwire/mipi_disco.c
+++ b/drivers/soundwire/mipi_disco.c
@@ -289,7 +289,7 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 	struct sdw_slave_prop *prop = &slave->prop;
 	struct device *dev = &slave->dev;
 	struct fwnode_handle *port;
-	int num_of_ports, nval, i, dp0 = 0;
+	int nval;
 
 	device_property_read_u32(dev, "mipi-sdw-sw-interface-revision",
 				 &prop->mipi_revision);
@@ -352,7 +352,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 			return -ENOMEM;
 
 		sdw_slave_read_dp0(slave, port, prop->dp0_prop);
-		dp0 = 1;
 	}
 
 	/*
@@ -383,21 +382,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
 	sdw_slave_read_dpn(slave, prop->sink_dpn_prop, nval,
 			   prop->sink_ports, "sink");
 
-	/* some ports are bidirectional so check total ports by ORing */
-	nval = prop->source_ports | prop->sink_ports;
-	num_of_ports = hweight32(nval) + dp0; /* add DP0 */
-
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					 sizeof(*slave->port_ready),
-					 GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	return 0;
 }
 EXPORT_SYMBOL(sdw_slave_read_prop);
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 0839445ee07b..a762ee24e6fa 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -25,6 +25,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
 {
 	struct sdw_slave *slave;
 	int ret;
+	int i;
 
 	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
 	if (!slave)
@@ -58,6 +59,9 @@ static int sdw_slave_add(struct sdw_bus *bus,
 	init_completion(&slave->probe_complete);
 	slave->probed = false;
 
+	for (i = 0; i < SDW_MAX_PORTS; i++)
+		init_completion(&slave->port_ready[i]);
+
 	mutex_lock(&bus->bus_lock);
 	list_add_tail(&slave->node, &bus->slaves);
 	mutex_unlock(&bus->bus_lock);
diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
index 0aa4c6af7554..63e71645fd13 100644
--- a/include/linux/soundwire/sdw.h
+++ b/include/linux/soundwire/sdw.h
@@ -619,7 +619,7 @@ struct sdw_slave {
 	struct dentry *debugfs;
 #endif
 	struct list_head node;
-	struct completion *port_ready;
+	struct completion port_ready[SDW_MAX_PORTS];
 	enum sdw_clk_stop_mode curr_clk_stop_mode;
 	u16 dev_num;
 	u16 dev_num_sticky;
diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c
index 5fe724728e84..a3ec92775ea7 100644
--- a/sound/soc/codecs/max98373-sdw.c
+++ b/sound/soc/codecs/max98373-sdw.c
@@ -282,7 +282,7 @@ static const struct dev_pm_ops max98373_pm = {
 static int max98373_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -295,7 +295,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
 	prop->clk_stop_timeout = 20;
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports = nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					  sizeof(*prop->src_dpn_prop),
 					  GFP_KERNEL);
@@ -315,7 +314,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					   sizeof(*prop->sink_dpn_prop),
 					   GFP_KERNEL);
@@ -333,17 +331,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					 sizeof(*slave->port_ready),
-					 GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
index b0ba0d2acbdd..09c69dbab12a 100644
--- a/sound/soc/codecs/rt1308-sdw.c
+++ b/sound/soc/codecs/rt1308-sdw.c
@@ -118,7 +118,7 @@ static int rt1308_clock_config(struct device *dev)
 static int rt1308_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -131,7 +131,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
 
 	/* for sink */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->sink_dpn_prop),
 						GFP_KERNEL);
@@ -149,17 +148,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
index 94bf6bee78e6..b7c97aba7f17 100644
--- a/sound/soc/codecs/rt5682-sdw.c
+++ b/sound/soc/codecs/rt5682-sdw.c
@@ -537,7 +537,7 @@ static int rt5682_update_status(struct sdw_slave *slave,
 static int rt5682_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -549,7 +549,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0x2;		/* BITMAP: 00000010 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					  sizeof(*prop->src_dpn_prop),
 					  GFP_KERNEL);
@@ -569,7 +568,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					   sizeof(*prop->sink_dpn_prop),
 					   GFP_KERNEL);
@@ -587,17 +585,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					 sizeof(*slave->port_ready),
-					 GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
index 4d14048d1197..b19fbcc12c69 100644
--- a/sound/soc/codecs/rt700-sdw.c
+++ b/sound/soc/codecs/rt700-sdw.c
@@ -333,7 +333,7 @@ static int rt700_update_status(struct sdw_slave *slave,
 static int rt700_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -345,7 +345,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0xA; /* BITMAP:  00001010 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->src_dpn_prop),
 						GFP_KERNEL);
@@ -365,7 +364,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->sink_dpn_prop),
 						GFP_KERNEL);
@@ -383,17 +381,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt711-sdw.c b/sound/soc/codecs/rt711-sdw.c
index 45b928954b58..dc4a2b482462 100644
--- a/sound/soc/codecs/rt711-sdw.c
+++ b/sound/soc/codecs/rt711-sdw.c
@@ -337,7 +337,7 @@ static int rt711_update_status(struct sdw_slave *slave,
 static int rt711_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -349,7 +349,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0x8; /* BITMAP:  00001000 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->src_dpn_prop),
 						GFP_KERNEL);
@@ -369,7 +368,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
 
 	/* do this again for sink now */
 	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
 	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
 						sizeof(*prop->sink_dpn_prop),
 						GFP_KERNEL);
@@ -387,17 +385,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
diff --git a/sound/soc/codecs/rt715-sdw.c b/sound/soc/codecs/rt715-sdw.c
index d11b23d6b240..d8ed07305ffc 100644
--- a/sound/soc/codecs/rt715-sdw.c
+++ b/sound/soc/codecs/rt715-sdw.c
@@ -431,7 +431,7 @@ static int rt715_update_status(struct sdw_slave *slave,
 static int rt715_read_prop(struct sdw_slave *slave)
 {
 	struct sdw_slave_prop *prop = &slave->prop;
-	int nval, i, num_of_ports = 1;
+	int nval, i;
 	u32 bit;
 	unsigned long addr;
 	struct sdw_dpn_prop *dpn;
@@ -443,7 +443,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
 	prop->sink_ports = 0x0;	/* BITMAP:  00000000 */
 
 	nval = hweight32(prop->source_ports);
-	num_of_ports += nval;
 	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
 					sizeof(*prop->src_dpn_prop),
 					GFP_KERNEL);
@@ -460,36 +459,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
 		i++;
 	}
 
-	/* do this again for sink now */
-	nval = hweight32(prop->sink_ports);
-	num_of_ports += nval;
-	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
-					sizeof(*prop->sink_dpn_prop),
-					GFP_KERNEL);
-	if (!prop->sink_dpn_prop)
-		return -ENOMEM;
-
-	dpn = prop->sink_dpn_prop;
-	i = 0;
-	addr = prop->sink_ports;
-	for_each_set_bit(bit, &addr, 32) {
-		dpn[i].num = bit;
-		dpn[i].simple_ch_prep_sm = true;
-		dpn[i].ch_prep_timeout = 10;
-		i++;
-	}
-
-	/* Allocate port_ready based on num_of_ports */
-	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
-					sizeof(*slave->port_ready),
-					GFP_KERNEL);
-	if (!slave->port_ready)
-		return -ENOMEM;
-
-	/* Initialize completion */
-	for (i = 0; i < num_of_ports; i++)
-		init_completion(&slave->port_ready[i]);
-
 	/* set the timeout values */
 	prop->clk_stop_timeout = 20;
 
-- 
2.17.1

Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Vinod Koul]

On 18-08-20, 01:47, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The existing code allocates memory for the total number of ports.
> This only works if the ports are contiguous, but will break if e.g. a
> Devices uses port0, 1, and 14. The port_ready[] array would contain 3
> elements, which would lead to an out-of-bounds access. Conversely in
> other cases, the wrong port index would be used leading to timeouts on
> prepare.
> 
> This can be fixed by allocating for the worst-case of 15
> ports (DP0..DP14). In addition since the number is now fixed, we can
> use an array instead of a dynamic allocation.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/mipi_disco.c  | 18 +-----------------
>  drivers/soundwire/slave.c       |  4 ++++
>  include/linux/soundwire/sdw.h   |  2 +-
>  sound/soc/codecs/max98373-sdw.c | 15 +--------------
>  sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
>  sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
>  sound/soc/codecs/rt700-sdw.c    | 15 +--------------
>  sound/soc/codecs/rt711-sdw.c    | 15 +--------------
>  sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------

This looks fine, but the asoc changes are not dependent, so maybe we
should split them up and then can go thru Mark. Or Mark acks, either way
would work for me

>  9 files changed, 12 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index 4ae62b452b8c..55a9c51c84c1 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -289,7 +289,7 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
>  	struct sdw_slave_prop *prop = &slave->prop;
>  	struct device *dev = &slave->dev;
>  	struct fwnode_handle *port;
> -	int num_of_ports, nval, i, dp0 = 0;
> +	int nval;
>  
>  	device_property_read_u32(dev, "mipi-sdw-sw-interface-revision",
>  				 &prop->mipi_revision);
> @@ -352,7 +352,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
>  			return -ENOMEM;
>  
>  		sdw_slave_read_dp0(slave, port, prop->dp0_prop);
> -		dp0 = 1;
>  	}
>  
>  	/*
> @@ -383,21 +382,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
>  	sdw_slave_read_dpn(slave, prop->sink_dpn_prop, nval,
>  			   prop->sink_ports, "sink");
>  
> -	/* some ports are bidirectional so check total ports by ORing */
> -	nval = prop->source_ports | prop->sink_ports;
> -	num_of_ports = hweight32(nval) + dp0; /* add DP0 */
> -
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					 sizeof(*slave->port_ready),
> -					 GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(sdw_slave_read_prop);
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 0839445ee07b..a762ee24e6fa 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -25,6 +25,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  {
>  	struct sdw_slave *slave;
>  	int ret;
> +	int i;
>  
>  	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
>  	if (!slave)
> @@ -58,6 +59,9 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  	init_completion(&slave->probe_complete);
>  	slave->probed = false;
>  
> +	for (i = 0; i < SDW_MAX_PORTS; i++)
> +		init_completion(&slave->port_ready[i]);
> +
>  	mutex_lock(&bus->bus_lock);
>  	list_add_tail(&slave->node, &bus->slaves);
>  	mutex_unlock(&bus->bus_lock);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 0aa4c6af7554..63e71645fd13 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -619,7 +619,7 @@ struct sdw_slave {
>  	struct dentry *debugfs;
>  #endif
>  	struct list_head node;
> -	struct completion *port_ready;
> +	struct completion port_ready[SDW_MAX_PORTS];
>  	enum sdw_clk_stop_mode curr_clk_stop_mode;
>  	u16 dev_num;
>  	u16 dev_num_sticky;
> diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c
> index 5fe724728e84..a3ec92775ea7 100644
> --- a/sound/soc/codecs/max98373-sdw.c
> +++ b/sound/soc/codecs/max98373-sdw.c
> @@ -282,7 +282,7 @@ static const struct dev_pm_ops max98373_pm = {
>  static int max98373_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -295,7 +295,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
>  	prop->clk_stop_timeout = 20;
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports = nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					  sizeof(*prop->src_dpn_prop),
>  					  GFP_KERNEL);
> @@ -315,7 +314,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					   sizeof(*prop->sink_dpn_prop),
>  					   GFP_KERNEL);
> @@ -333,17 +331,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					 sizeof(*slave->port_ready),
> -					 GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
> index b0ba0d2acbdd..09c69dbab12a 100644
> --- a/sound/soc/codecs/rt1308-sdw.c
> +++ b/sound/soc/codecs/rt1308-sdw.c
> @@ -118,7 +118,7 @@ static int rt1308_clock_config(struct device *dev)
>  static int rt1308_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -131,7 +131,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
>  
>  	/* for sink */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->sink_dpn_prop),
>  						GFP_KERNEL);
> @@ -149,17 +148,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
> index 94bf6bee78e6..b7c97aba7f17 100644
> --- a/sound/soc/codecs/rt5682-sdw.c
> +++ b/sound/soc/codecs/rt5682-sdw.c
> @@ -537,7 +537,7 @@ static int rt5682_update_status(struct sdw_slave *slave,
>  static int rt5682_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -549,7 +549,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0x2;		/* BITMAP: 00000010 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					  sizeof(*prop->src_dpn_prop),
>  					  GFP_KERNEL);
> @@ -569,7 +568,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					   sizeof(*prop->sink_dpn_prop),
>  					   GFP_KERNEL);
> @@ -587,17 +585,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					 sizeof(*slave->port_ready),
> -					 GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
> index 4d14048d1197..b19fbcc12c69 100644
> --- a/sound/soc/codecs/rt700-sdw.c
> +++ b/sound/soc/codecs/rt700-sdw.c
> @@ -333,7 +333,7 @@ static int rt700_update_status(struct sdw_slave *slave,
>  static int rt700_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -345,7 +345,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0xA; /* BITMAP:  00001010 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->src_dpn_prop),
>  						GFP_KERNEL);
> @@ -365,7 +364,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->sink_dpn_prop),
>  						GFP_KERNEL);
> @@ -383,17 +381,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt711-sdw.c b/sound/soc/codecs/rt711-sdw.c
> index 45b928954b58..dc4a2b482462 100644
> --- a/sound/soc/codecs/rt711-sdw.c
> +++ b/sound/soc/codecs/rt711-sdw.c
> @@ -337,7 +337,7 @@ static int rt711_update_status(struct sdw_slave *slave,
>  static int rt711_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -349,7 +349,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0x8; /* BITMAP:  00001000 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->src_dpn_prop),
>  						GFP_KERNEL);
> @@ -369,7 +368,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->sink_dpn_prop),
>  						GFP_KERNEL);
> @@ -387,17 +385,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt715-sdw.c b/sound/soc/codecs/rt715-sdw.c
> index d11b23d6b240..d8ed07305ffc 100644
> --- a/sound/soc/codecs/rt715-sdw.c
> +++ b/sound/soc/codecs/rt715-sdw.c
> @@ -431,7 +431,7 @@ static int rt715_update_status(struct sdw_slave *slave,
>  static int rt715_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -443,7 +443,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0x0;	/* BITMAP:  00000000 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					sizeof(*prop->src_dpn_prop),
>  					GFP_KERNEL);
> @@ -460,36 +459,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* do this again for sink now */
> -	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
> -	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
> -					sizeof(*prop->sink_dpn_prop),
> -					GFP_KERNEL);
> -	if (!prop->sink_dpn_prop)
> -		return -ENOMEM;
> -
> -	dpn = prop->sink_dpn_prop;
> -	i = 0;
> -	addr = prop->sink_ports;
> -	for_each_set_bit(bit, &addr, 32) {
> -		dpn[i].num = bit;
> -		dpn[i].simple_ch_prep_sm = true;
> -		dpn[i].ch_prep_timeout = 10;
> -		i++;
> -	}
> -
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Vinod Koul]

On 18-08-20, 01:47, Bard Liao wrote:
> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> 
> The existing code allocates memory for the total number of ports.
> This only works if the ports are contiguous, but will break if e.g. a
> Devices uses port0, 1, and 14. The port_ready[] array would contain 3
> elements, which would lead to an out-of-bounds access. Conversely in
> other cases, the wrong port index would be used leading to timeouts on
> prepare.
> 
> This can be fixed by allocating for the worst-case of 15
> ports (DP0..DP14). In addition since the number is now fixed, we can
> use an array instead of a dynamic allocation.
> 
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> ---
>  drivers/soundwire/mipi_disco.c  | 18 +-----------------
>  drivers/soundwire/slave.c       |  4 ++++
>  include/linux/soundwire/sdw.h   |  2 +-
>  sound/soc/codecs/max98373-sdw.c | 15 +--------------
>  sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
>  sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
>  sound/soc/codecs/rt700-sdw.c    | 15 +--------------
>  sound/soc/codecs/rt711-sdw.c    | 15 +--------------
>  sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------

This looks fine, but the asoc changes are not dependent, so maybe we
should split them up and then can go thru Mark. Or Mark acks, either way
would work for me

>  9 files changed, 12 insertions(+), 119 deletions(-)
> 
> diff --git a/drivers/soundwire/mipi_disco.c b/drivers/soundwire/mipi_disco.c
> index 4ae62b452b8c..55a9c51c84c1 100644
> --- a/drivers/soundwire/mipi_disco.c
> +++ b/drivers/soundwire/mipi_disco.c
> @@ -289,7 +289,7 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
>  	struct sdw_slave_prop *prop = &slave->prop;
>  	struct device *dev = &slave->dev;
>  	struct fwnode_handle *port;
> -	int num_of_ports, nval, i, dp0 = 0;
> +	int nval;
>  
>  	device_property_read_u32(dev, "mipi-sdw-sw-interface-revision",
>  				 &prop->mipi_revision);
> @@ -352,7 +352,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
>  			return -ENOMEM;
>  
>  		sdw_slave_read_dp0(slave, port, prop->dp0_prop);
> -		dp0 = 1;
>  	}
>  
>  	/*
> @@ -383,21 +382,6 @@ int sdw_slave_read_prop(struct sdw_slave *slave)
>  	sdw_slave_read_dpn(slave, prop->sink_dpn_prop, nval,
>  			   prop->sink_ports, "sink");
>  
> -	/* some ports are bidirectional so check total ports by ORing */
> -	nval = prop->source_ports | prop->sink_ports;
> -	num_of_ports = hweight32(nval) + dp0; /* add DP0 */
> -
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					 sizeof(*slave->port_ready),
> -					 GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	return 0;
>  }
>  EXPORT_SYMBOL(sdw_slave_read_prop);
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 0839445ee07b..a762ee24e6fa 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -25,6 +25,7 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  {
>  	struct sdw_slave *slave;
>  	int ret;
> +	int i;
>  
>  	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
>  	if (!slave)
> @@ -58,6 +59,9 @@ static int sdw_slave_add(struct sdw_bus *bus,
>  	init_completion(&slave->probe_complete);
>  	slave->probed = false;
>  
> +	for (i = 0; i < SDW_MAX_PORTS; i++)
> +		init_completion(&slave->port_ready[i]);
> +
>  	mutex_lock(&bus->bus_lock);
>  	list_add_tail(&slave->node, &bus->slaves);
>  	mutex_unlock(&bus->bus_lock);
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index 0aa4c6af7554..63e71645fd13 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -619,7 +619,7 @@ struct sdw_slave {
>  	struct dentry *debugfs;
>  #endif
>  	struct list_head node;
> -	struct completion *port_ready;
> +	struct completion port_ready[SDW_MAX_PORTS];
>  	enum sdw_clk_stop_mode curr_clk_stop_mode;
>  	u16 dev_num;
>  	u16 dev_num_sticky;
> diff --git a/sound/soc/codecs/max98373-sdw.c b/sound/soc/codecs/max98373-sdw.c
> index 5fe724728e84..a3ec92775ea7 100644
> --- a/sound/soc/codecs/max98373-sdw.c
> +++ b/sound/soc/codecs/max98373-sdw.c
> @@ -282,7 +282,7 @@ static const struct dev_pm_ops max98373_pm = {
>  static int max98373_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -295,7 +295,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
>  	prop->clk_stop_timeout = 20;
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports = nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					  sizeof(*prop->src_dpn_prop),
>  					  GFP_KERNEL);
> @@ -315,7 +314,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					   sizeof(*prop->sink_dpn_prop),
>  					   GFP_KERNEL);
> @@ -333,17 +331,6 @@ static int max98373_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					 sizeof(*slave->port_ready),
> -					 GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt1308-sdw.c b/sound/soc/codecs/rt1308-sdw.c
> index b0ba0d2acbdd..09c69dbab12a 100644
> --- a/sound/soc/codecs/rt1308-sdw.c
> +++ b/sound/soc/codecs/rt1308-sdw.c
> @@ -118,7 +118,7 @@ static int rt1308_clock_config(struct device *dev)
>  static int rt1308_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -131,7 +131,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
>  
>  	/* for sink */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->sink_dpn_prop),
>  						GFP_KERNEL);
> @@ -149,17 +148,6 @@ static int rt1308_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt5682-sdw.c b/sound/soc/codecs/rt5682-sdw.c
> index 94bf6bee78e6..b7c97aba7f17 100644
> --- a/sound/soc/codecs/rt5682-sdw.c
> +++ b/sound/soc/codecs/rt5682-sdw.c
> @@ -537,7 +537,7 @@ static int rt5682_update_status(struct sdw_slave *slave,
>  static int rt5682_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -549,7 +549,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0x2;		/* BITMAP: 00000010 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					  sizeof(*prop->src_dpn_prop),
>  					  GFP_KERNEL);
> @@ -569,7 +568,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					   sizeof(*prop->sink_dpn_prop),
>  					   GFP_KERNEL);
> @@ -587,17 +585,6 @@ static int rt5682_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					 sizeof(*slave->port_ready),
> -					 GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt700-sdw.c b/sound/soc/codecs/rt700-sdw.c
> index 4d14048d1197..b19fbcc12c69 100644
> --- a/sound/soc/codecs/rt700-sdw.c
> +++ b/sound/soc/codecs/rt700-sdw.c
> @@ -333,7 +333,7 @@ static int rt700_update_status(struct sdw_slave *slave,
>  static int rt700_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -345,7 +345,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0xA; /* BITMAP:  00001010 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->src_dpn_prop),
>  						GFP_KERNEL);
> @@ -365,7 +364,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->sink_dpn_prop),
>  						GFP_KERNEL);
> @@ -383,17 +381,6 @@ static int rt700_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt711-sdw.c b/sound/soc/codecs/rt711-sdw.c
> index 45b928954b58..dc4a2b482462 100644
> --- a/sound/soc/codecs/rt711-sdw.c
> +++ b/sound/soc/codecs/rt711-sdw.c
> @@ -337,7 +337,7 @@ static int rt711_update_status(struct sdw_slave *slave,
>  static int rt711_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -349,7 +349,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0x8; /* BITMAP:  00001000 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->src_dpn_prop),
>  						GFP_KERNEL);
> @@ -369,7 +368,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
>  
>  	/* do this again for sink now */
>  	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
>  	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  						sizeof(*prop->sink_dpn_prop),
>  						GFP_KERNEL);
> @@ -387,17 +385,6 @@ static int rt711_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> diff --git a/sound/soc/codecs/rt715-sdw.c b/sound/soc/codecs/rt715-sdw.c
> index d11b23d6b240..d8ed07305ffc 100644
> --- a/sound/soc/codecs/rt715-sdw.c
> +++ b/sound/soc/codecs/rt715-sdw.c
> @@ -431,7 +431,7 @@ static int rt715_update_status(struct sdw_slave *slave,
>  static int rt715_read_prop(struct sdw_slave *slave)
>  {
>  	struct sdw_slave_prop *prop = &slave->prop;
> -	int nval, i, num_of_ports = 1;
> +	int nval, i;
>  	u32 bit;
>  	unsigned long addr;
>  	struct sdw_dpn_prop *dpn;
> @@ -443,7 +443,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
>  	prop->sink_ports = 0x0;	/* BITMAP:  00000000 */
>  
>  	nval = hweight32(prop->source_ports);
> -	num_of_ports += nval;
>  	prop->src_dpn_prop = devm_kcalloc(&slave->dev, nval,
>  					sizeof(*prop->src_dpn_prop),
>  					GFP_KERNEL);
> @@ -460,36 +459,6 @@ static int rt715_read_prop(struct sdw_slave *slave)
>  		i++;
>  	}
>  
> -	/* do this again for sink now */
> -	nval = hweight32(prop->sink_ports);
> -	num_of_ports += nval;
> -	prop->sink_dpn_prop = devm_kcalloc(&slave->dev, nval,
> -					sizeof(*prop->sink_dpn_prop),
> -					GFP_KERNEL);
> -	if (!prop->sink_dpn_prop)
> -		return -ENOMEM;
> -
> -	dpn = prop->sink_dpn_prop;
> -	i = 0;
> -	addr = prop->sink_ports;
> -	for_each_set_bit(bit, &addr, 32) {
> -		dpn[i].num = bit;
> -		dpn[i].simple_ch_prep_sm = true;
> -		dpn[i].ch_prep_timeout = 10;
> -		i++;
> -	}
> -
> -	/* Allocate port_ready based on num_of_ports */
> -	slave->port_ready = devm_kcalloc(&slave->dev, num_of_ports,
> -					sizeof(*slave->port_ready),
> -					GFP_KERNEL);
> -	if (!slave->port_ready)
> -		return -ENOMEM;
> -
> -	/* Initialize completion */
> -	for (i = 0; i < num_of_ports; i++)
> -		init_completion(&slave->port_ready[i]);
> -
>  	/* set the timeout values */
>  	prop->clk_stop_timeout = 20;
>  
> -- 
> 2.17.1

-- 
~Vinod

Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Pierre-Louis Bossart]

On 8/18/20 1:36 AM, Vinod Koul wrote:
> On 18-08-20, 01:47, Bard Liao wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> The existing code allocates memory for the total number of ports.
>> This only works if the ports are contiguous, but will break if e.g. a
>> Devices uses port0, 1, and 14. The port_ready[] array would contain 3
>> elements, which would lead to an out-of-bounds access. Conversely in
>> other cases, the wrong port index would be used leading to timeouts on
>> prepare.
>>
>> This can be fixed by allocating for the worst-case of 15
>> ports (DP0..DP14). In addition since the number is now fixed, we can
>> use an array instead of a dynamic allocation.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
>> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
>> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> ---
>>   drivers/soundwire/mipi_disco.c  | 18 +-----------------
>>   drivers/soundwire/slave.c       |  4 ++++
>>   include/linux/soundwire/sdw.h   |  2 +-
>>   sound/soc/codecs/max98373-sdw.c | 15 +--------------
>>   sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
>>   sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
>>   sound/soc/codecs/rt700-sdw.c    | 15 +--------------
>>   sound/soc/codecs/rt711-sdw.c    | 15 +--------------
>>   sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
> 
> This looks fine, but the asoc changes are not dependent, so maybe we
> should split them up and then can go thru Mark. Or Mark acks, either way
> would work for me

There are 3 dependencies that we tracked between SoundWire and ASoC 
subsystems:

a) addition of SDCA control macro (needed before SDCA codec drivers can 
be shared)
b) this series - we could indeed submit the codec changes to Mark's tree 
separately, but then the SoundWire tree would be broken: the codec 
drivers would still try to allocate dynamically what is now a fixed-size 
array.
c) configuration of the interrupt masks in codec drivers instead of 
hard-coded in bus driver + spurious parity error workaround (not posted 
yet but ready).

The changes in ASoC codecs are really only on the initialization part 
(either removing a dynamic allocation or setting masks), there's no 
functional change otherwise.

I think the simplest to avoid multiple back-and-forth is to have these 
small interface/initialization changes merged through the SoundWire 
subsystem, then merged by Mark from a single immutable tag. Would this 
work for everyone?

In addition, there's a WIP change to regmap to add support for SoundWire 
1.2 MBQ-based register access, but this only affects regmap and ASoC 
trees, all handled by Mark.

I don't think we have any other cross-tree changes planned for now, the 
SDCA infrastructure plumbing is still rather open.

Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Pierre-Louis Bossart]

On 8/18/20 1:36 AM, Vinod Koul wrote:
> On 18-08-20, 01:47, Bard Liao wrote:
>> From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>>
>> The existing code allocates memory for the total number of ports.
>> This only works if the ports are contiguous, but will break if e.g. a
>> Devices uses port0, 1, and 14. The port_ready[] array would contain 3
>> elements, which would lead to an out-of-bounds access. Conversely in
>> other cases, the wrong port index would be used leading to timeouts on
>> prepare.
>>
>> This can be fixed by allocating for the worst-case of 15
>> ports (DP0..DP14). In addition since the number is now fixed, we can
>> use an array instead of a dynamic allocation.
>>
>> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
>> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
>> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
>> Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
>> ---
>>   drivers/soundwire/mipi_disco.c  | 18 +-----------------
>>   drivers/soundwire/slave.c       |  4 ++++
>>   include/linux/soundwire/sdw.h   |  2 +-
>>   sound/soc/codecs/max98373-sdw.c | 15 +--------------
>>   sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
>>   sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
>>   sound/soc/codecs/rt700-sdw.c    | 15 +--------------
>>   sound/soc/codecs/rt711-sdw.c    | 15 +--------------
>>   sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
> 
> This looks fine, but the asoc changes are not dependent, so maybe we
> should split them up and then can go thru Mark. Or Mark acks, either way
> would work for me

There are 3 dependencies that we tracked between SoundWire and ASoC 
subsystems:

a) addition of SDCA control macro (needed before SDCA codec drivers can 
be shared)
b) this series - we could indeed submit the codec changes to Mark's tree 
separately, but then the SoundWire tree would be broken: the codec 
drivers would still try to allocate dynamically what is now a fixed-size 
array.
c) configuration of the interrupt masks in codec drivers instead of 
hard-coded in bus driver + spurious parity error workaround (not posted 
yet but ready).

The changes in ASoC codecs are really only on the initialization part 
(either removing a dynamic allocation or setting masks), there's no 
functional change otherwise.

I think the simplest to avoid multiple back-and-forth is to have these 
small interface/initialization changes merged through the SoundWire 
subsystem, then merged by Mark from a single immutable tag. Would this 
work for everyone?

In addition, there's a WIP change to regmap to add support for SoundWire 
1.2 MBQ-based register access, but this only affects regmap and ASoC 
trees, all handled by Mark.

I don't think we have any other cross-tree changes planned for now, the 
SDCA infrastructure plumbing is still rather open.

Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Pierre-Louis Bossart]

> In addition, there's a WIP change to regmap to add support for SoundWire 
> 1.2 MBQ-based register access, but this only affects regmap and ASoC 
> trees, all handled by Mark.

I have to take this comment back, the regmap change will depend on the 
MBQ macro that should go in the SoundWire tree.

Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Vinod Koul]

On 18-08-20, 07:09, Pierre-Louis Bossart wrote:
> 
> 
> On 8/18/20 1:36 AM, Vinod Koul wrote:
> > On 18-08-20, 01:47, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > The existing code allocates memory for the total number of ports.
> > > This only works if the ports are contiguous, but will break if e.g. a
> > > Devices uses port0, 1, and 14. The port_ready[] array would contain 3
> > > elements, which would lead to an out-of-bounds access. Conversely in
> > > other cases, the wrong port index would be used leading to timeouts on
> > > prepare.
> > > 
> > > This can be fixed by allocating for the worst-case of 15
> > > ports (DP0..DP14). In addition since the number is now fixed, we can
> > > use an array instead of a dynamic allocation.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > ---
> > >   drivers/soundwire/mipi_disco.c  | 18 +-----------------
> > >   drivers/soundwire/slave.c       |  4 ++++
> > >   include/linux/soundwire/sdw.h   |  2 +-
> > >   sound/soc/codecs/max98373-sdw.c | 15 +--------------
> > >   sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
> > >   sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
> > >   sound/soc/codecs/rt700-sdw.c    | 15 +--------------
> > >   sound/soc/codecs/rt711-sdw.c    | 15 +--------------
> > >   sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
> > 
> > This looks fine, but the asoc changes are not dependent, so maybe we
> > should split them up and then can go thru Mark. Or Mark acks, either way
> > would work for me
> 
> There are 3 dependencies that we tracked between SoundWire and ASoC
> subsystems:
> 
> a) addition of SDCA control macro (needed before SDCA codec drivers can be
> shared)
> b) this series - we could indeed submit the codec changes to Mark's tree
> separately, but then the SoundWire tree would be broken: the codec drivers
> would still try to allocate dynamically what is now a fixed-size array.
> c) configuration of the interrupt masks in codec drivers instead of
> hard-coded in bus driver + spurious parity error workaround (not posted yet
> but ready).
> 
> The changes in ASoC codecs are really only on the initialization part
> (either removing a dynamic allocation or setting masks), there's no
> functional change otherwise.
> 
> I think the simplest to avoid multiple back-and-forth is to have these small
> interface/initialization changes merged through the SoundWire subsystem,
> then merged by Mark from a single immutable tag. Would this work for
> everyone?

That would work for me, but you need to split the asoc, regmap, sdw
patches. I am sure looking at patch tag, other maintainers would have
skipped these patches..

> In addition, there's a WIP change to regmap to add support for SoundWire 1.2
> MBQ-based register access, but this only affects regmap and ASoC trees, all
> handled by Mark.
> 
> I don't think we have any other cross-tree changes planned for now, the SDCA
> infrastructure plumbing is still rather open.

-- 
~Vinod

Re: [PATCH 2/2] soundwire: fix port_ready[] dynamic allocation in mipi_disco and ASoC codecs

[Vinod Koul]

On 18-08-20, 07:09, Pierre-Louis Bossart wrote:
> 
> 
> On 8/18/20 1:36 AM, Vinod Koul wrote:
> > On 18-08-20, 01:47, Bard Liao wrote:
> > > From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > 
> > > The existing code allocates memory for the total number of ports.
> > > This only works if the ports are contiguous, but will break if e.g. a
> > > Devices uses port0, 1, and 14. The port_ready[] array would contain 3
> > > elements, which would lead to an out-of-bounds access. Conversely in
> > > other cases, the wrong port index would be used leading to timeouts on
> > > prepare.
> > > 
> > > This can be fixed by allocating for the worst-case of 15
> > > ports (DP0..DP14). In addition since the number is now fixed, we can
> > > use an array instead of a dynamic allocation.
> > > 
> > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> > > Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> > > Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> > > Signed-off-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> > > ---
> > >   drivers/soundwire/mipi_disco.c  | 18 +-----------------
> > >   drivers/soundwire/slave.c       |  4 ++++
> > >   include/linux/soundwire/sdw.h   |  2 +-
> > >   sound/soc/codecs/max98373-sdw.c | 15 +--------------
> > >   sound/soc/codecs/rt1308-sdw.c   | 14 +-------------
> > >   sound/soc/codecs/rt5682-sdw.c   | 15 +--------------
> > >   sound/soc/codecs/rt700-sdw.c    | 15 +--------------
> > >   sound/soc/codecs/rt711-sdw.c    | 15 +--------------
> > >   sound/soc/codecs/rt715-sdw.c    | 33 +--------------------------------
> > 
> > This looks fine, but the asoc changes are not dependent, so maybe we
> > should split them up and then can go thru Mark. Or Mark acks, either way
> > would work for me
> 
> There are 3 dependencies that we tracked between SoundWire and ASoC
> subsystems:
> 
> a) addition of SDCA control macro (needed before SDCA codec drivers can be
> shared)
> b) this series - we could indeed submit the codec changes to Mark's tree
> separately, but then the SoundWire tree would be broken: the codec drivers
> would still try to allocate dynamically what is now a fixed-size array.
> c) configuration of the interrupt masks in codec drivers instead of
> hard-coded in bus driver + spurious parity error workaround (not posted yet
> but ready).
> 
> The changes in ASoC codecs are really only on the initialization part
> (either removing a dynamic allocation or setting masks), there's no
> functional change otherwise.
> 
> I think the simplest to avoid multiple back-and-forth is to have these small
> interface/initialization changes merged through the SoundWire subsystem,
> then merged by Mark from a single immutable tag. Would this work for
> everyone?

That would work for me, but you need to split the asoc, regmap, sdw
patches. I am sure looking at patch tag, other maintainers would have
skipped these patches..

> In addition, there's a WIP change to regmap to add support for SoundWire 1.2
> MBQ-based register access, but this only affects regmap and ASoC trees, all
> handled by Mark.
> 
> I don't think we have any other cross-tree changes planned for now, the SDCA
> infrastructure plumbing is still rather open.

-- 
~Vinod