[PATCH] soundwire: stream: validate slave port against DisCo port bitmap

[PATCH] soundwire: stream: validate slave port against DisCo port bitmap

[Eric Wu]

sdw_slave_port_config() checks that the port number falls in the
generic valid range, but never checks whether the Slave actually
exposes that port for the requested direction. An in-range but
unsupported port number (or the right port with the wrong direction)
gets accepted without complaint.

sdw_get_slave_dpn_prop() already does this lookup against the
source_ports/sink_ports bitmaps for other purposes, so reuse it here
instead of open-coding a new check.

Slaves that don't report source_ports/sink_ports at all (both zero)
are left unchecked, since that's treated elsewhere in the driver as
"property not provided" rather than "no ports supported".

Signed-off-by: Eric Wu <kunjinkao.jp@gmail.com>
---
Only compile-tested: x86_64 gcc, arm64 and arm32 clang (LLVM=1),
all clean with W=1. I do not have SoundWire hardware to test with.

 drivers/soundwire/stream.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 4ed8fb7663ad..30cb8a8552d8 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -1054,14 +1054,23 @@ static int sdw_slave_port_config(struct sdw_slave *slave,
 
 	i = 0;
 	list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
-		/*
-		 * TODO: Check valid port range as defined by DisCo/
-		 * slave
-		 */
 		if (!is_bpt_stream) {
 			ret = sdw_slave_port_is_valid_range(&slave->dev, port_config[i].num);
 			if (ret < 0)
 				return ret;
+
+			/*
+			 * source_ports/sink_ports are optional DisCo properties:
+			 * both zero means "not provided" rather than "no ports",
+			 * so only enforce the check when the Slave reports them.
+			 */
+			if ((slave->prop.source_ports || slave->prop.sink_ports) &&
+			    !sdw_get_slave_dpn_prop(slave, s_rt->direction, port_config[i].num)) {
+				dev_err(&slave->dev, "port %d not supported for %s\n",
+					port_config[i].num,
+					s_rt->direction == SDW_DATA_DIR_TX ? "TX" : "RX");
+				return -EINVAL;
+			}
 		} else if (port_config[i].num) {
 			return -EINVAL;
 		}
-- 
2.25.1

Re: [PATCH] soundwire: stream: validate slave port against DisCo port bitmap

[Pierre-Louis Bossart]

Thanks for the patch.

> sdw_slave_port_config() checks that the port number falls in the
> generic valid range, but never checks whether the Slave actually
> exposes that port for the requested direction. An in-range but
> unsupported port number (or the right port with the wrong direction)
> gets accepted without complaint.
> 
> sdw_get_slave_dpn_prop() already does this lookup against the
> source_ports/sink_ports bitmaps for other purposes, so reuse it here
> instead of open-coding a new check.

Smart, good idea. I would recommend adding the first sentence as code comments, I had to look up the code to figure out what it did.

> Slaves that don't report source_ports/sink_ports at all (both zero)
> are left unchecked, since that's treated elsewhere in the driver as
> "property not provided" rather than "no ports supported".
> 
> Signed-off-by: Eric Wu <kunjinkao.jp@gmail.com>
> ---
> Only compile-tested: x86_64 gcc, arm64 and arm32 clang (LLVM=1),
> all clean with W=1. I do not have SoundWire hardware to test with.

That's a problem in general, maybe push this patch first through the Intel/SOF CI to check that some SoundWire platforms are tested?

>  drivers/soundwire/stream.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 4ed8fb7663ad..30cb8a8552d8 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -1054,14 +1054,23 @@ static int sdw_slave_port_config(struct sdw_slave *slave,
>  
>  	i = 0;
>  	list_for_each_entry(p_rt, &s_rt->port_list, port_node) {
> -		/*
> -		 * TODO: Check valid port range as defined by DisCo/
> -		 * slave
> -		 */
>  		if (!is_bpt_stream) {
>  			ret = sdw_slave_port_is_valid_range(&slave->dev, port_config[i].num);
>  			if (ret < 0)
>  				return ret;
> +
> +			/*
> +			 * source_ports/sink_ports are optional DisCo properties:
> +			 * both zero means "not provided" rather than "no ports",
> +			 * so only enforce the check when the Slave reports them.
> +			 */

That comment is a tad misleading.

In the real world, all drivers ignore DisCo properties provided by platform firmware, and hard-code the values, e.g.

	prop->source_ports = 0x14; /* BITMAP: 00010100 */
	prop->sink_ports = 0x8; /* BITMAP:  00001000 */

Those properties are really not optional at all, what's optional is that they are blindly read from firmware.

> +			if ((slave->prop.source_ports || slave->prop.sink_ports) &&
> +			    !sdw_get_slave_dpn_prop(slave, s_rt->direction, port_config[i].num)) {

This test is a bit odd, there could be cases where only source_ports are defined so you would try to check for a sink_port in sdw_get_slave_dpn_prop()
It probably works in the end but maybe you could check s_rt->direction with source/sink port first?

> +				dev_err(&slave->dev, "port %d not supported for %s\n",
> +					port_config[i].num,
> +					s_rt->direction == SDW_DATA_DIR_TX ? "TX" : "RX");
> +				return -EINVAL;
> +			}
>  		} else if (port_config[i].num) {
>  			return -EINVAL;
>  		}

Re: [PATCH] soundwire: stream: validate slave port against DisCo port bitmap

[Eric Wu]

> Smart, good idea. I would recommend adding the first sentence as code comments, I had to look up the code to figure out what it did.

I'll add a short comment explaining that the generic range check does
not verify whether the Slave exposes the port for the requested
direction.

> That's a problem in general, maybe push this patch first through the Intel/SOF CI to check that some SoundWire platforms are tested?

I'll run the updated patch through the Intel/SOF CI before posting v2.

> That comment is a tad misleading.
> 
> In the real world, all drivers ignore DisCo properties provided by platform firmware, and hard-code the values, e.g.
> 
> 	prop->source_ports = 0x14; /* BITMAP: 00010100 */
> 	prop->sink_ports = 0x8; /* BITMAP:  00001000 */
> 
> Those properties are really not optional at all, what's optional is that they are blindly read from firmware.

Agreed. Describing source_ports and sink_ports as optional is 
inaccurate.So I'll rework the comment.


> This test is a bit odd, there could be cases where only source_ports are defined so you would try to check for a sink_port in sdw_get_slave_dpn_prop()
> It probably works in the end but maybe you could check s_rt->direction with source/sink port first?

I'll select the relevant bitmap based on the stream direction first and
reject a zero bitmap for that direction.

Re: [PATCH] soundwire: stream: validate slave port against DisCo port bitmap

[chang wu]

> Smart, good idea. I would recommend adding the first sentence as code comments, I had to look up the code to figure out what it did.

I'll add a short comment explaining that the generic range check does
not verify whether the Slave exposes the port for the requested
direction.

> That's a problem in general, maybe push this patch first through the Intel/SOF CI to check that some SoundWire platforms are tested?

I'll run the updated patch through the Intel/SOF CI before posting v2.

> That comment is a tad misleading.
>
> In the real world, all drivers ignore DisCo properties provided by platform firmware, and hard-code the values, e.g.
>
>       prop->source_ports = 0x14; /* BITMAP: 00010100 */
>       prop->sink_ports = 0x8; /* BITMAP:  00001000 */
>
> Those properties are really not optional at all, what's optional is that they are blindly read from firmware.

Agreed. Describing source_ports and sink_ports as optional is
inaccurate.So I'll rework the comment.


> This test is a bit odd, there could be cases where only source_ports are defined so you would try to check for a sink_port in sdw_get_slave_dpn_prop()
> It probably works in the end but maybe you could check s_rt->direction with source/sink port first?

I'll select the relevant bitmap based on the stream direction first and
reject a zero bitmap for that direction.