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
From: Pierre-Louis Bossart <[email protected]>
A Device may have at most 15 physical ports (DP0, DP1..DP14).
Signed-off-by: Pierre-Louis Bossart <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Reviewed-by: Guennadi Liakhovetski <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
From: Pierre-Louis Bossart <[email protected]>
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 <[email protected]>
Reviewed-by: Rander Wang <[email protected]>
Reviewed-by: Guennadi Liakhovetski <[email protected]>
Signed-off-by: Bard Liao <[email protected]>
---
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
On 18-08-20, 01:47, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> A Device may have at most 15 physical ports (DP0, DP1..DP14).
>
> Signed-off-by: Pierre-Louis Bossart <[email protected]>
> Reviewed-by: Rander Wang <[email protected]>
> Reviewed-by: Guennadi Liakhovetski <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> 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
On 18-08-20, 01:47, Bard Liao wrote:
> From: Pierre-Louis Bossart <[email protected]>
>
> 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 <[email protected]>
> Reviewed-by: Rander Wang <[email protected]>
> Reviewed-by: Guennadi Liakhovetski <[email protected]>
> Signed-off-by: Bard Liao <[email protected]>
> ---
> 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
> -----Original Message-----
> From: Vinod Koul <[email protected]>
> Sent: Tuesday, August 18, 2020 2:36 PM
> To: Bard Liao <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; pierre-
> [email protected]; Kale, Sanyog R <[email protected]>; Lin,
> Mengdong <[email protected]>; Liao, Bard <[email protected]>
> 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 <[email protected]>
> >
> > A Device may have at most 15 physical ports (DP0, DP1..DP14).
> >
> > Signed-off-by: Pierre-Louis Bossart
> > <[email protected]>
> > Reviewed-by: Rander Wang <[email protected]>
> > Reviewed-by: Guennadi Liakhovetski
> > <[email protected]>
> > Signed-off-by: Bard Liao <[email protected]>
> > ---
> > 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
On 18-08-20, 06:53, Liao, Bard wrote:
> > -----Original Message-----
> > From: Vinod Koul <[email protected]>
> > Sent: Tuesday, August 18, 2020 2:36 PM
> > To: Bard Liao <[email protected]>
> > Cc: [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; [email protected]; pierre-
> > [email protected]; Kale, Sanyog R <[email protected]>; Lin,
> > Mengdong <[email protected]>; Liao, Bard <[email protected]>
> > 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 <[email protected]>
> > >
> > > A Device may have at most 15 physical ports (DP0, DP1..DP14).
> > >
> > > Signed-off-by: Pierre-Louis Bossart
> > > <[email protected]>
> > > Reviewed-by: Rander Wang <[email protected]>
> > > Reviewed-by: Guennadi Liakhovetski
> > > <[email protected]>
> > > Signed-off-by: Bard Liao <[email protected]>
> > > ---
> > > 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
On 8/18/20 1:36 AM, Vinod Koul wrote:
> On 18-08-20, 01:47, Bard Liao wrote:
>> From: Pierre-Louis Bossart <[email protected]>
>>
>> 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 <[email protected]>
>> Reviewed-by: Rander Wang <[email protected]>
>> Reviewed-by: Guennadi Liakhovetski <[email protected]>
>> Signed-off-by: Bard Liao <[email protected]>
>> ---
>> 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.
> 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.
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 <[email protected]>
> > >
> > > 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 <[email protected]>
> > > Reviewed-by: Rander Wang <[email protected]>
> > > Reviewed-by: Guennadi Liakhovetski <[email protected]>
> > > Signed-off-by: Bard Liao <[email protected]>
> > > ---
> > > 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