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

[Pierre-Louis Bossart <pierre-louis.bossart@linux.dev>]

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;
>  		}